refactor: extract WalletManager accessors and error types#599
Merged
QuantumExplorer merged 1 commit intov0.42-devfrom Mar 30, 2026
Merged
Conversation
5 tasks
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR introduces a dedicated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 #599 +/- ##
=============================================
- Coverage 67.24% 67.03% -0.21%
=============================================
Files 318 320 +2
Lines 67018 67249 +231
=============================================
+ Hits 45066 45082 +16
- Misses 21952 22167 +215
|
![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: 4
🧹 Nitpick comments (2)
fuzz/Cargo.toml (1)
18-18: Minor: redundant feature configuration.Since
key-wallet's default feature is["getrandom"], writingdefault-features = false, features = ["getrandom"]is equivalent to just using defaults. This could be simplified to:key-wallet = { path = "../key-wallet" }However, if the intent is to be explicit about minimal dependencies for fuzz targets, the current form is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/Cargo.toml` at line 18, The dependency entry for key-wallet is redundant: it sets default-features = false and features = ["getrandom"] while key-wallet's default feature is already ["getrandom"]; simplify by removing the explicit feature override and use the default form for key-wallet (reference: dependency key-wallet in Cargo.toml) unless you intentionally want to force minimal features for fuzz targets—in that case keep the current explicit form.key-wallet-manager/src/error.rs (1)
6-30: Consider usingthiserrorfor error type derivation.The coding guidelines indicate using
thiserrorfor proper error types. The current manualDisplayandErrorimplementations work correctly but could be simplified:♻️ Example using thiserror
use thiserror::Error; #[derive(Debug, Error)] pub enum WalletError { #[error("Wallet creation failed: {0}")] WalletCreation(String), #[error("Wallet not found: {}", hex::encode(.0))] WalletNotFound(WalletId), // ... etc }Note: This would require adding
thiserrorandhexdependencies. The current implementation is correct; this is a stylistic suggestion per coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/error.rs` around lines 6 - 30, Replace the manual Debug/Display/Error implementations for WalletError by deriving thiserror::Error on the enum: add the thiserror dependency and annotate WalletError with #[derive(Debug, Error)] and put per-variant #[error("...")] messages for variants like WalletCreation, WalletNotFound(WalletId), WalletExists, InvalidMnemonic, AccountCreation, AccountNotFound, AddressGeneration, InvalidNetwork, InvalidParameter, TransactionBuild, InsufficientFunds; for hex-encoded WalletId messages add the hex crate and use hex::encode(.0) inside the error format strings, then remove the hand-rolled Display/Error impls to rely on thiserror.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/src/client/config.rs`:
- Around line 103-109: The current ClientConfig construction and
default_peers_for_network implicitly add regtest/localhost peers (via
ClientConfig::new using Self::default_peers_for_network), causing
ClientConfig::regtest() to be non-empty; change the logic so regtest has no
default peers and callers must opt-in explicitly. Concretely, update
default_peers_for_network to return an empty list for Network::Regtest (or make
ClientConfig::regtest() call Self::default() with peers cleared) and ensure
ClientConfig::new does not inject localhost for Regtest; keep add_peer(addr)
semantics unchanged so tests/CLI can explicitly add the local peer.
In `@key-wallet-manager/src/accessors.rs`:
- Around line 37-45: The remove_wallet method currently removes from
self.wallets before verifying a matching entry in self.wallet_infos, risking
partial mutation; change it to check existence of both entries first (e.g., use
self.wallets.get(wallet_id) and self.wallet_infos.get(wallet_id) or contains_key
for both) and return WalletError::WalletNotFound without mutating if either is
missing, then perform the removals (self.wallets.remove and
self.wallet_infos.remove), update self.structural_revision, and return the pair
(wallet, info); alternatively use a remove_entry-style API to atomically take
both after confirming presence.
- Around line 125-143: The update_wallet_metadata function currently always
advances last_synced (calls
managed_info.update_last_synced(current_timestamp())) even for metadata-only
updates; change it so update_last_synced is only called when a real chain sync
occurs (i.e., do NOT call update_last_synced inside update_wallet_metadata when
only name/description are provided or when both name and description are None),
by checking whether the update is a metadata-only change before invoking
managed_info.update_last_synced; remove current_timestamp from this file's
imports and, if audit is needed for metadata edits, add a dedicated field (e.g.,
metadata_last_modified) or similar on the managed_info struct instead of reusing
last_synced.
In `@key-wallet-manager/src/lib.rs`:
- Line 1: Replace the outer doc comment used for the crate overview with an
inner crate-level doc comment at the top of lib.rs: add a `//!` block as the
very first lines containing the crate overview (move or rewrite the existing
overview text there), and leave the existing `///` comments in place directly
above `pub use key_wallet;` and `mod accessors;` so they continue to document
those items rather than the crate itself; ensure the `//!` block appears before
any code or items in the file.
---
Nitpick comments:
In `@fuzz/Cargo.toml`:
- Line 18: The dependency entry for key-wallet is redundant: it sets
default-features = false and features = ["getrandom"] while key-wallet's default
feature is already ["getrandom"]; simplify by removing the explicit feature
override and use the default form for key-wallet (reference: dependency
key-wallet in Cargo.toml) unless you intentionally want to force minimal
features for fuzz targets—in that case keep the current explicit form.
In `@key-wallet-manager/src/error.rs`:
- Around line 6-30: Replace the manual Debug/Display/Error implementations for
WalletError by deriving thiserror::Error on the enum: add the thiserror
dependency and annotate WalletError with #[derive(Debug, Error)] and put
per-variant #[error("...")] messages for variants like WalletCreation,
WalletNotFound(WalletId), WalletExists, InvalidMnemonic, AccountCreation,
AccountNotFound, AddressGeneration, InvalidNetwork, InvalidParameter,
TransactionBuild, InsufficientFunds; for hex-encoded WalletId messages add the
hex crate and use hex::encode(.0) inside the error format strings, then remove
the hand-rolled Display/Error impls to rely on thiserror.
🪄 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: a7ba0ff7-bc5e-4ddb-93ec-22f57d61c25f
📒 Files selected for processing (17)
.codecov.ymldash-spv/Cargo.tomldash-spv/src/client/config.rsdash-spv/src/client/config_test.rsdash-spv/tests/dashd_sync/setup.rsfuzz/Cargo.tomlkey-wallet-manager/Cargo.tomlkey-wallet-manager/src/accessors.rskey-wallet-manager/src/error.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet/Cargo.tomlkey-wallet/examples/wallet_creation.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rsrpc-json/Cargo.toml
💤 Files with no reviewable changes (1)
- .codecov.yml
Comment on lines
+37
to
+45
| pub fn remove_wallet(&mut self, wallet_id: &WalletId) -> Result<(Wallet, T), WalletError> { | ||
| let wallet = | ||
| self.wallets.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | ||
| let info = | ||
| self.wallet_infos.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | ||
| // Absorb the removed wallet's account-level revision so the total | ||
| // stays monotonically increasing even though we lost a contributor. | ||
| self.structural_revision += info.monitor_revision() + 1; | ||
| Ok((wallet, info)) |
Contributor
There was a problem hiding this comment.
Make remove_wallet all-or-nothing.
Line 39 removes from self.wallets before Line 41 proves the paired wallet_infos entry exists. If those maps ever drift, this returns WalletNotFound after mutating state and leaves the manager more inconsistent.
Suggested fix
pub fn remove_wallet(&mut self, wallet_id: &WalletId) -> Result<(Wallet, T), WalletError> {
- let wallet =
- self.wallets.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?;
- let info =
- self.wallet_infos.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?;
+ let revision = match (
+ self.wallets.contains_key(wallet_id),
+ self.wallet_infos.get(wallet_id),
+ ) {
+ (true, Some(info)) => info.monitor_revision(),
+ _ => return Err(WalletError::WalletNotFound(*wallet_id)),
+ };
+ let wallet =
+ self.wallets.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?;
+ let info =
+ self.wallet_infos.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?;
// Absorb the removed wallet's account-level revision so the total
// stays monotonically increasing even though we lost a contributor.
- self.structural_revision += info.monitor_revision() + 1;
+ self.structural_revision += revision + 1;
Ok((wallet, info))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn remove_wallet(&mut self, wallet_id: &WalletId) -> Result<(Wallet, T), WalletError> { | |
| let wallet = | |
| self.wallets.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | |
| let info = | |
| self.wallet_infos.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | |
| // Absorb the removed wallet's account-level revision so the total | |
| // stays monotonically increasing even though we lost a contributor. | |
| self.structural_revision += info.monitor_revision() + 1; | |
| Ok((wallet, info)) | |
| pub fn remove_wallet(&mut self, wallet_id: &WalletId) -> Result<(Wallet, T), WalletError> { | |
| let revision = match ( | |
| self.wallets.contains_key(wallet_id), | |
| self.wallet_infos.get(wallet_id), | |
| ) { | |
| (true, Some(info)) => info.monitor_revision(), | |
| _ => return Err(WalletError::WalletNotFound(*wallet_id)), | |
| }; | |
| let wallet = | |
| self.wallets.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | |
| let info = | |
| self.wallet_infos.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | |
| // Absorb the removed wallet's account-level revision so the total | |
| // stays monotonically increasing even though we lost a contributor. | |
| self.structural_revision += revision + 1; | |
| Ok((wallet, info)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/accessors.rs` around lines 37 - 45, The remove_wallet
method currently removes from self.wallets before verifying a matching entry in
self.wallet_infos, risking partial mutation; change it to check existence of
both entries first (e.g., use self.wallets.get(wallet_id) and
self.wallet_infos.get(wallet_id) or contains_key for both) and return
WalletError::WalletNotFound without mutating if either is missing, then perform
the removals (self.wallets.remove and self.wallet_infos.remove), update
self.structural_revision, and return the pair (wallet, info); alternatively use
a remove_entry-style API to atomically take both after confirming presence.
Comment on lines
+125
to
+143
| /// Update wallet metadata | ||
| pub fn update_wallet_metadata( | ||
| &mut self, | ||
| wallet_id: &WalletId, | ||
| name: Option<String>, | ||
| description: Option<String>, | ||
| ) -> Result<(), WalletError> { | ||
| let managed_info = | ||
| self.wallet_infos.get_mut(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | ||
|
|
||
| if let Some(new_name) = name { | ||
| managed_info.set_name(new_name); | ||
| } | ||
|
|
||
| if let Some(desc) = description { | ||
| managed_info.set_description(Some(desc)); | ||
| } | ||
|
|
||
| managed_info.update_last_synced(current_timestamp()); |
Contributor
There was a problem hiding this comment.
Don't advance last_synced on metadata-only writes.
Line 143 runs even for update_wallet_metadata(id, None, None), so a rename or description edit looks like a fresh chain sync. Anything using that field for wallet freshness will get a false positive.
Suggested fix
- managed_info.update_last_synced(current_timestamp());
-
Ok(())If you need audit info for metadata edits, track it in a dedicated field instead. Also drop current_timestamp from the import list at the top of this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/accessors.rs` around lines 125 - 143, The
update_wallet_metadata function currently always advances last_synced (calls
managed_info.update_last_synced(current_timestamp())) even for metadata-only
updates; change it so update_last_synced is only called when a real chain sync
occurs (i.e., do NOT call update_last_synced inside update_wallet_metadata when
only name/description are provided or when both name and description are None),
by checking whether the update is a metadata-only change before invoking
managed_info.update_last_synced; remove current_timestamp from this file's
imports and, if audit is needed for metadata edits, add a dedicated field (e.g.,
metadata_last_modified) or similar on the managed_info struct instead of reusing
last_synced.
| //! - Address generation and gap limit handling | ||
| //! - Blockchain synchronization | ||
|
|
||
| /// Re-export key-wallet so consumers can access wallet primitives through this crate. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "lib.rs" | grep key-wallet-managerRepository: dashpay/rust-dashcore
Length of output: 235
🏁 Script executed:
head -30 key-wallet-manager/src/lib.rsRepository: dashpay/rust-dashcore
Length of output: 1039
Use inner doc comments at crate root for the crate overview.
The crate root currently uses only outer doc comments (///), which document the following items rather than the crate itself. Per Rust conventions, crate-level documentation should use inner doc comments (//!) at the start of lib.rs. Without //!, the generated rustdoc will have no overview on the crate's documentation homepage. The current /// at line 1 documents the pub use key_wallet; item; lines 4–15 document mod accessors;. Add a //! block at the file start with the intended crate overview, and keep the outer /// comments for their respective items.
Suggested fix
+//! High-level wallet management for Dash
+//!
+//! This module provides high-level wallet functionality that builds on top of
+//! the low-level primitives in `key-wallet`
+//!
+//! ## Features
+//!
+//! - Multiple wallet management
+//! - BIP 157/158 compact block filter support
+//! - Address generation and gap limit handling
+//! - Blockchain synchronization
+
/// Re-export key-wallet so consumers can access wallet primitives through this crate.
pub use key_wallet;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/lib.rs` at line 1, Replace the outer doc comment used
for the crate overview with an inner crate-level doc comment at the top of
lib.rs: add a `//!` block as the very first lines containing the crate overview
(move or rewrite the existing overview text there), and leave the existing `///`
comments in place directly above `pub use key_wallet;` and `mod accessors;` so
they continue to document those items rather than the crate itself; ensure the
`//!` block appears before any code or items in the file.
Improves the key-wallet-manager crate structure by breaking up lib.rs: - New accessors.rs (~213 lines): wallet creation, import, removal, and lookup methods extracted from WalletManager - New error.rs (~99 lines): WalletError enum extracted into its own module - Slimmed lib.rs: now contains only the core WalletManager struct definition and field management Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
refactor/key-wallet-manager-improvements
branch
from
472a397 to
5616282
Compare
ZocoLini
approved these changes
Mar 30, 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.
Summary
Follow-up to #594. Improves the
key-wallet-managercrate structure by breaking up the monolithiclib.rs:accessors.rs(~213 lines): Wallet creation, import, removal, and lookup methods extracted fromWalletManagererror.rs(~99 lines):WalletErrorenum extracted into its own modulelib.rs: Now contains only the coreWalletManagerstruct definition and field managementkey-wallet-managertokey-wallet(they test wallet primitives, not the manager layer)Supersedes #590 which was based on a pre-#594 branch.
Test plan
cargo check --workspacepasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor