feat: build out key-wallet ffi and improve ci #108
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces UniFFI-generated bindings with a manual C ABI: removes UniFFI scaffolding and generated artifacts, adds cbindgen header generation and tooling, implements explicit #[no_mangle] C-ABI modules (wallet, manager, keys, derivation, transaction, utxo, etc.), refactors wallet/account derivation and passphrase flows, and updates CI and build scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Native app (C)
participant FFI as key-wallet-ffi (C ABI)
participant KW as key-wallet (Rust)
participant KWM as key-wallet-manager (Rust)
rect rgba(232,244,255,0.9)
Note over App,FFI: Wallet creation & manager registration
App->>FFI: wallet_create_from_mnemonic(mnemonic, pass, network, options, error)
FFI->>KW: Wallet::from_mnemonic[_with_passphrase](..., options)
KW-->>FFI: FFIWallet* (opaque)
App->>FFI: wallet_manager_add_wallet_from_mnemonic(manager, mnemonic, pass, network, options, error)
FFI->>KWM: WalletManager::create_wallet_from_mnemonic(..., options)
KWM-->>FFI: managed wallet id / success
FFI-->>App: success / error
end
rect rgba(240,255,240,0.9)
Note over App,FFI: Address derivation & transaction checking
App->>FFI: managed_wallet_get_next_bip44_receive_address(managed, wallet, network, acct, error)
FFI->>KW: AddressPool / Account::derive_*/get_address_range
KW-->>FFI: char* address
FFI-->>App: address
App->>FFI: wallet_check_transaction(wallet, network, tx_bytes, tx_len, context, update_state, result_out, error)
FFI->>KW: ManagedWalletInfo::check_transaction(context)
KW-->>FFI: FFITransactionCheckResult
FFI-->>App: result_out
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Suggested reviewers
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: 53
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
169-176: Avoid constructing an unused ImmatureTransaction; don’t hardcode coinbase maturity; don’t fall back to a zero block hash.
- Unused construction:
let _immature_tx = ...allocates and clonestxwith no side effects. This adds cost and may trip clippy lints later.- Hardcoded network param:
100violates the coding guideline “Do not hardcode network parameters.” Please source coinbase maturity from consensus/network params.- Zero hash fallback:
unwrap_or_else(BlockHash::all_zeros)silently replaces missing block hashes with an invalid sentinel, which can mislead downstream logic. Prefer requiring a real block hash (or makeImmatureTransactionacceptOption<BlockHash>and handle it).Minimal change to remove the dead allocation now (keep the TODO) while you wire up proper tracking:
- let _immature_tx = ImmatureTransaction::new( - tx.clone(), - height, - block_hash.unwrap_or_else(BlockHash::all_zeros), - timestamp.unwrap_or(0) as u64, - 100, // Standard coinbase maturity - true, // is_coinbase - ); + // TODO(immature-tracking): construct and store ImmatureTransaction + // using consensus coinbase maturity for `network` and a real block_hash.Follow-ups:
- Introduce a consensus/params accessor (e.g.,
coinbase_maturity_for(network)) and use it instead of a literal.- Either validate
block_hash.is_some()here or adaptImmatureTransactionto acceptOption<BlockHash>.Also applies to: 178-178
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (2)
748-753: Potential type mismatch: push_slice expects PushBytes; pubkey is passed rawElsewhere you convert to PushBytes before push_slice. Passing pubkey.serialize() directly may not match the typed API and could fail to compile or break invariants.
Apply this diff to be consistent and explicit:
- let pubkey = secp256k1::PublicKey::from_secret_key(&secp, key); - - let script_sig = Builder::new() + let pubkey = secp256k1::PublicKey::from_secret_key(&secp, key); + let pubkey_ser = pubkey.serialize(); + + let script_sig = Builder::new() .push_slice(<&PushBytes>::try_from(sig_bytes.as_slice()).map_err(|_| { BuilderError::SigningFailed("Invalid signature length".into()) })?) - .push_slice(pubkey.serialize()) + .push_slice(<&PushBytes>::try_from(pubkey_ser.as_slice()).map_err(|_| { + BuilderError::SigningFailed("Invalid public key length".into()) + })?) .into_script();
771-785: Ensuresign_transactionaligns UTXO keys with transaction input order
- Verified no actual call sites of
sign_transactionin the codebase (only commented examples ininitialization.rs).- The current implementation sorts
self.inputsper BIP-69 but does not reordertx.input, so keys may be applied to the wrong inputs iftx.inputisn’t pre-sorted.- Refactor to match each
tx.input.previous_outputwith the corresponding UTXO key inself.inputs, preserving the transaction’s input order./// Sign the transaction (legacy method for backward compatibility) /// Aligns provided UTXO keys with the exact order of `tx.input` by matching OutPoints. - pub fn sign_transaction(&self, tx: Transaction) -> Result<Transaction, BuilderError> { - // For backward compatibility, we sort the inputs according to BIP-69 before signing - let mut sorted_inputs = self.inputs.clone(); - sorted_inputs.sort_by(|a, b| { - let tx_hash_a = a.0.outpoint.txid.to_byte_array(); - let tx_hash_b = b.0.outpoint.txid.to_byte_array(); - - match tx_hash_a.cmp(&tx_hash_b) { - std::cmp::Ordering::Equal => a.0.outpoint.vout.cmp(&b.0.outpoint.vout), - other => other, - } - }); - - self.sign_transaction_with_sorted_inputs(tx, sorted_inputs) - } + pub fn sign_transaction(&self, tx: Transaction) -> Result<Transaction, BuilderError> { + // Build inputs vector in the same order as tx.input by matching OutPoints + let mut inputs_in_tx_order = Vec::with_capacity(tx.input.len()); + for input in &tx.input { + let prev = input.previous_output; + match self.inputs.iter().find(|(u, _)| u.outpoint == prev) { + Some((u, k)) => inputs_in_tx_order.push((u.clone(), k.clone())), + None => { + return Err(BuilderError::SigningFailed(format!( + "No matching UTXO for input {}:{}", + prev.txid, prev.vout + ))) + } + } + } + self.sign_transaction_with_sorted_inputs(tx, inputs_in_tx_order) + }key-wallet-manager/src/wallet_manager/mod.rs (1)
183-196: Do not hardcode a known mnemonic in library codeUsing the fixed BIP39 test vector in create_wallet is a security footgun (any consumer calling this would create the same wallet deterministically). Production code should never embed keys or mnemonics.
Consider one of:
- Gate this function behind #[cfg(test)] or a dedicated feature flag (e.g., insecure-test-only).
- Generate a random mnemonic (wallet::new_random) under a proper feature/entropy source, or require the caller to provide a mnemonic.
I can propose a gated refactor if you confirm the intended usage of this method.
| key-wallet-manager = { path = "../key-wallet-manager", features = ["std"] } | ||
| dashcore = { path = "../dash", features = ["std"] } | ||
| dash-network = { path = "../dash-network" } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dependency switch aligns with UniFFI removal
Switching to key-wallet-manager, dashcore, and dash-network path deps is consistent with the new C-ABI FFI approach. Make sure there are no lingering UniFFI artifacts.
Run this to verify no uniffi references remain:
🏁 Script executed:
#!/bin/bash
# Search for lingering UniFFI references across the repo
rg -nP -C2 '\buniffi\b|UniFFI'Length of output: 13368
Lingering UniFFI artifacts detected – please remove across project
A repo-wide search still shows numerous UniFFI references and generated bindings. To fully complete the switch to the new C-ABI FFI approach, please remove or replace all of the following:
• key-wallet-ffi/build-ios.sh
– remove --features uniffi/cli and uniffi-bindgen commands
• key-wallet-ffi/README.md
– drop “UniFFI bindings” sections and related generate commands
• dash-network-ffi/
– build.rs (uniffi::generate_scaffolding)
– README.md (UniFFI-based features and examples)
– src/lib.rs (uniffi::include_scaffolding!)
– uniffi-bindgen.rs binary
– Cargo.toml (uniffi dependencies)
– autogenerated .h/.swift FFI files
• swift-dash-core-sdk/INTEGRATION_NOTES.md
– references to key-wallet-ffi via UniFFI
• swift-dash-core-sdk/Sources/KeyWalletFFI/*
– autogenerated FFI headers and Swift files containing “UniFFI”
• CLAUDE.md
– mentions of dash-network-ffi/ using UniFFI
Make sure all build scripts, docs, generated code, and Cargo.toml entries no longer reference UniFFI or uniffi-bindgen.
🤖 Prompt for AI Agents
In key-wallet-ffi/Cargo.toml around lines 21-23 and across the repo, remove all
UniFFI/uniffi-bindgen references and generated artifacts: delete uniffi
dependencies and any features like uniffi/cli from Cargo.toml entries; remove
uniffi-bindgen.rs, uniffi::generate_scaffolding and uniffi::include_scaffolding!
usage in build.rs and lib.rs; eliminate generated .h/.swift FFI files and the
swift KeyWalletFFI sources; update/remove UniFFI sections and generate commands
in README.md and INTEGRATION_NOTES.md, strip uniffi flags from build-ios.sh, and
delete mentions in CLAUDE.md so no build scripts, docs, or manifests reference
UniFFI anymore.
| // FFI type definitions (normally these would be in a header file) | ||
| typedef enum { | ||
| Dash = 0, | ||
| Testnet = 1, | ||
| Regtest = 2, | ||
| Devnet = 3 | ||
| } FFINetwork; | ||
|
|
||
| typedef enum { | ||
| Mempool = 0, | ||
| InBlock = 1, | ||
| InChainLockedBlock = 2 | ||
| } FFITransactionContext; | ||
|
|
||
| typedef struct { | ||
| bool is_relevant; | ||
| uint64_t total_received; | ||
| uint64_t total_sent; | ||
| uint32_t affected_accounts_count; | ||
| } FFITransactionCheckResult; | ||
|
|
||
| typedef struct { | ||
| int32_t code; | ||
| char* message; | ||
| } FFIError; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not redefine FFI types in the example; include the public header instead
Manually duplicating FFI type definitions risks divergence from the library ABI and breaks on future changes. Include the installed header to guarantee consistency.
Apply:
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include <string.h>
+#include <inttypes.h>
-// FFI type definitions (normally these would be in a header file)
-typedef enum {
- Dash = 0,
- Testnet = 1,
- Regtest = 2,
- Devnet = 3
-} FFINetwork;
-
-typedef enum {
- Mempool = 0,
- InBlock = 1,
- InChainLockedBlock = 2
-} FFITransactionContext;
-
-typedef struct {
- bool is_relevant;
- uint64_t total_received;
- uint64_t total_sent;
- uint32_t affected_accounts_count;
-} FFITransactionCheckResult;
-
-typedef struct {
- int32_t code;
- char* message;
-} FFIError;
+// Prefer including the SDK's public header. Adjust the path as needed when building standalone.
+#include "../include/key_wallet_ffi.h"Also applies to: 3-7
🤖 Prompt for AI Agents
In key-wallet-ffi/examples/check_transaction.c around lines 8 to 33 (and also
applicable to lines 3-7), the example incorrectly redefines FFI enums and
structs; remove these duplicated FFI type definitions and instead add an
#include for the public installed header that declares FFINetwork,
FFITransactionContext, FFITransactionCheckResult and FFIError; ensure the
example uses the header's types only (no local typedefs), update any include
path or build instruction so the compiler picks up the installed header, and
delete the redundant definitions to avoid ABI drift.
| if (success) { | ||
| if (result.is_relevant) { | ||
| printf("Transaction belongs to wallet!\n"); | ||
| printf(" Total received: %llu\n", (unsigned long long)result.total_received); | ||
| printf(" Total sent: %llu\n", (unsigned long long)result.total_sent); | ||
| printf(" Affected accounts: %u\n", result.affected_accounts_count); | ||
| } else { | ||
| printf("Transaction does not belong to wallet\n"); | ||
| } | ||
| } else { | ||
| printf("Failed to check transaction: %s\n", error.message); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Free error message buffers between calls to prevent leaks
FFIError.message is allocated on the Rust side (CString::into_raw). The example prints messages but never frees them, and also reuses the same FFIError across calls. This leaks memory and may overwrite pointers.
Search for the correct deallocation routine provided by the FFI (e.g., ffi_free_string, ffi_error_free, error_free_message):
Expected outcome:
- A function to free char* messages (e.g., ffi_free_string(char*)).
- If present, call it after printing any error message and before reusing the FFIError struct.
Also guard against NULL before printing:
- Only print error.message if not NULL; otherwise, print the error code.
Also applies to: 62-70, 109-121
🏁 Script executed:
#!/bin/bash
# Find available error/message free helpers
rg -nP --type=rs -C2 'extern\s+"C"\s+fn\s+.*free|extern\s+"C"\s+fn\s+.*error.*free|free_string|string_free|error_free' key-wallet-ffi/src
rg -nP --type=c -C2 'void\s+\w*free\w*\(' key-wallet-ffi/includeLength of output: 3541
🏁 Script executed:
#!/bin/bash
# Locate the FFIError struct to confirm the message field name and type
rg -nP --type=c 'struct\s+FFIError' key-wallet-ffi/include/key_wallet_ffi.h -C3Length of output: 243
Free error messages after printing to prevent leaks
The FFIError.message buffer must be released with error_message_free (or string_free) once you’ve printed it—and reset the pointer—otherwise every failing call leaks. Also guard against NULL before printing. Apply the same pattern in all example blocks (lines 62–70, 94–106, 109–121).
• File: key-wallet-ffi/examples/check_transaction.c
• Lines 94–106 (and similarly 62–70, 109–121):
- } else {
- printf("Failed to check transaction: %s\n", error.message);
- }
+ } else {
+ if (error.message) {
+ printf("Failed to check transaction: %s\n", error.message);
+ error_message_free(error.message);
+ error.message = NULL;
+ } else {
+ printf("Failed to check transaction: error code %d\n", error.code);
+ }
+ }• Ensure each example that prints error.message frees it immediately after use.
• Always check error.message != NULL before calling printf("%s").
🤖 Prompt for AI Agents
In key-wallet-ffi/examples/check_transaction.c around lines 62-70, 94-106, and
109-121, the code prints error.message without checking for NULL and without
freeing the allocated buffer, which leaks memory on failures; update each
error-handling block to (1) check error.message != NULL before passing it to
printf, (2) after printing call the appropriate free function
(error_message_free or string_free) to release the buffer, and (3) set
error.message = NULL after freeing to avoid dangling pointers.
key-wallet-ffi/src/account.rs
Outdated
| match wallet.inner().accounts.get(&network) { | ||
| Some(accounts) => { | ||
| FFIError::set_success(error); | ||
| let count = accounts.standard_bip44_accounts.len() | ||
| + accounts.standard_bip32_accounts.len() | ||
| + accounts.coinjoin_accounts.len() | ||
| + accounts.identity_registration.is_some() as usize | ||
| + accounts.identity_topup.len(); | ||
| count as c_uint | ||
| } | ||
| None => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t reach into private fields; use the accounts() and count() APIs
Accessing accounts.get(&network) and summing fields is brittle and may break with struct changes or private fields. The codebase exposes accounts(&self, Network) -> Option<&ManagedAccountCollection> and ManagedAccountCollection::count().
- match wallet.inner().accounts.get(&network) {
- Some(accounts) => {
- FFIError::set_success(error);
- let count = accounts.standard_bip44_accounts.len()
- + accounts.standard_bip32_accounts.len()
- + accounts.coinjoin_accounts.len()
- + accounts.identity_registration.is_some() as usize
- + accounts.identity_topup.len();
- count as c_uint
- }
+ match wallet.inner().accounts(network) {
+ Some(accounts) => {
+ FFIError::set_success(error);
+ accounts.count() as c_uint
+ }This reduces duplication and future-proofs the FFI against internal layout changes.
Please confirm wallet.inner().accounts(Network) is available from your imported traits, as shown in key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs.
I’ll verify that the accounts(&self, Network) and count() APIs exist in the expected modules.
🏁 Script executed:
#!/bin/bash
set -e
# 1. Locate wallet_info_interface.rs
file=$(fd wallet_info_interface.rs)
echo "wallet_info_interface.rs path: $file"
# 2. Show the definition of accounts(...) in that file
rg -nP 'fn\s+accounts' "$file" -C3
# 3. Locate ManagedAccountCollection source
mac_file=$(fd managed_account_collection.rs)
echo "managed_account_collection.rs path: $mac_file"
# 4. Show the definition of count() in that file
rg -nP 'fn\s+count' "$mac_file" -C3Length of output: 1718
Use the public accounts() and count() APIs instead of accessing private fields
Replace the brittle field-by-field summation with the maintained API to future-proof against layout changes.
Locations to update:
- File:
key-wallet-ffi/src/account.rs - Lines: 73–83
Suggested diff:
- match wallet.inner().accounts.get(&network) {
+ match wallet.inner().accounts(network) {
Some(accounts) => {
FFIError::set_success(error);
- let count = accounts.standard_bip44_accounts.len()
- + accounts.standard_bip32_accounts.len()
- + accounts.coinjoin_accounts.len()
- + accounts.identity_registration.is_some() as usize
- + accounts.identity_topup.len();
- count as c_uint
+ accounts.count() as c_uint
}
None => {Verified in
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs:
fn accounts(&self, network: Network) -> Option<&ManagedAccountCollection>key-wallet/src/account/managed_account_collection.rs:
pub fn count(&self) -> usize
📝 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.
| match wallet.inner().accounts.get(&network) { | |
| Some(accounts) => { | |
| FFIError::set_success(error); | |
| let count = accounts.standard_bip44_accounts.len() | |
| + accounts.standard_bip32_accounts.len() | |
| + accounts.coinjoin_accounts.len() | |
| + accounts.identity_registration.is_some() as usize | |
| + accounts.identity_topup.len(); | |
| count as c_uint | |
| } | |
| None => { | |
| match wallet.inner().accounts(network) { | |
| Some(accounts) => { | |
| FFIError::set_success(error); | |
| accounts.count() as c_uint | |
| } | |
| None => { |
🤖 Prompt for AI Agents
In key-wallet-ffi/src/account.rs around lines 73–83, the code manually sums
private collection fields to compute account count; replace that with the public
API by calling wallet.inner().accounts(network) and, when Some(collection), call
collection.count() to get the total, then set FFIError::set_success(error) and
return count as c_uint; keep the None branch unchanged.
| // For standard accounts, use the address pool to get the next unused address | ||
| if let ManagedAccountType::Standard { | ||
| external_addresses, | ||
| .. | ||
| } = &mut self.account_type | ||
| { | ||
| // Use the address pool's get_next_unused method which properly tracks addresses | ||
| let key_source = crate::account::address_pool::KeySource::Public(account_xpub.clone()); | ||
| external_addresses | ||
| .get_next_unused(&key_source) | ||
| .map_err(|_| "Failed to generate receive address") | ||
| } else { | ||
| Err("Cannot generate receive address for non-standard account type") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the crate’s Error/Result instead of &'static str.
Per guidelines, prefer proper error types (thiserror-based) over string errors. These methods should return crate::error::Result
and map underlying errors instead of fabricating strings.Proposed signature and error handling change:
- pub fn get_next_receive_address(
- &mut self,
- account_xpub: &ExtendedPubKey,
- ) -> Result<Address, &'static str> {
+ pub fn get_next_receive_address(
+ &mut self,
+ account_xpub: &ExtendedPubKey,
+ ) -> crate::error::Result<Address> {
@@
- external_addresses
- .get_next_unused(&key_source)
- .map_err(|_| "Failed to generate receive address")
+ external_addresses.get_next_unused(&key_source)
} else {
- Err("Cannot generate receive address for non-standard account type")
+ Err(crate::error::Error::InvalidParameter(
+ "Cannot generate receive address for non-standard account type".into(),
+ ))
}
}Add the missing import near the top of the file:
use crate::error::{Error, Result};🤖 Prompt for AI Agents
In key-wallet/src/account/managed_account.rs around lines 239–253, the function
currently returns string errors; change it to use crate::error::Result<Address>
by adding use crate::error::{Error, Result} near the top, replace the
string-mapped error from external_addresses.get_next_unused(...).map_err(|_|
"Failed to generate receive address") with mapping the underlying error into
your crate Error (e.g. .map_err(Error::from) or map to a new
Error::AddressGeneration variant), and replace the hardcoded Err("Cannot
generate receive address for non-standard account type") with an appropriate
crate Error variant (add one if needed) so the function consistently returns
Result<Address> using your error types.
| pub fn get_next_change_address( | ||
| &mut self, | ||
| account_xpub: &ExtendedPubKey, | ||
| network: Network, | ||
| ) -> Result<Address, &'static str> { | ||
| // Get the next change address index | ||
| let index = self | ||
| .get_next_change_address_index() | ||
| .ok_or("Cannot generate change address for this account type")?; | ||
|
|
||
| // Derive the address from the account's xpub | ||
| let secp = Secp256k1::new(); | ||
|
|
||
| // Derive m/1/index (change branch) | ||
| let change_xpub = account_xpub | ||
| .derive_pub(&secp, &[crate::ChildNumber::from_normal_idx(1).unwrap()]) | ||
| .map_err(|_| "Failed to derive change branch")?; | ||
|
|
||
| let address_xpub = change_xpub | ||
| .derive_pub(&secp, &[crate::ChildNumber::from_normal_idx(index).unwrap()]) | ||
| .map_err(|_| "Failed to derive address")?; | ||
|
|
||
| // Convert to public key and create address | ||
| let pubkey = PublicKey::from_slice(&address_xpub.public_key.serialize()) | ||
| .map_err(|_| "Failed to create public key")?; | ||
|
|
||
| let address = Address::p2pkh(&pubkey, network); | ||
|
|
||
| Ok(address) | ||
| // For standard accounts, use the address pool to get the next unused address | ||
| if let ManagedAccountType::Standard { | ||
| internal_addresses, | ||
| .. | ||
| } = &mut self.account_type | ||
| { | ||
| // Use the address pool's get_next_unused method which properly tracks addresses | ||
| let key_source = crate::account::address_pool::KeySource::Public(account_xpub.clone()); | ||
| internal_addresses | ||
| .get_next_unused(&key_source) | ||
| .map_err(|_| "Failed to generate change address") | ||
| } else { | ||
| Err("Cannot generate change address for non-standard account type") | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Apply the same error-type refactor to change-address generation.
- pub fn get_next_change_address(
- &mut self,
- account_xpub: &ExtendedPubKey,
- ) -> Result<Address, &'static str> {
+ pub fn get_next_change_address(
+ &mut self,
+ account_xpub: &ExtendedPubKey,
+ ) -> crate::error::Result<Address> {
@@
- internal_addresses
- .get_next_unused(&key_source)
- .map_err(|_| "Failed to generate change address")
+ internal_addresses.get_next_unused(&key_source)
} else {
- Err("Cannot generate change address for non-standard account type")
+ Err(crate::error::Error::InvalidParameter(
+ "Cannot generate change address for non-standard account type".into(),
+ ))
}
}This aligns with crate-wide error handling and avoids stringly-typed failures.
To find call sites that may need signature updates:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites of get_next_receive_address:"
rg -n -C2 --type=rs '\.get_next_receive_address\('
echo
echo "Call sites of get_next_change_address:"
rg -n -C2 --type=rs '\.get_next_change_address\('Length of output: 188
I’ll re-run with the correct Rust file type and broader pattern to locate any call sites:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites of get_next_receive_address:"
rg -nC2 --type rust '\.get_next_receive_address\('
echo
echo "Call sites of get_next_change_address:"
rg -nC2 --type rust '\.get_next_change_address\('Length of output: 11847
Refactor change‐address generation to use crate::error::Result
Apply the same error‐type pattern you used for receiving addresses to change‐address generation:
• key-wallet/src/account/managed_account.rs
- Change the signature from
Result<Address, &'static str>tocrate::error::Result<Address>. - Remove the bespoke
map_errand return the innerResultdirectly.
• key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
- Your existing
.get_next_change_address(...).map_err(|e| TransactionError::ChangeAddressGeneration(format!(…{}, e)))
will continue to work as long ascrate::error::ErrorimplementsDisplay.
• key-wallet/src/watch_only.rs (commented examples)
- If you ever enable those examples/tests, update the
.unwrap()calls on change‐address generation to handle the new error type or useexpect.
• key-wallet-manager/src/wallet_manager/mod.rs
- All
match managed_account.get_next_change_address(…) { Err(_) => … }sites already ignore the error payload and need no changes.
--- a/key-wallet/src/account/managed_account.rs
+++ b/key-wallet/src/account/managed_account.rs
pub fn get_next_change_address(
&mut self,
account_xpub: &ExtendedPubKey,
- ) -> Result<Address, &'static str> {
+ ) -> crate::error::Result<Address> {
if let ManagedAccountType::Standard {
internal_addresses,
..
} = &mut self.account_type
{
let key_source = crate::account::address_pool::KeySource::Public(account_xpub.clone());
- internal_addresses
- .get_next_unused(&key_source)
- .map_err(|_| "Failed to generate change address")
+ internal_addresses.get_next_unused(&key_source)
} else {
- Err("Cannot generate change address for non-standard account type")
+ Err(crate::error::Error::InvalidParameter(
+ "Cannot generate change address for non-standard account type".into(),
+ ))
}
}🤖 Prompt for AI Agents
In key-wallet/src/account/managed_account.rs around lines 257 to 275, change the
function signature from Result<Address, &'static str> to
crate::error::Result<Address>, remove the manual map_err and instead return the
inner Result from internal_addresses.get_next_unused(&key_source) directly
(i.e., propagate the crate::error::Result returned by get_next_unused), and keep
the non-standard-account branch returning Err(...) converted to the
crate::error::Error type (or using crate::error::Result::Err with an appropriate
crate::error::Error value) so the function consistently returns
crate::error::Result<Address>.
| /// Add a new account to a wallet that requires a passphrase | ||
| /// | ||
| /// This function only works with wallets created with a passphrase (MnemonicWithPassphrase type). | ||
| /// It will fail if called on other wallet types. | ||
| /// | ||
| /// # Arguments | ||
| /// * `account_type` - The type of account to create | ||
| /// * `network` - The network for the account | ||
| /// * `passphrase` - The passphrase used when creating the wallet | ||
| /// | ||
| /// # Returns | ||
| /// A reference to the newly created account | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error if: | ||
| /// - The wallet is not a passphrase wallet | ||
| /// - The account already exists | ||
| /// - The passphrase is incorrect (will fail during derivation) | ||
| pub fn add_account_with_passphrase( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Doc is incorrect: “incorrect passphrase will fail during derivation” is not true.
BIP39’s to_seed() will deterministically produce a seed for any passphrase; derivation will succeed, producing a different key hierarchy. There’s no failure signal for a wrong passphrase here. The method cannot validate the passphrase without extra metadata or a KDF check.
Apply:
@@
- /// - The passphrase is incorrect (will fail during derivation)
+ /// - The passphrase is incorrect (note: BIP39 derivation does not fail for a “wrong”
+ /// passphrase; it produces a different key hierarchy. This method cannot detect an
+ /// incorrect passphrase; callers must ensure they pass the correct one.)📝 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.
| /// Add a new account to a wallet that requires a passphrase | |
| /// | |
| /// This function only works with wallets created with a passphrase (MnemonicWithPassphrase type). | |
| /// It will fail if called on other wallet types. | |
| /// | |
| /// # Arguments | |
| /// * `account_type` - The type of account to create | |
| /// * `network` - The network for the account | |
| /// * `passphrase` - The passphrase used when creating the wallet | |
| /// | |
| /// # Returns | |
| /// A reference to the newly created account | |
| /// | |
| /// # Errors | |
| /// Returns an error if: | |
| /// - The wallet is not a passphrase wallet | |
| /// - The account already exists | |
| /// - The passphrase is incorrect (will fail during derivation) | |
| pub fn add_account_with_passphrase( | |
| /// # Errors | |
| /// Returns an error if: | |
| /// - The wallet is not a passphrase wallet | |
| /// - The account already exists | |
| /// - The passphrase is incorrect (note: BIP39 derivation does not fail for a “wrong” | |
| /// passphrase; it produces a different key hierarchy. This method cannot detect an | |
| /// incorrect passphrase; callers must ensure they pass the correct one.) | |
| pub fn add_account_with_passphrase( |
| fn test_utxo(value: u64, confirmed: bool) -> Utxo { | ||
| let outpoint = OutPoint { | ||
| txid: Txid::from_raw_hash(sha256d::Hash::from_slice(&[1u8; 32]).unwrap()), | ||
| vout: 0, | ||
| }; | ||
|
|
||
| let txout = TxOut { | ||
| value, | ||
| script_pubkey: ScriptBuf::new(), | ||
| }; | ||
|
|
||
| let address = Address::p2pkh( | ||
| &dashcore::PublicKey::from_slice(&[ | ||
| 0x02, 0x50, 0x86, 0x3a, 0xd6, 0x4a, 0x87, 0xae, 0x8a, 0x2f, 0xe8, 0x3c, 0x1a, 0xf1, | ||
| 0xa8, 0x40, 0x3c, 0xb5, 0x3f, 0x53, 0xe4, 0x86, 0xd8, 0x51, 0x1d, 0xad, 0x8a, 0x04, | ||
| 0x88, 0x7e, 0x5b, 0x23, 0x52, | ||
| ]) | ||
| .unwrap(), | ||
| Network::Testnet, | ||
| ); | ||
|
|
||
| let mut utxo = Utxo::new(outpoint, txout, address, 100, false); | ||
| utxo.is_confirmed = confirmed; | ||
| utxo | ||
| } |
There was a problem hiding this comment.
Fix duplicate OutPoint and use a realistic script_pubkey
- All UTXOs currently share the same OutPoint (constant txid and vout=0). That’s invalid and can make the selector behave unpredictably (dedup, double-count, or reject duplicates).
- TxOut uses an empty script, which can distort fee/weight estimates if any logic inspects script types.
Refactor the helper to:
- Use address.script_pubkey() for a realistic P2PKH output.
- Generate a deterministic, unique txid per UTXO (based on value here, which is unique in this test set).
Apply this diff:
fn test_utxo(value: u64, confirmed: bool) -> Utxo {
- let outpoint = OutPoint {
- txid: Txid::from_raw_hash(sha256d::Hash::from_slice(&[1u8; 32]).unwrap()),
- vout: 0,
- };
-
- let txout = TxOut {
- value,
- script_pubkey: ScriptBuf::new(),
- };
-
- let address = Address::p2pkh(
+ // Derive a deterministic Testnet P2PKH address for the output.
+ let address = Address::p2pkh(
&dashcore::PublicKey::from_slice(&[
0x02, 0x50, 0x86, 0x3a, 0xd6, 0x4a, 0x87, 0xae, 0x8a, 0x2f, 0xe8, 0x3c, 0x1a, 0xf1,
0xa8, 0x40, 0x3c, 0xb5, 0x3f, 0x53, 0xe4, 0x86, 0xd8, 0x51, 0x1d, 0xad, 0x8a, 0x04,
0x88, 0x7e, 0x5b, 0x23, 0x52,
])
.unwrap(),
Network::Testnet,
);
+ // Use the address' script_pubkey to produce a realistic output.
+ let txout = TxOut {
+ value,
+ script_pubkey: address.script_pubkey(),
+ };
+ // Ensure each OutPoint is unique across test UTXOs.
+ let outpoint = OutPoint {
+ txid: Txid::from_raw_hash(sha256d::Hash::hash(&value.to_le_bytes())),
+ vout: 0,
+ };
+
let mut utxo = Utxo::new(outpoint, txout, address, 100, false);
utxo.is_confirmed = confirmed;
utxo
}📝 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.
| fn test_utxo(value: u64, confirmed: bool) -> Utxo { | |
| let outpoint = OutPoint { | |
| txid: Txid::from_raw_hash(sha256d::Hash::from_slice(&[1u8; 32]).unwrap()), | |
| vout: 0, | |
| }; | |
| let txout = TxOut { | |
| value, | |
| script_pubkey: ScriptBuf::new(), | |
| }; | |
| let address = Address::p2pkh( | |
| &dashcore::PublicKey::from_slice(&[ | |
| 0x02, 0x50, 0x86, 0x3a, 0xd6, 0x4a, 0x87, 0xae, 0x8a, 0x2f, 0xe8, 0x3c, 0x1a, 0xf1, | |
| 0xa8, 0x40, 0x3c, 0xb5, 0x3f, 0x53, 0xe4, 0x86, 0xd8, 0x51, 0x1d, 0xad, 0x8a, 0x04, | |
| 0x88, 0x7e, 0x5b, 0x23, 0x52, | |
| ]) | |
| .unwrap(), | |
| Network::Testnet, | |
| ); | |
| let mut utxo = Utxo::new(outpoint, txout, address, 100, false); | |
| utxo.is_confirmed = confirmed; | |
| utxo | |
| } | |
| fn test_utxo(value: u64, confirmed: bool) -> Utxo { | |
| // Derive a deterministic Testnet P2PKH address for the output. | |
| let address = Address::p2pkh( | |
| &dashcore::PublicKey::from_slice(&[ | |
| 0x02, 0x50, 0x86, 0x3a, 0xd6, 0x4a, 0x87, 0xae, 0x8a, 0x2f, 0xe8, 0x3c, 0x1a, 0xf1, | |
| 0xa8, 0x40, 0x3c, 0xb5, 0x3f, 0x53, 0xe4, 0x86, 0xd8, 0x51, 0x1d, 0xad, 0x8a, 0x04, | |
| 0x88, 0x7e, 0x5b, 0x23, 0x52, | |
| ]) | |
| .unwrap(), | |
| Network::Testnet, | |
| ); | |
| // Use the address' script_pubkey to produce a realistic output. | |
| let txout = TxOut { | |
| value, | |
| script_pubkey: address.script_pubkey(), | |
| }; | |
| // Ensure each OutPoint is unique across test UTXOs. | |
| let outpoint = OutPoint { | |
| txid: Txid::from_raw_hash(sha256d::Hash::hash(&value.to_le_bytes())), | |
| vout: 0, | |
| }; | |
| let mut utxo = Utxo::new(outpoint, txout, address, 100, false); | |
| utxo.is_confirmed = confirmed; | |
| utxo | |
| } |
🤖 Prompt for AI Agents
In key-wallet/tests/test_optimal_consolidation.rs around lines 11 to 35, the
helper creates UTXOs that all share the same OutPoint and an empty
script_pubkey; update it to produce unique, deterministic OutPoint.txid per UTXO
(e.g., derive the txid from the value via a small deterministic hash) and keep
vout as needed (vout=0 is fine when txid differs), and set TxOut.script_pubkey
to address.script_pubkey() so the output reflects a realistic P2PKH script;
ensure the new txid generation always yields a valid 32-byte array for
sha256d::Hash::from_slice and preserve the confirmed parameter behavior.
| char* wallet_derive_receive_address( | ||
| FFIWallet* wallet, | ||
| FFINetwork network, | ||
| uint32_t account_index, | ||
| FFIError* error | ||
| ); | ||
|
|
||
| /** | ||
| * Derive a new change address | ||
| * | ||
| * @param wallet The wallet handle | ||
| * @param network The network type | ||
| * @param account_index The account index | ||
| * @param error Pointer to error structure | ||
| * @return Address string (must be freed) or NULL on error | ||
| */ | ||
| char* wallet_derive_change_address( | ||
| FFIWallet* wallet, | ||
| FFINetwork network, | ||
| uint32_t account_index, | ||
| FFIError* error | ||
| ); |
There was a problem hiding this comment.
Address derivation prototypes are missing address_index and const-correctness; update to match Rust.
Rust requires address_index and does not mutate the wallet for derivation. The current C prototypes will not link or will mislead users.
Proposed fix:
-char* wallet_derive_receive_address(
- FFIWallet* wallet,
- FFINetwork network,
- uint32_t account_index,
- FFIError* error
-);
+char* wallet_derive_receive_address(
+ const FFIWallet* wallet,
+ FFINetwork network,
+ uint32_t account_index,
+ uint32_t address_index,
+ FFIError* error
+);
-char* wallet_derive_change_address(
- FFIWallet* wallet,
- FFINetwork network,
- uint32_t account_index,
- FFIError* error
-);
+char* wallet_derive_change_address(
+ const FFIWallet* wallet,
+ FFINetwork network,
+ uint32_t account_index,
+ uint32_t address_index,
+ FFIError* error
+);📝 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.
| char* wallet_derive_receive_address( | |
| FFIWallet* wallet, | |
| FFINetwork network, | |
| uint32_t account_index, | |
| FFIError* error | |
| ); | |
| /** | |
| * Derive a new change address | |
| * | |
| * @param wallet The wallet handle | |
| * @param network The network type | |
| * @param account_index The account index | |
| * @param error Pointer to error structure | |
| * @return Address string (must be freed) or NULL on error | |
| */ | |
| char* wallet_derive_change_address( | |
| FFIWallet* wallet, | |
| FFINetwork network, | |
| uint32_t account_index, | |
| FFIError* error | |
| ); | |
| char* wallet_derive_receive_address( | |
| const FFIWallet* wallet, | |
| FFINetwork network, | |
| uint32_t account_index, | |
| uint32_t address_index, | |
| FFIError* error | |
| ); | |
| char* wallet_derive_change_address( | |
| const FFIWallet* wallet, | |
| FFINetwork network, | |
| uint32_t account_index, | |
| uint32_t address_index, | |
| FFIError* error | |
| ); |
🤖 Prompt for AI Agents
In key-wallet-ffi/include/key_wallet_ffi.h around lines 283 to 304, the two
address-derivation prototypes are missing the address_index parameter and are
not const-correct; update both function declarations to accept a uint32_t
address_index and change the wallet parameter to a const FFIWallet* (since
derivation does not mutate the wallet). Also update the accompanying comment
lines/param docs to include address_index so the header matches the Rust
signatures.
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 (5)
dash-network-ffi/src/lib.rs (4)
10-18: Mark the FFI enum with repr(C) and fixed discriminants.Without repr(C), the enum layout is not ABI-stable. Assign explicit values to avoid accidental reordering breaking C clients.
Apply:
-// Re-export Network enum for FFI -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +// Re-export Network enum for FFI +#[repr(C)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Network { - Dash, - Testnet, - Devnet, - Regtest, + Dash = 0, + Testnet = 1, + Devnet = 2, + Regtest = 3, }
30-40: Avoid wildcard + panic in From; remove the panicking catch-all.Panicking in a From impl is hostile to FFI consumers and will trip clippy (fallible From). Be exhaustive and drop the wildcard.
Apply:
impl From<DashNetwork> for Network { fn from(n: DashNetwork) -> Self { - match n { - DashNetwork::Dash => Network::Dash, - DashNetwork::Testnet => Network::Testnet, - DashNetwork::Devnet => Network::Devnet, - DashNetwork::Regtest => Network::Regtest, - unknown => panic!("Unhandled Network variant {:?}", unknown), - } + match n { + DashNetwork::Dash => Network::Dash, + DashNetwork::Testnet => Network::Testnet, + DashNetwork::Devnet => Network::Devnet, + DashNetwork::Regtest => Network::Regtest, + } } }
54-84: NetworkInfo is not FFI-safe; expose functions or an opaque handle.Struct with Rust String-returning methods isn’t directly consumable from C. Either:
- Expose pure functions operating on the repr(C) Network enum, or
- Wrap NetworkInfo behind an opaque pointer with create/destroy and function-style accessors.
At minimum, avoid returning String across FFI; use CString::into_raw and a corresponding free.
Example functional API (additions):
use std::ffi::CString; use std::os::raw::c_char; #[no_mangle] pub extern "C" fn dash_network_magic(net: Network) -> u32 { let info = NetworkInfo::new(net); info.magic() } #[no_mangle] pub extern "C" fn dash_network_to_string(net: Network) -> *mut c_char { let s = NetworkInfo::new(net).to_string(); CString::new(s).unwrap().into_raw() } #[no_mangle] pub extern "C" fn dash_network_string_free(s: *mut c_char) { if !s.is_null() { unsafe { drop(CString::from_raw(s)); } } } #[no_mangle] pub extern "C" fn dash_network_from_magic(magic: u32, out: *mut Network) -> bool { match NetworkInfo::from_magic(magic) { Ok(info) => { let ffi_net: Network = info.network.into(); unsafe { *out = ffi_net }; true } Err(_) => false, } }If you prefer opaque handles:
#[repr(C)] pub struct FFINetworkInfo(NetworkInfo); #[no_mangle] pub extern "C" fn dash_network_info_new(net: Network) -> *mut FFINetworkInfo { /* ... */ } #[no_mangle] pub extern "C" fn dash_network_info_free(ptr: *mut FFINetworkInfo) { /* ... */ }Also consider a small C-side error enum for from_magic instead of bool.
61-67: Result-based API isn’t C-friendly; map to error codes.NetworkInfo::from_magic returns Result<Self, NetworkError>. Expose an extern "C" wrapper returning a status code or bool with out-params, not a Rust Result.
See the dash_network_from_magic(...) wrapper in my previous comment.
dash-network-ffi/README.md (1)
3-15: Docs say “C-compatible FFI” but the crate doesn’t export C symbols yet. Align docs or export the symbols.Currently lib.rs exposes Rust types/methods only; there’s no #[no_mangle] extern "C" surface nor a generated header. Either adjust docs to “Rust API” for now or export the C ABI and document header generation (e.g., via cbindgen in build.rs).
Suggested additions to “Building”:
### Header generation Enable the `gen-headers` feature to emit `include/dash_network_ffi.h` via cbindgen:cargo build --features gen-headers --release
Make sure to run `./swift-dash-core-sdk/sync-headers.sh` after regenerating headers to keep the Swift SDK in sync.
♻️ Duplicate comments (14)
key-wallet/src/account/address_pool.rs (1)
203-218: Bug: Address derivation incorrectly ignores base_path and hardcodes branch logic.This implementation forces a BIP44-style [branch, index] derivation pattern regardless of the pool type. For non-BIP44 account types like CoinJoin, Identity*, or Provider* pools, this injects an unintended extra derivation level and produces incorrect addresses.
The
base_pathparameter exists to handle different pool types - callers already include the appropriate branch (0/1) for BIP44 accounts in the base_path. This code should derive atbase_path + indexonly.Apply this fix:
- // For derivation, we only need the relative path from where the key_source is - // The key_source xpub is at account level (e.g., m/44'/1'/0') - // We need to derive the receive/change branch and then the index - // So the relative path should be [0, index] for external or [1, index] for internal - let branch_num = if self.is_internal { - 1 - } else { - 0 - }; - let relative_path = DerivationPath::from(vec![ - ChildNumber::from_normal_idx(branch_num).map_err(Error::Bip32)?, - ChildNumber::from_normal_idx(index).map_err(Error::Bip32)?, - ]); - - // Derive the key using the relative path + // Derive at account-relative path: base_path (including branch, if any) + index + let mut relative_path = self.base_path.clone(); + relative_path.push(ChildNumber::from_normal_idx(index).map_err(Error::Bip32)?); + + // Derive the key using the account-relative path let pubkey = key_source.derive_at_path(&relative_path)?;key-wallet-ffi/src/address_tests.rs (2)
17-30: Fix dangling pointer in test helperThe helper returns a pointer to a stack-allocated
FFIErrorthat is dropped when the function exits, leading to use-after-free in all test functions.unsafe fn create_test_wallet() -> (*mut crate::types::FFIWallet, *mut FFIError) { - let mut error = FFIError::success(); - let error_ptr = &mut error as *mut FFIError; + // Heap-allocate the error to keep the pointer valid after return + let error_ptr: *mut FFIError = Box::into_raw(Box::new(FFIError::success())); let seed = [0x01u8; 64]; let wallet = wallet::wallet_create_from_seed( seed.as_ptr(), seed.len(), FFINetwork::Testnet, error_ptr, ); (wallet, error_ptr) }Additionally, each test should free the error pointer after use:
unsafe { let _ = Box::from_raw(error_ptr); }
323-338: Fix symmetric allocation/deallocation inaddress_array_freeThe test correctly allocates memory as a boxed slice, but the actual
address_array_freeimplementation inkey-wallet-ffi/src/address.rshas allocation/deallocation issues.File:
key-wallet-ffi/src/address.rsat lines ~426–434
The implementation uses:let _ = Box::from_raw(std::slice::from_raw_parts_mut(addresses, count));This is incorrect and will cause undefined behavior because:
- The original allocation was via
Vec::as_mut_ptr+forget(capacity may be > count)- Reconstructing a
Box<[T]>with only the count mismatches the allocation layoutThe allocator functions should convert to boxed slices before leaking:
// In wallet_get_all_addresses and similar functions: - let ptr = addresses.as_mut_ptr(); - std::mem::forget(addresses); - *addresses_out = ptr; + let boxed: Box<[*mut c_char]> = addresses.into_boxed_slice(); + *count_out = boxed.len(); + if *count_out == 0 { + *addresses_out = std::ptr::null_mut(); + } else { + let ptr = boxed.as_mut_ptr(); + std::mem::forget(boxed); + *addresses_out = ptr; + }And
address_array_freeshould reconstruct the exact boxed slice:pub extern "C" fn address_array_free(addresses: *mut *mut c_char, count: usize) { if addresses.is_null() { return; } unsafe { let slice_ptr: *mut [*mut c_char] = std::ptr::slice_from_raw_parts_mut(addresses, count); let mut boxed: Box<[*mut c_char]> = Box::from_raw(slice_ptr); for addr in &mut *boxed { if !addr.is_null() { let _ = CString::from_raw(*addr); } } // boxed drops here, freeing the array allocation correctly } }key-wallet-ffi/src/balance.rs (2)
33-62:wallet_get_balancereturns placeholder data instead of actual balanceThe function always returns a default (zero) balance instead of aggregating actual balance from accounts. This is misleading as it returns success without providing real data.
Would you like me to help implement proper balance aggregation from all accounts? I can create an implementation that iterates through accounts and sums their balances.
73-107:wallet_get_account_balancedoesn't retrieve actual balanceThe function finds the account but returns a default balance instead of the account's actual balance. The comment indicates this needs implementation.
Would you like me to help implement the actual balance retrieval from the account? The account should have balance information that can be converted to FFIBalance.
key-wallet-ffi/src/account.rs (1)
164-192: Don’t reach into collection internals; use accounts(network) + count() APIDirectly summing fields couples the FFI to internal layout and can break with private/renamed fields. Prefer the public accessor and its count.
- match wallet.inner().accounts.get(&network) { - Some(accounts) => { - FFIError::set_success(error); - let count = accounts.standard_bip44_accounts.len() - + accounts.standard_bip32_accounts.len() - + accounts.coinjoin_accounts.len() - + accounts.identity_registration.is_some() as usize - + accounts.identity_topup.len(); - count as c_uint - } + match wallet.inner().accounts(network) { + Some(accounts) => { + FFIError::set_success(error); + accounts.count() as c_uint + } None => { FFIError::set_success(error); 0 } }Run to confirm the APIs exist and are in scope:
#!/bin/bash rg -nP -C3 'fn\s+accounts\s*\(\s*&self\s*,\s*Network\s*\)\s*->\s*Option<&ManagedAccountCollection>' key-wallet/src rg -nP -C3 'impl\s+ManagedAccountCollection\b.*\bfn\s+count\s*\(' key-wallet/srckey-wallet-ffi/src/utxo.rs (1)
35-45: Prefer Box::into_raw on the boxed slice to avoid mem::forget patternUsing as_mut_ptr() + mem::forget works but is easy to misuse. Converting the boxed slice with Box::into_raw makes ownership explicit and avoids relying on a separate forget.
- let script_len = script.len(); - let script_ptr = if script.is_empty() { + let script_len = script.len(); + let script_ptr = if script.is_empty() { ptr::null_mut() } else { - let mut script_box = script.into_boxed_slice(); - let ptr = script_box.as_mut_ptr(); - std::mem::forget(script_box); - ptr + // Note: Box::into_raw for [u8] yields a fat pointer (*mut [u8]). + // We store a thin pointer + length, and reconstruct with slice_from_raw_parts_mut. + let boxed: Box<[u8]> = script.into_boxed_slice(); + let ptr = boxed.as_ptr() as *mut u8; + let _fat: *mut [u8] = Box::into_raw(boxed); + ptr };Document the ownership: the array must be reclaimed by reconstructing a Box<[u8]> via slice_from_raw_parts_mut(self.script_pubkey, self.script_len) in free(). Current free() already matches this.
key-wallet-ffi/src/address.rs (1)
365-375: Fix UB/double-free: allocate/freeze address arrays as a boxed slice and free with matching Box reconstructionLeaking a Vec with as_mut_ptr() and freeing via Box::from_raw on a slice is undefined behavior: the allocation did not originate from Box<[T]>. Convert the Vec into a boxed slice before exposing it and reconstruct the same allocation in the free function. Also prefer std::ptr::slice_from_raw_parts_mut for unsized pointer creation.
@@ - if addresses.is_empty() { - *count_out = 0; - *addresses_out = ptr::null_mut(); - } else { - *count_out = addresses.len(); - // addresses is Vec<*mut c_char> - // We need to get a raw pointer to the array and leak it - let ptr = addresses.as_mut_ptr(); // This is *mut *mut c_char - std::mem::forget(addresses); - *addresses_out = ptr; - } + if addresses.is_empty() { + *count_out = 0; + *addresses_out = ptr::null_mut(); + } else { + *count_out = addresses.len(); + let mut boxed = addresses.into_boxed_slice(); // Box<[ *mut c_char ]> + let ptr = boxed.as_mut_ptr(); + std::mem::forget(boxed); // pass ownership to caller + *addresses_out = ptr; + } @@ - // Free the array itself - let _ = Box::from_raw(std::slice::from_raw_parts_mut(addresses, count)); + // Free the array itself (must match boxed slice allocation) + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(addresses, count));Update the function docs to instruct callers to free with address_array_free, not libc::free.
Also applies to: 421-423
key-wallet-ffi/src/wallet.rs (1)
361-377: Watch-only wallet bug: use Account::from_xpub so is_watch_only=trueConstructing the account with Account::new leaves is_watch_only=false, which is incorrect for a watch-only wallet built from an xpub. Use the dedicated constructor so downstream logic never attempts signing.
- // Create account 0 with the provided xpub - match Account::new(None, account_type, xpub, network_rust) { + // Create account 0 with the provided xpub (watch-only) + match Account::from_xpub(None, account_type, xpub, network_rust) {key-wallet-ffi/src/wallet_manager.rs (2)
242-256: UB and potential double-free: fix buffer ownership for wallet IDsYou allocate with Vec and free with Box::from_raw on a slice pointer, which is undefined behavior. Allocate a boxed slice at the producer and reconstruct the exact DST pointer in the free function.
Apply these diffs:
@@ - // Allocate memory for wallet IDs (32 bytes each) - let mut ids_buffer = Vec::with_capacity(count * 32); + // Allocate memory for wallet IDs (32 bytes each) + let mut ids_buffer = Vec::with_capacity(count * 32); for wallet_id in ids_guard.iter() { ids_buffer.extend_from_slice(wallet_id); } - let ids_ptr = ids_buffer.as_mut_ptr(); - std::mem::forget(ids_buffer); - - *wallet_ids_out = ids_ptr; + // Convert to Box<[u8]> so consumer can free with Box::from_raw on a DST pointer + let mut ids_box = ids_buffer.into_boxed_slice(); + let ptr = ids_box.as_mut_ptr(); + std::mem::forget(ids_box); // transfer ownership to caller + + *wallet_ids_out = ptr; *count_out = count; @@ -pub unsafe extern "C" fn wallet_manager_free_wallet_ids(wallet_ids: *mut u8, count: usize) { +pub unsafe extern "C" fn wallet_manager_free_wallet_ids(wallet_ids: *mut u8, count: usize) { if !wallet_ids.is_null() && count > 0 { - unsafe { - let _ = Box::from_raw(std::slice::from_raw_parts_mut(wallet_ids, count * 32)); - } + // Rebuild Box<[u8]> using the known byte length + let slice_ptr: *mut [u8] = std::ptr::slice_from_raw_parts_mut(wallet_ids, count * 32); + let _ = Box::from_raw(slice_ptr); } }Also applies to: 831-837
674-682: UB for address arrays: box the slice in producer, free via DST pointerSame ownership issue for arrays of C strings. Produce Box<[ *mut c_char ]> and free using Box::from_raw on a DST pointer.
@@ - } else { - *count_out = all_addresses.len(); - let ptr = all_addresses.as_mut_ptr(); - std::mem::forget(all_addresses); - *addresses_out = ptr; - } + } else { + *count_out = all_addresses.len(); + let mut boxed = all_addresses.into_boxed_slice(); + let ptr = boxed.as_mut_ptr(); + std::mem::forget(boxed); + *addresses_out = ptr; + } @@ - if !addresses.is_null() { - let slice = std::slice::from_raw_parts_mut(addresses, count); + if !addresses.is_null() { + 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)); + // Free the array itself (DST pointer) + let arr_ptr: *mut [*mut c_char] = std::ptr::slice_from_raw_parts_mut(addresses, count); + let _ = Box::from_raw(arr_ptr); }Also applies to: 849-859
key-wallet-ffi/src/derivation.rs (2)
1-2: Expose derivation FFI in the public C header and sync itThese exported functions and opaque types must be declared in key_wallet_ffi.h for consumers to call them. Please update the generated header (and then sync into Swift via ./sync-headers.sh if applicable).
I can supply a header diff snippet if needed.
Also applies to: 52-82, 84-137, 139-195, 197-250, 252-362, 364-423, 425-506, 508-656, 658-699, 700-814, 816-819
772-791: Avoid panic across FFI: replace unwrap() on hardened indices with error mappingChildNumber::from_hardened_idx(...).unwrap() can panic and abort the process when inputs are invalid. Map errors to FFIError and return null instead.
- FFIDerivationPathType::BlockchainIdentities => { - // Authentication: identity_index'/key_index' - DerivationPath::from(vec![ - ChildNumber::from_hardened_idx(identity_index).unwrap(), - ChildNumber::from_hardened_idx(key_index).unwrap(), - ]) - } + FFIDerivationPathType::BlockchainIdentities => { + // Authentication: identity_index'/key_index' + let cn1 = match ChildNumber::from_hardened_idx(identity_index) { + Ok(v) => v, + Err(e) => { + FFIError::set_error(error, FFIErrorCode::InvalidDerivationPath, format!("Invalid identity_index: {}", e)); + return ptr::null_mut(); + } + }; + let cn2 = match ChildNumber::from_hardened_idx(key_index) { + Ok(v) => v, + Err(e) => { + FFIError::set_error(error, FFIErrorCode::InvalidDerivationPath, format!("Invalid key_index: {}", e)); + return ptr::null_mut(); + } + }; + DerivationPath::from(vec![cn1, cn2]) + } @@ - FFIDerivationPathType::BlockchainIdentityCreditRegistrationFunding => { - // Registration: index' - DerivationPath::from(vec![ChildNumber::from_hardened_idx(identity_index).unwrap()]) - } + FFIDerivationPathType::BlockchainIdentityCreditRegistrationFunding => { + // Registration: index' + let cn = match ChildNumber::from_hardened_idx(identity_index) { + Ok(v) => v, + Err(e) => { + FFIError::set_error(error, FFIErrorCode::InvalidDerivationPath, format!("Invalid identity_index: {}", e)); + return ptr::null_mut(); + } + }; + DerivationPath::from(vec![cn]) + } @@ - FFIDerivationPathType::BlockchainIdentityCreditTopupFunding => { - // Top-up: identity_index'/topup_index' - DerivationPath::from(vec![ - ChildNumber::from_hardened_idx(identity_index).unwrap(), - ChildNumber::from_hardened_idx(key_index).unwrap(), // key_index used as topup_index - ]) - } + FFIDerivationPathType::BlockchainIdentityCreditTopupFunding => { + // Top-up: identity_index'/topup_index' + let cn1 = match ChildNumber::from_hardened_idx(identity_index) { + Ok(v) => v, + Err(e) => { + FFIError::set_error(error, FFIErrorCode::InvalidDerivationPath, format!("Invalid identity_index: {}", e)); + return ptr::null_mut(); + } + }; + let cn2 = match ChildNumber::from_hardened_idx(key_index) { + Ok(v) => v, + Err(e) => { + FFIError::set_error(error, FFIErrorCode::InvalidDerivationPath, format!("Invalid topup_index: {}", e)); + return ptr::null_mut(); + } + }; + DerivationPath::from(vec![cn1, cn2]) + }key-wallet-ffi/src/types.rs (1)
28-36: Do not silently default unknown Network to mainnet (FFINetwork::Dash)Defaulting to Dash on unknown variants risks cross-network misuse. Make the mapping exhaustive and fail fast or introduce an Unknown variant in FFINetwork. At minimum, remove the wildcard arm.
impl From<Network> for FFINetwork { fn from(n: Network) -> Self { - match n { + match n { Network::Dash => FFINetwork::Dash, Network::Testnet => FFINetwork::Testnet, Network::Regtest => FFINetwork::Regtest, - Network::Devnet => FFINetwork::Devnet, - _ => FFINetwork::Dash, // Default to Dash for unknown networks + Network::Devnet => FFINetwork::Devnet, } } }If upstream adds variants later, consider TryFrom -> Result<FFINetwork, ()> to surface errors at call sites.
🧹 Nitpick comments (38)
dash-network-ffi/src/lib.rs (4)
111-124: Tests hardcode magic bytes; derive from dash_network to avoid brittleness.Hardcoding network constants violates our “no hardcoded network params” guideline and breaks if upstream changes. Compute expected values from DashNetwork.
Apply:
- let dash_info = NetworkInfo::new(Network::Dash); - assert_eq!(dash_info.magic(), 0xBD6B0CBF); - - let testnet_info = NetworkInfo::new(Network::Testnet); - assert_eq!(testnet_info.magic(), 0xFFCAE2CE); - - let devnet_info = NetworkInfo::new(Network::Devnet); - assert_eq!(devnet_info.magic(), 0xCEFFCAE2); - - let regtest_info = NetworkInfo::new(Network::Regtest); - assert_eq!(regtest_info.magic(), 0xDCB7C1FC); + let nets = [ + (Network::Dash, DashNetwork::Dash), + (Network::Testnet, DashNetwork::Testnet), + (Network::Devnet, DashNetwork::Devnet), + (Network::Regtest, DashNetwork::Regtest), + ]; + for (ffi_net, dash_net) in nets { + let info = NetworkInfo::new(ffi_net); + assert_eq!(info.magic(), dash_net.magic()); + }
126-136: Prefer round-trip tests over literal magic bytes.Use magic derived from DashNetwork and round-trip through from_magic, rather than literals.
Apply:
- // Valid magic bytes - assert!(NetworkInfo::from_magic(0xBD6B0CBF).is_ok()); - assert!(NetworkInfo::from_magic(0xFFCAE2CE).is_ok()); - assert!(NetworkInfo::from_magic(0xCEFFCAE2).is_ok()); - assert!(NetworkInfo::from_magic(0xDCB7C1FC).is_ok()); + for dash_net in [DashNetwork::Dash, DashNetwork::Testnet, DashNetwork::Devnet, DashNetwork::Regtest] { + let m = dash_net.magic(); + let info = NetworkInfo::from_magic(m).expect("magic should map to a network"); + assert_eq!(info.network, dash_net); + }
146-158: Avoid hardcoding activation heights; use the queried height.This will break if activation heights change upstream. Assert behavior relative to the reported activation height instead.
Apply:
- let dash_info = NetworkInfo::new(Network::Dash); - assert_eq!(dash_info.core_v20_activation_height(), 1987776); - assert!(!dash_info.is_core_v20_active(1987775)); - assert!(dash_info.is_core_v20_active(1987776)); - assert!(dash_info.is_core_v20_active(2000000)); - - let testnet_info = NetworkInfo::new(Network::Testnet); - assert_eq!(testnet_info.core_v20_activation_height(), 905100); - assert!(!testnet_info.is_core_v20_active(905099)); - assert!(testnet_info.is_core_v20_active(905100)); + for net in [Network::Dash, Network::Testnet] { + let info = NetworkInfo::new(net); + let h = info.core_v20_activation_height(); + assert!(!info.is_core_v20_active(h.saturating_sub(1))); + assert!(info.is_core_v20_active(h)); + assert!(info.is_core_v20_active(h + 1000)); + }
42-49: FFI error shape for C: consider a repr(C) error code enum.NetworkError is great for Rust callers, but C needs a stable numeric code. Add a small repr(C) FFI error enum and map NetworkError to it in extern functions.
I can provide a minimal FFIError/FFIErrorCode and mapping if you want to standardize errors across FFI crates in this PR.
dash-network-ffi/README.md (3)
24-49: Swift example uses APIs that don’t exist; update after exporting the C ABI.dashNetworkFfiInitialize(), NetworkInfo.fromMagic, and instance methods imply a C ABI and Swift overlay that we haven’t shipped. Either:
- Implement those extern "C" functions and provide the Swift wrapper, or
- Replace the example with Rust-only usage until the C surface lands.
When you export strings, document ownership (who frees) and provide a free function (e.g., dash_network_string_free).
51-76: Python example assumes a dash_network_ffi module; not currently provided.Unless you ship a cffi/ctypes binding or a PyO3 wrapper, this will confuse users. Consider removing or gating it behind “when bindings are generated.”
Also, avoid hardcoded magic constants in examples. Show round-tripping a magic value obtained from the API instead.
11-11: Grammar nit: parallelism in Features list.“Network information and utilities exposed through FFI” reads better than “...exposed...”.
Apply:
-- Network information and utilities exposed through FFI +- Network information and utilities exposed through FFIkey-wallet/src/gap_limit.rs (3)
93-101: Unreachable branch in mark_used; always falls into recalc path.You set
highest_used_index = max(current, index), then checkif index > highest { ... }using that updatedhighest, which can never be true. This is harmless but dead code.Refactor to unconditionally recalc the gap after updating
highest_used_index:- // Reset unused count if this breaks a gap - if let Some(highest) = self.highest_used_index { - if index > highest { - self.current_unused_count = 0; - } else { - // Recalculate unused count from highest used - self.current_unused_count = self.calculate_current_gap(); - } - } + if self.highest_used_index.is_some() { + // Recalculate unused count from highest used + self.current_unused_count = self.calculate_current_gap(); + }
219-221: Potential underflow in stats.unused_count; prefer saturating math.
unused_countis computed ashighest_generated_index + 1 - used_count. If callers mark used indices that are beyondhighest_generated_index, this can underflow. While today inputs likely avoid this, making it saturating is safer.- unused_count: self.highest_generated_index + 1 - self.used_indices.len() as u32, + unused_count: (self.highest_generated_index.saturating_add(1)) + .saturating_sub(self.used_indices.len() as u32),
187-191: next_index can skip index 0 in a fresh tracker.With
highest_generated_indexinitialized to 0,next_index()returns 1 on a fresh instance, which skips index 0 unless callers seed generation differently (tests currently sidestep by callingmark_generated(0)explicitly). Consider deriving this from generated count or introducing a sentinel.One simple fix without sentinels:
- pub fn next_index(&self) -> u32 { - self.highest_generated_index + 1 - } + pub fn next_index(&self) -> u32 { + if self.current_unused_count == 0 && self.highest_used_index.is_none() { + 0 + } else { + self.highest_generated_index.saturating_add(1) + } + }If you prefer a cleaner model, we can add a small
generated_count()helper and base bothnext_index()andaddresses_to_generate()on it.key-wallet-ffi/build-ios.sh (1)
20-33: Consider supporting Intel simulators and producing an .xcframework.Right now we only emit an arm64 simulator static lib and separate device lib. Many teams still need x86_64 simulator slices (Intel Macs), and Xcode integrates more smoothly with an
.xcframework.Optional refactor:
- Build both
aarch64-apple-ios-simandx86_64-apple-ios(orx86_64-apple-iosfor Intel sim, depending on your Rust toolchain).- Create a fat simulator lib via
lipoand then assemble an.xcframework.Example snippet you can adapt:
# Add Intel sim target (optional if you want to support Intel Macs) rustup target add x86_64-apple-ios # Build Intel sim cargo build --release --target x86_64-apple-ios # Create fat simulator lib (arm64 + x86_64) lipo -create \ target/aarch64-apple-ios-sim/release/libkey_wallet_ffi.a \ target/x86_64-apple-ios/release/libkey_wallet_ffi.a \ -output target/universal/release/libkey_wallet_ffi_sim.a # Create xcframework (static) xcodebuild -create-xcframework \ -library target/universal/release/libkey_wallet_ffi_device.a \ -library target/universal/release/libkey_wallet_ffi_sim.a \ -output target/universal/release/KeyWalletFFI.xcframeworkPlease verify whether your consumer(s) require Intel simulator support; if not, you can keep the current setup.
key-wallet/src/psbt/mod.rs (3)
526-533: Align network type usage across implementations (remove unnecessary conversion).You switched to
network: k.networkhere, which is good ifExtendedPrivKey.networknow matchesPrivateKey.network. However, the set-based implementation below still calls.into(), creating an inconsistency.If both sides share the same network type, prefer removing
.into()everywhere for consistency and to avoid a potential clippyneedless_intowarning.Apply to the set implementation below:
- network: k.network.into(), + network: k.network,
565-571: Keep network conversion consistent with the change above.See the note on Lines 526-533. If
k.networkalready matchesPrivateKey.network, drop.into()here as well.- network: k.network.into(), + network: k.network,
618-623: Typo in error message: “bip23” → “bip32”.Spotted a likely typo in the display message.
- Bip32(ref e) => write_err!(f, "a bip23 error"; e), + Bip32(ref e) => write_err!(f, "a bip32 error"; e),key-wallet-ffi/README.md (3)
24-29: Prerequisites: align MSRV and iOS build notes with the repo’s standards.
- The repo targets MSRV 1.80 (per coding guidelines). The README states Rust 1.70+. Please update to 1.80+ to avoid confusion.
- “cargo-lipo” is not used by the provided build-ios.sh; consider either removing that requirement or clarifying that the recommended path is the Unified SDK or the included script.
Suggested edits:
- - Rust 1.70+ - - For iOS: Xcode and cargo-lipo + - Rust 1.80+ (MSRV) + - For iOS: Xcode and either the Unified SDK build or the included build-ios.sh
9-13: Tone down “memory-safe/thread-safe” phrasing or add qualifiers.Cross-language FFI cannot be fully memory/thread safe by construction; we enforce safety on our side, but callers must follow the API contracts (ownership, lifetimes, free functions). Recommend softening the language or adding a qualifier such as “Designed for memory and thread safety when used as documented.”
16-21: Minor wording polish for supported languages.For clarity, consider rephrasing the lead-in sentence.
-This library provides C-compatible FFI that can be used by: +This library exposes a C-compatible FFI usable from:key-wallet-ffi/src/wallet_tests.rs (4)
72-92: Assert success code after wallet_get_xpub and prefer using a dedicated string_free (if exposed).You check for non-null xpub and free it via
CString::from_rawwhich matches the current implementation. To make failures easier to diagnose, also asserterror.code == Successafterwallet_get_xpub. If the crate exposes astring_freeFFI (common in this repo family), prefer using it for future-proofing allocator-boundaries.Example tweak:
- let xpub = unsafe { wallet::wallet_get_xpub(source_wallet, FFINetwork::Testnet, 0, error) }; - assert!(!xpub.is_null()); + let xpub = unsafe { wallet::wallet_get_xpub(source_wallet, FFINetwork::Testnet, 0, error) }; + assert!(!xpub.is_null()); + assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success);
121-138: Test name vs. behavior mismatch; consider exercising wallet_add_account instead.
test_wallet_multiple_accountscurrently creates multiple wallets rather than multiple accounts. Either rename to reflect the behavior or actually add multiple accounts to one wallet.Proposed refactor (sketch):
- for _account_index in 0..3 { - let wallet = wallet::wallet_create_from_seed(...); - ... - wallet::wallet_free(wallet); - } + let wallet = wallet::wallet_create_from_seed(...); + assert!(!wallet.is_null()); + for idx in 0..3u32 { + let res = wallet::wallet_add_account(wallet, FFINetwork::Testnet, 0 /* StandardBIP44 */, idx); + assert!(!res.account.is_null() || res.error_code != 0); + if !res.account.is_null() { account_free(res.account); } + if !res.error_message.is_null() { let _ = CString::from_raw(res.error_message); } + } + wallet::wallet_free(wallet);
256-268: Add a test for invalid seed length (not just null).
wallet_create_from_seed_bytesreturnsInvalidInputwhenseed_len != 64. Add a case likeseed_len = 32to assert the error code and null return.Example:
let mut err = FFIError::success(); let bad_bytes = [0xAAu8; 32]; let w = unsafe { wallet::wallet_create_from_seed_bytes(bad_bytes.as_ptr(), bad_bytes.len(), FFINetwork::Testnet, &mut err) }; assert!(w.is_null()); assert_eq!(unsafe { err.code }, FFIErrorCode::InvalidInput);
427-435: Test xpub prefix on both networks (optional).Asserting
tpubon testnet is good. Consider adding a quick mainnet case (FFINetwork::Dash) and assertxpubto catch accidental network mapping regressions.key-wallet-ffi/src/error.rs (1)
44-48: HandleCString::newerror case more gracefullyThe
unwrap_or_default()call will create an empty C string if the input message contains null bytes, which could mislead the caller. Consider using a fallback error message instead.pub fn error(code: FFIErrorCode, msg: String) -> Self { FFIError { code, - message: CString::new(msg).unwrap_or_default().into_raw(), + message: CString::new(msg) + .unwrap_or_else(|_| CString::new("Error message contains invalid characters").unwrap()) + .into_raw(), } }key-wallet-ffi/src/account.rs (1)
55-63: Avoid ambiguous error message when to_account_type() preconditions aren’t metThe format! placeholder prints the numeric
account_type(C value), not the Rust enum, which can mislead debugging. Also, this branch is specifically hit for variants that require extra params (e.g., IdentityTopUp). Make the message explicit and include the offending C value and Rust enum variant for clarity.- return FFIAccountResult::error( - FFIErrorCode::InvalidInput, - format!("Missing required parameters for account type {}", account_type), - ); + return FFIAccountResult::error( + FFIErrorCode::InvalidInput, + format!( + "Missing required parameters for account_type={} ({:?})", + account_type, account_type_enum + ), + );key-wallet-ffi/src/utxo.rs (1)
195-207: Free boxed slices with unsized pointer reconstructionYou’re reconstructing a Box<[FFIUTXO]> from a thin pointer via from_raw on a slice created with from_raw_parts_mut—excellent. Minor improvement: use std::ptr::slice_from_raw_parts_mut for clarity when constructing the unsized pointer.
- let _ = Box::from_raw(std::slice::from_raw_parts_mut(utxos, count)); + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(utxos, count));key-wallet-ffi/src/address.rs (2)
38-105: Optional: delegate derivation to Account convenience methods to reduce duplicationSince key-wallet::Account now exposes derive_receive_address(), you can avoid reimplementing path building and Secp setup here. This keeps FFI thin and consistent with core logic.
- match wallet.inner().get_bip44_account(network_rust, account_index) { - Some(account) => { - // Derive external address at index - use key_wallet::ChildNumber; - use secp256k1::Secp256k1; - let secp = Secp256k1::new(); - // External addresses use derivation path m/0/index - let child_external = match ChildNumber::from_normal_idx(0) { ... }; - let child_index = match ChildNumber::from_normal_idx(address_index) { ... }; - match account.account_xpub.derive_pub(&secp, &[child_external, child_index]) { - Ok(derived_key) => { ... build p2pkh ... } - Err(e) => { ... } - } - } + match wallet.inner().get_bip44_account(network_rust, account_index) { + Some(account) => match account.derive_receive_address(address_index) { + Ok(address) => { + FFIError::set_success(error); + match CString::new(address.to_string()) { + Ok(c_str) => c_str.into_raw(), + Err(_) => { + FFIError::set_error(error, FFIErrorCode::AllocationFailed, "Failed to allocate string".to_string()); + ptr::null_mut() + } + } + } + Err(e) => { + FFIError::set_error(error, FFIErrorCode::WalletError, format!("Failed to derive address: {}", e)); + ptr::null_mut() + } + }Repeat similarly for wallet_derive_change_address and wallet_get_address_at_index using derive_change_address() and derive_address_at().
412-425: Safety note: document that the array must be freed with address_array_free (not libc::free)Callers mixing allocators will cause heap corruption. Make this explicit in the Safety section.
/// - `count` must be the correct number of addresses in the array -/// - After calling this function, all pointers become invalid +/// - After calling this function, all pointers become invalid +/// - The array returned by wallet_get_all_addresses must be freed with this function, +/// not with libc::free or other deallocatorskey-wallet/src/account/mod.rs (2)
204-220: Small simplification: construct dashcore::PublicKey without serialize/parse roundtripYou’re serializing and reparsing the secp256k1 key. The conversion helper avoids the extra step.
- let xpub = self.derive_child_xpub(&path)?; - // Convert secp256k1::PublicKey to dashcore::PublicKey - let pubkey = - dashcore::PublicKey::from_slice(&xpub.public_key.serialize()).map_err(|e| { - crate::error::Error::InvalidParameter(format!("Invalid public key: {}", e)) - })?; - Ok(dashcore::Address::p2pkh(&pubkey, self.network)) + let xpub = self.derive_child_xpub(&path)?; + let pubkey = dashcore::PublicKey::new(xpub.public_key); + Ok(dashcore::Address::p2pkh(&pubkey, self.network))Repeat similarly in derive_change_address().
Also applies to: 232-248
259-269: Consider #[must_use] on batch derivation helpersThese return values are easy to ignore by accident.
- pub fn derive_receive_addresses( + #[must_use] + pub fn derive_receive_addresses( @@ - pub fn derive_change_addresses( + #[must_use] + pub fn derive_change_addresses(Also applies to: 280-290
key-wallet-ffi/src/managed_wallet.rs (1)
128-156: Success path returns CString::into_raw correctly; ensure error path never leaksBoth next-address functions are fine wrt CString ownership. Consider using wallet.inner().get_bip44_account + Account::derive_* to unify logic with key-wallet and reduce Secp ceremony (optional).
Also applies to: 248-276
key-wallet-ffi/src/wallet.rs (4)
540-569: Returned C string needs an explicit free function in the public C headerwallet_get_xpub returns a CString via into_raw(), but the file doesn’t expose a corresponding “string_free” in this module. Make sure the public header declares a single, canonical free function (e.g., key_wallet_string_free or ffi_string_free) and document callers must use it. If you already have a shared utils::string_free, ensure it’s exported and present in the generated header.
I can add a small FFI helper if you’d like:
+#[no_mangle] +pub extern "C" fn key_wallet_string_free(s: *mut c_char) { + if !s.is_null() { + unsafe { let _ = CString::from_raw(s); } + } +}
604-646: Reduce duplication: centralize account_type c_uint → FFIAccountType mappingThe match mapping account_type appears three times across add-account functions. Factor it into a small helper to cut repetition and future skew.
+fn map_account_type(tag: c_uint) -> Result<crate::types::FFIAccountType, String> { + use crate::types::FFIAccountType; + match tag { + 0 => Ok(FFIAccountType::StandardBIP44), + 1 => Ok(FFIAccountType::StandardBIP32), + 2 => Ok(FFIAccountType::CoinJoin), + 3 => Ok(FFIAccountType::IdentityRegistration), + 4 => Err("IdentityTopUp accounts require a registration_index. Use a specialized function instead".into()), + 5 => Ok(FFIAccountType::IdentityTopUpNotBoundToIdentity), + 6 => Ok(FFIAccountType::IdentityInvitation), + 7 => Ok(FFIAccountType::ProviderVotingKeys), + 8 => Ok(FFIAccountType::ProviderOwnerKeys), + 9 => Ok(FFIAccountType::ProviderOperatorKeys), + 10 => Ok(FFIAccountType::ProviderPlatformKeys), + _ => Err(format!("Invalid account type: {}", tag)), + } +}Then call map_account_type(account_type) and convert to AccountType once.
This shrinks three identical match blocks and reduces the chance of divergence.Also applies to: 717-746, 829-852
109-118: Comment drift: this path does create accountsThe comment “First create the wallet without accounts” no longer matches the implementation; from_mnemonic_with_passphrase builds accounts from options internally. Update the comment to avoid confusion.
308-336: Optional: validate xpub’s network/version matches FFINetworkExtendedPubKey encodes version bytes (xpub/ypub/tpub…), which imply a network. Consider rejecting mismatches early to prevent mixed-network wallets.
I can wire a small check if key_wallet::ExtendedPubKey exposes its network/version.
Also applies to: 340-350
key-wallet-ffi/src/wallet_manager.rs (2)
284-336: Provide a free for single returned strings (receive/change address)Both functions return CString::into_raw() but there’s no obvious single-string free in this module. Either document to use the shared string_free or export one from a common utils module and ensure the header declares it. Otherwise consumers will leak.
I can add a crate-wide key_wallet_string_free as shown in the wallet.rs comment if helpful.
Also applies to: 367-419
258-273: Not implemented path returns NotFoundwallet_manager_get_wallet currently always returns NotFound. If that’s intentional for this PR, consider returning a dedicated NotImplemented error to make the behavior explicit to SDK callers.
key-wallet-ffi/src/derivation.rs (1)
512-515: Docstring mismatches: fix free-function names in comments
- derivation_xpriv_to_xpub: says “free with extended_public_key_free” but the function is derivation_xpub_free.
- derivation_xpriv_to_string / derivation_xpub_to_string: docstrings say “string_free” but the function is derivation_string_free.
Update comments to match the exported symbol names to prevent header/docs confusion.
-/// - The returned pointer must be freed with `extended_public_key_free` +/// - The returned pointer must be freed with `derivation_xpub_free` @@ -/// - The returned string must be freed with `string_free` +/// - The returned string must be freed with `derivation_string_free` @@ -/// - The returned string must be freed with `string_free` +/// - The returned string must be freed with `derivation_string_free` @@ -/// - The pointer must not be used after calling this function +/// - The pointer must not be used after calling this functionAlso applies to: 552-555, 590-597, 694-698
key-wallet-ffi/src/types.rs (2)
64-96: Freeing error_message from FFIAccountResultFFIAccountResult::error allocates error_message via CString::into_raw(). Ensure there’s a public free function in the C header to release this string (or a generic key_wallet_string_free) and document that callers must invoke it when error_code != 0.
291-337: Safety: document lifetime and alignment for raw pointer arrays in FFIWalletAccountCreationOptionsThe to_wallet_options() contract relies on multiple raw arrays and counts. Add doc comments in the header describing ownership (borrowed-only), lifetime (must outlive call), and alignment/contiguity guarantees. Consider basic validation (e.g., rejecting obviously huge counts) to avoid accidental OOM.
Also applies to: 339-447
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
key-wallet-ffi/src/address.rs (1)
37-50: Fix array deallocation: use ptr::slice_from_raw_parts_mut and ensure creator used Box<[T]>.Two issues:
- Compile-time/type mismatch:
std::slice::from_raw_parts_mutreturns&mut [T], butBox::from_rawexpects*mut [T]. Usestd::ptr::slice_from_raw_parts_mutinstead.- Memory contract: This free is only correct if the array was allocated as a boxed slice (
Vec::into_boxed_slice→Box<[T]>→Box::into_raw). If the producer leaked aVecviaas_mut_ptr, freeing here withBox::from_rawis UB. Please verify all producers follow the boxed-slice pattern.This echoes the prior review; adding the compile fix below.
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)); + // Free the array itself (must match Box<[T]> allocation) + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(addresses, count)); } } }To confirm producers, search for array-returning FFI that set
*addresses_outand ensureinto_boxed_slice()is used before leaking the pointer.#!/bin/bash # Scan for address array producers and boxed-slice usage rg -n -C3 '(addresses?_out|address(es)?_out)\s*=' rg -n -C2 'into_boxed_slice\(\)'key-wallet-ffi/src/wallet_manager.rs (4)
219-256: Fix UB and potential double-free: produce boxed slice for wallet IDs and free via DST pointerYou're leaking a Vec and later freeing it as if it were a Box<[u8]>. Also, Box::from_raw expects a DST pointer, not an &mut [u8]. Allocate as a boxed slice here to match the free function and avoid UB.
Apply this diff:
- let count = ids_guard.len(); + let count = ids_guard.len(); if count == 0 { *count_out = 0; *wallet_ids_out = ptr::null_mut(); } else { // Allocate memory for wallet IDs (32 bytes each) - let mut ids_buffer = Vec::with_capacity(count * 32); - for wallet_id in ids_guard.iter() { - ids_buffer.extend_from_slice(wallet_id); - } - let ids_ptr = ids_buffer.as_mut_ptr(); - std::mem::forget(ids_buffer); - - *wallet_ids_out = ids_ptr; - *count_out = count; + let mut ids_buffer = Vec::with_capacity(count * 32); + for wallet_id in ids_guard.iter() { + ids_buffer.extend_from_slice(wallet_id); + } + // Match allocation with a boxed slice so the free function can reconstruct it safely + let mut boxed = ids_buffer.into_boxed_slice(); // Box<[u8]> + let ptr = boxed.as_mut_ptr(); + std::mem::forget(boxed); + *wallet_ids_out = ptr; + *count_out = count; // number of wallet IDs (each 32 bytes) }
936-942: Free wallet IDs using correct DST pointer typeThe current code calls Box::from_raw on a slice reference (not a pointer) and mismatches the producer allocation. Reconstruct a Box<[u8]> via ptr::slice_from_raw_parts_mut.
Apply this diff:
pub unsafe extern "C" fn wallet_manager_free_wallet_ids(wallet_ids: *mut u8, count: usize) { if !wallet_ids.is_null() && count > 0 { unsafe { - let _ = Box::from_raw(std::slice::from_raw_parts_mut(wallet_ids, count * 32)); + // count is number of wallet IDs; buffer length is count * 32 bytes + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(wallet_ids, count * 32)); } } }
783-787: Fix monitored address array ownership: return boxed slice to match freeSame pattern: you leak a Vec<*mut c_char> and free it later as if it were a boxed slice. Convert to a boxed slice before exposing the pointer.
Apply this diff:
- *count_out = all_addresses.len(); - let ptr = all_addresses.as_mut_ptr(); - std::mem::forget(all_addresses); - *addresses_out = ptr; + *count_out = all_addresses.len(); + let mut boxed = all_addresses.into_boxed_slice(); // Box<[*mut c_char]> + let ptr = boxed.as_mut_ptr(); + std::mem::forget(boxed); + *addresses_out = ptr;
954-964: Free monitored address array using DST pointer reconstructionUse ptr::slice_from_raw_parts_mut to reconstruct the boxed slice for the pointer array; the current approach is UB.
Apply this diff:
if !addresses.is_null() { 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)); + // Free the array itself (matches Box<[T]> allocation) + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(addresses, count)); }key-wallet-ffi/src/wallet_manager_tests.rs (1)
1013-1017: Do not call libc::free on Rust-owned buffer; use library free to avoid UBThis buffer was allocated by Rust and must be freed via wallet_manager_free_wallet_ids (or by reconstructing the exact Box type). Using libc::free is undefined behavior.
Apply this diff:
- unsafe { - libc::free(wallet_ids as *mut libc::c_void); - wallet_manager::wallet_manager_free(manager); - } + unsafe { + // count is number of wallet IDs (not bytes) + wallet_manager::wallet_manager_free_wallet_ids(wallet_ids, id_count); + wallet_manager::wallet_manager_free(manager); + }
🧹 Nitpick comments (13)
key-wallet-ffi/tests/test_addr_simple.rs (3)
5-5: Remove unused import (will trigger warning).
use std::ffi::CStr;is not used in this test. Please drop it to keep warnings clean under clippy -D warnings.- use std::ffi::CStr;
7-21: Assert FFIError stays Success after each call.You already thread an
FFIErrorthrough; assertingerror.code == Successstrengthens the test and catches silent failures at the FFI boundary.let mut error = FFIError::success(); let error = &mut error as *mut FFIError; @@ let wallet = unsafe { key_wallet_ffi::wallet::wallet_create_from_seed( seed.as_ptr(), seed.len(), FFINetwork::Testnet, error, ) }; assert!(!wallet.is_null()); + // Verify no error was reported + unsafe { assert!(matches!((*error).code, key_wallet_ffi::error::FFIErrorCode::Success)) }; @@ let is_watch_only = unsafe { key_wallet_ffi::wallet::wallet_is_watch_only(wallet, error) }; assert!(!is_watch_only); + unsafe { assert!(matches!((*error).code, key_wallet_ffi::error::FFIErrorCode::Success)) }; @@ let success = unsafe { key_wallet_ffi::wallet::wallet_get_id(wallet, wallet_id.as_mut_ptr(), error) }; assert!(success); + unsafe { assert!(matches!((*error).code, key_wallet_ffi::error::FFIErrorCode::Success)) };Also applies to: 24-31
1-41: Consider a lightweight proptest to broaden coverage.A small property test over random 64-byte seeds can validate that wallets are created and IDs are non-zero across many inputs (no key material logged). I can draft it if you want it in this PR.
key-wallet-ffi/src/address.rs (2)
52-58: Clarify Safety docs: error pointer may be null (functions guard for null).Docs say “must be a valid pointer,” but implementations tolerate null and simply drop errors. Adjusting docs prevents misuse expectations at the C boundary.
-/// - `error` must be a valid pointer to an FFIError +/// - `error` may be null; if non-null, it will be populated with success or errorApply in both functions’ Safety sections.
Also applies to: 116-122
156-163: Consider exporting an FFI enum/consts for address type codes (0=P2PKH, 1=P2SH, 2=INVALID).Magic numbers at the boundary are brittle. Defining constants in the C header and mirroring them here improves API clarity and reduces caller errors.
key-wallet-ffi/src/wallet_manager.rs (2)
262-269: Clarify lifetime docs for returned wallet pointerThe comment says “only valid while the manager exists,” but you clone into an Arc-backed FFIWallet. The pointer is valid until freed with wallet_free, independent of manager’s lifetime.
Apply this doc-only diff:
-/// - The returned wallet pointer is only valid while the manager exists +/// - The returned wallet pointer remains valid until freed with `wallet_free` +/// (it holds its own Arc to the inner wallet)Also applies to: 302-309
384-399: Document caller responsibility to free returned stringsBoth address getters return heap-allocated C strings. Add an explicit note to free via utils::string_free to prevent leaks in C/Swift/other FFI consumers.
Apply this doc-only diff:
/// - The caller must ensure all pointers remain valid for the duration of this call +/// - Returns a newly allocated C string; caller must free with `utils::string_free`(Apply to both get_receive_address and get_change_address.)
Also applies to: 514-525
key-wallet-ffi/tests/test_improved_watch_only.rs (2)
7-7: Remove unused constantTEST_MNEMONIC is unused in this test.
Apply this diff:
- const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about";
38-44: Drop unused wallet managers to reduce noiseThese managers are created but not used. Remove to keep the test focused.
Apply this diff:
- // 4. Create wallet managers to derive addresses - let source_manager = unsafe { key_wallet_ffi::wallet_manager::wallet_manager_create(error) }; - assert!(!source_manager.is_null()); - - let watch_manager = unsafe { key_wallet_ffi::wallet_manager::wallet_manager_create(error) }; - assert!(!watch_manager.is_null()); @@ - key_wallet_ffi::wallet_manager::wallet_manager_free(source_manager); - key_wallet_ffi::wallet_manager::wallet_manager_free(watch_manager);Also applies to: 57-58
key-wallet-ffi/src/wallet_manager_tests.rs (4)
669-677: *Avoid const-cast by aligning API: after changing get_wallet to return mut, remove castOnce wallet_manager_get_wallet returns *mut FFIWallet, this cast becomes unnecessary (and we avoid casting away const).
Apply this diff after the API change:
- // Clean up the wallet (cast from const to mut for free) - unsafe { - wallet::wallet_free(wallet as *mut _); - } + // Clean up the wallet + unsafe { wallet::wallet_free(wallet); }
1230-1235: Add explicit free for wallet pointer in this test tooTo prevent leaks, free the wallet returned by wallet_manager_get_wallet.
Apply this diff (after the API change returns *mut):
- assert!(!wallet.is_null()); - assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success); + assert!(!wallet.is_null()); + assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success); + unsafe { wallet::wallet_free(wallet); }
1237-1245: Free managed wallet info in tests once free function existsComplementing the added managed_wallet_info_free, free wallet_info here to avoid leaks.
Apply this diff once the free function is added:
- assert!(!wallet_info.is_null()); - assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success); + assert!(!wallet_info.is_null()); + assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success); + unsafe { crate::managed_wallet::managed_wallet_info_free(wallet_info); }
82-86: Comment about “library bug with passphrase wallets” will rot; prefer TODO with issue linkReplace anecdotal note with a TODO and issue reference so future maintainers know where to track it.
Apply this diff:
- // Note: We use different mnemonics instead of different passphrases - // because the library has a bug with passphrase wallets (see line 140-146 in wallet_manager/mod.rs) + // TODO(QE/maintainers): Use passphrase variants when bug XYZ is fixed + // See: <link to tracking issue> (passphrase wallet handling)
📜 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 (8)
key-wallet-ffi/src/address.rs(1 hunks)key-wallet-ffi/src/address_tests.rs(1 hunks)key-wallet-ffi/src/wallet_manager.rs(1 hunks)key-wallet-ffi/src/wallet_manager_tests.rs(1 hunks)key-wallet-ffi/tests/integration_test.rs(1 hunks)key-wallet-ffi/tests/test_addr_simple.rs(1 hunks)key-wallet-ffi/tests/test_improved_watch_only.rs(1 hunks)key-wallet-ffi/tests/test_passphrase_wallets.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet-ffi/tests/test_passphrase_wallets.rs
- key-wallet-ffi/tests/integration_test.rs
- key-wallet-ffi/src/address_tests.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{rs,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/src/address.rskey-wallet-ffi/tests/test_improved_watch_only.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
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/src/address.rskey-wallet-ffi/tests/test_improved_watch_only.rs
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/src/address.rskey-wallet-ffi/tests/test_improved_watch_only.rs
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/src/address.rskey-wallet-ffi/tests/test_improved_watch_only.rs
**/tests/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
Files:
key-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_improved_watch_only.rs
🧠 Learnings (4)
📓 Common learnings
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_addr_simple.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rs
🧬 Code Graph Analysis (5)
key-wallet-ffi/src/wallet_manager.rs (3)
key-wallet-ffi/src/types.rs (3)
error(86-95)new(47-51)new(105-109)key-wallet-ffi/src/error.rs (3)
error(43-48)set_success(62-68)set_error(52-58)key-wallet-ffi/src/managed_wallet.rs (1)
new(24-28)
key-wallet-ffi/src/wallet_manager_tests.rs (4)
key-wallet-ffi/src/types.rs (4)
error(86-95)success(77-83)new(47-51)new(105-109)key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)key-wallet-ffi/src/wallet_manager.rs (16)
wallet_manager_create(28-35)wallet_manager_wallet_count(883-909)wallet_manager_free(919-925)wallet_manager_add_wallet_from_mnemonic(186-201)wallet_manager_get_wallet_ids(213-256)wallet_manager_free_wallet_ids(936-942)wallet_manager_get_receive_address(389-461)wallet_manager_get_change_address(472-544)wallet_manager_get_wallet_balance(559-616)wallet_manager_get_monitored_addresses(715-791)wallet_manager_free_addresses(954-965)wallet_manager_current_height(840-873)wallet_manager_update_height(801-830)wallet_manager_get_wallet(270-318)wallet_manager_process_transaction(634-703)wallet_manager_get_managed_wallet_info(332-378)key-wallet-ffi/src/wallet.rs (1)
wallet_free(587-593)
key-wallet-ffi/tests/test_addr_simple.rs (3)
key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)key-wallet-ffi/src/wallet.rs (4)
wallet_create_from_seed(227-240)wallet_is_watch_only(515-529)wallet_get_id(465-481)wallet_free(587-593)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
wallet_id(32-32)wallet_id(124-126)
key-wallet-ffi/src/address.rs (2)
key-wallet-ffi/src/types.rs (1)
error(86-95)key-wallet-ffi/src/error.rs (3)
error(43-48)set_error(52-58)set_success(62-68)
key-wallet-ffi/tests/test_improved_watch_only.rs (4)
key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)key-wallet-ffi/src/wallet.rs (4)
wallet_create_from_seed(227-240)wallet_get_xpub(540-577)wallet_create_from_xpub(308-397)wallet_free(587-593)key-wallet-ffi/src/wallet_manager.rs (2)
wallet_manager_create(28-35)wallet_manager_free(919-925)key-wallet-ffi/src/utils.rs (1)
string_free(17-23)
🔇 Additional comments (3)
key-wallet-ffi/tests/test_improved_watch_only.rs (1)
12-36: LGTM: watch-only flow validates new xpub-based creation pathCreates a seed wallet, extracts xpub, and successfully builds a watch-only wallet. Properly frees wallet and xpub.
key-wallet-ffi/src/wallet_manager_tests.rs (2)
440-444: Good: frees all returned address strings and the array bufferThe test exercises wallet_manager_free_addresses correctly after using the monitored addresses.
821-854: Solid null-input coverageGood coverage of null manager and invalid pointer cases across APIs.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/Cargo.toml (1)
13-13: Gate hex’s std feature behind this crate’s “std” featureTo keep feature unification coherent and avoid accidental pulls of std-only code in no_std builds, enabling key-wallet’s
stdfeature should also enablehex/std.• File: key-wallet/Cargo.toml
• Line: 13- std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "dash-network/std", "rand"] + std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "dash-network/std", "rand", "hex/std"]
♻️ Duplicate comments (6)
key-wallet-ffi/src/address_tests.rs (1)
134-151: Potential issue with symmetric allocation/deallocation.The test creates a
Vecof raw C strings but then forgets the vector after extracting its pointer. This must be freed properly to avoid undefined behavior.key-wallet-ffi/src/wallet_manager.rs (4)
203-256: Fix allocation/deallocation mismatch for wallet IDs.The function allocates a Vec buffer but forgets it. The corresponding free function attempts to reconstruct a Box from slice parts, which causes undefined behavior.
705-791: Fix allocation/deallocation mismatch for address arrays.Similar to wallet IDs, this function allocates a Vec of addresses but the free function incorrectly attempts to reconstruct a Box.
927-942: Fix DST pointer reconstruction for wallet IDs.The free function incorrectly attempts to reconstruct a Box from slice parts.
#!/bin/bash # Check how wallet IDs are allocated in get_wallet_ids ast-grep --pattern 'fn wallet_manager_get_wallet_ids($$$) { $$$ let ids_buffer = $$$ $$$ }'
944-965: Fix DST pointer reconstruction for address array.Similar issue with reconstructing Box from slice parts.
key-wallet-ffi/src/types.rs (1)
28-36: Do not default unknown networks to mainnet; make mapping exhaustive.Silently mapping unknown
key_wallet::Networkvariants toFFINetwork::Dashis dangerous. Prefer an exhaustive match with no wildcard so additions upstream cause a compile-time error rather than a silent mainnet fallback.impl From<Network> for FFINetwork { fn from(n: Network) -> Self { - match n { + match n { Network::Dash => FFINetwork::Dash, Network::Testnet => FFINetwork::Testnet, Network::Regtest => FFINetwork::Regtest, - Network::Devnet => FFINetwork::Devnet, - _ => FFINetwork::Dash, // Default to Dash for unknown networks + Network::Devnet => FFINetwork::Devnet, } } }
🧹 Nitpick comments (10)
key-wallet/Cargo.toml (1)
42-44: Dev-dep duplication: consider removing hex from [dev-dependencies]Now that hex is a normal dependency, the separate dev-dependency is redundant and can be dropped to reduce noise and avoid future feature-drift. Keep it only if tests need a different feature set. If not, remove line 43.
Apply this diff:
-[dev-dependencies] -hex = "0.4" -key-wallet = { path = ".", features = ["bip38", "serde", "bincode"] } +[dev-dependencies] +key-wallet = { path = ".", features = ["bip38", "serde", "bincode"] }If tests require std-only helpers from hex while library is built no_std, you can keep the dev-dep but pin features explicitly:
[dev-dependencies] hex = { version = "0.4", features = ["std"] }key-wallet-ffi/src/types.rs (4)
58-61: Limit mutable access to wallets across the FFI surface.To align with the wallet-manager design (read-only references via const pointers, mutations via dedicated APIs), avoid exporting broad mutable access. Make this method crate-private (or remove if unused).
- pub fn inner_mut(&mut self) -> Option<&mut Wallet> { + pub(crate) fn inner_mut(&mut self) -> Option<&mut Wallet> { Arc::get_mut(&mut self.wallet) }If any externs rely on mutation, consider introducing targeted mutation APIs instead of exposing
inner_mut.
64-99: Inconsistent error pattern; provide a standard string-free or use FFIError.
FFIAccountResultembeds an error_code and allocateserror_messageviaCString::into_raw(), but there’s no obvious, module-agnostic function to free it (the only string-free in this PR isderivation_string_free, which is mis-scoped for generic use). Either:
- Switch this struct to carry a full
FFIErroror a pointer to one, or- Provide a globally named
ffi_string_free(orkey_wallet_ffi_string_free) and declare it in the public header.I can prepare a small patch that introduces
ffi_string_freein a shared module and re-exports it for C consumers, plus add the header declaration.
341-349: Safety docs mention stale field names.The safety section references
account_indiceswhich doesn’t exist anymore. Update the docs to reference the actual pointer/count pairs (bip44_indices/bip44_count, etc.).
119-126: FFIStandardAccountType appears unused here.If this is meant for the public header, fine. If not, consider removing to reduce surface area. Otherwise, ensure it’s declared in the C header and used by consumers.
key-wallet-ffi/src/derivation.rs (5)
84-137: Path-to-buffer routines are correct but repetitive; consider a small helper.Six nearly identical functions allocate a CString, length-check, and copy. A local helper like
fn write_path(path: DerivationPath, out: *mut c_char, cap: usize, error: *mut FFIError) -> boolwould reduce duplication and the chance of drift.Also applies to: 139-195, 197-250, 252-305, 307-362, 364-423
508-515: Doc fix: incorrect free function name.“The returned pointer must be freed with
extended_public_key_free” doesn’t exist. The function isderivation_xpub_free.-/// - The returned pointer must be freed with `extended_public_key_free` +/// - The returned pointer must be freed with `derivation_xpub_free`
546-551: Doc fix: string free function name.Use
derivation_string_freeto match what’s exported.-/// - The returned string must be freed with `string_free` +/// - The returned string must be freed with `derivation_string_free`
588-593: Doc fix: string free function name (xpub).Same here; reference
derivation_string_free.-/// - The returned string must be freed with `string_free` +/// - The returned string must be freed with `derivation_string_free`
694-699: Provide a crate-wide, generic string free for consistency.
derivation_string_freeis narrowly named but useful across the FFI. Consider re-exporting it under a generic name (e.g.,ffi_string_free) and documenting that all C strings from this crate must be freed with it.
📜 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 (13)
key-wallet-ffi/src/address.rs(1 hunks)key-wallet-ffi/src/address_tests.rs(1 hunks)key-wallet-ffi/src/derivation.rs(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/managed_wallet.rs(1 hunks)key-wallet-ffi/src/types.rs(1 hunks)key-wallet-ffi/src/wallet_manager.rs(1 hunks)key-wallet-ffi/src/wallet_manager_tests.rs(1 hunks)key-wallet-ffi/tests/integration_test.rs(1 hunks)key-wallet-ffi/tests/test_addr_simple.rs(1 hunks)key-wallet-ffi/tests/test_improved_watch_only.rs(1 hunks)key-wallet/Cargo.toml(1 hunks)key-wallet/src/wallet/helper.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- key-wallet-ffi/tests/test_improved_watch_only.rs
- key-wallet-ffi/tests/test_addr_simple.rs
- key-wallet-ffi/tests/integration_test.rs
- key-wallet-ffi/src/address.rs
- key-wallet-ffi/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/wallet/helper.rskey-wallet-ffi/src/types.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
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/wallet/helper.rskey-wallet-ffi/src/types.rs
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/types.rs
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/wallet/helper.rskey-wallet-ffi/src/types.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rs
📚 Learning: 2025-08-21T05:01:58.919Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/managed_wallet.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
key-wallet-ffi/src/address_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rs
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
key-wallet-ffi/src/derivation.rskey-wallet/src/wallet/helper.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Accept input strings from C as *const c_char (borrowed; do not free inputs)
Applied to files:
key-wallet-ffi/src/managed_wallet.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Applied to files:
key-wallet-ffi/src/managed_wallet.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/**/FFIBridge.swift : Add a Swift wrapper in FFIBridge.swift for each new FFI function, handling type conversions and cross-language error mapping
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
key-wallet-ffi/src/types.rs
🧬 Code graph analysis (7)
key-wallet-ffi/src/address_tests.rs (2)
key-wallet-ffi/src/address.rs (4)
address_array_free(37-50)address_free(20-26)address_get_type(129-189)address_validate(59-114)key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)
key-wallet-ffi/src/wallet_manager_tests.rs (5)
key-wallet-ffi/src/types.rs (4)
error(86-97)success(77-83)new(47-51)new(107-111)key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)key-wallet-ffi/src/wallet_manager.rs (15)
wallet_manager_create(28-35)wallet_manager_free(919-925)wallet_manager_add_wallet_from_mnemonic(186-201)wallet_manager_get_wallet_ids(213-256)wallet_manager_free_wallet_ids(936-942)wallet_manager_get_receive_address(389-461)wallet_manager_get_change_address(472-544)wallet_manager_get_wallet_balance(559-616)wallet_manager_get_monitored_addresses(715-791)wallet_manager_free_addresses(954-965)wallet_manager_current_height(840-873)wallet_manager_update_height(801-830)wallet_manager_get_wallet(270-318)wallet_manager_process_transaction(634-703)wallet_manager_get_managed_wallet_info(332-378)key-wallet-ffi/src/managed_wallet.rs (2)
new(24-28)managed_wallet_info_free(722-728)key-wallet-ffi/src/wallet.rs (1)
wallet_free(587-593)
key-wallet-ffi/src/derivation.rs (3)
key-wallet-ffi/src/types.rs (7)
error(86-97)new(47-51)new(107-111)from(18-25)from(29-37)from(259-265)from(269-274)key-wallet-ffi/src/error.rs (5)
error(43-48)set_error(52-58)set_success(62-68)from(129-154)from(159-193)key-wallet/src/bip32.rs (8)
bip_44_account(964-974)bip_44_payment_path(975-998)coinjoin_path(999-1009)identity_registration_path(1024-1034)identity_top_up_path(1036-1051)identity_authentication_path(1065-1088)from_priv(1731-1743)from_hardened_idx(614-622)
key-wallet-ffi/src/wallet_manager.rs (3)
key-wallet-ffi/src/types.rs (3)
error(86-97)new(47-51)new(107-111)key-wallet-ffi/src/error.rs (3)
error(43-48)set_success(62-68)set_error(52-58)key-wallet-ffi/src/managed_wallet.rs (1)
new(24-28)
key-wallet-ffi/src/managed_wallet.rs (7)
key-wallet-ffi/src/types.rs (11)
error(86-97)new(47-51)new(107-111)inner(54-56)inner(114-116)inner_mut(59-61)success(77-83)from(18-25)from(29-37)from(259-265)from(269-274)key-wallet-ffi/src/error.rs (4)
error(43-48)set_error(52-58)set_success(62-68)success(35-40)key-wallet/src/account/managed_account.rs (1)
new(50-67)key-wallet/src/account/address_pool.rs (6)
new(84-99)new(148-167)new(614-623)network(644-647)gap_limit(638-641)default(675-677)key-wallet/src/wallet/managed_wallet_info/mod.rs (2)
new(49-59)from_wallet(75-94)key-wallet-ffi/src/wallet.rs (2)
wallet_create_from_mnemonic(144-157)wallet_free(587-593)key-wallet-ffi/src/address.rs (1)
address_array_free(37-50)
key-wallet/src/wallet/helper.rs (4)
key-wallet/src/bip32.rs (4)
master(1273-1275)new_master(1447-1460)child(1284-1288)from_priv(1731-1743)key-wallet/src/wallet/root_extended_keys.rs (3)
new_master(29-58)new(21-26)new(183-188)key-wallet/src/account/account_collection.rs (1)
new(46-60)key-wallet/src/derivation.rs (1)
child(258-261)
key-wallet-ffi/src/types.rs (3)
key-wallet-ffi/src/error.rs (4)
from(129-154)from(159-193)success(35-40)error(43-48)dash-network-ffi/src/lib.rs (3)
from(20-27)from(31-39)new(55-59)key-wallet/src/account/mod.rs (2)
new(61-74)index(106-108)
🔇 Additional comments (62)
key-wallet-ffi/src/address_tests.rs (6)
11-46: LGTM! Address validation tests are comprehensive.The test covers valid/invalid addresses and null pointer handling with appropriate error codes.
48-71: LGTM! Well-structured test cases.These tests properly exercise the FFI boundary with valid/invalid inputs and verify error codes.
73-82: LGTM! Null pointer validation is handled correctly.The test ensures null inputs are gracefully handled with the appropriate InvalidInput error code.
83-133: LGTM! Comprehensive address type tests.The tests properly handle all edge cases including valid addresses, invalid formats, and null inputs with appropriate error codes.
153-159: LGTM! Null safety test is correct.The function gracefully handles null inputs without crashing.
161-204: LGTM! Thorough validation tests.The comprehensive test suite covers various invalid address formats and validates network prefixes correctly.
key-wallet-ffi/src/wallet_manager_tests.rs (20)
1-38: LGTM! Basic wallet manager lifecycle test.The test correctly verifies manager creation, initial state, and cleanup.
39-72: LGTM! Wallet addition test is correct.Successfully tests adding a wallet with mnemonic and passphrase, verifying the count increases.
74-150: LGTM! Comprehensive multi-wallet test.The test properly validates adding multiple wallets, retrieving unique IDs, and verifying uniqueness across wallets. Good error handling for failed additions.
152-234: LGTM! Address generation tests are thorough.The tests verify receive address generation, uniqueness across calls, and proper memory cleanup.
236-290: LGTM! Change address generation is properly tested.The test follows the same pattern as receive addresses with proper cleanup.
292-352: LGTM! Balance query test is correct.The test properly verifies balance retrieval for a new wallet (expecting zero balance) and includes cleanup.
354-453: LGTM! Monitored addresses test is comprehensive.The test verifies empty initial state, generates addresses, and properly cleans up allocated memory.
455-490: LGTM! Height management tests are correct.Tests verify initial height, updates, and persistence of the updated value.
492-525: LGTM! Error handling tests are well-designed.The tests validate null manager handling and invalid mnemonic errors with appropriate error codes.
527-599: LGTM! Multi-wallet management test is thorough.The test verifies adding multiple wallets, retrieving all IDs, and generating addresses for each wallet with proper cleanup.
601-632: LGTM! Account creation test is straightforward.Tests wallet creation with account count and verifies successful addition.
634-686: LGTM! Wallet retrieval test is correct.The test verifies wallet retrieval and properly casts const pointer for cleanup, which aligns with the learning about read-only wallet references.
688-750: LGTM! Change address retrieval test is comprehensive.Tests change address generation with proper null checks and cleanup.
752-812: LGTM! Balance retrieval test is well-structured.Verifies balance retrieval for a new wallet expecting zero balances with proper cleanup.
816-854: LGTM! Null input validation is thorough.Tests various null manager scenarios with appropriate error code verification.
856-862: LGTM! Null handling test is correct.Verifies that freeing null doesn't crash.
864-898: LGTM! Height operations test is complete.Tests height retrieval and update operations with verification.
900-1018: LGTM! Comprehensive balance implementation test.Tests various scenarios including successful retrieval, null manager, null wallet ID, null output pointers, and invalid wallet ID with appropriate error codes.
1020-1185: LGTM! Transaction processing tests are thorough.The tests cover multiple transaction contexts (mempool, block, chain-locked), null pointer validation, invalid transaction data, and appropriate error handling. The simplified transaction format is adequate for testing deserialization failures.
1187-1294: LGTM! Wallet and info retrieval tests are comprehensive.Tests wallet and managed wallet info retrieval, invalid wallet IDs, null manager scenarios, and proper cleanup of all allocated resources.
key-wallet-ffi/src/wallet_manager.rs (12)
1-35: LGTM! Clean FFI wrapper structure.The FFIWalletManager struct properly wraps the internal WalletManager with mutexes for thread safety and tracks wallet IDs for FFI purposes.
37-174: LGTM! Well-structured wallet addition with options.The function properly validates inputs, handles UTF-8 conversion, generates deterministic wallet IDs from mnemonic+passphrase hash, checks for duplicates, and manages error states correctly.
176-201: LGTM! Clean backward compatibility wrapper.The function delegates to the full implementation with null options for default behavior.
258-318: LGTM! Wallet retrieval with const pointer is correct.The function properly returns a const pointer to enforce read-only access, which aligns with the design principle that wallet manager controls wallet lifecycle. This matches the confirmed learning about read-only wallet references.
320-378: LGTM! Managed wallet info retrieval is well-implemented.The function follows the same pattern as wallet retrieval, properly boxing the managed wallet info and returning a mutable pointer for the caller to own and free.
380-461: LGTM! Receive address generation is comprehensive.The function properly handles wallet ID conversion, uses the wallet manager's public API with appropriate preferences (BIP44, mark_as_used), and converts addresses to C strings with error handling.
463-544: LGTM! Change address generation mirrors receive address.The implementation correctly follows the same pattern as receive address generation with appropriate change-specific logic.
546-616: LGTM! Balance retrieval using u64 is correct.The function properly uses u64 for balance outputs (avoiding platform-dependent c_ulong issues) and directly assigns balance values without unnecessary casting.
618-703: LGTM! Transaction processing is well-implemented.The function properly deserializes transactions, converts FFI context to native context, processes through all wallets, and returns whether any wallet was affected.
793-873: LGTM! Height management functions are correct.Both update and current height functions properly handle locking, network conversion, and error states.
875-909: LGTM! Wallet count retrieval is straightforward.Properly locks the manager and returns the count with error handling.
911-925: LGTM! Manager cleanup is correct.Properly frees the boxed FFIWalletManager.
key-wallet-ffi/src/managed_wallet.rs (11)
1-37: LGTM! Clean wrapper structure.The FFIManagedWalletInfo wrapper properly encapsulates ManagedWalletInfo with appropriate accessors.
39-157: LGTM! Comprehensive receive address generation.The function properly validates inputs, navigates the account hierarchy, retrieves the extended public key, and generates the next receive address with proper error handling.
159-277: LGTM! Change address generation mirrors receive address.The implementation correctly follows the same pattern with change-specific logic.
279-459: Excellent fix for the allocation/deallocation issue!The function now properly converts the Vec to a boxed slice and uses
Box::into_rawfor symmetric allocation. This ensures the memory layout matches whataddress_array_freeexpects.
461-641: LGTM! Internal address range properly uses boxed slice.The implementation correctly follows the same fixed pattern as external addresses.
643-698: LGTM! Balance retrieval is straightforward.The function properly validates inputs and returns all balance components.
700-728: LGTM! Free functions are correct.Both free functions properly reconstruct and drop the boxed structures.
730-924: LGTM! Comprehensive test coverage.The tests thoroughly cover null pointer handling, address generation, and memory management.
926-1111: LGTM! Excellent comprehensive address generation test.The test properly sets up managed accounts with address pools and verifies all address generation functions. Importantly, it correctly uses
address_array_freefor cleanup instead oflibc::free.
1113-1206: LGTM! Balance test is well-structured.The test properly sets up test balance values, verifies retrieval, and tests error conditions.
1208-1249: LGTM! Null output validation tests.The tests properly verify that null output pointers are rejected with appropriate error codes.
key-wallet/src/wallet/helper.rs (9)
14-18: LGTM! Clean public API addition.The
accounts_on_networkmethod provides a useful accessor for network-specific account collections.
157-317: LGTM! Well-designed passphrase validation.The function properly validates that non-passphrase wallets cannot use this method and handles all account creation options correctly.
319-480: LGTM! Comprehensive passphrase-aware account creation.The implementation correctly mirrors the non-passphrase version while using passphrase-enabled account creation methods throughout.
502-528: LGTM! Special accounts with passphrase support.The helper correctly creates all special purpose accounts using the passphrase.
530-583: LGTM! Flexible extended private key derivation.The function properly handles all wallet types including passphrase-protected wallets and correctly rejects watch-only wallets.
585-653: LGTM! Convenient derivation helpers.The helper methods provide clean APIs for deriving private keys in various formats with proper error handling.
655-694: LGTM! Smart hardened/non-hardened path handling.The function correctly handles hardened paths by deriving through private keys when available, and non-hardened paths through public key derivation for watch-only compatibility.
696-732: LGTM! Consistent public key derivation.The implementation properly mirrors the extended public key logic for regular public keys.
734-755: LGTM! Hex encoding is correctly implemented.The function properly serializes the compressed public key and hex-encodes it, which resolves the compilation issue mentioned in past reviews.
key-wallet-ffi/src/types.rs (1)
452-563: Transaction context conversion looks solid.Null handling for
block_hashandtimestampis correct; copies are bounded to 32 bytes. No FFI footguns spotted here.key-wallet-ffi/src/derivation.rs (3)
44-82: Master key creation: good FFI hygiene.Null checks, safe slice creation, and error propagation via
FFIErrorlook correct.
636-656: Fingerprint extraction is correct and bounded.Null checks, 4-byte copy, and success signaling look good.
708-863: DIP9 derivation: robust error handling, no panics, correct base-path selection.Good: avoids
unwrap, validates hardened indices, and returns detailedFFIErrors. Base-path selection for mainnet vs. test networks is explicit. No FFI safety issues found.
| bincode_derive = { version = "=2.0.0-rc.3", optional = true } | ||
| base64 = { version = "0.22", optional = true } | ||
| serde_json = { version = "1.0", optional = true } | ||
| hex = { version = "0.4"} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Preserve no_std: don’t pull in hex’s std by default
hex enables its std feature by default. As-is, this makes key-wallet always depend on std even when consumers disable this crate’s "std" feature, breaking no_std builds. Configure hex with default-features = false and enable alloc; then wire its std feature into this crate’s "std" feature (next comment). The hex docs explicitly recommend this for no_std. (docs.rs)
Apply this diff:
-hex = { version = "0.4"}
+hex = { version = "0.4", default-features = false, features = ["alloc"] }Nit: keep spacing consistent with other entries (add a space before the closing brace as shown above).
Run to sanity-check the dependency line across the repo:
🏁 Script executed:
#!/bin/bash
# Expect exactly one line configuring hex with default-features=false and alloc.
rg -nP '^\s*hex\s*=\s*\{[^}]*version\s*=\s*"0\.4"[^}]*\}' -C0Length of output: 291
🏁 Script executed:
#!/bin/bash
# Display the first 80 lines of key-wallet/Cargo.toml for context
sed -n '1,80p' key-wallet/Cargo.tomlLength of output: 2345
🏁 Script executed:
#!/bin/bash
# Extract the [features] section from dash/Cargo.toml for review
awk '/^\[features\]/,/^\[/{ if (!/^\[features\]/ || !found++) print; if (/^\[features\]/) found=0 }' dash/Cargo.tomlLength of output: 129
Preserve no_std in key-wallet: disable hex’s default std feature
hex enables its std feature by default, which means that, as written, key-wallet will always pull in std even when its own "std" feature is disabled—breaking any downstream no_std builds. To fix this, update the dependency at line 40 of key-wallet/Cargo.toml as follows (note the added space before the closing brace to match existing formatting):
-[dependencies]
-hex = { version = "0.4"}
+hex = { version = "0.4", default-features = false, features = ["alloc"] }This ensures that, under key-wallet’s default configuration, only hex’s alloc-based functionality is enabled, and that its std feature can be opt-in by wiring it into the crate’s "std" feature.
(You may leave the dev-dependencies entry for hex = "0.4" unchanged, since tests generally require std.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-wallet/Cargo.toml around line 40, the hex dependency currently enables
std by default; change the dependency to disable default features and explicitly
enable alloc (e.g. set version "0.4" with default-features = false and features
= ["alloc"]) and then wire hex’s "std" feature into the crate’s "std" feature in
the [features] section so that hex::std is opt-in; leave the dev-dependencies
hex = "0.4" entry unchanged.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
key-wallet-ffi/Cargo.toml (1)
21-23: Dependencies align with C-ABI migrationThe switch from dash-network-ffi to direct dependencies on key-wallet-manager, dashcore, and dash-network is appropriate for the new C-ABI approach. These path dependencies provide the necessary wallet management and network functionality.
key-wallet-ffi/src/lib.rs (1)
44-51: Version API now returns a static string — resolves prior leak/mismatchReturning a static, NUL-terminated string via concat!(..., "\0") is correct and matches the header’s “do not free” contract. This fixes the earlier concern about leaking CString allocations across calls.
key-wallet-ffi/src/wallet_manager.rs (2)
547-566: Balances use u64 across the FFI — portable and Windows‑safeSwitching away from c_ulong removes LLP64 truncation risk. Header and implementation are consistent.
Also applies to: 598-616
937-944: Correct boxed-slice reconstruction for wallet IDs — resolves prior UBUsing Box::from_raw on a DST pointer from ptr::slice_from_raw_parts_mut matches the Box<[u8]> allocation in the producer. Good fix.
🧹 Nitpick comments (12)
key-wallet-ffi/src/utxo.rs (1)
127-132: Simplify confirmations calculationThe current logic only distinguishes between 0 and 1 confirmations, but the field name suggests it should contain the actual confirmation count.
Consider calculating actual confirmations if block height information is available:
- // Calculate confirmations (0 if unconfirmed) - let confirmations = if utxo.is_confirmed { - 1 - } else { - 0 - }; + // Calculate confirmations (0 if unconfirmed) + let confirmations = if utxo.is_confirmed { + // TODO: Calculate actual confirmations from current block height - utxo.height + 1 // Placeholder: should be actual confirmation count + } else { + 0 + };key-wallet-ffi/src/lib.rs (1)
37-42: Optional: make initialization explicitly idempotent and future‑proofIf you expect to add real initialization later, consider guarding it with OnceCell/OnceLock to guarantee idempotency even under concurrent calls. No functional issue today.
use std::os::raw::c_char; +use std::sync::OnceLock; /// Initialize the library #[no_mangle] pub extern "C" fn key_wallet_ffi_initialize() -> bool { - // Any global initialization - true + static INIT: OnceLock<()> = OnceLock::new(); + let _ = INIT.get_or_init(|| ()); + true }key-wallet-ffi/src/wallet_manager.rs (6)
93-105: Wallet ID derivation scheme: document intent and stability guaranteesDeriving wallet_id as SHA‑256(mnemonic || passphrase) is deterministic and stable, but it ties identity to secrets. If that’s by design, add a brief comment stating that the ID is an internal manager key, not meant for external identity, and is stable across imports. Otherwise, consider namespacing the hash input to avoid accidental collisions with other schemes.
- // Generate wallet ID from mnemonic + passphrase + // Generate wallet ID from mnemonic + passphrase. + // Note: This ID is an internal WalletManager key (stable across imports), + // not intended as an external/public identity. use sha2::{Digest, Sha256};
119-127: Use a more specific error code for “already exists” (optional)Currently returns WalletError. If practical, consider introducing/using a dedicated AlreadyExists code to let callers distinguish conflicts without string matching.
242-253: Clarify wallet_ids buffer layout in the C contractYou return a flat buffer of count*32 bytes. Explicitly document this in the Rust docstring (picked up by cbindgen) so C callers don’t assume an array of pointers.
/// - `wallet_ids_out` must be a valid pointer to a pointer that will receive the wallet IDs -/// - `count_out` must be a valid pointer to receive the count +/// - `count_out` must be a valid pointer to receive the count +/// +/// Memory layout: +/// - On success, `*wallet_ids_out` points to a contiguous buffer of `*count_out * 32` bytes. +/// - The i-th wallet ID starts at offset `i * 32`. +/// - Free with `wallet_manager_free_wallet_ids(*wallet_ids_out, *count_out)`.
464-545: Nit: avoid repeatedly constructing Secp256k1Constructing Secp256k1 for each call is cheap but not free. Consider reusing a static once-initialized context if this path becomes hot.
635-705: Process-transaction path: robust validation and erroring; consider documenting context nullabilityDeserialize errors are reported properly. Add a brief note in the doc about when context.block_hash may be null and how timestamp=0 is interpreted (header already hints at this).
716-793: Deduplicate address-array free logic: reuse address_array_free to avoid driftThis module defines wallet_manager_free_addresses while address.rs already provides address_array_free. Prefer a single canonical free to reduce maintenance and keep allocation/freeing patterns aligned.
key-wallet-ffi/include/key_wallet_ffi.h (4)
1003-1015: Outdated doc: derivation_path_free no longer “leaks memory”Doc still claims a leak due to missing count, but the signature now includes count. Update the comment to avoid confusing consumers.
-/* - Free derivation path arrays - Note: This function expects the count to properly free the slices - For now, we're leaking memory as we don't have the count parameter -*/ +/* + Free derivation path arrays + Requires the original `count` returned by derivation_path_parse. +*/Note: change in Rust doc comments so cbindgen regenerates correctly.
1660-1664: Document wallet_ids buffer layout explicitlyState that the returned buffer is contiguous with 32-byte IDs back-to-back and must be freed with wallet_manager_free_wallet_ids(ids, count).
Also applies to: 1871-1871
18-25: Nit: duplicate includes<stdint.h> and <stdbool.h> are included twice. Not harmful, but tidy up for cleanliness.
1801-1806: Unify address array free APIYou expose both address_array_free and wallet_manager_free_addresses. Prefer a single canonical free (address_array_free) and have all producers document that as the matching free. This avoids confusion and divergence.
Also applies to: 1885-1887, 494-505
📜 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 (9)
key-wallet-ffi/Cargo.toml(1 hunks)key-wallet-ffi/build.rs(1 hunks)key-wallet-ffi/cbindgen.toml(1 hunks)key-wallet-ffi/generate_header.sh(1 hunks)key-wallet-ffi/include/key_wallet_ffi.h(1 hunks)key-wallet-ffi/src/keys.rs(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/utxo.rs(1 hunks)key-wallet-ffi/src/wallet_manager.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/src/keys.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet-ffi/build.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/utxo.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
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet-ffi/build.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/utxo.rs
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/build.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/include/key_wallet_ffi.h
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/build.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/utxo.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Applied to files:
key-wallet-ffi/build.rskey-wallet-ffi/cbindgen.toml
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: When adding new FFI functions in the Rust layer, implement them with #[no_mangle] extern "C" and proper cbindgen annotations
Applied to files:
key-wallet-ffi/build.rskey-wallet-ffi/cbindgen.toml
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
Applied to files:
key-wallet-ffi/generate_header.sh
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h : After updating dash-spv-ffi, synchronize the generated header into the Swift SDK using ./sync-headers.sh so Sources/DashSPVFFI/include/dash_spv_ffi.h stays up to date
Applied to files:
key-wallet-ffi/generate_header.sh
📚 Learning: 2025-08-21T05:01:58.919Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/utxo.rs
📚 Learning: 2025-06-26T15:48:36.342Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:48:36.342Z
Learning: In Rust FFI code, unwrap() must not be used on CString::new, especially for error messages or untrusted input, as it can panic if the string contains null bytes. Instead, use unwrap_or_else with a fallback to a known-safe, hardcoded string to prevent panics across FFI boundaries.
Applied to files:
key-wallet-ffi/src/lib.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Accept input strings from C as *const c_char (borrowed; do not free inputs)
Applied to files:
key-wallet-ffi/src/lib.rskey-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
key-wallet-ffi/src/lib.rskey-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
key-wallet-ffi/src/utxo.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
🧬 Code graph analysis (4)
key-wallet-ffi/src/lib.rs (2)
key-wallet-ffi/src/types.rs (1)
error(86-97)key-wallet-ffi/src/error.rs (1)
error(43-48)
key-wallet-ffi/src/wallet_manager.rs (4)
key-wallet-ffi/src/types.rs (3)
error(86-97)new(47-51)new(107-111)key-wallet-ffi/src/error.rs (3)
error(43-48)set_success(62-68)set_error(52-58)key-wallet-ffi/src/managed_wallet.rs (1)
new(24-28)key-wallet-manager/src/wallet_manager/mod.rs (2)
new(68-74)new(102-108)
key-wallet-ffi/src/utxo.rs (3)
key-wallet-ffi/src/error.rs (3)
error(43-48)set_error(52-58)set_success(62-68)key-wallet-ffi/src/managed_wallet.rs (1)
new(24-28)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
utxos(65-65)utxos(177-188)
key-wallet-ffi/include/key_wallet_ffi.h (16)
key-wallet-ffi/src/lib.rs (2)
key_wallet_ffi_initialize(39-42)key_wallet_ffi_version(48-51)key-wallet-ffi/src/account.rs (5)
wallet_get_account(16-76)wallet_get_top_up_account_with_registration_index(87-121)account_free(131-135)account_result_free_error(146-154)wallet_get_account_count(164-192)key-wallet-ffi/src/types.rs (1)
error(86-97)key-wallet-ffi/src/error.rs (2)
error(43-48)error_message_free(79-83)key-wallet-ffi/src/address.rs (4)
address_free(20-26)address_array_free(37-50)address_validate(59-114)address_get_type(129-189)key-wallet-ffi/src/balance.rs (2)
wallet_get_balance(42-62)wallet_get_account_balance(73-107)key-wallet-ffi/src/derivation.rs (16)
derivation_new_master_key(52-82)derivation_bip44_account_path(86-137)derivation_bip44_payment_path(141-195)derivation_coinjoin_path(199-250)derivation_identity_registration_path(254-305)derivation_identity_topup_path(309-362)derivation_identity_authentication_path(366-423)derivation_derive_private_key_from_seed(434-506)derivation_xpriv_to_xpub(516-542)derivation_xpriv_to_string(552-584)derivation_xpub_to_string(594-626)derivation_xpub_fingerprint(636-656)derivation_xpriv_free(666-670)derivation_xpub_free(680-684)derivation_string_free(694-698)dip9_derive_identity_key(708-863)key-wallet-ffi/src/keys.rs (16)
wallet_get_account_xpriv(41-83)wallet_get_account_xpub(93-130)wallet_derive_private_key(142-204)wallet_derive_extended_private_key(216-278)wallet_derive_private_key_as_wif(289-359)private_key_free(368-374)extended_private_key_free(383-389)private_key_to_wif(399-435)wallet_derive_public_key(447-509)wallet_derive_extended_public_key(521-583)wallet_derive_public_key_as_hex(594-664)public_key_free(673-679)extended_public_key_free(688-694)public_key_to_hex(704-731)derivation_path_parse(744-827)derivation_path_free(840-855)key-wallet-ffi/src/managed_wallet.rs (7)
managed_wallet_get_next_bip44_receive_address(51-157)managed_wallet_get_next_bip44_change_address(171-277)managed_wallet_get_bip_44_external_address_range(293-459)managed_wallet_get_bip_44_internal_address_range(475-641)managed_wallet_get_balance(656-698)managed_wallet_free(707-713)managed_wallet_info_free(722-728)key-wallet-ffi/src/mnemonic.rs (6)
mnemonic_generate(73-123)mnemonic_generate_with_language(127-183)mnemonic_validate(192-242)mnemonic_to_seed(254-330)mnemonic_word_count(339-365)mnemonic_free(374-380)key-wallet-ffi/src/transaction.rs (4)
wallet_build_transaction(27-59)wallet_sign_transaction(72-100)wallet_check_transaction(139-246)transaction_bytes_free(255-261)key-wallet-ffi/src/utils.rs (1)
string_free(17-23)key-wallet-ffi/src/utxo.rs (3)
managed_wallet_get_utxos(91-156)wallet_get_utxos(166-184)utxo_array_free(195-208)key-wallet-ffi/src/wallet.rs (16)
wallet_create_from_mnemonic_with_options(29-132)wallet_create_from_mnemonic(144-157)wallet_create_from_seed_with_options(168-217)wallet_create_from_seed(227-240)wallet_create_from_seed_bytes(251-298)wallet_create_from_xpub(308-397)wallet_create_random_with_options(407-436)wallet_create_random(445-454)wallet_get_id(465-481)wallet_has_mnemonic(491-505)wallet_is_watch_only(515-529)wallet_get_xpub(540-577)wallet_free(587-593)wallet_add_account(604-677)wallet_add_account_with_xpub_bytes(689-790)wallet_add_account_with_string_xpub(802-901)key-wallet-ffi/src/wallet_manager.rs (17)
wallet_manager_create(28-35)wallet_manager_add_wallet_from_mnemonic_with_options(48-174)wallet_manager_add_wallet_from_mnemonic(186-201)wallet_manager_get_wallet_ids(213-257)wallet_manager_get_wallet(271-319)wallet_manager_get_managed_wallet_info(333-379)wallet_manager_get_receive_address(390-462)wallet_manager_get_change_address(473-545)wallet_manager_get_wallet_balance(560-617)wallet_manager_process_transaction(635-704)wallet_manager_get_monitored_addresses(716-793)wallet_manager_update_height(803-832)wallet_manager_current_height(842-875)wallet_manager_wallet_count(885-911)wallet_manager_free(921-927)wallet_manager_free_wallet_ids(938-945)wallet_manager_free_addresses(957-968)key-wallet-ffi/src/bip38.rs (2)
bip38_encrypt_private_key(12-75)bip38_decrypt_private_key(79-141)
⏰ 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). (15)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (15)
key-wallet-ffi/build.rs (1)
7-41: LGTM! Clean migration from UniFFI to cbindgenThe build script correctly handles platform-specific linking for iOS/macOS and generates C headers using cbindgen. The implementation follows best practices with proper error handling and directory creation.
key-wallet-ffi/cbindgen.toml (1)
1-79: LGTM! Well-structured cbindgen configurationThe configuration correctly sets up C header generation with appropriate includes, type mappings, and formatting options. The enum and struct configurations align with C conventions, and the opaque type handling is properly configured for the FFI boundary.
key-wallet-ffi/generate_header.sh (1)
1-127: LGTM! Comprehensive header generation with intelligent post-processingThe script provides excellent automation for header generation with proper error handling, colorized output, and intelligent post-processing to ensure opaque types are properly declared. The verification step adds confidence in the generated header's validity.
key-wallet-ffi/Cargo.toml (3)
17-17: LGTM! New BIP38 feature correctly configuredThe feature properly delegates to the underlying key-wallet crate's BIP38 support.
31-31: LGTM! Build dependency updated for cbindgenCorrectly replaced UniFFI build dependency with cbindgen for C header generation.
26-28: LGTM! Runtime and development dependencies properly configuredThe new dependencies (libc, sha2, hex, tempfile) support the C-ABI FFI operations and testing requirements.
Also applies to: 34-35
key-wallet-ffi/src/utxo.rs (6)
11-22: LGTM! Well-designed FFI-compatible UTXO structureThe
#[repr(C)]layout ensures binary compatibility with C code. The field types and ordering are appropriate for cross-language interop.
24-54: Memory management correctly implementedThe constructor properly converts Rust types to C-compatible raw pointers using
CString::into_rawandBox::into_raw, avoiding the previousstd::mem::forgetpattern that could cause memory leaks.
56-78: LGTM! Comprehensive safety documentation and proper cleanupThe
freemethod now includes thorough safety documentation and correctly reconstructs the original allocations for proper deallocation. The use ofstd::ptr::slice_from_raw_parts_mutproperly handles the dynamically-sized script allocation.
80-156: Well-implemented UTXO retrieval with proper error handlingThe function correctly validates inputs, handles the FFI boundary safely, and properly converts between internal and FFI types. The memory management using
Box::into_rawfor the UTXO array is appropriate and matches the cleanup expectations inutxo_array_free.
158-184: LGTM! Proper deprecation handlingThe deprecated function maintains backward compatibility while clearly directing users to the new API. The implementation correctly returns an empty result as documented.
186-208: LGTM! Proper cleanup implementationThe
utxo_array_freefunction correctly frees individual UTXO allocations before freeing the array itself. The safety documentation clearly explains the requirements and constraints.key-wallet-ffi/src/wallet_manager.rs (3)
48-92: Input validation for mnemonic/passphrase is solid (UTF‑8, null checks)Null and UTF‑8 error paths are handled cleanly with FFIError set on failure. Good defensive FFI surface.
281-313: Ownership of returned FFIWallet is clear; Arc inside wrapper avoids dangling refsCloning the internal wallet into an Arc and boxing the FFI wrapper is a solid pattern for handing ownership to the caller.
381-463: Address generation: good error propagation and allocation disciplineNull checks, lock handling, and CString allocation errors are handled correctly. Returning an owned C string with a dedicated free function is the right pattern.
| /// - `manager` must be a valid pointer to an FFIWalletManager instance | ||
| /// - `wallet_id` must be a valid pointer to a 32-byte wallet ID | ||
| /// - `error` must be a valid pointer to an FFIError structure or null | ||
| /// - The caller must ensure all pointers remain valid for the duration of this call | ||
| /// - The returned wallet must be freed with wallet_free() | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn wallet_manager_get_wallet( | ||
| manager: *const FFIWalletManager, | ||
| wallet_id: *const u8, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Const-return + mutable-free mismatch: provide a const-safe free to avoid casts
API returns *const FFIWallet (good: read‑only), but the doc says to free with wallet_free(), which takes *mut. This forces callers to cast away const. Add a wallet_free_const(const FFIWallet*) wrapper, update docs and header, and recommend using it for wallets retrieved via the manager.
-/// - The returned wallet must be freed with wallet_free()
+/// - The returned wallet must be freed with wallet_free_const()Additional change (outside this file, in key-wallet-ffi/src/wallet.rs):
#[no_mangle]
pub unsafe extern "C" fn wallet_free_const(wallet: *const crate::types::FFIWallet) {
if !wallet.is_null() {
let wallet_mut = wallet as *mut crate::types::FFIWallet;
let _ = Box::from_raw(wallet_mut);
}
}And update the header to declare:
void wallet_free_const(const FFIWallet *wallet);🤖 Prompt for AI Agents
In key-wallet-ffi/src/wallet_manager.rs around lines 265 to 273, the FFI returns
a *const FFIWallet but docs instruct callers to free it with wallet_free() which
accepts *mut, forcing unsafe const-to-mut casts; add a const-safe free wrapper
and update docs and header: create a new extern "C" function wallet_free_const
in key-wallet-ffi/src/wallet.rs that accepts *const FFIWallet and performs the
Box::from_raw cast internally, update the comment here to instruct callers to
use wallet_free_const for wallets returned as const, and add a matching
declaration in the public C header (void wallet_free_const(const FFIWallet
*wallet)); ensure symbol is #[no_mangle] and unsafe extern "C".
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/rust.yml (3)
35-41: Replace actions-rs/toolchain with dtolnay/rust-toolchain.actions-rs/toolchain is archived and flagged by linters; dtolnay/rust-toolchain is the recommended replacement. Drop profile/override (not used by dtolnay).
Apply this diff:
- - name: Checkout Toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: ${{ matrix.rust }} - override: true + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ matrix.rust }}
233-241: Upgrade checkout and migrate toolchain in RPC Tests.Use actions/checkout@v4 (Node20) and dtolnay/rust-toolchain to avoid deprecations.
Apply this diff:
- - name: Checkout Crate - uses: actions/checkout@v2 - - name: Checkout Toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: ${{ matrix.rust }} - override: true + - name: Checkout Crate + uses: actions/checkout@v4 + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ matrix.rust }}
262-270: Upgrade checkout and migrate toolchain in Integration Tests.Apply this diff:
- - name: Checkout Crate - uses: actions/checkout@v2 - - name: Checkout Toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: ${{ matrix.rust }} - override: true + - name: Checkout Crate + uses: actions/checkout@v4 + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ matrix.rust }}
♻️ Duplicate comments (2)
key-wallet-ffi/src/types.rs (2)
28-38: Do not silently map unknown networks to mainnetMapping an unknown
key_wallet::Networkvariant toFFINetwork::Dashis risky and can cause cross-network misuse without any signal. This contradicts the pattern used in dash-network-ffi which panics on unknown variants.Apply this diff to fail loudly (consistent with dash-network-ffi pattern):
impl From<Network> for FFINetwork { fn from(n: Network) -> Self { match n { Network::Dash => FFINetwork::Dash, Network::Testnet => FFINetwork::Testnet, Network::Regtest => FFINetwork::Regtest, Network::Devnet => FFINetwork::Devnet, - _ => FFINetwork::Dash, // Default to Dash for unknown networks + unknown => panic!("Unhandled Network variant {:?}", unknown), } } }
407-461: BIP32 indices are ignored in SpecificAccounts branchIn the
SpecificAccountsbranch, you build BIP44/CoinJoin/TopUp sets but readbip32_indiceswithout using them. The comment states thatStandardBIP32is mapped viaspecial_account_typeswith hardcoded index 0 (line 445), so arbitrary BIP32 indices frombip32_indicesare unreachable.Apply this diff to include BIP32 indices:
FFIAccountCreationOptionType::SpecificAccounts => { let mut bip44_set = BTreeSet::new(); if !self.bip44_indices.is_null() && self.bip44_count > 0 { let slice = std::slice::from_raw_parts(self.bip44_indices, self.bip44_count); bip44_set.extend(slice.iter().copied()); } let mut bip32_set = BTreeSet::new(); if !self.bip32_indices.is_null() && self.bip32_count > 0 { let slice = std::slice::from_raw_parts(self.bip32_indices, self.bip32_count); bip32_set.extend(slice.iter().copied()); } let mut coinjoin_set = BTreeSet::new(); if !self.coinjoin_indices.is_null() && self.coinjoin_count > 0 { let slice = std::slice::from_raw_parts(self.coinjoin_indices, self.coinjoin_count); coinjoin_set.extend(slice.iter().copied()); } let mut topup_set = BTreeSet::new(); if !self.topup_indices.is_null() && self.topup_count > 0 { let slice = std::slice::from_raw_parts(self.topup_indices, self.topup_count); topup_set.extend(slice.iter().copied()); } // Convert special account types if provided let special_accounts = if !self.special_account_types.is_null() && self.special_account_types_count > 0 { let slice = std::slice::from_raw_parts( self.special_account_types, self.special_account_types_count, ); let mut accounts = Vec::new(); for &ffi_type in slice { - // Use a dummy index for special accounts that don't need one - // Skip accounts that require parameters we don't have - if let Some(account_type) = ffi_type.to_account_type(0, None) { + // Skip BIP32/BIP44 types here as they're handled via the sets above + match ffi_type { + FFIAccountType::StandardBIP32 | FFIAccountType::StandardBIP44 => continue, + _ => { + // Use a dummy index for special accounts that don't need one + // Skip accounts that require parameters we don't have + if let Some(account_type) = ffi_type.to_account_type(0, None) { + accounts.push(account_type); + } + } + } - accounts.push(account_type); - } } Some(accounts) } else { None }; WalletAccountCreationOptions::SpecificAccounts( bip44_set, bip32_set, coinjoin_set, topup_set, special_accounts, ) }
🧹 Nitpick comments (3)
.github/workflows/fuzz.yml (1)
46-49: Avoid caching ~/.cargo/bin.Caching ~/.cargo/bin can persist stale binaries across runs and interfere with toolchain updates. Prefer caching registries/git and build artifacts only.
Apply this diff:
path: | ~/.cargo/bin fuzz/target target + # Consider removing ~/.cargo/bin from the cache to avoid stale binariesOr remove the ~/.cargo/bin line entirely:
path: | - ~/.cargo/bin fuzz/target target.github/workflows/rust.yml (2)
90-96: Auto-discover non-strict Clippy crates to avoid drift.Hard-coding crate names is brittle. Use cargo metadata to enumerate workspace members and exclude the strict-checked set.
Apply this diff:
- - name: Run clippy (excluding strict-checked crates) - run: | - # Run clippy on all workspace members except the ones with strict checks - # We allow warnings for these crates - for crate in dash dash-network dash-network-ffi hashes internals fuzz rpc-client rpc-json rpc-integration-test dash-spv dash-spv-ffi test-utils; do - echo "Checking $crate (warnings allowed)..." - cargo clippy -p $crate --all-features --all-targets || true - done + - name: Run clippy (excluding strict-checked crates) + run: | + STRICT_CRATES=("key-wallet" "key-wallet-manager" "key-wallet-ffi") + mapfile -t ALL_CRATES < <(cargo metadata --no-deps --format-version=1 | jq -r '.packages[].name' | sort -u) + for crate in "${ALL_CRATES[@]}"; do + if printf '%s\n' "${STRICT_CRATES[@]}" | grep -qx "$crate"; then + continue + fi + echo "Checking $crate (warnings allowed)..." + cargo clippy -p "$crate" --all-features --all-targets || true + done
44-44: Remove trailing spaces flagged by YAML linters.These lines contain trailing whitespace. Clean them to keep CI linters quiet and avoid noisy diffs.
Also applies to: 75-75, 97-97, 119-119, 128-128, 131-131, 140-140, 143-143, 152-152, 155-155
📜 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 (10)
.github/workflows/fuzz.yml(1 hunks).github/workflows/rust.yml(4 hunks)key-wallet-ffi/src/keys.rs(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/types.rs(1 hunks)key-wallet-ffi/src/wallet_manager_tests.rs(1 hunks)key-wallet-ffi/tests/integration_test.rs(1 hunks)key-wallet-ffi/tests/test_addr_simple.rs(1 hunks)key-wallet-ffi/tests/test_improved_watch_only.rs(1 hunks)key-wallet-ffi/tests/test_passphrase_wallets.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- key-wallet-ffi/tests/integration_test.rs
- key-wallet-ffi/src/lib.rs
- key-wallet-ffi/tests/test_addr_simple.rs
- key-wallet-ffi/tests/test_improved_watch_only.rs
- key-wallet-ffi/src/keys.rs
- key-wallet-ffi/tests/test_passphrase_wallets.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/types.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
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/types.rs
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/types.rs
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/types.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
📚 Learning: 2025-08-21T04:45:50.400Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.400Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rs
📚 Learning: 2025-08-21T05:01:58.919Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/types.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/**/FFIBridge.swift : Add a Swift wrapper in FFIBridge.swift for each new FFI function, handling type conversions and cross-language error mapping
Applied to files:
key-wallet-ffi/src/types.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
key-wallet-ffi/src/types.rs
🧬 Code graph analysis (2)
key-wallet-ffi/src/wallet_manager_tests.rs (4)
key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)key-wallet-ffi/src/wallet_manager.rs (16)
wallet_manager_create(28-35)wallet_manager_wallet_count(885-911)wallet_manager_free(921-927)wallet_manager_add_wallet_from_mnemonic(186-201)wallet_manager_get_wallet_ids(213-257)wallet_manager_free_wallet_ids(938-945)wallet_manager_get_receive_address(390-462)wallet_manager_get_change_address(473-545)wallet_manager_get_wallet_balance(560-617)wallet_manager_get_monitored_addresses(716-793)wallet_manager_free_addresses(957-968)wallet_manager_current_height(842-875)wallet_manager_update_height(803-832)wallet_manager_get_wallet(271-319)wallet_manager_process_transaction(635-704)wallet_manager_get_managed_wallet_info(333-379)key-wallet-ffi/src/managed_wallet.rs (2)
new(24-28)managed_wallet_info_free(722-728)key-wallet-ffi/src/wallet.rs (1)
wallet_free(587-593)
key-wallet-ffi/src/types.rs (3)
key-wallet-ffi/src/error.rs (4)
from(129-154)from(159-193)success(35-40)error(43-48)dash-network-ffi/src/lib.rs (3)
from(20-27)from(31-39)new(55-59)key-wallet/src/account/mod.rs (2)
new(61-74)index(106-108)
🪛 actionlint (1.7.7)
.github/workflows/rust.yml
56-56: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
83-83: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
105-105: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
163-163: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/rust.yml
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 97-97: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 128-128: trailing spaces
(trailing-spaces)
[error] 131-131: trailing spaces
(trailing-spaces)
[error] 140-140: trailing spaces
(trailing-spaces)
[error] 143-143: trailing spaces
(trailing-spaces)
[error] 152-152: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
⏰ 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: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (8)
.github/workflows/rust.yml (1)
29-32: Good: Add Rust 1.89.0 to the primary test matrix.Consistent with the repository’s rust-version and other jobs. This helps catch MSRV regressions early.
key-wallet-ffi/src/types.rs (4)
40-62: LGTM!Clean wrapper implementation with proper Arc usage and constructor methods.
64-98: LGTM!Well-structured FFI result type with proper error handling and C string conversion.
100-117: LGTM!Clean account wrapper implementation following the same Arc pattern as FFIWallet.
466-577: LGTM!Well-structured transaction context types with proper FFI-to-native conversion. The
to_transaction_contextmethod correctly handles null checks and byte array conversion for block hashes.key-wallet-ffi/src/wallet_manager_tests.rs (3)
1-37: LGTM!Well-structured test setup with proper manager creation and initial state verification.
327-344: LGTM!Proper alignment with u64 types for balance tracking, matching the FFI signature.
1284-1294: LGTM!Proper cleanup using the newly available
managed_wallet_info_freefunction and correct usage ofwallet_manager_free_wallet_ids.
.github/workflows/fuzz.yml
Outdated
| - uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: 1.85 | ||
| toolchain: 1.89 | ||
| override: true | ||
| profile: minimal | ||
| - name: fuzz |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Migrate off actions-rs/toolchain and pin patch version (align with rust.yml).
actions-rs/toolchain is archived and increasingly incompatible with current runners. Also, pin the toolchain to 1.89.0 (not 1.89) for reproducibility and to match the rest of CI.
Apply this diff:
- - uses: actions-rs/toolchain@v1
- with:
- toolchain: 1.89
- override: true
- profile: minimal
+ - uses: dtolnay/rust-toolchain@stable
+ with:
+ toolchain: 1.89.0📝 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.
| - uses: actions-rs/toolchain@v1 | |
| with: | |
| toolchain: 1.85 | |
| toolchain: 1.89 | |
| override: true | |
| profile: minimal | |
| - name: fuzz | |
| - uses: dtolnay/rust-toolchain@stable | |
| with: | |
| toolchain: 1.89.0 |
🧰 Tools
🪛 actionlint (1.7.7)
51-51: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/fuzz.yml lines 51-56: replace the archived
actions-rs/toolchain step with the maintained GitHub action for setting up Rust
(e.g., actions/setup-rust@v1) and pin the patch version to 1.89.0 for
reproducibility; specifically remove the actions-rs/toolchain@v1 step and add a
step using actions/setup-rust@v1 (or another maintained setup action used in
rust.yml) with rust-version: 1.89.0 and remove/adjust override/profile fields as
appropriate.
.github/workflows/rust.yml
Outdated
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| profile: minimal | ||
| toolchain: stable | ||
| override: true | ||
| components: rustfmt | ||
| - name: Check formatting |
There was a problem hiding this comment.
🛠️ Refactor suggestion
fmt job: migrate toolchain action.
Apply this diff:
- - name: Setup Rust toolchain
- uses: actions-rs/toolchain@v1
- with:
- profile: minimal
- toolchain: stable
- override: true
- components: rustfmt
+ - name: Setup Rust toolchain
+ uses: dtolnay/rust-toolchain@stable
+ with:
+ toolchain: stable
+ components: rustfmt📝 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.
| uses: actions-rs/toolchain@v1 | |
| with: | |
| profile: minimal | |
| toolchain: stable | |
| override: true | |
| components: rustfmt | |
| - name: Check formatting | |
| - name: Setup Rust toolchain | |
| uses: dtolnay/rust-toolchain@stable | |
| with: | |
| toolchain: stable | |
| components: rustfmt | |
| - name: Check formatting |
🧰 Tools
🪛 actionlint (1.7.7)
163-163: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/rust.yml around lines 163 to 169: the toolchain action block
currently ends with an extra deletion marker causing the next step name to be
malformed; update the actions-rs/toolchain step to include profile: minimal,
toolchain: stable, override: true and components: rustfmt as shown, and remove
the stray '-' before "name: Check formatting" so the following step is a normal
job step (correct indentation and YAML structure).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
key-wallet-ffi/src/wallet.rs (1)
361-373: Bug: watch‑only wallet constructed as mutable account — use Account::from_xpubCreating the account with
Account::new(...)setsis_watch_only = false. For a watch‑only wallet from xpub, use the watch‑only constructor to avoid accidental signing paths and ensure consistent state. This was flagged earlier and still appears here.- // Create account 0 with the provided xpub - match Account::new(None, account_type, xpub, network_rust) { + // Create account 0 with the provided xpub (watch-only) + match Account::from_xpub(None, account_type, xpub, network_rust) {
🧹 Nitpick comments (7)
.github/strict-checks.json (2)
7-20: Confirm exclusions are intentional vs. prior policy to treat clippy warnings as errors globallyPast guidance indicated “no clippy warnings allowed” across Rust code. Here, many crates (e.g., dash-spv, dash-network-ffi) are excluded. If this is a staged rollout, all good—just ensure we track convergence to full coverage. If not, this might silently allow regressions outside the FFI focus.
- Action: Validate that none of the excluded crates need strict gating in this PR, given the large FFI surface changes mentioned in the summary.
- Optional: Add a short comment in the PR description pinning rationale and intended timeline to expand strict coverage.
21-21: Document consumer workflow and ensure CI reads this fileJSON can’t have real comments; using a “comment” field is fine, but discoverability depends on CI actually consuming this file.
- Action: Verify that the GitHub Actions workflow(s) parse this JSON and drive cargo clippy -D warnings accordingly. If not present yet, I can provide a job that reads strict_check_crates and iterates with matrix strategy.
- Optional: Mirror this note in CONTRIBUTING.md so contributors know where to add/remove crates.
key-wallet-ffi/src/wallet_manager.rs (2)
18-23: Make FFIWalletManager opaque to C headersMark the Rust handle as opaque so cbindgen always emits a forward declaration and never leaks internals. This is ABI-safe and future-proof.
/// FFI wrapper for WalletManager -pub struct FFIWalletManager { +#[cbindgen::opaque] +pub struct FFIWalletManager { manager: Mutex<WalletManager<ManagedWalletInfo>>, // Track wallet IDs for FFI purposes wallet_ids: Mutex<Vec<WalletId>>, }
705-792: Monitored addresses derivation path: prefer existing wallet/account APIs to avoid driftManually deriving “first 3 receive addresses” from xpub duplicates logic and risks diverging from wallet/account gap-tracking and format rules. Consider delegating to an existing address-range API (or add one) that returns receive addresses without marking them used.
If you want, I can sketch a helper on WalletManager that returns a boxed slice of C strings via the same allocation pattern you use here.
key-wallet-ffi/src/keys.rs (1)
9-27: Make FFI key handle types opaque in headersAdd
#[cbindgen::opaque]so the C header never exposes Rust internals for these handle types. You only pass pointers across the ABI.-/// Opaque type for a private key (SecretKey) -pub struct FFIPrivateKey { +/// Opaque type for a private key (SecretKey) +#[cbindgen::opaque] +pub struct FFIPrivateKey { inner: secp256k1::SecretKey, } -/// Opaque type for an extended private key -pub struct FFIExtendedPrivateKey { +/// Opaque type for an extended private key +#[cbindgen::opaque] +pub struct FFIExtendedPrivateKey { inner: key_wallet::bip32::ExtendedPrivKey, } -/// Opaque type for a public key -pub struct FFIPublicKey { +/// Opaque type for a public key +#[cbindgen::opaque] +pub struct FFIPublicKey { inner: secp256k1::PublicKey, } -/// Opaque type for an extended public key -pub struct FFIExtendedPublicKey { +/// Opaque type for an extended public key +#[cbindgen::opaque] +pub struct FFIExtendedPublicKey { inner: key_wallet::bip32::ExtendedPubKey, }key-wallet-ffi/include/key_wallet_ffi.h (2)
18-25: Minor: duplicate includes can be trimmed
<stdbool.h>and<stdint.h>are included twice. Not harmful, but can be cleaned up.
1-1979: Refine CI header‐drift check script
The current verification script is almost there but still emits file‐path prefixes and doesn’t fail CI on mismatches. Let’s tighten it up:• In both
rginvocations, add-h(suppress file names) and-oto only print the capture, e.g.:rg -h -oP 'pub\s+extern\s+"C"\s+fn\s+\K[A-Za-z_][A-Za-z0-9_]+' key-wallet-ffi/src• Similarly for the header, scope the regex to C declarations and avoid matching structs or types:
rg -h -oP '^[[:space:]]*(?:bool|char\s*\*|unsigned\s+\w+|\w+)\s+\K[A-Za-z_][A-Za-z0-9_]+' \ key-wallet-ffi/include/key_wallet_ffi.h• After computing the two sorted lists, have the script exit non‐zero if either “Missing in header” or “Missing in Rust” is non‐empty, e.g.:
diff1=$(comm -23 rust_exports.txt header_decls.txt) diff2=$(comm -13 rust_exports.txt header_decls.txt) if [[ -n "$diff1" || -n "$diff2" ]]; then echo "Header/Rust FFI export mismatch!" >&2 echo "Missing in header:" >&2; echo "$diff1" >&2 echo "Missing in Rust:" >&2; echo "$diff2" >&2 exit 1 fi• Add this script as a CI job (e.g. in GitHub Actions) so any new
pub extern "C"function or header declaration drift will block the build automatically.
📜 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 (8)
.github/strict-checks.json(1 hunks)key-wallet-ffi/cbindgen.toml(1 hunks)key-wallet-ffi/generate_header.sh(1 hunks)key-wallet-ffi/include/key_wallet_ffi.h(1 hunks)key-wallet-ffi/src/keys.rs(1 hunks)key-wallet-ffi/src/managed_wallet.rs(1 hunks)key-wallet-ffi/src/wallet.rs(1 hunks)key-wallet-ffi/src/wallet_manager.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet-ffi/generate_header.sh
- key-wallet-ffi/cbindgen.toml
- key-wallet-ffi/src/managed_wallet.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet-ffi/src/wallet.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.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
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet-ffi/src/wallet.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.rs
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/src/wallet.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/include/key_wallet_ffi.h
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/src/wallet.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h : After updating dash-spv-ffi, synchronize the generated header into the Swift SDK using ./sync-headers.sh so Sources/DashSPVFFI/include/dash_spv_ffi.h stays up to date
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Applied to files:
.github/strict-checks.json
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to **/*.rs : Code must pass clippy with -D warnings (no clippy warnings allowed)
Applied to files:
.github/strict-checks.json
📚 Learning: 2025-08-21T05:01:58.919Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/wallet.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
key-wallet-ffi/src/keys.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift} : Use secure random number generation for keys
Applied to files:
key-wallet-ffi/src/keys.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: When adding new FFI functions in the Rust layer, implement them with #[no_mangle] extern "C" and proper cbindgen annotations
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Applied to files:
key-wallet-ffi/src/wallet_manager.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Accept input strings from C as *const c_char (borrowed; do not free inputs)
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
🧬 Code graph analysis (4)
key-wallet-ffi/src/wallet.rs (5)
key-wallet-ffi/src/types.rs (4)
error(86-97)new(47-51)new(107-111)success(77-83)key-wallet-ffi/src/error.rs (4)
error(43-48)set_error(52-58)set_success(62-68)success(35-40)key-wallet/src/wallet/initialization.rs (5)
from_mnemonic(150-171)from_mnemonic_with_passphrase(182-210)from_seed(303-323)from_xpub(234-248)new_random(88-109)key-wallet/src/account/mod.rs (2)
new(61-74)from_xpub(90-103)key-wallet/src/account/account_collection.rs (1)
new(46-60)
key-wallet-ffi/src/keys.rs (4)
key-wallet-ffi/src/types.rs (3)
error(86-97)new(47-51)new(107-111)key-wallet-ffi/src/error.rs (3)
error(43-48)set_error(52-58)set_success(62-68)key-wallet/src/account/mod.rs (2)
new(61-74)extended_public_key(148-150)key-wallet/src/bip32.rs (1)
is_hardened(648-663)
key-wallet-ffi/src/wallet_manager.rs (3)
key-wallet-ffi/src/types.rs (3)
error(86-97)new(47-51)new(107-111)key-wallet-ffi/src/error.rs (3)
error(43-48)set_success(62-68)set_error(52-58)key-wallet-ffi/src/managed_wallet.rs (1)
new(23-27)
key-wallet-ffi/include/key_wallet_ffi.h (16)
key-wallet-ffi/src/lib.rs (2)
key_wallet_ffi_initialize(39-42)key_wallet_ffi_version(48-51)key-wallet-ffi/src/account.rs (5)
wallet_get_account(16-76)wallet_get_top_up_account_with_registration_index(87-121)account_free(131-135)account_result_free_error(146-154)wallet_get_account_count(164-192)key-wallet-ffi/src/types.rs (1)
error(86-97)key-wallet-ffi/src/error.rs (2)
error(43-48)error_message_free(79-83)key-wallet-ffi/src/address.rs (4)
address_free(20-26)address_array_free(37-50)address_validate(59-114)address_get_type(129-189)key-wallet-ffi/src/balance.rs (2)
wallet_get_balance(42-62)wallet_get_account_balance(73-107)key-wallet-ffi/src/derivation.rs (16)
derivation_new_master_key(52-82)derivation_bip44_account_path(86-137)derivation_bip44_payment_path(141-195)derivation_coinjoin_path(199-250)derivation_identity_registration_path(254-305)derivation_identity_topup_path(309-362)derivation_identity_authentication_path(366-423)derivation_derive_private_key_from_seed(434-506)derivation_xpriv_to_xpub(516-542)derivation_xpriv_to_string(552-584)derivation_xpub_to_string(594-626)derivation_xpub_fingerprint(636-656)derivation_xpriv_free(666-670)derivation_xpub_free(680-684)derivation_string_free(694-698)dip9_derive_identity_key(708-863)key-wallet-ffi/src/keys.rs (20)
wallet_get_account_xpriv(37-79)wallet_get_account_xpub(89-126)wallet_derive_private_key(138-200)wallet_derive_extended_private_key(212-274)wallet_derive_private_key_as_wif(285-355)private_key_free(364-370)extended_private_key_free(379-385)extended_private_key_to_string(398-428)extended_private_key_get_private_key(440-460)private_key_to_wif(470-506)wallet_derive_public_key(518-580)wallet_derive_extended_public_key(592-654)wallet_derive_public_key_as_hex(665-735)public_key_free(744-750)extended_public_key_free(759-765)extended_public_key_to_string(778-808)extended_public_key_get_public_key(820-840)public_key_to_hex(850-877)derivation_path_parse(890-973)derivation_path_free(986-1001)key-wallet-ffi/src/managed_wallet.rs (7)
managed_wallet_get_next_bip44_receive_address(50-156)managed_wallet_get_next_bip44_change_address(170-276)managed_wallet_get_bip_44_external_address_range(292-458)managed_wallet_get_bip_44_internal_address_range(474-640)managed_wallet_get_balance(655-697)managed_wallet_free(706-712)managed_wallet_info_free(721-727)key-wallet-ffi/src/mnemonic.rs (6)
mnemonic_generate(73-123)mnemonic_generate_with_language(127-183)mnemonic_validate(192-242)mnemonic_to_seed(254-330)mnemonic_word_count(339-365)mnemonic_free(374-380)key-wallet-ffi/src/transaction.rs (4)
wallet_build_transaction(27-59)wallet_sign_transaction(72-100)wallet_check_transaction(139-246)transaction_bytes_free(255-261)key-wallet-ffi/src/utils.rs (1)
string_free(17-23)key-wallet-ffi/src/utxo.rs (3)
managed_wallet_get_utxos(91-156)wallet_get_utxos(166-184)utxo_array_free(195-208)key-wallet-ffi/src/wallet.rs (17)
wallet_create_from_mnemonic_with_options(29-132)wallet_create_from_mnemonic(144-157)wallet_create_from_seed_with_options(168-217)wallet_create_from_seed(227-240)wallet_create_from_seed_bytes(251-298)wallet_create_from_xpub(308-397)wallet_create_random_with_options(407-436)wallet_create_random(445-454)wallet_get_id(465-481)wallet_has_mnemonic(491-505)wallet_is_watch_only(515-529)wallet_get_xpub(540-577)wallet_free(587-593)wallet_free_const(608-616)wallet_add_account(627-700)wallet_add_account_with_xpub_bytes(712-813)wallet_add_account_with_string_xpub(825-924)key-wallet-ffi/src/wallet_manager.rs (17)
wallet_manager_create(27-34)wallet_manager_add_wallet_from_mnemonic_with_options(47-173)wallet_manager_add_wallet_from_mnemonic(185-200)wallet_manager_get_wallet_ids(212-256)wallet_manager_get_wallet(270-318)wallet_manager_get_managed_wallet_info(332-378)wallet_manager_get_receive_address(389-461)wallet_manager_get_change_address(472-544)wallet_manager_get_wallet_balance(559-616)wallet_manager_process_transaction(634-703)wallet_manager_get_monitored_addresses(715-792)wallet_manager_update_height(802-831)wallet_manager_current_height(841-874)wallet_manager_wallet_count(884-910)wallet_manager_free(920-926)wallet_manager_free_wallet_ids(937-944)wallet_manager_free_addresses(956-967)key-wallet-ffi/src/bip38.rs (2)
bip38_encrypt_private_key(12-75)bip38_decrypt_private_key(79-141)
⏰ 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). (15)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (12)
.github/strict-checks.json (1)
1-22: LGTM: introducing a focused strict-checks gate for key-wallet crates is a good, low-risk stepThis config cleanly scopes stricter CI to the FFI work you’re iterating on. The structure is simple and self-explanatory.
key-wallet-ffi/src/wallet_manager.rs (4)
241-252: Wallet IDs buffer: correct boxed-slice allocation/free — LGTMProducer converts Vec -> Box<[u8]> and exposes raw pointer; the free reconstructs a DST via slice_from_raw_parts_mut. This removes the prior UB/double-free risk.
Also applies to: 937-944
269-318: Const-return wallet + const-safe free — API shape matches design intentReturning
*const FFIWalletand documentingwallet_free_const()avoids const-casts and preserves read-only semantics from the manager. Good alignment with the design learning.
92-103: Wallet ID derivation ignores network; verify collision risk across networksIDs are sha256(mnemonic || passphrase) with no network salt. If the same mnemonic+passphrase is added on multiple networks, IDs collide. If the manager is network-agnostic, this prevents multi-network wallets from coexisting.
Do we want wallet IDs to be globally unique or per-network? If per-network, salt the hash:
- hasher.update(mnemonic_str.as_bytes()); - hasher.update(passphrase_str.as_bytes()); + hasher.update(mnemonic_str.as_bytes()); + hasher.update(passphrase_str.as_bytes()); + hasher.update([network_rust as u8]); // salt with network discriminantIf global uniqueness is intended, please add a brief comment here to document the decision and constraints.
546-616: Balance outputs as u64 — cross‑platform safeReplacing platform‑dependent c_ulong with fixed‑width u64 prevents truncation on LLP64 (Windows). Header and implementation are consistent.
key-wallet-ffi/src/wallet.rs (2)
587-616: Const-safe wallet free — LGTM
wallet_free_constresolves the const-return + mutable-free mismatch and avoids forcing callers to cast away const. Good FFI ergonomics.
465-481: wallet_get_id copies 32 bytes into caller buffer — LGTMNull checks, exact-length copy, and error reporting look correct.
key-wallet-ffi/src/keys.rs (2)
879-973: Reject unsupported ChildNumber variants — LGTMFail‑fast behavior for non‑Normal/Hardened variants avoids lossy mappings and surprises at the FFI boundary.
470-506: WIF conversion implementation is tight and avoids logging secretsUses
dashcore::PrivateKeywith compression and returns a CString without leaking or logging private material. Good.key-wallet-ffi/include/key_wallet_ffi.h (3)
232-248: Opaque handles in header — LGTMHeader uses forward declarations for FFI handles (wallet, manager, key types, managed wallet info), which is correct and consumable from C.
Also applies to: 372-379
397-401: Version string lifetime — LGTMHeader documents a static string not to be freed; matches the Rust implementation returning a static C string.
779-817: Key-derivation ABIs match Rust (opaque handles + string helpers) — LGTMPrototypes now reflect the Rust FFI (opaque key handles, string-return helpers, and corresponding free functions). This resolves previous ABI drift.
Also applies to: 813-835, 931-970, 1005-1008, 1022-1024, 1035-1035
| "strict_check_crates": [ | ||
| "key-wallet", | ||
| "key-wallet-manager", | ||
| "key-wallet-ffi" | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify crate names map 1:1 to package.name across the workspace; consider adding a schema_version and sorting lists
- Action: Validate that "key-wallet", "key-wallet-manager", and "key-wallet-ffi" exactly match the package.name entries in their Cargo.toml files. Hyphens vs underscores can bite here at CI time.
- Nit: Add a schema_version and keep lists alphabetized to minimize churn in diffs and avoid tooling ambiguity.
Run this script from repo root to verify names and detect overlaps/missing entries:
If you want, I can wire a small CI step to consume this JSON and feed cargo clippy invocations accordingly.
Apply this diff to add a schema_version and alphabetize entries:
{
+ "schema_version": 1,
"strict_check_crates": [
- "key-wallet",
- "key-wallet-manager",
- "key-wallet-ffi"
+ "key-wallet",
+ "key-wallet-ffi",
+ "key-wallet-manager"
],
"excluded_crates": [
- "dash",
- "dash-network",
- "dash-network-ffi",
- "hashes",
- "internals",
- "fuzz",
- "rpc-client",
- "rpc-json",
- "rpc-integration-test",
- "dash-spv",
- "dash-spv-ffi",
- "test-utils"
+ "dash",
+ "dash-network",
+ "dash-network-ffi",
+ "dash-spv",
+ "dash-spv-ffi",
+ "fuzz",
+ "hashes",
+ "internals",
+ "rpc-client",
+ "rpc-integration-test",
+ "rpc-json",
+ "test-utils"
],
"comment": "Crates in strict_check_crates will fail CI on any warnings or clippy issues. Add or remove crates as needed."
}🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, os, sys
import tomllib
with open(".github/strict-checks.json","rb") as f:
cfg = json.load(f)
strict = set(cfg.get("strict_check_crates", []))
excluded = set(cfg.get("excluded_crates", []))
crate_names = set()
for root, dirs, files in os.walk("."):
if any(seg in root for seg in (".git", "target")):
continue
if "Cargo.toml" in files:
p = os.path.join(root, "Cargo.toml")
try:
with open(p,"rb") as tf:
data = tomllib.load(tf)
pkg = data.get("package", {}).get("name")
if pkg:
crate_names.add(pkg)
except Exception as e:
print(f"WARN: Failed to parse {p}: {e}", file=sys.stderr)
missing_strict = sorted(strict - crate_names)
missing_excl = sorted(excluded - crate_names)
overlap = sorted(strict & excluded)
unlisted = sorted(crate_names - (strict | excluded))
print("Crates discovered:", sorted(crate_names))
print("Missing in workspace (strict):", missing_strict)
print("Missing in workspace (excluded):", missing_excl)
print("Overlap (both strict and excluded):", overlap)
print("Unlisted crates (neither strict nor excluded):", unlisted)
if overlap:
sys.exit(2)
PY
echo
echo "Searching workflows referencing this config..."
rg -n --glob '.github/workflows/*.yml' 'strict-checks\.json|strict_check_crates|excluded_crates' -C2 || {
echo "No workflow references found. Ensure CI reads and uses this config." >&2
}Length of output: 1318
Update .github/strict-checks.json to match actual crates and wire into CI
- Verified that all entries under
strict_check_cratesexactly matchpackage.namein theirCargo.toml(no missing crates). - The
excluded_crateslist contains eight names that no longer exist in the workspace and must be removed or renamed:- dash
- fuzz
- hashes
- internals
- rpc-client
- rpc-integration-test
- rpc-json
- test-utils
- The following crates are present in the repo but not listed under either
strict_check_cratesorexcluded_crates. Decide whether each should be strictly checked or explicitly excluded:- dash-fuzz
- dashcore
- dashcore-private
- dashcore-rpc
- dashcore-rpc-json
- dashcore-test-utils
- dashcore_hashes
- dashcore_hashes-fuzz
- embedded
- integration_test
- schemars
- No workflows in
.github/workflows/*.ymlreference this config, so CI will not actually enforce these rules. Add a step to load and use this JSON in your lint/test pipeline.
Proposed patch—adds a schema_version, removes obsolete names, alphabetizes lists, and highlights where to wire into CI:
.github/strict-checks.json
{
+ "schema_version": 1,
"strict_check_crates": [
- "key-wallet",
- "key-wallet-ffi",
- "key-wallet-manager"
+ "key-wallet",
+ "key-wallet-ffi",
+ "key-wallet-manager"
],
"excluded_crates": [
- "dash",
- "dash-network",
- "dash-network-ffi",
- "dash-spv",
- "dash-spv-ffi",
- "fuzz",
- "hashes",
- "internals",
- "rpc-client",
- "rpc-integration-test",
- "rpc-json",
- "test-utils"
+ "dash-network",
+ "dash-network-ffi",
+ "dash-spv",
+ "dash-spv-ffi"
],
"comment": "Crates in strict_check_crates will fail CI on any warnings or clippy issues. Add or remove crates as needed."
}• Remove the eight missing/ex-obsolete crate names from excluded_crates.
• Decide and add the eleven “unlisted” crates under the appropriate list.
• Alphabetize both arrays to reduce churn.
• Add a CI job (e.g. in .github/workflows/ci.yml) that reads this JSON and feeds its lists into your cargo clippy/cargo test matrix.
📝 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.
| "strict_check_crates": [ | |
| "key-wallet", | |
| "key-wallet-manager", | |
| "key-wallet-ffi" | |
| ], | |
| .github/strict-checks.json | |
| { | |
| "schema_version": 1, | |
| "strict_check_crates": [ | |
| "key-wallet", | |
| "key-wallet-ffi", | |
| "key-wallet-manager" | |
| ], | |
| "excluded_crates": [ | |
| "dash-network", | |
| "dash-network-ffi", | |
| "dash-spv", | |
| "dash-spv-ffi" | |
| ], | |
| "comment": "Crates in strict_check_crates will fail CI on any warnings or clippy issues. Add or remove crates as needed." | |
| } |
🤖 Prompt for AI Agents
In .github/strict-checks.json around lines 2-6, remove the eight obsolete crate
names from "excluded_crates", add a "schema_version" field, and update both
arrays to include the unlisted crates: add library/runtime crates to
"strict_check_crates" (dashcore, dashcore-private, dashcore_hashes,
dashcore-rpc, schemars, embedded) and add fuzz/tests/tools/integration crates to
"excluded_crates" (dash-fuzz, dashcore-rpc-json, dashcore-test-utils,
dashcore_hashes-fuzz, dashcore-test-utils, integration_test, dashcore-rpc-json —
ensure no duplicates and use the exact package.name values), alphabetize both
arrays to reduce churn, and finally add a CI job (e.g., in
.github/workflows/ci.yml) that reads this JSON, uses its lists to generate the
cargo clippy/test matrix, and fails if a crate listed under strict_check_crates
violates strict checks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/rust.yml (1)
249-277: Integration Tests: modernize steps (checkout v4 + toolchain action).Bring this job in line with others to avoid action deprecations.
Apply this diff:
steps: - - name: Checkout Crate - uses: actions/checkout@v2 - - name: Checkout Toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: ${{ matrix.rust }} - override: true + - name: Checkout Crate + uses: actions/checkout@v4 + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ matrix.rust }}
♻️ Duplicate comments (7)
.github/workflows/fuzz.yml (1)
52-56: Migrate off actions-rs/toolchain@v1 and pin Rust to 1.89.0.actions-rs/toolchain@v1 is deprecated on current runners and will fail; also pin the patch version for reproducibility.
Apply this diff:
- - uses: actions-rs/toolchain@v1 - with: - toolchain: 1.89 - override: true - profile: minimal + - uses: dtolnay/rust-toolchain@stable + with: + toolchain: 1.89.0.github/workflows/rust.yml (4)
56-61: Workspace Tests: migrate toolchain action.Update to dtolnay/rust-toolchain and drop unsupported inputs. This also resolves actionlint failures.
Apply this diff:
- - name: Setup Rust toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: ${{ matrix.rust }} - override: true + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ matrix.rust }}
83-89: Clippy job: migrate toolchain action and keep clippy component.Switch to dtolnay action; pass components under with:. Resolves deprecation and ensures clippy is installed.
Apply this diff:
- - name: Setup Rust toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: stable - override: true - components: clippy + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: stable + components: clippy
109-115: Strict-checks job: migrate toolchain action and keep clippy component.Same update here to avoid runner incompatibilities.
Apply this diff:
- - name: Setup Rust toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: stable - override: true - components: clippy + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: stable + components: clippy
167-173: Fmt job: migrate toolchain action and keep rustfmt component.Update to dtolnay to prevent action runtime errors.
Apply this diff:
- - name: Setup Rust toolchain - uses: actions-rs/toolchain@v1 - with: - profile: minimal - toolchain: stable - override: true - components: rustfmt + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: stable + components: rustfmtkey-wallet-ffi/src/keys.rs (2)
128-200: Reconfirm C header parity for new/changed FFI exportsMultiple new/changed symbols (wallet_derive_private_key, wallet_derive_extended_private_key, wallet_derive_public_key[as_hex], extended* getters, free, derivation_path …) must have matching prototypes and opaque-type forward decls in the generated header.
Please re-run the existing verification script and update the header accordingly.
#!/bin/bash # Compare Rust FFI exports vs header declarations rg -nP '^\s*#\s*\[no_mangle\]\s*\n\s*pub\s+unsafe\s+extern\s+"C"\s+fn\s+([a-zA-Z0-9_]+)\s*\(' key-wallet-ffi/src \ -or '$1' | sort -u > /tmp/rust_exports.txt rg -nP '^\w[\w\s\*]*\s+([a-zA-Z0-9_]+)\s*\(' key-wallet-ffi/include/key_wallet_ffi.h \ -or '$1' | sort -u > /tmp/header_decls.txt echo "Missing in header:"; comm -23 /tmp/rust_exports.txt /tmp/header_decls.txt echo "Missing in Rust:"; comm -13 /tmp/rust_exports.txt /tmp/header_decls.txtAlso applies to: 516-589, 590-662, 664-743, 895-1017
1002-1016: UB in derivation_path_free: free boxed slices, not Vec::from_raw_partsindices/hardened were allocated via Box<[T]>. Reconstructing Vec with from_raw_parts is mismatched and can be UB. Free using Box::from_raw on slice fat pointers.
Apply:
pub unsafe extern "C" fn derivation_path_free( indices: *mut u32, hardened: *mut bool, count: usize, ) { - if !indices.is_null() && count > 0 { - unsafe { - let _ = Vec::from_raw_parts(indices, count, count); - } - } - if !hardened.is_null() && count > 0 { - unsafe { - let _ = Vec::from_raw_parts(hardened, count, count); - } - } + if !indices.is_null() && count > 0 { + // Rebuild Box<[u32]> for proper deallocation + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(indices, count)); + } + if !hardened.is_null() && count > 0 { + // Rebuild Box<[bool]> for proper deallocation + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(hardened, count)); + } }
🧹 Nitpick comments (12)
.github/workflows/fuzz.yml (1)
47-49: Cache paths: good expansion; consider adding restore-keys for partial hits.Adding ~/.cargo/registry and ~/.cargo/git will materially improve cache hits. Optionally add restore-keys to fall back to a less-specific key when Cargo.lock changes.
Apply this diff to add restore-keys:
- uses: actions/cache@v4 id: cache-fuzz with: path: | ~/.cargo/registry ~/.cargo/git fuzz/target target key: cache-${{ matrix.fuzz_target }}-${{ hashFiles('**/Cargo.toml','**/Cargo.lock') }} + restore-keys: | + cache-${{ matrix.fuzz_target }}-.github/workflows/rust.yml (2)
44-44: Trim trailing whitespace to satisfy linters.YAMLlint reports trailing spaces on these lines. Remove them to keep CI green.
Apply this diff (showing representative fixes):
- + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - +Also applies to: 75-75, 101-101, 123-123, 132-132, 135-135, 144-144, 147-147, 156-156, 159-159
76-101: Speed up Clippy job with a cargo cache (optional).Add a cache block like strict-checks/workspace-tests to avoid rebuilding dependencies.
Apply this diff:
clippy: @@ - name: Run clippy (excluding strict-checked crates) run: | # Auto-discover all workspace crates and exclude strict-checked ones + - name: Cache cargo registry + uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: ${{ runner.os }}-cargo-clippy-${{ hashFiles('**/Cargo.lock') }}key-wallet-ffi/src/keys_tests.rs (5)
166-171: Use string_free for freeing C strings for allocator/ABI consistencyIn a few places the tests free char* with CString::from_raw instead of the crate’s string_free. Using string_free everywhere is safer and mirrors what C/Swift callers must do.
Apply diffs:
@@ - let _ = CString::from_raw(xpub_str); + crate::utils::string_free(xpub_str); @@ - if !wif_str.is_null() { - unsafe { - let _ = CString::from_raw(wif_str); - } - } + if !wif_str.is_null() { + unsafe { crate::utils::string_free(wif_str); } + } @@ - if !hex_str.is_null() { - unsafe { - let _ = CString::from_raw(hex_str); - } - } + if !hex_str.is_null() { + unsafe { crate::utils::string_free(hex_str); } + } @@ - let _ = CString::from_raw(xpub_str); + crate::utils::string_free(xpub_str); @@ - if !hex_str.is_null() { - unsafe { - let _ = CString::from_raw(hex_str); - } - } + if !hex_str.is_null() { + unsafe { crate::utils::string_free(hex_str); } + }Also applies to: 206-211, 262-266, 552-559, 619-623
241-259: Unify BIP44 coin type in Testnet tests (use 1’ for testnets)You’re creating Testnet wallets but derive on 44'/5' paths in some tests. While functionally fine, it’s conceptually inconsistent. Prefer 44'/1' for Testnet to avoid confusion.
Example fixes:
- let path = CString::new("m/44'/5'/0'/0/0").unwrap(); + let path = CString::new("m/44'/1'/0'/0/0").unwrap(); @@ - let test_paths = [ - "m/44'/5'/0'/0/0", - "m/44'/5'/0'/0/1", - "m/44'/5'/0'/1/0", // Change address - "m/44'/5'/1'/0/0", // Different account - ]; + let test_paths = [ + "m/44'/1'/0'/0/0", + "m/44'/1'/0'/0/1", + "m/44'/1'/0'/1/0", // Change address + "m/44'/1'/1'/0/0", // Different account + ];Also applies to: 593-600
174-218: Add assertions on WIF network prefix for stronger validationSince you derive on Testnet, assert WIF starts with 'c' or '9' (common Dash testnet prefixes depending on compression/version) after private_key_to_wif. This guards against network mix-ups.
498-526: Exercise wallet_derive_public_key_as_hex directlyYou already validate public_key_to_hex; add a test invoking wallet_derive_public_key_as_hex to cover that FFI path too. I can push a patch if you want.
636-649: Drop “dummy” non-null pointers in free edge test; zero-count nulls sufficePassing 1 as *mut T is harmless here due to count=0, but it’s an anti-pattern that could become UB if the free implementation changes. Prefer only the null-pointer test.
#[test] fn test_derivation_path_free_edge_cases() { // Test freeing null pointers unsafe { derivation_path_free(ptr::null_mut(), ptr::null_mut(), 0); } - - // Test freeing with zero count - let dummy_ptr = 1 as *mut u32; - let dummy_bool_ptr = 1 as *mut bool; - unsafe { - derivation_path_free(dummy_ptr, dummy_bool_ptr, 0); - } }key-wallet-ffi/src/keys.rs (4)
786-820: Doc vs implementation mismatch: network parameter is ignoredextended_public_key_to_string (and similarly extended_private_key_to_string) accept network but ignore it. Either deprecate the parameter in docs or clarify “network is encoded in the key; parameter is ignored”.
Minimal doc fix suggestion:
-/// - `network` specifies the network for the key encoding +/// - `network` is ignored; the network is encoded in the extended key
398-432: Doc vs implementation mismatch: network parameter is ignoredSame comment as for xpub: clarify that network is ignored and encoding comes from the key itself.
444-468: Optional: consider zeroizing private key material on dropFFIPrivateKey and FFIExtendedPrivateKey expose secret material. If feasible, wrap secrets with a zeroizing type (e.g., zeroize/secrecy) or implement Drop to securely clear memory on free. This reduces key material lifetime in process memory. Gate behind a feature if necessary to avoid MSRV friction.
Also applies to: 832-856
138-199: Remove redundant nestedunsafeblocks in FFI functionsClippy’s
unnecessary_unsafelint will trigger (and fail the build under-D warnings) whenever you wrap code in an innerunsafe {…}inside a function already declaredunsafe. Since all pointer dereferences and FFI calls inside these functions are covered by the outerunsafe extern "C" fn, you can safely remove the inner blocks.• File
key-wallet-ffi/src/keys.rs
–wallet_derive_private_key(derivation‐path CStr conversion)
–wallet_derive_private_key_as_wif(same pattern)
– Any otherpub unsafe extern "C" fn …that uses only raw‐pointer derefs or CStr calls insideunsafe { … }Example diff for
wallet_derive_private_key:pub unsafe extern "C" fn wallet_derive_private_key( wallet: *const FFIWallet, network: FFINetwork, derivation_path: *const c_char, error: *mut FFIError, ) -> *mut FFIPrivateKey { if wallet.is_null() || derivation_path.is_null() { FFIError::set_error(error, FFIErrorCode::InvalidInput, "Null pointer provided".to_string()); return ptr::null_mut(); } - let path_str = unsafe { - match CStr::from_ptr(derivation_path).to_str() { - Ok(s) => s, - Err(_) => { - FFIError::set_error( - error, - FFIErrorCode::InvalidInput, - "Invalid UTF-8 in derivation path".to_string(), - ); - return ptr::null_mut(); - } - } - }; + let path_str = match unsafe { CStr::from_ptr(derivation_path) }.to_str() { + Ok(s) => s, + Err(_) => { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Invalid UTF-8 in derivation path".to_string(), + ); + return ptr::null_mut(); + } + }; // …rest of implementation… }Apply the same removal of inner
unsafe { … }wrappers across allunsafe extern "C"functions in this file.
📜 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 (4)
.github/workflows/fuzz.yml(1 hunks).github/workflows/rust.yml(4 hunks)key-wallet-ffi/src/keys.rs(1 hunks)key-wallet-ffi/src/keys_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.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
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rs
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rs
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: When adding new FFI functions in the Rust layer, implement them with #[no_mangle] extern "C" and proper cbindgen annotations
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
📚 Learning: 2025-08-21T05:01:58.919Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/keys.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/src/keys.rs
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift} : Use secure random number generation for keys
Applied to files:
key-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/src/keys.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
key-wallet-ffi/src/keys_tests.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/c_tests/**/*.c : Add a corresponding C test for each new FFI function
Applied to files:
key-wallet-ffi/src/keys_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
key-wallet-ffi/src/keys_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Ensure tests cover both mainnet and testnet configurations
Applied to files:
key-wallet-ffi/src/keys_tests.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to **/*.rs : Format code with rustfmt and keep formatting clean
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-21T04:45:50.400Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.400Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Applied to files:
.github/workflows/rust.yml
🧬 Code graph analysis (2)
key-wallet-ffi/src/keys.rs (4)
key-wallet-ffi/src/types.rs (3)
error(86-97)new(47-51)new(107-111)key-wallet-ffi/src/error.rs (3)
error(43-48)set_error(52-58)set_success(62-68)key-wallet/src/account/mod.rs (2)
new(61-74)extended_public_key(148-150)key-wallet/src/bip32.rs (1)
is_hardened(648-663)
key-wallet-ffi/src/keys_tests.rs (4)
key-wallet-ffi/src/error.rs (2)
error(43-48)success(35-40)key-wallet-ffi/src/wallet.rs (2)
wallet_create_from_mnemonic(144-157)wallet_free(587-593)key-wallet-ffi/src/keys.rs (16)
wallet_derive_extended_private_key(212-274)extended_private_key_to_string(398-432)extended_private_key_get_private_key(444-468)private_key_free(364-370)extended_private_key_free(379-385)wallet_derive_extended_public_key(600-662)extended_public_key_to_string(786-820)extended_public_key_get_public_key(832-856)public_key_to_hex(866-893)public_key_free(752-758)extended_public_key_free(767-773)wallet_get_account_xpriv(37-79)wallet_get_account_xpub(89-126)wallet_derive_private_key(138-200)derivation_path_parse(906-989)derivation_path_free(1002-1017)key-wallet-ffi/src/utils.rs (1)
string_free(17-23)
🪛 actionlint (1.7.7)
.github/workflows/fuzz.yml
52-52: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/rust.yml
56-56: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
83-83: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
109-109: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
167-167: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/rust.yml
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 101-101: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 132-132: trailing spaces
(trailing-spaces)
[error] 135-135: trailing spaces
(trailing-spaces)
[error] 144-144: trailing spaces
(trailing-spaces)
[error] 147-147: trailing spaces
(trailing-spaces)
[error] 156-156: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
.github/workflows/fuzz.yml (1)
51-51: LGTM: cache key now references the correct matrix variable.Using matrix.fuzz_target fixes the undefined-matrix issue and ensures cache separation per target.
.github/workflows/rust.yml (1)
29-29: LGTM: add Rust 1.89.0 to the primary matrix.Pinning to 1.89.0 aligns with the workspace toolchain and ensures deterministic CI.
key-wallet-ffi/src/keys_tests.rs (2)
12-103: Strong coverage of extended key lifecycles and conversionsGood job validating xprv/xpub round-trips, WIF conversion, hex serialization, and cleanup paths. Assertions on prefixes (tprv/tpub) and 33-byte compressed length are spot on.
383-421: Good negative coverage for null inputs and error codesNull wallet and null path checks for wallet_derive_public_key are thorough and validate error codes correctly.
key-wallet-ffi/src/keys.rs (2)
906-989: Good: fail fast on unsupported ChildNumber variants in derivation_path_parseExplicitly rejecting non-Normal/Hardened variants with InvalidDerivationPath avoids silent mis-parsing of 256-bit indices. Nice.
866-893: Hex encoding of compressed PublicKey is correct and unambiguousUsing key.inner.serialize() followed by hex::encode is the right approach and avoids earlier pitfalls with formatter traits. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
key-wallet-ffi/include/key_wallet_ffi.h (7)
18-25: Duplicate standard includes (harmless but noisy).stdint.h and stdbool.h are included twice. Clean up to reduce warnings and noise in some toolchains.
#include <stdarg.h> #include <stdbool.h> #include <stdint.h> #include <stdlib.h> -#include <stdint.h> #include <stddef.h> -#include <stdbool.h>
29-50: Enum variant names are not namespaced; high collision risk in C headers.Variants like DEFAULT, UNKNOWN, DASH, REGTEST, etc., are very generic and can collide with macros/types in downstream projects and platform headers. Prefer prefixing enum variants with the type name or an FFI-specific prefix via cbindgen configuration.
Suggested cbindgen.toml adjustments (regenerate header after):
[enum] prefix_with_name = true rename_variants = "ScreamingSnakeCase"This yields names like FFIAccountCreationOptionType_DEFAULT and FFINetwork_DASH, avoiding collisions.
Also applies to: 63-108, 113-131, 136-149, 158-169, 174-180, 184-197
1076-1082: Document the exact free function for returned strings (prevent leaks/misuse).Several docs say “must be freed by the caller” without specifying which function. Elsewhere you provide address_free and string_free; be explicit for consistency.
- - The returned string must be freed by the caller + - The returned string must be freed with address_free- - The returned C string must be freed by the caller when no longer needed + - The returned string must be freed with string_free# Safety ... - - The caller must ensure all pointers remain valid for the duration of this call + - The caller must ensure all pointers remain valid for the duration of this call + - The returned string must be freed with address_freeApplies to:
- managed_wallet_get_next_bip44_receive_address
- managed_wallet_get_next_bip44_change_address
- wallet_get_xpub
- wallet_manager_get_receive_address
- wallet_manager_get_change_address
Also applies to: 1097-1103, 1574-1584, 1779-1783, 1795-1801
1719-1734: Clarify wallet_manager_get_wallet_ids count semantics (bytes vs. ids).Docs do not define whether count is number of bytes or number of 32-byte IDs. The free API also says “number of wallet IDs”, which is ambiguous for a uint8_t buffer. Rust typically frees Vec with a byte length.
Proposed doc tweaks:
/* Get wallet IDs @@ - - `count_out` must be a valid pointer to receive the count + - `count_out` receives the number of bytes in the buffer (each wallet ID is 32 bytes; id_count = *count_out / 32) @@ /* Free wallet IDs buffer @@ - - `count` must match the number of wallet IDs in the buffer + - `count` must equal the byte length originally returned by wallet_manager_get_wallet_ids (not the number of IDs)If you intend count to be “number of IDs”, consider returning a char**/uint8_t[32]* instead for clarity.
Also applies to: 1932-1941
333-365: Prefer size_t over uintptr_t for lengths/counts in C API.Counts/lengths are modeled as usize in Rust; mapping to size_t is the conventional C choice and interoperates better with foreign toolchains. Using uintptr_t is usually same width but semantically “pointer value,” not “size.”
If feasible, enable cbindgen’s usize-is-size_t mapping:
[export] usize_is_size_t = trueThis will emit size_t for parameters like count_out, path_max_len, array counts, etc.
Also applies to: 574-579, 585-592, 598-603, 1050-1055, 1412-1412
1395-1401: Two address-array free APIs; consider consolidating to one to reduce confusion.Both address_array_free and wallet_manager_free_addresses free char** arrays. Unless they free different allocators, prefer a single API (address_array_free) and deprecate the other, or document that both are aliases.
Happy to propose a deprecation note and aliasing in Rust so cbindgen emits only one symbol next release.
Also applies to: 1955-1957, 491-491
204-248: Naming overlap: two “extended private key” handle types may confuse consumers.Both FFIExtendedPrivKey and FFIExtendedPrivateKey exist and represent extended private keys from different modules. Consider adding brief one-line comments clarifying their provenance/use (derivation vs wallet-scoped), or alias one to the other if they’re interchangeable.
Also applies to: 229-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
key-wallet-ffi/include/key_wallet_ffi.h(1 hunks)key-wallet-ffi/src/keys.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/src/keys.rs
🧰 Additional context used
📓 Path-based instructions (1)
{dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries
Files:
key-wallet-ffi/include/key_wallet_ffi.h
🧠 Learnings (9)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: When adding new FFI functions in the Rust layer, implement them with #[no_mangle] extern "C" and proper cbindgen annotations
📚 Learning: 2025-08-21T05:01:58.919Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Accept input strings from C as *const c_char (borrowed; do not free inputs)
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {dash-network-ffi/**,dash-spv-ffi/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,udl,h,swift} : Be careful with memory management at FFI boundaries
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: When adding new FFI functions in the Rust layer, implement them with #[no_mangle] extern "C" and proper cbindgen annotations
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : FFI functions must be declared with #[no_mangle] extern "C"
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
🧬 Code graph analysis (1)
key-wallet-ffi/include/key_wallet_ffi.h (16)
key-wallet-ffi/src/lib.rs (2)
key_wallet_ffi_initialize(39-42)key_wallet_ffi_version(48-51)key-wallet-ffi/src/account.rs (4)
wallet_get_account(16-76)wallet_get_top_up_account_with_registration_index(87-121)account_free(131-135)account_result_free_error(146-154)key-wallet-ffi/src/types.rs (1)
error(86-97)key-wallet-ffi/src/error.rs (2)
error(43-48)error_message_free(79-83)key-wallet-ffi/src/address.rs (4)
address_free(20-26)address_array_free(37-50)address_validate(59-114)address_get_type(129-189)key-wallet-ffi/src/balance.rs (2)
wallet_get_balance(42-62)wallet_get_account_balance(73-107)key-wallet-ffi/src/derivation.rs (11)
derivation_new_master_key(52-82)derivation_bip44_account_path(86-137)derivation_derive_private_key_from_seed(434-506)derivation_xpriv_to_xpub(516-542)derivation_xpriv_to_string(552-584)derivation_xpub_to_string(594-626)derivation_xpub_fingerprint(636-656)derivation_xpriv_free(666-670)derivation_xpub_free(680-684)derivation_string_free(694-698)dip9_derive_identity_key(708-863)key-wallet-ffi/src/keys.rs (18)
wallet_get_account_xpriv(37-79)wallet_get_account_xpub(89-126)wallet_derive_private_key(138-200)wallet_derive_extended_private_key(212-274)wallet_derive_private_key_as_wif(285-355)private_key_free(364-370)extended_private_key_free(379-385)extended_private_key_to_string(398-432)extended_private_key_get_private_key(444-468)private_key_to_wif(478-514)wallet_derive_public_key(526-588)wallet_derive_extended_public_key(600-662)wallet_derive_public_key_as_hex(673-743)public_key_free(752-758)extended_public_key_free(767-773)extended_public_key_to_string(786-820)extended_public_key_get_public_key(832-856)public_key_to_hex(866-893)key-wallet-ffi/src/managed_wallet.rs (7)
managed_wallet_get_next_bip44_receive_address(50-156)managed_wallet_get_next_bip44_change_address(170-276)managed_wallet_get_bip_44_external_address_range(292-458)managed_wallet_get_bip_44_internal_address_range(474-640)managed_wallet_get_balance(655-697)managed_wallet_free(706-712)managed_wallet_info_free(721-727)key-wallet-ffi/src/mnemonic.rs (6)
mnemonic_generate(73-123)mnemonic_generate_with_language(127-183)mnemonic_validate(192-242)mnemonic_to_seed(254-330)mnemonic_word_count(339-365)mnemonic_free(374-380)key-wallet-ffi/src/transaction.rs (4)
wallet_build_transaction(27-59)wallet_sign_transaction(72-100)wallet_check_transaction(139-246)transaction_bytes_free(255-261)key-wallet-ffi/src/utils.rs (1)
string_free(17-23)key-wallet-ffi/src/utxo.rs (3)
managed_wallet_get_utxos(91-156)wallet_get_utxos(166-184)utxo_array_free(195-208)key-wallet-ffi/src/wallet.rs (5)
wallet_create_from_mnemonic_with_options(29-132)wallet_create_from_mnemonic(144-157)wallet_get_xpub(540-577)wallet_free(587-593)wallet_free_const(608-616)key-wallet-ffi/src/wallet_manager.rs (17)
wallet_manager_create(27-34)wallet_manager_add_wallet_from_mnemonic_with_options(47-173)wallet_manager_add_wallet_from_mnemonic(185-200)wallet_manager_get_wallet_ids(212-256)wallet_manager_get_wallet(270-318)wallet_manager_get_managed_wallet_info(332-378)wallet_manager_get_receive_address(389-461)wallet_manager_get_change_address(472-544)wallet_manager_get_wallet_balance(559-616)wallet_manager_process_transaction(634-703)wallet_manager_get_monitored_addresses(715-792)wallet_manager_update_height(802-831)wallet_manager_current_height(841-874)wallet_manager_wallet_count(884-910)wallet_manager_free(920-926)wallet_manager_free_wallet_ids(937-944)wallet_manager_free_addresses(956-967)key-wallet-ffi/src/bip38.rs (2)
bip38_encrypt_private_key(12-75)bip38_decrypt_private_key(79-141)
⏰ 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: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (3)
key-wallet-ffi/include/key_wallet_ffi.h (3)
1749-1753: Const-correct wallet retrieval from manager—great.Returning const FFIWallet* aligns with the design goal of read-only access from lookups. This prevents accidental mutations and centralizes state changes in manager APIs.
1602-1616: wallet_free_const addition is the right companion for const returns.Providing wallet_free_const avoids const-cast boilerplate for callers and matches the manager’s const-returned wallets.
399-400: Version string ownership is documented correctly—good.Header now states the version string is static and must not be freed, which matches the Rust implementation using a static C string.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/rust.yml (1)
57-65: Prefer Swatinem/rust-cache over hand-rolled cache of registry/git/target.It’s more reliable across cargo versions, reduces cache key churn, and handles the target dir safely. Use it in both workspace-tests and strict-checks.
Apply these diffs:
- - name: Cache cargo registry - uses: actions/cache@v4 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - target - key: ${{ runner.os }}-cargo-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }} + - name: Cache Rust + uses: Swatinem/rust-cache@v2 + with: + key: ${{ matrix.rust }} + cache-on-failure: true- - name: Cache cargo registry - uses: actions/cache@v4 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - target - key: ${{ runner.os }}-cargo-strict-${{ hashFiles('**/Cargo.lock') }} + - name: Cache Rust + uses: Swatinem/rust-cache@v2 + with: + key: strict-stable + cache-on-failure: trueAlso applies to: 107-115
🧹 Nitpick comments (4)
.github/workflows/rust.yml (4)
66-70: Lock dependency resolution in CI.Add --locked to cargo invocations to ensure reproducible builds and avoid network resolution in CI.
Apply these diffs:
- - name: Run workspace tests - run: cargo test --workspace --all-features + - name: Run workspace tests + run: cargo test --workspace --all-features --locked - - name: Run workspace tests (no default features) - run: cargo test --workspace --no-default-features + - name: Run workspace tests (no default features) + run: cargo test --workspace --no-default-features --locked - - name: Build workspace (release mode) - run: cargo build --workspace --release + - name: Build workspace (release mode) + run: cargo build --workspace --release --locked- cargo check -p key-wallet --all-features --lib --bins --tests - cargo build -p key-wallet --all-features --lib --bins - cargo test -p key-wallet --all-features --lib --bins + cargo check -p key-wallet --all-features --lib --bins --tests --locked + cargo build -p key-wallet --all-features --lib --bins --locked + cargo test -p key-wallet --all-features --lib --bins --locked- run: cargo clippy -p key-wallet --all-features --lib --bins --tests -- -D warnings + run: cargo clippy -p key-wallet --all-features --lib --bins --tests --locked -- -D warnings- cargo check -p key-wallet-manager --all-features --lib --bins --tests - cargo build -p key-wallet-manager --all-features --lib --bins - cargo test -p key-wallet-manager --all-features --lib --bins + cargo check -p key-wallet-manager --all-features --lib --bins --tests --locked + cargo build -p key-wallet-manager --all-features --lib --bins --locked + cargo test -p key-wallet-manager --all-features --lib --bins --locked- run: cargo clippy -p key-wallet-manager --all-features --lib --bins --tests -- -D warnings + run: cargo clippy -p key-wallet-manager --all-features --lib --bins --tests --locked -- -D warnings- cargo check -p key-wallet-ffi --all-features --lib --bins --tests - cargo build -p key-wallet-ffi --all-features --lib --bins - cargo test -p key-wallet-ffi --all-features --lib --bins + cargo check -p key-wallet-ffi --all-features --lib --bins --tests --locked + cargo build -p key-wallet-ffi --all-features --lib --bins --locked + cargo test -p key-wallet-ffi --all-features --lib --bins --locked- run: cargo clippy -p key-wallet-ffi --all-features --lib --bins --tests -- -D warnings + run: cargo clippy -p key-wallet-ffi --all-features --lib --bins --tests --locked -- -D warnings- run: cargo fmt --all -- --check + run: cargo fmt --all -- --check --config-path ./rustfmt.tomlNote: fmt doesn’t use the resolver, but keeping a single source of truth via rustfmt.toml is helpful if present.
Also applies to: 121-147, 145-147, 149-150, 164-164, 70-70
6-32: Optionally allow failures on beta/nightly to avoid blocking PRs.If nightly/beta break, the entire CI will fail. Consider continue-on-error for those toolchains in matrixed jobs.
Apply these diffs:
Tests: name: Tests runs-on: ubuntu-latest + continue-on-error: ${{ matrix.rust == 'nightly' || matrix.rust == 'beta' }} strategy:workspace-tests: name: Workspace Tests runs-on: ubuntu-latest + continue-on-error: ${{ matrix.rust == 'nightly' || matrix.rust == 'beta' }} strategy:Also applies to: 43-50
1-4: Add least-privilege permissions and cancel-in-flight runs for faster CI.Reduces token scope and runner usage on rapid pushes.
Apply this diff near the top:
on: [push, pull_request] name: Continuous integration + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
42-42: Trim trailing whitespace flagged by YAML linter.These lines contain trailing spaces that can fail stricter linters. Remove trailing whitespace.
You can auto-fix with:
- Editor setting “trim trailing whitespace on save”, or
sed -i -e 's/[[:space:]]\+$//' .github/workflows/rust.ymlAlso applies to: 71-71, 95-95, 115-115, 124-124, 127-127, 136-136, 139-139, 148-148, 151-151
📜 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)
.github/workflows/fuzz.yml(1 hunks).github/workflows/rust.yml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/fuzz.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.919Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: When adding new FFI functions in the Rust layer, implement them with #[no_mangle] extern "C" and proper cbindgen annotations
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to **/*.rs : Format code with rustfmt and keep formatting clean
Applied to files:
.github/workflows/rust.yml
🪛 YAMLlint (1.37.1)
.github/workflows/rust.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 71-71: trailing spaces
(trailing-spaces)
[error] 95-95: trailing spaces
(trailing-spaces)
[error] 115-115: trailing spaces
(trailing-spaces)
[error] 124-124: trailing spaces
(trailing-spaces)
[error] 127-127: trailing spaces
(trailing-spaces)
[error] 136-136: trailing spaces
(trailing-spaces)
[error] 139-139: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 151-151: trailing spaces
(trailing-spaces)
⏰ 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). (10)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (3)
.github/workflows/rust.yml (3)
35-39: LGTM: modern toolchain action usage is consistent across jobs.Good migration to dtolnay/rust-toolchain@stable and consistent component installation. This should resolve action runtime issues and simplify toolchain setup.
Also applies to: 53-56, 79-82, 103-106, 159-162, 230-233, 257-259
173-176: Commented example uses actions/checkout@v4 and toolchain@stable—good.No action needed; leaving a note that these are already modernized in comments. If you later enable the Cross job, keep them as-is.
83-94: Clippy job will likely fail: jq is not guaranteed to be present on ubuntu-latest.You depend on jq to parse cargo metadata. Install it explicitly before the script.
Apply this diff to add an install step:
- name: Setup Rust toolchain uses: dtolnay/rust-toolchain@stable with: toolchain: stable components: clippy + - name: Install jq + run: sudo apt-get update && sudo apt-get install -y jq - name: Run clippy (excluding strict-checked crates) run: | # Auto-discover all workspace crates and exclude strict-checked ones STRICT_CRATES=("key-wallet" "key-wallet-manager" "key-wallet-ffi")⛔ Skipped due to learnings
Learnt from: CR PR: dashpay/rust-dashcore#0 File: CLAUDE.md:0-0 Timestamp: 2025-08-16T04:13:21.866Z Learning: Applies to **/*.rs : Code must pass clippy with -D warnings (no clippy warnings allowed)Learnt from: CR PR: dashpay/rust-dashcore#0 File: dash-spv/CLAUDE.md:0-0 Timestamp: 2025-08-16T04:14:57.808Z Learning: Applies to dash-spv/**/*.rs : Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
QuantumExplorer
changed the title
|
Self Reviewed, excellent work myself :) |
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores