refactor: extract key-wallet-manager into separate crate#590
refactor: extract key-wallet-manager into separate crate#590QuantumExplorer wants to merge 2 commits intov0.42-devfrom
Conversation
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (65)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (41)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughAdds a new crate Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "dash-spv CLI"
participant Client as "DashSpvClient"
participant Manager as "key-wallet-manager::WalletManager"
participant Core as "key_wallet (core wallet)"
rect rgba(200,200,255,0.5)
CLI->>Client: create with WalletManager reference
Client->>Manager: subscribe / call APIs (sync, tx, balance)
Manager->>Core: delegate wallet primitives (create, scan, utxos, tx)
Core-->>Manager: results / events
Manager-->>Client: emit WalletEvent / results
Client-->>CLI: progress / event callbacks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
key-wallet-manager/src/process_block.rs (1)
264-264: Minor typo in comment."Inrease" should be "Increase".
📝 Suggested fix
- //Increase synced height + // Increase synced height🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/process_block.rs` at line 264, Locate the inline comment that reads "// Inrease synced height" (the comment above the synced-height increment) and correct the typo to "// Increase synced height" so the comment is spelled properly and clear.key-wallet-manager/Cargo.toml (1)
21-24: Use consistent version precision in Cargo.toml dependencies.The dependencies mix loose specs (
"0.1","1.11") with precise ones ("2.0.1"). For better reproducibility, align all versions to the same precision level. Note that"1.11"means>=1.11.0, <1.12.0, which allows patch versions to drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/Cargo.toml` around lines 21 - 24, The Cargo.toml dependencies use mixed version precision (async-trait "0.1", rayon "1.11", tracing "0.1" vs bincode "2.0.1"); make them consistent by pinning or matching precision across all entries (e.g., change async-trait and tracing to "0.1.51" or similar exact patch versions, or change bincode to "2.0" if you prefer caret-style), update the entries for async-trait, rayon, bincode, and tracing so they all follow the same precision scheme (either exact patch versions or caret/tilde ranges) to ensure reproducible builds.key-wallet-manager/src/lib.rs (2)
1188-1223: Preservekey_wallet::Erroras a source instead of flattening it into strings.This remapping discards the original variant/source information, so callers can no longer match or chain the underlying error. A wrapped
WalletErrorvariant is easier to propagate and removes the need to keep a manual mapping table in sync.As per coding guidelines,
Use proper error types (thiserror) and propagate errors appropriately in Rust code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/lib.rs` around lines 1188 - 1223, The current impl From<key_wallet::Error> for WalletError flattens and stringifies the original key_wallet::Error variants; instead add a wrapped variant on WalletError (e.g., KeyWallet(#[from] key_wallet::Error) or KeyWalletError(Box<key_wallet::Error>)) and change the From implementation to return that wrapper for unhandled cases (or use #[derive(thiserror::Error)] with a #[from] field) so the original key_wallet::Error value and its variant information are preserved for chaining and matching; keep any special-case mappings you need (like InvalidMnemonic -> InvalidMnemonic) but fallback to the new KeyWallet/KeyWalletError wrapper to avoid losing the source error.
24-25: The fullkey_walletre-export blurs the boundary this PR is trying to create.One of the goals here is keeping lightweight consumers on
key-walletonly. Re-exporting the entire primitive crate fromkey-wallet-managermakes it easy for new code to depend on the heavier crate for primitives again; narrower re-exports would keep that boundary clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/lib.rs` around lines 24 - 25, The current top-level full re-export `pub use key_wallet;` in lib.rs exposes the entire heavier primitive crate and breaks the intended dependency boundary; remove that blanket re-export and instead audit which specific types/functions from the key_wallet crate this crate actually needs to expose to consumers, then add explicit, narrow re-exports for only those symbols (replace the `pub use key_wallet;` line with `pub use key_wallet::{...}` containing the audited identifiers) so lightweight consumers remain limited to the lighter crate surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet-manager/Cargo.toml`:
- Around line 21-24: The Cargo.toml dependencies use mixed version precision
(async-trait "0.1", rayon "1.11", tracing "0.1" vs bincode "2.0.1"); make them
consistent by pinning or matching precision across all entries (e.g., change
async-trait and tracing to "0.1.51" or similar exact patch versions, or change
bincode to "2.0" if you prefer caret-style), update the entries for async-trait,
rayon, bincode, and tracing so they all follow the same precision scheme (either
exact patch versions or caret/tilde ranges) to ensure reproducible builds.
In `@key-wallet-manager/src/lib.rs`:
- Around line 1188-1223: The current impl From<key_wallet::Error> for
WalletError flattens and stringifies the original key_wallet::Error variants;
instead add a wrapped variant on WalletError (e.g., KeyWallet(#[from]
key_wallet::Error) or KeyWalletError(Box<key_wallet::Error>)) and change the
From implementation to return that wrapper for unhandled cases (or use
#[derive(thiserror::Error)] with a #[from] field) so the original
key_wallet::Error value and its variant information are preserved for chaining
and matching; keep any special-case mappings you need (like InvalidMnemonic ->
InvalidMnemonic) but fallback to the new KeyWallet/KeyWalletError wrapper to
avoid losing the source error.
- Around line 24-25: The current top-level full re-export `pub use key_wallet;`
in lib.rs exposes the entire heavier primitive crate and breaks the intended
dependency boundary; remove that blanket re-export and instead audit which
specific types/functions from the key_wallet crate this crate actually needs to
expose to consumers, then add explicit, narrow re-exports for only those symbols
(replace the `pub use key_wallet;` line with `pub use key_wallet::{...}`
containing the audited identifiers) so lightweight consumers remain limited to
the lighter crate surface.
In `@key-wallet-manager/src/process_block.rs`:
- Line 264: Locate the inline comment that reads "// Inrease synced height" (the
comment above the synced-height increment) and correct the typo to "// Increase
synced height" so the comment is spelled properly and clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5444982c-194e-4d98-b826-e314eaf80538
📒 Files selected for processing (63)
Cargo.tomldash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/peer_reputation.jsondash-spv/src/client/core.rsdash-spv/src/client/event_handler.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/test_utils/event_handler.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rsfuzz/Cargo.tomlkey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/matching.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/wallet_interface.rskey-wallet/Cargo.tomlkey-wallet/examples/wallet_creation.rskey-wallet/src/lib.rskey-wallet/src/test_utils/mod.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (2)
- key-wallet/src/lib.rs
- key-wallet/src/test_utils/mod.rs
QuantumExplorer
force-pushed
the
refactor/extract-key-wallet-manager
branch
from
bca7ccc to
3c88605
Compare
github-actions
bot
removed
the
merge-conflict
QuantumExplorer
force-pushed
the
refactor/extract-key-wallet-manager
branch
2 times, most recently
from
43cfe1b to
f10064c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #590 +/- ##
=============================================
+ Coverage 67.06% 67.36% +0.29%
=============================================
Files 318 312 -6
Lines 67026 65947 -1079
=============================================
- Hits 44950 44422 -528
+ Misses 22076 21525 -551
|
Moves the manager module from key-wallet into its own key-wallet-manager crate to enforce a clean dependency boundary between wallet primitives and the async wallet runtime (tokio). This solves the problem where Cargo's feature unification caused tokio to leak into WASM and other lightweight consumers through the default "manager" feature flag — a fragile boundary that relied on every consumer remembering default-features = false. With separate crates, key-wallet is tokio-free by construction. Consumers that need WalletManager, WalletInterface, or event support add key-wallet-manager as an explicit dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
refactor/extract-key-wallet-manager
branch
from
f10064c to
f92f114
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
key-wallet-manager/src/error.rs (1)
5-62: PreferthiserrorforWalletError.This new crate-level error type is hand-rolling both
Displayandstd::error::Error, which makes future variant changes easy to desync. Please derivethiserror::Errorinstead and keep the formatting on the variants themselves.As per coding guidelines "Use proper error types (thiserror) and propagate errors appropriately in Rust code".
🤖 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 5 - 62, Replace the hand-rolled Display and Error impls for WalletError with thiserror: add #[derive(thiserror::Error, Debug)] on the WalletError enum, remove the manual impl core::fmt::Display for WalletError and impl std::error::Error for WalletError, and annotate each variant with a #[error("...")] string that preserves the current messages (e.g., #[error("Wallet creation failed: {0}")] for WalletCreation, #[error("Invalid mnemonic: {0}")] for InvalidMnemonic, etc.). For WalletNotFound and WalletExists (the WalletId variants) use an #[error] message that formats the id the same way as before (include a formatting expression that renders the id as hex), and keep the existing variant names (WalletError, WalletNotFound, WalletExists, InvalidMnemonic, AccountCreation, AccountNotFound, AddressGeneration, InvalidNetwork, InvalidParameter, TransactionBuild, InsufficientFunds) so callers/signatures remain unchanged.
🤖 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/accessors.rs`:
- Around line 74-82: get_account currently only calls wallet.get_bip44_account
and therefore returns Ok(None) for BIP32-only accounts; update the accessor to
accept an explicit account-type/preference parameter (e.g., AccountType or enum)
or implement fallback logic: first try wallet.get_bip44_account(index) and if
None then call the BIP32 lookup/generation path used by the address-generation
helpers in lib.rs (or wallet.get_bip32_account/get_account_by_index if present),
returning the found &Account; adjust the get_account signature and callers to
pass the account type when needed and ensure the Result<Option<&Account>,
WalletError> semantics remain unchanged.
- Around line 37-45: remove_wallet currently removes from self.wallets before
ensuring a matching entry exists in self.wallet_infos, which can leave the
manager inconsistent if the second lookup fails; update remove_wallet to first
check presence of both entries (e.g., using self.wallets.get and
self.wallet_infos.get or by borrowing checks) and only perform the removals
together (remove from self.wallets and self.wallet_infos) once both are
confirmed present, then adjust self.structural_revision using the removed info
and return Ok((wallet, info)); preserve error return of
WalletError::WalletNotFound for missing WalletId.
- Around line 169-172: The public accessor event_sender(&self) exposes
broadcast::Sender<WalletEvent> to consumers and must be restricted: change the
method visibility to pub(crate) so only crate-local code (e.g., process_block.rs
and lib.rs) can call it, and keep subscribe_events() public for consumers;
alternatively remove the accessor entirely and add controlled emit helpers
(e.g., emit_event or emit_wallet_event on the manager) that validate invariants
and call the internal sender. Locate the event_sender method and replace its
signature with a crate-only accessor or remove it, then update internal call
sites (process_block.rs, lib.rs) to use the new pub(crate) accessor or the new
emit_X helper(s), ensuring external code cannot call .send() directly.
In `@key-wallet-manager/src/error.rs`:
- Around line 95-96: Replace the catch-all `_` arm that maps any
key_wallet::Error into WalletError::InvalidParameter (and remove the
#[allow(unreachable_patterns)] attribute) with explicit match arms for each
variant of key_wallet::Error so the original error types are preserved;
specifically add explicit arms for Slip10 and BLS (each behind their respective
feature gates) that convert to the appropriate WalletError variant instead of
collapsing to WalletError::InvalidParameter, so the compiler will force you to
handle new upstream variants of key_wallet::Error.
---
Nitpick comments:
In `@key-wallet-manager/src/error.rs`:
- Around line 5-62: Replace the hand-rolled Display and Error impls for
WalletError with thiserror: add #[derive(thiserror::Error, Debug)] on the
WalletError enum, remove the manual impl core::fmt::Display for WalletError and
impl std::error::Error for WalletError, and annotate each variant with a
#[error("...")] string that preserves the current messages (e.g.,
#[error("Wallet creation failed: {0}")] for WalletCreation, #[error("Invalid
mnemonic: {0}")] for InvalidMnemonic, etc.). For WalletNotFound and WalletExists
(the WalletId variants) use an #[error] message that formats the id the same way
as before (include a formatting expression that renders the id as hex), and keep
the existing variant names (WalletError, WalletNotFound, WalletExists,
InvalidMnemonic, AccountCreation, AccountNotFound, AddressGeneration,
InvalidNetwork, InvalidParameter, TransactionBuild, InsufficientFunds) so
callers/signatures remain unchanged.
🪄 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: 2311ac35-6c84-4454-9f8e-e11452a8fab0
📒 Files selected for processing (64)
Cargo.tomldash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/event_handler.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/test_utils/event_handler.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rsfuzz/Cargo.tomlkey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/src/accessors.rskey-wallet-manager/src/error.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/matching.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/wallet_interface.rskey-wallet/Cargo.tomlkey-wallet/examples/wallet_creation.rskey-wallet/src/lib.rskey-wallet/src/test_utils/mod.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (2)
- key-wallet/src/lib.rs
- key-wallet/src/test_utils/mod.rs
✅ Files skipped from review due to trivial changes (44)
- dash-spv/src/lib.rs
- key-wallet-manager/src/wallet_interface.rs
- key-wallet-ffi/src/wallet_manager_tests.rs
- fuzz/Cargo.toml
- dash-spv/examples/simple_sync.rs
- dash-spv/tests/wallet_integration_test.rs
- key-wallet/examples/wallet_creation.rs
- dash-spv/src/validation/filter.rs
- key-wallet-manager/src/events.rs
- dash-spv/examples/spv_with_wallet.rs
- dash-spv/src/client/transactions.rs
- dash-spv/src/client/events.rs
- dash-spv/src/sync/filters/pipeline.rs
- dash-spv/src/sync/sync_coordinator.rs
- key-wallet-manager/src/test_helpers.rs
- key-wallet-manager/src/event_tests.rs
- dash-spv/src/client/lifecycle.rs
- dash-spv-ffi/Cargo.toml
- dash-spv/src/client/sync_coordinator.rs
- dash-spv/tests/dashd_sync/tests_basic.rs
- key-wallet-manager/src/test_utils/mock_wallet.rs
- key-wallet/tests/test_serialized_wallets.rs
- dash-spv/src/client/core.rs
- dash-spv/src/client/mod.rs
- key-wallet-ffi/Cargo.toml
- dash-spv/examples/filter_sync.rs
- key-wallet-manager/src/test_utils/mod.rs
- dash-spv/src/main.rs
- dash-spv/src/sync/blocks/manager.rs
- dash-spv/src/client/mempool.rs
- dash-spv/tests/peer_test.rs
- Cargo.toml
- dash-spv/src/client/queries.rs
- dash-spv/src/sync/events.rs
- key-wallet-ffi/tests/test_error_conversions.rs
- dash-spv/src/sync/filters/batch_tracker.rs
- dash-spv/src/sync/filters/sync_manager.rs
- dash-spv/src/sync/mempool/manager.rs
- key-wallet-manager/Cargo.toml
- key-wallet-manager/src/process_block.rs
- key-wallet-manager/src/matching.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- dash-spv/src/sync/blocks/pipeline.rs
- key-wallet/tests/integration_test.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- dash-spv-ffi/src/client.rs
- dash-spv/src/sync/filters/manager.rs
- dash-spv/Cargo.toml
- key-wallet-ffi/src/error.rs
- key-wallet/tests/spv_integration_tests.rs
- dash-spv/tests/dashd_sync/helpers.rs
- dash-spv/src/client/event_handler.rs
- key-wallet/Cargo.toml
- key-wallet-ffi/src/wallet_manager.rs
- dash-spv/src/test_utils/event_handler.rs
| 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)) |
There was a problem hiding this comment.
Keep wallet/info removal atomic.
If the two maps ever drift, Line 39 removes the wallet before Line 41 errors, leaving the manager in a worse inconsistent state. Check both entries before mutating, then remove them together.
💡 Minimal fix
pub fn remove_wallet(&mut self, wallet_id: &WalletId) -> Result<(Wallet, T), WalletError> {
+ if !self.wallets.contains_key(wallet_id) || !self.wallet_infos.contains_key(wallet_id) {
+ 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))?;📝 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.
| 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> { | |
| if !self.wallets.contains_key(wallet_id) || !self.wallet_infos.contains_key(wallet_id) { | |
| 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; | |
| 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, remove_wallet
currently removes from self.wallets before ensuring a matching entry exists in
self.wallet_infos, which can leave the manager inconsistent if the second lookup
fails; update remove_wallet to first check presence of both entries (e.g., using
self.wallets.get and self.wallet_infos.get or by borrowing checks) and only
perform the removals together (remove from self.wallets and self.wallet_infos)
once both are confirmed present, then adjust self.structural_revision using the
removed info and return Ok((wallet, info)); preserve error return of
WalletError::WalletNotFound for missing WalletId.
| /// Get account by index in a specific wallet | ||
| pub fn get_account( | ||
| &self, | ||
| wallet_id: &WalletId, | ||
| index: u32, | ||
| ) -> Result<Option<&Account>, WalletError> { | ||
| let wallet = self.wallets.get(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | ||
| Ok(wallet.get_bip44_account(index)) | ||
| } |
There was a problem hiding this comment.
get_account only sees BIP44 accounts right now.
This returns Ok(None) for a BIP32-only account at the same index, even though the wallet contains it. The accessor needs an explicit account type/preference parameter, or equivalent fallback logic to the address-generation methods in key-wallet-manager/src/lib.rs.
🤖 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 74 - 82, get_account
currently only calls wallet.get_bip44_account and therefore returns Ok(None) for
BIP32-only accounts; update the accessor to accept an explicit
account-type/preference parameter (e.g., AccountType or enum) or implement
fallback logic: first try wallet.get_bip44_account(index) and if None then call
the BIP32 lookup/generation path used by the address-generation helpers in
lib.rs (or wallet.get_bip32_account/get_account_by_index if present), returning
the found &Account; adjust the get_account signature and callers to pass the
account type when needed and ensure the Result<Option<&Account>, WalletError>
semantics remain unchanged.
| #[allow(unreachable_patterns)] | ||
| _ => WalletError::InvalidParameter(format!("Key wallet error: {}", err)), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the error.rs file
find . -path "*key-wallet-manager/src/error.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 98
🏁 Script executed:
# Read the context around lines 95-96
cat -n key-wallet-manager/src/error.rs | sed -n '80,110p'Repository: dashpay/rust-dashcore
Length of output: 1246
🏁 Script executed:
# Search for the key_wallet::Error definition and its variants
rg "enum Error|struct Error" --type rs -A 20 | grep -A 20 "key_wallet"Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Check for feature gating related to Slip10 and BLS
rg "Slip10|BLS" --type rs -B 2 -A 2Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Read broader context to see the full match statement
cat -n key-wallet-manager/src/error.rs | sed -n '50,105p'Repository: dashpay/rust-dashcore
Length of output: 3017
🏁 Script executed:
# Search for key_wallet::Error definition with correct syntax
rg "key_wallet" -A 15 key-wallet-manager/src/error.rsRepository: dashpay/rust-dashcore
Length of output: 981
🏁 Script executed:
# Look for the From implementation to understand the match structure
cat -n key-wallet-manager/src/error.rs | head -100Repository: dashpay/rust-dashcore
Length of output: 4831
🏁 Script executed:
# Search for Slip10 and BLS to understand feature gating
rg "Slip10|BLS"Repository: dashpay/rust-dashcore
Length of output: 50377
🏁 Script executed:
# Find the key_wallet error.rs file
find . -path "*key-wallet/src/error.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 90
🏁 Script executed:
# Read the full key_wallet::Error enum definition
cat -n key-wallet/src/error.rs | head -150Repository: dashpay/rust-dashcore
Length of output: 4311
🏁 Script executed:
# Check for feature gates around Slip10 and BLS variants
grep -n "Slip10\|BLS\|#\[cfg" key-wallet/src/error.rs | head -50Repository: dashpay/rust-dashcore
Length of output: 670
🏁 Script executed:
# Check for tests around the From impl or error conversion
grep -r "InvalidParameter.*Key wallet error" --include="*.rs"Repository: dashpay/rust-dashcore
Length of output: 182
🏁 Script executed:
# Check if there are any explicit feature-gated tests for Slip10/BLS error conversion
grep -r "Slip10\|BLS" key-wallet-manager/ --include="*.rs" -A 2 -B 2Repository: dashpay/rust-dashcore
Length of output: 47
Explicitly match all key_wallet::Error variants and remove the catch-all pattern.
The _ arm with #[allow(unreachable_patterns)] collapses feature-gated variants (Slip10, BLS) and any future upstream additions into InvalidParameter, causing callers and the FFI layer to lose the original error class. Replace the catch-all with explicit arms for Slip10 and BLS (guarded by their respective feature gates), allowing the compiler to enforce updates when key_wallet::Error changes.
🤖 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 95 - 96, Replace the catch-all
`_` arm that maps any key_wallet::Error into WalletError::InvalidParameter (and
remove the #[allow(unreachable_patterns)] attribute) with explicit match arms
for each variant of key_wallet::Error so the original error types are preserved;
specifically add explicit arms for Slip10 and BLS (each behind their respective
feature gates) that convert to the appropriate WalletError variant instead of
collapsing to WalletError::InvalidParameter, so the compiler will force you to
handle new upstream variants of key_wallet::Error.
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
I looked into the Cargo feature unification concern that motivates this split. A few observations:
Feature unification is real but more nuanced than described. Cargo unions features per-crate across a single build invocation. However, with resolver = "2" (which we already use), platform-specific dependencies are resolved per-target — they don't unify across targets.
The split doesn't fully solve the WASM problem either. If a WASM consumer lives in the same workspace alongside dash-spv (which pulls in key-wallet-manager → tokio), then cargo check --workspace will still try to compile tokio for WASM. The split only helps if the WASM crate is built with cargo build -p wasm-crate separately, or lives in a different workspace.
What actually solves it: target-gating tokio. This works whether the crates are split or merged:
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
tokio = { version = "1", features = ["macros", "rt", "sync"] }With resolver 2, this is compiler-enforced — WASM builds simply won't see tokio, even in --workspace builds. No feature discipline required.
Trade-off to consider: The split sacrifices our ability to tighten key-wallet's API surface. Types like AccountTypePreference, TransactionRouter, and AccountCollection's 17 public fields must stay pub for cross-crate access, when they could be pub(crate) if the manager lived inside key-wallet. We discussed this recently and it was a meaningful win.
The split still has value (lighter dep tree for consumers who don't need the manager), but the WASM/tokio argument specifically is better addressed with target-gating regardless of crate structure. Wanted to flag this before we commit to the direction.
|
Updating my position here after dustinface's comment. No, I wasn't aware tokio removal was planned when I weighed in. That's significant context that changes the trade-off. My original argument was mostly about the structural guarantee — crate boundaries preventing tokio from leaking into lightweight/WASM consumers. If tokio is being removed from the picture entirely, that protection becomes theoretical rather than practical. You're defending against a dep that won't exist. What's left in the "pro-split" column:
What's left in the "pro-merged" column:
Revised take: with tokio removal planned and CI hardening for flag combos, keeping the manager inside key-wallet with proper target-gating is the better trade-off. The pub(crate) wins are concrete and immediate; the split's benefits are now mostly theoretical. I got this one wrong. The tie-break should go the other way. |
|
Fair points, and to answer directly: no, I wasn't aware tokio removal from key-wallet-manager was planned. That does shift the weight of the argument. A few thoughts on the specifics: On target-gating: Technically correct that On pub(crate): This is the strongest counterargument. Widening the visibility of On "the flag was fragile because it wasn't done right": That's kind of the point though — feature flags are easy to get wrong, and the fact that it took three commits to sort out (5edf719 → 88eacdf → f92f114) illustrates the failure mode. CI flag combination testing would help, but that's additional infrastructure to build and maintain for a problem that crate boundaries solve structurally. Where I land with the new info: If tokio is genuinely going away from the manager entirely, then the core motivation (preventing tokio from leaking into lightweight consumers) becomes moot. The remaining argument is "crate boundaries prevent future dependency creep" — which is real but speculative, vs the I'm less confident in the split than I was. If the team has conviction that tokio removal is happening soon and the flag testing CI gets built, I could see the merged approach being the right call. But I'd want both of those to actually land, not just be planned. |
…-key-wallet-manager
|
This is not a pure move. I come up with a PR soon. |
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. |
These files were accidentally modified when cherry-picking from the old branch: - dash-spv/tests/dashd_sync/setup.rs: unnecessary peer clearing - dash-spv/Cargo.toml: dropped key-wallet-manager/test-utils feature - fuzz/Cargo.toml: unrelated dep change Reverted to match v0.42-dev. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
managermodule fromkey-walletinto a newkey-wallet-managercratetokioandrayonfromkey-wallet's dependency tree entirelydash-spv,key-wallet-ffi,dash-spv-ffi,fuzz) to import fromkey-wallet-managerdirectlykey_walletfromkey-wallet-managerfor convenienceMotivation
The merged crate approach (PR #503) with feature flags (PR #584) was fragile due to Cargo's feature unification semantics: if any crate in the dependency tree enables the
managerfeature (even transitively),tokiogets pulled in for all consumers. This caused:default-features = falsediscipline that inevitably breaks ("in a few months, we will forget why we have this thing" — Ivan)With separate crates, the boundary is compiler-enforced:
key-walletliterally cannot bring intokiobecause it's not in its dependency tree.What moved
key-wallet/src/manager/(7 files)key-wallet-manager/src/key-wallet/src/test_utils/mock_wallet.rskey-wallet-manager/src/test_utils/Verification
cargo check --workspacepassesMigration for consumers
Lightweight consumers (WASM, embedded, RPC types) just use
key-wallet— no tokio, no feature juggling.Test plan
cargo check --workspacepassescargo test -p key-wallet-manager— 35/35 passcargo test -p key-wallet— all passcargo tree -p key-wallet -e no-devconfirms no tokio🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Breaking Changes
Chores