refactor(wallet-events): atomic events carrying records + balance#680
Open
QuantumExplorer wants to merge 3 commits intov0.42-devfrom
Open
refactor(wallet-events): atomic events carrying records + balance#680QuantumExplorer wants to merge 3 commits intov0.42-devfrom
QuantumExplorer wants to merge 3 commits intov0.42-devfrom
Conversation
Replace `TransactionReceived` / `TransactionStatusChanged` / `BalanceUpdated` with three self-contained variants — `MempoolTransactionReceived`, `TransactionInstantSendLocked`, `BlockProcessChange` — each carrying the transaction record(s) it pertains to plus the wallet's post-event balance. Consumers can now persist transactions and balance atomically off a single event. - `BlockProcessChange` bundles all records updated by a block into a single per-wallet event; an empty `transactions_updated` signals a balance-only drift (e.g. coinbase maturing as synced height advanced). - `TransactionCheckResult` now exposes `affected_records` (populated for both new records and state-modifying confirmations / IS-locks). - `check_transaction_in_all_wallets` routes events by context; a silent variant lets `process_block` batch per-wallet records into one event. - FFI surface updated (three new callbacks on `FFIWalletEventCallbacks` carrying `FFIBalance*` and record arrays); `ffi_cli` and dashd_sync tests migrated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe pull request refactors the wallet event system to embed Changes
Sequence DiagramsequenceDiagram
participant User as Transaction<br/>Lifecycle
participant Manager as WalletManager
participant Checker as TransactionChecker
participant Events as Event Emitter
User->>Manager: process_mempool_transaction(record)
activate Manager
Manager->>Checker: check_transaction_in_all_wallets_silent(record)
activate Checker
Checker->>Checker: affected_records = [record]
Checker-->>Manager: CheckTransactionsResult { records_per_wallet }
deactivate Checker
Manager->>Events: emit_context_routed_events<br/>(Mempool context)
activate Events
Events-->>Manager: WalletEvent::MempoolTransactionReceived<br/>{ wallet_id, record, balance }
deactivate Events
deactivate Manager
User->>Manager: process_instant_send_lock(islock)
activate Manager
Manager->>Checker: update existing record
Checker->>Checker: affected_records = [updated_record]
Manager->>Events: emit TransactionInstantSendLocked<br/>{ wallet_id, txid, instant_send_lock, balance }
deactivate Manager
User->>Manager: process_block(height, txs)
activate Manager
Manager->>Checker: check_transaction_in_all_wallets_silent(txs)
Checker->>Checker: affected_records = [confirmed_records...]
Manager->>Events: emit_context_routed_events<br/>(InBlock context)
activate Events
Events-->>Manager: WalletEvent::BlockProcessChange<br/>{ wallet_id, height, transactions_updated, balance }
deactivate Events
deactivate Manager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #680 +/- ##
=============================================
- Coverage 70.40% 70.18% -0.22%
=============================================
Files 247 319 +72
Lines 48029 66802 +18773
=============================================
+ Hits 33816 46888 +13072
- Misses 14213 19914 +5701
|
- Run cargo fmt across touched files. - Replace `[check_transaction_in_all_wallets_silent]` intra-doc link with a plain backtick reference since the function is `pub(crate)` and can't be resolved from a `pub` doc under `-D rustdoc::broken-intra-doc-links`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv/tests/dashd_sync/helpers.rs (1)
129-143:⚠️ Potential issue | 🟠 MajorAccept IS-backed mempool arrivals here.
MempoolTransactionReceivednow also covers transactions first seen with anInstantSend(..)context. Keeping therecord.context == TransactionContext::Mempoolguard means this helper will ignore a valid mempool event and eventually time out, which makes the dashd sync tests race-sensitive.Minimal fix
- Ok(WalletEvent::MempoolTransactionReceived { ref record, .. }) if record.context == TransactionContext::Mempool => return Some(record.txid), + Ok(WalletEvent::MempoolTransactionReceived { ref record, .. }) => return Some(record.txid),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/dashd_sync/helpers.rs` around lines 129 - 143, The helper wait_for_mempool_tx currently only accepts WalletEvent::MempoolTransactionReceived when record.context == TransactionContext::Mempool, which ignores valid mempool arrivals that are first seen as InstantSend; update the match arm for WalletEvent::MempoolTransactionReceived in wait_for_mempool_tx to also accept InstantSend contexts (e.g., change the guard to allow record.context == TransactionContext::Mempool || matches!(record.context, TransactionContext::InstantSend(_)) or pattern-match the InstantSend variant) so the function returns Some(record.txid) for both mempool and InstantSend arrivals.key-wallet-manager/src/lib.rs (1)
502-583:⚠️ Potential issue | 🟡 MinorEmitted balance can be stale when the caller passes
update_balance=false.At line 562, the balance carried by the emitted event is whatever
info.balance()returns aftercheck_core_transaction(..., update_balance)— but when the caller invokescheck_transaction_in_all_wallets(..., update_state_if_found=true, update_balance=false), state is modified while the cached balance is not refreshed (the internalprocess_mempool_transaction/process_blockflows demonstrate this cached-balance behavior atprocess_block.rslines 102-106). That means consumers of the emitted event would see the pre-state-change balance, breaking the "event carries post-event balance" invariant.Either document this precondition on
check_transaction_in_all_wallets(e.g. "passupdate_balance=trueto ensure emitted events carry the post-change balance") or internally force a refresh before reading balance whenemit_events && state_modified.🔧 Option A: always refresh before reading balance in the emit path
if emit_events && check_result.state_modified { - // Re-read balance after state change - let balance = self - .wallet_infos - .get(&wallet_id) - .map(|info| info.balance()) - .unwrap_or_default(); + // Re-read balance after state change — force a refresh + // so the event always carries the post-event balance + // regardless of the caller's `update_balance` flag. + let balance = self + .wallet_infos + .get_mut(&wallet_id) + .map(|info| { + info.update_balance(); + info.balance() + }) + .unwrap_or_default(); self.emit_context_routed_events(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/lib.rs` around lines 502 - 583, The emitted event can contain a stale balance when check_transaction_in_all_wallets_inner calls emit_context_routed_events after check_core_transaction was run with update_balance=false; fix by ensuring the cached WalletInfo balance is refreshed before reading it for the event: inside check_transaction_in_all_wallets_inner (the branch where emit_events && check_result.state_modified) call a WalletInfo refresh method to recompute the balance from the updated state (e.g. implement and call a WalletInfo::refresh_balance_from_state or similar) or otherwise force-update the cache for wallet_infos.get(&wallet_id) prior to calling .balance(), then pass that fresh balance into emit_context_routed_events (alternatively document the function to require update_balance=true when emit_events is requested if you prefer not to add a refresh).
🧹 Nitpick comments (2)
key-wallet-manager/src/lib.rs (1)
594-649: LGTM — context routing mirrors the documented state machine exactly.The four documented routes (mempool new →
MempoolTransactionReceived; IS-lock new →MempoolTransactionReceivedwith IS context; IS-lock existing →TransactionInstantSendLocked; in-block →BlockProcessChange) map 1:1 to the match arms here, and theInBlock | InChainLockedBlockarm correctly carries a singleBlockProcessChangefor direct callers whileprocess_block.rsuses the silent batched path for the bundled-per-wallet case.One subtle observation worth leaving in a code comment: the
TransactionInstantSendLockedarm consumes (moves)recordsbut doesn't use them — intentional since the event variant doesn't carry records, but a one-line note would prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/lib.rs` around lines 594 - 649, The TransactionInstantSendLocked match arm in emit_context_routed_events intentionally consumes the parameter records but does not use it; add a one-line code comment inside that arm clarifying that records is moved (consumed) intentionally because the TransactionInstantSendLocked event does not carry per-record data, to prevent future confusion (reference: fn emit_context_routed_events, param records, match arm creating WalletEvent::TransactionInstantSendLocked).key-wallet-manager/src/process_block.rs (1)
77-137: Consolidate balance refresh into the silent checker by settingupdate_balance: true.The manual refresh loop at lines 102–106 is redundant. The
check_core_transactionmethod (when called withupdate_balance=true) updates balance for each affected wallet inside its own control flow, scoped to wallets whereis_relevant=true. Passingupdate_balance=trueto line 93 would eliminate the need for lines 102–106, consolidating the balance invariant into a single code path.♻️ Suggested simplification
- // Refresh cached balances only for affected wallets *before* emitting - // so the event carries the post-update balance. let check_result = - self.check_transaction_in_all_wallets_silent(tx, context.clone(), true, false).await; + self.check_transaction_in_all_wallets_silent(tx, context.clone(), true, true).await; let is_relevant = !check_result.affected_wallets.is_empty(); let net_amount = if is_relevant { check_result.total_received as i64 - check_result.total_sent as i64 } else { 0 }; - for wallet_id in &check_result.affected_wallets { - if let Some(info) = self.wallet_infos.get_mut(wallet_id) { - info.update_balance(); - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/process_block.rs` around lines 77 - 137, The manual per-wallet balance refresh loop in process_mempool_transaction is redundant—pass update_balance=true into check_transaction_in_all_wallets_silent (ensure the call to check_transaction_in_all_wallets_silent(tx, context.clone(), true, false) has the update flag set) so balances are refreshed inside the checker, then remove the loop that iterates check_result.affected_wallets calling self.wallet_infos.get_mut(...).update_balance(); keep the subsequent event emission that reads balances from self.wallet_infos (e.g., emit_context_routed_events) unchanged so they observe the refreshed balances.
🤖 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/test_helpers.rs`:
- Around line 94-129: The InstantSend branch currently trusts whatever
InstantLock the test caller provided, allowing default/mismatched locks to pass;
update the InstantSend handling in the test helper
(TransactionContext::InstantSend) to verify the lock belongs to the transaction
by either asserting the emitted WalletEvent::TransactionInstantSendLocked
contains an InstantLock with txid equal to the transaction's txid() or ensure
the submitted context uses dummy_instant_lock(tx.txid()) so the helper can match
on that txid; specifically add a check against tx.txid() when matching
TransactionInstantSendLocked (and when matching MempoolTransactionReceived for
the InstantSend case if the event carries the lock) to guarantee the lock
payload is tied to the correct transaction.
In `@key-wallet/src/transaction_checking/account_checker.rs`:
- Around line 49-55: The emitted affected_records field loses account identity
and per-account insert/update status; replace Vec<TransactionRecord> with a
typed per-account payload (e.g., define a new AffectedRecord struct containing
account_index (or AccountId), account_type, record: TransactionRecord, and
is_new_transaction) and change the field in the AccountChecker result from
affected_records: Vec<TransactionRecord> to affected_records:
Vec<AffectedRecord>; update the code paths that build affected_records inside
account_checker.rs to populate account_index/account_type and per-record
is_new_transaction, and update all consumers of affected_records to read the new
struct fields instead of assuming a single global is_new_transaction or account
context.
---
Outside diff comments:
In `@dash-spv/tests/dashd_sync/helpers.rs`:
- Around line 129-143: The helper wait_for_mempool_tx currently only accepts
WalletEvent::MempoolTransactionReceived when record.context ==
TransactionContext::Mempool, which ignores valid mempool arrivals that are first
seen as InstantSend; update the match arm for
WalletEvent::MempoolTransactionReceived in wait_for_mempool_tx to also accept
InstantSend contexts (e.g., change the guard to allow record.context ==
TransactionContext::Mempool || matches!(record.context,
TransactionContext::InstantSend(_)) or pattern-match the InstantSend variant) so
the function returns Some(record.txid) for both mempool and InstantSend
arrivals.
In `@key-wallet-manager/src/lib.rs`:
- Around line 502-583: The emitted event can contain a stale balance when
check_transaction_in_all_wallets_inner calls emit_context_routed_events after
check_core_transaction was run with update_balance=false; fix by ensuring the
cached WalletInfo balance is refreshed before reading it for the event: inside
check_transaction_in_all_wallets_inner (the branch where emit_events &&
check_result.state_modified) call a WalletInfo refresh method to recompute the
balance from the updated state (e.g. implement and call a
WalletInfo::refresh_balance_from_state or similar) or otherwise force-update the
cache for wallet_infos.get(&wallet_id) prior to calling .balance(), then pass
that fresh balance into emit_context_routed_events (alternatively document the
function to require update_balance=true when emit_events is requested if you
prefer not to add a refresh).
---
Nitpick comments:
In `@key-wallet-manager/src/lib.rs`:
- Around line 594-649: The TransactionInstantSendLocked match arm in
emit_context_routed_events intentionally consumes the parameter records but does
not use it; add a one-line code comment inside that arm clarifying that records
is moved (consumed) intentionally because the TransactionInstantSendLocked event
does not carry per-record data, to prevent future confusion (reference: fn
emit_context_routed_events, param records, match arm creating
WalletEvent::TransactionInstantSendLocked).
In `@key-wallet-manager/src/process_block.rs`:
- Around line 77-137: The manual per-wallet balance refresh loop in
process_mempool_transaction is redundant—pass update_balance=true into
check_transaction_in_all_wallets_silent (ensure the call to
check_transaction_in_all_wallets_silent(tx, context.clone(), true, false) has
the update flag set) so balances are refreshed inside the checker, then remove
the loop that iterates check_result.affected_wallets calling
self.wallet_infos.get_mut(...).update_balance(); keep the subsequent event
emission that reads balances from self.wallet_infos (e.g.,
emit_context_routed_events) unchanged so they observe the refreshed balances.
🪄 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: 42647109-3014-4d88-99eb-8ec2a9e50564
📒 Files selected for processing (14)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv/ARCHITECTURE.mddash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/tests_mempool.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rs
💤 Files with no reviewable changes (1)
- key-wallet-manager/src/accessors.rs
Comment on lines
+94
to
+129
| match ctx { | ||
| TransactionContext::Mempool => { | ||
| assert!( | ||
| matches!(&event, WalletEvent::MempoolTransactionReceived { wallet_id: wid, record, .. } if *wid == wallet_id && record.context == *ctx), | ||
| "context[{}]: expected MempoolTransactionReceived(Mempool), got {:?}", | ||
| i, | ||
| event | ||
| ); | ||
| } | ||
| TransactionContext::InstantSend(_) => { | ||
| if i == 0 { | ||
| assert!( | ||
| matches!(&event, WalletEvent::MempoolTransactionReceived { wallet_id: wid, record, .. } if *wid == wallet_id && record.context == *ctx), | ||
| "context[{}]: expected MempoolTransactionReceived(InstantSend), got {:?}", | ||
| i, | ||
| event | ||
| ); | ||
| } else { | ||
| assert!( | ||
| matches!(&event, WalletEvent::TransactionInstantSendLocked { wallet_id: wid, .. } if *wid == wallet_id), | ||
| "context[{}]: expected TransactionInstantSendLocked, got {:?}", | ||
| i, | ||
| event | ||
| ); | ||
| } | ||
| } | ||
| TransactionContext::InBlock(info) | TransactionContext::InChainLockedBlock(info) => { | ||
| let expected_height = info.height(); | ||
| assert!( | ||
| matches!(&event, WalletEvent::BlockProcessChange { wallet_id: wid, height, transactions_updated, .. } if *wid == wallet_id && *height == expected_height && transactions_updated.len() == 1), | ||
| "context[{}]: expected BlockProcessChange(height={}, 1 tx), got {:?}", | ||
| i, | ||
| expected_height, | ||
| event | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Assert that the InstantLock actually belongs to this tx.
This helper currently treats whatever InstantSend(lock) the caller passed as the expected value. Since several call sites use InstantLock::default(), these lifecycle tests never prove that the stored/emitted lock is tied to tx.txid(). Add an explicit txid check here, or build the submitted context with dummy_instant_lock(tx.txid()), so a mismatched lock payload can’t slip through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/test_helpers.rs` around lines 94 - 129, The
InstantSend branch currently trusts whatever InstantLock the test caller
provided, allowing default/mismatched locks to pass; update the InstantSend
handling in the test helper (TransactionContext::InstantSend) to verify the lock
belongs to the transaction by either asserting the emitted
WalletEvent::TransactionInstantSendLocked contains an InstantLock with txid
equal to the transaction's txid() or ensure the submitted context uses
dummy_instant_lock(tx.txid()) so the helper can match on that txid; specifically
add a check against tx.txid() when matching TransactionInstantSendLocked (and
when matching MempoolTransactionReceived for the InstantSend case if the event
carries the lock) to guarantee the lock payload is tied to the correct
transaction.
Comment on lines
+49
to
+55
| /// Transaction records recorded or updated by this check. | ||
| /// | ||
| /// Contains one entry per affected account. Entries are present when the | ||
| /// transaction was newly recorded, confirmed in a block, or InstantSend-locked | ||
| /// on top of an existing record. Use `is_new_transaction` to know whether | ||
| /// these records are new or updates to existing ones. | ||
| pub affected_records: Vec<TransactionRecord>, |
Contributor
There was a problem hiding this comment.
Keep account metadata with each emitted record.
TransactionRecord is account-scoped (net_amount, input/output details) but it does not carry account_index or account type. Flattening this to Vec<TransactionRecord> means a wallet event that touches multiple accounts can no longer be mapped back to the correct account, and the single is_new_transaction flag also can’t describe mixed insert/update cases. That makes the new atomic event payload ambiguous for downstream persistence.
Possible shape
+pub struct AffectedRecord {
+ pub account_type_match: CoreAccountTypeMatch,
+ pub record: TransactionRecord,
+ pub is_new: bool,
+}
+
- pub affected_records: Vec<TransactionRecord>,
+ pub affected_records: Vec<AffectedRecord>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction_checking/account_checker.rs` around lines 49 - 55,
The emitted affected_records field loses account identity and per-account
insert/update status; replace Vec<TransactionRecord> with a typed per-account
payload (e.g., define a new AffectedRecord struct containing account_index (or
AccountId), account_type, record: TransactionRecord, and is_new_transaction) and
change the field in the AccountChecker result from affected_records:
Vec<TransactionRecord> to affected_records: Vec<AffectedRecord>; update the code
paths that build affected_records inside account_checker.rs to populate
account_index/account_type and per-record is_new_transaction, and update all
consumers of affected_records to read the new struct fields instead of assuming
a single global is_new_transaction or account context.
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
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.
Summary
TransactionReceived/TransactionStatusChanged/BalanceUpdatedonWalletEventwith three self-contained variants:MempoolTransactionReceived,TransactionInstantSendLocked,BlockProcessChange. Each carries the transaction record(s) involved plus the wallet's post-event balance so consumers can persist transactions + balance atomically off a single event.BlockProcessChangebundles all records updated by a block into a single per-wallet event; an emptytransactions_updatedcarries balance-only drift (e.g. coinbase maturing as synced height advances).FFIWalletEventCallbackseach receiving anFFIBalance*; the block callback also receives anFFITransactionRecordarray + count, and the IS-lock callback receives consensus-serializedInstantLockbytes.Event routing
MempoolTransactionReceived(record.context=Mempool)MempoolTransactionReceived(record.context=InstantSend(..))TransactionInstantSendLockedBlockProcessChange(one per affected wallet, all records bundled)BlockProcessChange(empty records)Implementation notes
TransactionCheckResultnow exposesaffected_records: Vec<TransactionRecord>(populated for both new records and state-modifying confirmations / IS-locks on existing records).check_transaction_in_all_walletsemits context-routed events; a newcheck_transaction_in_all_wallets_silentletsprocess_blockbatch per-wallet records into a single consolidated event.emit_balance_changes/ standaloneBalanceUpdatedemission — balance is now embedded in every event.Test plan
cargo test --workspace— 1925 passed, 0 failed, 47 ignoredcargo clippy -p key-wallet-manager -p dash-spv-ffi --tests --all-features— cleantransaction_received_count/balance_updated_countnow count the new event surface (documented in comments)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes