refactor: cleanup WalletTransactionChecker::check_transaction
#402
Conversation
📝 WalkthroughWalkthroughRefactors account lookup into a macro-backed accessor and adds a mutable getter. Adds per-account transaction recording and UTXO update helpers. Reworks WalletTransactionChecker to accept TransactionContext, delegate per-account recording, add TransactionContext helpers/Display, and consolidate transaction logging and gap-limit handling. Changes
Sequence DiagramsequenceDiagram
participant Checker as "WalletTransactionChecker" rect rgba(60,120,200,0.5)
participant Account as "ManagedAccount" rect rgba(80,200,120,0.5)
participant UTXO as "UTXO Store" rect rgba(200,160,60,0.5)
Checker->>Checker: check_transaction(tx, context, wallet, update_state)
alt update_state && tx relevant
Checker->>Checker: identify affected accounts (AccountMatch)
loop per affected account
Checker->>Checker: get_by_account_type_match_mut(account_type_match)
Checker->>Account: record_transaction(tx, account_match, context)
Account->>Account: create TransactionRecord
Account->>UTXO: update_utxos(tx, account_match, context)
Account->>Checker: mark addresses used / update pools
end
Checker->>Checker: compute net_change & log final summary
Checker->>Checker: increment wallet.metadata.total_transactions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 23-105: The PlatformPayment branch in the
get_by_account_type_match_impl! macro currently returns None and must instead
build a PlatformPaymentAccountKey from AccountTypeMatch::PlatformPayment's
account_index and key_class and use the same accessor to look it up; update the
AccountTypeMatch::PlatformPayment arm to construct PlatformPaymentAccountKey {
account: *account_index, key_class: *key_class } and call
$self.platform_payment_accounts.$get(&key) (mirroring the Dashpay arms) so
platform_payment_accounts are properly returned by
get_by_account_type_match_impl!.
🧹 Nitpick comments (2)
key-wallet/src/transaction_checking/wallet_checker.rs (2)
162-197: Per-account processing looks correct, but silently skipping gap limit maintenance warrants logging.The flow properly:
- Records the transaction and updates UTXOs via
account.record_transaction- Marks involved addresses as used
- Maintains gap limits for address pools
However, when
extended_public_key_for_account_typereturnsNone(lines 176-181), the code silently continues without maintaining gap limits. This could leave address pools in an inconsistent state for watch-only wallets or accounts without xpub access.Consider logging when gap limit maintenance is skipped
let Some(xpub) = wallet.extended_public_key_for_account_type( &account_match.account_type_match.to_account_type_to_check(), account_match.account_type_match.account_index(), ) else { + tracing::debug!( + account_index = ?account_match.account_type_match.account_index(), + "Skipping gap limit maintenance: no xpub available for account" + ); continue; };
163-163: Clone ofaffected_accountsis necessary but could be documented.The
.clone()is required becauseresult.new_addressesis mutated during iteration (line 186). This is correct but not immediately obvious.Optional: Add a brief comment explaining the clone
- for account_match in result.affected_accounts.clone() { + // Clone required: we modify result.new_addresses while iterating + for account_match in result.affected_accounts.clone() {
xdustinface
force-pushed
the
refactor/check-transaction
branch
from
1b3de48 to
a36ee4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 74-75: Update the doc comment for the wallet_checker::block_hash
method: it currently reads "Get the block height if confirmed" but the function
returns an Option<BlockHash>, so change the comment to accurately describe that
it returns the block hash if the transaction is confirmed (e.g., "Get the block
hash if confirmed" or similar). Ensure the corrected doc refers to the block
hash and the Option return semantics for the block_hash method.
🧹 Nitpick comments (2)
key-wallet/src/transaction_checking/wallet_checker.rs (2)
162-197: Consider documenting partial-update behavior for multi-account transactions.The per-account processing loop can partially succeed: if
maintain_gap_limitfails for one account, other accounts have already been updated viarecord_transaction. This isn't necessarily wrong, but the non-atomic behavior across multiple affected accounts should be documented or considered.The error logging on lines 188-193 is appropriate for observability, but callers may not be aware that a transaction could be recorded in some accounts but not have proper gap-limit maintenance in others.
💡 Optional: Add a comment documenting the behavior
// Process each affected account + // Note: Processing is best-effort per account. If gap-limit maintenance fails + // for one account, other accounts will still have been updated. for account_match in result.affected_accounts.clone() {
176-181: Silent skip when extended public key is unavailable.When
extended_public_key_for_account_typereturnsNone, the loop continues without logging. This could mask configuration issues for accounts that should have an xpub. Consider adding a debug/trace log for observability.📋 Optional: Add trace logging
let Some(xpub) = wallet.extended_public_key_for_account_type( &account_match.account_type_match.to_account_type_to_check(), account_match.account_type_match.account_index(), ) else { + tracing::trace!( + account_type = ?account_match.account_type_match, + "Skipping gap limit maintenance - no extended public key available" + ); continue; };
- Moves parts of it out into separate functions/helpers - Combines the gap limit maintenance cases since the `ManagedAccountType::Standard` is also already handled in `ManagedAccountType::address_pools_mut` - Early return in case of an already known transaction, adding the record again and updating UTXOs is no-op if the transaction is known already.
xdustinface
force-pushed
the
refactor/check-transaction
branch
from
a36ee4e to
330a8ea
Compare
ManagedAccountType::Standardis also already handled inManagedAccountType::address_pools_mutSummary by CodeRabbit
New Features
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.