fix(account-tree-controller): address group names consistency bug

packages/account-tree-controller/CHANGELOG.md

@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- **CRITICAL**: Fix multi-wallet account naming bug causing duplicate and inconsistent names across wallets
+  - Each wallet now has independent sequential numbering (Account 1, Account 2, etc.)
+  - Names are immediately persisted to ensure consistency across app restarts
+  - Entropy groups use their actual groupIndex instead of broken alphabetical sorting
+  - Prevents issues like "Account 2, Account 2, Account 3..." and missing "Account 1"
+
 ### Changed
 
 - Bump `@metamask/utils` from `^11.4.2` to `^11.8.0` ([#6588](https://github.com/MetaMask/core/pull/6588))

packages/account-tree-controller/src/AccountTreeController.test.ts

@@ -2543,6 +2543,193 @@ describe('AccountTreeController', () => {
       expect(group?.metadata.name).toBe('Account 1');
       expect(group?.metadata.name).not.toBe('Solana Account 2');
     });
+
+    it('ensures consistent per-wallet numbering for multiple SRPs', () => {
+      // This test reproduces a bug scenario where multiple SRPs
+      // showed incorrect numbering like "Account 2, 2, 3, 4..."
+
+      // Setup first SRP with multiple accounts
+      const srp1Keyring: KeyringObject = {
+        ...MOCK_HD_KEYRING_1,
+        metadata: { ...MOCK_HD_KEYRING_1.metadata, id: 'srp1-id' },
+      };
+
+      const srp1Accounts: Bip44Account<InternalAccount>[] = [];
+      for (let i = 0; i < 5; i++) {
+        srp1Accounts.push({
+          ...MOCK_HD_ACCOUNT_1,
+          id: `srp1-account-${i}`,
+          address: `0x1${i}`,
+          metadata: {
+            ...MOCK_HD_ACCOUNT_1.metadata,
+            name: '', // Empty to force default naming
+          },
+          options: {
+            ...MOCK_HD_ACCOUNT_1.options,
+            entropy: {
+              type: 'mnemonic',
+              id: 'srp1-id',
+              derivationPath: `m/44'/60'/${i}'/0/0`,
+              groupIndex: i,
+            },
+          },
+        });
+      }
+
+      // Setup second SRP with multiple accounts
+      const srp2Keyring: KeyringObject = {
+        ...MOCK_HD_KEYRING_2,
+        metadata: { ...MOCK_HD_KEYRING_2.metadata, id: 'srp2-id' },
+      };
+
+      const srp2Accounts: Bip44Account<InternalAccount>[] = [];
+      for (let i = 0; i < 3; i++) {
+        srp2Accounts.push({
+          ...MOCK_HD_ACCOUNT_2,
+          id: `srp2-account-${i}`,
+          address: `0x2${i}`,
+          metadata: {
+            ...MOCK_HD_ACCOUNT_2.metadata,
+            name: '', // Empty to force default naming
+          },
+          options: {
+            ...MOCK_HD_ACCOUNT_2.options,
+            entropy: {
+              type: 'mnemonic',
+              id: 'srp2-id',
+              derivationPath: `m/44'/60'/${i}'/0/0`,
+              groupIndex: i,
+            },
+          },
+        });
+      }
+
+      const { controller } = setup({
+        accounts: [...srp1Accounts, ...srp2Accounts],
+        keyrings: [srp1Keyring, srp2Keyring],
+      });
+
+      controller.init();
+
+      const { state } = controller;
+
+      // Verify first SRP has correct sequential naming
+      const wallet1Id = toMultichainAccountWalletId('srp1-id');
+      const wallet1 = state.accountTree.wallets[wallet1Id];
+
+      expect(wallet1).toBeDefined();
+
+      // Get groups in order by their groupIndex
+      const wallet1Groups = [
+        wallet1.groups[toMultichainAccountGroupId(wallet1Id, 0)],
+        wallet1.groups[toMultichainAccountGroupId(wallet1Id, 1)],
+        wallet1.groups[toMultichainAccountGroupId(wallet1Id, 2)],
+        wallet1.groups[toMultichainAccountGroupId(wallet1Id, 3)],
+        wallet1.groups[toMultichainAccountGroupId(wallet1Id, 4)],
+      ];
+
+      expect(wallet1Groups).toHaveLength(5);
+      expect(wallet1Groups[0].metadata.name).toBe('Account 1');
+      expect(wallet1Groups[1].metadata.name).toBe('Account 2');
+      expect(wallet1Groups[2].metadata.name).toBe('Account 3');
+      expect(wallet1Groups[3].metadata.name).toBe('Account 4');
+      expect(wallet1Groups[4].metadata.name).toBe('Account 5');
+
+      // Verify second SRP ALSO starts from Account 1 (not Account 6!)
+      const wallet2Id = toMultichainAccountWalletId('srp2-id');
+      const wallet2 = state.accountTree.wallets[wallet2Id];
+
+      expect(wallet2).toBeDefined();
+
+      // Get groups in order by their groupIndex
+      const wallet2Groups = [
+        wallet2.groups[toMultichainAccountGroupId(wallet2Id, 0)],
+        wallet2.groups[toMultichainAccountGroupId(wallet2Id, 1)],
+        wallet2.groups[toMultichainAccountGroupId(wallet2Id, 2)],
+      ];
+
+      expect(wallet2Groups).toHaveLength(3);
+      expect(wallet2Groups[0].metadata.name).toBe('Account 1');
+      expect(wallet2Groups[1].metadata.name).toBe('Account 2');
+      expect(wallet2Groups[2].metadata.name).toBe('Account 3');
+
+      // Verify names are persisted
+      const wallet1Group1Id = toMultichainAccountGroupId(wallet1Id, 0);
+      const groupMetadata =
+        controller.state.accountGroupsMetadata[wallet1Group1Id];
+      expect(groupMetadata?.name?.value).toBe('Account 1');
+    });
+
+    it('handles account naming correctly after app restart', () => {
+      // This test verifies that account names remain consistent after restart
+      // and don't change from "Account 1" to "Account 2" etc.
+
+      const { controller, messenger } = setup({
+        accounts: [MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2],
+        keyrings: [MOCK_HD_KEYRING_1],
+      });
+
+      // First init - accounts get named
+      controller.init();
+
+      const walletId = toMultichainAccountWalletId(
+        MOCK_HD_KEYRING_1.metadata.id,
+      );
+      const group1Id = toMultichainAccountGroupId(walletId, 0);
+      const group2Id = toMultichainAccountGroupId(walletId, 1);
+
+      // Check initial names
+      const state1 = controller.state;
+      const wallet1 = state1.accountTree.wallets[walletId];
+      expect(wallet1.groups[group1Id].metadata.name).toBe('Account 1');
+      expect(wallet1.groups[group2Id].metadata.name).toBe('Account 2');
+
+      // Simulate app restart by re-initializing
+      controller.init();
+
+      // Names should remain the same
+      const state2 = controller.state;
+      const wallet2 = state2.accountTree.wallets[walletId];
+      expect(wallet2.groups[group1Id].metadata.name).toBe('Account 1');
+      expect(wallet2.groups[group2Id].metadata.name).toBe('Account 2');
+
+      // Add a new account after restart
+      const newAccount: Bip44Account<InternalAccount> = {
+        ...MOCK_HD_ACCOUNT_1,
+        id: 'new-account',
+        address: '0xNEW',
+        metadata: {
+          ...MOCK_HD_ACCOUNT_1.metadata,
+          name: '', // Empty to force default naming
+        },
+        options: {
+          ...MOCK_HD_ACCOUNT_1.options,
+          entropy: {
+            type: 'mnemonic',
+            id: MOCK_HD_KEYRING_1.metadata.id,
+            derivationPath: "m/44'/60'/2'/0/0",
+            groupIndex: 2,
+          },
+        },
+      };
+
+      messenger.publish('AccountsController:accountAdded', newAccount);
+
+      // New account should get Account 3, not duplicate an existing name
+      const group3Id = toMultichainAccountGroupId(walletId, 2);
+      const state3 = controller.state;
+      const wallet3 = state3.accountTree.wallets[walletId];
+      expect(wallet3.groups[group3Id].metadata.name).toBe('Account 3');
+
+      // All names should be different
+      const allNames = [
+        wallet3.groups[group1Id].metadata.name,
+        wallet3.groups[group2Id].metadata.name,
+        wallet3.groups[group3Id].metadata.name,
+      ];
+      const uniqueNames = new Set(allNames);
+      expect(uniqueNames.size).toBe(3); // All names should be unique
+    });
   });
 
   describe('actions', () => {

packages/account-tree-controller/src/AccountTreeController.ts

@@ -394,47 +394,99 @@
     wallet: AccountWalletObject,
     group: AccountGroupObject,
   ) {
-    const persistedMetadata = this.state.accountGroupsMetadata[group.id];
+    const persistedGroupMetadata = this.state.accountGroupsMetadata[group.id];
 
     // Apply persisted name if available (including empty strings)
-    if (persistedMetadata?.name !== undefined) {
-      group.metadata.name = persistedMetadata.name.value;
+    if (persistedGroupMetadata?.name !== undefined) {
+      group.metadata.name = persistedGroupMetadata.name.value;
     } else if (!group.metadata.name) {
       // Get the appropriate rule for this wallet type
       const rule = this.#getRuleForWallet(wallet);
       const typedWallet = wallet as AccountWalletObjectOf<typeof wallet.type>;
       const typedGroup = typedWallet.groups[group.id] as AccountGroupObject;
 
-      // Calculate group index based on position within sorted group IDs
-      // We sort to ensure consistent ordering across all wallet types:
-      // - Entropy: group IDs like "entropy:abc/0", "entropy:abc/1" sort to logical order
-      // - Snap/Keyring: group IDs like "keyring:ledger/0xABC" get consistent alphabetical order
-      const sortedGroupIds = Object.keys(wallet.groups).sort();
-      let groupIndex = sortedGroupIds.indexOf(group.id);
-
-      // Defensive fallback: if group.id is not found in sortedGroupIds (should never happen
-      // in normal operation since we iterate over wallet.groups), use index 0 to prevent
-      // passing -1 to getDefaultAccountGroupName which would result in "Account 0"
-      /* istanbul ignore next */
-      if (groupIndex === -1) {
-        groupIndex = 0;
+      // Try to get computed name first (e.g., from existing EVM account names)
+      const computedName = rule.getComputedAccountGroupName(typedGroup);
+
+      if (computedName) {
+        group.metadata.name = computedName;
+      } else {
+        // Generate default name based on wallet-specific numbering
+        group.metadata.name = this.#generateDefaultGroupName(
+          wallet,
+          group,
+          rule,
+        );
       }
 
-      // Use computed name first, then fallback to default naming if empty
-      group.metadata.name =
-        rule.getComputedAccountGroupName(typedGroup) ||
-        rule.getDefaultAccountGroupName(groupIndex);
+      // Persist the assigned name to ensure consistency across restarts
+      // This prevents the name from changing on subsequent inits
+      this.update((state) => {
+        if (!state.accountGroupsMetadata[group.id]) {
+          state.accountGroupsMetadata[group.id] = {};
+        }
+        state.accountGroupsMetadata[group.id].name = {
+          value: group.metadata.name,
+          lastUpdatedAt: Date.now(),
+        };
+      });
     }
 
     // Apply persisted UI states
-    if (persistedMetadata?.pinned?.value !== undefined) {
-      group.metadata.pinned = persistedMetadata.pinned.value;
+    if (persistedGroupMetadata?.pinned?.value !== undefined) {
+      group.metadata.pinned = persistedGroupMetadata.pinned.value;
     }
-    if (persistedMetadata?.hidden?.value !== undefined) {
-      group.metadata.hidden = persistedMetadata.hidden.value;
+    if (persistedGroupMetadata?.hidden?.value !== undefined) {
+      group.metadata.hidden = persistedGroupMetadata.hidden.value;
     }
   }
 
+  /**
+   * Generates a default name for an account group, ensuring unique numbering
+   * within each wallet.
+   *
+   * @param wallet - The wallet containing the group.
+   * @param group - The group to generate a name for.
+   * @param rule - The rule for this wallet type.
+   * @returns The generated default name.
+   */
+  #generateDefaultGroupName(
+    wallet: AccountWalletObject,
+    group: AccountGroupObject,
+    rule: Rule<AccountWalletType, AccountGroupType>,
+  ): string {
+    // For entropy groups, use the groupIndex from metadata for consistent numbering
+    if (group.type === 'multichain-account' && 'entropy' in group.metadata) {
+      return rule.getDefaultAccountGroupName(group.metadata.entropy.groupIndex);
+    }
+
+    // For other types, use sequential numbering within the wallet
+    // Count existing groups to determine the next number
+    const existingNumbers = new Set<number>();
+
+      // Collect all numbers currently in use by this wallet's groups
+      for (const existingGroup of Object.values(wallet.groups)) {
+        const { id } = existingGroup;
+        const { name: persistedName } = this.state.accountGroupsMetadata[id] || {};
+        const existingName = persistedName?.value || existingGroup.metadata.name;
+
+        // Extract number from "Account X" pattern
+        const match = existingName.match(/^Account (\d+)$/);
+        if (match) {
+          const [, numberStr] = match;
+          existingNumbers.add(parseInt(numberStr, 10) - 1); // Convert to 0-based index
+        }
+      }
+
+    // Find the first available number (0-based)
+    let accountNumber = 0;
+    while (existingNumbers.has(accountNumber)) {
+      accountNumber++;
+    }
+
+    return rule.getDefaultAccountGroupName(accountNumber);
+  }
+
   /**
    * Gets the account wallet object from its ID.
    *

packages/account-tree-controller/src/wallet.ts

@@ -23,13 +23,17 @@ import type { UpdatableField, ExtractFieldValues } from './type-utils.js';
 export type AccountTreeWalletPersistedMetadata = {
   /** Custom name set by user, overrides default naming logic */
   name?: UpdatableField<string>;
+  /** Next account number to use for default naming (Account 1, Account 2, etc.) */
+  nextAccountNumber?: number;
 };
 
 /**
  * Tree metadata for account wallets (required plain values extracted from persisted metadata).
  */
 export type AccountTreeWalletMetadata = Required<
-  ExtractFieldValues<AccountTreeWalletPersistedMetadata>
+  ExtractFieldValues<
+    Omit<AccountTreeWalletPersistedMetadata, 'nextAccountNumber'>
+  >
 >;
 
 /**