feat(platform-wallet): token-minting finalize from a funding path (spendable DashPay receival accounts) - #4256
Conversation
|
🕓 Ready for review — 2 ahead in queue (commit 862036b) |
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (39)
📝 WalkthroughWalkthroughThis PR adds a deferred "build-sign-later-broadcast" Core L1 payment workflow across the Rust wallet core, FFI, JNI, Kotlin SDK, and Swift SDK, using opaque reservation tokens. It adds a signed-payment registry, generation-bound wallet identity checks, funding-domain isolation guardrails, new error codes, and a dependency revision bump. ChangesDeferred Signed Core Payment Workflow
Dependency Revision Bump
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant KotlinSDK as Kotlin/Swift SDK
participant FFI as Rust FFI
participant Registry as SignedPaymentRegistry
participant Network
Client->>KotlinSDK: buildSignedPaymentWithToken(recipients)
KotlinSDK->>FFI: core_wallet_build_signed_payment_with_token
FFI->>Registry: register(signed tx, reservation)
Registry-->>FFI: token
FFI-->>KotlinSDK: token, txid, fee, change
KotlinSDK-->>Client: SignedCoreTransaction
Client->>KotlinSDK: broadcastSigned(transaction)
KotlinSDK->>FFI: core_wallet_signed_payment_broadcast(token)
FFI->>Registry: broadcast(token)
Registry->>Network: send transaction bytes
Network-->>Registry: accepted or rejected
Registry-->>FFI: Txid or SignedPaymentError
FFI-->>Client: txid or typed error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The single-account funding and owner-guarded reservation plumbing is generally coherent, but the fee cap relies on the 100,000-byte standard transaction limit without enforcing that limit. Oversized recipient lists can therefore overflow key-wallet's fee arithmetic, so this PR requires changes; several additional lifecycle, concurrency, FFI, test, and documentation issues remain as non-blocking follow-ups.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/send.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/send.rs:295-306: Fee cap does not prevent overflow for oversized recipient lists
`MAX_FEE_PER_KB` is safe only when the transaction is at most 100,000 bytes, but this method never enforces that size limit. The pinned key-wallet builder includes roughly `outputs.len() * 34` in its base size and then evaluates `sat_per_kb * size_bytes` with unchecked `u64` multiplication. At the accepted maximum rate, about 25,835 outputs produce an estimated size around 878,434 bytes and overflow that multiplication. Such a recipient list fits in a practical JNI blob. Overflow-checking builds can panic inside the `extern "C"` path, while release builds wrap the fee and may return a signed, non-standard transaction with a fee unrelated to the requested rate. Reject transactions whose estimated size exceeds the standard limit before invoking key-wallet, and ensure the final selected-input size is also bounded or calculated with checked arithmetic.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:308-311: Wallet-manager write lock remains held while awaiting the signer
`wm` is a named `tokio::sync::RwLockWriteGuard` that remains live through the later `build_signed_reserved(...).await` at lines 509-511. The selected account borrow ends before signing, but the manager's exclusive lock does not. A slow hardware or keystore signer therefore blocks wallet synchronization and every other manager read or build; a signer that re-enters this wallet can deadlock. Follow `finalize_transaction`'s established pattern: assemble and reserve the unsigned transaction inside a scoped manager lock, drop the guard, then await signing while retaining the owner token needed to release the reservation on failure.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:116-140: FinalizedCorePayment does not enforce its reservation lifecycle
This type represents a linear obligation: exactly one registration or abandonment should take responsibility for its held reservation. However, it derives `Clone`, exposes all coupled bookkeeping fields publicly, and `abandon_payment` only borrows it. Safe callers can abandon one clone and register another, register the same payment more than once, or pair the transaction with an unrelated funding path or owner token. That can expose a registered payment after its inputs have already become selectable or mint multiple registry tokens capable of broadcasting the same transaction. Make the finalized payment opaque and non-`Clone`, and expose consuming registration and abandonment operations that accept the payment as one inseparable value.
In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet.rs:395-427: Wallet cleanup is not atomic with deferred-payment registration
Handle removal, the final-alias check, and the registry sweep use separate synchronization boundaries. A concurrent finalize can clone the wallet before destruction, then insert its registry entry after `release_entries_for_wallet` has collected matching entries, leaving a token and reservation behind after the final alias is gone. The opposite ordering can let cleanup remove a newly registered entry while the finalize call is still returning its token, so the caller receives an already-stale token. `platform_wallet_manager_remove_wallet` has the analogous remove-then-sweep window. Coordinate generation closing state, registration, alias counts, and cleanup under one lifecycle boundary, or make registration atomically reject a generation that has begun closing.
In `packages/rs-platform-wallet-ffi/src/core_wallet/send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/send.rs:246-254: Initialize all owned C out-parameters before fallible work
Only `out_token` receives a sentinel value before parsing and wallet operations. Failures from funding-path parsing, handle lookup, recipient decoding, or signing leave `out_txid`, `out_tx_bytes`, `out_tx_len`, `out_fee`, and `out_change` unchanged. The JNI caller happens to pass zero-initialized locals, but this `extern "C"` API is independently public; a generated C or Swift caller that performs cleanup on an error can observe a stale pointer/length pair and free or consume old output state. After validating all out-pointers, initialize the owned pointers to null and all scalar outputs to zero before any fallible operation.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:293-310: Add coverage for the change-bearing registration blob
The new funding-path API uses a distinct wire layout with `changeDuffs` inserted before the txid, and decoding it with the existing registration decoder silently mis-frames every subsequent field. `SignedCoreTransactionTest` exercises only `fromRegisterBlob`; it never calls `fromPaymentRegisterBlob` or verifies `changeDuffs`. Add a focused decoder test using the JNI packing order and assert the token, fee, change, txid, and raw transaction bytes.
| let mut wm = self.wallet_manager.write().await; | ||
| let (wallet, info) = wm | ||
| .get_wallet_and_info_mut(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; |
There was a problem hiding this comment.
🟡 Suggestion: Wallet-manager write lock remains held while awaiting the signer
wm is a named tokio::sync::RwLockWriteGuard that remains live through the later build_signed_reserved(...).await at lines 509-511. The selected account borrow ends before signing, but the manager's exclusive lock does not. A slow hardware or keystore signer therefore blocks wallet synchronization and every other manager read or build; a signer that re-enters this wallet can deadlock. Follow finalize_transaction's established pattern: assemble and reserve the unsigned transaction inside a scoped manager lock, drop the guard, then await signing while retaining the owner token needed to release the reservation on failure.
source: ['codex']
There was a problem hiding this comment.
Deferring this one, and I want to be explicit about why rather than just deferring it — I attempted the change and backed it out.
The obvious form of the fix (drop wm once the builder is constructed, as the sibling CodeRabbit comment proposes) is not safe here, and would introduce a double-spend rather than a latency win. set_funding only snapshots the account's UTXOs and clones the Arc<ReservationSet>; both coin selection and the reserve() call happen inside build_signed_reserved (key-wallet's assemble_unsigned, where reservations.reserve(&outpoints, height) runs after selection). So dropping the guard before that call lets two concurrent finalizes each snapshot the same unreserved UTXO under their own lock hold, then both select and reserve it — two signed transactions spending one coin. It compiles and the single-threaded tests pass, which is exactly why I'm flagging it.
Your own wording is the correct shape — "assemble and reserve the unsigned transaction inside a scoped manager lock, drop the guard, then await signing" — and that's what finalize_transaction does. It isn't reachable from here: key-wallet's assemble_unsigned is private to its module, and there is no build_unsigned_reserved counterpart to build_signed_reserved. Splitting reserve-from-sign on this path needs a key-wallet API addition, which I don't think belongs in this PR.
What did change in 34c0894: the guard is now released immediately after the build rather than at end of scope. That doesn't shorten the signing hold, but it's what makes the new over-limit refusal able to call abandon_payment at all — that re-acquires the manager lock, so under the guard it would deadlock.
Happy to open the key-wallet-side issue for the build_unsigned_reserved split if you want this tracked rather than deferred.
There was a problem hiding this comment.
Resolved in 34c0894 — Wallet-manager write lock remains held while awaiting the signer no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| #[derive(Debug, Clone)] | ||
| pub struct FinalizedCorePayment { | ||
| /// The signed transaction. | ||
| pub transaction: Transaction, | ||
| /// The fee paid, in duffs — derived from the transaction itself | ||
| /// (`inputs − outputs`), the same ground truth [`SignedCorePayment::fee`] | ||
| /// uses. | ||
| pub fee: u64, | ||
| /// Duffs returned to the wallet's BIP44 change address (0 when the build | ||
| /// produced no change output). | ||
| pub change_amount: u64, | ||
| /// The ONE account the inputs were selected from and reserved in, as its | ||
| /// RESOLVED account-level derivation path — never the caller's `None`. A | ||
| /// later release must name this account, not the default BIP44 one. | ||
| pub funding: FundingAccountRef, | ||
| /// The wallet's `last_processed_height` captured in the funding critical | ||
| /// section, i.e. the exact clock `set_current_height` stamped the | ||
| /// reservation with. The registry's age guard must baseline off this — see | ||
| /// [`SignedCoreTransaction::reservation_height`](crate::SignedCoreTransaction::reservation_height). | ||
| pub reservation_height: u32, | ||
| /// The key-wallet [`ReservationToken`](key_wallet::ReservationToken) stamped | ||
| /// onto the selected inputs, so a later release is *owner-guarded* and frees | ||
| /// only inputs this build still owns (`dashpay/platform#4185`). `None` only | ||
| /// if the build reserved nothing, which the funded path never does. | ||
| pub reservation_token: Option<KeyWalletReservationToken>, |
There was a problem hiding this comment.
🟡 Suggestion: FinalizedCorePayment does not enforce its reservation lifecycle
This type represents a linear obligation: exactly one registration or abandonment should take responsibility for its held reservation. However, it derives Clone, exposes all coupled bookkeeping fields publicly, and abandon_payment only borrows it. Safe callers can abandon one clone and register another, register the same payment more than once, or pair the transaction with an unrelated funding path or owner token. That can expose a registered payment after its inputs have already become selectable or mint multiple registry tokens capable of broadcasting the same transaction. Make the finalized payment opaque and non-Clone, and expose consuming registration and abandonment operations that accept the payment as one inseparable value.
source: ['codex']
There was a problem hiding this comment.
Half done in 34c0894, half deferred with a reason.
Done: FinalizedCorePayment no longer derives Clone, and abandon_payment now takes it by value. Those two together close the cases you listed that are expressible today — abandoning one copy and registering another, abandoning twice, or minting two registry tokens able to broadcast the same transaction, all now fail to compile rather than being caught in review. Only two call sites existed (the FFI's CString::new failure arm and the new over-limit refusal), so the change was contained.
Deferred: making the type opaque with consuming registration. The blocker is that register_funded_by is #4185's API, not this PR's, and it takes the pieces separately:
register_funded_by(core, tx, funding, registered_height, funding_reservation_token)That signature is precisely what allows pairing a transaction with an unrelated funding path or owner token — so the real fix is folding those four parameters into one consuming register(payment). Doing that from this branch would fork a sibling PR's public surface while both are in review, and privatising the fields without it would only add accessors and close nothing (the FFI would still hand the four pieces over individually).
I've documented the reasoning on the type itself so the pub fields don't read as an oversight. If you'd rather it land as one change, the natural home is #4185 — happy to take it there.
There was a problem hiding this comment.
Resolved in 34c0894 — FinalizedCorePayment does not enforce its reservation lifecycle no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let Some(wallet) = PLATFORM_WALLET_STORAGE.remove(handle) else { | ||
| return PlatformWalletFFIResult::ok(); | ||
| }; | ||
|
|
||
| // `platform_wallet_manager_get_wallet` hands out an independent handle for | ||
| // each alias of the same wallet *generation* (they share the underlying | ||
| // `WalletManager` `Arc`, `wallet_id`, and the per-generation balance `Arc`). | ||
| // A deferred-payment token minted through one alias must NOT be invalidated | ||
| // when a *sibling* alias of the same generation is destroyed — the token is | ||
| // still live and broadcastable through the survivor. | ||
| // | ||
| // So only reconcile when THIS is the final live alias of the generation: no | ||
| // other stored handle is the same generation | ||
| // (`CoreWallet::is_same_generation`). While a sibling is live, the | ||
| // destructor just drops this handle. | ||
| // | ||
| // Once the last alias goes, RELEASE (not merely drop) each of this | ||
| // generation's deferred-payment reservations: destroying the last wrapper | ||
| // handle does NOT remove the logical wallet from its manager, so the wallet | ||
| // — and its accounts' still-live `ReservationSet`s — remain, and the same | ||
| // wallet can be handed out again. Dropping the tokens without releasing | ||
| // would leave those inputs reserved until key-wallet's TTL. Releasing here | ||
| // also frees the registry's `CoreWallet` pin on the shared `WalletManager`. | ||
| // (Actual generation teardown — `remove_wallet` — instead drops the tokens, | ||
| // since the reservation ceases to exist with the generation.) | ||
| let core = wallet.core(); | ||
| let sibling_alias_alive = | ||
| PLATFORM_WALLET_STORAGE.any(|other| other.core().is_same_generation(core)); | ||
| if !sibling_alias_alive { | ||
| runtime().block_on( | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .release_entries_for_wallet(core), | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Wallet cleanup is not atomic with deferred-payment registration
Handle removal, the final-alias check, and the registry sweep use separate synchronization boundaries. A concurrent finalize can clone the wallet before destruction, then insert its registry entry after release_entries_for_wallet has collected matching entries, leaving a token and reservation behind after the final alias is gone. The opposite ordering can let cleanup remove a newly registered entry while the finalize call is still returning its token, so the caller receives an already-stale token. platform_wallet_manager_remove_wallet has the analogous remove-then-sweep window. Coordinate generation closing state, registration, alias counts, and cleanup under one lifecycle boundary, or make registration atomically reject a generation that has begun closing.
source: ['codex']
There was a problem hiding this comment.
Not changed — this one I don't think I can fix correctly from this PR. The race described is between platform_wallet_destroy's alias/generation bookkeeping and the registry sweep, both of which are #4185's lifecycle machinery; closing it properly means coordinating generation-closing state, registration, alias counts and cleanup under one boundary, which is a change to that PR's design rather than an addition to this one. This PR only adds a new producer of registry entries, so it widens an existing window rather than opening it. Flagging rather than silently deferring — if you'd like it tracked, #4185 is the right home.
There was a problem hiding this comment.
Resolved in this update — Wallet cleanup is not atomic with deferred-payment registration no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| check_ptr!(outputs_blob); | ||
| check_ptr!(core_signer_handle); | ||
| check_ptr!(out_token); | ||
| check_ptr!(out_txid); | ||
| check_ptr!(out_tx_bytes); | ||
| check_ptr!(out_tx_len); | ||
| check_ptr!(out_fee); | ||
| check_ptr!(out_change); | ||
| *out_token = 0; |
There was a problem hiding this comment.
🟡 Suggestion: Initialize all owned C out-parameters before fallible work
Only out_token receives a sentinel value before parsing and wallet operations. Failures from funding-path parsing, handle lookup, recipient decoding, or signing leave out_txid, out_tx_bytes, out_tx_len, out_fee, and out_change unchanged. The JNI caller happens to pass zero-initialized locals, but this extern "C" API is independently public; a generated C or Swift caller that performs cleanup on an error can observe a stale pointer/length pair and free or consume old output state. After validating all out-pointers, initialize the owned pointers to null and all scalar outputs to zero before any fallible operation.
source: ['codex']
There was a problem hiding this comment.
Not changed in this round — agreed in principle, deferring on scope. The finding is accurate that only out_token gets a sentinel and that the in-repo JNI caller zero-initialises its locals, so there is no live defect. It's a real hardening gap for an independently-public extern "C" surface though. Grouping it with the sibling transaction_builder.rs / signed_payment.rs instances CodeRabbit raised, since they're the same one-line convention (core_wallet_broadcast_signed_transaction_v2 already does it) and are better fixed together than piecemeal across three PRs. Say the word if you'd rather it land here.
There was a problem hiding this comment.
Resolved in 34c0894 — Initialize all owned C out-parameters before fallible work no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| internal fun fromPaymentRegisterBlob(blob: ByteArray): SignedCoreTransaction { | ||
| val buffer = java.nio.ByteBuffer.wrap(blob) // big-endian by default | ||
| val token = buffer.long | ||
| val feeDuffs = buffer.long | ||
| val changeDuffs = buffer.long | ||
| val txidLen = buffer.int | ||
| val txidBytes = ByteArray(txidLen) | ||
| buffer.get(txidBytes) | ||
| val txBytesLen = buffer.int | ||
| val rawTxBytes = ByteArray(txBytesLen) | ||
| buffer.get(rawTxBytes) | ||
| return SignedCoreTransaction( | ||
| txidHex = String(txidBytes, Charsets.UTF_8), | ||
| rawTxBytes = rawTxBytes, | ||
| feeDuffs = feeDuffs, | ||
| reservationToken = token, | ||
| changeDuffs = changeDuffs, | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Add coverage for the change-bearing registration blob
The new funding-path API uses a distinct wire layout with changeDuffs inserted before the txid, and decoding it with the existing registration decoder silently mis-frames every subsequent field. SignedCoreTransactionTest exercises only fromRegisterBlob; it never calls fromPaymentRegisterBlob or verifies changeDuffs. Add a focused decoder test using the JNI packing order and assert the token, fee, change, txid, and raw transaction bytes.
source: ['codex']
There was a problem hiding this comment.
Not added in this round. Agreed the gap is real — SignedCoreTransactionTest only exercises fromRegisterBlob, and the payment blob's distinct layout (changeDuffs before the txid) would mis-frame every subsequent field if the wrong decoder were used. Deferring only because the Rust-side changes in this round were the funds-critical ones and I did not want to mix a Kotlin decoder test into the same commit. Noting it explicitly so it isn't lost: the test wants to assert token, fee, change, txid and raw bytes against the JNI packing order.
| /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) | ||
| /// could not cover the requested outputs plus fee from the union of the | ||
| /// wallet's *signable* funds accounts (BIP44 + BIP32 + CoinJoin + DashPay | ||
| /// receiving; watch-only DashPay external accounts are excluded). `available` | ||
| /// is the total selectable value across those accounts, `required` the | ||
| /// outputs-plus-fee target — carried as exact duff amounts (instead of being | ||
| /// flattened into a string) so callers can render a precise shortfall. |
There was a problem hiding this comment.
💬 Nitpick: Insufficient-funds error still documents union funding
The implementation deliberately confines selection to one caller-selected account, and map_send_builder_error reports that account's selectable value. This public error documentation still states that available is a union across all signable accounts, which gives downstream callers the opposite contract and may cause UI or retry logic to imply that privacy domains will be combined automatically.
| /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) | |
| /// could not cover the requested outputs plus fee from the union of the | |
| /// wallet's *signable* funds accounts (BIP44 + BIP32 + CoinJoin + DashPay | |
| /// receiving; watch-only DashPay external accounts are excluded). `available` | |
| /// is the total selectable value across those accounts, `required` the | |
| /// outputs-plus-fee target — carried as exact duff amounts (instead of being | |
| /// flattened into a string) so callers can render a precise shortfall. | |
| /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) | |
| /// could not cover the requested outputs plus fee from the single funding | |
| /// account selected by the caller. `available` is the total selectable value | |
| /// in that account, and `required` is the outputs-plus-fee target — carried | |
| /// as exact duff amounts so callers can render a precise shortfall without | |
| /// implying that another funding domain will be used automatically. |
source: ['codex']
There was a problem hiding this comment.
Same item as the CodeRabbit thread on error.rs — fixed in 34c0894. The variant doc now states single-account available/required and drops the union language.
There was a problem hiding this comment.
Resolved in 34c0894 — Insufficient-funds error still documents union funding no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/rs-platform-wallet/src/test_support.rs (1)
265-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated wallet-assembly tail into a shared helper.
funded_coinjoin_wallet_manager,split_funded_wallet_manager, andsplit_funded_wallet_manager_dashpayeach end with the same sequence: buildWalletSigner, wrapWalletBalance, assemblePlatformWalletInfo, createWalletManager::<PlatformWalletInfo>::new, andinsert_wallet. This mirrors the pre-existing tail infunded_wallet_manager_with_outputs(lines 247-263). As this fixture module keeps growing (four near-identical tails now), extracting a small private helper that takes the fundedTestWalletContextand returns the assembled manager/wallet-id/balance/signer tuple would reduce duplication and keep future fixtures consistent.♻️ Proposed helper extraction
fn finalize_test_wallet( ctx: TestWalletContext, ) -> ( Arc<RwLock<WalletManager<PlatformWalletInfo>>>, WalletId, Arc<WalletBalance>, WalletSigner, ) { let signer = WalletSigner { wallet: ctx.wallet.clone(), }; let balance = Arc::new(WalletBalance::new()); let info = PlatformWalletInfo { core_wallet: ctx.managed_wallet, balance: Arc::clone(&balance), identity_manager: IdentityManager::new(), tracked_asset_locks: BTreeMap::new(), }; let mut wm = WalletManager::<PlatformWalletInfo>::new(Network::Testnet); let wallet_id = wm.insert_wallet(ctx.wallet, info).expect("insert wallet"); (Arc::new(RwLock::new(wm)), wallet_id, balance, signer) }Callers that only need a 3-tuple (
split_funded_wallet_manager,split_funded_wallet_manager_dashpay) can drop thebalanceelement.Also applies to: 336-422, 467-642
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/test_support.rs` around lines 265 - 334, Extract the repeated wallet-assembly logic from the tail of funded_coinjoin_wallet_manager, split_funded_wallet_manager, and split_funded_wallet_manager_dashpay (which all build WalletSigner, wrap WalletBalance, assemble PlatformWalletInfo, create WalletManager::new, and call insert_wallet) into a private helper function that accepts a TestWalletContext and returns the tuple (Arc<RwLock<WalletManager<PlatformWalletInfo>>>, WalletId, Arc<WalletBalance>, WalletSigner). Replace the duplicate code in each of these functions with a single call to this helper, and apply the same refactoring to the existing tail in funded_wallet_manager_with_outputs to keep all fixtures consistent.packages/rs-platform-wallet/src/wallet/core/send.rs (2)
211-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
build_signed_paymentintentionally drops the reservation bookkeeping.This method discards
funding,reservation_height, andreservation_tokenfrom theFinalizedCorePayment. TheFinalizedCorePaymentdoc (lines 108-112) states that dropping one without registering or abandoning it strands the reservation until key-wallet's TTL backstop. The module docs (lines 29-44) explain that the stranding is intentional here, because the host broadcasts and a later sync reconciles the spend. A reader arriving at this projection sees the drop without that context. Add one sentence so the two contracts do not read as contradictory.📝 Proposed doc note
/// * `funding_path` — the account-level derivation path of the SINGLE funds /// account whose UTXOs fund the payment. `None` (the default) funds from /// the unmixed BIP44 account (dashpay/platform#4184). + /// + /// ## Reservation bookkeeping is intentionally dropped + /// + /// This projection discards the resolved [`FundingAccountRef`], the + /// reservation height, and key-wallet's owner token. The selected inputs + /// STAY reserved, and no token exists to release them early — the + /// build-only contract in the module docs: the host broadcasts, and a sync + /// (or the reservation TTL) reconciles the reservation. Callers that may + /// abandon the payment must use + /// [`finalize_signed_payment_from_funding_path`](Self::finalize_signed_payment_from_funding_path) + /// and [`abandon_payment`](Self::abandon_payment) instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/core/send.rs` around lines 211 - 226, Add a concise documentation sentence to build_signed_payment explaining that it intentionally omits FinalizedCorePayment reservation bookkeeping because the host broadcasts the transaction and a later sync reconciles the spend. Reference the discarded funding, reservation_height, and reservation_token fields without changing the payment-building logic.
1176-1294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
abandon_payment.The tests cover the registry release path (
registry.release(token)) and the rejected-broadcast release path. They do not coverabandon_payment, which thefinalize_signed_payment_from_funding_pathdoc names as one of the two required ways to discharge the reservation. It is the path a host takes when marshalling fails between the build and a successfulregister_funded_by— see theabandon_paymentcall inpackages/rs-platform-wallet-ffi/src/core_wallet/send.rs.A test in the shape of
receival_reservation_is_held_and_released_against_the_receival_accountwould cover it: build from the receival path on the wallet's own generation, confirm a second build is blocked, callcore.abandon_payment(&payment), then confirm the rebuild succeeds. That also pins the by-path release for a non-Standard account without the registry in the loop.Do you want me to generate that test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/core/send.rs` around lines 1176 - 1294, Add a dedicated test alongside receival_reservation_is_held_and_released_against_the_receival_account covering CoreWallet::abandon_payment: build a receival-funded payment using the wallet’s own generation, verify a second build is blocked by the reservation, call core.abandon_payment with the original payment, and verify rebuilding from the same receival path succeeds without involving SignedPaymentRegistry.packages/rs-platform-wallet-ffi/src/core_wallet/send.rs (2)
246-254: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInitialize every out-parameter before the first fallible step.
core_wallet_build_signed_payment_with_tokenonly zeroes*out_token. On every error return,*out_txid,*out_tx_bytes,*out_tx_len,*out_fee, and*out_changekeep whatever the caller passed in. The in-repo JNI caller pre-initializes its locals, so it is safe today. Any other C caller can read uninitialized memory.core_wallet_broadcast_signed_transaction_v2already nulls its out-pointer up front; follow that convention here.🛡️ Proposed fix
check_ptr!(out_change); *out_token = 0; + *out_txid = std::ptr::null_mut(); + *out_tx_bytes = std::ptr::null_mut(); + *out_tx_len = 0; + *out_fee = 0; + *out_change = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/core_wallet/send.rs` around lines 246 - 254, Update core_wallet_build_signed_payment_with_token to initialize every validated out-parameter immediately after the check_ptr! calls and before any fallible operation, setting pointer outputs to null and scalar outputs to zero as appropriate. Preserve the existing out_token initialization and follow the initialization convention used by core_wallet_broadcast_signed_transaction_v2.
329-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the token variant uses the same free function.
core_wallet_build_signed_payment_with_tokenalso writesout_tx_bytes/out_tx_len, and the JNI layer frees them withcore_wallet_free_payment_bytes. Name both producers in the doc comment so the ownership contract is unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/core_wallet/send.rs` around lines 329 - 333, Update the safety documentation for the payment-byte free function associated with core_wallet_free_payment_bytes to name both core_wallet_build_signed_payment and core_wallet_build_signed_payment_with_token as valid producers of the bytes/length pair, preserving the existing null/zero ownership contract.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt (1)
450-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo
buildSignedPaymentoverloads return different types with different reservation semantics.
buildSignedPayment(recipients, network, coreSignerHandle, accountType, accountIndex)returns aSignedCoreTransactionthat owns a reservation token. This overload returns aSignedCorePaymentwith no token. Overload resolution works, because the positional types differ. The hazard is at the call site: both names are identical, both move money, and only one produces a broadcastable, reservation-owning result.The KDoc at Line 479-484 already has to explain which
buildSignedPaymentis which, and the KDoc at Line 546 and Line 595 refer to "[buildSignedPayment]" without saying which overload.Rename the tokenless variant to state what it returns, for example
buildSignedPaymentBytes. Then update the KDoc cross-references inbroadcastSignedandreleaseReservationto name one specific method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt` around lines 450 - 455, Rename the tokenless `buildSignedPayment` overload to `buildSignedPaymentBytes` while preserving its existing parameters and `SignedCorePayment` result. Update all references and KDoc, including the cross-references in `broadcastSigned` and `releaseReservation`, to distinguish this method from the reservation-owning `buildSignedPayment` overload.packages/rs-platform-wallet-ffi/src/wallet.rs (1)
471-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe baseline-delta assertions depend on a process-global registry and can flake.
SIGNED_PAYMENT_REGISTRYis process-global andcargo testruns tests in parallel threads inside one process. Another test in this crate that registers or releases a token between thebaselinecapture and eachassert_eq!shiftsoutstanding()and fails this test for an unrelated reason. The comment already acknowledges the registry is shared.Assert on the specific token instead of the global count, or serialize the registry-touching tests behind a shared
Mutex.♻️ Sketch: assert on the token, not the count
- assert_eq!(SIGNED_PAYMENT_REGISTRY.outstanding(), baseline + 1); - (manager, handle_a, handle_b, baseline) + (manager, handle_a, handle_b, _token) }); let result = unsafe { platform_wallet_destroy(handle_a) }; assert_eq!(result.code, PlatformWalletFFIResultCode::Success); - assert_eq!( - SIGNED_PAYMENT_REGISTRY.outstanding(), - baseline + 1, - "a sibling alias's token must survive destroying another alias" - ); + assert!( + SIGNED_PAYMENT_REGISTRY.contains(token), + "a sibling alias's token must survive destroying another alias" + );This needs a token-presence predicate on
SignedPaymentRegistry; add one if it does not exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/wallet.rs` around lines 471 - 508, Replace the process-global count assertions in the wallet alias-destruction test with assertions targeting the specific token returned by SIGNED_PAYMENT_REGISTRY.register. Add or reuse a SignedPaymentRegistry token-presence predicate, then verify that token remains after destroying handle_a and is absent after destroying handle_b; remove the baseline/outstanding-based checks.packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs (1)
183-191: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe new deferred-payment entry points rely on the caller to pre-zero out-parameters. Both functions validate their out-pointers with
check_ptr!but do not write a default value to all of them before the first fallible step. The current JNI caller zeroes itsFFICoreTransactionbox and itsout_txidlocal, so no defect exists today. The guarantee belongs in the FFI, as the existingcore_wallet_broadcast_signed_transaction_v2already does with*out_txid = std::ptr::null_mut();.
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs#L183-L191: write*out_txid = std::ptr::null_mut();and zero*out_tx(nulltx_bytes, zerotx_len/fee) next to the existing*out_token = 0;, so the error returns at Line 198 and Line 206 leave defined values.packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs#L58-L63: add*out_txid = std::ptr::null_mut();immediately aftercheck_ptr!(out_txid);.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs` around lines 183 - 191, Initialize every deferred-payment out-parameter before fallible operations: in packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs#L183-L191, update the function containing the shown check_ptr! calls to null out_txid and zero out_tx fields (tx_bytes, tx_len, and fee) alongside *out_token = 0; in packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs#L58-L63, update the corresponding function to null out_txid immediately after check_ptr!(out_txid);.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/error.rs`:
- Around line 74-86: Update the PaymentInsufficientFunds doc comment to state
that available is the selected account’s spendable total, and that required is
the outputs-plus-fee target for that account. Remove references to the union of
signable funding accounts and total selectable value across domains; preserve
the exact-duff and recovery semantics.
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Around line 522-535: The comment describing the fee safety behavior around the
selected_input_value calculation is inverted. Update the comment to correctly
state that if an input somehow cannot be priced and defaults to 0, this lowers
the selected_input_value sum, which in turn makes the fee calculated via
saturating_sub smaller and therefore UNDER-reported (not over-reported). Keep
the explanation that the scenario is impossible because every spendable UTXO is
priced beforehand, but fix the direction of the safety claim to match the actual
mathematical consequence.
- Around line 406-421: Update the funding-account resolution in the send flow to
fail closed when no wallet-level account matching funding_path is found. Remove
the unwrap_or(&bip44_acc) fallback from the funding_wallet_acc lookup, and
propagate an appropriate error before calling set_funding so a different
account’s BIP44 xpub cannot be used.
In `@packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- Around line 1622-1629: Update the KDoc for
core_wallet_signed_payment_broadcast to document ErrorStaleReservationToken as
27, ErrorReservationTokenConsumed as 28, and ErrorReservationWalletMismatch as
29, matching the assignments in error.rs.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 450-455: Rename the tokenless `buildSignedPayment` overload to
`buildSignedPaymentBytes` while preserving its existing parameters and
`SignedCorePayment` result. Update all references and KDoc, including the
cross-references in `broadcastSigned` and `releaseReservation`, to distinguish
this method from the reservation-owning `buildSignedPayment` overload.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/send.rs`:
- Around line 246-254: Update core_wallet_build_signed_payment_with_token to
initialize every validated out-parameter immediately after the check_ptr! calls
and before any fallible operation, setting pointer outputs to null and scalar
outputs to zero as appropriate. Preserve the existing out_token initialization
and follow the initialization convention used by
core_wallet_broadcast_signed_transaction_v2.
- Around line 329-333: Update the safety documentation for the payment-byte free
function associated with core_wallet_free_payment_bytes to name both
core_wallet_build_signed_payment and core_wallet_build_signed_payment_with_token
as valid producers of the bytes/length pair, preserving the existing null/zero
ownership contract.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 183-191: Initialize every deferred-payment out-parameter before
fallible operations: in
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs#L183-L191,
update the function containing the shown check_ptr! calls to null out_txid and
zero out_tx fields (tx_bytes, tx_len, and fee) alongside *out_token = 0; in
packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs#L58-L63,
update the corresponding function to null out_txid immediately after
check_ptr!(out_txid);.
In `@packages/rs-platform-wallet-ffi/src/wallet.rs`:
- Around line 471-508: Replace the process-global count assertions in the wallet
alias-destruction test with assertions targeting the specific token returned by
SIGNED_PAYMENT_REGISTRY.register. Add or reuse a SignedPaymentRegistry
token-presence predicate, then verify that token remains after destroying
handle_a and is absent after destroying handle_b; remove the
baseline/outstanding-based checks.
In `@packages/rs-platform-wallet/src/test_support.rs`:
- Around line 265-334: Extract the repeated wallet-assembly logic from the tail
of funded_coinjoin_wallet_manager, split_funded_wallet_manager, and
split_funded_wallet_manager_dashpay (which all build WalletSigner, wrap
WalletBalance, assemble PlatformWalletInfo, create WalletManager::new, and call
insert_wallet) into a private helper function that accepts a TestWalletContext
and returns the tuple (Arc<RwLock<WalletManager<PlatformWalletInfo>>>, WalletId,
Arc<WalletBalance>, WalletSigner). Replace the duplicate code in each of these
functions with a single call to this helper, and apply the same refactoring to
the existing tail in funded_wallet_manager_with_outputs to keep all fixtures
consistent.
In `@packages/rs-platform-wallet/src/wallet/core/send.rs`:
- Around line 211-226: Add a concise documentation sentence to
build_signed_payment explaining that it intentionally omits FinalizedCorePayment
reservation bookkeeping because the host broadcasts the transaction and a later
sync reconciles the spend. Reference the discarded funding, reservation_height,
and reservation_token fields without changing the payment-building logic.
- Around line 1176-1294: Add a dedicated test alongside
receival_reservation_is_held_and_released_against_the_receival_account covering
CoreWallet::abandon_payment: build a receival-funded payment using the wallet’s
own generation, verify a second build is blocked by the reservation, call
core.abandon_payment with the original payment, and verify rebuilding from the
same receival path succeeds without involving SignedPaymentRegistry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9903f29f-5993-444c-9324-0009848c7b5a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/send.rspackages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/handle.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/utils.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/send.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/rs-platform-wallet/src/wallet/funding_privacy.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rspackages/rs-unified-sdk-jni/src/funding.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
|
Re the AccountBalanceEntryFFI stride concern (field appended for derivationPath, inherited here from #4247's branch): the only shipping consumer of this struct today is the Android JNI, which marshals it to JSON inside the same Rust build — producer and consumer always share one struct definition, so there is no cross-version stride exposure on Android. The iOS/Swift side does consume the C ABI directly, so any Swift binding regeneration must happen against the updated header before adopting these revs — called out in #4255 (the iOS adoption issue). If versioned structs are preferred as policy we're happy to follow up, but for this stack the append-only change plus the #4255 regeneration note covers the practical risk. 🤖 Generated with Claude Code |
… finalized payment linear Addresses the review findings on dashpay#4256. Fee-cap overflow (BLOCKING). `MAX_FEE_PER_KB = MAX_MONEY / 100` was only safe if the transaction stayed under Dash's 100_000-byte standard limit, which this method never enforced. key-wallet's `FeeRate::calculate_fee` then evaluates `sat_per_kb * size_bytes` with unchecked `u64` multiplication, overflowing at ~878 kB — reachable by a ~25.8k-recipient list that fits in a practical JNI blob. Three-part fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`) using checked arithmetic that mirrors key-wallet's own base-size formula, before the wallet lock and before anything is reserved; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, making the product representable for ANY size a `u32` can express. The previous bound depended on a size limit that was never enforced; this one does not depend on the input count at all, which also closes the CoinJoin-account case (a few thousand small denominations reach ~878 kB with no oversized recipient list); * the signed transaction is re-measured and refused if it exceeds the limit, since the input count is unknowable until coin selection has run. That refusal discharges the reservation it had already taken via `abandon_payment` rather than stranding the account's coins until the TTL backstop. `FinalizedCorePayment` linearity (partial). The type no longer derives `Clone` and `abandon_payment` now consumes it, so abandoning-then-registering, or minting two registry tokens able to broadcast the same transaction, no longer compiles. The remaining half — folding the four separate `register_funded_by` parameters into one consuming `register(payment)` — belongs on dashpay#4185, which owns that API; doing it here would fork a sibling PR's surface. Wallet-manager write lock: NOT dropped before signing (deferred, see the PR thread). `set_funding` only snapshots UTXOs and clones the `Arc<ReservationSet>` — selection and `reserve()` both run inside `build_signed_reserved`, so dropping the guard first would let two concurrent finalizes reserve the same outpoint and produce two signed transactions spending one coin. The narrower "reserve under the lock, sign outside" split needs a key-wallet API that does not exist (`assemble_unsigned` is private; there is no `build_unsigned_reserved`). The guard IS now released immediately after the build, before the release paths, which is what makes the in-function `abandon_payment` non-deadlocking. Docs: corrects the JNI `core_wallet_signed_payment_broadcast` result codes (27/28/29, was 26/27/28 — the Kotlin mirror was already right) and an inverted fee-direction comment. Tests: 537 passed (528 baseline + 9), platform-wallet-ffi 223 passed, Kotlin `:sdk:testDebugUnitTest` green, `cargo check -p platform-wallet-ffi -p rs-unified-sdk-jni` clean, `cargo fmt --check` clean. New coverage for the over-limit refusal on both the output side and the post-build side (the latter also pinning that the refusal releases its reservation), `abandon_payment` without the registry, dust rejection, MAX_MONEY aggregation and the fee-rate bound. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed The blocker — fee cap vs. unenforced size limit — is fixed. The finding was right that
One thing I want to flag rather than bury. I implemented the write-lock suggestion, then backed it out. Dropping
Also fixed: the fail-closed lookup (verified empirically first — Deferred with reasons on their threads: FFI out-parameter initialisation (grouped with the two sibling instances — same one-line convention, better fixed together); the wallet-cleanup/registration atomicity window (#4185's lifecycle machinery — this PR widens it, doesn't open it); and the Suites: New tests: over-limit refusal on the output side (the 25,835-recipient case at max fee rate) and on the post-build side (900 recipients × 500 UTXOs → ~496 inputs, just over 100 kB — the only test that reaches the post-build check, and the one that would deadlock rather than fail if the guard weren't released before 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the prior fee-overflow blocker and the stale insufficient-funds documentation. It introduces one new blocking error-contract issue: signer failures are now reported as request-invalid transaction builds; two carried-forward suggestions remain valid, while three broader lifecycle or C-ABI hardening items are explicitly deferred to their owning follow-up work.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/rust-quality=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/error.rs:407-408: Do not classify signing failures as request-invalid transaction builds
`map_send_builder_error` converts every non-shortfall `BuilderError` into `PlatformWalletError::TransactionBuild`, including `BuilderError::SigningFailed`. The new blanket mapping here consequently returns native code 32, whose Rust and Kotlin contracts state that the request itself is invalid and that retrying it unchanged cannot succeed. That is false for the production `MnemonicResolverCoreSigner`: a missing Keychain mnemonic or a resolver callback failure becomes `SigningFailed`, and key-wallet releases the owner-stamped input reservation on this path. Restoring or unlocking the signer can therefore make the same recipients, amount, fee, and funding path succeed. Keep signing failures out of `ErrorTransactionBuild`; map them to a signer-specific platform error and FFI result code, such as the code-31 signing-key contract reserved by the sibling stack.
Note: GitHub does not allow PastaClaw to approve or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
…ilds `build_signed_payment` reserves its selected inputs and leaves them reserved on success, expecting a broadcast to follow. Nothing across Rust/FFI/JNI/Kotlin let a caller that declines to commit give those coins back, so an abandoned build stranded them for RESERVATION_TTL_BLOCKS (24, ~1h) — and indefinitely while the wallet has no processed height, since `ReservationSet::sweep` early-returns at height 0 and therefore never reclaims a pre-sync reservation. A single abandoned build on a freshly restored wallet could strand the whole balance for the life of the process. Adds a standalone release across all four layers: CoreWallet::release_payment_reservation(&Transaction, Option<DerivationPath>) core_wallet_release_payment_reservation (FFI) coreWalletReleasePaymentReservation (JNI) ManagedPlatformWallet.releasePaymentReservation (Kotlin) The transaction is the ownership signal: a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released — releasing its inputs releases precisely this build's own reservation and can never free a competing build's coins. Same signal the internal `release_reservation_after_rejected_broadcast` cleanup already uses. The release consults no height, so it works pre-sync where the TTL backstop cannot. It is idempotent (per-outpoint map removal) and a silent no-op after a successful broadcast — it cannot resurrect a spent coin, since selection reads the UTXO set that sync already updated — so callers can wire it into an unconditional cleanup path. Also routes the build's default-funding-account resolution through the shared `bip44_account_path` helper, so a release and its build can never disagree about what `funding_path: None` means. Tests: release-then-reselect (with the second-build failure pinned as a precondition so it can't pass vacuously), release twice, release after a processed broadcast, release against the wrong account frees nothing, unknown funding path is refused, and the height-0 case — 30 build attempts prove the TTL never fires there, then the explicit release frees the inputs. Addresses review item 3 on dashpay#4247. The reviewer's suggestion to unify this with dashpay#4256's reservation-token finalize is tracked separately rather than done here, to avoid reshaping an API the Android app already calls and hard-coupling this PR to dashpay#4185. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The latest delta fixes the prior blocking signing-error contract by separating signing failures into PlatformWalletError::TransactionSigning, FFI code 33, and the retryable Kotlin type; no new defect was verified in that delta. Two carried-forward suggestions remain at the exact head: the wallet-manager write lock still spans the external signer callback, and the payment-specific Kotlin registration blob still lacks decoder coverage.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/rust-quality=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/rust-quality=claude-sonnet-5(failed); claude/ffi-engineer=claude-sonnet-5(failed); claude/general=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(completed); claude/ffi-engineer=claude-sonnet-5(completed); claude/general=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(completed); claude/ffi-engineer=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
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— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/send.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/send.rs:639-660: Release the wallet-manager lock before invoking the signer
The wallet-manager write guard acquired at line 421 remains live through `build_signed_reserved(...).await` and is dropped only at line 660. Production signing crosses into the host mnemonic resolver, so a slow Keystore/Keychain callback blocks every reader and writer using this manager; a callback that re-enters an operation requiring the manager can deadlock. The inline claim that a narrower split is unavailable is incorrect for the exact pinned key-wallet revision `e99959ced0062159d629930f488374e29f63c42b`: `TransactionBuilder::build_unsigned_reserved` and `TransactionSigner::sign_tx` are public, and `wallet/core/transaction.rs:210-315` already demonstrates the required sequence—assemble and reserve under the lock, reconstruct the selected UTXOs and paths, drop the guard, sign, and perform owner-guarded cleanup on failure. Apply that sequence here to preserve atomic selection without retaining the lock across external code.
| let (transaction, _estimated_fee, reservation_token) = builder | ||
| .build_signed_reserved(signer, move |addr| path_map.get(&addr).cloned()) | ||
| .await | ||
| .map_err(|e| map_send_builder_error(e, selectable_value, outputs_total))?; | ||
|
|
||
| // The manager write lock MUST span the build: `set_funding` only | ||
| // snapshots the account's UTXOs and clones the `Arc<ReservationSet>` — | ||
| // coin selection and `reserve()` both happen inside | ||
| // `build_signed_reserved` (key-wallet `assemble_unsigned`). Dropping the | ||
| // guard before this call would let two concurrent finalizes each | ||
| // snapshot the same unreserved UTXO under their own lock hold and then | ||
| // both select and reserve it, yielding two signed transactions that | ||
| // spend the same coin. See the reviewer note on this method for why the | ||
| // narrower "reserve under the lock, sign outside" split is not available | ||
| // here (key-wallet's `assemble_unsigned` is private and there is no | ||
| // `build_unsigned_reserved`). | ||
| // | ||
| // Released HERE, before any of the release paths below: the reservation | ||
| // now exists and is owner-stamped, so nothing further needs exclusivity, | ||
| // and `abandon_payment` re-acquires the manager lock itself — calling it | ||
| // under this guard would deadlock (tokio's `RwLock` is not reentrant). | ||
| drop(wm); |
There was a problem hiding this comment.
🟡 Suggestion: Release the wallet-manager lock before invoking the signer
The wallet-manager write guard acquired at line 421 remains live through build_signed_reserved(...).await and is dropped only at line 660. Production signing crosses into the host mnemonic resolver, so a slow Keystore/Keychain callback blocks every reader and writer using this manager; a callback that re-enters an operation requiring the manager can deadlock. The inline claim that a narrower split is unavailable is incorrect for the exact pinned key-wallet revision e99959ced0062159d629930f488374e29f63c42b: TransactionBuilder::build_unsigned_reserved and TransactionSigner::sign_tx are public, and wallet/core/transaction.rs:210-315 already demonstrates the required sequence—assemble and reserve under the lock, reconstruct the selected UTXOs and paths, drop the guard, sign, and perform owner-guarded cleanup on failure. Apply that sequence here to preserve atomic selection without retaining the lock across external code.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Release the wallet-manager lock before invoking the signer no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…30 (dashpay#4256) dashpay#4256 is stacked on dashpay#4185 and still carried the pre-renumber `29`, which now collides with `ErrorAssetLockInsufficientFunds = 29` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and the `ErrorReservationWalletMismatch` family moves to 30; dashpay#4185 already made that move in `d854debb`. This brings dashpay#4256 in line. CI could not have caught this: dashpay#4256 and dashpay#4184 are both MERGEABLE with green checks, because two branches assigning the same discriminant produce no textual conflict. It surfaces only as an E0081 after a textual merge, or silently as a wrong error code on the host. The discriminant is public ABI, so every mirror moves together: - Rust enum + its rustdoc cross-reference (error.rs) - doc reference in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value - Kotlin fromPlatformWalletNative branch, class KDoc, 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 corrects this PR's own numbering rationale on `ErrorTransactionSigning` (33), which claimed 30 was "reserved for dashpay#4184's ErrorAssetLockCrossDomainConsentRequired". That code does not exist on any branch — dashpay#4184 dropped it in a re-scope — and 30 is now ErrorReservationWalletMismatch. dashpay#4256's codes are unchanged otherwise: it keeps 32 (ErrorTransactionBuild, shared with dashpay#4247) and 33. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 805 passed / 0 failed; :sdk:test BUILD SUCCESSFUL with DashSdkErrorTest 9/9. Swift is unverified — it cannot be compiled here.
…cord dashpay#4196 scope Clears the two review blockers on dashpay#4261 and re-syncs the registry with what the code on each branch actually does, re-read at every head rather than trusted from this file. Blocker (a) — dashpay#3968 / dashpay#3954 / dashpay#4259 were described in prose but had no rows, which is exactly what rule 2 forbids. They now have them: - A "Non-conforming allocations" table for dashpay#3968 (26/27/28) and dashpay#3954 (27). These are deliberately kept out of the proposed table: each row is a claim to be withdrawn and reissued, not an allocation of record. - An inherited-code table for the 31 that dashpay#4204 and dashpay#4259 carry but did not allocate (dashpay#4183 owns it), so it is not double-counted. - dashpay#4196 is recorded as claiming no integer at all: it routes a new token-less `StaleReservation` variant through the existing `ErrorStaleReservationToken`. The dashpay#3968 half is the serious one and is called out as such. Its 28 is not a new claim — it *moves the already-shipped* `ErrorTransactionBroadcastRejected` off 26 to make room for its own persister code. Rule 3 forbids that: a host compiled against merged ABI returns 26 for a broadcast rejection, and after dashpay#3968 the same condition returns 28 while 26 means a transient persister failure. Neither branch's diff shows the contradiction. Blocker (b) — 30 marked both free and assigned was already resolved by the preceding commit; verified consistent here (30 is allocated to dashpay#4185 throughout, frontier is 34, and the one remaining "genuinely free" is past tense explaining why dashpay#4185 could take it). Also corrected, all verified against the branches: - Survey provenance had dashpay#4185 at `0b0d5c76d6` labelled "(post-renumber)". Wrong twice: that commit is the *parent* of the renumber `d854debb`, and the head has since moved to `6c37e8679e`. dashpay#4184, dashpay#4247 and dashpay#4256 SHAs refreshed too. - dashpay#4256 has now taken 30 (`9481e5783b`) and dropped its stale "30 is reserved for the consent code" rationale; the equivalent comments on dashpay#4183 and dashpay#4204 are flagged as still present. - dashpay#4184 has a comment-only drift: it reserves "Codes 27-28" but names three codes. Correct when the trio was 27/28/29; it is now 27/28/30. Its discriminant is right and is the resolution of record — only the prose is stale, and dashpay#4184 is left untouched. - The dashpay#4196 section now records why the restack has not happened: its three own commits conflict in 3 files / 10 hunks against dashpay#4185's head, and the registry redesign underneath it (mandatory `registered_height`, new `WalletRemoved` variant, owner-stamped funding token) makes it author work rather than conflict resolution. Its trio numbers come from the dashpay#4185 copy it carries, so the restack fixes 28 -> 30 for free; the number dashpay#4196 itself must chase is 27, not 30. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 738 passed / 0 failed. Docs-only change.
|
Heads-up on a silent cross-branch collision, now fixed in This branch defined Per the registry in #4261 the resolution is: #4184 keeps 29, the reservation-mismatch code moves to 30 (#4185 already made that move in 10 numeric sites across 7 files: Also corrected a stale rationale here: the
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The latest delta correctly moves ErrorReservationWalletMismatch from 29 to 30 across the Rust FFI enum, JNI/Kotlin mapping and documentation, Swift mirror, and Kotlin mapping test; no stale mapping or functional ABI mismatch was found. Both carried-forward prior suggestions remain valid: the wallet-manager write lock still spans the external signer callback, and the payment-specific Kotlin blob decoder still lacks coverage. Two new non-blocking delta findings remain: the Rust ABI value is not directly pinned by a regression test, and the new documentation references a registry file that is absent from this PR's tree.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/rust-quality=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(failed); claude/ffi-engineer=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
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— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:276-279: Pin the renumbered public ABI discriminant in Rust tests
This delta changes the `#[repr(C)]` public ABI value of `ErrorReservationWalletMismatch` from 29 to 30, but the Rust FFI tests do not assert the new numeric value. The Kotlin test independently hardcodes 30 and therefore cannot detect a future Rust-only drift; the Rust and host test suites could both pass while disagreeing at runtime. Neighboring public allocations already have `transaction_build_code_is_thirty_two` and `transaction_signing_code_is_thirty_three` regression tests for this purpose. Add the equivalent assertion that `PlatformWalletFFIResultCode::ErrorReservationWalletMismatch as i32 == 30`.
| /// Note: 29 is taken by `ErrorAssetLockInsufficientFunds` | ||
| /// (`dashpay/platform#4184`); this code is 30. See | ||
| /// `packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md`. | ||
| ErrorReservationWalletMismatch = 30, |
There was a problem hiding this comment.
🟡 Suggestion: Pin the renumbered public ABI discriminant in Rust tests
This delta changes the #[repr(C)] public ABI value of ErrorReservationWalletMismatch from 29 to 30, but the Rust FFI tests do not assert the new numeric value. The Kotlin test independently hardcodes 30 and therefore cannot detect a future Rust-only drift; the Rust and host test suites could both pass while disagreeing at runtime. Neighboring public allocations already have transaction_build_code_is_thirty_two and transaction_signing_code_is_thirty_three regression tests for this purpose. Add the equivalent assertion that PlatformWalletFFIResultCode::ErrorReservationWalletMismatch as i32 == 30.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Pin the renumbered public ABI discriminant in Rust tests no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// 30 was previously described here as reserved for #4184's | ||
| /// `ErrorAssetLockCrossDomainConsentRequired`. That code does not exist on | ||
| /// any branch — #4184 dropped it in a re-scope — and 30 is now | ||
| /// `ErrorReservationWalletMismatch`. See | ||
| /// `packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md`. |
There was a problem hiding this comment.
💬 Nitpick: Do not reference an error-code registry that is absent from this tree
The new rustdoc points readers to packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md, and the Kotlin mapping comment adds the same path, but that file does not exist at this PR's exact head. Repository history shows it only on the separate, unmerged #4261 branch at commit bb37a7082c, which is not an ancestor of this branch. Either include the registry here or reference #4261 explicitly so contributors inspecting this commit do not encounter a dangling source-of-truth path.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — Do not reference an error-code registry that is absent from this tree no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lease A deferred send reserves its funding inputs at build, awaits the broadcast, and on a definitive rejection releases the reservation for an immediate rebuild. That release was unconditional (release-by-outpoint): during the broadcast await, key-wallet's TTL sweep can reclaim the reservation and a concurrent build can re-reserve the same outpoint under a new token, so the by-outpoint release would free that other build's inputs — the dashpay#4185 release/re-reserve double-spend window. Capture the key_wallet::ReservationToken build_unsigned_reserved stamps onto the selected inputs, carry it on SignedCoreTransaction alongside reservation_height, thread it through the deferred registry (RegisteredPayment / register / broadcast / reconcile) and broadcast_payment_releasing_reservation, and release via ManagedCoreFundsAccount::release_reservation_if_owner so a rejected or abandoned send frees only inputs its own build still owns. The finalize sign-failure path (a platform-side await between reserve and release) is owner-guarded the same way. None (no reservation taken) keeps the old by-outpoint fallback, never reached on the funded finalize path. Adds a regression test: a rejected deferred broadcast whose outpoint was swept and re-reserved under a new token leaves that new reservation intact. Docs name the shared generation identity's sibling V2 handle path (dashpay#4196). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…per-destroy from consuming payments, type the token Addresses two of the three carried-forward lifecycle blockers on dashpay#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 (dashpay#4185 review) Addresses the two new thepastaclaw blockers plus the Kotlin owner-construction suggestion on PR dashpay#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 (dashpay#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 dashpay#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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At the exact head, two of the four carried-forward findings remain in scope: the Kotlin payment-blob decoder is still untested, and the manager-lock finding is now blocking because the latest merge removed the explicit guard drop before abandon_payment, creating a self-deadlock on oversized signed transactions. Three additional in-scope blockers remain in the new path-funded flow: registration can race wallet teardown, the public registry API does not preserve linear reservation ownership, and Kotlin cancellation can discard a minted token without deterministic cleanup; the two prior ABI/documentation findings are outdated after the restack because their code is inherited unchanged from parent #4185.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/rust-quality=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/core_wallet/send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/send.rs:317-327: Gate path-funded registration against wallet teardown
Finalization releases its manager write guard before returning, but this code registers the payment without taking the wallet generation's payment guard or checking `is_current_generation`. A concurrent `remove_wallet_with_teardown` can therefore remove the generation and finish sweeping its existing registry entries before this insertion; the function then publishes a fresh token for a removed or re-created wallet that the completed sweep can never see. The sibling finalized-transaction FFI path takes `generation_payment_guard`, checks liveness, abandons on failure, and keeps the guard through registration. Apply the same sequence here so teardown cannot interleave between the liveness check and token publication.
In `packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:474-493: Consume the finalized payment when registering path-funded reservations
This safe public API accepts separately cloneable or copyable pieces instead of consuming the `FinalizedCorePayment` ownership object. A caller can register cloned fields and then call `abandon_payment(payment)`, leaving a live registry token whose inputs have already become selectable; it can also register the same transaction twice, release one token, build a conflicting transaction, and still broadcast the old transaction through the second token. Callers may additionally pair a transaction with an unrelated funding path or owner token, causing cleanup to target the wrong reservation. The method is also `async` despite containing no await, so dropping its future before polling discards the moved bookkeeping without inserting a registry entry or reconciling the reservation. Make `FinalizedCorePayment` opaque, include its originating generation marker, and provide a synchronous registration method that consumes the entire value, validates its generation, and derives every stored field internally.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:533-555: Release the minted token when coroutine cancellation discards the result
This token-minting overload uses plain `gate.op`, while the existing builder-driven overload uses `opWithCleanupOnCancellation`. The blocking JNI call cannot observe coroutine cancellation and completes native registration before returning its blob. If cancellation occurs while JNI is running, the prompt-cancellation handoff from `withContext` can discard the newly constructed `SignedCoreTransaction` before the caller receives it. Only the nondeterministic `NativeCleaner` then owns the token, leaving the funding inputs reserved until GC or the reservation TTL—and indefinitely at processed height zero. Use the same deterministic `payment.close()` cancellation cleanup as the other token-returning overload.
Note: GitHub does not allow PastaClaw to approve or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
| let token = runtime().block_on( | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY.register_funded_by( | ||
| core.clone(), | ||
| payment.transaction.clone(), | ||
| payment.funding.clone(), | ||
| // The height sampled inside the funding critical section, mandatory | ||
| // since #4185: the age guard must baseline off the build's own clock. | ||
| payment.reservation_height, | ||
| payment.reservation_token, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Gate path-funded registration against wallet teardown
Finalization releases its manager write guard before returning, but this code registers the payment without taking the wallet generation's payment guard or checking is_current_generation. A concurrent remove_wallet_with_teardown can therefore remove the generation and finish sweeping its existing registry entries before this insertion; the function then publishes a fresh token for a removed or re-created wallet that the completed sweep can never see. The sibling finalized-transaction FFI path takes generation_payment_guard, checks liveness, abandons on failure, and keeps the guard through registration. Apply the same sequence here so teardown cannot interleave between the liveness check and token publication.
| let token = runtime().block_on( | |
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY.register_funded_by( | |
| core.clone(), | |
| payment.transaction.clone(), | |
| payment.funding.clone(), | |
| // The height sampled inside the funding critical section, mandatory | |
| // since #4185: the age guard must baseline off the build's own clock. | |
| payment.reservation_height, | |
| payment.reservation_token, | |
| ), | |
| ); | |
| let (_lifecycle, wallet_is_live) = runtime().block_on(async { | |
| let gate = core.generation_payment_guard().await; | |
| let live = core.is_current_generation().await; | |
| (gate, live) | |
| }); | |
| if !wallet_is_live { | |
| runtime().block_on(core.abandon_payment(payment)); | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::NotFound, | |
| "wallet is no longer registered in the manager (removed or re-created while the payment was being signed); the payment was not registered and its reservation was reconciled".to_string(), | |
| ); | |
| } | |
| let token = runtime().block_on( | |
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY.register_funded_by( | |
| core.clone(), | |
| payment.transaction.clone(), | |
| payment.funding.clone(), | |
| payment.reservation_height, | |
| payment.reservation_token, | |
| ), | |
| ); |
source: ['codex']
| pub async fn register_funded_by( | ||
| &self, | ||
| core: CoreWallet<B>, | ||
| tx: Transaction, | ||
| funding: FundingAccountRef, | ||
| registered_height: u32, | ||
| funding_reservation_token: Option<FundingReservationToken>, | ||
| ) -> ReservationToken { | ||
| let token = ReservationToken(self.next_token.fetch_add(1, Ordering::SeqCst)); | ||
| self.lock().insert( | ||
| token, | ||
| RegisteredPayment { | ||
| core, | ||
| tx, | ||
| funding, | ||
| registered_height, | ||
| funding_reservation_token, | ||
| }, | ||
| ); | ||
| token |
There was a problem hiding this comment.
🔴 Blocking: Consume the finalized payment when registering path-funded reservations
This safe public API accepts separately cloneable or copyable pieces instead of consuming the FinalizedCorePayment ownership object. A caller can register cloned fields and then call abandon_payment(payment), leaving a live registry token whose inputs have already become selectable; it can also register the same transaction twice, release one token, build a conflicting transaction, and still broadcast the old transaction through the second token. Callers may additionally pair a transaction with an unrelated funding path or owner token, causing cleanup to target the wrong reservation. The method is also async despite containing no await, so dropping its future before polling discards the moved bookkeeping without inserting a registry entry or reconciling the reservation. Make FinalizedCorePayment opaque, include its originating generation marker, and provide a synchronous registration method that consumes the entire value, validates its generation, and derives every stored field internally.
source: ['codex']
| suspend fun buildSignedPaymentWithToken( | ||
| recipients: List<Pair<String, Long>>, | ||
| coreSignerHandle: Long, | ||
| feePerKb: Long = 0, | ||
| fundingPath: String? = null, | ||
| ): SignedCoreTransaction = gate.op { | ||
| require(recipients.isNotEmpty()) { "recipients must not be empty" } | ||
| require(recipients.all { it.second > 0 }) { "every recipient amount must be positive" } | ||
| require(feePerKb >= 0) { "feePerKb must be non-negative, got $feePerKb" } | ||
|
|
||
| val outputsBlob = encodePaymentOutputs(recipients) | ||
| mapNativeErrors { | ||
| coreWallet().use { core -> | ||
| SignedCoreTransaction.fromPaymentRegisterBlob( | ||
| core.buildSignedPaymentWithToken( | ||
| outputsBlob, | ||
| feePerKb, | ||
| coreSignerHandle, | ||
| fundingPath, | ||
| ), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Release the minted token when coroutine cancellation discards the result
This token-minting overload uses plain gate.op, while the existing builder-driven overload uses opWithCleanupOnCancellation. The blocking JNI call cannot observe coroutine cancellation and completes native registration before returning its blob. If cancellation occurs while JNI is running, the prompt-cancellation handoff from withContext can discard the newly constructed SignedCoreTransaction before the caller receives it. Only the nondeterministic NativeCleaner then owns the token, leaving the funding inputs reserved until GC or the reservation TTL—and indefinitely at processed height zero. Use the same deterministic payment.close() cancellation cleanup as the other token-returning overload.
| suspend fun buildSignedPaymentWithToken( | |
| recipients: List<Pair<String, Long>>, | |
| coreSignerHandle: Long, | |
| feePerKb: Long = 0, | |
| fundingPath: String? = null, | |
| ): SignedCoreTransaction = gate.op { | |
| require(recipients.isNotEmpty()) { "recipients must not be empty" } | |
| require(recipients.all { it.second > 0 }) { "every recipient amount must be positive" } | |
| require(feePerKb >= 0) { "feePerKb must be non-negative, got $feePerKb" } | |
| val outputsBlob = encodePaymentOutputs(recipients) | |
| mapNativeErrors { | |
| coreWallet().use { core -> | |
| SignedCoreTransaction.fromPaymentRegisterBlob( | |
| core.buildSignedPaymentWithToken( | |
| outputsBlob, | |
| feePerKb, | |
| coreSignerHandle, | |
| fundingPath, | |
| ), | |
| ) | |
| } | |
| } | |
| ): SignedCoreTransaction = gate.opWithCleanupOnCancellation( | |
| cleanup = { payment: SignedCoreTransaction -> payment.close() }, | |
| ) { |
source: ['codex']
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…the final-alias policy removal (dashpay#4185 review) Blocker 2 removed the final-alias sweep from `platform_wallet_destroy` (wallet.rs:392-414 is now just a storage `remove`), but the helper that policy was introduced for survived it. `HandleStorage::any` was added by ade3999 for that sweep and has had zero call sites since the policy was dropped. Because `handle` is a `pub mod` and the method is `pub`, no dead-code lint fires and it stayed in the crate's public Rust surface, with Rustdoc still pointing at "the final-alias check in `platform_wallet_destroy`" — a policy that no longer exists. It is not on the base branch, so removing it restores the base surface rather than breaking an existing consumer. `HandleStorage::remove_matching` is retained: it still backs the generation sweep at manager.rs:494. Also corrects a doc cross-reference to the same removed policy in `test_support::test_platform_wallet_manager`, which described the helper as backing "final-alias registry-sweep gating" when its only FFI consumer now asserts the opposite (destroying wrapper aliases must NOT sweep). No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on (dashpay#4185 review) The gate added in 0b0d5c7 lived on the FFI's process-global `SIGNED_PAYMENT_REGISTRY`, so it excluded only registry-token operations and did so across every wallet at once. Two consequences, both real: 1. Under-coverage. The V2 finalized-transaction-handle path bypassed it entirely. `core_wallet_tx_builder_finalize` awaited the external signer and then inserted into `CORE_SIGNED_TRANSACTION_V2_STORAGE` with no gate and no liveness re-check, so a teardown could sweep while signing was pending and the late finalizer published a handle no sweep would ever catch. `core_wallet_broadcast_signed_transaction_v2` then consumed such a handle and reached the broadcaster with no gate either — its `is_same_generation` check compares two HANDLES, and a removed generation matches itself. The same hole existed on the public Rust surface: `PlatformWalletManager::remove_wallet` never took the write side at all, so a direct embedder (the manager is public and `SignedPaymentRegistry` is re-exported) removed wallets with no exclusion. 2. Cross-wallet contention. A deferred broadcast holds the shared side across an SPV send; on one process-global write-preferring lock that send blocked teardown — and every payment operation queued behind the waiting writer — for every unrelated wallet in the process. Remedy: move the gate into shared per-generation state. * New `WalletGeneration` owns the lock-free `WalletBalance` AND that generation's `RwLock` lifecycle gate, and replaces `Arc<WalletBalance>` as the generation-identity marker. Folding them into one `Arc` is deliberate: the identity and the gate cannot diverge, so two handles can never compare as the same generation while excluding each other through different locks. `Deref` keeps every existing balance read unchanged. * `PlatformWalletManager::remove_wallet_with_teardown` takes that generation's exclusive gate across BOTH the removal and a caller-supplied teardown hook, and `remove_wallet` routes through it. The gate is no longer optional for any caller, FFI or not. The FFI passes its registry + V2-handle sweep as the hook. Lock order stays gate-then-manager: the lookup that resolves the gate drops `wallets` before awaiting it, then re-validates under the gate. * Every publication/network path now takes the generation's shared gate across its liveness check and the action: the registry `broadcast`/`release`, the token `core_wallet_signed_payment_finalize`, and — newly — both `core_wallet_tx_builder_finalize` and `core_wallet_broadcast_signed_transaction_v2`, which report a dead generation as the existing `NotFound` (98) after reconciling the build's reservation. V2 abandon/free stay ungated: their release is already generation-bound. The gate is still NOT held across an external signer await — finalizers acquire it only after the signature returns, so an open signing prompt cannot stall teardown, and a late finalizer instead fails its liveness check and abandons. The lifecycle comments that claimed the opposite were wrong about the code and are corrected. Adds four deterministic FFI regression tests alongside the existing three: a V2 broadcast-after-removal refusal, a teardown that waits for an in-flight V2 operation and then sweeps its handle, a public `PlatformWalletManager::remove_wallet` that waits for an in-flight payment, and a cross-wallet isolation test pinning the per-generation scoping. With the three guards removed the first three fail; with the gate re-pointed at a single process-global lock the fourth fails. No error-code changes: `ErrorReservationWalletMismatch` stays 30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y key `remove_wallet_with_teardown` validated generation G1 under G1's lifecycle gate, then removed it from the two manager maps in two independently locked stages. Registration takes no gate at all — `register_wallet` mints its own `WalletGeneration` — so from the moment the inner-manager removal frees the id, a concurrent same-id registration can publish a different generation G2 into `wallet_manager` and then into `self.wallets`, with no happens-before edge to the remover's own `self.wallets` acquisition. A remover descheduled in that gap resumed into a map naming G2 and removed the entry BY KEY: it evicted a live wallet (still registered in the inner manager, so invisible and unremovable through the public map), returned it to the caller, and handed it to `tear_down` — which sweeps that generation's registry tokens and V2 finalized-transaction handles while holding only G1's gate, i.e. with G2's payment operations not excluded. That exclusion is the one property the gate exists to provide. Retain the `Arc<PlatformWallet>` validated under the gate and remove the public-map entry only while it still pointer-matches that generation, so the removed handle, the returned handle and the `tear_down` argument are all the one generation this call validated. The inner-manager removal needs no such check: G1 can only leave `wallet_manager` through this method (which requires G1's gate) or through a rollback for an insert that could not have happened while G1 occupied the id. Regression test `removal_leaves_a_generation_registered_during_it_intact` drives the real `create_wallet_from_seed_bytes` -> `register_wallet` path from a `cfg(test)` rendezvous fired in the exact window, so the interleaving is pinned with no sleep and no completion-order race. Against the previous code it fails on all three load-bearing assertions: the returned generation, the `tear_down` argument, and the survival of the re-registered wallet in the public map. Refs dashpay#4185 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add CoreWallet::build_signed_payment — a first-class "send" primitive that
selects inputs across the UNION of every signable funds account (BIP44 +
BIP32 + CoinJoin + DashPay receiving), builds and signs a standard L1
payment, and returns the signed transaction plus its fee and change amount.
This is the build-only half of the Android send-path cutover: during the
dashj->SDK transition the app keeps dashj's transaction bookkeeping
(maybeCommitTx drives CrowdNode, memos, confidence listeners), so the SDK
must build + sign from the bound wallet and hand back the bytes while dashj
commits/broadcasts. The method therefore does NOT broadcast and does NOT
persist a debit — the only state touched is the in-memory ReservationSet
that set_funding/build_signed use to stop a concurrent SDK build from
re-selecting the same coins (released when the spend is later observed by
sync or by the reservation-TTL backstop).
Reuses the shielded asset-lock union-funding machinery
(all_funding_accounts + spendable_utxos + a spanning path resolver +
LargestFirst selection to keep CoinJoin's many small denominations from
blowing up BranchAndBound), but excludes watch-only DashpayExternalAccounts
(a contact's addresses, which this wallet cannot sign). Fee and change are
derived from the transaction itself (inputs - outputs), the always
self-consistent ground truth, rather than from build_signed's signed-size
fee recomputation which can drift by a few duffs from what is actually paid.
Adds the typed PaymentInsufficientFunds { available, required } error so a
shortfall carries the exact union-wide selectable total.
Tests: correct output/change/fee, BIP44+CoinJoin union selection, typed
union shortfall, watch-only exclusion, and input validation.
cargo test -p platform-wallet --lib: 442 passed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 38012d1)
Expose the union-funding send primitive as a public Kotlin suspend fun that returns the signed raw transaction bytes (plus fee + change) WITHOUT broadcasting — the last SDK gap for the Android wallet's send-path cutover. - FFI (rs-platform-wallet-ffi): core_wallet_build_signed_payment, a one-shot call over CoreWallet::build_signed_payment. Recipients cross as a big-endian blob (u32 count; per row u32 addrLen, addr utf8, u64 amount), each address parsed + network-checked; returns the consensus-serialized signed bytes via out-pointers plus out_fee/out_change, freed with core_wallet_free_payment_bytes. cbindgen exports both automatically. - JNI (rs-unified-sdk-jni): WalletManagerNative.coreWalletBuildSignedPayment returns a byte[] packed big-endian as (u64 fee, u64 change, tx bytes); the FFI-owned bytes are freed before returning. - Kotlin (kotlin-sdk): ManagedPlatformWallet.buildSignedPayment(recipients, coreSignerHandle, feePerKb) -> SignedCorePayment(txBytes, fee, change), serialized under the same shared per-wallet coreSendMutex as sendToAddresses so a concurrent build cannot select the same UTXO. Unlike sendToAddresses it neither broadcasts nor picks a funding account (coin selection auto-spans every signable account). ManagedCoreWallet gains the matching native call on the transient core handle. cargo check/test green across platform-wallet-ffi (149) and rs-unified-sdk-jni (2); :sdk:compileDebugKotlin BUILD SUCCESSFUL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit d18c9c0) [port to v4.2-dev] ManagedPlatformWallet.buildSignedPayment is serialized through the manager's TeardownGate (gate.op {}), matching sendToAddresses and every other native op on this branch, instead of the pre-refactor per-wallet `coreSendMutex` (which no longer exists on v4.2-dev). Concurrent builds still cannot select the same UTXO: CoreWallet::build_signed_payment holds the wallet-manager write lock across coin selection and signing.
… funding build_signed_payment funded from the union of all signable funds accounts (BIP44 + CoinJoin + …) with BIP44 change — the privacy-domain-crossing design blocked on dashpay#4184 (shumkov, 2026-07-21) and replaced there by single-account selection. This code predated that re-scope. - add funding_path: Option<DerivationPath>; None = unmixed BIP44 account 0, Some(path) = strictly the one funds account whose account path matches. No union, no cross-account accumulation; shortfall returns PaymentInsufficientFunds for that account only. Change routes to BIP44 (explicit change addr when a non-Standard account funds). - new wallet::funding_privacy guardrail: crate-wide static test fails the build if any wallet-wide funds-account iteration lacks a PRIVACY-DOMAIN-OK marker. - replace the union-asserting test with default-never-crosses-domains and explicit-path-selects-strictly tests. - review-fix hardening: bounded FFI allocation + checked cursor math, output-total overflow guard, fee-rate bound, typed PaymentInsufficientFunds (code 22). - thread funding_path through FFI/JNI/Kotlin (null = unmixed BIP44). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Balances
Adds derivation_path (Option<String>) to AccountBalanceRow, the FFI
AccountBalanceEntryFFI (appended, ABI-additive), and the JNI JSON
("derivationPath": string|null). Computed from the same
AccountType::derivation_path(network) the funding-path spend selector
compares against, so the emitted string is byte-identical to what
build_signed_payment expects — the app passes it verbatim to spend a
DashPay receival account. Null for account types with no derivable path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ts for the send hardening Addresses the review findings on dashpay#4247. Dust outputs (BLOCKING). `TransactionBuilder::add_output` applies no relay policy, so a one-duff recipient produced fully signed bytes for a transaction every standard node rejects as nonstandard — from a primitive documented as building a *standard* payment for later broadcast. Each recipient amount is now checked against its OWN destination script's `dust_value()` (546 duffs for P2PKH), before the wallet lock, before any input is reserved and before the signer is called. Fee/size overflow (BLOCKING). The `MAX_FEE_PER_KB = MAX_MONEY / 100` bound assumed the transaction stayed under the 100 kB standard limit, which the method never enforced; key-wallet's `calculate_fee` then multiplies `sat_per_kb * size_bytes` unchecked and overflows at ~878 kB — reachable both by a ~25.8k-recipient list and by a funding account with a few thousand small denominations. Two-sided fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`), with checked arithmetic mirroring key-wallet's own base-size formula; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, which makes the product unrepresentable-free for ANY size a `u32` can express and therefore does not depend on the input count. ~43 DASH/kB is still three orders of magnitude above any legitimate rate; * the signed transaction is re-measured and refused if it exceeds the standard limit. Typed build errors. `PlatformWalletError::TransactionBuild` had no FFI arm, so every `funding_path` failure — "no spendable funds account matches" and "names a watch-only account", the two failure modes the single-account design rests on — reached Kotlin as `Generic(99)` and could only be told apart by string-matching. Adds `ErrorTransactionBuild = 32` (27-31 are claimed by sibling v4.1 stack PRs, so this needs no renumbering whichever order they land) plus the Kotlin `PlatformWallet.TransactionBuild` type. Tests for the six hardening fixes, which shipped with no coverage. `core_wallet/send.rs` had no test module at all; it now covers the `count` bound, `try_reserve_exact`, checked cursor math at every field boundary, UTF-8, and wrong-network address rejection. Also covers MAX_MONEY aggregation, the fee-rate bound, `parse_optional_derivation_path`, dust rejection (including that a refused request reserves nothing), and the new size bound. Also: corrects the stale `PaymentInsufficientFunds` doc, which still described the pre-dashpay#4184 union semantics; corrects an inverted fee-direction comment; adds the missing non-empty assertion to a funding-privacy guardrail test; and runs `cargo fmt` over the five files that failed `--check`. platform-wallet 510 passed, platform-wallet-ffi 222 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ilds `build_signed_payment` reserves its selected inputs and leaves them reserved on success, expecting a broadcast to follow. Nothing across Rust/FFI/JNI/Kotlin let a caller that declines to commit give those coins back, so an abandoned build stranded them for RESERVATION_TTL_BLOCKS (24, ~1h) — and indefinitely while the wallet has no processed height, since `ReservationSet::sweep` early-returns at height 0 and therefore never reclaims a pre-sync reservation. A single abandoned build on a freshly restored wallet could strand the whole balance for the life of the process. Adds a standalone release across all four layers: CoreWallet::release_payment_reservation(&Transaction, Option<DerivationPath>) core_wallet_release_payment_reservation (FFI) coreWalletReleasePaymentReservation (JNI) ManagedPlatformWallet.releasePaymentReservation (Kotlin) The transaction is the ownership signal: a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released — releasing its inputs releases precisely this build's own reservation and can never free a competing build's coins. Same signal the internal `release_reservation_after_rejected_broadcast` cleanup already uses. The release consults no height, so it works pre-sync where the TTL backstop cannot. It is idempotent (per-outpoint map removal) and a silent no-op after a successful broadcast — it cannot resurrect a spent coin, since selection reads the UTXO set that sync already updated — so callers can wire it into an unconditional cleanup path. Also routes the build's default-funding-account resolution through the shared `bip44_account_path` helper, so a release and its build can never disagree about what `funding_path: None` means. Tests: release-then-reselect (with the second-build failure pinned as a precondition so it can't pass vacuously), release twice, release after a processed broadcast, release against the wrong account frees nothing, unknown funding path is refused, and the height-0 case — 30 build attempts prove the TTL never fires there, then the explicit release frees the inputs. Addresses review item 3 on dashpay#4247. The reviewer's suggestion to unify this with dashpay#4256's reservation-token finalize is tracked separately rather than done here, to avoid reshaping an API the Android app already calls and hard-coupling this PR to dashpay#4185. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…le doc Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd-path test fixtures Restack adaptation only — no production-code change. This branch now sits on top of dashpay#4185 (port/v4.1/split-build-broadcast) rather than beside it. dashpay#4185 replaced the bare `Arc<WalletBalance>` generation marker with `Arc<WalletGeneration>` (balance + that generation's lifecycle gate in one Arc, so "same generation" and "same gate" cannot diverge), and renamed `PlatformWalletInfo::balance` to `::generation`. The send-path test fixtures added here still built wallets the old way, so they no longer compiled against the new base. Point them at the shared `WalletGeneration` the rest of the crate already uses: * `core/send.rs` — the local `core_wallet` fixture takes `Arc<WalletGeneration>`; `funded_wallet_manager` already hands one back. * `wallet/funding_privacy.rs` — same, for its two fixtures. * `test_support.rs` — the DashPay split fixture populates `PlatformWalletInfo::generation`. cargo test -p platform-wallet -p platform-wallet-ffi: 537 + 230 pass.
…lt space `ErrorTransactionBuild = 32` was added to the Rust FFI enum and to the Kotlin decoder by this PR, but not to the Swift mirror. Swift decodes an unlisted raw value through the `default:` arm of `PlatformWalletResultCode .init(ffi:)`, so every typed build rejection this PR introduced — an unresolvable or watch-only `fundingPath`, a breached monetary bound, a malformed recipients blob — reached iOS as `errorUnknown` (99) with only the message string to distinguish it. That is the exact failure the code was split out of `ErrorUnknown` to end, reintroduced on the other host. Adds the enum case, the `init(ffi:)` mapping, and the `PlatformWalletError.transactionBuild` arm. There is no compile-time check that the raw values match Rust, so the numbering comment is kept in sync with the registry (dashpay#4261) and now records only the codes this PR does NOT own. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ ERROR_TRANSACTION_BUILD = 32. No other exhaustive switch over PlatformWalletResultCode / PlatformWalletError exists in the Swift SDK.
`cargo fmt --check` fallout from the `WalletGeneration` adaptation two commits down: the shorter type name let two fixture signatures fit differently. No behaviour change.
…endable DashPay receival accounts) Bridges the two previously-disconnected flows: build_signed_payment's single-account-by-derivation-path selection (the only path that can reach a dashpay_receival account) and the reservation/broadcast flow (token -> broadcastSigned/releaseReservation). New finalize_signed_payment_from_funding_path returns FinalizedCorePayment (tx, fee, change, resolved FundingAccountRef, reservation token); the reservation is recorded against the RESOLVED account (never a BIP44 default), so release/broadcast bookkeeping lands in the ledger the build reserved into. FundingAccountRef::Path makes non-standard accounts nameable to the release machinery. FFI core_wallet_build_signed_payment_with_token + JNI + Kotlin buildSignedPaymentWithToken(recipients, coreSignerHandle, feePerKb, fundingPath) returning the existing SignedCoreTransaction. Invariants preserved and tested: single account only (funding-privacy guardrails pass), change to BIP44/0, watch-only refused, fee from the signed tx. 3 new tests incl. reservation hold/release cycles; full send suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ceival finalize tests The receival funding-path tests need a wallet whose balance is split between BIP44 account 0 and a DashPay funds account, which `WalletAccountCreationOptions::Default` does not provision. Ports the `DashpayLeg` / `foreign_contact_account_xpub` / `split_funded_wallet_manager_dashpay` test_support fixture verbatim from the asset-lock multi-account work (dashpay#4184, commit 4655eef) so this PR does not have to stack on 2.9k lines of unrelated asset-lock production code just to reach the fixture. Test-support only — no production code is taken from dashpay#4184. The three items are `#[cfg(test)]`-gated here (unlike on dashpay#4184, where the asset-lock `test-utils` build also consumes them) because only this crate's own unit tests use them; ungated they would trip `dead_code` in a `test-utils`-only build of the FFI crate. When dashpay#4184 and this PR both land, test_support.rs will conflict on these three items; resolve by keeping a single copy (drop the `#[cfg(test)]` gates, since dashpay#4184 widens the consumer set). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#4185's registry contract Restack adaptation. This branch now sits on top of dashpay#4247 (which sits on dashpay#4185) instead of beside them, so the funding-path registration meets the contract dashpay#4185 established for the deferred-payment registry: * `RegisteredPayment` keeps ONE funding handle — dashpay#4256's `funding: FundingAccountRef` — and dashpay#4185's MANDATORY `registered_height: u32`. The two evolutions are orthogonal (which account vs. which clock), so the union is a strict superset: the Path arm still reaches a DashPay receiving-funds account, and the age guard can no longer be silently disabled by a `None` height. * `register_funded_by` therefore takes `registered_height: u32`. `FinalizedCorePayment::reservation_height` was already a non-optional `u32` sampled inside the funding critical section, so every caller simply drops its `Some(..)` wrapper — no behaviour change, one less way to disable the guard. * `ReservationToken` is dashpay#4185's newtype; the FFI converts with `as_u64` at the C ABI boundary. * The over-limit refusal binds the payment first and discharges its reservation through `abandon_payment` before returning, rather than stranding the account's coins until the TTL backstop. * Test fixtures build wallets with `WalletGeneration` (balance + the generation's lifecycle gate in one Arc) and read `PlatformWalletInfo::generation`. `register_funded_by` still cannot prove its `core` is the generation that produced the payment the way `register` can — `FinalizedCorePayment` carries no `origin_generation` marker, so the FFI's single `generation_payment_guard` hold is what upholds the binding. That gap is pre-existing on this branch and is documented on the method as follow-up. cargo test -p platform-wallet -p platform-wallet-ffi: 540 + 230 pass.
Carries the `FinalizedCorePayment` half of dashpay#4256's review commit 34c0894. The fee-cap/tx-size half of that commit is already in the stack: dashpay#4247 landed the identical `MAX_STANDARD_TX_SIZE` bound and the `u64::MAX / u32::MAX` fee-rate derivation one commit below this one, so re-applying it here would only duplicate it. The reservation a finalized payment holds must be discharged exactly once. While the type derived `Clone`, safe code could abandon one copy and register another, or mint two registry tokens able to broadcast the same transaction — one release would then free inputs the other copy still believed it owned. Dropping `Clone` and having `abandon_payment` consume the value makes register-after-abandon fail to compile. Also brings that commit's coverage for the contract: `abandon_payment_releases_the_reservation_without_the_registry` — the receival account's inputs come back without the registry ever being involved, and the payment cannot be reused afterwards. cargo test -p platform-wallet -p platform-wallet-ffi: 541 + 230 pass.
…ode (33) `map_send_builder_error` folded `BuilderError::SigningFailed` into `TransactionBuild` → native code 32, whose documented contract is "the request itself is at fault; a verbatim retry fails identically". That is false for the production `MnemonicResolverCoreSigner`: a locked or missing Keychain mnemonic surfaces as `SigningFailed`, and key-wallet `release_if_owner`-releases the build's owner-stamped input reservation before returning, so the identical recipients/amount/fee/funding path succeed once the signer is usable. Hosts were being told to make the user edit a payment that was never wrong. Adds `PlatformWalletError::TransactionSigning` → `ErrorTransactionSigning = 33` → `DashSdkError.PlatformWallet.TransactionSigning` (isRetryable = true, matching the ShieldedNoRecordedAnchor convention for "nothing committed, reservations released, retry once the precondition is met"). Code choice — 33, not the reviewer's suggested 31. 27-32 are all claimed across the sibling v4.1 stack (27/28 dashpay#4185, 29 dashpay#4184, 30 reserved for free on every branch. 31 IS reserved, but for a different contract: asserting the signer holds no usable private key for a requested public key, restored from the typed `DashSDKSignerErrorCode::SigningKeyUnavailable`. This is a Core L1 input signing failure with no such provenance — `BuilderError::SigningFailed` also covers an unresolved input derivation path, a sighash computation failure and a malformed signature encoding — so reusing 31 would assert "the key is unavailable" for failures that are nothing of the kind. Kept separate so neither contract is weakened; the rationale is recorded on the variant for maintainers reconciling the range. Tests: an end-to-end locked-signer build asserting TransactionSigning (not TransactionBuild), a retry-after-recovery test proving the inputs really were released, FFI code/mapping tests, and the Kotlin decode + retry-contract test. platform-wallet 539 passed, platform-wallet-ffi 225 passed, kotlin-sdk 190 passed; fmt clean, no new clippy warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sult space Same gap this stack closed for 32 one PR down: the code was added to the Rust FFI enum and the Kotlin decoder here but not to the Swift mirror, so a signing failure reached iOS through `init(ffi:)`'s `default:` arm as `errorUnknown` (99). That misclassification is worse for 33 than for most codes because the two errors carry OPPOSITE advice. 32 means "the request is at fault, a verbatim retry fails identically"; 33 means "the request is fine, the reservation was released, resubmit the identical request once the signer is usable". Collapsing 33 into the unknown bucket loses the one distinction the code exists to make, and a locked Keychain is the common case on iOS. Adds the enum case, the `init(ffi:)` mapping, the `PlatformWalletError.transactionSigning` arm and its `errorDescription` entry. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_SIGNING = 33. Registry state after this stack (dashpay#4261): 32 ErrorTransactionBuild, 33 ErrorTransactionSigning, 34/35/36 the deferred-token trio — identical in Rust, Kotlin and Swift.
…tures `cargo fmt --check` fallout from the restack: `FundingAccountRef` joining the `wallet::core` re-export changed the import ordering in `lib.rs`, and the shorter `WalletGeneration` type name re-wrapped two test-fixture signatures. No behaviour change.
a456664 to
862036b
Compare
Re-verified the whole document against the CURRENT `origin/v4.2-dev` (`97904ed2fc`), not the `f53e5eef0a` the review comment cited and not the `5d68612a45` this file was last compiled against. `ErrorSigningKeyUnavailable = 31` is merged ABI. It landed in `189a3abb1c` (dashpay#4183, stacked on dashpay#4191) together with its Rust C-facing discriminant and complete Swift and Kotlin mirrors — the raw case, the `init(ffi:)` arm, the typed `PlatformWalletError` case with its `init(result:)` arm, and Kotlin's `31 -> PlatformWallet.SigningKeyUnavailable`. Leaving it under "Proposed allocations", whose preamble explicitly permits renumbering, contradicted rule 3. Moved to the merged table. Four PRs merged into `v4.2-dev` on 2026-08-04 and this file still treated all four as open: dashpay#4191 (`0e2282b586`), dashpay#4183 (`189a3abb1c`), dashpay#4277 (`6704a41a85`), dashpay#4251 (`7afc8a8ff3`). Only dashpay#4183 claimed an integer; the other three claimed none, and dashpay#4277 is now recorded as the merged precedent for "touches error.rs but allocates nothing" (it routes TxMetadataPayloadTooLarge onto the existing ErrorInvalidParameter). Dependent sections updated so nothing implies 31 may still move: the frontier breakdown (unchanged at 38), the proposed table, the inherited-code table (31 is trunk now, not an inheritable claim), the collision-history bullet list, the no-new-code open-PR inventory, the 31-vs-33 note (collapsing 31 is no longer available; only dashpay#4256's 33 is still open), and the survey provenance plus the PR-heads-of-record table. Also refreshed, because a re-dated provenance section must not carry claims that are now false: dashpay#4204's and dashpay#4256's Swift mirror gaps are both closed, and the stale ErrorAssetLockCrossDomainConsentRequired comments are gone from every branch that carried them. Every discriminant, mirror, PR state, and SHA above was read from git or the GitHub API on 2026-08-04. The four merge SHAs were confirmed ancestors of `97904ed2fc`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What this does
Bridges two flows that were previously disconnected in
platform-wallet:build_signed_payment's single-account-by-derivation-path selection (feat(kotlin-sdk): single-account build_signed_payment send API (funding_path) #4247) — the only build path that can reach aDashpayReceivingFundsaccount; andtoken → broadcastSigned/releaseReservation.New
finalize_signed_payment_from_funding_pathreturns aFinalizedCorePayment(signed tx, fee, change, the resolvedFundingAccountRef, and the reservation token). The key correctness property: the reservation is recorded against the resolved account, never a BIP44 default, so release/broadcast bookkeeping lands in the same ledger the build reserved into.FundingAccountRef::Pathmakes non-standard accounts (DashPay receival) nameable to the release machinery, which previously could only addressStandardAccountTypes.Surface:
wallet/core/transaction.rs—FundingAccountRefenum +release_reservation_forwallet/core/send.rs—finalize_signed_payment_from_funding_path,FinalizedCorePayment,abandon_paymentwallet/signed_payment_registry.rs—register_funded_bycore_wallet_build_signed_payment_with_tokencoreWalletBuildSignedPaymentWithTokenManagedPlatformWallet.buildSignedPaymentWithToken(recipients, coreSignerHandle, feePerKb, fundingPath), returning the existingSignedCoreTransaction(now carryingchangeDuffs)Guardrails preserved and covered by tests: single-account funding only (the funding-privacy guardrails still pass — no cross-account union), change goes to BIP44/0, watch-only accounts are refused, and the fee is taken from the signed transaction rather than re-estimated.
Stacked on
Stacked on #4185, and includes #4247's commits.
The branch is based on #4185's head (
port/v4.1/split-build-broadcast) with #4247's three commits (port/v4.1/send-raw-tx) cherry-picked beneath this change, because it needs both:bfoss765/rust-dashcorepin at reve99959ce— theReservationToken/build_unsigned_reservedAPI this work is built on.build_signed_payment(funding_path)primitive itself, which is not present on feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185's branch.Merge #4185 and #4247 first, then rebase this PR — its own contribution is the top two commits.
Two scoping notes for reviewers
1. The test-support commit is a verbatim carry of #4184's fixture — drop it on rebase once #4184 merges.
The receival tests need a wallet whose balance is split between BIP44 account 0 and a DashPay funds account, which
WalletAccountCreationOptions::Defaultdoes not provision. That fixture —split_funded_wallet_manager_dashpay,DashpayLeg, and theforeign_contact_account_xpubhelper — already exists in #4184'stest_support.rs, and the last commit here (test(platform-wallet): DashPay-funded split wallet fixture…) is a verbatim copy of it, carried so this PR does not have to stack on 2.9k lines of unrelated asset-lock production code just to reach a test fixture. No production code from #4184 is included.Once #4184 merges, drop that commit when rebasing — #4184's copy is the canonical one. The only intentional deviation is that the three items are
#[cfg(test)]-gated here (ungated they would tripdead_codein atest-utils-only build of the FFI crate, since this crate's unit tests are their sole consumer); #4184 widens the consumer set, so its ungated copy supersedes this one cleanly.2. De-contaminated from the source branch. On
kotlinSDK-v4-qa3the finalize commit and its parent had swapped pieces: the finalize commit carried the masternodes-by-voting-key JNI export (plus itsread_id20helper), while the parent carried this feature'sexternal fun coreWalletBuildSignedPaymentWithTokendeclaration. This PR drops the masternodes bridge (its FFI half is not in this stack, so it would not build here) and includes the Kotlin declaration that belongs to it. The masternodes-by-voting-key feature is untouched and remains to be submitted on its own.Tests
cargo test -p platform-wallet --lib— 528 passed, 0 failed, no warnings. Includes all 11wallet::core::sendtests, all 20wallet::signed_payment_registrytests, and all 3wallet::funding_privacy::guardrailtests.cargo check -p platform-wallet-ffi -p rs-unified-sdk-jni— cleanThree new tests cover the receival funding path end to end, including reservation hold/release cycles:
receival_funding_path_selects_signs_and_reserves_in_that_accountreceival_reservation_is_held_and_released_against_the_receival_accountdefault_funding_reservation_is_held_and_released_on_bip44References
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes