feat(key-wallet): Add DIP-17 Platform Payment account support #229
Conversation
WalkthroughA new PlatformPayment (DIP-17) account type was added across core and FFI layers: new enum variants, derivation path constants and gap limit, new account/key types and collections, managed-account plumbing and address-pool handling, transaction-checking wiring (no Core-chain processing), FFI mappings, and derivation tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Wallet / Creator
participant AccountColl as AccountCollection
participant ManagedColl as ManagedAccountCollection
participant AddrPool as AddressPool
participant FFI as FFI boundary
participant TxChecker as TransactionChecker
Caller->>AccountColl: insert AccountType::PlatformPayment{account,key_class}
AccountColl->>ManagedColl: from_account_collection(...) (copies platform_payment_accounts)
ManagedColl->>AddrPool: AddressPool::new(DIP17_GAP_LIMIT, derivation_path)
AddrPool-->>ManagedColl: addresses
ManagedColl-->>FFI: exposed via FFIAccountType::PlatformPayment (enum value 13)
Note right of FFI: External client may request account info
FFI->>ManagedColl: lookup managed account by PlatformPayment
ManagedColl-->>FFI: returns ManagedAccountType::PlatformPayment (addresses)
FFI->>TxChecker: pass account type to transaction checking
TxChecker-->>FFI: returns no-op / empty match (PlatformPayment not used on Core chain)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
pauldelucia
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
key-wallet/src/account/account_collection.rs (2)
553-576:is_empty()missing check forplatform_payment_accounts.The
is_empty()method doesn't include a check forplatform_payment_accounts.is_empty(), which means it will incorrectly returntrueif only PlatformPayment accounts exist.&& self.provider_owner_keys.is_none(); #[cfg(feature = "bls")] { is_empty = is_empty && self.provider_operator_keys.is_none(); } #[cfg(feature = "eddsa")] { is_empty = is_empty && self.provider_platform_keys.is_none(); } + is_empty = is_empty + && self.dashpay_receival_accounts.is_empty() + && self.dashpay_external_accounts.is_empty() + && self.platform_payment_accounts.is_empty(); + is_emptyNote:
dashpay_receival_accountsanddashpay_external_accountsalso appear to be missing from theis_empty()check.
579-597:clear()missing cleanup for several account maps.The
clear()method doesn't cleardashpay_receival_accounts,dashpay_external_accounts, or the newplatform_payment_accounts, which could leave stale data after clearing.pub fn clear(&mut self) { self.standard_bip44_accounts.clear(); self.standard_bip32_accounts.clear(); self.coinjoin_accounts.clear(); self.identity_registration = None; self.identity_topup.clear(); self.identity_topup_not_bound = None; self.identity_invitation = None; self.provider_voting_keys = None; self.provider_owner_keys = None; #[cfg(feature = "bls")] { self.provider_operator_keys = None; } #[cfg(feature = "eddsa")] { self.provider_platform_keys = None; } + self.dashpay_receival_accounts.clear(); + self.dashpay_external_accounts.clear(); + self.platform_payment_accounts.clear(); }
🧹 Nitpick comments (4)
key-wallet-ffi/src/transaction_checking.rs (1)
458-478: PlatformPayment mapping into FFITransactionCheckResult is correctThe
AccountTypeMatch::PlatformPaymentarm correctly:
- propagates
account_index,received, andsent,- derives
external_addresses_count/has_external_addressesfrominvolved_addresses,- sets
account_typeto 13 in line withFFIAccountType::PLATFORM_PAYMENT.This mirrors DashPay handling and keeps FFI in sync with the new account type, while remaining defensive about platform addresses not being used on Core chain.
If you ever touch this area again, consider defining a shared Rust
FFIAccountTypeenum or constants instead of raw integers (0–13) to avoid accidental drift with the generated C header.key-wallet/src/managed_account/address_pool.rs (1)
397-418: Platform address pool support is correct; consider tightening PlatformP2sh handling
new_platformcleanly reusesnew_without_generation, flipsaddress_typetoPlatformP2pkh, and optionally pre‑generates up to the gap limit when a key source is available, matching the existing pool semantics but with DIP‑18 address types.is_platformgives a simple, type‑level way to gate Platform pools (e.g., exclude them from Core‑chain transaction checking), which aligns with the DIP‑17 requirement that Platform addresses not be used on Core.- Extending
generate_address_at_indexto callAddress::platform_p2pkhforAddressType::PlatformP2pkhis exactly what we want for Platform payment addresses.The only nuance is the
AddressType::PlatformP2shbranch currently defaulting toplatform_p2pkh, which is safe but potentially surprising if anyone later assumes genuine P2SH semantics.Consider, when you need stricter behavior, returning a dedicated error for
AddressType::PlatformP2shin this context (e.g., “PlatformP2sh generation not supported in AddressPool; script required”) instead of silently mapping it to P2PKH. That keeps library code non‑panicking per guidelines while making any misuse explicit.Also applies to: 425-428, 513-519
key-wallet/src/managed_account/managed_account_collection.rs (1)
730-777:all_accounts()does not includeplatform_payment_accounts.The
all_accounts()method returns all accounts but does not includeplatform_payment_accounts. Similarly,all_accounts_mut()(lines 781-828),is_empty()(lines 856-870), andclear()(lines 873-887) also omit this field.This may be intentional to exclude Platform accounts from Core wallet operations, but could cause issues if these methods are used for wallet-wide operations like serialization or total balance calculation.
If Platform accounts should be accessible via these methods, add them:
// In all_accounts() accounts.extend(self.dashpay_receival_accounts.values()); accounts.extend(self.dashpay_external_accounts.values()); +accounts.extend(self.platform_payment_accounts.values()); accounts // In is_empty() && self.dashpay_receival_accounts.is_empty() && self.dashpay_external_accounts.is_empty() +&& self.platform_payment_accounts.is_empty() // In clear() self.dashpay_receival_accounts.clear(); self.dashpay_external_accounts.clear(); +self.platform_payment_accounts.clear();dash/src/address.rs (1)
535-548: Potential risk: Platform addresses can generate Core-compatible scripts.The
script_pubkey()method generates equivalent P2PKH/P2SH scripts for Platform addresses. While documented with a warning, this could lead to accidental fund loss if a developer callsscript_pubkey()on a Platform address and uses it in a Core transaction.Consider returning an error or a distinct script type for Platform addresses to prevent accidental misuse:
/// Generates a script pubkey spending to this [Payload]. /// /// Note: For Platform addresses, this generates the equivalent Core chain script, /// but Platform addresses should NOT be used in Core chain transactions. /// Consider using `script_pubkey_checked()` which returns an error for Platform addresses. pub fn script_pubkey(&self) -> ScriptBuf { // ... existing implementation } /// Generates a script pubkey, returning an error for Platform addresses. pub fn script_pubkey_checked(&self) -> Result<ScriptBuf, Error> { if self.is_platform() { return Err(Error::PlatformAddressNotForCore); } Ok(self.script_pubkey()) }Alternatively, add a runtime check or logging when
script_pubkey()is called on Platform addresses to help catch misuse during development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
dash/src/address.rs(17 hunks)dash/src/blockdata/constants.rs(1 hunks)key-wallet-ffi/include/key_wallet_ffi.h(1 hunks)key-wallet-ffi/src/address_pool.rs(2 hunks)key-wallet-ffi/src/managed_account.rs(3 hunks)key-wallet-ffi/src/transaction_checking.rs(1 hunks)key-wallet-ffi/src/types.rs(3 hunks)key-wallet/src/account/account_collection.rs(9 hunks)key-wallet/src/account/account_type.rs(5 hunks)key-wallet/src/dip9.rs(3 hunks)key-wallet/src/managed_account/address_pool.rs(2 hunks)key-wallet/src/managed_account/managed_account_collection.rs(7 hunks)key-wallet/src/managed_account/managed_account_type.rs(6 hunks)key-wallet/src/managed_account/mod.rs(4 hunks)key-wallet/src/transaction_checking/account_checker.rs(6 hunks)key-wallet/src/transaction_checking/transaction_router/mod.rs(3 hunks)key-wallet/src/transaction_checking/wallet_checker.rs(1 hunks)key-wallet/src/wallet/helper.rs(1 hunks)key-wallet/tests/derivation_tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/dip9.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rsdash/src/blockdata/constants.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rsdash/src/address.rs
key-wallet/**/transaction_checking/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/transaction_checking/**/*.rs: UseTransactionRouter::classify_transaction()to determine transaction type andget_relevant_account_types()to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
In SPV sync integration, usewallet_info.check_transaction()with appropriateTransactionContextand update wallet state atomically only whenis_relevantflag is true
Files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet/src/transaction_checking/account_checker.rs
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Never panic in library code; useResult<T>for all fallible operations with customErrortype variants and provide context in error messages
Use?operator for error propagation in Rust code to maintain clean error handling patterns
Files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/dip9.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rs
key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Files:
key-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet/src/account/account_collection.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_collection.rs
key-wallet/**/{wallet,account,managed_account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/{wallet,account,managed_account}/**/*.rs: Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Files:
key-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet/src/account/account_collection.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_collection.rs
key-wallet/**/{account,managed_account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/{account,managed_account}/**/*.rs: Separate immutable and mutable concerns: UseAccountfor immutable identity information (keys, derivation paths) andManagedAccountfor mutable state (address pools, metadata, balances)
Use strongly typed enum-based system forAccountTypewith specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Files:
key-wallet/src/account/account_type.rskey-wallet/src/account/account_collection.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_collection.rs
key-wallet/**/{bip32,derivation,account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Use
ExtendedPubKeyandExtendedPrivKeyfrom the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Files:
key-wallet/src/account/account_type.rskey-wallet/src/account/account_collection.rs
**/*-ffi/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Careful handling of memory safety at FFI boundaries in Rust code
Files:
key-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction_checking.rs
key-wallet/**/managed_account/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Files:
key-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_collection.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Write integration tests for network operations in Rust
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate in RustIntegration tests use
tests/directory (e.g.,rpc-integration-test)
Files:
key-wallet/tests/derivation_tests.rs
key-wallet/**/tests/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/tests/**/*.rs: Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility
Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files
Files:
key-wallet/tests/derivation_tests.rs
🧠 Learnings (32)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Support future enhancements: prepare architecture for Schnorr/Taproot support, descriptor wallets, native multisig account types, and Lightning Network payment channel key management
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : Use `TransactionRouter::classify_transaction()` to determine transaction type and `get_relevant_account_types()` to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/dip9.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet-ffi/include/key_wallet_ffi.hkey-wallet/src/dip9.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/transaction_checking.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/dip9.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet-ffi/src/transaction_checking.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Applied to files:
key-wallet/src/dip9.rsdash/src/blockdata/constants.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation}/**/*.rs : Cache intermediate derivation results and use hardened derivation only when required; batch derive child keys when possible to optimize derivation performance
Applied to files:
key-wallet/src/dip9.rskey-wallet/src/account/account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet/src/dip9.rskey-wallet/src/wallet/helper.rskey-wallet/src/account/account_type.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rskey-wallet/src/account/account_collection.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/transaction_checking/account_checker.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files
Applied to files:
key-wallet/src/dip9.rskey-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
key-wallet/src/wallet/helper.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Never panic in library code; use `Result<T>` for all fallible operations with custom `Error` type variants and provide context in error messages
Applied to files:
key-wallet/src/wallet/helper.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Support future enhancements: prepare architecture for Schnorr/Taproot support, descriptor wallets, native multisig account types, and Lightning Network payment channel key management
Applied to files:
key-wallet/src/account/account_type.rskey-wallet/src/account/account_collection.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Applied to files:
key-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_collection.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store in address pools; only derive on-demand when pool is exhausted to optimize performance and memory usage
Applied to files:
key-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/address_pool.rskey-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
key-wallet-ffi/src/types.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash/src/blockdata/constants.rsdash/src/address.rs
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Maintain Minimum Supported Rust Version (MSRV) of 1.89; ensure compatibility with Dash Core versions 0.18.0 - 0.21.0
Applied to files:
dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/tests/**/*.rs : Add integration tests in the `tests/` directory for comprehensive test suites
Applied to files:
key-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Add comprehensive unit tests in-module for individual components
Applied to files:
key-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/tests/**/*.rs : Add real network tests with live Dash Core node integration that gracefully handle node unavailability
Applied to files:
key-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Test both mainnet and testnet configurations
Applied to files:
key-wallet/tests/derivation_tests.rsdash/src/address.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Follow a layered architecture with clear separation of concerns: `client/`, `network/`, `storage/`, `sync/`, `validation/`, `wallet/`, `types.rs`, and `error.rs`
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Applied to files:
dash/src/address.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Unit tests should be placed near code with descriptive names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash/src/address.rs
🧬 Code graph analysis (7)
key-wallet/src/account/account_type.rs (1)
key-wallet/src/transaction_checking/transaction_router/mod.rs (2)
from(187-237)from(241-291)
key-wallet-ffi/src/types.rs (1)
key-wallet/src/derivation.rs (1)
account(224-227)
key-wallet/src/managed_account/managed_account_type.rs (2)
key-wallet/src/managed_account/address_pool.rs (3)
network(1135-1138)new_platform(402-418)key_source(1153-1156)key-wallet/src/bip32.rs (1)
master(1302-1304)
key-wallet/tests/derivation_tests.rs (2)
key-wallet/src/derivation.rs (5)
from_seed(72-75)new(64-69)new(161-166)new(200-209)new(366-372)dash/src/address.rs (7)
from_str(225-236)from_str(294-297)from_str(1493-1572)encode(949-954)encode(985-990)new(479-499)new(1136-1144)
key-wallet-ffi/src/transaction_checking.rs (1)
key-wallet/src/transaction_checking/account_checker.rs (1)
account_index(178-210)
key-wallet/src/managed_account/managed_account_collection.rs (2)
key-wallet/src/account/account_collection.rs (1)
new(81-100)key-wallet/src/managed_account/address_pool.rs (6)
new(357-372)new(1094-1104)new_platform(402-418)base_path(1107-1110)network(1135-1138)key_source(1153-1156)
key-wallet/src/transaction_checking/account_checker.rs (2)
key-wallet/src/managed_account/managed_account_collection.rs (1)
new(58-75)key-wallet/src/managed_account/mod.rs (1)
new(63-73)
🪛 Gitleaks (8.30.0)
key-wallet/tests/derivation_tests.rs
[high] 126-126: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 227-227: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 310-310: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (34)
key-wallet/src/wallet/helper.rs (1)
876-880: LGTM! Correct handling of PlatformPayment account type.The implementation correctly returns
Nonefor PlatformPayment accounts, consistent with DIP-17/DIP-18 requirements that Platform Payment addresses are not used for Core chain transactions. This prevents xpub retrieval through this helper for platform-side-only account types.key-wallet/src/managed_account/mod.rs (1)
247-251: LGTM! Consistent integration of PlatformPayment across address pool methods.PlatformPayment is correctly grouped with DashpayExternalAccount in all address pool management methods (get_next_address_index, next_address, next_address_with_info, and gap_limit). This maintains consistent behavior for single-pool account types and properly integrates the new variant into the existing address generation and gap limit management flows.
Also applies to: 499-503, 588-592, 817-821
key-wallet/src/transaction_checking/wallet_checker.rs (1)
144-150: LGTM! Appropriate defensive handling for PlatformPayment.The implementation correctly returns
Nonefor PlatformPayment accounts with clear documentation that this branch should never be reached by design. Per DIP-17/DIP-18, Platform Payment addresses are not used in Core chain transactions, so preventing any state updates here is correct. This represents good defensive programming for exhaustive pattern matching.key-wallet-ffi/src/address_pool.rs (1)
62-67: LGTM! Correct FFI handling for PlatformPayment accounts.Both helper functions properly handle PlatformPayment by returning
None, consistent with the architecture where Platform Payment accounts are not persisted in ManagedAccountCollection. This aligns with DashPay account handling and correctly prevents retrieval of platform-side-only accounts through the FFI boundary.Also applies to: 111-116
key-wallet-ffi/include/key_wallet_ffi.h (1)
95-99: FFIAccountType extension for PlatformPayment looks consistent
PLATFORM_PAYMENT = 13cleanly extends the existing 0–12 range and matches the Rust-side mapping intransaction_checking.rs. No behavioral concerns as long as cbindgen and all FFI consumers (Swift, etc.) are regenerated/updated together.Please confirm that all language bindings consuming
FFIAccountTypehave been re-generated or updated so they see the newPLATFORM_PAYMENTcase.key-wallet/tests/derivation_tests.rs (1)
114-388: DIP‑17/DIP‑18 platform payment test vectors are thorough and deterministicThe new tests validate full paths (including non‑default
key_class'), private keys, compressed pubkeys, HASH160, and final PlatformP2PKH encodings on both mainnet and testnet, all derived from a fixed mnemonic. This gives strong regression coverage for the DIP‑17 derivation scheme and DIP‑18 Base58 encodings and matches the deterministic‑vector guidance forkey-wallettests.dash/src/blockdata/constants.rs (1)
58-71: DIP‑18 platform prefix constants are well‑placed and consistentDefining
PLATFORM_P2PKH_PREFIX_*andPLATFORM_P2SH_PREFIX_*here, alongside the existing pubkey/script prefixes, keeps all Base58 version bytes centralized. The values and doc comments align with the DIP‑18 handling indash::addressand the new platform address tests.key-wallet/src/transaction_checking/transaction_router/mod.rs (2)
181-183: Correct exclusion from Core chain transaction checking.The
PlatformPaymentvariant is properly documented as not being checked for Core chain transactions, aligning with the DIP-17 security requirement. Theget_relevant_account_types()method correctly excludesPlatformPaymentfrom all transaction type checks (lines 72-128), which is the intended behavior per the PR objectives.
233-235: Consistent conversion implementations for PlatformPayment.Both
From<ManagedAccountType>andFrom<&ManagedAccountType>implementations correctly mapPlatformPaymenttoAccountTypeToCheck::PlatformPayment, maintaining consistency with other account type conversions.Also applies to: 287-289
key-wallet/src/dip9.rs (3)
30-30: PlatformPayment derivation reference correctly positioned.The
PlatformPayment = 16variant is properly sequenced within theDerivationPathReferenceenum, maintaining the established pattern for new references.
382-419: Platform Payment root paths correctly implement DIP-17 structure.The root paths (
m/9'/5'/17'for mainnet,m/9'/1'/17'for testnet) correctly use:
- Hardened
FEATURE_PURPOSE(9')- Hardened coin type (5' for mainnet, 1' for testnet)
- Hardened
FEATURE_PURPOSE_PLATFORM_PAYMENT(17')The
CLEAR_FUNDSpath type is appropriate for payment addresses, consistent with other payment-related paths like DashPay.
135-136: DIP-17 feature index constant is correctly defined.The
FEATURE_PURPOSE_PLATFORM_PAYMENT = 17constant aligns with DIP-17 HD derivation specifications. Test vectors inkey-wallet/tests/derivation_tests.rsconfirm the constant's correctness with validated derivation paths (m/9'/5'/17'/0'/0'/0) and expected addresses.key-wallet-ffi/src/types.rs (2)
269-270: FFI enum variant correctly added for PlatformPayment.The
PlatformPayment = 13variant is properly sequenced and the doc comment correctly references DIP-17/DIP-18 with the derivation path structure.
424-427: Correct extraction of PlatformPayment fields for FFI.The
from_account_typeimplementation correctly mapsaccountto the primary index andkey_classto the optional secondary index, following the established tuple return pattern.key-wallet-ffi/src/managed_account.rs (3)
533-535: Correct FFI type mapping for PlatformPayment.The
managed_account_get_account_typefunction correctly mapsAccountType::PlatformPaymenttoFFIAccountType::PlatformPayment, maintaining consistency with other account type conversions.
1019-1022: PlatformPayment correctly exposed via Single address pool.The
managed_account_get_address_poolfunction correctly handlesPlatformPaymentby returning itsaddressesfield whenFFIAddressPoolType::Singleis requested, consistent with other non-standard account types like CoinJoin and Identity accounts.
181-183: PlatformPayment correctly excluded from generic account lookup.Returning
NoneforPlatformPaymentis consistent with howDashpayReceivingFundsandDashpayExternalAccountare handled. This follows the established pattern where account types requiring dedicated handling returnNonefrom the generic lookup function.Currently, no dedicated retrieval function exists for
PlatformPaymentaccounts (unlike the dedicatedmanaged_wallet_get_dashpay_receiving_accountandmanaged_wallet_get_dashpay_external_accountfunctions). If FFI callers need direct access toPlatformPaymentaccounts, consider adding a similar dedicated function for consistency and completeness.key-wallet/src/account/account_type.rs (3)
86-95: Well-documented PlatformPayment variant with security warning.The
PlatformPaymentvariant is properly documented with:
- DIP-17 reference and path structure
- DIP-18 encoding format note ("D"/"d" prefix)
- Critical warning that Platform addresses must not be used in Core chain transactions
The
accountandkey_classfields with their documentation align with DIP-17 path structure.
156-159: Correct primary index extraction for PlatformPayment.Returning
Some(*account)as the primary index is appropriate sinceaccountrepresents the account-level index in the derivation path, similar to how other account types expose their primary index.
413-437: DIP-17 Platform Payment derivation path implementation is correct.The code correctly implements the DIP-17 path structure (
m/9'/coin_type'/17'/account'/key_class') using network-appropriate root path constants and appending hardened indices. Restricting support to Mainnet and Testnet is intentional—no constants are defined indip9.rsfor Regtest/Devnet. This aligns with the design of other Platform-related variants (Masternode, Platform), which similarly support only Mainnet and Testnet.key-wallet/src/account/account_collection.rs (4)
31-40: Clean key structure for PlatformPayment accounts.The
PlatformPaymentAccountKeystruct correctly models the composite key withaccountandkey_classfields, properly deriving necessary traits includingOrdfor BTreeMap compatibility. The doc comment references DIP-17.
75-76: Consistent storage pattern for PlatformPayment accounts.Using
BTreeMap<PlatformPaymentAccountKey, Account>follows the established pattern for account types with composite keys (like DashPay accounts). Initialization innew()is correct.Also applies to: 98-98
174-183: Complete CRUD implementation for PlatformPayment.The
insert(),contains_account_type(),account_of_type(), andaccount_of_type_mut()methods all correctly handlePlatformPaymentby constructing the composite key and delegating to theplatform_payment_accountsmap.Also applies to: 267-276, 330-336, 390-396
433-435: Aggregation methods include PlatformPayment accounts.Both
all_accounts()andall_accounts_mut()correctly extend their results withplatform_payment_accounts.values(), ensuring PlatformPayment accounts are included in collection-wide operations.Also applies to: 473-475
key-wallet/src/transaction_checking/account_checker.rs (4)
106-112: Well-structured PlatformPayment variant with clear documentation.The
PlatformPaymentvariant correctly capturesaccount_index,key_class, andinvolved_addresses, matching the DIP-17 derivation path structure. The doc comment clearly warns that Platform addresses are not used in Core chain transactions.
387-392: Correct enforcement of DIP-17 security requirement.Returning an empty
VecforAccountTypeToCheck::PlatformPaymentcorrectly implements the DIP-17 mandate that Platform Payment addresses must not be used in Core chain transactions. The comment clearly explains this design decision.
648-654: PlatformPayment match arm correctly extracts account and key_class.The match arm properly destructures
ManagedAccountType::PlatformPaymentand constructs the correspondingAccountTypeMatch::PlatformPaymentwith the correct field mappings.
914-994:find_address_accountdoes not checkplatform_payment_accounts.This method searches for addresses across account types but does not include
platform_payment_accounts. This is likely intentional per DIP-17 (Platform addresses shouldn't appear in Core transactions), but if Platform addresses should be discoverable for other purposes (e.g., wallet display), this method may need updating in the future.Confirm this omission is intentional. If Platform addresses should be findable via this method for non-transaction purposes, consider adding:
// Check Platform Payment accounts for ((account, key_class), account_obj) in &collection.platform_payment_accounts { if account_obj.contains_address(address) { return Some((AccountTypeToCheck::PlatformPayment, Some(*account))); } }key-wallet/src/managed_account/managed_account_type.rs (1)
107-118: Clear and well-documented PlatformPayment variant.The variant definition includes:
- DIP-17 derivation path in the doc comment
- DIP-18 encoding format note
- Critical warning about not using in Core chain transactions
- Appropriate fields matching the derivation path structure
key-wallet/src/managed_account/managed_account_collection.rs (2)
52-53: Platform payment accounts field follows established patterns.The
platform_payment_accountsfield usesBTreeMap<PlatformPaymentAccountKey, ManagedAccount>consistent with other keyed account collections likedashpay_receival_accounts. This follows the coding guideline to use BTreeMap for ordered data structures.Also applies to: 73-73
607-624: Consistent Platform account creation in factory method.The
create_managed_account_from_account_typecorrectly usesAddressPool::new_platform()for Platform Payment accounts, ensuring addresses are generated with the correctPlatformP2pkhaddress type.dash/src/address.rs (3)
201-206: Platform address types clearly documented with usage warning.The
PlatformP2pkhandPlatformP2shvariants include clear doc comments explaining they are for Dash Platform only and MUST NOT be used in Core chain transactions. This is critical for preventing fund loss.
2296-2422: Comprehensive DIP-18 test coverage.The tests cover:
- Mainnet and testnet P2PKH Platform addresses with DIP-18 vectors
- Multiple test vectors for different HASH160 values
- P2SH Platform addresses
- Round-trip parsing verification
- Distinction between Platform and Core addresses
is_platform()flag verificationThis provides strong confidence in the DIP-18 implementation correctness.
1005-1006: bincode serialization uses sequential integer values.Values 5 and 6 are assigned for
PlatformP2pkhandPlatformP2shrespectively, continuing the sequence fromP2tr = 4. This maintains backward compatibility for existing serialized data.Also applies to: 1024-1025
|
Just reopening to trigger CI after pre-commit fix. |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Just had a quick glance and wondered about all the comments like "this will never be reached because its not used in core", "this must not be used" etc.. ? Sounds like this shouldnt be in here then to begin with? Cant this go into one of the platform repos? @QuantumExplorer didnt you say there is some platform wallet also around somewhere which is built on the wallet in here?
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 (4)
key-wallet/src/managed_account/managed_account_collection.rs (4)
730-778: Platform Payment accounts excluded fromall_accounts().The
all_accounts()method does not includeplatform_payment_accounts, causing Platform Payment accounts to be invisible to any code that iterates over all accounts.Add Platform Payment accounts to the collection:
// Add DashPay accounts accounts.extend(self.dashpay_receival_accounts.values()); accounts.extend(self.dashpay_external_accounts.values()); + accounts.extend(self.platform_payment_accounts.values()); accounts }
781-829: Platform Payment accounts excluded fromall_accounts_mut().The
all_accounts_mut()method does not includeplatform_payment_accounts, preventing mutable access to Platform Payment accounts through collection-wide operations.Add Platform Payment accounts to the collection:
// Add DashPay accounts accounts.extend(self.dashpay_receival_accounts.values_mut()); accounts.extend(self.dashpay_external_accounts.values_mut()); + accounts.extend(self.platform_payment_accounts.values_mut()); accounts }
856-870: Platform Payment accounts not checked inis_empty().The
is_empty()method does not check ifplatform_payment_accountsis empty, which could cause the collection to be considered empty even when Platform Payment accounts exist.Add the check:
&& self.provider_platform_keys.is_none() && self.dashpay_receival_accounts.is_empty() && self.dashpay_external_accounts.is_empty() + && self.platform_payment_accounts.is_empty() }
873-887: Platform Payment accounts not cleared inclear().The
clear()method does not clearplatform_payment_accounts, leaving Platform Payment accounts in the collection after clearing.Add the clear call:
self.provider_platform_keys = None; self.dashpay_receival_accounts.clear(); self.dashpay_external_accounts.clear(); + self.platform_payment_accounts.clear(); }
🧹 Nitpick comments (3)
key-wallet/tests/derivation_tests.rs (3)
114-124: DIP‑17 test section is clearly delineatedThe banner + comments nicely scope the Platform Payment derivation vectors and document mnemonic/passphrase. If this section grows, consider moving it into a dedicated
dip17_platform_payment_tests.rsto keep derivation tests grouped by feature.Based on learnings, grouping tests by functionality (BIP32 vs. DIP‑17) will help long‑term maintenance.
125-173: Mainnet DIP‑17 vector 1 is a strong deterministic regression testThis test fully pins private key, compressed pubkey, and HASH160 for
m/9'/5'/17'/0'/0'/0using the canonical “abandon … about” mnemonic, which is ideal for catching derivation regressions. One practical follow‑up: static-analysis tools like gitleaks may flag these hex constants as secrets, so it’s worth adding a brief comment or repository‑level allow‑list entry noting they’re public DIP‑17 test vectors to avoid CI noise.Based on learnings, this satisfies the deterministic test‑vector requirement for mainnet derivation.
175-204: Strengthen testnet coverage:is_empty()on fixed-size arrays is a no-opIn all three testnet branches you only assert
!pubkey_hash.to_byte_array().is_empty(). Sinceto_byte_array()returns a fixed-length array,is_empty()is alwaysfalse, so these assertions can never fail and don’t actually validate derivation correctness.It would be more robust to:
- Capture the expected testnet HASH160 (and optionally priv/pub keys) for each path, and
- Assert equality against those hex strings, mirroring the mainnet vectors.
That would give you real, deterministic regression coverage for testnet as well, instead of a compile‑time tautology.
Based on learnings, both mainnet and testnet derivations should use fixed, known test vectors rather than non‑emptiness checks.
Also applies to: 253-263, 315-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
key-wallet/src/managed_account/address_pool.rs(1 hunks)key-wallet/src/managed_account/managed_account_collection.rs(7 hunks)key-wallet/src/managed_account/managed_account_type.rs(7 hunks)key-wallet/tests/derivation_tests.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet/src/managed_account/address_pool.rs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
key-wallet/**/{account,managed_account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/{account,managed_account}/**/*.rs: Separate immutable and mutable concerns: UseAccountfor immutable identity information (keys, derivation paths) andManagedAccountfor mutable state (address pools, metadata, balances)
Use strongly typed enum-based system forAccountTypewith specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/managed_account/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Never panic in library code; useResult<T>for all fallible operations with customErrortype variants and provide context in error messages
Use?operator for error propagation in Rust code to maintain clean error handling patterns
Files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
key-wallet/**/{wallet,account,managed_account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/{wallet,account,managed_account}/**/*.rs: Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Write integration tests for network operations in Rust
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate in RustIntegration tests use
tests/directory (e.g.,rpc-integration-test)
Files:
key-wallet/tests/derivation_tests.rs
key-wallet/**/tests/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/tests/**/*.rs: Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility
Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files
Files:
key-wallet/tests/derivation_tests.rs
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Support future enhancements: prepare architecture for Schnorr/Taproot support, descriptor wallets, native multisig account types, and Lightning Network payment channel key management
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : Use `TransactionRouter::classify_transaction()` to determine transaction type and `get_relevant_account_types()` to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Applied to files:
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store in address pools; only derive on-demand when pool is exhausted to optimize performance and memory usage
Applied to files:
key-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Applied to files:
key-wallet/src/managed_account/managed_account_type.rskey-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation}/**/*.rs : Cache intermediate derivation results and use hardened derivation only when required; batch derive child keys when possible to optimize derivation performance
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Test both mainnet and testnet configurations
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should be organized in `tests/` directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
key-wallet/tests/derivation_tests.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Keep test vectors deterministic in Rust test code
Applied to files:
key-wallet/tests/derivation_tests.rs
🧬 Code graph analysis (2)
key-wallet/src/managed_account/managed_account_collection.rs (1)
key-wallet/src/account/account_collection.rs (1)
new(81-100)
key-wallet/tests/derivation_tests.rs (2)
key-wallet/src/mnemonic.rs (1)
from_phrase(220-227)key-wallet/src/derivation.rs (5)
from_seed(72-75)new(64-69)new(167-172)new(206-215)new(372-378)
🪛 Gitleaks (8.30.0)
key-wallet/tests/derivation_tests.rs
[high] 127-127: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 208-208: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 230-230: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 270-270: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 292-292: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: Core Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: SPV Components Tests
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (16)
key-wallet/src/managed_account/managed_account_type.rs (7)
5-5: LGTM!The import of
DIP17_GAP_LIMITis correctly added and used in the Platform Payment account initialization.
107-118: LGTM!The
PlatformPaymentvariant is well-documented with clear DIP-17/DIP-18 references and includes an important security warning about not using Platform addresses in Core chain transactions.
167-170: LGTM!The
index()method correctly returns the account index forPlatformPayment, consistent with other indexed account types.
246-249: LGTM!The
address_pools()method correctly returns the single address pool forPlatformPayment, consistent with other single-pool account types.
309-312: LGTM!The
address_pools_mut()method correctly returns the mutable address pool, consistent with the immutable version.
428-435: LGTM!The
to_account_type()conversion correctly maps thePlatformPaymentmanaged type to its corresponding account type.
679-700: The code at lines 679-700 is correct as written.AddressPool::new()withAddressPoolType::Absentis the standard pattern used throughout the codebase for all special account types (Platform accounts, Identity accounts, etc.) that don't distinguish between external and internal address pools. There is nonew_platform()constructor in the AddressPool implementation; all address pools are created using the singlenew()method with appropriateAddressPoolTypevariants.Likely an incorrect or invalid review comment.
key-wallet/src/managed_account/managed_account_collection.rs (6)
6-6: LGTM!The import of
PlatformPaymentAccountKeyis correctly added to support the new Platform Payment account type.
52-53: LGTM!The
platform_payment_accountsfield is correctly defined usingBTreeMapfor ordered storage, consistent with other account collections. As per coding guidelines.
73-73: LGTM!The field is correctly initialized in the
new()constructor.
149-159: LGTM!The
contains_managed_account_type()method correctly checks for Platform Payment accounts using the composite key, consistent with DashPay account patterns.
253-263: LGTM!The
insert()method correctly handles Platform Payment accounts with proper key construction. The use ofacc_indexavoids variable shadowing.
360-365: LGTM!The conversion from
AccountCollectioncorrectly processes Platform Payment accounts, consistent with other account type conversions.key-wallet/tests/derivation_tests.rs (3)
3-3:Hashtrait import is correct for HASH160 usageBringing
dashcore::hashes::Hashinto scope is required forpubkey_hash.to_byte_array()to compile; this is the right, minimal import.
206-252: Vector 2 mainnet adds valuable coverage at a different indexChecking
m/9'/5'/17'/0'/0'/1with full privkey/pubkey/HASH160 expectations is a good complement to vector 1 and should catch index/path mixups in the DIP‑17 implementation.Based on learnings, adding multiple deterministic vectors per path family is preferred for key-derivation tests.
267-314: Non‑defaultkey_class' = 1'vector meaningfully exercises alternate path branchThis vector for
m/9'/5'/17'/0'/1'/0(non‑default key_class) with pinned privkey/pubkey/HASH160 is important coverage for the extra hardened step and guards against future changes to the key_class slot logic.Based on learnings, this strengthens coverage for alternate account/key classes in DIP‑17.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
key-wallet/src/managed_account/managed_account_type.rs (1)
107-117: PlatformPayment variant is correctly integrated; double‑check DIP‑18 doc wordingThe
PlatformPaymentvariant is wired consistently with existing patterns:
accountis treated as the primary index (returned byindex()), matching the “Account index (hardened)” doc.address_pools()/address_pools_mut()correctly include the singleaddressespool alongside other single‑pool account types.to_account_type()maps cleanly toAccountType::PlatformPayment { account, key_class }, preserving both parameters.Functionally this all looks coherent and in line with the other managed account variants.
One minor documentation nit: the comment says “Address encoding (DIP-18 bech32m) is handled by the Platform repo.” Given that this PR also introduces Base58Check encodings with
D/dandP/pprefixes in thedashcorecrate, please confirm that this wording accurately reflects the Platform‑side encoding (and whether both encodings coexist) to avoid confusion for future maintainers.Also applies to: 166-169, 241-247, 304-311, 427-434
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
key-wallet/src/account/account_type.rs(5 hunks)key-wallet/src/managed_account/managed_account_type.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet/src/account/account_type.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/{account,managed_account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/{account,managed_account}/**/*.rs: Separate immutable and mutable concerns: UseAccountfor immutable identity information (keys, derivation paths) andManagedAccountfor mutable state (address pools, metadata, balances)
Use strongly typed enum-based system forAccountTypewith specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Files:
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Files:
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/managed_account/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Files:
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Never panic in library code; useResult<T>for all fallible operations with customErrortype variants and provide context in error messages
Use?operator for error propagation in Rust code to maintain clean error handling patterns
Files:
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/**/{wallet,account,managed_account}/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/{wallet,account,managed_account}/**/*.rs: Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Files:
key-wallet/src/managed_account/managed_account_type.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Support future enhancements: prepare architecture for Schnorr/Taproot support, descriptor wallets, native multisig account types, and Lightning Network payment channel key management
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : Use `TransactionRouter::classify_transaction()` to determine transaction type and `get_relevant_account_types()` to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store in address pools; only derive on-demand when pool is exhausted to optimize performance and memory usage
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Applied to files:
key-wallet/src/managed_account/managed_account_type.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Core Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha256)
🔇 Additional comments (1)
key-wallet/src/managed_account/managed_account_type.rs (1)
5-5: DIP‑17 gap limit and AddressPool configuration for PlatformPayment look correctThe
AccountType::PlatformPaymentbranch infrom_account_type()is consistent and respects the DIP‑17 design:
- Uses
account_type.derivation_path(network)as the base path, which should already implementm/9'/coin_type'/17'/account'/key_class'.- Instantiates a single
AddressPoolwithAddressPoolType::Absent, matching other special, non‑Core‑UTXO account types that shouldn’t be scanned as regular Core transaction addresses.- Applies the dedicated
DIP17_GAP_LIMIT, decoupling DIP‑17 semantics fromDEFAULT_SPECIAL_GAP_LIMITand resolving earlier ambiguity.- Returns
ManagedAccountType::PlatformPayment { account, key_class, addresses: pool }, preserving both hardened indices and the pool.No functional issues spotted here; this is a clean addition.
Also applies to: 678-699
pauldelucia
changed the title
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Looks good, just got two questions.
| let addresses = AddressPool::new( | ||
| base_path, | ||
| AddressPoolType::Absent, | ||
| DEFAULT_SPECIAL_GAP_LIMIT, |
There was a problem hiding this comment.
You introduced a DIP17_GAP_LIMIT, was this supposed to be used here?
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (23)
dash-spv/CLAUDE.md (1)
18-18: Remove the stalesync/sequential/entry from line 18.The
sync/sequential/directory does not exist in the codebase. The synchronization logic is handled bySyncManagerdefined insync/manager.rs. Remove line 18 or consolidate its description into the parentsync/module entry.dash-spv/tests/handshake_test.rs (3)
10-11: Add#[ignore]attribute for network-dependent test.This test requires a live Dash Core node at 127.0.0.1:9999, making it network-dependent. As per coding guidelines, it should be marked with
#[ignore]and run explicitly with-- --ignoredflag.Apply this diff:
+#[ignore] #[tokio::test] async fn test_handshake_with_mainnet_peer() {Based on coding guidelines: "Use
#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag"
51-52: Add#[ignore]attribute for network-dependent test.Although this test uses a non-routable IP, it still performs network operations and may behave differently across environments. Mark it with
#[ignore]per coding guidelines.Apply this diff:
+#[ignore] #[tokio::test] async fn test_handshake_timeout() {Based on coding guidelines: "Use
#[ignore]for network-dependent or long-running tests"
86-87: Add#[ignore]attribute for network-dependent test.This test requires a live Dash Core node, making it network-dependent. Mark it with
#[ignore]per coding guidelines.Apply this diff:
+#[ignore] #[tokio::test] async fn test_multiple_connect_disconnect_cycles() {Based on coding guidelines: "Use
#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag"dash-spv/tests/segmented_storage_test.rs (1)
185-210: Ensure the sharedDiskStorageManageris cleanly shut down (avoid leaking the background worker).
WithArc<DiskStorageManager>, you currently can’t callshutdown()(if it requires&mut self). IfDropdoesn’t reliably stop the worker, this can cause test flakiness/hangs.One option is to unwrap after all tasks finish and then call shutdown:
// Wait for all readers for handle in handles { handle.await.unwrap(); } + + // Clean shutdown (only works if all task Arcs were dropped) + let mut storage = Arc::try_unwrap(storage).expect("storage Arc still has outstanding refs"); + storage.shutdown().await.unwrap();Based on learnings, the storage API uses interior mutability and concurrency patterns—explicitly shutting down here helps keep tests deterministic.
dash-spv/src/sync/filters/requests.rs (2)
17-49: Critical: unsigned underflow whencount == 0If
countisSome(0),(start + c - 1)underflows and can turn into a massiveend, potentially building/sending an unbounded queue.- let end = if let Some(c) = count { - (start + c - 1).min(filter_header_tip_height) - } else { - filter_header_tip_height - }; + let end = if let Some(c) = count { + if c == 0 { + tracing::warn!("Filter sync requested with count=0; nothing to do"); + return Ok(()); + } + start + .saturating_add(c.saturating_sub(1)) + .min(filter_header_tip_height) + } else { + filter_header_tip_height + };
133-240: Remove the unused_storageparameter fromprocess_filter_request_queueThe
_storageparameter is passed from the callsite indownload.rs:93but never used in the function body. Remove the parameter from both the function signature (requests.rs:137-140) and the callsite (download.rs:93).The queue pumping and flow-control accounting logic itself is sound—
pending_filter_requests,active_filter_requests, andMAX_CONCURRENT_FILTER_REQUESTSare properly defined and tracked with no dead state detected.dash-spv-ffi/src/bin/ffi_cli.rs (1)
120-125: Don’t ignoredash_spv_ffi_init_loggingresult; print last_error on failure.Suggested tweak:
- let _ = dash_spv_ffi_init_logging(level_c.as_ptr(), true, std::ptr::null(), 0); + let rc = dash_spv_ffi_init_logging(level_c.as_ptr(), true, std::ptr::null(), 0); + if rc != FFIErrorCode::Success as i32 { + eprintln!("Logging init failed: {}", ffi_string_to_rust(dash_spv_ffi_get_last_error())); + }dash-spv-ffi/src/error.rs (1)
39-45: FFI error pointer lifetime not guaranteed across threads; refactor error ownership model.
dash_spv_ffi_get_last_error()returns a direct pointer into aCStringprotected by a globalMutex. If another thread callsset_last_error()orclear_last_error()while a caller holds the pointer, the returned*const c_charbecomes invalid—a potential use-after-free bug in multi-threaded contexts.Recommended fix: Return a newly allocated C string and require callers to free it with
dash_string_free()(consistent with other FFI allocations like config objects). This aligns with FFI robustness and matches the pattern used elsewhere in the crate.dash-spv/src/storage/disk/manager.rs (1)
83-92: Lock acquisition should open the file without truncating; acquire lock first, then write PID.
LockFile::newcurrently usesFile::create, which truncates the lock file before attemptingtry_lock(). When two processes start concurrently, both truncate the file, and the one that fails to acquire the lock has already wiped its contents. Although the lock is held for the application's lifetime in this implementation, the proper pattern is to open without truncation, acquire the lock, then safely write the PID.Suggested change in
dash-spv/src/storage/disk/lockfile.rs:-use std::fs::File; +use std::fs::OpenOptions; pub(super) fn new(path: PathBuf) -> StorageResult<Self> { - let mut file = File::create(&path) + let mut file = OpenOptions::new() + .create(true) + .read(true) + .write(true) + .open(&path) .map_err(|e| StorageError::WriteFailed(format!("Failed to create lock file: {}", e)))?; file.try_lock().map_err(|e| match e { std::fs::TryLockError::WouldBlock => StorageError::DirectoryLocked(format!( "Data directory '{}' is already in use by another process", path.parent().map(|p| p.display().to_string()).unwrap_or_default() )), std::fs::TryLockError::Error(io_err) => { StorageError::WriteFailed(format!("Failed to acquire lock: {}", io_err)) } })?; + let _ = file.set_len(0); if let Err(e) = writeln!(file, "{}", std::process::id()) { tracing::warn!("Failed to write PID to lock file: {}", e); }dash-spv/src/sync/filters/retry.rs (1)
160-188: When CFHeaders retries are exhausted, consider failing the sync (don’t silently “give up”).
Currently, onceretry_count >= self.max_cfheader_retriesyou log and returnOk(())(Line 203-210). If that request was needed for contiguous CFHeaders, this can stall the sync indefinitely without a clear failure signal.One option is to return an error that bubbles up and transitions the client/sync state to “failed”:
if retry_count >= self.max_cfheader_retries { tracing::error!( "❌ CFHeaders request for height {} failed after {} retries, giving up", start_height, retry_count ); - return Ok(()); + return Err(SyncError::Network(format!( + "CFHeaders request timed out at start_height={} after {} retries", + start_height, retry_count + ))); }(Adjust the error variant if a more appropriate one exists.)
Also applies to: 190-233
dash-spv/src/storage/memory.rs (1)
20-50: Possible base-height source-of-truth mismatch (chain_state vs sync_state).
sync_base_height()derives fromself.chain_state, butstore_sync_state()/load_sync_state()now operate onself.sync_state. Ifsync_stateis restored/used without a correspondingchain_state(or ifchain_statelags), header/filter absolute-height mapping can silently fall back to base=0.Consider falling back to
self.sync_state.as_ref().map(|s| s.sync_base_height)(or whatever field represents the base) whenchain_stateisNone.As per retrieved learnings, document the thread-safety / state-source expectations clearly since
StorageManagerisSend + Syncbut uses&mut self.dash-spv/src/sync/phase_execution.rs (1)
138-171: Guard against unsigned underflow when computingcountfor filter downloads.If
start_height > filter_header_tip,filter_header_tip - start_height + 1underflows and becomes a hugeu32, causing pathological behavior.- let start_height = self.header_sync.get_sync_base_height().max(1); - let count = filter_header_tip - start_height + 1; + let start_height = self.header_sync.get_sync_base_height().max(1); + if filter_header_tip < start_height { + tracing::warn!( + "Filter header tip {} is below start_height {}, skipping filter download", + filter_header_tip, start_height + ); + self.transition_to_next_phase(storage, network, "No filters to download").await?; + return Ok(()); + } + let count = filter_header_tip - start_height + 1;dash-spv/src/client/sync_coordinator.rs (1)
918-965: Restore validation misses cross-batch continuity (can accept a broken chain at batch boundaries).
validate_headers()skips continuity for the first header in the slice, so a discontinuity betweenprev_batch_lastandthis_batch_firstwon’t be caught.Suggested minimal fix: compare
headers.first().prev_blockhashagainst the previous tip hash (fromChainStateor the last loaded header) before callingvalidate_headers().dash-spv/src/sync/filters/download.rs (1)
73-110: Ensuresyncing_filtersis reset on early error paths (currently can get stuck “in progress”).
sync_filters()setsself.syncing_filters = true(Line 84) and then performs fallible work (build_filter_request_queue,process_filter_request_queue). Any?early-return will leavesyncing_filterstrue and the cleared queues partially initialized, preventing later retries.Suggested fix (keep
syncing_filterstrue only after successful init, or explicitly unwind on error). For example:pub async fn sync_filters( &mut self, network: &mut N, storage: &mut S, start_height: Option<u32>, count: Option<u32>, ) -> SyncResult<SyncProgress> { if self.syncing_filters { return Err(SyncError::SyncInProgress); } - - self.syncing_filters = true; - - // Clear any stale state from previous attempts - self.clear_filter_sync_state(); - - // Build the queue of filter requests - self.build_filter_request_queue(storage, start_height, count).await?; - - // Start processing the queue - self.process_filter_request_queue(network, storage).await?; + // Clear any stale state from previous attempts + self.clear_filter_sync_state(); + + // Build the queue of filter requests + self.build_filter_request_queue(storage, start_height, count).await?; + + // Start processing the queue + self.process_filter_request_queue(network, storage).await?; + + // Mark in-progress only after successful init + self.syncing_filters = true; tracing::info!( "✅ Filter sync initiated ({} requests queued, {} active)", self.pending_filter_requests.len(), self.active_filter_requests.len() );dash-spv/src/storage/disk/state.rs (1)
330-345:store_metadata()allows path traversal viakey→ filename.
self.base_path.join(format!("state/{}.dat", key))will happily acceptkey = "../foo"(or path separators), escaping the state directory. Ifkeycan ever be influenced outside trusted code paths, this is a security bug.Recommend enforcing a strict key charset and rejecting anything else (or mapping keys to a safe encoding). For example:
pub async fn store_metadata(&mut self, key: &str, value: &[u8]) -> StorageResult<()> { + if key.is_empty() + || !key + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | '.')) + { + return Err(crate::error::StorageError::WriteFailed(format!( + "Invalid metadata key: {key}" + ))); + } let path = self.base_path.join(format!("state/{}.dat", key)); atomic_write(&path, value).await?; Ok(()) }dash-spv/src/client/message_handler.rs (1)
8-58: Rename lingering “Sequential sync manager” log messages to “SyncManager” to avoid confusion.Even after the type switch, error logs still read “Sequential sync manager error …”, which will mislead debugging (and grepping). (E.g., Line 89, 102, 130, 146, 163, 196 in this file.)
dash-spv/src/sync/filters/headers.rs (2)
144-155: Blocker:start_sync_headers()is incompatible with the new queue-basedhandle_filter_headers()and can stall sync forever.After Line 154, CFHeaders handling always goes through
handle_filter_headers(), which relies onself.next_cfheader_height_to_processbeing initialized (Line 610).start_sync_headers()does not set it (nor build the CFHeaders queue), so the first received batch will be buffered as “out of order” and never processed.Fix: make
start_sync_headers()a thin wrapper around the new entrypoint (or delete/privatize it). Minimal safe change:pub async fn start_sync_headers( &mut self, network: &mut N, storage: &mut S, ) -> SyncResult<bool> { - // (old one-shot request logic) - ... + self.start_sync_filter_headers(network, storage).await }This also reduces the chance that callers accidentally pick the wrong entrypoint. As per sync learnings, keeping a single SyncManager-driven phase entry is preferable.
Also applies to: 157-333, 375-460
579-652: Active-request tracking removal is keyed inconsistently; handle truncated CFHeaders responses explicitly.You insert
active_cfheader_requestsbyrequest.start_height(Line 567), but on response you remove bybatch_start_heightcomputed fromstop_hashandfilter_hashes.len()(Line 607). If a peer returns fewer entries than requested,batch_start_height != request.start_height, so the active entry won’t be removed and available slots won’t refill correctly.Recommendation:
- Identify the active request by
stop_hash(stored inActiveCFHeaderRequest), remove by that key.- Validate
batch_start_height == request_start_height; if not, treat as protocol/peer issue and retry with an adjusted range rather than silently buffering.(If you want, I can draft a concrete patch once you confirm whether you expect partial CFHeaders batches in practice.)
dash-spv/src/sync/headers/manager.rs (3)
164-214: Validation logging usestracing::error!in a confusing/likely-wrong wayThis:
- Line 210:
tracing::error!(error);will emit an event with a field named
error(and typically no human-readable message). Prefertracing::error!(%error);ortracing::error!("{}", error);.if self.config.validation_mode != ValidationMode::None { validate_headers(&cached_headers).map_err(|e| { let error = format!("Header validation failed: {}", e); - tracing::error!(error); + tracing::error!(%error); SyncError::Validation(error) })?; }
319-451:request_headers()can emit an empty locator for normalGetHeaderseven though headers2 is disabledBecause the “empty locator for headers2 genesis sync” branch is keyed off
network.has_headers2_peer().awaitrather thanuse_headers2, you can still sendGetHeaderswithlocator_hashes = []whenbase_hashis genesis. That’s a behavior change (and could cause peers to respond from genesis / unexpected ancestor logic).Suggested fix: only apply the empty-locator special-case when you’re actually sending headers2.
- } else if network.has_headers2_peer().await && !self.headers2_failed { + } else if use_headers2 && network.has_headers2_peer().await && !self.headers2_failed { // Check if this is genesis and we're using headers2 let genesis_hash = self.config.network.known_genesis_block_hash(); if genesis_hash == Some(hash) { tracing::info!("📍 Using empty locator for headers2 genesis sync"); vec![] } else { vec![hash] } } else { vec![hash] }(You’ll need to hoist
let use_headers2 = false;above the locator computation, or compute locator after deciding the protocol.)
456-563:handle_headers2_message()is permanently erroring but still carries a large unreachable implementationRight now the function always returns an error and keeps a large
#[allow(unreachable_code)]block. This makes maintenance risky (it will silently rot) and increases review surface.Recommendation: either remove the unreachable code entirely, or feature-gate the whole decompression path (e.g.,
#[cfg(feature = "headers2")]) and keep the stub in the default build.dash-spv/src/sync/manager.rs (1)
403-421:get_blockchain_height_from_storage()likely double-counts height when synced from checkpointOther codepaths treat
storage.get_tip_height()as an absolute chain height (e.g., comparisons toget_sync_base_height(), queryingget_header(0)for genesis). If so, addingsync_base_height + storage_heightis wrong and will overstate height.Suggested fix (assuming storage heights are absolute):
pub(super) async fn get_blockchain_height_from_storage(&self, storage: &S) -> SyncResult<u32> { let storage_height = storage @@ .unwrap_or(0); - // Check if we're syncing from a checkpoint - if self.header_sync.is_synced_from_checkpoint() { - // For checkpoint sync, blockchain height = sync_base_height + storage_height - Ok(self.header_sync.get_sync_base_height() + storage_height) - } else { - // Normal sync: storage height IS the blockchain height - Ok(storage_height) - } + // Storage height is already an absolute chain height. + Ok(storage_height) }If storage heights are actually relative in some backends, please add a clarifying doc comment + a single canonical conversion helper used everywhere (to avoid the current mixed assumptions). Based on learnings, clearer documentation around
StorageManagersemantics would help.
🧹 Nitpick comments (20)
.github/workflows/conflict-check.yml (1)
18-22: Pin the 3rd‑party action to a commit SHA (supply‑chain hardening).Using
eps1lon/actions-label-merge-conflict@v3is fine, but for a workflow with write permissions, pin to a full commit SHA instead of a mutable tag. The v3 release corresponds to commit SHA1df065ebe6e3310545d4f4c4e862e43bdca146f0.Recommended change:
uses: eps1lon/actions-label-merge-conflict@1df065ebe6e3310545d4f4c4e862e43bdca146f0GitHub recommends SHA pinning to make actions immutable and reduce supply-chain risk.
dash-spv/src/sync/filters/stats.rs (1)
20-26: Silent failure on lock contention may mislead callers.When
try_lock()fails due to lock contention, returning0is indistinguishable from actually having zero received filters. For statistics/monitoring purposes this is usually acceptable, but callers cannot differentiate between "no filters received" and "couldn't read count."Consider returning
Option<u32>or documenting this behavior clearly:- /// Get the total number of filters received. - pub fn get_received_filter_count(&self) -> u32 { + /// Get the total number of filters received. + /// Returns 0 if the lock cannot be acquired (non-blocking). + pub fn get_received_filter_count(&self) -> u32 {dash-spv/src/storage/disk/filters.rs (1)
186-190: Consider async existence check inload_filterto avoid sync FS call on async path.
path.exists()is a blocking metadata check;tokio::fs::try_exists(or just attemptreadand map NotFound →Ok(None)) avoids mixing sync FS calls into async code.dash-spv/src/sync/masternodes/embedded_data.rs (1)
9-15: Stabilizeinclude_bytes!paths to reduce refactor breakage.
The deep relative paths will keep breaking as modules move; consider anchoring viaenv!("CARGO_MANIFEST_DIR")(or moving these artifacts into this crate) to make builds less fragile.dash-spv-ffi/examples/basic_usage.c (1)
6-10: Improve example error reporting by printingdash_spv_ffi_get_last_error()on failure.
Proposed tweak:- if (dash_spv_ffi_init_logging("info", true, NULL, 0) != 0) { - fprintf(stderr, "Failed to initialize logging\n"); + if (dash_spv_ffi_init_logging("info", true, NULL, 0) != 0) { + fprintf(stderr, "Failed to initialize logging: %s\n", dash_spv_ffi_get_last_error()); return 1; }Based on learnings, check
dash_spv_ffi_get_last_error()when debugging FFI issues.dash-spv/src/storage/disk/mod.rs (1)
22-28: Module visibility/layout update looks good; consider documenting lock + thread-safety expectations.
Given the lockfile/atomic-write design and theStorageManagerAPI shape (Send+Sync with&mut selfmethods), a short note in module docs about the intended concurrency model would reduce confusion. Based on learnings, clarify thread-safety expectations/rationale.dash-spv/src/storage/disk/io.rs (1)
32-69: Solid atomic write implementation with crash resilience.The temp-file → sync → rename pattern is correctly implemented. Error handling properly cleans up temp files on both write and rename failures.
Minor consideration: For full POSIX durability guarantees on some filesystems (ext4 with default mount options), you may want to also
fsyncthe parent directory after the rename to ensure the directory entry is persisted. This is a defense-in-depth measure for edge cases:// Optional: sync parent directory for full durability if let Some(parent) = path.parent() { if let Ok(dir) = tokio::fs::File::open(parent).await { let _ = dir.sync_all().await; } }This is a nice-to-have rather than essential, as the current implementation handles the common crash scenarios correctly.
dash-spv/src/client/core.rs (1)
23-24: Clarify the “Arc<Mutex>” doc to avoid ambiguity about whichMutexyou mean.
The module usestokio::sync::Mutex(Line 12), but the comment (Line 129-131) reads likestd::sync::Mutex. Consider spelling it explicitly (e.g.,Arc<tokio::sync::Mutex<SyncManager<...>>>) to prevent architectural confusion.Also applies to: 129-135
dash-spv/src/sync/message_handlers.rs (1)
585-646: Consider consolidatingstats.write().awaitto avoid double write-locking in the hot path.
Right now you lock once forfilters_received(Line 627-630) and again forfilters_matched(Line 635-637). You can update both under one lock without holding it across any.await.- { - let mut stats_lock = self.stats.write().await; - stats_lock.filters_received += 1; - stats_lock.last_filter_received_time = Some(std::time::Instant::now()); - } + let mut filters_matched_inc = false; if matches { - // Update filter match statistics - { - let mut stats = self.stats.write().await; - stats.filters_matched += 1; - } + filters_matched_inc = true; tracing::info!("🎯 Filter match found! Requesting block {}", cfilter.block_hash); // Request the full block let inv = Inventory::Block(cfilter.block_hash); network .send_message(NetworkMessage::GetData(vec![inv])) .await .map_err(|e| SyncError::Network(format!("Failed to request block: {}", e)))?; } + + { + let mut stats = self.stats.write().await; + stats.filters_received += 1; + stats.last_filter_received_time = Some(std::time::Instant::now()); + if filters_matched_inc { + stats.filters_matched += 1; + } + }dash-spv/src/storage/memory.rs (1)
299-308: Prefer saturating add for absolute height math inget_header_height_by_hash().This avoids any theoretical overflow if base heights ever approach
u32::MAX.-Ok(Some(self.sync_base_height() + storage_index)) +Ok(Some(self.sync_base_height().saturating_add(storage_index)))dash-spv/src/sync/phase_execution.rs (1)
71-87: Drop or use_safe_height(currently dead computation).The computed
_safe_heightisn’t used, so it’s either leftover debug or missing logic to actually clamp subsequent operations.dash-spv/src/client/sync_coordinator.rs (1)
942-957: Consider movingvalidate_headers()CPU work off the async executor thread.Even though Rayon uses its own pool, the async task still blocks until completion (and this runs repeatedly for many batches). Wrapping the validation in
tokio::task::spawn_blockingwould keep the runtime more responsive during restores.dash-spv/src/sync/filters/download.rs (1)
112-160: Potential hot-path contention:mark_filter_received+is_request_completescans ranges under lock.
is_request_complete()locksreceived_filter_heightsand doescontains()for every height in[start..=end];mark_filter_received()repeats this for each active range. If ranges are large and filters arrive frequently, this can become a bottleneck. Consider tracking remaining-per-range counters (decrement on receipt) or storing per-range bitsets to avoid repeated full scans.dash-spv/src/storage/disk/state.rs (1)
51-99: Guardload_chain_state()againstsync_base_height > tip_heightto avoid silent “empty state”.If
state.sync_base_heightis ahead of the on-disk tip (corrupt/mismatched files),load_headers(range_start..tip+1)becomes an empty range and you’ll return a partially-initializedChainStatewithout signalling inconsistency. Consider:
- If
range_start > tip_height, return an explicitStorageError(or clamp + warn), and- Similar guard for filter headers.
dash-spv/src/sync/filters/headers.rs (1)
785-792: Consider resettingnext_cfheader_height_to_processwhen clearing CFHeaders sync state.
clear_filter_header_sync_state()clears queues/maps but leavesnext_cfheader_height_to_processas-is. Not strictly wrong (it’s reinitialized instart_sync_filter_headers()), but resetting it here would reduce “stale state” surprises when future refactors add alternate entrypoints.dash-spv/src/logging.rs (2)
146-249: Rotation/cleanup: tighten a couple edge cases (max_files semantics + TOCTOU)
cleanup_old_logs(..., max_files)currently treatsmax_filesas “archived logs to keep” (good), but the function name/docs could be explicit that it does not includerun.log.- The
.exists()+.find(|p| !p.exists())pattern inrotate_previous_log()is inherently TOCTOU; if you want to harden it, looprenameattempts instead of pre-checking existence.No must-fix correctness bug seen in this block.
251-629: Tests: potential flakiness from mtime resolution +sleep()Several tests depend on
std::thread::sleep(...)to ensure distinct mtimes. On filesystems with coarse timestamp resolution, this can still collapse mtimes and reduce coverage of the “oldest first” behavior. If this bites CI, consider setting mtimes explicitly (e.g., via a dev-dependency likefiletime) instead of sleeping.dash-spv/src/sync/headers/manager.rs (1)
104-137: Checkpoint/base-height cache: good direction; watch division-by-zero-ish logging
- Base-height-derived
is_synced_from_checkpoint()is simpler and matches thetip_height()pattern (nice).- Minor: the “headers/sec” log can emit
NaN/infwhenelapsed.as_secs_f64()is ~0 orloaded_count == 0. Consider guarding the rate calculation for cleaner logs.Also applies to: 944-955
dash-spv/src/sync/manager.rs (2)
160-176:wallet_birth_height_hint()holds aRwLockguard across.awaitThis can cause avoidable contention (and can deadlock if
earliest_required_height()tries to re-enter codepaths needing the same lock). If feasible, adjustWalletInterface/ call shape so the async work doesn’t require holding the lock the whole time. Based on learnings, consider documenting this tradeoff explicitly if it’s intentional.
250-262:reset_pending_requests()drops header reset errors
let _ = self.header_sync.reset_pending_requests();silently ignores failures. If this can fail in meaningful ways, consider surfacing it (or at least logging atwarn!) so restart issues aren’t masked.
pauldelucia
force-pushed
the
feat/platform-addresses
branch
2 times, most recently
from
4475aad to
8e331a2
Compare
Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
key-wallet/src/transaction_checking/account_checker.rs (1)
648-656: Consider making the no-match policy explicit in this method.While
check_account_type()correctly returns an empty vector forPlatformPayment(line 387-392), this method will still construct a match if called directly. Since Platform addresses should never match Core chain transactions by design, consider adding an early return forPlatformPaymentaccounts before the address checking logic, similar to how the type is handled incheck_account_type().Example:
pub fn check_transaction_for_match( &self, tx: &Transaction, index: Option<u32>, ) -> Option<AccountMatch> { + // Platform Payment addresses are not used in Core chain transactions + if matches!(self.account_type, ManagedAccountType::PlatformPayment { .. }) { + return None; + } + // Check regular outputs let mut involved_receive_addresses = Vec::new();This would make the no-match policy explicit and prevent confusion if this public method is called directly.
key-wallet-ffi/src/types.rs (1)
424-427: LGTM! Correct mapping of PlatformPayment indices to FFI tuple.The
from_account_typemapping correctly returns bothaccount(as primary index) andkey_class(as optional secondary index) using the tuple format(FFIAccountType, u32, Option<u32>). This is consistent with how other multi-index account types likeIdentityTopUpuse the same tuple structure.Consider adding a test for the
from_account_typedirection.While the panic behavior is tested at lines 450-456, there's no test verifying that
from_account_typecorrectly maps aPlatformPaymentaccount type to the expected FFI tuple with both indices. Add a test to verify the tuple structure:#[test] fn test_platform_payment_from_account_type() { let account_type = key_wallet::AccountType::PlatformPayment { account: 5, key_class: 3, }; let (ffi_type, primary_index, secondary_index) = FFIAccountType::from_account_type(&account_type); assert_eq!(ffi_type, FFIAccountType::PlatformPayment); assert_eq!(primary_index, 5); assert_eq!(secondary_index, Some(3)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
key-wallet-ffi/include/key_wallet_ffi.h(1 hunks)key-wallet-ffi/src/transaction_checking.rs(1 hunks)key-wallet-ffi/src/types.rs(4 hunks)key-wallet/src/transaction_checking/account_checker.rs(6 hunks)key-wallet/src/transaction_checking/wallet_checker.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-ffi/src/transaction_checking.rs
- key-wallet-ffi/include/key_wallet_ffi.h
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
key-wallet/**/transaction_checking/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/transaction_checking/**/*.rs: UseTransactionRouter::classify_transaction()to determine transaction type andget_relevant_account_types()to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
In SPV sync integration, usewallet_info.check_transaction()with appropriateTransactionContextand update wallet state atomically only whenis_relevantflag is true
Files:
key-wallet/src/transaction_checking/account_checker.rs
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Never panic in library code; useResult<T>for all fallible operations with customErrortype variants and provide context in error messages
Use?operator for error propagation in Rust code to maintain clean error handling patterns
Files:
key-wallet/src/transaction_checking/account_checker.rs
**/*-ffi/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Careful handling of memory safety at FFI boundaries in Rust code
Files:
key-wallet-ffi/src/types.rs
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Support future enhancements: prepare architecture for Schnorr/Taproot support, descriptor wallets, native multisig account types, and Lightning Network payment channel key management
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : Use `TransactionRouter::classify_transaction()` to determine transaction type and `get_relevant_account_types()` to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
key-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
key-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true
Applied to files:
key-wallet/src/transaction_checking/account_checker.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet/src/transaction_checking/account_checker.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Never panic in library code; use `Result<T>` for all fallible operations with custom `Error` type variants and provide context in error messages
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*ffi*.rs : For FFI bindings, expose safe C interfaces with appropriate memory management and use #[no_mangle] for public FFI functions
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Define traits for abstractions when adding new features, implement concrete types following existing patterns, add unit and integration tests, and update error types in `error.rs`
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
key-wallet-ffi/src/types.rs
🧬 Code graph analysis (2)
key-wallet/src/transaction_checking/account_checker.rs (3)
key-wallet/src/managed_account/managed_account_collection.rs (1)
new(58-75)key-wallet/src/account/account_collection.rs (1)
new(81-100)key-wallet/src/managed_account/mod.rs (1)
new(63-73)
key-wallet-ffi/src/types.rs (1)
key-wallet/src/derivation.rs (1)
account(230-233)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: SPV Components Tests
- GitHub Check: Core Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (7)
key-wallet/src/transaction_checking/account_checker.rs (4)
106-112: LGTM: PlatformPayment variant correctly added.The enum variant is properly structured with the necessary fields for DIP-17 support (
account_index,key_class,involved_addresses). The comment clearly documents that these addresses are not used in Core chain transactions, which aligns with the PR's stated design.
170-173: LGTM: Helper methods correctly extended.The
all_involved_addresses()andaccount_index()methods are properly extended to handle thePlatformPaymentvariant, following the established pattern for single-pool account types.Also applies to: 204-207
254-256: LGTM: Routing mapping correctly implemented.The
to_account_type_to_check()method properly mapsPlatformPaymenttoAccountTypeToCheck::PlatformPayment, completing the transaction routing chain.
387-392: LGTM: Correctly implements no-match policy for Platform addresses.The implementation correctly returns an empty vector for
PlatformPayment, as these addresses are exclusively for Platform-side payments and should never match Core chain transactions. The comment clearly documents this design decision.key-wallet-ffi/src/types.rs (3)
269-270: LGTM! Enum variant follows established pattern.The
PlatformPaymentvariant is correctly added with sequential value 13 and appropriate documentation referencing DIP-17 and the derivation path.
332-339: LGTM! Panic correctly prevents unsupported conversion.The panic follows the established pattern for FFI account types that cannot be converted due to API limitations. The message clearly explains that
accountandkey_classindices cannot be passed through the currentto_account_type(index: u32)signature, which only accepts a single index parameter. The corresponding panic test has been added at lines 450-456 per previous review feedback.
450-456: LGTM! Panic test correctly verifies unsupported conversion.The test properly verifies that attempting to convert
FFIAccountType::PlatformPaymenttoAccountTypepanics as expected. This follows the established pattern for DashPay panic tests and addresses the previous review feedback requesting this test.
Summary
m/9'/coin_type'/17'/account'/key_class'/indexDetails
This PR adds support for DIP-17 Platform Payment key derivation to the key-wallet crate. The address encoding (DIP-18 bech32m with
dashevo/tdashevoHRP) is handled by the Platform repo.Changes
accountandkey_classindicesPLATFORM_PAYMENT_PATH_MAINNETandPLATFORM_PAYMENT_PATH_TESTNETFFIAccountType::PlatformPaymentwith conversion functionsNotes
Test plan
cargo test -p key-walletpassesSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.