fix(accounts): fix normal account indexing naming with index gap

packages/accounts-controller/src/AccountsController.test.ts

@@ -5,6 +5,7 @@ import { KeyringTypes } from '@metamask/keyring-controller';
 import type { SnapControllerState } from '@metamask/snaps-controllers';
 import { SnapStatus } from '@metamask/snaps-utils';
 import * as uuid from 'uuid';
+import type { V4Options } from 'uuid';
 
 import type {
   AccountsControllerActions,
@@ -14,10 +15,14 @@ import type {
   AllowedEvents,
 } from './AccountsController';
 import { AccountsController } from './AccountsController';
-import { keyringTypeToName } from './utils';
+import {
+  getUUIDOptionsFromAddressOfNormalAccount,
+  keyringTypeToName,
+} from './utils';
 
 jest.mock('uuid');
 const mockUUID = jest.spyOn(uuid, 'v4');
+const actualUUID = jest.requireActual('uuid').v4; // We also use uuid.v4 in our mocks
 
 const defaultState: AccountsControllerState = {
   internalAccounts: {
@@ -101,6 +106,31 @@ const mockAccount4: InternalAccount = {
   },
 };
 
+/**
+ * Mock generated normal account ID to an actual "hard-coded" one.
+ */
+class MockNormalAccountUUID {
+  #accountIds: Record<string, string> = {};
+
+  constructor(accounts: InternalAccount[]) {
+    for (const account of accounts) {
+      const accountId = actualUUID(
+        getUUIDOptionsFromAddressOfNormalAccount(account.address),
+      );
+
+      // Register "hard-coded" (from test) account ID with the actual account UUID
+      this.#accountIds[accountId] = account.id;
+    }
+  }
+
+  mock(options?: V4Options | undefined) {
+    const accountId = actualUUID(options);
+
+    // If not found, we returns the generated UUID
+    return this.#accountIds[accountId] ?? accountId;
+  }
+}
+
 /**
  * Creates an `InternalAccount` object from the given normal account properties.
  *
@@ -1769,29 +1799,29 @@ describe('AccountsController', () => {
   });
 
   describe('#getNextAccountNumber', () => {
-    it('should return the next account number', async () => {
-      const messenger = buildMessenger();
-      mockUUID
-        .mockReturnValueOnce('mock-id') // call to check if its a new account
-        .mockReturnValueOnce('mock-id2') // call to check if its a new account
-        .mockReturnValueOnce('mock-id3') // call to check if its a new account
-        .mockReturnValueOnce('mock-id2') // call to add account
-        .mockReturnValueOnce('mock-id3'); // call to add account
-
-      const mockSimpleKeyring1 = createExpectedInternalAccount({
-        id: 'mock-id2',
-        name: 'Account 2',
-        address: '0x555',
-        keyringType: 'Simple Key Pair',
-      });
-      const mockSimpleKeyring2 = createExpectedInternalAccount({
-        id: 'mock-id3',
-        name: 'Account 3',
-        address: '0x666',
-        keyringType: 'Simple Key Pair',
-      });
+    // Account names start at 2 since have 1 HD account + 2 simple keypair accounts (and both
+    // those keyring types are "grouped" together)
+    const mockSimpleKeyring1 = createExpectedInternalAccount({
+      id: 'mock-id2',
+      name: 'Account 2',
+      address: '0x555',
+      keyringType: 'Simple Key Pair',
+    });
+    const mockSimpleKeyring2 = createExpectedInternalAccount({
+      id: 'mock-id3',
+      name: 'Account 3',
+      address: '0x666',
+      keyringType: 'Simple Key Pair',
+    });
+    const mockSimpleKeyring3 = createExpectedInternalAccount({
+      id: 'mock-id4',
+      name: 'Account 4',
+      address: '0x777',
+      keyringType: 'Simple Key Pair',
+    });
 
-      const mockNewKeyringState = {
+    const mockNewKeyringStateWith = (simpleAddressess: string[]) => {
+      return {
         isUnlocked: true,
         keyrings: [
           {
@@ -1800,10 +1830,21 @@ describe('AccountsController', () => {
           },
           {
             type: 'Simple Key Pair',
-            accounts: [mockSimpleKeyring1.address, mockSimpleKeyring2.address],
+            accounts: simpleAddressess,
           },
         ],
       };
+    };
+
+    it('should return the next account number', async () => {
+      const messenger = buildMessenger();
+      mockUUID
+        .mockReturnValueOnce('mock-id') // call to check if its a new account
+        .mockReturnValueOnce('mock-id2') // call to check if its a new account
+        .mockReturnValueOnce('mock-id3') // call to check if its a new account
+        .mockReturnValueOnce('mock-id2') // call to add account
+        .mockReturnValueOnce('mock-id3'); // call to add account
+
       const accountsController = setupAccountsController({
         initialState: {
           internalAccounts: {
@@ -1818,18 +1859,77 @@ describe('AccountsController', () => {
 
       messenger.publish(
         'KeyringController:stateChange',
-        mockNewKeyringState,
+        mockNewKeyringStateWith([
+          mockSimpleKeyring1.address,
+          mockSimpleKeyring2.address,
+        ]),
         [],
       );
 
       const accounts = accountsController.listAccounts();
-
       expect(accounts).toStrictEqual([
         mockAccount,
         setLastSelectedAsAny(mockSimpleKeyring1),
         setLastSelectedAsAny(mockSimpleKeyring2),
       ]);
     });
+
+    it('should return the next account number even with an index gap', async () => {
+      const messenger = buildMessenger();
+      const mockAccountUUIDs = new MockNormalAccountUUID([
+        mockAccount,
+        mockSimpleKeyring1,
+        mockSimpleKeyring2,
+        mockSimpleKeyring3,
+      ]);
+      mockUUID.mockImplementation(mockAccountUUIDs.mock.bind(mockAccountUUIDs));
+
+      const accountsController = setupAccountsController({
+        initialState: {
+          internalAccounts: {
+            accounts: {
+              [mockAccount.id]: mockAccount,
+            },
+            selectedAccount: mockAccount.id,
+          },
+        },
+        messenger,
+      });
+
+      messenger.publish(
+        'KeyringController:stateChange',
+        mockNewKeyringStateWith([
+          mockSimpleKeyring1.address,
+          mockSimpleKeyring2.address,
+        ]),
+        [],
+      );
+
+      // We then remove "Acccount 2" to create a gap
+      messenger.publish(
+        'KeyringController:stateChange',
+        mockNewKeyringStateWith([mockSimpleKeyring2.address]),
+        [],
+      );
+
+      // Finally we add a 3rd account, and it should be named "Account 4" (despite having a gap
+      // since "Account 2" no longer exists)
+      messenger.publish(
+        'KeyringController:stateChange',
+        mockNewKeyringStateWith([
+          mockSimpleKeyring2.address,
+          mockSimpleKeyring3.address,
+        ]),
+        [],
+      );
+
+      const accounts = accountsController.listAccounts();
+      expect(accounts).toStrictEqual([
+        mockAccount,
+        setLastSelectedAsAny(mockSimpleKeyring2),
+        setLastSelectedAsAny(mockSimpleKeyring3),
+      ]);
+    });
   });
 
   describe('getAccountByAddress', () => {

packages/accounts-controller/src/AccountsController.ts

@@ -1,4 +1,3 @@
-import { toBuffer } from '@ethereumjs/util';
 import type {
   ControllerGetStateAction,
   ControllerStateChangeEvent,
@@ -23,9 +22,7 @@ import type {
 import type { SnapId } from '@metamask/snaps-sdk';
 import type { Snap } from '@metamask/snaps-utils';
 import type { Keyring, Json } from '@metamask/utils';
-import { sha256 } from 'ethereum-cryptography/sha256';
 import type { Draft } from 'immer';
-import { v4 as uuid } from 'uuid';
 
 import { getUUIDFromAddressOfNormalAccount, keyringTypeToName } from './utils';
 
@@ -457,12 +454,9 @@ export class AccountsController extends BaseController<
         'KeyringController:getKeyringForAccount',
         address,
       );
-      const v4options = {
-        random: sha256(toBuffer(address)).slice(0, 16),
-      };
 
       internalAccounts.push({
-        id: uuid(v4options),
+        id: getUUIDFromAddressOfNormalAccount(address),
         address,
         options: {},
         methods: [
@@ -656,6 +650,29 @@ export class AccountsController extends BaseController<
     });
   }
 
+  /**
+   * Returns the list of accounts for a given keyring type.
+   * @param keyringType - The type of keyring.
+   * @returns The list of accounts associcated with this keyring type.
+   */
+  #getAccountsByKeyringType(keyringType: string) {
+    return this.listAccounts().filter((internalAccount) => {
+      // We do consider `hd` and `simple` keyrings to be of same type. So we check those 2 types
+      // to group those accounts together!
+      if (
+        keyringType === KeyringTypes.hd ||
+        keyringType === KeyringTypes.simple
+      ) {
+        return (
+          internalAccount.metadata.keyring.type === KeyringTypes.hd ||
+          internalAccount.metadata.keyring.type === KeyringTypes.simple
+        );
+      }
+
+      return internalAccount.metadata.keyring.type === keyringType;
+    });
+  }
+
   /**
    * Returns the next account number for a given keyring type.
    * @param keyringType - The type of keyring.
@@ -666,35 +683,31 @@ export class AccountsController extends BaseController<
     indexToUse: number;
   } {
     const keyringName = keyringTypeToName(keyringType);
-    const previousKeyringAccounts = this.listAccounts().filter(
-      (internalAccount) => {
-        if (
-          keyringType === KeyringTypes.hd ||
-          keyringType === KeyringTypes.simple
-        ) {
-          return (
-            internalAccount.metadata.keyring.type === KeyringTypes.hd ||
-            internalAccount.metadata.keyring.type === KeyringTypes.simple
-          );
+    const keyringAccounts = this.#getAccountsByKeyringType(keyringType);
+    const lastDefaultIndexUsedForKeyringType = keyringAccounts.reduce(
+      (maxInternalAccountIndex, internalAccount) => {
+        // We **DO NOT USE** `\d+` here to only consider valid "human"
+        // number (rounded decimal number)
+        const match = new RegExp(`${keyringName} ([0-9]+)$`, 'u').exec(
+          internalAccount.metadata.name,
+        );
+
+        if (match) {
+          // Quoting `RegExp.exec` documentation:
+          // > The returned array has the matched text as the first item, and then one item for
+          // > each capturing group of the matched text.
+          // So use `match[1]` to get the captured value
+          const internalAccountIndex = parseInt(match[1], 10);
+          return Math.max(maxInternalAccountIndex, internalAccountIndex);
         }
-        return internalAccount.metadata.keyring.type === keyringType;
+
+        return maxInternalAccountIndex;
       },
+      0,
     );
-    const lastDefaultIndexUsedForKeyringType =
-      previousKeyringAccounts
-        .filter((internalAccount) =>
-          new RegExp(`${keyringName} \\d+$`, 'u').test(
-            internalAccount.metadata.name,
-          ),
-        )
-        .map((internalAccount) => {
-          const nameToWords = internalAccount.metadata.name.split(' '); // get the index of a default account name
-          return parseInt(nameToWords[nameToWords.length], 10);
-        })
-        .sort((a, b) => b - a)[0] || 0;
 
     const indexToUse = Math.max(
-      previousKeyringAccounts.length + 1,
+      keyringAccounts.length + 1,
       lastDefaultIndexUsedForKeyringType + 1,
     );
 

packages/accounts-controller/src/utils.ts

@@ -1,6 +1,7 @@
 import { toBuffer } from '@ethereumjs/util';
 import { isCustodyKeyring, KeyringTypes } from '@metamask/keyring-controller';
 import { sha256 } from 'ethereum-cryptography/sha256';
+import type { V4Options } from 'uuid';
 import { v4 as uuid } from 'uuid';
 
 /**
@@ -45,14 +46,25 @@ export function keyringTypeToName(keyringType: string): string {
 }
 
 /**
- * Generates a UUID from a given Ethereum address.
+ * Generates a UUID v4 options from a given Ethereum address.
  * @param address - The Ethereum address to generate the UUID from.
- * @returns The generated UUID.
+ * @returns The UUID v4 options.
  */
-export function getUUIDFromAddressOfNormalAccount(address: string): string {
+export function getUUIDOptionsFromAddressOfNormalAccount(
+  address: string,
+): V4Options {
   const v4options = {
     random: sha256(toBuffer(address)).slice(0, 16),
   };
 
-  return uuid(v4options);
+  return v4options;
+}
+
+/**
+ * Generates a UUID from a given Ethereum address.
+ * @param address - The Ethereum address to generate the UUID from.
+ * @returns The generated UUID.
+ */
+export function getUUIDFromAddressOfNormalAccount(address: string): string {
+  return uuid(getUUIDOptionsFromAddressOfNormalAccount(address));
 }