feat(kotlin-sdk): bind OP_RETURN, output-order and VIN0-change builder controls - #4288
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.
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.
…r controls Kotlin/Android parity for the Swift-only surface dashpay#4286 added: four thin JNI trampolines over core_wallet_tx_builder_add_op_return / _preserve_output_order / _change_to_first_input and core_wallet_signed_transaction_v2_bytes, the matching WalletManagerNative declarations, CoreTransactionBuilder.addOpReturn/preserveOutputOrder/ changeToFirstInput, and FinalizedCoreTransaction.serializedData() — a non-consuming read so callers can assert the MAYACHAIN deposit shape (vault VOUT0, memo VOUT1, change VOUT2) before broadcasting. Instrumented binding test needs no funded wallet: symbols resolve, the Maya option sequence succeeds, an 81-byte memo throws while the builder survives, and the bytes reader rejects a null handle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesThe PR adds Kotlin and JNI support for MAYACHAIN payment construction. It adds OP_RETURN data, output-order preservation, first-input change routing, and non-consuming finalized transaction serialization. Android tests and parity metadata cover the new behavior. MAYACHAIN transaction-builder support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformWallet
participant CoreTransactionBuilder
participant WalletManagerNative
participant JNIWalletManager
participant PlatformWalletFFI
ManagedPlatformWallet->>CoreTransactionBuilder: configure recipients and MAYACHAIN options
CoreTransactionBuilder->>WalletManagerNative: invoke native builder controls
WalletManagerNative->>JNIWalletManager: call JNI methods
JNIWalletManager->>PlatformWalletFFI: apply OP_RETURN and output-shape controls
PlatformWalletFFI-->>JNIWalletManager: return result or error
JNIWalletManager-->>CoreTransactionBuilder: return result or DashSDKException
CoreTransactionBuilder-->>ManagedPlatformWallet: finalize signed payment
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🕓 Ready for review — next in queue (commit 3af7eb5) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The JNI bindings and finalized-transaction byte ownership are implemented consistently with the existing FFI patterns. However, the new MAYACHAIN controls are inaccessible to consumers of the published Kotlin SDK, so the feature cannot be used through the supported public API.
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. 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
🤖 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 `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt:71-95: Expose the MAYACHAIN controls through a public atomic API
All three new builder controls are `internal`, as are the builder constructor, `addOutput`, and `finalizeAtomic`; the underlying `WalletManagerNative` object is internal as well. The only public driver, `ManagedPlatformWallet.sendToAddresses`, accepts ordinary positive-value address outputs, applies none of these controls, immediately broadcasts, and never returns a `FinalizedCoreTransaction`. Consequently, an application consuming the published `dash-sdk-android` artifact cannot add the OP_RETURN memo, preserve the required output order, route change to VIN0, or obtain a finalized transaction to inspect with `serializedData()`. The instrumented test only exercises the internal native surface from within the SDK module, so it does not verify consumer accessibility. Add a public atomic prepare/send API that applies these options and returns a `FinalizedCoreTransaction` for inspection while keeping the deprecated split `setFunding`/`buildSigned` path inaccessible.
| internal fun addOpReturn(data: ByteArray): CoreTransactionBuilder = apply { | ||
| WalletManagerNative.coreTxBuilderAddOpReturn(handle, data) | ||
| } | ||
|
|
||
| /** Override the change address (network-checked Rust-side). */ | ||
| internal fun setChangeAddress(address: String): CoreTransactionBuilder = apply { | ||
| WalletManagerNative.coreTxBuilderSetChangeAddress(handle, address) | ||
| } | ||
|
|
||
| /** | ||
| * Preserve outputs in insertion order (skip BIP-69 sorting) for a | ||
| * MAYACHAIN-style deposit — vault must stay VOUT0, memo VOUT1 (mirror of | ||
| * Swift's `preserveOutputOrder`). | ||
| */ | ||
| internal fun preserveOutputOrder(): CoreTransactionBuilder = apply { | ||
| WalletManagerNative.coreTxBuilderPreserveOutputOrder(handle) | ||
| } | ||
|
|
||
| /** | ||
| * Route change to the first selected input's address (VIN0) for a | ||
| * MAYACHAIN-style deposit — MAYAChain identifies the depositor by VIN0 | ||
| * and pays refunds there (mirror of Swift's `changeToFirstInput`). | ||
| */ | ||
| internal fun changeToFirstInput(): CoreTransactionBuilder = apply { | ||
| WalletManagerNative.coreTxBuilderChangeToFirstInput(handle) |
There was a problem hiding this comment.
🔴 Blocking: Expose the MAYACHAIN controls through a public atomic API
All three new builder controls are internal, as are the builder constructor, addOutput, and finalizeAtomic; the underlying WalletManagerNative object is internal as well. The only public driver, ManagedPlatformWallet.sendToAddresses, accepts ordinary positive-value address outputs, applies none of these controls, immediately broadcasts, and never returns a FinalizedCoreTransaction. Consequently, an application consuming the published dash-sdk-android artifact cannot add the OP_RETURN memo, preserve the required output order, route change to VIN0, or obtain a finalized transaction to inspect with serializedData(). The instrumented test only exercises the internal native surface from within the SDK module, so it does not verify consumer accessibility. Add a public atomic prepare/send API that applies these options and returns a FinalizedCoreTransaction for inspection while keeping the deprecated split setFunding/buildSigned path inaccessible.
source: ['codex']
There was a problem hiding this comment.
Retracting this as a blocker after verifying the stacked integration context. This PR intentionally adds the JNI/Kotlin binding layer on top of #4286; the supported public call site is the option-carrying ManagedPlatformWallet.buildSignedPayment, whose reservation-token overload belongs to the separate #4185/#4247 deferred-payment stack. The downstream #1535 branch has already exercised that combined stack end to end on mainnet. Requiring a second public atomic API here would duplicate or preempt that stack rather than fix a defect in these bindings. The public entry-point follow-up remains a documented dependency before an integration AAR can ship.
|
Downstream consumer: dashpay/dash-wallet#1535 routes Maya/SwapKit swap deposits through these bindings and deletes the wallet's dashj transaction construction for that path. Mainnet-verified end to end with the full stack (rust-dashcore#922 + #4286 + this PR): tx One note for whoever cuts the next integration AAR: the wallet also needs an option-carrying Also worth recording: #4286's two commits are inseparable — the first alone references |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt (1)
64-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Swift source-file citations for all iOS-ported behavior.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt#L64-L96,L217-L227: add the applicable Swift source file paths to each KDoc block that identifies a Swift mirror.packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/wallet/CoreTxBuilderOpReturnBindingTest.kt#L12-L25: add the Swift source file path forMayaDepositVerificationIntegrationTests.As per coding guidelines, Kotlin behavior ported from iOS must cite the corresponding Swift source file in KDoc.
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt` around lines 64 - 96, Add citations to the corresponding Swift source files for every iOS-ported behavior in CoreTransactionBuilder.kt, including the KDoc blocks for addOpReturn, setChangeAddress, preserveOutputOrder, changeToFirstInput, and the additional cited block at L217-L227; update packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt lines 64-96 and 217-227. Also document the Swift source path for MayaDepositVerificationIntegrationTests in packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/wallet/CoreTxBuilderOpReturnBindingTest.kt lines 12-25.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.
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt`:
- Around line 64-96: Add citations to the corresponding Swift source files for
every iOS-ported behavior in CoreTransactionBuilder.kt, including the KDoc
blocks for addOpReturn, setChangeAddress, preserveOutputOrder,
changeToFirstInput, and the additional cited block at L217-L227; update
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt
lines 64-96 and 217-227. Also document the Swift source path for
MayaDepositVerificationIntegrationTests in
packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/wallet/CoreTxBuilderOpReturnBindingTest.kt
lines 12-25.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91de4dcd-af89-4212-a74c-899ea8f1edee
📒 Files selected for processing (4)
packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/wallet/CoreTxBuilderOpReturnBindingTest.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.ktpackages/rs-unified-sdk-jni/src/wallet_manager.rs
# Conflicts: # .github/workflows/tests-rs-workspace.yml
…SignedPayment The three builder controls, the builder itself, and WalletManagerNative are all internal, so a consumer of the published dash-sdk-android artifact could not add the OP_RETURN memo, preserve output order, route change to VIN0, or inspect the result before broadcast. Thread opReturnData / preserveOutputOrder / changeToFirstInput through the public ManagedPlatformWallet.buildSignedPayment: one atomic native select+reserve+sign+register returning a SignedCoreTransaction whose rawTxBytes lets the caller assert the deposit shape (vault VOUT0 / memo VOUT1 / change VOUT2) before broadcastSigned or releaseReservation. The deprecated setFunding/buildSigned split stays inaccessible. Instrumented coverage drives only the public overload: on an unfunded wallet the canonical Maya option set must reach atomic selection (CoreInsufficientFunds), an 81-byte memo must fail before selection, and the wallet must survive to run a well-formed build afterwards. Registers the core.maya_op_return_deposit parity capability over the four shared FFI symbols, verified by the gated Swift Maya integration suite and the Kotlin instrumented tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
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>
5 participants
Issue being fixed or feature implemented
Kotlin/Android parity for #4286. That PR exposes the MAYACHAIN-deposit
builder controls (
add_op_return,preserve_output_order,change_to_first_input, and the pre-broadcastcore_wallet_signed_transaction_v2_bytesreader) through the FFI and bindsthem for Swift only. The Android wallet is restoring the same MAYACHAIN swap
routes (dashpay/dash-wallet#1520, the Maya/SwapKit items), and its send path
runs on
dash-sdk-android— without JNI/Kotlin bindings the FFI surface isunreachable there.
What was done?
Stacked on #4286 (base =
feat/maya-op-return); the diff on top is only theKotlin-side plumbing:
packages/rs-unified-sdk-jni/src/wallet_manager.rs: four thin JNItrampolines following the module's one-export-one-FFI-call convention —
coreTxBuilderAddOpReturn,coreTxBuilderPreserveOutputOrder,coreTxBuilderChangeToFirstInput, andcoreSignedTransactionV2Bytes(copies the FFI-owned buffer into a
byte[]and frees it withplatform_wallet_bytes_freeon every path).packages/kotlin-sdk/.../ffi/WalletManagerNative.kt: the matchingexternal fundeclarations.packages/kotlin-sdk/.../wallet/CoreTransactionBuilder.kt:addOpReturn(ByteArray),preserveOutputOrder(),changeToFirstInput()mirroring the Swift
CoreTransactionBuildermethods, andFinalizedCoreTransaction.serializedData()mirroring Swift'sserializedData()— a non-consuming read so the caller can assert thedeposit shape (vault VOUT0 / memo VOUT1 / change VOUT2) before deciding to
broadcast or abandon.
packages/kotlin-sdk/.../androidTest/.../CoreTxBuilderOpReturnBindingTest.kt:instrumented binding coverage that needs no funded wallet — the four
symbols resolve, the canonical Maya option sequence succeeds on a live
builder, an 81-byte memo throws
DashSDKExceptionwhile the buildersurvives (the FFI rejects before consuming builder state), and the bytes
reader rejects a null handle instead of crashing. The full funded
deposit-shape assertion stays with feat(sdk): expose OP_RETURN, output-order and change-to-VIN0 controls #4286's gated Swift integration suite
and the wallet-side testnet verification.
Like #4286, building this locally needs the uncommitted
[patch."https://github.com/dashpay/rust-dashcore"]override untildashpay/rust-dashcore#922 merges and the workspace
revis bumped — the CIguard added there enforces that the override never lands.
How Has This Been Tested?
cargo check -p rs-unified-sdk-jniandcargo fmt --check/cargo clippy -p rs-unified-sdk-jniagainst chore(rs-dpp): fix build issues in rs-dpp #922's key-wallet commits(cherry-picked onto the pinned rev locally).
:sdk:compileDebugKotlin,:sdk:testDebugUnitTest,:sdk:compileDebugAndroidTestKotlin— green.:sdk:connectedDebugAndroidTeston a Pixel 8 API-34 arm64 emulator with alocally built
libdash_sdk_jni.so(dev profile, patched engine):4/4 tests pass — symbols resolve, options apply, the oversize-memo
rejection and null-handle rejection both surface as
DashSDKException.Breaking Changes
None — additive bindings only.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests