diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index fc636eaf9d4..67222becc3a 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/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 = {}; + + 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', () => { diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 28ad1690711..af6b48980d3 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/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, ); diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index 9888b31c2db..b3e7cbd639d 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/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)); }