feat(swift-sdk): auto-start Core SPV sync on app launch#4105
Conversation
Extracts the Sync tab Start button's config assembly (per-network data dir, peer-override modes, devnet name) into a shared CoreSpvLauncher and invokes it from bootstrap once the SDK and the launch network's wallets are loaded, so Core sync no longer needs a manual Start tap. Gated to once per launch, skipped when SPV is already running or the network has no wallets yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
llbartekll and
shumkov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a centralized Core SPV launcher, network-aware peer resolution, stale-start cancellation, guarded launch-time auto-start, coordinated stop behavior, and decision-matrix tests. ChangesCore SPV startup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftExampleAppApp
participant WalletManagerStore
participant CoreSpvLauncher
participant SDK
participant PlatformWalletManager
SwiftExampleAppApp->>WalletManagerStore: validate active manager and generation
SwiftExampleAppApp->>CoreSpvLauncher: auto-start launch network
CoreSpvLauncher->>SDK: discoverActiveMasternodes when needed
SDK-->>CoreSpvLauncher: return peer addresses
CoreSpvLauncher->>WalletManagerStore: revalidate generation and active manager
CoreSpvLauncher->>PlatformWalletManager: startSpv with startup config
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 215af6e) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR cleanly extracts Start-button SPV configuration into a shared CoreSpvLauncher and wires it into bootstrap with sound once-per-launch/no-double-start/wallet-gating. However, the devnet peer-discovery branch performs a synchronous, semaphore-blocking network call (up to 6s) from the MainActor-isolated automatic bootstrap path, and the new lifecycle gating has no test coverage.
Source: reviewers gpt-5.6-sol (Sol reviewer) and claude-sonnet-5 (Sonnet reviewer); verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
🔴 1 blocking | 🟡 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/CoreSpvLauncher.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/CoreSpvLauncher.swift:86-92: Automatic launch now freezes the main thread for up to 6s on devnet
peerOverride(network:) calls SDK.discoverActiveMasternodes(quorumBase:) for devnet (SDK.swift:141-180), which issues a URLSession request and blocks the calling thread on a DispatchSemaphore with a 6-second timeout. CoreSpvLauncher is @MainActor, and CoreSpvLauncher.start is now invoked from bootstrap() via the also-@MainActor autoStartCoreSpvIfNeeded() (SwiftExampleAppApp.swift:327-350, called at line 408). Before this PR, the blocking call only fired from an explicit Start tap; now every devnet cold launch with a restored wallet can freeze the UI for up to six seconds while waiting on the quorum service. Resolve peers off the main actor before invoking this path automatically.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:327-349: No test coverage for the new auto-start lifecycle gates
autoStartCoreSpvIfNeeded() introduces three distinct gating behaviors—skip when already latched this launch, skip when the active manager has no wallets, and skip when SPV is already running—with no automated test exercising them. These gates prevent duplicate starts and premature syncing during onboarding; regressions could silently restore manual-start-only behavior or cause redundant SPV starts.
…t gates The devnet peer discovery blocks on a semaphore for up to 6s; hop it to a detached task so the launch-time auto-start (and the Start button) can no longer freeze the UI. Extract the auto-start gate matrix into pure CoreSpvAutoStart.decision and pin its asymmetric latching invariants with unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta fixes both prior findings: devnet peer discovery now runs off the main actor, and the auto-start gate has five focused unit tests. However, making manual Start asynchronous introduces a race where a delayed devnet peer lookup can finish after a network switch and silently restart SPV on the old, inactive wallet manager.
Carried-forward prior findings: None. Prior reconciliation: b9b6bbe26ae4 FIXED; 755d18a7a136 FIXED. New findings in latest delta: 1 blocking.
Source: reviewers gpt-5.6-sol (Sol reviewer) and claude-sonnet-5 (Sonnet reviewer); verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
🔴 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/Views/CoreContentView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift:755-769: Async manual Start can silently revive SPV on a network the user already switched away from
`startSync()` launches an unstructured `Task` that captures the currently active `walletManager` and network before `CoreSpvLauncher.start` awaits the detached devnet peer lookup for up to six seconds. If the user switches networks during that wait, `OptionsView` updates `currentNetwork` and stops that old manager, while `WalletManagerStore.activate` swaps the UI to a different manager. The suspended task still owns the old manager reference, so when discovery completes it calls `startSpv` unconditionally and restarts sync for the abandoned network with no visible control to stop it. This interleaving did not exist before `start` became async. Cancel or revalidate the pending start on a network change, or verify the requested network and manager are still active before calling `startSpv`.
After the off-main peer resolution, revalidate that the captured wallet manager is still the store's active one before calling startSpv; a network switch (or devnet manager rebuild) mid-await now throws CancellationError, which both call sites treat as benign, instead of reviving sync on the abandoned network. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The stillCurrent guard added in c09ca8e only checks manager identity against WalletManagerStore.activeManager, but activeManager is not replaced synchronously when a network switch or devnet rebuild begins. A pending manual Start can still land in that gap and revive the manager the user just stopped, so the prior blocking finding remains valid. Two smaller in-scope observations from this delta are also kept: the new bail path has no test, and one comment overstates auto-start behavior.
Carried-forward prior findings: 1 blocking. New findings in latest delta: 1 suggestion, 1 nitpick.
Source: reviewers gpt-5.6-sol (Sol reviewer), claude-sonnet-5 (Sonnet reviewer), and claude-opus-4-8 (Opus reviewer); verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
🔴 1 blocking | 🟡 1 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift:759-777: stillCurrent guard misses the window between stopSpv() and activeManager replacement
`stillCurrent` only tests `store.activeManager === manager`, but the store is not swapped synchronously when a switch or rebuild starts stopping the old manager. In `OptionsView.swift`, changing the picker writes `currentNetwork` and immediately calls `stopSpv()` on the old manager; `WalletManagerStore.activate(...)` replaces `activeManager` later from the app-level SwiftUI `onChange` handler. If pending peer discovery resumes in that gap, the guard passes and `startSpv()` revives the manager the user switched away from. The devnet-rebuild path is wider: it stops the manager, awaits the SDK rebuild, and only then activates the replacement. Invalidate or cancel in-flight starts when a switch/rebuild begins, such as with a generation token captured at start and checked immediately before `startSpv`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/CoreSpvLauncher.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/CoreSpvLauncher.swift:33-57: No test covers the new stale-start bail path
The new `guard stillCurrent() else { throw CancellationError() }` path has no focused test. A fake-manager/fake-store test that asserts a stale closure throws before `startSpv` would lock in the intended behavior and expose timing gaps in the guard.
… token Manager identity alone missed the stop-to-activate gap (OptionsView stops the old manager synchronously; the store swaps activeManager later from onChange). WalletManagerStore now carries a monotonically increasing spvStartGeneration, bumped at the earliest synchronous point of every supersession path (network picker, docker toggle, devnet rebuild, custom-peers toggle, Pause) plus defensively on any actual activeManager swap in activate(). Start call sites capture the generation before initiating and stillCurrent requires both the generation and manager identity to match. Adds launcher tests via an injected startSpv seam and rewords the auto-start supersession comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This delta closes the prior blocking race with a spvStartGeneration token on WalletManagerStore, invalidated synchronously at every path that stops or replaces the active manager (network picker, Docker toggle, custom-peers toggle, devnet rebuild, and CoreContentView.pauseSync), checked alongside manager identity in CoreSpvLauncher.start's stillCurrent guard. All three prior findings are verified FIXED against the current source: the generation token closes the stop-to-activate window, CoreSpvLauncherStartTests.swift adds focused stale-start/generation-mismatch coverage, and the auto-start comment now accurately describes once-per-launch behavior. However, one new blocking issue was verified from first principles: GlobalSyncIndicator (ContentView.swift:619-674), a second, independent stop-sync affordance shown as an 'x' button on the sync-progress overlay, calls walletManager.stopSpv() directly without walletManagerStore.invalidatePendingSpvStarts() — unlike CoreContentView.pauseSync(), which this same PR updated to call it first. walletManagerStore is already injected as an environmentObject at the app-scene root (SwiftExampleAppApp.swift:128) and already declared on the parent ContentView, so nothing blocks wiring it into GlobalSyncIndicator. Because spvProgress.overallState and spvIsRunning are independently polled FFI queries (PlatformWalletManager.swift:1145-1170) with no code path that synchronously resets overallState when stopSpv is called, overallState can plausibly still read .syncing/.waitingForConnections — and the details-expanded overlay with its 'x' button remain visible — while a freshly-tapped Start's off-main peer resolution (up to ~6s on devnet) is still in flight. In that window the pending start's stillCurrent closure sees an unchanged generation and manager identity (stopSpv alone changes neither), so it proceeds to call startSpv and silently revives sync right after the user tapped stop — reopening the exact race class this PR set out to close.
Carried-forward prior findings: 0 (all three FIXED). New findings in latest delta: 1 blocking.
Source: reviewers gpt-5.6-sol (Sol reviewer) and claude-sonnet-5 (Sonnet reviewer), general + ffi-engineer; verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/ContentView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swift:619-674: GlobalSyncIndicator's stop button bypasses invalidatePendingSpvStarts(), reopening the stale-start race this PR fixes elsewhere
This delta fixes CoreContentView.pauseSync() (Core/Views/CoreContentView.swift:792-797) by calling walletManagerStore.invalidatePendingSpvStarts() before walletManager.stopSpv(), specifically so a pending CoreSpvLauncher.start suspended in its off-main peer-resolution await can't resume and revive sync after the user just stopped it. GlobalSyncIndicator (ContentView.swift:619-674) is a second, independent UI entry point for stopping sync — the 'x' button shown when `showDetails` is true (Sync tab active + details expanded) while `overallState` is `.syncing`/`.waitingForConnections`. Its action at line 650-651 calls `try? walletManager.stopSpv()` directly, with no invalidation call, even though `walletManagerStore` is already available: it's injected via `.environmentObject(walletManagerStore)` at the app-scene root (SwiftExampleAppApp.swift:128) and already declared as `@EnvironmentObject var walletManagerStore: WalletManagerStore` on the parent ContentView (ContentView.swift:28). Verified that stopSpv() itself changes neither spvStartGeneration nor activeManager (WalletManagerStore.swift only bumps the generation inside invalidatePendingSpvStarts(), called from OptionsView/CoreContentView.pauseSync/activate() — never from this button). Verified that overallState/spvIsRunning are two independently polled FFI queries (PlatformWalletManager.swift:1145-1170) with no code path resetting overallState synchronously on stop, so a fresh manual Start's peer resolution can begin while overallState is still reporting a stale .syncing/.waitingForConnections value from before the stop — keeping this button visible and tappable during that window. If tapped there, the in-flight start's stillCurrent check still passes (unchanged generation, unchanged manager), so startSpv fires right after the user's explicit stop.
Code ReviewThis delta correctly fixes all three prior findings: WalletManagerStore.spvStartGeneration is now invalidated synchronously at the earliest point of every network-switch, SDK-rebuild, peer-config, and pause path, both stillCurrent guards check it alongside manager identity, and CoreSpvLauncherStartTests.swift adds direct coverage for the stale-start and generation-mismatch bail paths. Both FFI-specialist passes (codex-ffi-engineer, sonnet5-ffi-engineer) confirm this PR touches no FFI/cross-language boundary code and introduces no boundary defect. However, one stop-adjacent path was missed: GlobalSyncIndicator's stop button (ContentView.swift:650-651) still calls stopSpv() directly without invalidatePendingSpvStarts(), unlike every other stop path this PR wires up, reopening the same stale-start race elsewhere in the PR for that one entry point. Carried-forward prior findings: None (all 3 FIXED). New findings in latest delta: 1 blocking. Source: reviewers gpt-5.6-sol (Sol general + FFI reviewer) and claude-sonnet-5 (Sonnet general + FFI reviewer); verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only). 🔴 1 blocking 🤖 Prompt for all review comments with AI agents |
GlobalSyncIndicator's overlay stop button bypassed the pending-start invalidation. Adds WalletManagerStore.stopSpvCancellingPendingStarts as the blessed user-facing stop path (used by the overlay button and pauseSync), documents the bump-before-stop invariant on invalidatePendingSpvStarts, and makes both start entry points bump then capture the generation so the latest start supersedes any still-pending one (collapses the rapid multi-tap window). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the GlobalSyncIndicator finding in d72de1c (review-body finding, no inline thread to resolve):
Verified: warnings-as-errors sim build, all 8 unit tests pass, and on the simulator auto-start still fires and Pause stops cleanly through the new helper. 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Rechecked exact head d72de1c against the prior GlobalSyncIndicator finding and full thread context. The overlay stop and Sync-tab Pause now share the invalidate-before-stop helper; the deeper OptionsView supersession paths retain their earlier synchronous invalidation; and Start/auto-start capture a fresh generation before awaiting, so stale and rapid overlapping starts cannot revive stopped sync. All required Swift SDK CI and CodeRabbit checks are green. No remaining actionable findings.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This head (d72de1c) closes the previously-blocking stale-start race: GlobalSyncIndicator's stop button and CoreContentView.pauseSync now both route through the new WalletManagerStore.stopSpvCancellingPendingStarts helper, which bumps the generation token before stopping SPV, so an in-flight start suspended in peer resolution can no longer resume and revive sync after a user-facing stop. The implementation is sound with no remaining blocking issues. Two verified suggestions remain: the new invalidate-then-stop helper has no direct test coverage, and the launch-time auto-start await keeps the app on the initialization screen for up to ~6s on devnet while masternode discovery blocks.
Source: reviewers gpt-5.6-sol (Sol reviewer) and claude-sonnet-5 (Sonnet reviewer), general lane; verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
🟡 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/WalletManagerStore.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift:96-99: No test exercises WalletManagerStore.stopSpvCancellingPendingStarts directly
`stopSpvCancellingPendingStarts` is the exact fix for the previously-flagged stale-start race and is now the sole blessed path for every user-facing SPV stop (`GlobalSyncIndicator`'s stop button, `CoreContentView.pauseSync`). Its correctness hinges entirely on ordering: `invalidatePendingSpvStarts()` must run before `try? manager.stopSpv()`. Neither of the two test files this PR adds (`CoreSpvAutoStartTests`, `CoreSpvLauncherStartTests`) instantiates a real `WalletManagerStore` — they only exercise the pure `CoreSpvAutoStart.decision` matrix and a mocked generation-comparison closure via `CoreSpvLauncher.start`'s injectable `startSpv` seam. A future refactor that reorders these two lines, or a call site that reverts to a raw `stopSpv()`, would silently reopen the exact race this PR set out to close, with nothing in CI to catch it.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:434-441: Devnet peer discovery can hold the app on the initialization screen for up to 6 seconds
`bootstrap()` awaits `autoStartCoreSpvIfNeeded()` before setting `isInitialized = true`, so `ContentView` stays gated on the "Initializing..." screen for the whole call. On devnet, that path runs `CoreSpvLauncher.start` → `peerOverride(network:)` → `SDK.discoverActiveMasternodes`, which the launcher's own doc comment says can block on a `DispatchSemaphore` for up to ~6s when the quorum endpoint is slow or unavailable (confirmed in `CoreSpvLauncher.swift:18-21`). This PR already moved that resolution off the main actor so the UI thread doesn't freeze, but the user still can't reach the app during that window because `isInitialized` isn't flipped until the whole auto-start completes. `autoStartCoreSpvIfNeeded()` fully handles its own errors internally (`CancellationError` and the general catch both just log, per `SwiftExampleAppApp.swift:362-379`), and the generation guard already resolves any race with a concurrent manual Start tap, so nothing depends on this call finishing before the UI unlocks.
Adds an injected-stop seam on stopSpvCancellingPendingStarts plus a test asserting the generation is already bumped when the stop runs, and moves the launch auto-start after isInitialized so devnet peer discovery can't hold the app on the initializing screen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Rechecked exact head 215af6e. The UI now unlocks before best-effort SPV auto-start awaits devnet discovery, and the new WalletManagerStoreStopSpvTests directly pins generation invalidation before the injected stop runs. Swift SDK warnings-as-errors build/tests are green; no remaining actionable findings.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This head (215af6e) closes both non-blocking suggestions carried over from d72de1c: the new WalletManagerStore test seam and focused unit test pin invalidate-before-stop ordering, and bootstrap now sets isInitialized before awaiting auto-start so devnet discovery no longer gates first render. Direct current-head and cumulative inspection found no new in-scope issue. The verifier also dropped one false FFI follow-up because PlatformWalletManager is already explicitly @mainactor.
Prior reconciliation: prior-1 FIXED; prior-2 FIXED. Carried-forward prior findings: none. New findings in the latest delta: none.
Source: reviewers gpt-5.6-sol (Sol reviewer), claude-sonnet-5 (Sonnet reviewer), and claude-opus-4-8 (Opus reviewer), across general and ffi-engineer lanes; verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
2 participants
Issue being fixed or feature implemented
Core SPV sync required a manual Start tap on the Sync tab every launch. For a wallet app the expected behavior is that the chain sync just runs — the app should come up syncing.
What was done?
Core/Services/CoreSpvLauncher.swift— single source of truth for starting Core SPV: assembles the per-network data dir, peer override (regtest+docker / devnet auto-discover / custom peers / default seeds), and devnet name into aPlatformSpvStartConfigand callsstartSpv. The peer-override mode resolution moved here verbatim fromCoreContentView.spvPeerOverride().CoreContentView.startSync()is now a thin call into the launcher (~80 lines of duplicated-to-be logic deleted), so the Start button and auto-start drive identical code.SwiftExampleAppApp.bootstrap()calls a newautoStartCoreSpvIfNeeded()right afterrebindWalletScopedServices()— i.e. once the SDK is initialized and the launch network's wallets are loaded. Guards: fires at most once per process (survivesretryBootstrap), skips if SPV is already running, and skips when the active network has no wallets (first-run onboarding doesn't burn bandwidth syncing headers for no wallet). Failures are logged, never fatal — they can't trip the bootstrap error UI.OptionsViewdoc-comment cross-reference updated to the launcher.Scope notes: auto-start is launch-time and launch-network only — creating the first wallet mid-session or switching networks still uses the Start button (unchanged behavior); broadening either would be a follow-up.
How Has This Been Tested?
build_ios.sh --target sim(warnings-as-errors) passes.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit