feat(swift-sdk): methods that consume the spv client marked as consuming#3478
feat(swift-sdk): methods that consume the spv client marked as consuming#3478
Conversation
📝 WalkthroughWalkthroughUpdated three lifecycle methods in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 12778ee) |
|
✅ 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: "4ec57f223f72723d370c0ad2e7723d0f6a3c505ba028fcc815eea0f73b2c688a"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift (2)
226-234:⚠️ Potential issue | 🔴 CriticalRemove
consumingmodifier fromstopSync()methodThe
consumingmodifier at line 226 of SPVClient.swift conflicts with the established teardown pattern in WalletService.deinit (lines 160-161), which callsstopSync()followed bydestroy()on the same instance. In Swift, a consuming method takes ownership ofself, making it impossible to call methods on that instance afterward. SincestopSync()only sends a stop signal without deallocating resources, it should not consume the instance.Proposed fix
- public consuming func stopSync() { + public func stopSync() { let cancelResult = dash_spv_ffi_client_stop(client) if cancelResult != 0 { let message = SPVClient.getLastDashFFIError() if swiftLoggingEnabled { print("[SPV][Cancel] client stop failed: \(message)") } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift` around lines 226 - 234, The stopSync() method is incorrectly marked consuming, which prevents subsequent calls (e.g., WalletService.deinit calling stopSync() then destroy()); remove the consuming modifier from SPVClient.stopSync() so the method does not take ownership of self and remains callable afterwards. Locate the stopSync declaration in SPVClient.swift and change its signature from "public consuming func stopSync()" to "public func stopSync()", leaving the body intact (still call dash_spv_ffi_client_stop and log errors via SPVClient.getLastDashFFIError()). Ensure no other code relies on the consuming behavior.
175-185:⚠️ Potential issue | 🟠 MajorError path must also reinitialize SPVClient after
clearStorage()failsThe error path (catch block) does not reinitialize the SPVClient when
clearStorage()throws, while the success path always callsinitializeNewSPVClient(). SinceclearStorage()stops the network connection (per the code comment), leaving the client uninitialized on error leaves the resource in a broken state. Either reinitialize in both paths or require the caller to handle reinitialization on error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift` around lines 175 - 185, The error path in consuming func clearStorage() currently throws when dash_spv_ffi_client_clear_storage(client) fails but does not call initializeNewSPVClient(), leaving the SPVClient uninitialized while the success path reinitializes; update clearStorage() so that before rethrowing the SPVError (storageOperationFailed(...)) you reinitialize the client by calling initializeNewSPVClient() (or ensure reinitialization happens in a finally/defer block) so both success and failure paths leave a usable SPVClient; reference the symbols clearStorage(), dash_spv_ffi_client_clear_storage(client), initializeNewSPVClient(), and SPVError.storageOperationFailed(...) to locate and implement the change.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift (1)
11-14: Clarify ownership behavior in the new noteLine 11 updates docs, but the note still describes manual unlock semantics without mentioning that lifecycle APIs are now
consuming. Please explicitly document which calls consume the client and expected replacement flow to avoid caller confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift` around lines 11 - 14, Update the note to clearly state ownership/lifecycle: explain that lifecycle APIs like SPVClient::destroy (and any close/consume methods on SPVClient/FFIDashSpvClient) are consuming operations that take ownership of and free the underlying pointer, unlocking the dataDir; instruct callers to treat the SPVClient instance as unusable after calling destroy (or other consuming APIs) and to create a fresh SPVClient instance if they need to re-open the same dataDir; keep the existing mention that the FFIDashSpvClient locks the dir to avoid concurrency corruption and tie that behavior to the consuming lifecycle methods so callers understand the correct replacement flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 226-234: The stopSync() method is incorrectly marked consuming,
which prevents subsequent calls (e.g., WalletService.deinit calling stopSync()
then destroy()); remove the consuming modifier from SPVClient.stopSync() so the
method does not take ownership of self and remains callable afterwards. Locate
the stopSync declaration in SPVClient.swift and change its signature from
"public consuming func stopSync()" to "public func stopSync()", leaving the body
intact (still call dash_spv_ffi_client_stop and log errors via
SPVClient.getLastDashFFIError()). Ensure no other code relies on the consuming
behavior.
- Around line 175-185: The error path in consuming func clearStorage() currently
throws when dash_spv_ffi_client_clear_storage(client) fails but does not call
initializeNewSPVClient(), leaving the SPVClient uninitialized while the success
path reinitializes; update clearStorage() so that before rethrowing the SPVError
(storageOperationFailed(...)) you reinitialize the client by calling
initializeNewSPVClient() (or ensure reinitialization happens in a finally/defer
block) so both success and failure paths leave a usable SPVClient; reference the
symbols clearStorage(), dash_spv_ffi_client_clear_storage(client),
initializeNewSPVClient(), and SPVError.storageOperationFailed(...) to locate and
implement the change.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 11-14: Update the note to clearly state ownership/lifecycle:
explain that lifecycle APIs like SPVClient::destroy (and any close/consume
methods on SPVClient/FFIDashSpvClient) are consuming operations that take
ownership of and free the underlying pointer, unlocking the dataDir; instruct
callers to treat the SPVClient instance as unusable after calling destroy (or
other consuming APIs) and to create a fresh SPVClient instance if they need to
re-open the same dataDir; keep the existing mention that the FFIDashSpvClient
locks the dir to avoid concurrency corruption and tie that behavior to the
consuming lifecycle methods so callers understand the correct replacement flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a610c307-af74-4eae-8020-0492dc785671
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This is a very small Swift SDK lifecycle annotation change with no obvious crash-level regression in the checked-in callers. The one thing that does not hold up is the core premise in the PR description: marking these methods consuming on a reference type does not actually prevent misuse or aliasing, so the wrapper still needs explicit invalidation if the goal is to make stop/clear/destroy one-way operations for SDK consumers.
Reviewed commit: 12778ee
🟡 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/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- [SUGGESTION] lines 175-226: `consuming` does not actually make this reference-type client single-use
`SPVClient` is still a normal reference type (`public class SPVClient`), so changing `clearStorage()`, `destroy()` and `stopSync()` to `consuming` does not revoke aliases or enforce one-way lifecycle at the API boundary. The codebase itself still demonstrates aliasing: `WalletService` assigns `dummyClient` into `self.spvClient`, then later calls `dummyClient.destroy()` while `self.spvClient` still references the same object (`packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift:125-138`). That means the old misuse pattern is still possible from another reference, even though the top-level documentation now removes the previous lifecycle limitations. If the intent is to make these terminal operations impossible to reuse incorrectly, the wrapper still needs explicit invalidation/state checks (or a genuinely move-only wrapper); the `consuming` annotation alone on a class does not provide that guarantee.
2 participants
marked as consuming methods that consume the spv client
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Refactor
Documentation