refactor(key-wallet): remove redundant HDWallet + AccountDerivation struct#674
Merged
xdustinface merged 1 commit intov0.42-devfrom Apr 22, 2026
Merged
refactor(key-wallet): remove redundant HDWallet + AccountDerivation struct#674xdustinface merged 1 commit intov0.42-devfrom
xdustinface merged 1 commit intov0.42-devfrom
Conversation
…truct Both were thin wrappers over ExtendedPrivKey + a Secp256k1 context, providing one-line helpers already available directly on dashcore's bip32 types. Every non-test caller (wallet/accounts.rs, wallet/bip38.rs) used them only as transient wrappers — inlined to raw derive_priv. Specifically removed: - `HDWallet` struct and impl (from_seed/new, master_key, derive, bip44_account, coinjoin_account, identity_authentication_key) - top-level `AccountDerivation` struct and impl (receive_address, change_address) Explicitly NOT touched: - The `AccountDerivation` *trait* in `account/derivation.rs` — a different item, implemented by Account/BLSAccount/EdDSAAccount and used heavily. The name collision was a trap; only the struct in `derivation.rs` is removed. - `KeyDerivation` trait, `DerivationPathBuilder`, `DerivationStrategy` in `derivation.rs` — still re-exported from lib.rs. `HDWallet::identity_authentication_key` was known-buggy (produced a 6-level path missing the DIP-13 `key_type'` level — incompatible with DashSync reference wallets). That bug evaporates with the struct. `tests/derivation_tests.rs`'s test for it (asserting the buggy depth) went with it. `key-wallet-ffi/src/lib_tests.rs` deleted — it only existed to exercise `HDWallet::from_seed` and assorted FFI-internal paths that are covered by integration_test.rs. Verified: both `--all-features` and `--no-default-features` pass build, lib tests (463 / 453), integration tests (21), clippy -D warnings, and fmt. key-wallet-ffi (217 lib tests) and key-wallet-manager (36) build and test clean.
Contributor
📝 WalkthroughWalkthroughThe PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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/derivation.rs (1)
442-459:⚠️ Potential issue | 🟡 MinorAvoid retaining the obsolete identity-authentication path as coverage.
Line 444 still derives
m/9'/5'/1'/0and Line 460 only checks that it differs from the master key. Since this PR removes a known-buggy identity derivation helper, this smoke assertion can make an incompatible legacy path look covered. Prefer deleting this block or asserting the correctAccountType/DIP-13 path against a deterministic vector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/derivation.rs` around lines 442 - 459, Remove the obsolete identity-authentication derivation test block that builds identity_auth_path (m/9'/5'/1'/0) and derives identity_key via master_key.derive_priv, because it falsely signals coverage for a removed buggy helper; instead either delete that entire block or replace it with an assertion that derives and verifies the correct DIP-13/AccountType path (use the project’s canonical AccountType/DIP-13 derivation helper and compare against a deterministic vector) so the test checks the supported derivation rather than the legacy m/9'/5'/1'/0 path.key-wallet/src/tests/account_tests.rs (1)
378-408:⚠️ Potential issue | 🟠 MajorDon’t create a mainnet account from a testnet-derived key.
Line 379 derives
account_keyfrom the testnet path, but Line 408 reuses that same xpriv forNetwork::Mainnet. This test currently bakes in network mixing instead of validating consistency. Derive a separate mainnet key, or assert that reusing the testnet key is rejected.Proposed test fix
// Test creating account with different network let dash_mainnet = Network::Mainnet; + let mainnet_master = create_test_extended_priv_key(dash_mainnet); + let mainnet_derivation_path = account_type.derivation_path(dash_mainnet).unwrap(); + let mainnet_account_key = mainnet_master + .derive_priv(&secp, &mainnet_derivation_path) + .unwrap(); let mainnet_account = - Account::from_xpriv(Some([0u8; 32]), account_type, account_key, dash_mainnet).unwrap(); + Account::from_xpriv(Some([0u8; 32]), account_type, mainnet_account_key, dash_mainnet) + .unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/tests/account_tests.rs` around lines 378 - 408, The test incorrectly reuses a testnet-derived xpriv (account_key) to create a Mainnet Account; either derive a new mainnet key or assert that mixing networks is rejected. Fix by computing a separate derivation_path for dash_mainnet via account_type.derivation_path(dash_mainnet).unwrap() and derive account_key_mainnet = master.derive_priv(&secp, &derivation_path_mainnet).unwrap(), then call Account::from_xpriv(Some([0u8;32]), account_type, account_key_mainnet, dash_mainnet).unwrap(); alternatively, change the test to call Account::from_xpriv(Some([0u8;32]), account_type, account_key, dash_mainnet) and assert that it returns an Err to validate the rejection behavior.key-wallet/tests/derivation_tests.rs (1)
87-101:⚠️ Potential issue | 🟡 MinorReplace tautological testnet assertions with deterministic checks.
pubkey_hash.to_byte_array()is fixed-size, so Lines 101, 162, and 224 will not catch an incorrect testnet derivation. Add exact testnet HASH160/public-key vectors, or at least assert the testnet result differs from the corresponding mainnet vector.Minimal stronger checks for the combined mainnet/testnet cases
let pubkey_hash_testnet = pubkey_testnet.pubkey_hash(); - // Verify testnet derivation produces valid hash - assert!(!pubkey_hash_testnet.to_byte_array().is_empty()); + // Verify testnet derivation is not accidentally using the mainnet path/vector. + assert_ne!(xprv_testnet.private_key, xprv_mainnet.private_key); + assert_ne!(pubkey_hash_testnet, pubkey_hash_mainnet);For
test_dip17_platform_payment_vector1_testnet, prefer an exact expected testnet HASH160/public-key assertion once that vector is available.Also applies to: 153-162, 215-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/tests/derivation_tests.rs` around lines 87 - 101, The current assertion using pubkey_hash.to_byte_array().is_empty() is tautological because HASH160 is fixed-size; update the test in test_dip17_platform_payment_vector1_testnet to assert a deterministic expected value: compute the derived pubkey_hash via the same pipeline (mnemonic.to_seed, ExtendedPrivKey::new_master(Network::Testnet, ...), DerivationPath::from_str(...), master_key.derive_priv(...), ExtendedPubKey::from_priv(...), PublicKey::new(...), pubkey.pubkey_hash()) and compare its byte array to a known expected HASH160 byte vector (or at minimum assert it differs from the mainnet result by deriving the same path with Network::Bitcoin and comparing those two byte arrays); replace the tautological assert! with assert_eq!(actual_bytes, expected_bytes) or assert_ne!(testnet_bytes, mainnet_bytes) using the symbols seed, master_key, path, xprv, xpub, pubkey, and pubkey_hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet/src/derivation.rs`:
- Around line 442-459: Remove the obsolete identity-authentication derivation
test block that builds identity_auth_path (m/9'/5'/1'/0) and derives
identity_key via master_key.derive_priv, because it falsely signals coverage for
a removed buggy helper; instead either delete that entire block or replace it
with an assertion that derives and verifies the correct DIP-13/AccountType path
(use the project’s canonical AccountType/DIP-13 derivation helper and compare
against a deterministic vector) so the test checks the supported derivation
rather than the legacy m/9'/5'/1'/0 path.
In `@key-wallet/src/tests/account_tests.rs`:
- Around line 378-408: The test incorrectly reuses a testnet-derived xpriv
(account_key) to create a Mainnet Account; either derive a new mainnet key or
assert that mixing networks is rejected. Fix by computing a separate
derivation_path for dash_mainnet via
account_type.derivation_path(dash_mainnet).unwrap() and derive
account_key_mainnet = master.derive_priv(&secp,
&derivation_path_mainnet).unwrap(), then call
Account::from_xpriv(Some([0u8;32]), account_type, account_key_mainnet,
dash_mainnet).unwrap(); alternatively, change the test to call
Account::from_xpriv(Some([0u8;32]), account_type, account_key, dash_mainnet) and
assert that it returns an Err to validate the rejection behavior.
In `@key-wallet/tests/derivation_tests.rs`:
- Around line 87-101: The current assertion using
pubkey_hash.to_byte_array().is_empty() is tautological because HASH160 is
fixed-size; update the test in test_dip17_platform_payment_vector1_testnet to
assert a deterministic expected value: compute the derived pubkey_hash via the
same pipeline (mnemonic.to_seed, ExtendedPrivKey::new_master(Network::Testnet,
...), DerivationPath::from_str(...), master_key.derive_priv(...),
ExtendedPubKey::from_priv(...), PublicKey::new(...), pubkey.pubkey_hash()) and
compare its byte array to a known expected HASH160 byte vector (or at minimum
assert it differs from the mainnet result by deriving the same path with
Network::Bitcoin and comparing those two byte arrays); replace the tautological
assert! with assert_eq!(actual_bytes, expected_bytes) or
assert_ne!(testnet_bytes, mainnet_bytes) using the symbols seed, master_key,
path, xprv, xpub, pubkey, and pubkey_hash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fd99719-9ec8-45e9-9ad9-b0858fef50c7
📒 Files selected for processing (6)
key-wallet-ffi/src/lib_tests.rskey-wallet/src/derivation.rskey-wallet/src/tests/account_tests.rskey-wallet/src/wallet/accounts.rskey-wallet/src/wallet/bip38.rskey-wallet/tests/derivation_tests.rs
💤 Files with no reviewable changes (1)
- key-wallet-ffi/src/lib_tests.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #674 +/- ##
=============================================
+ Coverage 68.39% 68.79% +0.40%
=============================================
Files 320 320
Lines 68023 67938 -85
=============================================
+ Hits 46524 46738 +214
+ Misses 21499 21200 -299
|
QuantumExplorer
added a commit
that referenced
this pull request
Apr 22, 2026
PR #674 dropped the add_to_state/bool second arg from next_unused, next_unused_with_info, next_receive_address, and next_change_address. PR #672's new IdentityAuthenticationBls arms and an asset_lock_builder caller still passed the old signatures. PR #672 added AccountType::IdentityAuthenticationEcdsa/Bls variants. Three match statements in ManagedAccountCollection (contains_account_type, get_by_account_type, get_by_account_type_mut) predated those variants and needed arms routing to the existing identity_authentication_ecdsa / _bls BTreeMaps by identity_index. Also update two FFI mark_address_used callsites to destructure the (bool, WalletChangeSet) tuple that #674 returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xdustinface
approved these changes
Apr 22, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Unrelated to here but since you are cleaning up here maybe have a look. Seems like we pass the same default secp context everywhere so maybe we could just move it into the function instead of creating extra on every callsite.
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
Removes two thin-wrapper legacy types from `key-wallet`'s `derivation` module:
Non-test call sites inline to `master.derive_priv(&secp, &path)` or `account.account_type.derivation_path(network)` + `derive_priv` — which the caller already had scope for in every case.
Also resolves a known bug
`HDWallet::identity_authentication_key(identity_index, key_index)` was producing the 6-level path `m/9'/coin'/5'/0'/identity_index'/key_index'` — missing the DIP-13 `key_type'` level. This produced keys incompatible with DashSync Android (dashj) and iOS, which both use the 7-level DIP-13 layout with `key_type'` at level 5. The buggy helper evaporates with the struct; `tests/derivation_tests.rs`'s assertion that encoded the bug (`assert_eq!(identity_key.depth, 6)`) was deleted along with it. DIP-13-correct derivations are available via the `AccountType::IdentityAuthenticationEcdsa { identity_index }` / `IdentityAuthenticationBls { identity_index }` variants added in #672.
Kept intact
Stats
6 files, 97 insertions / 513 deletions.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
HDWalletandAccountDerivationfrom the public API; key derivation now uses direct BIP32 operations.Tests