feat(rs-sdk-ffi): expose SDK protocol version pinning#3734
Draft
thepastaclaw wants to merge 2 commits into
Draft
feat(rs-sdk-ffi): expose SDK protocol version pinning#3734thepastaclaw wants to merge 2 commits into
thepastaclaw wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds protocol-version-aware SDK creation across Rust FFI, Swift, and documentation. New C API constructors accept an explicit protocol version parameter; internal helpers validate and resolve protocol versions or default to auto-detect. Trusted SDK setup now enforces stricter DAPI defaults and error handling. Swift wrapper and docs are updated to demonstrate both pinned and auto-detect usage patterns. ChangesProtocol Version Pinning Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🤖 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/rs-sdk-ffi/src/sdk.rs`:
- Around line 341-349: The branch that checks config.core_sdk_handle only tests
for non-null but never uses it, so per-instance handles in DashSDKConfigExtended
are ignored; update the logic in dash_sdk_create_extended_with_protocol_version
to use the provided handle when non-null (e.g. obtain a context provider tied to
core_sdk_handle instead of unconditionally calling
CallbackContextProvider::from_global()), and pass that provider into
builder.with_context_provider; ensure you still fall back to from_global() only
when core_sdk_handle is null or when a handle-specific provider cannot be
created, producing the same error path via internal_error_result when
appropriate.
- Around line 139-163: The trusted setup logic (trusted_builder/trusted_setup)
currently re-parses dapi_addresses and rejects an empty string while
parse_dapi_addresses treats "" as None; change the trusted setup to call
parse_dapi_addresses(dapi_addresses) and branch on the resulting
Option<AddressList> (None = fallback to network defaults, Some(list) = use
provided addresses) instead of re-parsing the raw C string; apply the same
replacement at the other duplicate site that currently re-parses (the second
block that mirrors this logic) so both places treat an empty C string
consistently as “not provided.”
- Around line 165-176: The shared builder functions (e.g., base_builder and the
protocol-version constructors that call builder_with_protocol_version and
ultimately SdkBuilder::new or SdkBuilder::new_mock) only apply
network/dapi_addresses/protocol_version and ignore DashSDKConfig fields
request_retry_count, request_timeout_ms, and skip_asset_lock_proof_verification;
update these builders to propagate those config fields into the SdkBuilder
before returning (e.g., chain calls to the builder to set retry count, request
timeout, and enable/disable asset-lock-proof verification using the SdkBuilder
methods that control transport retry/timeout and verification flags) so the
values from DashSDKConfig are honored when build() is called.
🪄 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: e421833c-e05a-4604-9e8c-e2755527c8a3
📒 Files selected for processing (3)
packages/rs-sdk-ffi/README.mdpackages/rs-sdk-ffi/src/sdk.rspackages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
thepastaclaw
force-pushed
the
feature/ffi-sdk-protocol-version
branch
from
29648e9 to
65f0822
Compare
Collaborator
Author
|
Addressed the CodeRabbit findings in the PR branch: trusted setup now reuses the shared DAPI address parser (so empty strings fall back to defaults), shared FFI builders propagate retry/timeout/proof settings, and extended SDK creation binds global callbacks to the provided per-instance Core SDK handle when present.\n\nValidation:\n- cargo test -p rs-sdk-ffi --no-default-features --features mocks\n- cargo check -p rs-sdk-ffi |
Collaborator
Author
|
Addressed CodeRabbit finding 1 on the current PR branch.
Validation run:
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an additive API across the Rust FFI layer and Swift wrapper to allow callers to explicitly pin the Dash Platform protocol version used by SdkBuilder, while preserving existing constructors’ default auto-detect behavior (protocol_version = 0).
Changes:
- Added new
*_with_protocol_versionFFI constructors and refactored SDK creation to share common builder/config logic. - Updated Swift
SDKinitializer to accept an optionalprotocolVersionparameter (defaulting to0) and call the new trusted FFI constructor. - Updated rs-sdk-ffi README examples for C/Swift/Python to show protocol-version pinning usage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift | Adds protocolVersion parameter (default 0) and forwards it into the trusted FFI constructor. |
| packages/rs-sdk-ffi/src/sdk.rs | Introduces protocol-version-aware creation entrypoints, shared builder helpers, and adds tests around pinning/validation. |
| packages/rs-sdk-ffi/src/context_callbacks.rs | Adds support for overriding the global callback handle with a per-instance Core SDK handle, plus unit tests. |
| packages/rs-sdk-ffi/README.md | Updates language binding examples and API reference to include the new protocol-version pinning constructor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+179
to
+181
| builder | ||
| .with_settings(request_settings_from_config(config)) | ||
| .with_proofs(!config.skip_asset_lock_proof_verification) |
Comment on lines
+341
to
+347
| } else if !config.core_sdk_handle.is_null() { | ||
| let core_sdk_handle = unsafe { &*config.core_sdk_handle }; | ||
| if let Some(callback_provider) = | ||
| crate::context_callbacks::CallbackContextProvider::from_core_sdk_handle(core_sdk_handle) | ||
| .or_else(crate::context_callbacks::CallbackContextProvider::from_global) | ||
| { | ||
| builder = builder.with_context_provider(callback_provider); |
Comment on lines
+255
to
+260
| unsafe { set_global_callbacks(callbacks).expect("set callbacks") }; | ||
|
|
||
| let provider = CallbackContextProvider::from_core_sdk_handle(&CoreSDKHandle { | ||
| client: per_instance_handle, | ||
| }) | ||
| .expect("provider from callbacks"); |
Comment on lines
80
to
86
| DashSDKConfig config = { | ||
| .network = DASH_SDK_NETWORK_TESTNET, | ||
| .dapi_addresses = "seed-1.testnet.networks.dash.org", | ||
| .skip_asset_lock_proof_verification = false, | ||
| .request_retry_count = 3, | ||
| .request_timeout_ms = 30000 | ||
| }; |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rs-sdk-ffi/src/context_callbacks.rs`:
- Around line 245-269: This test mutates global callback state via
set_global_callbacks and doesn't restore it; wrap the mutation so
GLOBAL_CALLBACKS is restored after the test (e.g., capture the previous
callbacks before calling set_global_callbacks and restore them in a
drop/teardown or use a scoped helper) to avoid leaking state to other tests;
update the test around ContextProviderCallbacks / set_global_callbacks /
CallbackContextProvider::from_core_sdk_handle to save the prior
GLOBAL_CALLBACKS, set the test callbacks, run the assertions, and then restore
the saved callbacks in all paths (including on panic) so parallel tests remain
deterministic.
🪄 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: 4053e0f6-0c16-453a-8c68-aae168b65dc7
📒 Files selected for processing (4)
packages/rs-sdk-ffi/README.mdpackages/rs-sdk-ffi/src/context_callbacks.rspackages/rs-sdk-ffi/src/sdk.rspackages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
Comment on lines
+245
to
+269
| #[test] | ||
| fn from_core_sdk_handle_overrides_global_handle() { | ||
| let registered_handle = std::ptr::dangling_mut::<c_void>(); | ||
| let per_instance_handle = 0x1234usize as *mut c_void; | ||
| let callbacks = ContextProviderCallbacks { | ||
| core_handle: registered_handle, | ||
| get_platform_activation_height: get_height_cb, | ||
| get_quorum_public_key: get_quorum_pk_cb, | ||
| }; | ||
|
|
||
| unsafe { set_global_callbacks(callbacks).expect("set callbacks") }; | ||
|
|
||
| let provider = CallbackContextProvider::from_core_sdk_handle(&CoreSDKHandle { | ||
| client: per_instance_handle, | ||
| }) | ||
| .expect("provider from callbacks"); | ||
| let height = provider | ||
| .get_platform_activation_height() | ||
| .expect("activation height"); | ||
|
|
||
| assert_eq!(height, 42); | ||
| assert_eq!( | ||
| LAST_HEIGHT_HANDLE.load(Ordering::Relaxed), | ||
| per_instance_handle as usize | ||
| ); |
Contributor
There was a problem hiding this comment.
Isolate this test from global callback state.
set_global_callbacks(...) mutates GLOBAL_CALLBACKS for the whole process, and this test never restores it. Rust tests run in parallel by default, so this can leak state into other tests or race with any other test that touches callback registration.
🧪 One way to make the test deterministic
#[cfg(test)]
mod tests {
use super::*;
use drive_proof_verifier::ContextProvider;
- use std::sync::atomic::{AtomicUsize, Ordering};
+ use std::sync::{
+ atomic::{AtomicUsize, Ordering},
+ Mutex, OnceLock,
+ };
static LAST_HEIGHT_HANDLE: AtomicUsize = AtomicUsize::new(0);
+ static CALLBACK_TEST_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
@@
#[test]
fn from_core_sdk_handle_overrides_global_handle() {
+ let _guard = CALLBACK_TEST_LOCK
+ .get_or_init(|| Mutex::new(()))
+ .lock()
+ .expect("lock callback tests");
+ let previous = get_global_callbacks();
+
let registered_handle = std::ptr::dangling_mut::<c_void>();
let per_instance_handle = 0x1234usize as *mut c_void;
let callbacks = ContextProviderCallbacks {
core_handle: registered_handle,
get_platform_activation_height: get_height_cb,
@@
assert_eq!(
LAST_HEIGHT_HANDLE.load(Ordering::Relaxed),
per_instance_handle as usize
);
+
+ if let Some(previous) = previous {
+ unsafe { set_global_callbacks(previous).expect("restore callbacks") };
+ } else if let Some(storage) = GLOBAL_CALLBACKS.get() {
+ *storage.write().expect("clear callbacks") = None;
+ }
}
}🤖 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/rs-sdk-ffi/src/context_callbacks.rs` around lines 245 - 269, This
test mutates global callback state via set_global_callbacks and doesn't restore
it; wrap the mutation so GLOBAL_CALLBACKS is restored after the test (e.g.,
capture the previous callbacks before calling set_global_callbacks and restore
them in a drop/teardown or use a scoped helper) to avoid leaking state to other
tests; update the test around ContextProviderCallbacks / set_global_callbacks /
CallbackContextProvider::from_core_sdk_handle to save the prior
GLOBAL_CALLBACKS, set the test callbacks, run the assertions, and then restore
the saved callbacks in all paths (including on panic) so parallel tests remain
deterministic.
thepastaclaw
force-pushed
the
feature/ffi-sdk-protocol-version
branch
3 times, most recently
from
feca7c0 to
22365d9
Compare
Collaborator
Author
|
Addressed the latest CodeRabbit/Copilot review items in feca7c0:
Validation:
|
thepastaclaw
force-pushed
the
feature/ffi-sdk-protocol-version
branch
2 times, most recently
from
feca7c0 to
22365d9
Compare
lklimek
reviewed
May 25, 2026
| /// - The returned handle inside `DashSDKResult` must be destroyed using the SDK destroy function to avoid leaks. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn dash_sdk_create_extended_with_protocol_version( | ||
| config: *const DashSDKConfigExtended, |
Contributor
There was a problem hiding this comment.
Why not just add protocol_version to DashSDKConfigExtended ? New constructors won't be needed.
Comment on lines
+873
to
+877
| debug!("dash_sdk_create_trusted: testing basic HTTP connectivity"); | ||
| match reqwest::get("https://www.google.com").await { | ||
| Ok(_) => debug!("dash_sdk_create_trusted: basic HTTP test successful (Google)"), | ||
| Err(e) => warn!(error = %e, "dash_sdk_create_trusted: basic HTTP test failed"), | ||
| } |
Comment on lines
+879
to
+886
| // Try the quorums endpoint directly | ||
| debug!("dash_sdk_create_trusted: testing quorums endpoint directly"); | ||
| match reqwest::get("https://quorums.testnet.networks.dash.org/quorums").await { | ||
| Ok(resp) => debug!(status = %resp.status(), "dash_sdk_create_trusted: direct quorums endpoint test successful"), | ||
| Err(e) => warn!(error = %e, "dash_sdk_create_trusted: direct quorums endpoint test failed"), | ||
| } | ||
|
|
||
| // Now try through the provider |
Comment on lines
+708
to
+709
| /// - `config` must be a valid pointer to a DashSDKConfig structure | ||
| /// # Safety |
Comment on lines
+188
to
+216
| /// Create a new SDK instance with an explicit protocol version override | ||
| /// | ||
| /// `protocol_version == 0` preserves the default auto-detect behavior. | ||
| /// | ||
| /// # Safety | ||
| /// - `config` must be a valid pointer to a DashSDKConfig structure for the duration of the call. | ||
| /// - The returned handle inside `DashSDKResult` must be destroyed using the SDK destroy function to avoid leaks. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn dash_sdk_create_with_protocol_version( | ||
| config: *const DashSDKConfig, | ||
| protocol_version: u32, | ||
| ) -> DashSDKResult { | ||
| if config.is_null() { | ||
| return DashSDKResult::error(DashSDKError::new( | ||
| DashSDKErrorCode::InvalidParameter, | ||
| "Config is null".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| let config = &*config; | ||
|
|
||
| // Parse configuration | ||
| let network: Network = config.network.into(); | ||
|
|
||
| let platform_version = match resolve_platform_version(protocol_version) { | ||
| Ok(platform_version) => platform_version, | ||
| Err(result) => return result, | ||
| }; | ||
|
|
Comment on lines
+145
to
+147
| # Create SDK instance pinned to protocol version 11. | ||
| # Pass 0 to keep the default auto-detect behavior. | ||
| result = lib.dash_sdk_create_with_protocol_version(byref(config), 11) |
Collaborator
Author
|
Addressed the valid review points on this branch:
I did not move Validation run:
@coderabbitai review |
Contributor
|
✏️ Learnings added
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/rs-sdk-ffi/src/sdk.rs (2)
137-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread the rest of
DashSDKConfigthrough both builder helpers.These paths still only apply network/DAPI/protocol version.
skip_asset_lock_proof_verification,request_retry_count, andrequest_timeout_msare never consumed beforebuild(), so callers can set them and still get the default SDK behavior.#!/bin/bash set -euo pipefail echo "== DashSDKConfig field usage in sdk.rs and types.rs ==" rg -n -C1 'skip_asset_lock_proof_verification|request_retry_count|request_timeout_ms' \ packages/rs-sdk-ffi/src/sdk.rs \ packages/rs-sdk-ffi/src/types.rs echo echo "== Candidate SdkBuilder setters/call sites in Rust sources ==" rg -n -C2 'with_.*(retry|timeout)|asset.*lock.*proof|proof.*verification|skip_asset_lock' . --glob '*.rs'Also applies to: 303-357
🤖 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/rs-sdk-ffi/src/sdk.rs` around lines 137 - 145, The builder helpers (both SdkBuilder::new(address_list) and SdkBuilder::new_mock()) fail to apply DashSDKConfig fields skip_asset_lock_proof_verification, request_retry_count, and request_timeout_ms; update build_sdk_builder (and the other helper path around lines 303-357) to call the corresponding SdkBuilder setters after creating the builder and before returning: read config.skip_asset_lock_proof_verification and call the builder method that disables/enables proof verification (e.g., with_skip_asset_lock_proof_verification or equivalent), set request retry count via the builder method for retries (e.g., with_request_retry_count), and convert config.request_timeout_ms into the builder timeout setter (e.g., with_request_timeout_ms or with_request_timeout using Duration::from_millis) so both branches (SdkBuilder::new and SdkBuilder::new_mock) consistently consume these config values.
166-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
core_sdk_handleinstead of falling back to globals here.Once this branch sees a non-null handle, it still calls
CallbackContextProvider::from_global()and never reads the handle value. That makes the per-instance handle inDashSDKConfigExtendeda no-op and can bind the SDK to the wrong Core SDK instance or fail with the global-registration error even when a valid handle was passed.#!/bin/bash set -euo pipefail echo "== core_sdk_handle branch in packages/rs-sdk-ffi/src/sdk.rs ==" rg -n -C2 'core_sdk_handle|from_global\(' packages/rs-sdk-ffi/src/sdk.rs echo echo "== handle-specific provider helpers in packages/rs-sdk-ffi/src/context_callbacks.rs ==" rg -n -C2 'from_core_sdk_handle|from_global' packages/rs-sdk-ffi/src/context_callbacks.rs🤖 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/rs-sdk-ffi/src/sdk.rs` around lines 166 - 175, The branch that checks config.core_sdk_handle.is_null() incorrectly always calls CallbackContextProvider::from_global(), ignoring the per-instance handle; change it to construct the provider from the passed handle when config.core_sdk_handle is non-null (e.g. call crate::context_callbacks::CallbackContextProvider::from_core_sdk_handle(core_sdk_handle) or equivalent) and pass that provider into builder.with_context_provider(callback_provider); if from_core_sdk_handle returns None or an error, return the same DashSDKResult::error as the current global-failure path. Ensure you reference config.core_sdk_handle (or the local core_sdk_handle variable) and use CallbackContextProvider::from_core_sdk_handle instead of CallbackContextProvider::from_global().
🤖 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.
Duplicate comments:
In `@packages/rs-sdk-ffi/src/sdk.rs`:
- Around line 137-145: The builder helpers (both SdkBuilder::new(address_list)
and SdkBuilder::new_mock()) fail to apply DashSDKConfig fields
skip_asset_lock_proof_verification, request_retry_count, and request_timeout_ms;
update build_sdk_builder (and the other helper path around lines 303-357) to
call the corresponding SdkBuilder setters after creating the builder and before
returning: read config.skip_asset_lock_proof_verification and call the builder
method that disables/enables proof verification (e.g.,
with_skip_asset_lock_proof_verification or equivalent), set request retry count
via the builder method for retries (e.g., with_request_retry_count), and convert
config.request_timeout_ms into the builder timeout setter (e.g.,
with_request_timeout_ms or with_request_timeout using Duration::from_millis) so
both branches (SdkBuilder::new and SdkBuilder::new_mock) consistently consume
these config values.
- Around line 166-175: The branch that checks config.core_sdk_handle.is_null()
incorrectly always calls CallbackContextProvider::from_global(), ignoring the
per-instance handle; change it to construct the provider from the passed handle
when config.core_sdk_handle is non-null (e.g. call
crate::context_callbacks::CallbackContextProvider::from_core_sdk_handle(core_sdk_handle)
or equivalent) and pass that provider into
builder.with_context_provider(callback_provider); if from_core_sdk_handle
returns None or an error, return the same DashSDKResult::error as the current
global-failure path. Ensure you reference config.core_sdk_handle (or the local
core_sdk_handle variable) and use CallbackContextProvider::from_core_sdk_handle
instead of CallbackContextProvider::from_global().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88e7bbd9-96a3-4710-aadd-8a459c530616
📒 Files selected for processing (3)
packages/rs-sdk-ffi/README.mdpackages/rs-sdk-ffi/src/sdk.rspackages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
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
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.
Summary
Adds additive FFI and Swift SDK support for explicitly pinning the Dash
Platform protocol version used by
SdkBuilder.dash_sdk_*_with_protocol_versionconstructors for plain, extended,trusted, and callback-backed FFI SDK creation
protocol_version = 0auto-detect behavior
SDK(network:protocolVersion:), defaulting to0, so iOScallers can pass
11to pin Platform protocol version v11Validation
cargo fmt --all -- --checkcargo test -p rs-sdk-ffi sdk::tests --lib— 4 passedcargo checkcode-review dashpay/platform upstream/v3.1-dev feature/ffi-sdk-protocol-version— Recommendation: ship
functions
Summary by CodeRabbit
New Features
skip_asset_lock_proof_verificationconfiguration option.Documentation