feat(dash-spv-ffi): fixed building and signing a transaction and implemented exposed broadcasting in the FFI

[ZocoLini]

• Broadcast transaction exposed through FFI
• dropeed create_transactiona nd sign transaction in the FFI leaving only one way to create signed transaction (they were not implemented or 'deprecatred' anyway)
• buildSignedTransaction method modified to be able to buid a valid transaction fixing bugs in the process

Summary by CodeRabbit

• New Features

  • Transaction broadcasting to the Dash network.
  • Predefined fee-rate options: Economy, Normal, Priority.

• Updates

  • Unified transaction build-and-sign API (manager-based) replacing separate build/sign calls.
  • Centralized, improved fee estimation for transactions.

• Tests

  • Removed legacy transaction test suite.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a transaction broadcast FFI to the SPV client and refactors wallet transaction APIs to a manager-based single build-and-sign function with FFIFeeRate. Centralizes fee calculation in TransactionBuilder and removes related FFI tests.

Changes

Cohort / File(s)	Summary
Transaction Broadcast dash-spv-ffi/FFI_API.md, dash-spv-ffi/include/dash_spv_ffi.h, dash-spv-ffi/src/client.rs	Adds dash_spv_ffi_client_broadcast_transaction(client: *mut FFIDashSpvClient, tx_bytes: *const u8, length: usize) -> i32. Validates pointers, deserializes tx bytes, broadcasts via SPV runtime, and records errors.
Wallet FFI: Build & Sign Refactor key-wallet-ffi/FFI_API.md, key-wallet-ffi/include/key_wallet_ffi.h, key-wallet-ffi/src/transaction.rs	Removes wallet_build_transaction and wallet_sign_transaction. Replaces with wallet_build_and_sign_transaction that accepts const FFIWalletManager *manager, uint32_t account_index, FFIFeeRate fee_rate, and uint64_t *fee_out. Adds FFIFeeRate enum (ECONOMY, NORMAL, PRIORITY) and maps it to internal FeeRate. Updates validation and manager-driven flow.
Tests Removed key-wallet-ffi/src/transaction_tests.rs	Deletes the transaction FFI test module (all tests removed).
Fee Calculation Centralized key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs	Adds TransactionBuilder::calculate_fee(&self) -> u64 and replaces inline fee computations with calls to this method, using estimated tx size × configured fee rate.

Sequence Diagram(s)

sequenceDiagram
    participant FFI as FFI Caller
    participant Client as FFIDashSpvClient
    participant Runtime as SPV Runtime
    participant Network as Dash Network

    FFI->>Client: dash_spv_ffi_client_broadcast_transaction(tx_bytes, length)
    Client->>Client: validate pointers & length
    Client->>Client: deserialize tx bytes -> Transaction
    Client->>Runtime: schedule broadcast(Transaction)
    Runtime->>Network: propagate transaction to peers
    Network-->>Runtime: ack / error
    Runtime-->>Client: result (ok / error)
    Client-->>FFI: return status code

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through headers, bytes in paw,

Broadcast bound and signatures law.
Fees now summed with one true call,
Tests cleared out to tidy the hall.
A little hop — the network hears my paw.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: adding transaction broadcasting FFI functionality and fixing the transaction building/signing path.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/transaction-broadcasting-ffi

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (2)

dash-spv-ffi/FFI_API.md (1)

601-603: Minor type inconsistency in the detailed signature block

The generated C API declares length as uintptr_t, but the doc block shows the Rust type usize. These are ABI-equivalent but may confuse C callers reading this documentation. Since the file is labelled as auto-generated, this should ideally be fixed at the generation step.

-dash_spv_ffi_client_broadcast_transaction(client: *mut FFIDashSpvClient, tx_bytes: *const u8, length: usize,) -> i32
+dash_spv_ffi_client_broadcast_transaction(client: *mut FFIDashSpvClient, tx_bytes: *const u8, length: uintptr_t,) -> i32

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dash-spv-ffi/FFI_API.md` around lines 601 - 603, The documentation shows the
Rust type `usize` for the parameter `length` of
`dash_spv_ffi_client_broadcast_transaction` while the generated C API uses
`uintptr_t`, which is inconsistent; update the auto-generation step that
produces FFI_API.md to render Rust's `usize` as the C ABI type used (e.g.,
`uintptr_t` or `size_t`) so the doc signature matches the emitted C signature —
specifically change the generator/template that formats
`dash_spv_ffi_client_broadcast_transaction`'s `length` param to output
`uintptr_t` (or the consistent C alias) instead of `usize`.

key-wallet-ffi/FFI_API.md (1)

4363-4374: FFIFeeRate is missing from the Type Definitions section.

FFIFeeRate (introduced in this PR) is referenced in the wallet_build_and_sign_transaction signature but does not appear in the Type Definitions → Core Types list. Since the section is otherwise thin, adding an entry would help consumers discover the type without grepping the header.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@key-wallet-ffi/FFI_API.md` around lines 4363 - 4374, Add the missing
FFIFeeRate entry to the "Type Definitions → Core Types" list so consumers can
discover it without searching; update the Core Types list to include a short
description like "FFIFeeRate - Fee rate structure used in transaction building"
and ensure it references the same type used by the
wallet_build_and_sign_transaction signature so readers can correlate the
function (wallet_build_and_sign_transaction) with the FFIFeeRate type.

