fix(account-tree-controller): fix duplicate names for out-of-order accounts

packages/account-tree-controller/CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Fix use of unknown `group.metadata.name` when checking for group name uniqueness ([#6706](https://github.com/MetaMask/core/pull/6706))
 - Added logic that prevents an account within a group from being out of order ([#6683](https://github.com/MetaMask/core/pull/6683))
 
 ## [1.1.0]

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

@@ -4154,6 +4154,51 @@ describe('AccountTreeController', () => {
       return controller.state.accountTree.wallets[mockWalletId].groups[groupId];
     };
 
+    it('names all accounts properly even if they are not ordered naturally', () => {
+      const mockHdAccount1 = MOCK_HD_ACCOUNT_1;
+      const mockHdAccount2 = {
+        ...MOCK_HD_ACCOUNT_1,
+        id: 'mock-id-2',
+        address: '0x456',
+        options: {
+          entropy: {
+            ...MOCK_HD_ACCOUNT_1.options.entropy,
+            groupIndex: 1,
+          },
+        },
+      };
+
+      const { controller, mocks } = setup({
+        // We start with 1 account (index 0).
+        accounts: [mockHdAccount1],
+        keyrings: [MOCK_HD_KEYRING_1],
+      });
+
+      controller.init();
+
+      // Then, we insert a second account (index 1), but we re-order it so it appears
+      // before the first account (index 0).
+      mocks.AccountsController.accounts = [mockHdAccount2, mockHdAccount1];
+
+      // Re-init the controller should still give proper naming.
+      controller.init();
+
+      [mockHdAccount1, mockHdAccount2].forEach((mockAccount, index) => {
+        const walletId = toMultichainAccountWalletId(
+          mockAccount.options.entropy.id,
+        );
+        const groupId = toMultichainAccountGroupId(
+          walletId,
+          mockAccount.options.entropy.groupIndex,
+        );
+
+        const mockGroup =
+          controller.state.accountTree.wallets[walletId].groups[groupId];
+        expect(mockGroup).toBeDefined();
+        expect(mockGroup.metadata.name).toBe(`Account ${index + 1}`);
+      });
+    });
+
     it('names non-HD keyrings accounts properly', () => {
       const { controller, messenger } = setup();
 

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

@@ -451,16 +451,22 @@ export class AccountTreeController extends BaseController<
 
       // Parse the highest account index being used (similar to accounts-controller)
       let highestNameIndex = 0;
-      for (const existingGroup of Object.values(
+      for (const { id: otherGroupId } of Object.values(
         wallet.groups,
       ) as AccountGroupObject[]) {
         // Skip the current group being processed
-        if (existingGroup.id === group.id) {
+        if (otherGroupId === groupId) {
           continue;
         }
+
+        // We always get the name from the persisted map, since `init` will clear the
+        // `state.accountTree.wallets`, thus, given empty `group.metadata.name`.
+        // NOTE: If the other group has not been named yet, we just use an empty name.
+        const otherGroupName =
+          state.accountGroupsMetadata[otherGroupId]?.name?.value ?? '';
+
         // Parse the existing group name to extract the numeric index
-        const nameMatch =
-          existingGroup.metadata.name.match(/account\s+(\d+)$/iu);
+        const nameMatch = otherGroupName.match(/account\s+(\d+)$/iu);
         if (nameMatch) {
           const nameIndex = parseInt(nameMatch[1], 10);
           if (nameIndex > highestNameIndex) {
@@ -512,8 +518,8 @@ export class AccountTreeController extends BaseController<
         proposedName;
 
       // Persist the generated name to ensure consistency
-      state.accountGroupsMetadata[group.id] ??= {};
-      state.accountGroupsMetadata[group.id].name = {
+      state.accountGroupsMetadata[groupId] ??= {};
+      state.accountGroupsMetadata[groupId].name = {
         value: proposedName,
         // The `lastUpdatedAt` field is used for backup and sync, when comparing local names
         // with backed up names. In this case, the generated name should never take precedence