refactor: rename wallet heights to reflect their meaning better#683
Merged
xdustinface merged 2 commits intov0.42-devfrom Apr 24, 2026
Merged
refactor: rename wallet heights to reflect their meaning better#683xdustinface merged 2 commits intov0.42-devfrom
xdustinface merged 2 commits intov0.42-devfrom
Conversation
Rename the wallet-applied chain height from synced_height to last_processed_height across key-wallet, key-wallet-manager, key-wallet-ffi, and dash-spv call sites. This also updates the FFI symbol managed_wallet_last_processed_height and picks up rename sites introduced by newer v0.42-dev changes. filter_committed_height is left unchanged here so the next commit can rename that concept to synced_height with a clear diff.
Rename the committed filter-scan checkpoint from filter_committed_height to synced_height in the wallet interface, wallet manager, and filter sync manager. With synced_height already moved to last_processed_height in the previous commit, synced_height now clearly names the durable committed restart point.
Contributor
📝 WalkthroughWalkthroughThis change systematically refactors block height tracking semantics throughout the wallet codebase, replacing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 #683 +/- ##
=============================================
- Coverage 70.40% 70.15% -0.26%
=============================================
Files 247 319 +72
Lines 48029 66692 +18663
=============================================
+ Hits 33816 46789 +12973
- Misses 14213 19903 +5690
|
![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: 3
🧹 Nitpick comments (2)
key-wallet-ffi/src/wallet_manager_tests.rs (1)
433-464: Nit: stale test name after rename.The test function is still named
test_wallet_manager_synced_heightbut it now exercisesupdate_last_processed_heightandwallet_manager_current_height. Consider renaming to keep intent aligned with the new terminology (e.g.,test_wallet_manager_last_processed_heightortest_wallet_manager_current_height). Note also that after commit 2 in this PR,synced_heighthas been repurposed to mean the filter-committed checkpoint, so the current name is now actively misleading.♻️ Proposed rename
- fn test_wallet_manager_synced_height() { + fn test_wallet_manager_last_processed_height() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 433 - 464, Rename the test function test_wallet_manager_synced_height to a name that matches its behavior (e.g., test_wallet_manager_last_processed_height) and update any references; specifically change the fn name and any mentions in comments to reflect that the test exercises wallet_manager_current_height and the manager_guard.update_last_processed_height flow (look for wallet_manager::wallet_manager_create, wallet_manager::wallet_manager_current_height, and update_last_processed_height to locate the test).dash-spv/src/sync/filters/manager.rs (1)
817-820: Prefer settingsynced_heightdirectly in these filter-manager tests.These tests currently set
last_processed_heightand rely on MockWallet’s default delegation. Settingupdate_synced_height(...)directly would better encode the contract under test and reduce future fragility if MockWallet decouples the two heights.Suggested test-intent alignment
- wallet.update_last_processed_height(50); + wallet.update_synced_height(50); ... - manager.wallet.write().await.update_last_processed_height(100); + manager.wallet.write().await.update_synced_height(100); ... - manager.wallet.write().await.update_last_processed_height(100); + manager.wallet.write().await.update_synced_height(100);Also applies to: 1047-1047, 1124-1124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/filters/manager.rs` around lines 817 - 820, The tests set the mock wallet's last_processed_height via MockWallet::update_last_processed_height, but reviewer asks to set the synced height directly to better express the contract; replace calls to update_last_processed_height(50) (and similar uses at other test sites) with MockWallet::update_synced_height(50) while keeping the same Arc::new(RwLock::new(wallet)) wrapping and MockWallet::new() initialization so the wallet state used by FilterManager tests reflects synced_height explicitly.
🤖 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/lib.rs`:
- Around line 347-349: When importing wallets you currently set birth height
from self.last_processed_height (via
managed_info.set_birth_height(self.last_processed_height)), which can skip the
[self.synced_height, self.last_processed_height) window; change these calls to
use self.synced_height instead (i.e.,
managed_info.set_birth_height(self.synced_height)) so
earliest_required_height()/birth_height logic sees historical blocks. Update the
initial occurrence around T::from_wallet(...) and the other analogous places
where set_birth_height is assigned from self.last_processed_height (the other
two import sites that also call set_first_loaded_at(...)) to use
self.synced_height.
In `@key-wallet-manager/src/wallet_interface.rs`:
- Around line 88-105: Existing external implementors still implement
synced_height and update_synced_height, so restore backward compatibility by
making the old methods default and delegating between old and new names: keep
the new required methods last_processed_height and update_last_processed_height,
but provide default implementations for synced_height() that return
self.last_processed_height() and for update_synced_height(&mut self, h) that
call self.update_last_processed_height(h) (and annotate them #[deprecated] with
a brief message); likewise ensure the new methods have sensible defaults that
call the old ones if present to cover both implementation styles (use the
function symbols last_processed_height, update_last_processed_height,
synced_height, update_synced_height).
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 68-69: The new required methods on the WalletInfoInterface trait
(the newly added get_spendable_utxos() and the renamed last_processed_height
APIs) are a source-breaking change for external implementors; restore
compatibility by providing default implementations on WalletInfoInterface for
get_spendable_utxos() and for the new last_processed_height accessors that
forward to the old methods (or return sensible defaults), and mark the old
methods as deprecated (or keep them as default shims that call the new
implementations) so downstream crates continue to compile until you cut a
breaking release. Ensure you reference the trait name WalletInfoInterface and
the method names get_spendable_utxos and last_processed_height when adding the
defaults and deprecation annotations.
---
Nitpick comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 817-820: The tests set the mock wallet's last_processed_height via
MockWallet::update_last_processed_height, but reviewer asks to set the synced
height directly to better express the contract; replace calls to
update_last_processed_height(50) (and similar uses at other test sites) with
MockWallet::update_synced_height(50) while keeping the same
Arc::new(RwLock::new(wallet)) wrapping and MockWallet::new() initialization so
the wallet state used by FilterManager tests reflects synced_height explicitly.
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 433-464: Rename the test function
test_wallet_manager_synced_height to a name that matches its behavior (e.g.,
test_wallet_manager_last_processed_height) and update any references;
specifically change the fn name and any mentions in comments to reflect that the
test exercises wallet_manager_current_height and the
manager_guard.update_last_processed_height flow (look for
wallet_manager::wallet_manager_create,
wallet_manager::wallet_manager_current_height, and update_last_processed_height
to locate the test).
🪄 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: e4ec8ce4-bb14-44ce-a464-892fce7fc6da
📒 Files selected for processing (22)
dash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/filters/manager.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/src/utxo_tests.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/tests/integration_test.rskey-wallet-manager/tests/spv_integration_tests.rskey-wallet/src/managed_account/mod.rskey-wallet/src/tests/balance_tests.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/metadata.rs
QuantumExplorer
approved these changes
Apr 24, 2026
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.
Rename:
synced_height->last_processed_heightfilter_committed_height->synced_heightSummary by CodeRabbit
Release Notes
Refactor
API Changes