feat(kotlin-sdk): expose the drain strategy and the amount it delivers - #4324
feat(kotlin-sdk): expose the drain strategy and the amount it delivers#4324HashEngineering wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe signed-payment flow now reports ChangesSigned payment drain support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PaymentCaller
participant ManagedPlatformWallet
participant WalletManager
participant SignedPaymentFinalizer
participant RegistrationBlob
PaymentCaller->>ManagedPlatformWallet: buildSignedPayment(selectionStrategy)
ManagedPlatformWallet->>WalletManager: submit configured outputs
WalletManager->>SignedPaymentFinalizer: finalize with deliverable output pointer
SignedPaymentFinalizer-->>WalletManager: return deliverableDuffs
WalletManager-->>RegistrationBlob: encode deliverableDuffs
RegistrationBlob-->>ManagedPlatformWallet: decode deliverableDuffs
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 db6816b) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt (1)
226-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse two-space indentation in the Kotlin additions.
Reformat the changed Kotlin ranges to use two spaces for each nesting level.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt#L226-L243: re-indent the new property documentation and declaration.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt#L272-L340: re-indent the parser and helper functions.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt#L428-L506: re-indent the API documentation and drain wiring.packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.kt#L76-L184: re-indent the helpers and new tests.As per coding guidelines, “Follow repository EditorConfig settings: 2-space indentation.”
🤖 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/ManagedPlatformWallet.kt` around lines 226 - 243, Re-indent all changed Kotlin code to use two spaces per nesting level: the deliverableAmountDuffs documentation and declaration in ManagedPlatformWallet.kt lines 226-243, the parser and helper functions in lines 272-340, the API documentation and drain wiring in lines 428-506, and the helpers and new tests in SignedCoreTransactionTest.kt lines 76-184. Preserve the existing code and behavior.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
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 469-472: Update the recipient amount validation in
ManagedPlatformWallet so SelectionStrategy.ALL permits drain amounts only when
amount >= 0, while all other strategies continue requiring amount > 0. Preserve
the existing validation error behavior and message.
- Around line 272-340: Move transaction decoding and protocol constants out of
Kotlin: replace ManagedPlatformWallet.parseSoleDeliverableValue with a thin
JNI/FFI mapping that retrieves the deliverable amount derived by Rust, leaving
Kotlin only to store and expose that value. In
packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.kt
lines 85-124, remove consensus-byte vectors and serialization/parser tests and
retain only tests covering FFI field mapping; implement the protocol
serialization and decoding coverage in Rust.
- Around line 303-318: Update the transaction output parsing block around the
repeat loop to validate the compact-size script length against the remaining
buffer before allocating the ByteArray, rejecting invalid lengths with
IllegalStateException. Ensure parsing consumes and validates the required
four-byte locktime after all outputs before returning the deliverable value,
including truncated-script cases.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 226-243: Re-indent all changed Kotlin code to use two spaces per
nesting level: the deliverableAmountDuffs documentation and declaration in
ManagedPlatformWallet.kt lines 226-243, the parser and helper functions in lines
272-340, the API documentation and drain wiring in lines 428-506, and the
helpers and new tests in SignedCoreTransactionTest.kt lines 76-184. Preserve the
existing code and behavior.
🪄 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: 1b604aa2-9483-4dd4-b84c-9969f0eacef8
📒 Files selected for processing (3)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.ktpackages/rs-unified-sdk-jni/src/wallet_manager.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The drain API is not merge-ready: the pinned key-wallet revision rejects the destination-plus-OP_RETURN shape that this PR advertises, and the reported amount is derived lazily from mutable public bytes rather than the registered Rust transaction that will be broadcast. The Kotlin consensus parser also violates the package's Rust-first architecture rule and has malformed-length handling gaps.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
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) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 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/ManagedPlatformWallet.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:494-506: Memo-bearing drains are rejected by the pinned engine
This builds a destination output plus a zero-value OP_RETURN and then selects ALL, but Cargo.toml and Cargo.lock still pin rust-dashcore to dca5b05b83a9a2d6fd7b650e7cce4356d1c2ec29. At that exact revision, key-wallet's assemble_unsigned uses `let [out] = tx_outputs.as_mut_slice()` and returns `SelectionStrategy::All requires exactly one output (the destination)` whenever the memo creates a second output. rust-dashcore PR #928 changes this block to count exactly one non-OP_RETURN value carrier while allowing zero-value OP_RETURN outputs, but this PR changes only the Kotlin/JNI files and does not update the dependency. Therefore the documented MAYACHAIN drain path always fails; pin a revision containing #928 or include equivalent engine support before exposing this behavior.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:237-243: Derive the deliverable amount from the registered Rust transaction
`deliverableAmountDuffs` is parsed lazily from the publicly exposed mutable `rawTxBytes`, while Rust's signed-payment registry stores an independent `Transaction` and `broadcastSigned(payment)` broadcasts that registered transaction using only the token. If any caller or library mutates the byte array before first reading this property, the quoted value can differ from the output that is actually broadcast, violating the PR's quote/payment consistency guarantee. The local parser also reimplements OP_RETURN, CompactSize, and consensus transaction layout in Kotlin despite packages/kotlin-sdk/CLAUDE.md explicitly limiting this layer to thin JNI wrappers and forbidding protocol constants. Derive the sole deliverable output amount from the Rust transaction during finalization/registration and return that immutable value in the registration result instead of reparsing mutable Kotlin bytes.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:303-318: Malformed script lengths can crash or bypass truncation checks
The parser allocates `ByteArray(scriptLen)` before checking whether that many bytes remain, so a malformed CompactSize value such as Int.MAX_VALUE can raise OutOfMemoryError instead of the documented IllegalStateException. It also returns immediately after the outputs without consuming the required four-byte locktime; a script length enlarged to absorb those trailing bytes can therefore be accepted as structurally valid. Validate lengths before allocation, skip the script in place, and require the locktime before returning. Moving the entire derivation to Rust as requested above also removes this parser from Kotlin.
| if (changeToFirstInput) { | ||
| builder.changeToFirstInput() | ||
| } | ||
| // Set LAST so it applies to the fully-composed output set: a | ||
| // drain (SelectionStrategy.ALL) requires exactly one | ||
| // value-carrying output, and the engine rejects the build here | ||
| // — before anything is reserved — if the OP_RETURN above | ||
| // carries a value or a second spendable output was added. | ||
| selectionStrategy?.let { builder.setSelectionStrategy(it) } |
There was a problem hiding this comment.
🔴 Blocking: Memo-bearing drains are rejected by the pinned engine
This builds a destination output plus a zero-value OP_RETURN and then selects ALL, but Cargo.toml and Cargo.lock still pin rust-dashcore to dca5b05b83a9a2d6fd7b650e7cce4356d1c2ec29. At that exact revision, key-wallet's assemble_unsigned uses let [out] = tx_outputs.as_mut_slice() and returns SelectionStrategy::All requires exactly one output (the destination) whenever the memo creates a second output. rust-dashcore PR #928 changes this block to count exactly one non-OP_RETURN value carrier while allowing zero-value OP_RETURN outputs, but this PR changes only the Kotlin/JNI files and does not update the dependency. Therefore the documented MAYACHAIN drain path always fails; pin a revision containing #928 or include equivalent engine support before exposing this behavior.
source: ['codex']
| * Derived from [rawTxBytes] (already present — no extra native call). | ||
| * Throws [IllegalStateException] if the bytes are malformed or hold | ||
| * anything other than exactly one non-OP_RETURN output; under a drain | ||
| * the engine guarantees exactly one, and a plain multi-recipient | ||
| * payment has no single "deliverable" amount to report. | ||
| */ | ||
| val deliverableAmountDuffs: Long by lazy { parseSoleDeliverableValue(rawTxBytes) } |
There was a problem hiding this comment.
🔴 Blocking: Derive the deliverable amount from the registered Rust transaction
deliverableAmountDuffs is parsed lazily from the publicly exposed mutable rawTxBytes, while Rust's signed-payment registry stores an independent Transaction and broadcastSigned(payment) broadcasts that registered transaction using only the token. If any caller or library mutates the byte array before first reading this property, the quoted value can differ from the output that is actually broadcast, violating the PR's quote/payment consistency guarantee. The local parser also reimplements OP_RETURN, CompactSize, and consensus transaction layout in Kotlin despite packages/kotlin-sdk/CLAUDE.md explicitly limiting this layer to thin JNI wrappers and forbidding protocol constants. Derive the sole deliverable output amount from the Rust transaction during finalization/registration and return that immutable value in the registration result instead of reparsing mutable Kotlin bytes.
source: ['codex', 'coderabbit']
There was a problem hiding this comment.
Resolved in 6100a75 — Derive the deliverable amount from the registered Rust transaction 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.
| repeat(readVarInt(buf).toIntExact("output count")) { | ||
| val outValue = buf.long | ||
| val script = ByteArray(readVarInt(buf).toIntExact("scriptPubKey length")) | ||
| buf.get(script) | ||
| if (script.isEmpty() || script[0] != OP_RETURN) { | ||
| check(value == null) { | ||
| "transaction has more than one non-OP_RETURN output; " + | ||
| "deliverableAmountDuffs is defined only for a single-" + | ||
| "destination payment (a drain always builds one)" | ||
| } | ||
| value = outValue | ||
| } | ||
| } | ||
| return checkNotNull(value) { | ||
| "transaction has no non-OP_RETURN output to deliver to" | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Malformed script lengths can crash or bypass truncation checks
The parser allocates ByteArray(scriptLen) before checking whether that many bytes remain, so a malformed CompactSize value such as Int.MAX_VALUE can raise OutOfMemoryError instead of the documented IllegalStateException. It also returns immediately after the outputs without consuming the required four-byte locktime; a script length enlarged to absorb those trailing bytes can therefore be accepted as structurally valid. Validate lengths before allocation, skip the script in place, and require the locktime before returning. Moving the entire derivation to Rust as requested above also removes this parser from Kotlin.
| repeat(readVarInt(buf).toIntExact("output count")) { | |
| val outValue = buf.long | |
| val script = ByteArray(readVarInt(buf).toIntExact("scriptPubKey length")) | |
| buf.get(script) | |
| if (script.isEmpty() || script[0] != OP_RETURN) { | |
| check(value == null) { | |
| "transaction has more than one non-OP_RETURN output; " + | |
| "deliverableAmountDuffs is defined only for a single-" + | |
| "destination payment (a drain always builds one)" | |
| } | |
| value = outValue | |
| } | |
| } | |
| return checkNotNull(value) { | |
| "transaction has no non-OP_RETURN output to deliver to" | |
| } | |
| repeat(readVarInt(buf).toIntExact("output count")) { | |
| val outValue = buf.long | |
| val scriptLen = readVarInt(buf).toIntExact("scriptPubKey length") | |
| if (scriptLen > buf.remaining()) { | |
| throw IllegalStateException("malformed signed transaction bytes") | |
| } | |
| val isOpReturn = | |
| scriptLen > 0 && buf.get(buf.position()) == OP_RETURN | |
| buf.position(buf.position() + scriptLen) | |
| if (!isOpReturn) { | |
| check(value == null) { | |
| "transaction has more than one non-OP_RETURN output; " + | |
| "deliverableAmountDuffs is defined only for a single-" + | |
| "destination payment (a drain always builds one)" | |
| } | |
| value = outValue | |
| } | |
| } | |
| buf.int // Require the consensus locktime. | |
| return checkNotNull(value) { | |
| "transaction has no non-OP_RETURN output to deliver to" | |
| } |
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in 6100a75 — Malformed script lengths can crash or bypass truncation checks 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Line 361: Move the assignment to out_deliverable_duffs after the
SIGNED_PAYMENT_REGISTRY.register call has completed successfully, keeping its
initialized zero value unchanged on every error path. Update the surrounding
transaction-building flow so the nonzero deliverable amount is published only
for successfully registered transactions.
- Line 234: Update the Swift finalizer invocation at
CoreTransactionBuilder.swift:454 to pass all required arguments, adding a UInt64
output variable for out_deliverable_duffs as the final pointer argument. Update
the generated binding to match if the binding declaration also omits this
parameter.
🪄 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: 63e2e38a-bea5-41d0-997f-7baf71a8b9b0
📒 Files selected for processing (4)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.ktpackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
|
Pushed Deliverable amount now comes from the registered transaction@thepastaclaw was right that this is a correctness problem and not only an architecture one. It is now computed in Rust from the finalized transaction, before
That also resolves the Rust-first rule in The six parser tests are replaced by three blob tests, one of which fills Swift does not call this entry point, so there are no Swift surface changes. Verified on this branch against its own pin: On the zero-amount recipient checkLeaving this as is, for the reason given in the thread: the JNI rejects negative amounts before the conversion to First blocker: the rust-dashcore pinThis one cannot be fixed here. The bindings compile fine on that pin, because So this is ordering, not a defect. Suggest holding this as a draft until #928 lands and the pin is bumped. Happy to follow whatever order the maintainers prefer. |
|
Thanks — The current
Until those first two steps land, #4324 should remain blocked from merge (draft is fine if you want the UI to reflect that). I'll keep the first review thread open as the dependency gate; no need to vendor or duplicate #928 in this PR. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: The memo-bearing drain remains blocked because Platform still pins the engine revision that rejects a destination plus OP_RETURN under SelectionStrategy::All. New findings in latest delta: The added C FFI out-parameter leaves the Swift caller one argument short, and the authoritative Rust-side deliverable calculation lacks direct automated coverage; the two earlier Kotlin parsing findings are fixed. 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)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 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 `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:445-466: New finalizer argument breaks the Swift FFI caller
The cbindgen-generated declaration of `core_wallet_signed_payment_finalize` now has twelve arguments, ending with `uint64_t *out_deliverable_duffs`, while this Swift call still passes the previous eleven. `packages/swift-sdk/build_ios.sh` builds the native library, packages the generated `platform-wallet-ffi` header into `DashSDKFFI`, and then builds the Swift application, so the Swift source cannot compile against the current header. Update the Swift caller with a writable `UInt64` output, or preserve the original C symbol and add a new/versioned finalizer for consumers that need the additional result. Preserving or versioning the old symbol is also necessary if separately compiled consumers are expected to remain ABI-compatible.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:350-359: Test the Rust-side deliverable calculation
This filter-and-match is now the authoritative calculation used for the amount a host quotes, but no Rust test executes it. The Kotlin tests construct a registration blob with a caller-selected scalar, so they cannot detect regressions in OP_RETURN classification, output-order independence, or the zero result for ambiguous output sets. Extract the classification into a small helper and test a destination beside an OP_RETURN in both orders, two non-OP_RETURN outputs, and an OP_RETURN-only transaction.
| let deliverable_duffs = { | ||
| let mut carriers = finalized | ||
| .transaction() | ||
| .output | ||
| .iter() | ||
| .filter(|out| !out.script_pubkey.is_op_return()); | ||
| match (carriers.next(), carriers.next()) { | ||
| (Some(only), None) => only.value, | ||
| _ => 0, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test the Rust-side deliverable calculation
This filter-and-match is now the authoritative calculation used for the amount a host quotes, but no Rust test executes it. The Kotlin tests construct a registration blob with a caller-selected scalar, so they cannot detect regressions in OP_RETURN classification, output-order independence, or the zero result for ambiguous output sets. Extract the classification into a small helper and test a destination beside an OP_RETURN in both orders, two non-OP_RETURN outputs, and an OP_RETURN-only transaction.
source: ['codex']
There was a problem hiding this comment.
Resolved in 9c85e01 — Test the Rust-side deliverable calculation 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 note on dashpay#4324: this filter-and-match is the authoritative calculation behind the amount a host quotes, and no Rust test exercised it. The Kotlin tests feed the registration blob a caller-chosen scalar, so they prove the value survives the wire but cannot catch a regression in OP_RETURN classification, output ordering, or the zero-for-ambiguous result. Extract it as `sole_deliverable_value(&[TxOut]) -> u64` and test the cases that matter: a lone destination; a destination beside a data carrier in BOTH orders (Maya puts its memo at VOUT1, and nothing here may depend on that); two spendable outputs, alone and beside a carrier; an OP_RETURN-only build and an empty output set; and a value-bearing OP_RETURN, which reports 0 because an asset lock's burn is not a payee a host would quote. 258 platform-wallet-ffi tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs (1)
360-368: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUpdate the
rust-dashcorepin before merging.
Cargo.tomlandCargo.lockcurrently pin commitdca5b05b83a9a2d6fd7b650e7cce4356d1c2ec29, which predatesdashpay/rust-dashcore#928. After#928merges, update all workspace pins and lockfile entries to the merged commit, then rebase and rerun drain validation.🤖 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 360 - 368, Update every workspace rust-dashcore dependency pin and corresponding Cargo.lock source/checksum entries from commit dca5b05b83a9a2d6fd7b650e7cce4356d1c2ec29 to the commit containing dashpay/rust-dashcore#928. Rebase onto the updated dependency state and rerun the drain validation before merging.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 360-368: Update every workspace rust-dashcore dependency pin and
corresponding Cargo.lock source/checksum entries from commit
dca5b05b83a9a2d6fd7b650e7cce4356d1c2ec29 to the commit containing
dashpay/rust-dashcore#928. Rebase onto the updated dependency state and rerun
the drain validation before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76b44693-a42c-4c53-a613-20ab82598131
📒 Files selected for processing (1)
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Rust-side deliverable calculation is now authoritative and directly tested, resolving the prior coverage concern. Two blockers remain: the pinned key-wallet revision rejects the destination-plus-memo drain this PR exposes, and the changed C finalizer signature leaves the existing Swift caller uncompilable while also changing the ABI for previously compiled consumers.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (ffi-engineer); final verifier backend: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
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 - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
1 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 `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:454-466: New finalizer argument breaks the Swift FFI caller
`core_wallet_signed_payment_finalize` now requires twelve arguments, ending with `out_deliverable_duffs: *mut u64`, while this Swift invocation still supplies the previous eleven. `packages/swift-sdk/build_ios.sh` packages the cbindgen-generated `platform-wallet-ffi` header in `DashSDKFFI`, so Swift compilation against the current declaration fails here. Add writable `UInt64` storage and pass its address if changing the API is acceptable, or preserve the original C symbol and add a new/versioned finalizer. The latter is required if separately compiled consumers must remain ABI-compatible, because an old eleven-argument caller invoking the changed symbol does not provide a valid output pointer.
A max swap deposit is a drain: spend every UTXO in the funding account and pay the destination (total inputs - fee), with the memo as a zero-value OP_RETURN beside it. key-wallet can build that, but nothing above it could ask for one, and nothing could report what it would pay. - buildSignedPayment gains `selectionStrategy` (null = the builder's default), applied after the outputs so a drain's "exactly one value carrier" check runs against the finished output set, before anything is reserved. - SignedCoreTransaction gains `deliverableAmountDuffs`: the value of the sole non-OP_RETURN output, parsed from the signed bytes already present (no ABI change, no extra native call). Under a drain the ENGINE computes that figure, so this is the only way a caller can learn what the transaction will pay — a swap must quote from it and then broadcast THIS transaction, so quote and payment cannot disagree. - The positive-amount rule now yields to a drain in both layers that enforced it. `buildSignedPayment` knows whether the caller is draining and keeps the rule for every other build; the JNI boundary does not know, so it no longer duplicates a check it cannot qualify and refuses only negatives (a negative jlong would bit-cast to a huge u64). Rejecting 0 made "send my whole balance" inexpressible and forced callers to invent a placeholder the engine then discarded. Tests: 288 pass in :sdk, including six covering the parse — the vault output beside a memo, order independence, multiple inputs with realistic scriptSigs, and refusals for two spendable outputs, OP_RETURN-only, and malformed bytes. Verified on-device (testnet, emulator): a max Maya deposit measures 27442985 duffs with a real 432-duff fee for an 80-byte memo, identical whether the caller passes 0 or a placeholder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review blocker: the drain's deliverable amount was parsed in Kotlin from `rawTxBytes`. Those bytes are a mutable copy the host owns, while a broadcast sends the REGISTERED transaction the reservation token refers to -- so the quote could report a value the payment does not pay. It also put consensus parsing in Kotlin, against the Rust-first rule in packages/kotlin-sdk/CLAUDE.md, with hand-rolled varint reads that had to defend against malformed lengths. Compute it in Rust instead, from the finalized transaction, before `register` consumes it: the sole non-OP_RETURN output's value, or 0 when there is no single such output (multi-recipient or OP_RETURN-only) -- which hosts read as "not applicable", not "pays nothing". - platform-wallet-ffi: new `out_deliverable_duffs` out param on `core_wallet_signed_payment_finalize`, null-checked and sentinel-zeroed with the others before any fallible step. - rs-unified-sdk-jni: carry it in the registration blob after the fee. - kotlin-sdk: `deliverableAmountDuffs` becomes a constructor property fed from the blob; delete the Kotlin parser (parseSoleDeliverableValue, readVarInt, toIntExact, the OP_RETURN constant). - tests: replace the six parser tests with three blob tests, including one proving that mutating rawTxBytes cannot change the reported amount. Swift does not call this entry point, so no Swift surface changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review note on dashpay#4324: this filter-and-match is the authoritative calculation behind the amount a host quotes, and no Rust test exercised it. The Kotlin tests feed the registration blob a caller-chosen scalar, so they prove the value survives the wire but cannot catch a regression in OP_RETURN classification, output ordering, or the zero-for-ambiguous result. Extract it as `sole_deliverable_value(&[TxOut]) -> u64` and test the cases that matter: a lone destination; a destination beside a data carrier in BOTH orders (Maya puts its memo at VOUT1, and nothing here may depend on that); two spendable outputs, alone and beside a carrier; an OP_RETURN-only build and an empty output set; and a value-bearing OP_RETURN, which reports 0 because an asset lock's burn is not a payee a host would quote. 258 platform-wallet-ffi tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9c85e01 to
db6816b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Rust-side deliverable calculation is authoritative, allocation-free, and directly tested across the relevant output classifications. Two blocking integration issues remain: the pinned key-wallet still rejects memo-bearing drains, and the changed C finalizer signature breaks the checked-in Swift caller as well as the prior binary ABI. Source: reviewer backends: codex/general=gpt-5.6-sol, codex/ffi-engineer=gpt-5.6-sol, codex/rust-quality=gpt-5.6-sol; final verifier backend: codex/verifier=gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
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),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
1 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 `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:377-389: New finalizer argument breaks the Swift FFI caller
`core_wallet_signed_payment_finalize` now has twelve parameters, ending with `out_deliverable_duffs: *mut u64`, while this checked-in Swift invocation still passes the previous eleven. `packages/swift-sdk/build_ios.sh` includes `platform-wallet-ffi` among the cbindgen-generated headers packaged into `DashSDKFFI`, so Swift compilation against the newly generated declaration fails here. Changing the existing exported symbol in place also breaks previously compiled eleven-argument consumers: the Rust implementation reads and writes through a final pointer that those callers never supplied. Update every source caller and explicitly version the binary interface, or preserve the original eleven-argument symbol as a wrapper and add an additive/versioned finalizer that returns the deliverable amount.
2 participants
Problem
A max swap deposit is a drain: spend every UTXO in the funding account and pay the destination
total inputs − fee, with the memo as a zero-value OP_RETURN beside it.key-walletcan build that, but nothing above it could ask for one, and nothing could report what it would pay.So a host had to derive the amount itself — read a balance, subtract an estimated fee and a change headroom, and quote the result. That estimate is made from a different pool than the build funds from, so it disagrees with reality exactly when a wallet holds funds outside the funding account, and it silently under-quotes even when it works.
Change
buildSignedPaymentgainsselectionStrategy(null = the builder's default). It is applied after the outputs, so a drain's "exactly one value carrier" check runs against the finished output set — before anything is reserved.SignedCoreTransactiongainsdeliverableAmountDuffs: the value of the sole non-OP_RETURN output, parsed from the signed bytes already present. No ABI change, no extra native call. Under a drain the engine computes that figure, so this is the only way a caller can learn what the transaction will pay — a swap must quote from it and then broadcast that transaction, so quote and payment cannot disagree.The positive-amount rule now yields to a drain, in both layers that enforced it:
buildSignedPaymentknows whether the caller is draining, and keeps the rule for every other build.jlongwould bit-cast to a hugeu64).Rejecting
0made "send my whole balance" inexpressible and forced callers to invent a placeholder the engine then discarded.Tests
Re-run after rebasing onto the current
v4.2-dev(the one conflict was an import line: the base had droppedTransactionandStandardAccountType, and this branch only needs to addTxOut).platform-wallet-ffi: 244 pass, including six new ones oversole_deliverable_value, the classification the reported amount comes from — a lone destination; a destination beside a data carrier in both orders (a MAYAChain deposit carries its memo at VOUT1, and nothing here may depend on that); two spendable outputs, alone and beside a carrier; an OP_RETURN-only build and an empty output set; and a value-bearing OP_RETURN, which reports 0 because an asset lock's burn is not a payee a host would quote.:sdk: 604 pass, 0 failures, includingSignedCoreTransactionTest— the amount comes from the registration blob, mutatingrawTxBytescannot change it, and it is 0 when the engine reports no single destination.cargo check -p rs-unified-sdk-jniclean.Run against this branch's actual base, not only the integration branch it was developed on. Worth stating explicitly because fork PRs skip the Rust suite in CI.
Verification
Exercised end to end on testnet through dash-wallet, against an AAR built from this branch plus
rust-dashcore#928.deliverableAmountDuffsand VOUT0 of the signed bytes agreed to the duff. Broadcast and accepted.0or a placeholder, confirming the engine discards the caller's amount on a drain.The first run also demonstrated why the amount has to come from the engine rather than the host's own arithmetic. The wallet was still verifying the deposit against its quote, which the drain legitimately exceeded by 9 duffs once the balance moved between quote and build, and it aborted a sound deposit. Verifying against
deliverableAmountDuffsfixed it. That was a host-side bug, not one in this PR, but it is the exact failure this API exists to prevent.Notes for reviewers
Needs
dashpay/rust-dashcore#928in the pinned engine. That PR lets a drain carry a zero-value OP_RETURN; without it, this parameter is reachable from Kotlin but the build is rejected underneath.v4.2-devcurrently pins a rev that predates it, so this cannot function until that pin moves — the merge itself is unaffected, sinceSelectionStrategy::Allalready exists at the pinned rev.One consequence worth flagging: because the JNI check moved up a layer, an older Kotlin layer paired with this native library would let a genuine zero-amount output through to a non-drain build. The two halves should ship together.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes