feat(rs-sdk-ffi): expose optional platform_version in DashSDKConfig#3751
Conversation
Adds a `platform_version: u32` field to `DashSDKConfig` so callers of `dash_sdk_create*` can pin the SDK to a specific `PlatformVersion`. `0` keeps the SDK default (auto-detect / latest); any non-zero value is forwarded to `SdkBuilder::with_version` and rejected via `InvalidParameter` if the version is unknown. The Swift wrapper forwards a new optional `platformVersion: UInt32 = 0` parameter on `SDK.init(network:)` into the config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 (7)
📝 WalkthroughWalkthroughThis PR adds platform version configuration support to the Dash SDK FFI layer and Swift SDK. A new ChangesPlatform Version Support
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR exposes a platform_version option through the Rust FFI SDK config and Swift SDK initializer so callers can optionally pin the SDK to a specific Dash Platform protocol version.
Changes:
- Adds
platform_versiontoDashSDKConfigand applies it viaSdkBuilder::with_version. - Propagates the field through standard, extended, trusted, and callback SDK creation paths.
- Adds a defaulted Swift initializer parameter and updates Rust test struct literals.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/rs-sdk-ffi/src/types.rs |
Adds the new FFI config field. |
packages/rs-sdk-ffi/src/sdk.rs |
Applies the configured platform version during SDK creation. |
packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift |
Exposes platformVersion in Swift and marshals it into FFI config. |
packages/rs-sdk-ffi/tests/context_provider_test.rs |
Updates config literal with default version. |
packages/rs-sdk-ffi/src/token/claim.rs |
Updates test config literal with default version. |
packages/rs-sdk-ffi/src/token/emergency_action.rs |
Updates test config literal with default version. |
packages/rs-sdk-ffi/src/token/freeze.rs |
Updates test config literal with default version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3751 +/- ##
============================================
+ Coverage 87.17% 87.32% +0.15%
============================================
Files 2607 2590 -17
Lines 319489 316982 -2507
============================================
- Hits 278507 276815 -1692
+ Misses 40982 40167 -815
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, scope-correct FFI change adding an optional platform_version: u32 to DashSDKConfig, threaded through all four dash_sdk_create* entry points and the Swift wrapper. All findings verified against the worktree at e013336. The repeated 'ABI break' concern is technically accurate (appending to a #[repr(C)] struct is source-compatible, not binary-compatible) but does not warrant blocking in this context: rs-sdk-ffi is a pre-1.0 internal SDK (workspace at 3.1.0-dev.6), the README examples already diverge from the current struct (they omit skip_asset_lock_proof_verification), and the cbindgen header ships with the library, so there is no committed binary-ABI surface to preserve. Downgraded to a suggestion to soften the 'preserves ABI' wording and document the rebuild requirement.
🟡 2 suggestion(s) | 💬 2 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/src/types.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/types.rs:64-84: Reword the "preserves ABI" claim — appending to a repr(C) struct is source-compatible only
The PR description and reasoning around appending `platform_version` describe the change as ABI-preserving. That is true at the source level (recompiling against the regenerated cbindgen header works), but it is not true for binary compatibility: `DashSDKConfig` is a `#[repr(C)]` struct passed by pointer, and the four entry points now unconditionally read `config.platform_version` (sdk.rs:165, sdk.rs:278, sdk.rs:446, sdk.rs:603). A consumer compiled against the previous header would still allocate the shorter layout, and the new library would read 4 bytes past the caller's allocation — UB resulting in either a crash or an unintended protocol-version pin.
In this repo's context the concrete risk is low: rs-sdk-ffi ships at 3.1.0-dev.6 with no committed binary-ABI promise, the cbindgen header is regenerated and shipped with the library, the Swift bridge is updated in this PR, and the README examples themselves already diverge from the struct layout (the C/Swift/Python snippets omit `skip_asset_lock_proof_verification`). Three independent Codex reviewers nevertheless flagged this as blocking, which is a fair read of the technical statement in the PR description.
Fix is documentation-only: in the PR description (and optionally the doc comment on `DashSDKConfig`), replace the "preserves ABI" wording with something like "source-compatible — consumers must rebuild against the regenerated FFI header." If long-term binary ABI matters, track the broader `struct_size`/`struct_version` discriminator design as a separate follow-up (see out-of-scope items).
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/sdk.rs:28-39: Add coverage for the new invalid-platform-version error path
`apply_version` introduces a new fallible branch: `0` short-circuits to the SDK default, a known version is forwarded to `SdkBuilder::with_version`, and an unknown version returns `DashSDKError { code: InvalidParameter, ... }`. None of the test updates in this PR exercise that — every modified test fixture (`token/{claim,emergency_action,freeze}.rs`, `tests/context_provider_test.rs`) sets `platform_version: 0`, which short-circuits before the lookup.
A small `#[cfg(test)]` block in `sdk.rs` calling `dash_sdk_create` (or `apply_version` directly) with `u32::MAX` and asserting `DashSDKErrorCode::InvalidParameter` would lock in the contract documented on the new field. The PR checklist already notes tests were not added, so this is a known gap and easy to close.
Out-of-scope follow-up suggestions (4)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- request_retry_count and request_timeout_ms appear declared but unused — While auditing
DashSDKConfig, the existingrequest_retry_countandrequest_timeout_msfields are declared on the struct but do not appear to be consumed insdk.rs— they are only ever written into the struct by callers. The newplatform_versionfield follows the same shape, so it is not a regression introduced by this PR, but the existing fields should be either wired intoSdkBuilderor documented as no-ops.- Follow-up: Create a separate issue to either wire
request_retry_count/request_timeout_msintoSdkBuilderor document on the struct that they are reserved for future use.
- Follow-up: Create a separate issue to either wire
- FFI input structs lack a struct_size/struct_version discriminator — Each time a field is appended to
#[repr(C)] DashSDKConfig(this PR addsplatform_version), every pre-built consumer must rebuild against the regenerated cbindgen header — there is no way for Rust to detect a stale caller. A leadingstruct_size: usizeorstruct_version: u32discriminator on public FFI input structs would let the entry points return a cleanInvalidParameterinstead of reading past a stale caller's allocation. Out of scope for this PR since it is its own design discussion.- Follow-up: Create a separate maintainer-requested PR introducing a struct-size/ABI-version discriminator on public FFI input structs (
DashSDKConfig,DashSDKConfigExtended) before more optional fields are appended.
- Follow-up: Create a separate maintainer-requested PR introducing a struct-size/ABI-version discriminator on public FFI input structs (
- extern "C" entry points lack catch_unwind guards — All
dash_sdk_create*entry points allocateStrings and call into Rust SDK code that can in principle panic (allocator OOM, internal invariants). Withoutstd::panic::catch_unwind, a panic would unwind across the C ABI which is UB on stable Rust. This predates this PR and is a broader hardening pass rather than something to bolt onto a small feature change.- Follow-up: Create a separate issue to wrap each rs-sdk-ffi
extern "C"entry point incatch_unwind, mapping caught panics toDashSDKErrorCode::InternalError.
- Follow-up: Create a separate issue to wrap each rs-sdk-ffi
- FFI still cannot express SdkBuilder::with_initial_version —
DashSDKConfignow exposes hard pinning viawith_version, but it still cannot seedwith_initial_versionfor older-network interop with auto-detect enabled. This is already called out by the existing TODO atpackages/rs-sdk-ffi/src/types.rs:59and predates this PR.- Follow-up: Create a separate follow-up PR to expose
with_initial_versionthrough the FFI once the core SDK auto-detect behavior (CMT-005) is settled.
- Follow-up: Create a separate follow-up PR to expose
|
✅ 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: "b428ce7d653e9e85fea39938b14ca711e69726259df0e8aeabb2f2077d8cea0a"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at 4d3a565 — the delta from prior review e013336 is purely a merge of v3.1-dev (#3671, address funding from asset-lock proofs) and does not touch rs-sdk-ffi or the Swift bridge. All four prior findings reference unchanged lines and are carried forward as STILL VALID. No new findings introduced by the merge.
🟡 2 suggestion(s) | 💬 2 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/src/types.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/types.rs:64-84: Reword the "preserves ABI" claim — appending to a repr(C) struct is source-compatible only
STILL VALID at 4d3a5654 (file unchanged in the latest delta). The current doc comment on `DashSDKConfig` (types.rs:64-84) does not itself make the ABI-preserving claim, but the PR description framing does. Appending `platform_version` to a `#[repr(C)]` struct passed by pointer is source-compatible (recompiling against the regenerated cbindgen header works), not binary-compatible: all four entry points unconditionally read `config.platform_version` (sdk.rs:165, sdk.rs:278, sdk.rs:446, sdk.rs:603), so a caller compiled against the previous header would allocate the shorter layout and the new library would read 4 bytes past the caller's allocation — UB resulting in a crash or unintended protocol-version pin.
Concrete risk is low: rs-sdk-ffi ships at 3.1.0-dev.6 with no committed binary-ABI promise, the cbindgen header is regenerated and shipped with the library, the Swift bridge is updated in lockstep, and the README examples already diverge from the struct layout (omitting `skip_asset_lock_proof_verification`). Fix is documentation-only — in the PR description (and optionally on the `DashSDKConfig` doc comment), replace the "preserves ABI" wording with "source-compatible — consumers must rebuild against the regenerated FFI header." Broader `struct_size`/`struct_version` discriminator design is tracked as out-of-scope.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/sdk.rs:28-39: Add coverage for the new invalid-platform-version error path
STILL VALID at 4d3a5654 (apply_version unchanged; no new tests in the latest delta). `apply_version` has three branches: `0` short-circuits to the SDK default, a known version is forwarded to `SdkBuilder::with_version`, and an unknown version returns `DashSDKError { code: InvalidParameter, ... }`. Every modified test fixture (`token/{claim,emergency_action,freeze}.rs`, `tests/context_provider_test.rs`) sets `platform_version: 0`, which short-circuits before the lookup — the InvalidParameter branch is unexercised.
A small `#[cfg(test)]` test in `sdk.rs` calling `apply_version` directly with `u32::MAX` (or `dash_sdk_create` with that value) and asserting `DashSDKErrorCode::InvalidParameter` would lock in the contract documented on the new field. The PR checklist already notes tests were not added, so this is a known gap.
Out-of-scope follow-up suggestions (4)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- FFI input structs lack a struct_size/struct_version discriminator — Each time a field is appended to
#[repr(C)] DashSDKConfig(this PR addsplatform_version), every pre-built consumer must rebuild against the regenerated cbindgen header — there is no runtime way for Rust to detect a stale caller. A leadingstruct_size: usizeorstruct_version: u32discriminator on public FFI input structs would let entry points return a cleanInvalidParameterinstead of reading past a stale caller's allocation. Out of scope as a broader FFI design discussion touching bothDashSDKConfigandDashSDKConfigExtended.- Follow-up: Create a separate maintainer-requested PR introducing a struct-size/ABI-version discriminator on public FFI input structs (
DashSDKConfig,DashSDKConfigExtended) before more optional fields are appended.
- Follow-up: Create a separate maintainer-requested PR introducing a struct-size/ABI-version discriminator on public FFI input structs (
- extern "C" entry points lack catch_unwind guards — All
dash_sdk_create*entry points allocateStrings and call into Rust SDK code that can in principle panic (allocator OOM, internal invariants). Withoutstd::panic::catch_unwind, a panic would unwind across the C ABI which is UB on stable Rust. This predates this PR and is a broader hardening pass.- Follow-up: Create a separate issue to wrap each rs-sdk-ffi
extern "C"entry point incatch_unwind, mapping caught panics toDashSDKErrorCode::InternalError.
- Follow-up: Create a separate issue to wrap each rs-sdk-ffi
- request_retry_count and request_timeout_ms appear declared but unused — While auditing
DashSDKConfig, the existingrequest_retry_countandrequest_timeout_msfields are written into the struct by callers but do not appear to be consumed insdk.rs. The newplatform_versionfield follows the same shape and is actually wired through, so this is not a regression introduced by this PR.- Follow-up: Create a separate issue to either wire
request_retry_count/request_timeout_msintoSdkBuilderor document on the struct that they are reserved for future use.
- Follow-up: Create a separate issue to either wire
- FFI still cannot express SdkBuilder::with_initial_version —
DashSDKConfignow exposes hard pinning viawith_version, but it still cannot seedwith_initial_versionfor older-network interop with auto-detect enabled. This is already called out by the existing TODO atpackages/rs-sdk-ffi/src/types.rs:59-63(CMT-007 / #3711) and predates this PR.- Follow-up: Track follow-up under existing CMT-007 / #3711 once core SDK auto-detect behavior (CMT-005) is settled.
| pub request_retry_count: u32, | ||
| /// Timeout for requests in milliseconds | ||
| pub request_timeout_ms: u64, | ||
| /// Pin to a specific Dash Platform protocol version. | ||
| /// `0` keeps the SDK default (auto-detect / latest); any non-zero value | ||
| /// is forwarded to `SdkBuilder::with_version` and rejected if unknown. | ||
| pub platform_version: u32, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Reword the "preserves ABI" claim — appending to a repr(C) struct is source-compatible only
STILL VALID at 4d3a565 (file unchanged in the latest delta). The current doc comment on DashSDKConfig (types.rs:64-84) does not itself make the ABI-preserving claim, but the PR description framing does. Appending platform_version to a #[repr(C)] struct passed by pointer is source-compatible (recompiling against the regenerated cbindgen header works), not binary-compatible: all four entry points unconditionally read config.platform_version (sdk.rs:165, sdk.rs:278, sdk.rs:446, sdk.rs:603), so a caller compiled against the previous header would allocate the shorter layout and the new library would read 4 bytes past the caller's allocation — UB resulting in a crash or unintended protocol-version pin.
Concrete risk is low: rs-sdk-ffi ships at 3.1.0-dev.6 with no committed binary-ABI promise, the cbindgen header is regenerated and shipped with the library, the Swift bridge is updated in lockstep, and the README examples already diverge from the struct layout (omitting skip_asset_lock_proof_verification). Fix is documentation-only — in the PR description (and optionally on the DashSDKConfig doc comment), replace the "preserves ABI" wording with "source-compatible — consumers must rebuild against the regenerated FFI header." Broader struct_size/struct_version discriminator design is tracked as out-of-scope.
source: ['claude', 'codex']
| fn apply_version(builder: SdkBuilder, platform_version: u32) -> Result<SdkBuilder, DashSDKError> { | ||
| if platform_version == 0 { | ||
| return Ok(builder); | ||
| } | ||
| match dash_sdk::dpp::version::PlatformVersion::get(platform_version) { | ||
| Ok(v) => Ok(builder.with_version(v)), | ||
| Err(e) => Err(DashSDKError::new( | ||
| DashSDKErrorCode::InvalidParameter, | ||
| format!("Invalid platform_version {}: {}", platform_version, e), | ||
| )), | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add coverage for the new invalid-platform-version error path
STILL VALID at 4d3a565 (apply_version unchanged; no new tests in the latest delta). apply_version has three branches: 0 short-circuits to the SDK default, a known version is forwarded to SdkBuilder::with_version, and an unknown version returns DashSDKError { code: InvalidParameter, ... }. Every modified test fixture (token/{claim,emergency_action,freeze}.rs, tests/context_provider_test.rs) sets platform_version: 0, which short-circuits before the lookup — the InvalidParameter branch is unexercised.
A small #[cfg(test)] test in sdk.rs calling apply_version directly with u32::MAX (or dash_sdk_create with that value) and asserting DashSDKErrorCode::InvalidParameter would lock in the contract documented on the new field. The PR checklist already notes tests were not added, so this is a known gap.
source: ['claude', 'codex']
| Err(e) => Err(DashSDKError::new( | ||
| DashSDKErrorCode::InvalidParameter, | ||
| format!("Invalid platform_version {}: {}", platform_version, e), | ||
| )), | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Redundant version number in error message
STILL VALID at 4d3a565 — sdk.rs:36 still reads format!("Invalid platform_version {}: {}", platform_version, e). PlatformVersionError::UnknownVersionError already formats as "no platform version {version}" (rs-platform-version/src/version/protocol_version.rs:92), so the FFI error reads "Invalid platform_version 99: no platform version 99". Either drop the inner error or drop the Invalid platform_version {n} prefix and propagate the inner string.
| Err(e) => Err(DashSDKError::new( | |
| DashSDKErrorCode::InvalidParameter, | |
| format!("Invalid platform_version {}: {}", platform_version, e), | |
| )), | |
| } | |
| Err(e) => Err(DashSDKError::new( | |
| DashSDKErrorCode::InvalidParameter, | |
| format!("Invalid platform_version: {}", e), | |
| )), |
source: ['claude', 'codex']
Issue being fixed or feature implemented
Callers of `dash_sdk_create*` had no way to pin the SDK to a specific Dash Platform protocol version. The Rust `SdkBuilder` already supports it via `SdkBuilder::with_version(&'static PlatformVersion)`; this PR forwards that knob across the FFI and into the Swift SDK.
What was done?
Diff stat: 7 files changed, +39 / -1.
How Has This Been Tested?
Breaking Changes
None. `platform_version: 0` is the sentinel for "leave the SDK at its default," so existing C/Swift callers that zero-initialise `DashSDKConfig` keep the prior behaviour. The Swift initializer parameter is defaulted, so existing call sites compile unchanged.
Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit