feat: add address and value fields to FFIOutputDetail#640
feat: add address and value fields to FFIOutputDetail#640xdustinface merged 3 commits intov0.42-devfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdded FFI functions to convert scripts to addresses and to extract transaction outputs, extended FFI types and account transaction conversions to include output value and address, and updated SPV FFI callbacks and client wiring to provide network context for address derivation; docs updated and a CLI struct init adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant C as C Caller
participant FFI as key-wallet-ffi
participant Script as dashcore::ScriptBuf
participant Addr as key_wallet::Address
C->>FFI: script_to_address(script_bytes, script_len, network)
FFI->>Script: construct ScriptBuf from bytes
Script-->>FFI: script object / error
FFI->>Addr: Address::from_script(script, network)
Addr-->>FFI: address string / error
FFI-->>C: *mut c_char (heap string) or null
sequenceDiagram
participant C as C Caller
participant FFI as key-wallet-ffi
participant Tx as dashcore::Transaction
C->>FFI: transaction_get_output(tx_bytes, tx_len, output_index, ...)
FFI->>Tx: deserialize Transaction from bytes
Tx-->>FFI: Transaction or error
FFI->>Tx: access tx.output[output_index]
Tx-->>FFI: script_pubkey bytes + value or index error
FFI->>FFI: allocate heap buffer, copy script bytes
FFI-->>C: true + script_ptr, script_len, value (or false on error)
C->>FFI: transaction_output_free(script_ptr, script_len)
FFI-->>FFI: reconstruct Vec and drop (no-op if null/len==0)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #640 +/- ##
=============================================
+ Coverage 67.81% 67.99% +0.17%
=============================================
Files 318 318
Lines 67976 67823 -153
=============================================
+ Hits 46100 46115 +15
+ Misses 21876 21708 -168
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
dash-spv-ffi/src/callbacks.rs (1)
728-742:⚠️ Potential issue | 🟠 MajorFree
output_details[*].addressafter the callback.Each
CString::into_raw()here transfers ownership, but this dispatch path only freesinput_details[*].address.output_details[*].addresswill leak on everyTransactionReceivedevent that includes a decoded output address.♻️ Suggested cleanup
cb( c_wallet_id.as_ptr(), *account_index, &ffi_record as *const FFITransactionRecord, self.user_data, ); // Free the CString addresses from input details for detail in input_details { if !detail.address.is_null() { unsafe { drop(CString::from_raw(detail.address)); } } } + for detail in output_details { + if !detail.address.is_null() { + unsafe { + drop(CString::from_raw(detail.address)); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/callbacks.rs` around lines 728 - 742, The code creates CStrings for output addresses with CString::into_raw() when building output_details (FFIOutputDetail) but never frees them, causing leaks on TransactionReceived paths; mirror the cleanup done for input_details by iterating over output_details after the callback and calling CString::from_raw(ptr) for each non-null address to take back ownership and drop it (ensure you check for std::ptr::null_mut() before from_raw), referencing the output_details variable and FFIOutputDetail::address so the free logic is colocated with the existing input_details free routine.
🤖 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/types.rs`:
- Around line 930-934: You changed the FFI struct layout by adding fields to
FFIOutputDetail (adding value: u64 and address: *mut std::os::raw::c_char) which
breaks the C ABI for consumers; to fix, revert the struct layout change or
coordinate a safe migration: keep the original repr(C) layout for
FFIOutputDetail, introduce a new struct (e.g., FFIOutputDetailV2) with the
additional fields, and update internal code to produce the new struct only for
consumers that are rebuilt together; ensure all FFI-producing functions and any
constructors/serializers that return *mut FFIOutputDetail are adjusted to return
the appropriate type and document the required binding updates for downstream
consumers.
In `@key-wallet/src/managed_account/transaction_record.rs`:
- Around line 40-43: The TransactionRecord struct's fields address and value
need serde defaults to avoid deserialization failures for older persisted data;
add the attribute #[cfg_attr(feature = "serde", serde(default))] to both the pub
address: Option<Address> and pub value: u64 fields in transaction_record.rs so
missing address defaults to None and missing value defaults to 0 when the
"serde" feature is enabled.
---
Outside diff comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 728-742: The code creates CStrings for output addresses with
CString::into_raw() when building output_details (FFIOutputDetail) but never
frees them, causing leaks on TransactionReceived paths; mirror the cleanup done
for input_details by iterating over output_details after the callback and
calling CString::from_raw(ptr) for each non-null address to take back ownership
and drop it (ensure you check for std::ptr::null_mut() before from_raw),
referencing the output_details variable and FFIOutputDetail::address so the free
logic is colocated with the existing input_details free routine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb2446aa-88b1-4323-8c61-bba9b71d7eb3
📒 Files selected for processing (5)
dash-spv-ffi/src/callbacks.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/transaction_record.rs
llbartekll
force-pushed
the
feat/output-detail-address
branch
from
8be40f8 to
7c66864
Compare
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Looks alright generally but i actually intentionally didn't add those because we store redundant data when they are added to the record since the TransactionRecord contains the full Transaction so with the index of the OutputDetail you can get TxOut.script_pubkey and convert it.
However.. it looks like there is no script_to_address function in the FFI but there might be one in the swift SDK? Can you have a look around there?
Or.. we could maybe just add it in the FFIOutputDetails and calculate it in the FFI when requested.
Im leaning more towards the first option (calculation helper) since we then don't diverge the FFI type from the rust type.
Would that work or is there something about all this that justifies storing address/amount twice?
llbartekll
force-pushed
the
feat/output-detail-address
branch
from
7c66864 to
ece3a61
Compare
makes sense, couldn't find a |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-wallet-ffi/src/transaction.rs (1)
523-537: Initialize out-params to safe defaults before fallible paths.When returning
false, stale caller values may remain inscript_out/script_len_out/value_out. Pre-initialize them to deterministic defaults.Suggested hardening
pub unsafe extern "C" fn transaction_get_output_script( @@ ) -> bool { if tx_bytes.is_null() || script_out.is_null() || script_len_out.is_null() || value_out.is_null() { return false; } + + *script_out = ptr::null_mut(); + *script_len_out = 0; + *value_out = 0; let tx_slice = slice::from_raw_parts(tx_bytes, tx_len);🤖 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 523 - 537, Before any fallible operations in the function, set the out-parameters to safe deterministic defaults so caller state cannot be left stale: assign script_out to null (or equivalent empty pointer), script_len_out to 0, and value_out to 0 before performing the null-checks and before calling slice::from_raw_parts, consensus::deserialize, or tx.output.get; keep these initializations near the top of the function (before the early returns that currently return false) so every error path returns with known values for script_out, script_len_out, and value_out.
🤖 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/address.rs`:
- Line 140: The code path in the match arm returning a CString for Ok(addr) uses
CString::new(...).unwrap_or_default().into_raw(), which can yield a pointer to
an empty string on conversion failure; update the Ok branch in the function (the
match arm currently containing Ok(addr) => ...) to handle
CString::new(addr.to_string())'s Result explicitly and return a null pointer on
Err (e.g., map or match the CString::new result and call into_raw() only on Ok,
otherwise return std::ptr::null_mut()), preserving the function's
null-on-failure contract.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 523-537: Before any fallible operations in the function, set the
out-parameters to safe deterministic defaults so caller state cannot be left
stale: assign script_out to null (or equivalent empty pointer), script_len_out
to 0, and value_out to 0 before performing the null-checks and before calling
slice::from_raw_parts, consensus::deserialize, or tx.output.get; keep these
initializations near the top of the function (before the early returns that
currently return false) so every error path returns with known values for
script_out, script_len_out, and value_out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c307d8a7-1b9a-4af2-976f-751d487f2499
📒 Files selected for processing (2)
key-wallet-ffi/src/address.rskey-wallet-ffi/src/transaction.rs
llbartekll
force-pushed
the
feat/output-detail-address
branch
3 times, most recently
from
e8ca18b to
19a9542
Compare
|
Which method are you calling to get the transaction?? 'managed_core_account_get_transactions'?? |
correct, I only modified |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
@llbartekll Seems like i was wrong with favouring Option 1 before. I guess since we dont have a proper way yet to inspect transactions in the FFI side this way feels also not quite right because we now deserialise the whole transaction for every output we pull out of it.
Lets maybe rather go with the address + value in the FFIOutputDetails? I was thinking more about all this and i think eventually it should also be in the OutputDetails on the rust side but we should overhaul the transaction storage situation first.
Or @ZocoLini you said you are working on the swift transaction record, did you have any more thoughts about this? If not lets just
llbartekll
force-pushed
the
feat/output-detail-address
branch
2 times, most recently
from
3d6cd97 to
9ba9f17
Compare
OutputDetail and FFIOutputDetail previously only had { index, role },
dropping the output address and value on the floor. The wallet crate
already resolves output addresses at managed_account/mod.rs:480-484
(via Address::from_script) — they just weren't stored in the struct.
- Add `address: Option<Address>` and `value: u64` to OutputDetail
(key-wallet/src/managed_account/transaction_record.rs)
- Populate at construction from resolved_outputs and output.value
(key-wallet/src/managed_account/mod.rs)
- Add `address: *mut c_char` and `value: u64` to FFIOutputDetail
(key-wallet-ffi/src/types.rs)
- Map in both key-wallet-ffi and dash-spv-ffi FFI conversions
- Update free function to free output address strings
- Fix test constructor to include new fields
This enables downstream Swift consumers (dashwallet-ios) to display
"Received at" and "Sent to" addresses in the transaction detail screen.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
llbartekll
force-pushed
the
feat/output-detail-address
branch
from
9ba9f17 to
c362f1d
Compare
Deserialization of old records without address/value should fail rather than silently defaulting — forces a resync which repopulates the fields correctly. Not used in production yet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
llbartekll
force-pushed
the
feat/output-detail-address
branch
from
2d2978b to
3f526cb
Compare
Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com>
3 participants
Summary
address: Option<Address>andvalue: u64toOutputDetail(key-wallet crate)address: *mut c_charandvalue: u64toFFIOutputDetail(key-wallet-ffi + dash-spv-ffi)indexandroleare unaffectedMotivation
The tx detail screen in dashwallet-ios needs output addresses to display "Received at" / "Sent to" sections. The wallet crate already resolves output addresses via
Address::from_scriptduring transaction processing but wasn't storing them inOutputDetail. This change surfaces what's already computed.Test plan
cargo fmtpassescargo test -p key-wallet-ffipassescargo test -p key-walletpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation