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>
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 was referenced Aug 7, 2026
Open
bfoss765
added a commit
that referenced
this pull request
Aug 10, 2026
…dcast Rebased down to the age-guard onto current v4.2-dev: the #4185/#4308 stack it was riding merged, and #4323/#4325 renamed the finalized- transaction surface (the v2 suffix is gone), so the guard now lands on core_wallet_broadcast_signed_transaction and the slice-based finalize_transaction signature. Mirrors the deferred registry-token age policy on the finalized-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 — and the refusal reconciles the reservation on the way out, exactly like the registry's stale-token branch: the FFI wrapper has already consumed the opaque handle, so no follow-up abandon is possible, and the owner- guarded release (safe at any age; a no-op once ownership transferred) frees the still-owned inputs for the instructed immediate rebuild. Abandon/free likewise release owner-guarded at any age, with the by-outpoint skip retained only for token-less builds. Boundary tests cover both account types on the platform and FFI layers, including the terminal FFI stale-broadcast path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
5 tasks
bfoss765
added a commit
that referenced
this pull request
Aug 11, 2026
The proposed-allocations table named the fork-era owners (#4184, #4185, #4204, #4247, #4256), all closed when the estate was recreated in-repository. Ownership now names the active successors (29 -> #4316, 32 -> #4310, 33 -> #4311, 34-36 -> #4308, 37 -> #4313, carriers updated), the no-code inventory is marked as the fork-era snapshot it is, and the provenance base is date-stamped instead of claiming to be current. Collision history keeps the fork-era numbers — it is record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
that referenced
this pull request
Aug 11, 2026
The proposed-allocation era this file tracked has largely settled: #4308 merged the reservation trio (34-36, 438153d), #4348 took 37-40 (6373e00) and #4360 took 41 (e0b8baa) - all now in the merged table. Codes 29/32/33 lapse to RESERVED (their in-repo owners closed without merging), the shielded-invite claim loses 37 to merged ABI and takes the frontier on revival, the frontier moves 38 -> 42, the #4196 'still outstanding' note is corrected to its 2026-08-03 close-out, and the 2026-08-04 heads-of-record table is labeled the historical snapshot it is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
that referenced
this pull request
Aug 11, 2026
The 'all branches clear' conclusion was true of the surveyed heads but merged #4308 later added Rust and Swift comments calling 28 and 30 free, against rule 1 and the primary table. The registry stays authoritative until an in-tree follow-up corrects them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.