feat: improve key wallet #112
Conversation
WalkthroughBroad refactor adds BLS and EdDSA account support, overhauls account/managed-account architecture, introduces new derivation backends, updates wallet/manager/FFI APIs, and revises transaction checking. Watch-only module is removed. AddressPool gains multi-key/key-source support. Numerous tests are updated/removed. Cargo features and dependencies are adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant WM as WalletManager
participant W as Wallet
participant AC as AccountCollection
participant MW as ManagedWalletInfo
App->>WM: create_wallet_from_mnemonic(id, name, options, mnemonic, passphrase, net, birth)
WM->>W: Wallet::from_mnemonic(..., options)
WM-->>App: &Wallet
App->>W: add_account(account_type, net)
W->>AC: insert/account-specific insert (Result<()>)
AC-->>W: Ok(())
App->>MW: add_managed_account(wallet, account_type, net)
MW->>W: fetch account/xpub
MW->>MW: construct ManagedAccount (AddressPools via KeySource)
MW-->>App: Ok
sequenceDiagram
autonumber
actor Caller
participant MA as ManagedAccount
participant AP as AddressPool
participant KS as KeySource
Caller->>MA: next_change_address(Some(&xpub))
MA->>AP: next_unused(&KS)
AP->>AP: derive path (pool_type → [chain/index])
AP->>KS: derive_at_path(path)
KS-->>AP: DerivedKey (ECDSA/BLS/EdDSA)
AP-->>MA: AddressInfo.address
MA-->>Caller: Address
sequenceDiagram
autonumber
participant MAc as ManagedAccountCollection
participant TR as Transaction
participant Res as TransactionCheckResult
MAc->>MAc: check_transaction(&TR, account_types[])
loop for each type
MAc->>MAc: check_account_type(...)
MAc-->>MAc: Vec<AccountMatch>
end
MAc-->>Res: total amounts + credit conversion + matches
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
key-wallet/src/tests/coinjoin_mixing_tests.rs (2)
60-68: Fix integer type mismatch in denomination aggregation in coinjoin_mixing_tests.rsThe
denominations_createdmap currently infers its value type asi32(because ofor_insert(0)), leading to a compile error when multiplying au64key by ani32value. Update the map to useu64for both key and value, and destructure the references when computing the total.Locations to update:
- key-wallet/src/tests/coinjoin_mixing_tests.rs: lines 60–68
- key-wallet/src/tests/coinjoin_mixing_tests.rs: lines 77–79
Proposed diff:
--- a/key-wallet/src/tests/coinjoin_mixing_tests.rs +++ b/key-wallet/src/tests/coinjoin_mixing_tests.rs @@ -60,7 +60,7 @@ // Simulate creating denominations from a large input let input_amount = 5_000_000_000u64; // 50 DASH let mut remaining = input_amount; - let mut denominations_created = HashMap::new(); + let mut denominations_created: HashMap<u64, u64> = HashMap::new(); // Create maximum denominations starting from largest for &denom in DENOMINATIONS.iter().rev() { @@ -68,8 +68,11 @@ assert!(denominations_created.len() > 0); - let total_denominated: u64 = - denominations_created.iter().map(|(denom, count)| denom * count).sum(); + let total_denominated: u64 = denominations_created + .iter() + .map(|(&denom, &count)| denom.saturating_mul(count)) + .sum(); assert_eq!(total_denominated, input_amount - remaining); } @@ -77,8 +80,11 @@ assert!(denominations_created.len() > 0); - let total_denominated: u64 = - denominations_created.iter().map(|(denom, count)| denom * count).sum(); + let total_denominated: u64 = denominations_created + .iter() + .map(|(&denom, &count)| denom.saturating_mul(count)) + .sum(); assert_eq!(total_denominated, input_amount - remaining); }After applying these changes, please run:
cargo clippy -- -D warningsto ensure no remaining type-inference warnings.
115-133: Unify fee calculation variables tou64to matchDENOMINATIONSand fix compile errorsThe test declares
const DENOMINATIONS: [u64; 5] = …;but then infers
num_inputs,num_outputs,estimated_size,fee_rate, andfee_per_participantasi32. Comparing ani32(fee_per_participant) against au64(denomination / 100) won’t compile. All of these should beu64so that bytes and duffs remain non-negative and the comparison is valid.Please update the test at
key-wallet/src/tests/coinjoin_mixing_tests.rs(around lines 115–133) as follows:- let denomination = DENOMINATIONS[2]; // 0.1 DASH - let num_inputs = 3; - let num_outputs = 3; + let denomination: u64 = DENOMINATIONS[2]; // 0.1 DASH + let num_inputs: u64 = 3; + let num_outputs: u64 = 3; - let estimated_size = 10 + // Version + locktime - (num_inputs * 148) + // Approximate input size - (num_outputs * 34); // Approximate output size + let estimated_size: u64 = 10u64 + // Version + locktime + (num_inputs * 148u64) + // Approximate input size + (num_outputs * 34u64); // Approximate output size - let fee_rate = 1; // duffs per byte - let total_fee = estimated_size * fee_rate; + let fee_rate: u64 = 1; // duffs per byte + let total_fee: u64 = estimated_size * fee_rate; - let fee_per_participant = total_fee / num_inputs; + let fee_per_participant: u64 = total_fee / num_inputs;After applying these changes, run
cargo clippy -- -D warningsto confirm there are no remaining numeric-type or cast warnings.
key-wallet/src/transaction_checking/wallet_checker.rs (1)
169-176: Avoid hardcoding coinbase maturity (network parameter)
100is a consensus/network parameter and should not be hardcoded. Pull it from a single source of truth (e.g., consensus params) to satisfy the “no hardcoded network parameters” guideline and to keep Devnet/Regtest flexibility.Apply this diff (example naming — wire to your existing params module):
- 100, // Standard coinbase maturity + consensus::coinbase_maturity(network),Add a small helper (outside this file) if a direct constant is not available:
// e.g., in crate::consensus or a params module used elsewhere pub fn coinbase_maturity(network: crate::Network) -> u32 { // TODO: read from actual consensus params if available 100 }key-wallet-manager/src/wallet_manager/mod.rs (2)
184-198: Critical: create_wallet uses a fixed test mnemonic (keys leak risk).This function hardcodes the 12-word BIP39 mnemonic and constructs a wallet from it. This is extremely dangerous if this code path is reachable outside tests, as every created wallet will be identical and predictable. It also violates the guideline to use secure RNG for keys and to avoid hardcoding secrets.
Replace with Wallet::new_random to use secure randomness and eliminate the hardcoded mnemonic.
- // For now, create a wallet with a fixed test mnemonic - // In production, you'd generate a random mnemonic or use new_random with proper features - let test_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; - let mnemonic = - Mnemonic::from_phrase(test_mnemonic, key_wallet::mnemonic::Language::English) - .map_err(|e| WalletError::WalletCreation(e.to_string()))?; - - let wallet = Wallet::from_mnemonic( - mnemonic, - WalletConfig::default(), - network, - account_creation_options, - ) + // Generate a fresh wallet using secure RNG + let wallet = Wallet::new_random( + WalletConfig::default(), + network, + account_creation_options, + ) .map_err(|e| WalletError::WalletCreation(e.to_string()))?;
328-347: Account creation picks an arbitrary network; require an explicit network.Deriving the network as the “first key” from wallet.accounts is nondeterministic and wrong if the wallet spans multiple networks or has no accounts yet. Align this API with other methods and accept Network explicitly.
- pub fn create_account( - &mut self, - wallet_id: &WalletId, - index: u32, - account_type: AccountType, - ) -> Result<(), WalletError> { + pub fn create_account( + &mut self, + wallet_id: &WalletId, + index: u32, + account_type: AccountType, + network: Network, + ) -> Result<(), WalletError> { - // Get the network from the wallet's accounts or require it to be passed - let network = wallet.accounts.keys().next().copied().ok_or( - WalletError::InvalidParameter("No network available for account creation".to_string()), - )?; + // Use the provided network explicitlyIf changing the signature is too invasive in this PR, add a temporary network: Option parameter and error when None and the wallet has no accounts.
key-wallet/src/tests/edge_case_tests.rs (1)
33-45: Activate the invalid-derivation-path checks (available from_str).The comment says from_str does not exist, but the crate exposes DerivationPath::from_str. Turn the test cases into assertions to actually validate error handling and avoid a dead test section.
- // DerivationPath doesn't have from_str in this version - // Would need to parse manually or use different test approach + for case in _test_cases { + let res = crate::bip32::DerivationPath::from_str(case); + assert!(res.is_err(), "Expected error for invalid path: {}", case); + }key-wallet-ffi/src/managed_wallet.rs (2)
398-404: Wrong path and private-field access for ManagedAccountType; use accessor + new module pathBoth address-range functions pattern-match on key_wallet::account::ManagedAccountType and directly access managed_account.account_type. ManagedAccountType moved to key_wallet::managed_account::managed_account_type, and ManagedAccount’s fields aren’t public outside the crate. Use account_type_mut() and the new path.
- let addresses = if let key_wallet::account::ManagedAccountType::Standard { - external_addresses, - .. - } = &mut managed_account.account_type + use key_wallet::managed_account::managed_account_type::ManagedAccountType; + let addresses = if let ManagedAccountType::Standard { + external_addresses, + .. + } = managed_account.account_type_mut() { match external_addresses.address_range(start_index, end_index, &key_source) { ... } } else { ... };- let addresses = if let key_wallet::account::ManagedAccountType::Standard { - internal_addresses, - .. - } = &mut managed_account.account_type + use key_wallet::managed_account::managed_account_type::ManagedAccountType; + let addresses = if let ManagedAccountType::Standard { + internal_addresses, + .. + } = managed_account.account_type_mut() { match internal_addresses.address_range(start_index, end_index, &key_source) { ... } } else { ... };Also applies to: 401-404, 580-586, 583-586
1049-1068: Do not free Box-allocated arrays with libc::free (UB)addresses_out is created from a Rust Box via Box::into_raw. Freeing it with libc::free is undefined behavior. Use the provided address_array_free for both external and internal address arrays, or reconstruct the Box and drop it.
- libc::free(addresses_out as *mut libc::c_void); + // Free both the array and its strings using the FFI helper + crate::address::address_array_free(addresses_out, count_out);Also applies to: 1067-1067
| #[test] | ||
| fn test_comprehensive_address_generation() { | ||
| use key_wallet::account::address_pool::AddressPool; | ||
| use key_wallet::account::{ | ||
| ManagedAccount, ManagedAccountCollection, ManagedAccountType, StandardAccountType, | ||
| }; | ||
| use key_wallet::account::{ManagedAccount, ManagedAccountCollection, StandardAccountType}; | ||
| use key_wallet::bip32::DerivationPath; | ||
| use key_wallet::gap_limit::GapLimitManager; | ||
| use key_wallet::managed_account::address_pool::AddressPool; | ||
|
|
||
| let mut error = FFIError::success(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests use outdated imports and AddressPool::new signature
AddressPool moved to key_wallet::managed_account::address_pool and its new signature requires a KeySource and returns Result. Update imports and handle the Result. Also use the new ManagedAccount/ManagedAccountCollection paths.
- use key_wallet::account::{ManagedAccount, ManagedAccountCollection, StandardAccountType};
+ use key_wallet::managed_account::{ManagedAccount, managed_account_collection::ManagedAccountCollection};
+ use key_wallet::account::StandardAccountType;
...
- use key_wallet::managed_account::address_pool::AddressPool;
+ use key_wallet::managed_account::address_pool::{AddressPool, AddressPoolType, KeySource};
...
- let external_pool = AddressPool::new(
+ let external_pool = AddressPool::new(
DerivationPath::from(vec![key_wallet::bip32::ChildNumber::from_normal_idx(0).unwrap()]),
- AddressPoolType::External,
- 20,
- network,
- );
+ AddressPoolType::External,
+ 20,
+ network,
+ &KeySource::NoKeySource,
+ ).expect("external pool");
- let internal_pool = AddressPool::new(
+ let internal_pool = AddressPool::new(
DerivationPath::from(vec![key_wallet::bip32::ChildNumber::from_normal_idx(1).unwrap()]),
- AddressPoolType::Internal,
- 20,
- network,
- );
+ AddressPoolType::Internal,
+ 20,
+ network,
+ &KeySource::NoKeySource,
+ ).expect("internal pool");Also applies to: 963-974, 970-974, 978-987, 989-991
🤖 Prompt for AI Agents
In key-wallet-ffi/src/managed_wallet.rs around lines 926-933 (and also update
the occurrences at 963-974, 970-974, 978-987, 989-991), the test imports and
AddressPool usage are outdated: change imports to use
key_wallet::managed_account::address_pool::AddressPool and update
ManagedAccount/ManagedAccountCollection to their current paths; call
AddressPool::new with the required KeySource parameter and handle the Result it
returns (propagate or unwrap in the test), and adjust any variable names or
types to match the new signatures so the test compiles.
| match managed_account.next_receive_address(Some(&wallet_account.account_xpub)) { | ||
| Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP44)), | ||
| Err(_) => (None, None), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not swallow address derivation errors; propagate them via Result.
Both BIP44 and BIP32 branches convert errors to (None, None), which hides actionable failures (e.g., NoKeySource) and defeats the purpose of returning Result<…>. Return Err instead so callers can react.
- match managed_account.next_receive_address(Some(&wallet_account.account_xpub)) {
- Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP44)),
- Err(_) => (None, None),
- }
+ match managed_account.next_receive_address(Some(&wallet_account.account_xpub)) {
+ Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP44)),
+ Err(e) => return Err(WalletError::from(e)),
+ }- match managed_account.next_receive_address(Some(&wallet_account.account_xpub)) {
- Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP32)),
- Err(_) => (None, None),
- }
+ match managed_account.next_receive_address(Some(&wallet_account.account_xpub)) {
+ Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP32)),
+ Err(e) => return Err(WalletError::from(e)),
+ }Follow-up: for PreferBIP44/PreferBIP32 branches, keep the fallback, but if both attempts fail, return the last error instead of (None, None). Example shape:
- Try preferred -> on Err(e1) try fallback
- Fallback Ok => return Ok
- Fallback Err(e2) => return Err(e2) or Err(e1) (pick one deterministically)
Also applies to: 417-420
🤖 Prompt for AI Agents
In key-wallet-manager/src/wallet_manager/mod.rs around lines 404-407 (and
similarly 417-420), the code currently swallows address derivation errors by
mapping Err(_) to (None, None); instead propagate errors via Result: change the
BIP44/BIP32 branches to return Err(e) when derivation fails rather than
returning None, and for PreferBIP44/PreferBIP32 implement the
try-preferred-then-fallback pattern — attempt preferred, on Err(e1) try
fallback, if fallback Ok return that, if fallback Err(e2) return Err(e2) (or
consistently return Err(e1) if you choose that deterministically) so callers
receive meaningful error information.
| match managed_account.next_change_address(Some(&wallet_account.account_xpub)) { | ||
| Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP44)), | ||
| Err(_) => (None, None), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same issue for change addresses: propagate errors rather than returning None.
Mirror the receive-address fix for change-address derivation to preserve error semantics.
- match managed_account.next_change_address(Some(&wallet_account.account_xpub)) {
- Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP44)),
- Err(_) => (None, None),
- }
+ match managed_account.next_change_address(Some(&wallet_account.account_xpub)) {
+ Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP44)),
+ Err(e) => return Err(WalletError::from(e)),
+ }- match managed_account.next_change_address(Some(&wallet_account.account_xpub)) {
- Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP32)),
- Err(_) => (None, None),
- }
+ match managed_account.next_change_address(Some(&wallet_account.account_xpub)) {
+ Ok(addr) => (Some(addr), Some(AccountTypeUsed::BIP32)),
+ Err(e) => return Err(WalletError::from(e)),
+ }Also applies to: 571-574
🤖 Prompt for AI Agents
In key-wallet-manager/src/wallet_manager/mod.rs around lines 558-561 (and
similarly for 571-574), the code swallows errors from
managed_account.next_change_address by turning Err(_) into (None, None);
instead, preserve error semantics by propagating the error. Replace the match
that returns (None, None) on Err with code that uses the Result directly (e.g.,
use the ? operator or map the Ok value and let the Err bubble up) so the
function returns Err when derivation fails and only maps successful addresses to
(Some(addr), Some(AccountTypeUsed::BIP44)).
| use key_wallet::account::derivation::AccountDerivation; | ||
| use key_wallet::account::{ | ||
| Account, AccountTrait, AccountType, BLSAccount, ECDSAAddressDerivation, EdDSAAccount, | ||
| StandardAccountType, | ||
| }; |
There was a problem hiding this comment.
Feature-gate imports for BLS/EdDSA types to avoid build failures without features
BLSAccount and EdDSAAccount are likely behind bls/eddsa features. Importing them unconditionally can break cargo build without those features.
Apply:
-use key_wallet::account::{
- Account, AccountTrait, AccountType, BLSAccount, ECDSAAddressDerivation, EdDSAAccount,
- StandardAccountType,
-};
+use key_wallet::account::{Account, AccountTrait, AccountType, ECDSAAddressDerivation, StandardAccountType};
+#[cfg(feature = "bls")]
+use key_wallet::account::BLSAccount;
+#[cfg(feature = "eddsa")]
+use key_wallet::account::EdDSAAccount;And, if AccountDerivation is feature-gated, also gate it:
-use key_wallet::account::derivation::AccountDerivation;
+#[cfg(any(feature = "bls", feature = "eddsa"))]
+use key_wallet::account::derivation::AccountDerivation;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use key_wallet::account::derivation::AccountDerivation; | |
| use key_wallet::account::{ | |
| Account, AccountTrait, AccountType, BLSAccount, ECDSAAddressDerivation, EdDSAAccount, | |
| StandardAccountType, | |
| }; | |
| #[cfg(any(feature = "bls", feature = "eddsa"))] | |
| use key_wallet::account::derivation::AccountDerivation; | |
| use key_wallet::account::{ | |
| Account, | |
| AccountTrait, | |
| AccountType, | |
| ECDSAAddressDerivation, | |
| StandardAccountType, | |
| }; | |
| #[cfg(feature = "bls")] | |
| use key_wallet::account::BLSAccount; | |
| #[cfg(feature = "eddsa")] | |
| use key_wallet::account::EdDSAAccount; |
🤖 Prompt for AI Agents
In key-wallet/examples/account_types.rs around lines 3 to 7, the unconditional
imports of BLSAccount, EdDSAAccount (and possibly AccountDerivation) are behind
feature flags and will cause build failures when those features are disabled;
wrap the imports with appropriate cfg(feature = "bls") and cfg(feature =
"eddsa") attributes (and cfg(feature = "...") for AccountDerivation if
applicable) so each type is only imported when its feature is enabled, and
ensure any uses in the file are likewise gated or conditionally compiled.
| #[cfg(feature = "bls")] | ||
| pub provider_operator_keys: Option<BLSAccount>, | ||
| /// Provider platform keys (optional) | ||
| pub provider_platform_keys: Option<Account>, | ||
| #[cfg(feature = "eddsa")] | ||
| pub provider_platform_keys: Option<EdDSAAccount>, |
There was a problem hiding this comment.
Feature flag configuration requires consistent handling across compilation contexts
The provider_operator_keys and provider_platform_keys fields are conditionally compiled with feature flags. This creates a risk of serialization/deserialization mismatches if binaries with different feature flags try to exchange data.
Consider adding a version or feature flag indicator in serialized data to detect incompatible configurations.
🤖 Prompt for AI Agents
In key-wallet/src/account/account_collection.rs around lines 43 to 47, the
conditional fields guarded by feature flags can produce incompatible serialized
formats across builds; add a stable version or feature bitmap field to the
serialized representation (e.g., a u8 version and/or a bitflag indicating which
optional keys are present) and include it in all (de)serialization paths, update
the (De)Serialize impls to write/read that header and to fail with a clear error
when a runtime detects an unsupported version/feature bitmap, and add unit tests
that round-trip serialization across combinations and assert a clear error on
mismatched feature bitmaps.
| pub mod helpers; | ||
| pub mod managed_account_operations; | ||
| pub mod managed_accounts; | ||
| pub mod transaction_builder; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Feature-gate if needed to keep minimal builds lean (nit)
If any of the newly exposed modules pull optional deps, consider #[cfg(feature = "...")] on the pub mod ... lines to avoid inflating MSRV-critical/no_std builds.
Scan for optional dependencies used by these modules and gate accordingly:
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust 'extern crate|use\s+([a-zA-Z0-9_]+)::' key-wallet/src/wallet/managed_wallet_info/{helpers.rs,managed_account_operations.rs,managed_accounts.rs}Length of output: 2047
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust 'extern crate|use\s+([a-zA-Z0-9_]+)::' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rsLength of output: 1501
Gate transaction_builder behind a feature to preserve no_std/minimal builds
The other three modules (helpers, managed_account_operations, managed_accounts) only pull in crate-local types or core/alloc and won’t affect MSRV-critical or no_std builds. However, transaction_builder.rs introduces:
std::net::SocketAddr- External crates:
dashcore,dashcore_hashes,secp256k1 - Potentially large optional deps that pull in
stdand increase compile-time and binary size
To avoid inflating minimal or no_std builds, wrap its export in a feature flag:
-pub mod transaction_builder;
+#[cfg(feature = "transaction-builder")]
+pub mod transaction_builder;And declare the feature in Cargo.toml, enabling the necessary dependencies only when opted in:
[features]
transaction-builder = [
"dashcore",
"dashcore-hashes",
"secp256k1",
# any other heavy deps your builder needs
]
default = [] # or retain existing defaults, omitting this feature— Other modules in managed_wallet_info/ can remain un-gated.
— Update documentation to note that building the transaction builder requires --features transaction-builder.
🤖 Prompt for AI Agents
In key-wallet/src/wallet/managed_wallet_info/mod.rs around lines 8 to 11, the
pub export of transaction_builder pulls in std and heavy external crates which
harms no_std/minimal builds; gate the transaction_builder module behind a
feature by replacing the unconditional pub mod with a conditional pub mod
guarded by #[cfg(feature = "transaction-builder")] (and add #[cfg_attr(docsrs,
doc(cfg(feature = "transaction-builder")))] if you want docs to reflect it), and
add a transaction-builder feature in Cargo.toml that lists the heavy deps
(dashcore, dashcore-hashes, secp256k1, etc.) so those crates are only enabled
when the feature is selected; update README/docs to note that building the
transaction builder requires --features transaction-builder.
| let txout = TxOut { | ||
| value: 100000, | ||
| script_pubkey: Script::new(), | ||
| script_pubkey: ScriptBuf::new(), | ||
| }; | ||
| let address = Address::from_script(&Script::new(), Network::Testnet).unwrap(); | ||
| let address = Address::p2pkh( | ||
| &PublicKey::from_slice(&[ | ||
| 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x01, | ||
| ]) | ||
| .unwrap(), | ||
| Network::Testnet, | ||
| ); | ||
| let utxo = Utxo::new(outpoint, txout, address, 0, false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make test UTXO script_pubkey consistent with the address.
The test currently uses an empty ScriptBuf, which can hide mismatches. Build the TxOut after constructing the address and set script_pubkey = address.script_pubkey().
- let txout = TxOut {
- value: 100000,
- script_pubkey: ScriptBuf::new(),
- };
- let address = Address::p2pkh(
+ let address = Address::p2pkh(
&PublicKey::from_slice(&[
0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x01,
])
.unwrap(),
Network::Testnet,
);
+ let txout = TxOut {
+ value: 100000,
+ script_pubkey: address.script_pubkey(),
+ };📝 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 txout = TxOut { | |
| value: 100000, | |
| script_pubkey: Script::new(), | |
| script_pubkey: ScriptBuf::new(), | |
| }; | |
| let address = Address::from_script(&Script::new(), Network::Testnet).unwrap(); | |
| let address = Address::p2pkh( | |
| &PublicKey::from_slice(&[ | |
| 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |
| 0x00, 0x00, 0x00, 0x00, 0x01, | |
| ]) | |
| .unwrap(), | |
| Network::Testnet, | |
| ); | |
| let utxo = Utxo::new(outpoint, txout, address, 0, false); | |
| let address = Address::p2pkh( | |
| &PublicKey::from_slice(&[ | |
| 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |
| 0x00, 0x00, 0x00, 0x00, 0x01, | |
| ]) | |
| .unwrap(), | |
| Network::Testnet, | |
| ); | |
| let txout = TxOut { | |
| value: 100000, | |
| script_pubkey: address.script_pubkey(), | |
| }; | |
| let utxo = Utxo::new(outpoint, txout, address, 0, false); |
🤖 Prompt for AI Agents
In key-wallet/src/wallet/managed_wallet_info/utxo.rs around lines 224 to 237,
the test constructs a TxOut with an empty ScriptBuf which may not match the
Address; construct the Address first, then build the TxOut using that address's
script_pubkey (i.e., set script_pubkey = address.script_pubkey()) and keep the
value/outpoint as before so the TxOut matches the address used to create the
Utxo.
| use crate::derivation_bls_bip32::ExtendedBLSPrivKey; | ||
| use crate::wallet::WalletType; | ||
| #[cfg(feature = "bincode")] | ||
| use bincode::{BorrowDecode, Decode, Encode}; | ||
| use dashcore::blsful::Bls12381G2Impl; |
There was a problem hiding this comment.
Fix feature-gated imports to resolve CI failures
derivation_bls_bip32 and blsful are behind the bls feature; derivation_slip10 is behind eddsa. Import them conditionally to make no-feature builds pass.
- use crate::derivation_bls_bip32::ExtendedBLSPrivKey;
+ #[cfg(feature = "bls")]
+ use crate::derivation_bls_bip32::ExtendedBLSPrivKey;
@@
- use dashcore::blsful::Bls12381G2Impl;
+ #[cfg(feature = "bls")]
+ use dashcore::blsful::Bls12381G2Impl;Also gate the derivation_slip10 path usages within the method signature/body (see below).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use crate::derivation_bls_bip32::ExtendedBLSPrivKey; | |
| use crate::wallet::WalletType; | |
| #[cfg(feature = "bincode")] | |
| use bincode::{BorrowDecode, Decode, Encode}; | |
| use dashcore::blsful::Bls12381G2Impl; | |
| #[cfg(feature = "bls")] | |
| use crate::derivation_bls_bip32::ExtendedBLSPrivKey; | |
| use crate::wallet::WalletType; | |
| #[cfg(feature = "bincode")] | |
| use bincode::{BorrowDecode, Decode, Encode}; | |
| #[cfg(feature = "bls")] | |
| use dashcore::blsful::Bls12381G2Impl; |
🧰 Tools
🪛 GitHub Actions: Continuous integration
[error] 7-7: unresolved import: crate::derivation_bls_bip32::ExtendedBLSPrivKey; could not find derivation_bls_bip32 in the crate root (the item is gated behind the bls feature).
[error] 11-11: unresolved import: crate::derivation_bls_bip32; could not find derivation_bls_bip32 in the crate root (the item is gated behind the bls feature).
🤖 Prompt for AI Agents
In key-wallet/src/wallet/root_extended_keys.rs around lines 7 to 11, imports for
derivation_bls_bip32 and dashcore::blsful must be feature-gated behind "bls" and
derivation_slip10 usages behind "eddsa" so no-feature builds compile; change the
top-level use statements to conditional imports (e.g. add #[cfg(feature =
"bls")] to the derivation_bls_bip32 and blsful use lines and #[cfg(feature =
"eddsa")] to derivation_slip10 imports), and wrap any method signature types and
code paths that reference those modules/types with the corresponding
#[cfg(feature = "...")] or cfg_if guards so the function compiles when those
features are disabled.
| /// Convert to BLS extended private key for a specific network | ||
| /// This converts the secp256k1 private key to a BLS12-381 private key | ||
| /// Note: This is a cross-curve conversion and should be used carefully | ||
| pub fn to_bls_extended_priv_key(&self, network: Network) -> Result<ExtendedBLSPrivKey, Error> { | ||
| // Convert secp256k1 private key bytes to BLS private key | ||
| // Using from_le_bytes for little-endian byte order | ||
| // Note: from_le_bytes returns a CtOption (constant-time option) for security | ||
| let bls_private_key_option = dashcore::blsful::SecretKey::<Bls12381G2Impl>::from_le_bytes( | ||
| &self.root_private_key.secret_bytes(), | ||
| ); | ||
|
|
||
| // Convert CtOption to Result | ||
| let bls_private_key = if bls_private_key_option.is_some().into() { | ||
| bls_private_key_option.unwrap() | ||
| } else { | ||
| return Err(Error::InvalidParameter( | ||
| "Failed to convert to BLS key: invalid key bytes".to_string(), | ||
| )); | ||
| }; | ||
|
|
||
| Ok(ExtendedBLSPrivKey { | ||
| network, | ||
| depth: 0, | ||
| parent_fingerprint: Default::default(), | ||
| child_number: ChildNumber::from(0), | ||
| private_key: bls_private_key, | ||
| chain_code: self.root_chain_code, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
BLS cross-curve conversion constructs an ExtendedBLSPrivKey with an incompatible chain code
You’re reusing the secp256k1 root chain code for the BLS extended key. BLS HD derivation in this crate derives its own chain code using HMAC-SHA256 with the “BLS HD seed” domain and seed||1 (see ExtendedBLSPrivKey::new_master). Mixing chain-code domains can break child derivations and interoperability.
Replace the manual construction with ExtendedBLSPrivKey::new_master(network, seed) using the 32-byte secp seed as input. Also gate the method behind #[cfg(feature = "bls")].
- /// Convert to BLS extended private key for a specific network
- /// This converts the secp256k1 private key to a BLS12-381 private key
- /// Note: This is a cross-curve conversion and should be used carefully
- pub fn to_bls_extended_priv_key(&self, network: Network) -> Result<ExtendedBLSPrivKey, Error> {
- // Convert secp256k1 private key bytes to BLS private key
- // Using from_le_bytes for little-endian byte order
- // Note: from_le_bytes returns a CtOption (constant-time option) for security
- let bls_private_key_option = dashcore::blsful::SecretKey::<Bls12381G2Impl>::from_le_bytes(
- &self.root_private_key.secret_bytes(),
- );
-
- // Convert CtOption to Result
- let bls_private_key = if bls_private_key_option.is_some().into() {
- bls_private_key_option.unwrap()
- } else {
- return Err(Error::InvalidParameter(
- "Failed to convert to BLS key: invalid key bytes".to_string(),
- ));
- };
-
- Ok(ExtendedBLSPrivKey {
- network,
- depth: 0,
- parent_fingerprint: Default::default(),
- child_number: ChildNumber::from(0),
- private_key: bls_private_key,
- chain_code: self.root_chain_code,
- })
- }
+ /// Convert to BLS extended private key for a specific network
+ /// Uses the secp256k1 root secret bytes as seed input to BLS new_master,
+ /// which derives both private key and chain code in the BLS domain.
+ #[cfg(feature = "bls")]
+ pub fn to_bls_extended_priv_key(&self, network: Network) -> Result<ExtendedBLSPrivKey, Error> {
+ let seed = self.root_private_key.secret_bytes();
+ // ExtendedBLSPrivKey::new_master validates seed length and derives proper chain code.
+ crate::derivation_bls_bip32::ExtendedBLSPrivKey::new_master(network, &seed)
+ .map_err(Error::from)
+ }Note: If deterministic cross-curve identity rather than re-seeding is required, please specify the intended mapping; otherwise, the BLS domain KDF is the safer default.
📝 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.
| /// Convert to BLS extended private key for a specific network | |
| /// This converts the secp256k1 private key to a BLS12-381 private key | |
| /// Note: This is a cross-curve conversion and should be used carefully | |
| pub fn to_bls_extended_priv_key(&self, network: Network) -> Result<ExtendedBLSPrivKey, Error> { | |
| // Convert secp256k1 private key bytes to BLS private key | |
| // Using from_le_bytes for little-endian byte order | |
| // Note: from_le_bytes returns a CtOption (constant-time option) for security | |
| let bls_private_key_option = dashcore::blsful::SecretKey::<Bls12381G2Impl>::from_le_bytes( | |
| &self.root_private_key.secret_bytes(), | |
| ); | |
| // Convert CtOption to Result | |
| let bls_private_key = if bls_private_key_option.is_some().into() { | |
| bls_private_key_option.unwrap() | |
| } else { | |
| return Err(Error::InvalidParameter( | |
| "Failed to convert to BLS key: invalid key bytes".to_string(), | |
| )); | |
| }; | |
| Ok(ExtendedBLSPrivKey { | |
| network, | |
| depth: 0, | |
| parent_fingerprint: Default::default(), | |
| child_number: ChildNumber::from(0), | |
| private_key: bls_private_key, | |
| chain_code: self.root_chain_code, | |
| }) | |
| } | |
| /// Convert to BLS extended private key for a specific network | |
| /// Uses the secp256k1 root secret bytes as seed input to BLS new_master, | |
| /// which derives both private key and chain code in the BLS domain. | |
| #[cfg(feature = "bls")] | |
| pub fn to_bls_extended_priv_key(&self, network: Network) -> Result<ExtendedBLSPrivKey, Error> { | |
| let seed = self.root_private_key.secret_bytes(); | |
| // ExtendedBLSPrivKey::new_master validates seed length and derives proper chain code. | |
| crate::derivation_bls_bip32::ExtendedBLSPrivKey::new_master(network, &seed) | |
| .map_err(Error::from) | |
| } |
🤖 Prompt for AI Agents
In key-wallet/src/wallet/root_extended_keys.rs around lines 82 to 110, the
method builds an ExtendedBLSPrivKey by reusing the secp256k1 root chain_code,
which is incompatible with the crate’s BLS HD derivation domain; replace the
manual construction with a call to ExtendedBLSPrivKey::new_master(network, seed)
using the 32-byte secp seed so the BLS-specific HMAC-SHA256 chain code is
derived correctly, and gate the entire method behind #[cfg(feature = "bls")];
also adjust error handling to propagate conversion failures into new_master’s
Result (or map errors accordingly) rather than returning an ExtendedBLSPrivKey
with the wrong chain_code.
| /// Convert to EdDSA/Ed25519 extended private key for a specific network | ||
| /// This converts the secp256k1 private key to an Ed25519 private key | ||
| /// Note: This is a cross-curve conversion and should be used carefully | ||
| pub fn to_eddsa_extended_priv_key( | ||
| &self, | ||
| network: Network, | ||
| ) -> Result<crate::derivation_slip10::ExtendedEd25519PrivKey, Error> { | ||
| use crate::derivation_slip10::ExtendedEd25519PrivKey; | ||
|
|
||
| // Convert secp256k1 private key bytes to Ed25519 seed | ||
| // Ed25519 uses 32-byte seeds to generate keys | ||
| let seed_bytes = self.root_private_key.secret_bytes(); | ||
|
|
||
| // Create Ed25519 extended private key from seed using new_master | ||
| let eddsa_key = ExtendedEd25519PrivKey::new_master(network, &seed_bytes).map_err(|e| { | ||
| Error::InvalidParameter(format!("Failed to convert to EdDSA key: {:?}", e)) | ||
| })?; | ||
|
|
||
| Ok(eddsa_key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate EdDSA conversion and avoid hard path references when the feature is off
This method references crate::derivation_slip10 unconditionally, which breaks no-feature builds.
- /// Convert to EdDSA/Ed25519 extended private key for a specific network
- /// This converts the secp256k1 private key to an Ed25519 private key
- /// Note: This is a cross-curve conversion and should be used carefully
- pub fn to_eddsa_extended_priv_key(
+ /// Convert to EdDSA/Ed25519 extended private key for a specific network.
+ /// Uses the secp256k1 root secret bytes as the SLIP-0010 seed.
+ #[cfg(feature = "eddsa")]
+ pub fn to_eddsa_extended_priv_key(
&self,
network: Network,
- ) -> Result<crate::derivation_slip10::ExtendedEd25519PrivKey, Error> {
- use crate::derivation_slip10::ExtendedEd25519PrivKey;
+ ) -> Result<crate::derivation_slip10::ExtendedEd25519PrivKey, Error> {
+ use crate::derivation_slip10::ExtendedEd25519PrivKey;
// Convert secp256k1 private key bytes to Ed25519 seed
// Ed25519 uses 32-byte seeds to generate keys
let seed_bytes = self.root_private_key.secret_bytes();
// Create Ed25519 extended private key from seed using new_master
let eddsa_key = ExtendedEd25519PrivKey::new_master(network, &seed_bytes).map_err(|e| {
Error::InvalidParameter(format!("Failed to convert to EdDSA key: {:?}", e))
})?;
Ok(eddsa_key)
}📝 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.
| /// Convert to EdDSA/Ed25519 extended private key for a specific network | |
| /// This converts the secp256k1 private key to an Ed25519 private key | |
| /// Note: This is a cross-curve conversion and should be used carefully | |
| pub fn to_eddsa_extended_priv_key( | |
| &self, | |
| network: Network, | |
| ) -> Result<crate::derivation_slip10::ExtendedEd25519PrivKey, Error> { | |
| use crate::derivation_slip10::ExtendedEd25519PrivKey; | |
| // Convert secp256k1 private key bytes to Ed25519 seed | |
| // Ed25519 uses 32-byte seeds to generate keys | |
| let seed_bytes = self.root_private_key.secret_bytes(); | |
| // Create Ed25519 extended private key from seed using new_master | |
| let eddsa_key = ExtendedEd25519PrivKey::new_master(network, &seed_bytes).map_err(|e| { | |
| Error::InvalidParameter(format!("Failed to convert to EdDSA key: {:?}", e)) | |
| })?; | |
| Ok(eddsa_key) | |
| } | |
| /// Convert to EdDSA/Ed25519 extended private key for a specific network. | |
| /// Uses the secp256k1 root secret bytes as the SLIP-0010 seed. | |
| #[cfg(feature = "eddsa")] | |
| pub fn to_eddsa_extended_priv_key( | |
| &self, | |
| network: Network, | |
| ) -> Result<crate::derivation_slip10::ExtendedEd25519PrivKey, Error> { | |
| use crate::derivation_slip10::ExtendedEd25519PrivKey; | |
| // Convert secp256k1 private key bytes to Ed25519 seed | |
| // Ed25519 uses 32-byte seeds to generate keys | |
| let seed_bytes = self.root_private_key.secret_bytes(); | |
| // Create Ed25519 extended private key from seed using new_master | |
| let eddsa_key = ExtendedEd25519PrivKey::new_master(network, &seed_bytes).map_err(|e| { | |
| Error::InvalidParameter(format!("Failed to convert to EdDSA key: {:?}", e)) | |
| })?; | |
| Ok(eddsa_key) | |
| } |
🧰 Tools
🪛 GitHub Actions: Continuous integration
[error] 118-118: unresolved path: crate::derivation_slip10; could not find derivation_slip10 in the crate root (the item is gated behind the eddsa feature).
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores