feat(platform-wallet): token-minting finalize from a funding path (spendable DashPay receival accounts) - #4256
Conversation
…BIP70 deferred submission BIP70/BIP270 (CTX/DashSpend) sends must sign, POST the raw bytes to a merchant server, and broadcast only on ack — structurally impossible on the one-shot `sendToAddresses`. Expose the existing internal build/broadcast split with an explicit reservation lifecycle, keeping `CoreTransactionBuilder` internal so the manager stays the sole driver of the setFunding/buildSigned race. Rust core (rs-platform-wallet): - New `SignedPaymentRegistry`: a generic, in-memory registry that owns a built+signed tx and its held UTXO reservation between build and submission, keyed by an opaque `ReservationToken`. `broadcast` removes the entry before sending (no double-broadcast — a repeat/concurrent call gets `StaleToken`), binds each token to its originating wallet instance (`Arc::ptr_eq` on the shared `WalletManager`, so a re-created wallet is rejected), and reconciles the reservation on failure via the existing release-on-rejection path. `release` is idempotent. Reservations are memory-only, so a crash between build and broadcast drops both the entry and the reservation on restart — the same property dashj has. - `CoreWallet::release_transaction_reservation` — the explicit "abandoned / nacked" release arm. FFI (platform-wallet-ffi) — additive C ABI: - `core_wallet_transaction_get_bytes`, `core_wallet_signed_payment_register` (token + fee + txid), `core_wallet_signed_payment_broadcast`, `core_wallet_signed_payment_release`, backed by one process-global registry pinned to `SpvBroadcaster`. - New `ErrorStaleReservationToken` (22) result code. JNI (rs-unified-sdk-jni) — additive: `coreTransactionGetBytes`, `coreWalletRegisterSignedPayment` (BLOB), `coreWalletBroadcastSignedPayment`, `coreWalletReleaseSignedPayment`. Kotlin — additive: `ManagedPlatformWallet.SignedCoreTransaction`, `buildSignedPayment` (build under coreSendMutex), `broadcastSigned(token)`, `releaseReservation(token)`; `DashSdkError.PlatformWallet.StaleReservationToken`. No existing signatures change. Refs dashpay#4089, dashpay/dash-wallet#1507 Phase 5c GAP-4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er/release Address review of the SignedPaymentRegistry deferred build→broadcast/release flow. BLOCKING: registry tokens never expired even though the key-wallet UTXO reservation they depend on is swept after RESERVATION_TTL_BLOCKS (24) and released by raw outpoint with no ownership check, so a long-outstanding token's broadcast/release could free or spend against an unrelated newer reservation. Bound the token lifetime: capture the wallet's synced height at register and refuse broadcast/release once the wallet has synced RESERVATION_MAX_AGE_BLOCKS (20, < TTL) past it, returning the typed StaleReservationToken WITHOUT releasing (which could free a newer build's reservation). The pinned key-wallet exposes no per-outpoint generation check, so this client-side bound is the primary guard. Also: - WalletMismatch now compares wallet_id in addition to Arc::ptr_eq on the shared WalletManager, so two wallets in one multi-wallet manager are told apart. - register() returns the raw tx bytes in the same native call and the JNI folds them into the register BLOB; the now-unused core_wallet_transaction_get_bytes / coreTransactionGetBytes is removed (one native round trip per kotlin-sdk rule). - register() does its fallible/pure marshalling before the reservation-holding insert, and the JNI releases the token if it can't hand the BLOB back to Kotlin — no orphaned reservation on a marshalling failure. - PlatformWallet teardown sweeps the registry of that wallet's tokens so a destroyed wallet's WalletManager is no longer pinned alive by a captured CoreWallet clone (hooked at platform_wallet_destroy, not the transient core-handle destroy the deferred flow cycles through). - Registry mutex recovers from poisoning instead of panicking, matching key-wallet's ReservationSet. Adds tests for token expiry (broadcast + release), same-manager different wallet_id mismatch, and the teardown sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dk-and-example-app Rebasing the split build/broadcast work onto the current base surfaced three semantic collisions the textual merge could not catch: - error code 22 was reassigned on base (ErrorCoreInsufficientFunds and the asset-lock family 22-25); moved ErrorStaleReservationToken to the next free code 26 in platform-wallet-ffi and DashSdkError's native-code mapping. - base added its own CoreWallet::release_transaction_reservation (taking AccountTypePreference, superset incl. CoinJoin) for the finalized-transaction abandon path, colliding with this PR's identically-named StandardAccountType method. Renamed this PR's deferred-payment release to release_payment_reservation (sole caller: SignedPaymentRegistry::release). - base removed the per-wallet coreSendMutex and now serializes/gates core sends through the TeardownGate (gate.op), moving send concurrency safety into the Rust reservation layer. buildSignedPayment now opens with gate.op like its sibling sendToAddresses instead of the removed mutex, which also satisfies the GateCoverageLintTest handle-borrowing fence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n mapping The rebase onto feat/kotlin-sdk-and-example-app reassigned native code 22 to ErrorCoreInsufficientFunds and moved ErrorStaleReservationToken to code 26 (on both the Rust enum and DashSdkError's mapping), but DashSdkErrorTest still constructed code 22 and asserted StaleReservationToken. That deterministically resolved to CoreInsufficientFunds, so platformWalletCodesMapToPlatformWalletSubtree failed and :sdk:testDebugUnitTest — the "Kotlin SDK build + tests (x86_64 emulator)" CI job — went red without actually verifying the code-26 mapping. Point the assertion at code 26 so it exercises the real production mapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s own height clock The SignedPaymentRegistry age guard stamped registered_height with CoreWallet::synced_height() and compared it against a later synced_height(), while the funding reservation it is meant to stay under is stamped with last_processed_height() (the height finalize_transaction / build_signed pass to set_current_height, and the clock key-wallet's ReservationSet TTL sweeps against). synced_height can regress during a rescan while last_processed_height is monotonic, so measuring the reservation's age against synced_height could let a token outlive its reservation and act on an outpoint key-wallet had already swept and re-selected for an unrelated build. Read last_processed_height() for both the registration stamp and the current comparison so the guard measures the same clock the reservation is stamped with, trips strictly before the underlying TTL, and never regresses. Add CoreWallet::last_processed_height(); drop the now-unused synced_height(). The registry's expiry tests now stamp and advance last_processed_height to match production, and outstanding() is exposed under test-utils for downstream FFI tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llet alias is destroyed platform_wallet_destroy unconditionally called remove_entries_for_wallet, which matches every registry entry sharing the destroyed handle's WalletManager pointer + wallet_id. But platform_wallet_manager_get_wallet hands out an independent handle per alias of the same logical wallet (the loadPersistedWallets path can publish a new wrapper while callers still hold an older one). Destroying one alias therefore consumed a sibling alias's still-live deferred-payment token: the sibling's later broadcast failed as stale while the sweep left the UTXO reserved until its TTL. Gate the sweep on final-alias liveness: after removing this handle, scan the remaining PlatformWallet handles for one that shares the same (WalletManager pointer + wallet_id) — exactly the key remove_entries_for_wallet matches. While a sibling is live the destructor only drops this handle; the sweep runs (releasing the registry's WalletManager pin) only once the last alias goes. Adds HandleStorage::any for the scan and a test_support helper that builds real PlatformWallet aliases; a new FFI test proves a sibling alias's token survives one alias's destruction and is swept when the final alias is destroyed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d-register path buildSignedPayment funded, signed, and registered a deferred payment as three separate native round-trips (setFunding + buildSigned + registerSignedPayment). Once the base branch removed the per-wallet coreSendMutex in favour of the TeardownGate — which only counts active ops for safe teardown and does not serialize sends — that split lost its atomic select-and-reserve boundary: two concurrent deferred builds, or a deferred build racing an immediate send, could select the same UTXO before either reserved it and return two signed transactions spending the same input. Restore atomicity in the Rust reservation layer, the correct home now that the Kotlin mutex is gone: add core_wallet_signed_payment_finalize, which runs the same finalize_transaction the immediate V2 path uses — selection and ReservationSet insertion commit as one unit under the wallet-manager lock, signing only after the lock drops — and then registers the built, reserved tx in the same call. buildSignedPayment now issues that single native operation (CoreTransactionBuilder.finalizeSignedPayment + coreWalletFinalizeSignedPayment), so the select+reserve window can no longer interleave. The existing concurrent_same_account_finalizers_cannot_reserve_the_same_input test already covers the atomic boundary the deferred path now shares. The deprecated split wrappers remain but are no longer on the deferred path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After finalize routing landed, the four-layer deferred-register chain core_wallet_signed_payment_register (FFI) → coreWalletRegisterSignedPayment (JNI) → WalletManagerNative.coreWalletRegisterSignedPayment (Kotlin) → ManagedCoreWallet.registerSignedPayment had zero callers. It is the unsafe variant whose age guard baselines registered_height at registration time (after external signing) rather than at the reservation's own height, so removing it also removes that mis-baselined path. The atomic core_wallet_signed_payment_finalize path is the only remaining register site. Delete all four layers; repoint the surviving broadcast/finalize doc comments at the finalize entry point; drop the now-unused FFICoreTransaction::fee accessor (keep the ABI field, silence the lint). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eservation height finalize_transaction captures last_processed_height inside the funding critical section and stamps the selected inputs' reservation with it, then signs after dropping the wallet-manager lock. The registry, however, sampled a FRESH last_processed_height in register() — run AFTER the (possibly slow, external) signer returned. A slow signer could let the wallet advance so the token's baseline was higher than the reservation's true stamp height, making the age guard measure from the wrong side of signing: the token looked young while its reservation had already aged toward key-wallet's TTL sweep, risking a release/broadcast against an outpoint key-wallet had swept and re-selected. Carry the stamp height on SignedCoreTransaction (reservation_height, captured in the funding section before signing) and have register() take the height as an explicit parameter instead of sampling. The atomic finalize FFI passes finalized.reservation_height(); the age guard now baselines on the same clock the reservation was stamped with. Adds a regression test that registers after the wallet advanced (modelling a slow signer) and proves the guard trips MAX_AGE past the reservation height, not past a post-signing sample. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion identity The V2 finalized-transaction handle validated only wallet_id, while the registry-token path validated the shared WalletManager Arc plus wallet_id. Neither can tell one wallet generation from another: after a wallet is removed and re-created under the same id, both the manager Arc and wallet_id are equal, so an old V2 handle could act through the old generation while the new generation selects the same inputs. Add CoreWallet::is_same_generation — the single generation identity both paths now share. Aliases of one generation share the per-generation Arc<WalletBalance> (created fresh in the wallet-lifecycle create/load paths); a re-created wallet gets a new one, so Arc::ptr_eq on it distinguishes generations that wallet_id + the manager Arc cannot. Holding either handle pins the balance Arc, so its address can't be reused for a different generation — the same soundness argument the registry already uses for the manager Arc. Apply it to both V2 broadcast and abandon (replacing the wallet_id-only check). The registry broadcast path adopts the same identity in the follow-up validate-under-lock change. Adds a unit test proving alias-vs-recreation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…only a match SignedPaymentRegistry::broadcast removed the entry first and validated the wallet binding second, so a mismatched caller (wrong wallet, or a re-created generation) destroyed the ORIGINAL wallet's token and left its reservation stranded until the TTL backstop — a wrong-wallet broadcast could grief the rightful owner's in-flight payment. Peek under the registry lock, reject a non-matching caller with WalletMismatch WITHOUT removing the entry, and only remove (consume) an entry whose generation matches. The check-then-remove is one lock hold, so it stays atomic against a concurrent broadcast — the double-broadcast guard is unchanged (the second consumer finds nothing → StaleToken). The binding check now uses the shared CoreWallet::is_same_generation identity, so the registry-token and V2 handle paths agree on when a caller owns a token. Updates the two existing mismatch tests (which asserted the old drop-on-mismatch behaviour) and adds a regression proving a wrong-wallet broadcast preserves the owner's token and the owner can still broadcast it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…; drop them at generation teardown platform_wallet_destroy called remove_entries_for_wallet, which only DROPPED the registry entries. But destroying the last wrapper alias does not remove the logical wallet from its manager — the accounts' ReservationSets stay live and the same wallet can be handed out again — so the dropped tokens' inputs stayed reserved until key-wallet's TTL. Tokens were consumed without releasing live reservations. Split the two teardown moments under one generation identity: - Final-alias destroy (wallet still live): release_entries_for_wallet RELEASES each of the generation's reservations against the still-live wallet (honouring the age guard), so a wallet handed out again can respend the inputs. The final-alias check and the match are both by CoreWallet::is_same_generation. - Actual generation teardown (platform_wallet_manager_remove_wallet): the wallet and its ReservationSets are gone, so remove_entries_for_wallet DROPS the generation's registry tokens (nothing to reconcile) and remove_matching drops its finalized-tx V2 handles. This makes any stale handle to the removed generation inert, which is what makes the destroy-time release provably race-free: a torn-down generation has already had its tokens swept here, so destroy/release can never release-by-outpoint against a re-created generation's inputs. platform_wallet_destroy now block_on's the release (as it already runs off the tokio runtime on the JNI / NativeCleaner threads). Adds HandleStorage::remove_matching, registry release_entries_for_wallet, a registry regression proving destroy-time release frees the reservation while teardown drop does not, and reworks the FFI destroy test to invoke destroy off-runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hree siblings Native code 26 (ErrorStaleReservationToken) mapped all three SignedPaymentError variants — StaleToken (unknown/already-broadcast/released), WalletMismatch (different wallet generation), and StaleReservationToken (aged out) — so a host could not tell "you already broadcast this", "wrong wallet", and "the reservation aged out; rebuild" apart, even though the remedy and messaging differ. Split at the FFI (additive sibling codes, no renumbering): - 26 ErrorStaleReservationToken -> StaleReservationToken (aged out) - 27 ErrorReservationTokenConsumed -> StaleToken (unknown/already broadcast/released) - 28 ErrorReservationWalletMismatch -> WalletMismatch (different generation) core_wallet_signed_payment_broadcast now maps each variant to its own code. All three remain non-retryable-in-place and none touch the network. Host impact (Kotlin SDK only — the Swift host does not map these codes): adds DashSdkError.PlatformWallet.ReservationTokenConsumed / ReservationWalletMismatch, maps 27/28, narrows the code-26 doc, updates the JNI/Kotlin broadcast KDocs, and extends DashSdkErrorTest to assert all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-register shape The KDoc still described the pre-finalize build (`new → addOutput* → setFunding → buildSigned`) and credited buildSigned with reserving the inputs. The deferred path now issues a single atomic finalizeSignedPayment (select + reserve + sign + register under the wallet-manager lock). Update the described step sequence and the atomicity claim to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ble with a Cleaner backstop buildSignedPayment returned a plain SignedCoreTransaction through a cancellable coroutine. The blocking JNI registration mints the reservation token before the Kotlin object exists, so if cancellation was observed after that native call returned — or the caller simply dropped the value — the token (and its funding reservation) was orphaned until key-wallet's TTL, with no release path. Make SignedCoreTransaction an AutoCloseable that registers a NativeCleaner backstop at construction: close(), or GC if the caller never calls it, releases the token exactly once. Native release is idempotent and tokens are process-unique, so releasing a token already consumed by broadcastSigned / releaseReservation (or closing twice) is a harmless no-op. This closes the cancellation window — the object is Cleaner-backed the instant it exists (no suspension point between the native return and construction), so a discarded object always releases its token. Adds a pure-JVM test pinning the ownership contract (owning AutoCloseable) and the Cleaner run-once guarantee it relies on, and documents the ownership on buildSignedPayment. :sdk:testDebugUnitTest passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etion Normalize two pre-existing long lines in coreWalletFinalizeSignedPayment that `cargo fmt --check` flags, so the JNI crate is formatting-clean after the register-chain removal touched this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…CoreTransaction Review round 2: the bare-Long token API couples the reservation's lifetime to the SignedCoreTransaction's GC-reachability — extracting the token and dropping the object lets the Cleaner backstop release the reservation out from under a pending broadcast. The object overloads keep the payment reachable across the native call (reachabilityFence) and disarm the backstop once the token is consumed; the bare-token docs now warn about the reachability requirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed deferred payments The deferred-payment registry stored only an `Option<StandardAccountType>`, so a CoinJoin funding — which has no `StandardAccountType` — reconciled nothing on rejection/abandon/free and kept its inputs reserved until key-wallet's 24-block TTL, even though `finalize` reserves the selected inputs for every account variant. Carry the full `AccountTypePreference` (BIP44/BIP32/CoinJoin) as the entry's releasable account handle. The registry now broadcasts through the new `broadcast_payment_releasing_reservation` and releases through `release_transaction_reservation` (both `AccountTypePreference`-typed and CoinJoin-capable), so a rejected or abandoned CoinJoin deferred payment frees its reservation immediately. The FFI finalize passes `account_type.into()` instead of the `StandardAccountType` subset; the now-unused `release_payment_reservation` (registry-only) is removed. Test: `coinjoin_funded_release_frees_the_reservation_immediately` funds CoinJoin account 0, finalizes a sweep, registers the token, and proves release makes the input immediately spendable again. Adds a `#[cfg(test)]` `funded_coinjoin_wallet_manager` fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… wallet generation The deferred registry validated a token's generation at the registry lock, then released its reservation later, off that lock. `ReservationSet::release` removes an outpoint unconditionally and is reached via `wallet_id` — an identity a same-id remove-then-recreate preserves — so a wallet re-created in that window could have the NEW generation's reservation freed by the old token's cleanup. Bind the cleanup to the token's own generation: `release_transaction_reservation` now re-validates the generation and mutates the `ReservationSet` under a single manager read-lock hold, acting only when the wallet still registered under the id carries the same per-generation balance `Arc` the handle captured. A recreation needs the manager write lock, so it cannot interleave between the check and the release — validate-and-mutate is atomic. This protects both the registry (release/abandon and broadcast-on-rejection) and the V2 finalized-transaction handle path, which share this primitive. Adds `CoreWallet::generation()`. Test: `recreation_between_validation_and_cleanup_cannot_release_new_generation` recreates the wallet under the same id between registration and release and asserts the input stays reserved (the reservation the new generation owns is untouched). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`PlatformWalletResultCode` jumped from 25 straight to 98, so the three deferred build->broadcast/release codes this PR owns (26 StaleReservationToken, 27 ReservationTokenConsumed, 28 ReservationWalletMismatch) fell through to `.errorUnknown` on iOS, erasing their distinct retry semantics. Add the three raw codes to `PlatformWalletResultCode`, matching cases to `PlatformWalletError`, and map them in both `init(ffi:)` and `init(result:)`. The `init(result:)` switch (no default) stays exhaustive — the same non-exhaustive-switch class shumkov flagged on dashpay#4184. Messages pass the Rust `Display` string straight through, matching the Kotlin SDK's mapping verbatim. Verified with `swiftc -parse` (the DashSDKFFI xcframework — cbindgen header + cdylib — is built separately by build_ios.sh and is not present in this checkout, so a full `swift build` type-check isn't possible here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…error codes `core_wallet_signed_payment_broadcast` still documented the pre-split semantics: a repeated broadcast and a re-created wallet both as `ErrorStaleReservationToken` (26). Since the three-way split, a repeated/concurrent broadcast yields `ErrorReservationTokenConsumed` (27) and a re-created wallet generation yields `ErrorReservationWalletMismatch` (28); 26 is reserved for the aged-out case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Point all rust-dashcore crates at bfoss765/rust-dashcore e99959ced0062159d629930f488374e29f63c42b (PR dashpay/rust-dashcore#916), which is v4.1-dev's rust-dashcore tip 70d4bf8 plus the additive owner-tagged reservation API: key_wallet::ReservationToken, ReservationSet::reserve/release_if_owner, TransactionBuilder::build_{unsigned,signed}_reserved, ManagedCoreFundsAccount::release_reservation_if_owner, and AssetLockResult.reservation_token. Additive over 70d4bf8, so it stays compatible with the rest of the v4.1-dev workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
v4.1-dev added ErrorTransactionBroadcastRejected = 26, colliding with this PR's three deferred-reservation siblings that also claimed 26/27/28. Keep v4.1-dev's 26 and shift this PR's codes up by one: 27 = ErrorStaleReservationToken (was 26) 28 = ErrorReservationTokenConsumed (was 27) 29 = ErrorReservationWalletMismatch (was 28) 29 is free on v4.1-dev (dashpay#4184's AssetLockInsufficientFunds is not yet merged there). The Rust FFI enum and Swift bindings were renumbered in the rebase conflict resolution; this finishes the propagation through the Kotlin runtime mapping and KDoc (DashSdkError.kt, WalletManagerNative.kt), the Kotlin error-code test, and the signed_payment FFI doc comments (also recast from fix-round narration to an as-built description). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lease A deferred send reserves its funding inputs at build, awaits the broadcast, and on a definitive rejection releases the reservation for an immediate rebuild. That release was unconditional (release-by-outpoint): during the broadcast await, key-wallet's TTL sweep can reclaim the reservation and a concurrent build can re-reserve the same outpoint under a new token, so the by-outpoint release would free that other build's inputs — the dashpay#4185 release/re-reserve double-spend window. Capture the key_wallet::ReservationToken build_unsigned_reserved stamps onto the selected inputs, carry it on SignedCoreTransaction alongside reservation_height, thread it through the deferred registry (RegisteredPayment / register / broadcast / reconcile) and broadcast_payment_releasing_reservation, and release via ManagedCoreFundsAccount::release_reservation_if_owner so a rejected or abandoned send frees only inputs its own build still owns. The finalize sign-failure path (a platform-side await between reserve and release) is owner-guarded the same way. None (no reservation taken) keeps the old by-outpoint fallback, never reached on the funded finalize path. Adds a regression test: a rejected deferred broadcast whose outpoint was swept and re-reserved under a new token leaves that new reservation intact. Docs name the shared generation identity's sibling V2 handle path (dashpay#4196). Co-Authored-By: Claude Fable 5 <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>
…endable DashPay receival accounts) Bridges the two previously-disconnected flows: build_signed_payment's single-account-by-derivation-path selection (the only path that can reach a dashpay_receival account) and the reservation/broadcast flow (token -> broadcastSigned/releaseReservation). New finalize_signed_payment_from_funding_path returns FinalizedCorePayment (tx, fee, change, resolved FundingAccountRef, reservation token); the reservation is recorded against the RESOLVED account (never a BIP44 default), so release/broadcast bookkeeping lands in the ledger the build reserved into. FundingAccountRef::Path makes non-standard accounts nameable to the release machinery. FFI core_wallet_build_signed_payment_with_token + JNI + Kotlin buildSignedPaymentWithToken(recipients, coreSignerHandle, feePerKb, fundingPath) returning the existing SignedCoreTransaction. Invariants preserved and tested: single account only (funding-privacy guardrails pass), change to BIP44/0, watch-only refused, fee from the signed tx. 3 new tests incl. reservation hold/release cycles; full send suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ceival finalize tests The receival funding-path tests need a wallet whose balance is split between BIP44 account 0 and a DashPay funds account, which `WalletAccountCreationOptions::Default` does not provision. Ports the `DashpayLeg` / `foreign_contact_account_xpub` / `split_funded_wallet_manager_dashpay` test_support fixture verbatim from the asset-lock multi-account work (dashpay#4184, commit 4655eef) so this PR does not have to stack on 2.9k lines of unrelated asset-lock production code just to reach the fixture. Test-support only — no production code is taken from dashpay#4184. The three items are `#[cfg(test)]`-gated here (unlike on dashpay#4184, where the asset-lock `test-utils` build also consumes them) because only this crate's own unit tests use them; ungated they would trip `dead_code` in a `test-utils`-only build of the FFI crate. When dashpay#4184 and this PR both land, test_support.rs will conflict on these three items; resolve by keeping a single copy (drop the `#[cfg(test)]` gates, since dashpay#4184 widens the consumer set). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
⛔ Blockers found — Sonnet deferred (commit 1a7a4b6) |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds a deferred "build-sign-later-broadcast" Core L1 payment workflow across the Rust wallet core, FFI, JNI, Kotlin SDK, and Swift SDK, using opaque reservation tokens. It adds a signed-payment registry, generation-bound wallet identity checks, funding-domain isolation guardrails, new error codes, and a dependency revision bump. ChangesDeferred Signed Core Payment Workflow
Dependency Revision Bump
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant KotlinSDK as Kotlin/Swift SDK
participant FFI as Rust FFI
participant Registry as SignedPaymentRegistry
participant Network
Client->>KotlinSDK: buildSignedPaymentWithToken(recipients)
KotlinSDK->>FFI: core_wallet_build_signed_payment_with_token
FFI->>Registry: register(signed tx, reservation)
Registry-->>FFI: token
FFI-->>KotlinSDK: token, txid, fee, change
KotlinSDK-->>Client: SignedCoreTransaction
Client->>KotlinSDK: broadcastSigned(transaction)
KotlinSDK->>FFI: core_wallet_signed_payment_broadcast(token)
FFI->>Registry: broadcast(token)
Registry->>Network: send transaction bytes
Network-->>Registry: accepted or rejected
Registry-->>FFI: Txid or SignedPaymentError
FFI-->>Client: txid or typed error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The single-account funding and owner-guarded reservation plumbing is generally coherent, but the fee cap relies on the 100,000-byte standard transaction limit without enforcing that limit. Oversized recipient lists can therefore overflow key-wallet's fee arithmetic, so this PR requires changes; several additional lifecycle, concurrency, FFI, test, and documentation issues remain as non-blocking follow-ups.
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)
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/send.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:295-306: Fee cap does not prevent overflow for oversized recipient lists
`MAX_FEE_PER_KB` is safe only when the transaction is at most 100,000 bytes, but this method never enforces that size limit. The pinned key-wallet builder includes roughly `outputs.len() * 34` in its base size and then evaluates `sat_per_kb * size_bytes` with unchecked `u64` multiplication. At the accepted maximum rate, about 25,835 outputs produce an estimated size around 878,434 bytes and overflow that multiplication. Such a recipient list fits in a practical JNI blob. Overflow-checking builds can panic inside the `extern "C"` path, while release builds wrap the fee and may return a signed, non-standard transaction with a fee unrelated to the requested rate. Reject transactions whose estimated size exceeds the standard limit before invoking key-wallet, and ensure the final selected-input size is also bounded or calculated with checked arithmetic.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:308-311: Wallet-manager write lock remains held while awaiting the signer
`wm` is a named `tokio::sync::RwLockWriteGuard` that remains live through the later `build_signed_reserved(...).await` at lines 509-511. The selected account borrow ends before signing, but the manager's exclusive lock does not. A slow hardware or keystore signer therefore blocks wallet synchronization and every other manager read or build; a signer that re-enters this wallet can deadlock. Follow `finalize_transaction`'s established pattern: assemble and reserve the unsigned transaction inside a scoped manager lock, drop the guard, then await signing while retaining the owner token needed to release the reservation on failure.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:116-140: FinalizedCorePayment does not enforce its reservation lifecycle
This type represents a linear obligation: exactly one registration or abandonment should take responsibility for its held reservation. However, it derives `Clone`, exposes all coupled bookkeeping fields publicly, and `abandon_payment` only borrows it. Safe callers can abandon one clone and register another, register the same payment more than once, or pair the transaction with an unrelated funding path or owner token. That can expose a registered payment after its inputs have already become selectable or mint multiple registry tokens capable of broadcasting the same transaction. Make the finalized payment opaque and non-`Clone`, and expose consuming registration and abandonment operations that accept the payment as one inseparable value.
In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet.rs:395-427: Wallet cleanup is not atomic with deferred-payment registration
Handle removal, the final-alias check, and the registry sweep use separate synchronization boundaries. A concurrent finalize can clone the wallet before destruction, then insert its registry entry after `release_entries_for_wallet` has collected matching entries, leaving a token and reservation behind after the final alias is gone. The opposite ordering can let cleanup remove a newly registered entry while the finalize call is still returning its token, so the caller receives an already-stale token. `platform_wallet_manager_remove_wallet` has the analogous remove-then-sweep window. Coordinate generation closing state, registration, alias counts, and cleanup under one lifecycle boundary, or make registration atomically reject a generation that has begun closing.
In `packages/rs-platform-wallet-ffi/src/core_wallet/send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/send.rs:246-254: Initialize all owned C out-parameters before fallible work
Only `out_token` receives a sentinel value before parsing and wallet operations. Failures from funding-path parsing, handle lookup, recipient decoding, or signing leave `out_txid`, `out_tx_bytes`, `out_tx_len`, `out_fee`, and `out_change` unchanged. The JNI caller happens to pass zero-initialized locals, but this `extern "C"` API is independently public; a generated C or Swift caller that performs cleanup on an error can observe a stale pointer/length pair and free or consume old output state. After validating all out-pointers, initialize the owned pointers to null and all scalar outputs to zero before any fallible operation.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:293-310: Add coverage for the change-bearing registration blob
The new funding-path API uses a distinct wire layout with `changeDuffs` inserted before the txid, and decoding it with the existing registration decoder silently mis-frames every subsequent field. `SignedCoreTransactionTest` exercises only `fromRegisterBlob`; it never calls `fromPaymentRegisterBlob` or verifies `changeDuffs`. Add a focused decoder test using the JNI packing order and assert the token, fee, change, txid, and raw transaction bytes.
| // Bound the caller-supplied fee rate for the same reason: key-wallet's | ||
| // `FeeRate::calculate_fee` computes `sat_per_kb * size_bytes` with | ||
| // unchecked `u64` multiplication, so a rate near `u64::MAX` (the public | ||
| // Kotlin/FFI APIs accept any non-negative `Long`) panics in an | ||
| // overflow-checking Android build, or wraps in release — turning an | ||
| // astronomical requested rate into a tiny fee. | ||
| let fee_per_kb = fee_per_kb.unwrap_or(DEFAULT_FEE_PER_KB); | ||
| if fee_per_kb > MAX_FEE_PER_KB { | ||
| return Err(PlatformWalletError::TransactionBuild(format!( | ||
| "fee rate {fee_per_kb} duffs/kB exceeds the maximum {MAX_FEE_PER_KB}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Fee cap does not prevent overflow for oversized recipient lists
MAX_FEE_PER_KB is safe only when the transaction is at most 100,000 bytes, but this method never enforces that size limit. The pinned key-wallet builder includes roughly outputs.len() * 34 in its base size and then evaluates sat_per_kb * size_bytes with unchecked u64 multiplication. At the accepted maximum rate, about 25,835 outputs produce an estimated size around 878,434 bytes and overflow that multiplication. Such a recipient list fits in a practical JNI blob. Overflow-checking builds can panic inside the extern "C" path, while release builds wrap the fee and may return a signed, non-standard transaction with a fee unrelated to the requested rate. Reject transactions whose estimated size exceeds the standard limit before invoking key-wallet, and ensure the final selected-input size is also bounded or calculated with checked arithmetic.
source: ['codex']
There was a problem hiding this comment.
Fixed in 34c0894. The finding was exactly right — MAX_FEE_PER_KB = MAX_MONEY / 100 was sound only under a 100 kB assumption the method never enforced, and ~25.8k recipients does fit in a practical JNI blob.
Three parts, in wallet/core/send.rs:
-
The size limit is now enforced at build time.
MAX_STANDARD_TX_SIZEis 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 —8 + 1 + varint + outputs*34 + 34, taking the varint at its 9-byte maximum so the estimate never comes in under key-wallet's — and requiring room for at least one 148-byte input. This runs before the wallet lock and before anything is reserved. -
MAX_FEE_PER_KBis re-derived asu64::MAX / u32::MAX. This is the part that does not depend on the input count, and I think it's needed independently of (1): the size limit bounds the output side, but a funding account holding a few thousand small denominations reaches ~878 kB through the input side with a perfectly ordinary recipient list. Withsat_per_kb ≤ u64::MAX / u32::MAX,sat_per_kb * size_bytesis representable for any size au32can express, which no transaction can exceed. ~43 DASH/kB is three orders of magnitude above the 1_000 default, so nothing legitimate is rejected. -
The final selected-input size is checked, per the last sentence of the finding: the signed transaction is re-measured and refused if over the limit. That refusal discharges the reservation it had already taken through
abandon_payment, rather than stranding the account's coins until the TTL backstop.
Tests:
an_oversized_finalize_is_refused_and_reserves_nothing— the 25,835-recipient case at the maximum accepted rate, on the finalize path, plus the assertion that an ordinary payment still funds immediately afterwards;an_over_limit_signed_transaction_is_refused_and_releases_its_reservation— 900 recipients funded by 500 small UTXOs, which passes the output-side bound and then pulls ~496 inputs to land just over 100 kB. This is the only test that exercises the post-build check, and it doubles as the proof that the manager guard must be released beforeabandon_payment(tokio'sRwLockis not reentrant, so it would deadlock rather than fail);max_fee_rate_cannot_overflow_key_wallets_fee_product— asserts no overflow at 100 kB, at the 878_434 figure from this finding, and atu32::MAX. It fails if a cleanup restoresMAX_MONEY / 100.
There was a problem hiding this comment.
Resolved in 34c0894 — Fee cap does not prevent overflow for oversized recipient lists 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 mut wm = self.wallet_manager.write().await; | ||
| let (wallet, info) = wm | ||
| .get_wallet_and_info_mut(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; |
There was a problem hiding this comment.
🟡 Suggestion: Wallet-manager write lock remains held while awaiting the signer
wm is a named tokio::sync::RwLockWriteGuard that remains live through the later build_signed_reserved(...).await at lines 509-511. The selected account borrow ends before signing, but the manager's exclusive lock does not. A slow hardware or keystore signer therefore blocks wallet synchronization and every other manager read or build; a signer that re-enters this wallet can deadlock. Follow finalize_transaction's established pattern: assemble and reserve the unsigned transaction inside a scoped manager lock, drop the guard, then await signing while retaining the owner token needed to release the reservation on failure.
source: ['codex']
There was a problem hiding this comment.
Deferring this one, and I want to be explicit about why rather than just deferring it — I attempted the change and backed it out.
The obvious form of the fix (drop wm once the builder is constructed, as the sibling CodeRabbit comment proposes) is not safe here, and would introduce a double-spend rather than a latency win. set_funding only snapshots the account's UTXOs and clones the Arc<ReservationSet>; both coin selection and the reserve() call happen inside build_signed_reserved (key-wallet's assemble_unsigned, where reservations.reserve(&outpoints, height) runs after selection). So dropping the guard before that call lets two concurrent finalizes each snapshot the same unreserved UTXO under their own lock hold, then both select and reserve it — two signed transactions spending one coin. It compiles and the single-threaded tests pass, which is exactly why I'm flagging it.
Your own wording is the correct shape — "assemble and reserve the unsigned transaction inside a scoped manager lock, drop the guard, then await signing" — and that's what finalize_transaction does. It isn't reachable from here: key-wallet's assemble_unsigned is private to its module, and there is no build_unsigned_reserved counterpart to build_signed_reserved. Splitting reserve-from-sign on this path needs a key-wallet API addition, which I don't think belongs in this PR.
What did change in 34c0894: the guard is now released immediately after the build rather than at end of scope. That doesn't shorten the signing hold, but it's what makes the new over-limit refusal able to call abandon_payment at all — that re-acquires the manager lock, so under the guard it would deadlock.
Happy to open the key-wallet-side issue for the build_unsigned_reserved split if you want this tracked rather than deferred.
| #[derive(Debug, Clone)] | ||
| pub struct FinalizedCorePayment { | ||
| /// The signed transaction. | ||
| pub transaction: Transaction, | ||
| /// The fee paid, in duffs — derived from the transaction itself | ||
| /// (`inputs − outputs`), the same ground truth [`SignedCorePayment::fee`] | ||
| /// uses. | ||
| pub fee: u64, | ||
| /// Duffs returned to the wallet's BIP44 change address (0 when the build | ||
| /// produced no change output). | ||
| pub change_amount: u64, | ||
| /// The ONE account the inputs were selected from and reserved in, as its | ||
| /// RESOLVED account-level derivation path — never the caller's `None`. A | ||
| /// later release must name this account, not the default BIP44 one. | ||
| pub funding: FundingAccountRef, | ||
| /// The wallet's `last_processed_height` captured in the funding critical | ||
| /// section, i.e. the exact clock `set_current_height` stamped the | ||
| /// reservation with. The registry's age guard must baseline off this — see | ||
| /// [`SignedCoreTransaction::reservation_height`](crate::SignedCoreTransaction::reservation_height). | ||
| pub reservation_height: u32, | ||
| /// The key-wallet [`ReservationToken`](key_wallet::ReservationToken) stamped | ||
| /// onto the selected inputs, so a later release is *owner-guarded* and frees | ||
| /// only inputs this build still owns (`dashpay/platform#4185`). `None` only | ||
| /// if the build reserved nothing, which the funded path never does. | ||
| pub reservation_token: Option<KeyWalletReservationToken>, |
There was a problem hiding this comment.
🟡 Suggestion: FinalizedCorePayment does not enforce its reservation lifecycle
This type represents a linear obligation: exactly one registration or abandonment should take responsibility for its held reservation. However, it derives Clone, exposes all coupled bookkeeping fields publicly, and abandon_payment only borrows it. Safe callers can abandon one clone and register another, register the same payment more than once, or pair the transaction with an unrelated funding path or owner token. That can expose a registered payment after its inputs have already become selectable or mint multiple registry tokens capable of broadcasting the same transaction. Make the finalized payment opaque and non-Clone, and expose consuming registration and abandonment operations that accept the payment as one inseparable value.
source: ['codex']
There was a problem hiding this comment.
Half done in 34c0894, half deferred with a reason.
Done: FinalizedCorePayment no longer derives Clone, and abandon_payment now takes it by value. Those two together close the cases you listed that are expressible today — abandoning one copy and registering another, abandoning twice, or minting two registry tokens able to broadcast the same transaction, all now fail to compile rather than being caught in review. Only two call sites existed (the FFI's CString::new failure arm and the new over-limit refusal), so the change was contained.
Deferred: making the type opaque with consuming registration. The blocker is that register_funded_by is #4185's API, not this PR's, and it takes the pieces separately:
register_funded_by(core, tx, funding, registered_height, funding_reservation_token)That signature is precisely what allows pairing a transaction with an unrelated funding path or owner token — so the real fix is folding those four parameters into one consuming register(payment). Doing that from this branch would fork a sibling PR's public surface while both are in review, and privatising the fields without it would only add accessors and close nothing (the FFI would still hand the four pieces over individually).
I've documented the reasoning on the type itself so the pub fields don't read as an oversight. If you'd rather it land as one change, the natural home is #4185 — happy to take it there.
There was a problem hiding this comment.
Resolved in 34c0894 — FinalizedCorePayment does not enforce its reservation lifecycle 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 Some(wallet) = PLATFORM_WALLET_STORAGE.remove(handle) else { | ||
| return PlatformWalletFFIResult::ok(); | ||
| }; | ||
|
|
||
| // `platform_wallet_manager_get_wallet` hands out an independent handle for | ||
| // each alias of the same wallet *generation* (they share the underlying | ||
| // `WalletManager` `Arc`, `wallet_id`, and the per-generation balance `Arc`). | ||
| // A deferred-payment token minted through one alias must NOT be invalidated | ||
| // when a *sibling* alias of the same generation is destroyed — the token is | ||
| // still live and broadcastable through the survivor. | ||
| // | ||
| // So only reconcile when THIS is the final live alias of the generation: no | ||
| // other stored handle is the same generation | ||
| // (`CoreWallet::is_same_generation`). While a sibling is live, the | ||
| // destructor just drops this handle. | ||
| // | ||
| // Once the last alias goes, RELEASE (not merely drop) each of this | ||
| // generation's deferred-payment reservations: destroying the last wrapper | ||
| // handle does NOT remove the logical wallet from its manager, so the wallet | ||
| // — and its accounts' still-live `ReservationSet`s — remain, and the same | ||
| // wallet can be handed out again. Dropping the tokens without releasing | ||
| // would leave those inputs reserved until key-wallet's TTL. Releasing here | ||
| // also frees the registry's `CoreWallet` pin on the shared `WalletManager`. | ||
| // (Actual generation teardown — `remove_wallet` — instead drops the tokens, | ||
| // since the reservation ceases to exist with the generation.) | ||
| let core = wallet.core(); | ||
| let sibling_alias_alive = | ||
| PLATFORM_WALLET_STORAGE.any(|other| other.core().is_same_generation(core)); | ||
| if !sibling_alias_alive { | ||
| runtime().block_on( | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .release_entries_for_wallet(core), | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Wallet cleanup is not atomic with deferred-payment registration
Handle removal, the final-alias check, and the registry sweep use separate synchronization boundaries. A concurrent finalize can clone the wallet before destruction, then insert its registry entry after release_entries_for_wallet has collected matching entries, leaving a token and reservation behind after the final alias is gone. The opposite ordering can let cleanup remove a newly registered entry while the finalize call is still returning its token, so the caller receives an already-stale token. platform_wallet_manager_remove_wallet has the analogous remove-then-sweep window. Coordinate generation closing state, registration, alias counts, and cleanup under one lifecycle boundary, or make registration atomically reject a generation that has begun closing.
source: ['codex']
There was a problem hiding this comment.
Not changed — this one I don't think I can fix correctly from this PR. The race described is between platform_wallet_destroy's alias/generation bookkeeping and the registry sweep, both of which are #4185's lifecycle machinery; closing it properly means coordinating generation-closing state, registration, alias counts and cleanup under one boundary, which is a change to that PR's design rather than an addition to this one. This PR only adds a new producer of registry entries, so it widens an existing window rather than opening it. Flagging rather than silently deferring — if you'd like it tracked, #4185 is the right home.
There was a problem hiding this comment.
Resolved in this update — Wallet cleanup is not atomic with deferred-payment registration 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.
| check_ptr!(outputs_blob); | ||
| check_ptr!(core_signer_handle); | ||
| check_ptr!(out_token); | ||
| check_ptr!(out_txid); | ||
| check_ptr!(out_tx_bytes); | ||
| check_ptr!(out_tx_len); | ||
| check_ptr!(out_fee); | ||
| check_ptr!(out_change); | ||
| *out_token = 0; |
There was a problem hiding this comment.
🟡 Suggestion: Initialize all owned C out-parameters before fallible work
Only out_token receives a sentinel value before parsing and wallet operations. Failures from funding-path parsing, handle lookup, recipient decoding, or signing leave out_txid, out_tx_bytes, out_tx_len, out_fee, and out_change unchanged. The JNI caller happens to pass zero-initialized locals, but this extern "C" API is independently public; a generated C or Swift caller that performs cleanup on an error can observe a stale pointer/length pair and free or consume old output state. After validating all out-pointers, initialize the owned pointers to null and all scalar outputs to zero before any fallible operation.
source: ['codex']
There was a problem hiding this comment.
Not changed in this round — agreed in principle, deferring on scope. The finding is accurate that only out_token gets a sentinel and that the in-repo JNI caller zero-initialises its locals, so there is no live defect. It's a real hardening gap for an independently-public extern "C" surface though. Grouping it with the sibling transaction_builder.rs / signed_payment.rs instances CodeRabbit raised, since they're the same one-line convention (core_wallet_broadcast_signed_transaction_v2 already does it) and are better fixed together than piecemeal across three PRs. Say the word if you'd rather it land here.
There was a problem hiding this comment.
Resolved in 34c0894 — Initialize all owned C out-parameters before fallible work 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.
| internal fun fromPaymentRegisterBlob(blob: ByteArray): SignedCoreTransaction { | ||
| val buffer = java.nio.ByteBuffer.wrap(blob) // big-endian by default | ||
| val token = buffer.long | ||
| val feeDuffs = buffer.long | ||
| val changeDuffs = buffer.long | ||
| val txidLen = buffer.int | ||
| val txidBytes = ByteArray(txidLen) | ||
| buffer.get(txidBytes) | ||
| val txBytesLen = buffer.int | ||
| val rawTxBytes = ByteArray(txBytesLen) | ||
| buffer.get(rawTxBytes) | ||
| return SignedCoreTransaction( | ||
| txidHex = String(txidBytes, Charsets.UTF_8), | ||
| rawTxBytes = rawTxBytes, | ||
| feeDuffs = feeDuffs, | ||
| reservationToken = token, | ||
| changeDuffs = changeDuffs, | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Add coverage for the change-bearing registration blob
The new funding-path API uses a distinct wire layout with changeDuffs inserted before the txid, and decoding it with the existing registration decoder silently mis-frames every subsequent field. SignedCoreTransactionTest exercises only fromRegisterBlob; it never calls fromPaymentRegisterBlob or verifies changeDuffs. Add a focused decoder test using the JNI packing order and assert the token, fee, change, txid, and raw transaction bytes.
source: ['codex']
There was a problem hiding this comment.
Not added in this round. Agreed the gap is real — SignedCoreTransactionTest only exercises fromRegisterBlob, and the payment blob's distinct layout (changeDuffs before the txid) would mis-frame every subsequent field if the wrong decoder were used. Deferring only because the Rust-side changes in this round were the funds-critical ones and I did not want to mix a Kotlin decoder test into the same commit. Noting it explicitly so it isn't lost: the test wants to assert token, fee, change, txid and raw bytes against the JNI packing order.
| /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) | ||
| /// could not cover the requested outputs plus fee from the union of the | ||
| /// wallet's *signable* funds accounts (BIP44 + BIP32 + CoinJoin + DashPay | ||
| /// receiving; watch-only DashPay external accounts are excluded). `available` | ||
| /// is the total selectable value across those accounts, `required` the | ||
| /// outputs-plus-fee target — carried as exact duff amounts (instead of being | ||
| /// flattened into a string) so callers can render a precise shortfall. |
There was a problem hiding this comment.
💬 Nitpick: Insufficient-funds error still documents union funding
The implementation deliberately confines selection to one caller-selected account, and map_send_builder_error reports that account's selectable value. This public error documentation still states that available is a union across all signable accounts, which gives downstream callers the opposite contract and may cause UI or retry logic to imply that privacy domains will be combined automatically.
| /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) | |
| /// could not cover the requested outputs plus fee from the union of the | |
| /// wallet's *signable* funds accounts (BIP44 + BIP32 + CoinJoin + DashPay | |
| /// receiving; watch-only DashPay external accounts are excluded). `available` | |
| /// is the total selectable value across those accounts, `required` the | |
| /// outputs-plus-fee target — carried as exact duff amounts (instead of being | |
| /// flattened into a string) so callers can render a precise shortfall. | |
| /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) | |
| /// could not cover the requested outputs plus fee from the single funding | |
| /// account selected by the caller. `available` is the total selectable value | |
| /// in that account, and `required` is the outputs-plus-fee target — carried | |
| /// as exact duff amounts so callers can render a precise shortfall without | |
| /// implying that another funding domain will be used automatically. |
source: ['codex']
There was a problem hiding this comment.
Same item as the CodeRabbit thread on error.rs — fixed in 34c0894. The variant doc now states single-account available/required and drops the union language.
There was a problem hiding this comment.
Resolved in 34c0894 — Insufficient-funds error still documents union funding 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/rs-platform-wallet/src/test_support.rs (1)
265-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated wallet-assembly tail into a shared helper.
funded_coinjoin_wallet_manager,split_funded_wallet_manager, andsplit_funded_wallet_manager_dashpayeach end with the same sequence: buildWalletSigner, wrapWalletBalance, assemblePlatformWalletInfo, createWalletManager::<PlatformWalletInfo>::new, andinsert_wallet. This mirrors the pre-existing tail infunded_wallet_manager_with_outputs(lines 247-263). As this fixture module keeps growing (four near-identical tails now), extracting a small private helper that takes the fundedTestWalletContextand returns the assembled manager/wallet-id/balance/signer tuple would reduce duplication and keep future fixtures consistent.♻️ Proposed helper extraction
fn finalize_test_wallet( ctx: TestWalletContext, ) -> ( Arc<RwLock<WalletManager<PlatformWalletInfo>>>, WalletId, Arc<WalletBalance>, WalletSigner, ) { let signer = WalletSigner { wallet: ctx.wallet.clone(), }; let balance = Arc::new(WalletBalance::new()); let info = PlatformWalletInfo { core_wallet: ctx.managed_wallet, balance: Arc::clone(&balance), identity_manager: IdentityManager::new(), tracked_asset_locks: BTreeMap::new(), }; let mut wm = WalletManager::<PlatformWalletInfo>::new(Network::Testnet); let wallet_id = wm.insert_wallet(ctx.wallet, info).expect("insert wallet"); (Arc::new(RwLock::new(wm)), wallet_id, balance, signer) }Callers that only need a 3-tuple (
split_funded_wallet_manager,split_funded_wallet_manager_dashpay) can drop thebalanceelement.Also applies to: 336-422, 467-642
🤖 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/test_support.rs` around lines 265 - 334, Extract the repeated wallet-assembly logic from the tail of funded_coinjoin_wallet_manager, split_funded_wallet_manager, and split_funded_wallet_manager_dashpay (which all build WalletSigner, wrap WalletBalance, assemble PlatformWalletInfo, create WalletManager::new, and call insert_wallet) into a private helper function that accepts a TestWalletContext and returns the tuple (Arc<RwLock<WalletManager<PlatformWalletInfo>>>, WalletId, Arc<WalletBalance>, WalletSigner). Replace the duplicate code in each of these functions with a single call to this helper, and apply the same refactoring to the existing tail in funded_wallet_manager_with_outputs to keep all fixtures consistent.packages/rs-platform-wallet/src/wallet/core/send.rs (2)
211-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
build_signed_paymentintentionally drops the reservation bookkeeping.This method discards
funding,reservation_height, andreservation_tokenfrom theFinalizedCorePayment. TheFinalizedCorePaymentdoc (lines 108-112) states that dropping one without registering or abandoning it strands the reservation until key-wallet's TTL backstop. The module docs (lines 29-44) explain that the stranding is intentional here, because the host broadcasts and a later sync reconciles the spend. A reader arriving at this projection sees the drop without that context. Add one sentence so the two contracts do not read as contradictory.📝 Proposed doc note
/// * `funding_path` — the account-level derivation path of the SINGLE funds /// account whose UTXOs fund the payment. `None` (the default) funds from /// the unmixed BIP44 account (dashpay/platform#4184). + /// + /// ## Reservation bookkeeping is intentionally dropped + /// + /// This projection discards the resolved [`FundingAccountRef`], the + /// reservation height, and key-wallet's owner token. The selected inputs + /// STAY reserved, and no token exists to release them early — the + /// build-only contract in the module docs: the host broadcasts, and a sync + /// (or the reservation TTL) reconciles the reservation. Callers that may + /// abandon the payment must use + /// [`finalize_signed_payment_from_funding_path`](Self::finalize_signed_payment_from_funding_path) + /// and [`abandon_payment`](Self::abandon_payment) instead.🤖 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 211 - 226, Add a concise documentation sentence to build_signed_payment explaining that it intentionally omits FinalizedCorePayment reservation bookkeeping because the host broadcasts the transaction and a later sync reconciles the spend. Reference the discarded funding, reservation_height, and reservation_token fields without changing the payment-building logic.
1176-1294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
abandon_payment.The tests cover the registry release path (
registry.release(token)) and the rejected-broadcast release path. They do not coverabandon_payment, which thefinalize_signed_payment_from_funding_pathdoc names as one of the two required ways to discharge the reservation. It is the path a host takes when marshalling fails between the build and a successfulregister_funded_by— see theabandon_paymentcall inpackages/rs-platform-wallet-ffi/src/core_wallet/send.rs.A test in the shape of
receival_reservation_is_held_and_released_against_the_receival_accountwould cover it: build from the receival path on the wallet's own generation, confirm a second build is blocked, callcore.abandon_payment(&payment), then confirm the rebuild succeeds. That also pins the by-path release for a non-Standard account without the registry in the loop.Do you want me to generate that test?
🤖 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 1176 - 1294, Add a dedicated test alongside receival_reservation_is_held_and_released_against_the_receival_account covering CoreWallet::abandon_payment: build a receival-funded payment using the wallet’s own generation, verify a second build is blocked by the reservation, call core.abandon_payment with the original payment, and verify rebuilding from the same receival path succeeds without involving SignedPaymentRegistry.packages/rs-platform-wallet-ffi/src/core_wallet/send.rs (2)
246-254: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInitialize every out-parameter before the first fallible step.
core_wallet_build_signed_payment_with_tokenonly zeroes*out_token. On every error return,*out_txid,*out_tx_bytes,*out_tx_len,*out_fee, and*out_changekeep whatever the caller passed in. The in-repo JNI caller pre-initializes its locals, so it is safe today. Any other C caller can read uninitialized memory.core_wallet_broadcast_signed_transaction_v2already nulls its out-pointer up front; follow that convention here.🛡️ Proposed fix
check_ptr!(out_change); *out_token = 0; + *out_txid = std::ptr::null_mut(); + *out_tx_bytes = std::ptr::null_mut(); + *out_tx_len = 0; + *out_fee = 0; + *out_change = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/core_wallet/send.rs` around lines 246 - 254, Update core_wallet_build_signed_payment_with_token to initialize every validated out-parameter immediately after the check_ptr! calls and before any fallible operation, setting pointer outputs to null and scalar outputs to zero as appropriate. Preserve the existing out_token initialization and follow the initialization convention used by core_wallet_broadcast_signed_transaction_v2.
329-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the token variant uses the same free function.
core_wallet_build_signed_payment_with_tokenalso writesout_tx_bytes/out_tx_len, and the JNI layer frees them withcore_wallet_free_payment_bytes. Name both producers in the doc comment so the ownership contract is unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/core_wallet/send.rs` around lines 329 - 333, Update the safety documentation for the payment-byte free function associated with core_wallet_free_payment_bytes to name both core_wallet_build_signed_payment and core_wallet_build_signed_payment_with_token as valid producers of the bytes/length pair, preserving the existing null/zero ownership contract.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt (1)
450-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo
buildSignedPaymentoverloads return different types with different reservation semantics.
buildSignedPayment(recipients, network, coreSignerHandle, accountType, accountIndex)returns aSignedCoreTransactionthat owns a reservation token. This overload returns aSignedCorePaymentwith no token. Overload resolution works, because the positional types differ. The hazard is at the call site: both names are identical, both move money, and only one produces a broadcastable, reservation-owning result.The KDoc at Line 479-484 already has to explain which
buildSignedPaymentis which, and the KDoc at Line 546 and Line 595 refer to "[buildSignedPayment]" without saying which overload.Rename the tokenless variant to state what it returns, for example
buildSignedPaymentBytes. Then update the KDoc cross-references inbroadcastSignedandreleaseReservationto name one specific method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt` around lines 450 - 455, Rename the tokenless `buildSignedPayment` overload to `buildSignedPaymentBytes` while preserving its existing parameters and `SignedCorePayment` result. Update all references and KDoc, including the cross-references in `broadcastSigned` and `releaseReservation`, to distinguish this method from the reservation-owning `buildSignedPayment` overload.packages/rs-platform-wallet-ffi/src/wallet.rs (1)
471-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe baseline-delta assertions depend on a process-global registry and can flake.
SIGNED_PAYMENT_REGISTRYis process-global andcargo testruns tests in parallel threads inside one process. Another test in this crate that registers or releases a token between thebaselinecapture and eachassert_eq!shiftsoutstanding()and fails this test for an unrelated reason. The comment already acknowledges the registry is shared.Assert on the specific token instead of the global count, or serialize the registry-touching tests behind a shared
Mutex.♻️ Sketch: assert on the token, not the count
- assert_eq!(SIGNED_PAYMENT_REGISTRY.outstanding(), baseline + 1); - (manager, handle_a, handle_b, baseline) + (manager, handle_a, handle_b, _token) }); let result = unsafe { platform_wallet_destroy(handle_a) }; assert_eq!(result.code, PlatformWalletFFIResultCode::Success); - assert_eq!( - SIGNED_PAYMENT_REGISTRY.outstanding(), - baseline + 1, - "a sibling alias's token must survive destroying another alias" - ); + assert!( + SIGNED_PAYMENT_REGISTRY.contains(token), + "a sibling alias's token must survive destroying another alias" + );This needs a token-presence predicate on
SignedPaymentRegistry; add one if it does not exist.🤖 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/wallet.rs` around lines 471 - 508, Replace the process-global count assertions in the wallet alias-destruction test with assertions targeting the specific token returned by SIGNED_PAYMENT_REGISTRY.register. Add or reuse a SignedPaymentRegistry token-presence predicate, then verify that token remains after destroying handle_a and is absent after destroying handle_b; remove the baseline/outstanding-based checks.packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs (1)
183-191: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe new deferred-payment entry points rely on the caller to pre-zero out-parameters. Both functions validate their out-pointers with
check_ptr!but do not write a default value to all of them before the first fallible step. The current JNI caller zeroes itsFFICoreTransactionbox and itsout_txidlocal, so no defect exists today. The guarantee belongs in the FFI, as the existingcore_wallet_broadcast_signed_transaction_v2already does with*out_txid = std::ptr::null_mut();.
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs#L183-L191: write*out_txid = std::ptr::null_mut();and zero*out_tx(nulltx_bytes, zerotx_len/fee) next to the existing*out_token = 0;, so the error returns at Line 198 and Line 206 leave defined values.packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs#L58-L63: add*out_txid = std::ptr::null_mut();immediately aftercheck_ptr!(out_txid);.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs` around lines 183 - 191, Initialize every deferred-payment out-parameter before fallible operations: in packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs#L183-L191, update the function containing the shown check_ptr! calls to null out_txid and zero out_tx fields (tx_bytes, tx_len, and fee) alongside *out_token = 0; in packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs#L58-L63, update the corresponding function to null out_txid immediately after check_ptr!(out_txid);.
🤖 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/src/error.rs`:
- Around line 74-86: Update the PaymentInsufficientFunds doc comment to state
that available is the selected account’s spendable total, and that required is
the outputs-plus-fee target for that account. Remove references to the union of
signable funding accounts and total selectable value across domains; preserve
the exact-duff and recovery semantics.
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Around line 522-535: The comment describing the fee safety behavior around the
selected_input_value calculation is inverted. Update the comment to correctly
state that if an input somehow cannot be priced and defaults to 0, this lowers
the selected_input_value sum, which in turn makes the fee calculated via
saturating_sub smaller and therefore UNDER-reported (not over-reported). Keep
the explanation that the scenario is impossible because every spendable UTXO is
priced beforehand, but fix the direction of the safety claim to match the actual
mathematical consequence.
- Around line 406-421: Update the funding-account resolution in the send flow to
fail closed when no wallet-level account matching funding_path is found. Remove
the unwrap_or(&bip44_acc) fallback from the funding_wallet_acc lookup, and
propagate an appropriate error before calling set_funding so a different
account’s BIP44 xpub cannot be used.
In `@packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- Around line 1622-1629: Update the KDoc for
core_wallet_signed_payment_broadcast to document ErrorStaleReservationToken as
27, ErrorReservationTokenConsumed as 28, and ErrorReservationWalletMismatch as
29, matching the assignments in error.rs.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 450-455: Rename the tokenless `buildSignedPayment` overload to
`buildSignedPaymentBytes` while preserving its existing parameters and
`SignedCorePayment` result. Update all references and KDoc, including the
cross-references in `broadcastSigned` and `releaseReservation`, to distinguish
this method from the reservation-owning `buildSignedPayment` overload.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/send.rs`:
- Around line 246-254: Update core_wallet_build_signed_payment_with_token to
initialize every validated out-parameter immediately after the check_ptr! calls
and before any fallible operation, setting pointer outputs to null and scalar
outputs to zero as appropriate. Preserve the existing out_token initialization
and follow the initialization convention used by
core_wallet_broadcast_signed_transaction_v2.
- Around line 329-333: Update the safety documentation for the payment-byte free
function associated with core_wallet_free_payment_bytes to name both
core_wallet_build_signed_payment and core_wallet_build_signed_payment_with_token
as valid producers of the bytes/length pair, preserving the existing null/zero
ownership contract.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 183-191: Initialize every deferred-payment out-parameter before
fallible operations: in
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs#L183-L191,
update the function containing the shown check_ptr! calls to null out_txid and
zero out_tx fields (tx_bytes, tx_len, and fee) alongside *out_token = 0; in
packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs#L58-L63,
update the corresponding function to null out_txid immediately after
check_ptr!(out_txid);.
In `@packages/rs-platform-wallet-ffi/src/wallet.rs`:
- Around line 471-508: Replace the process-global count assertions in the wallet
alias-destruction test with assertions targeting the specific token returned by
SIGNED_PAYMENT_REGISTRY.register. Add or reuse a SignedPaymentRegistry
token-presence predicate, then verify that token remains after destroying
handle_a and is absent after destroying handle_b; remove the
baseline/outstanding-based checks.
In `@packages/rs-platform-wallet/src/test_support.rs`:
- Around line 265-334: Extract the repeated wallet-assembly logic from the tail
of funded_coinjoin_wallet_manager, split_funded_wallet_manager, and
split_funded_wallet_manager_dashpay (which all build WalletSigner, wrap
WalletBalance, assemble PlatformWalletInfo, create WalletManager::new, and call
insert_wallet) into a private helper function that accepts a TestWalletContext
and returns the tuple (Arc<RwLock<WalletManager<PlatformWalletInfo>>>, WalletId,
Arc<WalletBalance>, WalletSigner). Replace the duplicate code in each of these
functions with a single call to this helper, and apply the same refactoring to
the existing tail in funded_wallet_manager_with_outputs to keep all fixtures
consistent.
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Around line 211-226: Add a concise documentation sentence to
build_signed_payment explaining that it intentionally omits FinalizedCorePayment
reservation bookkeeping because the host broadcasts the transaction and a later
sync reconciles the spend. Reference the discarded funding, reservation_height,
and reservation_token fields without changing the payment-building logic.
- Around line 1176-1294: Add a dedicated test alongside
receival_reservation_is_held_and_released_against_the_receival_account covering
CoreWallet::abandon_payment: build a receival-funded payment using the wallet’s
own generation, verify a second build is blocked by the reservation, call
core.abandon_payment with the original payment, and verify rebuilding from the
same receival path succeeds without involving SignedPaymentRegistry.
🪄 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: 9903f29f-5993-444c-9324-0009848c7b5a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/send.rspackages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/handle.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/utils.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/send.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/rs-platform-wallet/src/wallet/funding_privacy.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rspackages/rs-unified-sdk-jni/src/funding.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
|
Re the AccountBalanceEntryFFI stride concern (field appended for derivationPath, inherited here from #4247's branch): the only shipping consumer of this struct today is the Android JNI, which marshals it to JSON inside the same Rust build — producer and consumer always share one struct definition, so there is no cross-version stride exposure on Android. The iOS/Swift side does consume the C ABI directly, so any Swift binding regeneration must happen against the updated header before adopting these revs — called out in #4255 (the iOS adoption issue). If versioned structs are preferred as policy we're happy to follow up, but for this stack the append-only change plus the #4255 regeneration note covers the practical risk. 🤖 Generated with Claude Code |
… finalized payment linear Addresses the review findings on dashpay#4256. Fee-cap overflow (BLOCKING). `MAX_FEE_PER_KB = MAX_MONEY / 100` was only safe if the transaction stayed under Dash's 100_000-byte standard limit, which this method never enforced. key-wallet's `FeeRate::calculate_fee` then evaluates `sat_per_kb * size_bytes` with unchecked `u64` multiplication, overflowing at ~878 kB — reachable by a ~25.8k-recipient list that fits in a practical JNI blob. Three-part fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`) using checked arithmetic that mirrors key-wallet's own base-size formula, before the wallet lock and before anything is reserved; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, making the product representable for ANY size a `u32` can express. The previous bound depended on a size limit that was never enforced; this one does not depend on the input count at all, which also closes the CoinJoin-account case (a few thousand small denominations reach ~878 kB with no oversized recipient list); * the signed transaction is re-measured and refused if it exceeds the limit, since the input count is unknowable until coin selection has run. That refusal discharges the reservation it had already taken via `abandon_payment` rather than stranding the account's coins until the TTL backstop. `FinalizedCorePayment` linearity (partial). The type no longer derives `Clone` and `abandon_payment` now consumes it, so abandoning-then-registering, or minting two registry tokens able to broadcast the same transaction, no longer compiles. The remaining half — folding the four separate `register_funded_by` parameters into one consuming `register(payment)` — belongs on dashpay#4185, which owns that API; doing it here would fork a sibling PR's surface. Wallet-manager write lock: NOT dropped before signing (deferred, see the PR thread). `set_funding` only snapshots UTXOs and clones the `Arc<ReservationSet>` — selection and `reserve()` both run inside `build_signed_reserved`, so dropping the guard first would let two concurrent finalizes reserve the same outpoint and produce two signed transactions spending one coin. The narrower "reserve under the lock, sign outside" split needs a key-wallet API that does not exist (`assemble_unsigned` is private; there is no `build_unsigned_reserved`). The guard IS now released immediately after the build, before the release paths, which is what makes the in-function `abandon_payment` non-deadlocking. Docs: corrects the JNI `core_wallet_signed_payment_broadcast` result codes (27/28/29, was 26/27/28 — the Kotlin mirror was already right) and an inverted fee-direction comment. Tests: 537 passed (528 baseline + 9), platform-wallet-ffi 223 passed, Kotlin `:sdk:testDebugUnitTest` green, `cargo check -p platform-wallet-ffi -p rs-unified-sdk-jni` clean, `cargo fmt --check` clean. New coverage for the over-limit refusal on both the output side and the post-build side (the latter also pinning that the refusal releases its reservation), `abandon_payment` without the registry, dust rejection, MAX_MONEY aggregation and the fee-rate bound. 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>
…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>
|
Pushed The blocker — fee cap vs. unenforced size limit — is fixed. The finding was right that
One thing I want to flag rather than bury. I implemented the write-lock suggestion, then backed it out. Dropping
Also fixed: the fail-closed lookup (verified empirically first — Deferred with reasons on their threads: FFI out-parameter initialisation (grouped with the two sibling instances — same one-line convention, better fixed together); the wallet-cleanup/registration atomicity window (#4185's lifecycle machinery — this PR widens it, doesn't open it); and the Suites: New tests: over-limit refusal on the output side (the 25,835-recipient case at max fee rate) and on the post-build side (900 recipients × 500 UTXOs → ~496 inputs, just over 100 kB — the only test that reaches the post-build check, and the one that would deadlock rather than fail if the guard weren't released before 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the prior fee-overflow blocker and the stale insufficient-funds documentation. It introduces one new blocking error-contract issue: signer failures are now reported as request-invalid transaction builds; two carried-forward suggestions remain valid, while three broader lifecycle or C-ABI hardening items are explicitly deferred to their owning follow-up work.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/rust-quality=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
2 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-ffi/src/error.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/error.rs:407-408: Do not classify signing failures as request-invalid transaction builds
`map_send_builder_error` converts every non-shortfall `BuilderError` into `PlatformWalletError::TransactionBuild`, including `BuilderError::SigningFailed`. The new blanket mapping here consequently returns native code 32, whose Rust and Kotlin contracts state that the request itself is invalid and that retrying it unchanged cannot succeed. That is false for the production `MnemonicResolverCoreSigner`: a missing Keychain mnemonic or a resolver callback failure becomes `SigningFailed`, and key-wallet releases the owner-stamped input reservation on this path. Restoring or unlocking the signer can therefore make the same recipients, amount, fee, and funding path succeed. Keep signing failures out of `ErrorTransactionBuild`; map them to a signer-specific platform error and FFI result code, such as the code-31 signing-key contract reserved by the sibling stack.
Note: GitHub does not allow PastaClaw to approve or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
| PlatformWalletError::TransactionBuild(..) => { | ||
| PlatformWalletFFIResultCode::ErrorTransactionBuild |
There was a problem hiding this comment.
🔴 Blocking: Do not classify signing failures as request-invalid transaction builds
map_send_builder_error converts every non-shortfall BuilderError into PlatformWalletError::TransactionBuild, including BuilderError::SigningFailed. The new blanket mapping here consequently returns native code 32, whose Rust and Kotlin contracts state that the request itself is invalid and that retrying it unchanged cannot succeed. That is false for the production MnemonicResolverCoreSigner: a missing Keychain mnemonic or a resolver callback failure becomes SigningFailed, and key-wallet releases the owner-stamped input reservation on this path. Restoring or unlocking the signer can therefore make the same recipients, amount, fee, and funding path succeed. Keep signing failures out of ErrorTransactionBuild; map them to a signer-specific platform error and FFI result code, such as the code-31 signing-key contract reserved by the sibling stack.
source: ['codex']
2 participants
What this does
Bridges two flows that were previously disconnected in
platform-wallet:build_signed_payment's single-account-by-derivation-path selection (feat(kotlin-sdk): single-account build_signed_payment send API (funding_path) #4247) — the only build path that can reach aDashpayReceivingFundsaccount; andtoken → broadcastSigned/releaseReservation.New
finalize_signed_payment_from_funding_pathreturns aFinalizedCorePayment(signed tx, fee, change, the resolvedFundingAccountRef, and the reservation token). The key correctness property: the reservation is recorded against the resolved account, never a BIP44 default, so release/broadcast bookkeeping lands in the same ledger the build reserved into.FundingAccountRef::Pathmakes non-standard accounts (DashPay receival) nameable to the release machinery, which previously could only addressStandardAccountTypes.Surface:
wallet/core/transaction.rs—FundingAccountRefenum +release_reservation_forwallet/core/send.rs—finalize_signed_payment_from_funding_path,FinalizedCorePayment,abandon_paymentwallet/signed_payment_registry.rs—register_funded_bycore_wallet_build_signed_payment_with_tokencoreWalletBuildSignedPaymentWithTokenManagedPlatformWallet.buildSignedPaymentWithToken(recipients, coreSignerHandle, feePerKb, fundingPath), returning the existingSignedCoreTransaction(now carryingchangeDuffs)Guardrails preserved and covered by tests: single-account funding only (the funding-privacy guardrails still pass — no cross-account union), change goes to BIP44/0, watch-only accounts are refused, and the fee is taken from the signed transaction rather than re-estimated.
Stacked on
Stacked on #4185, and includes #4247's commits.
The branch is based on #4185's head (
port/v4.1/split-build-broadcast) with #4247's three commits (port/v4.1/send-raw-tx) cherry-picked beneath this change, because it needs both:bfoss765/rust-dashcorepin at reve99959ce— theReservationToken/build_unsigned_reservedAPI this work is built on.build_signed_payment(funding_path)primitive itself, which is not present on feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185's branch.Merge #4185 and #4247 first, then rebase this PR — its own contribution is the top two commits.
Two scoping notes for reviewers
1. The test-support commit is a verbatim carry of #4184's fixture — drop it on rebase once #4184 merges.
The receival tests need a wallet whose balance is split between BIP44 account 0 and a DashPay funds account, which
WalletAccountCreationOptions::Defaultdoes not provision. That fixture —split_funded_wallet_manager_dashpay,DashpayLeg, and theforeign_contact_account_xpubhelper — already exists in #4184'stest_support.rs, and the last commit here (test(platform-wallet): DashPay-funded split wallet fixture…) is a verbatim copy of it, carried so this PR does not have to stack on 2.9k lines of unrelated asset-lock production code just to reach a test fixture. No production code from #4184 is included.Once #4184 merges, drop that commit when rebasing — #4184's copy is the canonical one. The only intentional deviation is that the three items are
#[cfg(test)]-gated here (ungated they would tripdead_codein atest-utils-only build of the FFI crate, since this crate's unit tests are their sole consumer); #4184 widens the consumer set, so its ungated copy supersedes this one cleanly.2. De-contaminated from the source branch. On
kotlinSDK-v4-qa3the finalize commit and its parent had swapped pieces: the finalize commit carried the masternodes-by-voting-key JNI export (plus itsread_id20helper), while the parent carried this feature'sexternal fun coreWalletBuildSignedPaymentWithTokendeclaration. This PR drops the masternodes bridge (its FFI half is not in this stack, so it would not build here) and includes the Kotlin declaration that belongs to it. The masternodes-by-voting-key feature is untouched and remains to be submitted on its own.Tests
cargo test -p platform-wallet --lib— 528 passed, 0 failed, no warnings. Includes all 11wallet::core::sendtests, all 20wallet::signed_payment_registrytests, and all 3wallet::funding_privacy::guardrailtests.cargo check -p platform-wallet-ffi -p rs-unified-sdk-jni— cleanThree new tests cover the receival funding path end to end, including reservation hold/release cycles:
receival_funding_path_selects_signs_and_reserves_in_that_accountreceival_reservation_is_held_and_released_against_the_receival_accountdefault_funding_reservation_is_held_and_released_on_bip44References
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes