Uh oh!
There was an error while loading. Please reload this page.
feat(key-wallet): add DIP-13 identity authentication accounts (ECDSA + BLS) - #672
Closed
QuantumExplorer wants to merge 6 commits into
Closed
feat(key-wallet): add DIP-13 identity authentication accounts (ECDSA + BLS)#672QuantumExplorer wants to merge 6 commits into
QuantumExplorer wants to merge 6 commits into
Conversation
Contributor
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds support for per-identity authentication accounts (ECDSA and BLS variants) across the key-wallet codebase. New Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
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 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/accounts.rs (2)
163-173:⚠️ Potential issue | 🔴 CriticalDerive identity-auth BLS accounts on the BLS HD tree.
Now that
IdentityAuthenticationBlsis accepted, these paths derive a secp256k1 xpriv at the BLS account path and use its 32-byte secret as a new BLS seed. That does not match DIP-13 BLS derivation from the wallet seed/master, and can make the stored BLS xpub disagree with later BLS signing derivation. Build anExtendedBLSPrivKeymaster from the wallet seed and deriveaccount_type.derivation_path(...)on that BLS tree before constructing theBLSAccount.Also applies to: 223-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/accounts.rs` around lines 163 - 173, The code currently derives a secp256k1 xpriv (via root_extended_priv_key -> HDWallet -> account_xpriv) and uses its 32-byte secret as a BLS seed, which breaks DIP-13; instead construct an ExtendedBLSPrivKey master from the wallet seed/master and perform the derivation on the BLS HD tree using account_type.derivation_path(self.network) before creating the BLS account; specifically, replace the flow that uses HDWallet/ account_xpriv/ seed for BLSAccount::from_seed with building an ExtendedBLSPrivKey master (from the wallet seed obtained where root_extended_priv_key or wallet seed is available), call its derive method with account_type.derivation_path(...), extract the BLS-derived private key/seed, and pass that into BLSAccount::from_seed (affecting the same logic used at the other block referenced around lines 223-235).
190-196:⚠️ Potential issue | 🟡 MinorUpdate the passphrase BLS account docs.
Line 195 still says the account type “must be ProviderOperatorKeys”, but the method now also accepts
IdentityAuthenticationBls.Proposed doc fix
- /// * `account_type` - The type of account (must be ProviderOperatorKeys)+ /// * `account_type` - The type of account (must be ProviderOperatorKeys+ /// or IdentityAuthenticationBls)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/accounts.rs` around lines 190 - 196, Update the function doc that begins "Add a new BLS account to a wallet that requires a passphrase" to reflect that the accepted account_type is no longer only ProviderOperatorKeys but also IdentityAuthenticationBls; change the sentence that currently reads "must be ProviderOperatorKeys" to mention both ProviderOperatorKeys and IdentityAuthenticationBls (or "ProviderOperatorKeys or IdentityAuthenticationBls") so the documentation matches the updated parameter validation in the implementation.key-wallet-ffi/src/managed_account.rs (1)
546-580:⚠️ Potential issue | 🟠 MajorPreserve
identity_indexinindex_out.The getter now returns
IdentityAuthenticationEcdsa/Bls, butindex_outis populated viaaccount_type_rust.index().unwrap_or(0). IfAccountType::index()returnsNonefor these identity-scoped variants, FFI callers will seeindex_out = 0for every identity-auth account and cannot round-trip or fetch nonzeroidentity_indexaccounts correctly.Proposed fix
- // Set the index if output pointer is provided- if !index_out.is_null() {- *index_out = account_type_rust.index().unwrap_or(0);- }+ // Set the primary FFI index if output pointer is provided.+ if !index_out.is_null() {+ *index_out = match &account_type_rust {+ AccountType::IdentityAuthenticationEcdsa {+ identity_index,+ } => *identity_index,+ #[cfg(feature = "bls")]+ AccountType::IdentityAuthenticationBls {+ identity_index,+ } => *identity_index,+ _ => account_type_rust.index().unwrap_or(0),+ };+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 546 - 580, The code currently sets index_out from account_type_rust.index().unwrap_or(0), which loses the identity_index for identity-auth variants; update the logic so index_out is populated from the identity index for identity-scoped variants (e.g., AccountType::IdentityAuthenticationEcdsa and AccountType::IdentityAuthenticationBls) and falls back to account_type_rust.index().unwrap_or(0) for others—i.e., compute an index_val by matching account_type_rust (extracting the identity_index field for IdentityAuthenticationEcdsa/Bls and using index() for other variants) and then write *index_out = index_val when index_out is not null.
🤖 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 48-54: The global scan in managed_wallet_mark_address_used is
missing the identity-authentication maps (identity_authentication_ecdsa and,
when feature "bls" enabled, identity_authentication_bls), so FFI callers get
"Address not found"; update the scanning logic inside
managed_wallet_mark_address_used to include lookups into
collection.identity_authentication_ecdsa.get(identity_index) and
collection.identity_authentication_bls.get(identity_index) (guarded by
cfg(feature = "bls")) wherever other AccountType branches are checked (also
mirror the same additions in the other scan blocks referenced around the ranges
mentioned) so identity-auth addresses are found and their used/gap state is
updated.
In `@key-wallet-ffi/src/types.rs`:
- Around line 295-304: The to_account_type() function currently panics for
unsupported FFIAccountType variants (IdentityAuthenticationBls,
DashpayReceivingFunds, DashpayExternalAccount, PlatformPayment); change its
signature to return Result<key_wallet::AccountType, String> (or a suitable error
type) and replace panics with Err(...) carrying a clear message; then update all
call sites in address_pool.rs, account.rs, wallet.rs, and managed_account.rs to
propagate the error into their existing FFI error plumbing (FFIError,
FFIAccountResult, FFIManagedCoreAccountResult) instead of assuming success so
FFI callers receive an error rather than triggering a panic.
In `@key-wallet/src/managed_account/managed_account_type.rs`:
- Around line 665-701: The code for AccountType::IdentityAuthenticationEcdsa and
AccountType::IdentityAuthenticationBls currently falls back to
DerivationPath::master() on derivation_path(network) errors; change this to
propagate the error instead (e.g., use let path =
account_type.derivation_path(network)? ) so failures are returned rather than
silently using the master path, and keep the AddressPool::new(...) usage
unchanged; ensure the surrounding function's Result error type aligns with the
thiserror-based error enum so the ? operator compiles.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 860-876: The BLS branch
(ManagedAccountType::IdentityAuthenticationBls) is incorrectly treating an
Option<&ExtendedPubKey> (account_xpub) as an ECDSA public source; change the
logic so IdentityAuthenticationBls only accepts a BLS public key type or else
returns NoKeySource: locate the match in
ManagedAccountType::IdentityAuthenticationBls that currently maps Some(xpub) =>
address_pool::KeySource::Public(*xpub) and instead detect/accept an
ExtendedBLSPubKey (or map to address_pool::KeySource::BLSPublic) and when only
an ECDSA ExtendedPubKey is present, use address_pool::KeySource::NoKeySource (or
return the appropriate error) before calling addresses.next_unused(...,
add_to_state).
In `@key-wallet/src/wallet/helper.rs`:
- Around line 863-873: The match arm for
AccountTypeToCheck::IdentityAuthenticationEcdsa currently returns None even when
an identity index is supplied; update the logic in the helper that looks up
xpubs to use the provided account_index to fetch from
wallet.accounts.identity_authentication_ecdsa (e.g., call
identity_authentication_ecdsa.get(&account_index) or unwrap the
Option/account_index appropriately) and return the found xpub (Some(xpub))
instead of None; leave IdentityAuthenticationBls as None since BLS has no
standard xpub.
---
Outside diff comments:
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 546-580: The code currently sets index_out from
account_type_rust.index().unwrap_or(0), which loses the identity_index for
identity-auth variants; update the logic so index_out is populated from the
identity index for identity-scoped variants (e.g.,
AccountType::IdentityAuthenticationEcdsa and
AccountType::IdentityAuthenticationBls) and falls back to
account_type_rust.index().unwrap_or(0) for others—i.e., compute an index_val by
matching account_type_rust (extracting the identity_index field for
IdentityAuthenticationEcdsa/Bls and using index() for other variants) and then
write *index_out = index_val when index_out is not null.
In `@key-wallet/src/wallet/accounts.rs`:
- Around line 163-173: The code currently derives a secp256k1 xpriv (via
root_extended_priv_key -> HDWallet -> account_xpriv) and uses its 32-byte secret
as a BLS seed, which breaks DIP-13; instead construct an ExtendedBLSPrivKey
master from the wallet seed/master and perform the derivation on the BLS HD tree
using account_type.derivation_path(self.network) before creating the BLS
account; specifically, replace the flow that uses HDWallet/ account_xpriv/ seed
for BLSAccount::from_seed with building an ExtendedBLSPrivKey master (from the
wallet seed obtained where root_extended_priv_key or wallet seed is available),
call its derive method with account_type.derivation_path(...), extract the
BLS-derived private key/seed, and pass that into BLSAccount::from_seed
(affecting the same logic used at the other block referenced around lines
223-235).
- Around line 190-196: Update the function doc that begins "Add a new BLS
account to a wallet that requires a passphrase" to reflect that the accepted
account_type is no longer only ProviderOperatorKeys but also
IdentityAuthenticationBls; change the sentence that currently reads "must be
ProviderOperatorKeys" to mention both ProviderOperatorKeys and
IdentityAuthenticationBls (or "ProviderOperatorKeys or
IdentityAuthenticationBls") so the documentation matches the updated parameter
validation in the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64357549-e7f9-4794-a251-d9fc392142b0
📒 Files selected for processing (17)
key-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/account/account_collection_test.rskey-wallet/src/account/account_type.rskey-wallet/src/account/bls_account.rskey-wallet/src/account/mod.rskey-wallet/src/dip9.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/wallet/accounts.rskey-wallet/src/wallet/helper.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
QuantumExplorerforce-pushed
the
chore/account-collection-audit
branch
from
April 21, 2026 21:51
2140de6 to
b6ebb8bCompare8 tasks
QuantumExplorerforce-pushed
the
chore/account-collection-audit
branch
from
April 22, 2026 04:05
b6ebb8b to
ef345d1CompareQuantumExplorer 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
commented
Apr 22, 2026
Collaborator
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/accounts.rs (1)
190-213:⚠️ Potential issue | 🟡 MinorUpdate the passphrase BLS account docs.
Line 195 still says the account type must be
ProviderOperatorKeys, but the function now also acceptsIdentityAuthenticationBls.📝 Proposed doc fix
- /// * `account_type` - The type of account (must be ProviderOperatorKeys)+ /// * `account_type` - The type of account (must be ProviderOperatorKeys+ /// or IdentityAuthenticationBls)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/accounts.rs` around lines 190 - 213, Update the doc comment for add_bls_account_with_passphrase to reflect that the function accepts both AccountType::ProviderOperatorKeys and AccountType::IdentityAuthenticationBls (not just ProviderOperatorKeys); change the prose in the summary and the `# Arguments` section to list both allowed account types and remove the outdated single-type statement so the docs match the validation that checks AccountType::ProviderOperatorKeys | AccountType::IdentityAuthenticationBls.key-wallet-ffi/src/address_pool.rs (1)
542-585:⚠️ Potential issue | 🟠 MajorAvoid routing BLS identity-auth generation through an ECDSA xpub.
For
IdentityAuthenticationBls, this path either fails attry_into()because identity-auth accounts are not Core transaction-checking types, or proceeds to buildKeySource::Public(xpub), which is an ECDSA source for a BLS pool. Add a BLS-aware path usingKeySource::BLSPublic, or return a targeted unsupported error before this xpub lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/address_pool.rs` around lines 542 - 585, The current flow converts account_type via account_type.to_account_type(...) and then try_into() to a Core account type, and always builds KeySource::Public(xpub) from wallet.extended_public_key_for_account_type(...), which incorrectly routes BLS identity-auth through an ECDSA xpub; update the branch around account_type_rust and account_type_to_check to detect identity-auth BLS accounts (e.g. when account_type_rust corresponds to IdentityAuthenticationBls) and either (a) obtain the correct BLS public key and construct KeySource::BLSPublic(...) instead of KeySource::Public(...), or (b) return a clear FFIError::set_error(..., FFIErrorCode::InvalidInput, "...unsupported for BLS identity-auth") before calling extended_public_key_for_account_type; ensure you reference and adjust the logic using account_type.to_account_type, account_type_rust.try_into, extended_public_key_for_account_type, and KeySource::BLSPublic/KeySource::Public so BLS accounts are not treated as ECDSA.key-wallet-ffi/src/managed_account.rs (1)
558-591:⚠️ Potential issue | 🟠 MajorReturn
identity_indexthroughindex_outfor identity-auth accounts.
AccountType::index()intentionally returnsNonefor these variants, so the current code writes0for every identity-auth account. FFI callers then cannot tell which identity index was returned.Proposed fix
// Set the index if output pointer is provided if !index_out.is_null() { - *index_out = account_type_rust.index().unwrap_or(0);+ *index_out = match account_type_rust {+ AccountType::IdentityAuthenticationEcdsa {+ identity_index,+ }+ | AccountType::IdentityAuthenticationBls {+ identity_index,+ } => identity_index,+ _ => account_type_rust.index().unwrap_or(0),+ }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 558 - 591, The code currently uses account_type_rust.index().unwrap_or(0) which returns None for identity-auth variants, causing index_out to be set to 0; update the logic to extract and return the actual identity index for identity-auth variants (AccountType::IdentityAuthenticationEcdsa and AccountType::IdentityAuthenticationBls) by matching those variants, reading their identity index field (e.g., identity_index or the variant's index field), and writing that value to *index_out when index_out is non-null before converting to the FFIAccountType; keep the existing fallback to account_type_rust.index() for other variants.
🤖 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/managed_account.rs`:
- Around line 221-233: The match arm handling
account_type.to_account_type(account_index) returns early on error and leaks
managed_wallet_ptr; before returning FFIManagedCoreAccountResult::error, call
the same cleanup used later for the managed wallet handle (free or drop the
managed_wallet_ptr/managed_wallet_handle) so the managed wallet info is
released—modify the Err branch in the to_account_type match to free
managed_wallet_ptr (or invoke the existing cleanup helper) then return the error
result.
---
Outside diff comments:
In `@key-wallet-ffi/src/address_pool.rs`:
- Around line 542-585: The current flow converts account_type via
account_type.to_account_type(...) and then try_into() to a Core account type,
and always builds KeySource::Public(xpub) from
wallet.extended_public_key_for_account_type(...), which incorrectly routes BLS
identity-auth through an ECDSA xpub; update the branch around account_type_rust
and account_type_to_check to detect identity-auth BLS accounts (e.g. when
account_type_rust corresponds to IdentityAuthenticationBls) and either (a)
obtain the correct BLS public key and construct KeySource::BLSPublic(...)
instead of KeySource::Public(...), or (b) return a clear
FFIError::set_error(..., FFIErrorCode::InvalidInput, "...unsupported for BLS
identity-auth") before calling extended_public_key_for_account_type; ensure you
reference and adjust the logic using account_type.to_account_type,
account_type_rust.try_into, extended_public_key_for_account_type, and
KeySource::BLSPublic/KeySource::Public so BLS accounts are not treated as ECDSA.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 558-591: The code currently uses
account_type_rust.index().unwrap_or(0) which returns None for identity-auth
variants, causing index_out to be set to 0; update the logic to extract and
return the actual identity index for identity-auth variants
(AccountType::IdentityAuthenticationEcdsa and
AccountType::IdentityAuthenticationBls) by matching those variants, reading
their identity index field (e.g., identity_index or the variant's index field),
and writing that value to *index_out when index_out is non-null before
converting to the FFIAccountType; keep the existing fallback to
account_type_rust.index() for other variants.
In `@key-wallet/src/wallet/accounts.rs`:
- Around line 190-213: Update the doc comment for
add_bls_account_with_passphrase to reflect that the function accepts both
AccountType::ProviderOperatorKeys and AccountType::IdentityAuthenticationBls
(not just ProviderOperatorKeys); change the prose in the summary and the `#
Arguments` section to list both allowed account types and remove the outdated
single-type statement so the docs match the validation that checks
AccountType::ProviderOperatorKeys | AccountType::IdentityAuthenticationBls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 863ba828-f6cb-47f7-954d-20bf12880600
📒 Files selected for processing (17)
key-wallet-ffi/src/account.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet/src/account/account_collection.rskey-wallet/src/account/account_collection_test.rskey-wallet/src/account/account_type.rskey-wallet/src/account/bls_account.rskey-wallet/src/account/mod.rskey-wallet/src/dip9.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/wallet/accounts.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- key-wallet/src/account/account_collection_test.rs
- key-wallet/src/account/mod.rs
- key-wallet/src/account/bls_account.rs
- key-wallet/src/transaction_checking/transaction_router/mod.rs
Uh oh!
There was an error while loading. Please reload this page.
…+ BLS)
Add two new `AccountType` variants for DIP-13 sub-feature 0' (per-identity
signing keys the user employs to sign Dash Platform state transitions):
- `IdentityAuthenticationEcdsa { identity_index }` — key_type 0',
backed by a regular `Account` (secp256k1).
- `IdentityAuthenticationBls { identity_index }` — key_type 1',
backed by `BLSAccount`, gated on `#[cfg(feature = "bls")]`.
Both account types use the DIP-13 derivation path
`m/9'/coin_type'/5'/0'/key_type'/identity_index'` with hardened children
for individual keys (`.../identity_index'/key_index'`). Address pools use
`AbsentHardened` since DIP-13 mandates hardened leaves.
### Wiring
- `AccountCollection` gains `identity_authentication_ecdsa:
BTreeMap<u32, Account>` and (under `bls`) `identity_authentication_bls:
BTreeMap<u32, BLSAccount>`, keyed by `identity_index`. All collection
methods (`new`, `insert`, `insert_bls_account`, `contains_account_type`,
`account_of_type[_mut]`, `bls_account_of_type[_mut]`, `all_accounts[_mut]`,
`count`, `is_empty`, `clear`) are updated.
- `ManagedAccountCollection`, `ManagedAccountType`, `CoreAccountTypeMatch`
mirror the new variants and are routed through the usual matchers.
- `AccountTypeToCheck::IdentityAuthentication{Ecdsa,Bls}` variants are
added so conversions from `ManagedAccountType`/`AccountType` stay
total. Identity authentication accounts are **Platform-only**: they are
deliberately absent from every `TransactionType` relevance set
(`TransactionRouter::get_relevant_account_types`), and the
`ManagedAccountCollection::check_account_type` arms return empty
results. Address matching in `ManagedCoreAccount::check_transaction_for_match`
returns `None` for these variants for the same reason.
- `Wallet::add_bls_account` now accepts `IdentityAuthenticationBls` in
addition to `ProviderOperatorKeys`.
- Two new DIP-9 `IndexConstPath<5>` constants per network
(`IDENTITY_AUTHENTICATION_{ECDSA,BLS}_PATH_{MAINNET,TESTNET}`) and the
matching `DerivationPathReference::BlockchainIdentityAuthentication{Ecdsa,Bls}`
variants.
- `asset_lock_builder::resolve_funding_account` is intentionally left
untouched — identity authentication accounts do not fund asset locks.
- `WalletAccountCreationOptions` is unchanged. Identity authentication
accounts are per-identity and come into existence when the user
registers a Platform identity, not at wallet creation. Callers insert
them post-hoc via `Wallet::add_account` (ECDSA) or
`Wallet::add_bls_account` (BLS).
### FFI
`FFIAccountType` gains `IdentityAuthenticationEcdsa = 16` and
`IdentityAuthenticationBls = 17`; `to_account_type` / `from_account_type`
route the `index` parameter as `identity_index`. `FFIAccountMatch`
emission for `CoreAccountTypeMatch::IdentityAuthentication*` reports the
identity index in `account_index` (these variants are never produced by
the L1 transaction router, but the FFI matcher stays exhaustive).
### Tests
New `identity_authentication_tests` module in `account_type.rs` covers:
ECDSA and BLS mainnet/testnet/regtest path derivation, `index()` /
`derivation_path_reference()` / `AccountTypeToCheck` round-trip, and
end-to-end insert / `contains_account_type` / `account_of_type` /
`bls_account_of_type` round-trips through `AccountCollection`. BLS tests
are `#[cfg(feature = "bls")]`-gated. Existing
`test_wrong_account_type_for_bls` message was updated for the broadened
`insert_bls_account` validation.
### Serialization compatibility
Adding enum variants is forward-incompatible for `bincode::Encode`/
`Decode` — wallet blobs serialized by earlier v0.42-dev builds will fail
to decode after this change. This is acceptable given the unstable 0.x
API per `CLAUDE.md`. Serde uses its default (externally tagged)
representation, so new readers still decode old data identically and old
readers will error cleanly on new variants they cannot name.
Verified: `cargo build -p key-wallet --all-features`,
`cargo test -p key-wallet --lib --all-features`,
`cargo clippy -p key-wallet --all-features --all-targets -- -D warnings`,
`cargo fmt -p key-wallet --check`, and downstream `key-wallet-ffi` /
`key-wallet-manager` builds and lib tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>QuantumExplorerforce-pushed
the
chore/account-collection-audit
branch
from
April 23, 2026 03:45
ef345d1 to
f032d8fCompareContributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
key-wallet/src/wallet/accounts.rs (1)
190-213:⚠️ Potential issue | 🟡 MinorUpdate the passphrase BLS-account docs.
Line 195 still says the account type “must be ProviderOperatorKeys,” but the implementation now also accepts
IdentityAuthenticationBls.Proposed doc fix
- /// * `account_type` - The type of account (must be ProviderOperatorKeys)+ /// * `account_type` - The type of account (must be ProviderOperatorKeys+ /// or IdentityAuthenticationBls)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/accounts.rs` around lines 190 - 213, The doc comment for add_bls_account_with_passphrase is outdated: update the summary and argument description to reflect that the function accepts both AccountType::ProviderOperatorKeys and AccountType::IdentityAuthenticationBls (not just ProviderOperatorKeys). Edit the top comment and the `# Arguments` bullet for `account_type` to list both allowed variants and a short note that either ProviderOperatorKeys or IdentityAuthenticationBls is accepted.key-wallet-ffi/src/address_pool.rs (1)
542-567:⚠️ Potential issue | 🟠 MajorDo not route identity-auth pool generation through Core account checking.
The new identity-auth variants are Platform-only, so
account_type_rust.try_into()fails here before the managed account/pool is reached. As a result, FFI can query/set these pools but cannot generate them through this API, and the error incorrectly blames Platform Payment accounts. Branch on identity-auth before this conversion and either use the hardened/path/BLS-aware generation path or return an explicit unsupported error for identity-auth accounts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/address_pool.rs` around lines 542 - 567, The code currently converts account_type_rust via account_type_rust.try_into() which fails for identity-auth (Platform-only) and prevents pool generation; modify the logic in the function handling account pool ops so you first branch on the identity-auth variant returned by account_type.to_account_type(account_index) (inspect account_type_rust) and either route to the identity-auth/hardened/path/BLS-aware generation flow or return a clear unsupported error for identity-auth accounts using FFIError::set_error (do not reach the try_into path), otherwise continue with the existing try_into branch for managed accounts; update error text to accurately reflect identity-auth unsupported status when appropriate.key-wallet-ffi/src/account.rs (2)
91-112:⚠️ Potential issue | 🟠 MajorHandle BLS account retrieval separately.
IdentityAuthenticationBlsnow reaches this function, butaccount_of_typeis the plain/ECDSA accessor and is expected to returnNonefor BLS accounts. Add a BLS-specific getter/result or reject this variant with a clear “use BLS account getter” error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account.rs` around lines 91 - 112, The code currently calls account_of_type for all variants, but IdentityAuthenticationBls should not be returned by the plain/ECDSA accessor; update the match handling after account_type.to_account_type(account_index) to either (a) add a branch that detects the IdentityAuthenticationBls variant (from the resulting AccountType) and call a new BLS-specific getter (e.g., wallet.inner().accounts.account_of_bls or similar) and return an FFIAccountResult::success with FFIAccount::new(...) when found, or (b) if no BLS getter exists, return a clear error via FFIAccountResult::error(FFIErrorCode::InvalidArgument, "Use BLS account getter" or similar) so BLS is not silently treated as NotFound; modify the code paths around account_of_type, FFIAccountResult, and the error message to reflect this change.
575-583:⚠️ Potential issue | 🟠 MajorCount the new identity-auth accounts in the FFI account count.
This manual total omits
identity_authentication_ecdsaand the cfg-gated BLS identity-auth map, so FFI callers will under-report wallets after adding the new account types. Prefer delegating to the collection’s central count helper so future account types are not missed.Proposed simplification
let wallet = &*wallet; let accounts = &wallet.inner().accounts; FFIError::set_success(error); - let count = accounts.standard_bip44_accounts.len()- + accounts.standard_bip32_accounts.len()- + accounts.coinjoin_accounts.len()- + accounts.identity_registration.is_some() as usize- + accounts.identity_topup.len();- count as c_uint+ accounts.count() as c_uint }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account.rs` around lines 575 - 583, The manual sum using accounts.standard_bip44_accounts, standard_bip32_accounts, coinjoin_accounts, identity_registration, and identity_topup misses the new identity_authentication_ecdsa and the cfg-gated BLS map; replace the handcrafted total with the collection's central count helper on accounts (i.e., call the accounts' provided total/count method instead of summing individual maps) so all current and future account types are included (locate the helper on the same Accounts type/struct that owns standard_bip44_accounts, identity_authentication_ecdsa, identity_topup, etc., and use it in place of the manual addition).key-wallet-ffi/src/wallet.rs (1)
489-533:⚠️ Potential issue | 🟠 MajorReject or route
IdentityAuthenticationBlsbefore the ECDSA add path.
FFIAccountType::IdentityAuthenticationBlsnow converts successfully, so these generic add-account entrypoints pass it toWallet::add_account, which builds a regularAccountand then fails because BLS auth accounts requireWallet::add_bls_account/insert_bls_account. Add an early BLS-specific error or route to a BLS FFI creation function.Early-reject pattern to mirror in all three generic add functions
FFIAccountType::DashpayExternalAccount => { return crate::types::FFIAccountResult::error( FFIErrorCode::InvalidInput, "DashpayExternalAccount accounts require identity IDs. \ Use wallet_add_dashpay_external_account_with_xpub_bytes() instead." .to_string(), ); } + FFIAccountType::IdentityAuthenticationBls => {+ return crate::types::FFIAccountResult::error(+ FFIErrorCode::InvalidInput,+ "IdentityAuthenticationBls accounts require BLS account creation. \+ Use the BLS account creation FFI API instead."+ .to_string(),+ );+ } _ => {} // Other types are supported }Also applies to: 729-775, 865-911
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet.rs` around lines 489 - 533, The generic add-account entrypoints currently allow FFIAccountType::IdentityAuthenticationBls to pass through to Wallet::add_account, which later fails because BLS-auth accounts must be created via Wallet::add_bls_account / insert_bls_account; update the early account-type check (the match on account_type in wallet.rs handling PlatformPayment/Dashpay* ) to also either (a) return a clear FFIAccountResult::error for FFIAccountType::IdentityAuthenticationBls instructing callers to use the BLS-specific FFI function, or (b) route the call to the existing BLS creation path by invoking the appropriate FFI creation helper that calls Wallet::add_bls_account/insert_bls_account; ensure you reference FFIAccountType::IdentityAuthenticationBls, Wallet::add_account, Wallet::add_bls_account, and insert_bls_account when implementing the change so the behavior is consistent across the three generic add functions mentioned.
🤖 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/src/managed_account/mod.rs`:
- Around line 857-875: The IdentityAuthenticationBls branch rejects ECDSA xpubs
but the BLS generation path (next_bls_operator_key) only accepts
ProviderOperatorKeys so identity-auth BLS pools cannot be consumed; modify the
BLS handling to accept IdentityAuthenticationBls pools by either updating
next_bls_operator_key to also support
ManagedAccountType::IdentityAuthenticationBls (check addresses.next_unused with
address_pool::KeySource::NoKeySource and pass add_to_state) or add a dedicated
wrapper function next_bls_identity_authentication_key that delegates to the same
BLS derivation logic used by next_bls_operator_key so addresses in
IdentityAuthenticationBls can be produced through this API.
---
Outside diff comments:
In `@key-wallet-ffi/src/account.rs`:
- Around line 91-112: The code currently calls account_of_type for all variants,
but IdentityAuthenticationBls should not be returned by the plain/ECDSA
accessor; update the match handling after
account_type.to_account_type(account_index) to either (a) add a branch that
detects the IdentityAuthenticationBls variant (from the resulting AccountType)
and call a new BLS-specific getter (e.g., wallet.inner().accounts.account_of_bls
or similar) and return an FFIAccountResult::success with FFIAccount::new(...)
when found, or (b) if no BLS getter exists, return a clear error via
FFIAccountResult::error(FFIErrorCode::InvalidArgument, "Use BLS account getter"
or similar) so BLS is not silently treated as NotFound; modify the code paths
around account_of_type, FFIAccountResult, and the error message to reflect this
change.
- Around line 575-583: The manual sum using accounts.standard_bip44_accounts,
standard_bip32_accounts, coinjoin_accounts, identity_registration, and
identity_topup misses the new identity_authentication_ecdsa and the cfg-gated
BLS map; replace the handcrafted total with the collection's central count
helper on accounts (i.e., call the accounts' provided total/count method instead
of summing individual maps) so all current and future account types are included
(locate the helper on the same Accounts type/struct that owns
standard_bip44_accounts, identity_authentication_ecdsa, identity_topup, etc.,
and use it in place of the manual addition).
In `@key-wallet-ffi/src/address_pool.rs`:
- Around line 542-567: The code currently converts account_type_rust via
account_type_rust.try_into() which fails for identity-auth (Platform-only) and
prevents pool generation; modify the logic in the function handling account pool
ops so you first branch on the identity-auth variant returned by
account_type.to_account_type(account_index) (inspect account_type_rust) and
either route to the identity-auth/hardened/path/BLS-aware generation flow or
return a clear unsupported error for identity-auth accounts using
FFIError::set_error (do not reach the try_into path), otherwise continue with
the existing try_into branch for managed accounts; update error text to
accurately reflect identity-auth unsupported status when appropriate.
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 489-533: The generic add-account entrypoints currently allow
FFIAccountType::IdentityAuthenticationBls to pass through to
Wallet::add_account, which later fails because BLS-auth accounts must be created
via Wallet::add_bls_account / insert_bls_account; update the early account-type
check (the match on account_type in wallet.rs handling PlatformPayment/Dashpay*
) to also either (a) return a clear FFIAccountResult::error for
FFIAccountType::IdentityAuthenticationBls instructing callers to use the
BLS-specific FFI function, or (b) route the call to the existing BLS creation
path by invoking the appropriate FFI creation helper that calls
Wallet::add_bls_account/insert_bls_account; ensure you reference
FFIAccountType::IdentityAuthenticationBls, Wallet::add_account,
Wallet::add_bls_account, and insert_bls_account when implementing the change so
the behavior is consistent across the three generic add functions mentioned.
In `@key-wallet/src/wallet/accounts.rs`:
- Around line 190-213: The doc comment for add_bls_account_with_passphrase is
outdated: update the summary and argument description to reflect that the
function accepts both AccountType::ProviderOperatorKeys and
AccountType::IdentityAuthenticationBls (not just ProviderOperatorKeys). Edit the
top comment and the `# Arguments` bullet for `account_type` to list both allowed
variants and a short note that either ProviderOperatorKeys or
IdentityAuthenticationBls is accepted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db3dbeac-183c-444a-8a44-32240c4eca0a
📒 Files selected for processing (17)
key-wallet-ffi/src/account.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet/src/account/account_collection.rskey-wallet/src/account/account_collection_test.rskey-wallet/src/account/account_type.rskey-wallet/src/account/bls_account.rskey-wallet/src/account/mod.rskey-wallet/src/dip9.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/wallet/accounts.rs
✅ Files skipped from review due to trivial changes (5)
- key-wallet/src/account/account_collection_test.rs
- key-wallet/src/account/mod.rs
- key-wallet/src/dip9.rs
- key-wallet-ffi/src/types.rs
- key-wallet/src/account/account_collection.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- key-wallet/src/account/bls_account.rs
- key-wallet/src/transaction_checking/account_checker.rs
- key-wallet/src/transaction_checking/transaction_router/mod.rs
- key-wallet/src/managed_account/managed_account_collection.rs
Uh oh!
There was an error while loading. Please reload this page.
`next_bls_operator_key` only matched `ProviderOperatorKeys`, so the DIP-13 identity-authentication BLS pool had no API to derive fresh keys (the ECDSA `account_xpub` taken by `next_address` is unusable for BLS). Extract the shared pool-derivation logic and add `next_bls_identity_authentication_key` so `IdentityAuthenticationBls` can be consumed through the same path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#670 removed `FFIError::error`, `FFIError::set_error`, `FFIError::success`, and `FFIError::free_message` in favour of `FFIError::set`, `FFIError::clean`, and a `Drop` impl. The DIP-13 identity-authentication account additions in this PR still called the removed helpers, breaking `key-wallet-ffi` after merging the latest v0.42-dev. Replace each call site with the new API: - `FFIError::set_error(err, code, msg)` -> `(*err).set(code, &msg)` - `FFIError::error(code, msg)` -> struct literal; caller-owned, freed by Drop - `e.free_message()` -> let `FFIError`'s Drop clean up Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer added a commit
that referenced
this pull request
Apr 23, 2026
Brings in PR #678 (hardcoded masternode seed files + weekly auto-refresh, new dash-network-seeds crate, masternode-seeds-fetcher P2P refactor) so feat/platform-wallet2 has the embedded seed infrastructure available while #678 reviews. Conflict resolution: - dash-spv/src/network/constants.rs: kept both TESTNET_FIXED_PEERS (from PR #658 already on pw2) and new MAINNET_P2P_PORT / TESTNET_P2P_PORT constants from #678. - dash-spv/src/network/discovery.rs: kept both the testnet fixed-peers fallback loop and the richer embedded-seeds-count log message from #678. - key-wallet-ffi/src/error.rs: incoming #678 rewrote FFIError's impl with set()/clean() methods; preserved the HEAD-side associated functions (success/error/set_error/set_success/free_message) that PR #672 callers depend on, alongside the new methods. Deleted duplicate From<key_wallet::Error> and From<key_wallet_manager::WalletError> impls in favor of #678's newer versions, and extended the latter's match to cover pw2's ApplyChangeSet and Persistence variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n plumbing
`ManagedCoreAccount` is designed around L1 state (balance/transactions/
utxos/spent_outpoints/monitor_revision), none of which applies to DIP-13
identity authentication keys — they are Platform-only signing keys that
never hold L1 UTXOs and are already excluded from every
`TransactionRouter` relevance set. The immutable `Account` in
`AccountCollection` already carries the xpub/derivation path needed to
derive signing keys, so the managed-side wrapper is dead weight.
Remove:
- `ManagedAccountCollection::identity_authentication_{ecdsa,bls}` fields
+ all wiring (`new`/`insert`/`contains`/`all_accounts*`/`is_empty`/
`clear`/`from_account_collection`)
- `ManagedAccountType::IdentityAuthentication{Ecdsa,Bls}` variants and
their match arms across `managed_account/mod.rs`,
`transaction_checking/*`, and `key-wallet-ffi`
- `next_bls_identity_authentication_key()` (became dead code)
Preserve `AccountType::IdentityAuthentication{Ecdsa,Bls}` and the
immutable-side `AccountCollection` fields, DIP-9 constants, and the FFI
`FFIAccountType = 16/17` mappings — those stay on the immutable path.
`from_account_type` / `create_managed_account_from_account_type` still
receive the immutable variants in their signatures, so those arms now
return `Err(InvalidParameter)`; the path is unreachable in practice
because `from_account_collection` no longer feeds them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-ffi/src/types.rs (1)
680-703:⚠️ Potential issue | 🟠 MajorSkip identity-auth variants in
special_account_typesinstead of forcing identity index0.This conversion path has no identity-index field, but the new variants now succeed through
to_account_type(0). A caller that includesIdentityAuthenticationEcdsa/Blsinspecial_account_typeswill silently request identity index0, which conflicts with the post-hoc identity registration flow.Proposed fix
- // Variants that cannot be represented with a single `u32` index- // (DashpayReceivingFunds, DashpayExternalAccount, PlatformPayment)- // are silently skipped here because callers must use the dedicated- // entry points (e.g. `wallet_add_dashpay_receiving_account`). This- // conversion path has no error-return channel.+ // Variants that cannot be represented by this creation-options+ // shape are silently skipped here because callers must use the+ // dedicated post-hoc entry points. This conversion path has no+ // per-entry index/error-return channel. @@ let mut accounts = Vec::new(); for &ffi_type in slice { + if matches!(+ ffi_type,+ FFIAccountType::DashpayReceivingFunds+ | FFIAccountType::DashpayExternalAccount+ | FFIAccountType::PlatformPayment+ | FFIAccountType::IdentityAuthenticationEcdsa+ | FFIAccountType::IdentityAuthenticationBls+ ) {+ continue;+ } // Errors are silently dropped — callers must use the // dedicated entry points for account types that need // more than a single u32 index, and this signature has🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/types.rs` around lines 680 - 703, In the special_accounts conversion loop (the block that reads special_account_types / special_account_types_count and calls ffi_type.to_account_type(0)), skip any identity-authentication variants instead of calling to_account_type(0) with a forced index; specifically detect and silently ignore IdentityAuthenticationEcdsa and IdentityAuthenticationBls (the new variants) before invoking to_account_type so they do not map to identity index 0 and interfere with the post‑hoc identity registration flow.
🤖 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/src/managed_account/managed_account_type.rs`:
- Around line 610-624: The new Err for
AccountType::IdentityAuthenticationEcdsa/Bls will cause a panic because
ManagedCoreAccount::from_account and ManagedCoreAccount::from_bls_account
currently retry with NoKeySource and call .expect("Should succeed with
NoKeySource"); change those public constructors to be fallible (return Result)
or early-check/special-case the IdentityAuthenticationEcdsa and
IdentityAuthenticationBls variants before attempting the NoKeySource path so you
return the existing InvalidParameter error instead of hitting the expect; update
callers or conversion points to propagate the Result using ? and remove the
.expect to avoid panics.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 976-991: The current code calls
addresses.mark_index_used(info.index) before attempting to deserialize the
retrieved BLS key, which will consume the index even if
PublicKey::<Bls12381G2Impl>::from_bytes_with_mode fails; change the flow in the
block handling addresses.next_unused_with_info(&key_source, add_to_state) and
the Some(PublicKeyType::BLS(pub_key_bytes)) match so that you first call
PublicKey::<Bls12381G2Impl>::from_bytes_with_mode(&pub_key_bytes,
SerializationFormat::Modern) and only on Ok(...) then call
addresses.mark_index_used(info.index) and return the deserialized key
(propagating the same error mapping on Err). Ensure you reference info.index,
addresses.mark_index_used, and PublicKey::from_bytes_with_mode when making the
change.
In `@key-wallet/src/transaction_checking/transaction_router/mod.rs`:
- Around line 172-176: Update the rustdoc comment around the conversion note to
clarify that DIP-13 identity-authentication accounts are not variants of
ManagedAccountType and therefore are represented on the immutable account side;
state that the managed-account conversion only rejects PlatformPayment (DIP-17)
and that conversions from ManagedAccountType / crate::AccountType return
PlatformAccountConversionError only for PlatformPayment, not for DIP-13 auth
accounts; reference the types ManagedAccountType, crate::AccountType,
PlatformPayment and the error PlatformAccountConversionError in the revised text
to make the distinction explicit.
---
Outside diff comments:
In `@key-wallet-ffi/src/types.rs`:
- Around line 680-703: In the special_accounts conversion loop (the block that
reads special_account_types / special_account_types_count and calls
ffi_type.to_account_type(0)), skip any identity-authentication variants instead
of calling to_account_type(0) with a forced index; specifically detect and
silently ignore IdentityAuthenticationEcdsa and IdentityAuthenticationBls (the
new variants) before invoking to_account_type so they do not map to identity
index 0 and interfere with the post‑hoc identity registration flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3f94098-25ca-46f4-9235-cdce75eac074
📒 Files selected for processing (10)
key-wallet-ffi/src/account.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.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/transaction_router/mod.rskey-wallet/src/wallet/accounts.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- key-wallet-ffi/src/account.rs
- key-wallet/src/wallet/accounts.rs
- key-wallet-ffi/src/wallet.rs
- key-wallet-ffi/src/address_pool.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…emoval
Three independent issues flagged on the previous commit:
1. `ManagedCoreAccount::from_account` / `from_bls_account` would panic
on DIP-13 identity authentication `AccountType` variants. The primary
path returns `Err(InvalidParameter)` (no managed representation) and
the fallback to `NoKeySource` returns the same error — which the old
`.expect("Should succeed with NoKeySource")` then turned into a
panic. Make both constructors fallible, propagate the
`InvalidParameter` early without hitting the fallback, and update
the four call sites in `managed_accounts.rs` to use `?`. DIP-13
auth accounts are reachable via `AccountCollection::account_of_type`
so this path is exercisable by callers.
2. `next_bls_key_from_pool` marked the address index used before
attempting to deserialize the BLS public key, which consumed the
index even when `PublicKey::from_bytes_with_mode` failed. Reorder
so deserialization runs first and `mark_index_used` only runs on
success.
3. `AccountTypeToCheck` rustdoc claimed both
`ManagedAccountType`→`AccountTypeToCheck` and
`AccountType`→`AccountTypeToCheck` return
`PlatformAccountConversionError` for DIP-13 variants. After the
managed-side removal only the immutable `AccountType` conversion
does — `ManagedAccountType` rejects only `PlatformPayment`. Rewrite
the doc to make that asymmetry explicit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>QuantumExplorer
commented
Apr 23, 2026
MemberAuthor
I realized that because we are using hardened derivation on identity auth keys, there's no point having this in the key wallet. |
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 freeto 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
Adds two new `AccountType` variants for DIP-13 sub-feature `0'` — the per-identity signing keys the user employs to sign Dash Platform state transitions. Previously `AccountCollection` only covered identity-related funding accounts (registration, top-up, invitation, asset-lock) — there was no wallet-managed account to produce signatures once a Platform identity was created.
New variants
Derivation path (DIP-13)
The account-level prefix is the first 6 hardened levels through `identity_index'`. The AddressPool under each account iterates `key_index'` (hardened, sequential per DIP-13).
Wiring
Design notes
Bincode forward-compat
Adding enum variants to `AccountType` is a forward-incompatible change for `bincode::Encode`/`Decode`: on-disk wallet blobs serialized by earlier v0.42-dev builds will fail to decode after this. Acceptable per the unstable 0.x API documented in `CLAUDE.md`. Serde uses its default (externally tagged) representation — new readers still decode old data identically; old readers fail cleanly on unknown variants.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes