feat(key-wallet-manager): add mutable-pair accessors + insert_wallet#685
Conversation
Adds three accessors on WalletManager that exploit the split borrow between the independent `wallets` and `wallet_infos` maps: - `get_wallet_and_info_mut` — `(&Wallet, &mut T)` for mutations that read wallet config while writing info (UTXOs, balance, tx history). - `get_wallet_mut_and_info_mut` — `(&mut Wallet, &mut T)` for cases that need to mutate the wallet itself (e.g. idempotent re-derivation of HD accounts during changeset replay). - `insert_wallet` — register a pre-built `(Wallet, T)` pair, returns `WalletExists` on duplicate and bumps `structural_revision` on success. Enables restore paths that reconstruct wallets from persistence without going through the mnemonic-based builder. Unit tests cover dedup error, split-borrow mutation across both maps, and the monitor-revision bump on insert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThree new methods are added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
key-wallet-manager/src/accessors.rs (2)
316-337: Nit: assertion comment doesn't match the assertion.The inline comment says "the delta must be >= 1", but the
assert!uses a strict>, i.e. it requires delta> info_revision(equivalent to structural delta>= 1). The assertion is correct — just the comment is slightly misleading. Consider rewording to "so the structural delta must be >= 1, i.e.after > before + info_revision."🤖 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 316 - 337, The inline comment in the test function insert_wallet_bumps_monitor_revision is misleading: it says "the delta must be >= 1" but the assertion uses a strict greater-than (after > before + info_revision); update the comment near manager.monitor_revision() / info.monitor_revision() to state "so the structural delta must be >= 1, i.e. `after > before + info_revision`" (or similar wording) so the comment matches the assertion semantics.
63-72: Optional: collapse duplicate lookup viaEntryAPI.
contains_keyfollowed byinsertperforms two B-tree traversals for the common (non-duplicate) case. UsingBTreeMap::entrymakes the uniqueness check and insertion a single traversal and expresses intent more directly.♻️ Proposed refactor
pub fn insert_wallet(&mut self, wallet: Wallet, info: T) -> Result<WalletId, WalletError> { let wallet_id = wallet.compute_wallet_id(); - if self.wallets.contains_key(&wallet_id) { - return Err(WalletError::WalletExists(wallet_id)); - } - self.wallets.insert(wallet_id, wallet); + match self.wallets.entry(wallet_id) { + std::collections::btree_map::Entry::Occupied(_) => { + return Err(WalletError::WalletExists(wallet_id)); + } + std::collections::btree_map::Entry::Vacant(e) => { + e.insert(wallet); + } + } self.wallet_infos.insert(wallet_id, info); self.bump_structural_revision(); Ok(wallet_id) }Also worth noting: this method only guards against collisions in
self.wallets. If — due to an earlier bug or partial failure —wallet_infosalready contained an entry forwallet_idwhilewalletsdid not, the old info would be silently overwritten. In practice the two maps are expected to stay in lockstep, but adebug_assert!(!self.wallet_infos.contains_key(&wallet_id))before thewallet_infos.insertwould make that invariant explicit.🤖 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 63 - 72, Replace the two-step contains_key + insert in insert_wallet with a single BTreeMap::entry operation on self.wallets to perform uniqueness check and insert in one traversal (refer to insert_wallet, WalletId, WalletError, self.wallets); after a successful insertion, add debug_assert! to ensure self.wallet_infos does not already contain the same wallet_id before inserting into self.wallet_infos and then call bump_structural_revision() and return Ok(wallet_id).
🤖 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/src/accessors.rs`:
- Around line 316-337: The inline comment in the test function
insert_wallet_bumps_monitor_revision is misleading: it says "the delta must be
>= 1" but the assertion uses a strict greater-than (after > before +
info_revision); update the comment near manager.monitor_revision() /
info.monitor_revision() to state "so the structural delta must be >= 1, i.e.
`after > before + info_revision`" (or similar wording) so the comment matches
the assertion semantics.
- Around line 63-72: Replace the two-step contains_key + insert in insert_wallet
with a single BTreeMap::entry operation on self.wallets to perform uniqueness
check and insert in one traversal (refer to insert_wallet, WalletId,
WalletError, self.wallets); after a successful insertion, add debug_assert! to
ensure self.wallet_infos does not already contain the same wallet_id before
inserting into self.wallet_infos and then call bump_structural_revision() and
return Ok(wallet_id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5474659-a423-4dc1-a0df-ad2723c6e80a
📒 Files selected for processing (1)
key-wallet-manager/src/accessors.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #685 +/- ##
=============================================
- Coverage 70.22% 70.21% -0.01%
=============================================
Files 319 319
Lines 66686 66937 +251
=============================================
+ Hits 46830 47003 +173
- Misses 19856 19934 +78
|
Per ZocoLini's review on #685: the type system already proves the mutable-borrow accessors return the right shape, and monitor_revision is covered elsewhere. Keep insert_wallet_rejects_duplicate since it exercises actual error-path logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
dismissed stale reviews from xdustinface and coderabbitai[bot]
via
503e41a
3 participants
Summary
Adds three accessors on
WalletManagerinkey-wallet-manager/src/accessors.rsthat exploit the split borrow between the independentwallets: BTreeMap<WalletId, Wallet>andwallet_infos: BTreeMap<WalletId, T>maps:get_wallet_and_info_mut(&mut self, id) -> Option<(&Wallet, &mut T)>— for the common mutation pattern that reads wallet config (accounts, xpub) while writing info (UTXOs, balance, tx history).get_wallet_mut_and_info_mut(&mut self, id) -> Option<(&mut Wallet, &mut T)>— needed when theWalletitself mutates (e.g.Wallet::add_accountduring changeset replay / on-the-fly HD account derivation).insert_wallet(&mut self, wallet, info) -> Result<WalletId, WalletError>— registers a pre-built(Wallet, T)pair. Errors withWalletError::WalletExistson duplicate and callsbump_structural_revision()on success. Enables restore paths that reconstruct a wallet from persistence without going through the mnemonic-based builder.No
unsafe, no refactor, no new error variants —WalletError::WalletExists,Wallet::compute_wallet_id(), andWalletManager::bump_structural_revision()already exist on this branch.Test plan
cargo fmt --all --checkcleancargo clippy -p key-wallet-manager --all-targets -- -D warningscleancargo test -p key-wallet-manager— 40 tests pass including 4 new accessor tests:insert_wallet_rejects_duplicate— dedup returnsWalletExists(id)get_wallet_and_info_mut_returns_none_then_some— None pre-insert, Some post-insert, mutation through&mut Tpersistsget_wallet_mut_and_info_mut_allows_split_borrow— compile-time proof that&mut Wallet(viaget_bip44_account_mut) and&mut T(viaset_name) coexist in one borrow scopeinsert_wallet_bumps_monitor_revision—monitor_revision()strictly increases pastbefore + info.monitor_revision()🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests