fix(platform-wallet): address sync review feedback for #3990#3991
fix(platform-wallet): address sync review feedback for #3990#3991thepastaclaw wants to merge 3 commits into
Conversation
- commit_reconciliation: drop cross-pool index conflicts before mutating the found seed or emitting the entry - file store: purge shielded_pending_spends rows on wallet / all-subwallet purge; mirror redrive-row deletion on mark_spent for any known nullifier - SubwalletState::mark_spent: always clear pending/redrive for a known nullifier, returning true only on the first spent transition - anchor probe: report depth>0 store errors as a distinct skipped outcome instead of conflating them with an uncoverable note - register_from_addresses: best-effort local add_identity so a local persistence failure cannot suppress the accepted registration result - SwiftExampleApp: generation-token invalidation of in-flight balance snapshot refreshes during Clear, plus isClearing gating on the Sync Now / Clear buttons Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR hardens durability across rs-platform-wallet: identity registration persistence becomes best-effort, address reconciliation now reports whether persistence succeeded and gates asset-lock consumption and provider bijection updates on it, and shielded store/operations logic distinguishes store errors from unwitnessable notes and fixes SQLite purge/mark-spent consistency. Swift example app clear flow adds race-prevention state. ChangesRust Platform Wallet Durability and Shielded Store Fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Swift Example App Clear-Sync Race Fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CoreContentView
participant PlatformBalanceSyncService
User->>CoreContentView: tap Clear
CoreContentView->>PlatformBalanceSyncService: clearLocalState()
PlatformBalanceSyncService->>PlatformBalanceSyncService: bump refreshGeneration, isClearing=true
PlatformBalanceSyncService->>PlatformBalanceSyncService: in-flight refreshBalanceSnapshot captures old generation
PlatformBalanceSyncService-->>PlatformBalanceSyncService: wipe completes, defer bumps generation, isClearing=false
PlatformBalanceSyncService->>PlatformBalanceSyncService: stale result generation mismatch -> discarded
sequenceDiagram
participant FundFromAssetLock
participant PlatformAddressWallet
participant Persister
FundFromAssetLock->>PlatformAddressWallet: reconcile_address_infos_with_persistence()
PlatformAddressWallet->>Persister: store(entries)
Persister-->>PlatformAddressWallet: success or error
PlatformAddressWallet-->>FundFromAssetLock: (changeset, persisted)
alt tracked_out_point exists and not persisted
FundFromAssetLock-->>FundFromAssetLock: log error, return Ok(changeset), skip consume_asset_lock
else persisted or no tracked_out_point
FundFromAssetLock->>FundFromAssetLock: consume_asset_lock
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Review complete (commit cd9bf65) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/wallet/shielded/operations.rs`:
- Around line 1786-1832: The new ShieldedMerkleWitnessUnavailable probe failure
is currently being flattened into the generic wallet-operation error path, which
hides its retryable/non-broadcast meaning. Update the FFI handling in
shielded_send.rs to add a dedicated host result code for this error and map
PlatformWalletError::ShieldedMerkleWitnessUnavailable explicitly instead of
routing it through ErrorWalletOperation, preserving the distinct caller
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 774ebae4-08b8-4566-9239-06b6a023aba0
📒 Files selected for processing (9)
packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/shielded/file_store.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
There was a problem hiding this comment.
Code Review
Follow-up PR that lands six focused fixes on top of #3990: address-reconciliation conflict handling, shielded pending-spend cleanup on purge/already-spent transitions, anchor-probe error discrimination, best-effort local identity persistence, persist-before-consume ordering for asset-lock funding, and a Swift Clear/Sync race guarded by a generation token. Changes are internally consistent with existing lock ordering and error-handling conventions in this codebase. One in-scope suggestion around the new conflict guard's handling of zero-balance (removal) entries, plus one test-coverage note for the new guard branch.
🟡 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/platform_addresses/provider.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:1002-1033: Index-conflict guard drops removal entries too, leaving stale balances that can resurrect after restart
The new guard at provider.rs:1008-1020 `continue`s past *everything* on conflict — the `is_removal` branch that would call `state.found.remove(&entry.address)`, and the `outcome.entries.push(entry)` that would emit a durable zero-out to the persister. It fires whenever `entry.address` is not in the bijection but `entry.address_index` already maps to a different address (the drift condition).
For the non-removal (credit) branch this is correct: an orphaned `found` insert with no bijection pairing is invisible via `current_balances()` (which filters on the bijection), so dropping it changes nothing observable.
For the `is_removal` branch it is not: `found.remove` never needed a pairing to be correct, and `PlatformAddressWallet::initialize_from_persisted` (wallet.rs:144-146) re-seeds `account.address_credit_balance` directly from persisted `found`, bypassing the bijection filter. So if a stale non-zero row already exists on disk for `entry.address` from before the pairing drift, a proof-attested zero-balance entry for that same address is silently discarded — the disk row survives, and the next app restart resurrects a balance Platform has already proven is gone.
A closely related consequence: when every resolved entry is dropped by this guard, `outcome.entries` is empty but `outcome.resolved > 0`, so `reconcile_address_infos_with_persistence` returns `(default, true)` via the 'nothing new to store' branch (wallet.rs:311-315). `fund_from_asset_lock` then reads `persisted=true` and proceeds to `consume_asset_lock` — exactly the ordering the PR is trying to prevent (Consumed lock paired with un-updated durable rows).
Both issues share the same root cause: the guard is placed too broadly. Gating only the bijection insert (or splitting into `is_removal` / non-removal branches) preserves the guard's intent — never clobber another pairing — while restoring correct emission of removals and making the `persisted=true` path only claim persistence when there was genuinely nothing to write.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:1002-1033: No regression test for the new index-conflict guard branch
The existing `commit_reconciliation_*` tests do not construct the state-drift precondition the new guard targets (an `address_index` in the bijection paired with a *different* address than the entry's). A targeted test — especially one combining that precondition with a zero-balance removal entry — would both pin the guard's intent and would have caught the removal-branch behavior flagged above. Worth adding one credit-side and one removal-side case.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
Split the `commit_reconciliation` index-conflict guard so a zero-funds removal still zeros the in-memory `found` row and is emitted to the durable persister; only credit entries are dropped when their pool-resolved derivation index is already paired with a different address. Without this split a stale persisted balance for the removed address could resurrect on restart because the reconciliation swallowed the zero-out. Adds regression coverage for both branches — the removal case (must survive, must not touch the bijection) and the credit case (must be dropped so the pre-existing pairing isn't evicted). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…vailable The shielded spend paths currently flatten `PlatformWalletError::ShieldedMerkleWitnessUnavailable` into `ErrorWalletOperation`, so hosts can't tell it apart from a hard wallet-operation error and lose the retryable / non-broadcast semantics (the error fires *before* broadcast, notes are released — safe to retry after the transient store failure clears). Map it to a new `ErrorShieldedMerkleWitnessUnavailable = 20` result code in both the blanket `From<PlatformWalletError>` impl and each dedicated `shielded_send.rs` match, mirroring `ErrorShieldedNoRecordedAnchor`. Mirror the code and typed variant in the Swift `PlatformWalletResultCode` / `PlatformWalletError` enums so throw-side callers can branch on it. Adds test coverage in both the blanket-From and `map_spend_result` paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Incremental review of head cd9bf65 on top of prior SHA 2f6d9bc. Carried-forward prior findings: both are FIXED at head — the commit_reconciliation index-conflict guard in provider.rs now splits on is_removal (credits still dropped, removals zero state.found and still push to outcome.entries; only the bijection insert is gated on !index_conflict), and two targeted regression tests (commit_reconciliation_index_conflict_still_emits_removal, commit_reconciliation_index_conflict_drops_credit_entry) pin both branches. New in the latest delta: a dedicated FFI code PlatformWalletFFIResultCode::ErrorShieldedMerkleWitnessUnavailable = 20 mirrors the existing ErrorShieldedNoRecordedAnchor pattern end-to-end (Rust #[repr(C)] discriminant, blanket From, both dedicated shielded_send.rs match arms, Swift PlatformWalletResultCode / PlatformWalletError enums, unit tests on both paths). All 8 agent passes converged on zero new findings; verified provider.rs:1022-1059 in the worktree at head. No CodeRabbit inline comments provided.
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT while preserving the verified clean review.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
2 participants
Follow-up fixes for CodeRabbit feedback on #3990.\n\nThis branch addresses the platform wallet sync/reconciliation, shielded pending spend cleanup, Swift clear/sync race, and best-effort identity persistence review comments.\n\nValidation:\n- cargo fmt --manifest-path packages/rs-platform-wallet/Cargo.toml --check\n- cargo check -p platform-wallet --features shielded\n\nNote: I could not push directly to dashpay/platform:chore/sync-v4.1-dev-with-v4.0-dev (403/permission denied), so this PR targets that branch.
Summary by CodeRabbit
Bug Fixes
New Features