feat(key-wallet): CoinJoin funding account + drain mode for asset-lock builds#915
Open
QuantumExplorer wants to merge 1 commit into
Open
feat(key-wallet): CoinJoin funding account + drain mode for asset-lock builds#915QuantumExplorer wants to merge 1 commit into
QuantumExplorer wants to merge 1 commit into
Conversation
…k builds
Asset-lock builders (build_asset_lock / build_asset_lock_with_signer)
previously hardcoded the funding source to standard BIP44 accounts. This
adds:
- AssetLockFundingAccount { Bip44, CoinJoin }: which account family
supplies (and signs) the funding UTXOs. CoinJoin lets mixed coins fund
an asset lock directly, without first sweeping them through a
transparent BIP44 address.
- drain mode: consume every final UTXO of the funding account and
rewrite the single credit output's value to sum(inputs) - fee. The
TransactionBuilder's SelectionStrategy::All path now also rewrites the
asset-lock payload credit output so the payload always mirrors the
on-chain OP_RETURN burn (a mismatch is consensus-invalid), and rejects
a zero-value drain.
- CoinJoin funding is drain-only: partial spends would need CoinJoin
change re-denomination, which the builder does not do; a non-drain
CoinJoin build is rejected with InvalidData.
The key-wallet-ffi build_asset_lock entry point keeps its behavior
(BIP44, non-drain).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAsset-lock construction now accepts BIP44 or CoinJoin funding accounts. CoinJoin funding is restricted to drain mode, drain transactions select all inputs, and asset-lock payload values are validated against the drained amount. FFI and tests use the updated API. ChangesAsset-lock funding and drain flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FFI
participant ManagedWalletInfo
participant TransactionBuilder
FFI->>ManagedWalletInfo: build asset-lock transaction
ManagedWalletInfo->>TransactionBuilder: select funding inputs
TransactionBuilder-->>ManagedWalletInfo: assembled transaction
ManagedWalletInfo-->>FFI: serialized transaction output
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
271-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated drain/CoinJoin validation block.
The exact same 19-line validation (
drain && credit_output_fundings.len() != 1andCoinJoin && !drain) is repeated verbatim inbuild_asset_lock(Lines 271-289) andbuild_asset_lock_with_signer(Lines 393-411). Extracting it into a small private helper avoids future drift between the two entry points.♻️ Proposed extraction
+fn validate_drain_funding( + funding_account: AssetLockFundingAccount, + credit_output_count: usize, + drain: bool, +) -> Result<(), AssetLockError> { + if drain && credit_output_count != 1 { + return Err(AssetLockError::Builder(BuilderError::InvalidData( + "drain asset lock requires exactly one credit output".into(), + ))); + } + if matches!(funding_account, AssetLockFundingAccount::CoinJoin { .. }) && !drain { + return Err(AssetLockError::Builder(BuilderError::InvalidData( + "CoinJoin-funded asset locks support drain mode only".into(), + ))); + } + Ok(()) +}Then in both
build_asset_lockandbuild_asset_lock_with_signer, replace the duplicated blocks with:- if drain && credit_output_fundings.len() != 1 { - return Err(AssetLockError::Builder(BuilderError::InvalidData( - "drain asset lock requires exactly one credit output".into(), - ))); - } - if matches!(funding_account, AssetLockFundingAccount::CoinJoin { .. }) && !drain { - return Err(AssetLockError::Builder(BuilderError::InvalidData( - "CoinJoin-funded asset locks support drain mode only".into(), - ))); - } + validate_drain_funding(funding_account, credit_output_fundings.len(), drain)?;Also applies to: 393-411
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 271 - 289, Extract the duplicated drain and CoinJoin validation from build_asset_lock and build_asset_lock_with_signer into a small private helper that accepts the relevant funding account, drain flag, and credit_output_fundings, returning the existing AssetLockError on invalid input. Replace both inline validation blocks with calls to this helper, preserving the current validation order and error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 353-380: Hoist the drained == 0 validation in the
SelectionStrategy::All path to immediately after calculating drained and before
updating the destination output or entering the special_payload branch. Return
the existing BuilderError::InsufficientFunds error for zero-value drains, then
remove the duplicate check from the AssetLockPayloadType handling while
preserving its credit output update.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 271-289: Extract the duplicated drain and CoinJoin validation from
build_asset_lock and build_asset_lock_with_signer into a small private helper
that accepts the relevant funding account, drain flag, and
credit_output_fundings, returning the existing AssetLockError on invalid input.
Replace both inline validation blocks with calls to this helper, preserving the
current validation order and error messages.
🪄 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 Plus
Run ID: aef4ebcf-7d56-47db-a153-67371ddb5891
📒 Files selected for processing (3)
key-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Comment on lines
+353
to
+380
| let drained = total_input.saturating_sub(selection.estimated_fee); | ||
| let [out] = tx_outputs.as_mut_slice() else { | ||
| return Err(BuilderError::InvalidData( | ||
| "SelectionStrategy::All requires exactly one output (the destination)".into(), | ||
| )); | ||
| }; | ||
| out.value = total_input.saturating_sub(selection.estimated_fee); | ||
| out.value = drained; | ||
| // An asset-lock drain must also rewrite the payload's credit | ||
| // output: the on-chain OP_RETURN burn (`out` above) mirrors the | ||
| // payload credit sum, and a mismatch is consensus-invalid. | ||
| if let Some(TransactionPayload::AssetLockPayloadType(payload)) = | ||
| &mut self.special_payload | ||
| { | ||
| let [credit] = payload.credit_outputs.as_mut_slice() else { | ||
| return Err(BuilderError::InvalidData( | ||
| "SelectionStrategy::All with an asset-lock payload requires exactly one \ | ||
| credit output" | ||
| .into(), | ||
| )); | ||
| }; | ||
| if drained == 0 { | ||
| return Err(BuilderError::InsufficientFunds { | ||
| available: total_input, | ||
| required: selection.estimated_fee, | ||
| }); | ||
| } | ||
| credit.value = drained; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Zero-value drain check only guards the asset-lock branch.
drained == 0 is only rejected inside the AssetLockPayloadType branch (Lines 363-378). For a plain (non-asset-lock) drain via SelectionStrategy::All, out.value = drained (Line 359) can be set to 0 with no error — the "reject zero-value drains" guarantee doesn't hold for the general-purpose drain path this same public builder exposes.
🐛 Proposed fix: hoist the zero check above the payload-specific branch
if self.selection_strategy == SelectionStrategy::All {
// Drain: the single output takes the whole balance minus fee (the caller's amount is
// ignored); no change.
let drained = total_input.saturating_sub(selection.estimated_fee);
+ if drained == 0 {
+ return Err(BuilderError::InsufficientFunds {
+ available: total_input,
+ required: selection.estimated_fee,
+ });
+ }
let [out] = tx_outputs.as_mut_slice() else {
return Err(BuilderError::InvalidData(
"SelectionStrategy::All requires exactly one output (the destination)".into(),
));
};
out.value = drained;
// An asset-lock drain must also rewrite the payload's credit
// output: the on-chain OP_RETURN burn (`out` above) mirrors the
// payload credit sum, and a mismatch is consensus-invalid.
if let Some(TransactionPayload::AssetLockPayloadType(payload)) =
&mut self.special_payload
{
let [credit] = payload.credit_outputs.as_mut_slice() else {
return Err(BuilderError::InvalidData(
"SelectionStrategy::All with an asset-lock payload requires exactly one \
credit output"
.into(),
));
};
- if drained == 0 {
- return Err(BuilderError::InsufficientFunds {
- available: total_input,
- required: selection.estimated_fee,
- });
- }
credit.value = drained;
}
} else if change_amount > 546 {📝 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.
Suggested change
| let drained = total_input.saturating_sub(selection.estimated_fee); | |
| let [out] = tx_outputs.as_mut_slice() else { | |
| return Err(BuilderError::InvalidData( | |
| "SelectionStrategy::All requires exactly one output (the destination)".into(), | |
| )); | |
| }; | |
| out.value = total_input.saturating_sub(selection.estimated_fee); | |
| out.value = drained; | |
| // An asset-lock drain must also rewrite the payload's credit | |
| // output: the on-chain OP_RETURN burn (`out` above) mirrors the | |
| // payload credit sum, and a mismatch is consensus-invalid. | |
| if let Some(TransactionPayload::AssetLockPayloadType(payload)) = | |
| &mut self.special_payload | |
| { | |
| let [credit] = payload.credit_outputs.as_mut_slice() else { | |
| return Err(BuilderError::InvalidData( | |
| "SelectionStrategy::All with an asset-lock payload requires exactly one \ | |
| credit output" | |
| .into(), | |
| )); | |
| }; | |
| if drained == 0 { | |
| return Err(BuilderError::InsufficientFunds { | |
| available: total_input, | |
| required: selection.estimated_fee, | |
| }); | |
| } | |
| credit.value = drained; | |
| } | |
| let drained = total_input.saturating_sub(selection.estimated_fee); | |
| if drained == 0 { | |
| return Err(BuilderError::InsufficientFunds { | |
| available: total_input, | |
| required: selection.estimated_fee, | |
| }); | |
| } | |
| let [out] = tx_outputs.as_mut_slice() else { | |
| return Err(BuilderError::InvalidData( | |
| "SelectionStrategy::All requires exactly one output (the destination)".into(), | |
| )); | |
| }; | |
| out.value = drained; | |
| // An asset-lock drain must also rewrite the payload's credit | |
| // output: the on-chain OP_RETURN burn (`out` above) mirrors the | |
| // payload credit sum, and a mismatch is consensus-invalid. | |
| if let Some(TransactionPayload::AssetLockPayloadType(payload)) = | |
| &mut self.special_payload | |
| { | |
| let [credit] = payload.credit_outputs.as_mut_slice() else { | |
| return Err(BuilderError::InvalidData( | |
| "SelectionStrategy::All with an asset-lock payload requires exactly one \ | |
| credit output" | |
| .into(), | |
| )); | |
| }; | |
| credit.value = drained; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 353 - 380, Hoist the drained == 0 validation in the SelectionStrategy::All
path to immediately after calculating drained and before updating the
destination output or entering the special_payload branch. Return the existing
BuilderError::InsufficientFunds error for zero-value drains, then remove the
duplicate check from the AssetLockPayloadType handling while preserving its
credit output update.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #915 +/- ##
==========================================
+ Coverage 74.54% 74.61% +0.06%
==========================================
Files 327 328 +1
Lines 75032 75973 +941
==========================================
+ Hits 55936 56688 +752
- Misses 19096 19285 +189
|
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
1 participant
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.
What
The asset-lock builders (
build_asset_lock/build_asset_lock_with_signer) previously hardcoded their funding source to standard BIP44 accounts. This adds:AssetLockFundingAccount { Bip44, CoinJoin }— which account family supplies (and signs) the funding UTXOs. CoinJoin funding lets mixed coins fund an asset lock directly, without first sweeping them through a transparent BIP44 address (which links the mixed UTXOs to a reusable transparent address for an extra hop).Σ inputs − fee. TheTransactionBuilder'sSelectionStrategy::Allpath now also rewrites the asset-lock payload credit output so the payload always mirrors the on-chain OP_RETURN burn (a mismatch is consensus-invalid), and a zero-value drain is rejected withInsufficientFunds.InvalidData.key-wallet-ffi'sbuild_asset_lockentry point keeps its exact behavior (BIP44, non-drain).Why
Consumed by dashpay/platform (
AssetLockFunding::DrainAccountBalance+ ashielded_fund_from_asset_lock_coinjoin_drainFFI) so the dashwallet-ios post-migration "move your mixed coins" flow can offer a Shielded destination that moves the whole CoinJoin balance into the shielded pool in one transaction, with no transparent intermediate hop.Tests
test_drain_coinjoin_asset_lock— all CoinJoin UTXOs consumed, burn =Σ − fee, payload credit mirrors the burn, no change output, inputs signed.test_drain_requires_single_credit_output,test_coinjoin_funding_requires_drain— guard rejections.key-wallet(551) +key-wallet-manager(48) unit suites pass.🤖 Generated with Claude Code
Summary by CodeRabbit