feat(dash-spv-ffi): transaction broadcast ffi function#458
Merged
xdustinface merged 1 commit intov0.42-devfrom Feb 21, 2026
Merged
feat(dash-spv-ffi): transaction broadcast ffi function#458xdustinface merged 1 commit intov0.42-devfrom
xdustinface merged 1 commit intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new FFI function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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: 1
🧹 Nitpick comments (1)
dash-spv-ffi/src/client.rs (1)
489-489: Consider guarding againstlength == 0before creating the slice.While
from_raw_partswithlength == 0is technically safe (no memory is read), and deserialization would fail anyway, an explicit zero-length check would provide a clearer error message to FFI callers ("empty transaction data") rather than a cryptic deserialization error.🛡️ Optional: add zero-length guard
null_check!(client); null_check!(tx_bytes); + if length == 0 { + set_last_error("Invalid transaction: empty transaction data"); + return FFIErrorCode::InvalidArgument as i32; + } + let tx_bytes = std::slice::from_raw_parts(tx_bytes, length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` at line 489, Add an explicit guard before the slice creation: check if length == 0 before the line that creates tx_bytes with std::slice::from_raw_parts, and if so return an FFI-friendly error (e.g., propagate the module’s existing error/return pattern with a clear message like "empty transaction data") instead of attempting deserialization; this ensures the tx_bytes/length check happens early and callers receive a descriptive error rather than a cryptic deserialization failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/src/client.rs`:
- Around line 473-512: Add unit and C tests covering
dash_spv_ffi_client_broadcast_transaction: create a Rust unit test file
(tests/unit/test_broadcast_transaction.rs) with tests that (1) call
dash_spv_ffi_client_broadcast_transaction with a valid serialized
dashcore::Transaction and a test FFIDashSpvClient whose inner DashSpvClient mock
returns Ok => assert return is FFIErrorCode::Success and no last error, (2) call
with malformed tx bytes => assert it returns FFIErrorCode::InvalidArgument and
set_last_error contains "Invalid transaction", and (3) mock
inner.broadcast_transaction to return an error => assert returned code equals
FFIErrorCode::from(err) and last error contains "Failed to broadcast
transaction". Also add a C test (tests/c_tests/test_broadcast_transaction.c)
that links the FFI, invokes dash_spv_ffi_client_broadcast_transaction for the
same three scenarios, checks return codes and reads error text via
dash_spv_ffi_get_last_error, and cleans up the test client; reference the
function dash_spv_ffi_client_broadcast_transaction, the FFIDashSpvClient
construction helper used elsewhere in tests, set_last_error, and
dash_spv_ffi_get_last_error to locate relevant helpers. Ensure mocks or test
helpers initialize client.runtime consistently with other tests.
---
Nitpick comments:
In `@dash-spv-ffi/src/client.rs`:
- Line 489: Add an explicit guard before the slice creation: check if length ==
0 before the line that creates tx_bytes with std::slice::from_raw_parts, and if
so return an FFI-friendly error (e.g., propagate the module’s existing
error/return pattern with a clear message like "empty transaction data") instead
of attempting deserialization; this ensures the tx_bytes/length check happens
early and callers receive a descriptive error rather than a cryptic
deserialization failure.
Comment on lines
+473
to
+512
| /// Broadcasts a transaction to the Dash network via connected peers. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// - `client` must be a valid, non-null pointer to an initialized FFIDashSpvClient | ||
| /// - `tx_bytes` must be a valid, non-null pointer to the transaction data | ||
| /// - `length` must be the length of the transaction data in bytes | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn dash_spv_ffi_client_broadcast_transaction( | ||
| client: *mut FFIDashSpvClient, | ||
| tx_bytes: *const u8, | ||
| length: usize, | ||
| ) -> i32 { | ||
| null_check!(client); | ||
| null_check!(tx_bytes); | ||
|
|
||
| let tx_bytes = std::slice::from_raw_parts(tx_bytes, length); | ||
|
|
||
| let tx = match dashcore::consensus::deserialize::<dashcore::Transaction>(tx_bytes) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| set_last_error(&format!("Invalid transaction: {}", e)); | ||
| return FFIErrorCode::InvalidArgument as i32; | ||
| } | ||
| }; | ||
|
|
||
| let client = &(*client); | ||
|
|
||
| let spv_client = client.inner.clone(); | ||
|
|
||
| let result = client.runtime.block_on(async { spv_client.broadcast_transaction(&tx).await }); | ||
|
|
||
| match result { | ||
| Ok(_) => FFIErrorCode::Success as i32, | ||
| Err(e) => { | ||
| set_last_error(&format!("Failed to broadcast transaction: {}", e)); | ||
| FFIErrorCode::from(e) as i32 | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for broadcast_transaction method definition ==="
rg -n 'fn broadcast_transaction' --type=rust -C 3
echo ""
echo "=== Searching for unit tests related to broadcast_transaction ==="
rg -n 'broadcast_transaction' --type=rust dash-spv-ffi/tests/ 2>/dev/null || echo "No matches in tests/"
echo ""
echo "=== Checking dash-spv-ffi test directory structure ==="
find dash-spv-ffi/tests -type f -name "*.rs" 2>/dev/null | head -20
echo ""
echo "=== Checking for test modules in client.rs ==="
rg -n '#\[cfg\(test\)\]' dash-spv-ffi/src/client.rs -A 30 || echo "No test module found in client.rs"Repository: dashpay/rust-dashcore
Length of output: 2128
🏁 Script executed:
#!/bin/bash
echo "=== Checking for C tests directory ==="
find dash-spv-ffi/tests -type d -name 'c_tests' -o -type d -name '*c*test*'
echo ""
echo "=== Checking if broadcast_transaction FFI function appears in C tests ==="
fd -t f '\.(c|h)$' dash-spv-ffi/tests/ -x grep -l 'broadcast_transaction' {} \;
echo ""
echo "=== Looking at the test file that uses broadcast_transaction ==="
head -50 dash-spv-ffi/tests/integration/test_full_workflow.rs | tail -30Repository: dashpay/rust-dashcore
Length of output: 1459
Add unit and C tests for dash_spv_ffi_client_broadcast_transaction.
The implementation is correct and follows existing FFI patterns. The broadcast_transaction method exists on DashSpvClient and accepts the expected &dashcore::Transaction parameter.
However, per project conventions, new FFI functions require:
- Unit tests in
tests/unit/(currently missing; only integration and security tests exist) - C tests in
tests/c_tests/(currently missing)
Existing test coverage in tests/integration/test_full_workflow.rs and tests/security/test_security.rs is helpful but does not satisfy the unit test requirements outlined in the coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv-ffi/src/client.rs` around lines 473 - 512, Add unit and C tests
covering dash_spv_ffi_client_broadcast_transaction: create a Rust unit test file
(tests/unit/test_broadcast_transaction.rs) with tests that (1) call
dash_spv_ffi_client_broadcast_transaction with a valid serialized
dashcore::Transaction and a test FFIDashSpvClient whose inner DashSpvClient mock
returns Ok => assert return is FFIErrorCode::Success and no last error, (2) call
with malformed tx bytes => assert it returns FFIErrorCode::InvalidArgument and
set_last_error contains "Invalid transaction", and (3) mock
inner.broadcast_transaction to return an error => assert returned code equals
FFIErrorCode::from(err) and last error contains "Failed to broadcast
transaction". Also add a C test (tests/c_tests/test_broadcast_transaction.c)
that links the FFI, invokes dash_spv_ffi_client_broadcast_transaction for the
same three scenarios, checks return codes and reads error text via
dash_spv_ffi_get_last_error, and cleans up the test client; reference the
function dash_spv_ffi_client_broadcast_transaction, the FFIDashSpvClient
construction helper used elsewhere in tests, set_last_error, and
dash_spv_ffi_get_last_error to locate relevant helpers. Ensure mocks or test
helpers initialize client.runtime consistently with other tests.
xdustinface
approved these changes
Feb 21, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Agree with CodeRabbit, a test would have been nice.
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
2 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.
Summary by CodeRabbit
New Features
Refactor