Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFFI asset-lock building now delegates to a new managed-wallet asset-lock builder. The builder adds credit outputs before coin selection, derives one-time keys per credit output, selects and signs inputs, and returns the signed transaction, fee, and per-output private keys via the updated FFI API. Changes
sequenceDiagram
participant FFI as Wallet FFI
participant MWI as ManagedWalletInfo
participant TB as TransactionBuilder
participant KS as KeyStore
FFI->>MWI: build_asset_lock(params with credit_outputs, fundings, fee_per_kb)
MWI->>TB: new TransactionBuilder().set_change_address(...).set_fee_rate(...)
MWI->>TB: add_raw_output(credit_output) for each credit output
MWI->>KS: derive_one_time_key(funding_account, identity_index?) — mark address used
KS-->>MWI: return one-time privkey
MWI->>TB: select_inputs(utxos, derivation_closure using root_xpriv & path map)
TB-->>MWI: selected inputs and fee estimate
MWI->>TB: sign inputs with derived keys and attach asset-lock payload
MWI->>FFI: return AssetLockResult{ transaction, fee, keys } and fill private_keys_out[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #604 +/- ##
=============================================
+ Coverage 67.03% 67.10% +0.06%
=============================================
Files 320 321 +1
Lines 67249 68029 +780
=============================================
+ Hits 45083 45653 +570
- Misses 22166 22376 +210
|
QuantumExplorer
force-pushed
the
fix/asset-lock-coin-selection
branch
from
6638745 to
b9a9af0
Compare
QuantumExplorer
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 86-95: In build_asset_lock_transaction validate that the provided
network matches the crypto/network of all inputs before selection/signing: check
change_address.network equals network, iterate credit_outputs and verify each
TxOut.address/network (or scriptPubKey-derived network) matches network, and
verify funding_account and funding_key_account derived addresses/public keys
correspond to the same network; if any mismatch, return an AssetLockError (e.g.,
NetworkMismatch) before proceeding to UTXO selection or signing; update any
helper functions called by build_asset_lock_transaction (and analogous blocks
around lines 100-123 and 140-151) to enforce the same network-consistency
checks.
- Around line 1-175: Add a new #[cfg(test)] mod with unit tests in the same
asset_lock_builder.rs covering the new asset-lock flow: (1) test
build_asset_lock_transaction returns AssetLockError::NoCreditOutputs when
credit_outputs is empty; (2) test the one-time key derivation and that
funding_key_account.account_type.mark_address_used() is invoked by arranging a
funding_key_account with a known address index (use get_next_address_index() to
control the index) and asserting the returned private_key matches a derived
stub/root_xpriv; (3) test the fee-with-extra-output branch by constructing a
TransactionBuilder/utxos scenario where calculate_fee_with_extra_output() should
be chosen (verify fee equals calculate_fee_with_extra_output() when
transaction.output.len() > outputs_count_before). Use or create lightweight test
helpers/mocks for ManagedCoreAccount, RootExtendedPrivKey, and Utxo so you can
call build_asset_lock_transaction and assert returned AssetLockResult or
AssetLockError variants and the output_index/private_key/fee behavior; reference
functions/types: build_asset_lock_transaction, AssetLockError::{NoCreditOutputs,
KeyDerivation}, ManagedCoreAccount::get_next_address_index,
account_type.mark_address_used, TransactionBuilder::select_inputs,
TransactionBuilder::calculate_fee_with_extra_output, and build_asset_lock.
- Around line 111-116: The code currently calls
funding_key_account.get_next_address_index().unwrap_or(0), which silently falls
back to index 0 and can reuse a previously used key; instead, detect the None
return and surface an error so a fresh index is required. Replace the
unwrap_or(0) usage with logic that returns
Err(AssetLockError::NoAddressAvailable) (or a more specific error) when
get_next_address_index() yields None, then proceed to look up the address using
the obtained index; update references to key_index, key_path, and key_address
accordingly so address selection only happens when a valid next index is present
and you still return Err(AssetLockError::NoAddressAvailable) if the address pool
lookup fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db18fdab-c8ff-4fe2-9360-a6a9f6a57c32
📒 Files selected for processing (4)
key-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
✅ Files skipped from review due to trivial changes (2)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
- key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/src/transaction.rs
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Outdated
Show resolved
Hide resolved
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Outdated
Show resolved
Hide resolved
QuantumExplorer
force-pushed
the
fix/asset-lock-coin-selection
branch
from
b9a9af0 to
26ffe9f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (2)
166-177: Consider hoisting Secp256k1 and root key derivation outside the closure.The closure recreates
secp256k1::Secp256k1::new()and callsroot_xpriv.to_extended_priv_key(network)for every UTXO evaluated during coin selection. Since both are deterministic and already computed at lines 150-151, capturing them from the outer scope would be more efficient.Suggested optimization
+ let secp = secp256k1::Secp256k1::new(); + let root_ext_priv = root_xpriv.to_extended_priv_key(network); + let tx_builder_with_inputs = tx_builder.select_inputs( &utxos, SelectionStrategy::BranchAndBound, synced_height, |utxo| { let path = address_to_path.get(&utxo.address)?; - let root_ext_priv = root_xpriv.to_extended_priv_key(network); - let secp = secp256k1::Secp256k1::new(); let derived_xpriv = root_ext_priv.derive_priv(&secp, path).ok()?; Some(derived_xpriv.private_key) }, )?;Note: The
secpandroot_ext_privat lines 150-151 can be reused directly since they're still in scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 166 - 177, The closure passed to tx_builder.select_inputs is recreating secp256k1::Secp256k1::new() and calling root_xpriv.to_extended_priv_key(network) for every UTXO; hoist these into the outer scope (reuse the existing root_ext_priv and a single Secp256k1 instance) and capture them by reference in the closure used by select_inputs so the closure simply looks up address_to_path, derives using the precomputed root_ext_priv and secp, and returns derived_xpriv.private_key; update any mutable/borrow usages so types and lifetimes match the closure signature.
147-148: Address marked used before transaction build may waste key indices on failure.If
select_inputs()orbuild_asset_lock()fails after line 148, the address is permanently marked as used without a corresponding transaction. While this is arguably safer (prevents accidental key reuse on retry), it may exhaust the address pool faster under repeated failures.Consider deferring
mark_address_useduntil after a successful build, or document this behavior explicitly in the function's doc comment so callers understand the side effect occurs regardless of success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 147 - 148, The code currently calls funding_key_account.account_type.mark_address_used(&key_address) before calling select_inputs() and build_asset_lock(), which can consume address indices on failure; move the mark_address_used call to after a successful build (i.e., only call it once select_inputs() and build_asset_lock() return Ok and the transaction is ready) so the address is only marked when a real asset lock is created; alternatively, if you intentionally want the current behavior, update the function doc comment for build_asset_lock()/select_inputs() (and the surrounding method in asset_lock_builder.rs) to explicitly state that mark_address_used is applied even on failure so callers are aware of the side effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 195-199: The AssetLockResult currently hardcodes output_index: 0
while the function accepts multiple credit_outputs, which can misassociate the
returned private_key; update the implementation so output_index indicates the
actual index of the credit_outputs entry that corresponds to the returned
private_key (e.g., determine the index by matching the scriptPubKey/txout that
the private_key funds or by accepting an input parameter like
target_output_index), and return that computed index in AssetLockResult instead
of always 0; adjust callers of AssetLockResult/asset_lock_builder (and any
tests) to pass or consume the correct target index if you choose the parameter
approach.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 166-177: The closure passed to tx_builder.select_inputs is
recreating secp256k1::Secp256k1::new() and calling
root_xpriv.to_extended_priv_key(network) for every UTXO; hoist these into the
outer scope (reuse the existing root_ext_priv and a single Secp256k1 instance)
and capture them by reference in the closure used by select_inputs so the
closure simply looks up address_to_path, derives using the precomputed
root_ext_priv and secp, and returns derived_xpriv.private_key; update any
mutable/borrow usages so types and lifetimes match the closure signature.
- Around line 147-148: The code currently calls
funding_key_account.account_type.mark_address_used(&key_address) before calling
select_inputs() and build_asset_lock(), which can consume address indices on
failure; move the mark_address_used call to after a successful build (i.e., only
call it once select_inputs() and build_asset_lock() return Ok and the
transaction is ready) so the address is only marked when a real asset lock is
created; alternatively, if you intentionally want the current behavior, update
the function doc comment for build_asset_lock()/select_inputs() (and the
surrounding method in asset_lock_builder.rs) to explicitly state that
mark_address_used is applied even on failure so callers are aware of the side
effect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa01b0d1-6f21-46ee-8a1d-ec901ea1d564
📒 Files selected for processing (4)
key-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
✅ Files skipped from review due to trivial changes (1)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/src/transaction.rs
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Outdated
Show resolved
Hide resolved
QuantumExplorer
force-pushed
the
fix/asset-lock-coin-selection
branch
from
26ffe9f to
e54f53f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
key-wallet-ffi/src/transaction.rs (1)
1284-1291: Private key output limited to first credit output only.The code writes only
result.keys.first()toprivate_key_out, ignoring additional keys when multiple credit outputs are provided. While the comment at lines 1284-1286 documents this limitation, the FFI signature (credit_outputs_count: usize) suggests multi-output support but the singleprivate_key_out: *mut [u8; 32]cannot return all keys.Consider either:
- Adding
private_keys_out: *mut *mut [u8; 32]+private_keys_count_outfor full multi-key support, or- Documenting that only single-output asset locks are supported at the FFI level and validating
credit_outputs_count == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 1284 - 1291, The FFI currently writes only the first key (result.keys.first()) into private_key_out while the signature accepts credit_outputs_count implying multi-output support; either implement full multi-key return or enforce single-output. Fix by choosing one: (A) add multi-key outputs to the FFI by allocating and populating private_keys_out and setting private_keys_count_out from result.keys.len(), copying each result.keys[i].private_key into the allocated array and returning ownership to the caller; or (B) validate that credit_outputs_count == 1 (or result.keys.len() == 1) inside the transaction builder, return an error if not, and keep writing only to private_key_out as currently done; reference variables/functions: private_key_out, credit_outputs_count, result.keys, private_keys_out, private_keys_count_out, and ensure the chosen branch updates the FFI return/error handling consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 1288-1291: The code unconditionally writes 0 to *output_index_out
instead of using the actual assigned index; change the assignment to use the
domain builder's returned index from the selected key (e.g., set
*output_index_out to first_key.output_index or the corresponding field on the
element from result.keys) and keep the existing copy of the private key into
*private_key_out; locate this in the block that references result.keys.first(),
first_key, private_key_out and output_index_out and replace the hardcoded 0 with
the actual output_index field from first_key.
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 123-129: The address is currently marked as used before key
derivation succeeds
(funding_key_account.account_type.mark_address_used(&key_address)), which can
burn an index if derive_priv (root_xpriv.derive_priv via Secp256k1) fails; move
the call to mark_address_used so it runs only after one_time_xpriv is
successfully created (i.e., after root_xpriv.derive_priv returns Ok), and keep
the AssetLockError mapping for derivation failures unchanged so the address is
only marked on successful derivation.
- Around line 159-167: The assigned CreditOutputKey.output_index values are
based on pre-sort insertion order and therefore no longer match positions after
TransactionBuilder.build() applies BIP-69 sorting; update build_asset_lock()
(the place that creates the keys vector using derive_one_time_key and pushes
CreditOutputKey) to remap output_index after calling TransactionBuilder.build()
by iterating the final transaction.output list to find each credit output's
actual index (or by constructing keys from the built transaction outputs), and
then set each CreditOutputKey.output_index to that final position so downstream
code reads the correct transaction.output entry.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 1284-1291: The FFI currently writes only the first key
(result.keys.first()) into private_key_out while the signature accepts
credit_outputs_count implying multi-output support; either implement full
multi-key return or enforce single-output. Fix by choosing one: (A) add
multi-key outputs to the FFI by allocating and populating private_keys_out and
setting private_keys_count_out from result.keys.len(), copying each
result.keys[i].private_key into the allocated array and returning ownership to
the caller; or (B) validate that credit_outputs_count == 1 (or result.keys.len()
== 1) inside the transaction builder, return an error if not, and keep writing
only to private_key_out as currently done; reference variables/functions:
private_key_out, credit_outputs_count, result.keys, private_keys_out,
private_keys_count_out, and ensure the chosen branch updates the FFI
return/error handling consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec3714df-b262-4201-b28c-3bafd219cf96
📒 Files selected for processing (4)
key-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
✅ Files skipped from review due to trivial changes (1)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Outdated
Show resolved
Hide resolved
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Outdated
Show resolved
Hide resolved
QuantumExplorer
force-pushed
the
fix/asset-lock-coin-selection
branch
2 times, most recently
from
6d5ae4f to
2807833
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
165-171: RepeatedSecp256k1andExtendedPrivKeyinstantiation per UTXO is inefficient.Inside the closure,
secp256k1::Secp256k1::new()androot_xpriv.to_extended_priv_key(network)are called for every UTXO during input selection.Secp256k1::new()performs precomputation tables allocation which has non-trivial overhead.Hoist these outside the closure:
Proposed fix
+ let secp = secp256k1::Secp256k1::new(); + let root_ext_priv = root_xpriv.to_extended_priv_key(network); + let tx_builder_with_inputs = tx_builder.select_inputs( &utxos, SelectionStrategy::BranchAndBound, synced_height, |utxo| { let path = address_to_path.get(&utxo.address)?; - let root_ext_priv = root_xpriv.to_extended_priv_key(network); - let secp = secp256k1::Secp256k1::new(); let derived_xpriv = root_ext_priv.derive_priv(&secp, path).ok()?; Some(derived_xpriv.private_key) }, )?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 165 - 171, The closure used for mapping UTXOs repeatedly constructs secp256k1::Secp256k1::new() and calls root_xpriv.to_extended_priv_key(network) per UTXO, causing unnecessary allocation; hoist these out of the closure by creating a single Secp256k1 instance (e.g., let secp = secp256k1::Secp256k1::new()) and compute root_ext_priv once (let root_ext_priv = root_xpriv.to_extended_priv_key(network)) before the map/closure, then use those precomputed values inside the closure when deriving (derived_xpriv = root_ext_priv.derive_priv(&secp, path).ok()?) to avoid repeated instantiation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 165-171: The closure used for mapping UTXOs repeatedly constructs
secp256k1::Secp256k1::new() and calls root_xpriv.to_extended_priv_key(network)
per UTXO, causing unnecessary allocation; hoist these out of the closure by
creating a single Secp256k1 instance (e.g., let secp =
secp256k1::Secp256k1::new()) and compute root_ext_priv once (let root_ext_priv =
root_xpriv.to_extended_priv_key(network)) before the map/closure, then use those
precomputed values inside the closure when deriving (derived_xpriv =
root_ext_priv.derive_priv(&secp, path).ok()?) to avoid repeated instantiation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2bc9d14-716b-4552-84e6-a28276901400
📒 Files selected for processing (5)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
✅ Files skipped from review due to trivial changes (1)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
👮 Files not reviewed due to content moderation or server errors (3)
- key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
- key-wallet-ffi/src/transaction.rs
- key-wallet-ffi/FFI_API.md
QuantumExplorer
force-pushed
the
fix/asset-lock-coin-selection
branch
5 times, most recently
from
425e6c7 to
e0fc227
Compare
xdustinface
reviewed
Apr 1, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Just two questions.
| .next_unused_with_info(&address_pool::KeySource::NoKeySource, false) | ||
| .map_err(|_| "No unused address available")?; | ||
|
|
||
| pool.mark_index_used(info.index); |
Collaborator
There was a problem hiding this comment.
Could this somehow interfere or impact the gap limit maintainance?
Member
Author
There was a problem hiding this comment.
No — this only consumes pre-generated addresses via next_unused_with_info with NoKeySource (so it can't generate new ones), and mark_index_used just updates the used set. Gap limit maintenance happens during sync-time address discovery, not here.
The single-pool account types used for asset locks (IdentityRegistration, AssetLockAddressTopUp, etc.) don't use gap limit tracking — they have a flat address pool without the external/internal split.
Collaborator
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
key-wallet/src/managed_account/mod.rs (1)
944-972:⚠️ Potential issue | 🟡 MinorStandard accounts should be explicitly rejected.
The doc comment states "Only works for single-pool account types (not Standard accounts)", but there's no runtime enforcement. For Standard accounts,
address_pools_mut()returns[external_addresses, internal_addresses], sofirst_mut()would silently use the external (receiving) pool — which is likely incorrect for one-time key derivation.Consider adding an explicit check to fail fast:
Proposed fix
pub fn next_private_key( &mut self, root_xpriv: &crate::wallet::root_extended_keys::RootExtendedPrivKey, network: Network, ) -> Result<[u8; 32], &'static str> { + if matches!(self.account_type, ManagedAccountType::Standard { .. }) { + return Err("next_private_key is not supported for Standard accounts"); + } + let mut pools = self.account_type.address_pools_mut();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 944 - 972, The method next_private_key silently uses the first pool from self.account_type.address_pools_mut(), which allows Standard (two-pool) accounts to proceed incorrectly; add an explicit runtime guard that returns an Err for multi-pool/Standard accounts (e.g., check self.account_type or the length of address_pools_mut() and reject if not a single pool) before calling pools.first_mut(); ensure the error message is clear (like "Unsupported account type for one-time key derivation") so Standard accounts fail fast instead of deriving from the external pool.
🧹 Nitpick comments (3)
key-wallet-ffi/src/transaction.rs (1)
1147-1152: Silent truncation if keys count mismatches.The loop uses
if i < keys_out.len()which would silently skip keys ifresult.keys.len() > credit_outputs_count. Whilebuild_asset_lockshould return exactly one key per funding, a defensive check or assertion would catch any future inconsistency:Proposed defensive check
+ if result.keys.len() != credit_outputs_count { + FFIError::set_error( + error, + FFIErrorCode::InternalError, + format!( + "Key count mismatch: expected {}, got {}", + credit_outputs_count, + result.keys.len() + ), + ); + return false; + } + let keys_out = slice::from_raw_parts_mut(private_keys_out, credit_outputs_count); for (i, key) in result.keys.iter().enumerate() { - if i < keys_out.len() { - keys_out[i] = *key; - } + keys_out[i] = *key; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 1147 - 1152, The code silently truncates keys when result.keys.len() > credit_outputs_count; before creating keys_out or copying, validate that result.keys.len() == credit_outputs_count (or otherwise handle mismatch) and return an explicit error/abort rather than silently skipping: check the lengths of result.keys and credit_outputs_count (referencing result.keys, credit_outputs_count, private_keys_out and the build_asset_lock result) and if they differ, log/return a clear error or assert to prevent silent truncation and ensure callers get a deterministic failure.key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
59-107: Consider implementingstd::error::ErrorforAssetLockError.The error type has
DisplayandFrom<BuilderError>but lacksstd::error::Error. This limits interoperability with error-handling crates and the?operator in contexts expectingError.Proposed addition
+impl std::error::Error for AssetLockError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Builder(e) => Some(e), + _ => None, + } + } +}Note: This requires
BuilderErrorto also implementstd::error::Error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 59 - 107, The AssetLockError type implements Display and From<BuilderError> but does not implement std::error::Error; add an impl std::error::Error for AssetLockError (or derive it via thiserror if preferred) so it interoperates with error-handling traits—implement source(&self) to return Some(&inner) for the Builder(BuilderError) variant and None otherwise, and ensure BuilderError itself implements std::error::Error so the conversion and source reference are valid; update the impls near AssetLockError, fmt::Display, and the From<BuilderError> to reflect this.key-wallet/src/managed_account/mod.rs (1)
958-967: Address marked as used before key derivation succeeds.
mark_index_usedat line 962 mutates the address pool state beforederive_privat line 967. If derivation fails, the address index remains marked as used even though no key was returned — effectively "burning" an index.Consider reordering to mark after successful derivation:
Proposed fix
let info = pool .next_unused_with_info(&address_pool::KeySource::NoKeySource, false) .map_err(|_| "No unused address available")?; - pool.mark_index_used(info.index); - let secp = secp256k1::Secp256k1::new(); let root_ext_priv = root_xpriv.to_extended_priv_key(network); let derived_xpriv = root_ext_priv.derive_priv(&secp, &info.path).map_err(|_| "Key derivation failed")?; + pool.mark_index_used(info.index); + let mut private_key = [0u8; 32];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 958 - 967, You're marking an address index used before key derivation succeeds: call pool.next_unused_with_info to get info, then perform the derivation (use secp and root_xpriv.derive_priv with info.path) and only after derive_priv returns Ok should you call pool.mark_index_used(info.index); move the pool.mark_index_used call to after the successful derive_priv result (and ensure any early returns on derivation errors do not mutate the pool).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 944-972: The method next_private_key silently uses the first pool
from self.account_type.address_pools_mut(), which allows Standard (two-pool)
accounts to proceed incorrectly; add an explicit runtime guard that returns an
Err for multi-pool/Standard accounts (e.g., check self.account_type or the
length of address_pools_mut() and reject if not a single pool) before calling
pools.first_mut(); ensure the error message is clear (like "Unsupported account
type for one-time key derivation") so Standard accounts fail fast instead of
deriving from the external pool.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 1147-1152: The code silently truncates keys when result.keys.len()
> credit_outputs_count; before creating keys_out or copying, validate that
result.keys.len() == credit_outputs_count (or otherwise handle mismatch) and
return an explicit error/abort rather than silently skipping: check the lengths
of result.keys and credit_outputs_count (referencing result.keys,
credit_outputs_count, private_keys_out and the build_asset_lock result) and if
they differ, log/return a clear error or assert to prevent silent truncation and
ensure callers get a deterministic failure.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 958-967: You're marking an address index used before key
derivation succeeds: call pool.next_unused_with_info to get info, then perform
the derivation (use secp and root_xpriv.derive_priv with info.path) and only
after derive_priv returns Ok should you call pool.mark_index_used(info.index);
move the pool.mark_index_used call to after the successful derive_priv result
(and ensure any early returns on derivation errors do not mutate the pool).
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 59-107: The AssetLockError type implements Display and
From<BuilderError> but does not implement std::error::Error; add an impl
std::error::Error for AssetLockError (or derive it via thiserror if preferred)
so it interoperates with error-handling traits—implement source(&self) to return
Some(&inner) for the Builder(BuilderError) variant and None otherwise, and
ensure BuilderError itself implements std::error::Error so the conversion and
source reference are valid; update the impls near AssetLockError, fmt::Display,
and the From<BuilderError> to reflect this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 027fc88c-ff2a-4534-ab04-12cf46becbeb
📒 Files selected for processing (6)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/transaction.rskey-wallet/src/managed_account/mod.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
✅ Files skipped from review due to trivial changes (2)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
- key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet-ffi/FFI_API.md
Moves the asset lock transaction building logic out of the FFI layer into a dedicated `asset_lock_builder` module in key-wallet. The FFI function is now a thin bridge that converts types and delegates to `build_asset_lock_transaction()`. The new builder: - Lives in key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs - Handles credit output setup, coin selection, key derivation, signing - Returns AssetLockResult with transaction, fee, output_index, private_key - Is testable without FFI and reusable from any Rust consumer Also adds `TransactionBuilder::add_raw_output(TxOut)` for adding pre-built outputs (needed for credit outputs before coin selection). Closes #602 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/asset-lock-coin-selection
branch
from
e0fc227 to
b566fc4
Compare
shumkov
approved these changes
Apr 1, 2026
Collaborator
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
High-level reviewed and it looks good!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts the asset lock transaction building logic from the FFI layer into a dedicated
asset_lock_buildermodule inkey-wallet. The FFI function becomes a thin bridge that converts types and delegates to the domain-level builder.Also fixes #602 — coin selection was failing because credit outputs weren't added to the builder before
select_inputs().Closes #602
Architecture
Before: All domain logic (credit output setup, account lookup, key derivation, coin selection, signing) was inside the FFI function (~300 lines of unsafe C-bridge code).
After:
Changes
New file:
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rsbuild_asset_lock_transaction()— pure Rust, testable, reusable from any consumerAssetLockResult— return struct with transaction, fee, output_index, private_keyAssetLockError— dedicated error enumModified:
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rsadd_raw_output(TxOut)method for adding pre-built outputs (needed for credit outputs before coin selection)Modified:
key-wallet-ffi/src/transaction.rsTest plan
cargo check --workspace— no errors, no warningscargo fmt --check— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes