refactor(swift-sdk): spv client config wrapper#3469
Open
refactor(swift-sdk): spv client config wrapper#3469
Conversation
Contributor
📝 WalkthroughWalkthroughSPV configuration was moved into a new Changes
Sequence Diagram(s)sequenceDiagram
participant WalletService as WalletService
participant SPVConfig as SPVConfig
participant SPVClient as SPVClient
participant FFI as DashSDKFFI
WalletService->>SPVConfig: createSpvConfig(network, dataDir, flags)
SPVConfig->>FFI: ffi_client_config_new(...) / add peers / set options
WalletService->>SPVClient: init(config: SPVConfig, eventHandlers)
SPVClient->>FFI: dash_spv_ffi_client_new(config.config, ...)
Note right of SPVClient: retains Swift SPVConfig wrapper
WalletService->>SPVClient: setMasternodesEnabled(true)
WalletService->>SPVConfig: createSpvConfig() (updated)
SPVClient->>FFI: dash_spv_ffi_client_update_config(config.config)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
ZocoLini
force-pushed
the
refactor/spv-config-wrapper
branch
from
0bcda65 to
aeadc8d
Compare
Collaborator
|
✅ Review complete (commit a6c1fe4) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`:
- Around line 110-119: createSpvConfig(_:) currently always calls
clientConfig.setMasternodeSyncEnabled(true), which makes calls like
disableMasternodeSync() and setMasternodesEnabled(_:) ineffective; modify
createSpvConfig to accept a masternodeSyncEnabled: Bool parameter (or remove the
hardcoded set from inside it) and use that parameter when calling
clientConfig.setMasternodeSyncEnabled(...), then update callers such as
setMasternodesEnabled and disableMasternodeSync to pass the correct
enabled/disabled value so the rebuilt SPV config reflects the caller's intent.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 64-66: The updateConfig(_:) implementation currently assigns
self.config before calling dash_spv_ffi_client_update_config and doesn't check
its return value; change it to keep the old SPVConfig alive (e.g., let old =
self.config), call dash_spv_ffi_client_update_config(config.config), check the
return code and throw an error on non-zero result, and only on success assign
self.config = config (then release old); also remove the stray semicolon after
the assignment in the original code.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift`:
- Around line 33-35: The code falls back to a hard-coded "127.0.0.1:20001" when
UserDefaults.standard.string(forKey: "corePeerAddresses") is empty, which fixes
the port to regtest; change the fallback to construct the default peer using the
selected network's default port instead of 20001. Locate the SPVConfig code
reading corePeerAddresses (the UserDefaults.standard.string(forKey:
"corePeerAddresses") usage) and replace the literal "127.0.0.1:20001" with a
string built from "127.0.0.1" plus the port derived from the active network
configuration (e.g., use the network object's default/mainnet/testnet/devnet
port property or a helper like network.defaultPort), so
restrictToConfiguredPeers(true) on other networks will use the correct
local-core port.
- Around line 4-96: SPVConfig is currently internal so SDK consumers can't
construct or customize it; make the class and its API public so it becomes the
SDK-facing config surface: change the declaration of SPVConfig to public (retain
`@unchecked` Sendable as-is), mark the initializer init(_ network: Network) as
public, and mark all mutator methods used by consumers
(setMasternodeSyncEnabled, setMempoolStrategy, setUserAgent, setStartHeight,
setDataDir, clearPeers, addPeer, restrictToConfiguredPeers) as public; keep the
FFI pointer property appropriately scoped (make it private or fileprivate if you
want to hide internals) and leave deinit as-is.
🪄 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: ab011592-92a9-4950-83c9-88000ce70dae
📒 Files selected for processing (3)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
ZocoLini
force-pushed
the
refactor/spv-config-wrapper
branch
2 times, most recently
from
0ee33d9 to
ec22d3a
Compare
ZocoLini
changed the title
ZocoLini
force-pushed
the
refactor/spv-config-wrapper
branch
from
ec22d3a to
9ec8f9f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift (1)
64-66:⚠️ Potential issue | 🔴 Critical
updateConfig(_:)can silently fail and desynchronize client vs wrapper config.Line 64-66 ignores the FFI return code and mutates
self.configfirst. If the FFI update fails, the SDK state no longer reflects the active native config.Proposed fix
- func updateConfig(_ config: consuming SPVConfig) { - self.config = config; - dash_spv_ffi_client_update_config(client, config.config) + func updateConfig(_ config: consuming SPVConfig) throws { + let rc = dash_spv_ffi_client_update_config(client, config.config) + if rc != 0 { + throw SPVError.storageOperationFailed(SPVClient.getLastDashFFIError()) + } + self.config = config }#!/bin/bash # Verify DashSDKFFI update-config signature/usage and whether rc is currently ignored. rg -n -C3 '\bdash_spv_ffi_client_update_config\b' packages/swift-sdk --type swiftExpected result: declaration/call sites show a non-void return code and the current
SPVClient.updateConfig(_:)path not validating it.🤖 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 64 - 66, The updateConfig(_:) currently assigns self.config before calling dash_spv_ffi_client_update_config, and ignores its return code; change SPVClient.updateConfig(_:) to call dash_spv_ffi_client_update_config first, check its return value, and only set self.config on success (otherwise surface the error by throwing or returning a failure and logging the FFI error); ensure you reference dash_spv_ffi_client_update_config's actual return/errno semantics and map that to a Swift error type or Result so the wrapper and native client stay synchronized.packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift (1)
33-35:⚠️ Potential issue | 🟠 MajorFallback peer still hard-codes regtest port.
Line 34 still defaults to
127.0.0.1:20001, which conflicts with the “network default port” behavior in the comment and can dead-end non-regtest local setups when peer restriction is enabled.Proposed fix
- let peers = (raw?.isEmpty == false ? raw! : "127.0.0.1:20001") + let peers = (raw?.isEmpty == false ? raw! : "127.0.0.1") .split(separator: ",") .map { $0.trimmingCharacters(in: .whitespaces) } .filter { !$0.isEmpty }🤖 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/SPVConfig.swift` around lines 33 - 35, The fallback peer currently hard-codes "127.0.0.1:20001" which forces regtest port; change the fallback to use the configured network's default port instead. Replace the literal "127.0.0.1:20001" used when UserDefaults key "corePeerAddresses" is empty with a string built from "127.0.0.1" plus the network default port (obtain via your existing network config API used in SPVConfig, e.g., SPVConfig.network.defaultPort or Network.defaultPort(for: network)). Update the expression that produces peers (the variable raw and peers) so the fallback uses that computed host:port string rather than the hard-coded port.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`:
- Around line 187-190: The code force-unwraps dataDir when calling
clientConfig.setDataDir(dataDir!) which can crash if FileManager fails; update
the logic around dataDir creation (the FileManager.default.urls(...) chain and
the use of network.rawValue) to safely unwrap the optional (e.g., guard let or
if let) and handle the nil case gracefully by returning/throwing an error or
logging and using a sensible fallback before calling clientConfig.setDataDir;
ensure the fix touches the dataDir variable creation and the
clientConfig.setDataDir call so no force-unwrapping remains.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift`:
- Line 5: Remove the stray trailing semicolons that are causing SwiftLint/CI
failures: e.g., change the declaration "internal var config:
UnsafeMutablePointer<FFIClientConfig>;" to remove the semicolon and do the same
for the other declaration on line 39 (remove the trailing ";" from that symbol
as well). Ensure no semicolons remain at the ends of Swift declarations in
SPVConfig.swift.
---
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 64-66: The updateConfig(_:) currently assigns self.config before
calling dash_spv_ffi_client_update_config, and ignores its return code; change
SPVClient.updateConfig(_:) to call dash_spv_ffi_client_update_config first,
check its return value, and only set self.config on success (otherwise surface
the error by throwing or returning a failure and logging the FFI error); ensure
you reference dash_spv_ffi_client_update_config's actual return/errno semantics
and map that to a Swift error type or Result so the wrapper and native client
stay synchronized.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift`:
- Around line 33-35: The fallback peer currently hard-codes "127.0.0.1:20001"
which forces regtest port; change the fallback to use the configured network's
default port instead. Replace the literal "127.0.0.1:20001" used when
UserDefaults key "corePeerAddresses" is empty with a string built from
"127.0.0.1" plus the network default port (obtain via your existing network
config API used in SPVConfig, e.g., SPVConfig.network.defaultPort or
Network.defaultPort(for: network)). Update the expression that produces peers
(the variable raw and peers) so the fallback uses that computed host:port string
rather than the hard-coded port.
🪄 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: 80e31311-da79-45c5-9d84-971295bf636c
📒 Files selected for processing (3)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
ZocoLini
force-pushed
the
refactor/spv-config-wrapper
branch
from
9ec8f9f to
c558809
Compare
Contributor
|
✅ 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: "85038f8818f9b82f40c33a53d2f9b9bcd52aac0edf772944c3319f30c9c7690a"
)Xcode manual integration:
|
ZocoLini
force-pushed
the
refactor/spv-config-wrapper
branch
from
c558809 to
33f88e5
Compare
thepastaclaw
suggested changes
Apr 10, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The refactor is narrowly scoped, but it removes the only error propagation path at the Swift-to-FFI boundary. Both initial SPV configuration and runtime config updates can now fail silently, which is a behavioral regression from the previous implementation and leaves higher layers believing configuration changes succeeded when the native client may still be using older or default state.
Reviewed commit: 33f88e5
🔴 2 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/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift`:
- [BLOCKING] lines 7-94: SPVConfig drops all fallible FFI configuration results during client setup
This wrapper now makes SPV configuration effectively infallible even though the previous implementation treated several of these FFI calls as fallible. `setDataDir`, `setUserAgent`, `addPeer`, `restrictToConfiguredPeers`, and `setMasternodeSyncEnabled` all discard the native return code, and `SPVConfig.init` no longer has any way to surface configuration failure before client creation. In the prior `SPVClient.init`, rejected data directories and user-agent setup threw immediately, and peer-add failures were at least logged. With the new wrapper, invalid peer strings, unusable storage paths, or rejected options can be silently ignored and the client is then created with partially applied or default native state. That changes the API contract from fail-fast to silent misconfiguration without any compensating validation path.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- [BLOCKING] lines 64-66: Runtime config updates now report success even when the native client rejects them
`updateConfig` replaces the stored `SPVConfig` and ignores the result of `dash_spv_ffi_client_update_config`. The caller chain also has no recovery path: [WalletService.swift](/Users/claw/.openclaw/workspace/worktrees/review-platform-3469/packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift#L197) sets `masternodesEnabled` before calling `updateConfig`, so the published Swift state is updated even if the FFI refuses the live reconfiguration. This is a real regression from the previous implementation, which checked both `dash_spv_ffi_config_set_masternode_sync_enabled` and `dash_spv_ffi_client_update_config` and threw on failure. As written, the UI can claim masternode sync was toggled while the underlying SPV client continues running with the old config.
ZocoLini
force-pushed
the
refactor/spv-config-wrapper
branch
from
33f88e5 to
a6c1fe4
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift (1)
7-23: Add a helper method or extension to safely map Network to FFI config instead of switching on hardcoded rawValues.Switching on
network.rawValuewith hardcoded cases0/1/2/3is fragile and couples this code to undocumented numeric values. If the FFI binding's raw values ever change, this fails silently. Instead, add an extension onNetwork(orDashSDKNetwork) with a computed property or static method that maps each network to its corresponding FFI config constructor. This pattern is already used elsewhere in the SDK (e.g.,KeyWalletNetwork.ffiValue,PlatformNetwork.ffiValue).🤖 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/SPVConfig.swift` around lines 7 - 23, Replace the fragile rawValue switch in SPVConfig.init by adding a mapping on Network (e.g., an extension or computed property like Network.ffiSPVConfig or Network.toFFIConfig) that returns the correct FFI constructor call (dash_spv_ffi_config_mainnet(), dash_spv_ffi_config_testnet(), dash_spv_ffi_config_new(FFINetwork(rawValue: 2)), dash_spv_ffi_config_new(FFINetwork(rawValue: 3))) for each enum case, then use that property inside SPVConfig.init (instead of switching on network.rawValue) so the mapping is explicit, type-safe, and centralised (similar to KeyWalletNetwork.ffiValue / PlatformNetwork.ffiValue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift`:
- Around line 7-23: Replace the fragile rawValue switch in SPVConfig.init by
adding a mapping on Network (e.g., an extension or computed property like
Network.ffiSPVConfig or Network.toFFIConfig) that returns the correct FFI
constructor call (dash_spv_ffi_config_mainnet(), dash_spv_ffi_config_testnet(),
dash_spv_ffi_config_new(FFINetwork(rawValue: 2)),
dash_spv_ffi_config_new(FFINetwork(rawValue: 3))) for each enum case, then use
that property inside SPVConfig.init (instead of switching on network.rawValue)
so the mapping is explicit, type-safe, and centralised (similar to
KeyWalletNetwork.ffiValue / PlatformNetwork.ffiValue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cb1c084-131c-4330-a019-f4d8830bc363
📒 Files selected for processing (4)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDWallet.swift
✅ Files skipped from review due to trivial changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDWallet.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
thepastaclaw
suggested changes
Apr 10, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The SPV config-wrapper refactor is directionally fine, but it drops all error propagation on the newly wrapped configuration/apply path. The old Swift wrapper surfaced configuration/apply failures; this head silently ignores them, which means SDK callers can now believe an SPV client was configured or reconfigured successfully when the native FFI rejected the request.
Reviewed commit: a6c1fe4
🔴 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/Sources/SwiftDashSDK/Core/SPV/SPVConfig.swift`:
- [BLOCKING] lines 62-94: The new config wrapper silently discards every fallible FFI configuration error
The previous Swift wrapper treated several SPV configuration operations as fallible: `set_data_dir`, `set_user_agent`, `set_masternode_sync_enabled`, and `client_update_config` all returned status codes that were checked and surfaced back to callers. In this refactor, every `SPVConfig` mutator simply drops the return value, and `SPVClient.updateConfig` also ignores the result of `dash_spv_ffi_client_update_config(client, self.config.config)`. That turns real native failures into silent no-ops. For example, `WalletService.setMasternodesEnabled()` now always does `self.spvClient.updateConfig(createSpvConfig())` with no failure path, so if the native client rejects the update the Swift side will still act as though the change succeeded. Because `updateConfig` assigns `self.config = config` before making the FFI call, Swift can also end up believing it is running with a new config object even though the underlying client is still on the old settings. This is a regression in API correctness, not just ergonomics: callers have lost all ability to detect or react to rejected configuration changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Made a wrapper for the SPVClient config structure, giving more flexibility to the sdk users and better following the SPVClient library API.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Refactor
New Features