refactor(key-wallet)!: remove MnemonicWithPassphrase wallet type#747
Merged
Conversation
Drops the entire passphrase-as-callback feature: callers can no longer construct a wallet that retains a mnemonic and applies a BIP39 passphrase on-demand. Removed across the workspace: - `WalletType::MnemonicWithPassphrase` variant + Zeroize arm - `Wallet::from_mnemonic_with_passphrase` constructor - `add_account_with_passphrase`, `add_bls_account_with_passphrase`, `add_eddsa_account_with_passphrase` on `Wallet` - `derive_extended_private_key_with_passphrase`, `create_accounts_with_passphrase_from_options`, `create_special_purpose_accounts_with_passphrase`, `needs_passphrase`, and `root_extended_priv_key_with_callback` helpers - `add_managed_*_with_passphrase` on `ManagedAccountOperations` - `passphrase` parameter from `WalletManager::create_wallet_from_mnemonic` and `create_wallet_from_mnemonic_return_serialized_bytes` - `passphrase` parameter from FFI `wallet_create_from_mnemonic*`, `wallet_manager_add_wallet_from_mnemonic*`, `wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes` - All passphrase-only tests (`test_wallet_with_passphrase`, `test_passphrase_edge_cases`, the `passphrase_test` module, FFI `test_passphrase_wallets.rs`, etc.) Breaking change: serialized wallets containing the `MnemonicWithPassphrase` variant can no longer be deserialized, and the FFI `passphrase` parameter is gone — callers must update their code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR removes BIP39 passphrase parameter support from wallet creation across the FFI and Rust library layers. The passphrase parameter is eliminated from exported function signatures, conditional wallet-creation logic is consolidated to always use mnemonic-only initialization, and passphrase-specific test modules and methods are removed. The ChangesRemove BIP39 Passphrase Support from Wallet Creation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
key-wallet/src/wallet/helper.rs (1)
348-361:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDistinguish
ExternalSignablefromWatchOnlyin the error path.This branch now returns a
"watch-only wallet"error forWalletType::ExternalSignabletoo. That makes the hardenedderive_*helpers report the wrong reason for failure and no longer matches the docs below, which describe external-signable wallets as a separate unsupported case.Suggested fix
- WalletType::ExternalSignable | WalletType::WatchOnly => { + WalletType::WatchOnly => { return Err(Error::InvalidParameter( - "Cannot derive private keys from watch-only wallet".to_string(), + "Cannot derive private keys from watch-only wallet".to_string(), + )); + } + WalletType::ExternalSignable => { + return Err(Error::InvalidParameter( + "Cannot derive private keys from external-signable wallet without root key material" + .to_string(), )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/helper.rs` around lines 348 - 361, The match arm that currently groups WalletType::ExternalSignable with WatchOnly returns a "watch-only wallet" error; change it to return a distinct error for ExternalSignable so the derive_* helpers and docs are accurate. Specifically, in the match on self.wallet_type (the branch creating `master`), replace the WalletType::ExternalSignable | WalletType::WatchOnly arm with two separate arms: one for WalletType::ExternalSignable returning Err(Error::InvalidParameter("Cannot derive private keys from external-signable wallet".to_string())), and one for WalletType::WatchOnly returning Err(Error::InvalidParameter("Cannot derive private keys from watch-only wallet".to_string())). Ensure you update the arms that reference root_extended_private_key and keep other match arms unchanged.key-wallet-ffi/tests/debug_wallet_add.rs (1)
18-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale debug print messages still reference the removed passphrase.
Lines 18 and 32 say
"Adding wallet with passphrase 'pass1'"and"Successfully added wallet with passphrase"but there is no longer any passphrase involved. This misleads anyone reading the test output or the test source.✏️ Proposed fix
- println!("Adding wallet with passphrase 'pass1'"); + println!("Adding wallet from mnemonic (no passphrase)"); let success = unsafe { wallet_manager::wallet_manager_add_wallet_from_mnemonic(manager, mnemonic.as_ptr(), error) }; if !success { ... } else { - println!("Successfully added wallet with passphrase"); + println!("Successfully added wallet from mnemonic"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet-ffi/tests/debug_wallet_add.rs` around lines 18 - 32, The debug prints in the test still refer to a passphrase even though no passphrase is used; update the println! calls around the wallet creation that mention "Adding wallet with passphrase 'pass1'" and "Successfully added wallet with passphrase" to use accurate messages (e.g., "Adding wallet from mnemonic" and "Successfully added wallet from mnemonic") so the output matches the call to wallet_manager::wallet_manager_add_wallet_from_mnemonic and related error handling using the error pointer.key-wallet-ffi/src/wallet_manager_tests.rs (1)
82-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale passphrase-bug comment and the misleading
// No passphraseannotation.Lines 83-84 explain that different mnemonics are used "instead of different passphrases" due to a library bug with passphrase wallets, referencing
wallet_manager/mod.rs:140-146. Both the referenced bug and the feature itself no longer exist after this PR, so the comment now actively misleads readers.The new
// No passphraseannotation on line 91 compounds the confusion – it implies the caller is choosing not to supply a passphrase, when in fact the parameter has been removed entirely.🛠️ Suggested cleanup
- // Note: We use different mnemonics instead of different passphrases - // because the library has a bug with passphrase wallets (see line 140-146 in wallet_manager/mod.rs) + // Use distinct mnemonics so each wallet has a unique ID. let mnemonics = [TEST_MNEMONIC, TEST_MNEMONIC_2, TEST_MNEMONIC_3]; unsafe { for (i, mnemonic_str) in mnemonics.iter().enumerate() { let mnemonic = CString::new(*mnemonic_str).unwrap(); let success = wallet_manager::wallet_manager_add_wallet_from_mnemonic( manager, - mnemonic.as_ptr(), // No passphrase + mnemonic.as_ptr(), error, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 82 - 92, Remove the stale bug-note and the misleading inline annotation: delete the comment block that says different mnemonics are used "instead of different passphrases" and remove the trailing `// No passphrase` comment next to mnemonic.as_ptr(); simply keep the mnemonics array and the call to wallet_manager::wallet_manager_add_wallet_from_mnemonic(manager, mnemonic.as_ptr(), error, ...) as-is since the passphrase parameter/bug no longer exists. Ensure no other references in this test point to wallet_manager/mod.rs lines 140-146.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 82-92: Remove the stale bug-note and the misleading inline
annotation: delete the comment block that says different mnemonics are used
"instead of different passphrases" and remove the trailing `// No passphrase`
comment next to mnemonic.as_ptr(); simply keep the mnemonics array and the call
to wallet_manager::wallet_manager_add_wallet_from_mnemonic(manager,
mnemonic.as_ptr(), error, ...) as-is since the passphrase parameter/bug no
longer exists. Ensure no other references in this test point to
wallet_manager/mod.rs lines 140-146.
In `@key-wallet-ffi/tests/debug_wallet_add.rs`:
- Around line 18-32: The debug prints in the test still refer to a passphrase
even though no passphrase is used; update the println! calls around the wallet
creation that mention "Adding wallet with passphrase 'pass1'" and "Successfully
added wallet with passphrase" to use accurate messages (e.g., "Adding wallet
from mnemonic" and "Successfully added wallet from mnemonic") so the output
matches the call to wallet_manager::wallet_manager_add_wallet_from_mnemonic and
related error handling using the error pointer.
In `@key-wallet/src/wallet/helper.rs`:
- Around line 348-361: The match arm that currently groups
WalletType::ExternalSignable with WatchOnly returns a "watch-only wallet" error;
change it to return a distinct error for ExternalSignable so the derive_*
helpers and docs are accurate. Specifically, in the match on self.wallet_type
(the branch creating `master`), replace the WalletType::ExternalSignable |
WalletType::WatchOnly arm with two separate arms: one for
WalletType::ExternalSignable returning Err(Error::InvalidParameter("Cannot
derive private keys from external-signable wallet".to_string())), and one for
WalletType::WatchOnly returning Err(Error::InvalidParameter("Cannot derive
private keys from watch-only wallet".to_string())). Ensure you update the arms
that reference root_extended_private_key and keep other match arms unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc3550d9-6d47-480c-b04d-48f00a3730c4
📒 Files selected for processing (44)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/src/main.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_multi_wallet.rskey-wallet-ffi/examples/check_transaction.ckey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/managed_wallet_tests.rskey-wallet-ffi/src/mnemonic_tests.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet-manager/tests/integration_test.rskey-wallet-manager/tests/test_serialized_wallets.rskey-wallet/src/tests/edge_case_tests.rskey-wallet/src/tests/wallet_tests.rskey-wallet/src/wallet/accounts.rskey-wallet/src/wallet/helper.rskey-wallet/src/wallet/initialization.rskey-wallet/src/wallet/managed_wallet_info/managed_account_operations.rskey-wallet/src/wallet/managed_wallet_info/managed_accounts.rskey-wallet/src/wallet/mod.rskey-wallet/src/wallet/passphrase_test.rskey-wallet/src/wallet/root_extended_keys.rskey-wallet/src/wallet_comprehensive_tests.rs
💤 Files with no reviewable changes (23)
- key-wallet-ffi/src/address_pool.rs
- key-wallet/src/wallet_comprehensive_tests.rs
- key-wallet-ffi/tests/test_import_wallet.rs
- key-wallet-manager/tests/integration_test.rs
- key-wallet/src/wallet/managed_wallet_info/managed_account_operations.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet-manager/examples/wallet_creation.rs
- key-wallet/src/wallet/initialization.rs
- key-wallet-ffi/tests/test_managed_account_collection.rs
- key-wallet/src/tests/wallet_tests.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- key-wallet/src/wallet/root_extended_keys.rs
- key-wallet-ffi/tests/test_account_collection.rs
- key-wallet/src/tests/edge_case_tests.rs
- key-wallet-ffi/tests/test_passphrase_wallets.rs
- dash-spv/src/main.rs
- key-wallet/src/wallet/accounts.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- key-wallet-ffi/src/account_collection.rs
- key-wallet/src/wallet/mod.rs
- key-wallet-manager/tests/test_serialized_wallets.rs
- key-wallet-ffi/src/wallet_manager_serialization_tests.rs
- key-wallet/src/wallet/passphrase_test.rs
Auto-generated docs were stale after removing the `passphrase` parameter from `wallet_create_from_mnemonic*` and `wallet_manager_add_wallet_from_mnemonic*`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Distinguish ExternalSignable from WatchOnly in derive_extended_private_key error path so callers see why each variant rejects the request. - Drop the stale "passphrase 'pass1'" debug prints in debug_wallet_add.rs that no longer reflect what the test does. - Remove the obsolete bug-comment and "// No passphrase" annotation in wallet_manager_tests.rs — the bug and the parameter both no longer exist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #747 +/- ##
=============================================
+ Coverage 71.31% 71.78% +0.47%
=============================================
Files 321 320 -1
Lines 70339 69411 -928
=============================================
- Hits 50160 49828 -332
+ Misses 20179 19583 -596
|
xdustinface
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WalletType::MnemonicWithPassphrasevariant and the entire passphrase-as-callback feature it powered.*_with_passphraseconstructors/methods/trait fns onWallet,ManagedAccountOperations, and the wallet helpers (derive_extended_private_key_with_passphrase,create_accounts_with_passphrase_from_options,needs_passphrase,root_extended_priv_key_with_callback, etc.).passphraseparameter fromWalletManager::create_wallet_from_mnemonic[_return_serialized_bytes]and from the matchingkey-wallet-ffi/dash-spv-ffiextern functions; updates every call site (44 files).Rationale
There's no realistic use case for a passphrase-protected wallet that still keeps the mnemonic in memory. If a caller cares enough about the passphrase to want it, they almost certainly want the keys to live behind an external signer (hardware wallet / remote signer) — i.e.
WalletType::ExternalSignable. The other in-memory wallet variants (Mnemonic,Seed,ExtendedPrivKey) are primarily there for tests and embedded systems where the threat model that motivates a BIP39 passphrase doesn't apply. Carrying a callback-driven passphrase variant added a lot of API surface (everyadd_account/derive_*/add_managed_*had a_with_passphrasetwin) for a path no production caller should be on.Breaking changes
passphraseparameter is gone from the public Rust APIs and fromextern "C"FFI functions — callers must update their code.Test plan
cargo build --workspace --all-features --testscargo test --workspace --all-features --lib(key-wallet: 477, key-wallet-manager: 40, key-wallet-ffi: 215, dash-spv: 412, dash-spv-ffi: 44 — all pass)cargo clippy --workspace --all-features --all-targets -- -D warningscargo fmt🤖 Generated with Claude Code