refactor: cleanup WalletTransactionChecker::check_core_transaction#402
refactor: cleanup WalletTransactionChecker::check_core_transaction#402xdustinface merged 1 commit intov0.42-devfrom
WalletTransactionChecker::check_core_transaction#402Conversation
📝 WalkthroughWalkthroughIntroduces a macro-backed account lookup and a public mutable accessor; adds per-account transaction recording and UTXO update helpers; refactors wallet transaction checking to accept a TransactionContext, delegate per-account recording, add TransactionContext helpers/Display, and consolidate logging and gap-limit handling. Changes
Sequence DiagramsequenceDiagram
participant Checker as "WalletTransactionChecker" rect rgba(60,120,200,0.5)
participant Wallet as "Wallet" rect rgba(120,180,240,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_core_transaction(tx, context, wallet, update_state)
alt update_state && tx relevant
Checker->>Wallet: identify affected_accounts (AccountMatch)
loop per affected account
Checker->>Wallet: get_by_account_type_match_mut(account_type_match)
Wallet->>Account: record_transaction(tx, account_match, context)
Account->>Account: create TransactionRecord
Account->>UTXO: update_utxos(tx, account_match, context)
Account->>Wallet: mark addresses used / suggest new addresses
end
Checker->>Checker: compute net_change & log final summary
Checker->>Wallet: 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)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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; };
xdustinface
force-pushed
the
refactor/check-transaction
branch
from
a36ee4e to
330a8ea
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. |
xdustinface
force-pushed
the
refactor/check-transaction
branch
from
330a8ea to
b1ca36d
Compare
github-actions
bot
removed
the
merge-conflict
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 149-210: The early return when an existing tx is found (checking
txid via tx.txid() and self.accounts.get_by_account_type_match) causes skips of
account-level processing; instead, remove the return: set
result.is_new_transaction = false when a matching account.transaction exists but
continue to iterate and process all entries in result.affected_accounts (use the
same account.record_transaction and address pool maintenance logic), and guard
the wallet-level updates/logging (self.metadata.total_transactions increment and
tracing::info) so they only run when result.is_new_transaction is true to avoid
double-counting; keep other error handling (e.g., pool.maintain_gap_limit)
unchanged.
b1ca36d to
0ba654c
Compare
- 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
0ba654c to
c790ee6
Compare
xdustinface
changed the title
WalletTransactionChecker::check_transactionWalletTransactionChecker::check_core_transaction
2 participants
ManagedAccountType::Standardis also already handled inManagedAccountType::address_pools_mutSummary by CodeRabbit
New Features
Improvements
Refactor