fix(account-tree-controller): address group names consistency bug #6601
+984
−87
Conversation
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
CRITICAL FIX for account group naming inconsistencies across multiple wallets. Issues Fixed: - Duplicate account names (Account 2, Account 2, Account 3...) - Missing Account 1 in some wallets - Non-sequential numbering and random order - Second SRP incorrectly showing 'Account 3' instead of 'Account 1' Root Cause: The alphabetical sorting of group IDs was fundamentally broken for multi-wallet scenarios. Different wallets would conflict and produce inconsistent, duplicate names. Solution: 1. For entropy groups: Use the actual groupIndex from metadata 2. Persist assigned names immediately to ensure consistency 3. Track used numbers per wallet to avoid duplicates 4. Each wallet now has independent sequential numbering Changes: - Removed broken alphabetical sorting logic - Implemented proper per-wallet numbering - Added name persistence on first assignment - Added comprehensive tests for multi-wallet scenarios - Fixed wallet metadata types This ensures each wallet has its own 'Account 1', 'Account 2', etc., and names remain consistent across app restarts.
… issues - Use object destructuring for better code quality - Remove conditionals in tests by using deterministic array access - Simplify test assertions to avoid optional chaining in tests
- Restore import from @metamask/account-api instead of defining locally - Remove unnecessary local type definition - Keep only the changes needed for the naming bug fix
Issues Fixed: 1. Global indexing across wallets - now each wallet has independent numbering 2. Lost EVM account names - now preserves computed names like 'Main Account' Changes: - Use per-wallet sequential numbering for ALL wallet types instead of global entropy.groupIndex - Only persist default names, not computed names from existing EVM accounts - Each wallet now correctly shows: Account 1, Account 2, Account 3... - Preserves original EVM account names when they exist
ccharly
reviewed
Sep 15, 2025
Root Cause of Extension Crash: - Nested state updates during tree building caused state corruption - Over-aggressive name persistence broke existing functionality Solution: - Reverted to simpler approach: use entropy.groupIndex for consistent naming - Removed complex per-wallet tracking and nested state updates - Preserved EVM account names while fixing the core alphabetical sorting bug - Fixed test to use empty account names for proper default naming behavior Key Changes: - Use group.metadata.entropy.groupIndex instead of alphabetical sort position - No automatic name persistence (avoids state corruption during init) - Preserves existing behavior while fixing the core naming inconsistency - Each entropy group gets correct sequential numbering based on its groupIndex This fixes the multi-wallet naming bug without breaking extension startup.
fabiobozzo
added a commit
that referenced
this pull request
Sep 15, 2025
Clarified the changelog entry to include a reference to the related pull request (#6601) that addresses the account group naming inconsistency caused by using alphabetical sorting instead of the actual groupIndex. This ensures accurate sequential numbering across wallets.
- Fix prettier formatting for import statement - Import AccountGroupType as value instead of type to use in comparison - Ensure proper enum comparison instead of string literal
|
@ccharly I've simplified my approach, please re-review 🙏 |
- Add proper PR reference to the critical multi-wallet naming bug fix - Ensures changelog check passes with proper documentation
- Add istanbul ignore comment for the defensive fallback that should never be reached - This brings coverage back to 100% for all metrics (statements, branches, lines) - The fallback handles edge cases where group.id is not found in sortedGroupIds
- Move 'Changed' section before 'Fixed' section as per Keep a Changelog standard - Ensures changelog validation passes with proper section ordering
ccharly
reviewed
Sep 15, 2025
fabiobozzo
changed the title
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
ccharly
reviewed
Sep 15, 2025
ccharly
reviewed
Sep 19, 2025
…lication - Update the loop to iterate over state.accountTree.wallets instead of wallets variable. - Ensures consistency in accessing wallet data during metadata application.
ccharly
reviewed
Sep 19, 2025
ccharly
reviewed
Sep 19, 2025
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Skip computed names to fix global numbering bug from accounts-controller - All accounts now use proper per-wallet sequential naming (Account 1, 2, 3...) - Maintain 100% test coverage and implement all code review feedback - Phase 1 fix - computed names will be re-added in future PR with proper migration
ccharly
reviewed
Sep 19, 2025
…untGroupsMetadata
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
ccharly
reviewed
Sep 19, 2025
ccharly
reviewed
Sep 19, 2025
ccharly
reviewed
Sep 19, 2025
Comment on lines
+1115
to
+1118
| /* istanbul ignore next */ | ||
| if (!state.accountGroupsMetadata[groupId]) { | ||
| state.accountGroupsMetadata[groupId] = {}; | ||
| } |
There was a problem hiding this comment.
This is ok for now, we can refactor that later.
|
@metamaskbot publish-preview |
ccharly
approved these changes
Sep 19, 2025
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| } | ||
|
|
||
| // Use the higher of the two: highest parsed index or computed index | ||
| proposedNameIndex = Math.max(highestAccountNameIndex, proposedNameIndex); |
There was a problem hiding this comment.
Bug: Account Group Naming Inconsistency
The default naming for account groups, especially entropy-based ones, is inconsistent. The logic incorrectly uses Math.max to determine the starting index for new group names, which can lead to non-sequential names (e.g., 'Account 3' instead of 'Account 1' for the first account) and prevents entropy groups from consistently mapping their groupIndex to Account N+1.
Additional Locations (1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Explanation
CRITICAL FIX: Resolves a severe multi-wallet account naming bug that caused duplicate, inconsistent, and confusing account group names across different seed phrases (SRPs).
Current Problem
Users importing multiple seed phrases experienced catastrophic naming issues:
Root Cause
The account group naming logic had critical flaws:
Object.keys(wallet.groups).sort()which caused conflicts and duplicates in multi-wallet scenariosgroup.metadata.entropy.groupIndexwithout null checks-1index, causing invalid "Account 0" namesSolution
Implemented a robust fix with comprehensive improvements:
whileloop that finds next available unique name, handles user renames, and ensures unique sequential namesgroupIndexfrom entropy metadata when available with safe null checkswallet.groups.lengthto minimize iterations and improve efficiencyautoHandleConflictparameter tosetAccountGroupName()for automated conflict resolution with suffix generation ("Account 1 (2)","Account 1 (3)", etc.) - ready for Backup & Sync integrationReferences
Fixes:
Checklist