🤖 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/include/dash_spv_ffi.h`:
- Around line 614-618: The safety note in the Rust doc comment that cbindgen
uses to generate dash_spv_ffi.h is missing the requirement that tx_bytes be
valid for at least length bytes; edit the doc comment in client.rs for the
function that accepts a pointer to FFIDashSpvClient and the tx_bytes/length
parameters to add a bullet stating "`tx_bytes` must be a valid, non-null pointer
to the transaction data and must be valid for at least `length` bytes", then
regenerate the C header with cbindgen so the updated safety text appears in
dash_spv_ffi.h.

In `@dash-spv-ffi/src/client.rs`:
- Around line 572-575: Update the function safety doc for the FFIDashSpvClient
FFI entry that takes parameters named client, tx_bytes, and length to explicitly
state that the tx_bytes pointer must be non-null and valid for at least length
bytes (i.e., safe to read length bytes), in addition to existing constraints;
reference the std::slice::from_raw_parts usage in the implementation to justify
the added validity requirement so the cbindgen-generated header includes this
important safety note.

In `@key-wallet-ffi/FFI_API.md`:
- Around line 1287-1295: The docstring for wallet_build_and_sign_transaction is
missing documentation for the fee_out: *mut u64 parameter; update the Rust
docstring in key-wallet-ffi/src/transaction.rs (function
wallet_build_and_sign_transaction) to mention fee_out in both the Description
and Safety sections, stating it is an output pointer for the calculated fee (in
satoshis), must be a valid non-null pointer provided by the caller, and will be
written to by the function (note the implementation checks fee_out and writes to
it). Ensure the wording parallels the other pointer params (manager, wallet,
outputs, tx_bytes_out, tx_len_out, error) so callers know to provide a valid
pointer.

In `@key-wallet-ffi/src/transaction.rs`:
- Around line 183-191: The loop that converts output.address using
CStr::from_ptr must first guard against a null pointer: in the loop over
outputs_slice (the code that calls CStr::from_ptr(output.address) and assigns
address_str), add an explicit check if output.address.is_null() and if so call
FFIError::set_error(error, FFIErrorCode::InvalidInput, "Null
output.address".to_string()) and return or continue as appropriate for the
function's error handling policy; only call CStr::from_ptr when the pointer is
non-null and then handle UTF-8 errors as before.
- Around line 337-343: The FFI currently turns serialized.into_boxed_slice()
into a raw pointer via Box::into_raw and writes it to tx_bytes_out, but the
deallocation code reconstructs it as Box<u8> (Box::from_raw(tx_bytes)), causing
a layout mismatch; fix by choosing one consistent allocation/deallocation
strategy: either allocate with libc::malloc and return the pointer (so the
receiver must free with libc::free), or continue using boxed slices but also
return/store the length and reconstruct the same Box<[u8]> on deallocation using
Box::from_raw(std::slice::from_raw_parts_mut(tx_bytes, size)); update the code
paths that use serialized.into_boxed_slice, tx_bytes_out and the corresponding
free path to use the chosen strategy so allocation and deallocation types match.
- Around line 138-151: The code currently clones ManagedWalletInfo via
manager.get_wallet_info(...) and mutates the clone with
next_change_address(Some(&xpub), true), but those mutations are lost; instead
obtain and mutate the manager-held instance so changes persist: acquire the
write lock (manager_ref.manager.write().await), use
manager.get_wallet_info_mut(&wallet_ref.inner().wallet_id) (or otherwise get a
mutable reference to ManagedWalletInfo) and call next_change_address on that
mutable instance (or replace the stored value with the mutated struct via
get_wallet_info_mut) so the manager's state is updated before returning; ensure
the same pattern is applied to the other occurrence around lines 238–249.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 359-363: calculate_fee() always assumes a change output by calling
estimate_transaction_size(self.inputs.len(), self.outputs.len() + 1) which
diverges when the computed change is dust or omitted; update the logic so after
computing fee and change_amount you check if change_amount <= DUST_THRESHOLD
(546) and if so re-estimate size without a change output (use outputs.len()
instead of outputs.len()+1), recompute the fee via
fee_rate.calculate_fee(new_size), set change_amount to 0 (and ensure fee_out
reflects the recomputed fee), and recompute any dependent values; apply the same
pattern wherever fee and change are calculated (including the block around
functions/lines referenced by the reviewer: calculate_fee(),
estimate_transaction_size(), and the code paths at the other region noted
422-434).

---

Nitpick comments:
In `@dash-spv-ffi/FFI_API.md`:
- Around line 601-603: The documentation shows the Rust type `usize` for the
parameter `length` of `dash_spv_ffi_client_broadcast_transaction` while the
generated C API uses `uintptr_t`, which is inconsistent; update the
auto-generation step that produces FFI_API.md to render Rust's `usize` as the C
ABI type used (e.g., `uintptr_t` or `size_t`) so the doc signature matches the
emitted C signature — specifically change the generator/template that formats
`dash_spv_ffi_client_broadcast_transaction`'s `length` param to output
`uintptr_t` (or the consistent C alias) instead of `usize`.

In `@key-wallet-ffi/FFI_API.md`:
- Around line 4363-4374: Add the missing FFIFeeRate entry to the "Type
Definitions → Core Types" list so consumers can discover it without searching;
update the Core Types list to include a short description like "FFIFeeRate - Fee
rate structure used in transaction building" and ensure it references the same
type used by the wallet_build_and_sign_transaction signature so readers can
correlate the function (wallet_build_and_sign_transaction) with the FFIFeeRate
type.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | head -160

Repository: dashpay/rust-dashcore

Length of output: 6053

---

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | sed -n '230,260p'

Repository: dashpay/rust-dashcore

Length of output: 1318

---

🏁 Script executed:

rg -n "get_wallet_info" key-wallet-ffi/src/ -C 3

Repository: dashpay/rust-dashcore

Length of output: 1154

---

🏁 Script executed:

rg -n "set_wallet_info|update_wallet_info|put_wallet_info|store_wallet_info" key-wallet-ffi/src/ -C 3

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

rg -n "next_change_address" key-wallet-ffi/src/ -C 3

Repository: dashpay/rust-dashcore

Length of output: 3827

---

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | sed -n '250,330p'

Repository: dashpay/rust-dashcore

Length of output: 3441

---

🏁 Script executed:

rg -n "impl.*ManagedAccount" key-wallet/src/ -A 20 | head -100

Repository: dashpay/rust-dashcore

Length of output: 9418

---

🏁 Script executed:

fd "managed_account.rs" key-wallet/src/ | head -5

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

fd "\.rs" key-wallet/src/wallet/managed_wallet_info/ --type f

Repository: dashpay/rust-dashcore

Length of output: 607

---

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | sed -n '320,360p'

Repository: dashpay/rust-dashcore

Length of output: 1441

---

🏁 Script executed:

fd "managed_account.rs" key-wallet/src/wallet/managed_wallet_info/

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

cat key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs | head -200

Repository: dashpay/rust-dashcore

Length of output: 6423

---

🏁 Script executed:

rg -n "next_change_address" key-wallet/src/wallet/managed_wallet_info/ -B 2 -A 8

Repository: dashpay/rust-dashcore

Length of output: 1274

---

🏁 Script executed:

rg -n "fn next_change_address" key-wallet/src/ -A 15

Repository: dashpay/rust-dashcore

Length of output: 2755

---

🏁 Script executed:

fd "managed_account" key-wallet/src/wallet/managed_wallet_info/ --type f

Repository: dashpay/rust-dashcore

Length of output: 198

---

🏁 Script executed:

find key-wallet/src -name "*.rs" -type f -exec grep -l "struct ManagedAccount" {} \;

Repository: dashpay/rust-dashcore

Length of output: 125

---

🏁 Script executed:

cat -n key-wallet/src/managed_account/mod.rs | sed -n '479,530p'

Repository: dashpay/rust-dashcore

Length of output: 2617

---

🏁 Script executed:

cat -n key-wallet/src/managed_account/mod.rs | sed -n '530,560p'

Repository: dashpay/rust-dashcore

Length of output: 1618

---

🏁 Script executed:

rg -n "add_to_state" key-wallet/src/managed_account/mod.rs -B 2 -A 8

Repository: dashpay/rust-dashcore

Length of output: 9317

---

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | sed -n '335,348p'

Repository: dashpay/rust-dashcore

Length of output: 461

---

🏁 Script executed:

rg -n "set_wallet_info|put_wallet_info|update_wallet" key-wallet-ffi/src/wallet_manager.rs -A 5

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

cat -n key-wallet-ffi/src/wallet_manager.rs | sed -n '1,100p'

Repository: dashpay/rust-dashcore

Length of output: 3650

---

🏁 Script executed:

rg -n "fn get_wallet_info" key-wallet-ffi/src/ -A 10

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

rg -n "struct WalletManager" key-wallet-manager/src/ -A 30 | head -80

Repository: dashpay/rust-dashcore

Length of output: 2670

---

🏁 Script executed:

cat key-wallet-ffi/CLAUDE.md 2>/dev/null || echo "File not found"

Repository: dashpay/rust-dashcore

Length of output: 79

---

🏁 Script executed:

rg -n "update_wallet_info|put_wallet_info|set_wallet_info" key-wallet-manager/src/ -A 5

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

rg -n "get_wallet_info" key-wallet-manager/src/wallet_manager/ -A 10

Repository: dashpay/rust-dashcore

Length of output: 1451

---

🏁 Script executed:

cat key-wallet-ffi/CLAUDE.md 2>/dev/null | head -100

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

rg -n "get_wallet_info_mut" key-wallet-ffi/src/ -B 2 -A 8

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

cat key-wallet/CLAUDE.md 2>/dev/null | head -50

Repository: dashpay/rust-dashcore

Length of output: 1927

---

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | sed -n '138,250p'

Repository: dashpay/rust-dashcore

Length of output: 4582

---

🏁 Script executed:

rg -n "get_wallet_info_mut" key-wallet-manager/src/ -B 2 -A 5

Repository: dashpay/rust-dashcore

Length of output: 757

---

🏁 Script executed:

cd key-wallet-ffi && grep -r "get_wallet_info_mut\|update_wallet" src/ || echo "Not found"

Repository: dashpay/rust-dashcore

Length of output: 74

---

🏁 Script executed:

cat -n key-wallet-ffi/src/transaction.rs | sed -n '240,348p'

Repository: dashpay/rust-dashcore

Length of output: 4398

---

🏁 Script executed:

rg -n "managed_wallet_info.*clone|\.cloned\(\)" key-wallet-ffi/src/ | head -20

Repository: dashpay/rust-dashcore

Length of output: 628

---

Persist address state mutations after transaction building.

The cloned ManagedWalletInfo at line 141 is mutated via next_change_address(Some(&xpub), true) (line 240) to track address usage, but those changes are discarded when the function returns. Use get_wallet_info_mut() to update the manager's held instance with the modified state after transaction generation, preventing address reuse and ensuring consistent wallet state.

Also applies to lines 238-249.

🤖 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 138 - 151, The code currently
clones ManagedWalletInfo via manager.get_wallet_info(...) and mutates the clone
with next_change_address(Some(&xpub), true), but those mutations are lost;
instead obtain and mutate the manager-held instance so changes persist: acquire
the write lock (manager_ref.manager.write().await), use
manager.get_wallet_info_mut(&wallet_ref.inner().wallet_id) (or otherwise get a
mutable reference to ManagedWalletInfo) and call next_change_address on that
mutable instance (or replace the stored value with the mutated struct via
get_wallet_info_mut) so the manager's state is updated before returning; ensure
the same pattern is applied to the other occurrence around lines 238–249.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, locate the key-wallet-ffi/src/transaction.rs file
find . -name "transaction.rs" -path "*key-wallet-ffi*"

Repository: dashpay/rust-dashcore

Length of output: 100

---

🏁 Script executed:

# Examine the code at lines 183-191
head -n 200 key-wallet-ffi/src/transaction.rs | tail -n 50

Repository: dashpay/rust-dashcore

Length of output: 1680

---

🏁 Script executed:

# Search for FFITxOutput struct definition
rg -n "struct FFITxOutput" -A 10

Repository: dashpay/rust-dashcore

Length of output: 638

---

🏁 Script executed:

# Check for any existing null address checks in the file
rg -n "is_null\|address" key-wallet-ffi/src/transaction.rs -B 2 -A 2

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

# Look at the wallet_build_and_sign_transaction function
rg -n "wallet_build_and_sign_transaction" -B 5 -A 20

Repository: dashpay/rust-dashcore

Length of output: 12164

---

Guard against null output.address before CStr::from_ptr.

The code validates the outputs array pointer itself (line 111) but does not check individual address fields within each output struct before dereferencing them in the loop. A null output.address causes immediate undefined behavior at the FFI boundary.

Suggested fix

     for output in outputs_slice {
+        if output.address.is_null() {
+            FFIError::set_error(
+                error,
+                FFIErrorCode::InvalidInput,
+                "Output address is null".to_string(),
+            );
+            return false;
+        }
         // Convert address from C string
         let address_str = match CStr::from_ptr(output.address).to_str() {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	for output in outputs_slice {
	// Convert address from C string
	let address_str = match CStr::from_ptr(output.address).to_str() {
	Ok(s) => s,
	Err(_) => {
	FFIError::set_error(
	error,
	FFIErrorCode::WalletError,
	"Cannot sign with watch-only wallet".to_string(),
	FFIErrorCode::InvalidInput,
	"Invalid UTF-8 in output address".to_string(),
	for output in outputs_slice {
	if output.address.is_null() {
	FFIError::set_error(
	error,
	FFIErrorCode::InvalidInput,
	"Output address is null".to_string(),
	);
	return false;
	}
	// Convert address from C string
	let address_str = match CStr::from_ptr(output.address).to_str() {
	Ok(s) => s,
	Err(_) => {
	FFIError::set_error(
	error,
	FFIErrorCode::InvalidInput,
	"Invalid UTF-8 in output address".to_string(),

🤖 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 183 - 191, The loop that
converts output.address using CStr::from_ptr must first guard against a null
pointer: in the loop over outputs_slice (the code that calls
CStr::from_ptr(output.address) and assigns address_str), add an explicit check
if output.address.is_null() and if so call FFIError::set_error(error,
FFIErrorCode::InvalidInput, "Null output.address".to_string()) and return or
continue as appropriate for the function's error handling policy; only call
CStr::from_ptr when the pointer is non-null and then handle UTF-8 errors as
before.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check allocation/free patterns for tx_bytes in transaction.rs
rg -n "transaction_bytes_free|into_boxed_slice|Box::into_raw" -C 3 key-wallet-ffi/src/transaction.rs

Repository: dashpay/rust-dashcore

Length of output: 1550

---

🏁 Script executed:

#!/bin/bash
# Also check for the transaction_bytes_free function definition
rg -n "fn transaction_bytes_free" -A 5 key-wallet-ffi/src/

Repository: dashpay/rust-dashcore

Length of output: 468

---

🏁 Script executed:

#!/bin/bash
# Verify the full context around lines 337-343
sed -n '330,350p' key-wallet-ffi/src/transaction.rs

Repository: dashpay/rust-dashcore

Length of output: 557

---

Fix FFI memory layout mismatch in transaction bytes allocation.

The allocation creates Box<[u8]> (line 340) but deallocation (line 509) reconstructs it as Box<u8> via Box::from_raw(tx_bytes). This layout mismatch causes undefined behavior—the deallocator uses wrong metadata and corrupts the heap. Either use matching libc::malloc/libc::free pair or store the slice length for reconstruction:

Suggested fix (malloc/free)

-    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);

 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 as *mut libc::c_void);
     }
 }

🤖 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 - 343, The FFI currently
turns serialized.into_boxed_slice() into a raw pointer via Box::into_raw and
writes it to tx_bytes_out, but the deallocation code reconstructs it as Box<u8>
(Box::from_raw(tx_bytes)), causing a layout mismatch; fix by choosing one
consistent allocation/deallocation strategy: either allocate with libc::malloc
and return the pointer (so the receiver must free with libc::free), or continue
using boxed slices but also return/store the length and reconstruct the same
Box<[u8]> on deallocation using
Box::from_raw(std::slice::from_raw_parts_mut(tx_bytes, size)); update the code
paths that use serialized.into_boxed_slice, tx_bytes_out and the corresponding
free path to use the chosen strategy so allocation and deallocation types match.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Recompute fee when the change output is dropped.

calculate_fee() always estimates with outputs.len() + 1, so when change is below dust (or no change is added), the actual fee diverges from the returned estimate. This can underpay the intended fee rate and makes fee_out inaccurate. Recalculate the fee without a change output when change_amount <= 546, then recompute change_amount.

Suggested fix

-        let fee = self.calculate_fee();
-
-        let change_amount = total_input.saturating_sub(total_output).saturating_sub(fee);
+        let mut fee = self.calculate_fee();
+        let mut change_amount = total_input.saturating_sub(total_output).saturating_sub(fee);
+
+        if change_amount <= 546 {
+            let fee_rate = self.fee_level.fee_rate();
+            let estimated_size =
+                self.estimate_transaction_size(self.inputs.len(), self.outputs.len());
+            fee = fee_rate.calculate_fee(estimated_size);
+            change_amount = total_input.saturating_sub(total_output).saturating_sub(fee);
+        }

Also applies to: 422-434

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 359 - 363, calculate_fee() always assumes a change output by calling
estimate_transaction_size(self.inputs.len(), self.outputs.len() + 1) which
diverges when the computed change is dust or omitted; update the logic so after
computing fee and change_amount you check if change_amount <= DUST_THRESHOLD
(546) and if so re-estimate size without a change output (use outputs.len()
instead of outputs.len()+1), recompute the fee via
fee_rate.calculate_fee(new_size), set change_amount to 0 (and ensure fee_out
reflects the recomputed fee), and recompute any dependent values; apply the same
pattern wherever fee and change are calculated (including the block around
functions/lines referenced by the reviewer: calculate_fee(),
estimate_transaction_size(), and the code paths at the other region noted
422-434).

[xdustinface]

@ZocoLini Please have a look at CodeRabbit's responses, seems like some of them should be addressed. Also, i just quickly skimmed scrolled through this but it looks like there are few different things fixes/refactors/dropping unused code/adding new functions squashed into one PR? Seems hard follow whats going on, can you split it up?

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[coderabbitai]

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)

dash-spv-ffi/dash_spv_ffi.h (1)

395-407: ⚠️ Potential issue | 🟡 Minor

Remove the duplicate dash_spv_ffi_client_stop declaration and stale cancel-sync wording.

Line 406 repeats the dash_spv_ffi_client_stop prototype with “Cancels the sync operation” semantics even though the header already declares the stop API earlier. This is misleading for consumers and suggests two different behaviors for the same symbol. Please remove the stale block in the Rust doc source and regenerate the header so only the stop semantics remain.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dash-spv-ffi/dash_spv_ffi.h` around lines 395 - 407, Remove the duplicate
prototype and stale "Cancels the sync operation" docblock for
dash_spv_ffi_client_stop: find the duplicate declaration of int32_t
dash_spv_ffi_client_stop(struct FFIDashSpvClient *client) and delete that entire
redundant doc/comment block in the Rust doc source so only the original stop API
and its correct semantics remain, then regenerate the header so the header no
longer contains the repeated prototype or misleading cancel-sync wording.

dash-spv-ffi/src/client.rs (1)

491-522: ⚠️ Potential issue | 🟡 Minor

Add FFI unit and C tests for dash_spv_ffi_client_broadcast_transaction.

This new FFI entry point should be covered by unit tests in dash-spv-ffi/tests/unit and C tests in dash-spv-ffi/tests/c_tests to validate broadcasting and error propagation. While integration and security tests reference this function, dedicated unit and C tests in the specified directories are required per project 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 491 - 522, Add unit and C-level
tests covering the FFI function dash_spv_ffi_client_broadcast_transaction: write
Rust unit tests under dash-spv-ffi/tests/unit that create a mock or test
FFIDashSpvClient, call dash_spv_ffi_client_broadcast_transaction with valid and
invalid tx bytes to assert success and that set_last_error is populated on
failure, and add C tests under dash-spv-ffi/tests/c_tests that link against the
built library, construct an FFIDashSpvClient (or use provided test helpers),
call the exported dash_spv_ffi_client_broadcast_transaction symbol with various
inputs (valid tx, malformed bytes, null pointers) and verify returned
FFIErrorCode values and error propagation; ensure tests exercise both Ok path
and Err path from spv_client.broadcast_transaction and clean up
runtime/resources after each test.

🧹 Nitpick comments (1)

key-wallet-ffi/include/key_wallet_ffi.h (1)

172-176: FFIFeeRate enum missing a documentation comment

Every other enum in this header carries a description block (e.g., FFILanguage, FFITransactionContext, FFIAccountType). The new FFIFeeRate enum has none, leaving callers unable to determine what each tier means in concrete terms (e.g., duff/byte range or expected confirmation target). Since this file is cbindgen-generated, the doc comment needs to be added to the Rust source.

✏️ Suggested Rust doc comment (to be added to the source enum)

+/// Fee rate tiers for transaction construction.
+///
+/// - `Economy` – low-priority; suitable for non-time-sensitive transactions.
+/// - `Normal`  – standard network fee; confirms within a few blocks.
+/// - `Priority` – higher fee for faster confirmation.
 pub enum FeeRate {
     Economy  = 0,
     Normal   = 1,
     Priority = 2,
 }

🤖 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 172 - 176, Add a Rust
documentation comment on the source enum that backs FFIFeeRate so cbindgen will
emit the header comment for FFIFeeRate; specifically update the Rust enum
definition for FFIFeeRate to include a descriptive doc block that explains what
each variant (ECONOMY, NORMAL, PRIORITY) means in concrete terms (e.g.,
suggested duff/byte ranges or relative fee priority and expected confirmation
target) and then re-run cbindgen to regenerate key_wallet_ffi.h so the header
contains the new comment.

🤖 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 456-481: The clear-storage FFI function
dash_spv_ffi_client_clear_storage currently calls client.inner.stop().await with
an immutable borrow, which doesn't cancel the shutdown token or join monitoring
threads and can allow callbacks during the wipe; change the local binding to a
mutable borrow (let client = &mut (*client)) and, inside the runtime.block_on,
call stop_client_internal(&mut client.inner) (or the crate's
stop_client_internal function) to cancel the token and join threads before
invoking client.inner.clear_storage().await, then preserve the existing error
handling/FFI return handling.

In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 3519-3539: Update the safety notes for
wallet_build_and_sign_transaction to document the missing fee_out and tighten
the account_index constraint: state that fee_out must be a non-NULL pointer and
will be written with the transaction fee in the wallet's base currency atomic
units (e.g., satoshis), and change the account_index description to
"`account_index` must be a valid BIP44 account index present in `wallet`"
(referencing wallet_build_and_sign_transaction, fee_out, and account_index to
locate the docblock).
- Around line 3530-3539: External consumers must be updated to the new combined
function signature: replace calls to wallet_build_transaction and
wallet_sign_transaction with wallet_build_and_sign_transaction, pass a const
FFIWalletManager* (previously FFIWallet *managed_wallet) as the first arg,
supply an FFIFeeRate value instead of fee_per_kb/current_height, and provide
pointers for uint64_t *fee_out, uint8_t **tx_bytes_out and size_t *tx_len_out to
receive the fee and serialized tx; ensure callers construct or obtain an
FFIWalletManager instance, populate an FFIFeeRate struct, and handle the
returned fee_out/tx_bytes_out memory correctly (freeing if required) to match
the new wallet_build_and_sign_transaction behavior.

---

Outside diff comments:
In `@dash-spv-ffi/dash_spv_ffi.h`:
- Around line 395-407: Remove the duplicate prototype and stale "Cancels the
sync operation" docblock for dash_spv_ffi_client_stop: find the duplicate
declaration of int32_t dash_spv_ffi_client_stop(struct FFIDashSpvClient *client)
and delete that entire redundant doc/comment block in the Rust doc source so
only the original stop API and its correct semantics remain, then regenerate the
header so the header no longer contains the repeated prototype or misleading
cancel-sync wording.

In `@dash-spv-ffi/src/client.rs`:
- Around line 491-522: Add unit and C-level tests covering the FFI function
dash_spv_ffi_client_broadcast_transaction: write Rust unit tests under
dash-spv-ffi/tests/unit that create a mock or test FFIDashSpvClient, call
dash_spv_ffi_client_broadcast_transaction with valid and invalid tx bytes to
assert success and that set_last_error is populated on failure, and add C tests
under dash-spv-ffi/tests/c_tests that link against the built library, construct
an FFIDashSpvClient (or use provided test helpers), call the exported
dash_spv_ffi_client_broadcast_transaction symbol with various inputs (valid tx,
malformed bytes, null pointers) and verify returned FFIErrorCode values and
error propagation; ensure tests exercise both Ok path and Err path from
spv_client.broadcast_transaction and clean up runtime/resources after each test.

---

Duplicate comments:
In `@dash-spv-ffi/include/dash_spv_ffi.h`:
- Around line 539-549: Update the C header doc for
dash_spv_ffi_client_broadcast_transaction to state that tx_bytes must be
non-null and point to a buffer valid for at least length bytes (matching Rust's
from_raw_parts usage); modify the Rust doc comment for the corresponding
function to include this validity requirement, then regenerate the header so the
updated comment appears in dash_spv_ffi.h.

In `@dash-spv-ffi/src/client.rs`:
- Around line 484-490: Update the safety docs for the broadcast function to
state that the tx_bytes pointer must be valid for at least length bytes and that
length must accurately reflect the number of bytes (i.e., tx_bytes must be valid
for at least `length` bytes and `length` must match the actual buffer size),
matching the `from_raw_parts` usage; reference the existing parameter names
(`client`, `tx_bytes`, `length`) in the comment so the cbindgen header and Rust
safety contract are precise.

