fix(wallets): prevent duplicate main account labels#607
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes enhance derivation path categorization for wallet accounts by adding explicit BIP32/BIP44 checks in the wallet lifecycle, introducing a shared categorization helper function, delegating categorization logic across modules, and tightening the condition for displaying the "Add Receiving Address" UI to require explicit index 0 instead of treating None as default. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/wallets/account_summary.rs (1)
273-309: Good test coverage for the new categorization logic.The three tests cover the key scenarios:
None-index labeling, legacy-path-overrides-BIP44-reference, and BIP44-path-overrides-BIP32-reference. These directly validate the fix described in the PR objectives.One minor observation: consider adding a test for a genuine BIP44 account-0 path with a correct
BIP44reference to verify it still yields(Bip44, Some(0))— i.e., the happy path isn't broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/account_summary.rs` around lines 273 - 309, Add a unit test that constructs a true BIP44 account-0 derivation path (e.g., hardened 44', hardened 1', hardened 0' followed by normal 0/1) and calls categorize_account_path with DerivationPathReference::BIP44, then assert the result equals (AccountCategory::Bip44, Some(0)) to ensure the happy-path BIP44 classification still works; use the existing test module and helpers (DerivationPath, ChildNumber, categorize_account_path, AccountCategory::Bip44, DerivationPathReference::BIP44) to locate where to add this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/context/wallet_lifecycle.rs`:
- Around line 409-415: The UI and registration use inconsistent BIP44 checks:
wallet lifecycle uses DerivationPath::is_bip44(self.network) while the UI uses
looks_like_bip44(path), causing mismatched classification; choose and implement
a single validation rule and make both callers use it (either tighten
looks_like_bip44 to match is_bip44: check components[0]==Hardened(44),
components[1]==Hardened(coin_type) using network-aware coin_type and len≥4, or
relax is_bip44 and DerivationPath::is_bip44 to only check
components[0]==Hardened(44)), update the implementations in
src/model/wallet/mod.rs (is_bip44) and src/ui/wallets/account_summary.rs
(looks_like_bip44) and ensure derivation_path.is_bip44(self.network) and the UI
call both point to the same canonical helper (extract a shared function if
needed) so registration and UI classify paths identically.
---
Nitpick comments:
In `@src/ui/wallets/account_summary.rs`:
- Around line 273-309: Add a unit test that constructs a true BIP44 account-0
derivation path (e.g., hardened 44', hardened 1', hardened 0' followed by normal
0/1) and calls categorize_account_path with DerivationPathReference::BIP44, then
assert the result equals (AccountCategory::Bip44, Some(0)) to ensure the
happy-path BIP44 classification still works; use the existing test module and
helpers (DerivationPath, ChildNumber, categorize_account_path,
AccountCategory::Bip44, DerivationPathReference::BIP44) to locate where to add
this test.

Summary
m/0'/...paths are treated as BIP32Main AccountTesting
cargo test --lib account_summary -- --nocapturecargo test --test kittest wallets_screen -- --nocaptureThis pull request was created by Codex.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactoring