Skip to content

feat(key-wallet): add BlockchainIdentities account type#554

Open
lklimek wants to merge 5 commits intov0.42-devfrom
feat/blockchain-identities-account-type
Open

feat(key-wallet): add BlockchainIdentities account type#554
lklimek wants to merge 5 commits intov0.42-devfrom
feat/blockchain-identities-account-type

Conversation

@lklimek
Copy link
Contributor

@lklimek lklimek commented Mar 16, 2026

Summary

Adds four concrete BlockchainIdentities account type variants so SPV monitors identity authentication addresses at m/9'/coinType'/5'/subfeature'/index (DIP-9). Previously these addresses were invisible to bloom filters because they weren't created by WalletAccountCreationOptions::Default.

New Variants

Variant Subfeature Path
BlockchainIdentitiesECDSA 0 m/9'/coinType'/5'/0'/index
BlockchainIdentitiesECDSAHash160 1 m/9'/coinType'/5'/1'/index
BlockchainIdentitiesBLS 2 m/9'/coinType'/5'/2'/index
BlockchainIdentitiesBLSHash160 3 m/9'/coinType'/5'/3'/index

Design

Follows the established crate convention of concrete dedicated variants (like ProviderVotingKeys, ProviderOwnerKeys, etc.) rather than a parameterized BlockchainIdentities { subfeature: u32 }. Each variant gets its own Option<Account> field in collections.

  • key-wallet (10 files): AccountType, ManagedAccountType, AccountTypeToCheck, CoreAccountTypeMatch, AccountCollection, ManagedAccountCollection, transaction routing, wallet helper, unit tests
  • key-wallet-ffi (5 files): FFIAccountType values 16–19, account lookup, managed account conversion, transaction checking, C header
  • All use AddressPoolType::Absent (non-hardened leaf) with DEFAULT_SPECIAL_GAP_LIMIT
  • Added to TransactionType::Standard routing so SPV detects UTXOs at these addresses
  • All share DerivationPathReference::BlockchainIdentities

Closes #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

  • Unit tests verify derivation paths for all 4 variants on testnet and mainnet
  • cargo check --workspace --all-features — clean
  • cargo test -p key-wallet -p key-wallet-ffi — all pass
  • cargo clippy --workspace --all-features — no warnings
  • Integration: verify monitored_addresses() includes BlockchainIdentities addresses
  • DET follow-up: add spv_account_metadata() match arm (dash-evo-tool#692)

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features
    • Added four blockchain identity account types: ECDSA, ECDSA Hash160, BLS, BLS Hash160.
    • These accounts are automatically created and managed in the wallet and included in the default account set (now 16).
    • Identity-path/shared-identity addresses are now detected and routed to the appropriate identity account (affecting transaction relevance).
  • Tests
    • Updated/added tests to validate derivation paths and detection behavior for the new identity account types.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 16, 2026

📝 Walkthrough

Walkthrough

Adds four BlockchainIdentities account subtypes (ECDSA, ECDSA_HASH160, BLS, BLS_HASH160) across key-wallet and FFI: new AccountType variants, managed/collection storage, derivation paths, transaction-routing/checking support, wallet initialization, and FFI mappings.

Changes

Cohort / File(s) Summary
FFI Header & Type Definitions
key-wallet-ffi/include/key_wallet_ffi.h, key-wallet-ffi/src/types.rs
Added four new FFIAccountType variants (values 16–19) and bidirectional conversions between FFI and internal AccountType.
FFI Entry Points
key-wallet-ffi/src/managed_account.rs, key-wallet-ffi/src/address_pool.rs, key-wallet-ffi/src/transaction_checking.rs
Extended FFI functions to handle the new BlockchainIdentities account types for account access, address-pool access, and transaction matching.
Core Account Types & Derivation
key-wallet/src/account/account_type.rs, key-wallet/src/account/mod.rs
Added four AccountType variants for BlockchainIdentities with derivation_path_reference and hardened path m/9'/coinType'/5'/subfeature' and unit tests for Testnet/Mainnet derivation paths.
Account Collections
key-wallet/src/account/account_collection.rs
Added four optional fields to store BlockchainIdentities accounts; updated insertion, accessors, iteration, is_empty, clear, and all_accounts aggregation.
Managed Account Types & Collections
key-wallet/src/managed_account/managed_account_type.rs, key-wallet/src/managed_account/managed_account_collection.rs, key-wallet/src/managed_account/mod.rs
Introduced ManagedAccountType variants carrying an AddressPool for each BlockchainIdentities subfeature; wired creation, indexing, address pool access, lifecycle, and routing into ManagedAccountCollection and managed-account flows.
Wallet Initialization & Helpers
key-wallet/src/wallet/helper.rs
Create BlockchainIdentities special-purpose accounts (subfeatures 0–3) in default account creation paths and expose their extended public keys via existing routing.
Transaction Checking & Routing
key-wallet/src/transaction_checking/account_checker.rs, key-wallet/src/transaction_checking/transaction_router/mod.rs, key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
Added CoreAccountTypeMatch and AccountTypeToCheck variants for the four BlockchainIdentities types; included them in Standard routing and transaction-match flows; updated tests to reflect detection of identity-path payments via the new identity hash160 variant.
Integration Tests
key-wallet-manager/tests/integration_test.rs
Updated expected default account count from 12 to 16 to include the new BlockchainIdentities accounts.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant WalletManager
    participant AccountCollection
    participant ManagedAccounts
    participant TransactionChecker
    participant FFI

    Client->>WalletManager: create wallet (Default)
    WalletManager->>ManagedAccounts: create special-purpose accounts (incl. BlockchainIdentities 0..3)
    ManagedAccounts->>AccountCollection: insert ManagedAccountType::BlockchainIdentities
    WalletManager->>FFI: expose accounts & types (FFIAccountType 16..19)
    note right of FFI: FFI maps ↔ internal AccountType
    TransactionChecker->>AccountCollection: get monitored_addresses / check transaction
    AccountCollection-->>TransactionChecker: return BlockchainIdentities addresses
    TransactionChecker->>FFI: report matches via FFI interfaces
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰
Four little hops — ECDSA, BLS, and two hash'd too,
Paths hardened and tidy at m/9'/coin/5'/subfeature', woo!
SPV's bloom now catches each identity seed,
Addresses awake, transactions take heed.
A rabbit's cheer for accounts now seen anew.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(key-wallet): add BlockchainIdentities account type' clearly and concisely summarizes the main change - adding a new BlockchainIdentities account type variant to the key-wallet crate.
Linked Issues check ✅ Passed The PR successfully implements all coding requirements from issue #553: adds four BlockchainIdentities account type variants with correct derivation paths, creates them by default, includes them in transaction routing, and verifies with unit tests.
Out of Scope Changes check ✅ Passed All changes are within scope of adding BlockchainIdentities account support; the integration test update reflects the new account count, test expectation updates align with the new variants being created by default.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/blockchain-identities-account-type
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 across AccountType, ManagedAccountType, AccountTypeToCheck, CoreAccountTypeMatch, AccountCollection, and ManagedAccountCollection, 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 BlockchainIdentities accounts for TransactionType::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 <[email protected]>
@lklimek lklimek force-pushed the feat/blockchain-identities-account-type branch from 0025841 to bb8b5e4 Compare March 16, 2026 15:24
coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 16, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and AccountTypeToCheck variants across key-wallet, with corresponding AccountCollection and ManagedAccountCollection fields
  • Included the new account types in WalletAccountCreationOptions::Default creation and TransactionType::Standard routing
  • Extended key-wallet-ffi with FFIAccountType values 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 <[email protected]>
@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 46.55172% with 248 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.88%. Comparing base (4ada2b8) to head (80b3bd3).

Files with missing lines Patch % Lines
key-wallet-ffi/src/transaction_checking.rs 0.00% 56 Missing ⚠️
key-wallet-ffi/src/address_pool.rs 0.00% 36 Missing ⚠️
...wallet/src/transaction_checking/account_checker.rs 45.31% 35 Missing ⚠️
...wallet/src/managed_account/managed_account_type.rs 14.28% 30 Missing ⚠️
.../src/managed_account/managed_account_collection.rs 76.08% 22 Missing ⚠️
key-wallet-ffi/src/managed_account.rs 0.00% 16 Missing ⚠️
key-wallet/src/managed_account/mod.rs 0.00% 16 Missing ⚠️
key-wallet/src/account/account_collection.rs 80.00% 12 Missing ⚠️
key-wallet-ffi/src/types.rs 0.00% 8 Missing ⚠️
...src/transaction_checking/transaction_router/mod.rs 33.33% 8 Missing ⚠️
... and 2 more
Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #554      +/-   ##
=============================================
- Coverage      65.95%   65.88%   -0.08%     
=============================================
  Files            311      311              
  Lines          64586    65050     +464     
=============================================
+ Hits           42599    42859     +260     
- Misses         21987    22191     +204     
Flag Coverage Δ
core 75.02% <ø> (ø)
ffi 37.01% <0.00%> (-0.13%) ⬇️
rpc 19.92% <ø> (ø)
spv 81.03% <ø> (+0.01%) ⬆️
wallet 65.67% <62.06%> (-0.01%) ⬇️
Files with missing lines Coverage Δ
key-wallet/src/account/mod.rs 66.76% <100.00%> (+3.97%) ⬆️
key-wallet/src/wallet/helper.rs 42.59% <73.33%> (+0.96%) ⬆️
key-wallet/src/account/account_type.rs 55.72% <72.22%> (+5.99%) ⬆️
key-wallet-ffi/src/types.rs 58.42% <0.00%> (-1.26%) ⬇️
...src/transaction_checking/transaction_router/mod.rs 83.96% <33.33%> (-5.11%) ⬇️
key-wallet/src/account/account_collection.rs 60.17% <80.00%> (+2.99%) ⬆️
key-wallet-ffi/src/managed_account.rs 44.98% <0.00%> (+0.17%) ⬆️
key-wallet/src/managed_account/mod.rs 45.16% <0.00%> (-1.42%) ⬇️
.../src/managed_account/managed_account_collection.rs 58.07% <76.08%> (+2.40%) ⬆️
...wallet/src/managed_account/managed_account_type.rs 21.28% <14.28%> (-0.67%) ⬇️
... and 3 more

... and 4 files with indirect coverage changes

@lklimek lklimek requested a review from xdustinface March 16, 2026 16:57
@lklimek lklimek marked this pull request as ready for review March 16, 2026 16:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-pool addresses) 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/5 directly, 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 replacing unreachable!() 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4ada2b8 and 620c8d2.

