feat(kotlin-sdk): single-account build_signed_payment send API (funding_path) - #4247
feat(kotlin-sdk): single-account build_signed_payment send API (funding_path)#4247bfoss765 wants to merge 7 commits into
Conversation
Add CoreWallet::build_signed_payment — a first-class "send" primitive that
selects inputs across the UNION of every signable funds account (BIP44 +
BIP32 + CoinJoin + DashPay receiving), builds and signs a standard L1
payment, and returns the signed transaction plus its fee and change amount.
This is the build-only half of the Android send-path cutover: during the
dashj->SDK transition the app keeps dashj's transaction bookkeeping
(maybeCommitTx drives CrowdNode, memos, confidence listeners), so the SDK
must build + sign from the bound wallet and hand back the bytes while dashj
commits/broadcasts. The method therefore does NOT broadcast and does NOT
persist a debit — the only state touched is the in-memory ReservationSet
that set_funding/build_signed use to stop a concurrent SDK build from
re-selecting the same coins (released when the spend is later observed by
sync or by the reservation-TTL backstop).
Reuses the shielded asset-lock union-funding machinery
(all_funding_accounts + spendable_utxos + a spanning path resolver +
LargestFirst selection to keep CoinJoin's many small denominations from
blowing up BranchAndBound), but excludes watch-only DashpayExternalAccounts
(a contact's addresses, which this wallet cannot sign). Fee and change are
derived from the transaction itself (inputs - outputs), the always
self-consistent ground truth, rather than from build_signed's signed-size
fee recomputation which can drift by a few duffs from what is actually paid.
Adds the typed PaymentInsufficientFunds { available, required } error so a
shortfall carries the exact union-wide selectable total.
Tests: correct output/change/fee, BIP44+CoinJoin union selection, typed
union shortfall, watch-only exclusion, and input validation.
cargo test -p platform-wallet --lib: 442 passed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 38012d1)
Expose the union-funding send primitive as a public Kotlin suspend fun that returns the signed raw transaction bytes (plus fee + change) WITHOUT broadcasting — the last SDK gap for the Android wallet's send-path cutover. - FFI (rs-platform-wallet-ffi): core_wallet_build_signed_payment, a one-shot call over CoreWallet::build_signed_payment. Recipients cross as a big-endian blob (u32 count; per row u32 addrLen, addr utf8, u64 amount), each address parsed + network-checked; returns the consensus-serialized signed bytes via out-pointers plus out_fee/out_change, freed with core_wallet_free_payment_bytes. cbindgen exports both automatically. - JNI (rs-unified-sdk-jni): WalletManagerNative.coreWalletBuildSignedPayment returns a byte[] packed big-endian as (u64 fee, u64 change, tx bytes); the FFI-owned bytes are freed before returning. - Kotlin (kotlin-sdk): ManagedPlatformWallet.buildSignedPayment(recipients, coreSignerHandle, feePerKb) -> SignedCorePayment(txBytes, fee, change), serialized under the same shared per-wallet coreSendMutex as sendToAddresses so a concurrent build cannot select the same UTXO. Unlike sendToAddresses it neither broadcasts nor picks a funding account (coin selection auto-spans every signable account). ManagedCoreWallet gains the matching native call on the transient core handle. cargo check/test green across platform-wallet-ffi (149) and rs-unified-sdk-jni (2); :sdk:compileDebugKotlin BUILD SUCCESSFUL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit d18c9c0) [port to v4.2-dev] ManagedPlatformWallet.buildSignedPayment is serialized through the manager's TeardownGate (gate.op {}), matching sendToAddresses and every other native op on this branch, instead of the pre-refactor per-wallet `coreSendMutex` (which no longer exists on v4.2-dev). Concurrent builds still cannot select the same UTXO: CoreWallet::build_signed_payment holds the wallet-manager write lock across coin selection and signing.
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds a build-only Core L1 payment flow that selects one signable funding account, signs without broadcasting or debit persistence, and returns fee, change, and serialized transaction bytes through Rust FFI, JNI, and Kotlin APIs. It also exposes account derivation paths. ChangesCore signed payment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformWallet
participant ManagedCoreWallet
participant WalletManagerNative
participant JNI
participant CoreWalletFFI
participant CoreWallet
participant Signer
ManagedPlatformWallet->>ManagedCoreWallet: request signed payment
ManagedCoreWallet->>WalletManagerNative: pass outputs and funding path
WalletManagerNative->>JNI: invoke native API
JNI->>CoreWalletFFI: pass encoded outputs and handles
CoreWalletFFI->>CoreWallet: build and sign payment
CoreWallet->>Signer: sign selected inputs
Signer-->>CoreWallet: signed transaction
CoreWallet-->>CoreWalletFFI: transaction, fee, and change
CoreWalletFFI-->>JNI: serialized transaction bytes
JNI-->>ManagedPlatformWallet: packed fee, change, and transaction
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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/send.rs`:
- Around line 62-63: Validate the decoded count in the send transaction decoding
flow before the Vec::with_capacity(count) allocation. Use the remaining blob
length and the minimum encoded size of one output to reject impossible or
excessively large counts with the existing TransactionBuild decode error, then
allocate only after validation; preserve normal decoding for valid counts.
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Line 149: Update build_signed_payment to limit the wallet_manager write lock
to primary-account lookup, transaction input building, and reservation; clone
the primary account before releasing wm, then perform
builder.build_signed(signer, ...) after the lock is dropped. Match the
lock-lifetime split used by finalize_transaction so signing never holds the
global manager write lock.
🪄 Autofix (Beta)
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: 535b5447-abf3-4bea-99d0-99a1cbe6ebc6
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/send.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
|
🔍 Review in progress — actively reviewing now (commit 0dcdc74) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new payment API has six blocking defects: secondary-account inputs can be signed twice, malformed recipient metadata can abort the process, unchecked output and fee arithmetic can wrap, insufficient-funds errors lose their Kotlin type, and the added unit tests do not compile. The signing path also holds the wallet-manager write lock throughout repeated host-backed key derivation and signing, unnecessarily blocking wallet sync and access.
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— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 6 blocking | 🟡 1 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 `packages/rs-platform-wallet/src/wallet/core/send.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:237-238: Secondary-account inputs are not reserved in their owning accounts
`set_funding` filters against and captures only the primary BIP44 account's `ReservationSet`, while `add_inputs` appends UTXOs gathered from every other account without reservation filtering. `TransactionBuilder::assemble_unsigned` consequently records every selected outpoint in the primary set. A later union build re-adds the same secondary-account UTXO because `spendable_utxos` does not consult the primary set, and an account-specific build checks only the secondary account's own set. The manager lock serializes builds while each call is running but does not make the first build's secondary reservations visible after it returns, so sequential calls can return signed transactions that spend the same CoinJoin, BIP32, or DashPay UTXO. Implement wallet-wide or multi-account reservation tracking that filters and reserves every selected outpoint in the ledger consulted by all funding paths.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:329-331: The new payment tests reference a missing fixture
`split_funded_wallet_manager` is imported and used by the new union-funding tests, but it is not defined in `crate::test_support` or elsewhere at this head. Running `cargo test -p platform-wallet --lib --no-run` fails with E0432 at this import, followed by cascading type-inference errors, so the crate's unit-test target cannot compile. Add the omitted split-account fixture or rewrite these tests using an existing fixture.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:147: Unchecked output aggregation can build an invalid transaction
The requested output total is calculated with unchecked `u64` summation, and key-wallet independently performs the same unchecked sum while building the transaction. Kotlin can supply four positive amounts of `1L shl 62`; their mathematical total is `2^64`, which wraps to zero in release builds. Coin selection can then fund only the fee while retaining all four enormous outputs, returning a signed transaction that consensus and relay will reject and reporting meaningless fee/change metadata. Overflow-checking builds panic inside the C FFI path. Use checked aggregation and reject totals above Dash's `MAX_MONEY` before invoking the builder.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:154-155: Unbounded fee rates overflow key-wallet fee calculation
The public Kotlin and FFI APIs accept any non-negative fee rate, including values near `Long.MAX_VALUE`, and pass it directly to `FeeRate::new`. Key-wallet computes fees as `sat_per_kb * size_bytes` with unchecked `u64` multiplication. Large caller-supplied rates therefore panic in overflow-checking Android builds or wrap in release builds, potentially turning an astronomical requested rate into a tiny fee. Validate the rate against a monetary and transaction-size bound or change the fee calculation call chain to return an error from checked arithmetic.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:149: External signing holds the wallet-manager write lock
The write guard acquired here remains live through `build_signed(...).await`. A union-funded payment can select hundreds of inputs, and the production signer resolves and parses the mnemonic, derives the seed and BIP32 key, and invokes the host callback separately for each input. This exclusively blocks wallet sync and every other manager-backed operation for the entire signing phase. The existing `finalize_transaction` path reserves and snapshots the unsigned transaction, UTXOs, and paths under the lock, drops the guard, and signs afterward; this path should follow that model once reservations cover every funding account, releasing all reservations if signing fails.
In `packages/rs-platform-wallet-ffi/src/core_wallet/send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/send.rs:62-66: Untrusted recipient count can abort the process through an enormous allocation
The decoder passes the caller-controlled `u32` count directly to `Vec::with_capacity` before checking whether the blob contains that many rows. A four-byte blob declaring `u32::MAX` outputs can therefore request hundreds of gigabytes and enter Rust's process-aborting allocation-failure path instead of returning the documented decode error. On 32-bit targets, capacity or `cursor + addr_len` overflow can also panic inside the nested `extern "C"` function, where the JNI guard cannot safely recover it. Bound the count using the remaining blob length and the standard-transaction output limit, use `try_reserve_exact`, and use checked cursor arithmetic.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/error.rs:327-339: Payment insufficient-funds errors lose their Kotlin type
The new `PlatformWalletError::PaymentInsufficientFunds` variant has no conversion arm and falls through to `ErrorUnknown` (99). JNI consequently throws code 1099, which Kotlin maps to `DashSdkError.PlatformWallet.Generic`, instead of the existing `PlatformWallet.CoreInsufficientFunds` type mapped from FFI code 22. This erases the expected shortfall classification from the new API even though the Rust variant preserves `available` and `required` in its message. Map both Core and payment shortfalls to `ErrorCoreInsufficientFunds`, or introduce a dedicated ABI code and Kotlin type.
| .set_funding(primary_funds, &primary_account) | ||
| .add_inputs(extra_inputs); |
There was a problem hiding this comment.
🔴 Blocking: Secondary-account inputs are not reserved in their owning accounts
set_funding filters against and captures only the primary BIP44 account's ReservationSet, while add_inputs appends UTXOs gathered from every other account without reservation filtering. TransactionBuilder::assemble_unsigned consequently records every selected outpoint in the primary set. A later union build re-adds the same secondary-account UTXO because spendable_utxos does not consult the primary set, and an account-specific build checks only the secondary account's own set. The manager lock serializes builds while each call is running but does not make the first build's secondary reservations visible after it returns, so sequential calls can return signed transactions that spend the same CoinJoin, BIP32, or DashPay UTXO. Implement wallet-wide or multi-account reservation tracking that filters and reserves every selected outpoint in the ledger consulted by all funding paths.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Secondary-account inputs are not reserved in their owning accounts 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.
| use crate::test_support::{ | ||
| funded_wallet_manager, split_funded_wallet_manager, AlwaysRejectedBroadcaster, | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: The new payment tests reference a missing fixture
split_funded_wallet_manager is imported and used by the new union-funding tests, but it is not defined in crate::test_support or elsewhere at this head. Running cargo test -p platform-wallet --lib --no-run fails with E0432 at this import, followed by cascading type-inference errors, so the crate's unit-test target cannot compile. Add the omitted split-account fixture or rewrite these tests using an existing fixture.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — The new payment tests reference a missing fixture 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.
| let count = read_u32(blob, &mut cursor)? as usize; | ||
| let mut outputs = Vec::with_capacity(count); | ||
| for _ in 0..count { | ||
| let addr_len = read_u32(blob, &mut cursor)? as usize; | ||
| let end = cursor + addr_len; |
There was a problem hiding this comment.
🔴 Blocking: Untrusted recipient count can abort the process through an enormous allocation
The decoder passes the caller-controlled u32 count directly to Vec::with_capacity before checking whether the blob contains that many rows. A four-byte blob declaring u32::MAX outputs can therefore request hundreds of gigabytes and enter Rust's process-aborting allocation-failure path instead of returning the documented decode error. On 32-bit targets, capacity or cursor + addr_len overflow can also panic inside the nested extern "C" function, where the JNI guard cannot safely recover it. Bound the count using the remaining blob length and the standard-transaction output limit, use try_reserve_exact, and use checked cursor arithmetic.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Untrusted recipient count can abort the process through an enormous allocation 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.
| "every output amount must be greater than zero".to_string(), | ||
| )); | ||
| } | ||
| let outputs_total: u64 = outputs.iter().map(|(_, amount)| *amount).sum(); |
There was a problem hiding this comment.
🔴 Blocking: Unchecked output aggregation can build an invalid transaction
The requested output total is calculated with unchecked u64 summation, and key-wallet independently performs the same unchecked sum while building the transaction. Kotlin can supply four positive amounts of 1L shl 62; their mathematical total is 2^64, which wraps to zero in release builds. Coin selection can then fund only the fee while retaining all four enormous outputs, returning a signed transaction that consensus and relay will reject and reporting meaningless fee/change metadata. Overflow-checking builds panic inside the C FFI path. Use checked aggregation and reject totals above Dash's MAX_MONEY before invoking the builder.
| let outputs_total: u64 = outputs.iter().map(|(_, amount)| *amount).sum(); | |
| let outputs_total = outputs | |
| .iter() | |
| .try_fold(0u64, |total, (_, amount)| total.checked_add(*amount)) | |
| .ok_or_else(|| { | |
| PlatformWalletError::TransactionBuild("output amount total overflow".to_string()) | |
| })?; | |
| if outputs_total > dashcore::blockdata::constants::MAX_MONEY { | |
| return Err(PlatformWalletError::TransactionBuild( | |
| "output amount total exceeds MAX_MONEY".to_string(), | |
| )); | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Unchecked output aggregation can build an invalid 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.
| let height = info.core_wallet.last_processed_height(); | ||
| let fee_rate = FeeRate::new(fee_per_kb.unwrap_or(DEFAULT_FEE_PER_KB)); |
There was a problem hiding this comment.
🔴 Blocking: Unbounded fee rates overflow key-wallet fee calculation
The public Kotlin and FFI APIs accept any non-negative fee rate, including values near Long.MAX_VALUE, and pass it directly to FeeRate::new. Key-wallet computes fees as sat_per_kb * size_bytes with unchecked u64 multiplication. Large caller-supplied rates therefore panic in overflow-checking Android builds or wrap in release builds, potentially turning an astronomical requested rate into a tiny fee. Validate the rate against a monetary and transaction-size bound or change the fee calculation call chain to return an error from checked arithmetic.
source: ['codex']
There was a problem hiding this comment.
Fixed in 920b706 — and the finding was sharper than the bound that was already there.
MAX_FEE_PER_KB existed at fe72da65, but it was derived as MAX_MONEY / 100, which is only sound if the transaction stays under the 100 kB standard limit — and the method never enforced that limit. So the product still overflowed at ~878 kB, exactly as the sibling finding on #4256 works out. Two dimensions reach it: an oversized recipient list, and a funding account holding a few thousand small denominations (a realistic CoinJoin account) with no oversized list at all.
Three-part fix in wallet/core/send.rs:
MAX_STANDARD_TX_SIZEis now enforced, derived fromdashcore::policy::MAX_STANDARD_TX_WEIGHT / 4rather than hard-coded. The recipient count is bounded against it with checked arithmetic mirroring key-wallet's own base-size formula (outputs.len() * 34is an uncheckedusizemultiply inside key-wallet), requiring room for at least one 148-byte input.MAX_FEE_PER_KBis re-derived asu64::MAX / u32::MAX. That makessat_per_kb * size_bytesrepresentable for ANY size au32can express, so it does not depend on the input count — which is the part the size limit alone cannot cover. ~4.29e9 duffs/kB is ~43 DASH/kB, three orders of magnitude above the 1_000 default, so nothing legitimate is rejected.- the signed transaction is re-measured against the limit, since the input count is not knowable until selection has run.
Tests: excessive_fee_rates_are_rejected (u64::MAX, u64::MAX/2, bound+1, plus a sane rate that still works), max_fee_rate_cannot_overflow_key_wallets_fee_product (asserts no overflow at 100 kB, 878_434, and u32::MAX — it would fail if a cleanup restored MAX_MONEY / 100), and oversized_recipient_lists_are_rejected, which includes the 25,835-recipient case at the maximum accepted rate.
| } | ||
| let outputs_total: u64 = outputs.iter().map(|(_, amount)| *amount).sum(); | ||
|
|
||
| let mut wm = self.wallet_manager.write().await; |
There was a problem hiding this comment.
🟡 Suggestion: External signing holds the wallet-manager write lock
The write guard acquired here remains live through build_signed(...).await. A union-funded payment can select hundreds of inputs, and the production signer resolves and parses the mnemonic, derives the seed and BIP32 key, and invokes the host callback separately for each input. This exclusively blocks wallet sync and every other manager-backed operation for the entire signing phase. The existing finalize_transaction path reserves and snapshots the unsigned transaction, UTXOs, and paths under the lock, drops the guard, and signs afterward; this path should follow that model once reservations cover every funding account, releasing all reservations if signing fails.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — External signing holds the wallet-manager write lock 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.
… funding build_signed_payment funded from the union of all signable funds accounts (BIP44 + CoinJoin + …) with BIP44 change — the privacy-domain-crossing design blocked on dashpay#4184 (shumkov, 2026-07-21) and replaced there by single-account selection. This code predated that re-scope. - add funding_path: Option<DerivationPath>; None = unmixed BIP44 account 0, Some(path) = strictly the one funds account whose account path matches. No union, no cross-account accumulation; shortfall returns PaymentInsufficientFunds for that account only. Change routes to BIP44 (explicit change addr when a non-Standard account funds). - new wallet::funding_privacy guardrail: crate-wide static test fails the build if any wallet-wide funds-account iteration lacks a PRIVACY-DOMAIN-OK marker. - replace the union-asserting test with default-never-crosses-domains and explicit-path-selects-strictly tests. - review-fix hardening: bounded FFI allocation + checked cursor math, output-total overflow guard, fee-rate bound, typed PaymentInsufficientFunds (code 22). - thread funding_path through FFI/JNI/Kotlin (null = unmixed BIP44). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR reintroduced coin selection across the union of all signable funds accounts, which was blocked on #4184 by @shumkov on 2026-07-21 and replaced by the single-selected-account design (4d3e132, signed off 2026-07-23). The send-raw-tx code predates that re-scope; when it was extracted into this PR on 2026-07-28 it was not diffed against the design decision already settled on the sibling PR, and the extraction was treated as mechanical. Nothing surfaced it: the code compiled,
|
…Balances
Adds derivation_path (Option<String>) to AccountBalanceRow, the FFI
AccountBalanceEntryFFI (appended, ABI-additive), and the JNI JSON
("derivationPath": string|null). Computed from the same
AccountType::derivation_path(network) the funding-path spend selector
compares against, so the emitted string is byte-identical to what
build_signed_payment expects — the app passes it verbatim to spend a
DashPay receival account. Null for account types with no derivable path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Added: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
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/src/wallet/core/send.rs (1)
388-409: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop the wallet-manager write guard before signing.
wmis acquired at Line 212 and stays alive until the end of the function, so the global write lock is held acrossbuilder.build_signed(signer, ...).await. The account borrows end atset_funding(Line 396) and the builder owns cloned inputs, reservations, and change address, so the guard can be released before signing. In production the signer resolves the mnemonic and calls the host callback once per input, which blocks wallet sync and every other manager-backed operation for the whole signing phase.This repeats an earlier finding on this line range that was marked addressed.
🔒️ Proposed fix: release the guard after the builder is constructed
for (address, amount) in &outputs { builder = builder.add_output(address, *amount); } builder }; + // The builder owns cloned inputs, reservations, and change address, so + // the global manager write lock must not be held across the signer's + // per-input host round-trips. + drop(wm); let (transaction, _estimated_fee) = builder .build_signed(signer, move |addr| path_map.get(&addr).cloned())🤖 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/src/wallet/core/send.rs` around lines 388 - 409, Release the wallet-manager write guard `wm` immediately after constructing the transaction `builder` and before awaiting `builder.build_signed(...)`. Ensure no borrows from `wm` remain across the signing call, while preserving the existing builder configuration and signing behavior.
🧹 Nitpick comments (3)
packages/rs-platform-wallet/src/wallet/funding_privacy.rs (1)
186-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch this file by relative path, not by basename.
this_fileholds onlyfunding_privacy.rs, so the scan skips every file with that basename anywhere undersrc/. A futurewallet/<other>/funding_privacy.rswould then be exempt from the guardrail. Compare the path suffix fromfile!()instead.♻️ Proposed change
- let this_file = Path::new(file!()) - .file_name() - .expect("this file has a name") - .to_owned(); + // `file!()` is relative to the workspace root; match its tail so only + // this exact source file is exempt. + let this_file = PathBuf::from(file!());// then, inside the loop: if file.ends_with(&this_file) { continue; }🤖 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/src/wallet/funding_privacy.rs` around lines 186 - 195, Update the self-file exclusion in the loop over files to compare each path against the relative path suffix from file!(), rather than comparing only file_name(). Preserve skipping only the current funding_privacy.rs path while still scanning same-named files in other directories.packages/rs-platform-wallet/src/wallet/core/send.rs (1)
537-569: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated split-fixture snapshot logic. Both test modules read the manager, collect BIP44 and CoinJoin outpoints, and derive the CoinJoin account-level path. The shared root cause is that this helper does not live next to
split_funded_wallet_managerinpackages/rs-platform-wallet/src/test_support.rs.
packages/rs-platform-wallet/src/wallet/core/send.rs#L537-L569: movesplit_account_outpoints_and_coinjoin_pathintopackages/rs-platform-wallet/src/test_support.rsand import it here.packages/rs-platform-wallet/src/wallet/funding_privacy.rs#L300-L324: replace the inline block with a call to that shared helper.🤖 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/src/wallet/core/send.rs` around lines 537 - 569, Move split_account_outpoints_and_coinjoin_path from packages/rs-platform-wallet/src/wallet/core/send.rs lines 537-569 into packages/rs-platform-wallet/src/test_support.rs alongside split_funded_wallet_manager, then import and call it from send.rs. In packages/rs-platform-wallet/src/wallet/funding_privacy.rs lines 300-324, replace the duplicated manager-read, outpoint-collection, and derivation-path logic with the shared helper call.packages/rs-platform-wallet-ffi/src/error.rs (1)
327-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a dedicated test for the
PaymentInsufficientFundsmapping.The
From<PlatformWalletError>conversion now maps bothCoreInsufficientFundsandPaymentInsufficientFundstoErrorCoreInsufficientFunds. The existing testatomic_core_insufficient_funds_maps_to_dedicated_codeonly constructsCoreInsufficientFundsvalues. If a future edit accidentally dropsPaymentInsufficientFundsfrom this arm, it silently falls through toErrorUnknownand no test catches it.Add a sibling test that constructs a
PlatformWalletError::PaymentInsufficientFundsvalue and asserts it also maps toErrorCoreInsufficientFunds.Also applies to: 629-647
🤖 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/error.rs` around lines 327 - 339, Add a sibling conversion test next to atomic_core_insufficient_funds_maps_to_dedicated_code that constructs PlatformWalletError::PaymentInsufficientFunds with representative available and required amounts, converts it through From<PlatformWalletError>, and asserts PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds.
🤖 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_types.rs`:
- Around line 494-504: The added derivation_path field makes
AccountBalanceEntryFFI incompatible with existing consumers of
platform_wallet_manager_get_account_balances. Preserve the current V1 struct and
function unchanged, then expose derivation_path through a versioned struct and
entry point; alternatively, enforce coordinated native-binding regeneration and
deployment so older clients cannot load the changed ABI.
In `@packages/rs-platform-wallet/src/wallet/funding_privacy.rs`:
- Around line 344-357: Add a non-empty assertion for the spent inputs in the
test around the existing spent, coinjoin_ops, and bip44_ops checks, matching the
sibling explicit_coinjoin_path_selects_only_coinjoin test. Keep the existing
ownership assertions unchanged.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Around line 388-409: Release the wallet-manager write guard `wm` immediately
after constructing the transaction `builder` and before awaiting
`builder.build_signed(...)`. Ensure no borrows from `wm` remain across the
signing call, while preserving the existing builder configuration and signing
behavior.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 327-339: Add a sibling conversion test next to
atomic_core_insufficient_funds_maps_to_dedicated_code that constructs
PlatformWalletError::PaymentInsufficientFunds with representative available and
required amounts, converts it through From<PlatformWalletError>, and asserts
PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds.
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Around line 537-569: Move split_account_outpoints_and_coinjoin_path from
packages/rs-platform-wallet/src/wallet/core/send.rs lines 537-569 into
packages/rs-platform-wallet/src/test_support.rs alongside
split_funded_wallet_manager, then import and call it from send.rs. In
packages/rs-platform-wallet/src/wallet/funding_privacy.rs lines 300-324, replace
the duplicated manager-read, outpoint-collection, and derivation-path logic with
the shared helper call.
In `@packages/rs-platform-wallet/src/wallet/funding_privacy.rs`:
- Around line 186-195: Update the self-file exclusion in the loop over files to
compare each path against the relative path suffix from file!(), rather than
comparing only file_name(). Preserve skipping only the current
funding_privacy.rs path while still scanning same-named files in other
directories.
🪄 Autofix (Beta)
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: ed8a5fe0-e2ec-48c4-bf68-f61497e899aa
📒 Files selected for processing (16)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/send.rspackages/rs-platform-wallet-ffi/src/core_wallet_types.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/utils.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/send.rspackages/rs-platform-wallet/src/wallet/funding_privacy.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-unified-sdk-jni/src/dashpay.rspackages/rs-unified-sdk-jni/src/funding.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: prior-fee-overflow, prior-dust-output, and prior-reserved-balance are all STILL VALID because the latest commit does not change the affected send path. New findings in the latest delta: none; the derivation-path field consistently uses the same account-path derivation as payment selection and is correctly marshalled and freed across FFI/JNI, while the proposed round-trip test gap does not establish a concrete defect. The two carried-forward transaction-validity blockers require changes.
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— rust-quality (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 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/rs-platform-wallet/src/wallet/core/send.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:175-179: Positive dust outputs produce signed transactions that cannot be relayed
Output validation rejects only zero amounts, so callers can request a one-duff recipient output. `TransactionBuilder::add_output` preserves that amount without applying relay dust policy; for a standard P2PKH destination, `script_pubkey().dust_value()` is 546 duffs. The method can therefore return successfully signed bytes for a transaction that normal Dash nodes reject as nonstandard, despite documenting this as a standard payment primitive intended for later broadcast. Validate every recipient amount against its destination script's dust value before acquiring the wallet lock, reserving inputs, or signing.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:459-475: Insufficient-funds metadata counts reserved inputs as available
`selectable_value` is calculated from `selected.spendable_utxos(height)`, which applies UTXO spendability rules but does not exclude active reservations. `TransactionBuilder::set_funding`, by contrast, removes reserved outpoints before coin selection. After one build reserves an account's only UTXO, a second build receives `NoUtxosAvailable` but reports that reserved UTXO's full value as `PaymentInsufficientFunds.available`, giving callers an inaccurate actionable balance. Preserve the `available` value carried by the two `InsufficientFunds` variants and report zero for `NoUtxosAvailable` instead of substituting the reservation-blind total.
| if outputs.iter().any(|(_, amount)| *amount == 0) { | ||
| return Err(PlatformWalletError::TransactionBuild( | ||
| "every output amount must be greater than zero".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Positive dust outputs produce signed transactions that cannot be relayed
Output validation rejects only zero amounts, so callers can request a one-duff recipient output. TransactionBuilder::add_output preserves that amount without applying relay dust policy; for a standard P2PKH destination, script_pubkey().dust_value() is 546 duffs. The method can therefore return successfully signed bytes for a transaction that normal Dash nodes reject as nonstandard, despite documenting this as a standard payment primitive intended for later broadcast. Validate every recipient amount against its destination script's dust value before acquiring the wallet lock, reserving inputs, or signing.
source: ['codex']
There was a problem hiding this comment.
Fixed in 920b706 — the finding was valid at fe72da65; only zero amounts were rejected.
build_signed_payment now checks every recipient against its OWN destination script's dust_value() (wallet/core/send.rs), not a shared constant — the threshold is script-shaped, which is also why key-wallet's hard-coded 546 change-dust literal was not reusable. Placed after the cheap recipient-count bound (so an absurd list doesn't drive a script serialization per output) but before the wallet lock, before any input is reserved, and before the signer is called.
Tests: below_dust_outputs_are_rejected covers 1 duff, dust - 1, a below-dust output hidden among valid ones, and that exactly 546 still builds; a_rejected_dust_request_reserves_nothing pins that the refusal costs the caller nothing — three refused requests, then an ordinary payment still finds the account's coins selectable.
| BuilderError::InsufficientFunds { required, .. } => { | ||
| PlatformWalletError::PaymentInsufficientFunds { | ||
| available: available_in_account, | ||
| required: required.max(outputs_total), | ||
| } | ||
| } | ||
| BuilderError::CoinSelection(SelectionError::InsufficientFunds { required, .. }) => { | ||
| PlatformWalletError::PaymentInsufficientFunds { | ||
| available: available_in_account, | ||
| required: required.max(outputs_total), | ||
| } | ||
| } | ||
| BuilderError::CoinSelection(SelectionError::NoUtxosAvailable) => { | ||
| PlatformWalletError::PaymentInsufficientFunds { | ||
| available: available_in_account, | ||
| required: outputs_total, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Insufficient-funds metadata counts reserved inputs as available
selectable_value is calculated from selected.spendable_utxos(height), which applies UTXO spendability rules but does not exclude active reservations. TransactionBuilder::set_funding, by contrast, removes reserved outpoints before coin selection. After one build reserves an account's only UTXO, a second build receives NoUtxosAvailable but reports that reserved UTXO's full value as PaymentInsufficientFunds.available, giving callers an inaccurate actionable balance. Preserve the available value carried by the two InsufficientFunds variants and report zero for NoUtxosAvailable instead of substituting the reservation-blind total.
source: ['codex']
There was a problem hiding this comment.
Not changed — deliberately, and I'd like to push back on the suggested shape.
The finding is accurate about the mechanism: selectable_value comes from selected.spendable_utxos(height), which does not exclude active reservations, while set_funding removes reserved outpoints before selection. So with a concurrent in-flight build, available can over-report.
But the proposed remedy — report zero for NoUtxosAvailable — trades one wrong number for another. Zero tells the user "this account is empty", which is false and is the more misleading of the two: the coins exist and become selectable again the moment the other build broadcasts, is abandoned, or its reservation TTLs out. available is rendered as an actionable balance, and "0" invites the caller to conclude the account is unusable.
Reporting the reservation-aware figure would be the genuinely correct fix, and it is cheap — filter the spendable_utxos pass by the account's ReservationSet. I did not do it here because at this point in the function the reservation ledger has not been consulted yet (set_funding is what captures it), so it needs a small restructure rather than a one-liner, and this PR is already carrying two blocking fixes to funds-critical arithmetic. I'd rather not fold a third behavioural change into the same commit.
Happy to do it as a follow-up, or in this PR if you'd prefer it land together — but I'd want it to be the filtered real number, not zero. Flagging explicitly so this isn't read as silently dropped.
|
Reviewed at 1. Every 2. Six hardening fixes shipped with no tests — MAX_MONEY output total, MAX_FEE_PER_KB, the 3. No way to release an abandoned build's reservation. This primitive is build-only, but nothing across FFI/JNI/Kotlin exposes 4. Minor: the typed shortfall's Merge order: this belongs after #4184 (canonical |
|
Re merge order and the ReservationToken suggestion: agreed on the ordering — #4184 → #4185 → this PR → #4256. On the token shape: rather than reworking this PR's return type, #4256 (stacked on #4185 + this PR) already delivers exactly that — |
…ts for the send hardening Addresses the review findings on dashpay#4247. Dust outputs (BLOCKING). `TransactionBuilder::add_output` applies no relay policy, so a one-duff recipient produced fully signed bytes for a transaction every standard node rejects as nonstandard — from a primitive documented as building a *standard* payment for later broadcast. Each recipient amount is now checked against its OWN destination script's `dust_value()` (546 duffs for P2PKH), before the wallet lock, before any input is reserved and before the signer is called. Fee/size overflow (BLOCKING). The `MAX_FEE_PER_KB = MAX_MONEY / 100` bound assumed the transaction stayed under the 100 kB standard limit, which the method never enforced; key-wallet's `calculate_fee` then multiplies `sat_per_kb * size_bytes` unchecked and overflows at ~878 kB — reachable both by a ~25.8k-recipient list and by a funding account with a few thousand small denominations. Two-sided fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`), with checked arithmetic mirroring key-wallet's own base-size formula; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, which makes the product unrepresentable-free for ANY size a `u32` can express and therefore does not depend on the input count. ~43 DASH/kB is still three orders of magnitude above any legitimate rate; * the signed transaction is re-measured and refused if it exceeds the standard limit. Typed build errors. `PlatformWalletError::TransactionBuild` had no FFI arm, so every `funding_path` failure — "no spendable funds account matches" and "names a watch-only account", the two failure modes the single-account design rests on — reached Kotlin as `Generic(99)` and could only be told apart by string-matching. Adds `ErrorTransactionBuild = 32` (27-31 are claimed by sibling v4.1 stack PRs, so this needs no renumbering whichever order they land) plus the Kotlin `PlatformWallet.TransactionBuild` type. Tests for the six hardening fixes, which shipped with no coverage. `core_wallet/send.rs` had no test module at all; it now covers the `count` bound, `try_reserve_exact`, checked cursor math at every field boundary, UTF-8, and wrong-network address rejection. Also covers MAX_MONEY aggregation, the fee-rate bound, `parse_optional_derivation_path`, dust rejection (including that a refused request reserves nothing), and the new size bound. Also: corrects the stale `PaymentInsufficientFunds` doc, which still described the pre-dashpay#4184 union semantics; corrects an inverted fee-direction comment; adds the missing non-empty assertion to a funding-privacy guardrail test; and runs `cargo fmt` over the five files that failed `--check`. platform-wallet 510 passed, platform-wallet-ffi 222 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 1. Numbering: I took 32, not 27. On the integration line 27–31 are all claimed ( 2. Six hardening fixes with no tests — fixed. 3. No way to release an abandoned build's reservation — not addressed here, per your merge-order note; that's the token-minting shape that stacks on #4185 (#4256). 4. PastaClaw blockers, verified against
Also picked up from your "minor" list: the inverted fee-direction comment, and the Left open, deliberately, flagged so they aren't read as dropped: the reservation-blind Suites: 🤖 Generated with Claude Code |
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ode (33) `map_send_builder_error` folded `BuilderError::SigningFailed` into `TransactionBuild` → native code 32, whose documented contract is "the request itself is at fault; a verbatim retry fails identically". That is false for the production `MnemonicResolverCoreSigner`: a locked or missing Keychain mnemonic surfaces as `SigningFailed`, and key-wallet `release_if_owner`-releases the build's owner-stamped input reservation before returning, so the identical recipients/amount/fee/funding path succeed once the signer is usable. Hosts were being told to make the user edit a payment that was never wrong. Adds `PlatformWalletError::TransactionSigning` → `ErrorTransactionSigning = 33` → `DashSdkError.PlatformWallet.TransactionSigning` (isRetryable = true, matching the ShieldedNoRecordedAnchor convention for "nothing committed, reservations released, retry once the precondition is met"). Code choice — 33, not the reviewer's suggested 31. 27-32 are all claimed across the sibling v4.1 stack (27/28 dashpay#4185, 29 dashpay#4184, 30 reserved for dashpay#4184's cross-domain-consent, 31 dashpay#4183, 32 dashpay#4247), so 33 is the first slot free on every branch. 31 IS reserved, but for a different contract: dashpay#4183's `ErrorSigningKeyUnavailable` is a state-transition failure asserting the signer holds no usable private key for a requested public key, restored from the typed `DashSDKSignerErrorCode::SigningKeyUnavailable`. This is a Core L1 input signing failure with no such provenance — `BuilderError::SigningFailed` also covers an unresolved input derivation path, a sighash computation failure and a malformed signature encoding — so reusing 31 would assert "the key is unavailable" for failures that are nothing of the kind. Kept separate so neither contract is weakened; the rationale is recorded on the variant for maintainers reconciling the range. Tests: an end-to-end locked-signer build asserting TransactionSigning (not TransactionBuild), a retry-after-recovery test proving the inputs really were released, FFI code/mapping tests, and the Kotlin decode + retry-contract test. platform-wallet 539 passed, platform-wallet-ffi 225 passed, kotlin-sdk 190 passed; fmt clean, no new clippy warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ilds `build_signed_payment` reserves its selected inputs and leaves them reserved on success, expecting a broadcast to follow. Nothing across Rust/FFI/JNI/Kotlin let a caller that declines to commit give those coins back, so an abandoned build stranded them for RESERVATION_TTL_BLOCKS (24, ~1h) — and indefinitely while the wallet has no processed height, since `ReservationSet::sweep` early-returns at height 0 and therefore never reclaims a pre-sync reservation. A single abandoned build on a freshly restored wallet could strand the whole balance for the life of the process. Adds a standalone release across all four layers: CoreWallet::release_payment_reservation(&Transaction, Option<DerivationPath>) core_wallet_release_payment_reservation (FFI) coreWalletReleasePaymentReservation (JNI) ManagedPlatformWallet.releasePaymentReservation (Kotlin) The transaction is the ownership signal: a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released — releasing its inputs releases precisely this build's own reservation and can never free a competing build's coins. Same signal the internal `release_reservation_after_rejected_broadcast` cleanup already uses. The release consults no height, so it works pre-sync where the TTL backstop cannot. It is idempotent (per-outpoint map removal) and a silent no-op after a successful broadcast — it cannot resurrect a spent coin, since selection reads the UTXO set that sync already updated — so callers can wire it into an unconditional cleanup path. Also routes the build's default-funding-account resolution through the shared `bip44_account_path` helper, so a release and its build can never disagree about what `funding_path: None` means. Tests: release-then-reselect (with the second-build failure pinned as a precondition so it can't pass vacuously), release twice, release after a processed broadcast, release against the wrong account frees nothing, unknown funding path is refused, and the height-0 case — 30 build attempts prove the TTL never fires there, then the explicit release frees the inputs. Addresses review item 3 on dashpay#4247. The reviewer's suggestion to unify this with dashpay#4256's reservation-token finalize is tracked separately rather than done here, to avoid reshaping an API the Android app already calls and hard-coupling this PR to dashpay#4185. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@shumkov — following up on item 3. I said earlier this would ride on #4185/#4256; on reflection that left a real hazard sitting in a PR that is otherwise ready, so I've closed it here instead. Fixed in What landed. A standalone release/abandon across all four layers: A caller that declines to commit hands back the transaction it was given and the same On the height-0 case specifically — the part of your item that had no workaround at all: the release consults no height and never calls Scoping. The transaction is the ownership signal — a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released. That's the same signal the internal On collapsing the two "signed payment" concepts — agreed, and tracked in #4263. Deferring rather than doing it here for two reasons: minting a Suites: 🤖 Generated with Claude Code |
…30 (dashpay#4256) dashpay#4256 is stacked on dashpay#4185 and still carried the pre-renumber `29`, which now collides with `ErrorAssetLockInsufficientFunds = 29` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and the `ErrorReservationWalletMismatch` family moves to 30; dashpay#4185 already made that move in `d854debb`. This brings dashpay#4256 in line. CI could not have caught this: dashpay#4256 and dashpay#4184 are both MERGEABLE with green checks, because two branches assigning the same discriminant produce no textual conflict. It surfaces only as an E0081 after a textual merge, or silently as a wrong error code on the host. The discriminant is public ABI, so every mirror moves together: - Rust enum + its rustdoc cross-reference (error.rs) - doc reference in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value - Kotlin fromPlatformWalletNative branch, class KDoc, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also corrects this PR's own numbering rationale on `ErrorTransactionSigning` (33), which claimed 30 was "reserved for dashpay#4184's ErrorAssetLockCrossDomainConsentRequired". That code does not exist on any branch — dashpay#4184 dropped it in a re-scope — and 30 is now ErrorReservationWalletMismatch. dashpay#4256's codes are unchanged otherwise: it keeps 32 (ErrorTransactionBuild, shared with dashpay#4247) and 33. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 805 passed / 0 failed; :sdk:test BUILD SUCCESSFUL with DashSdkErrorTest 9/9. Swift is unverified — it cannot be compiled here.
…cord dashpay#4196 scope Clears the two review blockers on dashpay#4261 and re-syncs the registry with what the code on each branch actually does, re-read at every head rather than trusted from this file. Blocker (a) — dashpay#3968 / dashpay#3954 / dashpay#4259 were described in prose but had no rows, which is exactly what rule 2 forbids. They now have them: - A "Non-conforming allocations" table for dashpay#3968 (26/27/28) and dashpay#3954 (27). These are deliberately kept out of the proposed table: each row is a claim to be withdrawn and reissued, not an allocation of record. - An inherited-code table for the 31 that dashpay#4204 and dashpay#4259 carry but did not allocate (dashpay#4183 owns it), so it is not double-counted. - dashpay#4196 is recorded as claiming no integer at all: it routes a new token-less `StaleReservation` variant through the existing `ErrorStaleReservationToken`. The dashpay#3968 half is the serious one and is called out as such. Its 28 is not a new claim — it *moves the already-shipped* `ErrorTransactionBroadcastRejected` off 26 to make room for its own persister code. Rule 3 forbids that: a host compiled against merged ABI returns 26 for a broadcast rejection, and after dashpay#3968 the same condition returns 28 while 26 means a transient persister failure. Neither branch's diff shows the contradiction. Blocker (b) — 30 marked both free and assigned was already resolved by the preceding commit; verified consistent here (30 is allocated to dashpay#4185 throughout, frontier is 34, and the one remaining "genuinely free" is past tense explaining why dashpay#4185 could take it). Also corrected, all verified against the branches: - Survey provenance had dashpay#4185 at `0b0d5c76d6` labelled "(post-renumber)". Wrong twice: that commit is the *parent* of the renumber `d854debb`, and the head has since moved to `6c37e8679e`. dashpay#4184, dashpay#4247 and dashpay#4256 SHAs refreshed too. - dashpay#4256 has now taken 30 (`9481e5783b`) and dropped its stale "30 is reserved for the consent code" rationale; the equivalent comments on dashpay#4183 and dashpay#4204 are flagged as still present. - dashpay#4184 has a comment-only drift: it reserves "Codes 27-28" but names three codes. Correct when the trio was 27/28/29; it is now 27/28/30. Its discriminant is right and is the resolution of record — only the prose is stale, and dashpay#4184 is left untouched. - The dashpay#4196 section now records why the restack has not happened: its three own commits conflict in 3 files / 10 hunks against dashpay#4185's head, and the registry redesign underneath it (mandatory `registered_height`, new `WalletRemoved` variant, owner-stamped funding token) makes it author work rather than conflict resolution. Its trio numbers come from the dashpay#4185 copy it carries, so the restack fixes 28 -> 30 for free; the number dashpay#4196 itself must chase is 27, not 30. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 738 passed / 0 failed. Docs-only change.
What
Adds the SDK's L1 send primitive
build_signed_paymentand plumbs it through FFI → JNI → Kotlin.feat(platform-wallet): union-funding build_signed_payment core primitive— builds + signs an L1 payment transaction, union-funding across all funding accounts.feat(sdk): plumb build_signed_payment through FFI, JNI, and Kotlin— exposes it to the Android/Kotlin SDK.Why
This is the send half of the wallet SDK cutover: the Dash Wallet Android app calls it (
SdkL1SendService/CoreSendAllNative) for post-cutover L1 sends. Once dashj's L1 engine is held after cutover, this primitive is what actually builds/signs/sends transactions — so it is load-bearing for the cutover.Notes
v4.2-devbase (2 self-contained commits); mirrors the other v4.1 Kotlin-SDK feature PRs.cargo check -p platform-wallet -p platform-wallet-ffiis clean.Summary by CodeRabbit