feat(sdk): expose OP_RETURN, output-order and change-to-VIN0 controls - #4286
Open
romchornyi wants to merge 1 commit into
Open
feat(sdk): expose OP_RETURN, output-order and change-to-VIN0 controls#4286romchornyi wants to merge 1 commit into
romchornyi wants to merge 1 commit into
Conversation
…trols MAYAChain requires a UTXO deposit shaped as VOUT0=vault, VOUT1=OP_RETURN memo, VOUT2=change paid back to the VIN0 address, with no output reordering, and it identifies the depositor by VIN0 for refunds. https://docs.mayaprotocol.com/mayachain-dev-docs/concepts/sending-transactions `CoreTransactionBuilder.buildSigned` builds and signs in one FFI call, so none of this can be applied after the fact — it has to be expressed on the builder. FFI (rs-platform-wallet-ffi): - core_wallet_tx_builder_add_op_return / _preserve_output_order / _change_to_first_input, mirroring the existing setter style - an over-long payload is rejected before take_builder() runs, so a refused memo cannot leave the slot holding a mem::take default and silently drop outputs the caller already configured - core_wallet_signed_transaction_v2_bytes: read the finalized transaction bytes without broadcasting, so the deposit shape can be asserted pre-broadcast Swift SDK: - addOpReturn / preserveOutputOrder / changeToFirstInput - FinalizedCoreTransaction.serializedData() Tests: MayaDepositVerificationIntegrationTests builds short- and long-memo deposits and asserts output count/order, the OP_RETURN payload, VOUT2 == VIN0 scriptPubKey, the memo ceiling, the dust floor and a >= 1 duff/byte fee, then checks fee parity for ordinary, multi-recipient, selected-input, drain and asset-lock shapes so the precise output sizing does not move existing fees. CI: fail the workspace workflow if the local rust-dashcore [patch] override is still present in Cargo.toml. Depends on key-wallet gaining add_op_return / preserve_output_order / change_to_first_input (dashpay/rust-dashcore, branch feat/tx-builder-op-return). Until that lands and the rev in Cargo.toml is bumped, building this needs a local [patch] override, which is deliberately NOT committed.
romchornyi
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 4, 2026 12:39
Contributor
📝 WalkthroughWalkthroughThe PR adds finalized V2 transaction serialization, OP_RETURN and output-routing controls across the Rust FFI and Swift SDK, and integration tests for Maya deposits and fee parity. The macOS workflow rejects local ChangesCore wallet transaction flow
Workspace validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MayaDepositVerificationIntegrationTests
participant CoreTransactionBuilder
participant CoreWalletFFI
participant SPVWallet
MayaDepositVerificationIntegrationTests->>SPVWallet: fund wallet and select UTXOs
MayaDepositVerificationIntegrationTests->>CoreTransactionBuilder: build deposit transaction
CoreTransactionBuilder->>CoreWalletFFI: add OP_RETURN and configure outputs
CoreWalletFFI-->>CoreTransactionBuilder: finalize transaction
CoreTransactionBuilder-->>MayaDepositVerificationIntegrationTests: return transaction
MayaDepositVerificationIntegrationTests->>SPVWallet: decode transaction and calculate fee
SPVWallet-->>MayaDepositVerificationIntegrationTests: transaction data and fee
Possibly related PRs
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 |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit 6f70092) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4286 +/- ##
============================================
+ Coverage 87.52% 87.54% +0.01%
============================================
Files 2678 2679 +1
Lines 341047 341467 +420
============================================
+ Hits 298518 298933 +415
- Misses 42529 42534 +5
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs (1)
311-321: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestore the builder when
add_op_returnfails.
take_builder()replaces the storedTransactionBuilderwith themem::takedefault. Ifb.add_op_return(bytes)returnsErr,bis dropped and the slot keeps that default, so later builder operations or finalization are no longer based on previously configured inputs, outputs, and options.TransactionBuilderdoes not deriveClone, so the error path needs to avoid requiringb.clone()unless this dependency is changed to support it.🤖 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 `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs` around lines 311 - 321, Update the add_op_return error path in the transaction-building method to restore the original TransactionBuilder into the shared builder slot before returning the error. Preserve b without cloning, store it through the existing store_builder mechanism on failure, and keep the current error result unchanged.
🤖 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
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift`:
- Around line 147-158: Guard all transaction collection accesses in the
verification flow before indexing: replace direct access to decoded.outputs[1],
decoded.outputs[0], and decoded.inputs[0] with safe first-element handling via
XCTUnwrap or equivalent count assertions. Ensure malformed transaction shapes
produce readable XCTest failures before evaluating opReturnPayload or
findMatchedUTXO, while preserving the existing outputTwoMatchesInputZeroScript
logic.
- Around line 55-57: Prevent testPrompt04StaticProofAndLegacyFeeParity from
hanging local Swift SDK CI by skipping it or splitting it so the long SPV
bootstrap and waitForSpendable flow is not run by the enabled run_tests.sh suite
until bootstrap stalls are resolved; do not add a local stopSpv call because
IntegrationTestCase.tearDown and suite cleanup already handle SPV teardown.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 311-321: Update the add_op_return error path in the
transaction-building method to restore the original TransactionBuilder into the
shared builder slot before returning the error. Preserve b without cloning,
store it through the existing store_builder mechanism on failure, and keep the
current error result unchanged.
🪄 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: b079eaf5-5ec2-4fda-b22a-a9b48a57753f
📒 Files selected for processing (5)
.github/workflows/tests-rs-workspace.ymlpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift
Comment on lines
+55
to
+57
| func testPrompt04StaticProofAndLegacyFeeParity() async throws { | ||
| try env.walletManager.startSpv(config: env.spvConfig) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether integration tests run in CI and how SPV is torn down.
fd -t f -e yml -e yaml . .github/workflows | xargs -r rg -nP -C4 'SwiftDashSDKIntegrationTests|swift test'
fd -t f 'IntegrationTestCase.swift' | xargs -r rg -nP -C6 '(func tearDown|stopSpv|startSpv)'Repository: dashpay/platform
Length of output: 505
🏁 Script executed:
#!/bin/bash
set -e
echo "== workflow files =="
git ls-files .github/workflows | sed -n '1,120p'
echo
echo "== Swift test / SwiftDashSDKIntegrationTests references =="
rg -n -C3 'SwiftDashSDKIntegrationTests|swift-sdk|swift test|integration' .github packages/swift-sdk --glob '*.yml' --glob '*.yaml' --glob '*.swift' || true
echo
echo "== relevant swift test file slice =="
file="packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift"
if [ -f "$file" ]; then
wc -l "$file"
sed -n '1,180p' "$file"
fi
echo
echo "== integration test case references =="
rg -n -C8 'class IntegrationTestCase|open class IntegrationTestCase|func tearDown|func setUp|stopSpv|startSpv|wallet.*90|spendable|startSpv' packages/swift-sdk --glob '*.swift' || trueRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
echo "== locate IntegrationTestCase and relevant test file =="
fd -t f 'IntegrationTestCase.swift|MayaDepositVerificationIntegrationTests.swift' packages/swift-sdk | sort
echo
echo "== IntegrationTestCase.swift =="
file="$(fd -t f 'IntegrationTestCase.swift' packages/swift-sdk | head -n1)"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,180p' "$file"
fi
echo
echo "== targeted startSpv/stopSpv in IntegrationTestCase and MayaDeposit file =="
for f in $(fd -t f 'IntegrationTestCase.swift|MayaDepositVerificationIntegrationTests.swift' packages/swift-sdk); do
echo "-- $f --"
rg -n -C4 'class IntegrationTestCase|open class IntegrationTestCase|func setUp|func tearDown|async.*setUp|async.*tearDown|startSpv|stopSpv|restartWalletManager|resetState|skipIfDisabled' "$f" || true
done
echo
echo "== CI workflow names containing Swift or package names =="
python3 - <<'PY'
import os, re
root=".github/workflows"
matches=[]
for fn in sorted(os.listdir(root)):
path=os.path.join(root,fn)
if not os.path.isfile(path) or fn not in ('workflow' in fn or fn.endswith('.yml') or fn.endswith('.yaml')):
pass
try:
data=open(path,encoding='utf-8').read()
except Exception:
continue
if re.search(r'SwiftDashSDKIntegrationTests|swift-sdk|swift test', data):
matches.append((fn, len([l for l in data.splitlines() if re.search(r'SwiftDashSDKIntegrationTests|swift-sdk|swift test', l)])))
print('\n'.join(f"{n} {c}" for n,c in matches))
# Print focused lines for matches, bounded.
for fn, _ in matches:
print(f"\n--- {fn} ---")
out=os.popen(f"rg -n -C3 'SwiftDashSDKIntegrationTests|swift-sdk|swift test' .github/workflows/{fn}").read()
print(out or "(no focused lines)")
PYRepository: dashpay/platform
Length of output: 4991
🏁 Script executed:
#!/bin/bash
set -e
echo "== targeted workflow contents for SwiftDashSDKIntegrationTests =="
rg -n -C4 'SwiftDashSDKIntegrationTests|swift test|Swif(.*?)test|integration tests|RUN_INTEGRATION_TESTS' .github/workflows || echo "no matches found"
echo
echo "== workflow files list =="
git ls-files .github/workflows | sortRepository: dashpay/platform
Length of output: 3239
🏁 Script executed:
#!/bin/bash
set -e
echo "== swift-sdk-build.yml relevant lines =="
wc -l .github/workflows/swift-sdk-build.yml
sed -n '1,240p' .github/workflows/swift-sdk-build.yml
echo
echo "== swift-sdk package files mentioning tests/enablers =="
rg -n -C3 'RUN_INTEGRATION_TESTS|SwiftDashSDKIntegrationTests|swift-sdk-build|swift test|test' packages/swift-sdk .github -g '!**/*.swift' || trueRepository: dashpay/platform
Length of output: 50375
Keep this test from hanging in local Swift SDK CI runs.
This test is enabled by packages/swift-sdk/run_tests.sh, and CI runs that script for swift-sdk-build. It waits on multiple 90-second waitForSpendable windows, so a bootstrap stall before assertions can hang the local Swift SDK test job. Skip or split it until the SPV bootstrap path does not stall.
startSpv does not require a local stopSpv because IntegrationTestCase.tearDown calls env.resetState(), and the Swift bundle observer calls cleanupSpvCache() at suite end.
🤖 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
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift`
around lines 55 - 57, Prevent testPrompt04StaticProofAndLegacyFeeParity from
hanging local Swift SDK CI by skipping it or splitting it so the long SPV
bootstrap and waitForSpendable flow is not run by the enabled run_tests.sh suite
until bootstrap stalls are resolved; do not add a local stopSpv call because
IntegrationTestCase.tearDown and suite cleanup already handle SPV teardown.
Comment on lines
+147
to
+158
| let decoded = try TransactionDecoder.decode(txData, network: .regtest) | ||
| let memoOutput = decoded.outputs[1] | ||
| let decodedMemoData = try XCTUnwrap(opReturnPayload(from: memoOutput.scriptPubkey)) | ||
| let decodedMemo = try XCTUnwrap(String(data: decodedMemoData, encoding: .utf8)) | ||
|
|
||
| let inputZeroMatch = try findMatchedUTXO(for: decoded.inputs[0], in: utxosBeforeBuild) | ||
| let outputTwoMatchesInputZeroScript: Bool | ||
| if decoded.outputs.count == 3 { | ||
| outputTwoMatchesInputZeroScript = decoded.outputs[2].scriptPubkey == inputZeroMatch.scriptPubkey | ||
| } else { | ||
| outputTwoMatchesInputZeroScript = false | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the output and input counts before indexing.
decoded.outputs[1], decoded.outputs[0], and decoded.inputs[0] are indexed without a count check. If the builder returns fewer outputs or inputs than expected, the subscript traps and the whole test process crashes. The shape assertions in assertDepositObservation at Lines 181-182 run only after this indexing, so they cannot catch the case.
Use XCTUnwrap on first, or assert the counts here before indexing, so a wrong shape produces a readable test failure.
🛡️ Proposed fix
let decoded = try TransactionDecoder.decode(txData, network: .regtest)
+ try XCTSkipIf(false)
+ guard decoded.outputs.count >= 2, !decoded.inputs.isEmpty else {
+ throw NSError(domain: "MayaVerification", code: 6, userInfo: [
+ NSLocalizedDescriptionKey:
+ "Unexpected deposit shape: \(decoded.inputs.count) inputs, \(decoded.outputs.count) outputs"
+ ])
+ }
let memoOutput = decoded.outputs[1]📝 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 decoded = try TransactionDecoder.decode(txData, network: .regtest) | |
| let memoOutput = decoded.outputs[1] | |
| let decodedMemoData = try XCTUnwrap(opReturnPayload(from: memoOutput.scriptPubkey)) | |
| let decodedMemo = try XCTUnwrap(String(data: decodedMemoData, encoding: .utf8)) | |
| let inputZeroMatch = try findMatchedUTXO(for: decoded.inputs[0], in: utxosBeforeBuild) | |
| let outputTwoMatchesInputZeroScript: Bool | |
| if decoded.outputs.count == 3 { | |
| outputTwoMatchesInputZeroScript = decoded.outputs[2].scriptPubkey == inputZeroMatch.scriptPubkey | |
| } else { | |
| outputTwoMatchesInputZeroScript = false | |
| } | |
| let decoded = try TransactionDecoder.decode(txData, network: .regtest) | |
| try XCTSkipIf(false) | |
| guard decoded.outputs.count >= 2, !decoded.inputs.isEmpty else { | |
| throw NSError(domain: "MayaVerification", code: 6, userInfo: [ | |
| NSLocalizedDescriptionKey: | |
| "Unexpected deposit shape: \(decoded.inputs.count) inputs, \(decoded.outputs.count) outputs" | |
| ]) | |
| } | |
| let memoOutput = decoded.outputs[1] | |
| let decodedMemoData = try XCTUnwrap(opReturnPayload(from: memoOutput.scriptPubkey)) | |
| let decodedMemo = try XCTUnwrap(String(data: decodedMemoData, encoding: .utf8)) | |
| let inputZeroMatch = try findMatchedUTXO(for: decoded.inputs[0], in: utxosBeforeBuild) | |
| let outputTwoMatchesInputZeroScript: Bool | |
| if decoded.outputs.count == 3 { | |
| outputTwoMatchesInputZeroScript = decoded.outputs[2].scriptPubkey == inputZeroMatch.scriptPubkey | |
| } else { | |
| outputTwoMatchesInputZeroScript = false | |
| } |
🤖 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
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift`
around lines 147 - 158, Guard all transaction collection accesses in the
verification flow before indexing: replace direct access to decoded.outputs[1],
decoded.outputs[0], and decoded.inputs[0] with safe first-element handling via
XCTUnwrap or equivalent count assertions. Ensure malformed transaction shapes
produce readable XCTest failures before evaluating opReturnPayload or
findMatchedUTXO, while preserving the existing outputTwoMatchesInputZeroScript
logic.
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.
Issue being fixed or feature implemented
The Dash iOS wallet is restoring MAYACHAIN swap routes, which requires the DASH
deposit to carry the swap memo in an
OP_RETURN.CoreTransactionBuildercouldnot express that, so Maya was disabled during the DashSync unlink.
MAYAChain's UTXO deposit contract (docs,
"UTXO Chains") demands a specific shape:
VOUT0= Asgard vault,VOUT1= thememo as a zero-value
OP_RETURN,VOUT2= change paid back to the VIN0address, and no output reordering. The change rule matters because MAYAChain
identifies the depositor by VIN0 and pays refunds there — routing change to a
fresh HD address fails silently, with only a later refund going astray.
Because
finalize/build_signedfund and sign inside a single FFI call, none ofthis can be applied after the fact. It has to be expressible on the builder.
What was done?
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:core_wallet_tx_builder_add_op_return,..._preserve_output_orderand..._change_to_first_input, following the existing setter style. An over-longpayload is rejected before
take_builder()runs, so a refused memo cannotleave the slot holding a
mem::takedefault and silently drop outputs thecaller already configured.
packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:core_wallet_signed_transaction_v2_bytes— read a finalized transaction'sbytes without broadcasting, so the deposit shape can be asserted pre-broadcast.
packages/swift-sdk/.../CoreTransactionBuilder.swift:addOpReturn(_:),preserveOutputOrder(),changeToFirstInput(), andFinalizedCoreTransaction.serializedData()..github/workflows/tests-rs-workspace.yml: fail the workflow if a local[patch."https://github.com/dashpay/rust-dashcore"]override is left inCargo.toml— that override is invisible in review and produces a build thatonly works on one machine.
Depends on dashpay/rust-dashcore#922, which adds the underlying
add_op_return,preserve_output_orderandchange_to_first_inputtokey-wallet. Until thatmerges and the
revinCargo.tomlis bumped, building this locally needs thepatch override — deliberately not committed, which is what the new CI guard
enforces.
How Has This Been Tested?
Added
packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift,which builds short- and long-memo deposits against a local dashmate devnet and
asserts output count and order, the
OP_RETURNpayload,VOUT2== VIN0scriptPubKey, the 80-byte memo ceiling, Maya's dust floor and a ≥ 1 duff/byte
fee — then checks fee parity for ordinary, multi-recipient, selected-input,
drain and asset-lock shapes so the precise output sizing does not move existing
fees.
Also verified:
cargo check -p platform-wallet-ffi,./build_ios.sh --target ios --target sim, and a greendashpaybuild of theconsuming wallet app.
Known gap: the integration test currently stalls in SPV bootstrap on a
22k-block devnet (compact filters lag past the 180 s wait) and has not yet run
its assertions end to end.
Breaking Changes
None. All three builder controls are opt-in and default behaviour is unchanged.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit