feat(swift-example-app): enable SPV on devnet#3761
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps workspace rust-dashcore dependency revisions; routes SPV devnet identity and optional LLMQ overrides through a DevnetConfig exposed to the FFI and Swift example, and adds a WalletInfoInterface delegation for monitored script pubkeys. ChangesPlatform SPV / Devnet wiring
Workspace Dependency Revision Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified codex-general's blocker against upstream source at eb889af1 and against cargo check. The workspace does not compile against the pinned revision: dash-spv's ClientConfig no longer exposes llmq_devnet_params (replaced by devnet: Option<DevnetConfig>), and key-wallet's WalletInfoInterface trait gained a required monitored_script_pubkeys method that PlatformWalletInfo does not implement. The PR cannot land as-is without follow-up callsite updates. Both Claude agents missed these regressions.
🔴 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/rs-platform-wallet-ffi/src/spv.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/spv.rs:317-322: `platform-wallet-ffi` references `ClientConfig::llmq_devnet_params` removed at eb889af1
Verified against the upstream source at rev `eb889af1` (cargo git checkout `rust-dashcore-c6b13647c01f74b9/eb889af`): `dash_spv::ClientConfig` no longer exposes a `llmq_devnet_params` field. The devnet surface was consolidated into a single `pub devnet: Option<DevnetConfig>` field (see `dash-spv/src/client/config.rs:25` and `:76`, with `with_devnet(DevnetConfig)` as the builder API at `:191`). This file still does `config.llmq_devnet_params = Some(LlmqDevnetParams { size, threshold })`, which fails to compile against the pinned revision and will block the entire workspace build (and any transitive consumer such as the Swift SDK). The import of `dashcore::sml::llmq_type::LlmqDevnetParams` at line 6 is now also dead code at this callsite. Callsite needs to be migrated to construct a `DevnetConfig` (e.g. `DevnetConfig::new(name).with_llmq_params(LlmqDevnetParams { size, threshold })`) and assign it via `config.devnet = Some(...)` / `with_devnet(...)`. Note: the codex agent flagged this; the Claude agents missed it.
In `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:33: `PlatformWalletInfo` is missing `monitored_script_pubkeys` required by updated `WalletInfoInterface`
Confirmed by running `cargo check -p platform-wallet-ffi` against the pinned revision: `error[E0046]: not all trait items implemented, missing: 'monitored_script_pubkeys'`. At `eb889af1`, `key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface` adds a required method `fn monitored_script_pubkeys(&self) -> Vec<ScriptBuf>` (see `key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs:87`). `PlatformWalletInfo` only implements the older surface, so `platform-wallet` fails to compile — which prevents reaching the `llmq_devnet_params` error above. The natural fix is to delegate to the inner `ManagedWalletInfo` (which already implements it at `:310`): `fn monitored_script_pubkeys(&self) -> Vec<ScriptBuf> { self.core_wallet.monitored_script_pubkeys() }`. Treating as a co-symptom of the same revision bump.
shumkov
changed the title
shumkov
changed the title
Wires the devnet identity through the example app so users can run the SPV client against a `dashd -devnet=<name>` peer set: - OptionsView: new "Devnet Name" text field under the existing Quorum URL field, backed by `platformDevnetName` UserDefaults. - CoreContentView.startSync: reads the saved name and passes it to `PlatformSpvStartConfig.devnetName` when the active network is devnet. Without this the FFI now rejects the start with "devnet_name is required when network=Devnet". Migrates `platform-wallet-ffi` and `platform-wallet` to the new `dash-spv::DevnetConfig` surface (PR #788 upstream): the legacy `ClientConfig::llmq_devnet_params` field is gone and the `WalletInfoInterface` trait grew a `monitored_script_pubkeys` method.
shumkov
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3761 +/- ##
============================================
+ Coverage 87.17% 87.33% +0.15%
============================================
Files 2607 2590 -17
Lines 319589 317082 -2507
============================================
- Hits 278602 276911 -1691
+ Misses 40987 40171 -816
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at head 5811761. Both prior blocking findings from the 056ab3e review are FIXED at this head: (1) platform-wallet-ffi now constructs DevnetConfig::new(name).with_llmq_params(...) and assigns it via config.devnet = Some(devnet) at spv.rs:319-328, replacing the removed ClientConfig::llmq_devnet_params; (2) PlatformWalletInfo now implements the required monitored_script_pubkeys by delegating to core_wallet at platform_wallet_traits.rs:100-102. The new latest-delta surface (Swift platformDevnetName UserDefaults → PlatformSpvStartConfig.devnetName → FFI devnet_name C string) is wired end-to-end and the FFI already enforces network/devnet pairing and matched llmq params. One in-scope hardening suggestion remains: the FFI accepts empty ("") and BIP14-reserved-character devnet names from C callers, which would produce a malformed (devnet.devnet-) user agent. The Swift example app filters empty strings to nil before calling, so this PR's immediate path is unaffected; the public C ABI is the chokepoint where future callers (other language bindings, integration tests) cannot pre-filter.
🟡 1 suggestion(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-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:239-256: FFI accepts empty/whitespace `devnet_name` and produces malformed devnet user agent
`platform_wallet_manager_spv_start` validates that `devnet_name` is non-null and that network/llmq pairing is consistent, but it does not reject an empty C string. A caller passing a valid pointer to `""` (or whitespace-only) enters the devnet branch, constructs `DevnetConfig::new("")`, and at line 279 rewrites the user agent to `/<base>(devnet.devnet-)/` — a substring Dash Core devnet peers will not accept, which is the exact failure mode this PR aims to eliminate. The Swift caller in `CoreContentView.startSync` happens to trim and convert empty to `nil` (CoreContentView.swift:591-596), so the example-app path is safe, but this C ABI is the only chokepoint for other consumers (future bindings, Rust integration tests, the `dash-spv` binary equivalent). Reject empty names at the same validation step that rejects null/wrong-network so the failure is synchronous on the FFI return rather than asynchronous after `spawn_in_background`. Optionally also reject BIP14-reserved characters (`/`, `(`, `)`) since they would similarly corrupt the user-agent string.
Without this guard the FFI accepts a `""` C string and constructs
`DevnetConfig::new("")`, which produces a `(devnet.devnet-)` user
agent that Dash Core devnet peers silently drop. The Swift example
caller already filters empties to `nil`, but the C ABI is the
chokepoint for future consumers without a pre-filter (other
language bindings, integration tests).
Mirrors `dash_spv::DevnetConfig::validate` so the rejection is
synchronous on the FFI return rather than asynchronous from
`spawn_in_background`.
|
✅ 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: "7929f177d536b65aca3ae09fb60760905b4c95e229c63ab18687fa31fe9d9476"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The prior FFI validation finding was only partially fixed at head 9036567. Empty-string devnet_name is now rejected, but the explicit whitespace-only half of the prior finding remains valid because the new check only uses name.is_empty() and does not trim before building the devnet user agent and DevnetConfig.
🟡 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/rs-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:264: FFI still accepts whitespace-only `devnet_name` values and produces a malformed devnet identity
This change only rejects `name.is_empty()` and `name.contains('/')`. A whitespace-only C string such as `" "` survives `CStr::to_str()?.to_string()`, is not empty, contains no slash, and therefore passes validation. It is then appended into the devnet user agent and passed to `DevnetConfig::new(name)`, so the prior finding's explicit whitespace-only case is still reproducible at the exported FFI surface even though the empty-string case is fixed. Trim before validating, or treat `name.trim().is_empty()` as invalid so whitespace-only input is rejected synchronously like the prior finding requested.
| // integration tests) from a `(devnet.devnet-)` user agent that | ||
| // Dash Core peers silently drop. | ||
| if let Some(name) = devnet_name_str.as_deref() { | ||
| if name.is_empty() { |
There was a problem hiding this comment.
🟡 Suggestion: FFI still accepts whitespace-only devnet_name values and produces a malformed devnet identity
This change only rejects name.is_empty() and name.contains('/'). A whitespace-only C string such as " " survives CStr::to_str()?.to_string(), is not empty, contains no slash, and therefore passes validation. It is then appended into the devnet user agent and passed to DevnetConfig::new(name), so the prior finding's explicit whitespace-only case is still reproducible at the exported FFI surface even though the empty-string case is fixed. Trim before validating, or treat name.trim().is_empty() as invalid so whitespace-only input is rejected synchronously like the prior finding requested.
source: unknown
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
2 participants
Issue being fixed or feature implemented
The example app could already create an SDK against a devnet, but
Start Syncwould either fail or connect to the wrong peers because the SPV start path never tolddash-spvit was running against a devnet. This PR enables SPV on devnet end-to-end.What was done?
Example app — devnet identity is now part of the SPV start config
OptionsView: new "Devnet Name" text field next to the existing Quorum URL field, backed byplatformDevnetNameUserDefaults.CoreContentView.startSync: whencurrentNetwork == .devnet, readsplatformDevnetNameand passes it toPlatformSpvStartConfig.devnetName. Without this the FFI rejects the start with "devnet_name is required when network=Devnet".FFI — migrated to the new
dash-spv::DevnetConfigsurfacepackages/rs-platform-wallet-ffi/src/spv.rs: replaced the removedClientConfig::llmq_devnet_paramsfield withClientConfig.devnet = Some(DevnetConfig::new(name).with_llmq_params(...)).packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs: implemented the newWalletInfoInterface::monitored_script_pubkeysrequired method by delegating to the innerManagedWalletInfo.packages/rs-platform-wallet/src/spv/mod.rs: re-exportedDevnetConfigfromdash_spvso the FFI layer can build it without depending ondash-spvdirectly.Dependency bump (the enabler)
rust-dashcorepin58d61ea5→eb889af1(current dev HEAD), picking up:eb889af1feat(dash-spv): consolidate devnet config intoDevnetConfigstruct (feat(dashmate): refactor reindex behaviour #788)9655ea38refactor: avoid rebuildingscript_pubkeys for BIP158 filter matching (feat(dashmate): pack release script #781)How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-ffiagainst the bumped revision is clean.platformQuorumURL+platformDevnetNamein Options on devnet, hit "Start Sync" on the Wallet tab, confirm SPV reaches the masternodes auto-discovered from{quorumURL}/masternodesand that headers / filters / masternodes progress all advance.Breaking Changes
None for downstream consumers. The Swift
PlatformSpvStartConfig.devnetNameparameter was already public (added in #3758); this PR just starts using it.Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Chores