feat(key-wallet-ffi): change wallet_build_and_sign_transaction signature to be used in iOS#463
Merged
QuantumExplorer merged 1 commit intov0.42-devfrom Feb 26, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces managed-wallet pointer with a Changes
Sequence DiagramsequenceDiagram
participant Client as Client / FFI Caller
participant FFI as wallet_build_and_sign_transaction
participant Manager as FFIWalletManager
participant Wallet as ManagedWalletInfo
participant Builder as TransactionBuilder
Client->>FFI: call(manager, wallet, account_index, outputs, fee_rate)
FFI->>Manager: resolve wallet/account and managed state
Manager-->>FFI: return ManagedWalletInfo / account context
FFI->>Builder: initialize builder (outputs, change addr, fee_rate)
Builder->>Manager: request UTXOs / context for coin selection
Manager-->>Builder: provide UTXOs and account data
Builder->>Builder: select coins, derive keys, sign tx
Builder-->>FFI: return tx bytes and calculated fee
FFI->>Client: write `tx_bytes_out`, `tx_len_out`, `fee_out` and return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
ZocoLini
force-pushed
the
chore/build-transaction-ffi-for-ios
branch
from
6619a48 to
acf4b3c
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
chore/build-transaction-ffi-for-ios
branch
from
acf4b3c to
77e5813
Compare
github-actions
bot
removed
the
merge-conflict
![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: 2
🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
445-446:take()prevents reusing the builder for multiple builds with the same payload.Using
self.special_payload.take()moves the payload out ofself. Ifbuild()is called a second time,special_payloadwill beNone. This may be intentional (single-use builder pattern), but it creates asymmetry with other fields likeinputsandoutputswhich are cloned and remain available.If the intent is to allow
calculate_fee()afterbuild()but not support multiple builds, this is fine. If multiple builds should be supported, consider cloning the payload instead.🤖 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 445 - 446, The current build() moves the special payload out via self.special_payload.take(), preventing reuse of the TransactionBuilder while inputs/outputs remain reusable; either preserve the payload by cloning it (e.g., use a clone/as_ref().cloned() so special_payload remains in self) or intentionally enforce single-use by keeping take() but documenting it and making other fields (inputs/outputs) also consumed for consistency; update the code in build() where special_transaction_payload is set and adjust tests/docs accordingly so behavior is explicit (refer to special_payload, build(), calculate_fee(), inputs, outputs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 142-149: You are cloning ManagedWalletInfo out of the manager (via
manager.get_wallet_info(...).cloned()) then mutating that clone (e.g., calling
next_change_address(..., true)), so those state changes are never persisted back
into the manager; instead, acquire and mutate the wallet state atomically under
the manager's write lock (or call a manager method that performs the mutation),
e.g. use manager.get_wallet_info_mut or fetch a mutable reference to the
ManagedWalletInfo inside manager (rather than .cloned()), perform updates to
transactions/UTXOs/balances/address usage (the same code paths around
manager_ref, get_wallet_info, ManagedWalletInfo and next_change_address) while
holding the write lock, then release the lock so changes are stored;
alternatively add/use an update_wallet_info(...) helper on the manager that
applies the mutations inside the manager to ensure atomic persistence.
- Around line 60-75: The FFI enum FFIFeeRate is currently assumed valid but can
be constructed with invalid discriminants by C/Swift callers; instead implement
validation by adding TryFrom<u32> for FFIFeeRate (returning a Result<FFIFeeRate,
_> for invalid values) and change any external FFI-facing APIs to accept a u32
fee_rate and call FFIFeeRate::try_from(fee_rate) before converting to FeeRate;
update the conversion path used by impl From<FFIFeeRate> for FeeRate to remain
for internal use, but ensure all FFI entry points use try_from and return an
appropriate error when try_from fails (reference FFIFeeRate, impl TryFrom<u32>
for FFIFeeRate, impl From<FFIFeeRate> for FeeRate, and
FeeRate::economy()/normal()/priority()).
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 445-446: The current build() moves the special payload out via
self.special_payload.take(), preventing reuse of the TransactionBuilder while
inputs/outputs remain reusable; either preserve the payload by cloning it (e.g.,
use a clone/as_ref().cloned() so special_payload remains in self) or
intentionally enforce single-use by keeping take() but documenting it and making
other fields (inputs/outputs) also consumed for consistency; update the code in
build() where special_transaction_payload is set and adjust tests/docs
accordingly so behavior is explicit (refer to special_payload, build(),
calculate_fee(), inputs, outputs).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
dash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv/src/sync/events.rsdash-spv/src/sync/sync_coordinator.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
Collaborator
|
Seems like something went wrong with the rebase? |
Collaborator
Author
|
ou, will take a look |
ZocoLini
force-pushed
the
chore/build-transaction-ffi-for-ios
branch
from
77e5813 to
cc42f7c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
key-wallet-ffi/src/transaction.rs (2)
142-146:⚠️ Potential issue | 🟠 MajorMajor: state mutations are applied to a clone and then discarded
At Line 145,
get_wallet_info(...).cloned()creates a detached copy. Later mutation at Line 244 (next_change_address(..., true)) updates only that copy, so manager state is not persisted.Keep mutations inside manager-owned state (under the write lock), or add a manager API that atomically applies and persists transaction-building side effects.
Based on learnings: "Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together."
Also applies to: 148-160, 244-245, 302-303
🤖 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 142 - 146, The code is cloning wallet state out of the manager (via manager_ref.runtime.block_on { let manager = manager_ref.manager.write().await; manager.get_wallet_info(&wallet_ref.inner().wallet_id).cloned() }) and then later calling next_change_address(..., true) which mutates only that detached clone, so the manager's persisted state is never updated; fix by performing mutations while holding the manager write lock (e.g., obtain manager_ref.manager.write().await and mutate the wallet info in-place via a mutable accessor or add a manager API that performs the entire sequence atomically), or implement a new manager method (e.g., apply_transaction_side_effects / next_change_address_atomic) that takes the wallet_id and performs address allocation, UTXO/balance updates and transaction recording under the write lock so changes are persisted.
60-75:⚠️ Potential issue | 🔴 CriticalCritical: don’t accept
FFIFeeRateenum directly over FFIAt Line 105, taking
fee_rate: FFIFeeRatedirectly allows invalid discriminants from C/Swift, which is undefined behavior in Rust before thematch. Accept a raw integer and validate.Suggested fix
#[repr(C)] pub enum FFIFeeRate { Economy = 0, Normal = 1, Priority = 2, } +impl TryFrom<u32> for FFIFeeRate { + type Error = (); + fn try_from(value: u32) -> Result<Self, Self::Error> { + match value { + 0 => Ok(Self::Economy), + 1 => Ok(Self::Normal), + 2 => Ok(Self::Priority), + _ => Err(()), + } + } +} + 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_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 FFIFeeRate::try_from(fee_rate_raw) { + Ok(v) => v, + Err(_) => { + FFIError::set_error(error, FFIErrorCode::InvalidInput, "Invalid fee_rate".to_string()); + return false; + } + };Also applies to: 99-106
🤖 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 - 75, The FFIFeeRate enum must not be accepted directly over FFI; instead stop exposing FFIFeeRate in public FFI signatures and validate a raw integer: remove/avoid using impl From<FFIFeeRate> for FeeRate, implement TryFrom<i32> (or TryFrom<u32>) for FeeRate that matches 0=>FeeRate::economy, 1=>normal, 2=>priority and returns Err on any other value, and change any FFI functions that currently take FFIFeeRate to take a primitive (i32/u32) and call the TryFrom conversion, returning an appropriate error code or default if conversion fails; update references to FFIFeeRate->FeeRate conversion to use this validated TryFrom path (applies to the current impl and other similar spots around the FFIFeeRate usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 112-117: In the loop over outputs, validate the raw pointer before
dereferencing: for each item from outputs (the variable outputs and each
output.address), check output.address.is_null() and if so set the appropriate
FFI error (using the same error-setting helper used elsewhere in this file) and
return the error code instead of calling CStr::from_ptr; only call
CStr::from_ptr(output.address) after the null check and proceed as before.
Ensure you reference the existing symbols outputs, output.address,
CStr::from_ptr and the same error-setting/return pattern used for other
parameter checks (e.g., the checks that use tx_bytes_out/tx_len_out/fee_out) so
behavior and error handling remain consistent.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 353-354: The build() method currently mutates builder state by
calling take() on self.special_payload (and similar fields), which clears
payload-dependent data and breaks subsequent build()/fee calculations; change
build() to avoid consuming these fields — access them via references (e.g.,
as_ref()) or clone the payload where owned data is needed so build() is
non-mutating, or if temporary ownership is required, clone the value locally
rather than replacing self.special_payload; update the code paths in build() and
any fee calculation helpers to use the non-consuming access (references or
cloned copies) so repeated calls to TransactionBuilder::build() do not alter the
builder's internal state.
---
Duplicate comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 142-146: The code is cloning wallet state out of the manager (via
manager_ref.runtime.block_on { let manager = manager_ref.manager.write().await;
manager.get_wallet_info(&wallet_ref.inner().wallet_id).cloned() }) and then
later calling next_change_address(..., true) which mutates only that detached
clone, so the manager's persisted state is never updated; fix by performing
mutations while holding the manager write lock (e.g., obtain
manager_ref.manager.write().await and mutate the wallet info in-place via a
mutable accessor or add a manager API that performs the entire sequence
atomically), or implement a new manager method (e.g.,
apply_transaction_side_effects / next_change_address_atomic) that takes the
wallet_id and performs address allocation, UTXO/balance updates and transaction
recording under the write lock so changes are persisted.
- Around line 60-75: The FFIFeeRate enum must not be accepted directly over FFI;
instead stop exposing FFIFeeRate in public FFI signatures and validate a raw
integer: remove/avoid using impl From<FFIFeeRate> for FeeRate, implement
TryFrom<i32> (or TryFrom<u32>) for FeeRate that matches 0=>FeeRate::economy,
1=>normal, 2=>priority and returns Err on any other value, and change any FFI
functions that currently take FFIFeeRate to take a primitive (i32/u32) and call
the TryFrom conversion, returning an appropriate error code or default if
conversion fails; update references to FFIFeeRate->FeeRate conversion to use
this validated TryFrom path (applies to the current impl and other similar spots
around the FFIFeeRate usage).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
ZocoLini
force-pushed
the
chore/build-transaction-ffi-for-ios
branch
from
cc42f7c to
8a9e056
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
353-354:⚠️ Potential issue | 🟠 Major
build(&mut self)still mutates payload state viatake().At Line 445,
self.special_payload.take()clears payload state, so subsequentbuild()/fee calculations on the same builder can diverge for special transactions.🔧 Proposed fix
- special_transaction_payload: self.special_payload.take(), + special_transaction_payload: self.special_payload.clone(),Also applies to: 445-446
🤖 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 353 - 354, The build(&mut self) currently calls self.special_payload.take(), which mutates and clears the builder's special_payload so further build()/fee calculations on the same builder will differ; change build to borrow or clone the payload instead of taking it—use Option::as_ref()/as_ref().cloned() or otherwise create a local owned copy of self.special_payload (or its inner data) for constructing the Transaction while leaving self.special_payload intact; locate usage in the build method and any helper functions that call special_payload.take() and replace take() with non-consuming access so repeated build calls yield consistent results.key-wallet-ffi/src/transaction.rs (2)
186-189:⚠️ Potential issue | 🔴 CriticalNull-check each output address pointer before
CStr::from_ptr.At Line 188,
output.addressis untrusted FFI input; dereferencing null is UB.Based on learnings: "Applies to **/*-ffi/**/*.rs : Exercise careful memory safety handling at FFI boundaries".🔧 Proposed fix
for output in outputs_slice { + if output.address.is_null() { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Output address pointer is null".to_string(), + ); + return false; + } // Convert address from C string let address_str = match CStr::from_ptr(output.address).to_str() {🤖 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 186 - 189, In the loop that iterates over outputs_slice in transaction.rs, ensure you null-check each output.address pointer before calling CStr::from_ptr to avoid UB on null dereference; modify the code around the for output in outputs_slice block (where CStr::from_ptr(output.address) is used) to first test if output.address.is_null() and handle the null case (return an error/result indicating invalid FFI input or skip with a clear error path) and only call CStr::from_ptr when the pointer is non-null, preserving the existing conversion and error mapping logic.
60-65:⚠️ Potential issue | 🔴 CriticalDo not accept
FFIFeeRateenum directly across FFI boundary.At Line 105, an out-of-range discriminant from C/Swift can create invalid enum state before matching. Accept a raw integer (
u32) and validate withTryFrom<u32>.🔧 Proposed fix
#[repr(C)] pub enum FFIFeeRate { Economy = 0, Normal = 1, Priority = 2, } +impl TryFrom<u32> for FFIFeeRate { + type Error = (); + fn try_from(value: u32) -> Result<Self, Self::Error> { + match value { + 0 => Ok(Self::Economy), + 1 => Ok(Self::Normal), + 2 => Ok(Self::Priority), + _ => Err(()), + } + } +} + 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_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 FFIFeeRate::try_from(fee_rate_raw) { + Ok(v) => v, + Err(_) => { + FFIError::set_error(error, FFIErrorCode::InvalidInput, "Invalid fee rate".to_string()); + return false; + } + };Does Rust consider invalid discriminants in repr(C) enums from FFI undefined behavior, and is accepting a raw integer + TryFrom validation the recommended pattern?Also applies to: 99-106
🤖 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 - 65, The FFIFeeRate repr(C) enum must not be accepted directly across the FFI; instead change FFI-facing functions that currently take FFIFeeRate (search for usages of FFIFeeRate in FFI signatures around the matching code at/near line 105) to accept a raw integer type (u32), implement TryFrom<u32> for FFIFeeRate that validates discriminants and returns a Result, and convert the incoming u32 to FFIFeeRate via that TryFrom before any match/usage; on invalid values return an appropriate error code or sentinel to the caller rather than constructing an invalid enum.
🧹 Nitpick comments (1)
key-wallet-ffi/FFI_API.md (1)
1291-1291: Clarify wording to match the manager-centric call path.“using the wallet’s managed info” still sounds like the old managed-wallet direct flow. Consider explicitly saying it builds via
FFIWalletManager-owned state to avoid migration confusion.✏️ Suggested doc wording
-Build and sign a transaction using the wallet's managed info +Build and sign a transaction using manager-owned wallet managed state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/FFI_API.md` at line 1291, Update the documentation wording to explicitly state that the transaction is built and signed using state owned by FFIWalletManager (not the legacy managed-wallet flow); mention the manager parameter (manager must be a valid pointer to an FFIWalletManager) and that the call uses FFIWalletManager-owned info for UTXO selection, fee calculation, change address generation, and signing (so callers understand this is the manager-centric call path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Line 338: The current assignment *fee_out =
tx_builder_with_inputs.calculate_fee() can undercount when a change output is
added; instead, after building the final transaction (the result of
tx_builder_with_inputs.build() / whatever returns the built transaction),
compute the fee as sum(inputs) - sum(outputs) from the built transaction and
assign that value to *fee_out so the reported fee reflects the actual built tx;
locate usages of tx_builder_with_inputs and replace the pre-build
calculate_fee() result with the post-build computed fee derived from the built
transaction's input and output sums.
- Around line 344-346: The code casts a Box<[u8]> to *mut u8 (variables boxed ->
tx_bytes) and later reconstructs it as Box<u8>, corrupting the heap; fix by
passing the transaction length (e.g., tx_len) alongside tx_bytes into the
deallocation function and, in the deallocator, reconstruct the original boxed
slice as Box<[u8]> using the raw pointer + length (via
std::slice::from_raw_parts_mut(tx_bytes, tx_len) and Box::from_raw on that
slice) instead of creating Box<u8>; update the allocation site that creates
tx_bytes to also return/store the length and adjust the deallocation call site
accordingly.
---
Duplicate comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 186-189: In the loop that iterates over outputs_slice in
transaction.rs, ensure you null-check each output.address pointer before calling
CStr::from_ptr to avoid UB on null dereference; modify the code around the for
output in outputs_slice block (where CStr::from_ptr(output.address) is used) to
first test if output.address.is_null() and handle the null case (return an
error/result indicating invalid FFI input or skip with a clear error path) and
only call CStr::from_ptr when the pointer is non-null, preserving the existing
conversion and error mapping logic.
- Around line 60-65: The FFIFeeRate repr(C) enum must not be accepted directly
across the FFI; instead change FFI-facing functions that currently take
FFIFeeRate (search for usages of FFIFeeRate in FFI signatures around the
matching code at/near line 105) to accept a raw integer type (u32), implement
TryFrom<u32> for FFIFeeRate that validates discriminants and returns a Result,
and convert the incoming u32 to FFIFeeRate via that TryFrom before any
match/usage; on invalid values return an appropriate error code or sentinel to
the caller rather than constructing an invalid enum.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 353-354: The build(&mut self) currently calls
self.special_payload.take(), which mutates and clears the builder's
special_payload so further build()/fee calculations on the same builder will
differ; change build to borrow or clone the payload instead of taking it—use
Option::as_ref()/as_ref().cloned() or otherwise create a local owned copy of
self.special_payload (or its inner data) for constructing the Transaction while
leaving self.special_payload intact; locate usage in the build method and any
helper functions that call special_payload.take() and replace take() with
non-consuming access so repeated build calls yield consistent results.
---
Nitpick comments:
In `@key-wallet-ffi/FFI_API.md`:
- Line 1291: Update the documentation wording to explicitly state that the
transaction is built and signed using state owned by FFIWalletManager (not the
legacy managed-wallet flow); mention the manager parameter (manager must be a
valid pointer to an FFIWalletManager) and that the call uses
FFIWalletManager-owned info for UTXO selection, fee calculation, change address
generation, and signing (so callers understand this is the manager-centric call
path).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
Comment on lines
+344
to
346
| let boxed = serialized.into_boxed_slice(); | ||
| let tx_bytes = Box::into_raw(boxed) as *mut u8; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n key-wallet-ffi/src/transaction.rs | sed -n '340,350p'Repository: dashpay/rust-dashcore
Length of output: 518
🏁 Script executed:
cat -n key-wallet-ffi/src/transaction.rs | sed -n '510,520p'Repository: dashpay/rust-dashcore
Length of output: 428
🏁 Script executed:
rg -n "transaction_bytes_free" key-wallet-ffi/src/Repository: dashpay/rust-dashcore
Length of output: 289
🏁 Script executed:
cat -n key-wallet-ffi/src/transaction.rs | sed -n '90,110p'Repository: dashpay/rust-dashcore
Length of output: 1167
🏁 Script executed:
rg -n "Box::from_raw|into_boxed_slice|Box::into_raw" key-wallet-ffi/src/transaction.rsRepository: dashpay/rust-dashcore
Length of output: 385
Fix memory-unsafe transaction buffer deallocation.
At line 345, a Box<[u8]> is cast to *mut u8, discarding the slice metadata. The deallocation at line 515 reconstructs it as Box<u8> (single byte) instead of the original slice, causing heap corruption.
Pass the transaction length to the deallocation function:
Safer API direction
- pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8) {
+ pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8, tx_len: usize) {
if !tx_bytes.is_null() {
- let _ = Box::from_raw(tx_bytes);
+ let slice_ptr = core::ptr::slice_from_raw_parts_mut(tx_bytes, tx_len);
+ let _ = Box::from_raw(slice_ptr);
}
}🤖 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 344 - 346, The code casts a
Box<[u8]> to *mut u8 (variables boxed -> tx_bytes) and later reconstructs it as
Box<u8>, corrupting the heap; fix by passing the transaction length (e.g.,
tx_len) alongside tx_bytes into the deallocation function and, in the
deallocator, reconstruct the original boxed slice as Box<[u8]> using the raw
pointer + length (via std::slice::from_raw_parts_mut(tx_bytes, tx_len) and
Box::from_raw on that slice) instead of creating Box<u8>; update the allocation
site that creates tx_bytes to also return/store the length and adjust the
deallocation call site accordingly.
8a9e056 to
6850534
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
373-416:⚠️ Potential issue | 🟠 MajorFee affordability check is incomplete and can produce underfunded-fee transactions.
Line 373 validates only
total_input >= total_output, but fee is deducted later withsaturating_sub. If inputs can’t coveroutput + fee, build still succeeds with a lower-than-target fee.🔧 Proposed fix
- if total_input < total_output { - return Err(BuilderError::InsufficientFunds { - available: total_input, - required: total_output, - }); - } + let fee = self.calculate_fee_with_extra_output(); + let required_total = total_output + .checked_add(fee) + .ok_or_else(|| BuilderError::InvalidAmount("Output + fee overflow".into()))?; + + if total_input < required_total { + return Err(BuilderError::InsufficientFunds { + available: total_input, + required: required_total, + }); + } @@ - let fee = self.calculate_fee_with_extra_output(); - - let change_amount = total_input.saturating_sub(total_output).saturating_sub(fee); + let change_amount = total_input - required_total;🤖 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 373 - 416, The current early funds check using total_input < total_output allows builds that later deduct fee (via calculate_fee_with_extra_output) and produce underfunded-fee transactions; compute the fee (call calculate_fee_with_extra_output or its deterministic components) before finalizing funds validation and replace the existing check with one that ensures total_input >= total_output + fee (return BuilderError::InsufficientFunds with available: total_input and required: total_output + fee), and then compute change_amount as total_input - total_output - fee; update references in this function (inputs, total_input, total_output, calculate_fee_with_extra_output, BuilderError::InsufficientFunds, change_amount) accordingly so no transaction can be built if inputs cannot cover outputs plus fee.
♻️ Duplicate comments (2)
key-wallet-ffi/src/transaction.rs (2)
186-198:⚠️ Potential issue | 🔴 CriticalCritical: Missing null check before
CStr::from_ptr(output.address).This issue was raised in a previous review and remains unfixed.
CStr::from_ptrisunsafeand causes undefined behavior when passed a null pointer—it does not return aResult. Eachoutput.addressmust be validated before dereferencing.Proposed fix
for output in outputs_slice { + if output.address.is_null() { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "Output address pointer is null".to_string(), + ); + return false; + } // Convert address from C string let address_str = match CStr::from_ptr(output.address).to_str() {Based on learnings: "CStr::from_ptr does NOT return a Result; it causes undefined behavior if passed a null pointer. Always validate pointer is non-null before calling CStr::from_ptr on FFI string inputs."
🤖 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 186 - 198, Check for null before calling CStr::from_ptr on each output.address in the loop over outputs_slice: if output.address.is_null() set an FFIError via FFIError::set_error with FFIErrorCode::InvalidInput (message like "Null output address") and return false; only then safely call CStr::from_ptr(output.address).to_str() and handle the Utf8 error as already implemented.
355-357:⚠️ Potential issue | 🔴 CriticalMemory-unsafe deallocation of transaction bytes remains unfixed.
This issue was raised in a previous review. At line 355-356, a
Box<[u8]>is cast to*mut u8, discarding the slice length metadata. The deallocation at line 526 reconstructs it asBox<u8>(single byte) instead of the original boxed slice, causing heap corruption.The
transaction_bytes_freefunction needs the length to properly deallocate:Proposed fix
// In wallet_build_and_sign_transaction, document that caller must pass length to free // Update transaction_bytes_free signature: -pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8) { +pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8, tx_len: usize) { if !tx_bytes.is_null() { - let _ = Box::from_raw(tx_bytes); + let slice_ptr = core::ptr::slice_from_raw_parts_mut(tx_bytes, tx_len); + let _ = Box::from_raw(slice_ptr); } }As per coding guidelines: "Be careful with FFI memory management" and "All FFI-exposed types must have corresponding
_destroy()functions for explicit memory management."Also applies to: 523-528
🤖 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 355 - 357, The code casts a Box<[u8]> to *mut u8 losing the slice length (serialized -> boxed -> tx_bytes) and later deallocates it incorrectly in transaction_bytes_free (reconstructing Box<u8>), causing heap corruption; fix by returning/retaining the original length alongside the raw pointer (e.g., change the FFI type to carry (ptr, len) or allocate a struct that contains ptr and len), then in transaction_bytes_free reconstruct the boxed slice with Box::from_raw(std::slice::from_raw_parts_mut(ptr, len)) or Box::<[u8]>::from_raw(raw_ptr_with_len) to drop correctly; update all call sites that create tx_bytes (serialized, boxed, tx_bytes) and the transaction_bytes_free signature/implementation to accept the length and use it for safe deallocation.
🧹 Nitpick comments (3)
key-wallet-ffi/src/transaction.rs (1)
338-349: Fee calculation logic addresses prior concern but relies on output count comparison.The conditional logic comparing
transaction.output.len()totx_builder_with_inputs.outputs().len()to determine whether a change output was added is a reasonable approach. However, this couples the fee calculation to an implementation detail ofTransactionBuilder.Consider adding a comment or assertion to document this invariant, or exposing a method on
TransactionBuilderthat returns whether a change output was added, to make this less fragile.🤖 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 338 - 349, The fee selection currently infers whether a change output was added by comparing transaction.output.len() to tx_builder_with_inputs.outputs().len(), which couples logic to TransactionBuilder internals; update this by either (A) adding an explicit method on TransactionBuilder (e.g., TransactionBuilder::has_change_output or TransactionBuilder::did_add_change_output) and use it here to choose between tx_builder_with_inputs.calculate_fee_with_extra_output() and calculate_fee(), or (B) add a clear assertion/comment documenting the invariant (that a larger transaction.output.len() implies a change output) immediately above this conditional; reference TransactionBuilder, tx_builder_with_inputs.outputs(), calculate_fee_with_extra_output(), and calculate_fee() when making the change.key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (2)
1235-1237: Add a regression assertion for fee consistency in this updated test path.Since this PR’s goal is post-build fee reporting, assert that the reported fee path matches
sum(inputs) - sum(tx.outputs)for a case that adds change.🤖 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 1235 - 1237, After building the transaction (let mut builder = result.unwrap(); let tx = builder.build().unwrap();) add a regression assertion that the reported fee equals sum(inputs) - sum(tx.outputs): compute inputs_sum from the test's input UTXO amounts (or builder's input list), compute outputs_sum by summing tx.outputs amounts, set expected_fee = inputs_sum - outputs_sum and assert expected_fee == reported_fee (replace reported_fee with the actual variable/name used in the test that holds the post-build reported fee); place this assertion immediately after tx is built and after the reported fee is produced.
85-87: Change return type from&Vec<TxOut>to&[TxOut]for idiomatic API design.The single caller at
key-wallet-ffi/src/transaction.rs:345uses.outputs().len(), which is compatible with both slice and Vec return types. Returning a slice reduces API coupling and follows Rust conventions for collection accessors.🤖 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 85 - 87, Change the outputs accessor signature from returning &Vec<TxOut> to a slice & [TxOut] by updating the method signature outputs(&self) -> &[TxOut] and returning &self.outputs as a slice; this preserves callers like transaction.rs that call .outputs().len() while reducing API coupling (update the function named outputs in TransactionBuilder/managed_wallet_info::transaction_builder.rs accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 298-323: The closure passed to tx_builder.select_inputs currently
calls Secp256k1::new() for every UTXO; move the expensive Secp256k1 context
creation out of the closure so it is created once and reused. Create a secp
variable (using secp256k1::Secp256k1::new()) immediately before calling
tx_builder.select_inputs and capture a reference or clone of it into the
closure, then use that secp when calling
root_xpriv.to_extended_priv_key(...).derive_priv(...); keep the existing error
handling and the rest of the closure logic (address_to_path lookup, derive_priv
call, returning Some(private_key)).
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 357-358: The bug is that build() may add a change output via the
"extra-output" path so the fee used for the final Transaction differs from
calculate_fee(), which still uses self.outputs.len(); fix by making
calculate_fee() compute fees from the same source build() uses: either update
build() to append the change output into self.outputs (or set a dedicated
built_outputs vector) or set a built flag plus store the final outputs/count and
have calculate_fee() use that stored final outputs/count when present; update
symbols: TransactionBuilder::build, TransactionBuilder::calculate_fee,
self.outputs, any change_output/built_outputs field so calculate_fee() and
build() derive the output count identically.
---
Outside diff comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 373-416: The current early funds check using total_input <
total_output allows builds that later deduct fee (via
calculate_fee_with_extra_output) and produce underfunded-fee transactions;
compute the fee (call calculate_fee_with_extra_output or its deterministic
components) before finalizing funds validation and replace the existing check
with one that ensures total_input >= total_output + fee (return
BuilderError::InsufficientFunds with available: total_input and required:
total_output + fee), and then compute change_amount as total_input -
total_output - fee; update references in this function (inputs, total_input,
total_output, calculate_fee_with_extra_output, BuilderError::InsufficientFunds,
change_amount) accordingly so no transaction can be built if inputs cannot cover
outputs plus fee.
---
Duplicate comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 186-198: Check for null before calling CStr::from_ptr on each
output.address in the loop over outputs_slice: if output.address.is_null() set
an FFIError via FFIError::set_error with FFIErrorCode::InvalidInput (message
like "Null output address") and return false; only then safely call
CStr::from_ptr(output.address).to_str() and handle the Utf8 error as already
implemented.
- Around line 355-357: The code casts a Box<[u8]> to *mut u8 losing the slice
length (serialized -> boxed -> tx_bytes) and later deallocates it incorrectly in
transaction_bytes_free (reconstructing Box<u8>), causing heap corruption; fix by
returning/retaining the original length alongside the raw pointer (e.g., change
the FFI type to carry (ptr, len) or allocate a struct that contains ptr and
len), then in transaction_bytes_free reconstruct the boxed slice with
Box::from_raw(std::slice::from_raw_parts_mut(ptr, len)) or
Box::<[u8]>::from_raw(raw_ptr_with_len) to drop correctly; update all call sites
that create tx_bytes (serialized, boxed, tx_bytes) and the
transaction_bytes_free signature/implementation to accept the length and use it
for safe deallocation.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 338-349: The fee selection currently infers whether a change
output was added by comparing transaction.output.len() to
tx_builder_with_inputs.outputs().len(), which couples logic to
TransactionBuilder internals; update this by either (A) adding an explicit
method on TransactionBuilder (e.g., TransactionBuilder::has_change_output or
TransactionBuilder::did_add_change_output) and use it here to choose between
tx_builder_with_inputs.calculate_fee_with_extra_output() and calculate_fee(), or
(B) add a clear assertion/comment documenting the invariant (that a larger
transaction.output.len() implies a change output) immediately above this
conditional; reference TransactionBuilder, tx_builder_with_inputs.outputs(),
calculate_fee_with_extra_output(), and calculate_fee() when making the change.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 1235-1237: After building the transaction (let mut builder =
result.unwrap(); let tx = builder.build().unwrap();) add a regression assertion
that the reported fee equals sum(inputs) - sum(tx.outputs): compute inputs_sum
from the test's input UTXO amounts (or builder's input list), compute
outputs_sum by summing tx.outputs amounts, set expected_fee = inputs_sum -
outputs_sum and assert expected_fee == reported_fee (replace reported_fee with
the actual variable/name used in the test that holds the post-build reported
fee); place this assertion immediately after tx is built and after the reported
fee is produced.
- Around line 85-87: Change the outputs accessor signature from returning
&Vec<TxOut> to a slice & [TxOut] by updating the method signature outputs(&self)
-> &[TxOut] and returning &self.outputs as a slice; this preserves callers like
transaction.rs that call .outputs().len() while reducing API coupling (update
the function named outputs in
TransactionBuilder/managed_wallet_info::transaction_builder.rs accordingly).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet-ffi/FFI_API.md
- key-wallet-ffi/include/key_wallet_ffi.h
ZocoLini
force-pushed
the
chore/build-transaction-ffi-for-ios
branch
from
6850534 to
775ae2f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
357-358:⚠️ Potential issue | 🟠 MajorKeep post-build fee reporting consistent with the built transaction.
Line 357 changed
buildto&mut selffor post-build fee access, butcalculate_fee()still usesself.outputs.len(). If change is added only to localtx_outputsduring build, post-build fee can be underreported versus the built transaction shape.🤖 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 357 - 358, The build method was changed to &mut self but calculate_fee() still reads self.outputs while build assembles a local tx_outputs, causing a mismatch; fix by making calculate_fee accept the actual outputs used to build the transaction (e.g., add a signature like calculate_fee(&[TransactionOutput]) or calculate_fee_with(&self, &tx_outputs)) or by updating self.outputs to match tx_outputs before calling calculate_fee(), and then use that value for post-build fee reporting so the reported fee reflects the exact outputs in the constructed Transaction (refer to build and calculate_fee symbols).key-wallet-ffi/src/transaction.rs (2)
60-65:⚠️ Potential issue | 🔴 CriticalAvoid taking
FFIFeeRateenum directly in the extern C ABI.At Line 105, an out-of-range integer from C/Swift can produce an invalid Rust enum discriminant, which is UB before your match/conversion logic runs. Prefer
u32at the ABI boundary and validate withTryFrom<u32>.Suggested fix
#[repr(C)] pub enum FFIFeeRate { Economy = 0, Normal = 1, Priority = 2, } +impl TryFrom<u32> for FFIFeeRate { + type Error = (); + fn try_from(value: u32) -> Result<Self, Self::Error> { + match value { + 0 => Ok(Self::Economy), + 1 => Ok(Self::Normal), + 2 => Ok(Self::Priority), + _ => Err(()), + } + } +} + 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_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 FFIFeeRate::try_from(fee_rate_raw) { + Ok(v) => v, + Err(_) => { + FFIError::set_error(error, FFIErrorCode::InvalidInput, "Invalid fee rate".to_string()); + return false; + } + };Is passing an out-of-range integer from C into a Rust #[repr(C)] fieldless enum undefined behavior, and is validating via integer + TryFrom the recommended FFI pattern?As per coding guidelines,
**/*-ffi/**/*.rs: Exercise careful memory safety handling at FFI boundaries.Also applies to: 99-106
🤖 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 - 65, The FFIFeeRate enum must not be accepted directly across the C ABI; change any extern "C" function parameters and struct fields that currently take FFIFeeRate to use u32 (or libc::c_uint) at the ABI boundary, implement TryFrom<u32> for FFIFeeRate to validate the discriminant and convert to the safe Rust enum, and update call sites (e.g., conversion points around functions that previously accepted FFIFeeRate) to attempt the TryFrom conversion and return an error/NULL (or appropriate error code) on invalid values; ensure all references to FFIFeeRate in public FFI signatures are replaced by the integer type while keeping internal logic using the validated FFIFeeRate enum.
364-366:⚠️ Potential issue | 🔴 CriticalSerialized tx allocation is incompatible with current free path.
At Line 365,
Box<[u8]>is cast to*mut u8(thin pointer), buttransaction_bytes_freereconstructs withBox::from_raw(tx_bytes)(effectivelyBox<u8>). That is allocator-unsafe and can corrupt heap state.Suggested fix (length-aware free)
- let boxed = serialized.into_boxed_slice(); - let tx_bytes = Box::into_raw(boxed) as *mut u8; + let boxed = serialized.into_boxed_slice(); + let tx_bytes = Box::into_raw(boxed) as *mut u8; *tx_bytes_out = tx_bytes; *tx_len_out = size;-#[no_mangle] -pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8) { +#[no_mangle] +pub unsafe extern "C" fn transaction_bytes_free(tx_bytes: *mut u8, tx_len: usize) { if !tx_bytes.is_null() { - unsafe { - let _ = Box::from_raw(tx_bytes); - } + let slice_ptr = std::ptr::slice_from_raw_parts_mut(tx_bytes, tx_len); + let _ = Box::from_raw(slice_ptr); } }-/// - `tx_bytes` must be a valid pointer created by transaction functions or null +/// - `tx_bytes`/`tx_len` must match values returned by wallet_build_and_sign_transaction#!/bin/bash # Verify allocation/deallocation shape mismatch for transaction byte buffers rg -n -C3 'into_boxed_slice|Box::into_raw|transaction_bytes_free|Box::from_raw' key-wallet-ffi/src/transaction.rs key-wallet-ffi/include/key_wallet_ffi.hAs per coding guidelines,
**/*-ffi/**/*.rs: Be careful with FFI memory management.🤖 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 364 - 366, The current allocation casts a Box<[u8]> to a thin *mut u8 (boxed / serialized.into_boxed_slice() -> tx_bytes) but transaction_bytes_free reconstructs with Box::from_raw(tx_bytes), causing an allocation shape mismatch; change the API and implementation to be length-aware: when creating the buffer keep the length (let len = boxed.len()) and return both pointer and length, and in transaction_bytes_free accept (tx_bytes: *mut u8, len: usize) then reconstruct the original boxed slice with let slice_ptr = std::ptr::slice_from_raw_parts_mut(tx_bytes, len); Box::from_raw(slice_ptr) to drop it safely; update call sites to pass the length and remove the thin-pointer-only free path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 214-225: The code uses addr_network.unwrap() in the transaction
path; replace this with explicit error handling by matching on
addr.require_network(...) result (the addr_network variable) instead of
unwrapping: on Err call FFIError::set_error with FFIErrorCode::InvalidAddress
(as already done) and return false, and on Ok extract the value to continue;
update the block around addr.require_network, FFIError::set_error, and the
subsequent return so there is no unwrap() call and the function remains
panic-free.
---
Duplicate comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 60-65: The FFIFeeRate enum must not be accepted directly across
the C ABI; change any extern "C" function parameters and struct fields that
currently take FFIFeeRate to use u32 (or libc::c_uint) at the ABI boundary,
implement TryFrom<u32> for FFIFeeRate to validate the discriminant and convert
to the safe Rust enum, and update call sites (e.g., conversion points around
functions that previously accepted FFIFeeRate) to attempt the TryFrom conversion
and return an error/NULL (or appropriate error code) on invalid values; ensure
all references to FFIFeeRate in public FFI signatures are replaced by the
integer type while keeping internal logic using the validated FFIFeeRate enum.
- Around line 364-366: The current allocation casts a Box<[u8]> to a thin *mut
u8 (boxed / serialized.into_boxed_slice() -> tx_bytes) but
transaction_bytes_free reconstructs with Box::from_raw(tx_bytes), causing an
allocation shape mismatch; change the API and implementation to be length-aware:
when creating the buffer keep the length (let len = boxed.len()) and return both
pointer and length, and in transaction_bytes_free accept (tx_bytes: *mut u8,
len: usize) then reconstruct the original boxed slice with let slice_ptr =
std::ptr::slice_from_raw_parts_mut(tx_bytes, len); Box::from_raw(slice_ptr) to
drop it safely; update call sites to pass the length and remove the
thin-pointer-only free path.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 357-358: The build method was changed to &mut self but
calculate_fee() still reads self.outputs while build assembles a local
tx_outputs, causing a mismatch; fix by making calculate_fee accept the actual
outputs used to build the transaction (e.g., add a signature like
calculate_fee(&[TransactionOutput]) or calculate_fee_with(&self, &tx_outputs))
or by updating self.outputs to match tx_outputs before calling calculate_fee(),
and then use that value for post-build fee reporting so the reported fee
reflects the exact outputs in the constructed Transaction (refer to build and
calculate_fee symbols).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
QuantumExplorer
requested changes
Feb 26, 2026
| size_t outputs_count, | ||
| uint64_t fee_per_kb, | ||
| uint32_t current_height, | ||
| FFIFeeRate fee_rate, |
Member
There was a problem hiding this comment.
No... clients, especially iOS/Android have been able to set the fee_per_kb
Collaborator
Author
There was a problem hiding this comment.
If you want to, I thought it would be easier to expose the economy, normal, and priority values predefined in the FeeRate struct
Emit `SyncComplete` on every not-synced to synced transition instead of only once, with a cycle field (0 = initial, 1+ = incremental) so consumers can distinguish sync phases. Duration logging measures per-cycle time instead of time since client start.
ZocoLini
force-pushed
the
chore/build-transaction-ffi-for-ios
branch
from
775ae2f to
e054d67
Compare
QuantumExplorer
approved these changes
Feb 26, 2026
QuantumExplorer
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the wallet_build_and_sign_transaction method to fit our needs in iOS. It also returns the estimated fee to the caller for simplicity since we are not exposing the TransactionBuilder itself.
To be able to returns the fee I had to make build() not consume the builder so I can, after building and adding (or not) the output for change, call calculate_fee(). Had to modify a little bit the builder and drop one tests that was testing a functionality that I removed
I also had to execute almost the whole thing in the tokio runtime to be able to use the ManagedWalletInfo so a lot of lines are indented
Summary by CodeRabbit
New Features
Refactor