In `@key-wallet-ffi/src/transaction.rs`:
- Around line 183-195: The loop over outputs_slice dereferences output.address
via CStr::from_ptr without a null check, which can cause UB; before calling
CStr::from_ptr in the conversion for each output (the code handling
output.address inside the loop in transaction.rs), check if
output.address.is_null() and if so call FFIError::set_error with
FFIErrorCode::InvalidInput and a descriptive message (e.g., "Null output
address") and return false; otherwise proceed to call CStr::from_ptr and handle
UTF-8 errors as currently implemented.
- Around line 138-251: The code clones ManagedWalletInfo via
manager.get_wallet_info(...) into managed_wallet, mutates it with
managed_account.next_change_address(...), but never persists the mutation back
to the manager causing address pool desync; change the flow to acquire a mutable
wallet entry from the manager (e.g., use manager.get_wallet_info_mut(...) or
lock manager_ref.manager.write().await and access the wallet in-place) so that
modifications to managed_wallet.accounts (and next_change_address) are applied
to the manager-held instance, or explicitly write the updated ManagedWalletInfo
back into the manager before returning; update references to managed_wallet,
managed_account, next_change_address, and manager_ref.manager to use the mutable
path so the new change address state is persisted.
- Around line 337-510: The code allocates a Box<[u8]> (boxed via
into_boxed_slice -> tx_bytes) but transaction_bytes_free reconstructs a Box<u8>,
causing UB; fix by making the allocation/free pair match: either switch the
allocation to a raw malloc (use libc::malloc) and free with libc::free in
transaction_bytes_free, or keep the boxed slice and change
transaction_bytes_free to accept the original length and reconstruct Box<[u8]>
from (tx_bytes, len) before dropping; update the symbols noted (boxed, tx_bytes,
transaction_bytes_free) and adjust the FFI free signature to include tx length
if you choose the Box<[u8]> approach.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 359-434: The fee is initially calculated with outputs.len() + 1
(via calculate_fee) but when change is dust you drop the change output and must
recompute the fee and change_amount accordingly; in build_internal after
computing initial fee and change_amount, if change_amount <= 546 then recompute
fee using the fee rate and estimate_transaction_size(self.inputs.len(),
self.outputs.len()) (i.e. without the +1 change output) — call
fee_level.fee_rate().calculate_fee(...) or add a new helper that accepts an
outputs count — then recompute change_amount =
total_input.saturating_sub(total_output).saturating_sub(fee) and only add the
change output if the recomputed change_amount > 546 (otherwise omit it).

---

Nitpick comments:
In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 172-176: Add a Rust documentation comment on the source enum that
backs FFIFeeRate so cbindgen will emit the header comment for FFIFeeRate;
specifically update the Rust enum definition for FFIFeeRate to include a
descriptive doc block that explains what each variant (ECONOMY, NORMAL,
PRIORITY) means in concrete terms (e.g., suggested duff/byte ranges or relative
fee priority and expected confirmation target) and then re-run cbindgen to
regenerate key_wallet_ffi.h so the header contains the new comment.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect clear_storage usage and shutdown handling.
rg -n "dash_spv_ffi_client_clear_storage" -C 3 dash-spv-ffi/src dash-spv-ffi/tests 2>/dev/null || true
echo "---"
rg -n "stop_client_internal" -C 5 dash-spv-ffi/src/client.rs
echo "---"
# Also look for monitoring threads and active_threads
rg -n "active_threads\|shutdown_token" dash-spv-ffi/src/client.rs | head -20

Repository: dashpay/rust-dashcore

Length of output: 1251

---

🏁 Script executed:

# Check the full FFIDashSpvClient structure to understand its fields
rg -n "struct FFIDashSpvClient" -A 10 dash-spv-ffi/src/client.rs

Repository: dashpay/rust-dashcore

Length of output: 600

---

🏁 Script executed:

# Look for any test files related to clear_storage
find dash-spv-ffi/tests -name "*.rs" -type f -exec grep -l "clear_storage\|client" {} \;

Repository: dashpay/rust-dashcore

Length of output: 545

---

🏁 Script executed:

# Check test coverage for clear_storage
rg -n "clear_storage" dash-spv-ffi/tests/ -A 5 -B 2

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

# Check the join_active_threads implementation
rg -n "fn join_active_threads" dash-spv-ffi/src/client.rs -A 8

Repository: dashpay/rust-dashcore

Length of output: 442

---

🏁 Script executed:

# Check if clear_storage is called after or without stop in actual usage
rg -n "clear_storage\|stop_client_internal" dash-spv-ffi/src/client.rs | grep -E "(clear_storage|stop_client_internal|pub unsafe.*fn)"

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

# Read the full clear_storage function implementation
sed -n '456,481p' dash-spv-ffi/src/client.rs

Repository: dashpay/rust-dashcore

Length of output: 896

---

🏁 Script executed:

# Read the full stop_client_internal function
sed -n '214,222p' dash-spv-ffi/src/client.rs

Repository: dashpay/rust-dashcore

Length of output: 362

---

🏁 Script executed:

# Check test files more directly
ls -la dash-spv-ffi/tests/unit/
cat dash-spv-ffi/tests/unit/test_client_lifecycle.rs | head -100

Repository: dashpay/rust-dashcore

Length of output: 3916

---

🏁 Script executed:

# Check for any tests that cover clear_storage specifically
grep -n "clear_storage" dash-spv-ffi/tests/unit/test_client_lifecycle.rs

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

# Verify the complete stop_client_internal implementation
sed -n '214,225p' dash-spv-ffi/src/client.rs

Repository: dashpay/rust-dashcore

Length of output: 376

---

Reuse stop_client_internal to properly shut down monitoring threads before clearing storage.

Line 464 uses an immutable borrow and only calls client.inner.stop().await. This leaves the shutdown token uncancelled and active threads still running, so callbacks can execute while storage is being wiped. Call stop_client_internal (which cancels the token and joins threads) before clearing storage, and change to a mutable borrow to support it.

🔧 Suggested fix

pub unsafe extern "C" fn dash_spv_ffi_client_clear_storage(client: *mut FFIDashSpvClient) -> i32 {
    null_check!(client);

-    let client = &(*client);
+    let client = &mut (*client);

-    let result = client.runtime.block_on(async {
-        // Try to stop before clearing to ensure no in-flight writes race the wipe.
-        if let Err(e) = client.inner.stop().await {
-            tracing::warn!("Failed to stop client before clearing storage: {}", e);
-        }
-
-        client.inner.clear_storage().await
-    });
+    if let Err(e) = stop_client_internal(client) {
+        tracing::warn!("Failed to stop client before clearing storage: {}", e);
+    }
+
+    let result = client.runtime.block_on(async { client.inner.clear_storage().await });

🤖 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 456 - 481, The clear-storage FFI
function dash_spv_ffi_client_clear_storage currently calls
client.inner.stop().await with an immutable borrow, which doesn't cancel the
shutdown token or join monitoring threads and can allow callbacks during the
wipe; change the local binding to a mutable borrow (let client = &mut (*client))
and, inside the runtime.block_on, call stop_client_internal(&mut client.inner)
(or the crate's stop_client_internal function) to cancel the token and join
threads before invoking client.inner.clear_storage().await, then preserve the
existing error handling/FFI return handling.

[coderabbitai]

Actionable comments posted: 2

🤖 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/include/dash_spv_ffi.h`:
- Around line 548-551: Add comprehensive tests for
dash_spv_ffi_client_broadcast_transaction: create a Rust unit test that
constructs or mocks an FFIDashSpvClient, calls
dash_spv_ffi_client_broadcast_transaction with a valid serialized transaction
and asserts success, then test malformed/invalid tx_bytes, NULL tx_bytes or NULL
client pointer and assert the appropriate error return codes; similarly add a C
integration test that instantiates the FFIDashSpvClient (or test harness), calls
dash_spv_ffi_client_broadcast_transaction for the same scenarios (valid
broadcast, invalid bytes, null pointer handling, and simulated error cases),
asserts returned int32_t error codes and any expected side effects, and cleans
up client resources after each case. Ensure tests exercise both success path and
all error branches exposed by the function.

In `@key-wallet-ffi/src/transaction.rs`:
- Around line 60-74: The FFI enum FFIFeeRate must not be trusted as a repr(C)
enum—replace usages that accept FFIFeeRate from C with a primitive (u32 or
c_uint) parameter and add explicit validation converting that primitive to
FeeRate via a TryFrom (or a validate_and_map function) that returns a clear
InvalidInput error for values outside 0–2; specifically remove the impl
From<FFIFeeRate> for FeeRate, add TryFrom<u32> (or TryFrom<c_uint>) ->
Result<FeeRate, Error> that matches 0=>FeeRate::economy(), 1=>FeeRate::normal(),
2=>FeeRate::priority() and returns InvalidInput for anything else, and update
any FFI boundary functions to call this validator before using the FeeRate.

---

Duplicate comments:
In `@dash-spv-ffi/include/dash_spv_ffi.h`:
- Around line 541-546: Update the Rust doc comment in client.rs for the
FFIDashSpvClient function that accepts tx_bytes and length (the one whose
comment generates dash_spv_ffi.h) to explicitly state that tx_bytes must point
to a valid, non-null buffer that is allocated for at least length bytes (i.e.,
"tx_bytes must be valid for at least length bytes") and must not be mutated or
freed for the duration of the call; reference the use of
std::slice::from_raw_parts in the comment to justify the requirement. Ensure the
wording appears alongside the existing safety bullets for `client`, `tx_bytes`,
and `length` so cbindgen outputs the clarified safety note into dash_spv_ffi.h.

In `@key-wallet-ffi/FFI_API.md`:
- Around line 1287-1295: The generated FFI_API.md is missing documentation for
the new parameter fee_out in the wallet_build_and_sign_transaction signature;
re-run the documentation generation script so the docstring changes from commit
0dfc377 are reflected: ensure wallet_build_and_sign_transaction's Description
includes fee_out (what it returns/represents) and the Safety section documents
fee_out: *mut u64 as a valid pointer to receive the fee, then regenerate and
commit the updated FFI_API.md so the rendered output matches the Rust docstring.

In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 3520-3527: Update the Rust doc comment for the function that takes
`manager`, `wallet`, `account_index`, `outputs`, `fee_out`, `tx_bytes_out`,
`tx_len_out`, and `error` so cbindgen emits the missing safety contracts: state
that `fee_out` must be non-null and will receive the fee in duffs, and tighten
`account_index` to state it must reference a valid BIP44 account present in the
wallet; keep existing guarantees for `manager` (FFIWalletManager*), `wallet`
(FFIWallet*), `outputs` (array of FFITxOutput with at least `outputs_count`
elements), `tx_bytes_out`/`tx_len_out` (pointers to receive transaction bytes
and length) and note that returned bytes must be freed with
`transaction_bytes_free`, and ensure the parameter names `fee_out` and
`account_index` are mentioned exactly so cbindgen emits the updated comments.

In `@key-wallet-ffi/src/transaction.rs`:
- Around line 183-186: The loop over outputs_slice calls
CStr::from_ptr(output.address) without checking for null; add an explicit null
check for output.address before calling CStr::from_ptr in the loop (the code
around the CStr::from_ptr(output.address).to_str() call inside transaction.rs),
and handle the null case by returning an appropriate error/setting the FFI error
state or skipping the output as the surrounding API expects; ensure you
propagate/convert that error consistently with the surrounding functions that
process outputs so there is no UB from dereferencing a null pointer.
- Around line 138-151: The code clones ManagedWalletInfo via
manager.get_wallet_info(...) and mutates the clone (calling next_change_address)
but never writes it back, losing updated change-address state; modify the logic
to obtain a mutable reference or mutable owned value from the manager (use
manager.get_wallet_info_mut(...) or otherwise lock the manager and replace the
stored ManagedWalletInfo after mutation), perform next_change_address on that
managed_wallet contained in the manager, and persist the updated
ManagedWalletInfo back into manager_ref (e.g., via the mutable getter or an
explicit set/insert) before returning; apply the same fix to the analogous block
that starts around lines 238-249 so the manager-held state is updated rather
than discarding the clone.
- Around line 337-343: The current allocation uses serialized.into_boxed_slice()
and Box::into_raw (see serialized.into_boxed_slice, Box::into_raw, tx_bytes_out)
but transaction_bytes_free reconstructs it as Box<u8> (Box::from_raw), causing a
layout mismatch; fix by using a consistent FFI allocation/free strategy: either
(A) pass both pointer and length (store size alongside tx_bytes_out) and in
transaction_bytes_free reconstruct a Box<[u8]> from the raw pointer + length so
Box::from_raw restores the original Box<[u8]>, or (B) allocate with a
C-compatible allocator (e.g., libc::malloc) and free with libc::free in
transaction_bytes_free; update all places using serialized.into_boxed_slice /
Box::into_raw and the free path (transaction_bytes_free) to use the same
pairing.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 359-363: calculate_fee() always assumes a change output by using
self.estimate_transaction_size(self.inputs.len(), self.outputs.len() + 1), which
underestimates the true fee when the change is dust or when no change output
will be created; update calculate_fee in transaction_builder.rs to detect
whether the change output will actually be included (i.e., if change_amount <
dust_threshold or the code path that decides to omit change) and re-estimate
size/fee without the extra output when omitted by calling
estimate_transaction_size(self.inputs.len(), self.outputs.len()) so
fee_rate.calculate_fee uses the correct estimated_size; adjust or add any needed
access to the change amount or dust threshold used elsewhere in the builder so
the decision is consistent with the rest of the transaction construction (refer
to calculate_fee, estimate_transaction_size, and the code that decides to omit
change).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for test coverage of dash_spv_ffi_client_broadcast_transaction

echo "=== Searching for broadcast_transaction in Rust test files ==="
find . -type f -path "*/tests/unit/*" -name "*.rs" | xargs grep -l "broadcast_transaction" 2>/dev/null || echo "No matches in unit tests"

echo ""
echo "=== Searching for broadcast_transaction in C test files ==="
find . -type f -path "*/tests/c_tests/*" \( -name "*.c" -o -name "*.h" \) | xargs grep -l "broadcast_transaction" 2>/dev/null || echo "No matches in C tests"

echo ""
echo "=== All test directories in dash-spv-ffi ==="
find dash-spv-ffi/tests -type d 2>/dev/null | head -20

echo ""
echo "=== Checking specific test directories ==="
ls -la dash-spv-ffi/tests/unit/ 2>/dev/null || echo "unit directory not found"
ls -la dash-spv-ffi/tests/c_tests/ 2>/dev/null || echo "c_tests directory not found"

Repository: dashpay/rust-dashcore

Length of output: 1412

---

🏁 Script executed:

#!/bin/bash
# Check if dash_spv_ffi_client_broadcast_transaction is a new function
# and verify it exists in the source code

echo "=== Searching for dash_spv_ffi_client_broadcast_transaction implementation ==="
rg -rn "dash_spv_ffi_client_broadcast_transaction" --type=rust dash-spv-ffi/src/

echo ""
echo "=== Checking for broader transaction-related tests ==="
rg -rn "broadcast" --type=rust dash-spv-ffi/tests/unit/ | head -20

echo ""
echo "=== List of existing test files with their content overview ==="
for f in dash-spv-ffi/tests/unit/*.rs; do
  echo "File: $(basename $f)"
  head -20 "$f" | grep -E "^(mod |#\[test\]|fn test_)" || echo "  (examining...)"
done

Repository: dashpay/rust-dashcore

Length of output: 573

---

🏁 Script executed:

#!/bin/bash
# Get the actual implementation of dash_spv_ffi_client_broadcast_transaction

echo "=== Full context around broadcast_transaction in client.rs ==="
rg -B5 -A15 "broadcast_transaction" dash-spv-ffi/src/client.rs

echo ""
echo "=== Check what tests import or test client functions ==="
grep -r "client\|broadcast" dash-spv-ffi/tests/unit/ --include="*.rs" | head -30

echo ""
echo "=== Check C test files for transaction-related tests ==="
grep -r "broadcast\|transaction" dash-spv-ffi/tests/c_tests/ --include="*.c" | head -30

Repository: dashpay/rust-dashcore

Length of output: 5077

---

Add unit and C tests for dash_spv_ffi_client_broadcast_transaction

This new FFI function is missing required test coverage. Per project guidelines, add:

• Rust unit test in tests/unit/ (e.g., in test_async_operations.rs or a dedicated transaction test file)
• C test in tests/c_tests/ (e.g., in test_advanced.c)

Tests should cover valid transaction broadcast, invalid transaction bytes, null pointer handling, and error cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dash-spv-ffi/include/dash_spv_ffi.h` around lines 548 - 551, Add
comprehensive tests for dash_spv_ffi_client_broadcast_transaction: create a Rust
unit test that constructs or mocks an FFIDashSpvClient, calls
dash_spv_ffi_client_broadcast_transaction with a valid serialized transaction
and asserts success, then test malformed/invalid tx_bytes, NULL tx_bytes or NULL
client pointer and assert the appropriate error return codes; similarly add a C
integration test that instantiates the FFIDashSpvClient (or test harness), calls
dash_spv_ffi_client_broadcast_transaction for the same scenarios (valid
broadcast, invalid bytes, null pointer handling, and simulated error cases),
asserts returned int32_t error codes and any expected side effects, and cleans
up client resources after each case. Ensure tests exercise both success path and
all error branches exposed by the function.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, examine the file structure and locate the transaction.rs file
find . -name "transaction.rs" -type f | head -20

Repository: dashpay/rust-dashcore

Length of output: 167

---

🏁 Script executed:

#!/bin/bash
# Read the specific file around the lines in question
if [ -f "key-wallet-ffi/src/transaction.rs" ]; then
  echo "=== Lines 55-80 (FFIFeeRate enum definition) ==="
  sed -n '55,80p' key-wallet-ffi/src/transaction.rs
  echo ""
  echo "=== Lines 90-110 (Function signature) ==="
  sed -n '90,110p' key-wallet-ffi/src/transaction.rs
fi

Repository: dashpay/rust-dashcore

Length of output: 1642

---

🏁 Script executed:

#!/bin/bash
# Search for FFIFeeRate usage and definition throughout the codebase
echo "=== FFIFeeRate enum definition ==="
rg -n "enum FFIFeeRate" -A 5
echo ""
echo "=== Function signature with fee_rate parameter ==="
rg -n "fn wallet_build_and_sign_transaction" -A 10
echo ""
echo "=== FFIFeeRate usage patterns ==="
rg -n "FFIFeeRate::" -B 2 -A 2

Repository: dashpay/rust-dashcore

Length of output: 1747

---

🏁 Script executed:

#!/bin/bash
# Check for any validation or conversion of FFIFeeRate
echo "=== Validation/conversion patterns ==="
rg -n "match.*fee_rate|FFIFeeRate.*match" -B 2 -A 5
echo ""
echo "=== C/FFI bindings or header files ==="
find . -name "*.h" -o -name "*.swift" -o -name "*.kt" | xargs rg -l "FFIFeeRate" 2>/dev/null || echo "No header files found"

Repository: dashpay/rust-dashcore

Length of output: 185

---

🏁 Script executed:

#!/bin/bash
# Check the crate's overall structure and FFI-related files
echo "=== FFI module structure ==="
ls -la key-wallet-ffi/src/ 2>/dev/null || echo "Directory not found"
echo ""
echo "=== Check for lib.rs or mod.rs ==="
head -30 key-wallet-ffi/src/lib.rs 2>/dev/null || head -30 key-wallet-ffi/src/mod.rs 2>/dev/null

Repository: dashpay/rust-dashcore

Length of output: 3228

---

🏁 Script executed:

#!/bin/bash
# Check the C header file to see how FFIFeeRate is defined
cat key-wallet-ffi/include/key_wallet_ffi.h | grep -A 10 "FFIFeeRate"

Repository: dashpay/rust-dashcore

Length of output: 611

---

🏁 Script executed:

#!/bin/bash
# Search for any validation or defensive code before the From impl is used
sed -n '96,150p' key-wallet-ffi/src/transaction.rs | grep -A 30 "wallet_build_and_sign_transaction"

Repository: dashpay/rust-dashcore

Length of output: 963

---

🏁 Script executed:

#!/bin/bash
# Check if there's any validation of fee_rate after it's received
sed -n '96,200p' key-wallet-ffi/src/transaction.rs | grep -B 5 -A 10 "From.*fee_rate\|fee_rate.*into\|FeeRate::from"

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

#!/bin/bash
# Check the complete flow to see if fee_rate is converted and how it's used
grep -n "let.*fee_rate\|FeeRate.*from\|FeeRate::from" key-wallet-ffi/src/transaction.rs

Repository: dashpay/rust-dashcore

Length of output: 47

---

🏁 Script executed:

#!/bin/bash
# Read more of the function to see where fee_rate is actually used
sed -n '96,250p' key-wallet-ffi/src/transaction.rs

Repository: dashpay/rust-dashcore

Length of output: 5123

---

🏁 Script executed:

#!/bin/bash
# Continue reading to find where fee_rate is converted and used
sed -n '240,300p' key-wallet-ffi/src/transaction.rs

Repository: dashpay/rust-dashcore

Length of output: 2249

---

🏁 Script executed:

#!/bin/bash
# Look at the From impl to confirm it has no default/catch-all arm
sed -n '67,75p' key-wallet-ffi/src/transaction.rs

Repository: dashpay/rust-dashcore

Length of output: 346

---

Validate FFIFeeRate from FFI to avoid UB on invalid discriminants.

FFIFeeRate is a repr(C) enum received directly from C code at line 102. The From<FFIFeeRate> impl (lines 68-74) has only three match arms with no catch-all, so an invalid discriminant from C (e.g., any value outside 0-2) causes undefined behavior. Replace the enum with a primitive u32 and add explicit validation, returning InvalidInput error for unknown values—this is standard FFI practice and aligns with the guidelines to validate untrusted inputs and exercise memory safety at FFI boundaries.

🔧 Example adjustment (ABI + validation)

- pub unsafe extern "C" fn wallet_build_and_sign_transaction(
-     manager: *const FFIWalletManager,
-     wallet: *const FFIWallet,
-     account_index: u32,
-     outputs: *const FFITxOutput,
-     outputs_count: usize,
-     fee_rate: FFIFeeRate,
-     fee_out: *mut u64,
-     tx_bytes_out: *mut *mut u8,
-     tx_len_out: *mut usize,
-     error: *mut FFIError,
- ) -> bool {
+ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
+     manager: *const FFIWalletManager,
+     wallet: *const FFIWallet,
+     account_index: u32,
+     outputs: *const FFITxOutput,
+     outputs_count: usize,
+     fee_rate_raw: u32,
+     fee_out: *mut u64,
+     tx_bytes_out: *mut *mut u8,
+     tx_len_out: *mut usize,
+     error: *mut FFIError,
+ ) -> bool {
      ...
+     let fee_rate = match fee_rate_raw {
+         x if x == FFIFeeRate::Economy as u32 => FeeRate::economy(),
+         x if x == FFIFeeRate::Normal as u32 => FeeRate::normal(),
+         x if x == FFIFeeRate::Priority as u32 => FeeRate::priority(),
+         _ => {
+             FFIError::set_error(
+                 error,
+                 FFIErrorCode::InvalidInput,
+                 "Invalid fee_rate".to_string(),
+             );
+             return false;
+         }
+     };

🤖 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 60 - 74, The FFI enum
FFIFeeRate must not be trusted as a repr(C) enum—replace usages that accept
FFIFeeRate from C with a primitive (u32 or c_uint) parameter and add explicit
validation converting that primitive to FeeRate via a TryFrom (or a
validate_and_map function) that returns a clear InvalidInput error for values
outside 0–2; specifically remove the impl From<FFIFeeRate> for FeeRate, add
TryFrom<u32> (or TryFrom<c_uint>) -> Result<FeeRate, Error> that matches
0=>FeeRate::economy(), 1=>FeeRate::normal(), 2=>FeeRate::priority() and returns
InvalidInput for anything else, and update any FFI boundary functions to call
this validator before using the FeeRate.

[ZocoLini]

its a mess this diff tbh, i dont know what git did but shows a lot diffs in buildSignedTransaction where i only changed a couple of lines. I can split it but that only takes away 20 lines related to the transaction broadcasting.

About the removed code, originally I used build_transaction and sign_transaction and once i got a runtime error saying that I should be using buildSignedTransaction I got tilted and removed both empty functions. If you want i can split that tomorrow

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

dash-spv-ffi/src/client.rs (1)

505-506: Inconsistent set_last_error format vs. rest of file.

Every other function in this file calls set_last_error(&e.to_string()) directly, but the new function wraps the message with an extra prefix. While functional, this diverges from the file-wide convention and adds redundant context (the error type already implies the operation).

♻️ Proposed fix to align with file convention

-        set_last_error(&format!("Invalid transaction: {}", e));
+        set_last_error(&e.to_string());

-            set_last_error(&format!("Failed to broadcast transaction: {}", e));
+            set_last_error(&e.to_string());

Also applies to: 518-520

🤖 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 505 - 506, Replace the custom
formatted error string with the file-wide convention of passing the error's
string directly to set_last_error; specifically change
set_last_error(&format!("Invalid transaction: {}", e)) to
set_last_error(&e.to_string()) (and make the same change for the similar
occurrence around the 518-520 region) while keeping the same
FFIErrorCode::InvalidArgument return behavior.

🤖 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 492-523: Add unit tests for
dash_spv_ffi_client_broadcast_transaction that exercise the happy path,
invalid-transaction path, and broadcast-failure path: construct a
FFIDashSpvClient (or a test helper that builds one) with a controllable inner
client, call dash_spv_ffi_client_broadcast_transaction with valid tx bytes and
assert FFIErrorCode::Success, call it with invalid tx bytes and assert it
returns FFIErrorCode::InvalidArgument and sets set_last_error appropriately, and
simulate inner.broadcast_transaction returning an error to assert the returned
FFIErrorCode and set_last_error; ensure tests run code via the client's runtime
(client.runtime.block_on) or otherwise drive the async behavior and include
null-pointer checks test cases for client and tx_bytes inputs.

In `@key-wallet-ffi/FFI_API.md`:
- Around line 1287-1295: Document the valid FFIFeeRate variants and their
semantics and add the missing manager-wallet invariant to the function
docstring: update the docstring for wallet_build_and_sign_transaction to include
a "See FFIFeeRate" pointer and explicitly list each FFIFeeRate variant (e.g.,
ECONOMY, NORMAL, PRIORITY) with the units/meaning (e.g., duffs per byte or duffs
per KB) so callers know what value to pass, and update the FFIFeeRate enum's own
doc comments to describe each variant; also add a safety precondition that
wallet must be registered/known to the given FFIWalletManager (i.e., the wallet
id must be present in manager) and that a missing registration results in an
error via the FFIError output.

---

Duplicate comments:
In `@dash-spv-ffi/src/client.rs`:
- Around line 489-490: Update the safety documentation for the function that
takes tx_bytes and length to state that tx_bytes must be non-null and valid for
at least `length` bytes (i.e., the pointer must be dereferenceable for that many
bytes) because the implementation calls std::slice::from_raw_parts(tx_bytes,
length); ensure the safety section mirrors the original phrasing added in commit
0dfc377 so callers know the exact memory-validity requirement for tx_bytes and
length.

In `@key-wallet-ffi/src/transaction.rs`:
- Around line 60-74: The From<FFIFeeRate> impl currently pattern-matches on
FFIFeeRate without a fallback, which is undefined if an out-of-range
discriminant arrives from C; change the API to accept a primitive (e.g. u32 or
libc::c_uint) at the FFI boundary and validate it before converting: replace
uses of the From<FFIFeeRate> conversion with a checked mapper function (e.g. fn
fee_rate_from_u32(v: u32) -> Result<FeeRate, Error>) that matches
0→FeeRate::economy, 1→FeeRate::normal, 2→FeeRate::priority and returns an Err
for any other value; update call sites that currently pass FFIFeeRate (or rely
on the impl) to call this validator and handle the error, ensuring no direct
unsafe enum transmutation occurs for FFIFeeRate.
- Around line 186-198: The loop that converts each output address must guard
against a null pointer before calling CStr::from_ptr: in the block iterating
outputs_slice (where you call CStr::from_ptr(output.address)), first check if
output.address.is_null(), and if so call FFIError::set_error(error,
FFIErrorCode::InvalidInput, "Null output address".to_string()) and return false;
otherwise proceed to call CStr::from_ptr and to_str() as before. Ensure the
null-check is placed immediately before the existing CStr::from_ptr usage to
prevent UB at the FFI boundary.
- Around line 141-253: The code clones ManagedWalletInfo into managed_wallet,
then calls managed_account.next_change_address(...) which mutates that clone —
those mutations are lost; replace the clone-without-write behavior by obtaining
a mutable reference or writing the updated struct back into the manager: instead
of using manager.get_wallet_info(...).cloned(), call the manager method that
returns mutable access (e.g., get_wallet_info_mut or an equivalent) under
manager_ref.manager.write().await and use that mutable ManagedWalletInfo (or, if
no getter exists, after computing the new change address, write the modified
ManagedWalletInfo back into the manager under the write lock). Ensure you modify
the lookup code around manager_ref.manager.write().await, the variable
managed_wallet, and the use of managed_account/next_change_address so the
manager-held state is updated rather than only mutating a cloned copy.
- Around line 340-345: The allocation for transaction bytes uses
serialized.into_boxed_slice() and stores a raw pointer in tx_bytes but later the
code frees it with Box::from_raw(tx_bytes) which is a layout mismatch; update
the free path to reconstruct the original boxed slice using the pointer + length
(e.g. use unsafe Box::from_raw(std::slice::from_raw_parts_mut(tx_bytes, size) as
*mut [u8])) so the pointer is converted back to a Box<[u8]> before drop, and
apply the same fix where transaction_bytes_free / Box::from_raw is used around
the other occurrence (lines ~509-513) to ensure matching allocation/free
pairing.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 359-434: calculate_fee() always assumes a change output (uses
outputs.len() + 1), but build_internal may drop the change when change_amount <=
546, so recompute fee and change_amount after deciding whether to add the change
output. In build_internal (around calculate_fee, change_amount, tx_outputs, and
change_address), after you determine change_amount and decide to drop the change
(change_amount <= 546), call fee_rate.calculate_fee with an updated
estimated_size (use self.estimate_transaction_size(self.inputs.len(),
tx_outputs.len()) or make calculate_fee accept an outputs_count) to get the true
fee without the change output, then recompute change_amount =
total_input.saturating_sub(total_output).saturating_sub(new_fee) and re-check
for dust/insufficient funds (return BuilderError::InsufficientFunds if change
would be negative).

---

Nitpick comments:
In `@dash-spv-ffi/src/client.rs`:
- Around line 505-506: Replace the custom formatted error string with the
file-wide convention of passing the error's string directly to set_last_error;
specifically change set_last_error(&format!("Invalid transaction: {}", e)) to
set_last_error(&e.to_string()) (and make the same change for the similar
occurrence around the 518-520 region) while keeping the same
FFIErrorCode::InvalidArgument return behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for any tests related to broadcast_transaction in the FFI test directory
rg -rn "broadcast_transaction" dash-spv-ffi/tests/

# Also check the structure of unit tests
echo "=== Unit test files in dash-spv-ffi/tests/unit/ ==="
find dash-spv-ffi/tests/unit/ -name "*.rs" 2>/dev/null | head -20

Repository: dashpay/rust-dashcore

Length of output: 660

---

🏁 Script executed:

#!/bin/bash
# Search for the exact function name in unit tests
rg -rn "dash_spv_ffi_client_broadcast_transaction" dash-spv-ffi/tests/unit/

# Check if it's referenced anywhere in tests
rg -rn "dash_spv_ffi_client_broadcast_transaction" dash-spv-ffi/tests/

# Examine the async operations unit test (most likely place for this function)
echo "=== Checking test_async_operations.rs ==="
wc -l dash-spv-ffi/tests/unit/test_async_operations.rs

# Also check if the function exists in the source
echo "=== Checking if dash_spv_ffi_client_broadcast_transaction exists in source ==="
rg -n "fn dash_spv_ffi_client_broadcast_transaction" dash-spv-ffi/src/

Repository: dashpay/rust-dashcore

Length of output: 551

---

🏁 Script executed:

#!/bin/bash
# Read the test_async_operations.rs file to see what's covered
cat -n dash-spv-ffi/tests/unit/test_async_operations.rs | grep -i "broadcast\|test_\|fn " | head -40

Repository: dashpay/rust-dashcore

Length of output: 995

---

🏁 Script executed:

#!/bin/bash
# Search for broadcast in all unit test files
grep -r "broadcast" dash-spv-ffi/tests/unit/ 2>/dev/null || echo "No broadcast found in unit tests"

# Check test_client_lifecycle.rs for relevant tests
echo "=== Checking test_client_lifecycle.rs for client operation tests ==="
cat -n dash-spv-ffi/tests/unit/test_client_lifecycle.rs | grep -i "fn test_"

# Check test_error_handling.rs
echo "=== Checking test_error_handling.rs ==="
cat -n dash-spv-ffi/tests/unit/test_error_handling.rs | grep -i "fn test_"

Repository: dashpay/rust-dashcore

Length of output: 1015

---

Add unit tests for dash_spv_ffi_client_broadcast_transaction in tests/unit/.

Each new FFI function requires corresponding unit tests per project standards. Tests must cover the broadcast operation, error handling (invalid transaction, broadcast failures), and async behavior using the client's runtime.

🤖 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 492 - 523, Add unit tests for
dash_spv_ffi_client_broadcast_transaction that exercise the happy path,
invalid-transaction path, and broadcast-failure path: construct a
FFIDashSpvClient (or a test helper that builds one) with a controllable inner
client, call dash_spv_ffi_client_broadcast_transaction with valid tx bytes and
assert FFIErrorCode::Success, call it with invalid tx bytes and assert it
returns FFIErrorCode::InvalidArgument and sets set_last_error appropriately, and
simulate inner.broadcast_transaction returning an error to assert the returned
FFIErrorCode and set_last_error; ensure tests run code via the client's runtime
(client.runtime.block_on) or otherwise drive the async behavior and include
null-pointer checks test cases for client and tx_bytes inputs.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

FFIFeeRate enum variants and their semantics are undocumented.

The safety section states fee_rate must be "a valid variant of FFIFeeRate" but neither the function description, the safety section, nor the Type Definitions section at the end of this file defines what the valid variants are (e.g., ECONOMY, NORMAL, PRIORITY) or what fee rate each represents (e.g., duffs/KB). API consumers relying solely on this document have no way to know what values to pass.

Since this doc is auto-generated, the fix belongs in the Rust source docstring for wallet_build_and_sign_transaction in key-wallet-ffi/src/transaction.rs — e.g., add a line like "See FFIFeeRate for available variants" alongside the enum's own documentation of each variant.

Additionally, the safety section does not specify the invariant that wallet must be registered in manager. If the wallet ID is not found in the manager, the function presumably fails via error, but this undocumented precondition could cause silent misbehaviour in callers that don't check the return value.

📝 Suggested addition to the Rust source docstring

 /// - `fee_rate` must be a valid variant of FFIFeeRate
+///   (`FFIFeeRate::ECONOMY`, `FFIFeeRate::NORMAL`, or `FFIFeeRate::PRIORITY`)
+/// - `wallet` must be registered in `manager`; if the wallet ID is not found,
+///   `error` will be set and the function returns `false`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@key-wallet-ffi/FFI_API.md` around lines 1287 - 1295, Document the valid
FFIFeeRate variants and their semantics and add the missing manager-wallet
invariant to the function docstring: update the docstring for
wallet_build_and_sign_transaction to include a "See FFIFeeRate" pointer and
explicitly list each FFIFeeRate variant (e.g., ECONOMY, NORMAL, PRIORITY) with
the units/meaning (e.g., duffs per byte or duffs per KB) so callers know what
value to pass, and update the FFIFeeRate enum's own doc comments to describe
each variant; also add a safety precondition that wallet must be
registered/known to the given FFIWalletManager (i.e., the wallet id must be
present in manager) and that a missing registration results in an error via the
FFIError output.

[xdustinface]

its a mess this diff tbh, i dont know what git did but shows a lot diffs in buildSignedTransaction where i only changed a couple of lines. I can split it but that only takes away 20 lines related to the transaction broadcasting.

About the removed code, originally I used build_transaction and sign_transaction and once i got a runtime error saying that I should be using buildSignedTransaction I got tilted and removed both empty functions. If you want i can split that tomorrow

I asked claude to check the different topics merged into here and create separete PRs/commits from it, the splitting it did seem reasonable to me! Maybe open 4 PRs accordingly to https://github.com/dashpay/rust-dashcore/tree/refactor/split-pr-447. I didn't test any of those commits separately but looked alright when i quickly scrolled through it. Just makes it easier to review and better in the commit history to separate everything from fixes to refactorings or dropping unused code.