fix: clean up ffi methods #124
Conversation
WalkthroughThe PR migrates the key-wallet FFI API from a single-network enum (FFINetwork) to a bit-flag enum (FFINetworks) across headers, Rust modules, and tests. It also adjusts provider-key collection APIs to return void pointers, updates UTXO wrappers’ return types, and revises the Swift Dash SPV FFI header types and array struct. One minor import reordering occurs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as C Caller
participant F as FFI Wallet (Rust)
participant T as Types::FFINetworks
participant W as Core Wallet
C->>F: wallet_create_from_mnemonic_with_options(mnemonic, passphrase, networks: FFINetworks, opts)
F->>T: networks.parse_networks()
T-->>F: Vec<Network>
F->>W: create_wallet(mnemonic, passphrase, networks[])
W-->>F: *Wallet
F-->>C: *FFIWallet
sequenceDiagram
autonumber
actor C as C Caller
participant F as FFI Address
participant T as Types::FFINetworks
C->>F: address_validate(addr, network: FFINetworks)
F->>T: TryInto<Network>(network)
alt exactly one network
T-->>F: Ok(Network)
F-->>C: bool (validation result)
else invalid (0 or multiple)
T-->>F: Err("Must specify exactly one network")
F-->>C: false + set error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (24)
key-wallet-ffi/src/transaction_checking.rs (1)
171-214: Minor: share FFITransactionContext definition across modulesThis enum is also defined in src/transaction.rs. Duplicated FFI types are error-prone on the C side. Prefer a single definition in crate::types (or re-export from one module).
key-wallet-ffi/src/address_pool.rs (3)
672-681: Enforce network consistency when marking address usedRight now we parse as NetworkUnchecked and assume the address matches the provided network. Per project learnings, we should reject mismatches explicitly.
- // Assume the address uses the same network we're working with - let address = unchecked_addr.assume_checked(); + // Enforce that the address matches the requested network + let address = match unchecked_addr.require_network(network_rust) { + Ok(addr) => addr, + Err(_) => { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Address network does not match provided network".to_string(), + ); + return false; + } + };If require_network isn’t available in the current dashcore version, we can compare the address’s network to network_rust and error on mismatch.
Also applies to: 685-694, 709-727
971-981: Incorrect deallocation of boxed slices; use slice_from_raw_parts_mutBox::from_raw expects a pointer, not a borrowed slice. As written, this is UB. Reconstruct the original Box<[u8]> using std::ptr::slice_from_raw_parts_mut.
- if !info.script_pubkey.is_null() && info.script_pubkey_len > 0 { - let _ = Box::from_raw(std::slice::from_raw_parts_mut( - info.script_pubkey, - info.script_pubkey_len, - )); - } + if !info.script_pubkey.is_null() && info.script_pubkey_len > 0 { + let ptr = std::ptr::slice_from_raw_parts_mut(info.script_pubkey, info.script_pubkey_len); + let _ = Box::from_raw(ptr); + } @@ - if !info.public_key.is_null() && info.public_key_len > 0 { - let _ = - Box::from_raw(std::slice::from_raw_parts_mut(info.public_key, info.public_key_len)); - } + if !info.public_key.is_null() && info.public_key_len > 0 { + let ptr = std::ptr::slice_from_raw_parts_mut(info.public_key, info.public_key_len); + let _ = Box::from_raw(ptr); + }
572-643: Enforce a maximum ontarget_indexusing the pool’sgap_limit
Before computingneeded = target_index − current, add:if target_index > current + pool.gap_limit { FFIError::set_error( error, FFIErrorCode::InvalidInput, format!("Requested index {} exceeds gap limit {}, refusing unbounded generation", target_index, pool.gap_limit), ); return false; }This prevents OOM from untrusted FFI inputs.
key-wallet-ffi/src/transaction.rs (1)
100-109: Duplicate FFITransactionContext and FFITransactionCheckResult definitions across modulesThis file redefines both types, while transaction_checking.rs defines compatible-but-different versions (and the result struct has different fields). This will desynchronize the public C header and break ABI expectations.
-/// Transaction context for checking -#[repr(C)] -pub enum FFITransactionContext { ... } - -/// Transaction check result -#[repr(C)] -pub struct FFITransactionCheckResult { ... } +// Reuse the single source of truth from transaction_checking +use crate::transaction_checking::{FFITransactionContext, FFITransactionCheckResult};And in wallet_check_transaction, populate all fields of the shared FFITransactionCheckResult (set affected_accounts to NULL if you don’t return details):
- *result_out = FFITransactionCheckResult { - is_relevant: check_result.is_relevant, - total_received: check_result.total_received, - total_sent: check_result.total_sent, - affected_accounts_count: check_result.affected_accounts.len() as u32, - }; + *result_out = FFITransactionCheckResult { + is_relevant: check_result.is_relevant, + total_received: check_result.total_received, + total_sent: check_result.total_sent, + total_received_for_credit_conversion: check_result.total_received_for_credit_conversion, + affected_accounts: std::ptr::null_mut(), + affected_accounts_count: check_result.affected_accounts.len() as c_uint, + };Also update the import to bring c_uint into scope if needed.
Also applies to: 111-123
key-wallet-ffi/src/managed_wallet.rs (2)
1106-1115: Do not free Rust-allocated slices with libc::free()
boxed_slicecomes from Rust’s global allocator; freeing it withlibc::freeis UB. Use the providedaddress_array_freeand don’t free individual strings manually.Apply:
- 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); + 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); + } + // Frees the array and all inner strings + crate::address::address_array_free(addresses_out, count_out);
1258-1270: Passing&mut 0is invalid; use a real mutable bindingTaking a mutable ref to a literal won’t compile. Allocate a local and pass its mutable reference.
Apply:
- ptr::null_mut(), - &mut 0, + ptr::null_mut(), + { + let mut count: usize = 0; + &mut count + },Alternatively, hoist
let mut count: usize = 0;above and pass&mut count.key-wallet-ffi/tests/test_import_wallet.rs (1)
44-47: Leak: returned wallet must be freed with wallet_free_const()
wallet_manager_get_walletreturns a borrowed handle that the header documents must be freed viawallet_free_const. It isn’t freed here.Apply:
let wallet_ptr = wallet_manager_get_wallet(manager, wallet_ids_ptr, &mut error); assert!(!wallet_ptr.is_null()); assert_eq!(error.code, FFIErrorCode::Success); + // Free borrowed wallet handle when done + wallet_free_const(wallet_ptr);key-wallet-ffi/tests/test_account_collection.rs (1)
21-33: UB: pointers in FFIWalletAccountCreationOptions point to temporaries that are dropped.Using [0,1,2].as_ptr() etc. creates dangling pointers by the time the options are consumed. Define arrays with a stable lifetime in this scope and pass their pointers.
Apply this diff:
- let account_options = FFIWalletAccountCreationOptions { - option_type: FFIAccountCreationOptionType::AllAccounts, - bip44_indices: [0, 1, 2].as_ptr(), - bip44_count: 3, - bip32_indices: [0].as_ptr(), - bip32_count: 1, - coinjoin_indices: [0, 1].as_ptr(), - coinjoin_count: 2, - topup_indices: [0, 1, 2].as_ptr(), - topup_count: 3, - special_account_types: ptr::null(), - special_account_types_count: 0, - }; + let bip44 = [0u32, 1, 2]; + let bip32 = [0u32]; + let coinjoin = [0u32, 1]; + let topup = [0u32, 1, 2]; + + let account_options = FFIWalletAccountCreationOptions { + option_type: FFIAccountCreationOptionType::AllAccounts, + bip44_indices: bip44.as_ptr(), + bip44_count: bip44.len(), + bip32_indices: bip32.as_ptr(), + bip32_count: bip32.len(), + coinjoin_indices: coinjoin.as_ptr(), + coinjoin_count: coinjoin.len(), + topup_indices: topup.as_ptr(), + topup_count: topup.len(), + special_account_types: ptr::null(), + special_account_types_count: 0, + };key-wallet-ffi/src/wallet_manager_tests.rs (1)
225-234: Free returned C strings with the library’s free, notCString::from_raw.To avoid allocator mismatches across dynamic boundaries, use
address_freefor address pointers returned by FFI.Apply:
unsafe { - if !address.is_null() { - let _ = CString::from_raw(address); - } - if !address2.is_null() { - let _ = CString::from_raw(address2); - } + if !address.is_null() { + crate::address::address_free(address); + } + if !address2.is_null() { + crate::address::address_free(address2); + } wallet_manager::wallet_manager_free_wallet_ids(wallet_ids, count); wallet_manager::wallet_manager_free(manager); }-unsafe { - let _ = CString::from_raw(change_addr); -} +unsafe { + crate::address::address_free(change_addr); +}Also applies to: 740-742
key-wallet-ffi/src/provider_keys.rs (2)
239-247: Zeroize private key material before free.Ensure secret bytes aren’t left in heap after
free.if !info.private_key.is_null() { + // Zeroize before free + if info.private_key_len > 0 { + std::ptr::write_bytes(info.private_key, 0u8, info.private_key_len); + } libc::free(info.private_key as *mut libc::c_void); info.private_key = ptr::null_mut(); }
353-371: Provide a dedicated free for signatures instead of requiringlibc::free.FFI consumers shouldn’t rely on the same allocator. Add
provider_signature_freefor symmetry.FFIError::set_success(error); true } +/// Free a signature buffer previously returned by `wallet_sign_with_provider_key` +/// +/// # Safety +/// - `sig` must be a pointer returned by this library, or null. +#[no_mangle] +pub unsafe extern "C" fn provider_signature_free(sig: *mut u8) { + if !sig.is_null() { + libc::free(sig as *mut libc::c_void); + } +}key-wallet-ffi/src/wallet_manager_serialization_tests.rs (3)
250-253: Leak: original manager is shadowed and never freed.You rebind manager to a new instance before freeing the first one. Free the first manager prior to creating the second (or use a new name).
Apply:
- // Now import the wallet into a new manager - let manager = wallet_manager::wallet_manager_create(error); - assert!(!manager.is_null()); + // Now import the wallet into a new manager + unsafe { wallet_manager::wallet_manager_free(manager); } + let manager2 = wallet_manager::wallet_manager_create(error); + assert!(!manager2.is_null()); @@ - wallet_manager::wallet_manager_import_wallet_from_bytes( - manager, + wallet_manager::wallet_manager_import_wallet_from_bytes( + manager2, @@ - wallet_manager::wallet_manager_free(manager); + wallet_manager::wallet_manager_free(manager2);Also applies to: 271-277
280-317: Missing cleanup: manager not freed on invalid mnemonic path.Free the manager to avoid leaks in tests.
assert_eq!(wallet_bytes_len_out, 0); + unsafe { wallet_manager::wallet_manager_free(manager); }
319-351: Missing cleanup: manager not freed on null mnemonic path.Free the manager before returning.
assert_eq!(unsafe { (*error).code }, FFIErrorCode::InvalidInput); + unsafe { wallet_manager::wallet_manager_free(manager); }key-wallet-ffi/src/address.rs (1)
37-49: Unsafe deallocation contract should require library-owned arrays.address_array_free reconstructs a Box<[*mut c_char]>; passing arrays not allocated by this library is UB. Tighten docs to match utxo_array_free wording.
/// - `addresses` must be a valid pointer to an array of address strings or null +/// - The array must have been allocated by this library (Box<[*mut c_char]>/Vec.into_boxed_slice) /// - Each address in the array must be a valid C string pointer /// - `count` must be the correct number of addresses in the arraykey-wallet-ffi/src/address_tests.rs (1)
137-152: Potential UB: freeing an array allocated from Vec via address_array_free.address_array_free assumes Box<[*mut c_char]> ownership. Allocate the array accordingly to match the free function’s deallocator.
- let addrs_ptr = addresses.as_mut_ptr(); - let count = addresses.len(); - std::mem::forget(addresses); + // Convert to Box<[*mut c_char]> to match address_array_free ownership expectations + let count = addresses.len(); + let boxed: Box<[*mut i8]> = addresses.into_boxed_slice(); + let addrs_ptr = Box::into_raw(boxed) as *mut *mut i8;key-wallet-ffi/src/account_collection.rs (1)
67-74: ABI hazard: repr(C) struct has cfg-gated fieldsConditionally compiling fields in an FFI-exposed struct risks ABI/layout divergence across consumers. Make the fields unconditional booleans and gate only the assignment.
- #[cfg(feature = "bls")] - /// Whether provider operator keys account exists - pub has_provider_operator_keys: bool, - - #[cfg(feature = "eddsa")] - /// Whether provider platform keys account exists - pub has_provider_platform_keys: bool, + /// Whether provider operator keys account exists (false when `bls` is disabled) + pub has_provider_operator_keys: bool, + /// Whether provider platform keys account exists (false when `eddsa` is disabled) + pub has_provider_platform_keys: bool,And in account_collection_summary_data:
- #[cfg(feature = "bls")] - has_provider_operator_keys: collection.collection.provider_operator_keys.is_some(), - #[cfg(feature = "eddsa")] - has_provider_platform_keys: collection.collection.provider_platform_keys.is_some(), + has_provider_operator_keys: { + #[cfg(feature = "bls")] + { collection.collection.provider_operator_keys.is_some() } + #[cfg(not(feature = "bls"))] + { false } + }, + has_provider_platform_keys: { + #[cfg(feature = "eddsa")] + { collection.collection.provider_platform_keys.is_some() } + #[cfg(not(feature = "eddsa"))] + { false } + },key-wallet-ffi/src/managed_account.rs (2)
121-137: Leak on propagated FFIError.messageWhen wallet_manager_get_managed_wallet_info fails, error.message is re-wrapped into a Rust String but never freed, leaking the CString. Free it before returning.
- if managed_wallet_ptr.is_null() { - return FFIManagedAccountResult::error( - error.code, - if error.message.is_null() { - "Failed to get managed wallet info".to_string() - } else { - let c_str = std::ffi::CStr::from_ptr(error.message); - c_str.to_string_lossy().to_string() - }, - ); - } + if managed_wallet_ptr.is_null() { + let msg = if error.message.is_null() { + "Failed to get managed wallet info".to_string() + } else { + let c_str = std::ffi::CStr::from_ptr(error.message); + let s = c_str.to_string_lossy().to_string(); + // Free the allocated C string to avoid leaking it + let _ = std::ffi::CString::from_raw(error.message); + // Avoid double-free on any later drop paths + // (FFIError is a plain struct; we ensure message is null now) + // SAFETY: we own error.message allocation here + error.message = std::ptr::null_mut(); + s + }; + return FFIManagedAccountResult::error(error.code, msg); + }
252-269: Same leak in managed_wallet_get_top_up_account_with_registration_indexApply the same fix when propagating the error message.
- if managed_wallet_ptr.is_null() { - return FFIManagedAccountResult::error( - error.code, - if error.message.is_null() { - "Failed to get managed wallet info".to_string() - } else { - let c_str = std::ffi::CStr::from_ptr(error.message); - c_str.to_string_lossy().to_string() - }, - ); - } + if managed_wallet_ptr.is_null() { + let msg = if error.message.is_null() { + "Failed to get managed wallet info".to_string() + } else { + let c_str = std::ffi::CStr::from_ptr(error.message); + let s = c_str.to_string_lossy().to_string(); + let _ = std::ffi::CString::from_raw(error.message); + error.message = std::ptr::null_mut(); + s + }; + return FFIManagedAccountResult::error(error.code, msg); + }key-wallet-ffi/src/derivation.rs (3)
52-75: Critical: silent fallback to mainnet and type inconsistency.Two functions default to Network::Dash on conversion failure and use dash_network::Network while others use key_wallet::Network. This violates “always validate network consistency” and creates hard-to-debug mainnet/testnet mixups.
Fix both to (a) use key_wallet::Network and (b) return an error when not exactly one network is set.
@@ derivation_derive_private_key_from_seed - let network_rust: Network = network.try_into().unwrap_or(Network::Dash); + 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 ptr::null_mut(); + } + }; @@ dip9_derive_identity_key - let network_rust: Network = network.try_into().unwrap_or(Network::Dash); + 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 ptr::null_mut(); + } + };Optionally remove
use dash_network::Network;at the top if now unused.Also applies to: 94-157, 161-190, 229-256, 294-323, 359-387, 426-454, 505-575, 778-841, 842-915, 917-933
503-575: Error-code consistency for derivation path parsing.Elsewhere you use InvalidDerivationPath; here some branches still use InvalidInput for path errors.
Align to FFIErrorCode::InvalidDerivationPath for invalid path strings across derivation_derive_private_key_from_seed and dip9_derive_identity_key to make client handling consistent.
Also applies to: 776-933
503-518: Replace silent fallback defaulting to Network::Dash with explicit errorIn key-wallet-ffi/src/derivation.rs at lines 517 and 793, replace the
unwrap_or(Network::Dash)onnetwork.try_into()with explicit FFI error handling (e.g. returnFFIError::InvalidInputor a dedicatedInvalidNetworkcode) instead of defaulting to Dash.key-wallet-ffi/src/keys.rs (1)
175-191: Use InvalidDerivationPath for invalid path strings (not InvalidInput).Keeps parity with derivation.rs and improves client error handling.
Example change (apply similarly in all affected functions):
- FFIError::set_error( - error, - FFIErrorCode::InvalidInput, - format!("Invalid derivation path: {}", e), - ); + FFIError::set_error( + error, + FFIErrorCode::InvalidDerivationPath, + format!("Invalid derivation path: {}", e), + );Also applies to: 257-270, 336-349, 584-597, 706-719, 748-759
🧹 Nitpick comments (40)
key-wallet-ffi/src/transaction_checking.rs (3)
217-225: Simplify update_state wallet passing; avoid double reference + copied()Current code builds Option<&&Wallet> and then calls copied(). Create Option<&Wallet> directly for clarity.
- let update_wallet = if update_state && !wallet.is_null() { - Some(&(*wallet).inner()) - } else { - None - }; + let update_wallet: Option<&key_wallet::wallet::Wallet> = + if update_state && !wallet.is_null() { + Some((*wallet).inner()) + } else { + None + }; @@ - let check_result = - managed_wallet.check_transaction(&tx, network_rust, context, update_wallet.copied()); + let check_result = + managed_wallet.check_transaction(&tx, network_rust, context, update_wallet);
234-431: Factor repetitive AccountTypeMatch → FFIAccountMatch mappingThe 8+ near-identical arms can be moved into a small helper to cut duplication and reduce maintenance risk when adding new variants.
I can sketch a small to_ffi_account_match(&AccountTypeMatch) -> FFIAccountMatch helper if you want.
435-439: Drop unused _len local to satisfy clippy -D warningsThe variable isn’t used.
- let _len = ffi_accounts.len(); let ptr = ffi_accounts.as_mut_ptr(); std::mem::forget(ffi_accounts); ptrkey-wallet-ffi/tests/integration_test.rs (1)
38-41: Add a negative test for multi-flag networksAPIs that “require exactly one network” should reject combinations like Dash|Testnet. Add one assertion covering the InvalidInput path.
Also applies to: 65-69, 73-79, 138-141, 209-212, 221-224, 235-239
key-wallet-ffi/src/wallet_tests.rs (2)
384-395: Also test invalid/multi-network for wallet_get_xpubSince wallet_get_xpub requires exactly one network (per wallet.rs), add a case passing multiple flags and assert InvalidInput + null return.
162-168: Null pointer error-path tests are good; consider asserting error messages tooIf FFIError carries messages, assert they contain the expected substrings to harden contracts.
Also applies to: 228-235
key-wallet-ffi/src/transaction.rs (2)
143-146: FFI integer types: prefer c_uint for block_heightElsewhere (transaction_checking.rs) this arg is c_uint. Align here for FFI consistency.
- block_height: u32, + block_height: c_uint,
27-58: Explicit TODOs for unimplemented build/sign endpointsGood that these return WalletError. Consider adding a specific FFIErrorCode variant (e.g., NotImplemented) to allow callers to branch cleanly.
Happy to draft the enum addition and wiring if desired.
Also applies to: 71-98
key-wallet-ffi/src/managed_wallet.rs (2)
315-365: Pre-validate index range to fail fastConsider validating
start_index <= end_index(and optionally a sane max span) before touching wallet state to avoid invokingaddress_rangewith obviously-bad input.Apply:
+ if start_index > end_index { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + format!("start_index ({}) must be <= end_index ({})", start_index, end_index), + ); + *count_out = 0; + *addresses_out = ptr::null_mut(); + return false; + }
509-559: Mirror the same range guard for internal addressesKeep error handling uniform between external/internal range APIs.
Use the same diff as suggested for the external range above.
key-wallet-ffi/src/types.rs (1)
70-75: Stale error text references old enum nameMessage says “FFINetwork must represent exactly one network” but the type is
FFINetworks.Apply:
- None => Err("FFINetwork must represent exactly one network"), + None => Err("FFINetworks must represent exactly one network"),key-wallet-ffi/include/key_wallet_ffi.h (4)
881-883: Doc mismatch: enum name“Returns FFINetwork::NoNetworks” should be “FFINetworks::NO_NETWORKS”.
Please update the Rust doc comments driving cbindgen and regenerate this header.
936-937: Doc mismatch: enum nameSame issue for BLS account: use “FFINetworks::NO_NETWORKS”.
Regenerate from corrected Rust docs.
993-994: Doc mismatch: enum nameSame issue for EdDSA account: use “FFINetworks::NO_NETWORKS”.
Regenerate from corrected Rust docs.
2702-2708: Specify the correct free function for returned stringsDoc says “The returned string must be freed by the caller” but doesn’t name the function. For address strings, prefer calling
address_freeto avoid confusion withstring_free.Please adjust the Rust doc comments accordingly and regenerate.
key-wallet-ffi/tests/debug_wallet_add.rs (1)
24-27: Ensure exactly one network bit is set when calling FFI.Per the new bitflag API, passing multiple network bits should error. Consider adding a negative test that ORs two flags and asserts failure to guard against regressions.
key-wallet-ffi/tests/test_managed_account_collection.rs (1)
203-208: Feature-gated provider-key APIs: guard tests if features are optional.If provider keys are behind a cargo feature, wrap these assertions under the same cfg to keep the test suite green under minimal features.
Example:
- assert!(managed_account_collection_has_provider_voting_keys(collection)); - assert!(managed_account_collection_has_provider_owner_keys(collection)); + #[cfg(feature = "provider-keys")] + { + assert!(managed_account_collection_has_provider_voting_keys(collection)); + assert!(managed_account_collection_has_provider_owner_keys(collection)); + }key-wallet-ffi/src/bip38.rs (1)
19-24: Document that thenetworkparameter is ignored.BIP38 is network-agnostic. To prevent misuse, note this in the doc comment (or validate single-bit selection and discard).
Apply this minimal doc tweak:
/// Encrypt a private key with BIP38 /// +/// Note: `network` is currently ignored (BIP38 is network-agnostic). It is +/// accepted for API uniformity and may be used in the future. /// /// # Safetykey-wallet-ffi/src/wallet_manager_tests.rs (2)
41-61: Outdated comments/test name referencing “account_count”.The parameter is now a network, not an account count. Clean up misleading comments and rename the test to reflect current semantics.
-fn test_wallet_manager_add_wallet_with_account_count() { +fn test_wallet_manager_add_wallet() {- FFINetworks::Testnet, // Create 3 accounts + FFINetworks::Testnet,- ptr::null(), // No passphrase - FFINetworks::Testnet, // 2 accounts per wallet + ptr::null(), // No passphrase + FFINetworks::Testnet,- FFINetworks::Testnet, // account_count + FFINetworks::Testnet,Also applies to: 603-607, 619-621, 548-549, 57-57
464-486: Add a negative test for multi-network bitmasks.Since FFINetworks is bitflag-based and FFI paths require “exactly one network,” add a test that passes multiple bits (e.g., Dash|Testnet) and asserts
InvalidInput.I can push a small test if you want.
Also applies to: 875-895
key-wallet-ffi/tests/test_passphrase_wallets.rs (2)
71-73: Remove stale “account_count (ignored)” comment.Parameter is a network bitflag now.
- FFINetworks::Testnet, // account_count (ignored) + FFINetworks::Testnet,
205-220: Optional: add a negative-path assertion for combined networks.Add a small test that passes
FFINetworks::Dash | FFINetworks::Testnetto a getter and assertsInvalidInput.Want me to draft it?
key-wallet-ffi/src/provider_keys.rs (1)
107-114: Clarify error semantics for unsupported provider accounts.Returning NotFound for unsupported types is ambiguous; prefer a clear “not implemented/unsupported” error to aid callers.
- FFIError::set_error( - error, - FFIErrorCode::NotFound, - format!("Provider account type {:?} not found", account_type), - ); + FFIError::set_error( + error, + FFIErrorCode::WalletError, + format!("Provider account type {:?} unsupported (not implemented)", account_type), + );Also applies to: 118-124
key-wallet-ffi/src/utxo_tests.rs (1)
546-555: Add a multi-network bitmask negative test.Include a case like
FFINetworks::Dash | FFINetworks::Testnetthat should returnInvalidInputto assert validation paths.Happy to draft this test.
Also applies to: 562-571, 578-587
key-wallet-ffi/src/wallet_manager_serialization_tests.rs (1)
65-112: Minor: assert error codes on success paths for stronger guarantees.After success asserts, also assert FFIErrorCode::Success for consistency across all tests.
Also applies to: 114-161, 163-210, 212-279, 353-418
key-wallet-ffi/src/address.rs (2)
99-113: Unify error detail for wrong-network address.address_validate includes the network value in the error; address_get_type uses a generic string. Consider including the network for parity.
- "Address not valid for network".to_string(), + format!("Address not valid for network {:?}", network_rust),
100-101: Redundant intermediate variable.dash_network duplicates network_rust without value. Use network_rust directly.
- let dash_network = network_rust; - match addr.require_network(dash_network) { + match addr.require_network(network_rust) {Also applies to: 178-181
key-wallet-ffi/src/utxo.rs (1)
35-43: CString::new(address).unwrap_or_default() may silently drop interior NULs.If address can contain interior NULs (shouldn’t), unwrap() with explicit error handling is preferable to avoid silently producing empty strings.
- let address_cstr = CString::new(address).unwrap_or_default(); + let address_cstr = CString::new(address).map_err(|_| ()).unwrap_or_else(|_| CString::new("").unwrap());Also applies to: 63-75
key-wallet-ffi/src/derivation_tests.rs (1)
52-57: Reduce duplication in seed setup.Factor repeated 64-byte seed initialization into a small helper within the module to cut noise.
+fn test_seed() -> [u8;64] { + let mut s = [0u8;64]; + for (i,b) in s.iter_mut().enumerate() { *b = (i % 256) as u8; } + s +}Then replace repeated loops with
let seed = test_seed();.Also applies to: 81-85, 111-115, 145-150, 595-599, 968-973
key-wallet-ffi/src/address_tests.rs (1)
166-183: Strengthen negative-path assertions.Also assert specific FFIError codes for each invalid case to improve signal.
key-wallet-ffi/src/account_tests.rs (2)
203-205: Prefer enum equality over integer castsUse direct enum comparison to avoid losing type-safety.
- assert_eq!(network as u32, FFINetworks::Testnet as u32); + assert_eq!(network, FFINetworks::Testnet);
246-246: Prefer enum equality over integer castsSame nit as above.
- assert_eq!(network as u32, FFINetworks::NoNetworks as u32); + assert_eq!(network, FFINetworks::NoNetworks);key-wallet-ffi/src/keys_tests.rs (1)
192-195: Comment accuracy: coin type on testnetComments refer to “Dash coin type 5” while the paths use 44'/1' (testnet = coin type 1). Please align comments to avoid confusion.
- // Derive public key using derivation path (44'/1'/0'/0/0 for Dash) + // Derive public key using derivation path (44'/1'/0'/0/0 for Dash testnet, coin type 1)Also applies to: 200-201, 208-209, 242-245, 254-255
key-wallet-ffi/src/wallet_manager.rs (3)
537-585: wallet_manager_get_wallet returns owned wrapper — OKConst pointer enforces read-only; caller frees via wallet_free_const(). Consider documenting lifetime explicitly in the header.
51-62: Minor: error pointer may be nullA few setters assume non-null error; most helpers tolerate null, but please keep using FFIError::set_* which should handle nulls to be consistent.
480-523: wallet_manager_get_wallet_ids buffer contract — OK32-byte packing and single free function are clear. Consider zeroizing before free if IDs are considered sensitive.
key-wallet-ffi/src/account.rs (1)
265-272: Docstring: update FFINetwork → FFINetworks for null-account case.Comments still say “Returns FFINetwork::NoNetworks”. Please reflect the new enum name.
Apply this diff to each occurrence:
-/// - Returns FFINetwork::NoNetworks if the account is null +/// - Returns FFINetworks::NoNetworks if the account is nullAlso applies to: 377-384, 494-501
key-wallet-ffi/src/managed_account_collection.rs (2)
171-188: Indices API: sort BIP32 indices for consistency with BIP44/CoinJoin.bip44/coinjoin paths sort; bip32 does not, which is surprising for clients.
Apply:
pub unsafe extern "C" fn managed_account_collection_get_bip32_indices( @@ - let indices: Vec<c_uint> = - collection.collection.standard_bip32_accounts.keys().copied().collect(); + let mut indices: Vec<c_uint> = + collection.collection.standard_bip32_accounts.keys().copied().collect(); + indices.sort();And similarly in summary_data(), you already sort; no change needed there.
Also applies to: 239-255, 304-320, 331-360, 412-428, 439-467
636-669: Clarify void return and free semantics in header and add void‐safe free alias**
- In key-wallet-ffi/include/key_wallet_ffi.h above both
managed_account_collection_get_provider_*_keysdeclarations, add a comment: “Returns anFFIManagedAccount*asvoid*; must be freed withmanaged_account_free(void*).”- Expose a
#[no_mangle] pub extern "C" fn managed_account_free_void(p: *mut std::os::raw::c_void)that casts toFFIManagedAccount*and drops it, to avoid integrator casts.key-wallet-ffi/src/wallet.rs (1)
383-411: DRY the single-network conversion + error boilerplate.You repeat the try_into() + error setup. Consider a small helper to reduce duplication and keep messages uniform.
Example (in crate::types or a local module):
#[inline] fn single_network_or<'a>(nets: FFINetworks, err: *mut FFIError, ctx: &'a str) -> Option<key_wallet::Network> { match nets.try_into() { Ok(n) => Some(n), Err(_) => { FFIError::set_error(err, FFIErrorCode::InvalidInput, format!("Must specify exactly one network{}", ctx)); None } } }Then early-return None in call sites.
Also applies to: 483-505, 552-583, 650-681
| pub unsafe extern "C" fn account_collection_get_provider_operator_keys( | ||
| collection: *const FFIAccountCollection, | ||
| ) -> *mut crate::account::FFIBLSAccount { | ||
| if collection.is_null() { | ||
| return ptr::null_mut(); | ||
| } | ||
| ) -> *mut std::os::raw::c_void { | ||
| #[cfg(feature = "bls")] | ||
| { | ||
| if collection.is_null() { | ||
| return ptr::null_mut(); | ||
| } | ||
|
|
||
| let collection = &*collection; | ||
| match &collection.collection.provider_operator_keys { | ||
| Some(account) => { | ||
| let ffi_account = crate::account::FFIBLSAccount::new(account); | ||
| Box::into_raw(Box::new(ffi_account)) | ||
| let collection = &*collection; | ||
| match &collection.collection.provider_operator_keys { | ||
| Some(account) => { | ||
| let ffi_account = crate::account::FFIBLSAccount::new(account); | ||
| Box::into_raw(Box::new(ffi_account)) as *mut std::os::raw::c_void | ||
| } | ||
| None => ptr::null_mut(), | ||
| } | ||
| None => ptr::null_mut(), | ||
| } | ||
| } | ||
|
|
||
| /// Get the provider operator keys account if it exists (stub when BLS is disabled) | ||
| #[cfg(not(feature = "bls"))] | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn account_collection_get_provider_operator_keys( | ||
| _collection: *const FFIAccountCollection, | ||
| ) -> *mut std::os::raw::c_void { | ||
| // BLS feature not enabled, always return null | ||
| ptr::null_mut() | ||
| #[cfg(not(feature = "bls"))] | ||
| { | ||
| // BLS feature not enabled, always return null | ||
| let _ = collection; // Avoid unused parameter warning | ||
| ptr::null_mut() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Returning void for BLS operator account weakens type safety*
ABI now requires consumers to cast and call the correct free, risking UB if mismatched. Prefer typed return with a stable opaque type and return null when the feature is disabled.
If keeping this ABI, at minimum add a runtime type tag alongside the pointer or provide feature-gated typed getters (and deprecate the void* versions). I can draft a safe-ABI variant if desired.
🤖 Prompt for AI Agents
In key-wallet-ffi/src/account_collection.rs around lines 616–641, the FFI
currently returns a raw void* for a BLS operator account which weakens type
safety; instead add a feature-gated, typed getter and keep the void* API
deprecated (or add a tagged opaque wrapper). Concretely: add a new function pub
unsafe extern "C" fn
account_collection_get_provider_operator_bls_account(collection: *const
FFIAccountCollection) -> *mut crate::account::FFIBLSAccount that is
#[cfg(feature = "bls")] and returns a boxed FFIBLSAccount pointer (or null if
collection/account is null); provide an #[cfg(not(feature = "bls"))]
implementation that returns null; mark the existing void* function deprecated
(or modify it to return a small tagged opaque struct containing a type tag and
pointer) so consumers can safely detect type at runtime. Ensure null checks and
ownership semantics match existing allocation/free conventions.
| 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(); | ||
| } | ||
| ) -> *mut std::os::raw::c_void { | ||
| #[cfg(feature = "eddsa")] | ||
| { | ||
| 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)) | ||
| 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)) as *mut std::os::raw::c_void | ||
| } | ||
| None => ptr::null_mut(), | ||
| } | ||
| 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() | ||
| #[cfg(not(feature = "eddsa"))] | ||
| { | ||
| // EdDSA feature not enabled, always return null | ||
| let _ = collection; // Avoid unused parameter warning | ||
| ptr::null_mut() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same concern for EdDSA platform account void*
Mirror the fix for operator keys to keep the ABI type-safe.
🤖 Prompt for AI Agents
In key-wallet-ffi/src/account_collection.rs around lines 676 to 701, the
function currently returns a bare *mut c_void for the EdDSA
provider_platform_keys which is not ABI type-safe; mirror the operator-keys fix
by changing the function signature and returns to use the concrete FFI pointer
type used for platform accounts (the same FFI platform/account struct used for
operator keys — e.g., *mut FFIPlatformAccount or the concrete FFIEdDSAAccount
pointer type), construct and Box::into_raw the appropriate FFI wrapper instance
and return it as that concrete pointer (or return null_mut() when absent), and
update the not(feature = "eddsa") branch to avoid unused parameter warnings
while preserving the concrete return type.
| network: FFINetworks, | ||
| account_options: *const crate::types::FFIWalletAccountCreationOptions, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wallet creation: multi-network support — OK, add explicit empty-check
parse_networks() can yield an empty set (NoNetworks). Fail fast before calling into the manager for clearer errors.
let networks_rust = network.parse_networks();
+ if networks_rust.is_empty() {
+ FFIError::set_error(error, FFIErrorCode::InvalidInput, "No network specified".to_string());
+ return false;
+ }Apply similarly in wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes after let networks = network.parse_networks();.
Also applies to: 171-172, 210-211
🤖 Prompt for AI Agents
In key-wallet-ffi/src/wallet_manager.rs around lines 55-56, parse_networks() can
return NoNetworks; add an explicit check right after let networks =
network.parse_networks(); to fail fast (return an appropriate FFI error/result)
when networks is empty instead of proceeding into the manager. Do the same fix
at the other sites mentioned (around lines 171-172 and 210-211) where
parse_networks() is called: check for the NoNetworks variant, produce a clear
FFI error/return value, and avoid calling manager methods when no networks were
parsed.
| network: FFINetworks, | ||
| birth_height: c_uint, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeat: empty network set check
Apply the same “No network specified” guard here as noted above.
- let networks = network.parse_networks();
+ let networks = network.parse_networks();
+ if networks.is_empty() {
+ FFIError::set_error(error, FFIErrorCode::InvalidInput, "No network specified".to_string());
+ return false;
+ }Also applies to: 266-267
🤖 Prompt for AI Agents
In key-wallet-ffi/src/wallet_manager.rs around lines 210-211 (and similarly at
266-267) the function accepts a network argument but lacks the “No network
specified” guard used elsewhere; add the same check used previously to detect an
empty/unspecified FFINetworks value and immediately return the appropriate error
(or set the FFI error state) when network is empty, before proceeding to use
network and birth_height — mirror the earlier guard logic exactly so both
locations validate network input and fail fast with the same error handling.
| typedef ClientConfig FFIClientConfig; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Blocking: typedef ClientConfig FFIClientConfig; leaks an internal Rust type and breaks C compilation—restore opaque FFI type and re-sync header
ClientConfig is not declared in this C header. Exposing it via typedef both breaks build and violates the opaque-pointer FFI pattern used elsewhere. This is almost certainly a header drift issue; per guidelines, this header should be generated and synced.
Apply this fix and re-run ./sync-headers.sh:
-typedef ClientConfig FFIClientConfig;
+typedef struct FFIClientConfig FFIClientConfig;I leveraged the “sync the generated header” learning. Please run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare generated vs. synced header
diff -u dash-spv-ffi/include/dash_spv_ffi.h swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h || true
# Ensure no stray typedef to ClientConfig remains
rg -nS --context 2 'typedef\s+ClientConfig\s+FFIClientConfig'Length of output: 948
Blocking: Restore opaque-pointer typedef in dash-spv-ffi and re-sync headers
The line typedef ClientConfig FFIClientConfig; appears in both the upstream generated header and the Swift-SDK copy, leaking the internal Rust ClientConfig type and breaking C compilation. Update the upstream header (dash-spv-ffi/include/dash_spv_ffi.h) at line 40:
-typedef ClientConfig FFIClientConfig;
+typedef struct FFIClientConfig FFIClientConfig;Then re-run ./sync-headers.sh to propagate the fix into swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h.
🤖 Prompt for AI Agents
In swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h around lines 40
to 41, the line `typedef ClientConfig FFIClientConfig;` leaks the internal Rust
ClientConfig type; replace that concrete typedef with an opaque pointer typedef
(e.g. forward-declare a struct and typedef a pointer) in the upstream header
file dash-spv-ffi/include/dash_spv_ffi.h so the internal type is hidden, then
re-run ./sync-headers.sh to propagate the corrected header into the Swift SDK.
| uintptr_t elem_size; | ||
| uintptr_t elem_align; | ||
| } FFIArray; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ABI change in FFIArray: document elem_size/elem_align requirements and ensure all creators populate them
Adding elem_size and elem_align is fine, but the safety contract must reflect it, and all Rust constructors returning FFIArray must set both fields correctly.
- Update the doc above to say callers must not modify
elem_size/elem_align. - In Rust, when constructing any FFIArray, set:
elem_size = core::mem::size_of::<T>()elem_align = core::mem::align_of::<T>()
- In array destroy helpers, assert these match the expected element type when applicable.
Run to spot FFIArray initializations that may miss the new fields:
🏁 Script executed:
#!/bin/bash
rg -nP 'FFIArray\s*\{' -A5 -B2 dash-spv-ffi/srcLength of output: 11230
Ensure elem_size/elem_align are correctly initialized and documented
All manual FFIArray { … } constructors in dash-spv-ffi/src/client.rs currently default to elem_size: 0 and elem_align: 1. You must:
- In
include/dash_spv_ffi.h, update theFFIArraydocs to state that callers must not modifyelem_size/elem_alignand that these fields carry the element’ssize_ofandalign_of. - In every Rust
Box::new(FFIArray { … })initializer, replace the hard-coded0/1withcore::mem::size_of::<T>()andcore::mem::align_of::<T>()for the actualT. - In each array–destroy helper, add a debug-assert that the stored
elem_size/elem_alignmatchessize_of::<T>()/align_of::<T>()before freeing.
🤖 Prompt for AI Agents
In swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h around lines
103-105, the FFIArray struct needs a doc comment clarifying that callers must
not mutate elem_size/elem_align and that these fields MUST contain the element
type's size_of and align_of; update the header docs accordingly. In
dash-spv-ffi/src/client.rs replace every Box::new(FFIArray { ... elem_size: 0,
elem_align: 1, ... }) with elem_size: core::mem::size_of::<T>(), elem_align:
core::mem::align_of::<T>() for the concrete T used. In each array-destroy helper
function add a debug_assert!(stored.elem_size == core::mem::size_of::<T>()) and
debug_assert!(stored.elem_align == core::mem::align_of::<T>()) before freeing to
ensure the stored metadata matches the expected T.
| /** | ||
| * Destroy an array of FFIString pointers (Vec<*mut FFIString>) and their contents. | ||
| * | ||
| * This function: | ||
| * - Iterates the array elements as pointers to FFIString and destroys each via dash_spv_ffi_string_destroy | ||
| * - Frees the underlying vector buffer stored in FFIArray | ||
| * - Does not free the FFIArray struct itself (safe for both stack- and heap-allocated structs) | ||
| */ | ||
| void dash_spv_ffi_string_array_destroy(struct FFIArray *arr); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
*Clarify contract for destroying Vec<mut FFIString>; add runtime type checks in impl
Good addition. Tighten the contract to prevent misuse:
- Document that
arr->datamust point to an array ofFFIString*and thatarr->elem_sizemust equalsizeof(FFIString*)andarr->elem_alignmust equal_Alignof(FFIString*). - In the Rust implementation, guard with a runtime check; if sizes don’t match, set last error and return without dereferencing.
Proposed header doc tweak:
/**
- * Destroy an array of FFIString pointers (Vec<*mut FFIString>) and their contents.
+ * Destroy an array of FFIString pointers (Vec<*mut FFIString>) and their contents.
+ *
+ * Preconditions:
+ * - arr->data points to elements of type `FFIString*`
+ * - arr->elem_size == sizeof(FFIString*)
+ * - arr->elem_align == alignof(FFIString*)
*
* This function:
* - Iterates the array elements as pointers to FFIString and destroys each via dash_spv_ffi_string_destroy
* - Frees the underlying vector buffer stored in FFIArray
* - Does not free the FFIArray struct itself (safe for both stack- and heap-allocated structs)
*/📝 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.
| /** | |
| * Destroy an array of FFIString pointers (Vec<*mut FFIString>) and their contents. | |
| * | |
| * This function: | |
| * - Iterates the array elements as pointers to FFIString and destroys each via dash_spv_ffi_string_destroy | |
| * - Frees the underlying vector buffer stored in FFIArray | |
| * - Does not free the FFIArray struct itself (safe for both stack- and heap-allocated structs) | |
| */ | |
| void dash_spv_ffi_string_array_destroy(struct FFIArray *arr); | |
| /** | |
| * Destroy an array of FFIString pointers (Vec<*mut FFIString>) and their contents. | |
| * | |
| * Preconditions: | |
| * - arr->data points to elements of type `FFIString*` | |
| * - arr->elem_size == sizeof(FFIString*) | |
| * - arr->elem_align == alignof(FFIString*) | |
| * | |
| * This function: | |
| * - Iterates the array elements as pointers to FFIString and destroys each via dash_spv_ffi_string_destroy | |
| * - Frees the underlying vector buffer stored in FFIArray | |
| * - Does not free the FFIArray struct itself (safe for both stack- and heap-allocated structs) | |
| */ | |
| void dash_spv_ffi_string_array_destroy(struct FFIArray *arr); |
🤖 Prompt for AI Agents
In swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h around lines
492 to 501, tighten the contract and implement runtime guards: update the
comment to state that arr->data must point to an array of FFIString* and
arr->elem_size must equal sizeof(FFIString*) and arr->elem_align must equal
_Alignof(FFIString*); in the Rust implementation, before dereferencing, check
arr and arr->data are non-null and verify elem_size and elem_align match the
expected pointer size/align; on mismatch set the library last error and return
without dereferencing or freeing the buffer; if checks pass, iterate the
elements treating each as a FFIString* (skipping NULL entries), call
dash_spv_ffi_string_destroy for each, then free the underlying buffer but do not
free the FFIArray struct itself.
Summary by CodeRabbit
New Features
Refactor
Tests