📒 Files selected for processing (16)
  • key-wallet-ffi/include/key_wallet_ffi.h
  • key-wallet-ffi/src/address_pool.rs
  • key-wallet-ffi/src/managed_account.rs
  • key-wallet-ffi/src/transaction_checking.rs
  • key-wallet-ffi/src/types.rs
  • key-wallet-manager/tests/integration_test.rs
  • key-wallet/src/account/account_collection.rs
  • key-wallet/src/account/account_type.rs
  • key-wallet/src/account/mod.rs
  • key-wallet/src/managed_account/managed_account_collection.rs
  • key-wallet/src/managed_account/managed_account_type.rs
  • key-wallet/src/managed_account/mod.rs
  • key-wallet/src/transaction_checking/account_checker.rs
  • key-wallet/src/transaction_checking/transaction_router/mod.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
  • key-wallet/src/wallet/helper.rs

Copy link

Copilot AI commented Mar 16, 2026

@lklimek I've opened a new pull request, #555, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Mar 16, 2026

@lklimek I've opened a new pull request, #556, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Mar 16, 2026

@lklimek I've opened a new pull request, #557, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 3 commits March 16, 2026 18:39
…lookup (#555)

* Initial plan

* fix: add blockchain_identities_* checks to find_address_account()

Co-authored-by: lklimek <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: lklimek <[email protected]>
…_address_used scan (#557)

* Initial plan

* fix: add blockchain_identities_* scanning to managed_wallet_mark_address_used

Co-authored-by: lklimek <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: lklimek <[email protected]>
…ariant (#556)

* Initial plan

* test: rename and narrow BlockchainIdentities test to exact ECDSA_HASH160 shared-path variant

Co-authored-by: lklimek <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: lklimek <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

📥 Commits

Reviewing files that changed from the base of the PR and between 620c8d2 and 80b3bd3.

📒 Files selected for processing (3)
  • key-wallet-ffi/src/address_pool.rs
  • key-wallet/src/transaction_checking/account_checker.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(key-wallet): Add BlockchainIdentities account type so SPV monitors identity authentication addresses

3 participants