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 49 commits into
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 ignored due to path filters (1)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughAdds single-account Core L1 payment construction and deferred signed-payment management. The flow supports reservation tokens, broadcast, explicit release, wallet-generation checks, native error mapping, and account derivation-path metadata across Rust, FFI, JNI, Kotlin, and Swift. ChangesCore signed payment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformWallet
participant ManagedCoreWallet
participant WalletManagerNative
participant JNI
participant SignedPaymentRegistry
participant CoreWallet
participant SpvBroadcaster
ManagedPlatformWallet->>ManagedCoreWallet: build or finalize signed payment
ManagedCoreWallet->>WalletManagerNative: pass payment data and account selection
WalletManagerNative->>JNI: invoke native wallet operation
JNI->>CoreWallet: validate, fund, reserve, and sign
CoreWallet-->>JNI: signed transaction and reservation metadata
JNI->>SignedPaymentRegistry: register deferred signed payment
ManagedPlatformWallet->>SignedPaymentRegistry: broadcast or release reservation token
SignedPaymentRegistry->>SpvBroadcaster: broadcast registered transaction
SpvBroadcaster-->>SignedPaymentRegistry: transaction ID or broadcast result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
|
⛔ Blockers found — Sonnet deferred (commit 8c906d8) |
thepastaclaw
left a comment
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.
There was a problem hiding this comment.
Fixed in 51a31c5406 (fix(platform-wallet): re-scope build_signed_payment to single-account).
The union across accounts is gone. build_signed_payment now confines selection to a single funding account — the one named by funding_path, defaulting to the unmixed BIP44 account — so there is no set of secondary-account UTXOs to reserve in a foreign ledger. The module contract states it directly (wallet/core/send.rs header: "Selection is confined to a single funding account … never a union across accounts"), and every selected input is reserved in that one account's ReservationSet.
| 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.
There was a problem hiding this comment.
Fixed in 51a31c5406.
The decoder no longer hands the caller-controlled u32 to Vec::with_capacity. It computes max_possible from the remaining blob length (min bytes per entry) and rejects any count exceeding it before allocating (core_wallet/send.rs:85). Two regression tests pin it: an_impossible_count_is_rejected_without_allocating and a_count_exceeding_the_rows_present_is_rejected, plus a comment guarding against a future reintroduction of Vec::with_capacity(count).
| "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.
There was a problem hiding this comment.
Fixed in 51a31c5406. Output aggregation and the fee/amount sums are now checked (checked_add, with the per-amount consensus cap enforced before aggregation), so an overflowing request is rejected with a typed error rather than silently wrapping into an invalid transaction. key-wallet's own summation is reached only after this validation passes.
| 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.
There was a problem hiding this comment.
Resolved in this update — Unbounded fee rates overflow key-wallet fee 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.
| } | ||
| 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.
There was a problem hiding this comment.
Holding the wallet-manager write guard across the signer await is deliberate, and releasing it there would reintroduce the exact race this PR exists to close.
The guard spans coin selection → reservation → sign as one critical section. build_signed_reserved selects inputs, reserves them, and signs under a single hold; the reservation lives in the manager-owned ReservationSet. If the guard were dropped for the signer round-trip, another wallet-manager operation could interleave between reserve and sign — the funding/signing selection race the old setFunding + buildSigned split had, which the atomic finalize was introduced to eliminate (see ManagedPlatformWallet.kt, "closing the funding/signing selection race").
Splitting the hold so signing happens lock-free is not a clean change: it means teaching build_signed_reserved to reserve under the lock, drop it, sign, then re-acquire to register — reintroducing a coherence window and a substantial refactor of a primitive whose whole contract is atomicity. The sibling deferred-token path (#4256) makes the same choice for the same reason.
The real cost is latency: a keystore/biometric prompt serializes other wallet-manager work for its duration. That is an accepted tradeoff here, not an oversight — a single wallet's operations are already logically serial, and payment finalization is user-initiated and foreground. Leaving as-is by design.
bfoss765
changed the title
|
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,
|
|
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
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.
| 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 |
…NotFound The rebase onto v4.2-dev left `fromPlatformWalletNative` with TWO arms for platform-wallet code 98: this PR's original `7, 8, 98 -> NotFound(...)` and the merged-upstream `PLATFORM_WALLET_NOT_FOUND_CODE -> PlatformWallet.NotFound`. Kotlin's `when` takes the first matching branch, so 98 kept resolving to the top-level `DashSdkError.NotFound` and the second arm was dead code. That regressed the merged upstream behaviour and broke three assertions in `DashSdkErrorTest` (`platformWalletCodesMapToPlatformWalletSubtree`, `platformWalletNotFoundCodeMapsToTypedWalletNotFound`, `platformWalletNotFoundConvertsAtThePublicBoundary`), which all require offset + 98 to surface as the wallet-family `PlatformWallet.NotFound` and NOT as the top-level `NotFound` reserved for rs-sdk-ffi codes 7/8. Drop the stray `98,` so the 7/8 arm is exactly the rs-sdk-ffi pair, and move this PR's deferred-send documentation onto the arm that actually handles 98. The Rust side is unchanged and stays the source of truth: `PlatformWalletFFIResultCode::NotFound = 98` (packages/rs-platform-wallet-ffi/src/error.rs:296), listed as the terminal sentinel in ERROR_CODE_REGISTRY.md. No discriminant was renumbered. The rest of the branch already expected the corrected mapping — see the `PlatformWalletManager` KDoc, which documents this path as `DashSdkError.PlatformWallet.NotFound` (native code 98). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
… 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>
…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>
…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>
…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>
…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>
…le doc Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd-path test fixtures Restack adaptation only — no production-code change. This branch now sits on top of dashpay#4185 (port/v4.1/split-build-broadcast) rather than beside it. dashpay#4185 replaced the bare `Arc<WalletBalance>` generation marker with `Arc<WalletGeneration>` (balance + that generation's lifecycle gate in one Arc, so "same generation" and "same gate" cannot diverge), and renamed `PlatformWalletInfo::balance` to `::generation`. The send-path test fixtures added here still built wallets the old way, so they no longer compiled against the new base. Point them at the shared `WalletGeneration` the rest of the crate already uses: * `core/send.rs` — the local `core_wallet` fixture takes `Arc<WalletGeneration>`; `funded_wallet_manager` already hands one back. * `wallet/funding_privacy.rs` — same, for its two fixtures. * `test_support.rs` — the DashPay split fixture populates `PlatformWalletInfo::generation`. cargo test -p platform-wallet -p platform-wallet-ffi: 537 + 230 pass.
…lt space `ErrorTransactionBuild = 32` was added to the Rust FFI enum and to the Kotlin decoder by this PR, but not to the Swift mirror. Swift decodes an unlisted raw value through the `default:` arm of `PlatformWalletResultCode .init(ffi:)`, so every typed build rejection this PR introduced — an unresolvable or watch-only `fundingPath`, a breached monetary bound, a malformed recipients blob — reached iOS as `errorUnknown` (99) with only the message string to distinguish it. That is the exact failure the code was split out of `ErrorUnknown` to end, reintroduced on the other host. Adds the enum case, the `init(ffi:)` mapping, and the `PlatformWalletError.transactionBuild` arm. There is no compile-time check that the raw values match Rust, so the numbering comment is kept in sync with the registry (dashpay#4261) and now records only the codes this PR does NOT own. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ ERROR_TRANSACTION_BUILD = 32. No other exhaustive switch over PlatformWalletResultCode / PlatformWalletError exists in the Swift SDK.
`cargo fmt --check` fallout from the `WalletGeneration` adaptation two commits down: the shorter type name let two fixture signatures fit differently. No behaviour change.
Three blocking review findings on the send-raw-tx path, all in the
build/abandon reservation lifecycle.
1. Post-signing size rejection stranded its selected inputs.
`build_signed` had already reserved them when the
`signed_size > MAX_STANDARD_TX_SIZE` check ran, and the error path
returned without releasing and without handing back the transaction
the caller would need to release it itself. The TTL backstop is no
fallback: `ReservationSet::sweep` early-returns at height 0, so on a
freshly restored wallet those coins were stranded for the process
lifetime. The path now releases before returning.
2. Abandonment could release another build's inputs.
`release_reservation` removes entries by outpoint with no owner
check. Once key-wallet's TTL sweeps a reservation, a concurrent build
can legitimately re-reserve the same outpoint under a new token, and a
late release then freed THAT build's inputs — a double-spend window.
The build now keeps the `ReservationToken` from `build_signed_reserved`
and the release is owner-guarded via `release_reservation_if_owner`,
plus generation-bound the way `release_transaction_reservation` is.
The doc block claiming the transaction alone was a sufficient
ownership signal ("no concurrent build can hold a reservation on any
input") is corrected: that reasoning ignored the TTL sweep and is
refuted by key-wallet's own docs, which state the platform layer
cannot make this safe on its own.
The "safe after a broadcast / wire it into an unconditional cleanup
path" contract is also removed. This primitive does not broadcast, so
between the caller's successful broadcast and sync processing that
spend the inputs are still in the UTXO set and the reservation is the
only thing keeping a second build off them. Callers must release only
builds they did NOT broadcast.
The key-wallet token is deliberately unforgeable (private counter, no
public constructor) and so cannot cross the C ABI. It is threaded to the
host as an opaque handle from a bounded FIFO table, mirroring how
`SignedPaymentRegistry` keeps its funding token Rust-side while a u64
payment handle crosses the boundary. An unknown handle is refused rather
than downgraded to the unguarded release.
`split_funded_wallet_manager` now returns its real `WalletGeneration`.
Callers previously invented a fresh one, which silently makes every
generation-bound assertion vacuous.
Tests: the size-rejection path leaves no reservation held; an
owner-guarded release cannot free a re-reserved input (with the
unguarded release as the control that proves the guard is what closes
it); releasing a broadcast build before sync reopens its inputs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`buildSignedPayment` ran the blocking JNI build through `gate.op`, i.e. plain `withContext(Dispatchers.IO)`. Native signing cannot observe cancellation once started and reserves the funding UTXOs before it returns, so a caller cancelled mid-build had the completed `SignedCorePayment` discarded by prompt cancellation — taking with it the tx bytes and reservation handle that are the only way to release. The reservation then sat until the TTL backstop, or forever at height 0 where the sweep never runs. Switched to `gate.opWithCleanupOnCancellation`, the same handoff guard the token-owning `buildSignedPayment` overload already uses, with a synchronous best-effort release of the discarded payment. The cleanup runs from a `finally` and never throws: replacing the caller's `CancellationException` with an unrelated native error would be worse than the reservation it is trying to reclaim. Also threads the reservation handle through the Kotlin surface, which is what makes the release owner-guarded: - `SignedCorePayment` carries `reservationHandle`. - `decodeSignedPayment` reads the extended native blob (`u64 fee, u64 change, u64 reservationHandle`, then tx bytes). - `releasePaymentReservation` takes the handle; a new overload takes the `SignedCorePayment` itself so bytes and handle cannot be mismatched between two in-flight builds. The KDoc claiming the release is "safe after a broadcast" and therefore safe in an unconditional `finally` is removed — releasing between a successful broadcast and sync observing it reopens the inputs to selection. Callers branch on whether they broadcast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
647dbba to
bbe36ee
Compare
…-> 37 and mirror it (dashpay#4204) 32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#4185's registry contract Restack adaptation. This branch now sits on top of dashpay#4247 (which sits on dashpay#4185) instead of beside them, so the funding-path registration meets the contract dashpay#4185 established for the deferred-payment registry: * `RegisteredPayment` keeps ONE funding handle — dashpay#4256's `funding: FundingAccountRef` — and dashpay#4185's MANDATORY `registered_height: u32`. The two evolutions are orthogonal (which account vs. which clock), so the union is a strict superset: the Path arm still reaches a DashPay receiving-funds account, and the age guard can no longer be silently disabled by a `None` height. * `register_funded_by` therefore takes `registered_height: u32`. `FinalizedCorePayment::reservation_height` was already a non-optional `u32` sampled inside the funding critical section, so every caller simply drops its `Some(..)` wrapper — no behaviour change, one less way to disable the guard. * `ReservationToken` is dashpay#4185's newtype; the FFI converts with `as_u64` at the C ABI boundary. * The over-limit refusal binds the payment first and discharges its reservation through `abandon_payment` before returning, rather than stranding the account's coins until the TTL backstop. * Test fixtures build wallets with `WalletGeneration` (balance + the generation's lifecycle gate in one Arc) and read `PlatformWalletInfo::generation`. `register_funded_by` still cannot prove its `core` is the generation that produced the payment the way `register` can — `FinalizedCorePayment` carries no `origin_generation` marker, so the FFI's single `generation_payment_guard` hold is what upholds the binding. That gap is pre-existing on this branch and is documented on the method as follow-up. cargo test -p platform-wallet -p platform-wallet-ffi: 540 + 230 pass.
Carries the `FinalizedCorePayment` half of dashpay#4256's review commit 34c0894. The fee-cap/tx-size half of that commit is already in the stack: dashpay#4247 landed the identical `MAX_STANDARD_TX_SIZE` bound and the `u64::MAX / u32::MAX` fee-rate derivation one commit below this one, so re-applying it here would only duplicate it. The reservation a finalized payment holds must be discharged exactly once. While the type derived `Clone`, safe code could abandon one copy and register another, or mint two registry tokens able to broadcast the same transaction — one release would then free inputs the other copy still believed it owned. Dropping `Clone` and having `abandon_payment` consume the value makes register-after-abandon fail to compile. Also brings that commit's coverage for the contract: `abandon_payment_releases_the_reservation_without_the_registry` — the receival account's inputs come back without the registry ever being involved, and the payment cannot be reused afterwards. cargo test -p platform-wallet -p platform-wallet-ffi: 541 + 230 pass.
…s onto FundingReservationToken The dashpay#4247 restack briefly carried two aliases for the same key_wallet type; collapse onto the repo-wide spelling so both the split-build and funding-path call sites read against one name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…st outcomes Two corrections from the dashpay#4261 review round: - The code-32 collision peer was misidentified. dashpay#4204 defined ErrorShieldedInviteAlreadyClaimed = 32 (b6992a5) and dashpay#4247 defines ErrorTransactionBuild = 32 (error.rs:257) — that is the E0081 pair. ErrorReservationWalletMismatch never held 32; its history runs 26/28 -> 30 -> 36. - "silently wrong on every host" overstated it. The three host outcomes differ and only one is a misclassification; spell them out. Also describe the inherited-code table as reduced to one row and retained for provenance rather than gone, since the row is rendered right below. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rred-payment FFI out-params Five review findings on the split build/broadcast + atomic-finalize surface (dashpay#4247): - wallet_manager.rs: the build-signed-payment JNI export dropped the funding reservation on the byte_array_from_slice failure path — the native build had already reserved its inputs and returned the handle inside the packed blob, but on an alloc failure the export returned null without releasing, stranding the coins until key-wallet TTL (which never fires pre-first-sync). Now mirrors the sibling send export: releases via core_wallet_release_payment_reservation before returning null. Highest-impact of the set (latent fund availability). - transaction_builder.rs: core_wallet_signed_payment_finalize wrote only out_token up front; the other five out-params (out_fee/out_txid/out_tx/ out_bytes_ptr/out_bytes_len) were set only in the success block, so a caller ignoring the result code could read a dangling out_tx.tx_bytes after an error return. Zero-initialize all six at entry. - signed_payment.rs: core_wallet_signed_payment_broadcast wrote *out_txid only in the Ok arm; null-initialize at entry so the five error arms leave a defined value. - ManagedPlatformWallet.kt: fromRegisterBlob read two u32 length prefixes and allocated ByteArrays without bounds/negative-length guards. The blob comes from our own JNI packer (not attacker-reachable), but a malformed layout now fails with a precise message naming the field instead of an opaque NegativeArraySize/BufferUnderflow. - error.rs: two doc comments still described the deferred-token trio as occupying the 27-31 window; it moved to 34-36. Corrected both to defer to the authoritative in-file allocation block and ERROR_CODE_REGISTRY.md (dashpay#4261). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Swept in by cargo fmt on platform-wallet-ffi; the collapsed unsafe block at dashpay.rs:1334 predates this PR and fails cargo fmt --check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#4185's registry contract Restack adaptation. This branch now sits on top of dashpay#4247 (which sits on dashpay#4185) instead of beside them, so the funding-path registration meets the contract dashpay#4185 established for the deferred-payment registry: * `RegisteredPayment` keeps ONE funding handle — dashpay#4256's `funding: FundingAccountRef` — and dashpay#4185's MANDATORY `registered_height: u32`. The two evolutions are orthogonal (which account vs. which clock), so the union is a strict superset: the Path arm still reaches a DashPay receiving-funds account, and the age guard can no longer be silently disabled by a `None` height. * `register_funded_by` therefore takes `registered_height: u32`. `FinalizedCorePayment::reservation_height` was already a non-optional `u32` sampled inside the funding critical section, so every caller simply drops its `Some(..)` wrapper — no behaviour change, one less way to disable the guard. * `ReservationToken` is dashpay#4185's newtype; the FFI converts with `as_u64` at the C ABI boundary. * The over-limit refusal binds the payment first and discharges its reservation through `abandon_payment` before returning, rather than stranding the account's coins until the TTL backstop. * Test fixtures build wallets with `WalletGeneration` (balance + the generation's lifecycle gate in one Arc) and read `PlatformWalletInfo::generation`. `register_funded_by` still cannot prove its `core` is the generation that produced the payment the way `register` can — `FinalizedCorePayment` carries no `origin_generation` marker, so the FFI's single `generation_payment_guard` hold is what upholds the binding. That gap is pre-existing on this branch and is documented on the method as follow-up. cargo test -p platform-wallet -p platform-wallet-ffi: 540 + 230 pass.
Carries the `FinalizedCorePayment` half of dashpay#4256's review commit 34c0894. The fee-cap/tx-size half of that commit is already in the stack: dashpay#4247 landed the identical `MAX_STANDARD_TX_SIZE` bound and the `u64::MAX / u32::MAX` fee-rate derivation one commit below this one, so re-applying it here would only duplicate it. The reservation a finalized payment holds must be discharged exactly once. While the type derived `Clone`, safe code could abandon one copy and register another, or mint two registry tokens able to broadcast the same transaction — one release would then free inputs the other copy still believed it owned. Dropping `Clone` and having `abandon_payment` consume the value makes register-after-abandon fail to compile. Also brings that commit's coverage for the contract: `abandon_payment_releases_the_reservation_without_the_registry` — the receival account's inputs come back without the registry ever being involved, and the payment cannot be reused afterwards. cargo test -p platform-wallet -p platform-wallet-ffi: 541 + 230 pass.
…s onto FundingReservationToken The dashpay#4247 restack briefly carried two aliases for the same key_wallet type; collapse onto the repo-wide spelling so both the split-build and funding-path call sites read against one name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second half of the post-commit reservation-leak cluster (CodeRabbit, dashpay#4247). core_wallet_signed_payment_finalize registers the payment and mints its reservation token before returning; the JNI export then checks out_txid, and on the (contract-violating) null-txid arm it freed the transaction but left the token holding its inputs until the TTL sweep (which never fires pre-first-sync). Release the token via core_wallet_signed_payment_release before throwing, matching the sibling register-blob path Err arm. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#4185's registry contract Restack adaptation. This branch now sits on top of dashpay#4247 (which sits on dashpay#4185) instead of beside them, so the funding-path registration meets the contract dashpay#4185 established for the deferred-payment registry: * `RegisteredPayment` keeps ONE funding handle — dashpay#4256's `funding: FundingAccountRef` — and dashpay#4185's MANDATORY `registered_height: u32`. The two evolutions are orthogonal (which account vs. which clock), so the union is a strict superset: the Path arm still reaches a DashPay receiving-funds account, and the age guard can no longer be silently disabled by a `None` height. * `register_funded_by` therefore takes `registered_height: u32`. `FinalizedCorePayment::reservation_height` was already a non-optional `u32` sampled inside the funding critical section, so every caller simply drops its `Some(..)` wrapper — no behaviour change, one less way to disable the guard. * `ReservationToken` is dashpay#4185's newtype; the FFI converts with `as_u64` at the C ABI boundary. * The over-limit refusal binds the payment first and discharges its reservation through `abandon_payment` before returning, rather than stranding the account's coins until the TTL backstop. * Test fixtures build wallets with `WalletGeneration` (balance + the generation's lifecycle gate in one Arc) and read `PlatformWalletInfo::generation`. `register_funded_by` still cannot prove its `core` is the generation that produced the payment the way `register` can — `FinalizedCorePayment` carries no `origin_generation` marker, so the FFI's single `generation_payment_guard` hold is what upholds the binding. That gap is pre-existing on this branch and is documented on the method as follow-up. cargo test -p platform-wallet -p platform-wallet-ffi: 540 + 230 pass.
Carries the `FinalizedCorePayment` half of dashpay#4256's review commit 34c0894. The fee-cap/tx-size half of that commit is already in the stack: dashpay#4247 landed the identical `MAX_STANDARD_TX_SIZE` bound and the `u64::MAX / u32::MAX` fee-rate derivation one commit below this one, so re-applying it here would only duplicate it. The reservation a finalized payment holds must be discharged exactly once. While the type derived `Clone`, safe code could abandon one copy and register another, or mint two registry tokens able to broadcast the same transaction — one release would then free inputs the other copy still believed it owned. Dropping `Clone` and having `abandon_payment` consume the value makes register-after-abandon fail to compile. Also brings that commit's coverage for the contract: `abandon_payment_releases_the_reservation_without_the_registry` — the receival account's inputs come back without the registry ever being involved, and the payment cannot be reused afterwards. cargo test -p platform-wallet -p platform-wallet-ffi: 541 + 230 pass.
…s onto FundingReservationToken The dashpay#4247 restack briefly carried two aliases for the same key_wallet type; collapse onto the repo-wide spelling so both the split-build and funding-path call sites read against one name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
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
New Features
Bug Fixes