feat(sdk): expose OP_RETURN, output-order and change-to-VIN0 controls - #4286
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.
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds finalized V2 transaction serialization, OP_RETURN and output-routing controls across the Rust FFI and Swift SDK. It adds opt-in Maya deposit and fee verification tests, updates ChangesCore wallet transaction flow
Workspace validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MayaDepositVerificationIntegrationTests
participant SPVWallet
participant CoreTransactionBuilder
participant CoreWalletFFI
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 serialized 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 unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit b3f2801) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4286 +/- ##
============================================
- Coverage 87.61% 87.04% -0.57%
============================================
Files 2704 2704
Lines 345206 345206
============================================
- Hits 302446 300483 -1963
- Misses 42760 44723 +1963
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The FFI allocation and Swift ownership plumbing follow existing repository conventions, but the exact PR head is not buildable because the pinned key-wallet revision lacks every newly referenced builder API and the imported constant has a different name upstream. The Maya integration test also does not require the expected VOUT2 change output and does not exercise the 80/81-byte OP_RETURN boundary or builder-state preservation after rejection.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Pin a key-wallet revision that provides the new builder API
The workspace still pins rust-dashcore and key-wallet to `70d4bf8e36057c58e02d56769a6e9760f701dd06`, which does not provide `TransactionBuilder::add_op_return`, `preserve_output_order`, or `change_to_first_input`, nor the imported OP_RETURN limit constant. At this exact head, `cargo check -p platform-wallet-ffi --locked` fails with E0432/E0599, so the native library and the new Swift API cannot be built. Upstream rust-dashcore PR #922 currently provides the methods at `eebacae3d1152609b04e9c9af05acc87ea9b32ad`, but exports the limit as `DEFAULT_MAX_OP_RETURN_BYTES`, not `MAX_STANDARD_OP_RETURN_BYTES`. After the upstream change is merged, pin all rust-dashcore workspace dependencies to a compatible revision, update `Cargo.lock`, and align the import with the exported name.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift:181-191: Require VOUT2 for fixtures that necessarily produce change
Both deposit fixtures leave millions of duffs after the vault payment and fee, so each transaction must contain exactly three outputs: vault, memo, and change. Allowing `outputCount == 2` and conditionally skipping the VIN0-script assertion lets a regression that suppresses change pass, even though change-to-VIN0 is the load-bearing behavior this test claims to verify. Require exactly three outputs and always validate VOUT2 against VIN0.
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift:13-17: Exercise the OP_RETURN 80/81-byte boundary
The long memo fixture is 79 UTF-8 bytes, and the later `memoBytes <= 80` assertion only verifies the fixture rather than the new API's boundary behavior. No test proves that an 80-byte payload succeeds, an 81-byte payload is rejected, or that a rejection preserves outputs and options already stored in the FFI builder. Add deterministic boundary coverage that rejects 81 bytes and then successfully finalizes the same builder with its earlier configuration intact, which directly tests the pre-`take_builder()` guarantee introduced by this PR.
| XCTAssertGreaterThanOrEqual(observation.outputCount, 2, "\(observation.name) output count below Maya minimum") | ||
| XCTAssertLessThanOrEqual(observation.outputCount, 3, "\(observation.name) output count above Maya maximum") | ||
| XCTAssertEqual(observation.actualVaultAmount, observation.depositAmount, "\(observation.name) VOUT0 amount mismatch") | ||
| XCTAssertTrue(observation.outputOneIsOpReturn, "\(observation.name) VOUT1 is not OP_RETURN") | ||
| XCTAssertEqual(observation.decodedMemo, observation.memo, "\(observation.name) memo payload mismatch") | ||
| XCTAssertGreaterThanOrEqual(observation.depositAmount, Constants.minimumDepositDuffs, "\(observation.name) deposit fell below Maya dust floor") | ||
| XCTAssertLessThanOrEqual(observation.memoBytes, Constants.maxMemoBytes, "\(observation.name) memo exceeded 80 bytes") | ||
| XCTAssertGreaterThanOrEqual(observation.feeDuffs, UInt64(observation.serializedSize), "\(observation.name) fee fell below 1 duff/byte") | ||
| if observation.outputCount == 3 { | ||
| XCTAssertTrue(observation.outputTwoMatchesInputZeroScript, "\(observation.name) VOUT2 does not return change to VIN0 scriptPubKey") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Require VOUT2 for fixtures that necessarily produce change
Both deposit fixtures leave millions of duffs after the vault payment and fee, so each transaction must contain exactly three outputs: vault, memo, and change. Allowing outputCount == 2 and conditionally skipping the VIN0-script assertion lets a regression that suppresses change pass, even though change-to-VIN0 is the load-bearing behavior this test claims to verify. Require exactly three outputs and always validate VOUT2 against VIN0.
| XCTAssertGreaterThanOrEqual(observation.outputCount, 2, "\(observation.name) output count below Maya minimum") | |
| XCTAssertLessThanOrEqual(observation.outputCount, 3, "\(observation.name) output count above Maya maximum") | |
| XCTAssertEqual(observation.actualVaultAmount, observation.depositAmount, "\(observation.name) VOUT0 amount mismatch") | |
| XCTAssertTrue(observation.outputOneIsOpReturn, "\(observation.name) VOUT1 is not OP_RETURN") | |
| XCTAssertEqual(observation.decodedMemo, observation.memo, "\(observation.name) memo payload mismatch") | |
| XCTAssertGreaterThanOrEqual(observation.depositAmount, Constants.minimumDepositDuffs, "\(observation.name) deposit fell below Maya dust floor") | |
| XCTAssertLessThanOrEqual(observation.memoBytes, Constants.maxMemoBytes, "\(observation.name) memo exceeded 80 bytes") | |
| XCTAssertGreaterThanOrEqual(observation.feeDuffs, UInt64(observation.serializedSize), "\(observation.name) fee fell below 1 duff/byte") | |
| if observation.outputCount == 3 { | |
| XCTAssertTrue(observation.outputTwoMatchesInputZeroScript, "\(observation.name) VOUT2 does not return change to VIN0 scriptPubKey") | |
| } | |
| XCTAssertEqual( | |
| observation.outputCount, | |
| 3, | |
| "\(observation.name) must contain vault, memo, and change outputs" | |
| ) | |
| XCTAssertEqual(observation.actualVaultAmount, observation.depositAmount, "\(observation.name) VOUT0 amount mismatch") | |
| XCTAssertTrue(observation.outputOneIsOpReturn, "\(observation.name) VOUT1 is not OP_RETURN") | |
| XCTAssertEqual(observation.decodedMemo, observation.memo, "\(observation.name) memo payload mismatch") | |
| XCTAssertGreaterThanOrEqual(observation.depositAmount, Constants.minimumDepositDuffs, "\(observation.name) deposit fell below Maya dust floor") | |
| XCTAssertLessThanOrEqual(observation.memoBytes, Constants.maxMemoBytes, "\(observation.name) memo exceeded 80 bytes") | |
| XCTAssertGreaterThanOrEqual(observation.feeDuffs, UInt64(observation.serializedSize), "\(observation.name) fee fell below 1 duff/byte") | |
| XCTAssertTrue(observation.outputTwoMatchesInputZeroScript, "\(observation.name) VOUT2 does not return change to VIN0 scriptPubKey") |
source: ['codex']
There was a problem hiding this comment.
Agreed, and this was the one worth catching. Fixed in 8370b6a2.
You are right that the fixtures make change mandatory: 0.5 DASH funding against a 200_000-duff deposit, and 0.4 DASH against 35_000_000, both leave a change output far above the 546-duff dust threshold. outputCount == 2 was unreachable for them, so the range check bought nothing and cost the assertion — a regression that dropped change entirely would have satisfied >= 2 && <= 3 and then skipped the VIN0 check under if observation.outputCount == 3, which is precisely the behaviour this test exists to prove.
Now:
XCTAssertEqual(observation.outputCount, 3, "\(observation.name) must be vault + memo + change")
...
XCTAssertTrue(observation.outputTwoMatchesInputZeroScript, "\(observation.name) VOUT2 does not return change to VIN0 scriptPubKey")with the VOUT2 assertion no longer conditional. I also moved the shape check ahead of the subscripting in buildDepositObservation, so a wrong shape fails readably instead of trapping on decoded.outputs[2].
One deliberate asymmetry worth recording, since it looks like the same check in two places: the app-side guard in dashpay/dashwallet-ios#916 (assertSwapDepositShape) still accepts 2 or 3 outputs. There the transaction is real, so change genuinely can fall below dust and be dropped by the builder — MAYAChain still identifies the depositor by VIN0 in that case, so a two-output deposit is valid. It is only here, where the fixture amounts rule that out, that three is the correct fixed expectation.
There was a problem hiding this comment.
Resolved in 8370b6a — Require VOUT2 for fixtures that necessarily produce change no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| static let maxMemoBytes = 80 | ||
| static let longMemo = | ||
| "=:ARB.GLD:0x51a1449b3B6D635EddeC781cD47a99221712De97:344233230e4/1/0:_/def:15/0" | ||
| static let shortMemo = | ||
| "=:r:thor166n4w5039meulfa3p6ydg60ve6ueac7tlt0jws:669458827/1/0:_/def:15/0" |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the OP_RETURN 80/81-byte boundary
The long memo fixture is 79 UTF-8 bytes, and the later memoBytes <= 80 assertion only verifies the fixture rather than the new API's boundary behavior. No test proves that an 80-byte payload succeeds, an 81-byte payload is rejected, or that a rejection preserves outputs and options already stored in the FFI builder. Add deterministic boundary coverage that rejects 81 bytes and then successfully finalizes the same builder with its earlier configuration intact, which directly tests the pre-take_builder() guarantee introduced by this PR.
source: ['codex']
There was a problem hiding this comment.
Good catch on both halves — the fixture proved nothing about the boundary, and nothing at all covered the rejection path. Added testOpReturnCeilingBoundaryAndRejectionPreservesBuilder in 8370b6a2.
It does exactly the two things you asked for:
let builder = try CoreTransactionBuilder(network: .regtest)
try builder.addOutput(address: vaultAddress, amountDuffs: depositAmount)
try builder.preserveOutputOrder()
try builder.changeToFirstInput()
XCTAssertThrowsError(try builder.addOpReturn(overCeiling)) // 81 bytes
// Same builder instance: if the rejection had consumed it, this would build a
// transaction missing the vault output and the ordering flags.
try builder.addOpReturn(atCeiling) // exactly 80 bytes
let tx = try builder.finalizeAtomic(...)then asserts three outputs, that VOUT0 is still the vault payment for the original amount, and that the 80-byte payload round-trips verbatim out of VOUT1.
The reuse-after-rejection part is the one I care about most, because it pins a guarantee that is otherwise invisible: core_wallet_tx_builder_add_op_return takes the builder by value on the Rust side, so the naive implementation drops it on error and take_builder's mem::take leaves a default in the slot — the vault output and both ordering flags would vanish silently and the next finalize would produce a plain send. The FFI validates the payload before take_builder() specifically to avoid that, and until now nothing exercised it.
Two caveats I would rather state than have you discover: the new test is gated behind MAYA_DEPOSIT_VERIFICATION=1 along with the rest of the suite (CodeRabbit flagged that run_tests.sh runs this bundle and a bootstrap stall would hang the job), and it has not been run yet — the SDK does not currently build locally because platform-wallet-ffi needs the key-wallet API from dashpay/rust-dashcore#922, which is still open. So treat it as written-and-reviewed, not as passing.
There was a problem hiding this comment.
Resolved in 8370b6a — Exercise the OP_RETURN 80/81-byte boundary no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Review follow-ups on the deposit verification test: - Require exactly three outputs and always assert VOUT2 against VIN0. Both fixtures leave millions of duffs after the vault payment and fee, so change is mandatory; accepting two outputs let a regression that suppresses change pass the very test that exists to prove change-to-VIN0. - Assert the output and input counts before indexing, so a wrong shape fails readably instead of trapping on an out-of-range subscript and taking the test process down. - Cover the 80/81-byte OP_RETURN boundary rather than just the fixture, and reuse the same builder after a rejected payload. That pins the FFI guarantee this branch adds: the size check runs before `take_builder()`, so a refused memo must leave already-configured outputs and options intact. - Gate the suite behind MAYA_DEPOSIT_VERIFICATION=1. `run_tests.sh` runs this bundle in CI, and these tests sit behind several 90-second waits on top of a full SPV bootstrap, so a bootstrap stall would hang the job rather than fail it. Also renames MAX_STANDARD_OP_RETURN_BYTES to DEFAULT_MAX_OP_RETURN_BYTES, following key-wallet making the ceiling configurable per builder.
|
Pushed Require exactly three outputs, always assert VOUT2 → VIN0 Agreed, and this was the important one. Both fixtures leave millions of duffs after the vault payment and fee, so change is mandatory; (For contrast, the app-side assertion in dashpay/dashwallet-ios#916 deliberately accepts 2 or 3, because a real deposit can drop change below the 546-duff dust threshold. Here the fixture amounts rule that out.) Guard counts before indexing Done. Shape assertions now run before any subscripting, so a wrong shape produces a readable failure instead of trapping and killing the test process. Exercise the 80/81-byte boundary Added Keep the suite from hanging local Swift SDK CI Gated behind On the red CI Every failing check has one root cause: Same failure in Rust workspace tests (macOS) and Kotlin SDK build; CodeRabbit or PastaClaw review failed only because no review had been submitted at that point and re-runs on its own. It clears when dashpay/rust-dashcore#922 merges and the rev here is bumped. One thing to flag before that bump, because it is not a one-liner: current So the rev bump wants either a companion change adapting Verification status, honestly: the Swift changes are not compile-verified locally — the SDK cannot be built here right now, since |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 55-68: Reindent the changed Swift code in the Maya deposit
verification integration tests, including the isEnabled declaration,
skipUnlessEnabled(), and the additional referenced blocks, to use 2 spaces per
nesting level as required by the repository EditorConfig. Preserve all logic and
formatting content aside from indentation.
🪄 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: 8a1944b3-0642-4c4b-a2e7-2ef01a88468e
📒 Files selected for processing (2)
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
# Conflicts: # .github/workflows/tests-rs-workspace.yml
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The exact head does not compile because the pinned rust-dashcore revision lacks every key-wallet builder API added by this PR. The new Swift setters also permit use-after-free after finalization; test execution and failure-reporting gaps should be addressed alongside those blockers.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Pin a key-wallet revision that provides the new builder API
The workspace pins rust-dashcore revision `08bf729de819f52973002f754342fedac14a06db`, which does not provide `DEFAULT_MAX_OP_RETURN_BYTES`, `TransactionBuilder::add_op_return`, `preserve_output_order`, or `change_to_first_input`. Running `cargo check -p platform-wallet-ffi` at this exact head fails with an unresolved import and missing-method errors at all four new call sites. Update every rust-dashcore workspace dependency and `Cargo.lock` to a revision containing these APIs before merging.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:292-324: Guard the new Swift setters after Rust consumes the builder
`finalizeAtomic`, `buildSigned`, and `finalizeSignedPayment` set `consumed` after Rust reclaims the `FFITransactionBuilder` and its inner builder with `Box::from_raw`, but the new `addOpReturn`, `preserveOutputOrder`, and `changeToFirstInput` methods do not check that state. Calling one of these public Swift methods after any consuming finalizer passes the retained non-null but dangling `handle` through the C ABI; `check_ptr!` only rejects null and the FFI then dereferences freed memory. Add the same `consumed` guard used by the finalizers before each new method crosses the FFI boundary.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift:55-71: Run the Maya verification coverage in an automated test path
Both tests in this new suite skip unless `MAYA_DEPOSIT_VERIFICATION=1`, and repository workflows and test scripts never set that variable. Normal Swift and CI runs therefore exercise none of the deposit output ordering, VIN0 change routing, 80/81-byte boundary, rejected-builder preservation, or fee-parity assertions. Add hermetic lower-level FFI coverage for these guarantees, or enable the integration suite in a dedicated reliable job once its SPV bootstrap dependency is stable.
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/MayaDepositVerificationIntegrationTests.swift:174-235: Validate the output shape before indexing VOUT1
`buildDepositObservation` reads `decoded.outputs[1]` before its output-count guard, despite the comment claiming that shape validation precedes subscripting. The boundary test similarly continues to index outputs after `XCTAssertEqual`, which records a failure but does not stop execution. A transaction-shape regression can therefore trap instead of producing a readable test failure. Guard the count before every output subscript, and throw a normal test error rather than `XCTSkip` when an invalid transaction shape is observed.
| @@ -274,6 +308,22 @@ public final class CoreTransactionBuilder { | |||
| return self | |||
| } | |||
|
|
|||
| /// Preserve outputs in insertion order for a MAYACHAIN-style deposit. | |||
| /// See https://docs.mayaprotocol.com/mayachain-dev-docs/concepts/sending-transactions | |||
| @discardableResult | |||
| public func preserveOutputOrder() throws -> CoreTransactionBuilder { | |||
| try core_wallet_tx_builder_preserve_output_order(handle).check() | |||
| return self | |||
| } | |||
|
|
|||
| /// Route change to the first selected input address (VIN0) for a MAYACHAIN-style deposit. | |||
| /// See https://docs.mayaprotocol.com/mayachain-dev-docs/concepts/sending-transactions | |||
| @discardableResult | |||
| public func changeToFirstInput() throws -> CoreTransactionBuilder { | |||
| try core_wallet_tx_builder_change_to_first_input(handle).check() | |||
| return self | |||
There was a problem hiding this comment.
🔴 Blocking: Guard the new Swift setters after Rust consumes the builder
finalizeAtomic, buildSigned, and finalizeSignedPayment set consumed after Rust reclaims the FFITransactionBuilder and its inner builder with Box::from_raw, but the new addOpReturn, preserveOutputOrder, and changeToFirstInput methods do not check that state. Calling one of these public Swift methods after any consuming finalizer passes the retained non-null but dangling handle through the C ABI; check_ptr! only rejects null and the FFI then dereferences freed memory. Add the same consumed guard used by the finalizers before each new method crosses the FFI boundary.
source: ['codex']
| /// Opt-in gate. `run_tests.sh` (which CI runs for `swift-sdk-build`) executes this bundle, | ||
| /// and these tests sit behind several 90-second `waitForSpendable` windows on top of a full | ||
| /// SPV bootstrap. A bootstrap stall would hang the job rather than fail it, so they run only | ||
| /// when explicitly requested until the bootstrap path is reliable. | ||
| private static let isEnabled = | ||
| ProcessInfo.processInfo.environment["MAYA_DEPOSIT_VERIFICATION"] == "1" | ||
|
|
||
| private func skipUnlessEnabled() throws { | ||
| try XCTSkipUnless( | ||
| Self.isEnabled, | ||
| "Set MAYA_DEPOSIT_VERIFICATION=1 to run the Maya deposit verification suite " | ||
| + "(requires a local dashmate devnet and a completed SPV bootstrap)." | ||
| ) | ||
| } | ||
|
|
||
| func testPrompt04StaticProofAndLegacyFeeParity() async throws { | ||
| try skipUnlessEnabled() |
There was a problem hiding this comment.
🟡 Suggestion: Run the Maya verification coverage in an automated test path
Both tests in this new suite skip unless MAYA_DEPOSIT_VERIFICATION=1, and repository workflows and test scripts never set that variable. Normal Swift and CI runs therefore exercise none of the deposit output ordering, VIN0 change routing, 80/81-byte boundary, rejected-builder preservation, or fee-parity assertions. Add hermetic lower-level FFI coverage for these guarantees, or enable the integration suite in a dedicated reliable job once its SPV bootstrap dependency is stable.
source: ['codex']
| let decoded = try TransactionDecoder.decode(try tx.serializedData(), network: .regtest) | ||
| XCTAssertEqual(decoded.outputs.count, 3, "vault + memo + change survived the rejection") | ||
| XCTAssertEqual(decoded.outputs[0].address, vaultAddress, "VOUT0 lost after the rejected memo") | ||
| XCTAssertEqual(decoded.outputs[0].valueDuffs, depositAmount) | ||
| XCTAssertEqual(decoded.outputs[1].valueDuffs, 0) | ||
| XCTAssertEqual( | ||
| opReturnPayload(from: decoded.outputs[1].scriptPubkey), atCeiling, | ||
| "an exactly-80-byte payload must be accepted and carried verbatim" | ||
| ) | ||
| } | ||
|
|
||
| private func buildDepositObservation( | ||
| name: String, | ||
| fundingDashAmounts: [Double], | ||
| depositAmountDuffs: UInt64, | ||
| memo: String | ||
| ) async throws -> DepositObservation { | ||
| let wallet = try await env.makeTestWallet(name: "maya-\(name)") | ||
| let coreWallet = wallet.getCoreWallet() | ||
| let platformWallet = wallet.getPlatformWallet() | ||
|
|
||
| for amount in fundingDashAmounts { | ||
| let address = try coreWallet.nextReceiveAddress() | ||
| _ = try await fundByMining(address: address, dash: amount) | ||
| } | ||
|
|
||
| let expectedSpendable = fundingDashAmounts.reduce(UInt64(0)) { partial, amount in | ||
| partial + UInt64((amount * 100_000_000).rounded()) | ||
| } | ||
| try await wallet.waitForSpendable(exactly: expectedSpendable, timeout: 90) | ||
|
|
||
| let utxosBeforeBuild = try bip44Utxos(for: platformWallet) | ||
| let vaultAddress = try await env.coreRPC.getNewAddress() | ||
| let memoData = Data(memo.utf8) | ||
|
|
||
| let builder = try CoreTransactionBuilder(network: .regtest) | ||
| try builder.addOutput(address: vaultAddress, amountDuffs: depositAmountDuffs) | ||
| try builder.addOpReturn(memoData) | ||
| try builder.preserveOutputOrder() | ||
| try builder.changeToFirstInput() | ||
| let tx = try builder.finalizeAtomic( | ||
| wallet: platformWallet, | ||
| accountType: .bip44, | ||
| accountIndex: Constants.bip44AccountIndex | ||
| ) | ||
| let txData = try tx.serializedData() | ||
|
|
||
| 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)) | ||
|
|
||
| // Both fixtures fund far more than the deposit plus fee, so change is always well above | ||
| // dust and the transaction must be exactly vault + memo + change. Assert the shape here, | ||
| // before any subscripting: a wrong shape must surface as a readable failure rather than | ||
| // trapping on an out-of-range index and taking the whole test process down. | ||
| XCTAssertEqual( | ||
| decoded.outputs.count, 3, | ||
| "\(name) must contain vault, memo and change outputs" | ||
| ) | ||
| XCTAssertFalse(decoded.inputs.isEmpty, "\(name) has no inputs") | ||
| guard decoded.outputs.count == 3, let firstInput = decoded.inputs.first else { |
There was a problem hiding this comment.
🟡 Suggestion: Validate the output shape before indexing VOUT1
buildDepositObservation reads decoded.outputs[1] before its output-count guard, despite the comment claiming that shape validation precedes subscripting. The boundary test similarly continues to index outputs after XCTAssertEqual, which records a failure but does not stop execution. A transaction-shape regression can therefore trap instead of producing a readable test failure. Guard the count before every output subscript, and throw a normal test error rather than XCTSkip when an invalid transaction shape is observed.
source: ['codex']
| /// Opt-in gate. `run_tests.sh` (which CI runs for `swift-sdk-build`) executes this bundle, | ||
| /// and these tests sit behind several 90-second `waitForSpendable` windows on top of a full | ||
| /// SPV bootstrap. A bootstrap stall would hang the job rather than fail it, so they run only | ||
| /// when explicitly requested until the bootstrap path is reliable. | ||
| private static let isEnabled = | ||
| ProcessInfo.processInfo.environment["MAYA_DEPOSIT_VERIFICATION"] == "1" | ||
|
|
||
| private func skipUnlessEnabled() throws { | ||
| try XCTSkipUnless( | ||
| Self.isEnabled, | ||
| "Set MAYA_DEPOSIT_VERIFICATION=1 to run the Maya deposit verification suite " | ||
| + "(requires a local dashmate devnet and a completed SPV bootstrap)." | ||
| ) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Use the configured two-space Swift indentation
The repository root .editorconfig applies two-space indentation to Swift files, while the newly added test code uses four spaces per nesting level. Reindent the new test file consistently to match the configured repository style.
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in this update — Use the configured two-space Swift indentation no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
The engine PR dropped the configurable ceiling (set_max_op_return_bytes / DEFAULT_MAX_OP_RETURN_BYTES) and went back to the plain MAX_STANDARD_OP_RETURN_BYTES constant; the FFI pre-check tracks the rename. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: the incompatible key-wallet pin and the consumed-builder use-after-free remain blocking, the output-shape guard remains a valid test suggestion, and automated Maya coverage remains intentionally deferred; the indentation finding is outdated as an actionable review item. The latest delta only renames the OP_RETURN constant and introduces no new defect, but it does not fix the dependency blocker: cargo check -p platform-wallet-ffi still fails at the exact head. One additional cumulative PR nitpick was identified: the newly exported byte-buffer function does not document its C ownership contract.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 2 blocking | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Pin a key-wallet revision that provides the new builder API
STILL VALID. The workspace and Cargo.lock remain pinned to rust-dashcore revision `08bf729de819f52973002f754342fedac14a06db`. Direct inspection of that revision confirms it lacks `MAX_STANDARD_OP_RETURN_BYTES` and `TransactionBuilder::{add_op_return,preserve_output_order,change_to_first_input}`. Running `cargo check -p platform-wallet-ffi` at the exact reviewed head fails with E0432 and E0599 for those symbols, so the native library and dependent Swift SDK cannot build. Revision `399066748ef5d64e5132d07232b6a9000270450c` is a descendant of the current pin and contains all four required API elements; update every rust-dashcore workspace dependency consistently and refresh Cargo.lock.
| #[no_mangle] | ||
| pub unsafe extern "C" fn core_wallet_signed_transaction_v2_bytes( | ||
| transaction_handle: Handle, | ||
| out_bytes: *mut *mut u8, | ||
| out_len: *mut usize, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_bytes); | ||
| check_ptr!(out_len); | ||
| *out_bytes = std::ptr::null_mut(); | ||
| *out_len = 0; | ||
|
|
||
| let bytes = unwrap_option_or_return!(CORE_SIGNED_TRANSACTION_V2_STORAGE | ||
| .with_item(transaction_handle, |tx| dashcore::consensus::serialize( | ||
| tx.transaction.transaction() | ||
| ))); | ||
| let len = bytes.len(); | ||
| let boxed = bytes.into_boxed_slice(); | ||
| *out_bytes = Box::into_raw(boxed) as *mut u8; | ||
| *out_len = len; | ||
| PlatformWalletFFIResult::ok() |
There was a problem hiding this comment.
💬 Nitpick: Declare ownership of serialized byte buffers in the C ABI
core_wallet_signed_transaction_v2_bytes returns an allocation created with Box::into_raw, but its cbindgen-visible documentation does not state that the caller takes ownership or must release the pointer using platform_wallet_bytes_free with the returned length. The current Swift wrapper correctly copies and frees it, but direct C and future binding consumers cannot infer the allocator/deallocator contract and may leak the buffer or incorrectly call the platform allocator's free().
| #[no_mangle] | |
| pub unsafe extern "C" fn core_wallet_signed_transaction_v2_bytes( | |
| transaction_handle: Handle, | |
| out_bytes: *mut *mut u8, | |
| out_len: *mut usize, | |
| ) -> PlatformWalletFFIResult { | |
| check_ptr!(out_bytes); | |
| check_ptr!(out_len); | |
| *out_bytes = std::ptr::null_mut(); | |
| *out_len = 0; | |
| let bytes = unwrap_option_or_return!(CORE_SIGNED_TRANSACTION_V2_STORAGE | |
| .with_item(transaction_handle, |tx| dashcore::consensus::serialize( | |
| tx.transaction.transaction() | |
| ))); | |
| let len = bytes.len(); | |
| let boxed = bytes.into_boxed_slice(); | |
| *out_bytes = Box::into_raw(boxed) as *mut u8; | |
| *out_len = len; | |
| PlatformWalletFFIResult::ok() | |
| /// Copy the consensus-serialized finalized transaction into a newly allocated buffer. | |
| /// | |
| /// On success, `out_bytes` receives a Rust-allocated buffer of `out_len` bytes. | |
| /// The caller takes ownership and must release it exactly once with | |
| /// `platform_wallet_bytes_free(*out_bytes, *out_len)`. Errors leave the outputs | |
| /// initialized to null and zero. | |
| /// | |
| /// # Safety | |
| /// `out_bytes` and `out_len` must be valid writable pointers. | |
| #[no_mangle] | |
| pub unsafe extern "C" fn core_wallet_signed_transaction_v2_bytes( | |
| transaction_handle: Handle, | |
| out_bytes: *mut *mut u8, | |
| out_len: *mut usize, | |
| ) -> PlatformWalletFFIResult { |
source: ['codex']
Points the eight workspace pins at dca5b05b (rust-dashcore's merged tx-builder OP_RETURN/output-order/change-routing support), replacing the local patch override the CI guard forbids committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation Adds an integration test for the shape SpvLateWalletBackfillIntegrationTests does not reach: many transactions across many addresses, on a wallet imported into an SPV client that is already running. The funding straddles the registration so each recovery path is exercised by a half the other path provably cannot reach. The pre-registration half is mined and fully scanned while no wallet exists — script matching runs against the registered wallets' scripts, so nothing could have matched it then, and it can only return through the genesis rescan that registering a birthHeight-0 wallet starts. The post-registration half is mined after createWallet returns, above the ceiling that rescan swept, so only live filter matching can find it. Neither half depends on timing: the test reaches the tip before it registers, and registers before it funds again. Balance and persisted per-transaction history are both asserted, once after the rescan and once after the live half, so a failure names the path that broke. They are separate write paths, so a regression that restores the right total while dropping individual PersistentTransaction rows would otherwise pass. Funding goes through env.fund for the masternode broadcast and InstantSend-lock wait the rest of the suite relies on, and the FFIWallet that wallet_manager_get_wallet boxes fresh on every call is freed with wallet_free_const instead of leaking. Also marks TestWalletWrapper @unchecked Sendable, as IntegrationTestEnv already is. That is unrelated to the new test but blocks it: the Swift SDK job is path-filtered to packages/swift-sdk, and MayaDepositVerificationIntegrationTests (added in #4286) is the first file to call env.makeTestWallet across an isolation boundary, so the seven resulting errors have sat unbuilt on v4.2-dev since — this is the next PR to touch the package and trigger the job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 participants
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