feat(platform-wallet): reserve DashPay payout addresses without Core funding - #4623
feat(platform-wallet): reserve DashPay payout addresses without Core funding#4623PastaPastaPasta wants to merge 1 commit into
Conversation
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
|
✅ Final review complete — no blockers (commit d7d0224) · triage: critical · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied findings against the exact head and confirmed one nonblocking coverage gap: the new concurrency test checks progress during broadcast, but does not exercise overlapping pool persistence protected by the payment gate. No in-scope blocking defect was confirmed. The existing Swift persistence acknowledgement behavior is retained as a separate follow-up; independent validation here was source inspection and a passing diff whitespace check, not a rerun of the reported test suites.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — The change touches funds-related address derivation, durable address-consumption state, concurrent Core-send rollback ordering, seed verification, and Rust/Swift FFI ownership, where errors could cause address reuse, incorrect payment destinations, or wallet-state corruption. - Phase 1 reviewers: not run (skipped for throughput: 63 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🟡 1 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/identity/network/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:6194-6201: Exercise overlapping persistence to protect the new payment gate
The reservation completes its store and flush before this test releases the rejecting broadcaster. This verifies that reservation can proceed during broadcast and that the subsequent rollback preserves it, but does not exercise the stale-snapshot ordering that the new payment gate prevents. The default current-thread Tokio test and synchronous persistence callbacks do not overlap these snapshot/store sequences. Add a deterministic multithreaded test that pauses a pool store after snapshot capture, starts a competing reservation or rejection rollback, and verifies that the competing operation cannot persist ahead of the paused operation. Ensure the test fails when the corresponding gate acquisition is removed, so an older whole-pool snapshot cannot silently overwrite a newer reservation's used flag.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Make required address-pool persistence acknowledgements fail closed — PlatformWalletPersistenceHandler.swift:3559–3560 acknowledges a missing parent account for every account type except IdentityInvitation, and fetchAccount at lines 3763–3783 converts fetch failures into lookup misses. The address-pool callback consequently can return zero without staging a DashPay pool update, which the Rust persister treats as success. This deliberate tolerance for ordinary address sync predates the PR and already applies to Core-funded contact sends, but it can leave a payment-address used flag unpersisted and permit reuse after reload.
- Follow-up: Track a separate persistence-contract change that distinguishes required payment reservation writes from tolerant background synchronization, with Swift-backed failure-injection coverage for DashPay account lookup failures.
| let reserve = async { | ||
| entered.wait().await; | ||
| let address = iw | ||
| .dashpay() | ||
| .reserve_payment_address(&owner, &contact, &provider) | ||
| .await | ||
| .unwrap(); | ||
| release.wait().await; |
There was a problem hiding this comment.
🟡 Suggestion: Exercise overlapping persistence to protect the new payment gate
The reservation completes its store and flush before this test releases the rejecting broadcaster. This verifies that reservation can proceed during broadcast and that the subsequent rollback preserves it, but does not exercise the stale-snapshot ordering that the new payment gate prevents. The default current-thread Tokio test and synchronous persistence callbacks do not overlap these snapshot/store sequences. Add a deterministic multithreaded test that pauses a pool store after snapshot capture, starts a competing reservation or rejection rollback, and verifies that the competing operation cannot persist ahead of the paused operation. Ensure the test fails when the corresponding gate acquisition is removed, so an older whole-pool snapshot cannot silently overwrite a newer reservation's used flag.
source: ['claude']
2 participants
Supersedes #4614 (re-opened from the main repo so the Rust/Swift CI jobs run on it).
Issue being fixed or feature implemented
DashPay contact payments currently expose a combined address-resolution and Core-funded send operation. Wallets with Platform or shielded funds cannot obtain a fresh DIP-15 contact destination without selecting transparent Core inputs.
What was done?
Add
DashPayView::reserve_payment_addressand the SwiftManagedPlatformWallet.reserveDashPayPaymentAddress(fromIdentityId:toContactIdentityId:)wrapper through the C FFI. The operation completes the seed-verified deferred contact-crypto drain, consumes an address from the authoritative wallet/owner/contact external account pool, and persists and flushes its used flag before returning it.Core sends and external payout reservations share the derivation helper and a per-wallet gate that keeps their pool snapshots ordered. Core broadcasts release the gate while awaiting the network, then reacquire it before a definitive rejection's address rollback. The Swift wrapper retains its mnemonic resolver throughout the FFI call and frees the returned string.
Callers reserve only when submitting a confirmed payment. Exposed addresses stay consumed after cancellation, rejected withdrawal, or an unknown outcome; this API does not submit the withdrawal or record payment history. The companion iOS change owns the submitted-withdrawal history.
How Has This Been Tested?
cargo clippy -p platform-wallet -p platform-wallet-ffi --lib(only pre-existing upstream warnings).cargo fmt -p platform-wallet -p platform-wallet-ffi --checkandgit diff --check.Real testnet validation used two registered DashPay contacts: a 0.02 DASH Platform withdrawal with zero Transparent balance and a user-submitted 0.1 DASH Shielded withdrawal paid the reserved addresses. Recipient receipt verification used a combined test build with the separate AssetUnlock discovery fix in dashpay/rust-dashcore#1004. Screenshots and exact build provenance.
The receipt fix needs a separate upstream SDK dependency update. This PR only adds address reservation and leaves the existing rust-dashcore dependencies unchanged.
Breaking Changes
None. The existing Core-funded send API remains available.
Checklist:
For repository code-owners and collaborators only
This pull request was created by Codex.