feat(key-wallet): OP_RETURN outputs, output-order control and change-to-VIN0 - #922
feat(key-wallet): OP_RETURN outputs, output-order control and change-to-VIN0#922romchornyi wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds OP_RETURN output support to the transaction builder with an 80-byte relay-policy limit constant and dedicated size validation. The builder now supports configurable insertion-order preservation and change routing to the first selected input. Size estimation uses serialized output sizes. Tests validate OP_RETURN limits, feature interactions, and backward compatibility. ChangesTransaction Builder OP_RETURN and Change Routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TransactionBuilder
Caller->>TransactionBuilder: add_op_return(data)
TransactionBuilder->>TransactionBuilder: validate length against DEFAULT_MAX_OP_RETURN_BYTES
alt data exceeds limit
TransactionBuilder-->>Caller: BuilderError::OpReturnDataTooLarge
else valid
TransactionBuilder->>TransactionBuilder: append zero-value OP_RETURN output
Caller->>TransactionBuilder: change_to_first_input() / preserve_output_order()
TransactionBuilder->>TransactionBuilder: select inputs and estimate size
TransactionBuilder->>TransactionBuilder: sort inputs by BIP-69
TransactionBuilder->>TransactionBuilder: compute change output
TransactionBuilder->>TransactionBuilder: determine change script from first input or address
alt preserve_output_order
TransactionBuilder->>TransactionBuilder: skip output sorting
else default
TransactionBuilder->>TransactionBuilder: sort outputs by BIP-69
end
TransactionBuilder-->>Caller: assembled transaction
end
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}
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 74.76% 74.79% +0.03%
==========================================
Files 328 328
Lines 76593 76942 +349
==========================================
+ Hits 57267 57552 +285
- Misses 19326 19390 +64
|
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
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.
2 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
Tests