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 46 commits into
Conversation
|
Warning Review limit reached
Next review available in: 59 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 (40)
📝 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
|
🕓 Ready for review — 9 ahead in queue (commit bbe36ee) |
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.
| 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.
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.
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 |
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, 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 disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…the final-alias policy removal (dashpay#4185 review) Blocker 2 removed the final-alias sweep from `platform_wallet_destroy` (wallet.rs:392-414 is now just a storage `remove`), but the helper that policy was introduced for survived it. `HandleStorage::any` was added by ade3999 for that sweep and has had zero call sites since the policy was dropped. Because `handle` is a `pub mod` and the method is `pub`, no dead-code lint fires and it stayed in the crate's public Rust surface, with Rustdoc still pointing at "the final-alias check in `platform_wallet_destroy`" — a policy that no longer exists. It is not on the base branch, so removing it restores the base surface rather than breaking an existing consumer. `HandleStorage::remove_matching` is retained: it still backs the generation sweep at manager.rs:494. Also corrects a doc cross-reference to the same removed policy in `test_support::test_platform_wallet_manager`, which described the helper as backing "final-alias registry-sweep gating" when its only FFI consumer now asserts the opposite (destroying wrapper aliases must NOT sweep). No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on (dashpay#4185 review) The gate added in 0b0d5c7 lived on the FFI's process-global `SIGNED_PAYMENT_REGISTRY`, so it excluded only registry-token operations and did so across every wallet at once. Two consequences, both real: 1. Under-coverage. The V2 finalized-transaction-handle path bypassed it entirely. `core_wallet_tx_builder_finalize` awaited the external signer and then inserted into `CORE_SIGNED_TRANSACTION_V2_STORAGE` with no gate and no liveness re-check, so a teardown could sweep while signing was pending and the late finalizer published a handle no sweep would ever catch. `core_wallet_broadcast_signed_transaction_v2` then consumed such a handle and reached the broadcaster with no gate either — its `is_same_generation` check compares two HANDLES, and a removed generation matches itself. The same hole existed on the public Rust surface: `PlatformWalletManager::remove_wallet` never took the write side at all, so a direct embedder (the manager is public and `SignedPaymentRegistry` is re-exported) removed wallets with no exclusion. 2. Cross-wallet contention. A deferred broadcast holds the shared side across an SPV send; on one process-global write-preferring lock that send blocked teardown — and every payment operation queued behind the waiting writer — for every unrelated wallet in the process. Remedy: move the gate into shared per-generation state. * New `WalletGeneration` owns the lock-free `WalletBalance` AND that generation's `RwLock` lifecycle gate, and replaces `Arc<WalletBalance>` as the generation-identity marker. Folding them into one `Arc` is deliberate: the identity and the gate cannot diverge, so two handles can never compare as the same generation while excluding each other through different locks. `Deref` keeps every existing balance read unchanged. * `PlatformWalletManager::remove_wallet_with_teardown` takes that generation's exclusive gate across BOTH the removal and a caller-supplied teardown hook, and `remove_wallet` routes through it. The gate is no longer optional for any caller, FFI or not. The FFI passes its registry + V2-handle sweep as the hook. Lock order stays gate-then-manager: the lookup that resolves the gate drops `wallets` before awaiting it, then re-validates under the gate. * Every publication/network path now takes the generation's shared gate across its liveness check and the action: the registry `broadcast`/`release`, the token `core_wallet_signed_payment_finalize`, and — newly — both `core_wallet_tx_builder_finalize` and `core_wallet_broadcast_signed_transaction_v2`, which report a dead generation as the existing `NotFound` (98) after reconciling the build's reservation. V2 abandon/free stay ungated: their release is already generation-bound. The gate is still NOT held across an external signer await — finalizers acquire it only after the signature returns, so an open signing prompt cannot stall teardown, and a late finalizer instead fails its liveness check and abandons. The lifecycle comments that claimed the opposite were wrong about the code and are corrected. Adds four deterministic FFI regression tests alongside the existing three: a V2 broadcast-after-removal refusal, a teardown that waits for an in-flight V2 operation and then sweeps its handle, a public `PlatformWalletManager::remove_wallet` that waits for an in-flight payment, and a cross-wallet isolation test pinning the per-generation scoping. With the three guards removed the first three fail; with the gate re-pointed at a single process-global lock the fourth fails. No error-code changes: `ErrorReservationWalletMismatch` stays 30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y key `remove_wallet_with_teardown` validated generation G1 under G1's lifecycle gate, then removed it from the two manager maps in two independently locked stages. Registration takes no gate at all — `register_wallet` mints its own `WalletGeneration` — so from the moment the inner-manager removal frees the id, a concurrent same-id registration can publish a different generation G2 into `wallet_manager` and then into `self.wallets`, with no happens-before edge to the remover's own `self.wallets` acquisition. A remover descheduled in that gap resumed into a map naming G2 and removed the entry BY KEY: it evicted a live wallet (still registered in the inner manager, so invisible and unremovable through the public map), returned it to the caller, and handed it to `tear_down` — which sweeps that generation's registry tokens and V2 finalized-transaction handles while holding only G1's gate, i.e. with G2's payment operations not excluded. That exclusion is the one property the gate exists to provide. Retain the `Arc<PlatformWallet>` validated under the gate and remove the public-map entry only while it still pointer-matches that generation, so the removed handle, the returned handle and the `tear_down` argument are all the one generation this call validated. The inner-manager removal needs no such check: G1 can only leave `wallet_manager` through this method (which requires G1's gate) or through a rollback for an insert that could not have happened while G1 occupied the id. Regression test `removal_leaves_a_generation_registered_during_it_intact` drives the real `create_wallet_from_seed_bytes` -> `register_wallet` path from a `cfg(test)` rendezvous fired in the exact window, so the interleaving is pinned with no sleep and no completion-order race. Against the previous code it fails on all three load-bearing assertions: the returned generation, the `tear_down` argument, and the survival of the re-registered wallet in the public map. Refs dashpay#4185 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. 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>
540def1 to
8541073
Compare
…#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.
…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>
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