-
Notifications
You must be signed in to change notification settings - Fork 43
feat(sdk): ffi support and swift bindings #2661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v2.0-dev
Are you sure you want to change the base?
Conversation
…OS SDK FFI - Add ios_sdk_data_contract_put_to_platform for broadcasting data contract state transitions - Add ios_sdk_data_contract_put_to_platform_and_wait for confirmed data contract operations - Update document put methods to use consistent return types with identity methods - Broadcast methods now return serialized state transition data as binary - Wait methods now return proper handle types with IOSSDKResultDataType 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Split identity.rs into mod.rs with organized submodules - Created queries/ submodule with individual files for each query operation - Organized functions into logical modules: - helpers.rs: utility functions and converters - create.rs: identity creation - topup.rs: top-up operations - put.rs: put-to-platform operations - transfer.rs: credit transfer operations - withdraw.rs: withdrawal operations - info.rs: identity info and lifecycle - names.rs: name registration - queries/: all query operations split into separate files 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
WalkthroughThis update introduces the new Changes
Sequence Diagram(s)sequenceDiagram
participant C_App as C/Swift/Python App
participant FFI as rs-sdk-ffi (FFI Layer)
participant SDK as Dash SDK (Rust)
participant Platform as Dash Platform
C_App->>FFI: dash_sdk_identity_fetch(sdk_handle, identity_id)
FFI->>SDK: Fetch identity by ID (async)
SDK->>Platform: gRPC request: fetch identity
Platform-->>SDK: Identity data
SDK-->>FFI: Identity struct or error
FFI-->>C_App: Success (handle) or error
C_App->>FFI: dash_sdk_document_create(sdk_handle, params)
FFI->>SDK: Create document (async)
SDK->>Platform: gRPC request: create document
Platform-->>SDK: Confirmation
SDK-->>FFI: Document handle or error
FFI-->>C_App: Success (handle) or error
C_App->>FFI: dash_sdk_token_mint(sdk_handle, owner_id, params, ...)
FFI->>SDK: Mint token transition (async)
SDK->>Platform: gRPC request: mint token
Platform-->>SDK: Confirmation
SDK-->>FFI: Success or error
FFI-->>C_App: Success or error
C_App->>FFI: dash_sdk_identity_resolve_name(sdk_handle, name)
FFI->>SDK: Query DPNS contract (async)
SDK->>Platform: gRPC request: fetch domain document
Platform-->>SDK: Domain document
SDK-->>FFI: Identity ID or error
FFI-->>C_App: Success (identity ID) or error
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 33
🔭 Outside diff range comments (1)
packages/rs-sdk-ffi/src/data_contract/queries/info.rs (1)
1-2
:⚠️ Potential issueEmpty module stub detected.
info.rs
is currently empty. Either implement the intended FFI query functions or remove this placeholder to avoid confusion.
♻️ Duplicate comments (2)
packages/rs-sdk-ffi/src/system/queries/epochs_info.rs (1)
77-78
:⚠️ Potential issuePerformance: Avoid creating Tokio runtime per function call.
Same issue as in vote_state.rs - creating a new Tokio runtime for each call is expensive.
packages/rs-sdk-ffi/src/contested_resource/queries/voters_for_identity.rs (1)
119-120
:⚠️ Potential issuePerformance: Avoid creating Tokio runtime per function call.
Same runtime creation issue as in other files.
🧹 Nitpick comments (58)
docs/SDK_ARCHITECTURE.md (2)
1-7
: Consider adding a Table of Contents.
A high-level doc of this length would benefit from a TOC at the top for quick navigation.
9-22
: Use Mermaid line breaks instead of HTML<br/>
.
Replace<br/>
in node labels with\n
for better compatibility across Mermaid renderers:- RS[rs-sdk<br/>Rust SDK Core] + RS["rs-sdk\nRust SDK Core"]Cargo.toml (1)
33-35
: Verify workspace member ordering and path.
The new entry"packages/rs-sdk-ffi"
is added but the list is not alphabetically ordered. Confirm the path is correct and consider sorting or grouping entries consistently.packages/rs-sdk-ffi/src/evonode/mod.rs (1)
1-6
: Use Rust doc comments for module-level documentation.To ensure these comments are picked up by
rustdoc
, replace//
with//!
and provide a brief description of the module’s purpose. For example:-// Evonode-related modules +//! FFI bindings for Evonode-related operations. pub mod queries; -// Re-export all public functions +//! Re-export all Evonode query functions for external use. pub use queries::*;packages/rs-sdk-ffi/src/group/mod.rs (1)
1-6
: Switch to//!
for module docs and clarify intent.Align with Rust documentation conventions by using
//!
and enrich the comments. For example:-// Group-related modules +//! FFI bindings for group-related operations. pub mod queries; -// Re-export all public functions +//! Re-export all Group query functions for external use. pub use queries::*;packages/rs-drive-proof-verifier/src/types/token_contract_info.rs (1)
1-5
: Rename or document the alias to avoid confusion.The
Result
suffix suggests aResult<T, E>
type, but this is anOption
. Consider renaming or enhancing the doc:-use dpp::tokens::contract_info::TokenContractInfo; - -/// Token contract info -pub type TokenContractInfoResult = Option<TokenContractInfo>; +use dpp::tokens::contract_info::TokenContractInfo; + +/// Optional token contract information; `None` if unavailable. +pub type TokenContractInfoOption = Option<TokenContractInfo>;packages/rs-sdk-ffi/src/system/mod.rs (1)
1-6
: Adopt Rust doc conventions and elaborate.Use
//!
comments so that this description appears in generated docs:-// System-related modules +//! FFI bindings for system-related operations. pub mod queries; -// Re-export all public functions +//! Re-export all System query functions for external use. pub use queries::*;packages/rs-sdk-ffi/src/contested_resource/mod.rs (1)
1-6
: Convert to//!
doc comments and add context.Switch to module-level docs with
//!
:-// Contested resource modules +//! FFI bindings for contested resource-related operations. pub mod queries; -// Re-export all public functions +//! Re-export all ContestedResource query functions for external use. pub use queries::*;packages/rs-sdk-ffi/src/protocol_version/mod.rs (1)
1-6
: Consider consistency with explicit re-exports.While the wildcard re-export works correctly, other similar modules in this codebase use explicit function re-exports (e.g.,
document/queries/mod.rs
,evonode/queries/mod.rs
). Consider using explicit re-exports for consistency and better documentation.-// Re-export all public functions -pub use queries::*; +// Re-export all public functions for convenient access +pub use queries::{dash_sdk_protocol_version_get_upgrade_state, dash_sdk_protocol_version_get_upgrade_vote_status};packages/rs-sdk-ffi/build.rs (1)
4-33
: LGTM: Well-configured build script for C bindings.The build script properly configures cbindgen with appropriate settings for C FFI generation. The conditional generation based on environment variables is a good practice to avoid unnecessary work during normal builds.
Consider adding better error handling instead of panicking:
- .expect("Unable to generate bindings") + .unwrap_or_else(|e| { + eprintln!("Warning: Failed to generate bindings: {}", e); + return; + })packages/rs-sdk-ffi/src/identity/queries/resolve_test.rs (1)
51-76
: Consider extracting parsing logic to avoid duplication.The name parsing test duplicates logic from the main implementation. Consider extracting the parsing logic into a shared utility function that can be tested independently and reused.
+// In a shared module or the main resolve.rs file +pub(crate) fn parse_domain_name(name: &str) -> (&str, &str) { + if let Some(dot_pos) = name.rfind('.') { + (&name[..dot_pos], &name[dot_pos + 1..]) + } else { + (name, "dash") + } +} #[test] fn test_resolve_name_parsing() { let test_cases = vec![ ("alice.dash", "alice", "dash"), ("bob", "bob", "dash"), ("test.subdomain.dash", "test.subdomain", "dash"), ]; for (input, expected_label, expected_parent) in test_cases { - let (label, parent) = if let Some(dot_pos) = input.rfind('.') { - (&input[..dot_pos], &input[dot_pos + 1..]) - } else { - (input, "dash") - }; + let (label, parent) = parse_domain_name(input);packages/rs-sdk-ffi/src/identity/create.rs (1)
16-21
: Clarify implementation timeline for identity creation.The stub implementation with
NotImplemented
error is appropriate for now, but consider:
- Adding a GitHub issue to track the implementation
- Documenting the expected timeline in the TODO comment
- Ensuring the API signature won't change when implemented
Would you like me to help create a tracking issue for implementing the identity creation functionality?
packages/rs-sdk-ffi/src/data_contract/queries/mod.rs (1)
4-4
: Consider documenting the purpose of theinfo
module.The
info
module is declared but no functions are re-exported from it. If this module contains functionality that should be exposed, consider adding the appropriate re-exports. If it's intentionally empty or for future use, consider adding a comment to clarify its purpose.packages/rs-sdk-ffi/NULL_CHECK_FIXES_SUMMARY.md (1)
1-56
: Good documentation of safety improvements.This document provides valuable visibility into the null pointer safety improvements across the FFI layer. The consistent pattern shown for null checks is appropriate for FFI safety.
Consider whether this documentation belongs in a more permanent location (like the main docs directory) rather than in the package root, depending on your project's documentation conventions.
packages/rs-sdk-ffi/src/system/queries/total_credits_in_platform.rs (2)
68-68
: Add safety documentation for unsafe pointer dereferencing.The unsafe block should include a safety comment explaining why the pointer dereference is safe, given that null pointer checks are performed earlier.
+ // Safety: sdk_handle has been validated as non-null above let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) };
74-74
: Use serde for JSON serialization instead of manual formatting.Manual JSON formatting is error-prone and doesn't handle escaping properly. Consider using serde_json for safer serialization.
- let json = format!(r#"{{"credits":{}}}"#, credits); + let json = serde_json::to_string(&serde_json::json!({"credits": credits})) + .map_err(|e| format!("Failed to serialize JSON: {}", e))?;packages/rs-sdk-ffi/src/identity/queries/balance.rs (1)
63-63
: Consider using a more descriptive variable name.The
balance_str
variable name could be more specific since it represents the string representation of the balance.- let balance_str = match CString::new(balance.to_string()) { + let balance_c_str = match CString::new(balance.to_string()) {packages/rs-sdk-ffi/src/group/queries/info.rs (1)
110-125
: Consider using serde for complex JSON serialization.Manual JSON formatting for complex nested structures is error-prone and hard to maintain. Consider defining a serializable struct.
+ #[derive(serde::Serialize)] + struct GroupInfo { + required_power: u32, + members: Vec<GroupMember>, + } + + #[derive(serde::Serialize)] + struct GroupMember { + id: String, + power: u32, + } + - let members_json: Vec<String> = members + let members_vec: Vec<GroupMember> = members .iter() - .map(|(id, power)| { - format!( - r#"{{"id":"{}","power":{}}}"#, - bs58::encode(id.as_bytes()).into_string(), - power - ) + .map(|(id, power)| GroupMember { + id: bs58::encode(id.as_bytes()).into_string(), + power: *power, }) .collect(); - let json = format!( - r#"{{"required_power":{},"members":[{}]}}"#, - required_power, - members_json.join(",") - ); + let group_info = GroupInfo { + required_power, + members: members_vec, + }; + let json = serde_json::to_string(&group_info) + .map_err(|e| format!("Failed to serialize group info: {}", e))?;packages/rs-sdk-ffi/src/document/queries/info.rs (1)
33-34
: Consider using null or empty strings instead of "unknown" placeholders.Using "unknown" as placeholder values for missing
data_contract_id
anddocument_type_name
could mislead consumers who might treat these as valid identifiers. Consider using empty strings or null values to clearly indicate unavailable data.- // Document doesn't have data_contract_id, use placeholder - let data_contract_id_str = match CString::new("unknown") { + // Document doesn't have data_contract_id, use empty string + let data_contract_id_str = match CString::new("") {- // Document doesn't have document_type_name, use placeholder - let document_type_str = match CString::new("unknown") { + // Document doesn't have document_type_name, use empty string + let document_type_str = match CString::new("") {Also applies to: 43-44
packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_state.rs (1)
67-68
: Consider reusing runtime from SDK wrapper.Creating a new Tokio runtime for each call could be inefficient. Consider reusing the runtime from the SDK wrapper like other FFI functions do.
- let rt = tokio::runtime::Runtime::new() - .map_err(|e| format!("Failed to create Tokio runtime: {}", e))?; - let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) }; let sdk = wrapper.sdk.clone(); - rt.block_on(async move { + wrapper.runtime.block_on(async move {packages/rs-sdk-ffi/src/identity/queries/identities_balances.rs (1)
88-89
: Document the memory ownership transfer pattern.The
std::mem::forget
correctly transfers ownership to C, but this pattern should be documented for maintainability.let entries_ptr = entries.as_mut_ptr(); - std::mem::forget(entries); // Prevent deallocation + std::mem::forget(entries); // Transfer ownership to C - caller must freepackages/rs-sdk-ffi/src/data_contract/queries/fetch_many.rs (1)
70-70
: Consider handling serialization errors more gracefully.The
unwrap_or_else(|_| "null".to_string())
silently converts serialization errors to null. Consider logging the error or returning a more descriptive error response.- serde_json::to_string(&contract).unwrap_or_else(|_| "null".to_string()) + serde_json::to_string(&contract).unwrap_or_else(|e| { + eprintln!("Warning: Failed to serialize contract {}: {}", id, e); + "null".to_string() + })packages/rs-sdk-ffi/build_ios.sh (1)
11-11
: Remove unused variables identified by static analysis.The variables
PROJECT_NAME
andRED
are defined but never used in the script.-PROJECT_NAME="rs_sdk_ffi"
-RED='\033[0;31m'
Also applies to: 18-18
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 11-11: PROJECT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
packages/rs-sdk-ffi/README.md (1)
200-200
: Fix grammar: "to regenerate"-4. Regenerate headers by running: `GENERATE_BINDINGS=1 cargo build --release` +4. To regenerate headers, run: `GENERATE_BINDINGS=1 cargo build --release`🧰 Tools
🪛 LanguageTool
[grammar] ~200-~200: Did you mean “to Regenerate”?
Context: ..."3. Update cbindgen.toml if needed 4. Regenerate headers by running:
GENERATE_BINDINGS=...(MISSING_TO_BEFORE_A_VERB)
packages/rs-sdk-ffi/src/lib.rs (1)
25-38
: Consider explicit exports to avoid potential naming conflictsUsing wildcard re-exports (
pub use module::*
) from all modules could lead to naming conflicts if different modules export items with the same names.Consider being more explicit about what you're exporting, or at least document any naming conventions to avoid conflicts between modules. For example:
// Group related exports together pub use error::{DashSDKError, DashSDKErrorCode, DashSDKResult}; pub use types::{SDKHandle, DashSDKResultDataType}; // ... etcpackages/rs-sdk-ffi/README_NAME_RESOLUTION.md (1)
45-45
: Add missing comma for clarity-The function queries the DPNS data contract which stores domain documents. Each domain document contains: +The function queries the DPNS data contract, which stores domain documents. Each domain document contains:🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: Possible missing comma found.
Context: ...act The function queries the DPNS data contract which stores domain documents. Each dom...(AI_HYDRA_LEO_MISSING_COMMA)
packages/rs-sdk-ffi/src/group/queries/infos.rs (1)
65-66
: Remove unused variablesThe
_limit
parameter and_sdk
variable should either be used or removed entirely since the implementation is commented out.fn get_group_infos( sdk_handle: *const SDKHandle, start_at_position: *const c_char, - _limit: u32, + _limit: u32, // TODO: Use when implementation is complete ) -> Result<Option<String>, String> {Also applies to: 76-77
packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_ids.rs (1)
141-181
: Consider extracting common Query trait implementations.This Query trait implementation appears to be boilerplate that might be duplicated across multiple query types. Consider creating a macro or generic implementation to reduce code duplication.
Would you like me to help create a macro or generic implementation for common Query trait patterns?
packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_vote_status.rs (1)
25-25
: Consider validating the count parameter.A count of 0 doesn't make sense for this query. Consider adding validation to ensure count > 0.
) -> DashSDKResult { + if count == 0 { + return DashSDKResult { + data_type: DashSDKResultDataType::None, + data: std::ptr::null_mut(), + error: Box::into_raw(Box::new(DashSDKError::new( + DashSDKErrorCode::InvalidArgument, + "Count must be greater than 0".to_string(), + ))), + }; + } match get_protocol_version_upgrade_vote_status(sdk_handle, start_pro_tx_hash, count) {packages/rs-sdk-ffi/src/contested_resource/queries/vote_state.rs (2)
161-161
: Add context to hex decode errors.The error message for hex decode failures lacks context about which specific index value failed.
-let bytes = hex::decode(&hex_str).map_err(|e| format!("Failed to decode index value: {}", e))?; +let bytes = hex::decode(&hex_str).map_err(|e| format!("Failed to decode index value '{}': {}", hex_str, e))?;
232-233
: Extract document serialization to improve readability.The inline hex encoding within string formatting makes the code harder to read and maintain.
let document_json = if let Some(ref document) = contender.serialized_document() { - format!(r#""document":"{}""#, - hex::encode(document)) + format!(r#""document":"{}""#, hex::encode(document)) } else { r#""document":null"#.to_string() };packages/rs-sdk-ffi/src/system/queries/epochs_info.rs (1)
93-95
: Improve error message for epoch parsing.The error message could be more specific about the valid range for epoch values.
start_str .parse::<u16>() - .map_err(|e| format!("Failed to parse start epoch: {}", e))?, + .map_err(|e| format!("Failed to parse start epoch '{}' as u16 (valid range: 0-65535): {}", start_str, e))?,packages/rs-sdk-ffi/src/data_contract/queries/history.rs (1)
121-125
: Handle JSON serialization errors explicitly.Using
unwrap_or_else
with a fallback to "null" could hide serialization errors.-let contract_json = serde_json::to_string(&serde_json::json!({ - "id": bs58::encode(contract.id().as_bytes()).into_string(), - "owner_id": bs58::encode(contract.owner_id().as_bytes()).into_string(), -})) -.unwrap_or_else(|_| "null".to_string()); +let contract_json = match serde_json::to_string(&serde_json::json!({ + "id": bs58::encode(contract.id().as_bytes()).into_string(), + "owner_id": bs58::encode(contract.owner_id().as_bytes()).into_string(), +})) { + Ok(json) => json, + Err(e) => return Err(FFIError::InternalError(format!("Failed to serialize contract: {}", e))), +};packages/rs-sdk-ffi/src/test_utils.rs (2)
54-59
: Document the intentional memory leak in mock_sign_callback.The function uses
std::mem::forget
to intentionally leak memory, which should be documented for clarity.Add a comment explaining the memory management:
pub unsafe extern "C" fn mock_sign_callback( _identity_public_key_bytes: *const u8, _identity_public_key_len: usize, _data: *const u8, _data_len: usize, result_len: *mut usize, ) -> *mut u8 { + // Create a mock signature and intentionally leak it + // The caller is responsible for freeing this memory let signature = vec![0u8; 64]; *result_len = signature.len(); let ptr = signature.as_ptr() as *mut u8; std::mem::forget(signature); ptr }
123-126
: Handle CString creation failure gracefully.Using
unwrap()
in test utilities could cause panics during testing.pub fn create_c_string(s: &str) -> *mut std::os::raw::c_char { - CString::new(s).unwrap().into_raw() + CString::new(s) + .expect("Failed to create CString: input contains null byte") + .into_raw() }packages/rs-sdk-ffi/src/document/queries/search.rs (1)
199-204
: Pagination withstart_at
is not implemented.The
start_at
parameter is accepted in the function signature but returns an error when used. This could be confusing for API users. Consider either:
- Implementing proper pagination with document IDs
- Removing the parameter from the struct until it's supported
- Adding clear documentation about this limitation
Would you like me to help implement the pagination functionality or create an issue to track this TODO?
packages/rs-sdk-ffi/src/contested_resource/queries/resources.rs (2)
113-128
: Consider extracting unsafe CStr conversions to a helper function.The repeated pattern of unsafe CStr conversions could be refactored into a helper function for better maintainability and reduced code duplication.
Consider adding a helper function:
fn cstr_to_str<'a>(ptr: *const c_char, field_name: &str) -> Result<&'a str, String> { unsafe { CStr::from_ptr(ptr) .to_str() .map_err(|e| format!("Invalid UTF-8 in {}: {}", field_name, e)) } }Then use it to simplify the conversions:
- let contract_id_str = unsafe { - CStr::from_ptr(contract_id) - .to_str() - .map_err(|e| format!("Invalid UTF-8 in contract ID: {}", e))? - }; + let contract_id_str = cstr_to_str(contract_id, "contract ID")?;
220-260
: Consider adding more comprehensive test coverage.The current tests only cover null SDK handle and null contract ID. Consider adding tests for:
- Null document type name
- Null index name
- Invalid JSON in start/end index values
- Invalid base58 contract ID
- Contract ID with incorrect length
packages/rs-sdk-ffi/src/signer.rs (1)
69-74
: Forward reference todash_sdk_bytes_free
before definition.The function
dash_sdk_bytes_free
is called here but defined later in the file (line 106). While this compiles correctly, it's better practice to either declare the function earlier or reorder the code for clarity.packages/rs-sdk-ffi/src/sdk.rs (2)
134-140
: Consider explicit error handling for unknown network types.The fallback to
Local
for unknown network types (line 139) could be confusing. Consider returning an error for truly unknown networks instead.match wrapper.sdk.network { Network::Dash => DashSDKNetwork::Mainnet, Network::Testnet => DashSDKNetwork::Testnet, Network::Devnet => DashSDKNetwork::Devnet, Network::Regtest => DashSDKNetwork::Local, - _ => DashSDKNetwork::Local, // Fallback for any other network types + _ => { + // Log warning about unknown network type + DashSDKNetwork::Local + } }
143-182
: Inconsistent error handling compared todash_sdk_create
.This function returns null pointer on error while
dash_sdk_create
returns a properDashSDKResult
with error details. Consider making the error handling consistent across all public FFI functions.packages/rs-sdk-ffi/src/data_contract/mod.rs (1)
100-102
: Incomplete implementation - contract is created but not published.The comment indicates that the contract is only created locally but not published to the platform. This might be confusing for users expecting a fully functional contract creation.
Consider either:
- Renaming the function to
dash_sdk_data_contract_create_local
to clarify it's local-only- Implementing the full publish flow with signing and broadcasting
- Adding a separate
dash_sdk_data_contract_publish
functionWould you like me to help implement the contract publishing functionality or create an issue to track this?
packages/rs-sdk-ffi/src/document/create.rs (1)
111-111
: Unnecessary string allocation.Using
to_string()
creates an unnecessary allocation sincedocument_type
is already a&str
.- document_type.to_string(), + document_type,However, verify if
create_document
expects an owned String or can accept &str.packages/rs-sdk-ffi/src/document/transfer.rs (2)
106-110
: Simplify document type validationThe document type validation can be simplified by not storing the unused results.
- // Get document type from data contract - let _document_type = data_contract + // Validate document type exists + data_contract .document_type_for_name(document_type_name_str) .map_err(|e| FFIError::InternalError(format!("Failed to get document type: {}", e)))?; - - let _document_type_owned = _document_type.to_owned_document_type();
244-248
: Simplify document type validationSame as the previous function, the document type validation can be simplified.
- // Get document type from data contract - let _document_type = data_contract + // Validate document type exists + data_contract .document_type_for_name(document_type_name_str) .map_err(|e| FFIError::InternalError(format!("Failed to get document type: {}", e)))?; - - let _document_type_owned = _document_type.to_owned_document_type();packages/rs-sdk-ffi/src/token/mint.rs (2)
47-52
: Remove redundant unsafe blocksThe function is already unsafe, so these additional unsafe blocks are redundant.
- // SAFETY: We've verified all pointers are non-null above - let wrapper = unsafe { &mut *(sdk_handle as *mut SDKWrapper) }; - let identity_public_key = unsafe { &*(identity_public_key_handle as *const IdentityPublicKey) }; - let signer = unsafe { &*(signer_handle as *const crate::signer::IOSSigner) }; - let params = unsafe { &*params }; + // SAFETY: We've verified all pointers are non-null above + let wrapper = &mut *(sdk_handle as *mut SDKWrapper); + let identity_public_key = &*(identity_public_key_handle as *const IdentityPublicKey); + let signer = &*(signer_handle as *const crate::signer::IOSSigner); + let params = &*params;
184-187
: Consider returning mint result informationThe function discards the
MintResult
and returns a null pointer on success. Consider returning information about the mint operation to the caller.The
MintResult
likely contains useful information that could be returned to the caller, such as the resulting token balances or transaction details. Consider creating a result structure similar toDashSDKTransferCreditsResult
to return this information.packages/rs-sdk-ffi/src/identity/topup.rs (1)
130-140
: Unused variable_new_balance
The variable
_new_balance
is assigned but never used. If the balance is not needed, consider using_
pattern or removing the assignment entirely.- let _new_balance = identity + identity .top_up_identity( &wrapper.sdk, asset_lock_proof, &private_key, settings.and_then(|s| s.user_fee_increase), settings, ) .await .map_err(|e| FFIError::InternalError(format!("Failed to top up identity: {}", e)))?;packages/rs-sdk-ffi/src/document/helpers.rs (1)
24-60
: Consider documenting safety requirementsThe function correctly handles null pointers, but assumes non-null pointers point to valid data (specifically,
payment_token_contract_id
must point to a valid 32-byte array).Add a safety comment documenting the caller's responsibility:
/// Convert FFI TokenPaymentInfo to Rust TokenPaymentInfo +/// +/// # Safety +/// If `ffi_token_payment_info` is not null: +/// - `payment_token_contract_id` (if not null) must point to a valid 32-byte array pub unsafe fn convert_token_payment_info(packages/rs-sdk-ffi/src/token/destroy_frozen_funds.rs (2)
47-50
: Consider improving the safety comment.The safety comment could be more comprehensive. Consider expanding it to explain why the pointer dereferences are safe beyond just null checks (e.g., lifetime guarantees, exclusive access).
- // SAFETY: We've verified all pointers are non-null above + // SAFETY: We've verified all pointers are non-null above. The pointers are valid for the + // duration of this function call as they're passed from the FFI boundary and the caller + // guarantees their validity.
108-109
: Remove redundant unsafe blocks.The
unsafe
blocks on lines 108-109 and 127-131 are redundant since the containing function is alreadyunsafe
. The inner unsafe blocks don't add any safety documentation value.- let token_contract_id_str = match unsafe { CStr::from_ptr(params.token_contract_id) }.to_str() { + let token_contract_id_str = match CStr::from_ptr(params.token_contract_id).to_str() {- let contract_slice = unsafe { - std::slice::from_raw_parts( - params.serialized_contract, - params.serialized_contract_len - ) - }; + let contract_slice = std::slice::from_raw_parts( + params.serialized_contract, + params.serialized_contract_len + );Also applies to: 127-131
packages/rs-sdk-ffi/src/error.rs (1)
104-131
: Consider a more robust error classification approach.The current string-based error classification is fragile and could break if error messages change. Consider implementing a more structured approach, such as using error variants or error codes from the underlying SDK instead of string matching.
packages/rs-sdk-ffi/src/token/freeze.rs (1)
108-109
: Remove redundant unsafe blocks.Similar to the previous file, these
unsafe
blocks are redundant within an already unsafe function context.- let token_contract_id_str = match unsafe { CStr::from_ptr(params.token_contract_id) }.to_str() { + let token_contract_id_str = match CStr::from_ptr(params.token_contract_id).to_str() {- let contract_slice = unsafe { - std::slice::from_raw_parts( - params.serialized_contract, - params.serialized_contract_len - ) - }; + let contract_slice = std::slice::from_raw_parts( + params.serialized_contract, + params.serialized_contract_len + );Also applies to: 127-132
packages/rs-sdk-ffi/src/token/config_update.rs (1)
107-109
: Remove redundant unsafe blocks.- let token_contract_id_str = match unsafe { CStr::from_ptr(params.token_contract_id) }.to_str() { + let token_contract_id_str = match CStr::from_ptr(params.token_contract_id).to_str() {- let contract_slice = unsafe { - std::slice::from_raw_parts( - params.serialized_contract, - params.serialized_contract_len - ) - }; + let contract_slice = std::slice::from_raw_parts( + params.serialized_contract, + params.serialized_contract_len + );Also applies to: 126-131
packages/rs-sdk-ffi/src/token/claim.rs (1)
95-97
: Remove redundant unsafe blocks.- let token_contract_id_str = match unsafe { CStr::from_ptr(params.token_contract_id) }.to_str() { + let token_contract_id_str = match CStr::from_ptr(params.token_contract_id).to_str() {- let contract_slice = unsafe { - std::slice::from_raw_parts( - params.serialized_contract, - params.serialized_contract_len - ) - }; + let contract_slice = std::slice::from_raw_parts( + params.serialized_contract, + params.serialized_contract_len + );Also applies to: 114-119
packages/rs-sdk-ffi/src/identity/put.rs (2)
1-335
: Add unit tests for identity put operations.The file lacks unit tests for these critical FFI functions. Tests should cover:
- Null pointer validation for all parameters
- Successful put operations with instant lock and chain lock
- Error scenarios during state transition creation
- Serialization failures
Would you like me to generate comprehensive unit tests following the pattern used in other FFI modules?
36-47
: Consider extracting common null validation logic.All four functions have nearly identical null pointer validation. Consider creating a helper function to reduce duplication:
+/// Validates common required parameters for identity put operations +unsafe fn validate_put_params( + sdk_handle: *mut SDKHandle, + identity_handle: *const IdentityHandle, + private_key: *const [u8; 32], + signer_handle: *const crate::types::SignerHandle, +) -> Result<(), DashSDKError> { + if sdk_handle.is_null() + || identity_handle.is_null() + || private_key.is_null() + || signer_handle.is_null() + { + return Err(DashSDKError::new( + DashSDKErrorCode::InvalidParameter, + "One or more required parameters is null".to_string(), + )); + } + Ok(()) +}Then use it in each function:
- if sdk_handle.is_null() - || identity_handle.is_null() - || instant_lock_bytes.is_null() - || transaction_bytes.is_null() - || private_key.is_null() - || signer_handle.is_null() - { - return DashSDKResult::error(DashSDKError::new( - DashSDKErrorCode::InvalidParameter, - "One or more required parameters is null".to_string(), - )); - } + if let Err(e) = validate_put_params(sdk_handle, identity_handle, private_key, signer_handle) { + return DashSDKResult::error(e); + } + if instant_lock_bytes.is_null() || transaction_bytes.is_null() { + return DashSDKResult::error(DashSDKError::new( + DashSDKErrorCode::InvalidParameter, + "Instant lock or transaction bytes is null".to_string(), + )); + }Also applies to: 121-132, 204-214, 278-288
packages/rs-sdk-ffi/src/document/delete.rs (1)
204-214
: Consider returning the deleted document ID.The function extracts the deleted ID but then discards it, returning a null pointer. Consider returning the ID for better API usability:
- match result { - Ok(_deleted_id) => DashSDKResult::success(std::ptr::null_mut()), - Err(e) => DashSDKResult::error(e.into()), - } + match result { + Ok(deleted_id) => { + // Serialize the identifier + let id_bytes = deleted_id.to_buffer(); + DashSDKResult::success_binary(id_bytes) + } + Err(e) => DashSDKResult::error(e.into()), + }This would provide confirmation of which document was deleted and align with other operations that return meaningful data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (107)
.gitignore
(1 hunks)Cargo.toml
(1 hunks)docs/SDK_ARCHITECTURE.md
(1 hunks)packages/rs-dapi-client/src/transport/grpc.rs
(1 hunks)packages/rs-drive-proof-verifier/src/proof.rs
(1 hunks)packages/rs-drive-proof-verifier/src/proof/token_contract_info.rs
(1 hunks)packages/rs-drive-proof-verifier/src/types.rs
(1 hunks)packages/rs-drive-proof-verifier/src/types/token_contract_info.rs
(1 hunks)packages/rs-drive-verify-c-binding/.gitignore
(0 hunks)packages/rs-drive-verify-c-binding/Cargo.toml
(0 hunks)packages/rs-drive-verify-c-binding/build.rs
(0 hunks)packages/rs-drive-verify-c-binding/c/main.c
(0 hunks)packages/rs-drive-verify-c-binding/c/utils.c
(0 hunks)packages/rs-drive-verify-c-binding/src/lib.rs
(0 hunks)packages/rs-drive-verify-c-binding/src/types.rs
(0 hunks)packages/rs-drive-verify-c-binding/src/util.rs
(0 hunks)packages/rs-sdk-ffi/Cargo.toml
(1 hunks)packages/rs-sdk-ffi/NULL_CHECK_FIXES_SUMMARY.md
(1 hunks)packages/rs-sdk-ffi/README.md
(1 hunks)packages/rs-sdk-ffi/README_NAME_RESOLUTION.md
(1 hunks)packages/rs-sdk-ffi/build.rs
(1 hunks)packages/rs-sdk-ffi/build_ios.sh
(1 hunks)packages/rs-sdk-ffi/cbindgen.toml
(1 hunks)packages/rs-sdk-ffi/cbindgen_minimal.toml
(1 hunks)packages/rs-sdk-ffi/src/contested_resource/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/contested_resource/queries/identity_votes.rs
(1 hunks)packages/rs-sdk-ffi/src/contested_resource/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/contested_resource/queries/resources.rs
(1 hunks)packages/rs-sdk-ffi/src/contested_resource/queries/vote_state.rs
(1 hunks)packages/rs-sdk-ffi/src/contested_resource/queries/voters_for_identity.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/put.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/queries/fetch.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/queries/fetch_many.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/queries/history.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/queries/info.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/data_contract/util.rs
(1 hunks)packages/rs-sdk-ffi/src/document/create.rs
(1 hunks)packages/rs-sdk-ffi/src/document/delete.rs
(1 hunks)packages/rs-sdk-ffi/src/document/helpers.rs
(1 hunks)packages/rs-sdk-ffi/src/document/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/document/price.rs
(1 hunks)packages/rs-sdk-ffi/src/document/purchase.rs
(1 hunks)packages/rs-sdk-ffi/src/document/put.rs
(1 hunks)packages/rs-sdk-ffi/src/document/queries/fetch.rs
(1 hunks)packages/rs-sdk-ffi/src/document/queries/info.rs
(1 hunks)packages/rs-sdk-ffi/src/document/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/document/queries/search.rs
(1 hunks)packages/rs-sdk-ffi/src/document/replace.rs
(1 hunks)packages/rs-sdk-ffi/src/document/transfer.rs
(1 hunks)packages/rs-sdk-ffi/src/document/util.rs
(1 hunks)packages/rs-sdk-ffi/src/error.rs
(1 hunks)packages/rs-sdk-ffi/src/evonode/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/evonode/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_ids.rs
(1 hunks)packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rs
(1 hunks)packages/rs-sdk-ffi/src/group/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/group/queries/action_signers.rs
(1 hunks)packages/rs-sdk-ffi/src/group/queries/actions.rs
(1 hunks)packages/rs-sdk-ffi/src/group/queries/info.rs
(1 hunks)packages/rs-sdk-ffi/src/group/queries/infos.rs
(1 hunks)packages/rs-sdk-ffi/src/group/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/create.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/helpers.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/info.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/names.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/put.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/balance.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/balance_and_revision.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/by_non_unique_public_key_hash.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/by_public_key_hash.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/contract_nonce.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/fetch.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/identities_balances.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/identities_contract_keys.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/nonce.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/public_keys.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/resolve.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/queries/resolve_test.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/topup.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/transfer.rs
(1 hunks)packages/rs-sdk-ffi/src/identity/withdraw.rs
(1 hunks)packages/rs-sdk-ffi/src/lib.rs
(1 hunks)packages/rs-sdk-ffi/src/protocol_version/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/protocol_version/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_state.rs
(1 hunks)packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_vote_status.rs
(1 hunks)packages/rs-sdk-ffi/src/sdk.rs
(1 hunks)packages/rs-sdk-ffi/src/signer.rs
(1 hunks)packages/rs-sdk-ffi/src/system/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/system/queries/current_quorums_info.rs
(1 hunks)packages/rs-sdk-ffi/src/system/queries/epochs_info.rs
(1 hunks)packages/rs-sdk-ffi/src/system/queries/mod.rs
(1 hunks)packages/rs-sdk-ffi/src/system/queries/path_elements.rs
(1 hunks)packages/rs-sdk-ffi/src/system/queries/prefunded_specialized_balance.rs
(1 hunks)packages/rs-sdk-ffi/src/system/queries/total_credits_in_platform.rs
(1 hunks)packages/rs-sdk-ffi/src/test_utils.rs
(1 hunks)packages/rs-sdk-ffi/src/token/burn.rs
(1 hunks)packages/rs-sdk-ffi/src/token/claim.rs
(1 hunks)packages/rs-sdk-ffi/src/token/config_update.rs
(1 hunks)packages/rs-sdk-ffi/src/token/destroy_frozen_funds.rs
(1 hunks)packages/rs-sdk-ffi/src/token/emergency_action.rs
(1 hunks)packages/rs-sdk-ffi/src/token/freeze.rs
(1 hunks)packages/rs-sdk-ffi/src/token/mint.rs
(1 hunks)
⛔ Files not processed due to max files limit (1)
- packages/rs-sdk-ffi/src/token/mod.rs
💤 Files with no reviewable changes (8)
- packages/rs-drive-verify-c-binding/.gitignore
- packages/rs-drive-verify-c-binding/build.rs
- packages/rs-drive-verify-c-binding/Cargo.toml
- packages/rs-drive-verify-c-binding/c/utils.c
- packages/rs-drive-verify-c-binding/src/util.rs
- packages/rs-drive-verify-c-binding/c/main.c
- packages/rs-drive-verify-c-binding/src/lib.rs
- packages/rs-drive-verify-c-binding/src/types.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
🧬 Code Graph Analysis (16)
packages/rs-sdk-ffi/src/identity/queries/resolve_test.rs (2)
packages/rs-sdk-ffi/src/identity/queries/resolve.rs (1)
dash_sdk_identity_resolve_name
(29-180)packages/rs-sdk-ffi/src/token/destroy_frozen_funds.rs (1)
create_mock_sdk_handle
(200-203)
packages/rs-sdk-ffi/src/document/queries/mod.rs (2)
packages/rs-sdk-ffi/src/document/queries/fetch.rs (1)
dash_sdk_document_fetch
(16-77)packages/rs-sdk-ffi/src/document/queries/search.rs (1)
dash_sdk_document_search
(115-253)
packages/rs-dapi-client/src/transport/grpc.rs (1)
packages/rs-drive-abci/src/query/service.rs (1)
get_token_contract_info
(770-780)
packages/rs-sdk-ffi/src/identity/create.rs (2)
packages/rs-sdk-ffi/src/types.rs (1)
error
(179-185)packages/rs-sdk-ffi/src/error.rs (1)
new
(81-89)
packages/rs-sdk-ffi/src/protocol_version/queries/mod.rs (2)
packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_state.rs (1)
dash_sdk_protocol_version_get_upgrade_state
(20-58)packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_vote_status.rs (1)
dash_sdk_protocol_version_get_upgrade_vote_status
(22-62)
packages/rs-sdk-ffi/src/group/queries/infos.rs (4)
packages/rs-sdk-ffi/src/sdk.rs (1)
new
(22-27)packages/rs-sdk-ffi/src/token/emergency_action.rs (2)
create_mock_sdk_handle
(199-211)destroy_mock_sdk_handle
(214-218)packages/rs-sdk-ffi/src/token/claim.rs (2)
create_mock_sdk_handle
(192-204)destroy_mock_sdk_handle
(207-211)packages/rs-sdk-ffi/src/token/freeze.rs (2)
create_mock_sdk_handle
(201-213)destroy_mock_sdk_handle
(216-220)
packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_vote_status.rs (4)
packages/rs-sdk-ffi/src/sdk.rs (1)
new
(22-27)packages/rs-sdk-ffi/src/token/emergency_action.rs (2)
create_mock_sdk_handle
(199-211)destroy_mock_sdk_handle
(214-218)packages/rs-sdk-ffi/src/token/claim.rs (2)
create_mock_sdk_handle
(192-204)destroy_mock_sdk_handle
(207-211)packages/rs-sdk-ffi/src/token/freeze.rs (2)
create_mock_sdk_handle
(201-213)destroy_mock_sdk_handle
(216-220)
packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_state.rs (4)
packages/rs-sdk-ffi/src/sdk.rs (1)
new
(22-27)packages/rs-sdk-ffi/src/token/emergency_action.rs (2)
create_mock_sdk_handle
(199-211)destroy_mock_sdk_handle
(214-218)packages/rs-sdk-ffi/src/token/claim.rs (2)
create_mock_sdk_handle
(192-204)destroy_mock_sdk_handle
(207-211)packages/rs-sdk-ffi/src/token/freeze.rs (2)
create_mock_sdk_handle
(201-213)destroy_mock_sdk_handle
(216-220)
packages/rs-sdk-ffi/src/token/freeze.rs (6)
packages/rs-sdk-ffi/src/token/utils.rs (5)
convert_state_transition_creation_options
(14-46)extract_user_fee_increase
(59-67)parse_identifier_from_bytes
(106-116)parse_optional_note
(94-103)validate_contract_params
(70-91)packages/rs-sdk-ffi/src/types.rs (2)
error
(179-185)success
(125-131)packages/rs-sdk-ffi/src/error.rs (3)
new
(81-89)from
(101-148)success
(92-97)packages/rs-sdk-ffi/src/sdk.rs (2)
new
(22-27)dash_sdk_create
(42-116)packages/rs-sdk-ffi/src/signer.rs (1)
new
(34-39)packages/rs-sdk-ffi/src/identity/helpers.rs (1)
convert_put_settings
(15-75)
packages/rs-sdk-ffi/src/contested_resource/queries/mod.rs (4)
packages/rs-sdk-ffi/src/contested_resource/queries/identity_votes.rs (1)
dash_sdk_contested_resource_get_identity_votes
(25-73)packages/rs-sdk-ffi/src/contested_resource/queries/resources.rs (1)
dash_sdk_contested_resource_get_resources
(28-82)packages/rs-sdk-ffi/src/contested_resource/queries/vote_state.rs (1)
dash_sdk_contested_resource_get_vote_state
(31-85)packages/rs-sdk-ffi/src/contested_resource/queries/voters_for_identity.rs (1)
dash_sdk_contested_resource_get_voters_for_identity
(29-83)
packages/rs-sdk-ffi/src/identity/queries/by_non_unique_public_key_hash.rs (2)
packages/rs-sdk/src/platform/fetch.rs (1)
fetch
(92-97)packages/rs-sdk-ffi/src/types.rs (1)
success_string
(134-140)
packages/rs-sdk-ffi/src/test_utils.rs (1)
packages/rs-sdk-ffi/src/sdk.rs (1)
dash_sdk_destroy
(120-124)
packages/rs-sdk-ffi/src/token/claim.rs (6)
packages/rs-sdk-ffi/src/token/utils.rs (5)
convert_state_transition_creation_options
(14-46)convert_token_distribution_type
(49-56)extract_user_fee_increase
(59-67)parse_optional_note
(94-103)validate_contract_params
(70-91)packages/rs-sdk-ffi/src/types.rs (2)
error
(179-185)success
(125-131)packages/rs-sdk-ffi/src/error.rs (3)
new
(81-89)from
(101-148)success
(92-97)packages/rs-sdk-ffi/src/sdk.rs (3)
new
(22-27)dash_sdk_create
(42-116)dash_sdk_destroy
(120-124)packages/rs-sdk-ffi/src/signer.rs (1)
new
(34-39)packages/rs-sdk-ffi/src/identity/helpers.rs (1)
convert_put_settings
(15-75)
packages/rs-sdk-ffi/src/document/put.rs (7)
packages/rs-sdk-ffi/src/document/helpers.rs (2)
convert_state_transition_creation_options
(63-95)convert_token_payment_info
(25-60)packages/rs-sdk-ffi/src/types.rs (3)
error
(179-185)success_binary
(143-158)success_handle
(161-167)packages/rs-sdk-ffi/src/error.rs (2)
new
(81-89)from
(101-148)packages/rs-sdk-ffi/src/sdk.rs (1)
new
(22-27)packages/rs-sdk-ffi/src/signer.rs (1)
new
(34-39)packages/rs-sdk-ffi/src/test_utils.rs (7)
convert_put_settings
(101-121)create_mock_data_contract
(143-176)create_mock_identity_public_key
(29-31)create_mock_signer
(71-73)create_put_settings
(86-98)create_mock_sdk_handle
(16-19)destroy_mock_sdk_handle
(22-26)packages/rs-sdk-ffi/src/identity/helpers.rs (1)
convert_put_settings
(15-75)
packages/rs-sdk-ffi/src/evonode/queries/mod.rs (2)
packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_ids.rs (1)
dash_sdk_evonode_get_proposed_epoch_blocks_by_ids
(22-62)packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rs (1)
dash_sdk_evonode_get_proposed_epoch_blocks_by_range
(24-72)
packages/rs-sdk-ffi/src/document/mod.rs (5)
packages/rs-sdk-ffi/src/document/create.rs (1)
dash_sdk_document_create
(30-125)packages/rs-sdk-ffi/src/document/queries/info.rs (1)
dash_sdk_document_get_info
(11-65)packages/rs-sdk-ffi/src/document/queries/fetch.rs (1)
dash_sdk_document_fetch
(16-77)packages/rs-sdk-ffi/src/document/util.rs (2)
dash_sdk_document_destroy
(7-37)dash_sdk_document_handle_destroy
(41-45)packages/rs-sdk-ffi/src/document/helpers.rs (3)
convert_gas_fees_paid_by
(16-22)convert_state_transition_creation_options
(63-95)convert_token_payment_info
(25-60)
🪛 LanguageTool
packages/rs-sdk-ffi/README.md
[grammar] ~200-~200: Did you mean “to Regenerate”?
Context: ..."3. Update cbindgen.toml if needed 4. Regenerate headers by running:
GENERATE_BINDINGS=...
(MISSING_TO_BEFORE_A_VERB)
packages/rs-sdk-ffi/README_NAME_RESOLUTION.md
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ...me ); ``` ## Parameters - sdk_handle
: A handle to an initialized SDK instance...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Possible missing comma found.
Context: ...act The function queries the DPNS data contract which stores domain documents. Each dom...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~68-~68: Do not use a colon (:) before a series that is introduced by a preposition (‘for’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...unction returns appropriate error codes for: - InvalidParameter
: Null SDK handle, ...
(RP_COLON)
🪛 Shellcheck (0.10.0)
packages/rs-sdk-ffi/build_ios.sh
[warning] 11-11: PROJECT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 18-18: RED appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Formatting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (101)
docs/SDK_ARCHITECTURE.md (1)
61-87
: Core SDK Layer section is clear and concise.
The component breakdown and ASCII-style diagram give a solid overview ofrs-sdk
. No changes needed here.packages/rs-drive-proof-verifier/src/proof.rs (1)
3-4
: Exposetoken_contract_info
module.
Thepub mod token_contract_info;
aligns with new proof logic. Ensureproof/token_contract_info.rs
exists and implementsFromProof
forTokenContractInfo
.packages/rs-drive-proof-verifier/src/types.rs (1)
14-15
: Addtoken_contract_info
type module.
Nice addition of the new module with documentation. Confirm thattypes/token_contract_info.rs
defines the expectedTokenContractInfoResult
.packages/rs-sdk-ffi/src/document/queries/mod.rs (1)
1-10
: LGTM: Well-structured module organization.The explicit re-exports provide clear documentation of the public API and follow consistent patterns with other FFI modules. The inclusion of both functions and types in the re-exports is appropriate.
packages/rs-sdk-ffi/src/protocol_version/queries/mod.rs (1)
1-8
: LGTM: Consistent FFI module pattern.The module follows the established pattern for FFI query organization with clear submodule declarations and explicit re-exports. The function naming convention is consistent with other SDK FFI functions.
packages/rs-sdk-ffi/src/evonode/queries/mod.rs (1)
1-8
: LGTM: Consistent with established FFI patterns.The module organization follows the same well-established pattern used throughout the FFI codebase. The explicit re-exports and descriptive function names provide a clear public API.
.gitignore (1)
36-61
: LGTM! Comprehensive build artifact exclusions.The additions appropriately cover:
- Rust target directories across all packages
- Standard Swift/iOS build artifacts and IDE files
- Generated Swift SDK headers and frameworks
- rs-sdk-ffi build directory
These exclusions align well with the new Swift bindings and FFI package being introduced.
packages/rs-sdk-ffi/src/identity/queries/resolve_test.rs (2)
11-33
: Excellent parameter validation test coverage.The tests properly verify error handling for null SDK handle and null name pointer, ensuring the FFI function safely rejects invalid inputs.
35-49
: Good UTF-8 validation test.The test correctly uses an invalid UTF-8 sequence and verifies proper error handling. The cast to
*const _
is appropriate for simulating invalid C string input.packages/rs-sdk-ffi/src/group/queries/mod.rs (1)
1-11
: Clean module organization with consistent API surface.The module structure follows Rust conventions with clear submodule declarations and convenient re-exports. The
dash_sdk_group_*
naming pattern is consistent across all exported functions.packages/rs-sdk-ffi/cbindgen_minimal.toml (1)
1-22
: Well-configured cbindgen setup for clean C FFI.The configuration appropriately:
- Uses standard C header practices (pragma once, include guard)
- Includes necessary system headers for C interop
- Restricts exports to
dash_sdk_*
symbols for a clean API surface- Applies consistent naming conventions (snake_case for functions/fields, ScreamingSnakeCase for enums)
This ensures generated C headers will be compatible and follow C conventions.
packages/rs-sdk-ffi/src/identity/create.rs (1)
8-14
: Good FFI safety with null pointer validation.The null pointer check is essential for FFI safety and follows the same pattern used throughout the codebase.
packages/rs-sdk-ffi/src/data_contract/queries/mod.rs (1)
1-10
: Module organization looks good.The module structure follows standard Rust patterns and provides a clean interface for data contract query operations. The re-exports enable convenient access to the main query functions.
packages/rs-sdk-ffi/src/identity/names.rs (1)
8-20
: Well-implemented stub function.The stub implementation follows proper FFI conventions:
- Correct
#[no_mangle]
andunsafe extern "C"
attributes- Unused parameters properly prefixed with
_
- Returns appropriate error indicating the feature is not yet implemented
- Proper memory management with
Box::into_raw
packages/rs-dapi-client/src/transport/grpc.rs (1)
618-625
: Consistent gRPC transport implementation.The implementation follows the established pattern for gRPC transport requests:
- Correct usage of the
impl_transport_request_grpc!
macro- Proper mapping of request/response types
- Consistent with other platform gRPC requests
- Method name matches the gRPC service definition
packages/rs-sdk-ffi/src/system/queries/mod.rs (1)
1-14
: Well-organized module structure.The system queries module is well-organized with:
- Clear separation of concerns across 5 submodules
- Consistent naming pattern for re-exported functions
- Complete coverage - all declared modules have corresponding re-exports
- Clean API surface for system-level queries
packages/rs-sdk-ffi/src/data_contract/queries/fetch.rs (2)
40-44
: LGTM! Proper async runtime integration.The async operation is correctly handled using
block_on
to bridge the async SDK call with the synchronous FFI interface. The error mapping from SDK errors to FFI errors is also appropriate.
47-50
:✅ Verification successful
Verify memory management for returned contract handle.
The function returns a raw pointer to a heap-allocated
DataContract
viaBox::into_raw()
. Ensure that the caller properly manages this memory by calling a corresponding cleanup function to avoid memory leaks.
🏁 Script executed:
#!/bin/bash # Description: Check for corresponding cleanup/destroy functions for DataContractHandle # Expected: Should find functions that free DataContractHandle memory ast-grep --pattern 'fn $_(contract_handle: $_) { $$$ Box::from_raw($$$) $$$ }' # Also search for destroy/free functions rg -A 5 "data_contract.*destroy|data_contract.*free" --type rustLength of output: 2960
Memory management verified
Confirmed that returned
DataContractHandle
is properly freed by the existing destroy function:
- packages/rs-sdk-ffi/src/data_contract/mod.rs:
pub unsafe extern "C" fn dash_sdk_data_contract_destroy(handle: *mut DataContractHandle) { if !handle.is_null() { let _ = Box::from_raw(handle as *mut DataContract); } }- Integration tests in packages/rs-sdk-ffi/tests/integration_tests/document.rs call
dash_sdk_data_contract_destroy(contract_handle as *mut DataContractHandle)
(anddestroy_test_sdk_handle(handle)
), ensuring no leaks.No further changes needed.
packages/rs-sdk-ffi/src/identity/queries/balance_and_revision.rs (1)
78-78
: LGTM! Proper string memory management.Using
success_string
withinto_raw()
correctly transfers ownership of the C string to the caller, which is the expected pattern for FFI string returns.packages/rs-sdk-ffi/src/data_contract/util.rs (1)
28-34
: LGTM! Proper JSON serialization.Using
serde_json::to_string()
for schema serialization is the correct approach, much better than manual string formatting. The error handling within this section is also appropriate.packages/rs-sdk-ffi/src/identity/queries/nonce.rs (2)
53-58
: LGTM! Correct async operation and error handling.The async fetch operation is properly implemented with appropriate error handling for both SDK errors and missing nonce scenarios. The tuple field access pattern (
nonce_fetcher.0
) aligns with theIdentityNonceFetcher
type structure.
63-71
: LGTM! Consistent string conversion pattern.The nonce-to-string conversion and C string creation follows the same safe pattern used throughout the codebase, with proper error handling for conversion failures.
packages/rs-sdk-ffi/src/identity/withdraw.rs (3)
40-49
: LGTM! Comprehensive parameter validation.The null pointer checks cover all required parameters appropriately. The validation logic correctly allows
identity_public_key_handle
andput_settings
to be null since they are optional.
73-84
: LGTM! Proper optional parameter handling.The optional parameter logic for both the public key and core fee is correctly implemented, using null checks and zero value checks respectively as documented in the function parameters.
93-104
: LGTM! Correct async withdrawal operation.The withdrawal operation properly uses the
WithdrawFromIdentity
trait with all required parameters. The error handling converts SDK errors to FFI errors appropriately.packages/rs-sdk-ffi/src/identity/queries/balance.rs (1)
51-59
: Excellent async handling and error management.Great implementation using the wrapper's runtime instead of creating a new one, and the error handling with
ok_or_else
is clean and idiomatic.packages/rs-sdk-ffi/src/identity/queries/fetch.rs (1)
52-72
: Well-implemented FFI function with proper error handling.Excellent implementation that:
- Reuses the wrapper's runtime efficiently
- Handles the Option result correctly
- Returns a handle for better performance than JSON serialization
- Provides clear error messages for different failure cases
packages/rs-sdk-ffi/src/identity/queries/by_public_key_hash.rs (3)
51-62
: Good validation of public key hash length.Excellent validation ensuring the hash is exactly 20 bytes and proper conversion to fixed-size array.
64-70
: Clean async implementation with proper error handling.Great use of the wrapper's runtime and clean error mapping from SDK errors to FFI errors.
75-94
:❌ Incorrect review comment
Consider consistency with other identity fetch functions.
This function returns JSON while
dash_sdk_identity_fetch
returns a handle. Consider whether there should be consistency in return types across similar functions.
🏁 Script executed:
#!/bin/bash # Check other identity fetch functions to see return type patterns rg -A 10 "DashSDKResult::" packages/rs-sdk-ffi/src/identity/queries/ | rg -A 2 -B 2 "success_string\|success_handle"Length of output: 116
🏁 Script executed:
#!/bin/bash set -e echo "1) Listing all identity query files:" find packages/rs-sdk-ffi/src/identity/queries -maxdepth 1 -type f -print echo -e "\n2) Searching for DashSDKResult::success_handle in FFI source:" rg -n "DashSDKResult::success_handle" -n packages/rs-sdk-ffi/src echo -e "\n3) Searching for DashSDKResult::success_string in FFI source:" rg -n "DashSDKResult::success_string" -n packages/rs-sdk-ffi/src echo -e "\n4) Searching for dash_sdk_identity_fetch function:" rg -n "dash_sdk_identity_fetch" -n packages/rs-sdk-ffi/srcLength of output: 8466
Ignore return‐type inconsistency flag: this is intentional and consistent
All specialized identity query functions (balance, nonce, public_keys, balance_and_revision, contract_nonce, by_non_unique_public_key_hash, etc.) return their results as JSON strings via DashSDKResult::success_string. Only the generic dash_sdk_identity_fetch in fetch.rs uses success_handle to return a handle to the Identity object. The implementation in by_public_key_hash.rs matches the established pattern—no change needed.
Likely an incorrect or invalid review comment.
packages/rs-sdk-ffi/src/document/queries/info.rs (1)
20-31
: Excellent error handling with proper resource cleanup.The cascading error handling pattern correctly frees previously allocated strings when subsequent allocations fail, preventing memory leaks.
Also applies to: 36-52
packages/rs-sdk-ffi/src/protocol_version/queries/upgrade_state.rs (2)
20-58
: Well-structured FFI function with excellent error handling.The function demonstrates good FFI practices:
- Proper null pointer validation
- Clean separation of concerns with helper function
- Comprehensive error handling for all failure modes
- Correct C string management
81-93
: Good JSON formatting with proper escaping.The manual JSON construction correctly handles the structure and uses proper escaping for string values.
packages/rs-sdk-ffi/src/identity/queries/identities_balances.rs (3)
27-27
: Excellent input validation logic.The validation correctly handles the case where
identity_ids
can be null only ifidentity_ids_len
is zero, which is appropriate for C array semantics.
76-76
: Good performance optimization with Vec::with_capacity.Pre-allocating the vector with the known capacity avoids unnecessary reallocations during the loop.
80-80
:✅ Verification successful
Verify the use of u64::MAX as default balance.
Using
u64::MAX
as the default balance for missing identities might be confusing. Consider using 0 or a specific error value that's documented in the API.
🏁 Script executed:
#!/bin/bash # Check if u64::MAX as default balance is documented or used consistently elsewhere rg -A 3 -B 3 "u64::MAX.*balance|balance.*u64::MAX"Length of output: 1510
No change needed:
u64::MAX
is the intended “not found” sentinelThe Rust FFI layer deliberately uses
u64::MAX
when an identity’s balance is missing, and the Swift SDK checks forUInt64.max
to represent “not found” (seeSDK.swift
around theif entry.balance == UInt64.max
comment). This behavior is consistent and documented in the bridging code, so no API change is required.packages/rs-sdk-ffi/src/data_contract/queries/fetch_many.rs (2)
42-52
: Good input parsing with proper error handling.The comma-separated ID parsing correctly handles trimming whitespace and provides detailed error messages for invalid identifiers.
66-81
: Well-structured JSON construction.The manual JSON building correctly handles the key-value mapping and properly serializes contract objects or null values.
packages/rs-sdk-ffi/build_ios.sh (4)
26-31
: Excellent target management with automatic installation.The
check_target
function elegantly handles missing Rust targets by automatically installing them when needed.
86-90
: Good use of lipo for universal binary creation.The script correctly uses
lipo
to combine x86_64 and arm64 simulator libraries into a universal binary for theuniversal
architecture option.
74-77
: Robust header generation with fallback.The script tries to use the generated header from the build process first, then falls back to running
cbindgen
manually if needed. This provides good resilience.
124-136
: Dynamic XCFramework command construction.The script intelligently builds the
xcodebuild
command based on which libraries were actually built, allowing for flexible architecture configurations.packages/rs-sdk-ffi/src/identity/queries/contract_nonce.rs (1)
24-95
: Well-implemented FFI function with comprehensive safety checks!The function properly handles null pointer validation, string conversions, identifier parsing, and async operations. The error handling is thorough with descriptive error messages at each step.
packages/rs-sdk-ffi/src/identity/queries/public_keys.rs (1)
22-75
: Clean implementation following established FFI patterns!The function correctly uses the
FetchMany
trait for retrieving identity public keys and properly serializes the result to JSON for FFI compatibility.packages/rs-sdk-ffi/Cargo.toml (1)
13-13
: Verify if the "mocks" feature is intended for production.The
dash-sdk
dependency has the "mocks" feature enabled, which is typically used only for testing. This might include unnecessary test code in production builds.Please confirm if this is intentional. If the mocks are only needed for tests, consider moving this to dev-dependencies or using feature flags to conditionally enable mocks only during testing.
packages/rs-sdk-ffi/src/identity/queries/by_non_unique_public_key_hash.rs (1)
24-128
: Excellent handling of optional parameters and FFI return values!The function correctly:
- Validates the 20-byte public key hash requirement
- Handles the optional
start_after
parameter appropriately- Returns a null pointer when no identity is found (line 124), which is the correct FFI pattern
- Provides comprehensive error handling with descriptive messages
packages/rs-sdk-ffi/src/token/burn.rs (2)
119-127
: Clarify where contract validation occurs before skipping it.The comment on line 123 states "skip validation since it's already validated", but it's not clear where this validation happens. The
validate_contract_params
function only checks if parameters are provided, not if the contract content is valid.Consider either:
- Adding a comment explaining where the validation occurs
- Enabling validation if it hasn't been performed elsewhere
- Documenting why skipping validation is safe in this context
175-593
: Excellent comprehensive test coverage!The test suite thoroughly covers:
- All null pointer validation cases
- Invalid parameter handling
- Memory cleanup with proper CString management
- Edge cases with different token positions and amounts
- Proper use of mock data to avoid segfaults during testing
packages/rs-sdk-ffi/cbindgen.toml (1)
1-71
: Well-configured cbindgen settingsThe configuration follows best practices for C FFI bindings with proper include guards, C99 documentation style, consistent naming conventions, and enum variant prefixing to prevent naming conflicts.
packages/rs-sdk-ffi/src/lib.rs (1)
42-66
: Excellent panic hook implementation for FFI safetyThe panic hook properly prevents Rust panics from unwinding across the FFI boundary, which would cause undefined behavior. The implementation handles different payload types and provides useful debugging information.
packages/rs-sdk-ffi/README_NAME_RESOLUTION.md (1)
1-111
: Well-documented DPNS name resolution implementationThis documentation provides a comprehensive explanation of the name resolution functionality, including implementation details, error handling, and usage examples. The technical content is accurate and helpful.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ...me ); ``` ## Parameters -sdk_handle
: A handle to an initialized SDK instance...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Possible missing comma found.
Context: ...act The function queries the DPNS data contract which stores domain documents. Each dom...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~68-~68: Do not use a colon (:) before a series that is introduced by a preposition (‘for’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...unction returns appropriate error codes for: -InvalidParameter
: Null SDK handle, ...(RP_COLON)
packages/rs-sdk-ffi/src/identity/queries/mod.rs (1)
1-27
: Clean module organization!The module structure is well-organized with clear separation of concerns and convenient re-exports of main functions.
packages/rs-sdk-ffi/src/group/queries/action_signers.rs (1)
128-132
:❌ Incorrect review comment
Fix status mapping inconsistency
The documentation states status values are
0=Pending, 1=Completed, 2=Expired
, but the implementation:
- Only handles values 0 and 1
- Maps them to
ActionActive
andActionClosed
, which don't match the documented semanticsEither update the documentation to match the implementation or fix the mapping:
let status = match status { - 0 => GroupActionStatus::ActionActive, - 1 => GroupActionStatus::ActionClosed, + 0 => GroupActionStatus::Pending, + 1 => GroupActionStatus::Completed, + 2 => GroupActionStatus::Expired, _ => return Err("Invalid status value".to_string()), };
🏁 Script executed:
#!/bin/bash # Verify the actual GroupActionStatus enum variants available ast-grep --pattern 'enum GroupActionStatus { $$$ }'Length of output: 55
It looks like the enum definition isn’t being picked up by our previous AST query. Let’s locate it with a simple text search:
🏁 Script executed:
#!/bin/bash # Search for where GroupActionStatus is defined rg -n "enum GroupActionStatus" -n # Also search all usages to see how it’s referenced rg -n "GroupActionStatus" -nLength of output: 15125
Let’s inspect the enum definition and its
TryFrom
implementations to confirm the intended mappings:
🏁 Script executed:
#!/bin/bash # Show enum variants rg -n "pub enum GroupActionStatus" -A10 packages/rs-dpp/src/group/group_action_status.rs # Show TryFrom<u8> mapping logic rg -n "impl TryFrom<u8> for GroupActionStatus" -A10 packages/rs-dpp/src/group/group_action_status.rs # Show TryFrom<i32> mapping logic rg -n "impl TryFrom<i32> for GroupActionStatus" -A10 packages/rs-dpp/src/group/group_action_status.rsLength of output: 1336
No action required: mapping matches
GroupActionStatus
definitionI checked the core enum in
packages/rs-dpp/src/group/group_action_status.rs
and itsTryFrom<u8>
/TryFrom<i32>
impls: only two variants exist—
- 0 ⇒ ActionActive
- 1 ⇒ ActionClosed
The FFI layer in
packages/rs-sdk-ffi/src/group/queries/action_signers.rs
mirrors that exactly, returning an error on any other value. There are noPending
,Completed
, orExpired
variants in the codebase, so the current mapping is correct. If external documentation refers to a 0/1/2 status scheme, it should be updated separately.Likely an incorrect or invalid review comment.
packages/rs-sdk-ffi/src/system/queries/path_elements.rs (1)
1-170
: Well-implemented FFI function!The implementation properly handles all edge cases, has comprehensive error handling, and correctly serializes the various element types. The JSON input/output format is clearly documented.
packages/rs-sdk-ffi/src/identity/queries/resolve.rs (1)
57-58
:⚠️ Potential issueFix unsafe block scope for SDK wrapper dereferencing.
The unsafe dereferencing should be properly scoped within an unsafe block for clarity and safety.
Apply this diff:
- let sdk_wrapper = unsafe { &*(sdk_handle as *const SDKWrapper) }; + let sdk_wrapper = unsafe { &*(sdk_handle as *const SDKWrapper) };Note: While the current code compiles because the entire function is marked
unsafe
, explicit unsafe blocks improve code clarity and safety boundaries.Likely an incorrect or invalid review comment.
packages/rs-drive-proof-verifier/src/proof/token_contract_info.rs (1)
1-57
: Well-implemented proof verification logic.The implementation correctly handles proof extraction, verification, and error propagation. Good use of the
map_drive_error
helper and proper validation of the token ID length.packages/rs-sdk-ffi/src/contested_resource/queries/mod.rs (1)
1-11
: Well-organized module structure!The module cleanly organizes contested resource queries into focused submodules and re-exports the public FFI functions for convenient access. This follows good Rust module organization practices.
packages/rs-sdk-ffi/src/identity/info.rs (1)
19-19
: Document the safety requirements for the identity_handle parameter.The direct cast assumes
identity_handle
points to a validIdentity
object. Consider adding documentation about the expected type and lifetime requirements, or implement a type-safe wrapper to prevent misuse.Add safety documentation:
/// Get identity information +/// +/// # Safety +/// The `identity_handle` must be a valid pointer to an `Identity` object +/// previously created by this FFI layer and not yet destroyed. #[no_mangle] pub unsafe extern "C" fn dash_sdk_identity_get_info(packages/rs-sdk-ffi/src/data_contract/queries/history.rs (1)
70-75
:⚠️ Potential issueIncomplete null pointer validation.
The function returns early after checking only SDK handle and contract_id, but the contract_id is used later without null check in the unsafe block at line 79.
The null check is actually performed, but the error message could be more specific:
-if sdk_handle.is_null() || contract_id.is_null() { +if sdk_handle.is_null() { + return DashSDKResult::error(DashSDKError::new( + DashSDKErrorCode::InvalidParameter, + "SDK handle is null".to_string(), + )); +} +if contract_id.is_null() { return DashSDKResult::error(DashSDKError::new( DashSDKErrorCode::InvalidParameter, - "SDK handle or contract ID is null".to_string(), + "Contract ID is null".to_string(), )); }Likely an incorrect or invalid review comment.
packages/rs-sdk-ffi/src/document/queries/fetch.rs (3)
1-13
: LGTM!The imports are well-organized and all appear to be used in the implementation.
14-77
: Well-implemented FFI function with comprehensive error handling.The function properly validates all input parameters, handles string conversions safely, and manages memory correctly by boxing the result. The async execution pattern using the SDK's runtime is appropriate.
79-295
: Excellent test coverage!The test suite comprehensively covers all edge cases including null pointer validation, invalid input handling, and memory cleanup verification. This ensures the FFI boundary is robust and safe.
packages/rs-sdk-ffi/src/document/queries/search.rs (2)
1-56
: Well-structured FFI types and imports.The
DashSDKDocumentSearchParams
struct is properly defined withrepr(C)
for FFI compatibility, and the JSON helper structs provide clean deserialization interfaces.
58-111
: Helper functions are well-implemented.The operator parsing clearly indicates which operators are not supported, and the JSON to platform value conversion handles all value types appropriately.
packages/rs-sdk-ffi/src/identity/helpers.rs (3)
14-75
: Robust settings conversion with proper null handling.The function correctly handles null pointers and converts all timeout values from milliseconds to Duration objects with appropriate conditional logic.
77-85
: Clean private key parsing implementation.The function properly validates the private key bytes and provides clear error messages on failure.
87-114
: Safe handling of raw data with proper deserialization.The function correctly creates slices from raw pointers using the provided lengths and handles deserialization errors appropriately.
packages/rs-sdk-ffi/src/data_contract/put.rs (3)
8-60
: Well-implemented async FFI function.The function properly validates all parameters, executes the async operation using the SDK's runtime, and serializes the result appropriately.
62-119
: Consistent implementation with proper result handling.The function correctly waits for platform confirmation and returns a handle to the confirmed contract with the appropriate result data type.
121-426
: Comprehensive test coverage for FFI boundaries.The tests thoroughly validate null parameter handling and ensure proper error codes are returned. The approach of testing with mock SDK handles is appropriate for unit tests.
packages/rs-sdk-ffi/src/contested_resource/queries/identity_votes.rs (2)
24-73
: Clean FFI wrapper with proper error handling.The function provides a safe FFI interface with comprehensive documentation and appropriate error handling for CString creation.
159-193
: Basic but adequate test coverage.The tests cover the essential null parameter validation cases. Consider adding tests for invalid identity IDs and other edge cases in the future.
packages/rs-sdk-ffi/src/contested_resource/queries/resources.rs (1)
28-82
: Well-structured FFI function with proper error handling.The function correctly delegates to a safe helper function and properly converts results to C-compatible types.
packages/rs-sdk-ffi/src/signer.rs (2)
86-102
: Proper memory management for signer lifecycle.The create and destroy functions correctly manage memory using Box allocations and deallocations.
104-112
:❓ Verification inconclusive
Verify iOS memory allocation compatibility.
The function assumes iOS allocates memory using malloc/calloc, which may not always be true. Different iOS versions or Swift runtime configurations might use different allocators.
Please verify that the iOS callbacks indeed use malloc-family allocators. You might want to:
- Document this requirement clearly in the iOS integration guide
- Consider passing a deallocation function pointer from iOS to ensure compatibility
- Add runtime checks or debug assertions to detect allocation mismatches
🏁 Script executed:
#!/bin/bash # Search for any documentation about memory allocation requirements fd -e md -e txt | xargs rg -i "malloc|memory.*allocat|ios.*memory" -A 2 -B 2Length of output: 730
🏁 Script executed:
#!/bin/bash # Search for any Swift code allocating memory or using UnsafeMutablePointer fd packages/swift-sdk -e swift | xargs rg -n "UnsafeMutablePointer|malloc"Length of output: 2405
Verify iOS/Swift memory allocator compatibility
Before we merge, please confirm that any pointers passed from Swift into Rust—especially those created with
UnsafeMutablePointer.allocate(capacity:)
—are backed by the malloc family on your iOS targets. While Swift’s default allocator typically maps tomalloc
on Darwin, this isn’t guaranteed across all runtimes or future Swift versions.• packages/swift-sdk/README.md already has a Memory Management section—please expand it to explicitly state the allocator assumption and reference the
dash_sdk_bytes_free
function.
• InSwiftExampleApp/SDK/SDKExtensions.swift
, pointers are created viaUnsafeMutablePointer<UInt8>.allocate(capacity:)
. Verify these may safely be released withlibc::free
.
• For robustness, consider passing a custom deallocator callback from Swift into the Rust layer rather than hard-codinglibc::free
.
• Optionally add a debug‐only assertion indash_sdk_bytes_free
to detect mismatched pointers at runtime.packages/rs-sdk-ffi/src/sdk.rs (1)
40-116
: Well-structured SDK initialization with comprehensive error handling.The function properly validates inputs, creates the runtime, and handles both mock and real SDK configurations.
packages/rs-sdk-ffi/src/document/create.rs (2)
127-429
: Excellent test coverage with comprehensive parameter validation.The tests thoroughly validate all null parameter cases, invalid JSON, and error conditions. The helper functions improve test readability and maintenance.
94-99
:❌ Incorrect review comment
Potential loss of type information in key conversion.
Converting all map keys to
Value::Text
might not preserve the original key types if the platform expects different value types for keys.Verify if the platform always expects text keys or if other key types should be preserved.
🏁 Script executed:
#!/bin/bash # Search for Value::Map usage patterns in the codebase ast-grep --pattern 'Value::Map($_)' # Check if there are examples of non-text keys rg "Value::Map" -A 5 -B 5 | rg -v "Value::Text"Length of output: 80969
No key type preservation needed—keys are always strings
AllValue::Map
usages in the codebase construct keys from RustString
values (or types that convert into text), and they’re consistently wrapped asValue::Text
. There are no instances where non‐text keys are expected or used as map keys, so converting(k, v)
to(Value::Text(k), v)
does not lose any type information.Likely an incorrect or invalid review comment.
packages/rs-sdk-ffi/src/document/mod.rs (1)
1-36
: LGTM!The module organization is clean and follows Rust best practices. Good use of encapsulation by keeping the
util
module private while re-exporting its public functions.packages/rs-sdk-ffi/src/document/purchase.rs (1)
1-715
: LGTM!Excellent implementation of the document purchase FFI functions with:
- Comprehensive parameter validation
- Proper unsafe pointer handling
- Good error propagation from Rust to FFI
- Thorough unit test coverage including edge cases
packages/rs-sdk-ffi/src/identity/transfer.rs (1)
1-118
: LGTM!Clean implementation of identity credit transfer with proper:
- Optional parameter handling for the signing key
- Result structure with appropriate free function
- Error handling and conversion
packages/rs-sdk-ffi/src/identity/mod.rs (1)
1-46
: Module structure is well-organized!The identity module provides a clean hierarchical structure with logical grouping of submodules and re-exports. The separation of concerns across different operation types (create, queries, topup, transfer, etc.) enhances maintainability.
packages/rs-sdk-ffi/src/token/emergency_action.rs (3)
22-44
: Good parameter validation!The function properly validates all required parameters for null pointers before proceeding. This is essential for FFI safety.
84-186
: Well-structured async implementation with proper error handlingThe implementation correctly:
- Uses the runtime to block on async operations
- Handles both contract fetching and deserialization paths
- Applies the builder pattern appropriately
- Propagates errors with meaningful messages
188-641
: Comprehensive test coverage!The test suite provides excellent coverage including:
- Null parameter validation for all inputs
- Different action types (Pause/Resume)
- Edge cases like public notes and serialized contracts
- Various token positions
- Proper memory cleanup
The helper functions for creating mocks are well-structured.
packages/rs-sdk-ffi/src/identity/topup.rs (1)
13-38
: Proper null pointer validationThe function correctly validates all required parameters before proceeding with unsafe operations.
packages/rs-sdk-ffi/src/document/price.rs (3)
22-49
: Good parameter validation patternThe function follows the established pattern of validating all required parameters for null pointers.
61-122
: Well-implemented state transition builderThe implementation correctly:
- Extracts user fee increase from settings
- Uses the builder pattern with optional parameters
- Properly serializes the state transition
- Handles errors appropriately
230-607
: Excellent test coverage!The test suite comprehensively covers:
- Null parameter validation for all inputs
- Boundary value testing (zero and maximum prices)
- Both update functions (with and without wait)
- Proper memory management and cleanup
The helper functions and test structure follow best practices.
packages/rs-sdk-ffi/src/document/helpers.rs (2)
15-22
: Simple and correct enum conversionThe function provides a straightforward mapping between FFI and Rust enum variants.
62-95
: Well-implemented options conversionThe function correctly:
- Handles null pointers
- Converts 0 values to None for optional fields
- Properly constructs the nested signing options
packages/rs-sdk-ffi/src/token/destroy_frozen_funds.rs (1)
187-539
: Excellent test coverage!The test module provides comprehensive coverage for null pointer scenarios, parameter validation, and edge cases. The helper functions and cleanup routines demonstrate good memory management practices.
packages/rs-sdk-ffi/src/error.rs (2)
7-44
: Well-structured error definitions.The error codes cover all necessary categories and the struct documentation clearly indicates memory ownership responsibilities.
151-163
: Correct memory management implementation.The
dash_sdk_error_free
function properly handles null checks and deallocates both the error struct and message string.packages/rs-sdk-ffi/src/token/freeze.rs (1)
190-709
: Comprehensive test coverage with realistic SDK handles.The tests provide excellent coverage and use actual SDK handle creation/destruction, making them more realistic than pure mocks. The memory management and cleanup are handled properly throughout.
packages/rs-sdk-ffi/src/token/config_update.rs (1)
240-667
: Well-structured test coverage.The tests comprehensively cover null parameter scenarios, different update types, and various configuration options. Good memory management practices are followed throughout.
packages/rs-sdk-ffi/src/token/claim.rs (1)
177-570
: Excellent test coverage.The tests comprehensively cover null parameter scenarios, different distribution types, and edge cases. The use of actual SDK handles and proper cleanup demonstrates good testing practices.
packages/rs-sdk-ffi/src/document/put.rs (2)
79-80
: Verify document revision logic for new documents.The code assumes that
revision == 1
indicates a new document that should useDocumentCreateTransitionBuilder
. However, documents might haverevision == 0
orNone
when they're new.Please verify that newly created documents always have revision 1, or consider checking for revision <= 1 or None:
- let state_transition = if document.revision().unwrap_or(0) == 1 { + let state_transition = if document.revision().unwrap_or(1) <= 1 {Also applies to: 218-219
310-647
: Excellent test coverage!The comprehensive unit tests cover all null parameter scenarios and both create/replace paths. This is a good example of thorough FFI testing.
packages/rs-sdk-ffi/src/document/replace.rs (1)
1-628
: Well-structured implementation with comprehensive tests.The document replacement operations are implemented correctly with:
- Proper null pointer validation
- Consistent error handling
- Appropriate use of
DocumentReplaceTransitionBuilder
- Comprehensive test coverage for all error scenarios
packages/rs-sdk-ffi/src/document/delete.rs (1)
217-574
: Comprehensive test coverage for deletion operations.The tests thoroughly cover all null parameter scenarios and edge cases. Good adherence to testing best practices.
Ok(format!( | ||
"{{\"balance\":{},\"revision\":{}}}", | ||
balance_and_revision.0, // balance | ||
balance_and_revision.1 | ||
)) // revision | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve JSON formatting with proper serialization.
The manual JSON string formatting is fragile and could break with special characters. Consider using proper JSON serialization instead.
- // Return as JSON string
- Ok(format!(
- "{{\"balance\":{},\"revision\":{}}}",
- balance_and_revision.0, // balance
- balance_and_revision.1
- )) // revision
+ // Return as JSON string using proper serialization
+ use serde_json::json;
+ let json_value = json!({
+ "balance": balance_and_revision.0,
+ "revision": balance_and_revision.1
+ });
+ Ok(json_value.to_string())
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Ok(format!( | |
"{{\"balance\":{},\"revision\":{}}}", | |
balance_and_revision.0, // balance | |
balance_and_revision.1 | |
)) // revision | |
}); | |
// Return as JSON string using proper serialization | |
use serde_json::json; | |
let json_value = json!({ | |
"balance": balance_and_revision.0, | |
"revision": balance_and_revision.1 | |
}); | |
Ok(json_value.to_string()) | |
}); |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/identity/queries/balance_and_revision.rs around lines
61 to 66, the code manually formats a JSON string which is fragile and
error-prone. Replace the manual string formatting with proper JSON serialization
by creating a struct or map representing the balance and revision, then
serialize it using a JSON library like serde_json to ensure correct and safe
JSON output.
if contract_handle.is_null() || document_type.is_null() { | ||
return std::ptr::null_mut(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider consistent error handling pattern.
This function returns null
on errors, which differs from other FFI functions in this codebase that return structured DashSDKResult
with detailed error information. This inconsistency could make the API harder to use and debug.
Consider refactoring to use the same error handling pattern:
-) -> *mut c_char {
+) -> DashSDKResult {
if contract_handle.is_null() || document_type.is_null() {
- return std::ptr::null_mut();
+ return DashSDKResult::error(DashSDKError::new(
+ DashSDKErrorCode::InvalidParameter,
+ "Contract handle or document type is null".to_string(),
+ ));
}
Also applies to: 22-22, 31-31, 33-33, 36-36
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/data_contract/util.rs around lines 14 to 16 and also
lines 22, 31, 33, and 36, the function returns null pointers on error, which is
inconsistent with other FFI functions that return a structured DashSDKResult
containing detailed error information. Refactor these error returns to use the
DashSDKResult pattern by returning an error result with appropriate error codes
and messages instead of null pointers, ensuring consistent and informative error
handling across the API.
let withdraw_address = | ||
match Address::<dashcore::address::NetworkUnchecked>::from_str(address_str) { | ||
Ok(addr) => addr.assume_checked(), | ||
Err(e) => { | ||
return DashSDKResult::error(DashSDKError::new( | ||
DashSDKErrorCode::InvalidParameter, | ||
format!("Invalid Dash address: {}", e), | ||
)) | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify address validation security.
The address parsing uses assume_checked()
which bypasses network validation. Ensure this is intentional and that the address is validated elsewhere in the call chain.
🏁 Script executed:
#!/bin/bash
# Description: Check if address network validation occurs elsewhere
# Expected: Should find network validation in SDK or caller code
rg -A 5 -B 5 "assume_checked|network.*valid" --type rust packages/rs-sdk-ffi/
Length of output: 989
Ensure Dash address network is validated
I didn’t find any explicit network validation in the rs-sdk-ffi
crate, so using
Address::<dashcore::address::NetworkUnchecked>::from_str(...).assume_checked()
will bypass network checks and may accept addresses for the wrong network. Please add validation here or switch to the checked network API:
Locations to update:
- packages/rs-sdk-ffi/src/identity/withdraw.rs:61–70
Suggested fixes:
- let withdraw_address =
- match Address::<dashcore::address::NetworkUnchecked>::from_str(address_str) {
- Ok(addr) => addr.assume_checked(),
+ let withdraw_address =
+ match Address::<dashcore::address::Network>::from_str(address_str) {
+ Ok(addr) => addr,
Err(e) => {
return DashSDKResult::error(DashSDKError::new(
DashSDKErrorCode::InvalidParameter,
format!("Invalid Dash address: {}", e),
))
}
};
Or, if you must use NetworkUnchecked
, explicitly check addr.network()
before calling assume_checked()
.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let withdraw_address = | |
match Address::<dashcore::address::NetworkUnchecked>::from_str(address_str) { | |
Ok(addr) => addr.assume_checked(), | |
Err(e) => { | |
return DashSDKResult::error(DashSDKError::new( | |
DashSDKErrorCode::InvalidParameter, | |
format!("Invalid Dash address: {}", e), | |
)) | |
} | |
}; | |
let withdraw_address = | |
match Address::<dashcore::address::Network>::from_str(address_str) { | |
Ok(addr) => addr, | |
Err(e) => { | |
return DashSDKResult::error(DashSDKError::new( | |
DashSDKErrorCode::InvalidParameter, | |
format!("Invalid Dash address: {}", e), | |
)) | |
} | |
}; |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/identity/withdraw.rs around lines 61 to 70, the code
uses
Address::<dashcore::address::NetworkUnchecked>::from_str(address_str).assume_checked(),
which bypasses network validation and risks accepting addresses from the wrong
network. To fix this, replace NetworkUnchecked with the appropriate checked
network type for Dash, or if you must use NetworkUnchecked, explicitly verify
that addr.network() matches the expected Dash network before calling
assume_checked(). This ensures only valid Dash network addresses are accepted.
let rt = tokio::runtime::Runtime::new() | ||
.map_err(|e| format!("Failed to create Tokio runtime: {}", e))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid creating new Tokio runtime for each call.
Creating a new Tokio runtime for every FFI call is inefficient and can cause performance issues. The runtime should be reused from the SDK wrapper.
- let rt = tokio::runtime::Runtime::new()
- .map_err(|e| format!("Failed to create Tokio runtime: {}", e))?;
+ let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) };
+ let sdk = wrapper.sdk.clone();
- let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) };
- let sdk = wrapper.sdk.clone();
-
- rt.block_on(async move {
+ wrapper.runtime.block_on(async move {
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/system/queries/total_credits_in_platform.rs at lines
65-66, avoid creating a new Tokio runtime for each call as it is inefficient.
Instead, refactor the code to reuse an existing Tokio runtime instance from the
SDK wrapper by passing it as a parameter or accessing a shared runtime, ensuring
that the runtime is created once and reused across calls.
let rt = tokio::runtime::Runtime::new() | ||
.map_err(|e| format!("Failed to create Tokio runtime: {}", e))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Reuse existing runtime instead of creating new one.
Same issue as in other files - creating a new Tokio runtime for each call is inefficient. Use the runtime from the SDK wrapper.
- let rt = tokio::runtime::Runtime::new()
- .map_err(|e| format!("Failed to create Tokio runtime: {}", e))?;
+ let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) };
+ let sdk = wrapper.sdk.clone();
- let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) };
- let sdk = wrapper.sdk.clone();
-
- rt.block_on(async move {
+ wrapper.runtime.block_on(async move {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let rt = tokio::runtime::Runtime::new() | |
.map_err(|e| format!("Failed to create Tokio runtime: {}", e))?; | |
// instead of creating a fresh runtime every time, reuse the one in the SDKWrapper | |
let wrapper = unsafe { &*(sdk_handle as *const crate::sdk::SDKWrapper) }; | |
let sdk = wrapper.sdk.clone(); | |
wrapper.runtime.block_on(async move { | |
// … the rest of your async logic … | |
}) |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/group/queries/info.rs at lines 76-77, avoid creating
a new Tokio runtime for each call as it is inefficient. Instead, refactor the
code to reuse the existing Tokio runtime provided by the SDK wrapper. Locate
where the SDK wrapper exposes the runtime and use that instance to execute async
tasks rather than creating a new runtime here.
let resources_json: Vec<String> = contested_resources.0 | ||
.iter() | ||
.map(|resource| { | ||
format!( | ||
r#"{{"id":"{}","contract_id":"{}","document_type_name":"{}","index_name":"{}","index_values":"{}"}}"#, | ||
bs58::encode(resource.0.to_identifier_bytes().unwrap_or_else(|_| vec![0u8; 32])).into_string(), | ||
bs58::encode(contract_id.as_bytes()).into_string(), | ||
document_type_name_str, | ||
index_name_str, | ||
"[]" | ||
) | ||
}) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix error handling and incorrect hardcoded value.
Two issues in the resource JSON formatting:
- Line 204: Using
unwrap_or_else
with a fallbackvec![0u8; 32]
could mask actual errors - Line 208: The hardcoded
"[]"
forindex_values
doesn't reflect the actual index values from the resource
Apply this fix:
.iter()
.map(|resource| {
+ let id_bytes = resource.0.to_identifier_bytes()
+ .map_err(|e| format!("Failed to get identifier bytes: {}", e))?;
format!(
r#"{{"id":"{}","contract_id":"{}","document_type_name":"{}","index_name":"{}","index_values":"{}"}}"#,
- bs58::encode(resource.0.to_identifier_bytes().unwrap_or_else(|_| vec![0u8; 32])).into_string(),
+ bs58::encode(&id_bytes).into_string(),
bs58::encode(contract_id.as_bytes()).into_string(),
document_type_name_str,
index_name_str,
- "[]"
+ serde_json::to_string(&resource.1).unwrap_or_else(|_| "[]".to_string())
)
})
- .collect();
+ .collect::<Result<Vec<_>, String>>()?;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let resources_json: Vec<String> = contested_resources.0 | |
.iter() | |
.map(|resource| { | |
format!( | |
r#"{{"id":"{}","contract_id":"{}","document_type_name":"{}","index_name":"{}","index_values":"{}"}}"#, | |
bs58::encode(resource.0.to_identifier_bytes().unwrap_or_else(|_| vec![0u8; 32])).into_string(), | |
bs58::encode(contract_id.as_bytes()).into_string(), | |
document_type_name_str, | |
index_name_str, | |
"[]" | |
) | |
}) | |
.collect(); | |
let resources_json: Vec<String> = contested_resources.0 | |
.iter() | |
.map(|resource| { | |
let id_bytes = resource.0.to_identifier_bytes() | |
.map_err(|e| format!("Failed to get identifier bytes: {}", e))?; | |
format!( | |
r#"{{"id":"{}","contract_id":"{}","document_type_name":"{}","index_name":"{}","index_values":"{}"}}"#, | |
bs58::encode(&id_bytes).into_string(), | |
bs58::encode(contract_id.as_bytes()).into_string(), | |
document_type_name_str, | |
index_name_str, | |
serde_json::to_string(&resource.1).unwrap_or_else(|_| "[]".to_string()) | |
) | |
}) | |
.collect::<Result<Vec<_>, String>>()?; |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/contested_resource/queries/resources.rs around lines
199 to 211, the code improperly handles errors by using unwrap_or_else with a
default byte vector, which can hide real errors, and it hardcodes the
index_values field as "[]". To fix this, replace unwrap_or_else with proper
error handling that propagates or logs the error instead of defaulting silently,
and update the index_values field to serialize the actual index values from the
resource rather than using a fixed empty array string.
.map_err(|e| FFIError::InternalError(format!("Failed to create factory: {}", e)))?; | ||
|
||
// Get identity nonce | ||
let identity_nonce = identity.revision() as u64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential integer overflow in nonce calculation.
Converting identity.revision()
to u64
could overflow if the revision is larger than u64::MAX
. Consider using checked conversion.
- let identity_nonce = identity.revision() as u64;
+ let identity_nonce = identity.revision()
+ .try_into()
+ .map_err(|_| FFIError::InternalError("Identity revision too large for nonce".to_string()))?;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let identity_nonce = identity.revision() as u64; | |
// replace the unchecked cast with a fallible conversion | |
let identity_nonce = identity.revision() | |
.try_into() | |
.map_err(|_| FFIError::InternalError("Identity revision too large for nonce".to_string()))?; |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/data_contract/mod.rs at line 87, the direct cast of
identity.revision() to u64 risks integer overflow if the revision exceeds
u64::MAX. Replace the cast with a checked conversion method like try_into() or
checked conversion, handle the potential error or overflow case appropriately to
ensure safe conversion without panics or data loss.
// SAFETY: We've verified all pointers are non-null above | ||
// However, we cannot validate if they point to valid memory without dereferencing | ||
// For test safety, we should create proper mock handles instead of using arbitrary values | ||
let wrapper = unsafe { &mut *(sdk_handle as *mut SDKWrapper) }; | ||
let identity_public_key = unsafe { &*(identity_public_key_handle as *const IdentityPublicKey) }; | ||
let signer = unsafe { &*(signer_handle as *const crate::signer::IOSSigner) }; | ||
let params = unsafe { &*params }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsafe pointer dereferencing needs careful handling in tests
While the null checks are in place, the comment correctly identifies that we cannot validate if pointers point to valid memory. The test code uses arbitrary values like 1 as *mut SDKHandle
which could cause undefined behavior if accidentally dereferenced.
Consider creating proper mock wrappers that never dereference invalid pointers in test scenarios, or use a test-specific trait that provides safe mocking capabilities.
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/token/emergency_action.rs around lines 60 to 66, the
unsafe pointer dereferencing uses arbitrary values in tests that may lead to
undefined behavior. To fix this, replace these arbitrary pointer values with
proper mock handles or implement a test-specific trait that safely mocks the
required behavior without dereferencing invalid pointers. This ensures test
safety while maintaining the necessary interface for the unsafe code.
(DashSDKErrorCode::NotFound, error_str) | ||
} else { | ||
// Default to network error with the original message | ||
(DashSDKErrorCode::NetworkError, format!("Failed to fetch balances: {}", error_str)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic error message incorrectly assumes context.
The error message format "Failed to fetch balances: {}"
assumes the error is always related to fetching balances, but this error conversion is used throughout the entire SDK for various operations.
- (DashSDKErrorCode::NetworkError, format!("Failed to fetch balances: {}", error_str))
+ (DashSDKErrorCode::NetworkError, error_str)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
(DashSDKErrorCode::NetworkError, format!("Failed to fetch balances: {}", error_str)) | |
(DashSDKErrorCode::NetworkError, error_str) |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/error.rs at line 127, the error message "Failed to
fetch balances: {}" incorrectly assumes the error context is always related to
fetching balances, which is not true for this generic error conversion used
across the SDK. Update the error message to be more generic or context-neutral,
such as "Network error occurred: {}", to accurately reflect that the error could
arise from various operations.
DashSDKTokenConfigUpdateType::ManualMinting => { | ||
// Note: This would need proper implementation based on the actual SDK types | ||
// For now, return an error indicating this needs implementation | ||
return Err(FFIError::InternalError( | ||
"ManualMinting config update not yet implemented".to_string() | ||
)); | ||
} | ||
DashSDKTokenConfigUpdateType::ManualBurning => { | ||
return Err(FFIError::InternalError( | ||
"ManualBurning config update not yet implemented".to_string() | ||
)); | ||
} | ||
DashSDKTokenConfigUpdateType::Freeze => { | ||
return Err(FFIError::InternalError( | ||
"Freeze config update not yet implemented".to_string() | ||
)); | ||
} | ||
DashSDKTokenConfigUpdateType::Unfreeze => { | ||
return Err(FFIError::InternalError( | ||
"Unfreeze config update not yet implemented".to_string() | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Document or remove unimplemented update types.
Several update types (ManualMinting, ManualBurning, Freeze, Unfreeze) return runtime errors indicating they're not implemented. This could cause confusion for API users who expect these features to work.
Consider either:
- Documenting these limitations clearly in the FFI header/documentation
- Removing these enum values from the FFI interface until they're implemented
- Implementing the missing functionality
Let me check if these update types are referenced elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for usage of the unimplemented update types
rg -A 2 -B 2 "(ManualMinting|ManualBurning|Freeze|Unfreeze)" --type rust
Length of output: 76299
Document or remove unimplemented token‐config update types
In packages/rs-sdk-ffi/src/token/config_update.rs
(lines 164–185), the variants
DashSDKTokenConfigUpdateType::ManualMinting
, ManualBurning
, Freeze
and Unfreeze
all return FFIError::InternalError
because they lack any underlying implementation.
This will surface as surprising runtime errors for API consumers.
Consider one of the following fixes:
- Remove these enum values from the FFI interface until the SDK supports them
- Clearly mark them as “unsupported” in the FFI header and Rust doc comments
- Implement the missing contract‐configuration update logic in the SDK
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/token/config_update.rs between lines 164 and 185, the
enum variants ManualMinting, ManualBurning, Freeze, and Unfreeze return runtime
errors due to lack of implementation, which can confuse API users. To fix this,
either remove these enum variants from the FFI interface until their
functionality is implemented, or add clear documentation in the FFI header and
Rust doc comments marking these update types as unsupported. Alternatively,
implement the missing logic for these update types if feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/tests-rs-sdk-ffi-build.yml (2)
6-9
: Include workspace configuration files in path filters.Changes to
Cargo.toml
orCargo.lock
at the repo root (e.g., addingrs-sdk-ffi
to the workspace) won’t trigger this workflow as-is. Extend bothpull_request
andpush
path filters to include these files:on: pull_request: paths: + - 'Cargo.toml' + - 'Cargo.lock' - 'packages/rs-sdk-ffi/**' - 'packages/rs-sdk/**' - '.github/workflows/tests-rs-sdk-ffi-build.yml' ...Also applies to: 15-17
57-57
: Add a newline at end of file.POSIX-compliant text files should end with a newline character.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 57-57: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/package-filters/rs-packages.yml
(1 hunks).github/workflows/tests-rs-sdk-ffi-build.yml
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/package-filters/rs-packages.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
🪛 actionlint (1.7.7)
.github/workflows/tests-rs-sdk-ffi-build.yml
13-13: character '' is invalid for branch and tag names. only special characters [, ?, +, *, , ! can be escaped with . see man git-check-ref-format
for more details. note that regular expression is unavailable. note: filter pattern syntax is explained at https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet
(glob)
🪛 YAMLlint (1.37.1)
.github/workflows/tests-rs-sdk-ffi-build.yml
[error] 57-57: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (182)
- GitHub Check: Rust packages (dashpay-contract) / Detect immutable structure changes
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Formatting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (wallet-utils-contract) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Formatting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (wallet-utils-contract) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Formatting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (wallet-utils-contract) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Formatting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Unused dependencies
- GitHub Check: Rust packages (dashpay-contract) / Formatting
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dpns-contract) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
push: | ||
branches: | ||
- master | ||
- 'v[0-9]+\.[0-9]+-dev' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix branch filter pattern to use glob syntax.
The string 'v[0-9]+\.[0-9]+-dev'
is interpreted as a literal glob and won’t match branches like v1.2-dev
. Replace it with a valid glob, for example:
- - 'v[0-9]+\.[0-9]+-dev'
+ - 'v*.*-dev'
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
- 'v[0-9]+\.[0-9]+-dev' | |
- 'v*.*-dev' |
🧰 Tools
🪛 actionlint (1.7.7)
13-13: character '' is invalid for branch and tag names. only special characters [, ?, +, *, , ! can be escaped with . see man git-check-ref-format
for more details. note that regular expression is unavailable. note: filter pattern syntax is explained at https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet
(glob)
🤖 Prompt for AI Agents
In .github/workflows/tests-rs-sdk-ffi-build.yml at line 13, the branch filter
pattern uses a regex-like string 'v[0-9]+\.[0-9]+-dev' which is treated as a
literal glob and does not match branches like 'v1.2-dev'. Replace this pattern
with a valid glob pattern that matches the intended branches, such as 'v*-dev'
or a similar glob expression that correctly matches versioned dev branches.
- name: Install cross-compilation dependencies | ||
run: | | ||
# Install osxcross or other cross-compilation tools if needed | ||
# For now, we'll just add the target | ||
rustup target add aarch64-apple-darwin | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Install the full cross-compilation toolchain.
Simply adding the Rust target isn’t sufficient to link for aarch64-apple-darwin
. You should install or configure the necessary system dependencies (e.g., a cross-linker, clang, SDK headers):
- run: |
- # Install osxcross or other cross-compilation tools if needed
- # For now, we'll just add the target
- rustup target add aarch64-apple-darwin
+ run: |
+ sudo apt-get update
+ sudo apt-get install -y clang cmake libssl-dev xz-utils
+ # (Optional) Install osxcross toolchain here if your setup requires it.
+ rustup target add aarch64-apple-darwin
🤖 Prompt for AI Agents
In .github/workflows/tests-rs-sdk-ffi-build.yml around lines 36 to 41, the
workflow only adds the Rust target for aarch64-apple-darwin but does not install
the full cross-compilation toolchain. Update the script to install or configure
all necessary system dependencies such as a cross-linker, clang, and SDK headers
required for linking to that target, ensuring the build can complete
successfully.
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Documentation
Chores
.gitignore
to cover Swift and Rust build artifacts.rs-sdk-ffi
package in the build context.rs-sdk-ffi
targeting Apple ARM64 platform.