Uh oh!
There was an error while loading. Please reload this page.
feat: expose full TransactionRecord through FFI and wallet events - #614
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces multiple transaction callback parameters with a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WalletCore as Wallet Core
participant Manager as WalletManager
participant FFI as FFI Layer
participant Client as C Client
rect rgba(200,200,255,0.5)
WalletCore->>Manager: detect new transaction (tx)
Manager->>WalletCore: record_transaction(tx) -> TransactionRecord
Manager->>FFI: build FFITransactionRecord (tx_data, input/output details, label)
end
rect rgba(200,255,200,0.5)
FFI->>Client: invoke on_transaction_received(wallet_id, account_index, record_ptr, user_data)
Client-->>FFI: returns
FFI->>FFI: free nested allocations for record_ptr
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xdustinface
commented
Apr 1, 2026
CollaboratorAuthor
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Base automatically changed from
refactor/ffi-transaction-context-rename to
v0.42-devApril 1, 2026 10:35
xdustinfaceforce-pushed
the
feat/ffi-enrich-transaction-surface
branch
from
April 1, 2026 10:36
461105c to
a2bd33fCompareQuantumExplorer
marked this pull request as ready for review
April 1, 2026 10:37
xdustinface
commented
Apr 1, 2026
CollaboratorAuthor
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
key-wallet-manager/src/event_tests.rs (1)
12-92: Add one lifecycle test for non-index account types.Current updates validate the new
recordpayload well, but mostly through indexed account paths. Add aTransactionReceivedassertion for a non-index account type to catch dropped-event regressions innew_recordspropagation.As per coding guidelines, "Write unit tests for new functionality".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/event_tests.rs` around lines 12 - 92, Add a new lifecycle test that exercises a non-index account type and asserts a TransactionReceived event is emitted and carries the full `record` payload; use the same helpers as other tests (setup_manager_with_wallet, create_tx_paying_to, subscribe_events, assert_single_event) and call manager.check_transaction_in_all_wallets(&tx, TransactionContext::Mempool, true, true).await (or TransactionContext::InBlock for first-seen-in-block variants) to trigger new_records propagation, then match WalletEvent::TransactionReceived and assert record.context, record.txid == tx.txid(), ev_wid == wallet_id, and record.net_amount == TX_AMOUNT as i64 to catch dropped-event regressions for non-index accounts.key-wallet-ffi/src/managed_account.rs (1)
665-667: Scope the ownership note to the owned transaction API.
FFITransactionRecordis now also passed as a borrowed callback payload indash-spv-ffi/src/callbacks.rs:537-546. Keepingmanaged_core_account_free_transactionsin the type-level doc makes the ownership contract look universal, which can steer FFI callers toward freeing borrowed callback memory.Suggested wording
-/// Heap-allocated fields must be freed with `managed_core_account_free_transactions`.+/// When returned by `managed_core_account_get_transactions`, heap-allocated fields+/// must be freed with `managed_core_account_free_transactions`.+///+/// This struct is also used as a borrowed callback payload in `dash-spv-ffi`;+/// borrowed callback records and their nested pointers are only valid for the+/// duration of the callback and must not be freed by the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 665 - 667, The type-level doc for FFITransactionRecord wrongly implies all instances must be freed with managed_core_account_free_transactions; update the comment to scope that ownership contract to the "owned transaction" API only (e.g., instances returned by managed_core_account_free_transactions or the managed account owned API) and explicitly state that callback-provided/borrowed FFITransactionRecord payloads are not owned and must not be freed by callers (reference FFITransactionRecord and managed_core_account_free_transactions and the borrowed callback 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 `@dash-spv-ffi/src/callbacks.rs`:
- Around line 758-763: The code currently converts record.label into an owned
raw C string with CString::new(...).unwrap().into_raw(), leaking memory and
risking a panic on interior NULs; instead, create a local CString (e.g., let
c_label = CString::new(record.label.as_deref().unwrap_or("")).map_err(|e| /* map
to our error type */)?), keep it in scope across the cb(...) invocation and pass
c_label.as_ptr() (or c_label.as_ptr() as *mut _ if mutable is required) so it is
not into_raw'd, and propagate/handle the CString::new error instead of
unwrap/expect; apply the same change to the other occurrence mentioned (lines
~772-779) and replace unwrap() calls with proper error handling using the
crate's error type.
In `@dash-spv-ffi/tests/dashd_sync/callbacks.rs`:
- Around line 354-357: The two separate pushes to tracker.received_txids and
tracker.received_amounts can interleave across threads and break per-callback
pairing; change the callback in callbacks.rs so it captures the entire
FFITransactionRecord snapshot (use the unsafe deref of record to read r.txid,
r.net_amount and any new fields) and then perform a single lock/unlock to push
one combined snapshot (e.g., push a struct or tuple) into a new
tracker.received_snapshots Vec, replacing the two separate pushes to
received_txids and received_amounts to ensure each entry represents one callback
atomically.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 127-130: The code currently only pushes new_records when
account_match.account_type_match.account_index() returns Some, which discards
records for non-index account types and prevents downstream TransactionReceived
events; change the logic in the wallet checking flow so that record =
account.record_transaction(tx, &account_match, context, tx_type) is always
appended to result.new_records (e.g., push a variant that includes None for the
account index or push all records unconditionally and let downstream handle None
indices), updating any consumers to handle Option<usize> if necessary; then add
a regression unit test that simulates a non-index account receiving a
transaction and asserts that TransactionReceived is emitted for that account
type to prevent future regressions.
---
Nitpick comments:
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 665-667: The type-level doc for FFITransactionRecord wrongly
implies all instances must be freed with managed_core_account_free_transactions;
update the comment to scope that ownership contract to the "owned transaction"
API only (e.g., instances returned by managed_core_account_free_transactions or
the managed account owned API) and explicitly state that
callback-provided/borrowed FFITransactionRecord payloads are not owned and must
not be freed by callers (reference FFITransactionRecord and
managed_core_account_free_transactions and the borrowed callback usage).
In `@key-wallet-manager/src/event_tests.rs`:
- Around line 12-92: Add a new lifecycle test that exercises a non-index account
type and asserts a TransactionReceived event is emitted and carries the full
`record` payload; use the same helpers as other tests
(setup_manager_with_wallet, create_tx_paying_to, subscribe_events,
assert_single_event) and call manager.check_transaction_in_all_wallets(&tx,
TransactionContext::Mempool, true, true).await (or TransactionContext::InBlock
for first-seen-in-block variants) to trigger new_records propagation, then match
WalletEvent::TransactionReceived and assert record.context, record.txid ==
tx.txid(), ev_wid == wallet_id, and record.net_amount == TX_AMOUNT as i64 to
catch dropped-event regressions for non-index accounts.
🪄 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: f6a6bc2b-105d-48bb-a6df-bc96e0ecee56
📒 Files selected for processing (13)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv/tests/dashd_sync/helpers.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## v0.42-dev #614 +/- ##
=============================================
+ Coverage 67.45% 67.53% +0.08%
=============================================
Files 320 320 Lines 67949 68169 +220 =============================================
+ Hits 45832 46038 +206 - Misses 22117 22131 +14
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dash-spv-ffi/src/callbacks.rs (1)
758-763:⚠️ Potential issue | 🟠 MajorMemory leak:
labelCString is allocated but never freed after callback.The
labelfield usesCString::new(...).unwrap().into_raw()which transfers ownership to the raw pointer. Unlikeinput_detailsaddresses which are freed after the callback (lines 772-779), thelabelis never reclaimed, leaking memory for every labeled transaction event.Additionally,
unwrap()can panic on labels containing interior NUL bytes.🔧 Suggested fix: Keep CString alive across callback
+ let label_cstring = record+ .label+ .as_ref()+ .map(|l| CString::new(l.as_str()).unwrap_or_default());+ let ffi_record = FFITransactionRecord { // ... other fields ... - label: record- .label- .as_ref()- .map(|l| CString::new(l.as_str()).unwrap().into_raw())- .unwrap_or(std::ptr::null_mut()),+ label: label_cstring+ .as_ref()+ .map_or(std::ptr::null_mut(), |c| c.as_ptr() as *mut _), };As per coding guidelines,
**/src/**/*.rs: Avoidunwrap()andexpect()in library code; use proper error types.🤖 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 758 - 763, The code allocates a CString for record.label using CString::new(...).unwrap().into_raw() and never frees it, leaking on every callback and risking panics from unwrap() on interior NULs; change the approach in the callback construction (the usage around label and record.label) to safely create the CString without unwrap (return a Result or propagate an error) and ensure the raw pointer is reclaimed after the callback returns (mirror how input_details pointers are freed in the block that releases them), or keep the CString owned for the callback lifetime and convert to a raw pointer only for the call then call CString::from_raw(...) to free it immediately after; update any functions that build or consume label to accept a nullable pointer safely and remove unwrap() usage.
🧹 Nitpick comments (2)
key-wallet-ffi/src/types.rs (1)
861-874: Consider adding#[derive(Debug)]toFFIInputDetailandFFIOutputDetailfor consistency.The new FFI enums (
FFITransactionDirection,FFITransactionType,FFIOutputRole) all have#[derive(Debug, Clone, Copy)], but these structs lack derives. WhileFFIInputDetailcan't deriveDebugdirectly due to the raw pointer,FFIOutputDetailcould benefit from it for debugging consistency with other FFI types.♻️ Proposed fix for FFIOutputDetail
/// FFI-compatible output detail #[repr(C)] +#[derive(Debug, Clone, Copy)] pub struct FFIOutputDetail { pub index: u32, pub role: FFIOutputRole, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/types.rs` around lines 861 - 874, FFIOutputDetail should derive Debug for consistency with the other FFI types — add #[derive(Debug, Clone, Copy)] (or at least #[derive(Debug)]) to the FFIOutputDetail declaration; FFIInputDetail cannot use the automatic Debug derive because it contains a raw C string pointer, so implement Debug manually for FFIInputDetail (impl std::fmt::Debug for FFIInputDetail) and format index and value directly and print the address safely (either as the raw pointer or by converting to &CStr inside an unsafe block with clear fallback) to avoid deriving on the raw pointer.dash-spv-ffi/src/callbacks.rs (1)
740-756: Document the borrow semantics of array pointers in FFITransactionRecord.The
input_details,output_details, andtx_datapointers point to stack-allocated Vecs that become invalid after the callback returns. While the callback documentation mentions the record is "borrowed," callers might assume they can callmanaged_core_account_free_transactionson callback-delivered records, which would cause undefined behavior since these pointers weren't heap-allocated via that function.Consider adding a comment clarifying that records from callbacks have different ownership semantics than records from
managed_core_account_get_transactions.🤖 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 740 - 756, Add a clear comment to FFITransactionRecord documenting that the pointers input_details, output_details, and tx_data are borrows into stack/temporary Vecs provided only for the duration of the callback and must not be freed or passed to managed_core_account_free_transactions; explicitly state that ownership semantics differ from records returned by managed_core_account_get_transactions (which are heap-owned and must be freed), and mention callers who need persistent data should clone or allocate their own copies before the callback returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 758-763: The code allocates a CString for record.label using
CString::new(...).unwrap().into_raw() and never frees it, leaking on every
callback and risking panics from unwrap() on interior NULs; change the approach
in the callback construction (the usage around label and record.label) to safely
create the CString without unwrap (return a Result or propagate an error) and
ensure the raw pointer is reclaimed after the callback returns (mirror how
input_details pointers are freed in the block that releases them), or keep the
CString owned for the callback lifetime and convert to a raw pointer only for
the call then call CString::from_raw(...) to free it immediately after; update
any functions that build or consume label to accept a nullable pointer safely
and remove unwrap() usage.
---
Nitpick comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 740-756: Add a clear comment to FFITransactionRecord documenting
that the pointers input_details, output_details, and tx_data are borrows into
stack/temporary Vecs provided only for the duration of the callback and must not
be freed or passed to managed_core_account_free_transactions; explicitly state
that ownership semantics differ from records returned by
managed_core_account_get_transactions (which are heap-owned and must be freed),
and mention callers who need persistent data should clone or allocate their own
copies before the callback returns.
In `@key-wallet-ffi/src/types.rs`:
- Around line 861-874: FFIOutputDetail should derive Debug for consistency with
the other FFI types — add #[derive(Debug, Clone, Copy)] (or at least
#[derive(Debug)]) to the FFIOutputDetail declaration; FFIInputDetail cannot use
the automatic Debug derive because it contains a raw C string pointer, so
implement Debug manually for FFIInputDetail (impl std::fmt::Debug for
FFIInputDetail) and format index and value directly and print the address safely
(either as the raw pointer or by converting to &CStr inside an unsafe block with
clear fallback) to avoid deriving on the raw pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5a607be-a459-4b28-9036-25cdc4167292
📒 Files selected for processing (13)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv/tests/dashd_sync/helpers.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rs
✅ Files skipped from review due to trivial changes (1)
- dash-spv-ffi/tests/dashd_sync/callbacks.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- dash-spv/tests/dashd_sync/helpers.rs
- key-wallet/src/managed_account/mod.rs
- key-wallet-manager/src/event_tests.rs
- key-wallet/src/transaction_checking/account_checker.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-manager/src/events.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- Add FFI types for transaction direction, type, output role, and input/output details - Enrich `FFITransactionRecord` with classification, details, label and serialized tx bytes - Simplify `WalletEvent::TransactionReceived` to carry `Box<TransactionRecord>` - Return `TransactionRecord` from `record_transaction` and propagate via `TransactionCheckResult` - Refactor `OnTransactionReceivedCallback` to pass `*const FFITransactionRecord`
Addresses CodeRabbit review comment on PR #614#614 (comment)
xdustinface added a commit
that referenced
this pull request
Apr 1, 2026
Addresses CodeRabbit review comment on PR #614#614 (comment)
xdustinfaceforce-pushed
the
feat/ffi-enrich-transaction-surface
branch
from
April 1, 2026 11:17
a2bd33f to
0452c0aCompareAddresses CodeRabbit review comment on PR #614#614 (comment)
xdustinfaceforce-pushed
the
feat/ffi-enrich-transaction-surface
branch
from
April 1, 2026 11:21
0452c0a to
06490f9CompareThe test sends 1 DASH from the same wallet (same mnemonic) that the SPV client tracks, making it an internal transfer where both inputs and outputs belong to the wallet. The `net_amount` therefore equals approximately `-fee`, not +100_000_000. Replace the strict amount pairing assertion with a txid presence check and non-zero net_amount verification.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
127-130:⚠️ Potential issue | 🟠 MajorStill drops
TransactionReceivedrecords for non-index accounts.Lines 128-130 still discard the freshly recorded transaction whenever
account_index()isNone, so non-index account types can update wallet state without ever reachingnew_recordsor the downstreamWalletEvent::TransactionReceivedpath. Please carry a richer account identifier here (Option<u32>or account-type metadata) instead of filtering the record out, and add a regression test for a non-index account match. As per coding guidelines, "Write unit tests for new functionality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction_checking/wallet_checker.rs` around lines 127 - 130, The code currently drops TransactionReceived records when account_match.account_type_match.account_index() is None; change result.new_records to carry a richer identifier (e.g., change its element from (u32, Record) to (Option<u32>, Record) or include account-type metadata) so the record is always pushed: update the push site in wallet_checker.rs (currently using account.record_transaction and result.new_records.push((account_index, record))) to push the Option<u32> or metadata instead, then update downstream consumers (the collection that feeds WalletEvent::TransactionReceived) to accept the new identifier shape and preserve behavior for Some(account_index) and None; finally add a unit test that creates a non-index account match and asserts WalletEvent::TransactionReceived is produced for that case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 127-130: The code currently drops TransactionReceived records when
account_match.account_type_match.account_index() is None; change
result.new_records to carry a richer identifier (e.g., change its element from
(u32, Record) to (Option<u32>, Record) or include account-type metadata) so the
record is always pushed: update the push site in wallet_checker.rs (currently
using account.record_transaction and result.new_records.push((account_index,
record))) to push the Option<u32> or metadata instead, then update downstream
consumers (the collection that feeds WalletEvent::TransactionReceived) to accept
the new identifier shape and preserve behavior for Some(account_index) and None;
finally add a unit test that creates a non-index account match and asserts
WalletEvent::TransactionReceived is produced for that case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f216da13-7c17-4161-bf46-5c6f5d88f5fe
📒 Files selected for processing (15)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv-ffi/tests/dashd_sync/tests_callback.rsdash-spv-ffi/tests/dashd_sync/tests_transaction.rsdash-spv/tests/dashd_sync/helpers.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- dash-spv/tests/dashd_sync/helpers.rs
- key-wallet-manager/src/test_helpers.rs
- key-wallet/src/transaction_checking/account_checker.rs
- key-wallet-manager/src/event_tests.rs
- key-wallet-manager/src/events.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
ZocoLini
approved these changes
Apr 1, 2026
QuantumExplorer
approved these changes
Apr 1, 2026
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 freeto 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.
FFITransactionRecordwith classification, details, label and serialized tx bytesWalletEvent::TransactionReceivedto carryBox<TransactionRecord>TransactionRecordfromrecord_transactionand propagate viaTransactionCheckResultOnTransactionReceivedCallbackto pass*const FFITransactionRecordBased on:
key-walletnaming #612Summary by CodeRabbit
New Features
Bug Fixes
Tests