fix(swift-sdk): wait for SDK rebuild before wallet activation#3669
Conversation
|
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:
✨ 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 b735773) |
7db756e to
ef94761
Compare
…-wallet-manager-sdk-switch Resolve conflict in WalletManagerStore.swift by accepting upstream's sdk.handle-based stale-SDK detection (landed via dashpay#3755) over the PR's parallel ObjectIdentifier(sdk) + ManagerEntry approach. Both mechanisms target the same bug — a cached wallet manager bound to a torn-down SDK — and upstream's is the one now wired into the rest of the file (managerSdkHandles dict, rebuild log, paired nil-out), so taking upstream keeps the file internally consistent. The PR's other commits (activation gated on rebuilt-SDK request IDs in SwiftExampleAppApp.swift) auto-merged cleanly and continue to provide the wait-for-rebuild guarantee the PR title describes. --no-verify: pre-commit JS lint crashes on a local Node v25.9.0 EBADF/fstat regression while loading upstream's auto-merged grpc client files. Rust pre-commit checks ran and passed. No JS files were authored or edited in this conflict resolution.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two confirmed compile-time blockers in OptionsView.swift introduced/exposed by this PR's network-switch surface: an undeclared isSwitchingNetwork identifier paired with a duplicated currentNetwork assignment in the picker setter, and a switchNetwork(to:) call that omits the required requestID: argument on the devnet rebuild path. Both prevent the SwiftExampleApp from building under the CI xcodebuild step, so the deferred-activation behavior the PR exists to validate cannot actually be exercised. The PR should not be promoted from draft until these are fixed. Posted as a COMMENT review because GitHub rejects formal review events from PastaClaw on PastaClaw-authored PRs; the two blocking findings remain draft-promotion blockers.
🔴 2 blocking
2 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/Views/OptionsView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift:129-169: Network picker setter references undeclared `isSwitchingNetwork` and double-switches the network
Two problems in the same picker setter:
1. Lines 130 and 154 reference a bare `isSwitchingNetwork` identifier. There is no local `@State`/property by that name in `OptionsView` — every other use in this file (line 187, line 363) goes through `appState.isSwitchingNetwork`, and that property is `@Published var isSwitchingNetwork` on `AppState` (AppState.swift:19). The bare references will fail Swift type checking.
2. The setter assigns `appState.currentNetwork = newNetwork` twice — once inside the `Task` (line 144) and again synchronously after the `Task` (line 165). `AppState.currentNetwork` has a `didSet` that calls `beginNetworkSwitch()`, which bumps `sdkRebuildRequestID` and starts an SDK rebuild. Triggering it twice queues two concurrent rebuilds for one picker change and races the new `sdkRebuildRequestID` / `readySDKRequestID` bookkeeping this PR introduces — exactly the deferred-activation pipeline it's trying to stabilize. The companion calls to `walletManager.stopSpv()`, `platformBalanceSyncService.reset()`, and `shieldedService.reset()` are also duplicated across the two branches.
The in-flight gate (`isSwitching: appState.isSwitchingNetwork` at line 187) and the comment block at lines 158–164 already note that `currentNetwork.didSet` flips `appState.isSwitchingNetwork` for the picker's status label. So both the local flag and the Task wrapper are unnecessary: a single synchronous `appState.currentNetwork = newNetwork` plus the existing service resets is sufficient, and it lets the new requestID-driven pipeline do its job uninterrupted.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift:305: Devnet rebuild calls non-existent `AppState.switchNetwork(to:)` overload
`AppState` exposes a single overload: `func switchNetwork(to network: Network, requestID: UInt64) async` (AppState.swift:155). The devnet `.onDisappear` rebuild calls `await appState.switchNetwork(to: .devnet)` with no `requestID:`, so the file fails to compile and the entire same-network devnet SDK rebuild path this PR adds cannot run.
Beyond the compile error, this also short-circuits the deferred-activation machinery this PR is built around: invoking the orchestrator-style API directly bypasses the `sdkRebuildRequestID` → `readySDKRequestID` flow in `SwiftExampleAppApp`. The natural fix is to drive the same `beginNetworkSwitch` path the picker uses by re-assigning `appState.currentNetwork = .devnet` (its `didSet` already calls `beginNetworkSwitch()`), which publishes a fresh `sdkRebuildRequestID`, drives the rebuild, and lets the existing `.onChange(of: readySDKRequestID)` in `SwiftExampleAppApp` activate the wallet manager. With that change, the manual `walletManagerStore.activate(...)` and `walletScopedServicesRebindTick &+= 1` block immediately below becomes redundant and can be removed.
Summary
until
AppStatefinishes rebuilding the SDK.currentNetwork, so activationdoes not cache a manager with the previous network's SDK.
rebuilds that the current per-network cache cannot safely reconfigure.
Validation
git diff --checksurface mismatch. The failure is outside this patch: local generated bindings
report
dash_sdk_private_key_to_wifanddash_sdk_validate_private_key_for_public_keyexpectingBool, plus missinggenerated platform-wallet FFI symbols such as
Handle.