feat(key-wallet): let a drain carry zero-value OP_RETURN data outputs - #928
Open
HashEngineering wants to merge 3 commits into
Open
feat(key-wallet): let a drain carry zero-value OP_RETURN data outputs#928HashEngineering wants to merge 3 commits into
HashEngineering wants to merge 3 commits into
Conversation
SelectionStrategy::All required exactly one output, so a drain could not carry a memo. A MAYAChain deposit needs both: the vault output AND the memo as an OP_RETURN beside it, or the swap cannot be matched to its quote. That made "swap my whole balance" inexpressible — callers had to guess the fee, subtract it themselves and send an explicit amount, which under-pays the destination whenever the guess is low. Under-delivery is not a rounding annoyance: MAYAChain executes a swap for an amount the user never agreed to, and NEAR Intents refuses the deposit outright. Zero-value data carriers now ride along with a drain. They claim none of the drained balance, and effective_outputs_size already prices their bytes into the fee, so the drain arithmetic (total_input - estimated_fee to the single value carrier, no change) is unchanged. Guards kept tight: - an OP_RETURN with a non-zero value is refused: it would claim part of the drained balance - exactly one spendable output must remain; only zero-value OP_RETURN data outputs may accompany it - asset locks are unchanged: THEIR single output IS the OP_RETURN burn mirroring the payload credits, so it stays the value carrier - a drain whose inputs cannot cover the fee still returns typed InsufficientFunds Tests: 646 pass in key-wallet, including five new cases covering the data carrier beside the destination, the fee covering its bytes, two spendable outputs still refused, a value-bearing carrier refused, and the asset-lock carrier unchanged. Verified on testnet through the Android SDK and dash-wallet: a max Maya deposit builds as a drain carrying an 80-byte memo and reports 27442985 duffs deliverable with a measured 432-duff fee. The build is signed and released without broadcasting, so the path is proven without a transaction reaching the network. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDrain OP_RETURN support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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/transaction_builder.rs (1)
1882-1907: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the memo drain fee covers its signed serialized size.
memo_fee > plain_feepasses if the fee increases by too little. Return the memo transaction frombuildand assert thatmemo_feecovers its signed-size estimate.Proposed fix
- b.build_unsigned_reserved().expect("drain builds").1 + b.build_unsigned_reserved() + .map(|(tx, fee, _)| (tx, fee)) + .expect("drain builds") }; - let plain_fee = build(false); - let memo_fee = build(true); + let (_plain_tx, plain_fee) = build(false); + let (memo_tx, memo_fee) = build(true); // 60 payload bytes + OP_RETURN + push opcode + the 8-byte value and its script varint. assert!( memo_fee > plain_fee, "a drain carrying a {}-byte memo must cost more than a bare drain ({memo_fee} vs {plain_fee})", memo.len() ); + + const SIGNED_INPUT_SIZE: usize = 148; + const UNSIGNED_INPUT_SIZE: usize = 41; + let signed_size = (serialize(&memo_tx).len() + + memo_tx.input.len() * (SIGNED_INPUT_SIZE - UNSIGNED_INPUT_SIZE)) as u64; + assert!( + memo_fee >= signed_size, + "fee {memo_fee} must cover signed size {signed_size}" + );As per coding guidelines, “Write unit tests for new functionality.”
🤖 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 1882 - 1907, Strengthen test_drain_fee_covers_the_data_carrier_bytes by having build return the memo transaction alongside its fee, then assert memo_fee covers the transaction’s signed serialized-size estimate rather than only asserting it exceeds plain_fee. Preserve the existing memo and plain-drain comparisons while using the transaction’s actual signed-size calculation and fee-rate conventions already established in the surrounding code.Source: Coding guidelines
🤖 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 534-540: Replace the expect call in the transaction output
selection logic with error propagation returning BuilderError::InvalidData when
no spendable output is found. Preserve the existing asset-lock output path and
successful iterator selection behavior.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 1882-1907: Strengthen test_drain_fee_covers_the_data_carrier_bytes
by having build return the memo transaction alongside its fee, then assert
memo_fee covers the transaction’s signed serialized-size estimate rather than
only asserting it exceeds plain_fee. Preserve the existing memo and plain-drain
comparisons while using the transaction’s actual signed-size calculation and
fee-rate conventions already established in the surrounding code.
🪄 Autofix
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: 39272815-cfc0-4f90-9620-33d083156066
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
4 tasks
The fee test asserted only that a memo-carrying drain costs more than a bare one, which passes even when the extra fee falls short of the bytes the carrier adds — the case that would let a drain broadcast below the relay rate. Return the transactions from the build closure and assert against their serialized sizes: the fee exceeds the serialized bytes by the per-input signature allowance, that allowance is unchanged by the carrier, and the 71 bytes the carrier adds are priced at the transaction's own rate (equality, so under-pricing and upward drift both fail). Also assert the deliverable output shrank by exactly that extra fee — the zero-value carrier takes nothing from the destination beyond its bytes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Pushed The old assertion was only The actual numbers for a 60-byte memo: the carrier adds 71 bytes and 71 duffs, so it is priced at exactly 1 duff per byte, the same rate as the rest of the transaction. That let me replace the inequality with equalities:
|
The count check immediately above proves exactly one carrier exists, so neither arm can come back empty today — but this is library code, and a later change to that invariant would turn into a panic in a caller's process. Take `first_mut()` / `find(..)` as Options and return `BuilderError::InvalidData` when neither yields an output, so the invariant fails typed instead. Also drops the `tx_outputs[0]` index on the asset-lock arm, which could panic on an empty output set. Behavior is unchanged on every reachable path; 646 key-wallet tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #928 +/- ##
==========================================
+ Coverage 75.07% 75.54% +0.47%
==========================================
Files 328 328
Lines 77587 78116 +529
==========================================
+ Hits 58245 59011 +766
+ Misses 19342 19105 -237
|
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.
Problem
SelectionStrategy::Allrequires exactly one output, so a drain cannot carry a memo. A MAYAChain deposit needs both: the vault output and the memo as an OP_RETURN beside it, or the swap cannot be matched to its quote.That made "swap my whole balance" inexpressible. Callers had to guess the fee, subtract it themselves and send an explicit amount — which under-pays the destination whenever the guess is low. Under-delivery is not a rounding annoyance: MAYAChain executes a swap for an amount the user never agreed to, and NEAR Intents refuses the deposit outright, parking the funds for roughly an hour before refunding minus a 0.001 DASH fee.
Change
Zero-value data carriers now ride along with a drain. They claim none of the drained balance, and
effective_outputs_sizealready prices their bytes into the fee, so the drain arithmetic is unchanged: the single value-carrying output takestotal_input − estimated_fee, with no change output.Guards kept tight:
InsufficientFundsOne file, +168/−4.
Tests
646 pass in
key-wallet, including five new cases: the data carrier beside the destination, the fee covering its bytes, two spendable outputs still refused, a value-bearing carrier refused, and the asset-lock carrier unchanged.Verification
Exercised on testnet through the Android SDK and dash-wallet: a max Maya deposit builds as a drain carrying an 80-byte memo and reports 27,442,985 duffs deliverable with a measured 432-duff fee. The build is signed and released without broadcasting, so the whole path is verified without a transaction reaching the network.
Notes for reviewers
This is the drain case of #922 (merged), which added the OP_RETURN output, output-order control and change-to-VIN0 — the combination that PR did not cover. It sits directly on
devand needs no other PR as a base.Reaching it from a host also needs the SDK surface: a
selectionStrategyparameter onbuildSignedPaymentand a way to read the engine-computed amount. That is a separatedashpay/platformchange; without it the capability is present in the engine but unreachable from Kotlin.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes