feat(key-wallet): OP_RETURN outputs, output-order control and change-to-VIN0 - #922
Conversation
|
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:
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)
📝 WalkthroughWalkthroughThis PR adds OP_RETURN output support with an 80-byte limit, output-order preservation, and first-input change routing. Transaction size estimation now uses serialized output and script sizes. Tests cover routing, network validation, fee sizing, limits, dust thresholds, and legacy compatibility. ChangesTransaction Builder OP_RETURN and Change Routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant TransactionBuilder
participant CoinSelection
participant Transaction
Wallet->>TransactionBuilder: add outputs and builder options
TransactionBuilder->>CoinSelection: estimate serialized size and select inputs
CoinSelection-->>TransactionBuilder: selected inputs
TransactionBuilder->>Transaction: sort inputs and assemble outputs
Transaction-->>Wallet: built transaction and fee
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 188-192: Update the documentation for add_output and
preserve_output_order in the transaction builder to state that BIP-69 output
sorting is enabled by default, while calling preserve_output_order disables
sorting and retains insertion order.
- Around line 29-31: Replace the hardcoded MAX_STANDARD_OP_RETURN_BYTES constant
with a relay-limit value supplied by the selected network policy or
TransactionBuilder configuration, and thread that value through add_op_return
and related builder construction paths. Preserve the existing payload validation
behavior while allowing networks or nodes with different OP_RETURN limits to
provide their policy-specific value.
- Around line 816-825: Update the legacy helper’s
CoinSelector::select_coins_with_size call to pass CHANGE_OUTPUT_SIZE when
change_addr exists and 0 for drain builds, matching the production selector
path. Adjust the surrounding build_unsigned_legacy logic as needed so the
regression test uses the same selector input and fee behavior as production.
- Around line 462-472: Update the change-output selection in TransactionBuilder
around change_to_first_input and set_change_address so input-derived change is
validated against the configured change address network. Reject mismatched
first_input.address.network and change_addr.network with the existing builder
error mechanism before assembling outputs, while preserving the current behavior
for matching networks and explicit change addresses.
- Around line 220-222: Update the change-output selector in the transaction size
estimation flow to use should_estimate_change_output() rather than checking only
change_addr. Ensure calculate_base_size(), coin selection, and final assembly
consistently budget a change output when change_to_first_input is enabled.
🪄 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: 404b3092-33ba-4f3f-bf97-cdebb1a812ea
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #922 +/- ##
==========================================
- Coverage 75.06% 75.06% -0.01%
==========================================
Files 328 328
Lines 77255 77587 +332
==========================================
+ Hits 57992 58241 +249
- Misses 19263 19346 +83
|
df1fe31 to
8677e3d
Compare
|
Pushed Fixed
Not changed, with reasons
|
8677e3d to
c9d33a4
Compare
|
Pushed Injected the OP_RETURN relay limit (comment 1)
pub fn set_max_op_return_bytes(mut self, max_bytes: usize) -> Self
Network validation for input-derived change (comment 4) — not adding
There is also nothing to validate against:
Unrelated heads-up for whoever picks this up downstream: rebasing onto current |
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)
752-756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
thiserrorforBuilderError.
BuilderErrorstill manually implementsfmt::Displayandstd::error::Error. Addthiserrortokey-wallet/Cargo.toml, derivethiserror::Error, and move the variant messages into#[error(...)]; keepCoinSelectionas a source for proper cause support.🤖 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 752 - 756, Add the thiserror crate to key-wallet/Cargo.toml, then refactor the BuilderError enum to derive thiserror::Error. Move the error messages from the manual fmt::Display implementation into #[error(...)] attributes on each variant, including OpReturnDataTooLarge. Preserve the CoinSelection variant as a source by using #[source] to maintain proper error-cause support. Remove the old manual Display and Error trait implementations once all variants have error messages.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 240-254: Derive a conservative serialized change-output size from
all eligible input scripts before coin selection, accounting for routed change
addresses that may produce WitnessProgram scripts rather than assuming
CHANGE_OUTPUT_SIZE. Use this derived size consistently in calculate_base_size()
and select_coins_with_size() whenever change_to_first_input() or another change
path is enabled, while preserving existing behavior for P2PKH change. Extend the
tests around the existing change-selection cases near the routed change logic to
cover a larger change script and verify the estimate remains sufficient.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 752-756: Add the thiserror crate to key-wallet/Cargo.toml, then
refactor the BuilderError enum to derive thiserror::Error. Move the error
messages from the manual fmt::Display implementation into #[error(...)]
attributes on each variant, including OpReturnDataTooLarge. Preserve the
CoinSelection variant as a source by using #[source] to maintain proper
error-cause support. Remove the old manual Display and Error trait
implementations once all variants have error messages.
🪄 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: 15f53f53-9841-4632-8338-07da4aadf7d2
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
eebacae to
588eb21
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Estimate routed change using the largest eligible input script and reject configured network mismatches. Add regression coverage for P2WSH sizing and cross-network change routing.
588eb21 to
ac5a210
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (2)
884-985: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider pinning golden bytes instead of duplicating the assembly path.
build_unsigned_legacyre-implements about 100 lines of the pre-change assembly. The copy drifts asassemble_unsignedchanges, and a future change can be mirrored into both paths, which hides the regression the helper exists to catch. A recorded hex transaction plus the expected fee gives the same guarantee without the duplicate logic.This is optional. Keep the helper if you prefer the behavioural comparison.
🤖 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 884 - 985, Optionally replace the duplicated assembly logic in build_unsigned_legacy with a pinned golden transaction hex and expected fee, using the recorded bytes to validate the pre-change behavior. If retaining the helper, no change is required because the review explicitly allows the behavioral comparison.
512-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a typed error over
InvalidData.The network mismatch returns
BuilderError::InvalidDatawith a message. The test at Lines 1449-1453 asserts on a substring of that message, so any wording change breaks the test. Add a dedicated variant, in the same way as the newOpReturnDataTooLarge.♻️ Proposed variant
/// OP_RETURN payload exceeds the standard relay-policy size. OpReturnDataTooLarge { len: usize, max: usize, }, + /// The first-input change address and the configured change address + /// belong to different networks. + ChangeAddressNetworkMismatch,- return Err(BuilderError::InvalidData( - "Input-derived change address network does not match configured change address" - .into(), - )); + return Err(BuilderError::ChangeAddressNetworkMismatch);🤖 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 512 - 519, Replace the network-mismatch `BuilderError::InvalidData` return in the transaction builder with a dedicated typed `BuilderError` variant, following the existing `OpReturnDataTooLarge` pattern. Add and use the new variant so callers and tests no longer depend on the current error-message wording.
🤖 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 182-206: Update the pull request title to use the required
Conventional Commit prefix, using “feat” because this change adds
transaction-builder capability; preserve the existing title description after
the prefix.
- Around line 506-532: Replace the hardcoded 546 threshold in the change-output
branch with the script-specific threshold derived from
estimated_change_output_size(), and use that same threshold during coin
selection and output construction. Ensure routed change scripts, including
P2WSH, are rejected when change_amount is at or below their calculated dust
threshold.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 884-985: Optionally replace the duplicated assembly logic in
build_unsigned_legacy with a pinned golden transaction hex and expected fee,
using the recorded bytes to validate the pre-change behavior. If retaining the
helper, no change is required because the review explicitly allows the
behavioral comparison.
- Around line 512-519: Replace the network-mismatch `BuilderError::InvalidData`
return in the transaction builder with a dedicated typed `BuilderError` variant,
following the existing `OpReturnDataTooLarge` pattern. Add and use the new
variant so callers and tests no longer depend on the current error-message
wording.
🪄 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: 71f74a8a-d9b0-4c1f-be9c-7b85a55d02e8
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
| /// Add an OP_RETURN output carrying `data` (value 0). | ||
| /// | ||
| /// Errors if `data` exceeds [`MAX_STANDARD_OP_RETURN_BYTES`]. | ||
| pub fn add_op_return(mut self, data: &[u8]) -> Result<Self, BuilderError> { | ||
| if data.len() > MAX_STANDARD_OP_RETURN_BYTES { | ||
| return Err(BuilderError::OpReturnDataTooLarge { | ||
| len: data.len(), | ||
| max: MAX_STANDARD_OP_RETURN_BYTES, | ||
| }); | ||
| } | ||
|
|
||
| let push_bytes = | ||
| <&PushBytes>::try_from(data).map_err(|_| BuilderError::OpReturnDataTooLarge { | ||
| len: data.len(), | ||
| max: MAX_STANDARD_OP_RETURN_BYTES, | ||
| })?; | ||
| self.outputs.push(TxOut { | ||
| value: 0, | ||
| script_pubkey: Builder::new() | ||
| .push_opcode(opcodes::all::OP_RETURN) | ||
| .push_slice(push_bytes) | ||
| .into_script(), | ||
| }); | ||
| Ok(self) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a Conventional Commit prefix to the PR title.
The title reads "OP_RETURN Support and Change Routing in Transaction Builder". The pr-title.yml check requires one of build, chore, ci, docs, feat, fix, refactor, or test. This PR adds new builder capability, so use a feat prefix, for example feat: OP_RETURN support and change routing in transaction builder.
As per path instructions, "Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes."
🤖 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 182 - 206, Update the pull request title to use the required Conventional
Commit prefix, using “feat” because this change adds transaction-builder
capability; preserve the existing title description after the prefix.
Source: Path instructions
`change_amount > 546` hard-codes the dust threshold for a 34-byte P2PKH output. With `change_to_first_input` the change script is whatever VIN0 uses, and Dash Core derives dust from the serialized output plus the 148-byte input that would spend it — so a 43-byte output has a 573-duff threshold. A 550-duff routed change output cleared the flat 546 check and could then be rejected as dust by the network. Derive the threshold with `3 * (output_size + 148)`, sized from the same `estimated_change_output_size()` that coin selection already uses, so selection and output construction agree. Hoisted above the point `self.outputs` is moved. Nothing moves for the ordinary path: the formula is exactly 546 for a 34-byte P2PKH output, which `test_dust_threshold_follows_the_change_output_size` pins alongside the 43-byte case.
|
Pushed Dust threshold now follows the routed change script You are right, and the arithmetic lines up exactly:
Behaviour is unchanged for the ordinary path — the formula returns exactly 546 for P2PKH — and PR title That comment looks like it was raised against an earlier revision of the title. It currently reads
|
4 participants
Issue being fixed or feature implemented
The Dash iOS wallet is restoring MAYACHAIN swap routes. MAYAChain's UTXO deposit
contract (docs,
"UTXO Chains") requires a very specific transaction shape:
VOUT0— Asgard vault paymentVOUT1— the swap memo in anOP_RETURNVOUT2— change paid back to the VIN0 addressTransactionBuildercould express none of it. The change rule is theload-bearing one: "Do not use HD wallets that forward the change to a new
address, because MAYAChain IDs the user as the address in VIN0. The user must
keep their VIN0 address funded for refunds."
set_fundingassignsnext_change_address()— exactly the pattern that breaks — and it breakssilently: the swap succeeds, and only a later refund goes to an address the
user was never told to watch.
This cannot be worked around downstream. Consumers that fund and sign in a
single call (the Swift SDK's FFI builder) have no seam to patch outputs
afterwards, so the shape has to be expressible on the builder itself.
What was done?
All in
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs.add_op_return(&[u8]) -> Result<Self, BuilderError>— appends azero-value
OP_RETURNoutput. Payloads over the 80-byte standardness limitreturn the new
BuilderError::OpReturnDataTooLargeinstead of panickinginside
ScriptBuf.MAX_STANDARD_OP_RETURN_BYTESispubso FFI callers canpre-check before handing over a builder the call would consume.
preserve_output_order()— skips BIP-69 output sorting.change_to_first_input()— routes change to the address of the firstinput after the BIP-69 input sort. This required moving
selected_inputs.sort_by(bip69_input_sorter)above change construction: VIN0is not known until the sort has run, while change was previously pushed
before it.
calculate_base_sizenow measures each output's real serialized size(
8 + varint(script_len) + script_len) instead of charging a flatTX_OUTPUT_SIZEper output.The fee change is not cosmetic. For the canonical Maya shape — 1 input, vault +
80-byte memo + change — the flat estimate gives 260 bytes against a real 318,
i.e. 0.82 duff/byte, under the 1 duff/byte relay minimum, so the transaction
can be rejected outright rather than merely underpaying.
Deliberately conservative choices, called out for review:
TX_OUTPUT_SIZErather than itsreal ~11 bytes, so identity-funding fees stay byte-identical;
caller.
How Has This Been Tested?
cargo test -p key-wallet— 21/21 in thetransaction_buildermodule, run onthis branch rebased onto current
dev.New tests:
test_maya_deposit_shape_preserves_output_order_and_routes_change_to_first_input— asserts output count and order, the
OP_RETURNpayload round-trip,output[2].script_pubkey== VIN0's address script (built with two inputs, soit genuinely exercises the post-sort behaviour), and that the fee covers the
signed size. The last point matters:
build_unsignedleaves everyscript_sigempty, so comparing the fee against the serialized bytes as-iswould pass regardless of how badly the estimate under-counted.
test_add_op_return_rejects_oversized_payload— returns the error rather thanpanicking.
test_default_ordinary_send_matches_legacy_bytes— an ordinarytwo-recipient send assembles byte-identically to the pre-change logic.
test_base_size_unchanged_for_pre_op_return_shapes— P2PKH and asset-locksize estimates match the pre-change formula exactly, pinning the "no fee
movement for existing shapes" claim.
Downstream verification: consumed by dashpay/platform#4286, which adds an
integration test building real Maya-shaped deposits, and manually smoke-tested
via a full MAYACHAIN swap from the Dash iOS wallet (dashpay/dashwallet-ios#916).
Breaking Changes
None. The three new methods are additive and opt-in;
calculate_base_sizeproduces identical results for every transaction shape that existed before this
change, which
test_base_size_unchanged_for_pre_op_return_shapesenforces.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests