feat: add TransactionStatusChanged event and emit wallet events#552
Open
xdustinface wants to merge 3 commits intov0.42-devfrom
Open
feat: add TransactionStatusChanged event and emit wallet events#552xdustinface wants to merge 3 commits intov0.42-devfrom
TransactionStatusChanged event and emit wallet events#552xdustinface wants to merge 3 commits intov0.42-devfrom
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:
📝 WalkthroughWalkthroughAdds transaction-status awareness end-to-end: TransactionReceived now includes a TransactionContext status, a new TransactionStatusChanged event and FFI callback were added, WalletManager emits TransactionReceived only for new transactions and TransactionStatusChanged when state changes, and tests + helpers validate lifecycle and InstantSend lock handling. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant WM as WalletManager
participant TxCheck as TransactionChecker
participant EventEmit as EventEmitter
participant Observer as EventObserver
App->>WM: process_block(tx)
WM->>TxCheck: check_transaction_in_all_wallets(tx)
TxCheck->>TxCheck: evaluate context/status
alt is_new == true
TxCheck-->>WM: (is_new=true, state_modified=true, status)
WM->>EventEmit: emit TransactionReceived(wallet_id, status, account_index, txid, amount, addresses)
else state_modified == true
TxCheck-->>WM: (is_new=false, state_modified=true, status)
WM->>EventEmit: emit TransactionStatusChanged(wallet_id, txid, status)
else state_modified == false
TxCheck-->>WM: (state_modified=false)
WM->>WM: no event emitted
end
EventEmit->>Observer: deliver event
App->>WM: process_instant_send_lock(txid)
WM->>WM: mark_instant_send_utxos(txid)
WM->>EventEmit: emit TransactionStatusChanged(wallet_id, txid, InstantSend)
EventEmit->>Observer: deliver status change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
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
force-pushed
the
feat/transaction-status-events
branch
from
4c13f8a to
6116068
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #552 +/- ##
=============================================
+ Coverage 66.33% 66.40% +0.06%
=============================================
Files 311 312 +1
Lines 64976 65128 +152
=============================================
+ Hits 43103 43246 +143
- Misses 21873 21882 +9
|
xdustinface
force-pushed
the
feat/wallet-confirmation-lifecycle
branch
from
11405bc to
41d957f
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. |
Member
Review from ClaudeIssue 1: Duplicate
|
xdustinface
force-pushed
the
feat/wallet-confirmation-lifecycle
branch
from
41d957f to
698d75a
Compare
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
6116068 to
95f96fc
Compare
github-actions
bot
removed
the
merge-conflict
Base automatically changed from
feat/wallet-confirmation-lifecycle
to
v0.42-dev
March 18, 2026 16:42
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. |
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
95f96fc to
c8fd4b2
Compare
github-actions
bot
removed
the
merge-conflict
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
191-203:⚠️ Potential issue | 🟠 MajorGuard
state_modifiedon actual IS-lock changes.
state_modifiedis set unconditionally in the existing-tx InstantSend path, even when no UTXO changed. That can trigger redundantTransactionStatusChangedemissions downstream.Proposed fix
- // Mark UTXOs as IS-locked in affected accounts + // Mark UTXOs as IS-locked in affected accounts + let mut any_changed = false; for account_match in &result.affected_accounts { if let Some(account) = self .accounts .get_by_account_type_match_mut(&account_match.account_type_match) { - account.mark_utxos_instant_send(&txid); + if account.mark_utxos_instant_send(&txid) { + any_changed = true; + } } } - if update_balance { + if any_changed && update_balance { self.update_balance(); } - result.state_modified = true; + result.state_modified = any_changed; return result;🤖 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 191 - 203, The code currently sets result.state_modified = true unconditionally after calling account.mark_utxos_instant_send in the for loop; change this so state_modified is only set when at least one account actually changed a UTXO. Introduce a local boolean (e.g., let mut any_changed = false), call account.mark_utxos_instant_send(&txid) inside the loop and have that method return a bool indicating whether it modified any UTXO (or detect the change via account state), then set any_changed |= returned_bool. After the loop, only call self.update_balance() and set result.state_modified = true if any_changed is true; otherwise return result unchanged. Ensure you update the signature/behavior of mark_utxos_instant_send (or read account change flags) accordingly.dash-spv-ffi/src/callbacks.rs (1)
523-533:⚠️ Potential issue | 🔴 CriticalVersion the wallet callback ABI instead of changing v1 in place.
Line 526 inserts
statusahead of existing callback arguments, and line 575 addson_transaction_status_changedto the#[repr(C)] FFIWalletEventCallbacksstruct. Both changes break the C/Swift FFI contract: any external consumer built against the previous header will have misaligned callback signatures and struct layouts, causing runtime failures or silent memory corruption. No versioned callback surface exists in the codebase. Preserve the current v1 types/layout intact and introduceFFIWalletEventCallbacksV2/OnTransactionReceivedCallbackV2alongside a new registration functiondash_spv_ffi_client_set_wallet_event_callbacks_v2()to maintain forward compatibility.🤖 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 523 - 533, The change mutates the v1 ABI by inserting a new parameter into OnTransactionReceivedCallback and adding on_transaction_status_changed to FFIWalletEventCallbacks, breaking existing C/Swift consumers; instead keep the original OnTransactionReceivedCallback and FFIWalletEventCallbacks unchanged, and add new types FFIWalletEventCallbacksV2 and OnTransactionReceivedCallbackV2 (with the extra status param) plus a new registration function dash_spv_ffi_client_set_wallet_event_callbacks_v2() that accepts the V2 struct; update any internal wiring to support both registrations while preserving the original v1 symbols and layout so existing bindings remain valid.
🧹 Nitpick comments (1)
key-wallet-manager/src/wallet_interface.rs (1)
98-100: Clarify that the default implementation is intentionally a no-op.The doc comment currently states side effects (“marks UTXOs”, “emits events”), but the default method does nothing. Please explicitly note this behavior to avoid accidental reliance by implementations that don’t override it.
Suggested doc tweak
- /// Process an InstantSend lock for a transaction already in the wallet. - /// Marks UTXOs as IS-locked, emits status change and balance update events. + /// Process an InstantSend lock for a transaction already in the wallet. + /// Default implementation is a no-op; wallet implementations should override + /// this to mark UTXOs as IS-locked and emit status/balance events. fn process_instant_send_lock(&mut self, _txid: Txid) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_interface.rs` around lines 98 - 100, The doc comment for process_instant_send_lock currently describes side effects but the default implementation is a no-op; update the documentation for the method in wallet_interface.rs (fn process_instant_send_lock) to explicitly state that the provided default implementation does nothing and that implementations must override the method to perform actions such as marking UTXOs as IS-locked, emitting status-change events, and updating balances; keep the existing behavioral description but add a clear sentence like “The default implementation is a no-op — override to perform these actions.”
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/include/dash_spv_ffi.h`:
- Around line 377-383: The change inserted FFITransactionContext into the
OnTransactionReceivedCallback signature and added
OnTransactionStatusChangedCallback into the FFIWalletEventCallbacks layout,
breaking ABI; restore backward compatibility by keeping the original typedef and
struct intact and add new versions instead: define a new typedef
OnTransactionReceivedCallbackV2 (with the extra FFITransactionContext status
parameter) and a new struct FFIWalletEventCallbacksV2 that includes the new
OnTransactionStatusChangedCallback in the new layout, leaving the existing
OnTransactionReceivedCallback and FFIWalletEventCallbacks unchanged, and update
any registration APIs to accept either the original callbacks or the V2 variants
(or add an explicit registration function that requires the caller to opt into
V2) so precompiled clients keep the original memory layout while new clients can
opt into the new signatures.
In `@dash-spv-ffi/tests/dashd_sync/tests_callback.rs`:
- Around line 110-126: Add a positive end-to-end test that drives a real Mempool
-> InstantSend/InBlock transition and asserts the new status-bearing callback
arguments: update CallbackTracker to store the last wallet_id, txid, and status
(in addition to the existing counters) and expose accessors; in
tests_callback.rs create a scenario that submits a tx to the mempool, triggers
the confirmation/InstantSend transition, then assert that
on_transaction_received and on_transaction_status_changed were called with the
expected status value by reading the CallbackTracker's
last_wallet_id/last_txid/last_status and comparing them to the expected wallet
and txid and to the new status enum; reference the existing functions
on_transaction_received and on_transaction_status_changed and the
CallbackTracker struct so changes are colocated with the existing asserts.
In `@key-wallet-manager/src/test_utils/wallet.rs`:
- Around line 102-105: process_instant_send_lock currently uses try_lock on
status_changes which can silently drop events under contention; change it to
acquire the lock deterministically (e.g. call lock() / unwrap() or otherwise
block until acquired) and then push (txid, TransactionContext::InstantSend) so
status_changes never loses records under contention; update the function
process_instant_send_lock to use the blocking lock mechanism (or a retry loop
that logs/errs on failure) instead of try_lock to ensure deterministic test
behavior.
In `@key-wallet-manager/src/wallet_manager/event_tests.rs`:
- Around line 197-228: The test currently only calls
process_instant_send_lock(tx.txid()) and asserts presence of events; update it
to cover the mixed entry points by invoking both paths
(process_instant_send_lock and check_transaction_in_all_wallets(...,
TransactionContext::InstantSend, ...)) in both orders and collecting events via
subscribe_events/drain_events; then assert that across both calls there is
exactly one WalletEvent::TransactionStatusChanged with status
TransactionContext::InstantSend for the expected wallet_id (and still assert
BalanceUpdated once), to catch duplicate-emission bugs. Ensure you reference the
existing helpers (setup_manager_with_wallet, create_tx_paying_to, drain_events)
and keep the wallet_id equality checks as in the original assertions.
In `@key-wallet-manager/src/wallet_manager/test_helpers.rs`:
- Around line 14-22: Change setup_manager_with_wallet to accept a Network
parameter instead of hardcoding Network::Testnet: update the signature of
setup_manager_with_wallet to fn setup_manager_with_wallet(network: Network) ->
(WalletManager<ManagedWalletInfo>, WalletId, Address) and construct the manager
with WalletManager::new(network) (references: setup_manager_with_wallet,
WalletManager::new, Network); update all test call sites to call it with
Network::Testnet or Network::Mainnet as appropriate, and optionally add a small
no-arg wrapper (e.g., setup_manager_with_wallet_testnet) that forwards
Network::Testnet for backward compatibility so new tests can run both
configurations.
- Around line 46-52: The drain_events helper currently treats all try_recv
errors the same and can silently drop events; update the body of pub(crate) fn
drain_events(rx: &mut broadcast::Receiver<WalletEvent>) to use an explicit match
on rx.try_recv(): push the event on Ok(e), break the loop on
Err(broadcast::error::TryRecvError::Empty), and panic (or assert/unwrap with a
clear message) on Err(broadcast::error::TryRecvError::Lagged(n)) so tests fail
if the receiver lagged and events were lost; reference drain_events,
broadcast::Receiver, WalletEvent and try_recv when making the change.
---
Outside diff comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 523-533: The change mutates the v1 ABI by inserting a new
parameter into OnTransactionReceivedCallback and adding
on_transaction_status_changed to FFIWalletEventCallbacks, breaking existing
C/Swift consumers; instead keep the original OnTransactionReceivedCallback and
FFIWalletEventCallbacks unchanged, and add new types FFIWalletEventCallbacksV2
and OnTransactionReceivedCallbackV2 (with the extra status param) plus a new
registration function dash_spv_ffi_client_set_wallet_event_callbacks_v2() that
accepts the V2 struct; update any internal wiring to support both registrations
while preserving the original v1 symbols and layout so existing bindings remain
valid.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 191-203: The code currently sets result.state_modified = true
unconditionally after calling account.mark_utxos_instant_send in the for loop;
change this so state_modified is only set when at least one account actually
changed a UTXO. Introduce a local boolean (e.g., let mut any_changed = false),
call account.mark_utxos_instant_send(&txid) inside the loop and have that method
return a bool indicating whether it modified any UTXO (or detect the change via
account state), then set any_changed |= returned_bool. After the loop, only call
self.update_balance() and set result.state_modified = true if any_changed is
true; otherwise return result unchanged. Ensure you update the
signature/behavior of mark_utxos_instant_send (or read account change flags)
accordingly.
---
Nitpick comments:
In `@key-wallet-manager/src/wallet_interface.rs`:
- Around line 98-100: The doc comment for process_instant_send_lock currently
describes side effects but the default implementation is a no-op; update the
documentation for the method in wallet_interface.rs (fn
process_instant_send_lock) to explicitly state that the provided default
implementation does nothing and that implementations must override the method to
perform actions such as marking UTXOs as IS-locked, emitting status-change
events, and updating balances; keep the existing behavioral description but add
a clear sentence like “The default implementation is a no-op — override to
perform these actions.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53118de6-43ae-4900-8074-e21a55d76d96
📒 Files selected for processing (14)
dash-spv-ffi/include/dash_spv_ffi.hdash-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.rskey-wallet-manager/src/events.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/src/wallet_manager/event_tests.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rskey-wallet-manager/src/wallet_manager/test_helpers.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rs
xdustinface
added a commit
that referenced
this pull request
Mar 18, 2026
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
xdustinface
added a commit
that referenced
this pull request
Mar 18, 2026
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-manager/src/wallet_manager/event_tests.rs`:
- Around line 11-433: Update the event-flow tests to run against both mainnet
and testnet by parameterizing the core helpers and flows: modify
setup_manager_with_wallet, assert_lifecycle_flow, and assert_context_suppressed
to accept a Network/Chain parameter (or add an explicit wrapper that creates a
manager for both networks) and then change each top-level #[tokio::test] to
iterate or call the helper twice (once for mainnet and once for testnet); ensure
process_block, process_instant_send_lock and check_transaction_in_all_wallets
are invoked on the manager instance tied to the specified network so the same
assertions run for both configurations.
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 237-239: The early return after checking
self.instant_send_locks.insert(*txid) prevents mark_utxos_instant_send from
running for duplicate txids and can miss later InstantSend transitions; change
the logic in the method containing the insert so that you always call
mark_utxos_instant_send(txid) (or the equivalent method) regardless of whether
insert returned false, and only suppress duplicate-specific actions (like
emitting duplicate-only events) based on the insert result—i.e., use the boolean
from self.instant_send_locks.insert(*txid) to conditionally skip duplicate-side
effects but do not return early before calling mark_utxos_instant_send.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f17e26ed-5fd7-490a-af79-d6fe2bab356c
📒 Files selected for processing (3)
key-wallet-manager/src/test_utils/wallet.rskey-wallet-manager/src/wallet_manager/event_tests.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
xdustinface
added a commit
that referenced
this pull request
Mar 19, 2026
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
xdustinface
added a commit
that referenced
this pull request
Mar 19, 2026
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
52b07ae to
837c8b7
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv-ffi/src/callbacks.rs (2)
523-533:⚠️ Potential issue | 🟠 MajorABI breaking change:
statusparameter inserted mid-signature.The callback signature now has
statusas the second parameter, but existing C/Swift consumers compiled against the previous signature expectaccount_indexin that position. This causes argument misalignment and undefined behavior for pre-compiled binaries.To preserve backward compatibility:
- Option A: Append
statusas the last parameter beforeuser_data- Option B: Introduce
OnTransactionReceivedCallbackV2with the new signature while keeping the original intact🛡️ Option A: Append status before user_data
pub type OnTransactionReceivedCallback = Option< extern "C" fn( wallet_id: *const c_char, - status: FFITransactionContext, account_index: u32, txid: *const [u8; 32], amount: i64, addresses: *const c_char, + status: FFITransactionContext, user_data: *mut c_void, ), >;🤖 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 523 - 533, The callback signature was changed in-place causing an ABI break: restore backward compatibility by keeping the original OnTransactionReceivedCallback (with wallet_id, account_index, txid, amount, addresses, user_data) and implement one of two fixes—either append the new status parameter just before user_data (so original parameter order remains for existing consumers) or introduce a new typedef OnTransactionReceivedCallbackV2 that includes the new status parameter in its signature while leaving the existing OnTransactionReceivedCallback untouched; update internal call sites to use the V2 symbol where appropriate and ensure any registration/dispatch paths accept both callback types.
571-578:⚠️ Potential issue | 🟠 MajorStruct layout break: new field shifts
on_balance_updatedanduser_dataoffsets.Inserting
on_transaction_status_changedbeforeon_balance_updatedchanges the memory layout. Pre-compiled C/Swift code accessingon_balance_updatedoruser_datawill read incorrect memory locations.Append the new field just before
user_datato maintain backward-compatible offsets for existing fields:🛡️ Proposed layout fix
pub struct FFIWalletEventCallbacks { pub on_transaction_received: OnTransactionReceivedCallback, - pub on_transaction_status_changed: OnTransactionStatusChangedCallback, pub on_balance_updated: OnBalanceUpdatedCallback, + pub on_transaction_status_changed: OnTransactionStatusChangedCallback, pub user_data: *mut c_void, }Update the
Defaultimpl accordingly.🤖 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 571 - 578, The new field on_transaction_status_changed was inserted before on_balance_updated, altering the struct memory layout of FFIWalletEventCallbacks; move on_transaction_status_changed so it appears immediately before user_data (i.e., after on_balance_updated) to preserve existing offsets, and update the Default implementation for FFIWalletEventCallbacks to initialize the fields in the new order (including a sensible default for on_transaction_status_changed) so the struct layout and defaults remain backward-compatible for C/Swift callers.
♻️ Duplicate comments (2)
key-wallet-manager/src/wallet_manager/test_helpers.rs (2)
14-22:⚠️ Potential issue | 🟡 MinorParameterize test setup by network instead of hardcoding Testnet.
Line 15 hardcodes
Network::Testnet, so all callers are locked to one network path and can’t easily run the same lifecycle assertions on mainnet.Suggested fix
-pub(crate) fn setup_manager_with_wallet() -> (WalletManager<ManagedWalletInfo>, WalletId, Address) { - let mut manager = WalletManager::new(Network::Testnet); +pub(crate) fn setup_manager_with_wallet( + network: Network, +) -> (WalletManager<ManagedWalletInfo>, WalletId, Address) { + let mut manager = WalletManager::new(network); let wallet_id = manager .create_wallet_from_mnemonic(TEST_MNEMONIC, "", 0, WalletAccountCreationOptions::Default) .unwrap(); @@ let addr = addresses[0].clone(); (manager, wallet_id, addr) } + +pub(crate) fn setup_manager_with_wallet_testnet() -> (WalletManager<ManagedWalletInfo>, WalletId, Address) { + setup_manager_with_wallet(Network::Testnet) +}As per coding guidelines, "Test both mainnet and testnet configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_manager/test_helpers.rs` around lines 14 - 22, The helper setup_manager_with_wallet hardcodes Network::Testnet when constructing WalletManager (WalletManager::new(Network::Testnet)); change the function to accept a Network parameter (e.g., setup_manager_with_wallet(network: Network) -> (...)) and pass that into WalletManager::new so callers can invoke it for Mainnet or Testnet; update any callers/tests to pass Network::Testnet or Network::Mainnet accordingly and keep the same wallet creation flow (create_wallet_from_mnemonic, monitored_addresses) and return types (WalletManager<ManagedWalletInfo>, WalletId, Address).
46-50:⚠️ Potential issue | 🟡 MinorHandle
TryRecvError::Lagged(_)explicitly indrain_events.Current logic stops on any error, so
Lagged(_)is treated like “no more events,” which can hide dropped events and make assertions falsely pass.Suggested fix
pub(crate) fn drain_events(rx: &mut broadcast::Receiver<WalletEvent>) -> Vec<WalletEvent> { let mut events = Vec::new(); - while let Ok(e) = rx.try_recv() { - events.push(e); - } + loop { + match rx.try_recv() { + Ok(event) => events.push(event), + Err(broadcast::error::TryRecvError::Empty) => break, + Err(broadcast::error::TryRecvError::Closed) => break, + Err(broadcast::error::TryRecvError::Lagged(skipped)) => { + panic!("event receiver lagged and skipped {skipped} wallet events"); + } + } + } events }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_manager/test_helpers.rs` around lines 46 - 50, The current drain_events function treats any try_recv error as "no more events" and hides dropped messages; change the loop to match on rx.try_recv(): on Ok(e) push to events; on Err(broadcast::error::TryRecvError::Empty) break the loop; on Err(broadcast::error::TryRecvError::Lagged(skipped)) explicitly fail (panic or assert) with a message including skipped so tests surface dropped events instead of silently passing; reference the drain_events function, broadcast::Receiver, TryRecvError::Lagged and WalletEvent when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 523-533: The callback signature was changed in-place causing an
ABI break: restore backward compatibility by keeping the original
OnTransactionReceivedCallback (with wallet_id, account_index, txid, amount,
addresses, user_data) and implement one of two fixes—either append the new
status parameter just before user_data (so original parameter order remains for
existing consumers) or introduce a new typedef OnTransactionReceivedCallbackV2
that includes the new status parameter in its signature while leaving the
existing OnTransactionReceivedCallback untouched; update internal call sites to
use the V2 symbol where appropriate and ensure any registration/dispatch paths
accept both callback types.
- Around line 571-578: The new field on_transaction_status_changed was inserted
before on_balance_updated, altering the struct memory layout of
FFIWalletEventCallbacks; move on_transaction_status_changed so it appears
immediately before user_data (i.e., after on_balance_updated) to preserve
existing offsets, and update the Default implementation for
FFIWalletEventCallbacks to initialize the fields in the new order (including a
sensible default for on_transaction_status_changed) so the struct layout and
defaults remain backward-compatible for C/Swift callers.
---
Duplicate comments:
In `@key-wallet-manager/src/wallet_manager/test_helpers.rs`:
- Around line 14-22: The helper setup_manager_with_wallet hardcodes
Network::Testnet when constructing WalletManager
(WalletManager::new(Network::Testnet)); change the function to accept a Network
parameter (e.g., setup_manager_with_wallet(network: Network) -> (...)) and pass
that into WalletManager::new so callers can invoke it for Mainnet or Testnet;
update any callers/tests to pass Network::Testnet or Network::Mainnet
accordingly and keep the same wallet creation flow (create_wallet_from_mnemonic,
monitored_addresses) and return types (WalletManager<ManagedWalletInfo>,
WalletId, Address).
- Around line 46-50: The current drain_events function treats any try_recv error
as "no more events" and hides dropped messages; change the loop to match on
rx.try_recv(): on Ok(e) push to events; on
Err(broadcast::error::TryRecvError::Empty) break the loop; on
Err(broadcast::error::TryRecvError::Lagged(skipped)) explicitly fail (panic or
assert) with a message including skipped so tests surface dropped events instead
of silently passing; reference the drain_events function, broadcast::Receiver,
TryRecvError::Lagged and WalletEvent when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df333cc6-e462-4344-a2a2-3369928af9f4
📒 Files selected for processing (15)
dash-spv-ffi/include/dash_spv_ffi.hdash-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.rskey-wallet-manager/src/events.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/src/wallet_manager/event_tests.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rskey-wallet-manager/src/wallet_manager/test_helpers.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
✅ Files skipped from review due to trivial changes (2)
- dash-spv-ffi/tests/dashd_sync/tests_callback.rs
- dash-spv-ffi/tests/dashd_sync/callbacks.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- key-wallet-manager/src/wallet_interface.rs
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- key-wallet/src/transaction_checking/account_checker.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-manager/src/test_utils/wallet.rs
- key-wallet-manager/src/wallet_manager/process_block.rs
- key-wallet-manager/src/events.rs
- key-wallet-manager/src/wallet_manager/mod.rs
- key-wallet-manager/src/wallet_manager/event_tests.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet-manager/src/wallet_manager/event_tests.rs (1)
266-268: Remove redundant import.
dashcore::hashes::Hashis already imported at the module level (line 4). The local import is unnecessary.♻️ Suggested fix
async fn test_process_block_emits_events() { use dashcore::blockdata::block::{Block, Header, Version}; - use dashcore::hashes::Hash; use dashcore::{BlockHash, CompactTarget, TxMerkleNode};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_manager/event_tests.rs` around lines 266 - 268, Remove the redundant local import of dashcore::hashes::Hash in the import block that contains Block, Header, Version and BlockHash/CompactTarget/TxMerkleNode; the symbol is already imported at module scope so simply delete the `use dashcore::hashes::Hash;` line (leave the other imports and identifiers such as Block, Header, Version, BlockHash, CompactTarget, and TxMerkleNode unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet-manager/src/wallet_manager/event_tests.rs`:
- Around line 266-268: Remove the redundant local import of
dashcore::hashes::Hash in the import block that contains Block, Header, Version
and BlockHash/CompactTarget/TxMerkleNode; the symbol is already imported at
module scope so simply delete the `use dashcore::hashes::Hash;` line (leave the
other imports and identifiers such as Block, Header, Version, BlockHash,
CompactTarget, and TxMerkleNode unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 048166df-8544-4a75-93ac-5bb2485d64c7
📒 Files selected for processing (2)
key-wallet-manager/src/wallet_manager/event_tests.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
✅ Files skipped from review due to trivial changes (1)
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
xdustinface
added a commit
that referenced
this pull request
Mar 19, 2026
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
xdustinface
added a commit
that referenced
this pull request
Mar 19, 2026
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
baff83f to
92cf7db
Compare
Contributor
There was a problem hiding this comment.
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)
523-533:⚠️ Potential issue | 🟠 MajorNote: This is the source of the ABI break flagged in the header file.
The
statusparameter insertion at position 2 (afterwallet_id) causes the ABI compatibility issue. To preserve backward compatibility, consider movingstatusto be the last parameter beforeuser_data, or introduce a versioned callback type.🤖 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 523 - 533, The callback type OnTransactionReceivedCallback currently inserts the new status parameter as the second argument, breaking the ABI; update the signature so FFITransactionContext status is moved to be the last parameter before user_data (i.e., after addresses and amount) or alternatively introduce a new versioned type (e.g., OnTransactionReceivedCallbackV2) preserving the original OnTransactionReceivedCallback layout; ensure all references to OnTransactionReceivedCallback and any registrations/extern "C" callers are updated to the chosen approach to maintain backward compatibility.
♻️ Duplicate comments (1)
dash-spv-ffi/include/dash_spv_ffi.h (1)
377-394:⚠️ Potential issue | 🟠 MajorABI-breaking changes in callback signatures and struct layout.
The
OnTransactionReceivedCallbacksignature was modified by insertingFFITransactionContext statusas the second parameter, andFFIWalletEventCallbackslayout changed by insertingon_transaction_status_changedmid-struct. Pre-compiled C/Swift clients will experience undefined behavior when these callbacks are invoked with misaligned arguments.As noted in the PR objectives, preserve backward compatibility by either:
- Appending
statusas the last parameter beforeuser_data- Introducing versioned callbacks (
OnTransactionReceivedCallbackV2,FFIWalletEventCallbacksV2)Since this header is auto-generated by cbindgen, the fix must be applied to the Rust source in
dash-spv-ffi/src/callbacks.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/include/dash_spv_ffi.h` around lines 377 - 394, The header changed the ABI by inserting FFITransactionContext into the middle of OnTransactionReceivedCallback and by inserting on_transaction_status_changed into FFIWalletEventCallbacks; update the Rust source (dash-spv-ffi/src/callbacks.rs) to preserve ABI compatibility by either adding a new versioned type (e.g., define OnTransactionReceivedCallbackV2 and FFIWalletEventCallbacksV2) or by moving the new FFITransactionContext parameter to the end of the existing OnTransactionReceivedCallback before user_data so the original function pointer layout is unchanged; ensure the generated cbindgen output keeps the original OnTransactionReceivedCallback and FFIWalletEventCallbacks definitions intact (and emits V2 variants if you choose versioning), then re-run cbindgen to regenerate dash_spv_ffi.h.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 523-533: The callback type OnTransactionReceivedCallback currently
inserts the new status parameter as the second argument, breaking the ABI;
update the signature so FFITransactionContext status is moved to be the last
parameter before user_data (i.e., after addresses and amount) or alternatively
introduce a new versioned type (e.g., OnTransactionReceivedCallbackV2)
preserving the original OnTransactionReceivedCallback layout; ensure all
references to OnTransactionReceivedCallback and any registrations/extern "C"
callers are updated to the chosen approach to maintain backward compatibility.
---
Duplicate comments:
In `@dash-spv-ffi/include/dash_spv_ffi.h`:
- Around line 377-394: The header changed the ABI by inserting
FFITransactionContext into the middle of OnTransactionReceivedCallback and by
inserting on_transaction_status_changed into FFIWalletEventCallbacks; update the
Rust source (dash-spv-ffi/src/callbacks.rs) to preserve ABI compatibility by
either adding a new versioned type (e.g., define OnTransactionReceivedCallbackV2
and FFIWalletEventCallbacksV2) or by moving the new FFITransactionContext
parameter to the end of the existing OnTransactionReceivedCallback before
user_data so the original function pointer layout is unchanged; ensure the
generated cbindgen output keeps the original OnTransactionReceivedCallback and
FFIWalletEventCallbacks definitions intact (and emits V2 variants if you choose
versioning), then re-run cbindgen to regenerate dash_spv_ffi.h.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4edde5a-128e-4b81-8839-1a8dbc76d8db
📒 Files selected for processing (15)
dash-spv-ffi/include/dash_spv_ffi.hdash-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.rskey-wallet-manager/src/events.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/src/wallet_manager/event_tests.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rskey-wallet-manager/src/wallet_manager/test_helpers.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
✅ Files skipped from review due to trivial changes (3)
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- key-wallet-manager/src/events.rs
- key-wallet-manager/src/wallet_manager/event_tests.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- key-wallet-manager/src/wallet_interface.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet/src/transaction_checking/account_checker.rs
- key-wallet-manager/src/test_utils/wallet.rs
- dash-spv-ffi/tests/dashd_sync/tests_callback.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
xdustinface
added a commit
that referenced
this pull request
Mar 19, 2026
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
xdustinface
added a commit
that referenced
this pull request
Mar 19, 2026
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
92cf7db to
39d06fb
Compare
github-actions
bot
added
ready-for-review
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
added a commit
that referenced
this pull request
Mar 20, 2026
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
xdustinface
added a commit
that referenced
this pull request
Mar 20, 2026
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
39d06fb to
3063b15
Compare
github-actions
bot
added
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
added a commit
that referenced
this pull request
Mar 20, 2026
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
xdustinface
added a commit
that referenced
this pull request
Mar 20, 2026
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
- Add `TransactionStatusChanged` variant to `WalletEvent` and a `status` field to `TransactionReceived`. - Wire up event emission in `check_transaction_in_all_wallets`, `process_instant_send_lock`, and `update_synced_height`. - Add FFI callback for status changes.
…send_lock` Addresses CodeRabbit review comment on PR #552 File: key-wallet-manager/src/test_utils/wallet.rs
`process_instant_send_lock` did not record txids in `instant_send_locks`, so a subsequent `check_transaction_in_all_wallets(..., InstantSend)` for the same txid would emit a duplicate `TransactionStatusChanged` event. Record the txid in `instant_send_locks` inside `mark_instant_send_utxos` so both paths share the same deduplication set. Add tests covering both orderings of the mixed entry points. Addresses CodeRabbit review comment on PR #552 Files: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_manager/event_tests.rs
xdustinface
force-pushed
the
feat/transaction-status-events
branch
from
3063b15 to
7cf7b6c
Compare
github-actions
bot
removed
the
merge-conflict
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TransactionStatusChangedvariant toWalletEventand astatusfield toTransactionReceived.check_transaction_in_all_wallets,process_instant_send_lock, andupdate_synced_height.Based on:
Summary by CodeRabbit
New Features
Tests
Bug Fixes