feat(rs-sdk-ffi): add shielded pool FFI bindings with BLAST sync and transitions#3239
Conversation
Add C-compatible FFI bindings for querying the shielded pool state, including pool balance, encrypted notes, anchors, most recent anchor, and nullifier statuses. These enable iOS/Swift clients to interact with the shielded pool through the unified SDK. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR introduces comprehensive FFI (Foreign Function Interface) bindings for Dash shielded pool operations, including nullifier synchronization and shielded state transitions. It adds C-compatible types, unsafe extern "C" functions, and module infrastructure enabling non-Rust systems to interact with shielded pool functionality. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
✅ 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: "a954907049f3f9249404eb53b1355ae550a9fb4edd43ba0342026a8c0d8f707d"
)Xcode manual integration:
|
…I bindings Add nullifier BLAST sync (privacy-preserving trunk/branch queries) and FFI bindings for all 5 shielded state transitions: - Shield (platform address → shielded pool) - ShieldFromAssetLock (L1 asset lock → shielded pool, instant + chain) - ShieldedTransfer (shielded → shielded) - Unshield (shielded pool → platform address) - ShieldedWithdrawal (shielded pool → L1 Core address) Shared OrchardBundleParams FFI types allow the iOS client to construct Orchard bundles independently and pass pre-built bytes through FFI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…itions Add builder functions that construct and serialize shielded state transitions without broadcasting, allowing the iOS client to build, inspect/store, and later broadcast transitions separately: - dash_sdk_shielded_build_transfer - dash_sdk_shielded_build_unshield - dash_sdk_shielded_build_shield - dash_sdk_shielded_build_shield_from_instant_lock - dash_sdk_shielded_build_shield_from_chain_lock - dash_sdk_shielded_build_withdrawal - dash_sdk_shielded_broadcast (generic broadcast from serialized bytes) Builders return hex-encoded serialized StateTransition bytes. The broadcast function deserializes and broadcasts pre-built transitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…in builders Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/rs-sdk-ffi/src/shielded/queries/anchors.rs (1)
32-49: Reusewrapper.runtimefor query bindings.Query bindings allocate a fresh multithread Tokio runtime per call, while transition bindings reuse
wrapper.runtime. This creates unnecessary thread churn on mobile and introduces inconsistent patterns across the FFI layer. The same issue applies to all shielded query files:pool_state.rs,nullifiers.rs,most_recent_anchor.rs, andencrypted_notes.rs.Suggested change
- let sdk = wrapper.sdk.clone(); - - let rt = match tokio::runtime::Runtime::new() { - Ok(rt) => rt, - Err(e) => { - return DashSDKResult::error(DashSDKError::new( - DashSDKErrorCode::InternalError, - format!("Failed to create Tokio runtime: {}", e), - )); - } - }; - - let result = rt.block_on(async move { - ShieldedAnchors::fetch_current(&sdk) + let result = wrapper.runtime.block_on(async { + ShieldedAnchors::fetch_current(&wrapper.sdk) .await .map_err(|e| format!("Failed to fetch shielded anchors: {}", e)) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/queries/anchors.rs` around lines 32 - 49, The code creates a new Tokio runtime per call (using tokio::runtime::Runtime::new and rt.block_on) which causes thread churn; instead reuse the existing runtime stored on the SDK wrapper. Update the function to use the SDKWrapper's runtime (access wrapper.runtime) to spawn or block_on the async task for ShieldedAnchors::fetch_current(&sdk) rather than creating a new Runtime; replace the Runtime::new/rt.block_on usage with the wrapper.runtime handle and ensure the same change is applied to the other shielded query modules (pool_state.rs, nullifiers.rs, most_recent_anchor.rs, encrypted_notes.rs).packages/rs-sdk-ffi/src/shielded/transitions/shield.rs (1)
12-12: Use the SDK's configured network instead of hardcoding Testnet.
PrivateKey::new()is hardcoded toNetwork::Testnet, butwrapper.sdk.networkis available and should be used instead for consistency with the SDK's configuration.Suggested change
-use dash_sdk::dpp::dashcore::{Network, PrivateKey}; +use dash_sdk::dpp::dashcore::PrivateKey; @@ - let private_key = PrivateKey::new(secret_key, Network::Testnet); + let private_key = PrivateKey::new(secret_key, wrapper.sdk.network);Also applies to: 129-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/transitions/shield.rs` at line 12, The code currently constructs PrivateKey instances using Network::Testnet; update those calls to use the SDK-configured network instead by replacing Network::Testnet with wrapper.sdk.network (or otherwise obtain the network from wrapper.sdk) wherever PrivateKey::new(...) is invoked (e.g., the call sites around PrivateKey::new() at top-level and the occurrences around lines 129-130 in shield.rs) so the private keys are created using the configured SDK network rather than a hardcoded Testnet.packages/rs-sdk-ffi/src/shielded/transitions/shield_from_asset_lock.rs (1)
71-71: Private key length is assumed to be exactly 32 bytes without validation.The safety documentation states the requirement, but there's no runtime check. This is acceptable for FFI where the caller is responsible for meeting documented preconditions. However, consider adding a
private_key_lenparameter for defense-in-depth, similar to howoutput_address_lenis handled inunshield.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/transitions/shield_from_asset_lock.rs` at line 71, The code currently assumes the FFI private_key pointer is 32 bytes by using std::slice::from_raw_parts(private_key, 32) without validating length; add a new parameter private_key_len to the shield_from_asset_lock FFI function and validate that private_key_len == 32 before calling std::slice::from_raw_parts, returning an error (or early failure code) if it is not, mirroring the output_address_len pattern used in unshield.rs; reference the private_key pointer, the new private_key_len parameter, and the pk_bytes slice when adding the runtime check.packages/rs-sdk-ffi/src/shielded/queries/nullifiers.rs (1)
59-67: Consider reusing the wrapper's runtime instead of creating a new one per call.The transition functions (e.g.,
shielded_transfer.rs) usewrapper.runtime.block_on()directly, avoiding runtime creation overhead. The query functions create a new runtime per call, which is more expensive. This is a consistency and performance consideration.♻️ Proposed refactor to reuse wrapper runtime
- let rt = match tokio::runtime::Runtime::new() { - Ok(rt) => rt, - Err(e) => { - return DashSDKResult::error(DashSDKError::new( - DashSDKErrorCode::InternalError, - format!("Failed to create Tokio runtime: {}", e), - )); - } - }; - let query = ShieldedNullifiersQuery(nullifiers); - let result = rt.block_on(async move { + let result = wrapper.runtime.block_on(async { ShieldedNullifierStatuses::fetch(&sdk, query) .await .map_err(|e| format!("Failed to fetch nullifier statuses: {}", e)) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/queries/nullifiers.rs` around lines 59 - 67, The code creates a new Tokio runtime via tokio::runtime::Runtime::new() for each query; switch to reusing the existing wrapper runtime instead—replace the Runtime::new() match and subsequent rt.block_on(...) usage with wrapper.runtime.block_on(...) (same pattern used in shielded_transfer.rs), remove the DashSDKResult::error branch tied to runtime creation, and ensure the function uses the wrapper instance in scope to execute the async call so you avoid per-call runtime allocation.packages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rs (2)
67-70: Consider whether "no anchor found" should return an error or an empty result.Returning
InternalErrorwhenOk(None)may not be ideal if a fresh/empty shielded pool is a valid state. However, if an anchor is always expected to exist after pool initialization, this behavior is correct. Consider using a more specific error code likeNotFoundif available, or documenting this expectation in the function's doc comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rs` around lines 67 - 70, The current Ok(None) arm returns DashSDKResult::error with DashSDKErrorCode::InternalError which treats an empty shielded pool as an internal failure; decide whether an empty pool is a valid result or truly an error — if valid, change the Ok(None) branch to return a successful empty result (e.g., DashSDKResult::ok(None)) so callers can handle "no anchor" as absence, otherwise replace DashSDKErrorCode::InternalError with a more specific error like DashSDKErrorCode::NotFound and update the function's doc comment to state the expected invariant; locate the match handling Ok(None) in most_recent_anchor.rs and adjust the DashSDKResult/DashSDKError usage accordingly.
35-43: Same runtime creation pattern as other query files.Per the earlier comment on
nullifiers.rs, consider reusingwrapper.runtimefor consistency with transition functions and to avoid per-call runtime creation overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rs` around lines 35 - 43, The code creates a new Tokio runtime in most_recent_anchor.rs which duplicates work and differs from other query files; instead reuse the existing wrapper.runtime used by the transition functions: remove the tokio::runtime::Runtime::new() match and use wrapper.runtime to execute async work (e.g., call wrapper.runtime.block_on or otherwise run the future on wrapper.runtime) so the function uses the shared runtime instance; adjust any variable/name (rt) usage to reference wrapper.runtime consistently and keep the DashSDKResult/DashSDKError return behavior unchanged.packages/rs-sdk-ffi/src/shielded/transitions/mod.rs (1)
1-15: Consider re-exportingDashSDKShieldInputfor consistency.The parent module (
shielded/mod.rsline 17) accessesDashSDKShieldInputviatransitions::shield::DashSDKShieldInput, reaching directly into the submodule. For consistency with how functions are re-exported here, consider adding a re-export for the type as well.♻️ Optional: Re-export DashSDKShieldInput
pub use shield::dash_sdk_shielded_shield_funds; +pub use shield::DashSDKShieldInput; pub use shield_from_asset_lock::{ dash_sdk_shielded_shield_from_chain_lock, dash_sdk_shielded_shield_from_instant_lock, };Then in
shielded/mod.rs, simplify to:-pub use transitions::shield::DashSDKShieldInput; +pub use transitions::DashSDKShieldInput;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/transitions/mod.rs` around lines 1 - 15, The module currently re-exports functions from its submodules but not the DashSDKShieldInput type; add a public re-export for DashSDKShieldInput from the shield submodule (e.g., pub use shield::DashSDKShieldInput) so callers can access the type as transitions::DashSDKShieldInput instead of reaching into transitions::shield::DashSDKShieldInput; update the exports block in this file to include that new re-export alongside the existing dash_sdk_* function exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk-ffi/src/nullifier_sync/mod.rs`:
- Around line 97-105: The current code builds a NullifierSyncCheckpoint only
when both last_sync_height and last_sync_timestamp are non-zero, but the docs
require treating any zero field as "no previous sync" (full scan); update both
entrypoints that construct last_sync (the blocks using last_sync_height,
last_sync_timestamp and creating NullifierSyncCheckpoint) to return None when
either last_sync_height == 0 OR last_sync_timestamp == 0 (i.e., use a condition
like if last_sync_height == 0 || last_sync_timestamp == 0 { None } else {
Some(NullifierSyncCheckpoint { height: last_sync_height, timestamp:
last_sync_timestamp }) }) so sync_nullifiers receives None for any zeroed
checkpoint field.
In `@packages/rs-sdk-ffi/src/shielded/queries/nullifiers.rs`:
- Around line 48-57: Compute total_bytes using checked multiplication to avoid
overflow: cast nullifiers_count to usize and call checked_mul(32) (e.g., let
total_bytes = (nullifiers_count as usize).checked_mul(32).ok_or_else(|| /*
return or panic with clear message */)?;), then use that validated total_bytes
for from_raw_parts; if checked_mul returns None, return an error or panic
instead of proceeding. Ensure you still validate nullifiers_ptr is non-null
before calling std::slice::from_raw_parts and preserve subsequent logic that
builds raw_bytes and nullifiers from chunks_exact.
In `@packages/rs-sdk-ffi/src/shielded/transitions/shield.rs`:
- Around line 88-141: The code currently builds input_map as a BTreeMap which
reorders and can drop duplicates causing DeductFromInput(fee_from_input_index)
to reference the wrong input; replace the BTreeMap input_map with an ordered Vec
(e.g., input_vec: Vec<(PlatformAddress, Credits)>) built by iterating
inputs_slice in-order (use PlatformAddress::from_bytes, SecretKey::from_slice,
PrivateKey::new, signer.add_key as before) and either reject duplicate addresses
up-front or detect and error when inserting a duplicate; when creating the fee
strategy and any subsequent lookup that expects the caller order, index into
this ordered Vec (not a sorted map) so fee_from_input_index applies to the
original inputs order.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/shielded/queries/anchors.rs`:
- Around line 32-49: The code creates a new Tokio runtime per call (using
tokio::runtime::Runtime::new and rt.block_on) which causes thread churn; instead
reuse the existing runtime stored on the SDK wrapper. Update the function to use
the SDKWrapper's runtime (access wrapper.runtime) to spawn or block_on the async
task for ShieldedAnchors::fetch_current(&sdk) rather than creating a new
Runtime; replace the Runtime::new/rt.block_on usage with the wrapper.runtime
handle and ensure the same change is applied to the other shielded query modules
(pool_state.rs, nullifiers.rs, most_recent_anchor.rs, encrypted_notes.rs).
In `@packages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rs`:
- Around line 67-70: The current Ok(None) arm returns DashSDKResult::error with
DashSDKErrorCode::InternalError which treats an empty shielded pool as an
internal failure; decide whether an empty pool is a valid result or truly an
error — if valid, change the Ok(None) branch to return a successful empty result
(e.g., DashSDKResult::ok(None)) so callers can handle "no anchor" as absence,
otherwise replace DashSDKErrorCode::InternalError with a more specific error
like DashSDKErrorCode::NotFound and update the function's doc comment to state
the expected invariant; locate the match handling Ok(None) in
most_recent_anchor.rs and adjust the DashSDKResult/DashSDKError usage
accordingly.
- Around line 35-43: The code creates a new Tokio runtime in
most_recent_anchor.rs which duplicates work and differs from other query files;
instead reuse the existing wrapper.runtime used by the transition functions:
remove the tokio::runtime::Runtime::new() match and use wrapper.runtime to
execute async work (e.g., call wrapper.runtime.block_on or otherwise run the
future on wrapper.runtime) so the function uses the shared runtime instance;
adjust any variable/name (rt) usage to reference wrapper.runtime consistently
and keep the DashSDKResult/DashSDKError return behavior unchanged.
In `@packages/rs-sdk-ffi/src/shielded/queries/nullifiers.rs`:
- Around line 59-67: The code creates a new Tokio runtime via
tokio::runtime::Runtime::new() for each query; switch to reusing the existing
wrapper runtime instead—replace the Runtime::new() match and subsequent
rt.block_on(...) usage with wrapper.runtime.block_on(...) (same pattern used in
shielded_transfer.rs), remove the DashSDKResult::error branch tied to runtime
creation, and ensure the function uses the wrapper instance in scope to execute
the async call so you avoid per-call runtime allocation.
In `@packages/rs-sdk-ffi/src/shielded/transitions/mod.rs`:
- Around line 1-15: The module currently re-exports functions from its
submodules but not the DashSDKShieldInput type; add a public re-export for
DashSDKShieldInput from the shield submodule (e.g., pub use
shield::DashSDKShieldInput) so callers can access the type as
transitions::DashSDKShieldInput instead of reaching into
transitions::shield::DashSDKShieldInput; update the exports block in this file
to include that new re-export alongside the existing dash_sdk_* function
exports.
In `@packages/rs-sdk-ffi/src/shielded/transitions/shield_from_asset_lock.rs`:
- Line 71: The code currently assumes the FFI private_key pointer is 32 bytes by
using std::slice::from_raw_parts(private_key, 32) without validating length; add
a new parameter private_key_len to the shield_from_asset_lock FFI function and
validate that private_key_len == 32 before calling std::slice::from_raw_parts,
returning an error (or early failure code) if it is not, mirroring the
output_address_len pattern used in unshield.rs; reference the private_key
pointer, the new private_key_len parameter, and the pk_bytes slice when adding
the runtime check.
In `@packages/rs-sdk-ffi/src/shielded/transitions/shield.rs`:
- Line 12: The code currently constructs PrivateKey instances using
Network::Testnet; update those calls to use the SDK-configured network instead
by replacing Network::Testnet with wrapper.sdk.network (or otherwise obtain the
network from wrapper.sdk) wherever PrivateKey::new(...) is invoked (e.g., the
call sites around PrivateKey::new() at top-level and the occurrences around
lines 129-130 in shield.rs) so the private keys are created using the configured
SDK network rather than a hardcoded Testnet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b9a4391-a7d2-43f9-aefe-7b509f9604c9
📒 Files selected for processing (18)
packages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/src/lib.rspackages/rs-sdk-ffi/src/nullifier_sync/mod.rspackages/rs-sdk-ffi/src/nullifier_sync/types.rspackages/rs-sdk-ffi/src/shielded/mod.rspackages/rs-sdk-ffi/src/shielded/queries/anchors.rspackages/rs-sdk-ffi/src/shielded/queries/encrypted_notes.rspackages/rs-sdk-ffi/src/shielded/queries/mod.rspackages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rspackages/rs-sdk-ffi/src/shielded/queries/nullifiers.rspackages/rs-sdk-ffi/src/shielded/queries/pool_state.rspackages/rs-sdk-ffi/src/shielded/transitions/mod.rspackages/rs-sdk-ffi/src/shielded/transitions/shield.rspackages/rs-sdk-ffi/src/shielded/transitions/shield_from_asset_lock.rspackages/rs-sdk-ffi/src/shielded/transitions/shielded_transfer.rspackages/rs-sdk-ffi/src/shielded/transitions/shielded_withdrawal.rspackages/rs-sdk-ffi/src/shielded/transitions/unshield.rspackages/rs-sdk-ffi/src/shielded/types.rs
- Use `||` instead of `&&` for nullifier sync checkpoint zero check so either field being 0 triggers full scan - Add checked_mul for nullifier count to prevent overflow on 32-bit - Remap fee_from_input_index from caller's input order to BTreeMap's sorted key order for correct DeductFromInput resolution - Reject duplicate addresses in shield inputs upfront Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3239 +/- ##
============================================
- Coverage 55.87% 0 -55.88%
============================================
Files 3173 0 -3173
Lines 235215 0 -235215
============================================
- Hits 131435 0 -131435
+ Misses 103780 0 -103780
🚀 New features to boost your workflow:
|
Issue being fixed or feature implemented
Add comprehensive C-compatible FFI bindings for the shielded pool, enabling iOS/Swift clients to query, sync, build, and broadcast shielded transactions through the unified SDK.
What was done?
Shielded Pool Queries (5 functions)
dash_sdk_shielded_get_pool_state— Fetch total shielded pool balancedash_sdk_shielded_get_encrypted_notes— Fetch encrypted notes with paginationdash_sdk_shielded_get_anchors— Fetch all shielded pool anchorsdash_sdk_shielded_get_most_recent_anchor— Fetch the most recent anchordash_sdk_shielded_get_nullifiers— Check nullifier spent statusesNullifier BLAST Sync (3 functions)
Privacy-preserving nullifier status checking using trunk/branch chunk queries:
dash_sdk_sync_nullifiers— Full BLAST sync with result pointerdash_sdk_sync_nullifiers_with_result— BLAST sync with DashSDKResult error handlingdash_sdk_nullifier_sync_result_free— Free sync resultShielded State Transitions (7 functions)
Build and broadcast all 5 shielded transaction types:
dash_sdk_shielded_shield_funds— Shield from platform addressesdash_sdk_shielded_shield_from_instant_lock— Shield from L1 instant asset lockdash_sdk_shielded_shield_from_chain_lock— Shield from L1 chain asset lockdash_sdk_shielded_transfer— Shielded-to-shielded transferdash_sdk_shielded_unshield_funds— Unshield to platform addressdash_sdk_shielded_withdraw— Withdraw from shielded pool to L1 Core addressShared FFI Types
DashSDKOrchardBundleParams/DashSDKSerializedAction— shared across all transitionsDashSDKNullifierSyncConfig/DashSDKNullifierSyncResult/DashSDKNullifierSyncMetrics— BLAST sync typesDashSDKShieldInput— shield operation input entryInfrastructure
"shielded"feature todash-sdkdependency inCargo.tomlHow Has This Been Tested?
cargo check -p rs-sdk-ffi— compiles successfullycargo clippy -p rs-sdk-ffi— no warningscargo fmt -p rs-sdk-ffi -- --check— properly formattedBreaking Changes
None — additive only.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit