refactor: clean up key wallet some more #121
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors wallet creation to return WalletId and accept network slices; expands FFI for multi-network bitflags, typed account/collection handles, and serialized-wallet (bincode) export/import; removes FFI balance APIs; adds zeroization for sensitive types and broad test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant FFI as key-wallet-ffi
participant Mgr as WalletManager
participant Core as key-wallet
rect #e6f5ff
note right of App: Create wallet and get serialized bytes (bincode)
App->>FFI: wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes(mnemonic, pass, FFINetwork flags, opts, flags)
FFI->>Mgr: create_wallet_from_mnemonic_return_serialized_bytes(mnemonic, pass, &[Network], ...)
Mgr->>Core: Wallet::from_mnemonic(mnemonic, &[Network], opts)
Core-->>Mgr: Wallet + root pubkey
Mgr-->>FFI: (wallet_id, serialized_bytes)
FFI-->>App: wallet_id, bytes
end
rect #f0fff0
note right of App: Import serialized wallet bytes into manager
App->>FFI: wallet_manager_import_wallet_from_bytes(bytes, len)
FFI->>Mgr: import_wallet_from_bytes(bytes)
Mgr->>Core: deserialize -> Wallet
Core-->>Mgr: Wallet
Mgr-->>FFI: wallet_id
FFI-->>App: wallet_id
end
rect #fff5e6
note right of App: Single-network validation for account ops
App->>FFI: wallet_get_account(..., FFINetwork flags)
FFI->>FFI: try_into_single_network()
alt single network
FFI->>Core: lookup account on Network
Core-->>FFI: account handle
FFI-->>App: success (FFIAccount)
else ambiguous/none
FFI-->>App: error InvalidInput ("Must specify exactly one network")
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs (3)
34-45: Enforce wallet_id ownership to prevent cross-wallet contaminationBefore reading accounts from the passed Wallet, ensure it belongs to this ManagedWalletInfo.
Apply this diff near the start of add_managed_account:
- // First check if the account exists in the wallet + // Validate wallet ownership + if wallet.wallet_id != self.wallet_id { + return Err(Error::InvalidParameter( + "wallet_id does not match ManagedWalletInfo".to_string(), + )); + } + // First check if the account exists in the wallet
86-109: Factor passphrase verification into a single helper; zeroize seedThree functions duplicate passphrase checks and hold sensitive material in memory. Centralize verification and zeroize the seed after use.
Apply this diff to add a reusable verifier and use it (shown for add_managed_account_with_passphrase; mirror in the BLS/EdDSA variants):
@@ - // Verify this is a passphrase wallet - match &wallet.wallet_type { - WalletType::MnemonicWithPassphrase { mnemonic, .. } => { - // Verify the passphrase by deriving and comparing - let seed = mnemonic.to_seed(passphrase); - let root_key = crate::wallet::root_extended_keys::RootExtendedPrivKey::new_master(&seed)?; - - // Compare with wallet's stored public key - let derived_pub = root_key.to_root_extended_pub_key(); - let wallet_pub = wallet.root_extended_pub_key(); - - if derived_pub.root_public_key != wallet_pub.root_public_key { - return Err(Error::InvalidParameter( - "Invalid passphrase".to_string() - )); - } - - // Passphrase is valid, proceed with adding the managed account - self.add_managed_account(wallet, account_type, network) - } - _ => Err(Error::InvalidParameter( - "add_managed_account_with_passphrase can only be used with wallets created with a passphrase".to_string() - )), - } + verify_wallet_passphrase(wallet, passphrase)?; + self.add_managed_account(wallet, account_type, network)Add (outside this impl) a helper that zeroizes the seed:
use zeroize::Zeroize; fn verify_wallet_passphrase(wallet: &Wallet, passphrase: &str) -> Result<()> { match &wallet.wallet_type { WalletType::MnemonicWithPassphrase { mnemonic, .. } => { let mut seed = mnemonic.to_seed(passphrase); let root_key = crate::wallet::root_extended_keys::RootExtendedPrivKey::new_master(&seed)?; // seed no longer needed seed.zeroize(); let derived_pub = root_key.to_root_extended_pub_key(); let wallet_pub = wallet.root_extended_pub_key(); if derived_pub.root_public_key != wallet_pub.root_public_key { return Err(Error::InvalidParameter("Invalid passphrase".to_string())); } Ok(()) } _ => Err(Error::InvalidParameter( "function requires a wallet created with a passphrase".to_string(), )), } }Note: If zeroize is not yet a dependency, consider adding it (optional feature if desired).
Also applies to: 201-224, 325-348
117-122: Enforce xpub/network consistency
Inkey-wallet/src/wallet/managed_wallet_info/managed_accounts.rs’sadd_managed_account_from_xpub, add a guard before callingAccount::newto reject mismatched networks:if account_xpub.network != network { return Err(Error::InvalidParameter("xpub network mismatch".into())); } // Create an Account with no wallet ID (standalone managed account) let account = Account::new(None, account_type, account_xpub, network)?;Account::new (and its from_xpub) currently don’t verify that
account_xpub.network == network, so this check is required to prevent silent network mismatches.key-wallet-ffi/tests/test_valid_addr.rs (1)
17-48: Make the test assertive; it currently passes even on failure.
- Using
if let Some(...)allows the test to silently succeed when the account is missing.- The parse/require-network
matchonly prints; no assertion on success.Apply this to fail fast and validate expectations:
- if let Some(account) = wallet.get_bip44_account(Network::Testnet, 0) { + let account = wallet + .get_bip44_account(Network::Testnet, 0) + .expect("Expected BIP44 account 0 on testnet to exist"); @@ - println!("Generated testnet address: {}", address); + println!("Generated testnet address: {}", address); @@ - // Now try to validate it - let addr_str = address.to_string(); - match key_wallet::Address::from_str(&addr_str) { - Ok(parsed) => { - println!("Successfully parsed generated address"); - match parsed.require_network(dashcore::Network::Testnet) { - Ok(_) => println!("✓ Address is valid for testnet"), - Err(e) => println!("✗ Address not valid for testnet: {}", e), - } - } - Err(e) => { - println!("Failed to parse generated address: {}", e); - } - } + // Now validate it + let addr_str = address.to_string(); + let parsed = key_wallet::Address::from_str(&addr_str) + .expect("Failed to parse generated address"); + parsed + .require_network(dashcore::Network::Testnet) + .expect("Address not valid for testnet"); @@ - } + // unreachable: the expect above will fail the test if account is missingkey-wallet-ffi/src/address.rs (1)
37-50: UB: freeing Box<[T]> with a thin pointer; fix address_array_free.Box::from_raw(std::slice::from_raw_parts_mut(...)) is invalid; reconstruct a fat pointer with ptr::slice_from_raw_parts_mut before Box::from_raw. Also avoid double-unsafe nesting and iterate with iter_mut().
Apply:
pub unsafe extern "C" fn address_array_free(addresses: *mut *mut c_char, count: usize) { if !addresses.is_null() { - unsafe { - let slice = std::slice::from_raw_parts_mut(addresses, count); - for addr in slice { - if !addr.is_null() { - let _ = CString::from_raw(*addr); - } - } - // Free the array itself - let _ = Box::from_raw(std::slice::from_raw_parts_mut(addresses, count)); - } + let slice = std::slice::from_raw_parts_mut(addresses, count); + for addr in slice.iter_mut() { + if !addr.is_null() { + let _ = CString::from_raw(*addr); + } + } + // Free the array itself (allocated via Box::into_raw(Box<[*mut c_char]>)) + let fat_ptr: *mut [*mut c_char] = std::ptr::slice_from_raw_parts_mut(addresses, count); + let _ = Box::from_raw(fat_ptr); } }key-wallet-ffi/src/managed_wallet.rs (1)
1101-1111: Tests: avoid libc::free on Box allocations; use address_array_free.Manually freeing each string then calling libc::free is UB; free the array and strings via the provided FFI helper.
Apply:
- unsafe { - let addresses = std::slice::from_raw_parts(addresses_out, count_out); - for &addr_ptr in addresses { - let addr_str = CStr::from_ptr(addr_ptr).to_string_lossy(); - assert!(!addr_str.is_empty()); - println!("External address: {}", addr_str); - let _ = CString::from_raw(addr_ptr); - } - libc::free(addresses_out as *mut libc::c_void); - } + unsafe { + let addresses = std::slice::from_raw_parts(addresses_out, count_out); + for &addr_ptr in addresses { + let addr_str = CStr::from_ptr(addr_ptr).to_string_lossy(); + assert!(!addr_str.is_empty()); + println!("External address: {}", addr_str); + } + // Free the array and all inner strings correctly + crate::address::address_array_free(addresses_out, count_out); + }key-wallet-ffi/src/keys.rs (2)
1006-1013: Bug: invalid deref ofindexin ChildNumber match (likely compile error).
indexis au32in typical BIP32 enums;*indexdereferences a non-pointer. Useindexdirectly.- key_wallet::ChildNumber::Normal { - index, - } => (*index, false), - key_wallet::ChildNumber::Hardened { - index, - } => (*index, true), + key_wallet::ChildNumber::Normal { index } => (index, false), + key_wallet::ChildNumber::Hardened { index } => (index, true),
1058-1069: Fix: unsound/freeing boxed slices reconstructed viafrom_raw_parts_mut.
Box::from_rawexpects a fat pointer*mut [T]. Reconstruct it withstd::ptr::slice_from_raw_parts_mut(ptr, len). Current code passes a reference&mut [T]and will not compile; even if coerced, it’s incorrect.- if !indices.is_null() && count > 0 { - unsafe { - // Reconstruct the boxed slice from the raw pointer and let it drop - let _ = Box::from_raw(std::slice::from_raw_parts_mut(indices, count)); - } - } - if !hardened.is_null() && count > 0 { - unsafe { - // Reconstruct the boxed slice from the raw pointer and let it drop - let _ = Box::from_raw(std::slice::from_raw_parts_mut(hardened, count)); - } - } + if !indices.is_null() && count > 0 { + // Recreate fat pointer and drop the Box<[u32]> allocated in derivation_path_parse + let _ = unsafe { Box::from_raw(std::ptr::slice_from_raw_parts_mut(indices, count)) }; + } + if !hardened.is_null() && count > 0 { + let _ = unsafe { Box::from_raw(std::ptr::slice_from_raw_parts_mut(hardened, count)) }; + }key-wallet-ffi/src/provider_keys.rs (1)
239-247: Zeroize private key buffer before free.Avoid leaving key material in heap after
free.- if !info.private_key.is_null() { - libc::free(info.private_key as *mut libc::c_void); - info.private_key = ptr::null_mut(); - } + if !info.private_key.is_null() { + if info.private_key_len > 0 { + // Zeroize private key bytes before freeing + std::ptr::write_bytes(info.private_key, 0, info.private_key_len); + info.private_key_len = 0; + } + libc::free(info.private_key as *mut libc::c_void); + info.private_key = ptr::null_mut(); + }key-wallet-ffi/src/derivation.rs (2)
582-585: Doc fix: correct the deallocation function name.The function returns an
FFIExtendedPubKey*that must be freed viaderivation_xpub_free.-/// - The returned pointer must be freed with `extended_public_key_free` +/// - The returned pointer must be freed with `derivation_xpub_free`
620-621: Doc fix: correct the string free function.Use the actual exported symbol.
-/// - The returned string must be freed with `string_free` +/// - The returned string must be freed with `derivation_string_free`key-wallet-ffi/src/transaction.rs (2)
201-226: Potential u64→u32 timestamp truncation.Casting timestamp to u32 can overflow silently. Validate bounds or saturate; prefer explicit error.
- timestamp: if timestamp > 0 { - Some(timestamp as u32) - } else { - None - }, + timestamp: if timestamp == 0 { + None + } else if timestamp <= u32::MAX as u64 { + Some(timestamp as u32) + } else { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Timestamp exceeds u32::MAX".to_string(), + ); + return false; + },Apply the same guard in the InChainLockedBlock branch.
268-274: UB: freeing transaction bytes without length.Box::from_raw(tx_bytes) only drops a single u8, not the allocated slice; this is undefined behavior if the buffer was a Box<[u8]>. Mirror wallet_manager_free_wallet_bytes and accept length.
-#[no_mangle] -pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8) { - if !tx_bytes.is_null() { - unsafe { - let _ = Box::from_raw(tx_bytes); - } - } -} +#[no_mangle] +pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8, tx_len: usize) { + if !tx_bytes.is_null() && tx_len > 0 { + // Reconstruct the boxed slice for proper drop of DST allocation + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(tx_bytes, tx_len)); + } +}If ABI stability is required, keep the old symbol as a no-op shim and introduce transaction_bytes_free_with_len(tx_bytes, tx_len).
key-wallet/src/tests/edge_case_tests.rs (1)
31-45: This test is a no-op; either implement it or ignore it.The test builds cases but performs no assertions. Either parse and validate errors or mark the test ignored to avoid false positives.
#[test] -fn test_invalid_derivation_paths() { +#[ignore] // TODO(qa): implement parsing or remove +fn test_invalid_derivation_paths() {key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
114-120: Infinite recursion in trait impl for ManagedWalletInfo::from_wallet and from_wallet_with_name.Both methods call themselves via
Self::..., causing unbounded recursion at runtime. Delegate to the inherent constructors on ManagedWalletInfo instead.- fn from_wallet(wallet: &Wallet) -> Self { - Self::from_wallet_with_name(wallet, String::new()) - } + fn from_wallet(wallet: &Wallet) -> Self { + ManagedWalletInfo::from_wallet(wallet) + } @@ - fn from_wallet_with_name(wallet: &Wallet, name: String) -> Self { - Self::from_wallet_with_name(wallet, name) - } + fn from_wallet_with_name(wallet: &Wallet, name: String) -> Self { + ManagedWalletInfo::from_wallet_with_name(wallet, name) + }key-wallet-manager/src/wallet_manager/mod.rs (1)
113-165: Method documentation doesn't match implementationThe method returns a
WalletIdbut the documentation still says "add it to the manager", which is misleading since the primary purpose is now to create and return the wallet ID.Apply this diff to fix the documentation:
- /// Create a new wallet from mnemonic and add it to the manager - /// Returns the computed wallet ID + /// Create a wallet from mnemonic and add it to the manager + /// + /// # Returns + /// The computed wallet ID of the newly created walletkey-wallet-ffi/src/types.rs (1)
79-89: Defaulting unknown Network to Dash conflicts with prior preference to Testnet.Per prior preference (fallback to testnet prefixes), switch the fallback mapping to Testnet to keep behavior consistent across crates.
impl From<Network> for FFINetwork { fn from(n: Network) -> Self { match n { Network::Dash => FFINetwork::Dash, Network::Testnet => FFINetwork::Testnet, Network::Regtest => FFINetwork::Regtest, Network::Devnet => FFINetwork::Devnet, - _ => FFINetwork::Dash, // Default to Dash for unknown networks + _ => FFINetwork::Testnet, // Default to Testnet for unknown networks (preferred fallback) } } }
🧹 Nitpick comments (53)
key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs (2)
49-63: DRY: deduplicate “get/dup-check/insert” managed-account patternThe same insertion sequence is repeated 6x. Extract a helper to reduce errors and keep logic consistent.
Apply this diff in each location:
- // Get or create the managed account collection for this network - let managed_collection = self.accounts.entry(network).or_default(); - - // Check if managed account already exists - if managed_collection.contains_managed_account_type(managed_account.managed_type()) { - return Err(Error::InvalidParameter(format!( - "Managed account type {:?} already exists for network {:?}", - account_type, network - ))); - } - - // Insert into the collection - managed_collection.insert(managed_account); - Ok(()) + self.insert_managed(network, account_type, managed_account)Add this helper in an impl ManagedWalletInfo block:
impl ManagedWalletInfo { fn insert_managed( &mut self, network: Network, account_type: AccountType, managed_account: ManagedAccount, ) -> Result<()> { let managed_collection = self.accounts.entry(network).or_default(); if managed_collection.contains_managed_account_type(managed_account.managed_type()) { return Err(Error::InvalidParameter(format!( "Managed {:?} already exists for {:?}", account_type, network ))); } managed_collection.insert(managed_account); Ok(()) } }Also applies to: 123-137, 169-183, 247-261, 293-307, 371-385
388-463: Add coverage: xpub and passphrase flows (+feature-gated BLS/EdDSA)Consider unit tests for:
- add_managed_account_with_passphrase (valid/invalid passphrase).
- add_managed_account_from_xpub (mismatched network should fail).
- Feature-gated: BLS/EdDSA variants, including duplicate-insert rejection.
I can draft these tests aligned with existing helpers; want me to open a follow-up PR?
key-wallet/src/tests/advanced_transaction_tests.rs (2)
47-49: Prefer u32::MAX over magic literal 0xffffffff.Improves readability and avoids type inference surprises in tests.
Apply this diff:
- sequence: 0xffffffff, + sequence: u32::MAX,Also applies to: 222-224, 289-291, 327-329
8-8: Remove unused import if not needed.If Hash isn’t used by trait methods, drop it to keep clippy clean.
-use dashcore::hashes::Hash;key-wallet/src/account/account_type.rs (2)
7-7: Avoid layering inversion: move the From for AccountTypeToCheck out of account module.Having account_type depend on transaction_checking couples layers and risks cyclic deps. Define this impl in transaction_checking::transaction_router instead and keep account_type decoupled.
Apply this diff here:
-use crate::transaction_checking::transaction_router::AccountTypeToCheck; @@ -impl From<AccountType> for AccountTypeToCheck { - fn from(value: AccountType) -> Self { - match value { - AccountType::Standard { standard_account_type, .. } => match standard_account_type { - StandardAccountType::BIP44Account => AccountTypeToCheck::StandardBIP44, - StandardAccountType::BIP32Account => AccountTypeToCheck::StandardBIP32, - }, - AccountType::CoinJoin { .. } => AccountTypeToCheck::CoinJoin, - AccountType::IdentityRegistration => AccountTypeToCheck::IdentityRegistration, - AccountType::IdentityTopUp { .. } => AccountTypeToCheck::IdentityTopUp, - AccountType::IdentityTopUpNotBoundToIdentity => AccountTypeToCheck::IdentityTopUpNotBound, - AccountType::IdentityInvitation => AccountTypeToCheck::IdentityInvitation, - AccountType::ProviderVotingKeys => AccountTypeToCheck::ProviderVotingKeys, - AccountType::ProviderOwnerKeys => AccountTypeToCheck::ProviderOwnerKeys, - AccountType::ProviderOperatorKeys => AccountTypeToCheck::ProviderOperatorKeys, - AccountType::ProviderPlatformKeys => AccountTypeToCheck::ProviderPlatformKeys, - } - } -}Then add in transaction_checking/transaction_router (or a nearby module):
use crate::account::account_type::{AccountType, StandardAccountType}; impl From<AccountType> for AccountTypeToCheck { fn from(value: AccountType) -> Self { match value { AccountType::Standard { standard_account_type, .. } => match standard_account_type { StandardAccountType::BIP44Account => AccountTypeToCheck::StandardBIP44, StandardAccountType::BIP32Account => AccountTypeToCheck::StandardBIP32, }, AccountType::CoinJoin { .. } => AccountTypeToCheck::CoinJoin, AccountType::IdentityRegistration => AccountTypeToCheck::IdentityRegistration, AccountType::IdentityTopUp { .. } => AccountTypeToCheck::IdentityTopUp, AccountType::IdentityTopUpNotBoundToIdentity => AccountTypeToCheck::IdentityTopUpNotBound, AccountType::IdentityInvitation => AccountTypeToCheck::IdentityInvitation, AccountType::ProviderVotingKeys => AccountTypeToCheck::ProviderVotingKeys, AccountType::ProviderOwnerKeys => AccountTypeToCheck::ProviderOwnerKeys, AccountType::ProviderOperatorKeys => AccountTypeToCheck::ProviderOperatorKeys, AccountType::ProviderPlatformKeys => AccountTypeToCheck::ProviderPlatformKeys, } } }Also applies to: 68-95
68-95: Optional ergonomics: also implement From<&AccountType>.Keeps call sites flexible without relying on Copy semantics.
impl From<&AccountType> for AccountTypeToCheck { fn from(value: &AccountType) -> Self { (*value).into() } }key-wallet/src/address_metadata_tests.rs (1)
12-24: Track the ManagedAccount TODOs with issues and test plans.Convert these notes into actionable tasks (GH issues) so we don’t lose them; ManagedAccount coverage will be important.
I can draft test scaffolding (including proptest cases) once the ManagedAccount APIs stabilize—want me to open issues with checklists?
key-wallet/src/wallet/root_extended_keys.rs (1)
375-401: New Cow API is sensible; reduce duplication in match.
Combine identical arms for brevity; no behavior change.- WalletType::ExternalSignable(root_extended_public_key) => { - Cow::Borrowed(root_extended_public_key) - } - WalletType::WatchOnly(root_extended_public_key) => { - Cow::Borrowed(root_extended_public_key) - } + WalletType::ExternalSignable(root_extended_public_key) + | WalletType::WatchOnly(root_extended_public_key) => Cow::Borrowed(root_extended_public_key),key-wallet/Cargo.toml (1)
44-44: Prefer explicit default-features = false on zeroize.
Not strictly required, but prevents accidental feature drift.-zeroize = { version = "1.8", features = ["derive"] } +zeroize = { version = "1.8", default-features = false, features = ["derive"] }key-wallet-ffi/src/keys.rs (2)
49-59: Good: switched to fallible network conversion with clear InvalidInput on multi/none.This aligns FFI behavior with single-network enforcement and consistent error paths. Consider deduping the error string via a shared constant to avoid drift.
+const EXACTLY_ONE_NETWORK_ERR: &str = "Must specify exactly one network"; - "Must specify exactly one network".to_string(), + EXACTLY_ONE_NETWORK_ERR.to_string(),Also applies to: 109-119, 193-203, 273-283, 352-362, 515-525, 600-610, 682-692, 763-773
598-611: Style: inconsistent raw-pointer deref safety style.Some functions use
let wallet = unsafe { &*wallet };, others rely onunsafe fncontext. Pick one style for readability.Also applies to: 680-693, 761-774
key-wallet/src/seed.rs (1)
120-134: Optional: avoid revealing seed bytes inDisplay.Even partial exposure can leak sensitive information via logs. Consider redacting all bytes.
- write!(f, "Seed({}...{})", start, end) + write!(f, "Seed(REDACTED)")key-wallet-ffi/src/account_tests.rs (1)
45-51: Add success checks for wallet creation and guard against param-validation regressions
- After wallet_create_from_mnemonic, assert Success and non-null wallet.
- After wallet_get_account, assert it’s not an InvalidInput (to catch enum wiring regressions).
Apply:
let wallet = unsafe { wallet::wallet_create_from_mnemonic( mnemonic.as_ptr(), passphrase.as_ptr(), FFINetwork::Testnet, &mut error, ) }; + assert!(!wallet.is_null()); + assert_eq!(error.code, FFIErrorCode::Success); // Try to get the default account (should exist) let result = unsafe { wallet_get_account(wallet, FFINetwork::Testnet, 0, FFIAccountType::StandardBIP44) }; + assert_ne!(result.error_code, FFIErrorCode::InvalidInput as i32);key-wallet/src/wallet/passphrase_test.rs (1)
24-26: Network slice API updates — LGTM; add a wrong-passphrase negative checkThe conversion to &[Network] is consistent. Consider also asserting failure with an incorrect passphrase to harden behavior.
Add within test_add_account_with_passphrase:
// Wrong passphrase should fail let wrong = wallet.add_account_with_passphrase( AccountType::Standard { index: 2, standard_account_type: StandardAccountType::BIP44Account }, network, "wrong_passphrase" ); assert!(wrong.is_err()); assert!(wrong.unwrap_err().to_string().contains("passphrase"));Also applies to: 73-76, 107-111, 153-154
key-wallet/src/tests/performance_tests.rs (1)
92-95: Network slice changes — LGTM; gate perf tests to avoid CI flakinessThese tests use tight timing thresholds and can be noisy on shared runners. Consider feature-gating or marking them ignored by default.
Add before each performance test:
#[cfg_attr(not(feature = "perf-tests"), ignore)]Or gate the whole module:
#![cfg(any(test, feature = "perf-tests"))]Also applies to: 138-141, 206-209, 305-307
key-wallet/src/tests/integration_tests.rs (1)
20-22: Network slice conversion — LGTM; consider exercising multi-network creation directlyYou add accounts on Dash/Devnet post-creation; also consider initializing the wallet with multiple networks to test multi-network account bootstrapping.
Example:
- &[Network::Testnet], + &[Network::Testnet, Network::Dash, Network::Devnet],Also applies to: 76-79
key-wallet-manager/Cargo.toml (1)
26-26: Gate zeroize behind the bincode feature (if not used elsewhere).This avoids pulling
zeroizefor consumers not using serialization.-zeroize = { version = "1.8", features = ["derive"] } +zeroize = { version = "1.8", features = ["derive"], optional = true }key-wallet-ffi/src/derivation.rs (1)
736-740: Optional: zeroize secrets on free.Consider zeroizing the private key material before drop if supported by
key_wallet::bip32::ExtendedPrivKey.I can draft a zeroize-on-drop implementation gated by a feature if
ExtendedPrivKeyexposes zeroizable fields.key-wallet-manager/tests/spv_integration_tests.rs (1)
202-209: Prefer expect() over .ok() to surface failures.Hide-and-assert via
wallet_count()makes debugging harder if creation fails.- for _i in 0..3 { - spv.base - .create_wallet_with_random_mnemonic( - WalletAccountCreationOptions::Default, - Network::Testnet, - ) - .ok(); - } + for _i in 0..3 { + spv.base + .create_wallet_with_random_mnemonic( + WalletAccountCreationOptions::Default, + Network::Testnet, + ) + .expect("Failed to create wallet"); + }key-wallet-ffi/src/wallet_tests.rs (1)
285-292: Tighten assertions for wallet_add_account result.Assert success implies non-null account and error_code==0; failure implies null account and error_code!=0.
- // Some implementations may not support adding accounts, so just verify it doesn't crash - // and the error code is set appropriately - assert!(!result.account.is_null() || result.error_code != 0); + // Success -> account != null and error_code == 0; Failure -> account == null and error_code != 0 + if result.account.is_null() { + assert_ne!(result.error_code, 0); + } else { + assert_eq!(result.error_code, 0); + }key-wallet-manager/tests/test_serialized_wallets.rs (2)
20-20: Reduce derivation cap to speed tests.100_000 can slow CI; a much smaller cap exercises the path.
- Some(100_000), + Some(1_000),Also applies to: 36-36, 55-55
31-48: Add assertions about watch-only/external-signable semantics after import.If an API exists to query wallet capabilities, import these serialized bytes and assert “no private keys”/“externally signable” flags.
I can extend the tests once you confirm the accessor (e.g.,
WalletManager::wallet_info(id)or similar) to validate flags on the imported wallet.Also applies to: 50-65
key-wallet-ffi/src/account.rs (3)
157-167: Avoid parameter shadowing; keep naming consistent with other functions.Use network_rust (as above) to avoid shadowing the FFINetwork param and to keep consistency.
Apply this diff:
- let network: key_wallet::Network = match network.try_into() { + let network_rust: key_wallet::Network = match network.try_into() { Ok(n) => n, Err(_) => { FFIError::set_error( error, FFIErrorCode::InvalidInput, "Must specify exactly one network".to_string(), ); return 0; } }; - match wallet.inner().accounts.get(&network) { + match wallet.inner().accounts.get(&network_rust) {
169-178: Minor clippy clean-up: convert bool to usize without cast.Use usize::from(...) to avoid casts and keep clippy happy under -D warnings.
- let count = accounts.standard_bip44_accounts.len() + let count = accounts.standard_bip44_accounts.len() + accounts.standard_bip32_accounts.len() + accounts.coinjoin_accounts.len() - + accounts.identity_registration.is_some() as usize + + usize::from(accounts.identity_registration.is_some()) + accounts.identity_topup.len();
113-117: Safe free for FFIAccount.Box::from_raw drop is correct and idempotent at the callsite. Document “caller must null the pointer after free” in the header for extra safety.
key-wallet/src/wallet_comprehensive_tests.rs (2)
176-176: Comment mismatch with setup (None creates no defaults).This test uses WalletAccountCreationOptions::None earlier; update the comment to avoid confusion.
- // Different passphrases should generate different root keys + // Different passphrases should generate different root keys(Only the preceding comment needs rewording; code is fine.)
209-216: Stronger multi-network coverage suggestion.To exercise multi-network initialization as well as add_account, create wallets with the full networks slice (e.g., &[Testnet, Dash, Devnet]) in both creation and restore paths. Today you only pass Testnet and rely on add_account to populate others.
Also applies to: 251-258
key-wallet/src/tests/backup_restore_tests.rs (2)
176-179: Inaccurate comment about default account creation.This file uses WalletAccountCreationOptions::None; index 0 is not created by default. Tweak the comment to avoid misleading readers.
- index: 1, // Use index 1 since 0 is created by default + index: 1, // Using index 1; with None, no default accounts are created
284-292: Comment vs behavior.“account 0 is created by default” conflicts with None; the code correctly computes initial_account_count, so just update the comment.
- // Initial state - account 0 is created by default, no need to add it + // Initial state with None options: no accounts created by defaultkey-wallet-ffi/src/transaction.rs (2)
73-73: Unused network in wallet_sign_transaction.Same note as build; confirm intended API or drop the param until needed to avoid FFI churn.
49-56: Clear “not yet implemented” errors.Returning WalletError is fine for stubs; consider FFIErrorCode::NotImplemented to distinguish from runtime wallet failures.
Also applies to: 90-96
key-wallet/src/tests/edge_case_tests.rs (4)
57-60: Callsite updates to multi-network API look correct.Passing slices (e.g., &[Network::Testnet]) matches the new constructors and should exercise per-network account creation. Consider adding at least one test with multiple networks (e.g., &[Network::Dash, Network::Testnet]) to assert account collections are created for each network.
Also applies to: 66-71, 85-88, 92-97, 136-141, 240-246, 257-258, 281-286, 291-298, 301-306, 338-345, 368-374
206-214: Heavy test may stress CI (10k outputs).10k outputs inflate memory/time; unless you specifically need this bound, consider reducing to 1k or gating behind a feature.
Would you like me to add a cargo feature (e.g., "heavy-tests") and gate this with #[cfg(feature = "heavy-tests")]?
10-10: Remove unused import to keep clippy -D warnings clean.-use dashcore::hashes::Hash;
315-329: Depth-255 derivation-path test is fine; consider property testing.Looks good. Optionally switch to proptest to cover a range of depths without fixed upper bound.
key-wallet-ffi/src/wallet_manager_tests.rs (1)
1297-1596: FFI serialization/import tests are solid; replace unsafe manual copy with safe slice-to-vec.Great coverage across watch-only and externally signable flows, plus invalid mnemonic. Replace the manual
Vec::with_capacity+set_lencopy with a saferto_vec()to avoid accidental UB if lengths drift.- let wallet_bytes_copy = unsafe { - let mut copy = Vec::with_capacity(wallet_bytes_len_out); - ptr::copy_nonoverlapping(wallet_bytes_out, copy.as_mut_ptr(), wallet_bytes_len_out); - copy.set_len(wallet_bytes_len_out); - copy - }; + let wallet_bytes_copy = unsafe { + std::slice::from_raw_parts(wallet_bytes_out, wallet_bytes_len_out) + }.to_vec();key-wallet-manager/tests/integration_test.rs (3)
29-36: Mnemonic-based creation callsite looks correct.Parameters align with the new signature; unwrap handled via assert. Consider also testing Network::Dash to satisfy mainnet/testnet coverage preference.
65-69: Assertion couples to Default account count (8).This may become brittle if Default changes. Consider asserting “>= 1” for existence plus targeted checks on specific known accounts, or fetch the expected count from a helper constant.
76-82: Updated callsites to create_wallet_with_random_mnemonic are consistent.Looks good across tests. Optional: add a second test parametrized over Network to exercise both Dash and Testnet.
Also applies to: 133-139, 159-165
key-wallet-manager/src/wallet_manager/mod.rs (2)
255-259: Improve error message for serialization failuresThe error message could be more helpful by preserving the original error details rather than just formatting it.
Consider preserving the error chain:
- let serialized_bytes = bincode::encode_to_vec(&final_wallet, bincode::config::standard()) - .map_err(|e| { - WalletError::InvalidParameter(format!("Failed to serialize wallet: {}", e)) - })?; + let serialized_bytes = bincode::encode_to_vec(&final_wallet, bincode::config::standard()) + .map_err(|e| { + WalletError::Serialization(format!("Failed to encode wallet: {}", e)) + })?;Note: This would require adding a
Serializationvariant to theWalletErrorenum if it doesn't already exist.
466-498: Consider extracting duplicate wallet existence checkThe wallet existence check pattern (lines 480-483) is repeated across multiple functions. Consider extracting it into a helper method for consistency and maintainability.
Consider adding a helper method:
impl<T: WalletInfoInterface> WalletManager<T> { fn ensure_wallet_not_exists(&self, wallet_id: &WalletId) -> Result<(), WalletError> { if self.wallets.contains_key(wallet_id) { Err(WalletError::WalletExists(*wallet_id)) } else { Ok(()) } } }Then use it consistently:
- // Check if wallet already exists - if self.wallets.contains_key(&wallet_id) { - return Err(WalletError::WalletExists(wallet_id)); - } + // Check if wallet already exists + self.ensure_wallet_not_exists(&wallet_id)?;key-wallet/src/wallet/mod.rs (1)
149-150: Document zeroization limitations more prominentlyThe comments mention that
SecretKeydoesn't implementZeroize, which is an important security limitation. Consider adding a module-level or type-level documentation warning about this.Consider adding a security note to the module documentation:
//! Complete wallet management for Dash //! //! This module provides comprehensive wallet functionality including //! multiple accounts, seed management, and transaction coordination. //! //! # Security Note //! //! While this module implements `Zeroize` for sensitive data, note that //! `secp256k1::SecretKey` does not implement `Zeroize`. The chain codes //! and other sensitive data are properly zeroized, but the secret keys //! themselves may remain in memory after dropping.Also applies to: 173-173
key-wallet/src/wallet/initialization.rs (1)
99-102: Potential performance issue with cloning account creation optionsThe code clones
account_creation_optionsfor each network iteration. For complex options with many accounts, this could be inefficient.Consider borrowing the options instead of cloning if the methods don't need ownership:
- for network in networks { - wallet.create_accounts_from_options(account_creation_options.clone(), *network)?; - } + for network in networks { + wallet.create_accounts_from_options(&account_creation_options, *network)?; + }This would require updating the
create_accounts_from_optionsmethod signature to accept a reference.Also applies to: 157-160, 189-196, 308-311, 345-348
key-wallet-ffi/src/address_pool.rs (1)
619-698: Consider refactoring the address marking logicThe address marking logic iterates through all account types sequentially with repeated code. This could be simplified using a more functional approach.
Consider refactoring to reduce code duplication:
// Helper function to try marking address in an optional account fn try_mark_in_account(account: Option<&mut ManagedAccount>, address: &Address) -> bool { account.map_or(false, |acc| acc.mark_address_used(address)) } // Then in the main function: let marked = collection.standard_bip44_accounts.values_mut() .chain(collection.standard_bip32_accounts.values_mut()) .chain(collection.coinjoin_accounts.values_mut()) .chain(collection.identity_topup.values_mut()) .any(|account| account.mark_address_used(&address)) || try_mark_in_account(collection.identity_registration.as_mut(), &address) || try_mark_in_account(collection.identity_topup_not_bound.as_mut(), &address) || try_mark_in_account(collection.identity_invitation.as_mut(), &address) || try_mark_in_account(collection.provider_voting_keys.as_mut(), &address) || try_mark_in_account(collection.provider_owner_keys.as_mut(), &address) || try_mark_in_account(collection.provider_operator_keys.as_mut(), &address) || try_mark_in_account(collection.provider_platform_keys.as_mut(), &address);key-wallet-ffi/src/wallet_manager.rs (4)
96-106: Ensure TryInto is in scope (MSRV-friendly).Multiple sites call
network.try_into(). To avoid edition/prelude surprises and keep MSRV 1.89 happy, import the trait explicitly once at the top of this module.Apply this diff (top of file, near other use statements):
use std::sync::Mutex; +use std::convert::TryInto;Also applies to: 694-704, 787-797, 967-978, 1046-1056, 1142-1152, 1190-1200
287-297: Use consistent error code for lock poisoning.Here lock failure maps to InvalidInput; elsewhere in this file you use WalletError for lock-related failures. Recommend unifying on WalletError.
Apply this diff:
- FFIError::set_error( - error, - FFIErrorCode::InvalidInput, - "Failed to lock manager".to_string(), - ); + FFIError::set_error( + error, + FFIErrorCode::WalletError, + "Failed to lock manager".to_string(), + );
512-528: Document “count” semantics for wallet IDs.Count is number of 32-byte IDs, not bytes. Header comment already implies this; consider adding a short Rust-side doc note for future maintainers.
578-588: Returning const wallet handles matches the FFI design.This aligns with the “read-only by default” learning. The inline comment about lifetime storage is outdated—these are owned handles; consider pruning it to avoid confusion.
- // Note: We need to store this somewhere that will outlive this function - // For now, we'll return a raw pointer to the wallet - // In a real implementation, you might want to store these in the FFIWalletManager + // Return an owned FFIWallet handle (const for read-only by default).key-wallet-ffi/src/wallet.rs (3)
96-108: Passphrase handling works; consider zeroizing the heap copy.
from_mnemonic_with_passphrasereceives a heapString. For defense-in-depth, zeroize it after use (requires adding the zeroize crate).- match Wallet::from_mnemonic_with_passphrase( - mnemonic, - passphrase_str.to_string(), + let mut passphrase_owned = passphrase_str.to_string(); + let result = Wallet::from_mnemonic_with_passphrase( + mnemonic, + passphrase_owned.clone(), &networks_rust, creation_options, - ) { + ); + #[cfg(feature = "zeroize")] + { + use zeroize::Zeroize; + passphrase_owned.zeroize(); + } + match result {Note: add zeroize as an optional dependency and feature-gate if needed.
Also applies to: 111-127
170-171: Seed-based creation is good; consider zeroizing temporary seed copy.You make an owned
[u8; 64]. Zeroize after wallet creation.let mut seed_array = [0u8; 64]; seed_array.copy_from_slice(seed_bytes); let seed = Seed::new(seed_array); @@ - match Wallet::from_seed(seed, &networks_rust, creation_options) { + let result = Wallet::from_seed(seed, &networks_rust, creation_options); + #[cfg(feature = "zeroize")] + { + use zeroize::Zeroize; + seed_array.zeroize(); + } + match result {Also applies to: 193-195, 203-216
552-572: Bytes variant assumes UTF-8/base58 text; clarify API or parse real bytes.Current “bytes” path decodes UTF-8 then parses base58. Either rename param/docs to indicate it expects a textual xpub buffer, or accept raw xpub bytes and decode accordingly.
Also applies to: 585-609
key-wallet-ffi/src/types.rs (1)
474-486: SpecificAccounts: clarify IdentityTopUp semantics.Pushing
ffi_type.to_account_type(0)means IdentityTopUp gets registration_index=0 unless also provided viatopup_indices. Consider documenting or enforcing that top-ups come viatopup_indicesto avoid silent index=0 surprises.key-wallet-ffi/include/key_wallet_ffi.h (1)
2069-2074: Wallet ID buffer semantics could be emphasized.Consider adding “count is number of 32-byte IDs; total bytes = count*32” to avoid misuse by C callers. cbindgen doc tweak should suffice.
| #[cfg(test)] | ||
| mod account_tests; | ||
| #[cfg(test)] | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tests appear ungated; ensure they don’t compile in non-test builds.
If lib.rs doesn’t gate mod tests; with #[cfg(test)], these modules inflate non-test builds.
Apply gating on each module:
- mod account_tests;
+#[cfg(test)]
+mod account_tests;
- mod address_pool_tests;
+#[cfg(test)]
+mod address_pool_tests;
- mod advanced_transaction_tests;
+#[cfg(test)]
+mod advanced_transaction_tests;
- mod backup_restore_tests;
+#[cfg(test)]
+mod backup_restore_tests;
- mod edge_case_tests;
+#[cfg(test)]
+mod edge_case_tests;
- mod immature_transaction_tests;
+#[cfg(test)]
+mod immature_transaction_tests;
- mod integration_tests;
+#[cfg(test)]
+mod integration_tests;
- mod managed_account_collection_tests;
+#[cfg(test)]
+mod managed_account_collection_tests;
- mod performance_tests;
+#[cfg(test)]
+mod performance_tests;
- mod special_transaction_tests;
+#[cfg(test)]
+mod special_transaction_tests;
- mod transaction_tests;
+#[cfg(test)]
+mod transaction_tests;
- mod utxo_tests;
+#[cfg(test)]
+mod utxo_tests;
- mod wallet_tests;
+#[cfg(test)]
+mod wallet_tests;Run to verify parent gating:
Also applies to: 8-8, 10-10, 12-12, 14-14, 16-16, 18-18, 20-20, 22-22, 24-24, 26-26, 28-28
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect at least one #[cfg(test)] mod tests; declaration
rg -nP -C2 '^\s*#\s*\[cfg\(test\)\]\s*\n\s*mod\s+tests\b' key-wallet/src || {
echo "Missing #[cfg(test)] for top-level tests module in key-wallet/src"; exit 1;
}Length of output: 244
Add #[cfg(test)] to gate the top‐level tests module
In key-wallet/src/lib.rs, annotate the mod tests; declaration so none of the tests/ tree compiles in non‐test builds:
- mod tests;
+ #[cfg(test)]
+ mod tests;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-wallet/src/lib.rs around line 6, the top-level tests module is
unconditionally included so the entire tests/ tree compiles in non-test builds;
add the attribute #[cfg(test)] immediately above the mod tests; declaration to
gate the tests so they are only compiled for test builds (ensure there is no
other attribute on the same line and keep the attribute directly above the mod
line).
| impl zeroize::Zeroize for RootExtendedPubKey { | ||
| fn zeroize(&mut self) { | ||
| // Replace the public key with a dummy value (generator point G) | ||
| // This is a best-effort zeroization since PublicKey doesn't implement Zeroize | ||
| self.root_public_key = secp256k1::PublicKey::from_slice(&[ | ||
| 0x02, 0x79, 0xbe, 0x66, 0x7e, 0xf9, 0xdc, 0xbb, 0xac, 0x55, 0xa0, 0x62, 0x95, 0xce, | ||
| 0x87, 0x0b, 0x07, 0x02, 0x9b, 0xfc, 0xdb, 0x2d, 0xce, 0x28, 0xd9, 0x59, 0xf2, 0x81, | ||
| 0x5b, 0x16, 0xf8, 0x17, 0x98, | ||
| ]) | ||
| .expect("hardcoded generator point should be valid"); | ||
|
|
||
| // Zeroize the chain code | ||
| self.root_chain_code.zeroize(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Zeroize must not panic; replace expect() and use UFCS for chain code.
Panic in zeroize is a foot-gun and clippy-unfriendly. Use a non-panicking fallback and UFCS for Zeroize.
- self.root_public_key = secp256k1::PublicKey::from_slice(&[
- 0x02, 0x79, 0xbe, 0x66, 0x7e, 0xf9, 0xdc, 0xbb, 0xac, 0x55, 0xa0, 0x62, 0x95, 0xce,
- 0x87, 0x0b, 0x07, 0x02, 0x9b, 0xfc, 0xdb, 0x2d, 0xce, 0x28, 0xd9, 0x59, 0xf2, 0x81,
- 0x5b, 0x16, 0xf8, 0x17, 0x98,
- ])
- .expect("hardcoded generator point should be valid");
+ if let Ok(pk) = secp256k1::PublicKey::from_slice(&[
+ 0x02, 0x79, 0xbe, 0x66, 0x7e, 0xf9, 0xdc, 0xbb, 0xac, 0x55, 0xa0, 0x62, 0x95, 0xce,
+ 0x87, 0x0b, 0x07, 0x02, 0x9b, 0xfc, 0xdb, 0x2d, 0xce, 0x28, 0xd9, 0x59, 0xf2, 0x81,
+ 0x5b, 0x16, 0xf8, 0x17, 0x98,
+ ]) {
+ self.root_public_key = pk;
+ } else {
+ debug_assert!(false, "hardcoded generator point should be valid");
+ }
@@
- self.root_chain_code.zeroize();
+ zeroize::Zeroize::zeroize(&mut self.root_chain_code);Optional (outside range) for consistency:
impl zeroize::ZeroizeOnDrop for RootExtendedPubKey {}🤖 Prompt for AI Agents
In key-wallet/src/wallet/root_extended_keys.rs around lines 245 to 259, remove
the panic-causing expect() and instead perform a non-panicking match on
secp256k1::PublicKey::from_slice(...)—assign the parsed public key when Ok(...)
and silently skip replacement (or leave the existing value) on Err(...) to avoid
panics; also call the chain code zeroization via UFCS as
zeroize::Zeroize::zeroize(&mut self.root_chain_code); (optionally add impl
zeroize::ZeroizeOnDrop for RootExtendedPubKey {} elsewhere for consistency).
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 (1)
key-wallet-ffi/src/types.rs (1)
500-512: Skip IdentityTopUp in special_account_types conversion
In FFIWalletAccountCreationOptions.to_wallet_options (key-wallet-ffi/src/types.rs:500–512), mapping special_account_types always usesto_account_type(0), soFFIAccountType::IdentityTopUpis forced toregistration_index = 0instead of using the suppliedtopup_indices. Add a guard to skip it here and rely ontopup_indicesfor identity top-up accounts:-for &ffi_type in slice { - accounts.push(ffi_type.to_account_type(0)); -} +for &ffi_type in slice { + // Skip IdentityTopUp here—use `topup_indices` for registration indices + if matches!(ffi_type, FFIAccountType::IdentityTopUp) { + continue; + } + accounts.push(ffi_type.to_account_type(0)); +}Alternatively, change
to_wallet_optionsto return aResultand error onIdentityTopUpinspecial_account_types.
♻️ Duplicate comments (1)
key-wallet-ffi/src/types.rs (1)
241-244: Docstring is stale: function no longer returns Option.Update to reflect the new signature and semantics.
- /// Convert to AccountType with optional indices - /// Returns None if required parameters are missing (e.g., registration_index for IdentityTopUp) + /// Convert to key_wallet::AccountType using the provided `index` where applicable. + /// For IdentityTopUp, `index` is interpreted as the `registration_index`.
🧹 Nitpick comments (1)
key-wallet-ffi/src/types.rs (1)
66-76: Use a typed error for TryFrom to align with error-guidelines.Replace &'static str with a thiserror-backed type for consistency and better interop.
Apply this diff here:
impl TryFrom<FFINetwork> for Network { - type Error = &'static str; + type Error = FFINetworkError; @@ - Some(network) => Ok(network), - None => Err("FFINetwork must represent exactly one network"), + Some(network) => Ok(network), + None => Err(FFINetworkError::MultipleOrNone),And add (outside this hunk):
#[derive(thiserror::Error, Debug, Clone, Copy, PartialEq, Eq)] pub enum FFINetworkError { #[error("FFINetwork must represent exactly one network")] MultipleOrNone, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/types.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with the thiserror crate and propagate errors appropriately
Use the tokio runtime for async operations
Adhere to MSRV: code must compile on Rust 1.89 (avoid newer language/features)
Keep code formatted with rustfmt (cargo fmt)
Keep the codebase clippy-clean (clippy with -D warnings)
Files:
key-wallet-ffi/src/types.rs
{dash-network-ffi/**/*.rs,dash-spv-ffi/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.{swift,m,mm,c,h}}
📄 CodeRabbit inference engine (CLAUDE.md)
Be especially careful with memory safety at FFI boundaries
Files:
key-wallet-ffi/src/types.rs
{**/*.rs,**/Cargo.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use feature flags and conditional compilation for optional features
Files:
key-wallet-ffi/src/types.rs
{key-wallet/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/src/types.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/src/types.rs
🧬 Code graph analysis (1)
key-wallet-ffi/src/types.rs (4)
key-wallet/src/account/mod.rs (4)
new(72-79)network(153-155)index(117-119)account_type(149-151)key-wallet/src/managed_account/managed_account_trait.rs (4)
network(24-24)balance(36-36)index(60-62)account_type(18-18)key-wallet/src/account/account_type.rs (2)
from(69-94)index(100-121)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (4)
balance(69-69)balance(194-196)accounts(81-81)accounts(241-243)
🪛 GitHub Actions: Continuous integration
key-wallet-ffi/src/types.rs
[error] 102-102: Command 'cargo fmt --all -- --check' failed due to formatting differences. See diff at key-wallet-ffi/src/types.rs:102. Run 'cargo fmt' to fix formatting.
⏰ 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). (1)
- GitHub Check: fuzz (hashes_sha256)
🔇 Additional comments (2)
key-wallet-ffi/src/types.rs (2)
24-29: Defaulting “no networks” to Testnet may be surprising—confirm intent.Zero/NoNetworks → Testnet can mask caller errors. Consider returning an empty set or requiring an explicit network at call sites.
Would you like me to switch to returning an empty Vec to surface misconfiguration?
- // If no networks specified, default to testnet - return vec![Network::Testnet]; + // No networks specified -> return empty set to let callers decide defaults + return Vec::new();
91-116: Apply Rust formatting locally
Rust formatting couldn’t be verified in this environment. Runcargo fmt --all -- --check, fix any reported issues, and commit the changes to unblock CI.
| /// FFI Network type (bit flags for multiple networks) | ||
| #[repr(C)] | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum FFINetwork { | ||
| Dash = 0, | ||
| Testnet = 1, | ||
| Regtest = 2, | ||
| Devnet = 3, | ||
| NoNetworks = 0, | ||
| Dash = 1, | ||
| Testnet = 2, | ||
| Regtest = 4, | ||
| Devnet = 8, | ||
| AllNetworks = 15, // Dash | Testnet | Regtest | Devnet | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize FFI layout for FFINetwork and avoid c_uint casts.
Use a fixed integer representation for the bitflags and operate on u32 throughout. This removes implicit signedness/width assumptions about C enums and tightens FFI stability.
Apply this diff:
-/// FFI Network type (bit flags for multiple networks)
-#[repr(C)]
+/// FFI Network type (bit flags for multiple networks)
+#[repr(u32)]
#[derive(Debug, Clone, Copy)]
pub enum FFINetwork {
@@
- pub fn parse_networks(&self) -> Vec<Network> {
- let flags = *self as c_uint;
+ pub fn parse_networks(&self) -> Vec<Network> {
+ let flags = *self as u32;
@@
- if flags == FFINetwork::NoNetworks as c_uint || flags == 0 {
+ if flags == FFINetwork::NoNetworks as u32 || flags == 0 {
@@
- if flags & (FFINetwork::Dash as c_uint) != 0 {
+ if flags & (FFINetwork::Dash as u32) != 0 {
@@
- if flags & (FFINetwork::Testnet as c_uint) != 0 {
+ if flags & (FFINetwork::Testnet as u32) != 0 {
@@
- if flags & (FFINetwork::Regtest as c_uint) != 0 {
+ if flags & (FFINetwork::Regtest as u32) != 0 {
@@
- if flags & (FFINetwork::Devnet as c_uint) != 0 {
+ if flags & (FFINetwork::Devnet as u32) != 0 {
@@
- pub fn try_into_single_network(&self) -> Option<Network> {
- let flags = *self as c_uint;
+ pub fn try_into_single_network(&self) -> Option<Network> {
+ let flags = *self as u32;
@@
- x if x == FFINetwork::Dash as c_uint => Some(Network::Dash),
- x if x == FFINetwork::Testnet as c_uint => Some(Network::Testnet),
- x if x == FFINetwork::Regtest as c_uint => Some(Network::Regtest),
- x if x == FFINetwork::Devnet as c_uint => Some(Network::Devnet),
+ x if x == FFINetwork::Dash as u32 => Some(Network::Dash),
+ x if x == FFINetwork::Testnet as u32 => Some(Network::Testnet),
+ x if x == FFINetwork::Regtest as u32 => Some(Network::Regtest),
+ x if x == FFINetwork::Devnet as u32 => Some(Network::Devnet),Also applies to: 19-47, 49-64
| /// FFI Balance type for representing wallet balances | ||
| #[repr(C)] | ||
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub struct FFIBalance { | ||
| /// Confirmed balance in satoshis | ||
| pub confirmed: u64, | ||
| /// Unconfirmed balance in satoshis | ||
| pub unconfirmed: u64, | ||
| /// Immature balance in satoshis (e.g., mining rewards) | ||
| pub immature: u64, | ||
| /// Total balance (confirmed + unconfirmed) in satoshis | ||
| pub total: u64, | ||
| } | ||
|
|
||
|
|
||
| impl From<key_wallet::WalletBalance> for FFIBalance { | ||
| fn from(balance: key_wallet::WalletBalance) -> Self { | ||
| FFIBalance { | ||
| confirmed: balance.confirmed, | ||
| unconfirmed: balance.unconfirmed, | ||
| immature: balance.locked, // Map locked to immature for now | ||
| total: balance.total, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
FFIBalance docs and semantics are inconsistent with the mapping.
Field ‘immature’ is sourced from WalletBalance::locked; the docs claim “immature (e.g., mining rewards)” and ‘total’ is documented as “confirmed + unconfirmed”, which likely omits locked. This is misleading at the FFI boundary.
Apply this doc-only fix now; consider renaming the field to locked in a future breaking change.
-/// FFI Balance type for representing wallet balances
+/// FFI Balance type for representing wallet balances
@@
- /// Immature balance in satoshis (e.g., mining rewards)
+ /// Locked balance in satoshis (temporarily unspendable; mapped from WalletBalance::locked)
pub immature: u64,
@@
- /// Total balance (confirmed + unconfirmed) in satoshis
+ /// Total balance in satoshis (as reported by WalletBalance::total)
pub total: u64,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// FFI Balance type for representing wallet balances | |
| #[repr(C)] | |
| #[derive(Debug, Clone, Copy, Default)] | |
| pub struct FFIBalance { | |
| /// Confirmed balance in satoshis | |
| pub confirmed: u64, | |
| /// Unconfirmed balance in satoshis | |
| pub unconfirmed: u64, | |
| /// Immature balance in satoshis (e.g., mining rewards) | |
| pub immature: u64, | |
| /// Total balance (confirmed + unconfirmed) in satoshis | |
| pub total: u64, | |
| } | |
| impl From<key_wallet::WalletBalance> for FFIBalance { | |
| fn from(balance: key_wallet::WalletBalance) -> Self { | |
| FFIBalance { | |
| confirmed: balance.confirmed, | |
| unconfirmed: balance.unconfirmed, | |
| immature: balance.locked, // Map locked to immature for now | |
| total: balance.total, | |
| } | |
| } | |
| } | |
| /// FFI Balance type for representing wallet balances | |
| #[repr(C)] | |
| #[derive(Debug, Clone, Copy, Default)] | |
| pub struct FFIBalance { | |
| /// Confirmed balance in satoshis | |
| pub confirmed: u64, | |
| /// Unconfirmed balance in satoshis | |
| pub unconfirmed: u64, | |
| /// Locked balance in satoshis (temporarily unspendable; mapped from WalletBalance::locked) | |
| pub immature: u64, | |
| /// Total balance in satoshis (as reported by WalletBalance::total) | |
| pub total: u64, | |
| } |
🧰 Tools
🪛 GitHub Actions: Continuous integration
[error] 102-102: Command 'cargo fmt --all -- --check' failed due to formatting differences. See diff at key-wallet-ffi/src/types.rs:102. Run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
In key-wallet-ffi/src/types.rs around lines 91 to 116, the FFIBalance field docs
are inconsistent with the mapping: `immature` is actually populated from
WalletBalance::locked and `total` currently documents "confirmed + unconfirmed"
while the struct includes locked; update the field comments to accurately
reflect the runtime mapping (e.g., change the `immature` comment to indicate it
represents locked funds from the wallet and change the `total` comment to state
it includes confirmed, unconfirmed, and locked/immature funds), and add a short
note that this is a mapping choice at the FFI boundary (and consider renaming
the field to `locked` in a future breaking change).
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
key-wallet-ffi/src/types.rs (2)
79-89: Align unknown-network fallback with project preference (Testnet).Current default maps unknown variants to Dash; prior guidance prefers defaulting to Testnet for unknown networks.
- _ => FFINetwork::Dash, // Default to Dash for unknown networks + _ => FFINetwork::Testnet, // Default to Testnet for unknown/unsupported variants
487-499: IdentityTopUp leakage via special_account_types (index forced to 0).SpecificAccounts pushes every FFIAccountType via to_account_type(0), which creates IdentityTopUp { registration_index: 0 } when requested here, duplicating/conflicting with topup_indices. Filter out IdentityTopUp (indexed type) from special_account_types.
- for &ffi_type in slice { - accounts.push(ffi_type.to_account_type(0)); - } + for &ffi_type in slice { + // Exclude indexed types; IdentityTopUp must be specified via `topup_indices` + if !matches!(ffi_type, FFIAccountType::IdentityTopUp) { + accounts.push(ffi_type.to_account_type(0)); + } + }key-wallet-manager/src/wallet_manager/mod.rs (3)
801-807: Same bug for change addresses.Propagate
mark_as_usedto next_change_address.- .next_change_address(Some(&wallet_account.account_xpub), true) + .next_change_address(Some(&wallet_account.account_xpub), mark_as_used)Also applies to: 832-843, 857-865, 873-883, 898-906
156-165: Avoid unnecessary clone when inserting wallet.No need to clone
walletbefore insertion.- let wallet_mut = wallet.clone(); - self.wallets.insert(wallet_id, wallet_mut); + self.wallets.insert(wallet_id, wallet);
634-639: Replace hardcodedtruewithmark_as_usedin allnext_receive_addresscalls
Inkey-wallet-manager/src/wallet_manager/mod.rs(lines 634–639, 666–676, 691–699, 706–718) you’re always passingtruetonext_receive_address, ignoring themark_as_usedparameter and causing duplicate marking logic. Update each- .next_receive_address(Some(&wallet_account.account_xpub), true) + .next_receive_address(Some(&wallet_account.account_xpub), mark_as_used)Additionally, ripgrep found four occurrences of
.next_receive_address(None, true)inkey-wallet/src/transaction_checking/transaction_router/tests/provider.rs(lines 162, 298, 435, 634). Decide whether those tests should be updated to use a test‐scopedmark_as_usedflag or remain hardcoded for isolation.
♻️ Duplicate comments (3)
key-wallet-ffi/src/types.rs (3)
91-104: Docs don’t match runtime mapping for FFIBalance.immature is mapped from WalletBalance::locked; total is WalletBalance::total (not confirmed + unconfirmed).
- /// Immature balance in satoshis (e.g., mining rewards) + /// Locked balance in satoshis (temporarily unspendable; mapped from WalletBalance::locked) @@ - /// Total balance (confirmed + unconfirmed) in satoshis + /// Total balance in satoshis (as reported by WalletBalance::total)
227-230: Docstring is stale: function does not return Option.Update to reflect the actual signature and semantics.
- /// Convert to AccountType with optional indices - /// Returns None if required parameters are missing (e.g., registration_index for IdentityTopUp) + /// Convert to AccountType using the provided index where applicable. + /// For IdentityTopUp, the index is used as registration_index.
7-17: Stabilize FFINetwork layout and stop relying on c_uint casts.Use a fixed underlying type for bitflags and operate on u32. This avoids ABI ambiguity with C enums and keeps the bit math explicit and portable.
-/// FFI Network type (bit flags for multiple networks) -#[repr(C)] +/// FFI Network type (bit flags for multiple networks) +#[repr(u32)] #[derive(Debug, Clone, Copy, PartialEq)] pub enum FFINetwork { NoNetworks = 0, Dash = 1, Testnet = 2, Regtest = 4, Devnet = 8, AllNetworks = 15, // Dash | Testnet | Regtest | Devnet } @@ - pub fn parse_networks(&self) -> Vec<Network> { - let flags = *self as c_uint; + pub fn parse_networks(&self) -> Vec<Network> { + let flags = *self as u32; @@ - if flags == FFINetwork::NoNetworks as c_uint || flags == 0 { + if flags == FFINetwork::NoNetworks as u32 || flags == 0 { @@ - if flags & (FFINetwork::Dash as c_uint) != 0 { + if flags & (FFINetwork::Dash as u32) != 0 { @@ - if flags & (FFINetwork::Testnet as c_uint) != 0 { + if flags & (FFINetwork::Testnet as u32) != 0 { @@ - if flags & (FFINetwork::Regtest as c_uint) != 0 { + if flags & (FFINetwork::Regtest as u32) != 0 { @@ - if flags & (FFINetwork::Devnet as c_uint) != 0 { + if flags & (FFINetwork::Devnet as u32) != 0 { @@ - pub fn try_into_single_network(&self) -> Option<Network> { - let flags = *self as c_uint; + pub fn try_into_single_network(&self) -> Option<Network> { + let flags = *self as u32; @@ - x if x == FFINetwork::Dash as c_uint => Some(Network::Dash), - x if x == FFINetwork::Testnet as c_uint => Some(Network::Testnet), - x if x == FFINetwork::Regtest as c_uint => Some(Network::Regtest), - x if x == FFINetwork::Devnet as c_uint => Some(Network::Devnet), + x if x == FFINetwork::Dash as u32 => Some(Network::Dash), + x if x == FFINetwork::Testnet as u32 => Some(Network::Testnet), + x if x == FFINetwork::Regtest as u32 => Some(Network::Regtest), + x if x == FFINetwork::Devnet as u32 => Some(Network::Devnet),Also applies to: 19-47
🧹 Nitpick comments (15)
key-wallet-ffi/src/types.rs (1)
66-76: TryFrom error message is okay; consider including the offending flags.Optional: include numeric flags to aid debugging across FFI boundaries.
key-wallet-ffi/src/account.rs (2)
303-322: Duplicate logic across account_get_account_type variants.Minor: factor into a shared helper to reduce drift, e.g., fn account_type_with_index(at: &key_wallet::AccountType) -> (FFIAccountType, c_uint).
Also applies to: 418-438, 535-555
582-620: Counting accounts: avoid bool-to-usize casts to keep clippy clean.clippy -D warnings may flag is_some() as usize. Use explicit addition.
- let count = accounts.standard_bip44_accounts.len() - + accounts.standard_bip32_accounts.len() - + accounts.coinjoin_accounts.len() - + accounts.identity_registration.is_some() as usize - + accounts.identity_topup.len(); + let mut count = accounts.standard_bip44_accounts.len() + + accounts.standard_bip32_accounts.len() + + accounts.coinjoin_accounts.len() + + accounts.identity_topup.len(); + if accounts.identity_registration.is_some() { count += 1; }key-wallet-ffi/src/managed_account_collection.rs (1)
90-145: Good: Retrieves managed wallet info, clones collection, and frees wallet info.Consider setting error to success explicitly on success to make the contract obvious.
key-wallet-ffi/src/account_tests.rs (1)
195-200: Consider using assertions helper for xpub validation.The xpub validation could be more robust by checking the full format beyond just the prefix.
Consider adding a helper function to validate xpub format more thoroughly:
- assert!(xpub_string.starts_with("tpub")); // Testnet xpub should start with tpub + assert!(xpub_string.starts_with("tpub"), "Expected testnet xpub to start with 'tpub', got: {}", xpub_string); + assert!(xpub_string.len() > 100, "xpub string seems too short: {}", xpub_string.len());key-wallet-ffi/tests/test_managed_account_collection.rs (1)
15-16: Consider extracting test constants to a shared test utilities module.The
TEST_MNEMONICconstant is duplicated across multiple test files. Consider creating a shared test utilities module to avoid duplication and ensure consistency.Create a shared test utilities module:
// In tests/common/mod.rs or similar pub const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; pub const TEST_PASSPHRASE: &str = "";Then import it in test files:
-const TEST_MNEMONIC: &str = - "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; +mod common; +use common::TEST_MNEMONIC;key-wallet-manager/tests/integration_test.rs (2)
143-152: UTXO management tests could be expanded.The test acknowledges that UTXOs are created by processing transactions but doesn't actually test this flow. Consider adding a test that processes a transaction to create UTXOs.
Would you like me to generate a test that processes a transaction to create UTXOs and verify the balance updates correctly?
86-105: DocumentInvalidNetworkbehavior
Inkey-wallet-manager/src/wallet_manager/mod.rs, expand the/// Invalid networkdoc comment (and update the crate README or API docs) to note thatWalletError::InvalidNetworkis the expected result when account collections haven’t been initialized for a given network.key-wallet-manager/examples/wallet_creation.rs (1)
160-169: Minor: iterate wallet IDs by reference to avoid copy.WalletId is a [u8; 32]. Iterating by value copies; it’s negligible but you can iterate by reference explicitly for clarity.
Apply:
- for (i, wallet_id) in [wallet_id, wallet_id2].iter().enumerate() { - match manager.get_wallet_balance(wallet_id) { + for (i, wid) in [wallet_id, wallet_id2].iter().enumerate() { + match manager.get_wallet_balance(wid) {key-wallet-ffi/src/wallet_manager.rs (3)
205-219: Document sensitive output semantics.When downgrade_to_pubkey_wallet is false, serialized bytes may include private key material. Callers must store/transport securely and zeroize after use.
/// - The caller must free the returned wallet_bytes using wallet_manager_free_wallet_bytes() +/// - SECURITY: If `downgrade_to_pubkey_wallet` is false, `wallet_bytes_out` contains private keys. +/// Callers must handle it securely and zeroize the buffer after use by calling +/// `wallet_manager_free_wallet_bytes` (which zeroizes before free).
478-483: Add freeing guidance to docs for wallet IDs buffer.Make it explicit that callers must free with wallet_manager_free_wallet_ids.
/// - `count_out` must be a valid pointer to receive the count /// - `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 +/// - The returned buffer must be freed with `wallet_manager_free_wallet_ids`
1011-1018: Mention how to free returned address arrays.The function allocates an array of C strings; callers should free via wallet_manager_free_addresses.
/// - `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 +/// - The returned array must be freed with `wallet_manager_free_addresses`key-wallet-manager/src/wallet_manager/mod.rs (1)
1067-1091: Derive errors with thiserror for better interop.Adopt thiserror to provide Display and source automatically, keeping MSRV-compatible.
-#[derive(Debug)] -pub enum WalletError { +use thiserror::Error; +#[derive(Error, Debug)] +pub enum WalletError { - WalletCreation(String), + #[error("Wallet creation failed: {0}")] + WalletCreation(String), - WalletNotFound(WalletId), + #[error("Wallet not found: {0:x?}")] + WalletNotFound(WalletId), - WalletExists(WalletId), + #[error("Wallet already exists: {0:x?}")] + WalletExists(WalletId), - InvalidMnemonic(String), + #[error("Invalid mnemonic: {0}")] + InvalidMnemonic(String), ... }key-wallet-ffi/src/account_collection.rs (2)
1521-1522: Fix misleading test comment.It says “AllAccounts” but the test uses SpecificAccounts. Update for clarity.
- // With AllAccounts but empty lists, collection should still exist + // With SpecificAccounts and empty lists, collection may be absent or empty
145-161: Optional: consider consistent error reporting for not-found cases.Today these return null without setting FFIError. Exposing an optional FFIError* out-param (mirroring wallet_get_account_collection) would help callers distinguish “not found” vs misuse.
Happy to sketch a minimal pattern you can reuse across these getters if you want it in this PR.
Also applies to: 210-226, 275-291, 339-354, 380-396, 442-457, 483-498, 526-541, 567-582
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
dash-spv/src/main.rs(1 hunks)key-wallet-ffi/Cargo.toml(2 hunks)key-wallet-ffi/IMPORT_WALLET_FFI.md(1 hunks)key-wallet-ffi/src/account.rs(6 hunks)key-wallet-ffi/src/account_collection.rs(1 hunks)key-wallet-ffi/src/account_tests.rs(5 hunks)key-wallet-ffi/src/address_pool.rs(11 hunks)key-wallet-ffi/src/lib.rs(2 hunks)key-wallet-ffi/src/managed_account.rs(1 hunks)key-wallet-ffi/src/managed_account_collection.rs(1 hunks)key-wallet-ffi/src/types.rs(8 hunks)key-wallet-ffi/src/wallet_manager.rs(10 hunks)key-wallet-ffi/tests/test_account_collection.rs(1 hunks)key-wallet-ffi/tests/test_managed_account_collection.rs(1 hunks)key-wallet-manager/examples/wallet_creation.rs(3 hunks)key-wallet-manager/src/wallet_manager/mod.rs(5 hunks)key-wallet-manager/tests/integration_test.rs(5 hunks)key-wallet-manager/tests/test_serialized_wallets.rs(1 hunks)key-wallet/src/managed_account/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet-manager/tests/test_serialized_wallets.rs
- key-wallet-ffi/Cargo.toml
- dash-spv/src/main.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with the thiserror crate and propagate errors appropriately
Use the tokio runtime for async operations
Adhere to MSRV: code must compile on Rust 1.89 (avoid newer language/features)
Keep code formatted with rustfmt (cargo fmt)
Keep the codebase clippy-clean (clippy with -D warnings)
Files:
key-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/lib.rskey-wallet-manager/tests/integration_test.rskey-wallet-ffi/src/account_tests.rskey-wallet/src/managed_account/mod.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-ffi/src/types.rs
{dash-network-ffi/**/*.rs,dash-spv-ffi/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.{swift,m,mm,c,h}}
📄 CodeRabbit inference engine (CLAUDE.md)
Be especially careful with memory safety at FFI boundaries
Files:
key-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/types.rs
{**/*.rs,**/Cargo.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use feature flags and conditional compilation for optional features
Files:
key-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/lib.rskey-wallet-manager/tests/integration_test.rskey-wallet-ffi/src/account_tests.rskey-wallet/src/managed_account/mod.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-ffi/src/types.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Write unit tests for new functionality
Use proptest for property-based testing where appropriate
Files:
key-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-manager/tests/integration_test.rs
{key-wallet/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/account_tests.rskey-wallet/src/managed_account/mod.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/types.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
key-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/IMPORT_WALLET_FFI.mdkey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/account.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/src/lib.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-08-25T17:15:15.391Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:15.391Z
Learning: Applies to {dash-network-ffi/**/*.rs,dash-spv-ffi/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.{swift,m,mm,c,h}} : Be especially careful with memory safety at FFI boundaries
Applied to files:
key-wallet-ffi/src/lib.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-08-25T17:15:15.391Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:15.391Z
Learning: Applies to {dash-network,dash-spv}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
key-wallet-ffi/src/account_tests.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#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-ffi/src/account_tests.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/types.rs
🧬 Code graph analysis (12)
key-wallet-ffi/tests/test_account_collection.rs (1)
key-wallet-ffi/src/wallet.rs (2)
wallet_create_from_mnemonic_with_options(29-131)wallet_free(443-449)
key-wallet-ffi/tests/test_managed_account_collection.rs (5)
key-wallet-ffi/src/managed_account.rs (4)
error(60-70)success(51-57)new(26-30)managed_account_free(476-480)key-wallet-ffi/src/wallet_manager.rs (5)
wallet_manager_add_wallet_from_mnemonic_with_options(51-155)wallet_manager_create(31-38)wallet_manager_free(1246-1252)wallet_manager_free_wallet_ids(1263-1270)wallet_manager_get_wallet_ids(478-522)key-wallet-ffi/src/managed_account_collection.rs (20)
new(25-31)managed_wallet_get_account_collection(90-145)managed_account_collection_count(782-824)managed_account_collection_get_bip44_indices(201-228)managed_account_collection_get_bip44_account(172-190)managed_account_collection_free(154-160)managed_account_collection_get_bip32_indices(268-295)managed_account_collection_get_coinjoin_indices(335-361)managed_account_collection_has_identity_registration(397-406)managed_account_collection_has_identity_invitation(547-556)managed_account_collection_has_provider_voting_keys(592-601)managed_account_collection_has_provider_owner_keys(635-644)managed_account_collection_get_identity_registration(373-389)managed_account_collection_get_provider_voting_keys(568-584)managed_account_collection_summary(837-966)managed_account_collection_summary_data(980-1067)managed_account_collection_summary_free(1076-1117)managed_account_collection_get_bip32_account(240-257)managed_account_collection_get_coinjoin_account(307-324)managed_account_collection_get_identity_topup(416-433)key-wallet-ffi/src/types.rs (1)
default_options(386-400)key-wallet-ffi/src/utils.rs (1)
string_free(17-23)
key-wallet-ffi/src/managed_account.rs (6)
key-wallet-ffi/src/wallet_manager.rs (7)
wallet_manager_get_managed_wallet_info(598-644)wallet_manager_get_wallet(536-584)wallet_manager_add_wallet_from_mnemonic_with_options(51-155)wallet_manager_create(31-38)wallet_manager_free(1246-1252)wallet_manager_free_wallet_ids(1263-1270)wallet_manager_get_wallet_ids(478-522)key-wallet-ffi/src/managed_wallet.rs (1)
managed_wallet_info_free(761-767)key-wallet-ffi/src/error.rs (1)
set_success(63-69)key-wallet-ffi/src/wallet.rs (1)
wallet_free_const(464-472)key-wallet-ffi/src/address_pool.rs (1)
address_pool_free(216-220)key-wallet-ffi/src/types.rs (1)
default_options(386-400)
key-wallet-ffi/src/lib.rs (2)
key-wallet-ffi/src/types.rs (1)
error(162-173)key-wallet-ffi/src/error.rs (1)
error(44-49)
key-wallet-manager/tests/integration_test.rs (2)
key-wallet-manager/src/wallet_manager/mod.rs (3)
wallet_count(355-357)new(70-76)new(104-110)key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
new(54-64)
key-wallet-ffi/src/account_tests.rs (3)
key-wallet-ffi/src/types.rs (1)
error(162-173)key-wallet/src/managed_account/mod.rs (4)
network(790-792)index(141-143)account_type(782-784)is_watch_only(802-804)key-wallet/src/account/mod.rs (4)
network(153-155)index(117-119)account_type(149-151)is_watch_only(157-159)
key-wallet/src/managed_account/mod.rs (2)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
monitored_addresses(60-60)monitored_addresses(162-173)key-wallet/src/managed_account/managed_account_trait.rs (2)
monitored_addresses(48-48)monitored_addresses_mut(51-51)
key-wallet-ffi/src/address_pool.rs (5)
key-wallet-ffi/src/error.rs (4)
error(44-49)set_error(53-59)set_success(63-69)success(36-41)key-wallet-ffi/src/utils.rs (1)
rust_string_to_c(26-31)key-wallet/src/managed_account/address_pool.rs (4)
network(1099-1102)new(356-371)new(1058-1068)address_info(814-816)key-wallet-ffi/src/managed_account.rs (2)
managed_account_get_external_address_pool(596-620)managed_wallet_get_account(86-203)key-wallet-ffi/src/wallet_manager.rs (5)
wallet_manager_add_wallet_from_mnemonic_with_options(51-155)wallet_manager_create(31-38)wallet_manager_free(1246-1252)wallet_manager_free_wallet_ids(1263-1270)wallet_manager_get_wallet_ids(478-522)
key-wallet-ffi/src/managed_account_collection.rs (3)
key-wallet-ffi/src/managed_account.rs (2)
error(60-70)new(26-30)key-wallet-ffi/src/wallet_manager.rs (1)
wallet_manager_get_managed_wallet_info(598-644)key-wallet-ffi/src/managed_wallet.rs (1)
managed_wallet_info_free(761-767)
key-wallet-manager/src/wallet_manager/mod.rs (5)
key-wallet/src/wallet/mod.rs (2)
zeroize(136-184)wallet_id(115-115)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (6)
wallet_id(30-30)wallet_id(122-124)from_wallet(23-23)from_wallet(114-116)accounts(81-81)accounts(241-243)key-wallet/src/mnemonic.rs (3)
from_phrase(220-227)generate(106-129)generate(133-144)key-wallet/src/wallet/initialization.rs (4)
from_mnemonic(144-163)from_mnemonic_with_passphrase(173-199)from_extended_key(336-351)from_xpub(229-245)key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
from_wallet(80-99)
key-wallet-ffi/src/wallet_manager.rs (2)
key-wallet-ffi/src/types.rs (1)
error(162-173)key-wallet-ffi/src/error.rs (3)
error(44-49)set_error(53-59)set_success(63-69)
key-wallet-ffi/src/types.rs (3)
key-wallet/src/account/mod.rs (4)
new(72-79)network(153-155)index(117-119)account_type(149-151)key-wallet/src/account/account_type.rs (2)
from(69-94)index(100-121)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(69-69)balance(194-196)
🪛 LanguageTool
key-wallet-ffi/IMPORT_WALLET_FFI.md
[grammar] ~21-~21: There might be a mistake here.
Context: ... Pointer to an FFIWalletManager instance - wallet_bytes: Pointer to bincode-serialized wallet b...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...inter to bincode-serialized wallet bytes - wallet_bytes_len: Length of the wallet bytes - `wallet_i...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...t_bytes_len: Length of the wallet bytes - wallet_id_out`: Pointer to a 32-byte buffer that will ...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...e buffer that will receive the wallet ID - error: Pointer to an FFIError structure for e...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...2.0.0-rc.3). The serialization includes: - Wallet seed and key material - Account i...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...includes: - Wallet seed and key material - Account information - Address pools and ...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...d and key material - Account information - Address pools and indices - Transaction ...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ... information - Address pools and indices - Transaction history - Other wallet metad...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ... pools and indices - Transaction history - Other wallet metadata ## Safety Conside...
(QB_NEW_EN)
[style] ~104-~104: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...ocking, so this function is thread-safe with respect to other wallet manager operations on the ...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
⏰ 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_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Clippy (Non-strict)
- GitHub Check: SPV Components Tests
🔇 Additional comments (33)
key-wallet/src/managed_account/mod.rs (3)
74-75: LGTM: Properly initialized.
270-274: Inconsistency with wallet_info_interface monitored_addresses aggregation.WalletInfoInterface::monitored_addresses() currently collects account.all_addresses(), ignoring this new monitored_addresses set. Either rename that method to all_addresses or update it to use ManagedAccountTrait::monitored_addresses.
I can prep a follow-up diff in wallet_info_interface.rs to align semantics. Proceed?
Also applies to: 781-787
830-836: Deprecatemonitored_addresses_mutand add validated mutators
Replace the mutable accessor in the trait and impl withadd_monitored_address(&mut self, address: Address)andremove_monitored_address(&mut self, address: &Address)that enforce validity; confirm no external consumers rely onmonitored_addresses_mutbefore removal.key-wallet-ffi/src/account.rs (4)
82-116: LGTM: Validates wallet ptr, enforces single-network, returns opaque handle.
224-232: Memory management for error_message is correct.account_result_free_error safely frees and nulls the CString.
283-293: Returning a pointer into inner storage requires careful lifetime handling.The pointer is valid only while the account handle lives; the doc comments state this. Consider returning a copy (32-byte heap) to avoid use-after-free risks on the C side.
I can add a variant that returns an owned 32-byte buffer and a corresponding free function if desired.
350-367: Public-key string encodings: confirm consumer expectations.BLS/EdDSA xpubs are returned as hex of raw key bytes (no version/prefix). Ensure this matches SDK expectations; otherwise, add a type tag or length prefix.
Also applies to: 467-484
key-wallet-ffi/src/managed_account_collection.rs (3)
837-966: Summary string builder: solid and user-friendly.No issues.
980-1067: Summary struct allocation/free looks correct.Fields behind cfgs are handled, and arrays are freed via Vec::from_raw_parts.
Also applies to: 1076-1117
201-229: free_u32_array is exported and documented. All index-returning FFI functions include doc comments instructing callers to free returned arrays withfree_u32_array, which is defined and exposed as#[no_mangle] pub unsafe extern "C" fn free_u32_arrayinkey-wallet-ffi/src/account_collection.rs.key-wallet-ffi/src/lib.rs (1)
8-16: LGTM: module wiring and re-exports updated coherently.Also applies to: 34-35
key-wallet-ffi/IMPORT_WALLET_FFI.md (1)
1-104: Documentation looks comprehensive and well-structured!The documentation provides clear guidance on the
wallet_manager_import_wallet_from_bytesFFI function, including proper safety considerations, error handling, and usage examples. The thread safety note is particularly valuable for users of this API.key-wallet-ffi/src/account_tests.rs (3)
11-26: Good improvement with typed FFIAccountType!The migration from raw integers to the typed
FFIAccountTypeenum improves type safety at the FFI boundary. The test properly validates error handling for null wallet pointers.
150-164: Comprehensive enum value validation!Good test coverage for verifying the numerical values of
FFIAccountTypevariants. This ensures ABI compatibility across FFI boundaries.
238-262: Excellent null safety testing!Comprehensive coverage of null pointer handling for all getter functions. This is crucial for FFI safety and preventing crashes in calling code.
key-wallet-ffi/tests/test_account_collection.rs (2)
13-142: Comprehensive test coverage with proper memory management!Excellent test that exercises various account types and properly cleans up allocated memory using
free_u32_array. The assertions validate the expected counts for each account type.
183-196: Good null safety validation!Testing that null pointers don't crash the API is essential for FFI robustness.
key-wallet-ffi/tests/test_managed_account_collection.rs (3)
94-224: Well-structured test with comprehensive account type coverage!The test properly exercises special account types and validates their existence. Good memory management with proper cleanup of all allocated resources.
385-419: Excellent summary data validation!The test thoroughly validates the structure and contents of the summary data, including proper array slice validation and boolean flag checks.
512-562: Good edge case testing for wrong network scenarios!Testing behavior when requesting accounts for a different network than the wallet was created on is important for multi-network support.
key-wallet-manager/tests/integration_test.rs (1)
29-38: API improvements with direct WalletId return!Good change to have wallet creation methods return
WalletIddirectly. This aligns with the learning about controlling wallet lifecycle through the manager APIs.key-wallet-manager/examples/wallet_creation.rs (1)
26-31: API migration looks correct.Correctly adopts WalletManager::{create_wallet_with_random_mnemonic,create_wallet_from_mnemonic} returning WalletId and passes networks as a slice. No issues spotted.
Also applies to: 49-56, 57-67, 132-137, 139-145
key-wallet-ffi/src/wallet_manager.rs (2)
371-466: LGTM: import path and error mapping.import_wallet_from_bytes validates inputs, maps errors, pushes wallet_id, and copies out ID correctly.
535-541: Const wallet pointer return is correct.Returning *const FFIWallet aligns with the read-only FFI design. Matches prior guidance.
key-wallet-manager/src/wallet_manager/mod.rs (1)
190-270: Serialized-bytes path looks sound.Downgrade flow zeroizes the private wallet before drop and uses feature-gated bincode v2 API. Good separation and checks for duplicates.
key-wallet-ffi/src/address_pool.rs (2)
851-880: LGTM: single-address accessor and error handling.Returns allocated FFIAddressInfo and provides matching free; bounds are checked.
897-946: LGTM: range accessor semantics and cleanup.Validates inputs, returns null with NotFound on empty ranges, and provides array free helper.
key-wallet-ffi/src/managed_account.rs (1)
86-103: LGTM: managed account retrieval and cleanup.Fetches via managed_wallet_info, converts account type, frees wallet-info handle on exit. Error propagation uses FFIManagedAccountResult consistently.
Also applies to: 121-137
key-wallet-ffi/src/account_collection.rs (5)
84-106: Good null checks + clear error reporting in wallet_get_account_collection.Fits the “const wallet” guidance and validates single-network selection via try_into(); error propagation is consistent.
744-786: Count computation looks correct and feature-gated bits are handled.
799-928: String summary API LGTM.
- Handles null input.
- Produces user-friendly, sorted summaries.
- Uses CString::new safely.
1038-1079: Summary free routine is sound.Correctly reconstructs Vecs from raw parts and drops the Box; no double-free paths observed.
608-636: Ensure stable C ABI by adding an unconditional pointer alias
Add an always-availableaccount_collection_get_provider_operator_keys_ptr(collection: *const FFIAccountCollection) -> *mut c_voidthat forwards to the typedblsvariant and returns null otherwise, preserving the existing API; regenerate cbindgen headers with and withoutblsto confirm identical prototypes.
| #[no_mangle] | ||
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys( | ||
| collection: *const FFIAccountCollection, | ||
| ) -> *mut crate::account::FFIEdDSAAccount { | ||
| if collection.is_null() { | ||
| return ptr::null_mut(); | ||
| } | ||
|
|
||
| let collection = &*collection; | ||
| match &collection.collection.provider_platform_keys { | ||
| Some(account) => { | ||
| let ffi_account = crate::account::FFIEdDSAAccount::new(account); | ||
| Box::into_raw(Box::new(ffi_account)) | ||
| } | ||
| None => ptr::null_mut(), | ||
| } | ||
| } | ||
|
|
||
| /// Get the provider platform keys account if it exists (stub when EdDSA is disabled) | ||
| #[cfg(not(feature = "eddsa"))] | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys( | ||
| _collection: *const FFIAccountCollection, | ||
| ) -> *mut std::os::raw::c_void { | ||
| // EdDSA feature not enabled, always return null | ||
| ptr::null_mut() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the ABI-stable pointer pattern for EdDSA platform keys.
Same rationale as BLS: provide an always-available …_ptr symbol with void* return type.
@@
#[cfg(feature = "eddsa")]
#[no_mangle]
pub unsafe extern "C" fn account_collection_get_provider_platform_keys(
collection: *const FFIAccountCollection,
) -> *mut crate::account::FFIEdDSAAccount {
@@
}
+// Always-available pointer form with a stable signature across feature flags
+#[no_mangle]
+pub unsafe extern "C" fn account_collection_get_provider_platform_keys_ptr(
+ collection: *const FFIAccountCollection,
+) -> *mut std::os::raw::c_void {
+ if collection.is_null() {
+ return ptr::null_mut();
+ }
+ #[cfg(feature = "eddsa")]
+ {
+ let acc = account_collection_get_provider_platform_keys(collection);
+ acc.cast()
+ }
+ #[cfg(not(feature = "eddsa"))]
+ {
+ ptr::null_mut()
+ }
+}
@@
#[cfg(not(feature = "eddsa"))]
#[no_mangle]
pub unsafe extern "C" fn account_collection_get_provider_platform_keys(
_collection: *const FFIAccountCollection,
) -> *mut std::os::raw::c_void {
// EdDSA feature not enabled, always return null
ptr::null_mut()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys( | |
| collection: *const FFIAccountCollection, | |
| ) -> *mut crate::account::FFIEdDSAAccount { | |
| if collection.is_null() { | |
| return ptr::null_mut(); | |
| } | |
| let collection = &*collection; | |
| match &collection.collection.provider_platform_keys { | |
| Some(account) => { | |
| let ffi_account = crate::account::FFIEdDSAAccount::new(account); | |
| Box::into_raw(Box::new(ffi_account)) | |
| } | |
| None => ptr::null_mut(), | |
| } | |
| } | |
| /// Get the provider platform keys account if it exists (stub when EdDSA is disabled) | |
| #[cfg(not(feature = "eddsa"))] | |
| #[no_mangle] | |
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys( | |
| _collection: *const FFIAccountCollection, | |
| ) -> *mut std::os::raw::c_void { | |
| // EdDSA feature not enabled, always return null | |
| ptr::null_mut() | |
| } | |
| #[no_mangle] | |
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys( | |
| collection: *const FFIAccountCollection, | |
| ) -> *mut crate::account::FFIEdDSAAccount { | |
| if collection.is_null() { | |
| return ptr::null_mut(); | |
| } | |
| let collection = &*collection; | |
| match &collection.collection.provider_platform_keys { | |
| Some(account) => { | |
| let ffi_account = crate::account::FFIEdDSAAccount::new(account); | |
| Box::into_raw(Box::new(ffi_account)) | |
| } | |
| None => ptr::null_mut(), | |
| } | |
| } | |
| // Always-available pointer form with a stable signature across feature flags | |
| #[no_mangle] | |
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys_ptr( | |
| collection: *const FFIAccountCollection, | |
| ) -> *mut std::os::raw::c_void { | |
| if collection.is_null() { | |
| return ptr::null_mut(); | |
| } | |
| #[cfg(feature = "eddsa")] | |
| { | |
| let acc = account_collection_get_provider_platform_keys(collection); | |
| acc.cast() | |
| } | |
| #[cfg(not(feature = "eddsa"))] | |
| { | |
| ptr::null_mut() | |
| } | |
| } | |
| /// Get the provider platform keys account if it exists (stub when EdDSA is disabled) | |
| #[cfg(not(feature = "eddsa"))] | |
| #[no_mangle] | |
| pub unsafe extern "C" fn account_collection_get_provider_platform_keys( | |
| _collection: *const FFIAccountCollection, | |
| ) -> *mut std::os::raw::c_void { | |
| // EdDSA feature not enabled, always return null | |
| ptr::null_mut() | |
| } |
| pub unsafe extern "C" fn managed_account_get_external_address_pool( | ||
| account: *const FFIManagedAccount, | ||
| ) -> *mut FFIAddressPool { | ||
| if account.is_null() { | ||
| return std::ptr::null_mut(); | ||
| } | ||
|
|
||
| let account = &*account; | ||
| let managed_account = account.inner(); | ||
|
|
||
| // Get external address pool if this is a standard account | ||
| match &managed_account.account_type { | ||
| key_wallet::managed_account::managed_account_type::ManagedAccountType::Standard { | ||
| external_addresses, | ||
| .. | ||
| } => { | ||
| let ffi_pool = FFIAddressPool { | ||
| pool: external_addresses as *const AddressPool as *mut AddressPool, | ||
| pool_type: FFIAddressPoolType::External, | ||
| }; | ||
| Box::into_raw(Box::new(ffi_pool)) | ||
| } |
There was a problem hiding this comment.
Tie FFIAddressPool to account lifetime to prevent UAF.
Constructors must pass an Arc clone to FFIAddressPool so the underlying ManagedAccount outlives the pool handle.
// external
- let ffi_pool = FFIAddressPool {
- pool: external_addresses as *const AddressPool as *mut AddressPool,
- pool_type: FFIAddressPoolType::External,
- };
+ let ffi_pool = FFIAddressPool {
+ pool: external_addresses as *const AddressPool as *mut AddressPool,
+ _account_keepalive: account.account.clone(),
+ pool_type: FFIAddressPoolType::External,
+ };
// internal
- let ffi_pool = FFIAddressPool {
- pool: internal_addresses as *const AddressPool as *mut AddressPool,
- pool_type: FFIAddressPoolType::Internal,
- };
+ let ffi_pool = FFIAddressPool {
+ pool: internal_addresses as *const AddressPool as *mut AddressPool,
+ _account_keepalive: account.account.clone(),
+ pool_type: FFIAddressPoolType::Internal,
+ };
// generic by type
- let ffi_pool = FFIAddressPool {
- pool: external_addresses as *const AddressPool as *mut AddressPool,
- pool_type: FFIAddressPoolType::External,
- };
+ let ffi_pool = FFIAddressPool {
+ pool: external_addresses as *const AddressPool as *mut AddressPool,
+ _account_keepalive: account.account.clone(),
+ pool_type: FFIAddressPoolType::External,
+ };
...
- let ffi_pool = FFIAddressPool {
- pool: internal_addresses as *const AddressPool as *mut AddressPool,
- pool_type: FFIAddressPoolType::Internal,
- };
+ let ffi_pool = FFIAddressPool {
+ pool: internal_addresses as *const AddressPool as *mut AddressPool,
+ _account_keepalive: account.account.clone(),
+ pool_type: FFIAddressPoolType::Internal,
+ };
...
- let ffi_pool = FFIAddressPool {
- pool: pool_ref as *const AddressPool as *mut AddressPool,
- pool_type: FFIAddressPoolType::Single,
- };
+ let ffi_pool = FFIAddressPool {
+ pool: pool_ref as *const AddressPool as *mut AddressPool,
+ _account_keepalive: account.account.clone(),
+ pool_type: FFIAddressPoolType::Single,
+ };Also applies to: 632-653, 684-763
🤖 Prompt for AI Agents
In key-wallet-ffi/src/managed_account.rs around lines 596 to 617, the
FFIAddressPool returned currently contains a raw pointer to the account's
AddressPool but no ownership tie to the ManagedAccount, risking use-after-free;
fix by updating FFIAddressPool to hold an Arc<ManagedAccount> (or Arc to the
inner owner that holds AddressPool) and when creating ffi_pool clone and store
account.inner_arc (or wrap managed_account in Arc::clone(&account_arc)) so the
Arc is moved into the FFIAddressPool instance; apply the same pattern to the
other creation sites at lines 632-653 and 684-763 so every FFIAddressPool keeps
an Arc clone that ensures the underlying ManagedAccount outlives the pool
handle.
Summary by CodeRabbit
New Features
Breaking Changes
Security
Refactor
Chores / Tests