chore(key-wallet-ffi): drop key-wallet ffi functions that are not being used in the swift-sdk#528
Draft
chore(key-wallet-ffi): drop key-wallet ffi functions that are not being used in the swift-sdk#528
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors key-wallet-ffi's public C API to a wallet-centric, collection-driven model: removes many per-account and low-level key/transaction FFI functions, introduces account-collection/summary, address-pool, managed-wallet APIs, and removes bip38 feature and module export. Adds FFINetwork enum and various managed-wallet helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Client
rect rgba(200,200,255,0.5)
participant WM as WalletManager
end
rect rgba(200,255,200,0.5)
participant W as Wallet
participant MW as ManagedWallet
participant AP as AddressPool
end
Client->>WM: wallet_manager_get_wallet_ids()
Client->>WM: wallet_manager_get_wallet(wallet_id)
Client->>WM: wallet_manager_get_managed_wallet_info(wallet_id)
WM->>W: wallet_get_account_collection(wallet)
W->>MW: wallet_get_account_collection -> FFIAccountCollection
Client->>MW: managed_wallet_get_address_pool_info(account_index, pool_type)
MW->>AP: managed_core_account_get_address_pool(account)
AP-->>Client: address_pool_get_addresses_in_range(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #528 +/- ##
=============================================
+ Coverage 66.90% 67.51% +0.60%
=============================================
Files 313 313
Lines 64757 62269 -2488
=============================================
- Hits 43325 42038 -1287
+ Misses 21432 20231 -1201
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ZocoLini
force-pushed
the
chore/drop-unused-keywallet-ffi
branch
from
8ddba5f to
ee4fffe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
key-wallet-ffi/src/wallet_manager.rs (1)
57-80:⚠️ Potential issue | 🔴 CriticalRestore a public destructor for
wallet_manager_create.This function still transfers ownership of a heap-allocated
FFIWalletManagerto FFI callers, but this module no longer exposes any matchingwallet_manager_free.key-wallet-ffi/src/wallet_manager_tests.rs:36-38still calls that symbol, so the test target now fails to compile, and external callers have no supported way to release the manager/runtime.As per coding guidelines `**/*-ffi/**/*.rs`: Handle memory safety carefully at FFI boundaries.Suggested fix
#[no_mangle] pub extern "C" fn wallet_manager_create( network: FFINetwork, error: *mut FFIError, ) -> *mut FFIWalletManager { @@ Box::into_raw(Box::new(FFIWalletManager { network, manager: Arc::new(RwLock::new(manager)), runtime, })) } + +/// Free wallet manager +/// +/// # Safety +/// - `manager` must be a valid pointer returned by `wallet_manager_create` or null +/// - The pointer must not be used after calling this function +#[no_mangle] +pub unsafe extern "C" fn wallet_manager_free(manager: *mut FFIWalletManager) { + if !manager.is_null() { + let _ = Box::from_raw(manager); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager.rs` around lines 57 - 80, wallet_manager_create transfers ownership of an FFIWalletManager to callers but no destructor is exposed; add a public extern "C" destructor named wallet_manager_free that reverses that transfer, is safe for null, and is usable by tests and external callers. Implement wallet_manager_free(ptr: *mut FFIWalletManager, error: *mut FFIError) as an extern "C" fn that returns void, checks for null, uses Box::from_raw to reclaim the allocation, properly shuts down or drops the contained runtime and manager (drop the Arc<RwLock<WalletManager>> and cleanly stop the tokio::runtime::Runtime), and set FFIError success/failure via FFIError::set_success/set_error; ensure symbol name matches wallet_manager_free expected by tests and FFI consumers.key-wallet-ffi/src/managed_wallet.rs (1)
592-606:⚠️ Potential issue | 🔴 CriticalKeep a
managed_wallet_freealias, or rename all remaining call sites in this PR.The module now only exports
managed_wallet_info_free, but the in-file tests below still callmanaged_wallet_free(for example at Lines 624-628 and Lines 792-796). That makes the test target fail to compile and drops a previously public deallocator name without a compatibility shim.As per coding guidelines `**/*-ffi/**/*.rs`: Handle memory safety carefully at FFI boundaries.Suggested fix
#[no_mangle] pub unsafe extern "C" fn managed_wallet_info_free(wallet_info: *mut FFIManagedWalletInfo) { if !wallet_info.is_null() { let wrapper = Box::from_raw(wallet_info); if !wrapper.inner.is_null() { let _ = Box::from_raw(wrapper.inner as *mut ManagedWalletInfo); } } } + +#[no_mangle] +pub unsafe extern "C" fn managed_wallet_free(managed_wallet: *mut FFIManagedWalletInfo) { + managed_wallet_info_free(managed_wallet); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_wallet.rs` around lines 592 - 606, Tests and other call sites still reference the old deallocator name managed_wallet_free while the module only exposes managed_wallet_info_free; restore compatibility by adding a no_mangle public alias function named managed_wallet_free that simply forwards to managed_wallet_info_free (e.g., define #[no_mangle] pub unsafe extern "C" fn managed_wallet_free(wallet_info: *mut FFIManagedWalletInfo) { managed_wallet_info_free(wallet_info); }), or alternatively rename all remaining call sites to use managed_wallet_info_free consistently; reference the functions managed_wallet_info_free and managed_wallet_free when locating where to add the shim or update usages.key-wallet-ffi/src/transaction.rs (2)
337-345:⚠️ Potential issue | 🔴 CriticalFix critical memory safety violation at FFI boundary in transaction bytes handling.
wallet_build_and_sign_transactionallocates a boxed slice (Box<[u8]>), then casts it to*mut u8, discarding the slice length metadata. Whentransaction_bytes_freeattempts to reconstruct the pointer withBox::from_raw(tx_bytes), it treats it as a singleu8rather than a slice, causing undefined behavior during deallocation. This violates FFI memory safety requirements.Use
libc::mallocandlibc::freeto manage the transaction bytes—consistent with C allocation semantics at FFI boundaries:Suggested fix
- let boxed = serialized.into_boxed_slice(); - let tx_bytes = Box::into_raw(boxed) as *mut u8; + let tx_bytes = libc::malloc(size) as *mut u8; + if tx_bytes.is_null() { + FFIError::set_error( + error, + FFIErrorCode::AllocationFailed, + "Failed to allocate transaction bytes".to_string(), + ); + return false; + } + ptr::copy_nonoverlapping(serialized.as_ptr(), tx_bytes, size); *tx_bytes_out = tx_bytes; *tx_len_out = size; @@ #[no_mangle] pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8) { if !tx_bytes.is_null() { - unsafe { - let _ = Box::from_raw(tx_bytes); - } + libc::free(tx_bytes.cast()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 337 - 345, The current wallet_build_and_sign_transaction creates a Box<[u8]> and exposes its pointer as *mut u8 which loses the slice metadata and causes undefined behavior when transaction_bytes_free reconstructs with Box::from_raw; instead, allocate raw memory with libc::malloc of size serialized.len(), memcpy or ptr::copy_nonoverlapping the serialized bytes into that malloc'd buffer, set *tx_bytes_out to the returned pointer and *tx_len_out to the length, and update transaction_bytes_free to call libc::free on the pointer (ensuring it accepts the same pointer type), removing any use of Box::into_raw/Box::from_raw for the byte buffer to maintain correct FFI allocation semantics for wallet_build_and_sign_transaction and transaction_bytes_free.
481-483:⚠️ Potential issue | 🔴 Critical
wallet_check_transactioncan panic and has memory safety issues in FFI boundaries.This entrypoint calls
tokio::runtime::Handle::current()which panics when the caller is not already inside a Tokio runtime. For a C/Swift FFI API that crashes instead of returning anFFIError. Additionally, the transaction bytes allocation/deallocation has undefined behavior:serialized.into_boxed_slice()creates aBox<[u8]>, but casting it to*mut u8loses the slice metadata. The deallocation then tries to reconstruct withBox::from_raw()treating it asBox<u8>, causing a type mismatch across the FFI boundary.Solutions: (1) Store an
Arc<tokio::runtime::Runtime>as a field (seeFFIWalletManagerfor the correct pattern) and callruntime.block_on()instead ofHandle::current().block_on(). (2) For transaction bytes, either use a stable layout like a struct wrapping*mut u8with explicit length, or preserve the slice metadata through the FFI boundary without casting away type information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 481 - 483, The wallet_check_transaction FFI entry currently calls tokio::runtime::Handle::current() (and uses serialized.into_boxed_slice() then casts away slice metadata), which can panic outside a Tokio runtime and cause UB when deallocating bytes; fix by switching to the same runtime pattern used in FFIWalletManager: store an Arc<tokio::runtime::Runtime> on the wallet manager and call runtime.block_on(managed_info.check_core_transaction(...)) instead of Handle::current(), and change the transaction byte handling to preserve length/metadata across the FFI boundary (e.g. pass a struct with *mut u8 plus length or keep a Box<[u8]> and transfer it without casting to *mut u8 so you can reconstruct with Box<[u8]>::from_raw when freeing) to avoid casting Box<[u8]> to Box<u8> and the resulting type/size mismatch.key-wallet-ffi/src/account_collection.rs (1)
306-579:⚠️ Potential issue | 🔴 CriticalThe test module still targets APIs removed by this refactor.
These cases still reference
account_collection_count,account_collection_get_bip44_indices,account_collection_get_bip44_account,free_u32_array,account_collection_summary,account_collection_has_provider_operator_keys, and the provider-platform helpers, none of which exist in this module anymore. Because the tests importsuper::*,cargo testwill fail to compile until they're rewritten to useaccount_collection_summary_data/ the remaining entry points or deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account_collection.rs` around lines 306 - 579, Tests reference removed FFI functions (account_collection_count, account_collection_get_bip44_indices, account_collection_get_bip44_account, free_u32_array, account_collection_summary, account_collection_has_provider_operator_keys, provider-platform helpers); update or remove them so tests compile: replace uses of account_collection_summary/account_collection_summary_null_safety with account_collection_summary_data and assert on the returned struct/string fields, map calls that queried BIP44/BIP32/coinjoin/topup indices to the corresponding fields on account_collection_summary_data, replace provider operator/platform account helpers with the new summary data flags or account-specific getters if available, and remove/free any arrays via the new API's ownership conventions (do not call free_u32_array). Alternatively delete the obsolete tests that no longer apply. Ensure you reference account_collection_summary_data, wallet_get_account_collection, and any new summary field names when making changes.key-wallet-ffi/src/managed_account.rs (1)
1035-1094:⚠️ Potential issue | 🔴 CriticalUpdate the test module to stop calling removed helpers.
managed_wallet_get_account_countandmanaged_core_account_get_parent_wallet_idare no longer defined in this module, but the test module still calls them throughuse super::*. That makes the#[cfg(test)]target fail to compile until these cases are rewritten against the remaining account/collection APIs or removed.Also applies to: 1187-1189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 1035 - 1094, The test calls removed helpers managed_wallet_get_account_count and managed_core_account_get_parent_wallet_id (via use super::*); update the test(s) (e.g., test_managed_wallet_get_account_count) to stop calling these removed functions — either remove those assertions/calls or rewrite the checks to use the remaining public account/collection APIs (replace the managed_* calls with the appropriate wallet-manager/account enumeration functions present in the module), and ensure the test imports no longer rely on the deleted helpers.key-wallet-ffi/include/key_wallet_ffi.h (1)
316-377:⚠️ Potential issue | 🔴 CriticalDo not expose feature-dependent struct layouts through these summary APIs.
These declarations make
FFIAccountCollectionSummaryandFFIManagedCoreAccountCollectionSummarypart of the public C ABI, but the Rust#[repr(C)]definitions inkey-wallet-ffi/src/account_collection.rsandkey-wallet-ffi/src/managed_account_collection.rscompilehas_provider_operator_keys/has_provider_platform_keysin and out behind#[cfg]. The header always includes both fields, so builds withoutblsoreddsaproduce a different layout than the one C/Swift was compiled against.FFIManagedCoreAccountCollectionSummaryis especially unsafe becauseplatform_payment_keyssits after those optional booleans. Please make the Rust structs feature-independent and default those flags tofalse, or generate feature-specific headers. As per coding guidelines,**/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.Also applies to: 581-650, 915-940, 1975-2001
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/include/key_wallet_ffi.h` around lines 316 - 377, The public C ABI structs (e.g., FFIAccountCollectionSummary and FFIManagedCoreAccountCollectionSummary) expose feature-gated booleans (has_provider_operator_keys, has_provider_platform_keys) which causes layout mismatch; fix by making the Rust #[repr(C)] structs unconditionally include those boolean fields (and any other feature-gated fields) and default them to false in all build configurations so the layout matches the generated header, and update the code that initializes/returns these structs to set those flags to false when the feature is disabled (ensure platform_payment_keys remains at the same offset). Alternatively, if you prefer feature-specific ABIs, make the header generation conditional so the C header matches the exact Rust cfg layout for each build. Ensure changes touch the Rust struct definitions used for FFI and the header generation so the C header and Rust repr(C) layout are always consistent.key-wallet-ffi/src/wallet.rs (1)
400-445:⚠️ Potential issue | 🟡 MinorReplace the dead-end guidance in these error paths.
These notes/messages still tell callers to use
wallet_add_platform_payment_account(),wallet_add_dashpay_receiving_account(), andwallet_add_dashpay_external_account_with_xpub_bytes(), but those entry points were removed in this PR. Please point callers to the actual supported replacement or return a neutral “unsupported through this FFI surface” error instead.Also applies to: 494-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet.rs` around lines 400 - 445, The error messages in wallet_add_account that direct callers to removed functions (wallet_add_platform_payment_account, wallet_add_dashpay_receiving_account, wallet_add_dashpay_external_account_with_xpub_bytes) must be replaced: update the match arms for FFIAccountType::PlatformPayment, ::DashpayReceivingFunds and ::DashpayExternalAccount to return a neutral unsupported error (e.g. FFIAccountResult::error with FFIErrorCode::UnsupportedOperation or InvalidInput and a message like "Account type <X> is not supported through this FFI surface") or, if there is a real replacement API, point to that new API instead; apply the same change to the repeated/error paths later in the file (the other matching blocks around the 494-547 region) so callers are not pointed at removed entry points.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 316-377: The public C ABI structs (e.g.,
FFIAccountCollectionSummary and FFIManagedCoreAccountCollectionSummary) expose
feature-gated booleans (has_provider_operator_keys, has_provider_platform_keys)
which causes layout mismatch; fix by making the Rust #[repr(C)] structs
unconditionally include those boolean fields (and any other feature-gated
fields) and default them to false in all build configurations so the layout
matches the generated header, and update the code that initializes/returns these
structs to set those flags to false when the feature is disabled (ensure
platform_payment_keys remains at the same offset). Alternatively, if you prefer
feature-specific ABIs, make the header generation conditional so the C header
matches the exact Rust cfg layout for each build. Ensure changes touch the Rust
struct definitions used for FFI and the header generation so the C header and
Rust repr(C) layout are always consistent.
In `@key-wallet-ffi/src/account_collection.rs`:
- Around line 306-579: Tests reference removed FFI functions
(account_collection_count, account_collection_get_bip44_indices,
account_collection_get_bip44_account, free_u32_array,
account_collection_summary, account_collection_has_provider_operator_keys,
provider-platform helpers); update or remove them so tests compile: replace uses
of account_collection_summary/account_collection_summary_null_safety with
account_collection_summary_data and assert on the returned struct/string fields,
map calls that queried BIP44/BIP32/coinjoin/topup indices to the corresponding
fields on account_collection_summary_data, replace provider operator/platform
account helpers with the new summary data flags or account-specific getters if
available, and remove/free any arrays via the new API's ownership conventions
(do not call free_u32_array). Alternatively delete the obsolete tests that no
longer apply. Ensure you reference account_collection_summary_data,
wallet_get_account_collection, and any new summary field names when making
changes.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 1035-1094: The test calls removed helpers
managed_wallet_get_account_count and managed_core_account_get_parent_wallet_id
(via use super::*); update the test(s) (e.g.,
test_managed_wallet_get_account_count) to stop calling these removed functions —
either remove those assertions/calls or rewrite the checks to use the remaining
public account/collection APIs (replace the managed_* calls with the appropriate
wallet-manager/account enumeration functions present in the module), and ensure
the test imports no longer rely on the deleted helpers.
In `@key-wallet-ffi/src/managed_wallet.rs`:
- Around line 592-606: Tests and other call sites still reference the old
deallocator name managed_wallet_free while the module only exposes
managed_wallet_info_free; restore compatibility by adding a no_mangle public
alias function named managed_wallet_free that simply forwards to
managed_wallet_info_free (e.g., define #[no_mangle] pub unsafe extern "C" fn
managed_wallet_free(wallet_info: *mut FFIManagedWalletInfo) {
managed_wallet_info_free(wallet_info); }), or alternatively rename all remaining
call sites to use managed_wallet_info_free consistently; reference the functions
managed_wallet_info_free and managed_wallet_free when locating where to add the
shim or update usages.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 337-345: The current wallet_build_and_sign_transaction creates a
Box<[u8]> and exposes its pointer as *mut u8 which loses the slice metadata and
causes undefined behavior when transaction_bytes_free reconstructs with
Box::from_raw; instead, allocate raw memory with libc::malloc of size
serialized.len(), memcpy or ptr::copy_nonoverlapping the serialized bytes into
that malloc'd buffer, set *tx_bytes_out to the returned pointer and *tx_len_out
to the length, and update transaction_bytes_free to call libc::free on the
pointer (ensuring it accepts the same pointer type), removing any use of
Box::into_raw/Box::from_raw for the byte buffer to maintain correct FFI
allocation semantics for wallet_build_and_sign_transaction and
transaction_bytes_free.
- Around line 481-483: The wallet_check_transaction FFI entry currently calls
tokio::runtime::Handle::current() (and uses serialized.into_boxed_slice() then
casts away slice metadata), which can panic outside a Tokio runtime and cause UB
when deallocating bytes; fix by switching to the same runtime pattern used in
FFIWalletManager: store an Arc<tokio::runtime::Runtime> on the wallet manager
and call runtime.block_on(managed_info.check_core_transaction(...)) instead of
Handle::current(), and change the transaction byte handling to preserve
length/metadata across the FFI boundary (e.g. pass a struct with *mut u8 plus
length or keep a Box<[u8]> and transfer it without casting to *mut u8 so you can
reconstruct with Box<[u8]>::from_raw when freeing) to avoid casting Box<[u8]> to
Box<u8> and the resulting type/size mismatch.
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Around line 57-80: wallet_manager_create transfers ownership of an
FFIWalletManager to callers but no destructor is exposed; add a public extern
"C" destructor named wallet_manager_free that reverses that transfer, is safe
for null, and is usable by tests and external callers. Implement
wallet_manager_free(ptr: *mut FFIWalletManager, error: *mut FFIError) as an
extern "C" fn that returns void, checks for null, uses Box::from_raw to reclaim
the allocation, properly shuts down or drops the contained runtime and manager
(drop the Arc<RwLock<WalletManager>> and cleanly stop the
tokio::runtime::Runtime), and set FFIError success/failure via
FFIError::set_success/set_error; ensure symbol name matches wallet_manager_free
expected by tests and FFI consumers.
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 400-445: The error messages in wallet_add_account that direct
callers to removed functions (wallet_add_platform_payment_account,
wallet_add_dashpay_receiving_account,
wallet_add_dashpay_external_account_with_xpub_bytes) must be replaced: update
the match arms for FFIAccountType::PlatformPayment, ::DashpayReceivingFunds and
::DashpayExternalAccount to return a neutral unsupported error (e.g.
FFIAccountResult::error with FFIErrorCode::UnsupportedOperation or InvalidInput
and a message like "Account type <X> is not supported through this FFI surface")
or, if there is a real replacement API, point to that new API instead; apply the
same change to the repeated/error paths later in the file (the other matching
blocks around the 494-547 region) so callers are not pointed at removed entry
points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cb33def-9e6b-4ca9-8cd3-d7e9c34fb5d9
📒 Files selected for processing (19)
key-wallet-ffi/Cargo.tomlkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation.rskey-wallet-ffi/src/bip38.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/mnemonic.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/utils.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rs
💤 Files with no reviewable changes (5)
- key-wallet-ffi/src/utils.rs
- key-wallet-ffi/src/types.rs
- key-wallet-ffi/src/mnemonic.rs
- key-wallet-ffi/src/utxo.rs
- key-wallet-ffi/src/bip38.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
key-wallet-ffi/src/managed_wallet.rs (1)
592-606:⚠️ Potential issue | 🔴 CriticalUpdate the test module or keep a
managed_wallet_freealias.
managed_wallet_info_free()is now the only free function here, but this file's test module still callsmanaged_wallet_free()at Lines 627, 793, 980, and 1076. As written, the module won't compile once the removal lands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_wallet.rs` around lines 592 - 606, The tests still call managed_wallet_free but the implementation now exposes managed_wallet_info_free; either update the test module to call managed_wallet_info_free everywhere (replace usages of managed_wallet_free in the test module) or add a simple FFI alias named managed_wallet_free that forwards to managed_wallet_info_free (maintain the same signature and safety semantics) so existing tests compile; update references to the symbols managed_wallet_free and managed_wallet_info_free accordingly.key-wallet-ffi/src/managed_account.rs (1)
205-214:⚠️ Potential issue | 🟡 MinorFree the intermediate
FFIError.messageafter copying it.Both error paths convert
error.messageinto a new RustString, then allocate a second CString inFFIManagedCoreAccountResult::error(). The originalFFIError.messagebuffer is never released, so repeated lookup failures leak memory.💡 Suggested cleanup
- return FFIManagedCoreAccountResult::error( - error.code, - if error.message.is_null() { - "Failed to get managed wallet info".to_string() - } else { - let c_str = std::ffi::CStr::from_ptr(error.message); - c_str.to_string_lossy().to_string() - }, - ); + let message = if error.message.is_null() { + "Failed to get managed wallet info".to_string() + } else { + let message = std::ffi::CStr::from_ptr(error.message) + .to_string_lossy() + .into_owned(); + error.free_message(); + message + }; + return FFIManagedCoreAccountResult::error(error.code, message);As per coding guidelines,
**/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.Also applies to: 323-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 205 - 214, The code is leaking the original FFIError.message buffer after converting it to a Rust String before calling FFIManagedCoreAccountResult::error; update the branches where you read error.message (e.g., the managed_wallet_ptr null branch and the similar block around lines 323-332) to copy the message into a Rust String, then free the original FFIError.message buffer (using the matching FFI deallocator: e.g., CString::from_raw(error.message as *mut _) or the appropriate libc::free wrapper) before passing the copied String into FFIManagedCoreAccountResult::error so the original C allocation is released.key-wallet-ffi/src/wallet.rs (1)
373-387:⚠️ Potential issue | 🔴 CriticalPreserve a const-safe wallet destructor.
wallet_manager_get_wallet()inkey-wallet-ffi/src/wallet_manager.rs:494-540still returns*const FFIWallet, so removingwallet_free_const()leaves callers with no documented/safe way to release that allocation.💡 Minimal compatibility shim
+#[no_mangle] +pub unsafe extern "C" fn wallet_free_const(wallet: *const FFIWallet) { + wallet_free(wallet as *mut FFIWallet); +}As per coding guidelines,
**/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet.rs` around lines 373 - 387, The FFI exposed destructor removed the const-safe variant: restore a wallet_free_const function that accepts *const FFIWallet (to match wallet_manager_get_wallet which returns *const FFIWallet) and frees the allocation safely by checking for null, casting the const pointer to *mut FFIWallet, and calling Box::from_raw to drop it (mirror wallet_free behavior); keep wallet_free as-is and ensure both functions are #[no_mangle] pub unsafe extern "C" fn with the same safety doc comment so callers holding *const FFIWallet can release memory correctly.key-wallet-ffi/src/transaction.rs (2)
336-340:⚠️ Potential issue | 🔴 Critical
transaction_bytes_freeis freeing the wrong allocation type.
wallet_build_and_sign_transaction()allocates a boxed slice, buttransaction_bytes_free()reconstructs it asBox<u8>. That loses the slice metadata and makes the deallocation undefined behavior at the FFI boundary.💡 One safe fix
-#[no_mangle] -pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8) { - if !tx_bytes.is_null() { - unsafe { - let _ = Box::from_raw(tx_bytes); - } - } -} +#[no_mangle] +pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8, tx_len: usize) { + if !tx_bytes.is_null() && tx_len > 0 { + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(tx_bytes, tx_len)); + } +}As per coding guidelines,
**/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.Also applies to: 504-508
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 336 - 340, The FFI allocator in wallet_build_and_sign_transaction produces a Box<[u8]> (boxed slice) but transaction_bytes_free reconstructs it as Box<u8>, losing slice metadata and causing undefined behavior; update transaction_bytes_free to rebuild the boxed slice using the raw pointer and the known length (recreate a Box<[u8]> via from_raw on a slice created from ptr and len) and then let it drop, and apply the same fix to the other freeing site (the similar free at the other location referenced in the comment) so both deallocations match the original Box<[u8]> allocation.
477-478:⚠️ Potential issue | 🟠 MajorPass the runtime to these FFI functions or use an owned runtime instead of relying on an ambient Tokio context.
tokio::runtime::Handle::current()panics whenwallet_check_transaction()is called from a non-Tokio thread, which is the standard case for Swift/C consumers. The function signature has no way to access the runtime that exists inFFIWalletManager. Either pass a handle to the async work or create a new runtime for the operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 477 - 478, The current use of tokio::runtime::Handle::current() in the wallet_check_transaction path causes panics on non-Tokio threads; modify the API so the runtime is provided or created: update wallet_check_transaction (and the FFIWalletManager boundary) to accept a tokio::runtime::Handle or Arc<Runtime>/Runtime reference and use that handle to block_on managed_info.check_core_transaction(&tx, context, wallet_mut, update_state), or alternatively construct an owned Runtime (tokio::runtime::Runtime::new()) inside wallet_check_transaction and use its block_on; ensure the chosen approach propagates a runtime from FFIWalletManager (or creates/drops an owned runtime) rather than calling Handle::current().key-wallet-ffi/src/wallet_manager.rs (1)
59-80:⚠️ Potential issue | 🔴 CriticalKeep a destructor for
wallet_manager_create().This function still returns an owning raw pointer, but the PR removes
wallet_manager_free(). That turns every manager into a permanent leak, and it is already a compile blocker for existing callers (wallet_manager_freeis the current pipeline failure).💡 Minimal compatibility shim
+#[no_mangle] +pub unsafe extern "C" fn wallet_manager_free(manager: *mut FFIWalletManager) { + if !manager.is_null() { + let _ = Box::from_raw(manager); + } +}As per coding guidelines,
**/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager.rs` around lines 59 - 80, wallet_manager_create returns an owning raw pointer (FFIWalletManager) but there is no destructor anymore, causing leaks and breaking callers; add an extern "C" fn wallet_manager_free(ptr: *mut FFIWalletManager, error: *mut FFIError) that checks for null, wraps the pointer with Box::from_raw to drop the FFIWalletManager (and its Arc/RwLock/runtime), and uses FFIError::set_success(error) on success or FFIError::set_error(..., FFIErrorCode::NullPointer, "null pointer") when ptr is null; ensure the function signature and safety comments match other FFI functions and do not attempt to use the freed pointer after Box::from_raw.key-wallet-ffi/src/keys.rs (1)
9-27:⚠️ Potential issue | 🟡 MinorDead code: structs with unused
innerfields.Pipeline failures indicate
FFIPrivateKey,FFIPublicKey, andFFIExtendedPublicKeyhave unusedinnerfields. Since the associated FFI functions (private_key_free,public_key_free,extended_public_key_free, etc.) were removed, these structs appear to be dead code.Consider removing these unused structs entirely, or if they're needed for future use, suppress the warnings with
#[allow(dead_code)].Option 1: Remove unused structs
-/// Opaque type for a private key (SecretKey) -pub struct FFIPrivateKey { - inner: secp256k1::SecretKey, -} - /// Opaque type for an extended private key pub struct FFIExtendedPrivateKey { inner: key_wallet::bip32::ExtendedPrivKey, } -/// Opaque type for a public key -pub struct FFIPublicKey { - inner: secp256k1::PublicKey, -} - -/// Opaque type for an extended public key -pub struct FFIExtendedPublicKey { - inner: key_wallet::bip32::ExtendedPubKey, -}Option 2: Suppress warnings if needed for future use
/// Opaque type for a private key (SecretKey) +#[allow(dead_code)] pub struct FFIPrivateKey { inner: secp256k1::SecretKey, } /// Opaque type for a public key +#[allow(dead_code)] pub struct FFIPublicKey { inner: secp256k1::PublicKey, } /// Opaque type for an extended public key +#[allow(dead_code)] pub struct FFIExtendedPublicKey { inner: key_wallet::bip32::ExtendedPubKey, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/keys.rs` around lines 9 - 27, The structs FFIPrivateKey, FFIPublicKey, and FFIExtendedPublicKey are flagged for dead code because their inner fields are unused; either delete these struct definitions entirely (remove FFIPrivateKey, FFIPublicKey, FFIExtendedPublicKey from keys.rs and any unused references) or mark them as intentionally unused by adding #[allow(dead_code)] above each struct declaration; keep FFIExtendedPrivateKey untouched if it is still used. Ensure any associated functions/FFI symbols referencing these structs are also removed or updated to avoid dangling references.key-wallet-ffi/src/account_collection.rs (1)
300-579:⚠️ Potential issue | 🔴 CriticalTests reference undefined functions — compilation will fail.
The test module (lines 307–579 in the provided snippet) calls functions that are not defined in this file:
account_collection_count(line 326)account_collection_get_bip44_indices(line 333)account_collection_get_bip44_account(line 338)free_u32_array(line 344)account_collection_has_provider_operator_keys(line 380)account_collection_has_provider_platform_keys(line 426)account_collection_get_provider_platform_keys(line 428)account_collection_summary(lines 496, 554, 576)The actual API provides
account_collection_summary_dataandaccount_collection_summary_free. Either update these tests to use the correct functions or remove them if they are obsolete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account_collection.rs` around lines 300 - 579, The tests call several undefined functions; update the test code to use the real API or remove obsolete calls: replace account_collection_summary(...) usages with account_collection_summary_data(...) and then free the returned summary via account_collection_summary_free(...); for account_collection_count, account_collection_get_bip44_indices, account_collection_get_bip44_account, account_collection_has_provider_operator_keys, account_collection_has_provider_platform_keys, account_collection_get_provider_platform_keys and free_u32_array either change the tests to call the corresponding real functions from the public API (or their correct crate::... paths) or remove those assertions if those helpers no longer exist—ensure any new summary/data pointer is converted via CStr::from_ptr and freed with account_collection_summary_free to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet-ffi/src/account_collection.rs`:
- Around line 300-579: The tests call several undefined functions; update the
test code to use the real API or remove obsolete calls: replace
account_collection_summary(...) usages with account_collection_summary_data(...)
and then free the returned summary via account_collection_summary_free(...); for
account_collection_count, account_collection_get_bip44_indices,
account_collection_get_bip44_account,
account_collection_has_provider_operator_keys,
account_collection_has_provider_platform_keys,
account_collection_get_provider_platform_keys and free_u32_array either change
the tests to call the corresponding real functions from the public API (or their
correct crate::... paths) or remove those assertions if those helpers no longer
exist—ensure any new summary/data pointer is converted via CStr::from_ptr and
freed with account_collection_summary_free to avoid leaks.
In `@key-wallet-ffi/src/keys.rs`:
- Around line 9-27: The structs FFIPrivateKey, FFIPublicKey, and
FFIExtendedPublicKey are flagged for dead code because their inner fields are
unused; either delete these struct definitions entirely (remove FFIPrivateKey,
FFIPublicKey, FFIExtendedPublicKey from keys.rs and any unused references) or
mark them as intentionally unused by adding #[allow(dead_code)] above each
struct declaration; keep FFIExtendedPrivateKey untouched if it is still used.
Ensure any associated functions/FFI symbols referencing these structs are also
removed or updated to avoid dangling references.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 205-214: The code is leaking the original FFIError.message buffer
after converting it to a Rust String before calling
FFIManagedCoreAccountResult::error; update the branches where you read
error.message (e.g., the managed_wallet_ptr null branch and the similar block
around lines 323-332) to copy the message into a Rust String, then free the
original FFIError.message buffer (using the matching FFI deallocator: e.g.,
CString::from_raw(error.message as *mut _) or the appropriate libc::free
wrapper) before passing the copied String into
FFIManagedCoreAccountResult::error so the original C allocation is released.
In `@key-wallet-ffi/src/managed_wallet.rs`:
- Around line 592-606: The tests still call managed_wallet_free but the
implementation now exposes managed_wallet_info_free; either update the test
module to call managed_wallet_info_free everywhere (replace usages of
managed_wallet_free in the test module) or add a simple FFI alias named
managed_wallet_free that forwards to managed_wallet_info_free (maintain the same
signature and safety semantics) so existing tests compile; update references to
the symbols managed_wallet_free and managed_wallet_info_free accordingly.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 336-340: The FFI allocator in wallet_build_and_sign_transaction
produces a Box<[u8]> (boxed slice) but transaction_bytes_free reconstructs it as
Box<u8>, losing slice metadata and causing undefined behavior; update
transaction_bytes_free to rebuild the boxed slice using the raw pointer and the
known length (recreate a Box<[u8]> via from_raw on a slice created from ptr and
len) and then let it drop, and apply the same fix to the other freeing site (the
similar free at the other location referenced in the comment) so both
deallocations match the original Box<[u8]> allocation.
- Around line 477-478: The current use of tokio::runtime::Handle::current() in
the wallet_check_transaction path causes panics on non-Tokio threads; modify the
API so the runtime is provided or created: update wallet_check_transaction (and
the FFIWalletManager boundary) to accept a tokio::runtime::Handle or
Arc<Runtime>/Runtime reference and use that handle to block_on
managed_info.check_core_transaction(&tx, context, wallet_mut, update_state), or
alternatively construct an owned Runtime (tokio::runtime::Runtime::new()) inside
wallet_check_transaction and use its block_on; ensure the chosen approach
propagates a runtime from FFIWalletManager (or creates/drops an owned runtime)
rather than calling Handle::current().
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Around line 59-80: wallet_manager_create returns an owning raw pointer
(FFIWalletManager) but there is no destructor anymore, causing leaks and
breaking callers; add an extern "C" fn wallet_manager_free(ptr: *mut
FFIWalletManager, error: *mut FFIError) that checks for null, wraps the pointer
with Box::from_raw to drop the FFIWalletManager (and its Arc/RwLock/runtime),
and uses FFIError::set_success(error) on success or FFIError::set_error(...,
FFIErrorCode::NullPointer, "null pointer") when ptr is null; ensure the function
signature and safety comments match other FFI functions and do not attempt to
use the freed pointer after Box::from_raw.
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 373-387: The FFI exposed destructor removed the const-safe
variant: restore a wallet_free_const function that accepts *const FFIWallet (to
match wallet_manager_get_wallet which returns *const FFIWallet) and frees the
allocation safely by checking for null, casting the const pointer to *mut
FFIWallet, and calling Box::from_raw to drop it (mirror wallet_free behavior);
keep wallet_free as-is and ensure both functions are #[no_mangle] pub unsafe
extern "C" fn with the same safety doc comment so callers holding *const
FFIWallet can release memory correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba110343-51de-40e3-9c0f-33d4163aa08d
📒 Files selected for processing (19)
key-wallet-ffi/Cargo.tomlkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation.rskey-wallet-ffi/src/bip38.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/mnemonic.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/utils.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rs
💤 Files with no reviewable changes (5)
- key-wallet-ffi/src/utxo.rs
- key-wallet-ffi/src/utils.rs
- key-wallet-ffi/src/types.rs
- key-wallet-ffi/src/bip38.rs
- key-wallet-ffi/src/mnemonic.rs
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
1 participant
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.
I wanted to see what we are currently exposing through FFI versus what we are actually using. This is not meant to be merged (yet), but rather to serve as a guide for what is not yet implemented or may be unnecessary.
-I didn’t update the tests; I will remove those that test removed methods.
Summary by CodeRabbit
Breaking Changes
Refactor