Merge branch 'main' into release/575.0.0

Author: chaitanyapotti

packages/account-tree-controller/CHANGELOG.md

@@ -7,10 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Add new group naming for non-HD keyring accounts ([#6679](https://github.com/MetaMask/core/pull/6679))
+  - Hardware-wallet account groups are now named: "Ledger|Trezor|QR|Lattice|OneKey Account N".
+  - Private key account groups are now named: "Imported Account N".
+  - Snap account groups are now named: "Snap Account N".
+- Account group names now use natural indexing as a fallback ([#6677](https://github.com/MetaMask/core/pull/6677)), ([#6679](https://github.com/MetaMask/core/pull/6679))
+  - If a user names his accounts without any indexes, we would just use the number of accounts to compute the next available index.
+
 ### Fixed
 
-- Fix group naming for PK/Hardware accounts ([#6677](https://github.com/MetaMask/core/pull/6677))
-  - Previously, the first PK/Hardware account would start as `Account 2` as opposed to `Account 1` and thus subsequent group names were off as well.
+- Fix group naming for non-HD keyring accounts ([#6677](https://github.com/MetaMask/core/pull/6677)), ([#6679](https://github.com/MetaMask/core/pull/6679))
+  - Previously, the first non-HD keyring account would start as `Account 2` as opposed to `Account 1` and thus subsequent group names were off as well.
 
 ## [1.0.0]
 

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

@@ -587,7 +587,7 @@ describe('AccountTreeController', () => {
                   type: AccountGroupType.SingleAccount,
                   accounts: [MOCK_SNAP_ACCOUNT_2.id],
                   metadata: {
-                    name: 'Account 1', // Updated: per-wallet numbering (different wallet)
+                    name: 'Snap Account 1', // Updated: per-wallet numbering (different wallet)
                     pinned: false,
                     hidden: false,
                   },
@@ -610,7 +610,7 @@ describe('AccountTreeController', () => {
                   type: AccountGroupType.SingleAccount,
                   accounts: [MOCK_HARDWARE_ACCOUNT_1.id],
                   metadata: {
-                    name: 'Account 1', // Updated: per-wallet numbering (different wallet)
+                    name: 'Ledger Account 1', // Updated: per-wallet numbering (different wallet)
                     pinned: false,
                     hidden: false,
                   },
@@ -652,13 +652,13 @@ describe('AccountTreeController', () => {
           },
           [expectedKeyringWalletIdGroup]: {
             name: {
-              value: 'Account 1', // Updated: per-wallet numbering (different wallet)
+              value: 'Ledger Account 1', // Updated: per-wallet numbering (different wallet)
               lastUpdatedAt: expect.any(Number),
             },
           },
           [expectedSnapWalletIdGroup]: {
             name: {
-              value: 'Account 1', // Updated: per-wallet numbering (different wallet)
+              value: 'Snap Account 1', // Updated: per-wallet numbering (different wallet)
               lastUpdatedAt: expect.any(Number),
             },
           },
@@ -2902,9 +2902,9 @@ describe('AccountTreeController', () => {
       // Critical assertion: should have 2 unique names (no duplicates)
       expect(uniqueNames.size).toBe(2);
 
-      // Due to optimization, names start at wallet.length, so we get "Account 1" and "Account 2"
-      expect(allNames).toContain('Account 1');
-      expect(allNames).toContain('Account 2');
+      // Due to optimization, names start at wallet.length, so we get "Account 3" and "Account 4"
+      expect(allNames).toContain('Ledger Account 1');
+      expect(allNames).toContain('Ledger Account 2');
 
       // Verify they're actually different
       expect(group1.metadata.name).not.toBe(group2.metadata.name);
@@ -4033,4 +4033,125 @@ describe('AccountTreeController', () => {
       ).toBe('Conflict Name (2)');
     });
   });
+
+  describe('naming', () => {
+    const mockAccount1 = {
+      ...MOCK_HARDWARE_ACCOUNT_1,
+      id: 'mock-id-1',
+      address: '0x123',
+    };
+    const mockAccount2 = {
+      ...MOCK_HARDWARE_ACCOUNT_1,
+      id: 'mock-id-2',
+      address: '0x456',
+    };
+    const mockAccount3 = {
+      ...MOCK_HARDWARE_ACCOUNT_1,
+      id: 'mock-id-3',
+      address: '0x789',
+    };
+    const mockAccount4 = {
+      ...MOCK_HARDWARE_ACCOUNT_1,
+      id: 'mock-id-4',
+      address: '0xabc',
+    };
+
+    const mockWalletId = toAccountWalletId(
+      AccountWalletType.Keyring,
+      KeyringTypes.ledger,
+    );
+
+    const getAccountGroupFromAccount = (
+      controller: AccountTreeController,
+      mockAccount: InternalAccount,
+    ) => {
+      const groupId = toAccountGroupId(mockWalletId, mockAccount.address);
+      return controller.state.accountTree.wallets[mockWalletId].groups[groupId];
+    };
+
+    it('names non-HD keyrings accounts properly', () => {
+      const { controller, messenger } = setup();
+
+      // Add all 3 accounts.
+      [mockAccount1, mockAccount2, mockAccount3].forEach(
+        (mockAccount, index) => {
+          messenger.publish('AccountsController:accountAdded', mockAccount);
+
+          const mockGroup = getAccountGroupFromAccount(controller, mockAccount);
+          expect(mockGroup).toBeDefined();
+          expect(mockGroup.metadata.name).toBe(`Ledger Account ${index + 1}`);
+        },
+      );
+
+      // Remove account 2, should still create account 4 afterward.
+      messenger.publish('AccountsController:accountRemoved', mockAccount2.id);
+
+      expect(
+        getAccountGroupFromAccount(controller, mockAccount4),
+      ).toBeUndefined();
+      messenger.publish('AccountsController:accountAdded', mockAccount4);
+
+      const mockGroup4 = getAccountGroupFromAccount(controller, mockAccount4);
+      expect(mockGroup4).toBeDefined();
+      expect(mockGroup4.metadata.name).toBe('Ledger Account 4');
+
+      // Now, removing account 3 and 4, should defaults to an index of "2" (since only
+      // account 1 remains), thus, re-inserting account 2, should be named "* Account 2".
+      messenger.publish('AccountsController:accountRemoved', mockAccount4.id);
+      messenger.publish('AccountsController:accountRemoved', mockAccount3.id);
+
+      expect(
+        getAccountGroupFromAccount(controller, mockAccount2),
+      ).toBeUndefined();
+      messenger.publish('AccountsController:accountAdded', mockAccount2);
+
+      const mockGroup2 = getAccountGroupFromAccount(controller, mockAccount2);
+      expect(mockGroup2).toBeDefined();
+      expect(mockGroup2.metadata.name).toBe('Ledger Account 2');
+    });
+
+    it('uses natural indexing for pre-existing accounts', () => {
+      const { controller } = setup({
+        accounts: [mockAccount1, mockAccount2, mockAccount3],
+      });
+
+      controller.init();
+
+      // After initializing the controller, all accounts should be named appropriately.
+      [mockAccount1, mockAccount2, mockAccount3].forEach(
+        (mockAccount, index) => {
+          const mockGroup = getAccountGroupFromAccount(controller, mockAccount);
+          expect(mockGroup).toBeDefined();
+          expect(mockGroup.metadata.name).toBe(`Ledger Account ${index + 1}`);
+        },
+      );
+    });
+
+    it('fallbacks to natural indexing if group names are not using our default name pattern', () => {
+      const { controller, messenger } = setup();
+
+      [mockAccount1, mockAccount2, mockAccount3].forEach((mockAccount) =>
+        messenger.publish('AccountsController:accountAdded', mockAccount),
+      );
+
+      const mockGroup1 = getAccountGroupFromAccount(controller, mockAccount1);
+      const mockGroup2 = getAccountGroupFromAccount(controller, mockAccount2);
+      const mockGroup3 = getAccountGroupFromAccount(controller, mockAccount3);
+      expect(mockGroup1).toBeDefined();
+      expect(mockGroup2).toBeDefined();
+      expect(mockGroup3).toBeDefined();
+
+      // Rename all accounts to something different than "* Account <index>".
+      controller.setAccountGroupName(mockGroup1.id, 'Account A');
+      controller.setAccountGroupName(mockGroup2.id, 'The next account');
+      controller.setAccountGroupName(mockGroup3.id, 'Best account so far');
+
+      // Adding a new account should not reset back to "Account 1", but it should
+      // use the next natural index, here, "Account 4".
+      messenger.publish('AccountsController:accountAdded', mockAccount4);
+      const mockGroup4 = getAccountGroupFromAccount(controller, mockAccount4);
+      expect(mockGroup4).toBeDefined();
+      expect(mockGroup4.metadata.name).toBe('Ledger Account 4');
+    });
+  });
 });

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

@@ -1,13 +1,10 @@
-import {
-  AccountWalletType,
-  AccountGroupType,
-  select,
-} from '@metamask/account-api';
+import { AccountWalletType, select } from '@metamask/account-api';
 import type {
   AccountGroupId,
   AccountWalletId,
   AccountSelector,
   MultichainAccountWalletId,
+  AccountGroupType,
 } from '@metamask/account-api';
 import type { MultichainAccountWalletStatus } from '@metamask/account-api';
 import { type AccountId } from '@metamask/accounts-controller';
@@ -259,23 +256,35 @@ export class AccountTreeController extends BaseController<
 
     // Once we have the account tree, we can apply persisted metadata (names + UI states).
     let previousSelectedAccountGroupStillExists = false;
-    for (const wallet of Object.values(wallets)) {
-      this.#applyAccountWalletMetadata(wallet);
-
-      for (const group of Object.values(wallet.groups)) {
-        if (group.id === previousSelectedAccountGroup) {
-          previousSelectedAccountGroupStillExists = true;
-        }
-      }
-    }
-
     this.update((state) => {
       state.accountTree.wallets = wallets;
 
       // Apply group metadata within the state update
       for (const wallet of Object.values(state.accountTree.wallets)) {
+        this.#applyAccountWalletMetadata(state, wallet.id);
+
+        // Used for default group default names (so we use human-indexing here).
+        let nextNaturalNameIndex = 1;
         for (const group of Object.values(wallet.groups)) {
-          this.#applyAccountGroupMetadata(state, wallet.id, group.id);
+          this.#applyAccountGroupMetadata(
+            state,
+            wallet.id,
+            group.id,
+            // FIXME: We should not need this kind of logic if we were not inserting accounts
+            // 1 by 1. Instead, we should be inserting wallets and groups directly. This would
+            // allow us to naturally insert a group in the tree AND update its metadata right
+            // away...
+            // But here, we have to wait for the entire group to be ready before updating
+            // its metadata (mainly because we're dealing with single accounts rather than entire
+            // groups).
+            // That is why we need this kind of extra parameter.
+            nextNaturalNameIndex,
+          );
+
+          if (group.id === previousSelectedAccountGroup) {
+            previousSelectedAccountGroupStillExists = true;
+          }
+          nextNaturalNameIndex += 1;
         }
       }
 
@@ -342,10 +351,15 @@ export class AccountTreeController extends BaseController<
    * first, and then fallbacks to default values (based on the wallet's
    * type).
    *
-   * @param wallet Account wallet object to update.
+   * @param state Controller state to update for persistence.
+   * @param walletId The wallet ID to update.
    */
-  #applyAccountWalletMetadata(wallet: AccountWalletObject) {
-    const persistedMetadata = this.state.accountWalletsMetadata[wallet.id];
+  #applyAccountWalletMetadata(
+    state: AccountTreeControllerState,
+    walletId: AccountWalletId,
+  ) {
+    const wallet = state.accountTree.wallets[walletId];
+    const persistedMetadata = state.accountWalletsMetadata[walletId];
 
     // Apply persisted name if available (including empty strings)
     if (persistedMetadata?.name !== undefined) {
@@ -402,11 +416,13 @@ export class AccountTreeController extends BaseController<
    * @param state Controller state to update for persistence.
    * @param walletId The wallet ID containing the group.
    * @param groupId The account group ID to update.
+   * @param nextNaturalNameIndex The next natural name index for this group (only used for default names).
    */
   #applyAccountGroupMetadata(
     state: AccountTreeControllerState,
     walletId: AccountWalletId,
     groupId: AccountGroupId,
+    nextNaturalNameIndex?: number,
   ) {
     const wallet = state.accountTree.wallets[walletId];
     const group = wallet.groups[groupId];
@@ -420,15 +436,14 @@ export class AccountTreeController extends BaseController<
       // Get the appropriate rule for this wallet type
       const rule = this.#getRuleForWallet(wallet);
 
+      // Get the prefix for groups of this wallet
+      const namePrefix = rule.getDefaultAccountGroupPrefix(wallet);
+
       // Skip computed names for now - use default naming with per-wallet logic
       // TODO: Implement computed names in a future iteration
 
-      // Generate default name and ensure it's unique within the wallet
-      let proposedName = '';
-      let proposedNameIndex: number;
-
       // Parse the highest account index being used (similar to accounts-controller)
-      let highestAccountNameIndex = 0;
+      let highestNameIndex = 0;
       for (const existingGroup of Object.values(
         wallet.groups,
       ) as AccountGroupObject[]) {
@@ -437,50 +452,53 @@ export class AccountTreeController extends BaseController<
           continue;
         }
         // Parse the existing group name to extract the numeric index
-        // TODO: This regex only matches "Account N" pattern. Hardware wallets (Trezor, Ledger, etc.)
-        // use different patterns like "Trezor N", "Ledger N" per keyringTypeToName().
-        // We'll enhance this to handle all keyring types in a future iteration.
         const nameMatch = existingGroup.metadata.name.match(/Account (\d+)$/u);
         if (nameMatch) {
           const nameIndex = parseInt(nameMatch[1], 10);
-          if (nameIndex > highestAccountNameIndex) {
-            highestAccountNameIndex = nameIndex;
+          if (nameIndex > highestNameIndex) {
+            highestNameIndex = nameIndex;
           }
         }
       }
 
-      // For entropy-based multichain groups, start with the actual groupIndex
-      if (
-        group.type === AccountGroupType.MultichainAccount &&
-        group.metadata.entropy
-      ) {
-        proposedNameIndex = group.metadata.entropy.groupIndex;
-      } else {
-        // For other wallet types, start with the number of existing groups
-        // This gives us the next logical sequential number
-        proposedNameIndex = Object.keys(wallet.groups).length - 1;
-      }
-
-      // Use the higher of the two: highest parsed index or computed index
-      proposedNameIndex = Math.min(highestAccountNameIndex, proposedNameIndex);
+      // We just use the highest known index no matter the wallet type.
+      //
+      // For entropy-based wallets (bip44), if a multichain account group with group index 1
+      // is inserted before another one with group index 0, then the naming will be:
+      // - "Account 1" (group index 1)
+      // - "Account 2" (group index 0)
+      // This naming makes more sense for the end-user.
+      //
+      // For other type of wallets, since those wallets can create arbitrary gaps, we still
+      // rely on the highest know index to avoid back-filling account with "old names".
+      let proposedNameIndex = Math.max(
+        // Use + 1 to use the next available index.
+        highestNameIndex + 1,
+        // In case all accounts have been renamed differently than the usual "Account <index>"
+        // pattern, we want to use the next "natural" index, which is just the number of groups
+        // in that wallet (e.g. ["Account A", "Another Account"], next natural index would be
+        // "Account 3" in this case).
+        nextNaturalNameIndex ?? Object.keys(wallet.groups).length,
+      );
 
       // Find a unique name by checking for conflicts and incrementing if needed
-      let nameExists: boolean;
+      let proposedNameExists: boolean;
+      let proposedName = '';
       do {
-        proposedName = rule.getDefaultAccountGroupName(proposedNameIndex);
+        proposedName = `${namePrefix} ${proposedNameIndex}`;
 
         // Check if this name already exists in the wallet (excluding current group)
-        nameExists = !isAccountGroupNameUniqueFromWallet(
+        proposedNameExists = !isAccountGroupNameUniqueFromWallet(
           wallet,
           group.id,
           proposedName,
         );
 
         /* istanbul ignore next */
-        if (nameExists) {
+        if (proposedNameExists) {
           proposedNameIndex += 1; // Try next number
         }
-      } while (nameExists);
+      } while (proposedNameExists);
 
       state.accountTree.wallets[walletId].groups[groupId].metadata.name =
         proposedName;
@@ -608,7 +626,7 @@ export class AccountTreeController extends BaseController<
 
         const wallet = state.accountTree.wallets[walletId];
         if (wallet) {
-          this.#applyAccountWalletMetadata(wallet);
+          this.#applyAccountWalletMetadata(state, walletId);
           this.#applyAccountGroupMetadata(state, walletId, groupId);
         }
       }