feat(swift-sdk,rs-sdk-ffi): wire devnet SDK config + auto-discover masternodes#3755
Open
shumkov wants to merge 4 commits into
Open
feat(swift-sdk,rs-sdk-ffi): wire devnet SDK config + auto-discover masternodes#3755shumkov wants to merge 4 commits into
shumkov wants to merge 4 commits into
Conversation
…rnodes
Adds the devnet ergonomics fixes that were riding along with the 1M-note
shielded-sync work. Split out so they can land independently of the
chain-side snapshot bake and the iOS sync-visibility UI changes.
Changes:
- `DashSDKConfig` gains a `quorum_url` field (rs-sdk-ffi). Plumbed into
`dash_sdk_create_trusted` → `TrustedHttpContextProvider::new_with_url`
so devnet builds can point at the quorum-list service.
- `SDK.swift` adds `discoverActiveMasternodes(quorumBase:)` — fetches
`{base}/masternodes` and returns `[(spvPeer, dapiUrl)]`. Used at SDK
build time to populate DAPI addresses without manual entry.
- `SDK.init` for devnet now always auto-discovers fresh from
`/masternodes`; no UserDefaults write-back so node churn self-heals
on every relaunch / network switch.
- `OptionsView` devnet branch reduced from three inputs (SPV peers /
DAPI URL / Quorum URL) to one (Quorum URL only) — both SPV and DAPI
derive from the same `/masternodes` response.
- `CoreContentView.spvPeerOverride` devnet branch maps `discoverActiveMasternodes`
output to its `spvPeer` field (paloma reports `ip:20001` per node;
we use the verbatim values rather than guessing 29999).
- `WalletManagerStore.activate` adds a stale-SDK cache check — the
cached `PlatformWalletManager` is rebuilt when `AppState` swaps the
SDK (network switch or endpoint override change) so proof
verification doesn't fail forever against stale DAPI endpoints.
- `SwiftExampleApp/Info.plist` is now hand-managed (peer to .xcodeproj
so `PBXFileSystemSynchronizedRootGroup` doesn't auto-include it as a
bundle resource). Adds `NSAllowsArbitraryLoads=true` so the HTTP
`/masternodes` fetch isn't blocked by ATS. Test-only banner in the
plist makes the dev intent explicit.
- `SDKLogger.log` now mirrors to `NSLog` in addition to `Swift.print` so
`simctl spawn booted log stream` captures it without Xcode attached.
NOT in this PR (separate follow-ups):
- Per-pass timing UI rows / longest-pass tracking / per-chunk progress
callback (sync visibility bucket).
- Genesis-bake of 1M Orchard notes on the chain side
(`rs-drive-abci`, `dashmate`, `rs-platform-wallet` test scaffolding).
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an FFI ChangesQuorum URL Configuration and Masternode Discovery
Sequence DiagramsequenceDiagram
participant CoreContentView
participant SDK_discoverActiveMasternodes
participant QuorumServer
participant SwiftSDKInit
participant RustFFI
CoreContentView->>SDK_discoverActiveMasternodes: call with quorumBase (from UserDefaults)
SDK_discoverActiveMasternodes->>QuorumServer: GET {quorumBase}/masternodes
QuorumServer-->>SDK_discoverActiveMasternodes: return JSON masternodes
SDK_discoverActiveMasternodes->>SwiftSDKInit: return spvPeer,dapiUrl list
SwiftSDKInit->>RustFFI: call dash_sdk_create_trusted(config with quorum_url,dapi_addresses)
RustFFI-->>SwiftSDKInit: return SDK handle / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift (1)
118-157: 🏗️ Heavy liftAvoid blocking SDK initialization on a synchronous semaphore-backed network request.
This path can freeze UI threads for up to the timeout window when
SDK.initis called from the main actor. Consider moving discovery to an async initializer/factory and surfacing progress/error state instead of blocking.Also applies to: 205-286
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift` around lines 118 - 157, The discoverActiveMasternodes implementation blocks with a DispatchSemaphore and URLSession.dataTask (using ResponseBox) which can freeze the main actor; convert it to an async implementation (or provide an async factory) and stop using semaphore/ResponseBox. Change public static func discoverActiveMasternodes(quorumBase: String) -> [(spvPeer: String, dapiUrl: String)]? into an async/throwing variant (e.g. public static func discoverActiveMasternodes(quorumBase: String) async throws -> [(spvPeer: String, dapiUrl: String)]) and replace the URLSession.dataTask/semaphore logic with URLSession.shared.data(for: request) or URLSession.shared.data(from: url) using await, propagate errors instead of returning nil, and remove ResponseBox, semaphore, task.resume()/cancel(). If SDK.init currently calls the synchronous version, add an async initializer or a separate async factory method that invokes the new async discoverActiveMasternodes so initialization no longer blocks the main thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- Around line 163-179: The current masternode filtering only checks mn.status ==
"ENABLED" and will include nodes that failed a version check; update the
Masternode Decodable struct to include a version_check: String property and
change the compactMap predicate for env.data (the active computation) to require
both mn.status == "ENABLED" and mn.version_check == "success" before building
the host/URL tuple so only nodes that passed the version_check are returned.
In `@packages/swift-sdk/SwiftExampleApp/Info.plist`:
- Around line 62-66: Replace the global NSAllowsArbitraryLoads entry under
NSAppTransportSecurity with domain-scoped exceptions or move the permissive
plist into debug-only config: remove NSAllowsArbitraryLoads and add an
NSExceptionDomains dictionary under NSAppTransportSecurity containing explicit
host keys for your devnet hosts (e.g., "dev.example.com") with
NSExceptionAllowsInsecureHTTPLoads/NSIncludesSubdomains as needed, or
alternatively keep the current permissive entry only in a debug-specific
Info.plist used for development and ensure release builds use a tightened
Info.plist; update any build settings or plist configuration to reference the
debug plist for debug configurations and the secure plist for release.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/project.pbxproj`:
- Around line 464-466: The Release build is using the test-oriented Info.plist
(INFOPLIST_FILE = Info.plist) which relaxes ATS; update project settings so
Release uses a separate strict plist or override the plist-binding via xcconfig
for the Release configuration. Specifically, change the Release build's
GENERATE_INFOPLIST_FILE / INFOPLIST_FILE reference (or add a Release-specific
INFOPLIST_FILE like Info-Release.plist) or add an xcconfig entry to enforce
NSAppTransportSecurity defaults for Release, ensuring
INFOPLIST_KEY_UIApplicationSceneManifest_Generation remains unchanged but the
Release configuration points to the strict plist/override.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:
- Around line 632-637: CoreContentView.spvPeerOverride() currently calls
SDK.discoverActiveMasternodes(quorumBase:) synchronously during the startSync()
devnet path which can block the UI because the SDK uses a URLSession with a
DispatchSemaphore.wait; change this so discovery runs asynchronously off the
main thread (or prefetch/cache results ahead of Start) and return immediately
from spvPeerOverride() when discovery is pending; specifically, replace the
inline call to SDK.discoverActiveMasternodes(quorumBase:) with an async dispatch
or a cached lookup and update the syncing logic to use the async result (or
cached value) once available to avoid blocking startSync().
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- Around line 118-157: The discoverActiveMasternodes implementation blocks with
a DispatchSemaphore and URLSession.dataTask (using ResponseBox) which can freeze
the main actor; convert it to an async implementation (or provide an async
factory) and stop using semaphore/ResponseBox. Change public static func
discoverActiveMasternodes(quorumBase: String) -> [(spvPeer: String, dapiUrl:
String)]? into an async/throwing variant (e.g. public static func
discoverActiveMasternodes(quorumBase: String) async throws -> [(spvPeer: String,
dapiUrl: String)]) and replace the URLSession.dataTask/semaphore logic with
URLSession.shared.data(for: request) or URLSession.shared.data(from: url) using
await, propagate errors instead of returning nil, and remove ResponseBox,
semaphore, task.resume()/cancel(). If SDK.init currently calls the synchronous
version, add an async initializer or a separate async factory method that
invokes the new async discoverActiveMasternodes so initialization no longer
blocks the main thread.
🪄 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: 48107086-a816-4fd2-985e-16c1570a1e40
📒 Files selected for processing (12)
packages/rs-sdk-ffi/src/sdk.rspackages/rs-sdk-ffi/src/token/claim.rspackages/rs-sdk-ffi/src/token/emergency_action.rspackages/rs-sdk-ffi/src/token/freeze.rspackages/rs-sdk-ffi/src/types.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/SwiftExampleApp/Info.plistpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/project.pbxprojpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift
thepastaclaw
suggested changes
May 27, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two blocking issues confirmed: an integration test struct literal in rs-sdk-ffi is missing the new quorum_url field (test target won't compile), and the Swift discoverActiveMasternodes decoder is materially stricter than the Rust trusted-context provider it shadows — it requires platformHTTPPort on every entry and skips the versionCheck == "success" filter, so a single quirky masternode row can break devnet SDK creation entirely. Remaining items are devnet-scaffolding concerns: synchronous fetch on the main actor, broad ATS bypass that bleeds into Release builds, cleartext-HTTP trust in the discovery path, and a couple of doc / dead-code nits.
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 3 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/rs-sdk-ffi/tests/context_provider_test.rs`:
- [BLOCKING] packages/rs-sdk-ffi/tests/context_provider_test.rs:82-89: Integration test struct literal missing new `quorum_url` field — won't compile
`DashSDKConfig` in `packages/rs-sdk-ffi/src/types.rs:88` gained a required `quorum_url: *const c_char` field. The in-crate token unit tests were updated, but this integration test under `tests/` was missed. Because integration tests are compiled as a separate crate, `cargo check -p rs-sdk-ffi` (which the PR description cites as clean) does not type-check them — only `cargo check -p rs-sdk-ffi --tests` / `cargo test -p rs-sdk-ffi` does. CI for the FFI crate's test target is broken on this commit and will fail with `error[E0063]: missing field 'quorum_url' in initializer of 'DashSDKConfig'`.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:159-179: Swift masternode discovery is stricter than the Rust provider it shadows
`discoverActiveMasternodes` declares `platformHTTPPort: UInt16` as a required Decodable field and filters only on `status == "ENABLED"`. The equivalent Rust trusted-context provider at `packages/rs-sdk-trusted-context-provider/src/provider.rs:316-327` treats `platform_http_port` as optional with a per-network default and additionally filters on `version_check == "success"`. Two practical consequences for the devnet path this PR enables:
1. A single masternode entry in the JSON missing `platformHTTPPort` makes the whole decode fail (`try?` returns nil), so `init(.devnet)` falls through with `overrideAddresses = nil` and the SDK refuses to build — even though the Rust-side discovery would have produced a usable list.
2. Nodes the quorum service has already flagged as `versionCheck != success` get seeded into both the DAPI fan-out and `spvPeerOverride`, undermining the self-healing behavior this PR cites as the reason for re-discovering on every build.
Align the Swift decoder with the Rust source of truth.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:147-156: Devnet discovery blocks the main actor for up to 6 s per SDK build
`discoverActiveMasternodes` issues an async `URLSession` request and then waits on a `DispatchSemaphore` with a 6 s ceiling on top of the 5 s `URLRequest.timeoutInterval`. `SDK.init(network:)` calls this synchronously for devnet, and `init` is invoked from `AppState.initializeSDK` / `switchNetwork` on a `@MainActor` class, plus a second call from `CoreContentView.spvPeerOverride()` on the SPV start path. A slow or unreachable quorum-list service will freeze the UI exactly when the user is trying to fix connectivity, with no loading affordance. Since this path is now mandatory for devnet, expose an `async` variant (or run discovery off the main thread before constructing the SDK) so the synchronous shape doesn't bleed into UI-driven code.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:121-130: Devnet discovery accepts cleartext quorum URLs, enabling MITM of trusted SDK setup
`discoverActiveMasternodes` accepts any URL scheme, and the same `platformQuorumURL` is passed to both auto-discovery and `dash_sdk_create_trusted` (used for `TrustedHttpContextProvider`'s quorum-key fetch). With the new `NSAllowsArbitraryLoads = true` plist, an attacker on the network — or a hostile DNS resolver — that intercepts an `http://...` quorum URL can rewrite the `/masternodes` payload to redirect SPV/DAPI traffic at attacker hosts, and serve matching quorum keys to the trusted provider so forged platform state verifies as authentic. There is also no validation on the parsed `address`: `mn.address.split(separator: ":").first` is used verbatim in URL construction, so an entry like `"address": "evil.example:1@victim:443"` could parse surprisingly. At minimum, reject non-`https` quorum URLs unless the host is a loopback literal (so a devnet `http://127.0.0.1:8080` still works), and validate that the parsed host is a plain IP literal or DNS label before reusing it.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/project.pbxproj`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/project.pbxproj:461-465: Release config also points at Info.plist with `NSAllowsArbitraryLoads = true`
`GENERATE_INFOPLIST_FILE = NO` + `INFOPLIST_FILE = Info.plist` is set in both Debug (~line 432) and Release (~line 461) configs, and the new `Info.plist` enables `NSAllowsArbitraryLoads`, globally disabling ATS. Every `URLSession` request in the app — not just the documented devnet `/masternodes` fetch — can then use plaintext HTTP. The plist's own comment says this MUST be removed before shipping, but nothing in the build system enforces it: a Release archive built today ships with ATS off. Gate the exception to Debug-only via a separate `Info.plist` / `xcconfig` per configuration, or scope it through `NSExceptionDomains` to the quorum-list host(s) only.
shumkov
changed the title
CI macOS Rust test job failed compilation — the integration test under `packages/rs-sdk-ffi/tests/context_provider_test.rs` builds a DashSDKConfig struct literal that was missed when the new field was added. Production and unit-test sites were already updated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3755 +/- ##
============================================
- Coverage 87.17% 87.17% -0.01%
============================================
Files 2607 2607
Lines 319489 319484 -5
============================================
- Hits 278507 278500 -7
- Misses 40982 40984 +2
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior compile-break (missing quorum_url in context_provider_test) is resolved at this SHA. Two convergent blocking issues remain: the Swift /masternodes decoder diverges from the Rust trusted-provider contract (non-optional platformHTTPPort, missing version_check filter), and the Info.plist that disables ATS is wired into both Debug and Release configs. Additional suggestions cover main-actor blocking discovery, cleartext quorum URLs feeding the trusted setup, and a dropped quorum_url on the callback-based create path.
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SDK.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:159-179: Swift /masternodes decoder diverges from Rust trusted-provider contract
`discoverActiveMasternodes` declares `platformHTTPPort: UInt16` (non-optional) and filters only on `status == "ENABLED"`. The Rust source of truth at `packages/rs-sdk-trusted-context-provider/src/provider.rs:309-336` treats `platform_http_port` as optional with a per-network default (443 mainnet / 1443 testnet / 443 otherwise) and additionally requires `version_check == "success"`. Two consequences on this PR's mandatory devnet path:
1. A single `/masternodes` row missing `platformHTTPPort` makes the whole decode fail (`try?` → nil), so `SDK.init(.devnet)` ends up with `overrideAddresses = nil` and devnet SDK creation fails even though Rust would have produced a usable list.
2. Nodes already flagged `versionCheck != success` get seeded into both the DAPI fan-out and `spvPeerOverride`, undercutting the self-healing claim in the PR description.
Align the Swift decoder with Rust.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/project.pbxproj`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/project.pbxproj:464-465: Release builds inherit a global ATS bypass via shared Info.plist
Both Debug (project.pbxproj:435-436) and Release (project.pbxproj:464-465) set `GENERATE_INFOPLIST_FILE = NO; INFOPLIST_FILE = Info.plist;`, and that `Info.plist` enables `NSAllowsArbitraryLoads = true` (Info.plist:62-66). The plist comment explicitly says this exception is test-only and MUST be removed before shipping, but the build system does not enforce that: a Release archive built from this branch disables ATS for every `URLSession` in the app, not just the devnet `/masternodes` probe. Combined with the unrestricted-scheme quorum URL accepted by `discoverActiveMasternodes`, this widens the MITM surface for any user running the example app on an untrusted network. Gate the exception to Debug only (separate per-config Info.plist / xcconfig) or scope it through `NSExceptionDomains` to the quorum-list host(s) only.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/sdk.rs:633-646: `quorum_url` propagated into callback-based config but silently ignored
`dash_sdk_create_with_callbacks` copies `config_ref.quorum_url` into `DashSDKConfigExtended.base_config` at sdk.rs:634-643, but the callback path uses `CallbackContextProvider` and `dash_sdk_create_extended` never reads `base_config.quorum_url`. A C/Swift caller populating the field gets no error and no effect — the API surface implies support the Rust side does not provide. Either reject non-null `quorum_url` here with `InvalidParameter`, or zero it and document that `quorum_url` is honored only by `dash_sdk_create_trusted`.
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: "849b5e4af2728fad6a8a57612f7e95d5f0288c3df7053b5a1b062bb3025721c8"
)Xcode manual integration:
|
Triaged review feedback on PR #3755. Applied the five findings that were in scope for "devnet wiring + auto-discover masternodes": 1. `SDK.swift::discoverActiveMasternodes` — align active-node filter with the Rust trusted-context provider. Make `platformHTTPPort` optional (default 443 when missing — matches Rust's per-network fallback) so a single misbehaving JSON entry no longer fails the whole decode. Add `version_check == "success"` to the filter alongside `status == "ENABLED"` so nodes the quorum service has flagged as incompatible don't get seeded into the DAPI fan-out or SPV peer list. 2. `project.pbxproj` Release config — flip back to `GENERATE_INFOPLIST_FILE = YES`. The hand-managed `Info.plist` with `NSAllowsArbitraryLoads = true` is a Debug-only dev tool; Release builds now use the auto-generated plist with default ATS (preventing a release archive from accidentally shipping with ATS off if anyone runs `xcodebuild -configuration Release`). Debug still uses the permissive plist for `/masternodes` HTTP fetches. 3. `SDK.swift::discoverActiveMasternodes` doc — fix incorrect claim that the method's `public` visibility saves a round-trip. The two known call sites (`SDK.init` via `discoverDAPIAddresses`, and `CoreContentView.spvPeerOverride`) each call independently with no shared cache, so an SDK rebuild on devnet performs two `/masternodes` fetches. Clarified in the doc. 4. `DashSDKConfig::quorum_url` doc — document that the field is only honored on the `dash_sdk_create_trusted` path (which builds a `TrustedHttpContextProvider`); the callback-based path uses `CallbackContextProvider` and silently drops non-null values. `dash_sdk_create_with_callbacks` still forwards the value into the extended config since the struct field is required, but readers now know it's a dead field on that path. 5. `Info.plist` — replace the leaked `/Users/ivanshumkov/Projects/dashpay/quorum-list-server` absolute path in the test-only banner comment with the public GitHub URL. Verified `cargo check -p rs-sdk-ffi --tests` clean. Verified `xcodebuild` succeeds on both Debug and Release configurations (iPhone 17 simulator, arm64) — Release with the new auto-generated plist builds without errors. Out-of-scope findings deliberately not addressed in this PR: - `SDK.swift:130` cleartext-URL guard: enforcing https-or-loopback would actively break the documented use case (paloma devnet at `http://44.238.203.84:8080` — not a loopback). - `SDK.swift:156` main-thread blocking: fixing properly requires making `SDK.init` async, an API-breaking refactor that spans multiple call sites (`AppState.initializeSDK`, `switchNetwork`, `spvPeerOverride`). Better as its own focused PR.
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward: two convergent suggestion-level issues remain — cleartext quorum URLs accepted at the trusted-setup entry, and synchronous main-actor blocking during devnet discovery. New in this delta: the freshly added versionCheck CodingKey maps to "version_check" (snake_case) while the Rust source of truth and quorum-list-server emit camelCase versionCheck. This makes the new filter universally false, so every devnet SDK build returns nil from discoverActiveMasternodes and fails — a hard regression that wipes out the PR's mandatory devnet path. Prior fixes for the decoder divergence, Release-config ATS bleed, leaked developer path, and stale doc claim are confirmed.
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SDK.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:185-190: versionCheck CodingKey maps to the wrong JSON key, breaking every devnet SDK build
The new `CodingKeys` enum declares `case versionCheck = "version_check"`, telling `JSONDecoder` to look for snake_case `version_check` in the `/masternodes` response. The Rust source of truth (`packages/rs-sdk-trusted-context-provider/src/provider.rs:73-80`) renames the Rust field with `#[serde(rename = "versionCheck")]`, i.e. the actual JSON key the quorum-list-server emits is camelCase `versionCheck`. Because Swift now looks for the wrong key, every decoded `Masternode.versionCheck` is `nil`, so the new filter `mn.versionCheck == "success"` (line 202) is false for every entry, `active` is always empty, and `discoverActiveMasternodes` returns `nil`. `SDK.init(network: .devnet)` mandatorily routes through this helper (lines 289-299) and falls through to `overrideAddresses = nil`, which the Rust side then refuses to build — so every cold devnet boot at this SHA fails SDK creation, exactly the regression the version_check filter was meant to prevent. The PR's manual paloma test would have passed before this rename was added (no filter at all, so all ENABLED nodes passed). Fix by mapping the case to the camelCase key the server actually emits, or drop the rename entirely so Swift's default property-name match handles it.
My earlier fix added `case versionCheck = "version_check"` to `Masternode.CodingKeys`, telling JSONDecoder to look for snake_case `version_check` in the `/masternodes` response. The actual JSON wire key is camelCase `versionCheck` — the Rust source of truth at `packages/rs-sdk-trusted-context-provider/src/provider.rs:72-80` renames its snake_case Rust field with `#[serde(rename = "versionCheck")]`, so the server emits camelCase on the wire. Consequence: every decoded entry had `versionCheck == nil`, the filter `mn.versionCheck == "success"` was false for every node, `active` was always empty, `discoverActiveMasternodes` returned nil, and `SDK.init(.devnet)` fell through to `overrideAddresses = nil` — breaking every cold devnet boot. Fix: drop the `CodingKeys` enum entirely. Swift's default `Decodable` synthesis matches property names to JSON keys literally, so `platformHTTPPort` and `versionCheck` properties match the camelCase wire keys with no rename machinery. Added a doc comment recording the wire-format invariant so it stays in sync with the Rust source if either side renames.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta (7cfede7..a6a9c5a) is a 9-line, single-purpose fix: it removes the wrong CodingKeys enum on Swift's Masternode decoder so that the property names platformHTTPPort/versionCheck now match the camelCase wire keys emitted by the Rust trusted-context provider via #[serde(rename = ...)] (provider.rs:76-79). Verified — the fix is correct and resolves the prior blocking issue; no new defects introduced. Prior-finding reconciliation: version-check-coding-key-wrong = FIXED; release-ats-bypass, developer-path-leak, single-roundtrip-doc = FIXED in earlier pushes and still resolved; cleartext quorum URLs accepted into trusted SDK setup (SDK.swift:129-138) = STILL VALID; main-actor sync semaphore on devnet discovery (SDK.swift:140-165) = STILL VALID; testnet default DAPI port divergence = STILL VALID but only reachable on the devnet code path today (dropped as not-in-scope nitpick); callback-path quorum_url silently dropped = INTENTIONALLY DEFERRED by author via doc comment (kept out of the public review). Carrying forward two suggestion-level findings.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SDK.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:129-138: Carried-forward: `discoverActiveMasternodes` accepts cleartext quorum URLs that feed the trusted SDK setup
Carried forward from the 7cfede7b review — STILL VALID at a6a9c5a9; the latest delta does not touch this path. `discoverActiveMasternodes` validates only that `URLComponents.scheme` is non-empty (SDK.swift:130-133), so plain `http://` is accepted. The same `platformQuorumURL` is then forwarded across the FFI as `config.quorum_url` into `dash_sdk_create_trusted` (SDK.swift:308-313), where the Rust `TrustedHttpContextProvider` uses it as the base URL for quorum-public-key lookups (rs-sdk-trusted-context-provider/src/provider.rs:309-330). Because the Debug `Info.plist` still ships `NSAllowsArbitraryLoads = true` and the OptionsView placeholder encourages `http://<host>:8080`, an on-path attacker on a devnet/coffee-shop network can rewrite `/masternodes` to substitute attacker-controlled DAPI/SPV peers AND serve matching forged quorum public keys to the trusted provider — collapsing the trust boundary `dash_sdk_create_trusted` is supposed to give. Require `https` unless the host is a loopback literal so dashmate-local `http://127.0.0.1:8080` keeps working.
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.
Issue being fixed or feature implemented
Until now, getting the SwiftExampleApp talking to a devnet required manually entering three pieces of information in the Options screen: SPV peers, DAPI URL, and the quorum-list URL. The DAPI URL was load-bearing — without it the SDK had no addresses to talk to — but it was also redundant with the quorum-list service, which already exposes every active masternode's address. As nodes churn the manually-entered DAPI URL would silently go stale and proof verification would start failing with "no available addresses to use". The Swift side had no plumbing to feed the quorum URL into
TrustedHttpContextProvider, and the cachedPlatformWalletManagercontinued referencing the previous Sdk clone after a network / endpoint switch.What was done?
DashSDKConfig(rs-sdk-ffi) gains aquorum_urlfield plumbed intoTrustedHttpContextProvider::new_with_urlfromdash_sdk_create_trusted.SDK.swiftaddsdiscoverActiveMasternodes(quorumBase:)— fetches{base}/masternodesand returns[(spvPeer, dapiUrl)]. DevnetSDK.initnow always auto-discovers fresh on every build, so node churn self-heals.OptionsViewdevnet UX shrinks from three inputs to one — Quorum URL only.CoreContentView.spvPeerOverridedevnet branch derives SPV peers from the same/masternodesresponse (verbatimip:CoreP2PPortper node — paloma reports:20001).WalletManagerStore.activaterebuilds the cached manager when the SDK handle changes, so endpoint overrides take effect on the next activation.SwiftExampleApp/Info.plistis now hand-managed (peer to the .xcodeproj soPBXFileSystemSynchronizedRootGroupdoesn't auto-bundle it), gains an ATS exception so the HTTP/masternodesfetch isn't blocked. Test-only banner in the plist makes the dev intent explicit.SDKLogger.logmirrors toNSLogin addition toSwift.printsosimctl spawn booted log streamcaptures it without an attached debugger.How Has This Been Tested?
cargo check -p rs-sdk-ffi— clean.xcodebuild -scheme SwiftExampleApp -destination 'platform=iOS Simulator,name=iPhone 17'— builds successfully (only a benign__eh_framelinker warning).test/sheilded_test_data): launching the app cold, switching to devnet, and entering only the quorum URL produces a successful SPV sync and ShieldedNetworkSummary lookup with no further configuration.Breaking Changes
DashSDKConfigadds a new field. C/Swift callers that construct the struct literal must initializequorum_url(useptr::null()/nilwhen not configuring a quorum URL).Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Tests