fix(swift-example-app): make Platform Sync "Clear" actually clear synced data#3959
fix(swift-example-app): make Platform Sync "Clear" actually clear synced data#3959llbartekll wants to merge 4 commits into
Conversation
…ced data The "Clear" button in the Platform Sync Status section only called `clearDisplay()`, which zeroes the in-memory @published mirror but leaves the synced data intact in all three places it actually lives: - SwiftData rows (`PersistentPlatformAddress`, `PersistentPlatformAddressesSyncState`) - the in-memory Rust incremental-sync watermark in the address provider - the in-memory managed-account balance map (read by spend paths) Because the watermark survived, the next "Sync Now" resumed incrementally in ~2s instead of doing a full rescan, so it looked like Clear did nothing. This wires the button to a real wipe mirroring the Shielded Sync "Clear" precedent. Reset is manager-level (all wallets on the network) to match the global SwiftData delete; the sync loop is quiesced first so no in-flight pass re-persists the rows after the reset, and is left stopped (manual "Sync Now" still works) so the data stays cleared until the user explicitly resyncs. - rs-platform-wallet: `PlatformPaymentAddressProvider::reset_sync_state` (zero watermark + drop `found`/`absent` seed) and `PlatformAddressWallet::reset_sync_state` (also clears managed-account balances via `all_platform_payment_managed_accounts_mut().clear_balances()`). - rs-platform-wallet manager: `reset_platform_address_sync_state` (quiesce + reset every registered wallet). - rs-platform-wallet-ffi: `platform_wallet_manager_platform_address_sync_reset`. - swift-sdk: `PlatformWalletManager.resetPlatformAddressSyncState()`. - SwiftExampleApp: `PlatformBalanceSyncService.clearLocalState(modelContext:)` (Rust reset -> delete SwiftData rows -> clearDisplay) and the rewired button. Tests: Rust unit test `reset_sync_state_clears_watermark_and_seed` and Swift `PlatformBalanceSyncServiceClearTests` both pass; full platform-wallet lib suite (203) green; `build_ios.sh --target sim` and the SwiftExampleApp build succeed with the new FFI symbol in the regenerated header. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
llbartekll
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 47 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds platform-address sync reset across Rust, FFI, Swift SDK, and the example app, and adds generation-based dropping of stale completion events. The example app’s clear action now resets Rust state first, then removes scoped SwiftData rows. ChangesPlatform Address Sync State Reset
Sequence Diagram(s)sequenceDiagram
participant CoreContentView
participant PlatformBalanceSyncService
participant PlatformWalletManager
participant ModelContext
CoreContentView->>PlatformBalanceSyncService: clearLocalState(modelContext:network:walletIdsOnNetwork:)
PlatformBalanceSyncService->>PlatformWalletManager: resetPlatformAddressSyncState()
PlatformBalanceSyncService->>ModelContext: delete active-network rows
PlatformBalanceSyncService->>ModelContext: save()
PlatformBalanceSyncService->>PlatformBalanceSyncService: clearDisplay()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 93f3bf0) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformBalanceSyncServiceClearTests.swift (1)
21-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a cross-network fixture to pin the delete scope.
This only inserts one network, so it still passes if
clearLocalStatewipes other networks'PersistentPlatformAddress/PersistentPlatformAddressesSyncStaterows too. Seeding a second-network fixture and asserting it survives would catch that regression.🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformBalanceSyncServiceClearTests.swift` around lines 21 - 64, The current test only seeds one network, so it does not verify that PlatformBalanceSyncService.clearLocalState limits deletion to the target network. Update testClearLocalStateWipesPlatformAddressRows in PlatformBalanceSyncServiceClearTests to add a second-network fixture for PersistentPlatformAddress and PersistentPlatformAddressesSyncState, then assert those rows still exist after calling clearLocalState(modelContext:). Keep the existing test focused on the testnet scope while using the same fetch helpers and Self.syncStateScopeId(for:) pattern to pin the delete scope.
🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift`:
- Around line 225-231: The SwiftData wipe in PlatformBalanceSyncService is
deleting all PersistentPlatformAddress and PersistentPlatformAddressesSyncState
rows across every network, not just the active one. Update the reset logic in
the platform-address cleanup path to filter by the current network before
deleting, using the existing network context from the service so only rows for
the active network are removed. Keep the scope aligned with the manager-level
reset contract by adjusting the delete/query logic around modelContext and the
PersistentPlatformAddress / PersistentPlatformAddressesSyncState types.
- Around line 215-238: The clearLocalState flow in PlatformBalanceSyncService is
not failing closed: after resetPlatformAddressSyncState throws it still proceeds
to the SwiftData wipe, and after the delete/save block fails it still calls
clearDisplay(), which clears lastError and presents a false success state.
Update clearLocalState so the reset step and the modelContext delete/save step
each stop the method on failure, and only call clearDisplay() when both the
walletManager reset and the
PersistentPlatformAddress/PersistentPlatformAddressesSyncState deletion complete
successfully.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformBalanceSyncServiceClearTests.swift`:
- Around line 21-64: The current test only seeds one network, so it does not
verify that PlatformBalanceSyncService.clearLocalState limits deletion to the
target network. Update testClearLocalStateWipesPlatformAddressRows in
PlatformBalanceSyncServiceClearTests to add a second-network fixture for
PersistentPlatformAddress and PersistentPlatformAddressesSyncState, then assert
those rows still exist after calling clearLocalState(modelContext:). Keep the
existing test focused on the testnet scope while using the same fetch helpers
and Self.syncStateScopeId(for:) pattern to pin the delete scope.
🪄 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: e1f08534-186d-4faf-9f7e-a38b248f67ca
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformBalanceSyncServiceClearTests.swift
…e network Addresses CodeRabbit review on #3959: - Fail closed: `clearLocalState` previously fell through to `clearDisplay()` (which nils `lastError`) even when the SwiftData delete threw, showing a false "cleared" UI over data still on disk. Now both the Rust reset and the delete return early on failure with `lastError` set; `clearDisplay()` runs only on full success — matching the ShieldedService precedent. - Scope to active network: the SwiftData store holds every network's `PersistentPlatformAddress` / `PersistentPlatformAddressesSyncState` rows at once (the UI filters by network), so a blanket `delete(model:)` also wiped mainnet/devnet cached state when clearing on testnet. The delete is now scoped to the active network — addresses via the `walletIdsOnNetwork` pivot the view already uses, sync-state via `networkRaw` — matching the manager-level Rust reset, which only touches the active network's wallets. The test now seeds two networks and asserts the active one is wiped while the other survives. Passes on the iPhone 17 simulator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses a Codex review finding on #3959: platform-address sync completions were dispatched onto the main actor with no generation guard, unlike the shielded path. A completion callback from a pass drained by `resetPlatformAddressSyncState` (Clear) could already be enqueued as a `Task { @mainactor … }`; when it ran after `clearDisplay()`, it repainted chain-tip height, last-sync time, block time, metrics, and `lastSyncTime` over the just-cleared UI — making it look synced again immediately after Clear (the balance itself stayed 0 since it reads from the deleted SwiftData rows). Mirror the proven shielded generation guard for platform-address sync: - Generalize `ShieldedSyncGenerationCounter` -> `SyncGenerationCounter` (its mechanism was already generic) and add a `platformAddressSyncGeneration` counter on PlatformWalletManager. - The FFI completion callback snapshots the generation on its own thread before the main-actor hop; `handlePlatformAddressSyncCompleted` now guards `generation == platformAddressSyncGeneration.current()` and drops stale events. - `resetPlatformAddressSyncState` (Clear) and `stopPlatformAddressSync` bump the counter after the Rust drain returns, so a trailing completion whose snapshot predates the bump is dropped. Adds `PlatformAddressSyncGenerationTests` (stale dropped after bump, current published, straggler doesn't clobber) — mirrors `ShieldedSyncGenerationTests`. All pass on the iPhone 17 simulator; existing clear test still green. Pure Swift change — the Rust xcframework is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift`:
- Around line 260-264: `resetPlatformAddressSyncState()` only bumps
`platformAddressSyncGeneration`, so the retained published state in
`lastPlatformAddressSyncEvent` and `platformAddressSyncIsSyncing` can survive a
reset and be replayed by a later `configure()` call. Update
`PlatformWalletManagerAddressSync.resetPlatformAddressSyncState()` to clear
those `@Published` properties back to `nil` and `false` after the reset
succeeds, alongside the existing generation bump, so the UI does not repaint
stale sync state.
🪄 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: 01e9fe10-f645-4a7c-b80d-cb4850a94f5c
📒 Files selected for processing (6)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformAddressSyncGenerationTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformBalanceSyncServiceClearTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
Addresses a CodeRabbit re-review finding on #3959. The generation guard only blocks *future* stale completion callbacks; it can't un-publish the value already held on `lastPlatformAddressSyncEvent`. The service's `configure()` re-subscribes with a fresh `.sink`, and Combine replays the current `@Published` value to a new subscriber — so after Clear, a later re-configure would replay the retained event and repaint chain-tip height, last-sync time, and metrics over the cleared UI. `resetPlatformAddressSyncState()` now calls a new `resetPlatformAddressPublishedMirror()` (lives in PlatformWalletManager.swift because `platformAddressSyncIsSyncing` is `private(set)`) which nils `lastPlatformAddressSyncEvent` and sets `platformAddressSyncIsSyncing = false` after the Rust drain. Adds `testResetPublishedMirrorDropsRetainedEvent`. All four generation tests pass on the iPhone 17 simulator. Pure Swift — the Rust xcframework is unchanged. 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.
Code Review
PR cleanly threads a real three-store wipe behind the Platform Sync 'Clear' button across Rust, FFI, Swift SDK, and the example app, and the lock ordering / quiesce-then-reset design is sound. However, the SwiftData wipe deletes durable address metadata (derivation path, public key, account/address index) that the BLAST balance callback hard-depends on (continues silently when the row is missing), and the address-emit path only re-fires on pool extension or wallet reload — so during a live session after Clear, balances cannot be re-persisted and spend/signing paths lose their derivation lookups until app restart. Two smaller issues: a stale completion-event race that the sibling shielded path guards against with a generation counter, and a SwiftData-wipe failure message that clearDisplay() immediately overwrites.
🔴 1 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift:228-231: Clear wipes durable address metadata that the balance callback and spend paths depend on
`PersistentPlatformAddress` is not a balance cache — it also stores the bech32m address, 20-byte hash, account/address index, 33-byte public key, and derivation path (see PersistentPlatformAddress.swift:33-71). After this `delete(model: PersistentPlatformAddress.self)`:
1. The Rust-side `addresses` bijection is intentionally preserved (see `PlatformPaymentAddressProvider::reset_sync_state` docstring), so the `on_persist_account_address_pools_fn` re-emit path does NOT fire on the next sync — it only fires on pool extension or wallet reload.
2. The next full rescan emits balance changesets via `persistAddressBalances` (PlatformWalletPersistenceHandler.swift:88). That callback receives only `addressHash` and at line 97-99 hits `guard let existing = ... else { continue }` — silently dropping the update when the row is missing. The accompanying comment ("the next address-emit pass will bring the row back") is wrong for this scenario.
3. Spend / signing paths that look up derivation metadata from `PersistentPlatformAddress` (account index, derivation path, public key) lose those lookups until app restart re-seeds the rows from Rust persistence.
Fix: zero the balance / nonce / seen-height fields in-place rather than deleting the rows. The watermark wipe via `PersistentPlatformAddressesSyncState` is what actually forces a full rescan; the address rows themselves carry irreplaceable derivation metadata.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift:232-238: SwiftData-wipe failure message is immediately erased by clearDisplay()
On a SwiftData wipe failure, the catch sets `lastError = "Failed to wipe persisted platform-address state: ..."`, then control falls through to `clearDisplay()` at line 238, which unconditionally sets `lastError = nil` (see clearDisplay() line 171). The user never sees the wipe failure, and the published mirror is reset to a fully-cleared state even though the on-disk rows still exist — misrepresenting state. `ShieldedService.clearLocalState` handles this by returning on wipe failure; mirror that here so the error is surfaced and the display isn't lied about.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift:83-86: Stale completion-event Task can repopulate display after Clear
The Rust-side `quiesce()` waits for `platformAddressSyncCompletedCallback` (the C function) to return, but that callback only enqueues `Task { @MainActor ... handlePlatformAddressSyncCompleted(event) }` and returns immediately. If the callback fires near the tail of quiesce, the `@MainActor` Task can be enqueued after `clearLocalState` resumes from its await, then run after `clearDisplay()` completes — restoring `lastSyncHeight`, `chainTipHeight`, `lastSyncTime`, `syncCountSinceLaunch`, metrics counters, and triggering `refreshBalanceSnapshot()` (PlatformBalanceSyncService.swift:279-309). The user-visible impact is small (transient stale metrics until next sync), but it is the same race the sibling shielded path solves with a generation counter snapshotted in the FFI callback and bumped on clear/reset. Worth replicating here since the rest of the PR explicitly mirrors the shielded pattern.
Note: inline comments could not be attached because the PR head moved after this exact-SHA review was claimed; preserving the review as a top-level exact-commit review.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reconciled three prior findings against head 7ee5034: the fail-closed Clear path (prior-2) and the generation-guarded stale completion (prior-3) are FIXED; prior-1 is STILL VALID and remains the sole blocker. The Clear flow now scopes the SwiftData wipe to the active network, but it still deletes PersistentPlatformAddress rows — durable derivation/address metadata that Rust does not re-emit on subsequent syncs — so the next BLAST sync's balance callback (which only carries addressHash) has nothing to upsert against and the UI stays empty. Recommend zeroing balance/nonce/seen-height fields in place and only deleting the network-scoped sync-state row.
🔴 1 blocking
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift:244-247: Clear deletes durable platform-address metadata that the balance callback cannot rebuild
`PersistentPlatformAddress` is not a balance cache — it stores the bech32m address, 20-byte hash, derivation path, account/address index, and 33-byte public key (see PersistentPlatformAddress.swift:33-71). On the Rust side, `PlatformPaymentAddressProvider::reset_sync_state` intentionally preserves the `addresses` bijection (provider.rs:491-494), and `AccountAddressPoolEntry`s are only pushed during initial `register_wallet` (wallet_lifecycle.rs:312) — no BLAST sync round, pool extension, or used-flip re-emits them. The balance callback `persistAddressBalances` keys by `addressHash` and explicitly `continue`s when the row is missing (PlatformWalletPersistenceHandler.swift:97-99). So after Clear deletes the rows, the next BLAST sync updates Rust-side balances but cannot re-create the SwiftData rows, and the Platform Sync UI stays at zero for the remainder of the live session. The fail-closed handling and active-network scoping added in this revision are correct but don't address this root cause. Zero the volatile fields in place instead, and still delete the network-scoped `PersistentPlatformAddressesSyncState` row to force a full rescan.
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | ||
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | ||
| modelContext.delete(row) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Clear deletes durable platform-address metadata that the balance callback cannot rebuild
PersistentPlatformAddress is not a balance cache — it stores the bech32m address, 20-byte hash, derivation path, account/address index, and 33-byte public key (see PersistentPlatformAddress.swift:33-71). On the Rust side, PlatformPaymentAddressProvider::reset_sync_state intentionally preserves the addresses bijection (provider.rs:491-494), and AccountAddressPoolEntrys are only pushed during initial register_wallet (wallet_lifecycle.rs:312) — no BLAST sync round, pool extension, or used-flip re-emits them. The balance callback persistAddressBalances keys by addressHash and explicitly continues when the row is missing (PlatformWalletPersistenceHandler.swift:97-99). So after Clear deletes the rows, the next BLAST sync updates Rust-side balances but cannot re-create the SwiftData rows, and the Platform Sync UI stays at zero for the remainder of the live session. The fail-closed handling and active-network scoping added in this revision are correct but don't address this root cause. Zero the volatile fields in place instead, and still delete the network-scoped PersistentPlatformAddressesSyncState row to force a full rescan.
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | |
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | |
| modelContext.delete(row) | |
| } | |
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | |
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | |
| row.balance = 0 | |
| row.nonce = 0 | |
| row.isUsed = false | |
| row.firstSeenHeight = 0 | |
| row.lastSeenHeight = 0 | |
| row.lastUpdated = Date() | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Clear deletes durable platform-address metadata that the balance callback cannot rebuild 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta correctly resolves the published-mirror replay issue flagged in the prior review (resetPlatformAddressPublishedMirror is now called after the Rust drain). However, the prior blocking finding about Clear deleting durable PersistentPlatformAddress derivation metadata remains valid in this head: persistAddressBalances keys by addressHash and skips missing rows, Rust's reset_sync_state preserves the addresses bijection without re-emitting account_address_pools, and sync_balances emits only a balance diff — so an explicit 'Sync Now' after Clear silently no-ops in SwiftData and the UI stays empty until app restart. Carrying that finding forward; no new in-scope issues.
🔴 1 blocking
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift:244-247: Clear deletes platform-address derivation metadata that no later sync can rebuild
PersistentPlatformAddress is durable derivation state — bech32m address, 20-byte hash, 33-byte compressed public key, account/address index, derivation path (see PersistentPlatformAddress.swift:33-71) — not a balance cache. Deleting these rows on Clear creates an asymmetry that the rest of the pipeline cannot recover from in-session:
1. The Rust-side reset (provider.rs:495-506) intentionally preserves the `addresses` bijection and only clears `found`/`absent`/watermark scalars — its own doc comment notes this is to avoid 'needless re-derivation'.
2. `sync_balances` (sync.rs:110-165) emits only a `PlatformAddressChangeSet { addresses: <balance diff>, ... }`. The `on_persist_account_address_pools_fn` callback that creates `PersistentPlatformAddress` rows is fired only at wallet creation and gap-limit extension, not on every sync.
3. `persistAddressBalances` (PlatformWalletPersistenceHandler.swift:88-114) keys by `addressHash` and `guard let existing = ... else { continue }` skips when the row is missing.
Net effect: after Clear, the next 'Sync Now' produces balance updates against now-missing rows that silently no-op in SwiftData. The Platform Sync UI stays empty (and the spend/signing path loses its derivation-path source) until app restart triggers re-derivation through the wallet load path. This contradicts the file's own contract: 'data stays cleared until the user explicitly resyncs'.
Fix by zeroing only the volatile balance/sync fields in place for `walletIdsOnNetwork` and keeping the durable derivation row, while still deleting the network-scoped `PersistentPlatformAddressesSyncState` row to force a full rescan. The full rescan + preserved rows together give Clear the semantics the doc comment promises.
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | ||
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | ||
| modelContext.delete(row) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Clear deletes platform-address derivation metadata that no later sync can rebuild
PersistentPlatformAddress is durable derivation state — bech32m address, 20-byte hash, 33-byte compressed public key, account/address index, derivation path (see PersistentPlatformAddress.swift:33-71) — not a balance cache. Deleting these rows on Clear creates an asymmetry that the rest of the pipeline cannot recover from in-session:
- The Rust-side reset (provider.rs:495-506) intentionally preserves the
addressesbijection and only clearsfound/absent/watermark scalars — its own doc comment notes this is to avoid 'needless re-derivation'. sync_balances(sync.rs:110-165) emits only aPlatformAddressChangeSet { addresses: <balance diff>, ... }. Theon_persist_account_address_pools_fncallback that createsPersistentPlatformAddressrows is fired only at wallet creation and gap-limit extension, not on every sync.persistAddressBalances(PlatformWalletPersistenceHandler.swift:88-114) keys byaddressHashandguard let existing = ... else { continue }skips when the row is missing.
Net effect: after Clear, the next 'Sync Now' produces balance updates against now-missing rows that silently no-op in SwiftData. The Platform Sync UI stays empty (and the spend/signing path loses its derivation-path source) until app restart triggers re-derivation through the wallet load path. This contradicts the file's own contract: 'data stays cleared until the user explicitly resyncs'.
Fix by zeroing only the volatile balance/sync fields in place for walletIdsOnNetwork and keeping the durable derivation row, while still deleting the network-scoped PersistentPlatformAddressesSyncState row to force a full rescan. The full rescan + preserved rows together give Clear the semantics the doc comment promises.
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | |
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | |
| modelContext.delete(row) | |
| } | |
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | |
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | |
| row.balance = 0 | |
| row.nonce = 0 | |
| row.isUsed = false | |
| row.firstSeenHeight = 0 | |
| row.lastSeenHeight = 0 | |
| row.lastUpdated = Date() | |
| } |
source: ['claude', 'codex']
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3959 +/- ##
=============================================
+ Coverage 52.54% 87.17% +34.62%
=============================================
Files 11 2629 +2618
Lines 1707 327221 +325514
=============================================
+ Hits 897 285265 +284368
- Misses 810 41956 +41146
🚀 New features to boost your workflow:
|
2 participants
Issue being fixed or feature implemented
In the SwiftExampleApp, the Clear button in the Platform Sync Status section (BLAST / DIP-17 address sync) did not remove any data — resyncing afterwards took ~2 seconds, so it looked like Clear did nothing.
Root cause: the button was wired to
PlatformBalanceSyncService.clearDisplay(), which only zeroes the in-memory@PublishedUI vars. The synced data actually lives in three places, none of which Clear touched:PersistentPlatformAddress(cached balances) andPersistentPlatformAddressesSyncState(the network-scoped incremental watermark).PlatformPaymentAddressProvider. This is what makes the resync fast: the nextsync_nowresumes incrementally from the in-memory height instead of doing a full rescan. Because it lives in Rust process memory, a Swift-only delete cannot reset it.ManagedPlatformAccount.address_balances) — whattotal_credits()/addresses_with_balances()and the Transfer/Withdraw spend paths read.The two sibling Clear buttons in the same view already do this correctly: Core Sync →
clearSpvStorage(); Shielded Sync →ShieldedService.clearLocalState(...)(Rust-sideclearShielded()then a SwiftData delete). This PR mirrors that proven pattern for platform addresses.What was done?
A real, three-store wipe wired to the button, bottom-up across Rust → FFI → Swift. The reset is manager-level (all wallets on the network), matching the global SwiftData delete and the shielded precedent (a per-wallet reset would leave sibling wallets' in-memory watermarks to re-populate the deleted rows on the next sync). The background sync loop is quiesced first so no in-flight pass can re-persist after the reset, and is left stopped (manual Sync Now still works) — so the data stays cleared until the user explicitly resyncs.
rs-platform-wallet(provider) —PlatformPaymentAddressProvider::reset_sync_state: zero the watermark and drop the cachedfound/absentseed so the next pass is a full rescan (zeroing the scalars alone is not enough —founddoubles as the next-pass seed).rs-platform-wallet(wallet) —PlatformAddressWallet::reset_sync_state: the provider reset plus clears each managed account's balances viaall_platform_payment_managed_accounts_mut().clear_balances(), so spend paths/total_credits()are immediately consistent rather than self-healing only on the next sync.rs-platform-wallet(manager) —reset_platform_address_sync_state: quiesce the platform-address sync manager, then reset every registered wallet.rs-platform-wallet-ffi— newplatform_wallet_manager_platform_address_sync_reset(mirrorsplatform_wallet_manager_shielded_clear; cbindgen auto-exports it).swift-sdk— thinPlatformWalletManager.resetPlatformAddressSyncState()wrapper.PlatformBalanceSyncService.clearLocalState(modelContext:)(Rust reset → delete the two SwiftData models →clearDisplay()), and the Platform Sync Clear button rewired to call it.New FFI signature
How Has This Been Tested?
cargo fmt --checkclean;cargo clippy -p platform-wallet -p platform-wallet-fficlean.cargo test -p platform-wallet --lib→ 203 passed, including the new unit testreset_sync_state_clears_watermark_and_seed(asserts the watermark zeroes,last_sync_timestamp()returnsNone→ full-scan mode, and thefoundseed is dropped).PlatformBalanceSyncServiceClearTests.testClearLocalStateWipesPlatformAddressRows→ passed on the iPhone 17 simulator (verifies both SwiftData stores are deleted).packages/swift-sdk/build_ios.sh --target sim→ BUILD SUCCEEDED; the new symbol is present in the regenerated cbindgen header and the xcframework.** BUILD SUCCEEDED **.Not yet runtime-verified: no funded testnet platform account was available in this session, so the full UI flow (sync funded DIP-17 wallet → Clear → confirm balance/height hit 0 → Sync Now performs a visibly slow full rescan) was not driven end-to-end on a booted simulator.
Breaking Changes
None. Additive FFI symbol + new Swift wrapper/method; no consensus or public-API breaking changes.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests