feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission - #4308
Merged
Merged
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 #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 #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>
…ged reservation API Point all rust-dashcore workspace crates at dashpay/rust-dashcore 8f78baa6b7979b9bea56501ad75b5a7b7150a711, the dev merge commit of PR dashpay/rust-dashcore#916, which lands the additive owner-tagged reservation API this PR consumes: key_wallet::ReservationToken, ReservationSet::reserve/release_if_owner, TransactionBuilder::build_{unsigned,signed}_reserved, ManagedCoreFundsAccount::release_reservation_if_owner, and AssetLockResult.reservation_token. Previously pinned to bfoss765/rust-dashcore because the API existed only on the fork branch before #916 merged. Now repointed to the canonical upstream repo (no personal-fork dependency). 8f78baa6 is a strict descendant of v4.2-dev's prior pin 70d4bf8, so this is a forward-only bump. Co-Authored-By: Claude Opus 4.8 <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 (#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 #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 (#4196). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…per-destroy from consuming payments, type the token Addresses two of the three carried-forward lifecycle blockers on #4185 plus the two smaller review items. The wallet-removal/finalize linearization blocker is intentionally NOT included here (see PR discussion) — it needs a shared lifecycle gate that is a design change on a money path. Blocker 1 — unique reservation ownership: `SignedPaymentRegistry::register` now CONSUMES the non-`Clone` `SignedCoreTransaction` and derives the transaction, funding account, mandatory reservation height, and owner-guard token from it (new `SignedCoreTransaction::into_registered_parts`). Because the ownership object is moved exactly once, a single finalize can no longer mint two live tokens naming the same held reservation. The FFI finalizer passes the finalized object straight in; the former duplicate-registration test (16 clones of one reserved tx) is removed as it modelled the now-impossible pattern. Blocker 2 — final wallet-alias destroy no longer consumes independently-owned payments: `platform_wallet_destroy` no longer releases the generation's tokens when the last wrapper alias is dropped. A wrapper handle does not own the logical wallet or the registered payment (the manager still owns the wallet; each registry entry pins its own `CoreWallet`). Token cleanup now follows the payment owner (broadcast/release) or actual generation teardown (`remove_wallet` → `remove_entries_for_wallet`), never a transient alias count. The unused `release_entries_for_wallet` method and its test are removed; the destroy test now asserts tokens survive destroying every alias. Nit — typed token: `ReservationToken` is now a `#[repr(transparent)]` newtype instead of a bare `u64` alias, converted to/from `u64` only at the FFI boundary, so a payment handle can't be silently confused with another numeric id. Docs — JNI Rustdoc: the `coreWalletBroadcastSignedPayment` block referenced the pre-renumber codes (26/27/28); updated to the current enum values (27 StaleReservationToken / 28 ReservationTokenConsumed / 29 ReservationWalletMismatch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…+ retain reservation owner through insertion (#4185 review) Addresses the two new thepastaclaw blockers plus the Kotlin owner-construction suggestion on PR #4185: 1. Registration could bind a reservation to the wrong wallet generation. SignedCoreTransaction now carries the unforgeable per-generation balance Arc (origin_generation) captured from the finalizing CoreWallet. SignedPaymentRegistry::register validates the supplied core against it and refuses a mismatch with the new typed RegisterWrongGeneration error, handing the rejected SignedCoreTransaction back so its reservation is not stranded. 2. Async registration could drop the reservation owner before insertion. register is now synchronous (its body has no await), so the consumed SignedCoreTransaction cannot be lost to a future dropped before its first poll. The FFI finalizer and all callers invoke it directly. 3. Kotlin: CoreTransactionBuilder.finalizeSignedPayment parses the native token first and releases it (owner-guarded) if SignedCoreTransaction construction throws, so an ABI/allocation/Cleaner failure never leaves the native token without a JVM owner. Adds a register_rejects_a_different_wallet_generation regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t + finalize (#4185 review) Wallet removal was not linearized with the deferred-payment registry, so a retained handle could push a removed wallet's payment onto the network. Two independent windows: 1. remove-then-sweep. `platform_wallet_manager_remove_wallet` called `manager.remove_wallet` and only afterwards swept the registry, with no shared lock spanning the two — and the removal's own awaits (shielded coordinator + identity-sync unregistration) sat in the gap. A concurrent `core_wallet_signed_payment_broadcast` in that window passed every guard: `is_same_generation` compares two handles, so a removed generation matches itself; `last_processed_height` is `None` once the wallet is gone and `reservation_expired` maps `None` to "not expired"; and `broadcast_payment_releasing_reservation` has no wallet-existence gate. 2. in-flight finalizer. `finalize_transaction` drops the manager write lock before awaiting the signer, and `register` only validates the payment against its finalizing generation — never that the generation still exists. A removal during the signer await swept the registry, then the finalizer inserted a fresh token no later sweep would catch, contradicting the documented teardown invariant that dropping tokens makes stale handles inert. Remedies: * `SignedPaymentRegistry` gains a lifecycle gate (`tokio::RwLock`). Teardown takes the exclusive side across BOTH the manager removal and the sweep, making them one linearization point; broadcast and release take the shared side for their whole duration. The existing `entries` mutex cannot do this — it is dropped before every await by design. Lock order is always gate then manager. * Broadcast rejects an absent current generation via the new `CoreWallet::is_current_generation`, returning `SignedPaymentError:: WalletRemoved` instead of silently proceeding to the broadcaster. * `core_wallet_signed_payment_finalize` holds the shared gate across its liveness check and the synchronous `register`, abandoning the payment (reconciling its reservation) if the wallet went away during signing. The gate is taken after the signer await, not around it, so an open signing prompt cannot stall teardown. No new FFI error code: the wallet-removed case is reported as the existing `NotFound` (98), which both hosts already map. Deliberately avoids the 29/30 renumbering contested in #4261. Swift/Kotlin/Rust docs updated to record that 98 now also carries this case, and how it differs from `ErrorReservationWalletMismatch` (29). Adds three FFI regression tests. All three fail against the pre-fix code — the race test reports a payment reaching the broadcaster after teardown completed. Also serializes the registry-count-asserting tests, which the new tests would otherwise race in the shared process-global registry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…30 (#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on #4184. Per the resolution of record in #4261's ERROR_CODE_REGISTRY.md, #4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after #4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: #4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…the final-alias policy removal (#4185 review) Blocker 2 removed the final-alias sweep from `platform_wallet_destroy` (wallet.rs:392-414 is now just a storage `remove`), but the helper that policy was introduced for survived it. `HandleStorage::any` was added by ade3999 for that sweep and has had zero call sites since the policy was dropped. Because `handle` is a `pub mod` and the method is `pub`, no dead-code lint fires and it stayed in the crate's public Rust surface, with Rustdoc still pointing at "the final-alias check in `platform_wallet_destroy`" — a policy that no longer exists. It is not on the base branch, so removing it restores the base surface rather than breaking an existing consumer. `HandleStorage::remove_matching` is retained: it still backs the generation sweep at manager.rs:494. Also corrects a doc cross-reference to the same removed policy in `test_support::test_platform_wallet_manager`, which described the helper as backing "final-alias registry-sweep gating" when its only FFI consumer now asserts the opposite (destroying wrapper aliases must NOT sweep). No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on (#4185 review) The gate added in 0b0d5c7 lived on the FFI's process-global `SIGNED_PAYMENT_REGISTRY`, so it excluded only registry-token operations and did so across every wallet at once. Two consequences, both real: 1. Under-coverage. The V2 finalized-transaction-handle path bypassed it entirely. `core_wallet_tx_builder_finalize` awaited the external signer and then inserted into `CORE_SIGNED_TRANSACTION_V2_STORAGE` with no gate and no liveness re-check, so a teardown could sweep while signing was pending and the late finalizer published a handle no sweep would ever catch. `core_wallet_broadcast_signed_transaction_v2` then consumed such a handle and reached the broadcaster with no gate either — its `is_same_generation` check compares two HANDLES, and a removed generation matches itself. The same hole existed on the public Rust surface: `PlatformWalletManager::remove_wallet` never took the write side at all, so a direct embedder (the manager is public and `SignedPaymentRegistry` is re-exported) removed wallets with no exclusion. 2. Cross-wallet contention. A deferred broadcast holds the shared side across an SPV send; on one process-global write-preferring lock that send blocked teardown — and every payment operation queued behind the waiting writer — for every unrelated wallet in the process. Remedy: move the gate into shared per-generation state. * New `WalletGeneration` owns the lock-free `WalletBalance` AND that generation's `RwLock` lifecycle gate, and replaces `Arc<WalletBalance>` as the generation-identity marker. Folding them into one `Arc` is deliberate: the identity and the gate cannot diverge, so two handles can never compare as the same generation while excluding each other through different locks. `Deref` keeps every existing balance read unchanged. * `PlatformWalletManager::remove_wallet_with_teardown` takes that generation's exclusive gate across BOTH the removal and a caller-supplied teardown hook, and `remove_wallet` routes through it. The gate is no longer optional for any caller, FFI or not. The FFI passes its registry + V2-handle sweep as the hook. Lock order stays gate-then-manager: the lookup that resolves the gate drops `wallets` before awaiting it, then re-validates under the gate. * Every publication/network path now takes the generation's shared gate across its liveness check and the action: the registry `broadcast`/`release`, the token `core_wallet_signed_payment_finalize`, and — newly — both `core_wallet_tx_builder_finalize` and `core_wallet_broadcast_signed_transaction_v2`, which report a dead generation as the existing `NotFound` (98) after reconciling the build's reservation. V2 abandon/free stay ungated: their release is already generation-bound. The gate is still NOT held across an external signer await — finalizers acquire it only after the signature returns, so an open signing prompt cannot stall teardown, and a late finalizer instead fails its liveness check and abandons. The lifecycle comments that claimed the opposite were wrong about the code and are corrected. Adds four deterministic FFI regression tests alongside the existing three: a V2 broadcast-after-removal refusal, a teardown that waits for an in-flight V2 operation and then sweeps its handle, a public `PlatformWalletManager::remove_wallet` that waits for an in-flight payment, and a cross-wallet isolation test pinning the per-generation scoping. With the three guards removed the first three fail; with the gate re-pointed at a single process-global lock the fourth fails. No error-code changes: `ErrorReservationWalletMismatch` stays 30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y key `remove_wallet_with_teardown` validated generation G1 under G1's lifecycle gate, then removed it from the two manager maps in two independently locked stages. Registration takes no gate at all — `register_wallet` mints its own `WalletGeneration` — so from the moment the inner-manager removal frees the id, a concurrent same-id registration can publish a different generation G2 into `wallet_manager` and then into `self.wallets`, with no happens-before edge to the remover's own `self.wallets` acquisition. A remover descheduled in that gap resumed into a map naming G2 and removed the entry BY KEY: it evicted a live wallet (still registered in the inner manager, so invisible and unremovable through the public map), returned it to the caller, and handed it to `tear_down` — which sweeps that generation's registry tokens and V2 finalized-transaction handles while holding only G1's gate, i.e. with G2's payment operations not excluded. That exclusion is the one property the gate exists to provide. Retain the `Arc<PlatformWallet>` validated under the gate and remove the public-map entry only while it still pointer-matches that generation, so the removed handle, the returned handle and the `tear_down` argument are all the one generation this call validated. The inner-manager removal needs no such check: G1 can only leave `wallet_manager` through this method (which requires G1's gate) or through a rollback for an insert that could not have happened while G1 occupied the id. Regression test `removal_leaves_a_generation_registered_during_it_intact` drives the real `create_wallet_from_seed_bytes` -> `register_wallet` path from a `cfg(test)` rendezvous fired in the exact window, so the interleaving is pinned with no sleep and no completion-order race. Against the previous code it fails on all three load-bearing assertions: the returned generation, the `tear_down` argument, and the survival of the re-registered wallet in the public map. Refs #4185 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds deferred signed-payment creation, token-based broadcast and release, generation-aware wallet lifecycle handling, native FFI bridges, and typed Kotlin and Swift error mappings. ChangesDeferred signed-payment workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit ebaca83) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4308 +/- ##
============================================
- Coverage 87.61% 87.04% -0.57%
============================================
Files 2704 2704
Lines 345206 345206
============================================
- Hits 302445 300482 -1963
- Misses 42761 44724 +1963
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs (1)
665-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the stranded
usestatement to the import block.
use crate::PlatformWalletError;at line 675 sits after thepreferencefunction definition. Every other import in this test module is at the top (lines 642-663). Move it there.♻️ Proposed move
use crate::wallet::core::{CoreWallet, SignedCoreTransaction}; + use crate::PlatformWalletError; /// The [`AccountTypePreference`] a `build_signed_tx` funding account maps to /// — the registry now retains the full account handle (CoinJoin included), /// so the tests register with the preference rather than the narrower /// `StandardAccountType`. fn preference(account_type: StandardAccountType) -> AccountTypePreference { match account_type { StandardAccountType::BIP44Account => AccountTypePreference::BIP44, StandardAccountType::BIP32Account => AccountTypePreference::BIP32, } } - use crate::PlatformWalletError;🤖 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/signed_payment_registry.rs` around lines 665 - 675, Move the crate::PlatformWalletError import into the test module’s existing top-level import block alongside the other use statements, and remove the stranded declaration after preference. Leave preference and the surrounding test logic unchanged.packages/rs-platform-wallet/src/test_support.rs (2)
265-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared tail of the two funded-wallet fixtures.
funded_coinjoin_wallet_managerrepeats the funding-transaction check, the signer construction, theWalletGenerationcreation, thePlatformWalletInfobuild, and theWalletManagerinsert fromfunded_wallet_manager_with_outputs(lines 226-262). Only the address-derivation step differs.A small helper taking the derived
receive_addressand the output amounts would remove the duplication. This is test support code, so the payoff is limited to future maintenance.🤖 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 - 333, Extract the shared funding, signer, generation, PlatformWalletInfo, and WalletManager setup from funded_wallet_manager_with_outputs and funded_coinjoin_wallet_manager into a helper that accepts the derived receive_address and output amounts. Update both fixtures to perform only their account-specific address derivation, then delegate to the helper while preserving their existing return values and assertions.
385-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the doc comment: the function returns the manager, not a wallet.
The first line states "Build a full [
PlatformWallet] over a mock SDK". The return type is(Arc<PlatformWalletManager<NoopTestPersister>>, WalletId). The caller must resolve the wallet throughmanager.get_wallet(&wallet_id).📝 Proposed doc fix
-/// Build a full [`PlatformWallet`] over a mock SDK and a no-op persister, wired -/// through a real [`PlatformWalletManager`] so its `wallet_manager` `Arc` and -/// `wallet_id` are production-shaped. Returns the manager (which the caller must -/// keep alive — it owns the wallet-event adapter task and the registered -/// `Arc<PlatformWallet>`) alongside the wallet id. +/// Register a full [`PlatformWallet`] over a mock SDK and a no-op persister, +/// through a real [`PlatformWalletManager`] so its `wallet_manager` `Arc` and +/// `wallet_id` are production-shaped. Returns the manager and the wallet id; +/// resolve the wallet with `manager.get_wallet(&wallet_id)`. The caller must +/// keep the manager alive — it owns the wallet-event adapter task and the +/// registered `Arc<PlatformWallet>`.🤖 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 385 - 393, Update the doc comment for the helper returning `(Arc<PlatformWalletManager<NoopTestPersister>>, WalletId)` to state that it builds and returns a manager plus wallet ID, rather than a `PlatformWallet`; note that callers resolve the wallet via `manager.get_wallet(&wallet_id)`, while preserving the existing manager-lifetime guidance.packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (1)
54-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the midpoint hook against cross-test interference.
REMOVE_WALLET_MIDPOINT_HOOKis a process-global static.cargo testruns test functions in the same binary on multiple threads by default. If another test in this crate ever arms the hook, or runs a removal whileremoval_leaves_a_generation_registered_during_it_intactholds it armed, that unrelated removal fires the hook and re-registers a wallet.Today only one test arms the hook, so this is latent rather than active. Consider documenting the constraint on the static, or serializing hook-armed tests behind a dedicated
Mutex.Also applies to: 748-760
🤖 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/manager/wallet_lifecycle.rs` around lines 54 - 80, Protect the process-global REMOVE_WALLET_MIDPOINT_HOOK from concurrent test use by introducing a dedicated test-only synchronization guard and acquiring it for the entire duration of removal_leaves_a_generation_registered_during_it_intact while the hook is armed. Ensure other hook-armed tests follow the same guard, and document the serialization requirement on the static or guard.packages/rs-platform-wallet/src/wallet/core/broadcast.rs (1)
132-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider delegating
broadcast_finalized_transactionto this method.The two methods now encode the same release policy: broadcast, release only on
BroadcastError::Rejected, and forward the reservation token. A future policy change must be applied in both places.broadcast_finalized_transactionhas all four arguments available through its accessors, so it can call this method directly. The delegation also removes the fully-qualifiedcrate::broadcaster::BroadcastError::Rejectedat Line 29, which now duplicates the importedBroadcastError.♻️ Proposed delegation
pub async fn broadcast_finalized_transaction( &self, transaction: &SignedCoreTransaction, ) -> Result<dashcore::Txid, PlatformWalletError> { - match self.broadcaster.broadcast(transaction.transaction()).await { - Ok(txid) => Ok(txid), - Err(error) => { - if matches!(error, crate::broadcaster::BroadcastError::Rejected { .. }) { - self.release_transaction_reservation( - transaction.funding_account_type(), - transaction.funding_account_index(), - transaction.transaction(), - transaction.reservation_token(), - ) - .await; - } - Err(error.into()) - } - } + self.broadcast_payment_releasing_reservation( + transaction.funding_account_type(), + transaction.funding_account_index(), + transaction.transaction(), + transaction.reservation_token(), + ) + .await }🤖 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/broadcast.rs` around lines 132 - 154, Update broadcast_finalized_transaction to delegate to broadcast_payment_releasing_reservation using its account type, account index, transaction, and reservation token accessors. Remove its duplicated broadcast/release handling and fully qualified BroadcastError::Rejected reference, preserving the existing return behavior through the shared method.packages/rs-platform-wallet/src/lib.rs (1)
61-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the re-exported
ReservationTokento avoid a collision withkey_wallet::ReservationToken.This crate now exposes two distinct types named
ReservationToken:
platform_wallet::ReservationToken— the registry's opaqueu64payment token minted bySignedPaymentRegistry::register.key_wallet::ReservationToken— the funding-input reservation token thatSignedCoreTransaction::reservation_token()returns and thatbroadcast_payment_releasing_reservationaccepts (seepackages/rs-platform-wallet/src/wallet/core/broadcast.rsLine 4).Both travel through the same public deferred-payment API. A caller that imports
platform_wallet::ReservationTokenand passes it to a function expecting the key-wallet type gets a confusing type error, and reviewers of call sites cannot tell the two apart by name. A distinct name such asSignedPaymentTokenremoves the ambiguity.🤖 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/lib.rs` around lines 61 - 63, Rename the re-exported registry type in the `signed_payment_registry` export to `SignedPaymentToken`, while preserving its underlying type and all registry APIs such as `SignedPaymentRegistry::register`. Update affected references to distinguish it from `key_wallet::ReservationToken`, including deferred-payment call sites.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt (1)
184-210: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a null blob with a typed error.
coreWalletFinalizeSignedPaymentreturns a null array in one path without throwing: the JNI bridge returnsptr::null_mut()whenenv.byte_array_from_slicefails (packages/rs-unified-sdk-jni/src/wallet_manager.rs, lines 1347-1356). That path already releases the token native-side, so no reservation leaks. But Kotlin then callsByteBuffer.wrap(null)and surfaces a bareNullPointerExceptioninstead of an SDK error.Add an explicit null check so the caller sees a clear failure.
♻️ Proposed guard
val blob = WalletManagerNative.coreWalletFinalizeSignedPayment( builderPtr, walletHandle, accountType.ffiValue, accountIndex, coreSignerHandle, ) + // The JNI bridge returns null (without throwing) only when the result + // array allocation fails; it releases the token itself on that path. + checkNotNull(blob) { "finalizeSignedPayment returned no result blob" }🤖 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/CoreTransactionBuilder.kt` around lines 184 - 210, Update the try block in the payment-finalization flow around coreWalletFinalizeSignedPayment and SignedCoreTransaction.fromRegisterBlob to explicitly handle a null blob before parsing it, throwing the SDK’s established typed error with a clear failure message. Preserve the existing token cleanup for non-null blobs when construction fails.packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs (1)
225-233: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winZero every out-parameter before the first fallible return.
Only
*out_tokenis pre-zeroed. On every error return,*out_fee,*out_txid,*out_tx,*out_bytes_ptr, and*out_bytes_lenkeep the caller-supplied contents. The doc comment statesout_txshould be "typically zeroed", which makes correct cleanup depend on caller discipline. The in-tree JNI caller zeroes its storage, but a Swift or C caller that passes stack storage and then callscore_wallet_transaction_freeon the error path would free an uninitialized pointer.Initialize all out-parameters up front so error paths leave a well-defined state.
♻️ Proposed defensive initialization
check_ptr!(out_bytes_len); *out_token = 0; + *out_fee = 0; + *out_txid = std::ptr::null_mut(); + *out_bytes_ptr = std::ptr::null(); + *out_bytes_len = 0; + std::ptr::write( + out_tx, + FFICoreTransaction { + tx_bytes: std::ptr::null_mut(), + tx_len: 0, + fee: 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/transaction_builder.rs` around lines 225 - 233, In the transaction-builder function, immediately after the existing check_ptr! calls and before any fallible operation, initialize every out-parameter to its documented zero/null state: out_token, out_fee, out_txid, out_tx, out_bytes_ptr, and out_bytes_len. Preserve the existing output assignments on success while ensuring all early error returns leave caller-provided storage safe to clean up.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt (1)
217-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
close()can block on the calling thread.
close()runsTokenRelease.run(), which calls the blocking nativecoreWalletReleaseSignedPayment. For a token that was already broadcast or released, the native side returns early, so the cost is negligible. For an abandoned payment it takes the registry lock and runs the reservation reconciliation underruntime().block_on.
close()is not a suspend function, so a host that writespayment.use { ... }on the Android main thread performs that work on the main thread. The suspendreleaseReservation(payment)overload already avoids this by consuming the token onDispatchers.IOfirst.State the threading expectation in the KDoc so hosts choose
releaseReservation(payment)for the abandon path.♻️ Proposed KDoc addition
* [broadcastSigned] / [releaseReservation] (native no-op) and safe to * call twice. The [NativeCleaner] backstop runs the same release on GC * if you never call [close]. + * + * BLOCKING: this is not a suspend function and it calls into native + * code. For an already-consumed token the native release returns + * immediately. For an abandoned payment it performs the reservation + * reconciliation inline, so call it off the main thread, or prefer the + * suspend [releaseReservation] overload. */ override fun close() = cleanable.clean()🤖 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 217 - 250, Update the KDoc for SignedCoreTransaction.close() to explicitly state that it may block the calling thread while releasing an abandoned payment, including when invoked via use on Android’s main thread. Direct hosts to use the suspend releaseReservation(payment) overload for the abandon path, while preserving the existing idempotence and GC backstop documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- Around line 423-432: Update the stale reserved-code comment near the mappings
for PlatformWallet errors to remove the contradictory “Codes 26-30” ownership
statement and align it with the current ownership list documenting code 26 and
codes 27-33. Keep the error mappings unchanged and ensure the file contains one
consistent description of reserved-code ownership.
In `@packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- Around line 1392-1403: Update the `env.new_string(txid)` failure path in
`broadcastSignedPayment` to throw an SDK exception, matching the equivalent
finalize bridge behavior, before returning a null pointer. Preserve the existing
successful string conversion and ensure allocation failure is reported rather
than silently returned as a null txid.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt`:
- Around line 184-210: Update the try block in the payment-finalization flow
around coreWalletFinalizeSignedPayment and
SignedCoreTransaction.fromRegisterBlob to explicitly handle a null blob before
parsing it, throwing the SDK’s established typed error with a clear failure
message. Preserve the existing token cleanup for non-null blobs when
construction fails.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 217-250: Update the KDoc for SignedCoreTransaction.close() to
explicitly state that it may block the calling thread while releasing an
abandoned payment, including when invoked via use on Android’s main thread.
Direct hosts to use the suspend releaseReservation(payment) overload for the
abandon path, while preserving the existing idempotence and GC backstop
documentation.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 225-233: In the transaction-builder function, immediately after
the existing check_ptr! calls and before any fallible operation, initialize
every out-parameter to its documented zero/null state: out_token, out_fee,
out_txid, out_tx, out_bytes_ptr, and out_bytes_len. Preserve the existing output
assignments on success while ensuring all early error returns leave
caller-provided storage safe to clean up.
In `@packages/rs-platform-wallet/src/lib.rs`:
- Around line 61-63: Rename the re-exported registry type in the
`signed_payment_registry` export to `SignedPaymentToken`, while preserving its
underlying type and all registry APIs such as `SignedPaymentRegistry::register`.
Update affected references to distinguish it from
`key_wallet::ReservationToken`, including deferred-payment call sites.
In `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- Around line 54-80: Protect the process-global REMOVE_WALLET_MIDPOINT_HOOK from
concurrent test use by introducing a dedicated test-only synchronization guard
and acquiring it for the entire duration of
removal_leaves_a_generation_registered_during_it_intact while the hook is armed.
Ensure other hook-armed tests follow the same guard, and document the
serialization requirement on the static or guard.
In `@packages/rs-platform-wallet/src/test_support.rs`:
- Around line 265-333: Extract the shared funding, signer, generation,
PlatformWalletInfo, and WalletManager setup from
funded_wallet_manager_with_outputs and funded_coinjoin_wallet_manager into a
helper that accepts the derived receive_address and output amounts. Update both
fixtures to perform only their account-specific address derivation, then
delegate to the helper while preserving their existing return values and
assertions.
- Around line 385-393: Update the doc comment for the helper returning
`(Arc<PlatformWalletManager<NoopTestPersister>>, WalletId)` to state that it
builds and returns a manager plus wallet ID, rather than a `PlatformWallet`;
note that callers resolve the wallet via `manager.get_wallet(&wallet_id)`, while
preserving the existing manager-lifetime guidance.
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 132-154: Update broadcast_finalized_transaction to delegate to
broadcast_payment_releasing_reservation using its account type, account index,
transaction, and reservation token accessors. Remove its duplicated
broadcast/release handling and fully qualified BroadcastError::Rejected
reference, preserving the existing return behavior through the shared method.
In `@packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs`:
- Around line 665-675: Move the crate::PlatformWalletError import into the test
module’s existing top-level import block alongside the other use statements, and
remove the stranded declaration after preference. Leave preference and the
surrounding test logic unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4bb31b0-b041-4c02-b00a-51f7d0011ff8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
packages/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/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/wallet.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/manager/load.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/generation.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/platform_wallet_traits.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
thepastaclaw
suggested changes
Aug 6, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The deferred-payment ownership model is strong, but two blocking lifecycle defects remain: stale tokens can strand reservations that are still safely owner-releasable, and same-ID recreation can lose the new generation's identity or shielded side-manager state during old-generation teardown. Four non-blocking FFI/JNI/documentation issues should also be corrected for deterministic cleanup and consistent error reporting.
Source: reviewer backend models: gpt-5.6-sol (codex-general), gpt-5.6-sol (codex-rust-quality), gpt-5.6-sol (codex-ffi-engineer); final verifier backend model: gpt-5.6-sol. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:504-554: Release stale entries with their retained owner token
Both the stale-broadcast branch and `reconcile_removed_entry` discard an expired payment without releasing its reservation. The exact key-wallet revision used here retains per-reservation ownership and implements `release_reservation_if_owner` atomically: it removes inputs only while they still belong to this payment and becomes a no-op after a TTL sweep or re-reservation transfers ownership. The current tests advance 22 blocks, below key-wallet's 24-block TTL, prove that the original reservation is still held, and require an immediate rebuild to fail. Consequently, the stale-token error instructs callers to rebuild while leaving the required inputs unavailable for several more blocks, and explicit `releaseReservation` reports success without releasing them. Run the owner-guarded reconciliation for stale entries whenever `funding_reservation_token` is present; retain the no-release fallback only for the unowned `None` case.
In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:721-816: Prevent old-generation teardown from deleting recreated wallet state
Removing G1 from the inner wallet manager frees `wallet_id`, allowing the deliberately supported concurrent G2 registration to publish a new generation. Teardown then unconditionally calls `coordinator.unregister_wallet(*wallet_id)` and unregisters G1's identity IDs, but both side registries are keyed only by wallet or identity ID rather than `WalletGeneration`. If G2 binds shielded state or registers the same identity-sync row before G1 reaches these awaited removals, G1 deletes G2's newly installed state. The generation check around `self.wallets.remove` protects only the public wallet map. Keep same-ID registration fenced until all ID-keyed teardown is complete, or make these side registries and unregister operations generation-aware; extend the midpoint recreation test to install G2 side-manager state and verify that it survives.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:1319-1323: Release the registered token when defensive txid validation fails
A successful `core_wallet_signed_payment_finalize` has already inserted the payment into the process-global registry and transferred reservation ownership to `token`. If this defensive null-txid branch fires because of an FFI regression, it frees the transaction buffer but returns without releasing that token, orphaning the registry entry and reservation until the TTL backstop. The later byte-array allocation failure path correctly releases the token; this post-registration failure path must do the same.
In `packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs:81-83: Clear the deferred-broadcast txid output before validation
This public C ABI validates `out_txid` but does not publish a null sentinel before wallet lookup and token validation. Invalid handles, consumed tokens, mismatches, and stale tokens therefore leave the caller's previous output value untouched. The sibling `core_wallet_broadcast_transaction` explicitly nulls this output before fallible work and documents that operational errors leave it null. Matching that convention prevents direct C or Swift consumers from observing or cleaning up a stale pointer when an error is returned.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:225-233: Initialize every deferred-finalize output before fallible work
Only `out_token` is initialized before wallet resolution, network validation, signing, and registration. On an error, `out_fee`, `out_txid`, `out_tx`, `out_bytes_ptr`, and `out_bytes_len` retain caller-supplied contents. The in-tree JNI caller independently zeroes its storage, but direct C or Swift consumers must otherwise know this hidden cleanup precondition. Publish deterministic zero/null values for every output immediately after validating the pointers, while preserving the existing success assignments.
Union-resolves module/test-helper both-sides-adds with #4319 (signMessage); adapts its merged-in test fixtures to this branch's PlatformWalletInfo.generation (Arc<WalletGeneration> replaced the bare balance Arc as the per-generation identity). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…instead of stranding them A stale token refused its broadcast AND dropped its entry without releasing, on the theory that a post-sweep release could free a newer build's reservation. That policy predates the funding owner token: release_reservation_if_owner frees inputs only while this build still owns them and no-ops after a sweep/re-reservation, so reconciling is safe at any age — and between RESERVATION_MAX_AGE_BLOCKS and key-wallet's TTL the reservation is typically STILL HELD, so dropping stranded the inputs for several blocks while the StaleReservationToken error told the caller to rebuild, and the rebuild failed selection. Now the stale broadcast branch and reconcile_removed_entry release owner-guarded whenever the token is present; only a token-less entry keeps the drop-without-release fallback. Tests flipped from rebuild-fails to rebuild-succeeds. Addresses #4308 review finding a579bd40062f. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the wallet id frees remove_wallet_with_teardown removed the wallet from the inner manager (freeing its id for a concurrent same-id registration) and only then unregistered the shielded-coordinator binding and identity-sync rows — both keyed by wallet/identity id, not generation. A recreation (G2) committing in that window had its freshly installed side-registry state deleted by G1's late unregisters. Reorder: shielded detach/unregister and identity-sync unregisters now run while the inner manager still holds the id — insert_wallet is the create path's commit point, so a same-id create fails WalletAlreadyExists until the id frees, making the window unreachable. The inner removal is the last id-keyed step; identities added mid-removal are surfaced with a warning instead of unregistered post-removal (which would reopen the window for G2's rows). The midpoint recreation test now threads a shared identity through both generations and asserts G2's re-registered identity-sync row survives the removal. Addresses #4308 review finding baae4dd108ca. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…face + stale code comment - JNI signedPaymentFinalize: the defensive null-txid branch now releases the already-registered token before throwing (same policy as the byte-array failure path) instead of orphaning its reservation to the TTL backstop. - core_wallet_signed_payment_broadcast publishes the null out_txid sentinel before any fallible step, so token-validation failures never leave a previous output value readable as a txid. - core_wallet_signed_payment_finalize initializes every out param (token, fee, txid, tx, bytes ptr/len) before wallet resolution / signing / registration. - DashSdkError.kt: replace the stale 'codes 26-30 reserved' comment with the actual claim map (26-33 claimed; deferred trio at 34-36). Addresses #4308 review findings 0d7bc4eab444, 03beed560500, d7c1ff66cd6f and the CodeRabbit reserved-code comment note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Aug 6, 2026
…stranding still-owned inputs Mirror of the deferred registry's stale-entry policy on the V2 handle path: with the build's owner token present, release_reservation_if_owner is safe at any age (no-op after a sweep/re-reservation), and between RESERVATION_MAX_AGE_BLOCKS and key-wallet's TTL the reservation is typically still this build's — so an aged abandon that skipped the release stranded the inputs for several blocks after the host discarded the payment. Only a token-less build (unreachable on the funded finalize path) still honours the bound and skips its unguarded by-outpoint release. Test flipped from rebuild-fails to rebuild-succeeds. Extends #4308 review finding a579bd40062f to the V2 age-guard surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
Conflicts and review findings are all addressed on head
549 + 238 lib tests green on this branch (and on the stacked #4309 after its base merge), clippy 🤖 Posted by Claude Code |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-platform-wallet/src/wallet/core/sign_message.rs (2)
218-247: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve key-unavailable errors before adding signer context.
At Line 244, every
sign_ecdsaerror becomesMessageSigningFailed.KeyUnavailableSignerreturnsSIGNER_KEY_UNAVAILABLE_PREFIX, but the wrapper adds text before that marker. The native error mapper cannot recognize the condition, so a missing private key becomesErrorUnknowninstead of the typed key-unavailable error.Expose key unavailability as a typed
Signer::Errorcondition and map it toMessageSigningKeyUnavailablebefore adding context. Do not make the native boundary parse arbitrary formatted reasons. Updatesigner_key_unavailable_is_not_preserved_during_message_signingto assert the typed result.The PR objective requires typed native errors across the boundary.
#!/usr/bin/env bash set -euo pipefail rg -n -C 8 \ 'SIGNER_KEY_UNAVAILABLE_PREFIX|MessageSigningKeyUnavailable|MessageSigningFailed|ErrorUnknown|code 31' \ packages/rs-platform-wallet \ packages/rs-platform-wallet-ffi \ packages/rs-unified-sdk-jni \ packages/kotlin-sdk || trueAlso applies to: 550-620
🤖 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/sign_message.rs` around lines 218 - 247, Update sign_ecdsa error handling in the message-signing flow to preserve key-unavailable as a typed Signer::Error condition, mapping it to MessageSigningKeyUnavailable before adding signer context; map all other failures to MessageSigningFailed as before. Keep recognition typed at the native boundary rather than parsing formatted reason strings, and update signer_key_unavailable_is_not_preserved_during_message_signing to assert the typed result.
129-191: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep message signing inside the wallet-generation lifecycle.
The
wallet_managerread guard ends at Line 191. The method then awaitssigner.sign_ecdsaat Lines 241-243 without acquiring the wallet-generation operation gate. A same-ID replacement or teardown can start after path lookup and before signing completes. The stale call can then sign with the old path after its generation is gone, or race cleanup of a generation-owned signer context.Acquire the same generation guard used by other wallet operations before the lookup and hold it through the signer await. Add a same-ID replacement and teardown regression test.
The PR objective requires wallet-generation gates to protect teardown and stale actions.
#!/usr/bin/env bash set -euo pipefail rg -n -C 10 \ 'WalletGeneration|generation|sign_message|sign_ecdsa|quiesce|destroy|remove_wallet' \ packages/rs-platform-wallet/src \ packages/rs-platform-wallet-ffi/src || trueAlso applies to: 241-247
🤖 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/sign_message.rs` around lines 129 - 191, Update sign_message to acquire the wallet-generation operation guard before the wallet_manager lookup and retain it through signer.sign_ecdsa and completion of the signer await. Reuse the existing generation-gate mechanism used by other wallet operations so same-ID replacement and teardown cannot invalidate the signing generation. Add regression coverage for signing during same-ID replacement and teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/core/sign_message.rs`:
- Around line 218-247: Update sign_ecdsa error handling in the message-signing
flow to preserve key-unavailable as a typed Signer::Error condition, mapping it
to MessageSigningKeyUnavailable before adding signer context; map all other
failures to MessageSigningFailed as before. Keep recognition typed at the native
boundary rather than parsing formatted reason strings, and update
signer_key_unavailable_is_not_preserved_during_message_signing to assert the
typed result.
- Around line 129-191: Update sign_message to acquire the wallet-generation
operation guard before the wallet_manager lookup and retain it through
signer.sign_ecdsa and completion of the signer await. Reuse the existing
generation-gate mechanism used by other wallet operations so same-ID replacement
and teardown cannot invalidate the signing generation. Add regression coverage
for signing during same-ID replacement and teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f2f8add-eba2-4bc9-9bfe-a554b3fd6e21
📒 Files selected for processing (17)
packages/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/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/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/manager.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/sign_message.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/rs-platform-wallet/src/wallet/core/mod.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt
- packages/rs-platform-wallet-ffi/src/manager.rs
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
- packages/rs-unified-sdk-jni/src/wallet_manager.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
- packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt
- packages/rs-platform-wallet/src/test_support.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs
QuantumExplorer
previously approved these changes
Aug 6, 2026
… resolution The union resolution of the ManagedCoreWallet.kt conflict dropped the /** opener of nextReceiveAddress's KDoc, glueing it onto the deferred broadcastSignedPayment block and breaking compileDebugKotlin. Also moves the registry test module's stranded use statement into its import block (CodeRabbit nitpick). :sdk:compileDebugKotlin and :sdk:testDebugUnitTest verified green locally this time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…2-dev merge resolution Same class as the ManagedCoreWallet.kt repair: the union resolution of ManagedPlatformWallet.kt's second conflict dropped the closing brace of releaseReservation(payment:) and the /** opener of the hasNoUnpairedSurrogate KDoc. Verified with a real compiler run this time — the earlier local 'verification' was doubly broken (no JAVA_HOME in the shell, so gradle never started, and the pipeline read head's exit code) — :sdk:compileDebugKotlin and :sdk:testDebugUnitTest now BUILD SUCCESSFUL with checked exit codes under JAVA_HOME + ANDROID_HOME. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
approved these changes
Aug 6, 2026
QuantumExplorer
pushed a commit
that referenced
this pull request
Aug 6, 2026
…roadcast Rebased onto v4.2-dev post-#4308 (squash) — the six pre-rebase commits collapse to their verified end-state tree; full history on the PR. Mirrors the deferred registry-token age policy on the V2 handle path: RESERVATION_MAX_AGE_BLOCKS (20; key-wallet TTL 24) and reservation_expired() live in wallet::reservations, shared by both surfaces. broadcast_finalized_transaction refuses with StaleReservation (FFI ErrorStaleReservationToken, 34) before touching the broadcaster once the reservation's stamp height has aged past the bound. An aged abandon/free releases OWNER-GUARDED — release_reservation_if_owner is safe at any age and frees still-owned inputs for an immediate rebuild — with the by-outpoint skip retained only for token-less builds (#4308 review finding a579bd40062f extended to this surface). Boundary tests cover both account types on the platform and FFI layers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 tasks
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continues #4185 — moved from a fork branch to an in-repo branch (rebased onto v4.2-dev post-#4305) so maintainers can push changes directly, per review request. Full review history on #4185.
Splits transaction build from broadcast for BIP70-style deferred submission: a signed-payment registry with reservation release, a deferred-payment token bounded to the reservation's lifetime, native code 27 for stale reservation tokens, token sweeping only when the final wallet write wins, and routing of deferred builds through the atomic finalize-and-register path — across
rs-platform-wallet,platform-wallet-ffi,rs-unified-sdk-jni, and the Kotlin SDK surface.Re-opens #4090 which was auto-closed when the #3999 base branch was deleted; rebased onto
v4.1-dev. All seven original commits replayed cleanly — no hunks needed to be dropped as already-absorbed.Verified:
cargo test -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jniall pass (504 / 229 / 10);:sdk:assembleRelease+ sdk unit tests pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Why the token registry instead of the V2 handle surface
The deferred BIP70/BIP270 flow uses the reservation-token registry rather than the V2 finalized-transaction handle for two concrete reasons. First, ownership and cleanup: the token is wrapped in an owning,
AutoCloseableKotlin object with a GC/Cleanerbackstop, so a payment that is signed but then abandoned — the merchant server never acks, the user backs out, or the coroutine is cancelled after the native registration returned — always releases its funding reservation, for free, without the caller having to remember to abandon a handle. Second, the token path carries a lifetime bound the V2 handle does not: it stamps each token with the reservation's own pre-signing height and refuses to act once that reservation could have aged into key-wallet's TTL sweep, so a slow external signer can never let a stale token spend against an outpoint the wallet already swept and re-selected. A pinned V2CoreWallethandle has no such age guard and would keep the old wallet actionable indefinitely. Both paths now share one wallet-generation identity and one teardown policy, so the V2 surface stays correct for the immediate send it was built for while the deferred flow gets the GC-safe, age-bounded ownership it needs. A follow-up adds the age guard to the V2 handle path itself (it becomes live the moment iOS does deferred sends).Review-response summary (2026-07-21)
All five lifetime findings addressed as merge blockers, one commit each, with regression tests:
platform_wallet_destroyreleases the generation's reservations against the still-live wallet; actual generation removal drops tokens and V2 handles — token cleanup is now tied to wallet-generation removal.SignedCoreTransactionandregisteruses it — no post-signing resample; boundary test pins the TTL margin.CoreWallet::is_same_generation(per-generation identity) is checked by BOTH the V2-handle and registry-token paths, with one teardown policy.SignedCoreTransactionis an owningAutoCloseablewith aNativeCleanerbackstop; round-2 adds object-owningbroadcastSigned/releaseReservationoverloads that hold the object reachable across the native call and disarm the backstop on consumption (the bare-token forms remain but document the reachability requirement).broadcastpeeks and consumes atomically under one lock hold (network I/O outside the lock); a wrong-wallet caller leaves the owner's token untouched, pinned by test.Also per review: the dead
core_wallet_signed_payment_registerfour-layer chain is deleted; the single stale-token code is split into typed siblings 27ErrorStaleReservationToken/ 28ErrorReservationTokenConsumed/ 29ErrorReservationWalletMismatch(code 26 is not used by this PR — upstream now owns it asErrorTransactionBroadcastRejected; both the Kotlin and Swift enums on this branch map 27/28/29 explicitly); the stalebuildSignedPaymentKDoc is fixed.Local test evidence (fork PRs skip the Rust CI suite):
platform-wallet --lib508 passed,platform-wallet-ffi --lib197 passed, clippy/fmt clean, Kotlin:sdk:testDebugUnitTestgreen.