refactor: key wallet clippy warnings #123
Conversation
WalkthroughMarked BIP38 FFI functions as unsafe and added Safety docs; renamed an unused network parameter to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as External C caller
participant H as FFI header boundary
participant R as Rust bip38 impl
Note over C,H: Caller invokes BIP38 FFI (unsafe boundary)
C->>H: bip38_encrypt_private_key(private_key*, passphrase*, _network, error*)
H->>R: enter unsafe extern "C" function
R->>R: dereference pointers via CStr::from_ptr, validate UTF‑8
alt success
R->>H: return allocated C string pointer
H->>C: pointer to encrypted string
else failure
R->>H: write `error` (if non-null), return null
H->>C: null
end
Note over C,R: Caller must ensure pointer validity for the call duration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (16)
✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rs (1)
312-331: Exact-match flag is incorrect when change is dustDocs say exact_match means “no change needed”. When change_amount is dust, you suppress the change output (final_change == 0) but still set exact_match = false. Set it based on the absence of a change output.
Apply:
- // Check if change is dust - let (final_change, exact_match) = if change_amount < self.dust_threshold { - // Add dust to fee - (0, change_amount == 0) - } else { - (change_amount, false) - }; + // Check if change is dust (no change output if dust or exactly zero) + let is_dust = change_amount < self.dust_threshold; + let final_change = if is_dust { 0 } else { change_amount }; + let exact_match = final_change == 0;key-wallet/src/bip32.rs (2)
108-111: Fix extra closing paren in Debug fmt for ChainCode.Current output includes an extra “)”.
Apply:
- write!(f, "ChainCode({}))", self) + write!(f, "ChainCode({})", self)
253-256: Fix extra closing paren in Debug fmt for Fingerprint.Same issue as ChainCode.
Apply:
- write!(f, "Fingerprint({}))", self) + write!(f, "Fingerprint({})", self)key-wallet/src/account/serialization.rs (2)
10-13: Preserve error sources and add decode size limits for bincode.
- Don’t downgrade bincode errors to strings; keep sources with thiserror so callers can act on them.
- Add a safe upper bound when decoding to avoid OOM on untrusted input.
@@ -use crate::Account; +use crate::Account; + +// Safer bincode configuration with a conservative size limit (tune as needed) +#[cfg(feature = "bincode")] +fn bincode_cfg() -> bincode::config::Configuration { + // e.g., 64 KiB cap for account blobs; adjust if legitimate payloads exceed this. + bincode::config::standard().with_limit(64 * 1024) +} @@ - bincode::encode_to_vec(self, bincode::config::standard()) - .map_err(|e| crate::error::Error::Serialization(e.to_string())) + bincode::encode_to_vec(self, bincode_cfg()) + .map_err(crate::error::Error::from) @@ - bincode::decode_from_slice(data, bincode::config::standard()) + bincode::decode_from_slice(data, bincode_cfg()) .map(|(account, _)| account) - .map_err(|e| crate::error::Error::Serialization(e.to_string())) + .map_err(crate::error::Error::from) @@ - bincode::encode_to_vec(self, bincode::config::standard()) - .map_err(|e| crate::error::Error::Serialization(e.to_string())) + bincode::encode_to_vec(self, bincode_cfg()) + .map_err(crate::error::Error::from) @@ - bincode::decode_from_slice(data, bincode::config::standard()) + bincode::decode_from_slice(data, bincode_cfg()) .map(|(account, _)| account) - .map_err(|e| crate::error::Error::Serialization(e.to_string())) + .map_err(crate::error::Error::from) @@ - bincode::encode_to_vec(self, bincode::config::standard()) - .map_err(|e| crate::error::Error::Serialization(e.to_string())) + bincode::encode_to_vec(self, bincode_cfg()) + .map_err(crate::error::Error::from) @@ - bincode::decode_from_slice(data, bincode::config::standard()) + bincode::decode_from_slice(data, bincode_cfg()) .map(|(account, _)| account) - .map_err(|e| crate::error::Error::Serialization(e.to_string())) + .map_err(crate::error::Error::from)Follow-up:
- Ensure crate::error::Error implements From for bincode encode/decode errors (or add dedicated variants) instead of stringifying. If not present, I can add the variants and impls.
Also applies to: 17-21, 28-31, 35-39, 46-49, 53-57
24-40: Disallow bincode serialization of private keys
BLSAccount and EdDSAAccount each contain private key fields (ExtendedBLSPrivKey/ExtendedEd25519PrivKey), and the existing#[cfg(feature = "bincode")] derive(Encode, Decode)plusto_bytes/from_bytesimplementations will serialize raw secret material. Replace these with custom serialization that omits or encrypts secrets under an explicit feature flag.key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
154-161: Don’t mark change address used when no change output is createdYou always mark the change address used even if the builder didn’t create a change output (e.g., exact spend). This prematurely burns an address.
Apply:
- // Mark the change address as used in the managed account - managed_account.mark_address_used(&change_address); + // Mark change address as used only if a change output was actually created + let change_script = change_address.script_pubkey(); + let has_change = transaction + .output + .iter() + .any(|o| o.script_pubkey == change_script); + if has_change { + managed_account.mark_address_used(&change_address); + }key-wallet/src/derivation_slip10.rs (2)
60-66: Remove unreachable expects in library codeThese try_into().expect(...) cannot fail but still introduce panic paths in library code. Prefer copy_from_slice to avoid unwrap/expect entirely.
Apply this diff:
- // First 32 bytes are the private key seed - let private_key: [u8; 32] = hmac_bytes[..32].try_into().expect("HMAC output is 64 bytes"); - - // Last 32 bytes are the chain code - let chain_code = - ChainCode::from_bytes(hmac_bytes[32..].try_into().expect("HMAC output is 64 bytes")); + // First 32 bytes are the private key seed + let mut private_key = [0u8; 32]; + private_key.copy_from_slice(&hmac_bytes[..32]); + + // Last 32 bytes are the chain code + let mut cc = [0u8; 32]; + cc.copy_from_slice(&hmac_bytes[32..]); + let chain_code = ChainCode::from_bytes(cc); @@ - // First 32 bytes become the new private key seed - let private_key: [u8; 32] = hmac_bytes[..32].try_into().expect("HMAC output is 64 bytes"); - - // Last 32 bytes become the new chain code - let chain_code = - ChainCode::from_bytes(hmac_bytes[32..].try_into().expect("HMAC output is 64 bytes")); + // First 32 bytes become the new private key seed + let mut private_key = [0u8; 32]; + private_key.copy_from_slice(&hmac_bytes[..32]); + + // Last 32 bytes become the new chain code + let mut cc = [0u8; 32]; + cc.copy_from_slice(&hmac_bytes[32..]); + let chain_code = ChainCode::from_bytes(cc);Also applies to: 106-112
136-139: Avoid expects when slicing for fingerprintsReplace try_into().expect(...) with copy_from_slice to eliminate panics and keep library code non-panicking.
- let hash = hash160::Hash::hash(&pubkey.to_bytes()); - Ok(Fingerprint::from_bytes(hash[..4].try_into().expect("hash160 has enough bytes"))) + let hash = hash160::Hash::hash(&pubkey.to_bytes()); + let mut fp = [0u8; 4]; + fp.copy_from_slice(&hash[..4]); + Ok(Fingerprint::from_bytes(fp)) @@ - let hash = hash160::Hash::hash(self.public_key.as_ref()); - Fingerprint::from_bytes(hash[..4].try_into().expect("hash160 has enough bytes")) + let hash = hash160::Hash::hash(self.public_key.as_ref()); + let mut fp = [0u8; 4]; + fp.copy_from_slice(&hash[..4]); + Fingerprint::from_bytes(fp)Also applies to: 286-288
key-wallet-ffi/src/managed_wallet.rs (1)
1105-1115: UB: freeing Rust-allocated slice with libc::freeaddresses_out points to a Box<[ *mut c_char ]> allocated via Box::into_raw (Rust global allocator). Using libc::free here is undefined behavior. Use the provided address_array_free which frees both the array and its strings.
Apply this diff:
- unsafe { - let addresses = std::slice::from_raw_parts(addresses_out, count_out); - for &addr_ptr in addresses { - let addr_str = CStr::from_ptr(addr_ptr).to_string_lossy(); - assert!(!addr_str.is_empty()); - println!("External address: {}", addr_str); - let _ = CString::from_raw(addr_ptr); - } - libc::free(addresses_out as *mut libc::c_void); - } + unsafe { + let addresses = std::slice::from_raw_parts(addresses_out, count_out); + for &addr_ptr in addresses { + let addr_str = CStr::from_ptr(addr_ptr).to_string_lossy(); + assert!(!addr_str.is_empty()); + println!("External address: {}", addr_str); + } + // Correct Rust-side deallocation of array + strings + crate::address::address_array_free(addresses_out, count_out); + }I can scan other tests for the same pattern if you’d like.
key-wallet/src/tests/immature_transaction_tests.rs (1)
214-222: Use u32 for heights and avoid implicit i32 math. Also remove unused txids.Make the heights array explicitly u32 to match block heights and prevent mixed-type arithmetic. The txids Vec is built but never used.
Apply:
- let heights = [100, 0, 200, 50]; - let mut txids = Vec::new(); + let heights: [u32; 4] = [100, 0, 200, 50]; - for (i, height) in heights.iter().enumerate() { + for (i, height) in heights.iter().copied().enumerate() { let tx = create_test_coinbase(100000 + i as u32, 5000000000); - txids.push(tx.txid()); - - collection.insert(ImmatureTransaction::new(tx, 100000 + height, block_hash, 0, 100, true)); + collection.insert(ImmatureTransaction::new( + tx, + 100_000u32 + height, + block_hash, + 0, + 100, + true, + )); }key-wallet-ffi/include/key_wallet_ffi.h (3)
1294-1301: Conflicting duplicate function declarations (invalid C).You declare the same symbol twice with different return types or duplicate signatures. C has no overloading; this will break consumers.
- account_collection_get_provider_operator_keys: returns FFIBLSAccount* (Line 1294) and void* (Line 1300).
- account_collection_get_provider_platform_keys: returns FFIEdDSAAccount* (Line 1321) and void* (Line 1327).
- managed_account_collection_get_provider_operator_keys: declared twice with same signature (Lines 2609 and 2616).
- managed_account_collection_get_provider_platform_keys: declared twice with same signature (Lines 2641 and 2648).
Minimum fix (keep one symbol, one signature; return NULL when feature disabled):
-FFIBLSAccount *account_collection_get_provider_operator_keys(...); -void *account_collection_get_provider_operator_keys(const FFIAccountCollection *_collection); +FFIBLSAccount *account_collection_get_provider_operator_keys(...); +/* When BLS is disabled, this function returns NULL at runtime. */ -FFIEdDSAAccount *account_collection_get_provider_platform_keys(...); -void *account_collection_get_provider_platform_keys(const FFIAccountCollection *_collection); +FFIEdDSAAccount *account_collection_get_provider_platform_keys(...); +/* When EdDSA is disabled, this function returns NULL at runtime. */ -FFIManagedAccount *managed_account_collection_get_provider_operator_keys(...); -FFIManagedAccount *managed_account_collection_get_provider_operator_keys(const FFIManagedAccountCollection *_collection); +FFIManagedAccount *managed_account_collection_get_provider_operator_keys(...); -FFIManagedAccount *managed_account_collection_get_provider_platform_keys(...); -FFIManagedAccount *managed_account_collection_get_provider_platform_keys(const FFIManagedAccountCollection *_collection); +FFIManagedAccount *managed_account_collection_get_provider_platform_keys(...);Action on Rust side: gate only the implementation with #[cfg(...)] but always emit the same extern "C" signature; remove separate stub exports.
Also applies to: 1321-1328, 2616-2617, 2648-2649
425-429: Unknown type ManagedWalletInfo in a public struct.The definition exposes ManagedWalletInfo*, which is not declared. This is a hard C compile error. Either forward-declare it or, better, reference the existing FFIManagedWalletInfo.
Apply one of:
-typedef struct { - ManagedWalletInfo *inner; -} FFIManagedWallet; +typedef struct { + FFIManagedWalletInfo *inner; +} FFIManagedWallet;or make FFIManagedWallet opaque (preferable for ABI stability):
-typedef struct { - ManagedWalletInfo *inner; -} FFIManagedWallet; +/* Opaque wrapper */ +typedef struct FFIManagedWallet FFIManagedWallet;
1294-1301: Remove duplicate provider-keys prototypes
- account_collection_get_provider_operator_keys: real at 1294 vs stub at 1300
- account_collection_get_provider_platform_keys: real at 1321 vs stub at 1327
- managed_account_collection_get_provider_operator_keys: real at 2609 vs stub at 2616
- managed_account_collection_get_provider_platform_keys: real at 2641 vs stub at 2648
Either remove the redundant stub declarations or wrap/rename them under the BLS-disabled guard so they don’t collide with the real prototypes.
key-wallet/src/tests/utxo_tests.rs (1)
400-417: Non-deterministic UTXO selection may cause flaky tests.Iterating a HashMap yields arbitrary order; selected.len() may vary. For stable tests, iterate deterministically.
Apply:
- for utxo in self.utxos.values() { + let mut utxos: Vec<&Utxo> = self.utxos.values().collect(); + utxos.sort_by_key(|u| (u.confirmations.unwrap_or(0), u.value)); // or by outpoint + for utxo in utxos { if total >= target { break; } selected.push(utxo.clone()); total += utxo.value; }key-wallet-ffi/src/account_tests.rs (1)
21-27: Free error message with the provided FFI helper to avoid allocator mismatches.Use account_result_free_error instead of reconstructing CString.
Apply:
- if !result.error_message.is_null() { - unsafe { - let _ = CString::from_raw(result.error_message); - } - } + if !result.error_message.is_null() { + let mut r = result; + unsafe { account_result_free_error(&mut r) }; + }
🧹 Nitpick comments (28)
key-wallet/src/transaction_checking/transaction_router/tests/provider.rs (3)
878-885: Replace magic numbers with a coin unit constant for clarity and maintainability.Use a local constant and multiply, so "10 DASH" intent is explicit and consistent across tests.
- let mut tx = create_test_transaction(1, vec![1_000_000_000]); // 10 DASH returned collateral + let mut tx = create_test_transaction(1, vec![10 * COIN]); // 10 DASH returned collateral @@ - tx.output.push(TxOut { - value: 1_000_000_000, + tx.output.push(TxOut { + value: 10 * COIN, script_pubkey: return_address.script_pubkey(), });Add once near the top of this module (outside the selected range):
const COIN: u64 = 100_000_000; // duffs per DASH
932-934: Align the assertion with the coin unit constant.Keeps the expectation in step with the suggested constant usage.
- result.total_received, 1_000_000_000, + result.total_received, 10 * COIN,
225-231: Use readable numeric literals to satisfy clippy’sunreadable_literaland match the new style.Underscores improve readability and keep formatting consistent in this file.
- height: 100000, + height: 100_000, @@ - timestamp: Some(1234567890), + timestamp: Some(1_234_567_890),(Apply the same in all four TransactionContext blocks in this file.)
Also applies to: 361-366, 701-707, 919-923
key-wallet/tests/test_optimal_consolidation.rs (1)
62-64: Remove debug prints from testsThese add noise in CI logs. Prefer asserting on values instead.
- println!("Selected {} UTXOs with total value {}", result.selected.len(), result.total_value); - println!("Selected values: {:?}", selected_values);key-wallet/tests/psbt.rs (1)
279-286: Nit: fix spelling and param name (“indices”).Rename for clarity and consistency; no behavioral change.
-/// `pk_path` holds tuples of `(public_key, derivation_path)`. `indecies` is used to access the +/// `pk_path` holds tuples of `(public_key, derivation_path)`. `indices` is used to access the /// `pk_path` vector. `fingerprint` is from the parent extended public key. fn bip32_derivation( fingerprint: Fingerprint, pk_path: &[(&str, &str)], - indecies: Vec<usize>, + indices: Vec<usize>, ) -> BTreeMap<secp256k1::PublicKey, KeySource> { let mut tree = BTreeMap::new(); - for i in indecies { + for i in indices { let pk = pk_path[i].0; let path = pk_path[i].1;Also applies to: 287-296
key-wallet/src/bip32.rs (3)
940-942: Doc typo: “Convers” → “Converts”.Minor doc fix.
- /// Convers a given type into a [`DerivationPath`] with possible error + /// Converts a given type into a [`DerivationPath`] with possible error
1696-1716: Avoid panics in library code (encode_256, priv).These arms are unreachable by construction but still panic; prefer unreachable! or refactor to prove invariants at the type level.
- _ => panic!("Invalid child number for 256-bit format"), + _ => unreachable!("Invalid child number for 256-bit format"),- _ => panic!("Invalid child number for 256-bit format"), + _ => unreachable!("Invalid child number for 256-bit format"),
1942-1963: Avoid panics in library code (encode_256, pub).Mirror the change in the public-key variant.
- _ => panic!("Invalid child number for 256-bit format"), + _ => unreachable!("Invalid child number for 256-bit format"),- _ => panic!("Invalid child number for 256-bit format"), + _ => unreachable!("Invalid child number for 256-bit format"),key-wallet/src/account/serialization.rs (1)
127-137: Be explicit about borrowing when passing public key bytes.For consistency with the BLS test and to avoid accidental moves when signatures change, pass a slice reference.
- public_key_bytes, + &public_key_bytes,key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
24-37: Unify error handling with thiserror and From conversionsLocal stringly-typed errors make propagation noisy. Prefer a crate-level Error with thiserror and From/From to use ? directly.
Example sketch:
-#[derive(Debug)] -pub enum TransactionError { ... } +#[derive(thiserror::Error, Debug)] +pub enum TransactionError { + #[error("no account for requested type")] + NoAccount, + #[error("insufficient funds")] + InsufficientFunds, + #[error("change address generation failed: {0}")] + ChangeAddressGeneration(String), + #[error(transparent)] + BuildFailed(#[from] BuilderError), + #[error(transparent)] + CoinSelection(#[from] SelectionError), +}Then replace manual map_err chains with ?.
key-wallet-ffi/src/keys_tests.rs (1)
642-648: Fix misleading comment about coin typePaths below use testnet coin type 1, not Dash mainnet coin type 5.
- // Test different derivation paths (Dash coin type 5) + // Test different derivation paths on testnet (coin type 1)key-wallet/src/tests/edge_case_tests.rs (1)
33-41: This test is currently inert; either implement assertions or mark ignored.
_test_casesis unused and no validation occurs. Implement parsing assertions once available, or add#[ignore]with a TODO to avoid a false-positive “passing” test. Consider proptest for fuzzing invalid paths later.key-wallet-ffi/include/key_wallet_ffi.h (1)
14-26: Minor: deduplicate repeated stdint/stddef/stdbool includes.Harmless but noisy for consumers. Consider having cbindgen emit each once.
key-wallet/src/tests/utxo_tests.rs (1)
442-457: Prefer derive over manual Clone impl.Manual clone mirrors field list and can drift. All fields already implement Clone/Copy.
Apply:
- pub struct Utxo { + #[derive(Clone)] + pub struct Utxo { pub outpoint: OutPoint, pub value: u64, pub script_pubkey: ScriptBuf, pub address: Option<Address>, pub is_coinbase: bool, pub confirmations: Option<u32>, pub block_height: Option<u32>, pub account_index: Option<u32>, pub address_index: Option<u32>, pub is_change: bool, } - - impl Clone for Utxo { - fn clone(&self) -> Self { /* ... */ } - }key-wallet-ffi/src/account_tests.rs (2)
195-203: Free returned strings with string_free after reading.Safer and matches the C header contract than taking ownership via CString::from_raw.
Apply:
- let xpub_str = account_get_extended_public_key_as_string(result.account); - assert!(!xpub_str.is_null()); - let xpub = CString::from_raw(xpub_str); - let xpub_string = xpub.to_string_lossy(); + let xpub_str = account_get_extended_public_key_as_string(result.account); + assert!(!xpub_str.is_null()); + let xpub_string = std::ffi::CStr::from_ptr(xpub_str).to_string_lossy().into_owned(); + string_free(xpub_str);
225-231: Same: free account_result error via FFI helper.Apply:
- if !result.error_message.is_null() { - unsafe { - let _ = CString::from_raw(result.error_message); - } - } + if !result.error_message.is_null() { + let mut r = result; + unsafe { account_result_free_error(&mut r) }; + }key-wallet-ffi/src/wallet_manager_serialization_tests.rs (1)
368-374: Switch to stack array is fine; align element type with FFI to avoid implicit inference.Looks good for lifetime and contiguity. Minor: explicitly use the FFI’s integer type (u32 or c_uint) to avoid inference depending on field types in FFIWalletAccountCreationOptions.
Example:
- let bip44_indices = [0u32, 1u32, 2u32]; + use std::os::raw::c_uint; + let bip44_indices: [c_uint; 3] = [0, 1, 2];key-wallet-ffi/tests/integration_test.rs (1)
229-239: LGTM; deterministic invalid-seed case maintained. Consider an additional length-mismatch test.Optional: also pass a valid pointer with an incorrect larger length (e.g., 65) to ensure the callee validates both pointer and size.
key-wallet-ffi/src/managed_account.rs (5)
107-111: Remove unused wallet_id_array copy.wallet_id_array is written but never read; the function uses wallet_id directly. Drop the dead copy to reduce confusion.
- // Convert wallet_id to array - let mut wallet_id_array = [0u8; 32]; - std::ptr::copy_nonoverlapping(wallet_id, wallet_id_array.as_mut_ptr(), 32); + // wallet_id is validated above; no need to copy it locally
239-241: Same here: remove unused wallet_id_array copy.This block mirrors the earlier redundant copy; safe to delete.
- // Convert wallet_id to array - let mut wallet_id_array = [0u8; 32]; - std::ptr::copy_nonoverlapping(wallet_id, wallet_id_array.as_mut_ptr(), 32); + // wallet_id is validated above; no need to copy it locally
859-862: Be explicit about index element type to match FFI.Use u32 or c_uint explicitly for indices to avoid type inference surprises and keep clippy quiet across platforms.
- let bip44_indices = [0]; + use std::os::raw::c_uint; + let bip44_indices: [c_uint; 1] = [0];
932-942: Same type explicitness for all index arrays.Make element types explicit for BIP44/BIP32/CoinJoin arrays for consistency with the FFI fields.
- let bip44_indices = [0, 1, 2]; - let bip32_indices = [0]; - let coinjoin_indices = [0]; + use std::os::raw::c_uint; + let bip44_indices: [c_uint; 3] = [0, 1, 2]; + let bip32_indices: [c_uint; 1] = [0]; + let coinjoin_indices: [c_uint; 1] = [0];
1264-1267: Type the CoinJoin indices explicitly.Match the FFI integer width explicitly for clarity.
- let coinjoin_indices = [0]; + use std::os::raw::c_uint; + let coinjoin_indices: [c_uint; 1] = [0];key-wallet-ffi/tests/test_addr_simple.rs (1)
10-18: LGTM; optional: zeroize seed after use in tests.If you want to keep test hygiene, wipe the seed before returning.
- let seed = [0x42u8; 64]; + let mut seed = [0x42u8; 64]; @@ unsafe { key_wallet_ffi::wallet::wallet_free(wallet); } + // Test-only hygiene + seed.fill(0);key-wallet-ffi/src/account_collection.rs (2)
1261-1277: Make index arrays’ element type explicit to match FFI fields.Minor consistency/readability win and avoids cross-platform integer width assumptions.
- let bip44_indices = [0, 4, 5, 8]; - let bip32_indices = [0]; - let coinjoin_indices = [0, 1]; - let topup_indices = [0, 1, 2]; + use std::os::raw::c_uint; + let bip44_indices: [c_uint; 4] = [0, 4, 5, 8]; + let bip32_indices: [c_uint; 1] = [0]; + let coinjoin_indices: [c_uint; 2] = [0, 1]; + let topup_indices: [c_uint; 3] = [0, 1, 2];
1405-1421: Same nit: type arrays explicitly for FFI.Mirror earlier suggestion to keep types explicit in summary_data setup.
- let bip44_indices = [0, 4, 5, 8]; - let bip32_indices = [0]; - let coinjoin_indices = [0, 1]; - let topup_indices = [0, 1, 2]; + use std::os::raw::c_uint; + let bip44_indices: [c_uint; 4] = [0, 4, 5, 8]; + let bip32_indices: [c_uint; 1] = [0]; + let coinjoin_indices: [c_uint; 2] = [0, 1]; + let topup_indices: [c_uint; 3] = [0, 1, 2];key-wallet-ffi/src/wallet_manager_tests.rs (2)
1002-1002: Prefer a shared ZERO_ID constant.You introduce
[0u8; 32]here and elsewhere; consider a module-level const for reuse.Add once at top-level:
+const WALLET_ID_LEN: usize = 32; +const ZERO_WALLET_ID: [u8; WALLET_ID_LEN] = [0u8; WALLET_ID_LEN];Then replace occurrences:
-let invalid_wallet_id = [0u8; 32]; +let invalid_wallet_id = ZERO_WALLET_ID;
1250-1250: Reuse ZERO_ID here as well.Same suggestion as above to de-duplicate the all-zero ID.
-let invalid_wallet_id = [0u8; 32]; +let invalid_wallet_id = ZERO_WALLET_ID;
| if let Err(ref e) = change { | ||
| match e { | ||
| WalletError::InvalidNetwork => { | ||
| // This is expected given the current implementation | ||
| return; | ||
| } | ||
| WalletError::InvalidNetwork => {} | ||
| _ => panic!("Unexpected error: {:?}", e), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not swallow the error: this makes the test vacuous.
Turning the InvalidNetwork arm into a no-op means the test can pass without asserting anything about change-address generation. Prefer asserting the expected outcome (Ok or the exact Err) so failures surface.
Apply this diff to strengthen the assertion:
- if let Err(ref e) = change {
- match e {
- WalletError::InvalidNetwork => {}
- _ => panic!("Unexpected error: {:?}", e),
- }
- }
+ assert!(
+ matches!(change, Ok(_) | Err(WalletError::InvalidNetwork)),
+ "Unexpected error: {:?}",
+ change
+ );Optional: mirror this pattern for the receive-address block above so both paths are asserted consistently instead of early-returning.
🤖 Prompt for AI Agents
In key-wallet-manager/tests/integration_test.rs around lines 115 to 119, the
test currently swallows WalletError::InvalidNetwork by leaving that match arm
empty which makes the test vacuous; change the match to assert the exact
expected outcome instead of a no-op — e.g., replace the match with an assertion
that `change` equals `Err(WalletError::InvalidNetwork)` (or use
`assert!(matches!(change, Err(WalletError::InvalidNetwork)))`), so the test
fails on any other result; optionally apply the same stronger assertion pattern
to the receive-address block above for consistent behavior.
| public_key_bytes, | ||
| Network::Testnet, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass typed 48-byte key or an explicit slice; avoid latent length drift.
Use the dedicated 48-byte accessor and pass a slice explicitly. This makes the intent clear and prevents accidental acceptance of non-48-byte encodings.
- let public_key_bytes = bls_public.to_bytes();
+ let public_key_bytes = bls_public.public_key_bytes(); // [u8; 48]
...
- public_key_bytes,
+ &public_key_bytes,📝 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.
| public_key_bytes, | |
| Network::Testnet, | |
| ) | |
| // Use the fixed-length accessor to guarantee exactly 48 bytes | |
| let public_key_bytes = bls_public.public_key_bytes(); // [u8; 48] | |
| // … other code … | |
| // Pass an explicit slice to make byte-length intent clear | |
| &public_key_bytes, | |
| Network::Testnet, | |
| ) |
🤖 Prompt for AI Agents
In key-wallet/src/account/serialization.rs around lines 99 to 101, the code
currently passes public_key_bytes directly which can mask incorrect lengths;
change to use the dedicated 48-byte accessor (e.g., public_key.to_bytes48() or
equivalent method) and pass an explicit slice (&bytes[..48]) so the API receives
a typed 48-byte value/slice, ensuring compilation-level length correctness and
avoiding latent length drift.
| let mainnet_account = | ||
| Account::from_xpriv(Some([0u8; 32]), account_type.clone(), account_key, dash_mainnet) | ||
| .unwrap(); | ||
| Account::from_xpriv(Some([0u8; 32]), account_type, account_key, dash_mainnet).unwrap(); |
There was a problem hiding this comment.
Avoid cross-network key reuse in tests.
You derive account_key using Testnet path, then construct a Dash-mainnet account with that key. This violates the “never mix mainnet and testnet operations” guidance and could mask bugs.
Use a mainnet-derived key:
- let dash_mainnet = Network::Dash;
- let mainnet_account =
- Account::from_xpriv(Some([0u8; 32]), account_type, account_key, dash_mainnet).unwrap();
+ let dash_mainnet = Network::Dash;
+ let derivation_path_mainnet = account_type.derivation_path(dash_mainnet).unwrap();
+ let master_mainnet = create_test_extended_priv_key(dash_mainnet);
+ let hd_wallet_mainnet = HDWallet::new(master_mainnet);
+ let account_key_mainnet = hd_wallet_mainnet.derive(&derivation_path_mainnet).unwrap();
+ let mainnet_account = Account::from_xpriv(
+ Some([0u8; 32]),
+ account_type,
+ account_key_mainnet,
+ dash_mainnet,
+ ).unwrap();📝 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.
| Account::from_xpriv(Some([0u8; 32]), account_type, account_key, dash_mainnet).unwrap(); | |
| let dash_mainnet = Network::Dash; | |
| - let mainnet_account = | |
| // Derive a fresh account key for mainnet instead of reusing the testnet key | |
| let derivation_path_mainnet = account_type.derivation_path(dash_mainnet).unwrap(); | |
| let master_mainnet = create_test_extended_priv_key(dash_mainnet); | |
| let hd_wallet_mainnet = HDWallet::new(master_mainnet); | |
| let account_key_mainnet = hd_wallet_mainnet | |
| .derive(&derivation_path_mainnet) | |
| .unwrap(); | |
| let mainnet_account = Account::from_xpriv( | |
| Some([0u8; 32]), | |
| account_type, | |
| account_key_mainnet, | |
| dash_mainnet, | |
| ).unwrap(); |
🤖 Prompt for AI Agents
In key-wallet/src/tests/account_tests.rs around line 411, the test currently
derives account_key using a Testnet derivation path then constructs a
Dash-mainnet Account with that key, which mixes networks; update the test to
derive the account_key using the Dash mainnet derivation path (or use a mainnet
fixture) so the key and Account network match, then pass that mainnet-derived
account_key into Account::from_xpriv instead of the testnet-derived one.
| let address = | ||
| Address::p2pkh(&dashcore::PublicKey::from_slice(&[0x02; 33]).unwrap(), Network::Testnet); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panic: use a known-valid compressed secp256k1 pubkey; optional: keep .into() if required by Address API.
PublicKey::from_slice(&[0x02; 33]).unwrap() will fail on invalid point bytes and can panic; swap to a well-known valid key (G). If Address::p2pkh still requires dashcore::Network, append .into().
- let address =
- Address::p2pkh(&dashcore::PublicKey::from_slice(&[0x02; 33]).unwrap(), Network::Testnet);
+ let address = Address::p2pkh(
+ &dashcore::PublicKey::from_slice(&[
+ 0x02, 0x79, 0xBE, 0x66, 0x7E, 0xF9, 0xDC, 0xBB, 0xAC, 0x55, 0xA0, 0x62, 0x95, 0xCE,
+ 0x87, 0x0B, 0x07, 0x02, 0x9B, 0xFC, 0xDB, 0x2D, 0xCE, 0x28, 0xD9, 0x59, 0xF2, 0x81,
+ 0x5B, 0x16, 0xF8, 0x17, 0x98,
+ ]).unwrap(),
+ Network::Testnet
+ // .into() // uncomment if Address::p2pkh expects dashcore::Network
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let address = | |
| Address::p2pkh(&dashcore::PublicKey::from_slice(&[0x02; 33]).unwrap(), Network::Testnet); | |
| let address = Address::p2pkh( | |
| &dashcore::PublicKey::from_slice(&[ | |
| 0x02, 0x79, 0xBE, 0x66, 0x7E, 0xF9, 0xDC, 0xBB, 0xAC, 0x55, 0xA0, 0x62, 0x95, 0xCE, | |
| 0x87, 0x0B, 0x07, 0x02, 0x9B, 0xFC, 0xDB, 0x2D, 0xCE, 0x28, 0xD9, 0x59, 0xF2, 0x81, | |
| 0x5B, 0x16, 0xF8, 0x17, 0x98, | |
| ]).unwrap(), | |
| Network::Testnet | |
| // .into() // uncomment if Address::p2pkh expects dashcore::Network | |
| ); |
🤖 Prompt for AI Agents
In key-wallet/src/tests/transaction_tests.rs around lines 66–67, replace the
ad-hoc &[0x02; 33] bytes with a known-valid compressed secp256k1 public key (the
standard generator/compressed G bytes) so PublicKey::from_slice(...) cannot fail
at runtime, and keep or add `.into()` on the Network argument to Address::p2pkh
if the Address API requires conversion; in short, use a vetted 33-byte
compressed pubkey literal instead of repeated 0x02 bytes and ensure the Network
value is converted with `.into()` when needed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet-ffi/include/key_wallet_ffi.h (2)
3807-3821: BIP38 decrypt docs: missing free semantics and runtime behaviorLikewise, add return/free semantics and current behavior (NULL + WALLET_ERROR).
-/* - Decrypt a BIP38 encrypted private key - - # Safety - - This function is unsafe because it dereferences raw pointers: - - `encrypted_key` must be a valid, null-terminated C string - - `passphrase` must be a valid, null-terminated C string - - `error` must be a valid pointer to an FFIError or null - */ +/* + Decrypt a BIP38 encrypted private key + + Returns: newly allocated C string (WIF) on success; NULL on error. + - Free the returned string with `string_free`. + - Currently unimplemented: returns NULL and sets error->code = WALLET_ERROR (or when the `bip38` feature is disabled). + + # Safety + This function is unsafe because it dereferences raw pointers: + - `encrypted_key` must be a valid, null-terminated C string + - `passphrase` must be a valid, null-terminated C string + - `error` must be a valid pointer to an FFIError or null + */
1294-1301: Resolve duplicate/conflicting operator-keys prototypes and remove redundant managed-collection declaration
- Lines 1294–1300: Two conflicting variants of
account_collection_get_provider_operator_keys—returningFFIBLSAccount*vsvoid*—must be wrapped in a preprocessor guard (e.g.#if defined(KEY_WALLET_FFI_BLS) … #else … #endif).- Lines 2609–2616: Duplicate
managed_account_collection_get_provider_operator_keysprototypes; remove the redundant declaration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
dash/src/blockdata/transaction/mod.rs(2 hunks)key-wallet-ffi/include/key_wallet_ffi.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
Files:
dash/src/blockdata/transaction/mod.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
key-wallet-ffi/include/key_wallet_ffi.h
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/ffi/**/*.rs : Expose safe C FFI with #[no_mangle] extern "C" functions only for vetted APIs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Include integration tests that cover full wallet workflows
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Use property-based tests for complex invariants where suitable
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/**/*.rs : Prefer using public keys or key fingerprints for identification in logs and messages instead of private keys
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
🧬 Code graph analysis (2)
dash/src/blockdata/transaction/mod.rs (1)
dash/src/consensus/encode.rs (1)
serialize(222-227)
key-wallet-ffi/include/key_wallet_ffi.h (1)
key-wallet-ffi/src/bip38.rs (1)
bip38_encrypt_private_key(19-78)
⏰ 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). (20)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Clippy (Non-strict)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
🔇 Additional comments (2)
dash/src/blockdata/transaction/mod.rs (2)
471-471: Clippy: remove redundant&*— good cleanupPassing
selfdirectly toencode::serializeis idiomatic and equivalent to the previous&*self. No behavioral change; resolves the lint.
482-482: Avoiding unnecessary clone onOutPointReturning
UnknownSpentOutput(input.previous_output)removes the allocation. This is correct assumingOutPoint: Copy(as in rust-bitcoin). If that ever changes, this will fail to compile, which is acceptable.
| typedef struct { | ||
| ManagedWalletInfo *inner; | ||
| FFIManagedWalletInfo *inner; | ||
|
|
||
| } FFIManagedWallet; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make FFIManagedWallet opaque; avoid exposing inner pointer and ABI fragility
Publicly exposing the inner field invites consumers to dereference internals and makes future refactors risky. Prefer an opaque handle and keep mutations behind API calls. Also, several APIs still accept FFIManagedWalletInfo* directly (e.g., Lines 2728–2754, 2749–2754), which defeats the wrapper; consider unifying on one handle type.
Proposed header-side shape (implement by adjusting Rust types/cbindgen config, not by hand-editing this generated file):
-typedef struct {
- FFIManagedWalletInfo *inner;
-} FFIManagedWallet;
+typedef struct FFIManagedWallet FFIManagedWallet;Verification script to find mixed usage of FFIManagedWallet and FFIManagedWalletInfo in FFI entry points:
🏁 Script executed:
#!/bin/bash
rg -n -C1 -e '\bFFIManagedWallet\b' -e '\bFFIManagedWalletInfo\b' key-wallet-ffi/include/key_wallet_ffi.hLength of output: 4718
Make FFIManagedWallet opaque and consolidate on one handle
FFIManagedWallet is currently a concrete struct exposing an inner pointer (key-wallet-ffi/include/key_wallet_ffi.h:425–429), which couples consumers to Rust’s layout and risks ABI breakage. Switch to an opaque forward declaration and update all APIs—e.g., managed_wallet_get_next_bip44_receive_address (line 2728) and managed_wallet_get_next_bip44_change_address (line 2749)—to take FFIManagedWallet* instead of FFIManagedWalletInfo*.
-typedef struct {
- FFIManagedWalletInfo *inner;
-} FFIManagedWallet;
+typedef struct FFIManagedWallet FFIManagedWallet;📝 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.
| typedef struct { | |
| ManagedWalletInfo *inner; | |
| FFIManagedWalletInfo *inner; | |
| } FFIManagedWallet; | |
| typedef struct FFIManagedWallet FFIManagedWallet; |
🤖 Prompt for AI Agents
In key-wallet-ffi/include/key_wallet_ffi.h around lines 425 to 429, the
FFIManagedWallet is defined as a concrete struct exposing an inner pointer which
leaks Rust layout; change it to an opaque forward declaration (typedef struct
FFIManagedWallet FFIManagedWallet;) and remove the inner field, and then update
all public APIs that currently accept or return FFIManagedWalletInfo* to use
FFIManagedWallet* instead (notably managed_wallet_get_next_bip44_receive_address
at ~line 2728 and managed_wallet_get_next_bip44_change_address at ~line 2749) so
every function consistently uses the single opaque handle type; ensure header
includes no direct accessors to the inner type and update all
declarations/usages across the C header and any C callers to match the new
opaque pointer type.
| # Safety | ||
| This function is unsafe because it dereferences raw pointers: | ||
| - `private_key` must be a valid, null-terminated C string | ||
| - `passphrase` must be a valid, null-terminated C string | ||
| - `error` must be a valid pointer to an FFIError or null | ||
| */ | ||
|
|
||
| char *bip38_encrypt_private_key(const char *private_key, | ||
| const char *passphrase, | ||
| FFINetwork network, | ||
| FFINetwork _network, | ||
| FFIError *error) | ||
| ; |
There was a problem hiding this comment.
BIP38 encrypt docs: document allocation/free and current unimplemented behavior
The header doesn’t say how to free the returned string and that this currently always returns NULL with WALLET_ERROR (feature disabled or not implemented). Please document both; also note _network is reserved/ignored.
Apply this header doc tweak (mirror in Rust doc comments so cbindgen regenerates it):
-/*
- Encrypt a private key with BIP38
-
- # Safety
-
- This function is unsafe because it dereferences raw pointers:
- - `private_key` must be a valid, null-terminated C string
- - `passphrase` must be a valid, null-terminated C string
- - `error` must be a valid pointer to an FFIError or null
- */
+/*
+ Encrypt a private key with BIP38
+
+ Returns: newly allocated C string on success; NULL on error.
+ - Free the returned string with `string_free`.
+ - Currently unimplemented: returns NULL and sets error->code = WALLET_ERROR (or when the `bip38` feature is disabled).
+ - Note: `_network` is reserved/ignored at present.
+
+ # Safety
+ This function is unsafe because it dereferences raw pointers:
+ - `private_key` must be a valid, null-terminated C string
+ - `passphrase` must be a valid, null-terminated C string
+ - `error` must be a valid pointer to an FFIError or null
+ */📝 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.
| # Safety | |
| This function is unsafe because it dereferences raw pointers: | |
| - `private_key` must be a valid, null-terminated C string | |
| - `passphrase` must be a valid, null-terminated C string | |
| - `error` must be a valid pointer to an FFIError or null | |
| */ | |
| char *bip38_encrypt_private_key(const char *private_key, | |
| const char *passphrase, | |
| FFINetwork network, | |
| FFINetwork _network, | |
| FFIError *error) | |
| ; | |
| /* | |
| Encrypt a private key with BIP38 | |
| Returns: newly allocated C string on success; NULL on error. | |
| - Free the returned string with `string_free`. | |
| - Currently unimplemented: returns NULL and sets error->code = WALLET_ERROR (or when the `bip38` feature is disabled). | |
| - Note: `_network` is reserved/ignored at present. | |
| # Safety | |
| This function is unsafe because it dereferences raw pointers: | |
| - `private_key` must be a valid, null-terminated C string | |
| - `passphrase` must be a valid, null-terminated C string | |
| - `error` must be a valid pointer to an FFIError or null | |
| */ | |
| char *bip38_encrypt_private_key(const char *private_key, | |
| const char *passphrase, | |
| FFINetwork _network, | |
| FFIError *error) | |
| ; |
🤖 Prompt for AI Agents
In key-wallet-ffi/include/key_wallet_ffi.h around lines 3792 to 3805, the BIP38
encrypt function header lacks docs about ownership and current unimplemented
behavior: update the comment to state that the returned char* is heap-allocated
and must be freed by the caller (e.g. free()), or NULL on error; explicitly
document that this implementation currently always returns NULL and sets
error->code = WALLET_ERROR when the feature is disabled/not implemented; also
note that the _network parameter is reserved/ignored. Make the same text change
in the corresponding Rust doc comments so cbindgen regenerates the header with
these notes.
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)
dash/src/bip152.rs (1)
263-279: Make overflow behavior deterministic across debug/release builds.Right now, encode relies on plain
+/-overflow to panic, which only happens in debug. In release, it wraps, so the “should panic” test can silently pass with invalid encoding. Use checked arithmetic withexpectto guarantee the documented panic in all profiles, and clarify the docs.Apply:
impl Encodable for BlockTransactionsRequest { /// # Panics /// - /// Panics if the index overflows [`u64::MAX`]. This happens when [`BlockTransactionsRequest::indexes`] - /// contains an entry with the value [`u64::MAX`] as `u64` overflows during differential encoding. + /// Panics on differential-encoding under/overflow. + /// + /// This includes the case where `indexes` contains [`u64::MAX`] (overflow on `+ 1`) or is not + /// non-decreasing (underflow on the differential). fn consensus_encode<S: io::Write + ?Sized>(&self, mut s: &mut S) -> Result<usize, io::Error> { let mut len = self.block_hash.consensus_encode(&mut s)?; // Manually encode indexes because they are differentially encoded VarInts. len += VarInt(self.indexes.len() as u64).consensus_encode(&mut s)?; let mut last_idx = 0; - for idx in &self.indexes { - len += VarInt(*idx - last_idx).consensus_encode(&mut s)?; - last_idx = *idx + 1; // can panic here - } + for &idx in &self.indexes { + let diff = idx + .checked_sub(last_idx) + .expect("block index underflow during differential encoding"); + len += VarInt(diff).consensus_encode(&mut s)?; + last_idx = idx + .checked_add(1) + .expect("block index overflow during differential encoding"); + } Ok(len) } }
♻️ Duplicate comments (2)
key-wallet-ffi/include/key_wallet_ffi.h (2)
425-429: Make FFIManagedWallet opaque; stop exposing inner pointerRe-exposing
innerkeeps ABI brittle and enables misuse. Use an opaque handle and consolidate APIs to consumeFFIManagedWallet*(const where read-only) instead ofFFIManagedWalletInfo*.-typedef struct { - FFIManagedWalletInfo *inner; -} FFIManagedWallet; +typedef struct FFIManagedWallet FFIManagedWallet;Follow-up (Rust/cbindgen): model as an opaque type (e.g.,
pub struct FFIManagedWallet { _private: [u8; 0] }) and regenerate the header; update managed-wallet entry points to consistently takeFFIManagedWallet*.
3784-3791: FFI contract: document allocation/ownership and current behavior for BIP38 encryptCallers need explicit ownership rules and present behavior (currently returns NULL with WALLET_ERROR). Add these notes to avoid leaks and confusion.
-/* - Encrypt a private key with BIP38 - - # Safety +/* + Encrypt a private key with BIP38 + + Returns: newly allocated C string on success; NULL on error. + - Free the returned string with `string_free`. + - Currently unimplemented: returns NULL and sets error->code = WALLET_ERROR + (or when the `bip38` feature is disabled). + - Note: `_network` is reserved/ignored at present. + + # Safety This function is unsafe because it dereferences raw pointers: - `private_key` must be a valid, null-terminated C string - `passphrase` must be a valid, null-terminated C string - `error` must be a valid pointer to an FFIError or null */
🧹 Nitpick comments (4)
dash/src/merkle_tree/block.rs (1)
632-633: Guard against potential overflow in(258 * n)(optional).While tests keep
nsmall,(258 * n)can overflowusizein pathological/fuzz cases. Consider upcasting tou128for the intermediate.Apply:
- assert!(serialized.len() <= 10 + (258 * n).div_ceil(8)); + let expected_bytes = ((258u128 * n as u128).div_ceil(8)) as usize; + assert!(serialized.len() <= 10 + expected_bytes);dash/src/pow.rs (1)
1483-1485: Gate the test on bothserdeandbincodefeaturesThis test uses bincode Encode/Decode derives gated behind
feature = "bincode". With onlyserdeenabled, it won’t compile. Gate the test on both features.- #[cfg(feature = "serde")] + #[cfg(all(feature = "serde", feature = "bincode"))] #[test] fn u256_serde() {dash/src/sml/masternode_list_engine/mod.rs (1)
1406-1408: Include the error in panic for better triageCapture and print the error to speed up failure diagnosis.
- mn_list_engine.validate_quorum(quorum).unwrap_or_else(|_| { - panic!("expected to validate quorum {} in cycle hash {}", i, cycle_hash) - }); + mn_list_engine.validate_quorum(quorum).unwrap_or_else(|e| { + panic!("expected to validate quorum {} in cycle hash {}: {}", i, cycle_hash, e) + });dash/src/sml/quorum_entry/validation.rs (1)
137-147: Good switch to fixed-size array; zero alloc and cleaner iterationUsing a literal array for
real_keysavoids heap allocs and cloning in the loop. Consider doing the same in other tests for consistency.
📜 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)
dash/src/bip152.rs(1 hunks)dash/src/blockdata/fee_rate.rs(0 hunks)dash/src/consensus/encode.rs(3 hunks)dash/src/crypto/sighash.rs(0 hunks)dash/src/merkle_tree/block.rs(2 hunks)dash/src/network/message.rs(2 hunks)dash/src/pow.rs(1 hunks)dash/src/sml/masternode_list_engine/mod.rs(2 hunks)dash/src/sml/quorum_entry/validation.rs(4 hunks)key-wallet-ffi/include/key_wallet_ffi.h(2 hunks)
💤 Files with no reviewable changes (2)
- dash/src/crypto/sighash.rs
- dash/src/blockdata/fee_rate.rs
✅ Files skipped from review due to trivial changes (1)
- dash/src/network/message.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
Files:
dash/src/pow.rsdash/src/merkle_tree/block.rsdash/src/consensus/encode.rsdash/src/bip152.rsdash/src/sml/masternode_list_engine/mod.rsdash/src/sml/quorum_entry/validation.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
key-wallet-ffi/include/key_wallet_ffi.h
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/ffi/**/*.rs : Expose safe C FFI with #[no_mangle] extern "C" functions only for vetted APIs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.140Z
Learning: Applies to {dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift} : Be careful with memory management at FFI boundaries (C/Swift interop)
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Applied to files:
dash/src/consensus/encode.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash/src/bip152.rsdash/src/sml/quorum_entry/validation.rs
📚 Learning: 2025-02-25T06:19:32.230Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#51
File: dash/src/sml/masternode_list_entry/hash.rs:7-12
Timestamp: 2025-02-25T06:19:32.230Z
Learning: The `consensus_encode` method on `MasternodeListEntry` writing to a `Vec` buffer cannot fail, so using `.expect()` is appropriate rather than propagating the error with the `?` operator.
Applied to files:
dash/src/sml/masternode_list_engine/mod.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.h
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/address_tests.rs : Place address generation and validation tests in tests/address_tests.rs
Applied to files:
dash/src/sml/quorum_entry/validation.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Applied to files:
dash/src/sml/quorum_entry/validation.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Use property-based tests for complex invariants where suitable
Applied to files:
dash/src/sml/quorum_entry/validation.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Use deterministic testing with known vectors (fixed seeds) in Rust tests
Applied to files:
dash/src/sml/quorum_entry/validation.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible
Applied to files:
dash/src/sml/quorum_entry/validation.rs
🧬 Code graph analysis (3)
dash/src/sml/masternode_list_engine/mod.rs (1)
dash-spv/src/chain/chainlock_manager.rs (1)
bincode(418-418)
key-wallet-ffi/include/key_wallet_ffi.h (2)
key-wallet-ffi/src/bip38.rs (1)
bip38_encrypt_private_key(19-78)key-wallet-ffi/src/error.rs (1)
error(44-49)
dash/src/sml/quorum_entry/validation.rs (1)
dash/src/blockdata/block.rs (2)
block_hash(59-63)block_hash(211-213)
⏰ 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). (19)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: Core Components Tests
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: Clippy (Non-strict)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (11)
dash/src/merkle_tree/block.rs (1)
604-606: Ceiling division switch is correct and clearer.
ntx.div_ceil(2)precisely expresses ceil(ntx/2) and matches the previous(ntx + 1) / 2semantics. LGTM.dash/src/consensus/encode.rs (3)
1442-1442: Use ofio::Error::otheris idiomatic.Shorter and clearer than
io::Error::new(io::ErrorKind::Other, ...). LGTM.
1470-1470: Same here: idiomaticio::Error::other.Consistent with MSRV 1.89; good cleanup.
1621-1621: Ceiling-bytes calculation is correct.
size.div_ceil(8)matches byte-packing semantics and aligns with read/write helpers. LGTM.dash/src/pow.rs (1)
1493-1495: Good fix: remove needless borrow in bincode encode callPassing
uintby value is correct (U256 isCopy) and silences clippy::needless_borrow without changing behavior.dash/src/bip152.rs (1)
503-509: LGTM: simplify constant path tou64::MAX.This aligns with Clippy’s unnecessary qualification lint and keeps the intent clear.
dash/src/sml/masternode_list_engine/mod.rs (1)
1281-1281: Clippy: needless_borrow fixed — LGTMPassing the slice directly to bincode::decode_from_slice is correct and removes the extra reference without changing behavior.
Also applies to: 1287-1287, 1293-1293
key-wallet-ffi/include/key_wallet_ffi.h (1)
3795-3795: Param rename to _network to silence unused — LGTMMatches Rust side and avoids clippy::unused_variable without changing ABI.
dash/src/sml/quorum_entry/validation.rs (3)
297-301: Prefer copy over clone for hash reversal — LGTM
let mut reversed_hash = block_hash;leveragesCopysemantics of the array; the reversal is in-place and clear.
398-406: Passinner_sigby reference if supported
In the warm-up loop,inner_sigis moved on each call. Ifverify_secure_basic_with_mode::<Bls12381G2Impl, _>accepts a borrow, update the invocation to:- verify_secure_basic_with_mode::<Bls12381G2Impl, _>(&operator_keys, inner_sig, msg, SerializationFormat::Modern) + verify_secure_basic_with_mode::<Bls12381G2Impl, _>(&operator_keys, &inner_sig, msg, SerializationFormat::Modern)Otherwise retain a
.clone()or wrap in a borrowable helper. Then run
cargo clippy --all-targets --all-features -- -D warnings
to catch any move or unused-must-use issues.
412-419: Could not locateverify_secure_basic_with_modein the public API for blsful 2.5.7—signature is likely hidden or re-exported under a different name. Passinginner_sigby value works because the signature type implementsCopy. No change required here.
| Decrypt a BIP38 encrypted private key | ||
| # Safety | ||
| This function is unsafe because it dereferences raw pointers: | ||
| - `encrypted_key` must be a valid, null-terminated C string | ||
| - `passphrase` must be a valid, null-terminated C string | ||
| - `error` must be a valid pointer to an FFIError or null | ||
| */ |
There was a problem hiding this comment.
FFI contract: document allocation/ownership for BIP38 decrypt
State who frees the returned string and typical error path.
-/*
- Decrypt a BIP38 encrypted private key
-
- # Safety
+/*
+ Decrypt a BIP38 encrypted private key
+
+ Returns: newly allocated C string (WIF) on success; NULL on error.
+ - Free the returned string with `string_free`.
+
+ # Safety
This function is unsafe because it dereferences raw pointers:
- `encrypted_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/📝 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.
| Decrypt a BIP38 encrypted private key | |
| # Safety | |
| This function is unsafe because it dereferences raw pointers: | |
| - `encrypted_key` must be a valid, null-terminated C string | |
| - `passphrase` must be a valid, null-terminated C string | |
| - `error` must be a valid pointer to an FFIError or null | |
| */ | |
| /* | |
| Decrypt a BIP38 encrypted private key | |
| Returns: newly allocated C string (WIF) on success; NULL on error. | |
| - Free the returned string with `string_free`. | |
| # Safety | |
| This function is unsafe because it dereferences raw pointers: | |
| - `encrypted_key` must be a valid, null-terminated C string | |
| - `passphrase` must be a valid, null-terminated C string | |
| - `error` must be a valid pointer to an FFIError or null | |
| */ |
🤖 Prompt for AI Agents
In key-wallet-ffi/include/key_wallet_ffi.h around lines 3800 to 3808, the BIP38
decrypt FFI comment lacks allocation/ownership and error-path details; update
the docblock to state precisely who owns and must free returned memory (e.g.,
the function returns a heap-allocated NUL-terminated C string which the caller
must free with the FFI's free function/standard free), and describe the error
path (on failure return NULL and, if an error pointer is provided, populate
error->message with a heap-allocated NUL-terminated C string that the caller
must also free; clarify that the error object itself is written by callee into
the provided pointer and not allocated). Ensure the wording names the allocator
used (malloc/ffi_free) and that both success and error message lifetimes are the
caller's responsibility.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
hashes/Cargo.toml (1)
8-8: Typo in package description.“eccosystem” → “ecosystem”.
-description = "Hash functions used by the rust-dash eccosystem" +description = "Hash functions used by the rust-dash ecosystem"hashes/src/util.rs (1)
594-598: Optional: enforce exact hex length in the regex (keep min/max if desired).Current minLength/maxLength already enforce length; making the regex exact reduces ambiguity and duplication.
- "pattern": "^[0-9a-fA-F]+$" + // Keep min/max to aid validators, and tighten the regex to exact length too. + "pattern": format!("^[0-9a-fA-F]{{{}}}$", $len * 2)Note:
json_schema!accepts interpolated expressions (likeformat!(...)) per schemars 1.0 docs. (docs.rs)
📜 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 (3)
hashes/Cargo.toml(2 hunks)hashes/src/util.rs(1 hunks)key-wallet-ffi/include/key_wallet_ffi.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/include/key_wallet_ffi.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
Files:
hashes/src/util.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/**/*.rs : Always validate network consistency; never mix mainnet and testnet operations
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
Learning: Applies to key-wallet/src/ffi/**/*.rs : Expose safe C FFI with #[no_mangle] extern "C" functions only for vetted APIs
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.140Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
🔇 Additional comments (1)
hashes/src/util.rs (1)
591-599: Good migration to schemars 1.0json_schema!and rootSchematypes.The new approach aligns with 1.0 changes (root
Schema,json_schema!), and the signature matcheswith = "..."expectations. LGTM.References: json_schema! macro and root
Schema/SchemaGeneratorin schemars 1.0. (docs.rs, graham.cool)
| std = ["alloc", "internals/std"] | ||
| alloc = ["internals/alloc"] | ||
| schemars = ["actual-schemars", "dyn-clone"] | ||
| schemars = ["actual-schemars"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Feature should gate on std to reflect the comment and avoid accidental no_std builds.
Tie the schemars feature to this crate’s std feature.
-schemars = ["actual-schemars"]
+schemars = ["std", "actual-schemars"]📝 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.
| schemars = ["actual-schemars"] | |
| schemars = ["std", "actual-schemars"] |
🤖 Prompt for AI Agents
In hashes/Cargo.toml around line 19, the "schemars" feature currently enables
only "actual-schemars" but must be gated on this crate's "std" feature; update
the features section so the "schemars" feature depends on "std" as well (i.e.,
include "std" in the feature's activation list) and ensure a "std" feature
exists and enables the standard-library-dependent dependencies.
| # Do NOT enable this dependency, this is just to pin dyn-clone (transitive dep from schemars) | ||
| # because 1.0.8 does not build with Rust 1.41.1 (because of useage of `Arc::as_ptr`). | ||
| dyn-clone = { version = "<=1.0.7", default-features = false, optional = true } | ||
| actual-schemars = { package = "schemars", version = "1.0", optional = true } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Crate alias changed to actual-schemars; ensure code imports use the alias or re-export it.
Code under hashes/src/util.rs imports schemars::…, which will not resolve unless you re-export the alias as schemars somewhere (e.g., in src/lib.rs) or switch imports to actual_schemars::…. Recommend aligning imports or adding a re-export (see util.rs comment for a concrete diff).
Run to verify any remaining schemars:: imports and a crate-level re-export:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Cargo alias:"
rg -nP '^\s*actual-schemars\s*=' -n --glob 'hashes/Cargo.toml' -N
echo -e "\nRust imports using schemars:: :"
rg -nP --type=rust -C2 '\buse\s+schemars::' | sed -n '1,120p'
echo -e "\nRe-export present?"
rg -nP --type=rust -C2 'pub\s+use\s+actual_schemars\s+as\s+schemars;'Length of output: 788
Align schemars imports with the new crate alias
hashes/src/util.rs lines 586–587 import schemars::…, but hashes/Cargo.toml aliases the package as actual-schemars (crate name actual_schemars) and there’s no pub use actual_schemars as schemars; present—these imports will fail. Change imports to use actual_schemars::… or add a crate-level re-export (pub use actual_schemars as schemars;, e.g. in lib.rs).
🤖 Prompt for AI Agents
In hashes/Cargo.toml around line 34 and in hashes/src/util.rs around lines
586–587, the Cargo.toml aliases the schemars crate as actual-schemars (crate
name actual_schemars) but the code imports use schemars::... — update imports to
use actual_schemars::... or add a crate-level re-export so schemars resolves:
add pub use actual_schemars as schemars; in the crate root (e.g., lib.rs).
Ensure the chosen fix is applied consistently (either change the two imports to
actual_schemars or add the re-export) and run cargo build to verify imports
succeed.
hashes/src/util.rs
Outdated
| use schemars::SchemaGenerator; | ||
| use schemars::{Schema, json_schema}; | ||
|
|
There was a problem hiding this comment.
Imports should target the aliased crate name (actual-schemars) or rely on a re-export.
Given Cargo uses actual-schemars = { package = "schemars", ... }, direct use schemars::… will fail unless re-exported. Either import from actual_schemars here, or add pub use actual_schemars as schemars; behind the feature in the crate root.
- use schemars::SchemaGenerator;
- use schemars::{Schema, json_schema};
+ use actual_schemars::SchemaGenerator;
+ use actual_schemars::{Schema, json_schema};Alternatively, add in hashes/src/lib.rs (or the crate root) so existing schemars:: paths continue to work:
#[cfg(feature = "schemars")]
pub use actual_schemars as schemars;🤖 Prompt for AI Agents
In hashes/src/util.rs around lines 586-588 the file imports schemars directly
(use schemars::SchemaGenerator; use schemars::{Schema, json_schema};) but Cargo
aliases the dependency as actual-schemars, so update the imports to use the
aliased crate (use actual_schemars::SchemaGenerator; use
actual_schemars::{Schema, json_schema};) OR add a re-export in the crate root
(hashes/src/lib.rs) such as #[cfg(feature = "schemars")] pub use actual_schemars
as schemars; so existing schemars:: paths keep working; pick one approach and
apply it consistently.
Summary by CodeRabbit
New Features
Documentation
API
Refactor
Tests
Chores