refactor: spv cleanup #126
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds CI to verify autogenerated FFI docs and extensive FFI/API refactors: replaces SPVWalletManager with WalletManager, introduces a single-network FFINetwork alongside FFINetworks flags, adds transaction-building and derivation APIs, migrates WalletManager FFI to an async Arc/RwLock + Tokio runtime, updates headers/examples/tests, and adds docgen scripts/Makefiles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant FFI as dash-spv-ffi (C ABI)
participant Client as FFIDashSpvClient
participant WM as FFIWalletManager
App->>FFI: dash_spv_ffi_client_get_wallet_manager(client*)
FFI->>Client: lookup wallet manager (if initialized)
Client-->>FFI: return void* (opaque FFIWalletManager) or NULL
FFI-->>App: wallet_manager* (use while client exists)
note right of App: Host uses wallet-manager FFI APIs via opaque pointer
sequenceDiagram
autonumber
actor Dev
participant GH as GitHub Actions
participant Gen as generate_ffi_docs.py
participant Repo as git
Dev->>GH: push/PR touching FFI files
GH->>Gen: run generate_ffi_docs.py (per crate)
Gen-->>GH: write FFI_API.md
GH->>Repo: run git diff --quiet FFI_API.md
alt docs up-to-date
Repo-->>GH: no diff
GH-->>Dev: job passes
else docs out-of-date
Repo-->>GH: diff present
GH-->>Dev: fail job and post PR comment with update steps
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: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (30)
dash-spv/src/client/block_processor_test.rs (1)
135-144: Remove stale as_any comment/import; also avoid holding RwLock while awaiting inner Mutex
- The comment about using WalletInterface/as_any is stale.
- The local use key_wallet_manager::wallet_interface::WalletInterface is now unused and will fail clippy with -D warnings.
- Minor: drop the RwLock read guard before awaiting the inner Mutex to reduce lock hold time and prevent future lock-order issues.
{ - let wallet = wallet.read().await; - // Since we're using key_wallet_manager::wallet_interface::WalletInterface, - // we need to use the trait to access as_any - use key_wallet_manager::wallet_interface::WalletInterface; - let processed = wallet.processed_blocks.lock().await; + let wallet_guard = wallet.read().await; + let processed_blocks = wallet_guard.processed_blocks.clone(); + drop(wallet_guard); + let processed = processed_blocks.lock().await; assert_eq!(processed.len(), 1); assert_eq!(processed[0].0, block_hash); }key-wallet-ffi/src/address_pool.rs (2)
542-614: Off-by-one/None-case bug: generate_to_index can skip index 0 when no addresses existWhen highest_generated is None, unwrap_or(0) treats “no addresses yet” as if index 0 exists, so target_index = 0 won’t generate the first address. Use counts (highest_generated + 1) vs. target_index + 1.
Apply this pattern to External, Internal, and Single branches:
- let current = external_addresses.highest_generated.unwrap_or(0); - if target_index > current { - let needed = target_index - current; - external_addresses.generate_addresses(needed, &key_source, true) + let current_count: u32 = + external_addresses.highest_generated.map(|i| i + 1).unwrap_or(0); + let target_count: u32 = (target_index.saturating_add(1)) as u32; + if target_count > current_count { + let needed = target_count - current_count; + external_addresses.generate_addresses(needed, &key_source, true) } else { Ok(Vec::new()) }- let current = internal_addresses.highest_generated.unwrap_or(0); - if target_index > current { - let needed = target_index - current; - internal_addresses.generate_addresses(needed, &key_source, true) + let current_count: u32 = + internal_addresses.highest_generated.map(|i| i + 1).unwrap_or(0); + let target_count: u32 = (target_index.saturating_add(1)) as u32; + if target_count > current_count { + let needed = target_count - current_count; + internal_addresses.generate_addresses(needed, &key_source, true) } else { Ok(Vec::new()) }- let current = pool.highest_generated.unwrap_or(0); - if target_index > current { - let needed = target_index - current; - pool.generate_addresses(needed, &key_source, true) + let current_count: u32 = + pool.highest_generated.map(|i| i + 1).unwrap_or(0); + let target_count: u32 = (target_index.saturating_add(1)) as u32; + if target_count > current_count { + let needed = target_count - current_count; + pool.generate_addresses(needed, &key_source, true) } else { Ok(Vec::new()) }
643-799: Validate address network against the provided FFINetwork before marking usedComment says “assume same network,” but code doesn’t check. Reject mismatches to prevent confusing NotFound on cross-network input.
Example change:
- // Assume the address uses the same network we're working with - let address = unchecked_addr.assume_checked(); + // Enforce network match + let address = match unchecked_addr.require_network(network_rust) { + Ok(a) => a, + Err(_) => { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Address/network mismatch".to_string(), + ); + return false; + } + };If require_network isn’t available in this dashcore version, compare the payload/network prefix and error out on mismatch.
key-wallet-manager/src/wallet_manager/process_block.rs (1)
56-78: Reorg handling removes records above from_height; consider full state rollbackOnly pruning transaction records may leave UTXOs/balances/“used” flags inconsistent. If reconciliation is done elsewhere, ignore. Otherwise, add a targeted rollback or trigger a rescan from to_height.
dash-spv-ffi/tests/unit/test_error_handling.rs (1)
34-74: Potential UAF: global shared error CString across threads.
dash_spv_ffi_get_last_error()likely returns a pointer into a globalCString. Another thread can replace/clear it while readers still dereference, leading to dangling pointers. Use thread-local storage for the last error, or return an owned FFIString.Apply in src/error.rs:
- static LAST_ERROR: Mutex<Option<CString>> = Mutex::new(None); + thread_local! { + static LAST_ERROR: std::cell::RefCell<Option<CString>> = std::cell::RefCell::new(None); + } pub fn set_last_error(err: &str) { - let c_err = CString::new(err).unwrap_or_else(|_| CString::new("Unknown error").unwrap()); - if let Ok(mut guard) = LAST_ERROR.lock() { - *guard = Some(c_err); - } + let c_err = CString::new(err).unwrap_or_else(|_| CString::new("Unknown error").unwrap()); + LAST_ERROR.with(|cell| *cell.borrow_mut() = Some(c_err)); }Optionally add:
+#[no_mangle] +pub extern "C" fn dash_spv_ffi_get_last_error_owned() -> FFIString { + LAST_ERROR.with(|cell| { + if let Some(ref c) = *cell.borrow() { + FFIString::new(c.to_str().unwrap_or_default()) + } else { + FFIString { ptr: std::ptr::null_mut(), length: 0 } + } + }) +}dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
145-147: Free returned FFI structs to avoid leaks in the loop.Destroy sync_progress and stats when present.
- let _ = dash_spv_ffi_client_get_sync_progress(client); - let _ = dash_spv_ffi_client_get_stats(client); + let progress = dash_spv_ffi_client_get_sync_progress(client); + if !progress.is_null() { + dash_spv_ffi_sync_progress_destroy(progress); + } + let stats = dash_spv_ffi_client_get_stats(client); + if !stats.is_null() { + dash_spv_ffi_spv_stats_destroy(stats); + }key-wallet-ffi/src/utxo.rs (1)
89-103: Validate network consistency before fetching UTXOsGuard against mixing networks: compare the ManagedWalletInfo’s network with the requested FFINetwork and fail fast.
pub unsafe extern "C" fn managed_wallet_get_utxos( managed_info: *const FFIManagedWalletInfo, - network: FFINetwork, + network: FFINetwork, utxos_out: *mut *mut FFIUTXO, count_out: *mut usize, error: *mut FFIError, ) -> bool { @@ - let managed_info = &*managed_info; - let network_rust: key_wallet::Network = network.into(); + let managed_info = &*managed_info; + let network_rust: key_wallet::Network = network.into(); + if managed_info.inner().network() != network_rust { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Network mismatch between wallet and request".to_string(), + ); + return false; + }dash-spv/tests/integration_real_node_test.rs (4)
16-16: Remove unused import to keep clippy cleanenv_logger is referenced via its full path; the
use env_logger;import is unused.-use env_logger;
27-44: Return type ties create_test_client to MemoryStorage; switch to DiskStorage to enable later readsSubsequent tests read headers from a separate storage handle; with memory storage this always reads empty. Use DiskStorage with a shared path.
-async fn create_test_client( - config: ClientConfig, -) -> Result< - DashSpvClient<WalletManager<ManagedWalletInfo>, MultiPeerNetworkManager, MemoryStorageManager>, - Box<dyn std::error::Error>, -> { +async fn create_test_client( + config: ClientConfig, + storage_path: std::path::PathBuf, +) -> Result< + DashSpvClient<WalletManager<ManagedWalletInfo>, MultiPeerNetworkManager, dash_spv::storage::DiskStorageManager>, + Box<dyn std::error::Error>, +> { @@ - // Create storage manager - let storage_manager = MemoryStorageManager::new().await?; + // Create storage manager (disk) so tests can re-open it for reads + let storage_manager = dash_spv::storage::DiskStorageManager::new(storage_path).await?;Also add at top:
+use std::path::PathBuf;
200-207: Fix: reading from the wrong storage instanceThe test creates a fresh MemoryStorageManager and reads from it, but the client wrote to a different storage. Use a shared on-disk path for both the client and subsequent reads.
- // Create fresh storage and client - let storage = MemoryStorageManager::new().await.expect("Failed to create storage"); - - // Verify starting from empty state - assert_eq!(storage.get_tip_height().await.unwrap(), None); - - let mut client = create_test_client(config.clone()).await.expect("Failed to create SPV client"); + // Create fresh storage path and client + let storage_dir = std::env::temp_dir().join(format!("dash-spv-test-10k-{}", std::process::id())); + let mut client = + create_test_client(config.clone(), storage_dir.clone()).await.expect("Failed to create SPV client"); @@ - let genesis_headers = - storage.load_headers(0..10).await.expect("Failed to load genesis headers"); + let disk_storage = dash_spv::storage::DiskStorageManager::new(storage_dir.clone()) + .await + .expect("Failed to reopen disk storage"); + let genesis_headers = + disk_storage.load_headers(0..10).await.expect("Failed to load genesis headers"); @@ - let mid_headers = - storage.load_headers(500..510).await.expect("Failed to load mid-chain headers"); + let mid_headers = disk_storage + .load_headers(500..510) + .await + .expect("Failed to load mid-chain headers"); @@ - let recent_headers = storage + let recent_headers = disk_storage .load_headers(recent_start..(recent_start + 10)) .await .expect("Failed to load recent headers");Also applies to: 293-314
412-437: Same issue: chain continuity reads from a different storage than the client wrote toSwitch to a shared DiskStorage path and reopen it for assertions.
- let storage = MemoryStorageManager::new().await expect("Failed to create storage"); + let storage_dir = std::env::temp_dir().join(format!("dash-spv-test-continuity-{}", std::process::id())); + let mut client = create_test_client(config, storage_dir.clone()) + .await + .expect("Failed to create SPV client"); @@ - let headers = storage + let disk_storage = dash_spv::storage::DiskStorageManager::new(storage_dir) + .await + .expect("Failed to reopen disk storage"); + let headers = disk_storage .load_headers(0..test_range) .await .expect("Failed to load headers for continuity test");dash-spv-ffi/tests/test_event_callbacks.rs (1)
100-126: Fix UB: guard wallet_id before CStr::from_ptr in wallet tx callbackwallet_id may be null per FFI; calling CStr::from_ptr without a null check is UB. Guard and reuse the parsed string for logging and storage.
extern "C" fn test_wallet_transaction_callback( wallet_id: *const c_char, account_index: u32, txid: *const [u8; 32], confirmed: bool, amount: i64, _addresses: *const c_char, _block_height: u32, is_ours: bool, user_data: *mut c_void, ) { - println!("Test wallet transaction callback called: wallet={}, account={}, confirmed={}, amount={}, is_ours={}", - unsafe { CStr::from_ptr(wallet_id).to_string_lossy() }, account_index, confirmed, amount, is_ours); + let wallet_id_str = if wallet_id.is_null() { + "<null>".to_string() + } else { + unsafe { CStr::from_ptr(wallet_id).to_string_lossy().into_owned() } + }; + println!( + "Test wallet transaction callback called: wallet={}, account={}, confirmed={}, amount={}, is_ours={}", + wallet_id_str, account_index, confirmed, amount, is_ours + ); let data = unsafe { &*(user_data as *const TestEventData) }; - // Convert wallet ID to string - let wallet_id_str = unsafe { CStr::from_ptr(wallet_id).to_string_lossy().into_owned() }; + // wallet_id already parsed abovekey-wallet-ffi/src/address.rs (2)
37-50: address_array_free: incorrect deallocation of the array bufferBox::from_raw on a slice created via from_raw_parts_mut is invalid here. Reconstruct the original Vec to free the buffer, after freeing each string.
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)); - } + // Rebuild the vector to free the buffer; free each C string first + let mut vec = Vec::from_raw_parts(addresses, count, count); + for addr in vec.drain(..) { + if !addr.is_null() { + let _ = CString::from_raw(addr); + } + } + // vec drops here, releasing the array buffer } }
61-67: Validate FFINetwork before useLike in transaction_checking.rs, prefer a fallible conversion to avoid UB on bad discriminants from C.
- network: FFINetwork, + network: FFINetwork, @@ - let network_rust: key_wallet::Network = network.into(); + use core::convert::TryInto; + let network_rust: key_wallet::Network = match network.try_into() { + Ok(n) => n, + Err(_) => { + FFIError::set_error(error, FFIErrorCode::InvalidInput, "Invalid network".to_string()); + return false; + } + };And similarly in address_get_type:
- let network_rust: key_wallet::Network = network.into(); + use core::convert::TryInto; + let network_rust: key_wallet::Network = match network.try_into() { + Ok(n) => n, + Err(_) => { + FFIError::set_error(error, FFIErrorCode::InvalidInput, "Invalid network".to_string()); + return u8::MAX; + } + };Also applies to: 83-84, 130-137, 153-154
key-wallet-ffi/src/managed_account.rs (2)
119-129: Free error.message to avoid leaking the error C string.When wallet_manager_get_managed_wallet_info fails, error.message (if set) is converted to Rust String but never freed.
- 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 message buffer allocated by the callee + let _ = std::ffi::CString::from_raw(error.message); + s + }; + return FFIManagedAccountResult::error(error.code, msg); + }
578-602: AddressPool lifetime: avoid returning raw pointers into ManagedAccount without ownership ties.Current FFIAddressPool embeds a raw pointer to AddressPool owned by ManagedAccount. If the caller frees the account before the pool, this becomes UAF. Return a handle that keeps the owning ManagedAccount alive (e.g., store Arc inside FFIAddressPool) or allocate/clone an owned pool structure for FFI.
I can draft a minimal FFIAddressPool that includes an Arc to tie lifetimes if you want.
Also applies to: 614-638, 653-746
key-wallet-ffi/src/managed_wallet.rs (2)
746-752: Tests: use the proper free routine for address arrays everywhereIn test_comprehensive_address_generation, external addresses are freed with libc::free, while internal addresses use address_array_free. Replace the former with address_array_free to avoid allocator mismatches.
- libc::free(addresses_out as *mut libc::c_void); + // Free both the array and the contained C strings + crate::address::address_array_free(addresses_out, count_out);Also applies to: 1057-1072, 1090-1106, 1231-1266
301-315: Replace manuallibc::freewithaddress_array_freein tests
- In key-wallet-ffi/src/managed_wallet.rs at line 1087, change
tolibc::free(addresses_out as *mut libc::c_void);so that each C string and the array are reclaimed correctly instead of invoking undefined behavior.crate::address::address_array_free(addresses_out, count);dash-spv-ffi/include/dash_spv_ffi.h (3)
33-33: Fix opaque type:FFIClientConfigis aliased to an unknownClientConfig(C compile break).This must be an opaque struct forward-decl, not a typedef to an undefined Rust type.
Apply:
-typedef ClientConfig FFIClientConfig; +typedef struct FFIClientConfig FFIClientConfig;
1-1: Regenerate and sync Swift FFI header.The new
FFINetworkenum isn’t declared indash-spv-ffi/include/dash_spv_ffi.h, so run:./scripts/generate_headers.sh && \ ./swift-dash-core-sdk/sync-headers.shto update
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h.
166-198: Correct FFIUnconfirmedTransaction ownership in header and Rust docs
This struct is output-only: Rust allocatesraw_txand theaddressesarray when producingFFIUnconfirmedTransaction, and C callers must free them viadash_spv_ffi_unconfirmed_transaction_destroy_raw_tx,dash_spv_ffi_unconfirmed_transaction_destroy_addresses(and finallydash_spv_ffi_unconfirmed_transaction_destroy). Remove all “caller allocates before passing to Rust” language and update both the C header and Rust doc comments to reflect Rust-owned allocation.key-wallet-ffi/src/keys.rs (4)
469-476: Likely type mismatch:dashcore::PrivateKey.networkexpectsdashcore::Network, notkey_wallet::Network.Convert the network before constructing the Dash key.
- let dash_key = DashPrivateKey { + let network_dc: dashcore::Network = network_rust.into(); + let dash_key = DashPrivateKey { compressed: true, - network: network_rust, + network: network_dc, inner: key.inner, };
437-444: Move out of borrowedextended.inner.private_key— clone required.Avoid moving fields from a borrowed value.
- let private_key = FFIPrivateKey { - inner: extended.inner.private_key, - }; + let private_key = FFIPrivateKey { + inner: extended.inner.private_key.clone(), + };
813-820: Move out of borrowedextended.inner.public_key— clone required.Same issue for public key extraction.
- let public_key = FFIPublicKey { - inner: extended.inner.public_key, - }; + let public_key = FFIPublicKey { + inner: extended.inner.public_key.clone(), + };
454-488: SecretKey move inprivate_key_to_wifmay also require cloning.If
secp256k1::SecretKeyisn’tCopy,inner: key.innermoves out of a borrow. Clone to be safe.- inner: key.inner, + inner: key.inner.clone(),dash-spv-ffi/src/client.rs (1)
224-258: Don’t invoke user callbacks while holding the event_callbacks mutex.You lock event_callbacks and then call into foreign/user code via callbacks.call_…(). That can deadlock and amplifies contention. Copy out the function pointers (or clone a lightweight callbacks struct) under the lock, drop the lock, then invoke the callbacks.
If FFIEventCallbacks isn’t Clone, extract the raw fn pointers + user_data into locals while holding the lock, then drop it before calling.
Also applies to: 263-271, 280-299, 300-316
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h (2)
33-33: Broken typedef for FFIClientConfig leaks Rust type into C headerC sees
ClientConfigas unknown. This must be an opaque C struct, not an alias to a Rust type.Apply:
-typedef ClientConfig FFIClientConfig; +typedef struct FFIClientConfig FFIClientConfig;
624-639: Do not free C-allocated memory from Rust
dash_spv_ffi_unconfirmed_transaction_destroy_raw_txdocs say the buffer is allocated by the caller (C) but then freed by a Rust function. That is undefined if allocators differ.Either:
- Rust allocates and C frees via this function (document that clearly), or
- C allocates and C frees (remove this function; callers use
free()), or- Provide paired alloc/free in the same library.
Recommend: have Rust allocate the raw_tx when returning to C; document that
dash_spv_ffi_unconfirmed_transaction_destroy_raw_txmust be used to free it.key-wallet-ffi/include/key_wallet_ffi.h (2)
422-426: C-invisible Rust type in public struct breaks compilation
FFIManagedWalletembedsManagedWalletInfo *innerbutManagedWalletInfois unknown to C. Make this type opaque or usevoid*.Apply:
-typedef struct { - ManagedWalletInfo *inner; -} FFIManagedWallet; +typedef struct FFIManagedWallet FFIManagedWallet;And keep all APIs using
FFIManagedWallet*without exposing internals.
894-895: Inconsistent network return type: migrate to single-networkThese still return
FFINetworkswhile most APIs moved toFFINetwork. This inconsistency will confuse consumers and complicate Swift bridging.Change return types to
FFINetwork(and update Rust impls):-FFINetworks account_get_network(const FFIAccount *account); -FFINetworks bls_account_get_network(const FFIBLSAccount *account); -FFINetworks eddsa_account_get_network(const FFIEdDSAAccount *account); +FFINetwork account_get_network(const FFIAccount *account); +FFINetwork bls_account_get_network(const FFIBLSAccount *account); +FFINetwork eddsa_account_get_network(const FFIEdDSAAccount *account);If multi-network is needed for creation APIs, keep FFINetworks only there.
Also applies to: 948-949, 1005-1006
| pub unsafe extern "C" fn transaction_add_output( | ||
| tx: *mut FFITransaction, | ||
| output: *const FFITxOut, | ||
| ) -> i32 { | ||
| if tx.is_null() || output.is_null() { | ||
| return -1; | ||
| } | ||
|
|
||
| let tx = &mut *tx; | ||
| let output = &*output; | ||
|
|
||
| // Convert script | ||
| let script_pubkey = if output.script_pubkey.is_null() || output.script_pubkey_len == 0 { | ||
| return -1; | ||
| } else { | ||
| let script_slice = | ||
| slice::from_raw_parts(output.script_pubkey, output.script_pubkey_len as usize); | ||
| ScriptBuf::from(script_slice.to_vec()) | ||
| }; | ||
|
|
||
| let tx_out = TxOut { | ||
| value: output.amount, | ||
| script_pubkey, | ||
| }; | ||
|
|
||
| tx.inner.output.push(tx_out); | ||
| 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate script_pubkey length before allocation
Mirror the input guard here to prevent oversized allocations.
| mnemonic.as_ptr(), | ||
| passphrase.as_ptr(), | ||
| FFINetworks::Testnet, | ||
| FFINetworks::TestnetFlag, |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Regenerate FFI headers after enum changes
Enum layout changes must be reflected in generated C headers; please run cbindgen and sync headers to downstream (Swift SDK) so CI for FFI docs stays green.
Also applies to: 87-87, 136-136, 185-185, 234-234, 301-301, 337-337, 393-393
🏁 Script executed:
#!/bin/bash
# From repo root, verify headers mention TestnetFlag and are up to date
fd dash_spv_ffi.h | xargs -I{} rg -n "TestnetFlag" {}
fd "include/*.h" key-wallet-ffi swift-dash-core-sdk | xargs -I{} rg -n "TestnetFlag" {}Length of output: 149
Regenerate and sync FFI headers after adding TestnetFlag
Run cbindgen to update dash-spv-ffi/include/dash_spv_ffi.h, then execute ./sync-headers.sh to propagate the new enum layout (including TestnetFlag) into swift-dash-core-sdk/Sources/DashSPVFFI/include so the C headers and Swift SDK stay in sync.
🤖 Prompt for AI Agents
In key-wallet-ffi/src/wallet_manager_serialization_tests.rs around line 35, you
added FFINetworks::TestnetFlag but did not regenerate FFI headers; run cbindgen
to update dash-spv-ffi/include/dash_spv_ffi.h and then run ./sync-headers.sh to
copy the updated header into swift-dash-core-sdk/Sources/DashSPVFFI/include so
the new enum layout (including TestnetFlag) is propagated and C/Swift headers
remain in sync.
| use crate::FFINetwork; | ||
|
|
There was a problem hiding this comment.
Fix incorrect import of FFINetwork
FFINetwork is defined in types.rs and (per PR notes) is no longer re-exported at the crate root. Import from crate::types to avoid compile errors.
- use crate::FFINetwork;
+ use crate::types::FFINetwork;📝 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.
| use crate::FFINetwork; | |
| use crate::types::FFINetwork; |
🤖 Prompt for AI Agents
In key-wallet-ffi/src/wallet.rs around lines 17 to 18, the file imports
FFINetwork from the crate root but FFINetwork now lives in crate::types; update
the import to use crate::types::FFINetwork (replace use crate::FFINetwork; with
use crate::types::FFINetwork;), recompile to ensure there are no other broken
re-exports or path issues, and adjust any other local references if necessary.
| /** | ||
| * Get the wallet manager from the SPV client | ||
| * | ||
| * Returns an opaque pointer to FFIWalletManager that contains a cloned Arc reference to the wallet manager. | ||
| * This allows direct interaction with the wallet manager without going through the client. | ||
| * | ||
| * # Safety | ||
| * | ||
| * The caller must ensure that: | ||
| * - The client pointer is valid | ||
| * - The returned pointer is freed using wallet_manager_free() | ||
| * | ||
| * # Returns | ||
| * | ||
| * An opaque pointer (void*) to the wallet manager, or NULL if the client is not initialized. | ||
| * Swift should treat this as an OpaquePointer. | ||
| */ | ||
| void *dash_spv_ffi_client_get_wallet_manager(struct FFIDashSpvClient *client); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cross-allocator free hazard for wallet manager pointer
Returning void* and instructing callers to free with wallet_manager_free() (from a different crate) risks freeing across allocators if these are separate dylibs. The library that allocates must also free.
Prefer one of:
- Return
struct FFIWalletManager*and ensure it is the same concrete type from the same crate (single allocator), or - Keep
void*but add a matchingdash_spv_ffi_wallet_manager_destroy(void*)exported from this crate and require callers to use it.
Example header change:
-void *dash_spv_ffi_client_get_wallet_manager(struct FFIDashSpvClient *client);
+struct FFIWalletManager *dash_spv_ffi_client_get_wallet_manager(struct FFIDashSpvClient *client);
+void dash_spv_ffi_wallet_manager_destroy(struct FFIWalletManager *manager);🤖 Prompt for AI Agents
In swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h around lines
319 to 337, the function dash_spv_ffi_client_get_wallet_manager returns a void*
which risks cross-allocator free hazards; change the API so the allocator that
creates the wallet manager also provides the deallocator: either (preferred) add
and export a companion destructor in this crate (e.g.
dash_spv_ffi_wallet_manager_destroy(void*)) and update the comment to require
callers to use that destroy function, or switch the return type to a concrete
struct FFIWalletManager* that is always allocated/freed by this crate; implement
the chosen deallocator in Rust and update the header and documentation to
mandate using the crate-provided free function (do not rely on external
wallet_manager_free()).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dash-spv/tests/edge_case_filter_sync_test.rs (3)
178-182: Avoid unwrap and double-check; collapse into single assert with error context.This keeps the error message if the call fails and avoids moving
resulttwice.- let result = filter_sync.start_sync_headers(&mut network, &mut storage).await; - assert!(result.is_ok()); - assert!(!result.unwrap(), "Should not start sync when already at tip"); + let started = filter_sync + .start_sync_headers(&mut network, &mut storage) + .await + .expect("start_sync_headers failed"); + assert!(!started, "Should not start sync when already at tip");
282-286: Apply the same pattern here for consistency and clarity.- let result = filter_sync.start_sync_headers(&mut network, &mut storage).await; - assert!(result.is_ok()); - assert!(result.unwrap(), "Should start sync when behind by 1 block"); + let started = filter_sync + .start_sync_headers(&mut network, &mut storage) + .await + .expect("start_sync_headers failed"); + assert!(started, "Should start sync when behind by 1 block");
6-6: Redundant skipping: cfg(feature) gate + #[ignore].You’re gating the whole file behind a feature and also marking tests
#[ignore]. Consider using just one mechanism to reduce confusion (keep thecfg(feature = ...)if the mock doesn’t compile outside the feature).Also applies to: 145-147
📜 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)
dash-spv/tests/block_download_test.rs(2 hunks)dash-spv/tests/edge_case_filter_sync_test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/tests/block_download_test.rs
🧰 Additional context used
📓 Path-based instructions (5)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/tests/edge_case_filter_sync_test.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Place integration tests under the
tests/directory
Files:
dash-spv/tests/edge_case_filter_sync_test.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
Files:
dash-spv/tests/edge_case_filter_sync_test.rs
{dash-spv,rpc-integration-test}/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide integration tests for network operations
Files:
dash-spv/tests/edge_case_filter_sync_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
Files:
dash-spv/tests/edge_case_filter_sync_test.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
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/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash-spv/tests/edge_case_filter_sync_test.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Core Components Tests
- GitHub Check: Clippy (Non-strict)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (1)
dash-spv/tests/edge_case_filter_sync_test.rs (1)
181-181: Idiomatic boolean assert — LGTM.Switching to
assert!(!...)is clearer for boolean checks.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
dash-spv-ffi/include/dash_spv_ffi.h (1)
33-35: FFIClientConfig typedef is invalid C; header appears out of sync with Rust
typedef ClientConfig FFIClientConfig;won’t compile in C becauseClientConfigis unknown here. The Rust side exposes a concreteFFIClientConfig { inner: ClientConfig }, so cbindgen should emittypedef struct FFIClientConfig FFIClientConfig;.Action:
- Regenerate this header via cbindgen; if needed, add cbindgen rename/export annotations so
FFIClientConfigis emitted as a named struct.- As a temporary hotfix (not preferred since generated), change to:
-typedef ClientConfig FFIClientConfig; +typedef struct FFIClientConfig FFIClientConfig;dash-spv-ffi/src/client.rs (2)
811-820: Same here: don’t unwrap() mutex in FFI pathReturn null and set an error instead of panicking when the lock is poisoned.
- let result = client.runtime.block_on(async { - let guard = inner.lock().unwrap(); + let result = client.runtime.block_on(async { + let guard = match inner.lock() { + Ok(g) => g, + Err(_) => { + set_last_error("Client mutex poisoned"); + return Err(dash_spv::SpvError::Config("Client lock poisoned".into())); + } + };
840-849: Repeat: replace unwrap() on mutex with fallible handlingPrevent process aborts from crossing FFI.
- let result = client.runtime.block_on(async { - let guard = inner.lock().unwrap(); + let result = client.runtime.block_on(async { + let guard = match inner.lock() { + Ok(g) => g, + Err(_) => { + set_last_error("Client mutex poisoned"); + return Err(dash_spv::SpvError::Config("Client lock poisoned".into())); + } + };swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h (1)
617-641: Ownership docs for FFIUnconfirmedTransaction inputs are inconsistentComments say raw_tx and addresses are allocated by the caller but provide destroy_* functions here, implying the library frees them. Pick one model and document it consistently. Best practice: Rust allocates outputs, C/Swift frees via provided destroy_* functions; inputs from C are borrowed and never freed by Rust.
Suggested doc updates:
- * - `raw_tx`: A pointer to the raw transaction bytes. The caller is responsible for: - * - Allocating this memory before passing it to Rust + * - `raw_tx`: A pointer to raw transaction bytes allocated by Rust when returning a value to C. + * The caller is responsible for: - * - Freeing the memory after use with `dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx` + * - Freeing the memory after use with `dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx` @@ - * - `addresses`: A pointer to an array of FFIString objects. The caller is responsible for: - * - Allocating this array before passing it to Rust + * - `addresses`: A pointer to an array of FFIString values allocated by Rust when returning a value to C. + * The caller is responsible for: @@ - * - The pointer must not be used after this function is called + * - The pointer must not be used after this function is calledAlso fix string-array docs to reflect values, not “pointers”:
- * Destroy an array of FFIString pointers (Vec<*mut FFIString>) and their contents. + * Destroy an array of FFIString values (Vec<FFIString>) and their contents.
♻️ Duplicate comments (6)
key-wallet-ffi/src/keys_tests.rs (1)
8-8: FFINetwork import added — resolves earlier compile errorImporting both FFINetwork and FFINetworks fixes the symbol-resolution issue flagged in a prior review. Good catch.
dash-spv-ffi/FFI_API.md (1)
651-656: Clarify ownership to prevent double-free of addresses.Wording says each FFIString must be destroyed separately, but elsewhere the parent destroy frees “all resources.” Make the exclusivity explicit.
- Destroys the addresses array allocated for an FFIUnconfirmedTransaction + Destroys only the contiguous addresses array buffer for an FFIUnconfirmedTransaction. + DOES NOT destroy the individual FFIString elements. + Do not call this if `dash_spv_ffi_unconfirmed_transaction_destroy` will be used..github/workflows/verify-ffi-docs.yml (1)
23-37: Install Python deps for the doc generators to avoid runtime import errorsIf scripts import external packages, these jobs will fail. Add a conditional pip install step in both jobs before running the generators.
verify-key-wallet-docs: runs-on: ubuntu-latest steps: - name: Checkout repository uses: actions/checkout@v4 - name: Set up Python uses: actions/setup-python@v5 with: python-version: '3.10' + - name: Install generator requirements (if any) + run: | + if [ -f key-wallet-ffi/scripts/requirements.txt ]; then + python3 -m pip install -r key-wallet-ffi/scripts/requirements.txt + fi - name: Generate key-wallet-ffi documentation run: | cd key-wallet-ffi python3 scripts/generate_ffi_docs.py @@ verify-dash-spv-docs: runs-on: ubuntu-latest steps: - name: Checkout repository uses: actions/checkout@v4 - name: Set up Python uses: actions/setup-python@v5 with: python-version: '3.10' + - name: Install generator requirements (if any) + run: | + if [ -f dash-spv-ffi/scripts/requirements.txt ]; then + python3 -m pip install -r dash-spv-ffi/scripts/requirements.txt + fi - name: Generate dash-spv-ffi documentation run: | cd dash-spv-ffi python3 scripts/generate_ffi_docs.pyAlso applies to: 55-68
dash-spv-ffi/include/dash_spv_ffi.h (2)
319-336: Clarify ownership and lifetime of the returned wallet manager pointerCallers need explicit guidance on freeing and lifetime independence from the client (Arc clone semantics).
/** * Get the wallet manager from the SPV client * * Returns an opaque pointer to FFIWalletManager that contains a cloned Arc reference to the wallet manager. * This allows direct interaction with the wallet manager without going through the client. * * # Safety * * The caller must ensure that: * - The client pointer is valid - * - The returned pointer is freed using `wallet_manager_free` from key-wallet-ffi + * - The returned pointer is freed using `wallet_manager_free` declared in key-wallet-ffi's public header + * (include that header in consumers). The returned FFIWalletManager remains valid independently of + * the client instance because it holds an Arc clone. * * # Returns * * An opaque pointer (void*) to the wallet manager, or NULL if the client is not initialized. * Swift should treat this as an OpaquePointer. */
338-338: FFINetwork is referenced but not declared/includedFunctions use
FFINetworkbut the enum isn’t present and no header that defines it is included. C/Swift consumers will fail to compile.Action (pick the correct source of truth in this repo):
- Prefer include:
+#include "dash_network_ffi.h"
- Or ensure cbindgen in this crate emits the enum definition directly.
- Regenerate and sync headers (also the Swift copy).
Also applies to: 424-424
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h (1)
322-339: Cross-allocator free hazard for wallet manager pointer (repeat)Returning
void*and asking callers to free viawallet_manager_free()(from a different crate/lib) risks freeing across allocators. The allocator that allocates must free. Prefer a concrete type here and provide a destroy function from this crate.Preferred fix:
-/** - * Get the wallet manager from the SPV client - * - * Returns an opaque pointer to FFIWalletManager that contains a cloned Arc reference to the wallet manager. - * This allows direct interaction with the wallet manager without going through the client. - * - * # Safety - * - * The caller must ensure that: - * - The client pointer is valid - * - The returned pointer is freed using wallet_manager_free() - * - * # Returns - * - * An opaque pointer (void*) to the wallet manager, or NULL if the client is not initialized. - * Swift should treat this as an OpaquePointer. - */ -void *dash_spv_ffi_client_get_wallet_manager(struct FFIDashSpvClient *client); +/** + * Get the wallet manager from the SPV client. + * Ownership: returned pointer must be released with dash_spv_ffi_wallet_manager_destroy. + */ +struct FFIWalletManager *dash_spv_ffi_client_get_wallet_manager(struct FFIDashSpvClient *client); +void dash_spv_ffi_wallet_manager_destroy(struct FFIWalletManager *manager);If you must keep
void*, then at minimum exportvoid dash_spv_ffi_wallet_manager_destroy(void *manager);from this crate and mandate its use here.
🧹 Nitpick comments (23)
key-wallet-ffi/src/keys_tests.rs (6)
43-45: extended_*_to_string ignores the network arg — document/verify invariance in testsPer keys.rs, extended_private_key_to_string and extended_public_key_to_string ignore the network parameter (network already encoded in the key). Consider asserting this invariance in tests to prevent future regressions.
Add after current assertions:
// Network arg should be ignored by *_to_string (should yield the same string) let xprv_str_mainnet = extended_private_key_to_string(ext_priv, FFINetwork::Mainnet, &mut error); assert_eq!(CStr::from_ptr(xprv_str_mainnet).to_str().unwrap(), xprv); crate::utils::string_free(xprv_str_mainnet); let xpub_str_mainnet = extended_public_key_to_string(ext_pub, FFINetwork::Mainnet, &mut error); assert_eq!(CStr::from_ptr(xpub_str_mainnet).to_str().unwrap(), xpub); crate::utils::string_free(xpub_str_mainnet);Also applies to: 81-83
57-59: Possible footgun: private_key_to_wif trusts the passed networkprivate_key_to_wif encodes WIF using the provided FFINetwork without cross-checking the wallet/key network. This can silently produce mismatched-network WIFs. Recommend either (a) enforcing a consistency check in keys.rs or (b) documenting this behavior and adding a negative test that expects an error on mismatch (preferred per “never mix networks” guidance).
If you want, I can open a follow-up to add a consistency check in private_key_to_wif and update tests accordingly.
Also applies to: 208-211
197-205: Fix coin-type comments (clarify testnet vs mainnet)Comments mention “Dash” or “coin type 5” while paths use testnet coin type 1. Tighten wording for clarity.
Apply:
- // Try to derive private key - should now succeed (44'/1'/0'/0/0 for Dash) + // Try to derive private key - should now succeed (44'/1'/0'/0/0 for Dash testnet; coin type 1) - // 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) - // Test different derivation paths (Dash coin type 5) + // Test different derivation paths (Dash testnet, coin type 1; mainnet would be coin type 5)Also applies to: 251-256, 642-648
595-612: Tighten assertions when xpub is null (expect NotFound)When an account index doesn’t exist, wallet_get_account_xpub should set NotFound. Assert it explicitly to validate error semantics.
for account_index in 0..3 { let xpub_str = unsafe { wallet_get_account_xpub(wallet, FFINetwork::Testnet, account_index, &mut error) }; if !xpub_str.is_null() { let xpub = unsafe { CStr::from_ptr(xpub_str).to_str().unwrap() }; assert!(xpub.starts_with("tpub")); // Testnet public key // Clean up unsafe { crate::utils::string_free(xpub_str); } + } else { + assert_eq!(error.code, FFIErrorCode::NotFound); } }
419-430: Also assert the error code for invalid derivation pathYou already assert !success. Add the expected error code to pin behavior.
let success = unsafe { derivation_path_parse( invalid_path.as_ptr(), &mut indices_out, &mut hardened_out, &mut count_out, &mut error, ) }; assert!(!success); + assert_eq!(error.code, FFIErrorCode::InvalidInput);
18-26: Reduce duplication with a small helper to create a test walletWallet creation boilerplate repeats across tests. A tiny helper improves readability and maintenance.
Add inside the tests module:
fn make_test_wallet(error: &mut FFIError) -> *mut wallet::FFIWallet { let mnemonic = CString::new("abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about").unwrap(); let passphrase = CString::new("").unwrap(); unsafe { wallet::wallet_create_from_mnemonic( mnemonic.as_ptr(), passphrase.as_ptr(), FFINetworks::TestnetFlag, error, ) } }Then replace repeated creation calls with:
let wallet = make_test_wallet(&mut error);Also applies to: 120-127, 152-159, 188-195, 238-245, 295-301, 449-456, 585-592, 633-640
swift-dash-core-sdk/build-ios.sh (1)
4-4: Harden the script: pipefail + guard sync-headers.sh.Add pipefail/unset protection and a friendly check so failures surface clearly.
-set -e +set -euo pipefailecho -e "${GREEN}Syncing headers to Swift SDK...${NC}" -./sync-headers.sh +if [[ ! -x ./sync-headers.sh ]]; then + echo -e "${RED}sync-headers.sh not found or not executable${NC}" + exit 1 +fi +./sync-headers.shAlso applies to: 81-82
dash-spv-ffi/FFI_API.md (3)
720-777: Unify FFI symbol prefixes.Platform integration uses
ffi_dash_spv_*while the rest usedash_spv_ffi_*. Consider standardizing for consistency and easier discovery.
938-958: Add missing type definitions (FFIString, FFIArray, FFIResult).These are referenced by APIs but not defined in the Types section; add concise field/ownership descriptions.
### Core Types @@ - `FFIEventCallbacks` - Event callback structure - `CoreSDKHandle` - Platform SDK integration handle + - `FFIString` - Owned UTF-8 string returned across FFI; free with `dash_spv_ffi_string_destroy` + - `FFIArray` - Heap-allocated array wrapper { data: *mut T, len: usize, cap: usize }; free with the matching destroy + - `FFIResult` - Result code/details for platform helpers (success flag, optional error)
235-240: Deduplicate Safety text in Description blocks (generator issue).“Description” includes “# Safety …” and you also render a separate Safety section, causing duplication throughout. Fix in the generator to strip the Safety subsection from Description.
key-wallet-ffi/scripts/check_ffi_docs.sh (1)
5-5: Enable pipefail/unset var protection.Improves reliability of CI checks.
-set -e +set -euo pipefailkey-wallet-ffi/FFI_DOCS_README.md (1)
78-96: Tighten safety note for wallet_id length.State the invariant on the length parameter explicitly.
-/// - `wallet_id` must point to exactly 32 bytes +/// - `wallet_id` must point to exactly 32 bytes +/// - `wallet_id_len` must be 32dash-spv-ffi/scripts/generate_ffi_docs.py (4)
6-13: Remove unused imports.
osandsubprocessaren’t used.-import os import re import sys @@ -import subprocess
93-121: Strip “# Safety” from Description to avoid duplication.Currently, Description includes Safety text; you also render a Safety section. Trim doc_comment at the first “# Safety”.
- # Extract safety sub-section from doc lines + # Extract safety sub-section from doc lines and strip it from description safety_comment = None if doc_lines: joined = '\n'.join(doc_lines) if '# Safety' in joined: safety_lines: List[str] = [] in_safety = False + main_doc: List[str] = [] for dl in doc_lines: if dl.strip().startswith('# Safety'): in_safety = True continue if in_safety and dl.strip().startswith('#'): break if in_safety: safety_lines.append(dl) - safety_comment = ' '.join(safety_lines).strip() if safety_lines else None + else: + main_doc.append(dl) + safety_comment = ' '.join(safety_lines).strip() if safety_lines else None + doc_lines = main_doc # remove safety from description @@ - doc_comment=' '.join(doc_lines) if doc_lines else None, + doc_comment=' '.join(doc_lines) if doc_lines else None,
153-155: Parenthesize mixed and/or condition for clarity (RUF021).Avoid precedence surprises.
- elif 'watch' in name or 'unwatch' in name or 'address' in name and 'balance' not in name: + elif 'watch' in name or 'unwatch' in name or ('address' in name and 'balance' not in name):
260-276: Include FFIString/FFIArray/FFIResult in Type Definitions.These types are referenced in generated docs; add them here.
md.append("- `FFIEventCallbacks` - Event callback structure") md.append("- `CoreSDKHandle` - Platform SDK integration handle") md.append("") + md.append("- `FFIString` - Owned UTF-8 string returned across FFI; free with `dash_spv_ffi_string_destroy`") + md.append("- `FFIArray` - Heap-allocated array wrapper { data: *mut T, len: usize, cap: usize }") + md.append("- `FFIResult` - Result code/details for platform helpers")dash-spv-ffi/include/dash_spv_ffi.h (1)
272-279: Document that FFIDetailedSyncProgress passed to the progress callback is ephemeralYou allocate on Rust side and pass a pointer only for the duration of the callback; consumers must not retain it.
* - `progress_callback`: Optional callback invoked periodically with sync progress + * The `FFIDetailedSyncProgress*` passed is only valid for the duration of the callback. + * Do not store the pointer; copy out needed fields immediately.dash-spv-ffi/src/client.rs (3)
651-689: Don’t hold event_callbacks lock while invoking user callbacksYou lock
event_callbacksand then call into user code viacallbacks.call_*. If user code re-entersdash_spv_ffi_client_set_event_callbacks, this can deadlock.Minimal fix: clone/copy the function pointers out while holding the lock, drop the lock, then invoke callbacks. If
FFIEventCallbacksisn’tCopy/Clone, extract only the fn pointers and user_data.
1027-1072: Provide a 32-byte Txid variant to avoid string parsing overhead and encoding risksGuideline is to pass txids as fixed-size 32-byte arrays across FFI.
Propose adding:
#[no_mangle] pub unsafe extern "C" fn dash_spv_ffi_client_record_send_bytes( client: *mut FFIDashSpvClient, txid_bytes: *const u8, // must be 32 bytes ) -> i32 { /* parse from slice<[u8;32]>, same logic; validate non-null + length */ }I can draft the exact impl if desired.
621-689: Progress callback struct lifetime: document ephemeral pointer in the C headerYou Box the progress struct and pass a pointer valid only during the callback; ensure the header comment explicitly says “do not store the pointer.”
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h (3)
150-168: Clarify FFIArray ownership semantics in docsSpecify explicitly that dash_spv_ffi_array_destroy frees only the underlying buffer (data/len/capacity) and never frees the FFIArray wrapper itself.
/** * FFI-safe array that transfers ownership of memory to the C caller. * * # Safety * @@ - * - Calling `dash_spv_ffi_array_destroy` to properly deallocate the memory + * - Calling `dash_spv_ffi_array_destroy` to free the underlying buffer only. + * This function does not free the FFIArray struct itself (safe for stack- or heap-allocated wrappers).
275-281: Document error_message lifetime in completion callbackState that the const char* passed to completion_callback is valid only for the duration of the call and must not be retained.
- * - `completion_callback`: Optional callback invoked on completion + * - `completion_callback`: Optional callback invoked on completion. + * The error message pointer (if non-null) is valid only during the callback.
661-663: Broadcast API is stubbed; document or gate itThe Rust impl returns “Not implemented” and sets last error. Call this out in the header or behind a feature flag to avoid confusion for SDK consumers.
Proposed doc tweak:
- int32_t dash_spv_ffi_client_broadcast_transaction(struct FFIDashSpvClient *client, - const char *tx_hex); + /** Currently not implemented; returns a non-zero error and sets last error. */ + int32_t dash_spv_ffi_client_broadcast_transaction(struct FFIDashSpvClient *client, + const char *tx_hex);
| **Description:** | ||
| Destroys an FFIUnconfirmedTransaction and all its associated resources # Safety - `tx` must be a valid pointer to an FFIUnconfirmedTransaction - All resources (raw_tx, addresses array, and individual FFIStrings) will be freed - The pointer must not be used after this function is called - This function should only be called once per FFIUnconfirmedTransaction | ||
|
|
||
| **Safety:** | ||
| - `tx` must be a valid pointer to an FFIUnconfirmedTransaction - All resources (raw_tx, addresses array, and individual FFIStrings) will be freed - The pointer must not be used after this function is called - This function should only be called once per FFIUnconfirmedTransaction | ||
|
|
There was a problem hiding this comment.
Document exclusivity of the “destroy all” API.
Avoid callers freeing children and then calling the parent destroy.
- All resources (raw_tx, addresses array, and individual FFIStrings) will be freed
+ All resources (raw_tx, addresses array, and individual FFIStrings) will be freed.
+ After calling this, do not call any of:
+ `dash_spv_ffi_unconfirmed_transaction_destroy_addresses`,
+ `dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx`,
+ or `dash_spv_ffi_string_destroy` for elements derived from this object.📝 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.
| **Description:** | |
| Destroys an FFIUnconfirmedTransaction and all its associated resources # Safety - `tx` must be a valid pointer to an FFIUnconfirmedTransaction - All resources (raw_tx, addresses array, and individual FFIStrings) will be freed - The pointer must not be used after this function is called - This function should only be called once per FFIUnconfirmedTransaction | |
| **Safety:** | |
| - `tx` must be a valid pointer to an FFIUnconfirmedTransaction - All resources (raw_tx, addresses array, and individual FFIStrings) will be freed - The pointer must not be used after this function is called - This function should only be called once per FFIUnconfirmedTransaction | |
| **Description:** | |
| Destroys an FFIUnconfirmedTransaction and all its associated resources | |
| # Safety | |
| - `tx` must be a valid pointer to an FFIUnconfirmedTransaction | |
| - All resources (raw_tx, addresses array, and individual FFIStrings) will be freed. | |
| - After calling this, do not call any of: | |
| - `dash_spv_ffi_unconfirmed_transaction_destroy_addresses`, | |
| - `dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx`, | |
| - or `dash_spv_ffi_string_destroy` for elements derived from this object. | |
| - The pointer must not be used after this function is called | |
| - This function should only be called once per FFIUnconfirmedTransaction |
🧰 Tools
🪛 LanguageTool
[grammar] ~679-~679: There might be a mistake here.
Context: ...ransaction,) -> () ``` Description: Destroys an FFIUnconfirmedTransaction an...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In dash-spv-ffi/FFI_API.md around lines 679 to 684, the documentation for
destroying an FFIUnconfirmedTransaction must explicitly state exclusive
ownership semantics so callers do not free child resources before calling the
parent destructor; update the description and safety sections to say that the
FFIUnconfirmedTransaction takes ownership of all child resources (raw_tx,
addresses array, individual FFIStrings), that callers must not free or retain
pointers to any child resources and must call either individual child
destructors OR the parent destroy but never both, and add a short note that
freeing children prior to calling the parent (or using any pointer after parent
destroy) is undefined behavior to prevent double-free/use-after-free errors.
| **Description:** | ||
| Destroys the raw transaction bytes allocated for an FFIUnconfirmedTransaction # Safety - `raw_tx` must be a valid pointer to memory allocated by the caller - `raw_tx_len` must be the correct length of the allocated memory - The pointer must not be used after this function is called - This function should only be called once per allocation | ||
| **Safety:** | ||
| - `raw_tx` must be a valid pointer to memory allocated by the caller - `raw_tx_len` must be the correct length of the allocated memory - The pointer must not be used after this function is called - This function should only be called once per allocation | ||
There was a problem hiding this comment.
Fix allocator attribution for raw_tx.
“allocated by the caller” is contradictory for a library-owned destroy. Correct to “allocated by the library and returned to the caller,” or remove if not applicable.
- `raw_tx` must be a valid pointer to memory allocated by the caller
+ `raw_tx` must be a valid pointer to memory allocated by the library and returned to the caller📝 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.
| **Description:** | |
| Destroys the raw transaction bytes allocated for an FFIUnconfirmedTransaction # Safety - `raw_tx` must be a valid pointer to memory allocated by the caller - `raw_tx_len` must be the correct length of the allocated memory - The pointer must not be used after this function is called - This function should only be called once per allocation | |
| **Safety:** | |
| - `raw_tx` must be a valid pointer to memory allocated by the caller - `raw_tx_len` must be the correct length of the allocated memory - The pointer must not be used after this function is called - This function should only be called once per allocation | |
| **Description:** | |
| Destroys the raw transaction bytes allocated for an FFIUnconfirmedTransaction | |
| **Safety:** | |
| - `raw_tx` must be a valid pointer to memory allocated by the library and returned to the caller | |
| - `raw_tx_len` must be the correct length of the allocated memory | |
| - The pointer must not be used after this function is called | |
| - This function should only be called once per allocation |
🧰 Tools
🪛 LanguageTool
[grammar] ~695-~695: There might be a mistake here.
Context: ...len: usize,) -> () ``` Description: Destroys the raw transaction bytes alloc...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In dash-spv-ffi/FFI_API.md around lines 695 to 700, the safety text incorrectly
states that raw_tx is "allocated by the caller" while the function destroys
library-owned memory; update the allocation attribution to state that raw_tx was
"allocated by the library and returned to the caller" (or remove the allocation
statement if the pointer may originate from either side), and ensure the Safety
section consistently reflects that the caller must not use the pointer after
calling this destroy function and that it should only be called once per
allocation.
| let client = &*client; | ||
| let inner = client.inner.lock().unwrap(); | ||
|
|
||
| let client = &(*client); | ||
| let inner = client.inner.clone(); | ||
| if let Some(ref spv_client) = *inner { | ||
| // Clone the Arc to the wallet manager | ||
| let wallet_arc = spv_client.wallet().clone(); | ||
| let runtime = client.runtime.clone(); | ||
|
|
||
| let result: Result<dash_spv::types::MempoolBalance, dash_spv::SpvError> = | ||
| client.runtime.block_on(async { | ||
| let guard = inner.lock().unwrap(); | ||
| if let Some(ref spv_client) = *guard { | ||
| spv_client.get_mempool_balance(&addr).await | ||
| } else { | ||
| Err(dash_spv::SpvError::Storage(dash_spv::StorageError::NotFound( | ||
| "Client not initialized".to_string(), | ||
| ))) | ||
| } | ||
| }); | ||
| // Create the FFIWalletManager with the cloned Arc | ||
| let manager = FFIWalletManager::from_arc(wallet_arc, runtime); | ||
|
|
||
| match result { | ||
| Ok(mempool_balance) => { | ||
| // Convert MempoolBalance to FFIBalance | ||
| let balance = FFIBalance { | ||
| confirmed: 0, // No confirmed balance in mempool | ||
| unconfirmed: mempool_balance.pending.to_sat() | ||
| + mempool_balance.pending_instant.to_sat(), | ||
| immature: 0, | ||
| total: mempool_balance.pending.to_sat() + mempool_balance.pending_instant.to_sat(), | ||
| }; | ||
| Box::into_raw(Box::new(balance)) | ||
| } | ||
| Err(e) => { | ||
| set_last_error(&e.to_string()); | ||
| std::ptr::null_mut() | ||
| } | ||
| Box::into_raw(Box::new(manager)) as *mut c_void | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panicking on poisoned mutex; return NULL and set_last_error instead
inner.lock().unwrap() can abort the host app across FFI. Handle lock errors gracefully.
- let inner = client.inner.lock().unwrap();
+ let inner = match client.inner.lock() {
+ Ok(g) => g,
+ Err(_) => {
+ set_last_error("Client mutex poisoned");
+ return std::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.
| let client = &*client; | |
| let inner = client.inner.lock().unwrap(); | |
| let client = &(*client); | |
| let inner = client.inner.clone(); | |
| if let Some(ref spv_client) = *inner { | |
| // Clone the Arc to the wallet manager | |
| let wallet_arc = spv_client.wallet().clone(); | |
| let runtime = client.runtime.clone(); | |
| let result: Result<dash_spv::types::MempoolBalance, dash_spv::SpvError> = | |
| client.runtime.block_on(async { | |
| let guard = inner.lock().unwrap(); | |
| if let Some(ref spv_client) = *guard { | |
| spv_client.get_mempool_balance(&addr).await | |
| } else { | |
| Err(dash_spv::SpvError::Storage(dash_spv::StorageError::NotFound( | |
| "Client not initialized".to_string(), | |
| ))) | |
| } | |
| }); | |
| // Create the FFIWalletManager with the cloned Arc | |
| let manager = FFIWalletManager::from_arc(wallet_arc, runtime); | |
| match result { | |
| Ok(mempool_balance) => { | |
| // Convert MempoolBalance to FFIBalance | |
| let balance = FFIBalance { | |
| confirmed: 0, // No confirmed balance in mempool | |
| unconfirmed: mempool_balance.pending.to_sat() | |
| + mempool_balance.pending_instant.to_sat(), | |
| immature: 0, | |
| total: mempool_balance.pending.to_sat() + mempool_balance.pending_instant.to_sat(), | |
| }; | |
| Box::into_raw(Box::new(balance)) | |
| } | |
| Err(e) => { | |
| set_last_error(&e.to_string()); | |
| std::ptr::null_mut() | |
| } | |
| Box::into_raw(Box::new(manager)) as *mut c_void | |
| } else { | |
| let client = &*client; | |
| // Avoid panicking on a poisoned mutex; report and return null instead | |
| let inner = match client.inner.lock() { | |
| Ok(g) => g, | |
| Err(_) => { | |
| set_last_error("Client mutex poisoned"); | |
| return std::ptr::null_mut(); | |
| } | |
| }; | |
| if let Some(ref spv_client) = *inner { | |
| // Clone the Arc to the wallet manager | |
| let wallet_arc = spv_client.wallet().clone(); | |
| let runtime = client.runtime.clone(); | |
| // Create the FFIWalletManager with the cloned Arc | |
| let manager = FFIWalletManager::from_arc(wallet_arc, runtime); | |
| Box::into_raw(Box::new(manager)) as *mut c_void | |
| } else { | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In dash-spv-ffi/src/client.rs around lines 1095 to 1107, the code calls
inner.lock().unwrap() which can panic if the mutex is poisoned; modify this to
handle the Result from lock() (e.g., match or map_err) so that on Err you call
set_last_error with a clear error string and return std::ptr::null_mut() instead
of panicking; only proceed to clone the wallet Arc, create the FFIWalletManager
and Box::into_raw on Ok(lock_guard), and ensure any error path returns NULL and
sets the last error for the FFI caller.
| echo "Or use the make command:" | ||
| echo " make update-ffi-docs" |
There was a problem hiding this comment.
Fix Makefile target name in guidance.
README uses make update-docs; this prints make update-ffi-docs. Align to avoid confusing contributors.
- echo " make update-ffi-docs"
+ echo " make update-docs"🤖 Prompt for AI Agents
In key-wallet-ffi/scripts/check_ffi_docs.sh around lines 26 to 27, the help text
prints the wrong Make target ("make update-ffi-docs") which conflicts with
README; update the echo to show the correct target "make update-docs" so the
guidance matches README (i.e., change the second echo to print " make
update-docs").
A pretty heavy refactoring in order to get spv working on ios.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores