refactor: rs-platform-wallet-ffi error framework#3566
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (69)
📝 WalkthroughWalkthroughReplaces the two-channel FFI error model (return code + Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
ZocoLini
changed the title
ZocoLini
changed the title
Collaborator
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs (1)
118-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winZeroize partially derived keys on the error cleanup path.
cleanup(rows)frees previously derived rows without wipingprivate_key_wiforprivate_key_bytes, so a mid-loop failure can leave private key material in heap pages after return.🔒 Proposed fix
- let cleanup = |rows: Vec<IdentityKeyPreviewFFI>| { - for row in rows { + let cleanup = |rows: Vec<IdentityKeyPreviewFFI>| { + for mut row in rows { if !row.derivation_path.is_null() { let _ = CString::from_raw(row.derivation_path); } if !row.public_key.is_null() && row.public_key_len > 0 { let _ = Vec::from_raw_parts(row.public_key, row.public_key_len, row.public_key_len); } if !row.private_key_wif.is_null() { - let _ = CString::from_raw(row.private_key_wif); + let mut wif = CString::from_raw(row.private_key_wif).into_bytes_with_nul(); + zeroize::Zeroize::zeroize(&mut wif); + row.private_key_wif = std::ptr::null_mut(); } + zeroize::Zeroize::zeroize(&mut row.private_key_bytes); } };Also applies to: 137-157, 167-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs` around lines 118 - 130, The cleanup closure for IdentityKeyPreviewFFI currently frees strings and buffers but does not wipe secret material; update the cleanup (and the other similar cleanup blocks handling IdentityKeyPreviewFFI) to securely zero the private_key_bytes buffer and the private_key_wif bytes before freeing them: for any non-null private_key_bytes (and private_key_bytes_len > 0) overwrite the raw pointer bytes with zeros (or use a secure_zero function) then call Vec::from_raw_parts to free, and for private_key_wif convert the raw C string bytes to a slice, overwrite those bytes with zeros, then call CString::from_raw to free; ensure you handle lengths safely and avoid double-free by doing the zeroing right before reclaiming ownership of the allocation.packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs (1)
48-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate
key_idrows instead of overwriting them.
keys_map.insert(row.key_id, ...)silently drops the earlier entry when the FFI array repeats akey_id. That means the registration can proceed with a different key set than the caller passed in, which is hard to diagnose and can leave the identity misconfigured.Suggested fix
- keys_map.insert( - row.key_id, - IdentityPublicKey::V0(IdentityPublicKeyV0 { - id: row.key_id, - purpose, - security_level, - contract_bounds: None, - key_type, - read_only: row.read_only, - data: BinaryData::new(pubkey_bytes), - disabled_at: None, - }), - ); + if keys_map + .insert( + row.key_id, + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: row.key_id, + purpose, + security_level, + contract_bounds: None, + key_type, + read_only: row.read_only, + data: BinaryData::new(pubkey_bytes), + disabled_at: None, + }), + ) + .is_some() + { + return PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidParameter, + format!("identity_pubkeys[{i}] reuses key_id {}", row.key_id), + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs` around lines 48 - 74, The loop that builds keys_map currently overwrites duplicate row.key_id entries; change it to detect duplicates (e.g., check keys_map.contains_key(&row.key_id) or use BTreeMap::entry) and return a PlatformWalletFfiResult::err when a duplicate is found instead of inserting, including the offending row index (i) and key id in the error message; update the error code to an appropriate variant (e.g., ErrorInvalidArgument or a new ErrorDuplicateKeyId) so callers can diagnose repeated key_id entries coming from pubkey_rows rather than silently overwriting earlier IdentityPublicKeyV0 values.packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs (1)
148-183:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject oversized
mnemonic_lenbefore slicing the callback buffer.The resolver controls
mnemonic_len, but the code slices&mnemonic_buf[..mnemonic_len]without checking it againstMNEMONIC_RESOLVER_BUFFER_CAPACITY. A bad or buggy callback can panic thisextern "C"function.Suggested fix
match rc { x if x == mnemonic_resolver_result::SUCCESS => {} x if x == mnemonic_resolver_result::NOT_FOUND => { return PlatformWalletFfiResult::err( PlatformWalletFfiResultCode::ErrorWalletOperation, @@ } } + if mnemonic_len > MNEMONIC_RESOLVER_BUFFER_CAPACITY { + return PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorWalletOperation, + "mnemonic resolver: returned length larger than buffer capacity", + ); + } + let mnemonic_str = unwrap_result_or_return!(std::str::from_utf8(&mnemonic_buf[..mnemonic_len]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs` around lines 148 - 183, The code is slicing mnemonic_buf with mnemonic_len provided by the external resolver without validating it; add a bounds check after the resolver_vtable.resolve call to ensure mnemonic_len <= MNEMONIC_RESOLVER_BUFFER_CAPACITY (and treat any value > capacity as an error) before calling std::str::from_utf8(&mnemonic_buf[..mnemonic_len]); if the check fails return a PlatformWalletFfiResult::err (e.g., PlatformWalletFfiResultCode::ErrorWalletOperation) with a clear message like "mnemonic resolver: reported length exceeds buffer capacity"; reference the symbols mnemonic_len, mnemonic_buf, MNEMONIC_RESOLVER_BUFFER_CAPACITY and resolver_vtable.resolve when making this change.packages/rs-platform-wallet-ffi/src/spv.rs (1)
199-227:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed configured peers instead of silently skipping them.
Bad UTF-8 or an unparsable peer address is currently dropped and the function still returns
Success. Withrestrict_to_configured_peers = true, that can leave SPV starting with an empty or partial peer set and fail much later with no actionable FFI error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/spv.rs` around lines 199 - 227, The code currently silently drops malformed peer entries when building peer_list (CStr::from_ptr and p.parse()) and still returns Success, which can break SPV when restrict_to_configured_peers is true; update the peer parsing logic inside PLATFORM_WALLET_MANAGER_STORAGE.with_item (and where peer_list is consumed to populate config.peers) to validate each peer: if CStr::from_ptr fails UTF-8 or p.parse() fails, return an explicit FFI error/result (propagate a failure from the with_item callback) instead of skipping the entry so the FFI caller receives an error when configured peers are malformed.packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs (1)
303-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate
key_idrows before building the placeholder identity.
BTreeMap::insertoverwrites an existing entry, so duplicateidentity_pubkeys[*].key_idvalues currently succeed and silently change the key set being registered.Suggested fix
- keys_map.insert( + if keys_map + .insert( row.key_id, IdentityPublicKey::V0(IdentityPublicKeyV0 { id: row.key_id, purpose, security_level, contract_bounds, key_type, read_only: row.read_only, data: BinaryData::new(pubkey_bytes), disabled_at: None, }), - ); + ) + .is_some() + { + return PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidParameter, + format!("identity_pubkeys[{i}].key_id {} is duplicated", row.key_id), + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs` around lines 303 - 329, The loop over pubkey_rows currently uses keys_map.insert which overwrites duplicate identity_pubkeys[*].key_id entries; before inserting (inside the for (i, row) in pubkey_rows.iter().enumerate() loop, prior to keys_map.insert), detect if keys_map already contains row.key_id and return an appropriate PlatformWalletFfiResult error (including index i and the duplicate key_id in the message) to reject duplicate key_id rows rather than silently overwriting them; ensure the check uses the same key identifier (row.key_id) and returns the same error handling pattern used elsewhere (e.g., PlatformWalletFfiResult::err with a descriptive message).
🧹 Nitpick comments (2)
packages/rs-platform-wallet-ffi/src/established_contact.rs (1)
245-251: 💤 Low valueDestroy function returns generic error instead of
ErrorInvalidHandle.The
unwrap_option_or_return!macro likely returns a generic error code. For consistency with other destroy functions (e.g.,contact_request_destroy,managed_identity_destroy), consider using the explicit pattern that returnsErrorInvalidHandle.♻️ Suggested fix for consistent destroy error handling
pub unsafe extern "C" fn established_contact_destroy( contact_handle: Handle, ) -> PlatformWalletFfiResult { - let option = ESTABLISHED_CONTACT_STORAGE.remove(contact_handle); - let _ = unwrap_option_or_return!(option); - PlatformWalletFfiResult::ok() + if ESTABLISHED_CONTACT_STORAGE.remove(contact_handle).is_some() { + PlatformWalletFfiResult::ok() + } else { + PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidHandle, + "Invalid established contact handle", + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/established_contact.rs` around lines 245 - 251, The destroy function established_contact_destroy currently uses unwrap_option_or_return!(option) which yields a generic error; change it to the explicit pattern used by contact_request_destroy and managed_identity_destroy: remove the handle from ESTABLISHED_CONTACT_STORAGE into an Option, return PlatformWalletFfiResult::err(ErrorInvalidHandle) if None, otherwise proceed and return PlatformWalletFfiResult::ok(); ensure you reference ESTABLISHED_CONTACT_STORAGE and ErrorInvalidHandle in the check so the function consistently returns ErrorInvalidHandle for invalid handles.packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs (1)
17-22: 💤 Low valueInconsistent error handling for invalid handle in destroy function.
Unlike
contact_request_destroy(which returnsErrorInvalidHandlewhen the handle doesn't exist), this function always returnsok()even ifhandleis invalid. This inconsistency could mask bugs where callers pass stale or invalid handles.Consider aligning with the pattern used elsewhere:
♻️ Suggested fix for consistent destroy behavior
pub unsafe extern "C" fn platform_address_wallet_destroy( handle: Handle, ) -> PlatformWalletFfiResult { - PLATFORM_ADDRESS_WALLET_STORAGE.remove(handle); - PlatformWalletFfiResult::ok() + if PLATFORM_ADDRESS_WALLET_STORAGE.remove(handle).is_some() { + PlatformWalletFfiResult::ok() + } else { + PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidHandle, + "Invalid platform address wallet handle", + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs` around lines 17 - 22, The destroy function platform_address_wallet_destroy currently always returns PlatformWalletFfiResult::ok() even when removing a non-existent handle; change it to mirror the contact_request_destroy pattern by checking the result of PLATFORM_ADDRESS_WALLET_STORAGE.remove(handle) and returning an invalid-handle error when the handle was not found (e.g., return PlatformWalletFfiResult::err_invalid_handle() or the same ErrorInvalidHandle variant used elsewhere) and only return ok() when removal succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/asset_lock/build.rs`:
- Around line 25-52: Before any early-return (including all uses of
unwrap_option_or_return! and unwrap_result_or_return!) ensure the out parameters
are cleared: set *out_tx_bytes to null (0), *out_tx_len to 0, and
*out_private_key to zeroed bytes/empty value so callers won't see stale
pointers/lengths; do this at the top of the function and before every return
path that can occur during ASSET_LOCK_MANAGER_STORAGE.with_item(...).
Specifically update the function that calls
ASSET_LOCK_MANAGER_STORAGE.with_item(...).build_asset_lock_transaction and the
analogous function covering lines 75-109 to zero out out_tx_bytes, out_tx_len,
and out_private_key before calling parse_funding_type, before invoking the
storage closure, and immediately before any unwrap_* early returns.
In `@packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs`:
- Around line 46-93: Before calling ASSET_LOCK_MANAGER_STORAGE.with_item (the
fallible lookup), set *out_count = 0 and *out_locks = std::ptr::null_mut() so
the outputs are always initialized even if unwrap_option_or_return!(option)
returns early; then proceed with the existing lookup and only overwrite
out_count/out_locks after entries are successfully created (the relevant symbols
are out_locks, out_count, unwrap_option_or_return!(option), and
ASSET_LOCK_MANAGER_STORAGE.with_item).
In `@packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- Line 62: core_wallet_send_to_addresses currently parses addresses using
dashcore::Address::from_str(...).assume_checked(), which skips network
validation and can allow cross-network addresses; replace this by parsing
without assume_checked and explicitly validating each Address against the wallet
network (use the same pattern as identity_withdrawal.rs — call the address
network check helper, e.g., require_network or an equivalent method, passing
wallet.network()) before using the address to build the transaction; update any
unwrap_result_or_return! usage to propagate parse errors and return early if the
address fails network validation.
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 145-155: The blanket impl From<Option<T>> for
PlatformWalletFfiResult turns every None into
PlatformWalletFfiResultCode::NotFound, which masks invalid-handle cases (used by
unwrap_option_or_return!) and changes the ABI; revert handling by removing or
narrowing this generic impl and instead ensure call sites differentiate
handle-misses from domain-not-found: keep explicit checks where handles are
validated (e.g. where unwrap_option_or_return! is used for manager/wallet
lookups) to return PlatformWalletFfiResultCode::ErrorInvalidHandle, or add a
separate helper/From<Option<T>> implementation specifically for storage-domain
lookups (e.g. from_storage_option_to_ffi_result) that returns NotFound while
leaving handle-related conversions to return ErrorInvalidHandle at the call
site; update uses of unwrap_option_or_return! to call the appropriate explicit
check or helper.
In `@packages/rs-platform-wallet-ffi/src/identity_key_preview.rs`:
- Around line 219-225: The loop currently uses
start_index.saturating_add(offset) which causes duplicate identity_index values
once the range hits u32::MAX; replace the saturating add with a checked add and
fail fast: in the for offset in 0..count loop compute identity_index via
start_index.checked_add(offset) and return/propagate ErrorInvalidParameter (or
the function's appropriate error path) if checked_add returns None, then call
build_row(identity_index) only for valid indices; this preserves the API
contract and prevents duplicate keys.
In `@packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- Around line 41-47: The current construction of add_keys (in the block that
reads add_public_keys and add_public_keys_count when building
Vec<IdentityPublicKey>) treats a NULL pointer as an empty Vec even if
add_public_keys_count > 0; change this to validate inputs: if
add_public_keys_count == 0 return Vec::new(), but if add_public_keys is null and
add_public_keys_count > 0 return an error (or propagate/abort) instead of
silently creating an empty Vec; apply the same validation pattern to the
corresponding disable keys block (the code handling
disable_public_keys/disable_public_keys_count) so any NULL pointer paired with a
non-zero count is rejected rather than ignored, referencing the
IdentityPubkeyFFI -> IdentityPublicKey conversion logic in those blocks.
In `@packages/rs-platform-wallet-ffi/src/memory_explorer.rs`:
- Around line 137-140: Replace the unguarded `i + 1` used when computing
`last_scanned_index` (from the
`info.identity_manager.highest_registration_index(&wallet_id)` result) with a
saturating increment to avoid overflow from a `u32::MAX` FFI value;
specifically, call `saturating_add(1)` on the returned `u32` before assigning to
`last_scanned_index` so the value safely stays at `u32::MAX` instead of
wrapping.
In `@packages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rs`:
- Around line 38-69: Initialize the out_changeset to a known empty/default
PlatformAddressChangeSetFFI immediately after check_ptr!(out_changeset) to
ensure it is always safe to free if the function returns early; in the transfer
function set *out_changeset to a default/empty PlatformAddressChangeSetFFI
(e.g., PlatformAddressChangeSetFFI::default() or an explicit empty constructor)
before any fallible calls like parse_outputs, parse_input_selection, or
wallet.transfer so callers never receive uninitialized/stale output.
In `@packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs`:
- Around line 17-20: Initialize the out parameter to NULL_HANDLE before any
early returns: set *out_handle = NULL_HANDLE at the top of each FFI entry (the
functions that call check_ptr!(seed_bytes) and check_ptr!(out_handle)) so a
reused caller variable cannot retain a stale handle; do this in the same
functions referenced around the check_ptr! calls (the blocks using seed_bytes
and out_handle) and repeat the same initialization for the other similar
entrypoints noted (the ones around lines 66-68 and 123-125) so every path that
returns early leaves *out_handle as NULL_HANDLE.
In `@packages/rs-platform-wallet-ffi/src/xpub_render.rs`:
- Around line 28-36: Reset the out-param before any fallible return: move or add
the assignment that clears *out_string (currently present after the checks) so
it occurs before validating inputs (e.g., before or immediately after check_ptr!
and prior to the bytes_len == 0 check) or ensure every early return path
(including the PlatformWalletFfiResult::err return in the bytes_len == 0 branch)
sets *out_string = ptr::null_mut();; update references in xpub_render.rs where
bytes, bytes_len, out_string and the PlatformWalletFfiResult::err return are
used so the out param is always nullified on error.
In `@packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs`:
- Around line 230-231: The FFI calls (e.g.,
managed_identity_get_established_contact_ids) are being invoked without checking
the returned PlatformWalletFfiResult; update each call site to capture the
returned result (e.g., let result =
managed_identity_get_established_contact_ids(...)) and assert that result.code
indicates success before touching out parameters like established_array or
consuming handles/buffers; do the same for all other listed call sites (238-239,
246-247, 280-283, 569-598, 664-668, 689-696, 729-739, 833-839) so failures are
asserted early and you avoid using invalid handles or buffers.
In `@packages/rs-platform-wallet-ffi/tests/integration_tests.rs`:
- Around line 289-293: The calls to managed_identity_set_label and
identity_manager_add_identity ignore their PlatformWalletFfiResult return
values; update the test to capture those returns (from
managed_identity_set_label(...) and identity_manager_add_identity(...)) and
assert they indicate success (e.g., equal to PlatformWalletFfiResult::Ok or via
the crate's success check) before proceeding so failures are reported
immediately and with a clear message.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs`:
- Around line 148-183: The code is slicing mnemonic_buf with mnemonic_len
provided by the external resolver without validating it; add a bounds check
after the resolver_vtable.resolve call to ensure mnemonic_len <=
MNEMONIC_RESOLVER_BUFFER_CAPACITY (and treat any value > capacity as an error)
before calling std::str::from_utf8(&mnemonic_buf[..mnemonic_len]); if the check
fails return a PlatformWalletFfiResult::err (e.g.,
PlatformWalletFfiResultCode::ErrorWalletOperation) with a clear message like
"mnemonic resolver: reported length exceeds buffer capacity"; reference the
symbols mnemonic_len, mnemonic_buf, MNEMONIC_RESOLVER_BUFFER_CAPACITY and
resolver_vtable.resolve when making this change.
In `@packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs`:
- Around line 118-130: The cleanup closure for IdentityKeyPreviewFFI currently
frees strings and buffers but does not wipe secret material; update the cleanup
(and the other similar cleanup blocks handling IdentityKeyPreviewFFI) to
securely zero the private_key_bytes buffer and the private_key_wif bytes before
freeing them: for any non-null private_key_bytes (and private_key_bytes_len > 0)
overwrite the raw pointer bytes with zeros (or use a secure_zero function) then
call Vec::from_raw_parts to free, and for private_key_wif convert the raw C
string bytes to a slice, overwrite those bytes with zeros, then call
CString::from_raw to free; ensure you handle lengths safely and avoid
double-free by doing the zeroing right before reclaiming ownership of the
allocation.
In
`@packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs`:
- Around line 48-74: The loop that builds keys_map currently overwrites
duplicate row.key_id entries; change it to detect duplicates (e.g., check
keys_map.contains_key(&row.key_id) or use BTreeMap::entry) and return a
PlatformWalletFfiResult::err when a duplicate is found instead of inserting,
including the offending row index (i) and key id in the error message; update
the error code to an appropriate variant (e.g., ErrorInvalidArgument or a new
ErrorDuplicateKeyId) so callers can diagnose repeated key_id entries coming from
pubkey_rows rather than silently overwriting earlier IdentityPublicKeyV0 values.
In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs`:
- Around line 303-329: The loop over pubkey_rows currently uses keys_map.insert
which overwrites duplicate identity_pubkeys[*].key_id entries; before inserting
(inside the for (i, row) in pubkey_rows.iter().enumerate() loop, prior to
keys_map.insert), detect if keys_map already contains row.key_id and return an
appropriate PlatformWalletFfiResult error (including index i and the duplicate
key_id in the message) to reject duplicate key_id rows rather than silently
overwriting them; ensure the check uses the same key identifier (row.key_id) and
returns the same error handling pattern used elsewhere (e.g.,
PlatformWalletFfiResult::err with a descriptive message).
In `@packages/rs-platform-wallet-ffi/src/spv.rs`:
- Around line 199-227: The code currently silently drops malformed peer entries
when building peer_list (CStr::from_ptr and p.parse()) and still returns
Success, which can break SPV when restrict_to_configured_peers is true; update
the peer parsing logic inside PLATFORM_WALLET_MANAGER_STORAGE.with_item (and
where peer_list is consumed to populate config.peers) to validate each peer: if
CStr::from_ptr fails UTF-8 or p.parse() fails, return an explicit FFI
error/result (propagate a failure from the with_item callback) instead of
skipping the entry so the FFI caller receives an error when configured peers are
malformed.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- Around line 245-251: The destroy function established_contact_destroy
currently uses unwrap_option_or_return!(option) which yields a generic error;
change it to the explicit pattern used by contact_request_destroy and
managed_identity_destroy: remove the handle from ESTABLISHED_CONTACT_STORAGE
into an Option, return PlatformWalletFfiResult::err(ErrorInvalidHandle) if None,
otherwise proceed and return PlatformWalletFfiResult::ok(); ensure you reference
ESTABLISHED_CONTACT_STORAGE and ErrorInvalidHandle in the check so the function
consistently returns ErrorInvalidHandle for invalid handles.
In `@packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs`:
- Around line 17-22: The destroy function platform_address_wallet_destroy
currently always returns PlatformWalletFfiResult::ok() even when removing a
non-existent handle; change it to mirror the contact_request_destroy pattern by
checking the result of PLATFORM_ADDRESS_WALLET_STORAGE.remove(handle) and
returning an invalid-handle error when the handle was not found (e.g., return
PlatformWalletFfiResult::err_invalid_handle() or the same ErrorInvalidHandle
variant used elsewhere) and only return ok() when removal succeeded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 175d633a-9f9a-48eb-bcf2-6d4c7f9d17f8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/src/asset_lock/build.rspackages/rs-platform-wallet-ffi/src/asset_lock/manager.rspackages/rs-platform-wallet-ffi/src/asset_lock/sync.rspackages/rs-platform-wallet-ffi/src/contact.rspackages/rs-platform-wallet-ffi/src/contact_request.rspackages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/wallet.rspackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/dashpay_profile.rspackages/rs-platform-wallet-ffi/src/data_contract.rspackages/rs-platform-wallet-ffi/src/derivation.rspackages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rspackages/rs-platform-wallet-ffi/src/dpns.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/established_contact.rspackages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rspackages/rs-platform-wallet-ffi/src/identity_discovery.rspackages/rs-platform-wallet-ffi/src/identity_key_preview.rspackages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rspackages/rs-platform-wallet-ffi/src/identity_manager.rspackages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_sync.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/identity_transfer.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet-ffi/src/identity_withdrawal.rspackages/rs-platform-wallet-ffi/src/managed_identity.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/memory_explorer.rspackages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet-ffi/src/platform_addresses/mod.rspackages/rs-platform-wallet-ffi/src/platform_addresses/sync.rspackages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rspackages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rspackages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rspackages/rs-platform-wallet-ffi/src/platform_wallet_info.rspackages/rs-platform-wallet-ffi/src/spv.rspackages/rs-platform-wallet-ffi/src/tokens/burn.rspackages/rs-platform-wallet-ffi/src/tokens/claim.rspackages/rs-platform-wallet-ffi/src/tokens/destroy_frozen_funds.rspackages/rs-platform-wallet-ffi/src/tokens/freeze.rspackages/rs-platform-wallet-ffi/src/tokens/group_info.rspackages/rs-platform-wallet-ffi/src/tokens/group_queries.rspackages/rs-platform-wallet-ffi/src/tokens/mint.rspackages/rs-platform-wallet-ffi/src/tokens/pause.rspackages/rs-platform-wallet-ffi/src/tokens/purchase.rspackages/rs-platform-wallet-ffi/src/tokens/resume.rspackages/rs-platform-wallet-ffi/src/tokens/set_price.rspackages/rs-platform-wallet-ffi/src/tokens/transfer.rspackages/rs-platform-wallet-ffi/src/tokens/unfreeze.rspackages/rs-platform-wallet-ffi/src/tokens/update_config.rspackages/rs-platform-wallet-ffi/src/utils.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet-ffi/src/xpub_render.rspackages/rs-platform-wallet-ffi/tests/comprehensive_tests.rspackages/rs-platform-wallet-ffi/tests/integration_tests.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/EstablishedContact.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerIdentitySync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenActions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenGroupActionQueries.swift
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "5ceb7e17b52a84e232061181445d9ea86bccb298be86b3ad8677e20c16791c17"
)Xcode manual integration:
|
ZocoLini
force-pushed
the
refactor/error-fw-pw
branch
from
30d4af7 to
6654a23
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs (1)
349-378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate
identity_pubkeys[*].key_idvalues.
keys_map.insert(...)silently overwrites an earlier row with the same key id, so malformed FFI input can register a different key set than the caller persisted. Fail fast when an insert replaces an existing entry instead of accepting the last one wins.Proposed fix
- keys_map.insert( + if keys_map + .insert( row.key_id, IdentityPublicKey::V0(IdentityPublicKeyV0 { id: row.key_id, purpose, security_level, contract_bounds, key_type, read_only: row.read_only, data: BinaryData::new(pubkey_bytes), disabled_at: None, }), - ); + ) + .is_some() + { + return PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidParameter, + format!("identity_pubkeys[{i}].key_id {} is duplicated", row.key_id), + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs` around lines 349 - 378, The loop over identity_pubkeys currently uses keys_map.insert(...) which silently overwrites duplicate key_id values; change it to detect duplicates before insertion by checking keys_map.contains_key(&row.key_id) (or using entry API) and immediately return a PlatformWalletFfiResult::err with an appropriate PlatformWalletFfiResultCode (e.g., ErrorInvalidArgument or a new duplicate key error) and a clear message including the duplicate key_id and the index i (and optionally which input array "identity_pubkeys") instead of inserting; update the code path around keys_map.insert and the surrounding IdentityPublicKey::V0 construction to perform this duplicate check and fail fast on duplicates.packages/rs-platform-wallet-ffi/src/manager.rs (2)
108-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsupported
account_optionsshould fail instead of silently usingDefault.Both constructors currently coerce unknown discriminants to
WalletAccountCreationOptions::Default. That hides FFI caller bugs/version skew and can create a wallet layout the caller did not request. ReturnErrorInvalidParameterfor any unsupported value.Also applies to: 158-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/manager.rs` around lines 108 - 112, The match over account_options currently coerces unknown discriminants into WalletAccountCreationOptions::Default; change it to reject unsupported values by returning ErrorInvalidParameter instead of silently using Default. Specifically, update the match that assigns accounts (matching on account_options and producing WalletAccountCreationOptions) so the 0 and 1 arms map to WalletAccountCreationOptions::None and ::Default respectively, and the _ arm returns or propagates an ErrorInvalidParameter (e.g., via Err(...) or an early return) so callers receive an error for invalid discriminants; apply the same fix to the second occurrence that handles account_options at the other site (the block referenced at 158-160).
105-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWipe the copied seed buffer before returning.
This creates a full
[u8; 64]copy of the caller's seed and leaves it resident after both success and error paths. Wrap it in a zeroizing guard so the temporary buffer is scrubbed on scope exit.Proposed fix
- let mut seed = [0u8; 64]; - std::ptr::copy_nonoverlapping(seed_bytes, seed.as_mut_ptr(), 64); + let mut seed = zeroize::Zeroizing::new([0u8; 64]); + std::ptr::copy_nonoverlapping(seed_bytes, seed.as_mut_ptr(), 64); @@ - runtime().block_on(manager.create_wallet_from_seed_bytes(network, seed, accounts)) + runtime().block_on(manager.create_wallet_from_seed_bytes(network, *seed, accounts))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/manager.rs` around lines 105 - 123, The temporary 64-byte seed buffer (seed) created from seed_bytes is left in memory after create_wallet_from_seed_bytes returns; wrap seed in a zeroizing guard so it is securely zeroed on scope exit (both success and error paths). Replace the plain array with a zeroize-on-drop type (e.g., zeroize::Zeroizing<[u8;64]>) or otherwise ensure seed is dropped/zeroed immediately after PLATFORM_WALLET_MANAGER_STORAGE.with_item(...) / create_wallet_from_seed_bytes completes; keep the same flow around PLATFORM_WALLET_MANAGER_STORAGE.with_item(manager_handle, |manager| runtime().block_on(manager.create_wallet_from_seed_bytes(...))) and then proceed to extract wallet_id/handle, but ensure the zeroizing wrapper is used so the buffer is scrubbed when it goes out of scope.packages/rs-platform-wallet-ffi/tests/integration_tests.rs (1)
140-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest assertion will fail: label stub returns null.
The test at Lines 140-144 asserts that
label_ptris not null and attempts to read a string from it, but themanaged_identity_get_labelfunction is documented as a stub that returns null (seemanaged_identity.rsLine 76). This test will fail at theassert!(!label_ptr.is_null())line.🐛 Suggested fix
let mut label_ptr: *mut std::os::raw::c_char = std::ptr::null_mut(); let result = managed_identity_get_label(handle, &mut label_ptr); assert_eq!(result.code, PlatformWalletFfiResultCode::Success); - assert!(!label_ptr.is_null()); - - let retrieved_label = std::ffi::CStr::from_ptr(label_ptr).to_str().unwrap(); - assert_eq!(retrieved_label, "Test Identity"); - - platform_wallet_string_free(label_ptr); + // Label stub returns null — labels live on PersistentIdentity.alias. + assert!(label_ptr.is_null());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/tests/integration_tests.rs` around lines 140 - 145, The test assumes managed_identity_get_label returns a non-null C string but that function is a documented stub that returns null; update the integration test (around the assertions on result.code, label_ptr and retrieved_label) to handle the stub: keep the assert that result.code == PlatformWalletFfiResultCode::Success, then either assert that label_ptr.is_null() (and skip the CStr conversion) or branch so that if label_ptr.is_null() you don't call CStr::from_ptr, otherwise convert and assert the string equals "Test Identity"; reference managed_identity_get_label and the label_ptr/retrieved_label locals when making the change.
♻️ Duplicate comments (5)
packages/rs-platform-wallet-ffi/src/xpub_render.rs (1)
28-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
*out_stringbefore any fallible return.
*out_string = ptr::null_mut()on line 36 is only reached after bothcheck_ptr!andbytes_lenchecks pass. If either check fails,out_stringretains its previous value. A caller reusing the same variable across calls may read/free stale data.Proposed fix: null out_string immediately after validation
pub unsafe extern "C" fn platform_wallet_account_xpub_to_string( bytes: *const u8, bytes_len: usize, out_string: *mut *mut c_char, ) -> PlatformWalletFfiResult { - check_ptr!(bytes); check_ptr!(out_string); + *out_string = ptr::null_mut(); + check_ptr!(bytes); if bytes_len == 0 { return PlatformWalletFfiResult::err( PlatformWalletFfiResultCode::ErrorInvalidParameter, "bytes_len is zero", ); } - *out_string = ptr::null_mut();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/xpub_render.rs` around lines 28 - 36, The out_string pointer must be nulled before any fallible return so callers don't hold stale pointers; move or add the assignment *out_string = ptr::null_mut(); immediately after the check_ptr! validations (i.e., after the check_ptr!(bytes); check_ptr!(out_string); lines) and before any code that can return PlatformWalletFfiResult::err (such as the bytes_len == 0 check), ensuring every early return leaves *out_string set to null.packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs (1)
52-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the wallet's configured network when decoding the private key.
dashcore::Network::Mainnetstill hardcodes mainnet semantics here, so testnet/devnet/regtest wallets will deserialize the caller's key with the wrong network metadata. Please derive the network from the wallet behindhandleinstead of baking mainnet into this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs` around lines 52 - 55, The code currently hardcodes dashcore::Network::Mainnet when calling dashcore::PrivateKey::from_byte_array to decode the private key; change this to use the wallet's configured network obtained from the wallet behind handle (e.g., read the network/chain setting from the Wallet/Handle object you already have access to) and pass that network value into dashcore::PrivateKey::from_byte_array instead of dashcore::Network::Mainnet so testnet/devnet/regtest keys deserialize with the correct network metadata.packages/rs-platform-wallet-ffi/src/identity_key_preview.rs (1)
219-225:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't saturate the preview index sequence.
start_index.saturating_add(offset)silently repeatsu32::MAXonce the range overflows, so callers can get duplicate preview rows instead of a clear parameter error. A checked add keeps the API honest here.Proposed fix
for offset in 0..count { - // Saturating add: the discovery scan caps identity - // indices well below u32::MAX in practice; if a caller - // intentionally passes near-max values we simply repeat - // the cap rather than wrap. - let identity_index = start_index.saturating_add(offset); + let identity_index = match start_index.checked_add(offset) { + Some(value) => value, + None => { + free_rows(rows); + return Err(PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidParameter, + "start_index + count exceeds u32::MAX", + )); + } + }; match build_row(identity_index) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_key_preview.rs` around lines 219 - 225, The loop currently uses start_index.saturating_add(offset) which can silently repeat u32::MAX; change it to use start_index.checked_add(offset) inside the for offset in 0..count loop (the place where identity_index is computed and passed to build_row). If checked_add returns None, return a clear parameter error from this function (or propagate an appropriate error variant) instead of producing duplicate preview rows; otherwise use the checked sum as identity_index and continue calling build_row.packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs (1)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
*out_handletoNULL_HANDLEbefore any early return.These entrypoints still only write the handle on success. On any earlier failure, a reused caller variable can keep a stale handle and later use or destroy the wrong object.
Suggested fix
pub unsafe extern "C" fn platform_wallet_info_create_from_seed( @@ ) -> PlatformWalletFfiResult { - check_ptr!(seed_bytes); check_ptr!(out_handle); + *out_handle = NULL_HANDLE; + check_ptr!(seed_bytes);pub unsafe extern "C" fn platform_wallet_info_create_from_mnemonic( @@ ) -> PlatformWalletFfiResult { - check_ptr!(mnemonic); check_ptr!(out_handle); + *out_handle = NULL_HANDLE; + check_ptr!(mnemonic);pub unsafe extern "C" fn platform_wallet_info_get_identity_manager( @@ ) -> PlatformWalletFfiResult { check_ptr!(out_handle); + *out_handle = NULL_HANDLE;Also applies to: 67-68, 124-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs` around lines 17 - 19, Reset the caller-provided handle to NULL_HANDLE on function entry so any early return cannot leave a stale handle: after validating pointers (e.g., after check_ptr!(seed_bytes); check_ptr!(out_handle);) write *out_handle = NULL_HANDLE before any further work or early returns; apply the same pattern in the other affected functions/locations that use out_handle (the spots around the check_ptr! calls at the other ranges) so only successful paths later overwrite out_handle with a live handle.packages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rs (1)
38-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-clear
out_changesetbefore the first fallible branch.This still has early returns before Line 69 writes
*out_changeset, so a reused caller variable can retain a stale/freeable changeset on failure.Suggested fix
check_ptr!(out_changeset); check_ptr!(signer_address_handle); + *out_changeset = PlatformAddressChangeSetFFI::default(); let output_map = unwrap_result_or_return!(parse_outputs(outputs, outputs_count));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rs` around lines 38 - 69, The out_changeset pointer must be cleared before any fallible operations so a caller's reused variable can't retain a stale/freeable value; after check_ptr!(out_changeset) but before calling parse_outputs/parse_input_selection/parse_fee_strategy or any unwrap_* macros, set *out_changeset to a safe empty/default PlatformAddressChangeSetFFI (e.g. PlatformAddressChangeSetFFI::default() or an explicit empty/zeroed instance) so that in any early-return path (unwrap_result_or_return!, unwrap_option_or_return!, etc.) the caller sees a valid empty changeset rather than a stale one; apply this change in the function containing out_changeset, VTableSigner usage and PLATFORM_ADDRESS_WALLET_STORAGE.with_item/runtime().block_on(wallet.transfer).
🧹 Nitpick comments (2)
packages/rs-platform-wallet-ffi/src/managed_identity.rs (1)
341-352: 💤 Low valueTests should assert FFI success before continuing.
Several test functions discard the FFI result without verifying success. If the call fails, the test continues with uninitialized or stale data, making failures harder to diagnose.
♻️ Suggested fix
#[test] fn test_get_balance() { unsafe { let identity = create_test_identity(); let managed = platform_wallet::ManagedIdentity::new(identity, 0); let handle = MANAGED_IDENTITY_STORAGE.insert(managed); let mut balance: u64 = 0; - let _ = managed_identity_get_balance(handle, &mut balance); + let result = managed_identity_get_balance(handle, &mut balance); + assert_eq!(result.code, PlatformWalletFfiResultCode::Success); managed_identity_destroy(handle); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/managed_identity.rs` around lines 341 - 352, The test calls (e.g., test_get_balance) currently ignore the FFI return value from managed_identity_get_balance, which can leave balance uninitialized; update the test to capture the FFI return (e.g., let res = managed_identity_get_balance(handle, &mut balance);) and assert success (e.g., assert_eq!(res, 0) or assert!(is_success(res))) before using balance or calling managed_identity_destroy; apply the same pattern to other tests that call FFI functions so every FFI call’s result is checked immediately after invocation.packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs (1)
793-797: 💤 Low valueInconsistent destroy error mapping for
established_contact_destroy.The comment says
established_contact_destroyusesunwrap_option_or_return!and returnsNotFound, but other destroy functions likemanaged_identity_destroyandcontact_request_destroyreturnErrorInvalidHandlefor invalid handles. This inconsistency might confuse callers.Consider aligning the destroy semantics: either all destroy functions should return
ErrorInvalidHandlefor invalid handles (asmanaged_identity_destroydoes), or document whyestablished_contact_destroydiffers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs` around lines 793 - 797, The test and comment reveal an inconsistent error mapping: established_contact_destroy currently uses unwrap_option_or_return! and yields PlatformWalletFfiResultCode::NotFound for an invalid handle, whereas managed_identity_destroy and contact_request_destroy return ErrorInvalidHandle; update established_contact_destroy so an invalid/bogus handle returns PlatformWalletFfiResultCode::ErrorInvalidHandle to match the other destroy functions (either remove or replace the unwrap_option_or_return! usage with the same invalid-handle check/return logic used by managed_identity_destroy/contact_request_destroy, or map the NotFound outcome to ErrorInvalidHandle before returning).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs`:
- Around line 20-29: After the check_ptr!(out_address) in both
core_wallet_next_receive_address_for_account and
core_wallet_next_change_address, immediately set *out_address =
std::ptr::null_mut() before performing any fallible calls (e.g.,
CORE_WALLET_STORAGE.with_item, runtime().block_on, CString::new) so a stale
pointer is never left behind if the function returns early; apply the same
initialization in both functions and add std::ptr import if not already present.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- Around line 48-60: The code unsafely calls std::slice::from_raw_parts on
potentially null pointers when count == 0 and dereferences address entries
without null-checks; update the function to early-return an error if count == 0
(or explicitly create empty slices when count == 0) before calling
from_raw_parts, and add a null-check for each entry in the addresses array
(addr_ptrs[i]) prior to calling std::ffi::CStr::from_ptr in the loop that builds
outputs; ensure amounts, out_tx_bytes and out_tx_len are still checked via
check_ptr! and preserve construction of outputs and amount_slice only after
validating count and non-null address pointers.
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 53-61: The unwrap_option_or_return! macro double-evaluates $expr;
change it to evaluate $expr once by binding it to a temporary (e.g., let __val =
$expr;) and then match or use let Some(v) = __val { ... } else { return
__val.into(); } so the expression is stored and reused; update the macro body
(keep the macro name unwrap_option_or_return) to use a uniquely named temp to
avoid hygiene collisions and ensure the None branch returns the bound value
converted with into().
In `@packages/rs-platform-wallet-ffi/src/identity_discovery.rs`:
- Around line 70-71: Update the doc comment in identity_discovery.rs to remove
the stale reference to `out_error` and `PlatformWalletFFIError`; locate the doc
block for the function (the comment containing “out_error — populated on
failure...”) and delete or rewrite that sentence to reflect the current
signature and error handling used by the surrounding function (e.g., the
function defined near `discover_identities`/`identity_discovery`), ensuring the
docs no longer mention `out_error` or `PlatformWalletFFIError`.
In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs`:
- Around line 504-583: The cleanup closure currently frees derivation_path,
public_key and private_key_wif but does not zeroize the secret bytes; update
cleanup (and any early-error branches that free a pub_ptr or drop an ext_priv)
to securely overwrite each IdentityKeyPreviewFFI.private_key_bytes with zeros
before dropping memory, e.g. obtain a mutable view of the private_key_bytes
array and fill it with 0 (or call Zeroize) prior to releasing/public pointer
deallocation; ensure this is applied for all places that call cleanup(rows) and
the error branches around derive_identity_auth_keypair and the WIF CString
creation so no derived private key material remains in memory after an error.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs`:
- Around line 349-378: The loop over identity_pubkeys currently uses
keys_map.insert(...) which silently overwrites duplicate key_id values; change
it to detect duplicates before insertion by checking
keys_map.contains_key(&row.key_id) (or using entry API) and immediately return a
PlatformWalletFfiResult::err with an appropriate PlatformWalletFfiResultCode
(e.g., ErrorInvalidArgument or a new duplicate key error) and a clear message
including the duplicate key_id and the index i (and optionally which input array
"identity_pubkeys") instead of inserting; update the code path around
keys_map.insert and the surrounding IdentityPublicKey::V0 construction to
perform this duplicate check and fail fast on duplicates.
In `@packages/rs-platform-wallet-ffi/src/manager.rs`:
- Around line 108-112: The match over account_options currently coerces unknown
discriminants into WalletAccountCreationOptions::Default; change it to reject
unsupported values by returning ErrorInvalidParameter instead of silently using
Default. Specifically, update the match that assigns accounts (matching on
account_options and producing WalletAccountCreationOptions) so the 0 and 1 arms
map to WalletAccountCreationOptions::None and ::Default respectively, and the _
arm returns or propagates an ErrorInvalidParameter (e.g., via Err(...) or an
early return) so callers receive an error for invalid discriminants; apply the
same fix to the second occurrence that handles account_options at the other site
(the block referenced at 158-160).
- Around line 105-123: The temporary 64-byte seed buffer (seed) created from
seed_bytes is left in memory after create_wallet_from_seed_bytes returns; wrap
seed in a zeroizing guard so it is securely zeroed on scope exit (both success
and error paths). Replace the plain array with a zeroize-on-drop type (e.g.,
zeroize::Zeroizing<[u8;64]>) or otherwise ensure seed is dropped/zeroed
immediately after PLATFORM_WALLET_MANAGER_STORAGE.with_item(...) /
create_wallet_from_seed_bytes completes; keep the same flow around
PLATFORM_WALLET_MANAGER_STORAGE.with_item(manager_handle, |manager|
runtime().block_on(manager.create_wallet_from_seed_bytes(...))) and then proceed
to extract wallet_id/handle, but ensure the zeroizing wrapper is used so the
buffer is scrubbed when it goes out of scope.
In `@packages/rs-platform-wallet-ffi/tests/integration_tests.rs`:
- Around line 140-145: The test assumes managed_identity_get_label returns a
non-null C string but that function is a documented stub that returns null;
update the integration test (around the assertions on result.code, label_ptr and
retrieved_label) to handle the stub: keep the assert that result.code ==
PlatformWalletFfiResultCode::Success, then either assert that
label_ptr.is_null() (and skip the CStr conversion) or branch so that if
label_ptr.is_null() you don't call CStr::from_ptr, otherwise convert and assert
the string equals "Test Identity"; reference managed_identity_get_label and the
label_ptr/retrieved_label locals when making the change.
---
Duplicate comments:
In `@packages/rs-platform-wallet-ffi/src/identity_key_preview.rs`:
- Around line 219-225: The loop currently uses
start_index.saturating_add(offset) which can silently repeat u32::MAX; change it
to use start_index.checked_add(offset) inside the for offset in 0..count loop
(the place where identity_index is computed and passed to build_row). If
checked_add returns None, return a clear parameter error from this function (or
propagate an appropriate error variant) instead of producing duplicate preview
rows; otherwise use the checked sum as identity_index and continue calling
build_row.
In
`@packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs`:
- Around line 52-55: The code currently hardcodes dashcore::Network::Mainnet
when calling dashcore::PrivateKey::from_byte_array to decode the private key;
change this to use the wallet's configured network obtained from the wallet
behind handle (e.g., read the network/chain setting from the Wallet/Handle
object you already have access to) and pass that network value into
dashcore::PrivateKey::from_byte_array instead of dashcore::Network::Mainnet so
testnet/devnet/regtest keys deserialize with the correct network metadata.
In `@packages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rs`:
- Around line 38-69: The out_changeset pointer must be cleared before any
fallible operations so a caller's reused variable can't retain a stale/freeable
value; after check_ptr!(out_changeset) but before calling
parse_outputs/parse_input_selection/parse_fee_strategy or any unwrap_* macros,
set *out_changeset to a safe empty/default PlatformAddressChangeSetFFI (e.g.
PlatformAddressChangeSetFFI::default() or an explicit empty/zeroed instance) so
that in any early-return path (unwrap_result_or_return!,
unwrap_option_or_return!, etc.) the caller sees a valid empty changeset rather
than a stale one; apply this change in the function containing out_changeset,
VTableSigner usage and
PLATFORM_ADDRESS_WALLET_STORAGE.with_item/runtime().block_on(wallet.transfer).
In `@packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs`:
- Around line 17-19: Reset the caller-provided handle to NULL_HANDLE on function
entry so any early return cannot leave a stale handle: after validating pointers
(e.g., after check_ptr!(seed_bytes); check_ptr!(out_handle);) write *out_handle
= NULL_HANDLE before any further work or early returns; apply the same pattern
in the other affected functions/locations that use out_handle (the spots around
the check_ptr! calls at the other ranges) so only successful paths later
overwrite out_handle with a live handle.
In `@packages/rs-platform-wallet-ffi/src/xpub_render.rs`:
- Around line 28-36: The out_string pointer must be nulled before any fallible
return so callers don't hold stale pointers; move or add the assignment
*out_string = ptr::null_mut(); immediately after the check_ptr! validations
(i.e., after the check_ptr!(bytes); check_ptr!(out_string); lines) and before
any code that can return PlatformWalletFfiResult::err (such as the bytes_len ==
0 check), ensuring every early return leaves *out_string set to null.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/managed_identity.rs`:
- Around line 341-352: The test calls (e.g., test_get_balance) currently ignore
the FFI return value from managed_identity_get_balance, which can leave balance
uninitialized; update the test to capture the FFI return (e.g., let res =
managed_identity_get_balance(handle, &mut balance);) and assert success (e.g.,
assert_eq!(res, 0) or assert!(is_success(res))) before using balance or calling
managed_identity_destroy; apply the same pattern to other tests that call FFI
functions so every FFI call’s result is checked immediately after invocation.
In `@packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs`:
- Around line 793-797: The test and comment reveal an inconsistent error
mapping: established_contact_destroy currently uses unwrap_option_or_return! and
yields PlatformWalletFfiResultCode::NotFound for an invalid handle, whereas
managed_identity_destroy and contact_request_destroy return ErrorInvalidHandle;
update established_contact_destroy so an invalid/bogus handle returns
PlatformWalletFfiResultCode::ErrorInvalidHandle to match the other destroy
functions (either remove or replace the unwrap_option_or_return! usage with the
same invalid-handle check/return logic used by
managed_identity_destroy/contact_request_destroy, or map the NotFound outcome to
ErrorInvalidHandle before returning).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d30d74b6-050c-4d46-8343-6f4f7e01d593
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/src/asset_lock/build.rspackages/rs-platform-wallet-ffi/src/asset_lock/manager.rspackages/rs-platform-wallet-ffi/src/asset_lock/sync.rspackages/rs-platform-wallet-ffi/src/contact.rspackages/rs-platform-wallet-ffi/src/contact_request.rspackages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/wallet.rspackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/dashpay_profile.rspackages/rs-platform-wallet-ffi/src/data_contract.rspackages/rs-platform-wallet-ffi/src/derivation.rspackages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rspackages/rs-platform-wallet-ffi/src/dpns.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/established_contact.rspackages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rspackages/rs-platform-wallet-ffi/src/identity_discovery.rspackages/rs-platform-wallet-ffi/src/identity_key_preview.rspackages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rspackages/rs-platform-wallet-ffi/src/identity_manager.rspackages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_sync.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/identity_transfer.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet-ffi/src/identity_withdrawal.rspackages/rs-platform-wallet-ffi/src/managed_identity.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/memory_explorer.rspackages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet-ffi/src/platform_addresses/mod.rspackages/rs-platform-wallet-ffi/src/platform_addresses/sync.rspackages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rspackages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rspackages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rspackages/rs-platform-wallet-ffi/src/platform_wallet_info.rspackages/rs-platform-wallet-ffi/src/spv.rspackages/rs-platform-wallet-ffi/src/tokens/burn.rspackages/rs-platform-wallet-ffi/src/tokens/claim.rspackages/rs-platform-wallet-ffi/src/tokens/destroy_frozen_funds.rspackages/rs-platform-wallet-ffi/src/tokens/freeze.rspackages/rs-platform-wallet-ffi/src/tokens/group_info.rspackages/rs-platform-wallet-ffi/src/tokens/group_queries.rspackages/rs-platform-wallet-ffi/src/tokens/mint.rspackages/rs-platform-wallet-ffi/src/tokens/pause.rspackages/rs-platform-wallet-ffi/src/tokens/purchase.rspackages/rs-platform-wallet-ffi/src/tokens/resume.rspackages/rs-platform-wallet-ffi/src/tokens/set_price.rspackages/rs-platform-wallet-ffi/src/tokens/transfer.rspackages/rs-platform-wallet-ffi/src/tokens/unfreeze.rspackages/rs-platform-wallet-ffi/src/tokens/update_config.rspackages/rs-platform-wallet-ffi/src/utils.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet-ffi/src/xpub_render.rspackages/rs-platform-wallet-ffi/tests/comprehensive_tests.rspackages/rs-platform-wallet-ffi/tests/integration_tests.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/EstablishedContact.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerIdentitySync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenActions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenGroupActionQueries.swift
✅ Files skipped from review due to trivial changes (2)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swift
- packages/rs-platform-wallet-ffi/src/identity_transfer.rs
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs
- packages/rs-platform-wallet-ffi/Cargo.toml
- packages/rs-platform-wallet-ffi/src/platform_addresses/mod.rs
- packages/rs-platform-wallet-ffi/src/tokens/destroy_frozen_funds.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/EstablishedContact.swift
- packages/rs-platform-wallet-ffi/src/tokens/claim.rs
- packages/rs-platform-wallet-ffi/src/data_contract.rs
- packages/rs-platform-wallet-ffi/src/derivation.rs
- packages/rs-platform-wallet-ffi/src/tokens/unfreeze.rs
- packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContactRequest.swift
- packages/rs-platform-wallet-ffi/src/spv.rs
- packages/rs-platform-wallet-ffi/src/contact.rs
- packages/rs-platform-wallet-ffi/src/tokens/group_queries.rs
- packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs
- packages/rs-platform-wallet-ffi/src/dashpay.rs
- packages/rs-platform-wallet-ffi/src/dashpay_profile.rs
- packages/rs-platform-wallet-ffi/src/tokens/set_price.rs
- packages/rs-platform-wallet-ffi/src/identity_update.rs
- packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs
- packages/rs-platform-wallet-ffi/src/established_contact.rs
- packages/rs-platform-wallet-ffi/src/platform_address_sync.rs
ZocoLini
force-pushed
the
refactor/error-fw-pw
branch
2 times, most recently
from
1bb968d to
0dd57ea
Compare
ZocoLini
force-pushed
the
refactor/error-fw-pw
branch
from
0dd57ea to
51b63ae
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-platform-wallet-ffi/src/platform_addresses/sync.rs (1)
19-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
has_config = truewith a nullconfigpointer.Right now that combination silently degrades to
Noneand runs a default sync. For an FFI boundary, this should fail fast instead of mutating wallet state with unintended settings.Suggested fix
pub unsafe extern "C" fn platform_address_wallet_sync_balances( handle: Handle, has_config: bool, config: *const AddressSyncConfigFFI, out_result: *mut AddressSyncResultFFI, ) -> PlatformWalletFfiResult { check_ptr!(out_result); + if has_config { + check_ptr!(config); + } let config_opt = if has_config && !config.is_null() { Some(dash_sdk::platform::address_sync::AddressSyncConfig::from( *config, ))Also applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/platform_addresses/sync.rs` around lines 19 - 25, In platform_address_wallet_sync_balances (and the similar sync function around lines 27-33), add an early validation that if has_config is true and config is null, you immediately set an error on out_result and return a failing PlatformWalletFfiResult instead of proceeding; use the same null-check pattern as check_ptr!(out_result) for config, populate out_result with an appropriate AddressSyncResultFFI error status/message, and return the error PlatformWalletFfiResult (e.g., InvalidArguments/NullPointer) so the FFI boundary fails fast and does not run a default sync.packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs (1)
349-378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate
key_idrows before inserting intokeys_map.
BTreeMap::insertsilently overwrites an earlier entry with the samekey_id, so malformed FFI input can register fewer keys than the caller actually supplied.Suggested fix
let contract_bounds = unwrap_result_or_return!(decode_contract_bounds(row, purpose, i, "identity_pubkeys")); + if keys_map.contains_key(&row.key_id) { + return PlatformWalletFfiResult::err( + PlatformWalletFfiResultCode::ErrorInvalidParameter, + format!("identity_pubkeys[{i}].key_id = {} is duplicated", row.key_id), + ); + } keys_map.insert( row.key_id, IdentityPublicKey::V0(IdentityPublicKeyV0 { id: row.key_id, purpose,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs` around lines 349 - 378, The loop building keys_map currently uses keys_map.insert which silently overwrites duplicate key_id values; update the loop that iterates over pubkey_rows (the block creating IdentityPublicKey::V0 and calling keys_map.insert) to first check whether keys_map.contains_key(&row.key_id) and if so return a PlatformWalletFfiResult::err with an appropriate PlatformWalletFfiResultCode (e.g., ErrorInvalidArgument or a new duplicate-key error) and a message identifying the duplicate key_id and index i; only call keys_map.insert when the key_id is not already present to reject duplicate key rows instead of overwriting them.
♻️ Duplicate comments (6)
packages/rs-platform-wallet-ffi/tests/integration_tests.rs (1)
289-292:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCapture and assert these FFI results.
Both calls can fail, but this test ignores the returned
PlatformWalletFfiResultand only fails later with a weaker signal. Please assert success here so the workflow breaks at the actual fault site.Suggested fix
- managed_identity_set_label(identity_handle, label.as_ptr()); + let result = managed_identity_set_label(identity_handle, label.as_ptr()); + assert_eq!(result.code, PlatformWalletFfiResultCode::Success); - identity_manager_add_identity(manager_handle, identity_handle); + let result = identity_manager_add_identity(manager_handle, identity_handle); + assert_eq!(result.code, PlatformWalletFfiResultCode::Success);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/tests/integration_tests.rs` around lines 289 - 292, The calls to managed_identity_set_label and identity_manager_add_identity can fail but their return values are ignored; capture each call's PlatformWalletFfiResult (e.g., let res = managed_identity_set_label(...)) and assert success immediately (e.g., assert_eq!(res, PlatformWalletFfiResult::Success) or use a helper like ensure_success(res)) so failures surface at the exact FFI call site; do this for both managed_identity_set_label(identity_handle, label.as_ptr()) and identity_manager_add_identity(manager_handle, identity_handle).packages/rs-platform-wallet-ffi/src/xpub_render.rs (1)
28-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
*out_stringis still not cleared before thebytes_len == 0early return.The past review noted that
*out_stringshould be cleared before any fallible return to prevent callers from retaining stale pointers. While the review was marked as addressed, the current code still clears*out_stringat line 36, which is after thebytes_len == 0check at lines 30-35. Ifbytes_len == 0, the function returns without clearing*out_string.Move the clearing before the
bytes_lencheck:Proposed fix
check_ptr!(bytes); check_ptr!(out_string); + *out_string = ptr::null_mut(); if bytes_len == 0 { return PlatformWalletFfiResult::err( PlatformWalletFfiResultCode::ErrorInvalidParameter, "bytes_len is zero", ); } - *out_string = ptr::null_mut();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/xpub_render.rs` around lines 28 - 36, The function currently checks bytes_len and returns a PlatformWalletFfiResult::err before clearing the out_string pointer, which can leave callers with a stale pointer; move the statement that clears *out_string (currently at the end of the shown block) to immediately after check_ptr!(out_string) and before the bytes_len == 0 check so that *out_string = ptr::null_mut() is executed on every early return (including the PlatformWalletFfiResult::err path).packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs (1)
230-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCapture and assert
PlatformWalletFfiResultbefore consuming out params.These calls currently ignore the returned result and then read output buffers/handles. Please assert success (
result.code == Success) first at each call site to avoid cascading invalid-state assertions.Pattern to apply at each affected call site
- managed_identity_get_established_contact_ids(alice_handle, &mut established_array); + let result = managed_identity_get_established_contact_ids(alice_handle, &mut established_array); + assert_eq!(result.code, PlatformWalletFfiResultCode::Success);Also applies to: 238-239, 246-247, 280-283, 569-570, 577-578, 585-586, 590-590, 597-598, 664-668, 689-690, 695-696, 721-725, 729-729, 736-739, 833-833, 838-838
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs` around lines 230 - 231, The call to managed_identity_get_established_contact_ids currently ignores the returned PlatformWalletFfiResult and immediately reads the out param established_array; capture the return value (e.g., let result = managed_identity_get_established_contact_ids(alice_handle, &mut established_array);) and assert result.code == Success before inspecting established_array.count or other out parameters; apply this same pattern for all similar FFI calls in the test (capture the PlatformWalletFfiResult, assert result.code == Success, then consume the out params) so you prevent reading invalid/undefined output on failure.packages/rs-platform-wallet-ffi/src/identity_discovery.rs (1)
66-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale
PlatformWalletFFIErrordoc fragment.The doc block still contains a leftover line about
PlatformWalletFFIError, but this function no longer has anout_errorchannel. Please delete that stale line to keep the API contract accurate.Suggested fix
/// - `out_found` — populated on success with a heap-allocated array /// of the newly-discovered identity ids. Release with /// [`platform_wallet_discover_identities_free`]. On error the /// struct is left at its empty-zero state. -/// [`PlatformWalletFFIError`] detail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_discovery.rs` around lines 66 - 70, Remove the stale doc fragment that mentions `PlatformWalletFFIError` and the non-existent `out_error` channel from the doc comment describing `out_found` (and its `platform_wallet_discover_identities_free` release note) in identity_discovery.rs; simply delete the line referencing `PlatformWalletFFIError` so the doc accurately reflects that `out_found` is populated on success and that there is no `out_error` output field.packages/rs-platform-wallet-ffi/src/error.rs (2)
53-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEvaluate
$expronce inunwrap_option_or_return!.The macro currently evaluates
$exprin both the pattern match and the return branch. This can duplicate side effects and produce incorrect behavior for non-idempotent expressions.Suggested fix
#[macro_export] macro_rules! unwrap_option_or_return { ($expr:expr) => {{ - let Some(v) = $expr else { - return $expr.into(); + let opt = $expr; + let Some(v) = opt else { + return opt.into(); }; v }}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/error.rs` around lines 53 - 61, The macro unwrap_option_or_return! currently evaluates $expr twice; change it to evaluate $expr once by first binding it to a fresh local variable (e.g., let __tmp = $expr;) inside the macro and then match or use let Some(v) = __tmp else { return __tmp.into(); }; update the macro body for unwrap_option_or_return! to use that temporary binding so side-effecting expressions are only evaluated once and keep the temp name hygiene-safe (use a name unlikely to collide or macro hygiene patterns).
145-155:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
From<Option<T>>currently masks invalid-handle misses asNotFound.Using a blanket
Option<T>conversion makesNoneindistinguishable across contexts (domain miss vs stale/invalid handle when paired with storage lookups). This weakens API diagnostics and behavior predictability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/error.rs` around lines 145 - 155, The blanket impl From<Option<T>> for PlatformWalletFfiResult masks different None causes (e.g., domain miss vs stale/invalid handle); remove this generic impl and replace it with explicit conversions so callers decide the proper error code. Concretely: delete the impl for From<Option<T>>, update call sites using PlatformWalletFfiResult::from(option) to instead map None -> an appropriate PlatformWalletFfiResult (e.g., PlatformWalletFfiResult::err(PlatformWalletFfiResultCode::NotFound, ...) for genuine not-found cases or a distinct InvalidHandle/BadHandle code for stale handle lookups), or introduce targeted impls only for specific types (e.g., impl From<Option<MyDomainType>> or impl From<Option<HandleType>>) that produce the correct PlatformWalletFfiResultCode; keep all logic in the functions that perform storage/handle lookups so the error semantics remain explicit.
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/dashpay.rs (1)
170-183: ⚡ Quick winPre-zero
out_arrayin both contact-request array producers.Both functions can return before assignment, which leaves the paired free path with whatever stale value the caller passed in. Initializing to
ContactRequestHandleArray::empty()up front makes the failure path deterministic.Suggested fix
pub unsafe extern "C" fn platform_wallet_sync_contact_requests( wallet_handle: Handle, out_array: *mut ContactRequestHandleArray, ) -> PlatformWalletFfiResult { check_ptr!(out_array); + *out_array = ContactRequestHandleArray::empty(); let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity = wallet.identity().clone(); block_on_worker(async move { identity.sync_contact_requests().await }) @@ pub unsafe extern "C" fn platform_wallet_fetch_sent_contact_requests( wallet_handle: Handle, identity_id: *const u8, out_array: *mut ContactRequestHandleArray, ) -> PlatformWalletFfiResult { check_ptr!(out_array); + *out_array = ContactRequestHandleArray::empty(); let id = unwrap_result_or_return!(unsafe { read_identifier(identity_id) });Also applies to: 337-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/dashpay.rs` around lines 170 - 183, Pre-zero the output pointer so failure paths can't leave a stale value: immediately after the check_ptr! in platform_wallet_sync_contact_requests, set unsafe { *out_array = ContactRequestHandleArray::empty() } (and do the same in the other function in this file that produces a ContactRequestHandleArray) so any early returns use a deterministic empty array; ensure this assignment happens before calling PLATFORM_WALLET_STORAGE.with_item / any await/unwrap macros and before any return paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs`:
- Around line 34-57: Clear the resume outputs (set *out_proof_bytes to null,
*out_proof_len to 0 and zero/clear *out_private_key) immediately after the
pointer checks and before calling ASSET_LOCK_MANAGER_STORAGE.with_item /
runtime().block_on(manager.resume_asset_lock(...)) and before using
unwrap_option_or_return! / unwrap_result_or_return! so any early return leaves
safe defaults; update the block around resume_asset_lock/unwrap_* to initialize
these outputs to safe values prior to the first fallible operation (references:
out_proof_bytes, out_proof_len, out_private_key, resume_asset_lock,
unwrap_option_or_return!, unwrap_result_or_return!).
In `@packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- Around line 156-168: In established_contact_get_note, ensure the out parameter
is set to a safe null state before any early returns: after check_ptr!(out_note)
but before any unwrap_*_or_return! calls, write a null pointer to *out_note so
callers receive nullptr on error/invalid handle; keep the rest of the logic that
reads from ESTABLISHED_CONTACT_STORAGE.with_item, uses unwrap_option_or_return!,
and converts to CString unchanged, only adding the initialization step to
guarantee out_note is assigned on all exit paths.
In `@packages/rs-platform-wallet-ffi/src/spv.rs`:
- Around line 239-248: In platform_wallet_manager_spv_stop, the Result from
manager.spv().stop().await is currently ignored so failures always return
success; change the async block to capture the stop() result, check for Err and
convert/return that error as a PlatformWalletFfiResult instead of discarding it
(for example, map the Err into PlatformWalletFfiResult::err or the crate's
error-to-ffi helper) while preserving the existing
unwrap_option_or_return!(option) flow; update the closure passed to
PLATFORM_WALLET_MANAGER_STORAGE.with_item to return the stop() result and
propagate it out so platform_wallet_manager_spv_stop returns an error when
stop() fails.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs`:
- Around line 349-378: The loop building keys_map currently uses keys_map.insert
which silently overwrites duplicate key_id values; update the loop that iterates
over pubkey_rows (the block creating IdentityPublicKey::V0 and calling
keys_map.insert) to first check whether keys_map.contains_key(&row.key_id) and
if so return a PlatformWalletFfiResult::err with an appropriate
PlatformWalletFfiResultCode (e.g., ErrorInvalidArgument or a new duplicate-key
error) and a message identifying the duplicate key_id and index i; only call
keys_map.insert when the key_id is not already present to reject duplicate key
rows instead of overwriting them.
In `@packages/rs-platform-wallet-ffi/src/platform_addresses/sync.rs`:
- Around line 19-25: In platform_address_wallet_sync_balances (and the similar
sync function around lines 27-33), add an early validation that if has_config is
true and config is null, you immediately set an error on out_result and return a
failing PlatformWalletFfiResult instead of proceeding; use the same null-check
pattern as check_ptr!(out_result) for config, populate out_result with an
appropriate AddressSyncResultFFI error status/message, and return the error
PlatformWalletFfiResult (e.g., InvalidArguments/NullPointer) so the FFI boundary
fails fast and does not run a default sync.
---
Duplicate comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 53-61: The macro unwrap_option_or_return! currently evaluates
$expr twice; change it to evaluate $expr once by first binding it to a fresh
local variable (e.g., let __tmp = $expr;) inside the macro and then match or use
let Some(v) = __tmp else { return __tmp.into(); }; update the macro body for
unwrap_option_or_return! to use that temporary binding so side-effecting
expressions are only evaluated once and keep the temp name hygiene-safe (use a
name unlikely to collide or macro hygiene patterns).
- Around line 145-155: The blanket impl From<Option<T>> for
PlatformWalletFfiResult masks different None causes (e.g., domain miss vs
stale/invalid handle); remove this generic impl and replace it with explicit
conversions so callers decide the proper error code. Concretely: delete the impl
for From<Option<T>>, update call sites using
PlatformWalletFfiResult::from(option) to instead map None -> an appropriate
PlatformWalletFfiResult (e.g.,
PlatformWalletFfiResult::err(PlatformWalletFfiResultCode::NotFound, ...) for
genuine not-found cases or a distinct InvalidHandle/BadHandle code for stale
handle lookups), or introduce targeted impls only for specific types (e.g., impl
From<Option<MyDomainType>> or impl From<Option<HandleType>>) that produce the
correct PlatformWalletFfiResultCode; keep all logic in the functions that
perform storage/handle lookups so the error semantics remain explicit.
In `@packages/rs-platform-wallet-ffi/src/identity_discovery.rs`:
- Around line 66-70: Remove the stale doc fragment that mentions
`PlatformWalletFFIError` and the non-existent `out_error` channel from the doc
comment describing `out_found` (and its
`platform_wallet_discover_identities_free` release note) in
identity_discovery.rs; simply delete the line referencing
`PlatformWalletFFIError` so the doc accurately reflects that `out_found` is
populated on success and that there is no `out_error` output field.
In `@packages/rs-platform-wallet-ffi/src/xpub_render.rs`:
- Around line 28-36: The function currently checks bytes_len and returns a
PlatformWalletFfiResult::err before clearing the out_string pointer, which can
leave callers with a stale pointer; move the statement that clears *out_string
(currently at the end of the shown block) to immediately after
check_ptr!(out_string) and before the bytes_len == 0 check so that *out_string =
ptr::null_mut() is executed on every early return (including the
PlatformWalletFfiResult::err path).
In `@packages/rs-platform-wallet-ffi/tests/comprehensive_tests.rs`:
- Around line 230-231: The call to managed_identity_get_established_contact_ids
currently ignores the returned PlatformWalletFfiResult and immediately reads the
out param established_array; capture the return value (e.g., let result =
managed_identity_get_established_contact_ids(alice_handle, &mut
established_array);) and assert result.code == Success before inspecting
established_array.count or other out parameters; apply this same pattern for all
similar FFI calls in the test (capture the PlatformWalletFfiResult, assert
result.code == Success, then consume the out params) so you prevent reading
invalid/undefined output on failure.
In `@packages/rs-platform-wallet-ffi/tests/integration_tests.rs`:
- Around line 289-292: The calls to managed_identity_set_label and
identity_manager_add_identity can fail but their return values are ignored;
capture each call's PlatformWalletFfiResult (e.g., let res =
managed_identity_set_label(...)) and assert success immediately (e.g.,
assert_eq!(res, PlatformWalletFfiResult::Success) or use a helper like
ensure_success(res)) so failures surface at the exact FFI call site; do this for
both managed_identity_set_label(identity_handle, label.as_ptr()) and
identity_manager_add_identity(manager_handle, identity_handle).
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- Around line 170-183: Pre-zero the output pointer so failure paths can't leave
a stale value: immediately after the check_ptr! in
platform_wallet_sync_contact_requests, set unsafe { *out_array =
ContactRequestHandleArray::empty() } (and do the same in the other function in
this file that produces a ContactRequestHandleArray) so any early returns use a
deterministic empty array; ensure this assignment happens before calling
PLATFORM_WALLET_STORAGE.with_item / any await/unwrap macros and before any
return paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cef42593-4d84-4b22-8994-6d30eeca2a06
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/src/asset_lock/build.rspackages/rs-platform-wallet-ffi/src/asset_lock/manager.rspackages/rs-platform-wallet-ffi/src/asset_lock/sync.rspackages/rs-platform-wallet-ffi/src/contact.rspackages/rs-platform-wallet-ffi/src/contact_request.rspackages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/wallet.rspackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/dashpay_profile.rspackages/rs-platform-wallet-ffi/src/data_contract.rspackages/rs-platform-wallet-ffi/src/derivation.rspackages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rspackages/rs-platform-wallet-ffi/src/dpns.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/established_contact.rspackages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rspackages/rs-platform-wallet-ffi/src/identity_discovery.rspackages/rs-platform-wallet-ffi/src/identity_key_preview.rspackages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rspackages/rs-platform-wallet-ffi/src/identity_manager.rspackages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_sync.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/identity_transfer.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet-ffi/src/identity_withdrawal.rspackages/rs-platform-wallet-ffi/src/managed_identity.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/memory_explorer.rspackages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet-ffi/src/platform_addresses/mod.rspackages/rs-platform-wallet-ffi/src/platform_addresses/sync.rspackages/rs-platform-wallet-ffi/src/platform_addresses/transfer.rspackages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rspackages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rspackages/rs-platform-wallet-ffi/src/platform_wallet_info.rspackages/rs-platform-wallet-ffi/src/spv.rspackages/rs-platform-wallet-ffi/src/tokens/burn.rspackages/rs-platform-wallet-ffi/src/tokens/claim.rspackages/rs-platform-wallet-ffi/src/tokens/destroy_frozen_funds.rspackages/rs-platform-wallet-ffi/src/tokens/freeze.rspackages/rs-platform-wallet-ffi/src/tokens/group_info.rspackages/rs-platform-wallet-ffi/src/tokens/group_queries.rspackages/rs-platform-wallet-ffi/src/tokens/mint.rspackages/rs-platform-wallet-ffi/src/tokens/pause.rspackages/rs-platform-wallet-ffi/src/tokens/purchase.rspackages/rs-platform-wallet-ffi/src/tokens/resume.rspackages/rs-platform-wallet-ffi/src/tokens/set_price.rspackages/rs-platform-wallet-ffi/src/tokens/transfer.rspackages/rs-platform-wallet-ffi/src/tokens/unfreeze.rspackages/rs-platform-wallet-ffi/src/tokens/update_config.rspackages/rs-platform-wallet-ffi/src/utils.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet-ffi/src/xpub_render.rspackages/rs-platform-wallet-ffi/tests/comprehensive_tests.rspackages/rs-platform-wallet-ffi/tests/integration_tests.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/EstablishedContact.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerIdentitySync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenActions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenGroupActionQueries.swift
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet/src/wallet/apply.rs
- packages/rs-platform-wallet-ffi/src/tokens/group_queries.rs
- packages/rs-platform-wallet-ffi/src/managed_identity.rs
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/rs-platform-wallet-ffi/Cargo.toml
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swift
- packages/rs-platform-wallet-ffi/src/data_contract.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs
- packages/rs-platform-wallet-ffi/src/tokens/purchase.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/wallet.rs
- packages/rs-platform-wallet-ffi/src/identity_key_preview.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
- packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs
- packages/rs-platform-wallet-ffi/src/identity_update.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContactRequest.swift
- packages/rs-platform-wallet-ffi/src/tokens/mint.rs
- packages/rs-platform-wallet-ffi/src/tokens/transfer.rs
- packages/rs-platform-wallet-ffi/src/identity_transfer.rs
- packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs
- packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs
- packages/rs-platform-wallet-ffi/src/identity_withdrawal.rs
- packages/rs-platform-wallet-ffi/src/dashpay_profile.rs
- packages/rs-platform-wallet-ffi/src/contact.rs
- packages/rs-platform-wallet-ffi/src/tokens/freeze.rs
- packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs
- packages/rs-platform-wallet-ffi/src/contact_request.rs
- packages/rs-platform-wallet-ffi/src/identity_sync.rs
- packages/rs-platform-wallet-ffi/src/tokens/resume.rs
QuantumExplorer
requested changes
May 1, 2026
Rename PlatformWalletFfiResult/PlatformWalletFfiResultCode back to PlatformWalletFFIResult/PlatformWalletFFIResultCode to maintain the existing FFI-uppercase convention used elsewhere in the workspace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # packages/rs-platform-wallet-ffi/src/core_wallet/wallet.rs # packages/rs-platform-wallet-ffi/src/derivation.rs # packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs # packages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rs # packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs # packages/rs-platform-wallet-ffi/src/manager.rs # packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs # packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs # packages/rs-platform-wallet-ffi/src/spv.rs # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift
QuantumExplorer
approved these changes
May 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The error/result patterns used across
rs-platform-wallet-ffihad grown inconsistent: a separate by-valuePlatformWalletFFIErrorout-parameter, ad-hoc null-pointer checks scattered through every entry point, and verbose match arms for unwrappingResult/Optionat the FFI boundary. This made FFI functions noisy, error-prone (easy to forget a null check), and difficult to keep uniform as new entry points were added.What was done?
Reworked the FFI error framework in
rs-platform-wallet-ffiand updated all consumers (Rust + Swift) to use it.PlatformWalletFFIResult(enum) +PlatformWalletFFIError(struct with code + message) pair with a singlePlatformWalletFFIResultstruct holding aPlatformWalletFFIResultCodeand an optional message.Dropfrees the message;platform_wallet_ffi_result_freeis now idempotent and safe onSuccess.deref_ptr!,deref_ptr_mut!,check_ptr!,unwrap_result_or_return!, andunwrap_option_or_return!so each FFI entry point can validate inputs and unwrap fallible operations in one line, returning a properly-formed result on failure.Fromconversions. ImplementedFrom<Option<T>>,From<PlatformWalletError>,From<dashcore::consensus::encode::Error>,From<std::ffi::NulError>, etc. intoPlatformWalletFFIResultso error propagation via?/.into()is uniform.NotFoundcode. Added a dedicated code (98) used exclusively forOption::Nonereturns, replacing previous overloaded uses ofErrorUnknown.asset_lock,core_wallet,platform_addresses,tokens/*,identity_*,dashpay,dpns,contact*,manager,wallet,spv,memory_explorer, etc. to the new pattern. Tests intests/comprehensive_tests.rsandtests/integration_tests.rswere updated accordingly.PlatformWalletResult.swiftnow wraps the new struct (reference-counted), and consumers inPlatformWallet/,KeyWallet/,Persistence/, etc. have been updated to use the new result-checking pattern.SwiftExampleAppviews and tests updated to match.rs-sdk-ffi/rs-unified-sdk-ffi. Minor follow-on adjustments where they consume platform-wallet-ffi types.FFI, notFfi) — types arePlatformWalletFFIResult/PlatformWalletFFIResultCode.How Has This Been Tested?
cargo check -p platform-wallet-ffi --tests✓cargo check -p rs-sdk-ffi✓cargo check -p rs-unified-sdk-ffi✓cargo fmt --all✓tests/comprehensive_tests.rsandtests/integration_tests.rsmigrated to the new result API and continue to compile/run.SwiftExampleAppbuild via the standard iOS build flow.Breaking Changes
This is an internal FFI refactor — the platform-wallet-ffi C ABI surface (function names, struct layout) is changed but the crate is not consumed outside this workspace, so no consensus-relevant or external-API breakage. Swift consumers in this repo are updated in the same PR.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit