feat(key-wallet): add BlockchainIdentities account type#554
feat(key-wallet): add BlockchainIdentities account type#554
Conversation
📝 WalkthroughWalkthroughThis pull request adds four new BlockchainIdentities account type variants (ECDSA, ECDSA_HASH160, BLS, BLS_HASH160) across the key-wallet and key-wallet-ffi crates. These variants derive authentication keys at Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Pull request overview
Adds a new BlockchainIdentities account type to the key-wallet crate so that identity authentication addresses at m/9'/coinType'/5'/subfeature'/index are automatically monitored by SPV bloom filters. Previously these addresses were only bootstrapped externally by Dash Evo Tool.
Changes:
- New
BlockchainIdentities { subfeature: u32 }variant added acrossAccountType,ManagedAccountType,AccountTypeToCheck,CoreAccountTypeMatch,AccountCollection, andManagedAccountCollection, with accounts for subfeatures 0–3 created during default wallet setup. - FFI support via
FFIAccountType::BlockchainIdentities(value 16) with corresponding match arms in all FFI functions. - Transaction routing updated to check
BlockchainIdentitiesaccounts forTransactionType::Standard, enabling SPV detection of UTXOs sent to these addresses.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| key-wallet/src/account/account_type.rs | New BlockchainIdentities variant with derivation path, path reference, and type conversions |
| key-wallet/src/account/account_collection.rs | BTreeMap<u32, Account> storage for blockchain identity accounts |
| key-wallet/src/account/mod.rs | Unit tests for derivation paths on testnet and mainnet |
| key-wallet/src/managed_account/managed_account_type.rs | ManagedAccountType::BlockchainIdentities with AddressPoolType::Absent |
| key-wallet/src/managed_account/managed_account_collection.rs | Collection management, creation, and iteration for blockchain identity accounts |
| key-wallet/src/managed_account/mod.rs | Pattern match additions for address pool access |
| key-wallet/src/transaction_checking/transaction_router/mod.rs | AccountTypeToCheck::BlockchainIdentities and inclusion in Standard tx checks |
| key-wallet/src/transaction_checking/account_checker.rs | CoreAccountTypeMatch::BlockchainIdentities and indexed account checking |
| key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs | Updated test expectations for shared-path detection |
| key-wallet/src/wallet/helper.rs | Account creation in default setup and xpub retrieval |
| key-wallet-ffi/src/types.rs | FFI enum value 16 and conversions |
| key-wallet-ffi/src/transaction_checking.rs | FFI match arm for transaction check results |
| key-wallet-ffi/src/managed_account.rs | FFI account lookup and type conversion |
| key-wallet-ffi/src/address_pool.rs | FFI address pool access by subfeature |
| key-wallet-ffi/include/key_wallet_ffi.h | C header enum addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add four concrete BlockchainIdentities account type variants for identity authentication keys at m/9'/coinType'/5'/subfeature': - BlockchainIdentitiesECDSA (subfeature 0) - BlockchainIdentitiesECDSAHash160 (subfeature 1) - BlockchainIdentitiesBLS (subfeature 2) - BlockchainIdentitiesBLSHash160 (subfeature 3) These accounts are created by WalletAccountCreationOptions::Default, use AddressPoolType::Absent (non-hardened leaf), and are routed via TransactionType::Standard so SPV bloom filters cover them automatically. Includes FFI bindings (FFIAccountType values 16-19). Closes #553 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
force-pushed
the
feat/blockchain-identities-account-type
branch
from
0025841 to
bb8b5e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds four concrete BlockchainIdentities account type variants (ECDSA, ECDSAHash160, BLS, BLSHash160) to enable SPV bloom filter monitoring of identity authentication addresses at DIP-9 derivation paths m/9'/coinType'/5'/subfeature'/index.
Changes:
- Added four new
AccountType,ManagedAccountType,CoreAccountTypeMatch, andAccountTypeToCheckvariants across key-wallet, with correspondingAccountCollectionandManagedAccountCollectionfields - Included the new account types in
WalletAccountCreationOptions::Defaultcreation andTransactionType::Standardrouting - Extended key-wallet-ffi with
FFIAccountTypevalues 16–19, account lookup, and C header definitions
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| key-wallet/src/account/account_type.rs | New AccountType variants with derivation paths and conversions |
| key-wallet/src/account/account_collection.rs | Option<Account> fields and match arms for new variants |
| key-wallet/src/account/mod.rs | Unit tests for derivation paths on testnet/mainnet |
| key-wallet/src/managed_account/managed_account_type.rs | ManagedAccountType variants with AddressPoolType::Absent |
| key-wallet/src/managed_account/managed_account_collection.rs | Collection fields, creation, iteration for new variants |
| key-wallet/src/managed_account/mod.rs | Address extraction match arms for new variants |
| key-wallet/src/transaction_checking/account_checker.rs | CoreAccountTypeMatch variants and transaction checking |
| key-wallet/src/transaction_checking/transaction_router/mod.rs | Added to Standard tx routing and AccountTypeToCheck |
| key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs | Updated test to reflect shared-path detection behavior |
| key-wallet/src/wallet/helper.rs | Account creation and xpub lookup for new variants |
| key-wallet-ffi/src/types.rs | FFI enum values 16–19 and conversions |
| key-wallet-ffi/src/transaction_checking.rs | FFI match arms for transaction checking |
| key-wallet-ffi/src/managed_account.rs | FFI account lookup and type conversion |
| key-wallet-ffi/src/address_pool.rs | FFI address pool access for new variants |
| key-wallet-ffi/include/key_wallet_ffi.h | C header enum additions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Default wallet creation now produces 15 accounts (was 11) with the addition of 4 BlockchainIdentities variants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
key-wallet/src/managed_account/mod.rs (1)
246-261: Consider centralizing single-pool variant handling to reduce future miss risk.The same variant list is repeated across multiple methods. A shared helper on
ManagedAccountType(e.g., returning single-pooladdresses) would reduce maintenance drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 246 - 261, The pattern matching for single-pool variants is duplicated; add a helper on ManagedAccountType (e.g., a method like fn single_pool_addresses(&self) -> Option<&Vec<AddressType>>) that returns Some(&addresses) for BlockchainIdentitiesECDSA, BlockchainIdentitiesECDSAHash160, BlockchainIdentitiesBLS, and BlockchainIdentitiesBLSHash160 and None otherwise, then replace the repeated match arms in each method with a call to single_pool_addresses() to centralize handling and avoid future misses.key-wallet/src/account/mod.rs (1)
576-579: Avoid hardcoding coin-type network parameters in these assertions.Please derive expected coin-type values from the network/source-of-truth helper instead of embedding
1/5directly, so tests stay aligned with network parameter definitions.As per coding guidelines "Never hardcode network parameters, addresses, or keys in Rust code."
Also applies to: 597-600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/account/mod.rs` around lines 576 - 579, The assertions hardcode coin-type indices (1 and 5) instead of using the network source-of-truth; replace those literals with the coin-type derived from the network/helper (e.g., call the project’s network coin-type accessor or a helper like get_coin_type(network) and use ChildNumber::from_hardened_idx(coin_type).unwrap() in place of 1 and 5), updating both the children assertions shown (the ones using ChildNumber::from_hardened_idx(1) and ChildNumber::from_hardened_idx(5)) and the similar assertions at the other occurrence (lines referenced in the comment), so tests derive coin-type from the canonical network helper rather than hardcoded values.key-wallet/src/managed_account/managed_account_type.rs (1)
718-752: Consider replacingunreachable!()with explicit match arms.The
unreachable!()at line 750 could theoretically panic if the code is refactored incorrectly in the future. While it's currently safe due to the outer match guard, per coding guidelines for library code, prefer avoiding panic paths.♻️ Alternative: Use explicit match arms instead of combined pattern
- AccountType::BlockchainIdentitiesECDSA - | AccountType::BlockchainIdentitiesECDSAHash160 - | AccountType::BlockchainIdentitiesBLS - | AccountType::BlockchainIdentitiesBLSHash160 => { - let path = account_type - .derivation_path(network) - .unwrap_or_else(|_| DerivationPath::master()); - let pool = AddressPool::new( - path, - AddressPoolType::Absent, - DEFAULT_SPECIAL_GAP_LIMIT, - network, - key_source, - )?; - - Ok(match account_type { - AccountType::BlockchainIdentitiesECDSA => Self::BlockchainIdentitiesECDSA { - addresses: pool, - }, - AccountType::BlockchainIdentitiesECDSAHash160 => { - Self::BlockchainIdentitiesECDSAHash160 { - addresses: pool, - } - } - AccountType::BlockchainIdentitiesBLS => Self::BlockchainIdentitiesBLS { - addresses: pool, - }, - AccountType::BlockchainIdentitiesBLSHash160 => { - Self::BlockchainIdentitiesBLSHash160 { - addresses: pool, - } - } - _ => unreachable!(), - }) - } + AccountType::BlockchainIdentitiesECDSA => { + let path = account_type + .derivation_path(network) + .unwrap_or_else(|_| DerivationPath::master()); + let pool = AddressPool::new( + path, + AddressPoolType::Absent, + DEFAULT_SPECIAL_GAP_LIMIT, + network, + key_source, + )?; + Ok(Self::BlockchainIdentitiesECDSA { addresses: pool }) + } + AccountType::BlockchainIdentitiesECDSAHash160 => { + let path = account_type + .derivation_path(network) + .unwrap_or_else(|_| DerivationPath::master()); + let pool = AddressPool::new( + path, + AddressPoolType::Absent, + DEFAULT_SPECIAL_GAP_LIMIT, + network, + key_source, + )?; + Ok(Self::BlockchainIdentitiesECDSAHash160 { addresses: pool }) + } + AccountType::BlockchainIdentitiesBLS => { + let path = account_type + .derivation_path(network) + .unwrap_or_else(|_| DerivationPath::master()); + let pool = AddressPool::new( + path, + AddressPoolType::Absent, + DEFAULT_SPECIAL_GAP_LIMIT, + network, + key_source, + )?; + Ok(Self::BlockchainIdentitiesBLS { addresses: pool }) + } + AccountType::BlockchainIdentitiesBLSHash160 => { + let path = account_type + .derivation_path(network) + .unwrap_or_else(|_| DerivationPath::master()); + let pool = AddressPool::new( + path, + AddressPoolType::Absent, + DEFAULT_SPECIAL_GAP_LIMIT, + network, + key_source, + )?; + Ok(Self::BlockchainIdentitiesBLSHash160 { addresses: pool }) + }Alternatively, you could extract a helper function to reduce duplication while keeping explicit arms.
As per coding guidelines: "Never panic in library code; always use Result for fallible operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/managed_account_type.rs` around lines 718 - 752, The match currently uses a combined pattern for AccountType variants then returns a nested match with a final `_ => unreachable!()` which can panic if refactored; instead enumerate explicit match arms for each variant (AccountType::BlockchainIdentitiesECDSA, AccountType::BlockchainIdentitiesECDSAHash160, AccountType::BlockchainIdentitiesBLS, AccountType::BlockchainIdentitiesBLSHash160) and return the corresponding Self::... variants directly after creating the AddressPool (from derivation_path(network).unwrap_or_else(|_| DerivationPath::master()) and AddressPool::new(..., DEFAULT_SPECIAL_GAP_LIMIT, network, key_source)), removing the wildcard arm so all handled cases are explicit and no unreachable!() remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/address_pool.rs`:
- Around line 52-59: The managed_wallet_mark_address_used path fails to scan the
new AccountType variants that live in collection.blockchain_identities_ecdsa,
collection.blockchain_identities_ecdsa_hash160,
collection.blockchain_identities_bls, and
collection.blockchain_identities_bls_hash160, causing NotFound and no gap/usage
advancement; update managed_wallet_mark_address_used to include these four
collections when matching AccountType::BlockchainIdentitiesECDSA,
::BlockchainIdentitiesECDSAHash160, ::BlockchainIdentitiesBLS, and
::BlockchainIdentitiesBLSHash160 so the function searches the correct
collection, marks the address used, and advances the usage/gap state (mirroring
the logic used for other account types).
In `@key-wallet-ffi/src/types.rs`:
- Around line 197-204: Add unit tests that perform round-trip conversions for
the new FFI account variants (BlockchainIdentitiesECDSA,
BlockchainIdentitiesECDSAHash160, BlockchainIdentitiesBLS,
BlockchainIdentitiesBLSHash160) to ensure the public ABI discriminants and
conversion branches remain correct; locate the conversion functions in this
module (the enum and its to/from-ffi conversion implementations) and add tests
that convert each new variant to the FFI representation and back, asserting
equality, and mirror similar tests already present for older variants so the new
branches are exercised.
In `@key-wallet/src/transaction_checking/account_checker.rs`:
- Around line 404-427: find_address_account in ManagedCoreAccount does not
consider the new BlockchainIdentities* collections so address-to-account lookups
return None for DIP-9 paths; update ManagedCoreAccount::find_address_account to
include checks against collection.blockchain_identities_ecdsa,
collection.blockchain_identities_ecdsa_hash160,
collection.blockchain_identities_bls, and
collection.blockchain_identities_bls_hash160 (mirroring how AccountTypeToCheck
variants are handled) so that the lookup iterates those collections and returns
the matching account when an address is found; ensure you use the same matching
helper (e.g., the existing per-account check function used elsewhere) and
maintain existing return types and error handling.
In
`@key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs`:
- Around line 231-260: The test uses the shared m/9'/coinType'/5'/1' path but
allows any BlockchainIdentities* variant, so tighten the assertion to check the
single exact expected enum variant (e.g., replace the multi-branch match over
AccountTypeToCheck::BlockchainIdentitiesECDSA | ... with the specific variant
your fixture actually produces such as
AccountTypeToCheck::BlockchainIdentitiesECDSAHash160 or BlockchainIdentitiesBLS,
referencing the result variable and its affected_accounts iterator), update the
assertion message accordingly, and rename the test function from the misleading
*_not_detected name to something like
*_detected_via_blockchain_identities_shared_path to reflect the expected
behavior.
---
Nitpick comments:
In `@key-wallet/src/account/mod.rs`:
- Around line 576-579: The assertions hardcode coin-type indices (1 and 5)
instead of using the network source-of-truth; replace those literals with the
coin-type derived from the network/helper (e.g., call the project’s network
coin-type accessor or a helper like get_coin_type(network) and use
ChildNumber::from_hardened_idx(coin_type).unwrap() in place of 1 and 5),
updating both the children assertions shown (the ones using
ChildNumber::from_hardened_idx(1) and ChildNumber::from_hardened_idx(5)) and the
similar assertions at the other occurrence (lines referenced in the comment), so
tests derive coin-type from the canonical network helper rather than hardcoded
values.
In `@key-wallet/src/managed_account/managed_account_type.rs`:
- Around line 718-752: The match currently uses a combined pattern for
AccountType variants then returns a nested match with a final `_ =>
unreachable!()` which can panic if refactored; instead enumerate explicit match
arms for each variant (AccountType::BlockchainIdentitiesECDSA,
AccountType::BlockchainIdentitiesECDSAHash160,
AccountType::BlockchainIdentitiesBLS,
AccountType::BlockchainIdentitiesBLSHash160) and return the corresponding
Self::... variants directly after creating the AddressPool (from
derivation_path(network).unwrap_or_else(|_| DerivationPath::master()) and
AddressPool::new(..., DEFAULT_SPECIAL_GAP_LIMIT, network, key_source)), removing
the wildcard arm so all handled cases are explicit and no unreachable!()
remains.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 246-261: The pattern matching for single-pool variants is
duplicated; add a helper on ManagedAccountType (e.g., a method like fn
single_pool_addresses(&self) -> Option<&Vec<AddressType>>) that returns
Some(&addresses) for BlockchainIdentitiesECDSA,
BlockchainIdentitiesECDSAHash160, BlockchainIdentitiesBLS, and
BlockchainIdentitiesBLSHash160 and None otherwise, then replace the repeated
match arms in each method with a call to single_pool_addresses() to centralize
handling and avoid future misses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44c65f5b-19a8-4b11-80fd-82d5821ded62
📒 Files selected for processing (16)
key-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/account/account_collection.rskey-wallet/src/account/account_type.rskey-wallet/src/account/mod.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/wallet/helper.rs
key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
Show resolved
Hide resolved
…lookup (#555) * Initial plan * fix: add blockchain_identities_* checks to find_address_account() Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
…_address_used scan (#557) * Initial plan * fix: add blockchain_identities_* scanning to managed_wallet_mark_address_used Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
…ariant (#556) * Initial plan * test: rename and narrow BlockchainIdentities test to exact ECDSA_HASH160 shared-path variant Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/src/transaction_checking/account_checker.rs (1)
404-427: Optional: reduce variant-duplication in account dispatch/lookup.This is correct as-is, but a small helper-driven dispatch for
BlockchainIdentities*would reduce maintenance drift if more subfeatures are added later.Also applies to: 1106-1130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction_checking/account_checker.rs` around lines 404 - 427, Reduce duplication by adding a small helper on AccountChecker (e.g., a method like check_blockchain_identity(&self, chooser: impl Fn(&Self) -> Option<&AccountType>, tx: &Tx) -> Vec<_>) that encapsulates the pattern .as_ref().and_then(|account| account.check_transaction_for_match(tx, None)).into_iter().collect(); then replace the four match arms for AccountTypeToCheck::BlockchainIdentitiesECDSA, ::BlockchainIdentitiesECDSAHash160, ::BlockchainIdentitiesBLS, and ::BlockchainIdentitiesBLSHash160 to call that helper with a selector that returns the corresponding field (blockchain_identities_ecdsa, blockchain_identities_ecdsa_hash160, blockchain_identities_bls, blockchain_identities_bls_hash160), keeping the same behavior and return type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet/src/transaction_checking/account_checker.rs`:
- Around line 404-427: Reduce duplication by adding a small helper on
AccountChecker (e.g., a method like check_blockchain_identity(&self, chooser:
impl Fn(&Self) -> Option<&AccountType>, tx: &Tx) -> Vec<_>) that encapsulates
the pattern .as_ref().and_then(|account| account.check_transaction_for_match(tx,
None)).into_iter().collect(); then replace the four match arms for
AccountTypeToCheck::BlockchainIdentitiesECDSA,
::BlockchainIdentitiesECDSAHash160, ::BlockchainIdentitiesBLS, and
::BlockchainIdentitiesBLSHash160 to call that helper with a selector that
returns the corresponding field (blockchain_identities_ecdsa,
blockchain_identities_ecdsa_hash160, blockchain_identities_bls,
blockchain_identities_bls_hash160), keeping the same behavior and return type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b014f95-122b-499d-8144-a3d7822c781e
📒 Files selected for processing (3)
key-wallet-ffi/src/address_pool.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
…identities-account-type
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Pull request overview
Adds dedicated BlockchainIdentities account-type variants across key-wallet and key-wallet-ffi so the SPV transaction router checks identity-derivation-path addresses during standard transaction relevance checks (and exposes them via FFI).
Changes:
- Introduces four new
AccountType/ManagedAccountType/AccountTypeToCheckvariants for BlockchainIdentities (ECDSA, ECDSAHash160, BLS, BLSHash160). - Creates these accounts as part of
WalletAccountCreationOptions::Defaultand routesTransactionType::Standardthrough them. - Extends account collections, matchers, and FFI enums/headers to support lookup and transaction matching for the new variants.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| key-wallet/src/account/account_type.rs | Adds new AccountType variants, derivation paths, and conversions to AccountTypeToCheck. |
| key-wallet/src/account/account_collection.rs | Adds storage/accessors for the new account variants in AccountCollection. |
| key-wallet/src/account/mod.rs | Adds unit tests validating derivation paths and DerivationPathReference for the new variants. |
| key-wallet/src/wallet/helper.rs | Ensures Default wallet creation includes the new BlockchainIdentities accounts and exposes their xpubs. |
| key-wallet/src/managed_account/managed_account_type.rs | Adds new ManagedAccountType variants and address-pool creation for them. |
| key-wallet/src/managed_account/managed_account_collection.rs | Adds the new variants into managed collection storage, creation, and iteration. |
| key-wallet/src/managed_account/mod.rs | Extends ManagedCoreAccount helpers to treat the new variants as single-pool address accounts. |
| key-wallet/src/transaction_checking/transaction_router/mod.rs | Routes standard transactions through the new account types and adds conversions from managed account types. |
| key-wallet/src/transaction_checking/account_checker.rs | Extends matching and “contains address” checks to include the new account types. |
| key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs | Updates routing expectations for identity-path standard transactions. |
| key-wallet/tests/integration_test.rs | Updates Default account-count expectation in integration test. |
| key-wallet-ffi/src/types.rs | Adds FFIAccountType values (16–19) and conversions to/from AccountType. |
| key-wallet-ffi/src/managed_account.rs | Exposes the new account types via managed-account getters and type conversions. |
| key-wallet-ffi/src/address_pool.rs | Extends address-pool lookup/mark-used logic to include the new accounts. |
| key-wallet-ffi/src/transaction_checking.rs | Adds transaction-match mapping for the new core account match variants. |
| key-wallet-ffi/include/key_wallet_ffi.h | Exposes the new FFI account-type constants in the C header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Code Review: feat(key-wallet): add BlockchainIdentities account type
Summary
Adds 4 BlockchainIdentitiesECDSA/ECDSAHash160/BLS/BLSHash160 account type variants for DIP-9 identity authentication key monitoring at m/9'/coinType'/5'/subfeature'/index. Plumbed through all the expected layers: AccountType, ManagedAccountType, AccountTypeToCheck, CoreAccountTypeMatch, both collection structs, FFI bindings, transaction routing, wallet creation, and tests.
The implementation follows existing crate conventions consistently. A few items worth addressing:
Issues
1. Shared derivation path with IdentityRegistration
The test rename reveals that BlockchainIdentitiesECDSAHash160 (subfeature 1) shares path m/9'/coinType'/5'/1' with IdentityRegistration. This means the same addresses are monitored by two account types — standard transactions match via BlockchainIdentitiesECDSAHash160, asset-lock transactions via IdentityRegistration. Is this intentional? Could it cause duplicate bloom filter entries or double-counted funds?
2. Silent fallback on derivation error
In ManagedAccountType::from_account_type():
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());This silently falls back to the master path if derivation fails. Should propagate the error instead — a wrong derivation path would be very hard to debug.
3. unreachable!() in nested matches
The subfeature dispatch in derivation_path() and ManagedAccountCollection uses an inner match with _ => unreachable!() after the outer match already narrowed to the 4 variants. A small fn subfeature(&self) -> u32 helper on AccountType would eliminate these and be reusable.
4. Fully-qualified paths in wallet/helper.rs
The new match arms use crate::transaction_checking::transaction_router::AccountTypeToCheck::BlockchainIdentitiesECDSA inline rather than a top-level use import. Follows the existing pattern in that function but the lines are getting quite long.
Looks Good
- Exhaustive pattern matching everywhere — compiler will catch missing cases
- Tests verify derivation paths for both testnet and mainnet
- Correctly added to
TransactionType::Standardrouting - FFI enum values 16-19 are contiguous and consistent
AddressPoolType::AbsentwithDEFAULT_SPECIAL_GAP_LIMITmatches similar account types
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
I don't get the point of this PR. It will slow down sync, and for what case? Someone did a registration where the key used for registration was not in the wallet, but then used a key for identities that was in the wallet?
|
Note: this response was prepared by my AI assistant (Claudius) based on the context of DET issues and PRs — posting on my behalf. The use case is DET (Dash Evo Tool) wallet integration. DET derives BlockchainIdentities addresses at The concrete scenario is: user creates an identity through DET, tops it up from a Core wallet address. The topup transaction sends funds to an identity authentication key address derived at the BlockchainIdentities path. Without the SDK knowing about these addresses, SPV never notifies DET about the transaction → the user's wallet shows stale balances and no transaction history for platform operations. dash-evo-tool PR #752 patched the UI to fetch balances via RPC as a workaround, but the proper fix is making these addresses first-class in the SDK so SPV monitors them. dash-evo-tool Issue #779 (full transaction history for platform addresses) is blocked on this. Re: sync slowdown — these are 4 additional account types with 🤖 Co-authored by Claudius the Magnificent AI Agent |
…identities-account-type # Conflicts: # key-wallet-ffi/include/key_wallet_ffi.h
…) match Post-merge fix: upstream PR #605 added `involved_receive_addresses()` method with exhaustive match on CoreAccountTypeMatch, but our 4 new BlockchainIdentities variants were not covered. They follow the same single-pool `involved_addresses` pattern as other special account types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/src/managed_account/managed_account_collection.rs (1)
509-528: Silent error handling may hide conversion failures.The
from_account_collection()method silently discards conversion errors viaif let Ok(...). While this follows the existing pattern in the file, consider logging a warning when account conversion fails to aid debugging, especially since these are new account types that could encounter unexpected issues during migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/managed_account_collection.rs` around lines 509 - 528, The from_account_collection() conversion currently swallows errors when calling Self::create_managed_account_from_account(...) for each new field (blockchain_identities_ecdsa, blockchain_identities_ecdsa_hash160, blockchain_identities_bls, blockchain_identities_bls_hash160); change the pattern so failures are logged as warnings instead of ignored — e.g. match the Result and on Err emit a warning that includes the target field name and the error details, and on Ok set the corresponding managed_collection.<field> so you still preserve the success path while surfacing conversion problems for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 509-528: The from_account_collection() conversion currently
swallows errors when calling Self::create_managed_account_from_account(...) for
each new field (blockchain_identities_ecdsa,
blockchain_identities_ecdsa_hash160, blockchain_identities_bls,
blockchain_identities_bls_hash160); change the pattern so failures are logged as
warnings instead of ignored — e.g. match the Result and on Err emit a warning
that includes the target field name and the error details, and on Ok set the
corresponding managed_collection.<field> so you still preserve the success path
while surfacing conversion problems for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0460ea1-22bb-43fd-8bfd-03660dc70fec
📒 Files selected for processing (12)
key-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/account/account_collection.rskey-wallet/src/account/mod.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/wallet/helper.rs
✅ Files skipped from review due to trivial changes (4)
- key-wallet-manager/tests/integration_test.rs
- key-wallet/src/managed_account/mod.rs
- key-wallet/src/account/mod.rs
- key-wallet-ffi/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
- key-wallet-ffi/src/transaction_checking.rs
- key-wallet/src/wallet/helper.rs
- key-wallet/src/transaction_checking/transaction_router/mod.rs
- key-wallet/src/account/account_collection.rs
Summary
Adds four concrete
BlockchainIdentitiesaccount type variants so SPV monitors identity authentication addresses atm/9'/coinType'/5'/subfeature'/index(DIP-9). Previously these addresses were invisible to bloom filters because they weren't created byWalletAccountCreationOptions::Default.New Variants
BlockchainIdentitiesECDSAm/9'/coinType'/5'/0'/indexBlockchainIdentitiesECDSAHash160m/9'/coinType'/5'/1'/indexBlockchainIdentitiesBLSm/9'/coinType'/5'/2'/indexBlockchainIdentitiesBLSHash160m/9'/coinType'/5'/3'/indexDesign
Follows the established crate convention of concrete dedicated variants (like
ProviderVotingKeys,ProviderOwnerKeys, etc.) rather than a parameterizedBlockchainIdentities { subfeature: u32 }. Each variant gets its ownOption<Account>field in collections.AccountType,ManagedAccountType,AccountTypeToCheck,CoreAccountTypeMatch,AccountCollection,ManagedAccountCollection, transaction routing, wallet helper, unit testsFFIAccountTypevalues 16–19, account lookup, managed account conversion, transaction checking, C headerAddressPoolType::Absent(non-hardened leaf) withDEFAULT_SPECIAL_GAP_LIMITTransactionType::Standardrouting so SPV detects UTXOs at these addressesDerivationPathReference::BlockchainIdentitiesCloses #553
User Story
As a Dash Evo Tool user, I want my identity authentication addresses to be automatically monitored by the SPV client so that I receive transaction notifications for funds sent to those addresses without manually bootstrapping them outside the SDK.
Test Plan
cargo check --workspace --all-features— cleancargo test -p key-wallet -p key-wallet-ffi— all passcargo clippy --workspace --all-features— no warningsmonitored_addresses()includes BlockchainIdentities addressesspv_account_metadata()match arm (dash-evo-tool#692)🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Release Notes
New Features
Tests