Skip to content

refactor: extract snapshot_balances/emit_balance_changes helpers#542

Open
xdustinface wants to merge 1 commit intov0.42-devfrom
refactor/balance-emit-helper
Open

refactor: extract snapshot_balances/emit_balance_changes helpers#542
xdustinface wants to merge 1 commit intov0.42-devfrom
refactor/balance-emit-helper

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Mar 15, 2026

To avoid cloning the same pattern in other places in other coming PRs.

Summary by CodeRabbit

  • Refactor
    • Optimized internal balance change tracking mechanism by implementing a snapshot-based approach for wallet balance updates, improving efficiency of balance event processing without impacting the public API.

To avoid cloning the same pattern in other places in other coming PRs.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 15, 2026

📝 Walkthrough

Walkthrough

Introduces two internal utility methods (snapshot_balances and emit_balance_changes) for wallet balance tracking and refactors balance change emission logic to use a snapshot-based batch approach instead of per-wallet comparisons within update loops.

Changes

Cohort / File(s) Summary
Internal balance utilities
key-wallet-manager/src/wallet_manager/mod.rs
Adds two pub(crate) methods: snapshot_balances creates a snapshot vector of wallet IDs and core balances; emit_balance_changes compares old snapshots with current balances and emits BalanceUpdated events for changed wallets.
Snapshot-based balance emission
key-wallet-manager/src/wallet_manager/process_block.rs
Refactors update_synced_height to capture balance snapshots before updates, then batch-emit balance changes via emit_balance_changes instead of per-wallet event construction inside the update loop.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A snapshot of wallets so wise,
Balance changes caught by our eyes,
No per-wallet toil, just batch events now,
Efficiency grows—take a bow!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: extracting two new helper methods (snapshot_balances and emit_balance_changes) to reduce code duplication.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/balance-emit-helper
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/mod.rs`:
- Around line 1024-1041: The method emit_balance_changes references
self.event_sender (which only exists under the "std" feature) causing no-std
builds to fail; fix by gating the function with #[cfg(feature = "std")] and
providing a matching #[cfg(not(feature = "std"))] no-op implementation (or
alternatively gate the parts that touch self.event_sender) so that
emit_balance_changes and uses of WalletEvent/send are only compiled when the std
feature is enabled; locate the emit_balance_changes function and the
event_sender field to apply the cfg attributes consistently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 615839af-4c59-40bb-bdee-f9af09364a50

📥 Commits

Reviewing files that changed from the base of the PR and between 6749114 and 6b72eb4.

📒 Files selected for processing (2)
  • key-wallet-manager/src/wallet_manager/mod.rs
  • key-wallet-manager/src/wallet_manager/process_block.rs

@codecov
Copy link

codecov bot commented Mar 15, 2026

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 66.87%. Comparing base (b946271) to head (6b72eb4).
⚠️ Report is 1 commits behind head on v0.42-dev.

Files with missing lines Patch % Lines
key-wallet-manager/src/wallet_manager/mod.rs 94.73% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #542      +/-   ##
=============================================
- Coverage      66.89%   66.87%   -0.03%     
=============================================
  Files            313      313              
  Lines          64753    64763      +10     
=============================================
- Hits           43317    43308       -9     
- Misses         21436    21455      +19     
Flag Coverage Δ *Carryforward flag
core 75.02% <ø> (ø)
dash-network 75.00% <ø> (ø) Carriedforward from b946271
dash-network-ffi 34.76% <ø> (ø) Carriedforward from b946271
dash-spv 68.26% <ø> (ø) Carriedforward from b946271
dash-spv-ffi 34.76% <ø> (ø) Carriedforward from b946271
dashcore 75.00% <ø> (ø) Carriedforward from b946271
dashcore-private 75.00% <ø> (ø) Carriedforward from b946271
dashcore-rpc 19.92% <ø> (ø) Carriedforward from b946271
dashcore-rpc-json 19.92% <ø> (ø) Carriedforward from b946271
dashcore_hashes 75.00% <ø> (ø) Carriedforward from b946271
ffi 37.14% <ø> (+0.54%) ⬆️
key-wallet 65.62% <ø> (-0.03%) ⬇️ Carriedforward from b946271
key-wallet-ffi 34.76% <ø> (ø) Carriedforward from b946271
key-wallet-manager 65.62% <ø> (-0.03%) ⬇️ Carriedforward from b946271
rpc 19.92% <ø> (ø)
spv 81.03% <ø> (-0.05%) ⬇️
wallet 65.69% <95.45%> (+0.01%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...wallet-manager/src/wallet_manager/process_block.rs 48.97% <100.00%> (-7.92%) ⬇️
key-wallet-manager/src/wallet_manager/mod.rs 47.92% <94.73%> (+1.52%) ⬆️

... and 15 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant