suggestion(dash-spv-ffi): unused ffi functions removed from dash-spv-ffi crate #377
Conversation
📝 WalkthroughWalkthroughThe pull request significantly reduces the FFI API surface by removing 25+ exported C functions, several type definitions (FFIArray, FFIUnconfirmedTransaction, DashSpvValidationMode, CoreSDKHandle), and extensive test coverage. Removed functionalities include config getters/setters, platform integration handles, checkpoint queries, and memory management helpers. The remaining API focuses on core client lifecycle and synchronization control. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
I think it's a good plan. If we don't need/use it in there at the moment i agree there is no reason to keep it. Easy enough to bring it back and even improve on it if we actually need it and know how and where we actually want to use it.
ZocoLini
force-pushed
the
chore/ffi-cleanup-example
branch
2 times, most recently
from
abfa99e to
8714ec8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv-ffi/tests/unit/test_error_handling.rs (1)
109-133: Correct misleading comment at line 130 about "public API".The comment "Clear using public API" is incorrect—
clear_last_error()is an internal Rust function (no#[no_mangle]decorator) and is not exported to C callers.The actual design pattern is that errors are automatically cleared on successful FFI operations (confirmed by test assertions at lines 170 and 178). However, C callers have no way to manually clear the error state between failed operations, which the comment misrepresents. Update the comment to clarify that this tests the internal clearing mechanism, not a public API function:
// Clear using internal function (not public API) clear_last_error();dash-spv-ffi/src/client.rs (1)
537-571: Restore the client handle before early returns.If the initial
sync_progress()call fails, the function returns without puttingspv_clientback intoclient.inner, leaving the client in aNonestate and dropping the running instance.🐛 Proposed fix
- let start_height = match spv_client.sync_progress().await { - Ok(progress) => progress.header_height, - Err(e) => { - tracing::error!("Failed to get initial height: {}", e); - return Err(e); - } - }; + let start_height = match spv_client.sync_progress().await { + Ok(progress) => progress.header_height, + Err(e) => { + tracing::error!("Failed to get initial height: {}", e); + let mut guard = client.inner.lock().unwrap(); + *guard = Some(spv_client); + return Err(e); + } + };
🤖 Fix all issues with AI agents
In `@dash-spv-ffi/src/bin/ffi_cli.rs`:
- Around line 196-199: The loop's completion check can hang if filter sync is
disabled because filters_complete is computed unconditionally; update the
condition that sets filters_complete to only require filter heights when
filter_sync_available is true (e.g., compute filters_complete as
filter_sync_available && prog.filter_header_height >= prog.header_height &&
prog.last_synced_filter_height >= prog.filter_header_height), and then use that
guarded filters_complete together with SYNC_COMPLETED.load(Ordering::SeqCst) in
the if check so the loop can complete when filters are not available.
🧹 Nitpick comments (3)
dash-spv-ffi/src/types.rs (1)
45-52: Consider narrowing visibility of the internal string destructor.This helper is only used inside the crate, but it’s publicly exported with an FFI-style name. Making it
pub(crate)avoids suggesting a public API surface that was intentionally reduced.♻️ Proposed tweak
-pub unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { +pub(crate) unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { if !s.ptr.is_null() { let _ = CString::from_raw(s.ptr); } }dash-spv-ffi/tests/test_platform_integration_safety.rs (1)
1-8: Module docstring claims coverage that was removed.The module docstring at lines 3-8 claims to test "Memory safety (double-free, use-after-free)" but the
test_memory_safety_patternsfunction (lines 207-227) now only tests buffer overflow prevention. The use-after-free and double-free tests appear to have been removed as part of this cleanup.Consider updating the docstring to reflect the actual test coverage, or add a note that these scenarios are covered elsewhere.
📝 Suggested docstring update
//! Comprehensive safety tests for platform_integration FFI functions //! //! Tests focus on: //! - Null pointer handling //! - Buffer overflow prevention -//! - Memory safety (double-free, use-after-free) +//! - Memory safety (buffer overflow prevention) //! - Thread safety //! - Error propagationdash-spv-ffi/tests/unit/test_async_operations.rs (1)
243-282: Redundant innerunsafeblock after addingunsafeto function signature.Adding
unsafeto theextern "C"function signature is a good practice to document that it performs unsafe operations. However, the innerunsafeblock at line 248 is now redundant since the entire function body is already an unsafe context.♻️ Remove redundant inner unsafe block
unsafe extern "C" fn reentrant_callback( _success: bool, _error: *const c_char, user_data: *mut c_void, ) { - let data = unsafe { &*(user_data as *const ReentrantData) }; + let data = &*(user_data as *const ReentrantData); let count = data.count.fetch_add(1, Ordering::SeqCst);
| let headers_done = SYNC_COMPLETED.load(Ordering::SeqCst); | ||
| let filters_complete = if disable_filter_sync || !prog.filter_sync_available { | ||
| false | ||
| } else { | ||
| prog.filter_header_height >= prog.header_height | ||
| && prog.last_synced_filter_height >= prog.filter_header_height | ||
| }; | ||
| if headers_done && (filters_complete || disable_filter_sync) { | ||
| let filters_complete = prog.filter_header_height >= prog.header_height | ||
| && prog.last_synced_filter_height >= prog.filter_header_height; | ||
| if headers_done && filters_complete { |
There was a problem hiding this comment.
Prevent a potential hang when filter sync is unavailable.
If filter_sync_available is false, filter heights can stay at 0 while header_height advances, so filters_complete never becomes true and the polling loop can run forever. Gate the condition on filter_sync_available (or otherwise allow completion without filters).
🐛 Proposed fix
- let filters_complete = prog.filter_header_height >= prog.header_height
- && prog.last_synced_filter_height >= prog.filter_header_height;
+ let filters_complete = !prog.filter_sync_available
+ || (prog.filter_header_height >= prog.header_height
+ && prog.last_synced_filter_height >= prog.filter_header_height);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let headers_done = SYNC_COMPLETED.load(Ordering::SeqCst); | |
| let filters_complete = if disable_filter_sync || !prog.filter_sync_available { | |
| false | |
| } else { | |
| prog.filter_header_height >= prog.header_height | |
| && prog.last_synced_filter_height >= prog.filter_header_height | |
| }; | |
| if headers_done && (filters_complete || disable_filter_sync) { | |
| let filters_complete = prog.filter_header_height >= prog.header_height | |
| && prog.last_synced_filter_height >= prog.filter_header_height; | |
| if headers_done && filters_complete { | |
| let headers_done = SYNC_COMPLETED.load(Ordering::SeqCst); | |
| let filters_complete = !prog.filter_sync_available | |
| || (prog.filter_header_height >= prog.header_height | |
| && prog.last_synced_filter_height >= prog.filter_header_height); | |
| if headers_done && filters_complete { |
🤖 Prompt for AI Agents
In `@dash-spv-ffi/src/bin/ffi_cli.rs` around lines 196 - 199, The loop's
completion check can hang if filter sync is disabled because filters_complete is
computed unconditionally; update the condition that sets filters_complete to
only require filter heights when filter_sync_available is true (e.g., compute
filters_complete as filter_sync_available && prog.filter_header_height >=
prog.header_height && prog.last_synced_filter_height >=
prog.filter_header_height), and then use that guarded filters_complete together
with SYNC_COMPLETED.load(Ordering::SeqCst) in the if check so the loop can
complete when filters are not available.
This are all the functions that are not being used in any place in platform repo, branch feat/iOSSupport along with structs or functions that ended up being unused after the removals. For sure there are more thing that are not being used in the crate but I wanted to focus on the ones not used when this is added as a dependency.
IMO, if we need something in a future to implement a feature though ffi we can recover it from this PR or re-implemente it with the API we had at that moment in time. Having this many things makes everything harder to work with by adding a lot fo noise and probably useless changes
Test updated removing references to the functions dropped. Discovered a LOT of stuff being tested more than once and tests that are basically useless IMO but didnt touch those, just making a note about things that can be improved
Summary by CodeRabbit
Release Notes
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.