fix(swift-sdk): example app ask if the spv is running directly instead of using the sync state#3821
Conversation
|
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 (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAdds a published ChangesSPV Running State Indicator
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 c59d12b) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused change replacing a derived isRunning enum property with a directly-polled @published Bool from the SPV FFI. The convergent concern is that removing the public PlatformSpvSyncState.isRunning is technically source-breaking for downstream SDK consumers, though the new spvIsRunning property cleanly replaces it. Treated as a suggestion rather than blocking given the swift-sdk's active-development status and trivial migration path.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:5-11: Removing public PlatformSpvSyncState.isRunning is source-breaking
`PlatformSpvSyncState.isRunning` was a public computed property on a public enum in `SwiftDashSDK`. Downstream apps reading `progress.overallState.isRunning` will fail to compile after this PR. The PR title isn't marked breaking and the breaking-change checkbox is unchecked, but the deletion silently changes the public surface. Either mark the PR breaking, or retain the property with a `@available(*, deprecated, message: "Use PlatformWalletManager.spvIsRunning")` annotation pointing to the new source of truth. Note the new signal (FFI `spv_is_running`) is semantically different from the old (derived from progress state: only `waitingForConnections`/`syncing` returned true), so silently keeping a no-op shim is not equivalent — deprecation with redirect is the right shape.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "0f0be9e3e230fa6fd622481b8867b9c76fd7bb3756058ab7c047d1ac308d8dd2"
)Xcode manual integration:
|
…d of using the sync state
c59d12b to
77ced90
Compare
Before the arquitectural change the example app was checking if the spv client was running using the sync progress state, in this PR we are replacing that logic with directly calls to the exposed spv client methods that correctly return if the spv client is running or not
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit