feat(platform-wallet): CoinJoin-drain asset-lock funding for the shielded pool - #4327
Conversation
…lded pool
Consumes rust-dashcore feat/coinjoin-asset-lock-funding (rev bump to
1846079b): the key-wallet asset-lock builder can now fund from a CoinJoin
account in whole-balance drain mode (lock value = sum(inputs) - fee).
- AssetLockBuildAmount { Exact, DrainAll } + *_with_funding forms of
build_asset_lock_transaction / broadcast_funded_asset_lock /
create_funded_asset_lock_proof. The historical entry points delegate
with Exact + Bip44, so identity/top-up/address-funding flows are
unchanged. The tracked amount is read back from the built payload
(for a drain it is only known post-build).
- AssetLockFunding::DrainAccountBalance { account } resolver variant —
the CoinJoin -> Shielded migration path (no transparent intermediate
hop). The shielded fund preflight gains a drain sizing guard: refuse a
drain whose balance (minus an upper-bound L1 fee) could not clear the
Type 18 pool fee, so an unrecoverable dust lock is never broadcast.
- Reservation release after a rejected broadcast is funding-family-aware
(ReservedFundingAccount: Standard | CoinJoin); proof upgrade's
funding-tx lookup falls back to the CoinJoin account map.
- New FFI entry point
platform_wallet_manager_shielded_fund_from_asset_lock_coinjoin_drain
and Swift wrapper PlatformWalletManager.shieldedFundFromCoinJoinDrain
(same recipient/resume contract as shieldedFundFromAssetLock).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds CoinJoin account drain funding for shielded asset locks. It extends asset-lock building, reservation handling, proof lookup, finality resolution, and Swift/Rust wallet APIs. It also updates SDK parity metadata for DashPay payment history. ChangesCoinJoin drain funding
SDK parity metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant RustFFI
participant AssetLockResolver
participant CoinJoinAccount
participant ChainLock
PlatformWalletManager->>RustFFI: invoke shielded CoinJoin drain
RustFFI->>AssetLockResolver: resolve DrainAccountBalance
AssetLockResolver->>CoinJoinAccount: consume all selected account UTXOs
AssetLockResolver->>ChainLock: wait for finality
ChainLock-->>RustFFI: return proof or typed broadcast error
RustFFI-->>PlatformWalletManager: return FFI result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🕓 Ready for review — next in queue (commit 074a21e) |
…dedPubKeySigner bound The funding-parameterized asset-lock forms were cherry-picked with the older `S: Signer` bound; v4.2-dev's builder surface requires `ExtendedPubKeySigner` (the selected-account xpub work). Also rewrite the stale delegation comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new CoinJoin drain is not safe to merge: proof polling still searches only BIP44 in the paths used before validation, and the preflight can overstate the actual built lock enough to broadcast an unconsumable Type 18 outpoint. The rejection path also loses reservation ownership, and the FFI entry point flattens retry-critical broadcast outcome codes.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/security-auditor=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— security-auditor (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)
🔴 2 blocking | 🟡 2 suggestion(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/asset_lock/sync/proof.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:101-117: CoinJoin fallback is missing from the proof-wait paths
The new CoinJoin fallback exists only in `validate_or_upgrade_proof`, which is reached after a proof has already been acquired. The fresh drain first calls `wait_for_proof`, whose diagnostic and authoritative lookups at lines 423-453 still search only `standard_bip44_accounts`. The timeout fallback in `upgrade_to_chain_lock_proof` and its `wait_for_chain_lock` loop are also BIP44-only, as is proofless recovery in `sync/recovery.rs`. Key-wallet records a transaction spending CoinJoin inputs under `coinjoin_accounts`; with `NoPlatformPersistence`, whose `get_core_tx_record` implementation returns `Ok(None)`, a valid SPV InstantSend or ChainLock record therefore remains invisible. The fresh operation waits 300 seconds and then the fallback fails with “Transaction ... not found”; resume repeats the same family-blind path after the asset lock has already been broadcast. Centralize transaction-record lookup across both account families, or persist the funding family in `TrackedAssetLock`, and use it in every proof, ChainLock, and recovery lookup.
In `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:197-232: Drain preflight is not a conservative estimate of the builder output
This guard can overstate the lock value it claims to lower-bound. First, the pinned builder's one-credit-output asset-lock base size is 81 bytes—8 bytes of header and locktime, two one-byte counts, a conservatively sized 34-byte burn output, a one-byte payload-length prefix, and a 36-byte payload—not 54 bytes, so the estimate is already 27 duffs too high at the configured 1 duff/byte rate. Second, this loop sums raw final, unlocked UTXOs, while `set_funding` excludes outpoints held in the account's `ReservationSet` and `SelectionStrategy::All` applies `Utxo::is_spendable(current_height)`, including coinbase maturity. Finally, the wallet-manager lock is released before acquiring `shield_guard` and performing the actual build, so the candidate set can change between the estimate and selection. A partially reserved account can consequently pass preflight using outputs the builder omits, after which the smaller lock is broadcast and the authoritative pool-fee subtraction fails. Because the asset-lock outpoint is single-use, this defeats the PR's stated guarantee that an unrecoverable dust lock is never broadcast. Make the safety decision from the actual built payload before broadcast, retaining the reservation owner token so an undersized build can be abandoned safely, or use a key-wallet quote API that shares the builder's complete selection and fee logic under the same funding lock.
In `packages/rs-platform-wallet/src/wallet/reservations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/reservations.rs:108-115: Preserve the reservation ownership token through rejection cleanup
The new CoinJoin rejection branch calls unconditional `release_reservation(tx)`, while the pinned key-wallet builder returns `AssetLockResult::reservation_token` and documents that cleanup after an `await` must use `release_reservation_if_owner`. The asset-lock build wrapper discards that token. If the original reservation is swept and the same CoinJoin outpoint is reserved by another build while broadcast is awaited, a late rejection from the first build removes the newer reservation and makes the input selectable for another conflicting transaction. This ownership defect predates the PR for BIP44 funding, but the added branch newly exposes CoinJoin drain inputs to it. Carry the token with the built transaction through the broadcast pipeline and perform owner-guarded release for definitive rejection.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1139-1145: Preserve asset-lock broadcast outcome codes across the Swift boundary
This new entry point converts every `PlatformWalletError` to `ErrorWalletOperation`, discarding the typed distinction between `TransactionBroadcastUnconfirmed` and a definitive `TransactionBroadcast` rejection. The Rust flow deliberately keeps the tracked lock and reservation for an ambiguous outcome but untracks and releases after rejection, and the existing `From<PlatformWalletError>` conversion maps these variants to `ErrorTransactionBroadcastUnconfirmed` and `ErrorTransactionBroadcastRejected`. Swift already exposes dedicated cases for both. Flattening them prevents the caller from choosing resume/do-not-redrain behavior for a possibly broadcast whole-account lock versus correcting and safely retrying a rejected operation.
- Family-aware funding-tx lookups everywhere (review blocker 1): the BIP44+CoinJoin record lookup is centralized in `sync::proof::funding_tx_record` and used by every proof, ChainLock-wait, and recovery path (`wait_for_proof`'s diagnostic + authoritative reads, `upgrade_to_chain_lock_proof`, `wait_for_chain_lock`, `validate_or_upgrade_proof`, and proofless recovery) — previously only `validate_or_upgrade_proof` had the CoinJoin fallback, so a drain lock's IS/CL record was invisible to the fresh-wait paths under `NoPlatformPersistence` and the flow burned the full 300 s timeout. - Authoritative drain floor instead of a pre-build estimate (review blocker 2): the size/balance preflight is gone — it could overstate the drained value (wrong base size, no reservation/spendability filters, TOCTOU after the lock drop). `AssetLockBuildAmount::DrainAll` and `AssetLockFunding::DrainAccountBalance` now carry `minimum_lock_duffs`; the shielded fund flow stamps the pool-fee floor, and `broadcast_funded_asset_lock_with_funding` enforces it against the BUILT payload before anything is tracked or broadcast, abandoning an undersized build with an owner-guarded reservation release. - Reservation ownership through cleanup (review suggestion 3): the builder's `ReservationToken` is threaded from `build_asset_lock_transaction_with_funding` through the funded pipeline; both the rejected-broadcast release and the undersized-drain abandon use `release_reservation_if_owner`, so a late cleanup can no longer clobber a newer build's reservation of the same outpoints. Token-less legacy callers keep the historical unconditional release. - Typed FFI broadcast outcomes (review suggestion 4): the coinjoin-drain FFI entry point converts errors via `From<PlatformWalletError>`, preserving ErrorTransactionBroadcastUnconfirmed vs ...Rejected so the host can distinguish resume-don't-redrain from safe-retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/asset_lock/build.rs (1)
763-772: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider failing instead of recording
0for a missing asset-lock payload.The
_ => 0arm silently records a locked amount of0. For aDrainAllbuild with a minimum this is safe (the floor check aborts). For anExactbuild the tracked row would storeamount: 0, and the shielded CL-only path derives the shield amount from that tracked row (lookup_asset_lock_value_credits). The arm is unreachable with the current key-wallet builder, so this is defensive only.♻️ Proposed change
- _ => 0, - }; + _ => { + return Err(PlatformWalletError::AssetLockTransaction( + "built transaction carries no asset-lock payload".to_string(), + )); + } + };🤖 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/asset_lock/build.rs` around lines 763 - 772, Update the locked_amount_duffs extraction in the asset-lock build flow to fail immediately when special_transaction_payload is missing or not an AssetLockPayloadType, instead of recording 0. Preserve summing credit_outputs for valid asset-lock payloads so Exact and DrainAll continue using the built amount.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- Around line 763-772: Update the locked_amount_duffs extraction in the
asset-lock build flow to fail immediately when special_transaction_payload is
missing or not an AssetLockPayloadType, instead of recording 0. Preserve summing
credit_outputs for valid asset-lock payloads so Exact and DrainAll continue
using the built amount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3471cc4-d788-4d0f-af8e-fc5f77b05119
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/reservations.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
carried-forward: three of the four prior findings are fixed; prior-7bd26f36-3 remains as one in-scope suggestion because the owner token is still applied to whichever wallet generation currently occupies the wallet ID. latest-delta: two in-scope test-coverage suggestions remain for the CoinJoin proof-record fallback and the pre-broadcast undersized-drain abandonment path; no blocking findings remain, and the existing targeted proof/build tests pass.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/security-auditor=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/security-auditor=claude-sonnet-5(failed); claude/rust-quality=claude-sonnet-5(failed); claude/ffi-engineer=claude-sonnet-5(completed); claude/general=claude-sonnet-5(failed); claude/security-auditor=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(completed); claude/general=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— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback for Sonnet verifier) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (failed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— general (completed)
🟡 2 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/asset_lock/sync/proof.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:55-69: Add a CoinJoin regression test for funding-record lookup
The centralized helper correctly checks BIP44 and then CoinJoin, but the six tests in this module exercise only `record_or_persister`; none places a transaction exclusively in `coinjoin_accounts` or invokes `funding_tx_record`. This dispatch is the exact fix for the prior 300-second proof-wait failure and can regress while every current test remains green. Add at least a CoinJoin-only helper test and preferably drive `wait_for_proof` or proofless recovery with `NoPlatformPersistence`, where the in-memory CoinJoin record is authoritative.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:782-811: Cover undersized drain abandonment before broadcast
No platform-wallet test constructs `AssetLockBuildAmount::DrainAll`, so the branch that enforces this PR's central safety guarantee is untested. The existing build tests use exact-amount BIP44 funding and do not prove that an undersized completed payload is rejected before tracking or broadcast, or that its reservation is released through the returned owner token. Add a CoinJoin-funded test with a counting broadcaster, set `minimum_lock_duffs` above the built payload value, and assert that the broadcaster is never called, no tracked row is created, and an immediate subsequent drain can select the inputs again.
…ain abandonment Review follow-ups on the CoinJoin-drain PR: - funding_tx_record_finds_coinjoin_only_record / ..._finds_bip44_record: a record filed only under `coinjoin_accounts` (how key-wallet files a tx spending CoinJoin inputs) must be visible to the shared proof/ recovery lookup — the exact regression behind the pre-fix 300 s proof-wait failure — and the historical BIP44 path still resolves. - undersized_drain_abandoned_before_broadcast: over the CoinJoin-funded fixture, a drain whose floor exceeds the built lock value is refused with nothing broadcast (counting broadcaster at 0), no tracked row and no queued removal, and the owner-guarded reservation release lets an immediate follow-up drain over the same single-UTXO account select the inputs and broadcast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/asset_lock/build.rs (1)
750-758: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the reservation when the invitation durability gate aborts.
The
IdentityInvitationerror path at lines 834-842 returns before tracking or broadcasting. It does not callrelease_reservation_after_rejected_broadcast.The retained
reservation_tokenthen leaves the built inputs reserved with no tracked row that can resume the transaction. A retry can fail at input selection after a transient persistence or flush failure.Release the reservation before returning from that abort path. Add a regression assertion that a subsequent build can reuse the input.
🤖 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/asset_lock/build.rs` around lines 750 - 758, Update the IdentityInvitation durability-gate error path to call release_reservation_after_rejected_broadcast with the retained reservation_token before returning the error. Add a regression assertion verifying that a subsequent transaction build can reuse the released input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- Around line 750-758: Update the IdentityInvitation durability-gate error path
to call release_reservation_after_rejected_broadcast with the retained
reservation_token before returning the error. Add a regression assertion
verifying that a subsequent transaction build can reuse the released input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1af161d1-249d-4ea9-bc60-c35dcc1a8972
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs
…arity entry check_sdk_parity_manifest.py requires `reason: null` on hosts with no parity gap (both statuses supported/not-applicable); the new persistence.dashpay_payment_history entry landed on v4.2-dev with a rationale string on its fully not-applicable kotlin host, turning the parity gate red for every PR merged against the branch. Summary regenerated with --write-summary. The dropped rationale, for the record: Android derives contact payment attribution from transaction history on reads and does not consume PaymentEntry rows (confirmed by the Android team during the sent-payment reconstruction review), so the JNI vtable deliberately leaves the slot None and there is nothing to persist or restore on this host. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Newer strict-concurrency toolchains reject `makeTestWallet`'s non-Sendable result crossing its async boundary (7 hard errors in the Maya deposit integration suite on the CI runner's Swift). Same justification as the existing `IntegrationTestEnv: @unchecked Sendable`: immutable stored SDK handles, one test task drives a wrapper at a time. Pre-existing on v4.2-dev — the swift-sdk CI job is path-filtered, so it only surfaces on PRs that touch the package. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4327 +/- ##
============================================
- Coverage 87.78% 87.78% -0.01%
============================================
Files 2677 2677
Lines 342371 342371
============================================
- Hits 300551 300550 -1
- Misses 41820 41821 +1
🚀 New features to boost your workflow:
|
What
Lets the wallet's CoinJoin (mixed-coin) balance fund a
ShieldFromAssetLock(Type 18) directly — one transaction draining every final CoinJoin UTXO into a single asset lock (lock value = Σ inputs − L1 fee), with the shielded recipient receivinglock_value − pool_fee. No transparent BIP44 intermediate hop, so the mixed coins are never parked on a reusable transparent address.Builds on the key-wallet side merged in dashpay/rust-dashcore#915 (
AssetLockFundingAccount+ drain mode, already in this repo's current rev pin):AssetLockFunding::FromAccountDrain { account }— resolver variant that drains the given funding account into a fresh tracked lock;FromWalletBalanceand every existing flow are untouched.build_asset_lock_transaction_with_funding/create_funded_asset_lock_proof_with_funding— funding-parameterized forms (AssetLockBuildAmount::Exactvs::DrainAll); the historical entry points delegate with BIP44 + exact amount.shielded_fund_from_asset_lock: estimates the drained lock value (Σ spendable − size-based fee) and rejects the flow before broadcasting when it could not clear the Type 18 pool fee — a dust lock that could never be consumed is never created. The post-resolutionlock_value − pool_feederivation is unchanged (reads the real on-chain value, so estimate drift is harmless).platform_wallet_manager_shielded_fund_from_asset_lock_coinjoin_drain+ Swift wrapperPlatformWalletManager.shieldedFundFromCoinJoinDrain(walletId:coinJoinAccountIndex:recipients:), mirroring the existing fund-from-asset-lock signer/worker-thread pattern. Resume-by-outpoint reuses the existingshieldedResumeFundFromAssetLockunchanged.Consumer
dashpay/dashwallet-ios#858 — the post-migration "move your mixed coins" prompt offers a Shielded destination that runs this drain.
Verification
cargo check -p platform-wallet --features shieldedand-p platform-wallet-fficlean on top of v4.2-dev (signer bounds aligned with theExtendedPubKeySignermigration).DashSDKFFI.xcframeworkrebuilt (ios + sim); the SDK example app and dashwallet-iosdashpayscheme build against it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation