refactor: Single network Wallet and ManagedWalletInfo#271
refactor: Single network Wallet and ManagedWalletInfo#271xdustinface merged 1 commit intov0.41-devfrom
Wallet and ManagedWalletInfo#271Conversation
WalkthroughThis PR removes per-call network parameters throughout wallet, manager, and FFI layers, switching APIs from multi-network/slice usage to a single-network model and flattening per-network collections into unified account collections tied to each wallet/managed-wallet. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs (2)
127-160: Add network consistency validation.Similar to
add_managed_account, this method accepts aWalletparameter but doesn't validate thatwallet.networkmatchesself.networkbefore adding the BLS account. This could allow mixing mainnet and testnet accounts.Apply this diff to add the validation:
fn add_managed_bls_account( &mut self, wallet: &Wallet, account_type: AccountType, ) -> Result<()> { // Validate account type if !matches!(account_type, AccountType::ProviderOperatorKeys) { return Err(Error::InvalidParameter( "BLS accounts can only be ProviderOperatorKeys".to_string(), )); } + + // Validate network consistency + if wallet.network != self.network { + return Err(Error::InvalidParameter(format!( + "Network mismatch: wallet network {:?} does not match managed wallet info network {:?}", + wallet.network, self.network + ))); + } let bls_account = wallet.accounts.bls_account_of_type(account_type).ok_or_else(|| {
236-270: Add network consistency validation.Similar to
add_managed_account, this method accepts aWalletparameter but doesn't validate thatwallet.networkmatchesself.networkbefore adding the EdDSA account. This could allow mixing mainnet and testnet accounts.Apply this diff to add the validation:
fn add_managed_eddsa_account( &mut self, wallet: &Wallet, account_type: AccountType, ) -> Result<()> { // Validate account type if !matches!(account_type, AccountType::ProviderPlatformKeys) { return Err(Error::InvalidParameter( "EdDSA accounts can only be ProviderPlatformKeys".to_string(), )); } + + // Validate network consistency + if wallet.network != self.network { + return Err(Error::InvalidParameter(format!( + "Network mismatch: wallet network {:?} does not match managed wallet info network {:?}", + wallet.network, self.network + ))); + } let eddsa_account =key-wallet-ffi/src/transaction.rs (1)
410-418: Potential undefined behavior in memory allocation.The memory allocation pattern is unsafe:
let bytes = Vec::<u8>::with_capacity(size).into_boxed_slice(); let tx_bytes = Box::into_raw(bytes) as *mut u8; ptr::copy_nonoverlapping(serialized.as_ptr(), tx_bytes, size);
Vec::with_capacity(size).into_boxed_slice()creates a boxed slice with length 0 (notsize), so copyingsizebytes into it writes beyond the allocated length, which is undefined behavior.Apply this diff to fix the allocation:
- // Allocate memory for the result - let bytes = Vec::<u8>::with_capacity(size).into_boxed_slice(); - let tx_bytes = Box::into_raw(bytes) as *mut u8; - - // Copy the serialized transaction - ptr::copy_nonoverlapping(serialized.as_ptr(), tx_bytes, size); + // Allocate memory for the result and copy the serialized transaction + let mut bytes = serialized.into_boxed_slice(); + let tx_bytes = bytes.as_mut_ptr(); + std::mem::forget(bytes);Or alternatively:
- let bytes = Vec::<u8>::with_capacity(size).into_boxed_slice(); - let tx_bytes = Box::into_raw(bytes) as *mut u8; - - // Copy the serialized transaction - ptr::copy_nonoverlapping(serialized.as_ptr(), tx_bytes, size); + let tx_bytes = Box::into_raw(serialized.into_boxed_slice()) as *mut u8;key-wallet-ffi/src/managed_wallet.rs (1)
944-944: Inconsistent memory deallocation for address arrays.At line 944,
libc::freeis used to free the address array, but at line 978, the propercrate::address::address_array_freeis used. Since the addresses are allocated viaBox::into_raw(boxed_slice)(line 371), you should use the corresponding Rust deallocation or the provided FFI function consistently.Consider using the same deallocation approach for consistency:
- libc::free(addresses_out as *mut libc::c_void); + crate::address::address_array_free(addresses_out, count_out);key-wallet-manager/src/wallet_manager/mod.rs (2)
375-405: Network mismatch risk when importing from xprv
import_wallet_from_extended_priv_keyignores thenetworkargument when constructing theWallet(viaWallet::from_extended_key) but later uses the samenetworkvalue to pick the network state forbirth_height:let wallet = Wallet::from_extended_key(extended_priv_key, account_creation_options)?; let mut managed_info = T::from_wallet(&wallet); managed_info.set_birth_height(Some(self.get_or_create_network_state(network).current_height));If the xprv encodes a different network than the caller-provided
network, you’ll end up with a wallet whosewallet.networkdisagrees with theNetworkStateit’s tied to, which violates the “single-network wallet” invariant and the “no mainnet/testnet mixing” guideline.Consider deriving the network from
wallet.networkinstead, and optionally rejecting mismatches:- let mut managed_info = T::from_wallet(&wallet); - managed_info - .set_birth_height(Some(self.get_or_create_network_state(network).current_height)); + let mut managed_info = T::from_wallet(&wallet); + let wallet_network = wallet.network; + if wallet_network != network { + return Err(WalletError::InvalidNetwork); + } + managed_info.set_birth_height(Some( + self.get_or_create_network_state(wallet_network).current_height, + ));Or drop the
networkparameter entirely here and always usewallet.networkfor state lookups.Based on learnings, we should enforce network consistency when importing keys.
423-456: Same network-consistency concern when importing from xpub
import_wallet_from_xpubalso usesnetworkonly forget_or_create_network_state(network)whileWallet::from_xpubinfers the actual network from the xpub andAccountCollection:let wallet = Wallet::from_xpub(extended_pub_key, accounts, can_sign_externally)?; let mut managed_info = T::from_wallet(&wallet); managed_info.set_birth_height(Some(self.get_or_create_network_state(network).current_height));This has the same mismatch risk as the xprv import path. Please either:
- Validate
wallet.network == networkand error on mismatch, and/or- Use
wallet.networkwhen selecting theNetworkState.A similar diff as suggested for the xprv path would apply here.
Based on learnings, we should not allow silent mainnet/testnet mixing.
key-wallet-ffi/FFI_API.md (6)
870-877: Outdated documentation: remove stale network parameter references.The description for
managed_wallet_get_account(line 874-877) still referencesnetworkmust specify exactly one network" in the safety section, but the function signature (line 870) shows nonetworkparameter. This is inconsistent and misleading.- `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `network` must specify exactly one network - The caller must ensure all pointers remain valid for the duration of this call - The returned account must be freed with `managed_account_free` when no longer needed + `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - The wallet must have a single, well-defined network (not a multi-network slice) - The caller must ensure all pointers remain valid for the duration of this call - The returned account must be freed with `managed_account_free` when no longer needed
886-893: Inconsistent documentation: safety notes reference removed network parameter.Line 890 in
managed_wallet_get_account_collectiondescription states: "wallet_idmust be a valid pointer to a 32-byte wallet ID" but the safety section still says "network" must specify exactly one network" (line 893), yet nonetworkparameter appears in the signature (line 886).- `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `error` must be a valid pointer to an FFIError structure or null - The returned pointer must be freed with `managed_account_collection_free` when no longer needed + `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID (wallet is single-network, network determined by wallet) - `error` must be a valid pointer to an FFIError structure or null - The returned pointer must be freed with `managed_account_collection_free` when no longer needed
902-909: Documentation mismatch: stale network parameter in safety notes.
managed_wallet_get_account_count(line 902) signature has nonetworkparameter, but the safety section (lines 908) incorrectly states "networkmust specify exactly one network". This contradicts the function signature and creates confusion for FFI consumers.- `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `network` must specify exactly one network - `error` must be a valid pointer to an FFIError structure or null - The caller must ensure all pointers remain valid for the duration of this call + `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `error` must be a valid pointer to an FFIError structure or null - The caller must ensure all pointers remain valid for the duration of this call
1046-1053: Stale network parameter reference in documentation.
managed_wallet_get_top_up_account_with_registration_index(line 1046) signature lacks anetworkparameter, but the safety section (lines 1052) references "networkmust specify exactly one network". Remove this obsolete reference.- `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `network` must specify exactly one network - The caller must ensure all pointers remain valid for the duration of this call - The returned account must be freed with `managed_account_free` when no longer needed + `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - The caller must ensure all pointers remain valid for the duration of this call - The returned account must be freed with `managed_account_free` when no longer needed
1206-1213: Documentation still references removed network parameter.
wallet_build_and_sign_transaction(line 1206) signature shows nonetworkparameter, yet the safety section (line 1212) incorrectly states "networkmust be a valid FFINetwork". This is a stale reference that contradicts the refactored signature.- `managed_wallet` must be a valid pointer to an FFIManagedWalletInfo - `wallet` must be a valid pointer to an FFIWallet - `network` must be a valid FFINetwork - `outputs` must be a valid pointer to an array of FFITxOutput with at least `outputs_count` elements - `tx_bytes_out` must be a valid pointer to store the transaction bytes pointer - `tx_len_out` must be a valid pointer to store the transaction length - `error` must be a valid pointer to an FFIError - The returned transaction bytes must be freed with `transaction_bytes_free` + `managed_wallet` must be a valid pointer to an FFIManagedWalletInfo (determines network context) - `wallet` must be a valid pointer to an FFIWallet with matching network - `outputs` must be a valid pointer to an array of FFITxOutput with at least `outputs_count` elements - `tx_bytes_out` must be a valid pointer to store the transaction bytes pointer - `tx_len_out` must be a valid pointer to store the transaction length - `error` must be a valid pointer to an FFIError - The returned transaction bytes must be freed with `transaction_bytes_free`
1238-1245: Safety documentation contradicts signature for wallet_check_transaction.Line 1238 shows
wallet_check_transactionwith no explicitnetworkparameter in the signature, yet the safety section (line 1245) lists outdated guidance about inputs/outputs that doesn't match the refactored signature. The description needs updating to reflect single-network context.key-wallet-ffi/include/key_wallet_ffi.h (4)
895-903: Docs mentionFFINetwork::NoNetworksalthough the APIs returnFFINetworksFor
account_get_network,bls_account_get_network, andeddsa_account_get_networkthe return type isFFINetworks, but the comments still say “Returns FFINetwork::NoNetworks if the account is null”. That enum/variant does not exist; the intended value is presumablyFFINetworks::NO_NETWORKS. Please update the Rust doc comments so cbindgen regenerates accurate header docs.Also applies to: 949-956, 1006-1013
1062-1072: Updatewallet_get_account_collectiondocs to remove stale “specific network” wordingThe comment still says “Get account collection for a specific network from wallet”, but the function no longer takes a network parameter and the wallet is single-network. Please adjust the Rust doc comment (so this header regenerates) to describe that it returns the account collection for the wallet’s configured network, not for an arbitrary per-call network.
Based on learnings, keeping docs aligned with the single-network wallet model helps prevent accidental mainnet/testnet mixing.
2363-2401: Managed wallet account APIs still document anetworkparameter that no longer existsThe safety docs for:
managed_wallet_get_accountmanaged_wallet_get_top_up_account_with_registration_indexmanaged_wallet_get_account_countstill list a
networkargument (e.g., “networkmust specify exactly one network”), but the corresponding FFI signatures no longer take any network parameter. This is confusing for FFI callers. Please update the Rust doc comments for these functions and regenerate the header so the documented parameter list matches the actual ABI.Based on learnings, making it explicit that these APIs derive network from the wallet/manager context reinforces single-network invariants.
Also applies to: 2560-2574
3247-3268:wallet_build_and_sign_transactiondocs still reference anetworkparameter that was removedThe safety section here says “
networkmust be a valid FFINetwork”, but the function signature no longer includes anynetworkparameter and instead relies onmanaged_wallet/walletcontext. This stale documentation can mislead FFI users. Please fix the Rust doc comment and regenerate the header so the documented parameters match the actual function prototype.Based on learnings, it’s important to highlight that the wallet’s own network now drives transaction building/signing.
🧹 Nitpick comments (27)
key-wallet/src/wallet/managed_wallet_info/managed_account_operations.rs (2)
22-22: add_managed_account signature now correctly relies onWalletfor network contextDropping the explicit network parameter here matches the single‑network wallet design and keeps the API cleaner; implementations should consistently rely on
wallet.network(and the managed wallet’s own network field) for validation and error messages to maintain mainnet/testnet separation.
70-72: BLS managed account API is consistent with the new single‑network modelThe updated
add_managed_bls_accountsignature (no explicit network, gated byblsfeature) is consistent with the rest of the trait and the refactor; as with the non‑BLS path, ensure implementations enforce that the underlying BLS account and the managed wallet share the same network rather than accepting any externally constructed account.key-wallet/src/address_metadata_tests.rs (3)
29-46: Consider adding mainnet test coverage and network consistency validation.The test correctly uses the new single-network API, but only covers
Network::Testnet. Based on learnings, tests should cover both mainnet and testnet configurations. Additionally, consider adding an assertion to verify that the created account uses the same network as the wallet to prevent mainnet/testnet mixing.Consider adding a companion test:
#[test] fn test_basic_wallet_creation_mainnet() { let wallet = Wallet::new_random( Network::Dash, crate::wallet::initialization::WalletAccountCreationOptions::Default, ) .unwrap(); assert!(wallet.get_bip44_account(0).is_some()); let account = wallet.get_bip44_account(0).unwrap(); // Verify network consistency assert_eq!(account.network(), Network::Dash); match &account.account_type { AccountType::Standard { index, .. } => assert_eq!(*index, 0), _ => panic!("Expected Standard account type"), } }
82-91: Verify type compatibility in loop index.The loop variable
ifrom range0..3is inferred as a numeric type (likelyi32orusize), butget_bip44_accountexpectsu32based on the signature. While Rust's type inference may resolve this if the code compiles, consider being explicit for clarity.Consider making the type explicit:
- for i in 0..3 { + for i in 0u32..3u32 { let account = wallet.get_bip44_account(i).unwrap();Alternatively, verify that this compiles without warnings. If it does, the current code is acceptable.
12-23: Address metadata tests are deferred.The TODO comments clearly document the missing test coverage for address metadata functionality that now resides in
ManagedAccount. This is reasonable for a refactor-focused PR, but the missing tests should be tracked.Would you like me to open an issue to track the implementation of these missing tests:
- test_address_labeling
- test_address_pool_info
- test_address_usage_tracking
- test_gap_limit_handling
- test_address_metadata_persistence
- test_coinjoin_address_pools
- test_change_address_handling
- test_concurrent_address_access
- test_address_metadata_updates
- test_pool_statistics
key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs (1)
356-421: Add test coverage for network mismatch validation.Once the network validation is added to
add_managed_accountand related methods, consider adding a test case that verifies the proper error is returned when attempting to add an account from a wallet with a different network than theManagedWalletInfo.Example test case to add:
#[test] fn test_add_managed_account_network_mismatch() { // Create a mainnet wallet let mut mainnet_wallet = Wallet::new_random( Network::Mainnet, crate::wallet::initialization::WalletAccountCreationOptions::None, ) .unwrap(); mainnet_wallet .add_account( AccountType::Standard { index: 0, standard_account_type: crate::account::StandardAccountType::BIP44Account, }, None, ) .unwrap(); // Create testnet managed wallet info let mut managed_info = ManagedWalletInfo::new(Network::Testnet, mainnet_wallet.wallet_id); let account_type = AccountType::Standard { index: 0, standard_account_type: crate::account::StandardAccountType::BIP44Account, }; // Attempt to add mainnet account to testnet managed info - should fail let result = managed_info.add_managed_account(&mainnet_wallet, account_type); assert!(result.is_err(), "Should fail due to network mismatch"); }key-wallet/src/tests/wallet_tests.rs (1)
340-343: Consider adding mainnet test coverage.Per coding guidelines, tests should cover both mainnet and testnet configurations. This file tests only on
Network::Testnet. Consider adding a companion test that verifies wallet behavior onNetwork::Dash(mainnet), or at minimum a parameterized test, to ensure network-specific logic (e.g., address prefixes) is exercised.key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
45-45: Unused_networkparameter - consider removing in follow-up.The parameter is now unused since account access no longer requires network context. While keeping it may be intentional for API stability during the refactor, consider removing it in a follow-up PR to avoid dead code, or document why it's retained.
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
241-283: Consider extracting repeatedTransactionRecordconstruction.The same
TransactionRecord::new_confirmed(...)pattern is repeated three times (for BIP44, BIP32, and CoinJoin accounts). Consider extracting this to a helper closure or method to reduce duplication.fn process_matured_transactions(&mut self, current_height: u32) -> Vec<ImmatureTransaction> { let matured = self.immature_transactions.remove_matured(current_height); + // Helper to create transaction record and insert into account + let insert_tx_record = |account: &mut ManagedAccount, tx: &ImmatureTransaction| { + let tx_record = TransactionRecord::new_confirmed( + tx.transaction.clone(), + tx.height, + tx.block_hash, + tx.timestamp, + tx.total_received as i64, + false, + ); + account.transactions.insert(tx.txid, tx_record); + }; + // Update accounts with matured transactions for tx in &matured { // Process BIP44 accounts for &index in &tx.affected_accounts.bip44_accounts { if let Some(account) = self.accounts.standard_bip44_accounts.get_mut(&index) { - let tx_record = TransactionRecord::new_confirmed( - tx.transaction.clone(), - tx.height, - tx.block_hash, - tx.timestamp, - tx.total_received as i64, - false, - ); - account.transactions.insert(tx.txid, tx_record); + insert_tx_record(account, tx); } } // ... similar for BIP32 and CoinJoin }key-wallet/src/wallet_comprehensive_tests.rs (1)
23-176: Consider adding mainnet configuration tests.Per coding guidelines, tests should cover both mainnet and testnet configurations. All tests currently use only
Network::Testnet. Consider adding parallel tests or parameterizing existing tests to verify behavior withNetwork::Dash(mainnet) to ensure network consistency validation works correctly across networks.key-wallet/src/wallet/managed_wallet_info/helpers.rs (1)
85-93: TopUp "first" semantics depend on BTreeMap ordering.
values().next()returns the account with the lowestregistration_indexkey due to BTreeMap's sorted iteration. This is likely the intended behavior but differs from the explicitindex=0pattern used for BIP44/BIP32/CoinJoin. The doc comment should clarify this returns the account with the lowest registration index, not necessarily registration index 0.Consider clarifying the doc comment:
- /// Get the first TopUp managed account + /// Get the TopUp managed account with the lowest registration index pub fn first_topup_managed_account(&self) -> Option<&ManagedAccount> {key-wallet/src/wallet/stats.rs (1)
32-33: Consider using a consistent accessor pattern for coinjoin_enabled_accounts.Line 32 uses
self.accounts.count()(a method), while line 33 directly accessesself.accounts.coinjoin_accounts.len()(field access). For consistency, consider whetherAccountCollectionshould provide acoinjoin_account_count()method, similar to howcount()is provided.- let coinjoin_enabled_accounts: usize = self.accounts.coinjoin_accounts.len(); + let coinjoin_enabled_accounts: usize = self.accounts.coinjoin_account_count();This would require adding a helper method to
AccountCollection:pub fn coinjoin_account_count(&self) -> usize { self.coinjoin_accounts.len() }key-wallet-ffi/src/transaction_checking.rs (1)
211-213: Network removal from check_transaction is fine; consider hardening tokio handle usageDropping the network argument from
check_transactionand relying on the wallet/managed-wallet’s internal network context is consistent with the single-network refactor. One thing to consider (not new to this PR):tokio::runtime::Handle::current()will panic if this is ever called outside a Tokio runtime, which is undesirable at an FFI boundary; usingHandle::try_current()and returning anFFIErrorinstead would make this more robust.// Sketch of a safer pattern let handle = match tokio::runtime::Handle::try_current() { Ok(h) => h, Err(_) => { FFIError::set_error( error, FFIErrorCode::InternalError, "No Tokio runtime in current thread".to_string(), ); return false; } }; let check_result = handle.block_on(managed_wallet.check_transaction(&tx, context, wallet_mut, update_state));key-wallet-ffi/tests/test_valid_addr.rs (1)
13-17: Test now uses single-network wallet API; consider tightening assertionsCreating the wallet with
Wallet::from_mnemonic(..., Network::Testnet, ...)and usingwallet.get_bip44_account(0)without a network argument is consistent with the refactored single-network API. However, this test still only logs address validation results and never asserts on them, so it won’t catch regressions in address derivation or network checks. It would be stronger to assert thatget_bip44_account(0)returnsSome(_)and thatrequire_network(Network::Testnet)succeeds.- if let Some(account) = wallet.get_bip44_account(0) { + let account = wallet.get_bip44_account(0).expect("BIP44 account 0 should exist"); @@ - match parsed.require_network(dashcore::Network::Testnet) { - Ok(_) => println!("✓ Address is valid for testnet"), - Err(e) => println!("✗ Address not valid for testnet: {}", e), - } + parsed + .require_network(dashcore::Network::Testnet) + .expect("Generated address must be valid for testnet");dash-spv/src/main.rs (1)
292-298: Hardcoded test mnemonic in CLI tool.The mnemonic phrase is hardcoded, which is appropriate for development/testing but should not be used in production. Consider making this configurable via CLI argument or environment variable for real-world usage.
key-wallet-ffi/src/account.rs (1)
563-571: Account count logic duplicated; consider centralizing in accounts typeThe count aggregation over
standard_bip44_accounts,standard_bip32_accounts,coinjoin_accounts,identity_registration, andidentity_topupmatches the pattern used elsewhere (e.g., inmanaged_wallet_get_account_count). To avoid divergence later, you might factor this into a helper on the underlying accounts collection and call it from both FFIs.key-wallet/src/wallet/managed_wallet_info/utxo.rs (1)
16-135: UTXO aggregation across BIP44/BIP32/CoinJoin looks correct; could share an internal iteratorThe new methods correctly:
- aggregate UTXOs from
standard_bip44_accounts,standard_bip32_accounts, andcoinjoin_accountsonly (matching the docs’ exclusion of identity/provider accounts),- derive spendable UTXOs and total value from that aggregate,
- and count UTXOs by summing each account’s
utxos.len().The accompanying tests using
ManagedWalletInfo::new(Network::Testnet, …)and a single BIP44 account validate the basic path. As a minor improvement, consider factoring a small internal iterator (e.g.,iter_all_utxos()) to avoid cloning/iterating multiple times acrossget_utxos,get_spendable_utxos,get_total_utxo_value, andget_utxo_count.Also applies to: 148-215
key-wallet-ffi/src/managed_account.rs (1)
739-751: Account counting duplicated with wallet FFI; consider a shared helper
managed_wallet_get_account_countnow mirrors the same counting logic aswallet_get_account_count, summing standard BIP44/BIP32, CoinJoin,identity_registration, andidentity_topup. To keep behavior in sync between “wallet” and “managed wallet” account counts, you might factor this into a method on the shared accounts collection type and call it from both FFI entry points.key-wallet-manager/tests/integration_test.rs (1)
70-108: Consider testing mainnet configuration for network coverage.The address generation test handles the
InvalidNetworkerror case appropriately, but per coding guidelines, tests should cover both mainnet and testnet configurations. Consider adding a similar test withNetwork::Dashto ensure the single-network refactor works across networks.key-wallet/src/tests/backup_restore_tests.rs (1)
218-228: Silent error suppression with.ok()may hide account creation failures.Using
.ok()onadd_account(lines 227 and 265) silently ignores any errors. While this may be intentional to allow duplicate account handling, consider usinglet _ = wallet.add_account(...);with a comment explaining the intent, or explicitly check for expected errors.for i in 0..2 { wallet .add_account( AccountType::Standard { index: i, standard_account_type: StandardAccountType::BIP44Account, }, None, ) - .ok(); + .ok(); // Ignore duplicate account errors when re-adding }key-wallet-manager/src/wallet_manager/mod.rs (3)
503-569: Check that caller-suppliednetworkalways matches wallet networks
check_transaction_in_all_walletstakes anetwork: Networkused only when recording theTransactionRecord:let network_state = self.get_or_create_network_state(network); network_state.transactions.insert(txid, record);while each
wallet_info.check_transaction(..., wallet, ...)now relies on wallet-internal network context.If a caller ever passes a
networkthat doesn’t match the wallets being iterated, transaction history will be stored under the wrongNetworkState. Consider:
- Documenting that
networkmust equal each wallet’swallet.networkfor which the tx is relevant, and/or- Deriving the network from any relevant wallet (e.g.,
wallet.network) before inserting intonetwork_states.Given SPV is already single-network per manager, this may be fine, but it’s worth double-checking call sites.
606-768: Receive address generation logic aligns with new account helpers
get_receive_addresscorrectly:
- Uses
managed_info.accounts_mut()(no network) andwallet.get_bip44_account/get_bip32_accounthelpers.- Supports all four
AccountTypePreferencevariants, with BIP32/BIP44 fallback where appropriate.- Optionally marks the derived address as used via
account.mark_address_used(address)on the corresponding managed account.This looks logically sound with the new single-network
Wallet+AccountCollectiondesign.If you want to reduce duplication, you could factor the repeated “try BIP44 then BIP32 / try BIP32 then BIP44” patterns into a small helper that returns
(address_opt, account_type_used), but that’s not required for correctness.
771-932: Change address generation mirrors receive address path correctly
get_change_addressmirrorsget_receive_addresswithnext_change_addressand identical preference/fallback semantics, and uses the same mark-as-used flow viamanaged_info.accounts_mut()andmark_address_used. This symmetry is good for maintainability and correctness under the new API.key-wallet-ffi/src/wallet.rs (1)
87-99: Be stricter when converting FFINetworks to a singleNetworkIn all three creation functions you now do:
let networks_rust = networks.parse_networks(); let network_rust = match networks_rust.first() { Some(n) => *n, None => { set_error("No network specified"); return ptr::null_mut(); } };This is correct for the single-network
Walletmodel, but if the caller passes multiple network flags, you silently pick the first. That’s ambiguous and may hide bugs at the FFI boundary.Consider rejecting “multiple networks set” explicitly:
- let network_rust = match networks_rust.first() { - Some(n) => *n, - None => { /* No network specified */ } - }; + if networks_rust.is_empty() { + FFIError::set_error(error, FFIErrorCode::InvalidInput, "No network specified".to_string()); + return ptr::null_mut(); + } + if networks_rust.len() > 1 { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Multiple networks not supported; specify a single network".to_string(), + ); + return ptr::null_mut(); + } + let network_rust = networks_rust[0];Same pattern applies to mnemonic, seed, and random wallet creation.
Also applies to: 205-217, 279-290
key-wallet/src/wallet/helper.rs (1)
475-495: Special-purpose account creation correctly encapsulated, no network leakage
create_special_purpose_accountsand its passphrase variant are now pure helpers onWalletthat:
- Add identity and provider accounts entirely via
add_account/add_account_with_passphrase(and BLS/EdDSA-specific helpers behind feature flags).- Do not take a network parameter and rely on the wallet’s internal state.
This matches the broader refactor and keeps network handling centralized at the wallet.
Also applies to: 498-517
key-wallet-ffi/FFI_API.md (1)
822-823: Stale safety documentation: remove references to removed network parameter.Line 822's signature shows
managed_walletandwalletparameters but nonetworkparameter. However, the safety notes below (line 828) don't mention that network context is now implicit in the wallet. The description should clarify that network is determined by the wallet's network, not by a function parameter.- `managed_wallet` must be a valid pointer to an FFIManagedWalletInfo - `wallet` must be a valid pointer to an FFIWallet (needed for address generation and DashPay queries) - `tx_bytes` must be a valid pointer to transaction bytes with at least `tx_len` bytes - `result_out` must be a valid pointer to store the result - `error` must be a valid pointer to an FFIError - The affected_accounts array in the result must be freed with `transaction_check_result_free` + `managed_wallet` must be a valid pointer to an FFIManagedWalletInfo (determines network context) - `wallet` must be a valid pointer to an FFIWallet with matching network (needed for address generation and DashPay queries) - `tx_bytes` must be a valid pointer to transaction bytes with at least `tx_len` bytes - `result_out` must be a valid pointer to store the result - `error` must be a valid pointer to an FFIError - The affected_accounts array in the result must be freed with `transaction_check_result_free`key-wallet-ffi/include/key_wallet_ffi.h (1)
3205-3214: Clarify that_networkparameters on tx build/sign helpers are ABI-only/deprecatedBoth
wallet_build_transactionandwallet_sign_transactionstill acceptFFINetworks _network, but the underscore name strongly suggests the parameter is now unused and that network scope comes from the wallet. It would help FFI consumers if the docs explicitly stated that_networkis deprecated/ignored (kept only for ABI compatibility) and that callers should rely on the wallet’s configured network instead.Based on learnings, this clarification reduces the chance that clients assume they can override or mix networks per call.
Also applies to: 3229-3236
xdustinface
force-pushed
the
refactor/single-network-wallet
branch
from
299a4c1 to
5511fc7
Compare
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/wallet/bip38.rs (1)
19-34: Inconsistent with single-network refactor: Remove network parameter.The
export_master_key_bip38method still accepts anetworkparameter, which is inconsistent with the PR's goal of making wallets single-network instances. Theexport_bip44_account_key_bip38method below has been correctly refactored to useself.networkinstead of accepting a network parameter.Apply this diff to align with the single-network design:
- pub fn export_master_key_bip38( - &self, - password: &str, - network: Network, - ) -> Result<Bip38EncryptedKey> { + pub fn export_master_key_bip38(&self, password: &str) -> Result<Bip38EncryptedKey> { if self.is_watch_only() { return Err(Error::InvalidParameter( "Cannot export private key from watch-only wallet".into(), )); } let root_key = self.root_extended_priv_key()?; let secret_key = root_key.root_private_key; - encrypt_private_key(&secret_key, password, true, network) + encrypt_private_key(&secret_key, password, true, self.network) }key-wallet-ffi/FFI_API.md (1)
1238-1245: Critical: Documentation does not match function signature.The safety section for
wallet_check_transactionreferences parameters that do not exist in the documented signature (e.g.,inputs_spent_out,addresses_used_out,new_balance_out,new_address_out,new_address_count_out). The actual signature shows different parameters:managed_wallet,wallet,tx_bytes,tx_len,context_type,block_height,block_hash,timestamp,update_state,result_out,error. The description and safety notes must be updated to match the actual function contract.Since this file is auto-generated, ensure the documentation generation script correctly reflects the actual Rust FFI function signatures, especially safety notes and parameter descriptions. The stale safety documentation could mislead users about which parameters are required and what they should contain.
♻️ Duplicate comments (1)
key-wallet-ffi/src/wallet_manager.rs (1)
186-197: Temporary single-network extraction is documented and guarded.The error handling for missing network is correct. The comment clearly indicates this is temporary pending a follow-up PR to remove
FFINetworksentirely. This aligns with the acknowledged plan from past reviews.
🧹 Nitpick comments (13)
key-wallet/src/wallet_comprehensive_tests.rs (1)
178-199: Good documentation of pending work.The TODO section clearly outlines tests requiring reimplementation with the Account/ManagedAccount architecture split. Based on learnings, this separation of immutable (Account) and mutable (ManagedAccount) concerns is the expected pattern.
Would you like me to help draft implementation stubs for any of these pending tests, or open an issue to track this work?
key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs (2)
60-65: API migration correctly applied.The wallet creation and account addition properly use the new single-network API. The
add_accountcall withNonewill use the wallet's network internally.Consider adding an explicit network consistency assertion after line 65 to align with the network validation learning:
wallet.add_account(account_type, None).expect("Failed to add account to wallet"); +assert_eq!(wallet.network(), network, "Wallet network should match expected network");Based on learnings, validating network consistency helps prevent mainnet/testnet mixing issues.
71-79: Correct usage of the new network-agnostic account access pattern.The direct field access to
wallet.accounts.identity_registrationand the parameter-freeidentity_registration_managed_account_mut()call properly reflect the single-network API.Consider verifying that the managed account's network matches the wallet's network after line 80:
let managed_account = managed_wallet_info .identity_registration_managed_account_mut() .expect("Failed to get identity registration managed account"); +assert_eq!(managed_account.network(), network, "Account network should match wallet network");Based on learnings, explicit network consistency checks help ensure the account uses the expected network for address generation.
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
68-113: Potential inconsistency in fallback account selection.The
PreferBIP44/PreferBIP32fallback logic is applied independently towallet_account(lines 77-86) andmanaged_account(lines 101-112). If the account maps differ betweenWalletandManagedWalletInfo(e.g., BIP44 exists in wallet but corresponding managed account wasn't created), the fallbacks could resolve to different account types.Consider validating that both lookups resolve to the same account type, or restructure to perform the preference resolution once and use the result for both lookups.
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs (1)
193-250: CoinJoin test uses hardcodedNetwork::Testnetin fallback address generation.Lines 235 and 247 use
Network::Testnetdirectly when generating fallback addresses. While this works since the wallet is created withNetwork::Testnet, consider deriving the network from the wallet or account to ensure consistency if this test is ever parameterized for multiple networks.Consider extracting the network from the wallet for future-proofing:
- Network::Testnet, + wallet.network(),Based on learnings, always validate network consistency: use the account's network type when creating addresses.
key-wallet-ffi/src/account.rs (1)
553-571: wallet_get_account_count manually aggregates specific account categories; consider centralizing in collectionThe count implementation sums only
standard_bip44_accounts,standard_bip32_accounts,coinjoin_accounts,identity_registration(boolean), andidentity_topup.len(). This mirrors common “spendable/special core” accounts, but omits other fields present inManagedAccountCollection(e.g.,identity_invitation,identity_topup_not_bound, provider keys, DashPay/platform accounts).If the intent is “total number of all account types”, you may want to:
- Delegate to a
ManagedAccountCollection::count()helper, or- Explicitly document and centralize which variants are included so new account types aren’t silently excluded in one place but not others.
Right now this works, but it’s easy to drift from the rest of the code if new account kinds are added.
key-wallet/src/wallet/managed_wallet_info/utxo.rs (2)
15-45: Core UTXO aggregation APIs are consistent with the new account layout
ManagedWalletInfo::get_utxos,get_spendable_utxos, andget_utxo_countnow operate directly onstandard_bip44_accounts,standard_bip32_accounts, andcoinjoin_accounts, explicitly excluding identity/provider accounts as documented. CloningUtxovalues to return owned data is reasonable here, andget_utxo_countcorrectly mirrors the same account subsets without the extra allocation cost ofget_utxos(). This matches the intended “spendable wallet funds” semantics under the single‑network, unified account collection.You could consider a small internal helper (e.g., an iterator over all included account
utxos) to reduce repetition betweenget_utxosandget_utxo_count, but it’s not strictly necessary.Also applies to: 97-116
47-95: get_utxos_by_account_type API is fine; consider avoiding raw string keysGrouping UTXOs by account type into a
BTreeMap<&'static str, …>with keys"bip44","bip32", and"coinjoin"matches the underlying collections and avoids empty entries. If this map is consumed beyond debugging, you might eventually want to:
- Replace the raw
&'static strkeys with an enum or constants to avoid typos and make refactoring safer.Current implementation is otherwise straightforward and correct.
key-wallet-ffi/src/managed_account.rs (1)
161-169: Verify intentional exclusion of DashPay/PlatformPayment from generic lookup.Lines 161-169 return
NoneforDashpayReceivingFunds,DashpayExternalAccount, andPlatformPaymentvariants. This is correct since these types require composite keys (identity IDs) and have dedicated functions (managed_wallet_get_dashpay_receiving_account, etc.), but the caller may not expect a generic lookup to silently fail for these types.Consider returning a more descriptive error or documenting this behavior in the function's doc comment.
key-wallet-ffi/src/address_pool.rs (2)
21-67: Consider consolidating duplicate account lookup logic.The
get_managed_account_by_typefunction duplicates the lookup logic frommanaged_account.rs(lines 130-170). While both files are in the same crate, this duplication increases maintenance burden.Consider extracting this to a shared helper module (e.g.,
utils.rsor within the account collection itself) to ensure consistency and reduce code duplication.
52-66: DashPay and PlatformPayment accounts returnNonesilently.Similar to the observation in
managed_account.rs, these account types returnNonewithout explanation. The comment on lines 58-59 and 64-65 clarifies they're "not currently persisted in ManagedAccountCollection," which is helpful but may not reach API consumers.key-wallet-ffi/include/key_wallet_ffi.h (1)
122-130: Clarify role ofFFINetworkvs existingFFINetworksin docs/API commentsIntroducing a single‑network
FFINetworkalongside the existing bitflagFFINetworksmakes the type separation clearer, but from a C/Swift caller’s perspective it’s easy to confuse the two. Consider tightening doc comments (in the Rust sources that generate this header) to explicitly state:
FFINetwork= single runtime network for a wallet/manager or derivation helper.FFINetworks= bitflag set used only at creation time and for legacy multi‑network APIs.This will help avoid accidental misuse at call sites without changing the ABI.
key-wallet/src/wallet/managed_wallet_info/helpers.rs (1)
5-210: Helpers correctly reflect single‑network account storage; consider avoiding extra allocation inaccount_countThe refactor of these helpers to drop the network parameter and read directly from
self.accounts(standard_bip44_accounts,standard_bip32_accounts,coinjoin_accounts, identity/provider fields, etc.) is consistent with the new single‑networkManagedWalletInfodesign. Thefirst_*and*_at_indexvariants are thin, readable wrappers and keep account access nicely centralized.One minor nit:
account_count(&self)currently callsself.accounts.all_accounts().len(), which allocates aVecof references just to count. Ifall_accounts()is used in hot paths, it could be worth adding a dedicatedself.accounts.total_len()(or similar) that computes the count over the underlying maps/options without allocating, and haveaccount_count()delegate to that.
all_managed_accounts(&self) -> Vec<&ManagedAccount>is fine as a convenience API when callers actually need the collection.
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
I understand the rationale of this change, even though I was leaning in the other direction. In the end what matters most is that you can maintain this code.
2 participants
This will make the
WalletandManagedWalletInfosingle network instances. I think its a cleaner API and makes sense because we pass it to theDashSpvClientwhich is also running only on one specific network so instead of having a shared wallet between clients that are running on different networks we will have one wallet per client when the refactoring is done. This PR does the main part but i will have some follow up PRs to clean up few other places.Summary by CodeRabbit
New Features & Breaking Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.