feat(platform-wallet): token-minting finalize from a funding path (spendable DashPay receival accounts) - #4256
Conversation
|
🕓 Ready for review — 14 ahead in queue (commit dd9f852) |
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (40)
📝 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>
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.
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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All five carried-forward prior findings remain valid at exact head 862036b: four blocking lifecycle or ownership defects and one Kotlin decoder coverage gap. The latest delta introduces no additional blocker; its Swift code-33 mapping is correct across the ABI, but the newly added mapping lacks focused regression 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); 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 | 🟡 1 suggestion(s)
5 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:207-210: Add Swift coverage for transaction-signing code 33
The latest delta correctly maps `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_SIGNING` to `.errorTransactionSigning` and converts that result to `PlatformWalletError.transactionSigning`, but `ErrorHandlingTests.swift` has no assertion covering either mapping. This exact omission previously sent code 33 through the default `.errorUnknown` arm, losing the distinction between an invalid request and a retryable signer failure. Add a test that passes the generated code-33 constant through `PlatformWalletResultCode(ffi:)` and asserts `.errorTransactionSigning`; also cover the `PlatformWalletError` conversion if practical.
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.
| case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_BUILD: | ||
| self = .errorTransactionBuild | ||
| case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_SIGNING: | ||
| self = .errorTransactionSigning |
There was a problem hiding this comment.
🟡 Suggestion: Add Swift coverage for transaction-signing code 33
The latest delta correctly maps PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_SIGNING to .errorTransactionSigning and converts that result to PlatformWalletError.transactionSigning, but ErrorHandlingTests.swift has no assertion covering either mapping. This exact omission previously sent code 33 through the default .errorUnknown arm, losing the distinction between an invalid request and a retryable signer failure. Add a test that passes the generated code-33 constant through PlatformWalletResultCode(ffi:) and asserts .errorTransactionSigning; also cover the PlatformWalletError conversion if practical.
source: ['codex']
…NotFound The rebase onto v4.2-dev left `fromPlatformWalletNative` with TWO arms for platform-wallet code 98: this PR's original `7, 8, 98 -> NotFound(...)` and the merged-upstream `PLATFORM_WALLET_NOT_FOUND_CODE -> PlatformWallet.NotFound`. Kotlin's `when` takes the first matching branch, so 98 kept resolving to the top-level `DashSdkError.NotFound` and the second arm was dead code. That regressed the merged upstream behaviour and broke three assertions in `DashSdkErrorTest` (`platformWalletCodesMapToPlatformWalletSubtree`, `platformWalletNotFoundCodeMapsToTypedWalletNotFound`, `platformWalletNotFoundConvertsAtThePublicBoundary`), which all require offset + 98 to surface as the wallet-family `PlatformWallet.NotFound` and NOT as the top-level `NotFound` reserved for rs-sdk-ffi codes 7/8. Drop the stray `98,` so the 7/8 arm is exactly the rs-sdk-ffi pair, and move this PR's deferred-send documentation onto the arm that actually handles 98. The Rust side is unchanged and stays the source of truth: `PlatformWalletFFIResultCode::NotFound = 98` (packages/rs-platform-wallet-ffi/src/error.rs:296), listed as the terminal sentinel in ERROR_CODE_REGISTRY.md. No discriminant was renumbered. The rest of the branch already expected the corrected mapping — see the `PlatformWalletManager` KDoc, which documents this path as `DashSdkError.PlatformWallet.NotFound` (native code 98). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add CoreWallet::build_signed_payment — a first-class "send" primitive that
selects inputs across the UNION of every signable funds account (BIP44 +
BIP32 + CoinJoin + DashPay receiving), builds and signs a standard L1
payment, and returns the signed transaction plus its fee and change amount.
This is the build-only half of the Android send-path cutover: during the
dashj->SDK transition the app keeps dashj's transaction bookkeeping
(maybeCommitTx drives CrowdNode, memos, confidence listeners), so the SDK
must build + sign from the bound wallet and hand back the bytes while dashj
commits/broadcasts. The method therefore does NOT broadcast and does NOT
persist a debit — the only state touched is the in-memory ReservationSet
that set_funding/build_signed use to stop a concurrent SDK build from
re-selecting the same coins (released when the spend is later observed by
sync or by the reservation-TTL backstop).
Reuses the shielded asset-lock union-funding machinery
(all_funding_accounts + spendable_utxos + a spanning path resolver +
LargestFirst selection to keep CoinJoin's many small denominations from
blowing up BranchAndBound), but excludes watch-only DashpayExternalAccounts
(a contact's addresses, which this wallet cannot sign). Fee and change are
derived from the transaction itself (inputs - outputs), the always
self-consistent ground truth, rather than from build_signed's signed-size
fee recomputation which can drift by a few duffs from what is actually paid.
Adds the typed PaymentInsufficientFunds { available, required } error so a
shortfall carries the exact union-wide selectable total.
Tests: correct output/change/fee, BIP44+CoinJoin union selection, typed
union shortfall, watch-only exclusion, and input validation.
cargo test -p platform-wallet --lib: 442 passed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 38012d1)
Expose the union-funding send primitive as a public Kotlin suspend fun that returns the signed raw transaction bytes (plus fee + change) WITHOUT broadcasting — the last SDK gap for the Android wallet's send-path cutover. - FFI (rs-platform-wallet-ffi): core_wallet_build_signed_payment, a one-shot call over CoreWallet::build_signed_payment. Recipients cross as a big-endian blob (u32 count; per row u32 addrLen, addr utf8, u64 amount), each address parsed + network-checked; returns the consensus-serialized signed bytes via out-pointers plus out_fee/out_change, freed with core_wallet_free_payment_bytes. cbindgen exports both automatically. - JNI (rs-unified-sdk-jni): WalletManagerNative.coreWalletBuildSignedPayment returns a byte[] packed big-endian as (u64 fee, u64 change, tx bytes); the FFI-owned bytes are freed before returning. - Kotlin (kotlin-sdk): ManagedPlatformWallet.buildSignedPayment(recipients, coreSignerHandle, feePerKb) -> SignedCorePayment(txBytes, fee, change), serialized under the same shared per-wallet coreSendMutex as sendToAddresses so a concurrent build cannot select the same UTXO. Unlike sendToAddresses it neither broadcasts nor picks a funding account (coin selection auto-spans every signable account). ManagedCoreWallet gains the matching native call on the transient core handle. cargo check/test green across platform-wallet-ffi (149) and rs-unified-sdk-jni (2); :sdk:compileDebugKotlin BUILD SUCCESSFUL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit d18c9c0) [port to v4.2-dev] ManagedPlatformWallet.buildSignedPayment is serialized through the manager's TeardownGate (gate.op {}), matching sendToAddresses and every other native op on this branch, instead of the pre-refactor per-wallet `coreSendMutex` (which no longer exists on v4.2-dev). Concurrent builds still cannot select the same UTXO: CoreWallet::build_signed_payment holds the wallet-manager write lock across coin selection and signing.
… funding build_signed_payment funded from the union of all signable funds accounts (BIP44 + CoinJoin + …) with BIP44 change — the privacy-domain-crossing design blocked on dashpay#4184 (shumkov, 2026-07-21) and replaced there by single-account selection. This code predated that re-scope. - add funding_path: Option<DerivationPath>; None = unmixed BIP44 account 0, Some(path) = strictly the one funds account whose account path matches. No union, no cross-account accumulation; shortfall returns PaymentInsufficientFunds for that account only. Change routes to BIP44 (explicit change addr when a non-Standard account funds). - new wallet::funding_privacy guardrail: crate-wide static test fails the build if any wallet-wide funds-account iteration lacks a PRIVACY-DOMAIN-OK marker. - replace the union-asserting test with default-never-crosses-domains and explicit-path-selects-strictly tests. - review-fix hardening: bounded FFI allocation + checked cursor math, output-total overflow guard, fee-rate bound, typed PaymentInsufficientFunds (code 22). - thread funding_path through FFI/JNI/Kotlin (null = unmixed BIP44). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Balances
Adds derivation_path (Option<String>) to AccountBalanceRow, the FFI
AccountBalanceEntryFFI (appended, ABI-additive), and the JNI JSON
("derivationPath": string|null). Computed from the same
AccountType::derivation_path(network) the funding-path spend selector
compares against, so the emitted string is byte-identical to what
build_signed_payment expects — the app passes it verbatim to spend a
DashPay receival account. Null for account types with no derivable path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ts for the send hardening Addresses the review findings on dashpay#4247. Dust outputs (BLOCKING). `TransactionBuilder::add_output` applies no relay policy, so a one-duff recipient produced fully signed bytes for a transaction every standard node rejects as nonstandard — from a primitive documented as building a *standard* payment for later broadcast. Each recipient amount is now checked against its OWN destination script's `dust_value()` (546 duffs for P2PKH), before the wallet lock, before any input is reserved and before the signer is called. Fee/size overflow (BLOCKING). The `MAX_FEE_PER_KB = MAX_MONEY / 100` bound assumed the transaction stayed under the 100 kB standard limit, which the method never enforced; key-wallet's `calculate_fee` then multiplies `sat_per_kb * size_bytes` unchecked and overflows at ~878 kB — reachable both by a ~25.8k-recipient list and by a funding account with a few thousand small denominations. Two-sided fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`), with checked arithmetic mirroring key-wallet's own base-size formula; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, which makes the product unrepresentable-free for ANY size a `u32` can express and therefore does not depend on the input count. ~43 DASH/kB is still three orders of magnitude above any legitimate rate; * the signed transaction is re-measured and refused if it exceeds the standard limit. Typed build errors. `PlatformWalletError::TransactionBuild` had no FFI arm, so every `funding_path` failure — "no spendable funds account matches" and "names a watch-only account", the two failure modes the single-account design rests on — reached Kotlin as `Generic(99)` and could only be told apart by string-matching. Adds `ErrorTransactionBuild = 32` (27-31 are claimed by sibling v4.1 stack PRs, so this needs no renumbering whichever order they land) plus the Kotlin `PlatformWallet.TransactionBuild` type. Tests for the six hardening fixes, which shipped with no coverage. `core_wallet/send.rs` had no test module at all; it now covers the `count` bound, `try_reserve_exact`, checked cursor math at every field boundary, UTF-8, and wrong-network address rejection. Also covers MAX_MONEY aggregation, the fee-rate bound, `parse_optional_derivation_path`, dust rejection (including that a refused request reserves nothing), and the new size bound. Also: corrects the stale `PaymentInsufficientFunds` doc, which still described the pre-dashpay#4184 union semantics; corrects an inverted fee-direction comment; adds the missing non-empty assertion to a funding-privacy guardrail test; and runs `cargo fmt` over the five files that failed `--check`. platform-wallet 510 passed, platform-wallet-ffi 222 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ilds `build_signed_payment` reserves its selected inputs and leaves them reserved on success, expecting a broadcast to follow. Nothing across Rust/FFI/JNI/Kotlin let a caller that declines to commit give those coins back, so an abandoned build stranded them for RESERVATION_TTL_BLOCKS (24, ~1h) — and indefinitely while the wallet has no processed height, since `ReservationSet::sweep` early-returns at height 0 and therefore never reclaims a pre-sync reservation. A single abandoned build on a freshly restored wallet could strand the whole balance for the life of the process. Adds a standalone release across all four layers: CoreWallet::release_payment_reservation(&Transaction, Option<DerivationPath>) core_wallet_release_payment_reservation (FFI) coreWalletReleasePaymentReservation (JNI) ManagedPlatformWallet.releasePaymentReservation (Kotlin) The transaction is the ownership signal: a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released — releasing its inputs releases precisely this build's own reservation and can never free a competing build's coins. Same signal the internal `release_reservation_after_rejected_broadcast` cleanup already uses. The release consults no height, so it works pre-sync where the TTL backstop cannot. It is idempotent (per-outpoint map removal) and a silent no-op after a successful broadcast — it cannot resurrect a spent coin, since selection reads the UTXO set that sync already updated — so callers can wire it into an unconditional cleanup path. Also routes the build's default-funding-account resolution through the shared `bip44_account_path` helper, so a release and its build can never disagree about what `funding_path: None` means. Tests: release-then-reselect (with the second-build failure pinned as a precondition so it can't pass vacuously), release twice, release after a processed broadcast, release against the wrong account frees nothing, unknown funding path is refused, and the height-0 case — 30 build attempts prove the TTL never fires there, then the explicit release frees the inputs. Addresses review item 3 on dashpay#4247. The reviewer's suggestion to unify this with dashpay#4256's reservation-token finalize is tracked separately rather than done here, to avoid reshaping an API the Android app already calls and hard-coupling this PR to dashpay#4185. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…le doc Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd-path test fixtures Restack adaptation only — no production-code change. This branch now sits on top of dashpay#4185 (port/v4.1/split-build-broadcast) rather than beside it. dashpay#4185 replaced the bare `Arc<WalletBalance>` generation marker with `Arc<WalletGeneration>` (balance + that generation's lifecycle gate in one Arc, so "same generation" and "same gate" cannot diverge), and renamed `PlatformWalletInfo::balance` to `::generation`. The send-path test fixtures added here still built wallets the old way, so they no longer compiled against the new base. Point them at the shared `WalletGeneration` the rest of the crate already uses: * `core/send.rs` — the local `core_wallet` fixture takes `Arc<WalletGeneration>`; `funded_wallet_manager` already hands one back. * `wallet/funding_privacy.rs` — same, for its two fixtures. * `test_support.rs` — the DashPay split fixture populates `PlatformWalletInfo::generation`. cargo test -p platform-wallet -p platform-wallet-ffi: 537 + 230 pass.
…lt space `ErrorTransactionBuild = 32` was added to the Rust FFI enum and to the Kotlin decoder by this PR, but not to the Swift mirror. Swift decodes an unlisted raw value through the `default:` arm of `PlatformWalletResultCode .init(ffi:)`, so every typed build rejection this PR introduced — an unresolvable or watch-only `fundingPath`, a breached monetary bound, a malformed recipients blob — reached iOS as `errorUnknown` (99) with only the message string to distinguish it. That is the exact failure the code was split out of `ErrorUnknown` to end, reintroduced on the other host. Adds the enum case, the `init(ffi:)` mapping, and the `PlatformWalletError.transactionBuild` arm. There is no compile-time check that the raw values match Rust, so the numbering comment is kept in sync with the registry (dashpay#4261) and now records only the codes this PR does NOT own. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ ERROR_TRANSACTION_BUILD = 32. No other exhaustive switch over PlatformWalletResultCode / PlatformWalletError exists in the Swift SDK.
`cargo fmt --check` fallout from the `WalletGeneration` adaptation two commits down: the shorter type name let two fixture signatures fit differently. No behaviour change.
Three blocking review findings on the send-raw-tx path, all in the
build/abandon reservation lifecycle.
1. Post-signing size rejection stranded its selected inputs.
`build_signed` had already reserved them when the
`signed_size > MAX_STANDARD_TX_SIZE` check ran, and the error path
returned without releasing and without handing back the transaction
the caller would need to release it itself. The TTL backstop is no
fallback: `ReservationSet::sweep` early-returns at height 0, so on a
freshly restored wallet those coins were stranded for the process
lifetime. The path now releases before returning.
2. Abandonment could release another build's inputs.
`release_reservation` removes entries by outpoint with no owner
check. Once key-wallet's TTL sweeps a reservation, a concurrent build
can legitimately re-reserve the same outpoint under a new token, and a
late release then freed THAT build's inputs — a double-spend window.
The build now keeps the `ReservationToken` from `build_signed_reserved`
and the release is owner-guarded via `release_reservation_if_owner`,
plus generation-bound the way `release_transaction_reservation` is.
The doc block claiming the transaction alone was a sufficient
ownership signal ("no concurrent build can hold a reservation on any
input") is corrected: that reasoning ignored the TTL sweep and is
refuted by key-wallet's own docs, which state the platform layer
cannot make this safe on its own.
The "safe after a broadcast / wire it into an unconditional cleanup
path" contract is also removed. This primitive does not broadcast, so
between the caller's successful broadcast and sync processing that
spend the inputs are still in the UTXO set and the reservation is the
only thing keeping a second build off them. Callers must release only
builds they did NOT broadcast.
The key-wallet token is deliberately unforgeable (private counter, no
public constructor) and so cannot cross the C ABI. It is threaded to the
host as an opaque handle from a bounded FIFO table, mirroring how
`SignedPaymentRegistry` keeps its funding token Rust-side while a u64
payment handle crosses the boundary. An unknown handle is refused rather
than downgraded to the unguarded release.
`split_funded_wallet_manager` now returns its real `WalletGeneration`.
Callers previously invented a fresh one, which silently makes every
generation-bound assertion vacuous.
Tests: the size-rejection path leaves no reservation held; an
owner-guarded release cannot free a re-reserved input (with the
unguarded release as the control that proves the guard is what closes
it); releasing a broadcast build before sync reopens its inputs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`buildSignedPayment` ran the blocking JNI build through `gate.op`, i.e. plain `withContext(Dispatchers.IO)`. Native signing cannot observe cancellation once started and reserves the funding UTXOs before it returns, so a caller cancelled mid-build had the completed `SignedCorePayment` discarded by prompt cancellation — taking with it the tx bytes and reservation handle that are the only way to release. The reservation then sat until the TTL backstop, or forever at height 0 where the sweep never runs. Switched to `gate.opWithCleanupOnCancellation`, the same handoff guard the token-owning `buildSignedPayment` overload already uses, with a synchronous best-effort release of the discarded payment. The cleanup runs from a `finally` and never throws: replacing the caller's `CancellationException` with an unrelated native error would be worse than the reservation it is trying to reclaim. Also threads the reservation handle through the Kotlin surface, which is what makes the release owner-guarded: - `SignedCorePayment` carries `reservationHandle`. - `decodeSignedPayment` reads the extended native blob (`u64 fee, u64 change, u64 reservationHandle`, then tx bytes). - `releasePaymentReservation` takes the handle; a new overload takes the `SignedCorePayment` itself so bytes and handle cannot be mismatched between two in-flight builds. The KDoc claiming the release is "safe after a broadcast" and therefore safe in an unconditional `finally` is removed — releasing between a successful broadcast and sync observing it reopens the inputs to selection. Callers branch on whether they broadcast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-> 37 and mirror it (dashpay#4204) 32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…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.
…nd gate it against teardown Two review findings on dashpay#4256, fixed together because the second changes the very call site the first guards. Gate path-funded registration against wallet teardown (finding a9da0344e085). `core_wallet_build_signed_payment_with_token` published a registry token — `*out_token` — with neither this generation's `generation_payment_guard` nor an `is_current_generation` check, though `finalize_signed_payment_from_funding_path` releases the wallet-manager write guard before awaiting the external signer. A concurrent `remove_wallet_with_teardown` could therefore remove the generation and finish sweeping its registry entries while the signer ran, after which this call published a fresh token for a removed wallet that the completed sweep can never see. It now takes the gate after the signer returns (never around it — that would stall teardown for as long as the user takes at the prompt) and holds it across both the liveness check and the registration, mirroring `core_wallet_signed_payment_finalize` and `core_wallet_tx_builder_finalize`. On a dead generation the reservation is reconciled with `abandon_payment` and the call returns `NotFound` (98), the code both siblings already use for that case. Consume the finalized payment when registering path-funded reservations (finding a6bf21c99468). `register_funded_by` took separately cloneable pieces instead of the ownership object, so safe public code could register cloned fields and then `abandon_payment(payment)` — leaving a live token over inputs that had become selectable again — register the same transaction twice and broadcast a superseded build through the second token, or pair a transaction with an unrelated funding path, height or owner token so cleanup targeted the wrong reservation. Being `async` with no await in its body, dropping its future before the first poll also discarded the moved bookkeeping without inserting a registry entry. `FinalizedCorePayment` is now opaque (private fields, still not `Clone`) and carries the same unforgeable `origin_generation` marker `SignedCoreTransaction` has. `register_funded_by` is replaced by `register_payment`, which is synchronous, consumes the whole value, validates its generation the way `register` does — handing the payment back in `RegisterPaymentWrongGeneration` so its reservation is never stranded — and derives every stored field internally. Accessors are added only where callers genuinely need them: the FFI's transaction, fee and change. This is the follow-up the old doc comment tracked on this same PR; that note and the "generation binding is the caller's obligation" section are removed accordingly. Tests: registration through a foreign generation is refused, mints no token, leaves the reservation held, and the handed-back payment still abandons to return the receival inputs to spendable. A `compile_fail` doctest pins the linearity — `register_payment` after `abandon_payment` is E0382, use of moved value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…buildSignedPaymentWithToken Review finding 694cba77d412 on dashpay#4256. `buildSignedPaymentWithToken` ran under plain `gate.op` while the token-minting `buildSignedPayment` overload already used `opWithCleanupOnCancellation`. The blocking JNI call cannot observe coroutine cancellation and completes native registration — minting the token and transferring reservation ownership to it — before it returns its blob. If the caller is cancelled while JNI runs, the prompt-cancellation handoff `withContext` performs on resume discards the freshly constructed `SignedCoreTransaction` before anyone can hold it, leaving only the nondeterministic `NativeCleaner` owning the token. The funding inputs then stay reserved until a GC cycle or the reservation TTL — and indefinitely at processed height zero, where key-wallet's TTL sweep never fires at all. It now uses the same `opWithCleanupOnCancellation` handoff as its sibling, closing the discarded result so the reservation is released deterministically. The cleanup wraps `close()` in `runCatching`: it runs a JNI release, and letting it throw would escape the helper's `finally` and displace the CancellationException that triggered it. The GC backstop still sits behind a failed release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s onto FundingReservationToken The dashpay#4247 restack briefly carried two aliases for the same key_wallet type; collapse onto the repo-wide spelling so both the split-build and funding-path call sites read against one name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
862036b to
dd9f852
Compare
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