From 7f1608abb49f531e05b75eb4d7d7611cd6b6ad14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Mon, 16 Dec 2024 14:15:18 +0100 Subject: [PATCH 01/33] feat(multi-srp): add id and typeIndex to hd keyrings --- .../src/AccountsController.test.ts | 44 +++++++ packages/keyring-controller/package.json | 9 +- .../src/KeyringController.test.ts | 47 ++++++- .../src/KeyringController.ts | 122 +++++++++++++++--- .../src/PreferencesController.test.ts | 64 +++++++-- yarn.lock | 1 + 6 files changed, 251 insertions(+), 36 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 07c400f9a4e..8e7afac3b98 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -507,6 +507,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, ], }; @@ -551,6 +553,8 @@ describe('AccountsController', () => { { accounts: [mockAccount.address, mockAccount2.address], type: KeyringTypes.hd, + typeIndex: 0, + id: '123', }, ], }; @@ -587,6 +591,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -640,10 +646,14 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], + typeIndex: 0, + id: '123', }, ], }; @@ -707,10 +717,14 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], + typeIndex: 0, + id: '123', }, ], }; @@ -757,6 +771,8 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], + typeIndex: 0, + id: '123', }, ], }; @@ -819,6 +835,8 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], + typeIndex: 0, + id: '123', }, ], }; @@ -875,10 +893,14 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [], + typeIndex: 0, + id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address], + typeIndex: 0, + id: '123', }, ], }; @@ -915,6 +937,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -970,6 +994,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1001,6 +1027,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1042,6 +1070,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1102,6 +1132,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1175,6 +1207,8 @@ describe('AccountsController', () => { mockAccountWithoutLastSelected.address, mockAccount2.address, ], + typeIndex: 0, + id: '123', }, ], }; @@ -1243,6 +1277,8 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { + typeIndex: 0, + id: '123', type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], }, @@ -1287,6 +1323,8 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { + typeIndex: 0, + id: '123', type: KeyringTypes.hd, accounts: [mockReinitialisedAccount.address], }, @@ -1380,6 +1418,8 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { + typeIndex: 0, + id: '123', type: KeyringTypes.hd, accounts: [ mockExistingAccount1.address, @@ -2649,10 +2689,14 @@ describe('AccountsController', () => { { type: 'HD Key Tree', accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, { type: 'Simple Key Pair', accounts: simpleAddressess, + typeIndex: 0, + id: '123', }, ], }; diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index a028d09d5e4..9b817565fe0 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -60,7 +60,8 @@ "@metamask/utils": "^11.1.0", "async-mutex": "^0.5.0", "ethereumjs-wallet": "^1.0.1", - "immer": "^9.0.6" + "immer": "^9.0.6", + "ulid": "^2.3.0" }, "devDependencies": { "@ethereumjs/common": "^3.2.0", @@ -89,6 +90,10 @@ "registry": "https://registry.npmjs.org/" }, "lavamoat": { - "allowScripts": {} + "allowScripts": { + "@lavamoat/preinstall-always-fail": false, + "ethereumjs-wallet>ethereum-cryptography>keccak": false, + "ethereumjs-wallet>ethereum-cryptography>secp256k1": false + } } } diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index bbba030eee3..ed9082f7b0f 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -74,6 +74,25 @@ const password = 'password123'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; +/** + * Strip the `id` property from each keyring in the state as it is generated + * by the `ulid()` function and is not deterministic. + * + * @param state - The state to strip the `id` property from. + * @returns The state with the `id` property stripped from each keyring. + */ +function stripKeyringIds(state: KeyringControllerState): Omit< + KeyringControllerState, + 'keyrings' +> & { + keyrings: Omit<(typeof state.keyrings)[number], 'id'>[]; +} { + return { + ...state, + keyrings: state.keyrings.map(({ id, ...rest }) => rest), + }; +} + describe('KeyringController', () => { afterEach(() => { sinon.restore(); @@ -408,7 +427,9 @@ describe('KeyringController', () => { password, currentSeedWord, ); - expect(initialState).toStrictEqual(controller.state); + expect(stripKeyringIds(initialState)).toStrictEqual( + stripKeyringIds(controller.state), + ); }, ); }); @@ -977,7 +998,9 @@ describe('KeyringController', () => { const address = '0x51253087e6f8358b5f10c0a94315d69db3357859'; const newKeyring = { accounts: [address], + id: undefined, type: 'Simple Key Pair', + typeIndex: undefined, }; const importedAccountAddress = await controller.importAccountWithStrategy( @@ -1055,7 +1078,9 @@ describe('KeyringController', () => { const newKeyring = { accounts: [address], + id: undefined, type: 'Simple Key Pair', + typeIndex: undefined, }; const modifiedState = { ...initialState, @@ -2059,7 +2084,9 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { await controller.submitPassword(password); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }, ); }); @@ -2161,7 +2188,9 @@ describe('KeyringController', () => { MOCK_ENCRYPTION_KEY, initialState.encryptionSalt as string, ); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }, ); }); @@ -3414,7 +3443,9 @@ describe('KeyringController', () => { controller.submitPassword(password), ]); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }); }); @@ -3427,7 +3458,9 @@ describe('KeyringController', () => { controller.persistAllKeyrings(), ]); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }); }); @@ -3446,7 +3479,9 @@ describe('KeyringController', () => { await controller.submitPassword(password); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); expect(listener).toHaveBeenCalled(); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 138fb6e6e4f..329b51168fd 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -43,6 +43,7 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; +import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; @@ -248,7 +249,13 @@ export type KeyringControllerOptions = { ); /** - * A keyring object representation. + * @type KeyringObject + * + * Keyring object to return in fullUpdate + * @property type - Keyring type + * @property accounts - Associated accounts + * @property typeIndex - The index of the keyring in the array of keyrings of the same type + * @property id - The id of the keyring */ export type KeyringObject = { /** @@ -259,6 +266,8 @@ export type KeyringObject = { * Keyring type. */ type: string; + typeIndex: number; + id: string; }; /** @@ -391,6 +400,9 @@ export type KeyringSelector = } | { address: Hex; + } + | { + id: string; }; /** @@ -516,16 +528,24 @@ function isSerializedKeyringsArray( * @param keyring - The keyring to display. * @returns A keyring display object, with type and accounts properties. */ -async function displayForKeyring( - keyring: EthKeyring, -): Promise<{ type: string; accounts: string[] }> { +async function displayForKeyring(keyring: EthKeyring): Promise<{ + type: string; + accounts: string[]; + typeIndex: number; + id: string; +}> { const accounts = await keyring.getAccounts(); + const { opts } = keyring as EthKeyring & { + opts: { typeIndex: number; id: string }; + }; return { type: keyring.type, // Cast to `string[]` here is safe here because `accounts` has no nullish // values, and `normalize` returns `string` unless given a nullish value accounts: accounts.map(normalize) as string[], + typeIndex: opts?.typeIndex, + id: opts?.id, }; } @@ -651,18 +671,27 @@ export class KeyringController extends BaseController< * Adds a new account to the default (first) HD seed phrase keyring. * * @param accountCount - Number of accounts before adding a new one, used to + * @param keyringId - The id of the keyring to add the account to. * make the method idempotent. * @returns Promise resolving to the added account address. */ - async addNewAccount(accountCount?: number): Promise { + async addNewAccount( + accountCount?: number, + keyringId?: string, + ): Promise { return this.#persistOrRollback(async () => { - const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as - | EthKeyring - | undefined; - if (!primaryKeyring) { + let selectedKeyring: EthKeyring | undefined; + if (keyringId) { + selectedKeyring = this.getKeyringById(keyringId) as EthKeyring; + } else { + selectedKeyring = this.getKeyringsByType( + 'HD Key Tree', + )[0] as EthKeyring; + } + if (!selectedKeyring) { throw new Error('No HD keyring found'); } - const oldAccounts = await primaryKeyring.getAccounts(); + const oldAccounts = await selectedKeyring.getAccounts(); if (accountCount && oldAccounts.length !== accountCount) { if (accountCount > oldAccounts.length) { @@ -678,8 +707,8 @@ export class KeyringController extends BaseController< return existingAccount; } - const [addedAccountAddress] = await primaryKeyring.addAccounts(1); - await this.#verifySeedPhrase(); + const [addedAccountAddress] = await selectedKeyring.addAccounts(1); + await this.verifySeedPhrase(); return addedAccountAddress; }); @@ -751,6 +780,15 @@ export class KeyringController extends BaseController< }); } + async createKeyringFromMnemonic(mnemonic: string): Promise { + return this.#persistOrRollback(async () => { + return await this.#createKeyringWithFirstAccount(KeyringTypes.hd, { + mnemonic, + numberOfAccounts: 1, + }); + }); + } + /** * Create a new vault and primary keyring. * @@ -847,13 +885,16 @@ export class KeyringController extends BaseController< /** * Returns the public addresses of all accounts from every keyring. * + * @param keyringId - The id of the keyring to get the accounts from. * @returns A promise resolving to an array of addresses. */ - async getAccounts(): Promise { - return this.state.keyrings.reduce( - (accounts, keyring) => accounts.concat(keyring.accounts), - [], - ); + async getAccounts(keyringId?: string): Promise { + return this.state.keyrings + .filter((keyring) => (keyringId ? keyring.id === keyringId : true)) + .reduce( + (accounts, keyring) => accounts.concat(keyring.accounts), + [], + ); } /** @@ -902,6 +943,24 @@ export class KeyringController extends BaseController< return keyring.decryptMessage(address, messageParams.data); } + /** + * Returns the keyring with the given id. + * + * @param id - The id of the keyring to return. + * @returns The keyring with the given id. + */ + getKeyringById(id: string): unknown { + const keyring = this.#keyrings.find( + (item) => + (item as EthKeyring & { opts: { id: string } }).opts.id === id, + ); + if (!keyring) { + throw new Error(KeyringControllerError.KeyringNotFound); + } + + return keyring; + } + /** * Returns the currently initialized keyring that manages * the specified `address` if one exists. @@ -1425,7 +1484,7 @@ export class KeyringController extends BaseController< keyring = (await this.getKeyringForAccount(selector.address)) as | SelectedKeyring | undefined; - } else { + } else if ('type' in selector) { keyring = this.getKeyringsByType(selector.type)[selector.index || 0] as | SelectedKeyring | undefined; @@ -1436,6 +1495,8 @@ export class KeyringController extends BaseController< options.createWithData, )) as SelectedKeyring; } + } else if ('id' in selector) { + keyring = this.getKeyringById(selector.id) as SelectedKeyring; } if (!keyring) { @@ -2151,6 +2212,7 @@ export class KeyringController extends BaseController< if (!firstAccount) { throw new Error(KeyringControllerError.NoFirstAccount); } + return firstAccount; } /** @@ -2177,8 +2239,28 @@ export class KeyringController extends BaseController< const keyring = keyringBuilder(); - // @ts-expect-error Enforce data type after updating clients - await keyring.deserialize(data); + // find the last index of the type + const lastIndexOfType: number = this.#keyrings.reduce((maxIndex, item) => { + if (item.type === type) { + return Math.max( + maxIndex, + (item as EthKeyring & { opts: { typeIndex: number } }).opts + ?.typeIndex ?? 0, + ); + } + return maxIndex; + }, 0); + + if (type === KeyringTypes.hd) { + await keyring.deserialize({ + ...(data ?? {}), + typeIndex: lastIndexOfType + 1, + id: ulid(), + }); + } else { + // @ts-expect-error Enforce data type after updating clients + await keyring.deserialize(data); + } if (keyring.init) { await keyring.init(); diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index 2b22a708e3c..f4459af8e84 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -66,7 +66,12 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, ], }, [], @@ -108,7 +113,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); @@ -138,7 +150,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); @@ -167,7 +186,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: [], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: [], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); @@ -197,7 +223,12 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, ], }, [], @@ -228,8 +259,18 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 1, + id: '456', + }, ], }, [], @@ -259,7 +300,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00', '0x01'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00', '0x01'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); diff --git a/yarn.lock b/yarn.lock index afb813337a7..5e88d88c082 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3283,6 +3283,7 @@ __metadata: typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" typescript: "npm:~5.2.2" + ulid: "npm:^2.3.0" uuid: "npm:^8.3.2" languageName: unknown linkType: soft From 281d85ffa948d2fb44d6c5b10bfd3806daae6031 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 8 Jan 2025 10:12:58 +0100 Subject: [PATCH 02/33] feat: use keyring index to select srp --- .../src/AccountsController.test.ts | 44 ------ packages/keyring-controller/package.json | 3 +- .../src/KeyringController.test.ts | 47 +------ .../src/KeyringController.ts | 131 ++++++------------ .../src/PreferencesController.test.ts | 64 ++------- yarn.lock | 1 - 6 files changed, 61 insertions(+), 229 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 8e7afac3b98..07c400f9a4e 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -507,8 +507,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, ], }; @@ -553,8 +551,6 @@ describe('AccountsController', () => { { accounts: [mockAccount.address, mockAccount2.address], type: KeyringTypes.hd, - typeIndex: 0, - id: '123', }, ], }; @@ -591,8 +587,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -646,14 +640,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], - typeIndex: 0, - id: '123', }, ], }; @@ -717,14 +707,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], - typeIndex: 0, - id: '123', }, ], }; @@ -771,8 +757,6 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], - typeIndex: 0, - id: '123', }, ], }; @@ -835,8 +819,6 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], - typeIndex: 0, - id: '123', }, ], }; @@ -893,14 +875,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [], - typeIndex: 0, - id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address], - typeIndex: 0, - id: '123', }, ], }; @@ -937,8 +915,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -994,8 +970,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1027,8 +1001,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1070,8 +1042,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1132,8 +1102,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1207,8 +1175,6 @@ describe('AccountsController', () => { mockAccountWithoutLastSelected.address, mockAccount2.address, ], - typeIndex: 0, - id: '123', }, ], }; @@ -1277,8 +1243,6 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { - typeIndex: 0, - id: '123', type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], }, @@ -1323,8 +1287,6 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { - typeIndex: 0, - id: '123', type: KeyringTypes.hd, accounts: [mockReinitialisedAccount.address], }, @@ -1418,8 +1380,6 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { - typeIndex: 0, - id: '123', type: KeyringTypes.hd, accounts: [ mockExistingAccount1.address, @@ -2689,14 +2649,10 @@ describe('AccountsController', () => { { type: 'HD Key Tree', accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, { type: 'Simple Key Pair', accounts: simpleAddressess, - typeIndex: 0, - id: '123', }, ], }; diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 9b817565fe0..0cfd5f186bb 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -60,8 +60,7 @@ "@metamask/utils": "^11.1.0", "async-mutex": "^0.5.0", "ethereumjs-wallet": "^1.0.1", - "immer": "^9.0.6", - "ulid": "^2.3.0" + "immer": "^9.0.6" }, "devDependencies": { "@ethereumjs/common": "^3.2.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index ed9082f7b0f..bbba030eee3 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -74,25 +74,6 @@ const password = 'password123'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; -/** - * Strip the `id` property from each keyring in the state as it is generated - * by the `ulid()` function and is not deterministic. - * - * @param state - The state to strip the `id` property from. - * @returns The state with the `id` property stripped from each keyring. - */ -function stripKeyringIds(state: KeyringControllerState): Omit< - KeyringControllerState, - 'keyrings' -> & { - keyrings: Omit<(typeof state.keyrings)[number], 'id'>[]; -} { - return { - ...state, - keyrings: state.keyrings.map(({ id, ...rest }) => rest), - }; -} - describe('KeyringController', () => { afterEach(() => { sinon.restore(); @@ -427,9 +408,7 @@ describe('KeyringController', () => { password, currentSeedWord, ); - expect(stripKeyringIds(initialState)).toStrictEqual( - stripKeyringIds(controller.state), - ); + expect(initialState).toStrictEqual(controller.state); }, ); }); @@ -998,9 +977,7 @@ describe('KeyringController', () => { const address = '0x51253087e6f8358b5f10c0a94315d69db3357859'; const newKeyring = { accounts: [address], - id: undefined, type: 'Simple Key Pair', - typeIndex: undefined, }; const importedAccountAddress = await controller.importAccountWithStrategy( @@ -1078,9 +1055,7 @@ describe('KeyringController', () => { const newKeyring = { accounts: [address], - id: undefined, type: 'Simple Key Pair', - typeIndex: undefined, }; const modifiedState = { ...initialState, @@ -2084,9 +2059,7 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { await controller.submitPassword(password); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }, ); }); @@ -2188,9 +2161,7 @@ describe('KeyringController', () => { MOCK_ENCRYPTION_KEY, initialState.encryptionSalt as string, ); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }, ); }); @@ -3443,9 +3414,7 @@ describe('KeyringController', () => { controller.submitPassword(password), ]); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }); }); @@ -3458,9 +3427,7 @@ describe('KeyringController', () => { controller.persistAllKeyrings(), ]); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }); }); @@ -3479,9 +3446,7 @@ describe('KeyringController', () => { await controller.submitPassword(password); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); expect(listener).toHaveBeenCalled(); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 329b51168fd..00ed3d76db4 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -43,7 +43,6 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; -import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; @@ -254,8 +253,6 @@ export type KeyringControllerOptions = { * Keyring object to return in fullUpdate * @property type - Keyring type * @property accounts - Associated accounts - * @property typeIndex - The index of the keyring in the array of keyrings of the same type - * @property id - The id of the keyring */ export type KeyringObject = { /** @@ -266,8 +263,6 @@ export type KeyringObject = { * Keyring type. */ type: string; - typeIndex: number; - id: string; }; /** @@ -400,9 +395,6 @@ export type KeyringSelector = } | { address: Hex; - } - | { - id: string; }; /** @@ -528,24 +520,16 @@ function isSerializedKeyringsArray( * @param keyring - The keyring to display. * @returns A keyring display object, with type and accounts properties. */ -async function displayForKeyring(keyring: EthKeyring): Promise<{ - type: string; - accounts: string[]; - typeIndex: number; - id: string; -}> { +async function displayForKeyring( + keyring: EthKeyring, +): Promise<{ type: string; accounts: string[] }> { const accounts = await keyring.getAccounts(); - const { opts } = keyring as EthKeyring & { - opts: { typeIndex: number; id: string }; - }; return { type: keyring.type, // Cast to `string[]` here is safe here because `accounts` has no nullish // values, and `normalize` returns `string` unless given a nullish value accounts: accounts.map(normalize) as string[], - typeIndex: opts?.typeIndex, - id: opts?.id, }; } @@ -671,23 +655,18 @@ export class KeyringController extends BaseController< * Adds a new account to the default (first) HD seed phrase keyring. * * @param accountCount - Number of accounts before adding a new one, used to - * @param keyringId - The id of the keyring to add the account to. + * @param typeIndex - The id of the keyring to add the account to. * make the method idempotent. * @returns Promise resolving to the added account address. */ async addNewAccount( accountCount?: number, - keyringId?: string, + typeIndex?: number, ): Promise { return this.#persistOrRollback(async () => { - let selectedKeyring: EthKeyring | undefined; - if (keyringId) { - selectedKeyring = this.getKeyringById(keyringId) as EthKeyring; - } else { - selectedKeyring = this.getKeyringsByType( - 'HD Key Tree', - )[0] as EthKeyring; - } + const selectedKeyring = this.getKeyringsByType('HD Key Tree')[ + typeIndex ?? 0 + ] as EthKeyring; if (!selectedKeyring) { throw new Error('No HD keyring found'); } @@ -708,7 +687,7 @@ export class KeyringController extends BaseController< } const [addedAccountAddress] = await selectedKeyring.addAccounts(1); - await this.verifySeedPhrase(); + await this.verifySeedPhrase(typeIndex ?? 0); return addedAccountAddress; }); @@ -854,12 +833,30 @@ export class KeyringController extends BaseController< * Gets the seed phrase of the HD keyring. * * @param password - Password of the keyring. + * @param typeIndex - The index of the keyring type. * @returns Promise resolving to the seed phrase. */ - async exportSeedPhrase(password: string): Promise { + async exportSeedPhrase( + password: string, + typeIndex?: number, + ): Promise { await this.verifyPassword(password); - assertHasUint8ArrayMnemonic(this.#keyrings[0]); - return this.#keyrings[0].mnemonic; + if (!typeIndex) { + assertHasUint8ArrayMnemonic(this.#keyrings[0]); + return this.#keyrings[0].mnemonic; + } + const selectedKeyring = ( + this.#keyrings as (EthKeyring & { + opts: { typeIndex: number }; + mnemonic: Uint8Array; + })[] + ).filter((keyring) => keyring.type === KeyringTypes.hd)[typeIndex]; + if (!selectedKeyring) { + throw new Error('Keyring not found'); + } + assertHasUint8ArrayMnemonic(selectedKeyring as EthKeyring); + + return selectedKeyring.mnemonic; } /** @@ -885,16 +882,20 @@ export class KeyringController extends BaseController< /** * Returns the public addresses of all accounts from every keyring. * - * @param keyringId - The id of the keyring to get the accounts from. + * @param keyringIndex - The index of the keyring to get the accounts from. * @returns A promise resolving to an array of addresses. */ - async getAccounts(keyringId?: string): Promise { - return this.state.keyrings - .filter((keyring) => (keyringId ? keyring.id === keyringId : true)) - .reduce( - (accounts, keyring) => accounts.concat(keyring.accounts), - [], - ); + async getAccounts(keyringIndex?: number): Promise { + const keyrings = this.state.keyrings.filter( + (keyring) => keyring.type === KeyringTypes.hd, + ); + if (keyringIndex) { + return keyrings[keyringIndex].accounts; + } + return keyrings.reduce( + (accounts, keyring) => accounts.concat(keyring.accounts), + [], + ); } /** @@ -943,24 +944,6 @@ export class KeyringController extends BaseController< return keyring.decryptMessage(address, messageParams.data); } - /** - * Returns the keyring with the given id. - * - * @param id - The id of the keyring to return. - * @returns The keyring with the given id. - */ - getKeyringById(id: string): unknown { - const keyring = this.#keyrings.find( - (item) => - (item as EthKeyring & { opts: { id: string } }).opts.id === id, - ); - if (!keyring) { - throw new Error(KeyringControllerError.KeyringNotFound); - } - - return keyring; - } - /** * Returns the currently initialized keyring that manages * the specified `address` if one exists. @@ -1405,6 +1388,7 @@ export class KeyringController extends BaseController< /** * Verifies the that the seed phrase restores the current keychain's accounts. * + * @param typeIndex - The index of the keyring to verify. * @returns Promise resolving to the seed phrase as Uint8Array. */ async verifySeedPhrase(): Promise { @@ -1484,7 +1468,7 @@ export class KeyringController extends BaseController< keyring = (await this.getKeyringForAccount(selector.address)) as | SelectedKeyring | undefined; - } else if ('type' in selector) { + } else { keyring = this.getKeyringsByType(selector.type)[selector.index || 0] as | SelectedKeyring | undefined; @@ -1495,8 +1479,6 @@ export class KeyringController extends BaseController< options.createWithData, )) as SelectedKeyring; } - } else if ('id' in selector) { - keyring = this.getKeyringById(selector.id) as SelectedKeyring; } if (!keyring) { @@ -2238,29 +2220,8 @@ export class KeyringController extends BaseController< } const keyring = keyringBuilder(); - - // find the last index of the type - const lastIndexOfType: number = this.#keyrings.reduce((maxIndex, item) => { - if (item.type === type) { - return Math.max( - maxIndex, - (item as EthKeyring & { opts: { typeIndex: number } }).opts - ?.typeIndex ?? 0, - ); - } - return maxIndex; - }, 0); - - if (type === KeyringTypes.hd) { - await keyring.deserialize({ - ...(data ?? {}), - typeIndex: lastIndexOfType + 1, - id: ulid(), - }); - } else { - // @ts-expect-error Enforce data type after updating clients - await keyring.deserialize(data); - } + // @ts-expect-error Enforce data type after updating clients + await keyring.deserialize(data); if (keyring.init) { await keyring.init(); diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index f4459af8e84..2b22a708e3c 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -66,12 +66,7 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, ], }, [], @@ -113,14 +108,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: ['0x00'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], }, [], ); @@ -150,14 +138,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: ['0x00'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], }, [], ); @@ -186,14 +167,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: [], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: [], type: 'CustomKeyring' }], }, [], ); @@ -223,12 +197,7 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, ], }, [], @@ -259,18 +228,8 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 1, - id: '456', - }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, ], }, [], @@ -300,14 +259,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: ['0x00', '0x01'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: ['0x00', '0x01'], type: 'CustomKeyring' }], }, [], ); diff --git a/yarn.lock b/yarn.lock index 5e88d88c082..afb813337a7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3283,7 +3283,6 @@ __metadata: typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" typescript: "npm:~5.2.2" - ulid: "npm:^2.3.0" uuid: "npm:^8.3.2" languageName: unknown linkType: soft From 9c88af9a5121ad3568d8ec13697f61092a1f999b Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Tue, 17 Dec 2024 15:13:31 +0100 Subject: [PATCH 03/33] fix: lock `KeyringController` mutex on `verifySeedPhrase` (#5077) The `KeyringController.verifySeedPhrase` method was not included in the mutable methods that lock the controller mutex because it doesn't change the state. Though, if another operation gets somehow overlapped (e.g. a consumer calls `addNewAccount`), the call to `verifySeedPhrase` can potentially fail. To fix this, this PR is moving verifySeedPhrase behind KeyringController's mutex. Since `addNewAccount` internally calls `verifySeedPhrase`, and having a lock on both would create a deadlock, the `verifySeedPhrase` implementation has been moved to an internal method. - **FIXED**: `verifySeedPhrase` is now mutually exclusive - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 00ed3d76db4..8ce4a176956 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -687,7 +687,7 @@ export class KeyringController extends BaseController< } const [addedAccountAddress] = await selectedKeyring.addAccounts(1); - await this.verifySeedPhrase(typeIndex ?? 0); + await this.#verifySeedPhrase(); return addedAccountAddress; }); From ec9e428277da840bd7bea36553624d376eb84202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Mon, 16 Dec 2024 14:15:18 +0100 Subject: [PATCH 04/33] feat(multi-srp): add id and typeIndex to hd keyrings --- .../src/AccountsController.test.ts | 44 ++++++++++ packages/keyring-controller/package.json | 3 +- .../src/KeyringController.test.ts | 47 +++++++++-- .../src/KeyringController.ts | 81 ++++++++++++++----- .../src/PreferencesController.test.ts | 64 +++++++++++++-- yarn.lock | 1 + 6 files changed, 203 insertions(+), 37 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 07c400f9a4e..8e7afac3b98 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -507,6 +507,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, ], }; @@ -551,6 +553,8 @@ describe('AccountsController', () => { { accounts: [mockAccount.address, mockAccount2.address], type: KeyringTypes.hd, + typeIndex: 0, + id: '123', }, ], }; @@ -587,6 +591,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -640,10 +646,14 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], + typeIndex: 0, + id: '123', }, ], }; @@ -707,10 +717,14 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], + typeIndex: 0, + id: '123', }, ], }; @@ -757,6 +771,8 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], + typeIndex: 0, + id: '123', }, ], }; @@ -819,6 +835,8 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], + typeIndex: 0, + id: '123', }, ], }; @@ -875,10 +893,14 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [], + typeIndex: 0, + id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address], + typeIndex: 0, + id: '123', }, ], }; @@ -915,6 +937,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -970,6 +994,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1001,6 +1027,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1042,6 +1070,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1102,6 +1132,8 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], + typeIndex: 0, + id: '123', }, ], }; @@ -1175,6 +1207,8 @@ describe('AccountsController', () => { mockAccountWithoutLastSelected.address, mockAccount2.address, ], + typeIndex: 0, + id: '123', }, ], }; @@ -1243,6 +1277,8 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { + typeIndex: 0, + id: '123', type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], }, @@ -1287,6 +1323,8 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { + typeIndex: 0, + id: '123', type: KeyringTypes.hd, accounts: [mockReinitialisedAccount.address], }, @@ -1380,6 +1418,8 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { + typeIndex: 0, + id: '123', type: KeyringTypes.hd, accounts: [ mockExistingAccount1.address, @@ -2649,10 +2689,14 @@ describe('AccountsController', () => { { type: 'HD Key Tree', accounts: [mockAccount.address], + typeIndex: 0, + id: '123', }, { type: 'Simple Key Pair', accounts: simpleAddressess, + typeIndex: 0, + id: '123', }, ], }; diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 0cfd5f186bb..9b817565fe0 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -60,7 +60,8 @@ "@metamask/utils": "^11.1.0", "async-mutex": "^0.5.0", "ethereumjs-wallet": "^1.0.1", - "immer": "^9.0.6" + "immer": "^9.0.6", + "ulid": "^2.3.0" }, "devDependencies": { "@ethereumjs/common": "^3.2.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index bbba030eee3..ed9082f7b0f 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -74,6 +74,25 @@ const password = 'password123'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; +/** + * Strip the `id` property from each keyring in the state as it is generated + * by the `ulid()` function and is not deterministic. + * + * @param state - The state to strip the `id` property from. + * @returns The state with the `id` property stripped from each keyring. + */ +function stripKeyringIds(state: KeyringControllerState): Omit< + KeyringControllerState, + 'keyrings' +> & { + keyrings: Omit<(typeof state.keyrings)[number], 'id'>[]; +} { + return { + ...state, + keyrings: state.keyrings.map(({ id, ...rest }) => rest), + }; +} + describe('KeyringController', () => { afterEach(() => { sinon.restore(); @@ -408,7 +427,9 @@ describe('KeyringController', () => { password, currentSeedWord, ); - expect(initialState).toStrictEqual(controller.state); + expect(stripKeyringIds(initialState)).toStrictEqual( + stripKeyringIds(controller.state), + ); }, ); }); @@ -977,7 +998,9 @@ describe('KeyringController', () => { const address = '0x51253087e6f8358b5f10c0a94315d69db3357859'; const newKeyring = { accounts: [address], + id: undefined, type: 'Simple Key Pair', + typeIndex: undefined, }; const importedAccountAddress = await controller.importAccountWithStrategy( @@ -1055,7 +1078,9 @@ describe('KeyringController', () => { const newKeyring = { accounts: [address], + id: undefined, type: 'Simple Key Pair', + typeIndex: undefined, }; const modifiedState = { ...initialState, @@ -2059,7 +2084,9 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { await controller.submitPassword(password); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }, ); }); @@ -2161,7 +2188,9 @@ describe('KeyringController', () => { MOCK_ENCRYPTION_KEY, initialState.encryptionSalt as string, ); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }, ); }); @@ -3414,7 +3443,9 @@ describe('KeyringController', () => { controller.submitPassword(password), ]); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }); }); @@ -3427,7 +3458,9 @@ describe('KeyringController', () => { controller.persistAllKeyrings(), ]); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); }); }); @@ -3446,7 +3479,9 @@ describe('KeyringController', () => { await controller.submitPassword(password); - expect(controller.state).toStrictEqual(initialState); + expect(stripKeyringIds(controller.state)).toStrictEqual( + stripKeyringIds(initialState), + ); expect(listener).toHaveBeenCalled(); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8ce4a176956..cfb78b04f0b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -43,6 +43,7 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; +import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; @@ -253,6 +254,8 @@ export type KeyringControllerOptions = { * Keyring object to return in fullUpdate * @property type - Keyring type * @property accounts - Associated accounts + * @property typeIndex - The index of the keyring in the array of keyrings of the same type + * @property id - The id of the keyring */ export type KeyringObject = { /** @@ -263,6 +266,8 @@ export type KeyringObject = { * Keyring type. */ type: string; + typeIndex: number; + id: string; }; /** @@ -395,6 +400,9 @@ export type KeyringSelector = } | { address: Hex; + } + | { + id: string; }; /** @@ -520,16 +528,24 @@ function isSerializedKeyringsArray( * @param keyring - The keyring to display. * @returns A keyring display object, with type and accounts properties. */ -async function displayForKeyring( - keyring: EthKeyring, -): Promise<{ type: string; accounts: string[] }> { +async function displayForKeyring(keyring: EthKeyring): Promise<{ + type: string; + accounts: string[]; + typeIndex: number; + id: string; +}> { const accounts = await keyring.getAccounts(); + const { opts } = keyring as EthKeyring & { + opts: { typeIndex: number; id: string }; + }; return { type: keyring.type, // Cast to `string[]` here is safe here because `accounts` has no nullish // values, and `normalize` returns `string` unless given a nullish value accounts: accounts.map(normalize) as string[], + typeIndex: opts?.typeIndex, + id: opts?.id, }; } @@ -655,18 +671,23 @@ export class KeyringController extends BaseController< * Adds a new account to the default (first) HD seed phrase keyring. * * @param accountCount - Number of accounts before adding a new one, used to - * @param typeIndex - The id of the keyring to add the account to. + * @param keyringId - The id of the keyring to add the account to. * make the method idempotent. * @returns Promise resolving to the added account address. */ async addNewAccount( accountCount?: number, - typeIndex?: number, + keyringId?: string, ): Promise { return this.#persistOrRollback(async () => { - const selectedKeyring = this.getKeyringsByType('HD Key Tree')[ - typeIndex ?? 0 - ] as EthKeyring; + let selectedKeyring: EthKeyring | undefined; + if (keyringId) { + selectedKeyring = this.getKeyringById(keyringId) as EthKeyring; + } else { + selectedKeyring = this.getKeyringsByType( + 'HD Key Tree', + )[0] as EthKeyring; + } if (!selectedKeyring) { throw new Error('No HD keyring found'); } @@ -687,7 +708,7 @@ export class KeyringController extends BaseController< } const [addedAccountAddress] = await selectedKeyring.addAccounts(1); - await this.#verifySeedPhrase(); + await this.verifySeedPhrase(); return addedAccountAddress; }); @@ -882,20 +903,16 @@ export class KeyringController extends BaseController< /** * Returns the public addresses of all accounts from every keyring. * - * @param keyringIndex - The index of the keyring to get the accounts from. + * @param keyringId - The id of the keyring to get the accounts from. * @returns A promise resolving to an array of addresses. */ - async getAccounts(keyringIndex?: number): Promise { - const keyrings = this.state.keyrings.filter( - (keyring) => keyring.type === KeyringTypes.hd, - ); - if (keyringIndex) { - return keyrings[keyringIndex].accounts; - } - return keyrings.reduce( - (accounts, keyring) => accounts.concat(keyring.accounts), - [], - ); + async getAccounts(keyringId?: string): Promise { + return this.state.keyrings + .filter((keyring) => (keyringId ? keyring.id === keyringId : true)) + .reduce( + (accounts, keyring) => accounts.concat(keyring.accounts), + [], + ); } /** @@ -944,6 +961,24 @@ export class KeyringController extends BaseController< return keyring.decryptMessage(address, messageParams.data); } + /** + * Returns the keyring with the given id. + * + * @param id - The id of the keyring to return. + * @returns The keyring with the given id. + */ + getKeyringById(id: string): unknown { + const keyring = this.#keyrings.find( + (item) => + (item as EthKeyring & { opts: { id: string } }).opts.id === id, + ); + if (!keyring) { + throw new Error(KeyringControllerError.KeyringNotFound); + } + + return keyring; + } + /** * Returns the currently initialized keyring that manages * the specified `address` if one exists. @@ -1468,7 +1503,7 @@ export class KeyringController extends BaseController< keyring = (await this.getKeyringForAccount(selector.address)) as | SelectedKeyring | undefined; - } else { + } else if ('type' in selector) { keyring = this.getKeyringsByType(selector.type)[selector.index || 0] as | SelectedKeyring | undefined; @@ -1479,6 +1514,8 @@ export class KeyringController extends BaseController< options.createWithData, )) as SelectedKeyring; } + } else if ('id' in selector) { + keyring = this.getKeyringById(selector.id) as SelectedKeyring; } if (!keyring) { diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index 2b22a708e3c..f4459af8e84 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -66,7 +66,12 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, ], }, [], @@ -108,7 +113,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); @@ -138,7 +150,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); @@ -167,7 +186,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: [], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: [], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); @@ -197,7 +223,12 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, ], }, [], @@ -228,8 +259,18 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + typeIndex: 1, + id: '456', + }, ], }, [], @@ -259,7 +300,14 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00', '0x01'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00', '0x01'], + type: 'CustomKeyring', + typeIndex: 0, + id: '123', + }, + ], }, [], ); diff --git a/yarn.lock b/yarn.lock index afb813337a7..5e88d88c082 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3283,6 +3283,7 @@ __metadata: typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" typescript: "npm:~5.2.2" + ulid: "npm:^2.3.0" uuid: "npm:^8.3.2" languageName: unknown linkType: soft From cbb3803492bf988a84e1e060f6f6b567bfdbef4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 8 Jan 2025 10:12:58 +0100 Subject: [PATCH 05/33] feat: use keyring index to select srp --- .../src/AccountsController.test.ts | 44 ---------- packages/keyring-controller/package.json | 3 +- .../src/KeyringController.test.ts | 47 ++--------- .../src/KeyringController.ts | 81 +++++-------------- .../src/PreferencesController.test.ts | 64 ++------------- yarn.lock | 1 - 6 files changed, 37 insertions(+), 203 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 8e7afac3b98..07c400f9a4e 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -507,8 +507,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, ], }; @@ -553,8 +551,6 @@ describe('AccountsController', () => { { accounts: [mockAccount.address, mockAccount2.address], type: KeyringTypes.hd, - typeIndex: 0, - id: '123', }, ], }; @@ -591,8 +587,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -646,14 +640,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], - typeIndex: 0, - id: '123', }, ], }; @@ -717,14 +707,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], - typeIndex: 0, - id: '123', }, ], }; @@ -771,8 +757,6 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], - typeIndex: 0, - id: '123', }, ], }; @@ -835,8 +819,6 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], - typeIndex: 0, - id: '123', }, ], }; @@ -893,14 +875,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [], - typeIndex: 0, - id: '123', }, { type: KeyringTypes.snap, accounts: [mockAccount3.address], - typeIndex: 0, - id: '123', }, ], }; @@ -937,8 +915,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -994,8 +970,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1027,8 +1001,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1070,8 +1042,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1132,8 +1102,6 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - typeIndex: 0, - id: '123', }, ], }; @@ -1207,8 +1175,6 @@ describe('AccountsController', () => { mockAccountWithoutLastSelected.address, mockAccount2.address, ], - typeIndex: 0, - id: '123', }, ], }; @@ -1277,8 +1243,6 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { - typeIndex: 0, - id: '123', type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], }, @@ -1323,8 +1287,6 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { - typeIndex: 0, - id: '123', type: KeyringTypes.hd, accounts: [mockReinitialisedAccount.address], }, @@ -1418,8 +1380,6 @@ describe('AccountsController', () => { isUnlocked: true, keyrings: [ { - typeIndex: 0, - id: '123', type: KeyringTypes.hd, accounts: [ mockExistingAccount1.address, @@ -2689,14 +2649,10 @@ describe('AccountsController', () => { { type: 'HD Key Tree', accounts: [mockAccount.address], - typeIndex: 0, - id: '123', }, { type: 'Simple Key Pair', accounts: simpleAddressess, - typeIndex: 0, - id: '123', }, ], }; diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 9b817565fe0..0cfd5f186bb 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -60,8 +60,7 @@ "@metamask/utils": "^11.1.0", "async-mutex": "^0.5.0", "ethereumjs-wallet": "^1.0.1", - "immer": "^9.0.6", - "ulid": "^2.3.0" + "immer": "^9.0.6" }, "devDependencies": { "@ethereumjs/common": "^3.2.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index ed9082f7b0f..bbba030eee3 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -74,25 +74,6 @@ const password = 'password123'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; -/** - * Strip the `id` property from each keyring in the state as it is generated - * by the `ulid()` function and is not deterministic. - * - * @param state - The state to strip the `id` property from. - * @returns The state with the `id` property stripped from each keyring. - */ -function stripKeyringIds(state: KeyringControllerState): Omit< - KeyringControllerState, - 'keyrings' -> & { - keyrings: Omit<(typeof state.keyrings)[number], 'id'>[]; -} { - return { - ...state, - keyrings: state.keyrings.map(({ id, ...rest }) => rest), - }; -} - describe('KeyringController', () => { afterEach(() => { sinon.restore(); @@ -427,9 +408,7 @@ describe('KeyringController', () => { password, currentSeedWord, ); - expect(stripKeyringIds(initialState)).toStrictEqual( - stripKeyringIds(controller.state), - ); + expect(initialState).toStrictEqual(controller.state); }, ); }); @@ -998,9 +977,7 @@ describe('KeyringController', () => { const address = '0x51253087e6f8358b5f10c0a94315d69db3357859'; const newKeyring = { accounts: [address], - id: undefined, type: 'Simple Key Pair', - typeIndex: undefined, }; const importedAccountAddress = await controller.importAccountWithStrategy( @@ -1078,9 +1055,7 @@ describe('KeyringController', () => { const newKeyring = { accounts: [address], - id: undefined, type: 'Simple Key Pair', - typeIndex: undefined, }; const modifiedState = { ...initialState, @@ -2084,9 +2059,7 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { await controller.submitPassword(password); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }, ); }); @@ -2188,9 +2161,7 @@ describe('KeyringController', () => { MOCK_ENCRYPTION_KEY, initialState.encryptionSalt as string, ); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }, ); }); @@ -3443,9 +3414,7 @@ describe('KeyringController', () => { controller.submitPassword(password), ]); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }); }); @@ -3458,9 +3427,7 @@ describe('KeyringController', () => { controller.persistAllKeyrings(), ]); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); }); }); @@ -3479,9 +3446,7 @@ describe('KeyringController', () => { await controller.submitPassword(password); - expect(stripKeyringIds(controller.state)).toStrictEqual( - stripKeyringIds(initialState), - ); + expect(controller.state).toStrictEqual(initialState); expect(listener).toHaveBeenCalled(); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index cfb78b04f0b..00ed3d76db4 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -43,7 +43,6 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; -import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; @@ -254,8 +253,6 @@ export type KeyringControllerOptions = { * Keyring object to return in fullUpdate * @property type - Keyring type * @property accounts - Associated accounts - * @property typeIndex - The index of the keyring in the array of keyrings of the same type - * @property id - The id of the keyring */ export type KeyringObject = { /** @@ -266,8 +263,6 @@ export type KeyringObject = { * Keyring type. */ type: string; - typeIndex: number; - id: string; }; /** @@ -400,9 +395,6 @@ export type KeyringSelector = } | { address: Hex; - } - | { - id: string; }; /** @@ -528,24 +520,16 @@ function isSerializedKeyringsArray( * @param keyring - The keyring to display. * @returns A keyring display object, with type and accounts properties. */ -async function displayForKeyring(keyring: EthKeyring): Promise<{ - type: string; - accounts: string[]; - typeIndex: number; - id: string; -}> { +async function displayForKeyring( + keyring: EthKeyring, +): Promise<{ type: string; accounts: string[] }> { const accounts = await keyring.getAccounts(); - const { opts } = keyring as EthKeyring & { - opts: { typeIndex: number; id: string }; - }; return { type: keyring.type, // Cast to `string[]` here is safe here because `accounts` has no nullish // values, and `normalize` returns `string` unless given a nullish value accounts: accounts.map(normalize) as string[], - typeIndex: opts?.typeIndex, - id: opts?.id, }; } @@ -671,23 +655,18 @@ export class KeyringController extends BaseController< * Adds a new account to the default (first) HD seed phrase keyring. * * @param accountCount - Number of accounts before adding a new one, used to - * @param keyringId - The id of the keyring to add the account to. + * @param typeIndex - The id of the keyring to add the account to. * make the method idempotent. * @returns Promise resolving to the added account address. */ async addNewAccount( accountCount?: number, - keyringId?: string, + typeIndex?: number, ): Promise { return this.#persistOrRollback(async () => { - let selectedKeyring: EthKeyring | undefined; - if (keyringId) { - selectedKeyring = this.getKeyringById(keyringId) as EthKeyring; - } else { - selectedKeyring = this.getKeyringsByType( - 'HD Key Tree', - )[0] as EthKeyring; - } + const selectedKeyring = this.getKeyringsByType('HD Key Tree')[ + typeIndex ?? 0 + ] as EthKeyring; if (!selectedKeyring) { throw new Error('No HD keyring found'); } @@ -708,7 +687,7 @@ export class KeyringController extends BaseController< } const [addedAccountAddress] = await selectedKeyring.addAccounts(1); - await this.verifySeedPhrase(); + await this.verifySeedPhrase(typeIndex ?? 0); return addedAccountAddress; }); @@ -903,16 +882,20 @@ export class KeyringController extends BaseController< /** * Returns the public addresses of all accounts from every keyring. * - * @param keyringId - The id of the keyring to get the accounts from. + * @param keyringIndex - The index of the keyring to get the accounts from. * @returns A promise resolving to an array of addresses. */ - async getAccounts(keyringId?: string): Promise { - return this.state.keyrings - .filter((keyring) => (keyringId ? keyring.id === keyringId : true)) - .reduce( - (accounts, keyring) => accounts.concat(keyring.accounts), - [], - ); + async getAccounts(keyringIndex?: number): Promise { + const keyrings = this.state.keyrings.filter( + (keyring) => keyring.type === KeyringTypes.hd, + ); + if (keyringIndex) { + return keyrings[keyringIndex].accounts; + } + return keyrings.reduce( + (accounts, keyring) => accounts.concat(keyring.accounts), + [], + ); } /** @@ -961,24 +944,6 @@ export class KeyringController extends BaseController< return keyring.decryptMessage(address, messageParams.data); } - /** - * Returns the keyring with the given id. - * - * @param id - The id of the keyring to return. - * @returns The keyring with the given id. - */ - getKeyringById(id: string): unknown { - const keyring = this.#keyrings.find( - (item) => - (item as EthKeyring & { opts: { id: string } }).opts.id === id, - ); - if (!keyring) { - throw new Error(KeyringControllerError.KeyringNotFound); - } - - return keyring; - } - /** * Returns the currently initialized keyring that manages * the specified `address` if one exists. @@ -1503,7 +1468,7 @@ export class KeyringController extends BaseController< keyring = (await this.getKeyringForAccount(selector.address)) as | SelectedKeyring | undefined; - } else if ('type' in selector) { + } else { keyring = this.getKeyringsByType(selector.type)[selector.index || 0] as | SelectedKeyring | undefined; @@ -1514,8 +1479,6 @@ export class KeyringController extends BaseController< options.createWithData, )) as SelectedKeyring; } - } else if ('id' in selector) { - keyring = this.getKeyringById(selector.id) as SelectedKeyring; } if (!keyring) { diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index f4459af8e84..2b22a708e3c 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -66,12 +66,7 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, ], }, [], @@ -113,14 +108,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: ['0x00'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], }, [], ); @@ -150,14 +138,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: ['0x00'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], }, [], ); @@ -186,14 +167,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: [], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: [], type: 'CustomKeyring' }], }, [], ); @@ -223,12 +197,7 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, ], }, [], @@ -259,18 +228,8 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - { - accounts: ['0x00', '0x01', '0x02'], - type: 'CustomKeyring', - typeIndex: 1, - id: '456', - }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, ], }, [], @@ -300,14 +259,7 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [ - { - accounts: ['0x00', '0x01'], - type: 'CustomKeyring', - typeIndex: 0, - id: '123', - }, - ], + keyrings: [{ accounts: ['0x00', '0x01'], type: 'CustomKeyring' }], }, [], ); diff --git a/yarn.lock b/yarn.lock index 5e88d88c082..afb813337a7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3283,7 +3283,6 @@ __metadata: typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" typescript: "npm:~5.2.2" - ulid: "npm:^2.3.0" uuid: "npm:^8.3.2" languageName: unknown linkType: soft From af226d57145df5f2c93184ddfc26204de3cb22c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Mon, 13 Jan 2025 13:59:21 +0100 Subject: [PATCH 06/33] feat: add keyringsMetadata --- .../src/AccountsController.test.ts | 127 ++++++++++++- packages/keyring-controller/package.json | 3 +- .../src/KeyringController.test.ts | 178 +++++++++++++++++- .../src/KeyringController.ts | 175 +++++++++++------ .../src/PreferencesController.test.ts | 20 +- yarn.lock | 10 + 6 files changed, 450 insertions(+), 63 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 07c400f9a4e..1a5f1bd6c7a 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -509,6 +509,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; messenger.publish( @@ -533,7 +539,7 @@ describe('AccountsController', () => { messenger.publish( 'KeyringController:stateChange', - { isUnlocked: true, keyrings: [] }, + { isUnlocked: true, keyrings: [], keyringsMetadata: [] }, [], ); @@ -551,6 +557,13 @@ describe('AccountsController', () => { { accounts: [mockAccount.address, mockAccount2.address], type: KeyringTypes.hd, + id: '123', + }, + ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', }, ], }; @@ -589,6 +602,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -646,6 +665,16 @@ describe('AccountsController', () => { accounts: [mockAccount3.address, mockAccount4.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + { + id: 'mock-id2', + name: 'mock-name2', + }, + ], }; const { accountsController } = setupAccountsController({ @@ -713,6 +742,16 @@ describe('AccountsController', () => { accounts: [mockAccount3.address, mockAccount4.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + { + id: 'mock-id2', + name: 'mock-name2', + }, + ], }; const { accountsController } = setupAccountsController({ @@ -759,6 +798,12 @@ describe('AccountsController', () => { ], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -821,6 +866,12 @@ describe('AccountsController', () => { ], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -881,6 +932,16 @@ describe('AccountsController', () => { accounts: [mockAccount3.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + { + id: 'mock-id2', + name: 'mock-name2', + }, + ], }; const { accountsController } = setupAccountsController({ @@ -917,6 +978,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -972,6 +1039,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; messenger.publish( @@ -1003,6 +1076,12 @@ describe('AccountsController', () => { accounts: [mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -1044,6 +1123,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -1104,6 +1189,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -1177,6 +1268,12 @@ describe('AccountsController', () => { ], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -1247,6 +1344,12 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; messenger.publish( 'KeyringController:stateChange', @@ -1291,6 +1394,12 @@ describe('AccountsController', () => { accounts: [mockReinitialisedAccount.address], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; const { accountsController } = setupAccountsController({ initialState: { @@ -1387,6 +1496,12 @@ describe('AccountsController', () => { ], }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + ], }; messenger.publish( 'KeyringController:stateChange', @@ -2655,6 +2770,16 @@ describe('AccountsController', () => { accounts: simpleAddressess, }, ], + keyringsMetadata: [ + { + id: 'mock-id', + name: 'mock-name', + }, + { + id: 'mock-id2', + name: 'mock-name2', + }, + ], }; }; diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 0cfd5f186bb..9b817565fe0 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -60,7 +60,8 @@ "@metamask/utils": "^11.1.0", "async-mutex": "^0.5.0", "ethereumjs-wallet": "^1.0.1", - "immer": "^9.0.6" + "immer": "^9.0.6", + "ulid": "^2.3.0" }, "devDependencies": { "@ethereumjs/common": "^3.2.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index bbba030eee3..314fc7cafbf 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -55,6 +55,11 @@ jest.mock('uuid', () => { }; }); +jest.mock('ulid', () => ({ + ulid: () => 'ULID01234567890ABCDEFGHIJKLMN', + monotonicFactory: () => () => 'ULID01234567890ABCDEFGHIJKLMN', +})); + const input = '{"version":3,"id":"534e0199-53f6-41a9-a8fe-d504702ee5e8","address":"b97c80fab7a3793bbe746864db80d236f1345ea7",' + '"crypto":{"ciphertext":"974fec42023c2d6340d9710863aa82a2961aa03b9d7e5dd19aa77ab4aab1f344",' + @@ -245,6 +250,70 @@ describe('KeyringController', () => { ); }); }); + + describe('when keyringId is provided', () => { + it('should add new account to specified HD keyring', async () => { + await withController(async ({ controller, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + const addedAccountAddress = await controller.addNewAccount( + undefined, + keyringId, + ); + + expect(initialState.keyrings).toHaveLength(1); + expect(initialState.keyrings[0].accounts).not.toStrictEqual( + controller.state.keyrings[0].accounts, + ); + expect(controller.state.keyrings[0].accounts).toHaveLength(2); + expect(initialState.keyrings[0].accounts).not.toContain( + addedAccountAddress, + ); + expect(addedAccountAddress).toBe( + controller.state.keyrings[0].accounts[1], + ); + }); + }); + + it('should throw error if keyringId is invalid', async () => { + await withController(async ({ controller }) => { + await expect( + controller.addNewAccount(undefined, 'invalid-id'), + ).rejects.toThrow('No HD keyring found'); + }); + }); + + it('should throw error if accountCount is out of sequence for specified keyring', async () => { + await withController(async ({ controller, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + const accountCount = initialState.keyrings[0].accounts.length; + + await expect( + controller.addNewAccount(accountCount + 1, keyringId), + ).rejects.toThrow('Account out of sequence'); + }); + }); + + it('should return existing account if called twice with same accountCount for specified keyring', async () => { + await withController(async ({ controller, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + const accountCount = initialState.keyrings[0].accounts.length; + + const firstAccountAdded = await controller.addNewAccount( + accountCount, + keyringId, + ); + const secondAccountAdded = await controller.addNewAccount( + accountCount, + keyringId, + ); + + expect(firstAccountAdded).toBe(secondAccountAdded); + expect(controller.state.keyrings[0].accounts).toHaveLength( + accountCount + 1, + ); + }); + }); + }); }); describe('addNewAccountForKeyring', () => { @@ -651,12 +720,28 @@ describe('KeyringController', () => { describe('when mnemonic is exportable', () => { describe('when correct password is provided', () => { - it('should export seed phrase', async () => { + it('should export seed phrase without keyringId', async () => { await withController(async ({ controller }) => { const seed = await controller.exportSeedPhrase(password); expect(seed).not.toBe(''); }); }); + + it('should export seed phrase with valid keyringId', async () => { + await withController(async ({ controller, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + const seed = await controller.exportSeedPhrase(password, keyringId); + expect(seed).not.toBe(''); + }); + }); + + it('should throw error if keyringId is invalid', async () => { + await withController(async ({ controller }) => { + await expect( + controller.exportSeedPhrase(password, 'invalid-id') + ).rejects.toThrow('Keyring not found'); + }); + }); }); describe('when wrong password is provided', () => { @@ -670,6 +755,18 @@ describe('KeyringController', () => { ); }); }); + + it('should throw invalid password error with valid keyringId', async () => { + await withController(async ({ controller, encryptor, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + sinon + .stub(encryptor, 'decrypt') + .throws(new Error('Invalid password')); + await expect( + controller.exportSeedPhrase('', keyringId) + ).rejects.toThrow('Invalid password'); + }); + }); }); }); }); @@ -751,6 +848,22 @@ describe('KeyringController', () => { expect(accounts).toStrictEqual(initialAccount); }); }); + + it('should get accounts for specific keyring when keyringId is provided', async () => { + await withController(async ({ controller, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + const keyringAccounts = await controller.getAccounts(keyringId); + expect(keyringAccounts).toStrictEqual(initialState.keyrings[0].accounts); + }); + }); + + it('should throw error if keyringId is invalid', async () => { + await withController(async ({ controller }) => { + await expect(controller.getAccounts('invalid-id')).rejects.toThrow( + 'Keyring not found', + ); + }); + }); }); describe('getEncryptionPublicKey', () => { @@ -987,6 +1100,7 @@ describe('KeyringController', () => { const modifiedState = { ...initialState, keyrings: [initialState.keyrings[0], newKeyring], + keyringsMetadata: [initialState.keyringsMetadata[0], initialState.keyringsMetadata[0]], }; expect(controller.state).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); @@ -1060,6 +1174,7 @@ describe('KeyringController', () => { const modifiedState = { ...initialState, keyrings: [initialState.keyrings[0], newKeyring], + keyringsMetadata: [initialState.keyringsMetadata[0], initialState.keyringsMetadata[0]], }; expect(controller.state).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); @@ -2457,6 +2572,67 @@ describe('KeyringController', () => { ); }); }); + + describe('when the keyring is selected by id', () => { + it('should call the given function with the selected keyring', async () => { + await withController(async ({ controller, initialState }) => { + const fn = jest.fn(); + const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0]; + const selector = { id: initialState.keyringsMetadata[0].id }; + + await controller.withKeyring(selector, fn); + + expect(fn).toHaveBeenCalledWith(keyring); + }); + }); + + it('should return the result of the function', async () => { + await withController(async ({ controller, initialState }) => { + const fn = async () => Promise.resolve('hello'); + const selector = { id: initialState.keyringsMetadata[0].id }; + + expect(await controller.withKeyring(selector, fn)).toBe('hello'); + }); + }); + + it('should throw an error if the callback returns the selected keyring', async () => { + await withController(async ({ controller, initialState }) => { + const selector = { id: initialState.keyringsMetadata[0].id }; + + await expect( + controller.withKeyring(selector, async (selectedKeyring) => { + return selectedKeyring; + }), + ).rejects.toThrow(KeyringControllerError.UnsafeDirectKeyringAccess); + }); + }); + + describe('when the keyring is not found', () => { + it('should throw an error if the keyring is not found and `createIfMissing` is false', async () => { + await withController(async ({ controller, initialState }) => { + const selector = { id: 'non-existent-id' }; + const fn = jest.fn(); + + await expect(controller.withKeyring(selector, fn)).rejects.toThrow( + KeyringControllerError.KeyringNotFound, + ); + expect(fn).not.toHaveBeenCalled(); + }); + }); + + it('should throw an error even if `createIfMissing` is true', async () => { + await withController(async ({ controller, initialState }) => { + const selector = { id: 'non-existent-id' }; + const fn = jest.fn(); + + await expect( + controller.withKeyring(selector, fn, { createIfMissing: true }), + ).rejects.toThrow(KeyringControllerError.KeyringNotFound); + expect(fn).not.toHaveBeenCalled(); + }); + }); + }); + }); }); describe('QR keyring', () => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 00ed3d76db4..043674288ca 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -43,9 +43,12 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; +// When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. +import { monotonicFactory } from 'ulid'; import { KeyringControllerError } from './constants'; +const ulid = monotonicFactory(); const name = 'KeyringController'; /** @@ -89,6 +92,10 @@ export type KeyringControllerState = { * Representations of managed keyrings. */ keyrings: KeyringObject[]; + /** + * Metadata for each keyring. + */ + keyringsMetadata: KeyringMetadata[]; /** * The encryption key derived from the password and used to encrypt * the vault. This is only stored if the `cacheEncryptionKey` option @@ -265,6 +272,18 @@ export type KeyringObject = { type: string; }; +/** + * @type KeyringMetadata + * + * Keyring metadata + * @property id - Keyring ID + * @property name - Keyring name + */ +export type KeyringMetadata = { + id: string; + name: string; +}; + /** * A strategy for importing an account */ @@ -290,6 +309,7 @@ export enum SignTypedDataVersion { export type SerializedKeyring = { type: string; data: Json; + metadata: KeyringMetadata; }; /** @@ -395,6 +415,9 @@ export type KeyringSelector = } | { address: Hex; + } + | { + id: string; }; /** @@ -434,6 +457,7 @@ export const getDefaultKeyringState = (): KeyringControllerState => { return { isUnlocked: false, keyrings: [], + keyringsMetadata: [], }; }; @@ -623,6 +647,7 @@ export class KeyringController extends BaseController< vault: { persist: true, anonymous: false }, isUnlocked: { persist: false, anonymous: true }, keyrings: { persist: false, anonymous: false }, + keyringsMetadata: { persist: false, anonymous: false }, encryptionKey: { persist: false, anonymous: false }, encryptionSalt: { persist: false, anonymous: false }, }, @@ -639,6 +664,7 @@ export class KeyringController extends BaseController< this.#encryptor = encryptor; this.#keyrings = []; + this.#keyringsMetadata = []; this.#unsupportedKeyrings = []; // This option allows the controller to cache an exported key @@ -655,18 +681,21 @@ export class KeyringController extends BaseController< * Adds a new account to the default (first) HD seed phrase keyring. * * @param accountCount - Number of accounts before adding a new one, used to - * @param typeIndex - The id of the keyring to add the account to. + * @param keyringId - The id of the keyring to add the account to. * make the method idempotent. * @returns Promise resolving to the added account address. */ async addNewAccount( accountCount?: number, - typeIndex?: number, + keyringId?: string, ): Promise { return this.#persistOrRollback(async () => { - const selectedKeyring = this.getKeyringsByType('HD Key Tree')[ - typeIndex ?? 0 - ] as EthKeyring; + let selectedKeyring = this.getKeyringsByType( + 'HD Key Tree', + )[0] as EthKeyring; + if (keyringId) { + selectedKeyring = this.#getKeyringById(keyringId); + } if (!selectedKeyring) { throw new Error('No HD keyring found'); } @@ -687,7 +716,7 @@ export class KeyringController extends BaseController< } const [addedAccountAddress] = await selectedKeyring.addAccounts(1); - await this.verifySeedPhrase(typeIndex ?? 0); + await this.#verifySeedPhrase(keyringId); return addedAccountAddress; }); @@ -759,15 +788,6 @@ export class KeyringController extends BaseController< }); } - async createKeyringFromMnemonic(mnemonic: string): Promise { - return this.#persistOrRollback(async () => { - return await this.#createKeyringWithFirstAccount(KeyringTypes.hd, { - mnemonic, - numberOfAccounts: 1, - }); - }); - } - /** * Create a new vault and primary keyring. * @@ -804,7 +824,9 @@ export class KeyringController extends BaseController< return this.getOrAddQRKeyring(); } - return this.#persistOrRollback(async () => this.#newKeyring(type, opts)); + return this.#persistOrRollback(async () => + this.#newKeyring(type, opts, { id: ulid(), name: '' }), + ); } /** @@ -833,28 +855,23 @@ export class KeyringController extends BaseController< * Gets the seed phrase of the HD keyring. * * @param password - Password of the keyring. - * @param typeIndex - The index of the keyring type. + * @param keyringId - The id of the keyring. * @returns Promise resolving to the seed phrase. */ async exportSeedPhrase( password: string, - typeIndex?: number, + keyringId?: string, ): Promise { await this.verifyPassword(password); - if (!typeIndex) { + if (!keyringId) { assertHasUint8ArrayMnemonic(this.#keyrings[0]); return this.#keyrings[0].mnemonic; } - const selectedKeyring = ( - this.#keyrings as (EthKeyring & { - opts: { typeIndex: number }; - mnemonic: Uint8Array; - })[] - ).filter((keyring) => keyring.type === KeyringTypes.hd)[typeIndex]; + const selectedKeyring = this.#getKeyringById(keyringId); if (!selectedKeyring) { throw new Error('Keyring not found'); } - assertHasUint8ArrayMnemonic(selectedKeyring as EthKeyring); + assertHasUint8ArrayMnemonic(selectedKeyring); return selectedKeyring.mnemonic; } @@ -882,17 +899,21 @@ export class KeyringController extends BaseController< /** * Returns the public addresses of all accounts from every keyring. * - * @param keyringIndex - The index of the keyring to get the accounts from. + * @param keyringId - The id of the keyring to get the accounts from. * @returns A promise resolving to an array of addresses. */ - async getAccounts(keyringIndex?: number): Promise { - const keyrings = this.state.keyrings.filter( - (keyring) => keyring.type === KeyringTypes.hd, - ); - if (keyringIndex) { - return keyrings[keyringIndex].accounts; + async getAccounts(keyringId?: string): Promise { + if (keyringId) { + const keyringIndex = this.state.keyringsMetadata.findIndex( + (keyring) => keyring.id === keyringId, + ); + const keyring = this.state.keyrings[keyringIndex]; + if (!keyring) { + throw new Error('Keyring not found'); + } + return keyring.accounts; } - return keyrings.reduce( + return this.state.keyrings.reduce( (accounts, keyring) => accounts.concat(keyring.accounts), [], ); @@ -1064,7 +1085,7 @@ export class KeyringController extends BaseController< } const newKeyring = (await this.#newKeyring(KeyringTypes.simple, [ privateKey, - ])) as EthKeyring; + ], { id: ulid(), name: '' })) as EthKeyring; const accounts = await newKeyring.getAccounts(); return accounts[0]; }); @@ -1388,11 +1409,13 @@ export class KeyringController extends BaseController< /** * Verifies the that the seed phrase restores the current keychain's accounts. * - * @param typeIndex - The index of the keyring to verify. + * @param keyringId - The id of the keyring to verify. * @returns Promise resolving to the seed phrase as Uint8Array. */ - async verifySeedPhrase(): Promise { - return this.#withControllerLock(async () => this.#verifySeedPhrase()); + async verifySeedPhrase(keyringId?: string): Promise { + return this.#withControllerLock(async () => + this.#verifySeedPhrase(keyringId), + ); } /** @@ -1468,17 +1491,21 @@ export class KeyringController extends BaseController< keyring = (await this.getKeyringForAccount(selector.address)) as | SelectedKeyring | undefined; - } else { + } else if ('type' in selector) { keyring = this.getKeyringsByType(selector.type)[selector.index || 0] as | SelectedKeyring | undefined; if (!keyring && options.createIfMissing) { + const newMetadata = { id: ulid(), name: '' }; keyring = (await this.#newKeyring( selector.type, options.createWithData, + newMetadata, )) as SelectedKeyring; } + } else if ('id' in selector) { + keyring = this.#getKeyringById(selector.id) as SelectedKeyring; } if (!keyring) { @@ -1785,6 +1812,19 @@ export class KeyringController extends BaseController< ); } + /** + * Get the keyring by id. + * + * @param keyringId - The id of the keyring. + * @returns The keyring. + */ + #getKeyringById(keyringId: string): EthKeyring { + const index = this.#keyringsMetadata.findIndex( + (metadata) => metadata.id === keyringId, + ); + return this.#keyrings[index] as EthKeyring; + } + /** * Get the keyring builder for the given `type`. * @@ -1810,7 +1850,10 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); // QRKeyring is not yet compatible with Keyring type from @metamask/utils - return (await this.#newKeyring(KeyringTypes.qr)) as unknown as QRKeyring; + return (await this.#newKeyring(KeyringTypes.qr, undefined, { + id: ulid(), + name: '', + })) as unknown as QRKeyring; } /** @@ -1881,22 +1924,25 @@ export class KeyringController extends BaseController< /** * Internal non-exclusive method to verify the seed phrase. * + * @param keyringId - The id of the keyring to verify the seed phrase for. * @returns A promise resolving to the seed phrase as Uint8Array. */ - async #verifySeedPhrase(): Promise { + async #verifySeedPhrase(keyringId?: string): Promise { this.#assertControllerMutexIsLocked(); - const primaryKeyring = this.getKeyringsByType(KeyringTypes.hd)[0] as - | EthKeyring - | undefined; - if (!primaryKeyring) { + const keyring = keyringId + ? this.#getKeyringById(keyringId) + : this.getKeyringsByType(KeyringTypes.hd)[0] as + | EthKeyring + | undefined; + if (!keyring) { throw new Error('No HD keyring found.'); } - assertHasUint8ArrayMnemonic(primaryKeyring); + assertHasUint8ArrayMnemonic(keyring); - const seedWords = primaryKeyring.mnemonic; - const accounts = await primaryKeyring.getAccounts(); + const seedWords = keyring.mnemonic; + const accounts = await keyring.getAccounts(); /* istanbul ignore if */ if (accounts.length === 0) { throw new Error('Cannot verify an empty keyring.'); @@ -1953,12 +1999,13 @@ export class KeyringController extends BaseController< }, ): Promise { const serializedKeyrings = await Promise.all( - this.#keyrings.map(async (keyring) => { - const [type, data] = await Promise.all([ + this.#keyrings.map(async (keyring, index) => { + const [type, data, metadata] = await Promise.all([ keyring.type, keyring.serialize(), + this.#keyringsMetadata[index], ]); - return { type, data }; + return { type, data, metadata }; }), ); @@ -2060,9 +2107,11 @@ export class KeyringController extends BaseController< await this.#restoreSerializedKeyrings(vault); const updatedKeyrings = await this.#getUpdatedKeyrings(); + const updatedKeyringsMetadata = this.#keyringsMetadata; this.update((state) => { state.keyrings = updatedKeyrings; + state.keyringsMetadata = updatedKeyringsMetadata; if (updatedState.encryptionKey || updatedState.encryptionSalt) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = updatedState.encryptionSalt; @@ -2143,9 +2192,11 @@ export class KeyringController extends BaseController< } const updatedKeyrings = await this.#getUpdatedKeyrings(); + const updatedKeyringsMetadata = this.#keyringsMetadata; this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; + state.keyringsMetadata = updatedKeyringsMetadata; if (updatedState.encryptionKey) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; @@ -2188,7 +2239,10 @@ export class KeyringController extends BaseController< async #createKeyringWithFirstAccount(type: string, opts?: unknown) { this.#assertControllerMutexIsLocked(); - const keyring = (await this.#newKeyring(type, opts)) as EthKeyring; + const keyring = (await this.#newKeyring(type, opts, { + id: ulid(), + name: '', + })) as EthKeyring; const [firstAccount] = await keyring.getAccounts(); if (!firstAccount) { @@ -2205,10 +2259,15 @@ export class KeyringController extends BaseController< * * @param type - The type of keyring to add. * @param data - The data to restore a previously serialized keyring. + * @param metadata - The metadata to add to the keyring. * @returns The new keyring. * @throws If the keyring includes duplicated accounts. */ - async #newKeyring(type: string, data?: unknown): Promise> { + async #newKeyring( + type: string, + data?: unknown, + metadata?: KeyringMetadata, + ): Promise> { this.#assertControllerMutexIsLocked(); const keyringBuilder = this.#getKeyringBuilderForType(type); @@ -2250,6 +2309,9 @@ export class KeyringController extends BaseController< } this.#keyrings.push(keyring); + if (metadata && this.#keyringsMetadata.length < this.#keyrings.length) { + this.#keyringsMetadata = [...this.#keyringsMetadata, metadata]; + } return keyring; } @@ -2279,8 +2341,8 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); try { - const { type, data } = serialized; - return await this.#newKeyring(type, data); + const { type, data, metadata } = serialized; + return await this.#newKeyring(type, data, metadata); } catch (_) { this.#unsupportedKeyrings.push(serialized); return undefined; @@ -2315,12 +2377,13 @@ export class KeyringController extends BaseController< // in order to decide which ones are now valid (accounts.length > 0) await Promise.all( - this.#keyrings.map(async (keyring: EthKeyring) => { + this.#keyrings.map(async (keyring: EthKeyring, index: number) => { const accounts = await keyring.getAccounts(); if (accounts.length > 0) { validKeyrings.push(keyring); } else { await this.#destroyKeyring(keyring); + this.#keyringsMetadata = this.#keyringsMetadata.filter((_, i) => i !== index); } }), ); diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index 2b22a708e3c..30ff3ea64e8 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -66,7 +66,10 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + }, ], }, [], @@ -197,7 +200,10 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + }, ], }, [], @@ -228,8 +234,14 @@ describe('PreferencesController', () => { { ...getDefaultKeyringState(), keyrings: [ - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, - { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring' }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + }, + { + accounts: ['0x00', '0x01', '0x02'], + type: 'CustomKeyring', + }, ], }, [], diff --git a/yarn.lock b/yarn.lock index afb813337a7..735e2b30a45 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3283,6 +3283,7 @@ __metadata: typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" typescript: "npm:~5.2.2" + ulid: "npm:^2.3.0" uuid: "npm:^8.3.2" languageName: unknown linkType: soft @@ -13158,6 +13159,15 @@ __metadata: languageName: node linkType: hard +"ulid@npm:^2.3.0": + version: 2.3.0 + resolution: "ulid@npm:2.3.0" + bin: + ulid: ./bin/cli.js + checksum: 10/11d7dd35072b863effb1249f66fb03070142a625610f00e5afd99af7e909b5de9cc7ebca6ede621a6bb1b7479b2489d6f064db6742b55c14bff6496ac60f290f + languageName: node + linkType: hard + "undici-types@npm:~5.26.4": version: 5.26.5 resolution: "undici-types@npm:5.26.5" From 3062aeb4b1e958bf07586ffbc7dbde93561b4991 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Tue, 14 Jan 2025 14:35:27 +0100 Subject: [PATCH 07/33] feat: keep metadata in state only --- .../src/KeyringController.ts | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 043674288ca..5ae51d326b9 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -309,7 +309,6 @@ export enum SignTypedDataVersion { export type SerializedKeyring = { type: string; data: Json; - metadata: KeyringMetadata; }; /** @@ -617,6 +616,8 @@ export class KeyringController extends BaseController< #keyrings: EthKeyring[]; + #unsupportedKeyrings: SerializedKeyring[]; + #password?: string; #qrKeyringStateListener?: ( @@ -647,7 +648,7 @@ export class KeyringController extends BaseController< vault: { persist: true, anonymous: false }, isUnlocked: { persist: false, anonymous: true }, keyrings: { persist: false, anonymous: false }, - keyringsMetadata: { persist: false, anonymous: false }, + keyringsMetadata: { persist: true, anonymous: false }, encryptionKey: { persist: false, anonymous: false }, encryptionSalt: { persist: false, anonymous: false }, }, @@ -664,7 +665,6 @@ export class KeyringController extends BaseController< this.#encryptor = encryptor; this.#keyrings = []; - this.#keyringsMetadata = []; this.#unsupportedKeyrings = []; // This option allows the controller to cache an exported key @@ -1819,7 +1819,7 @@ export class KeyringController extends BaseController< * @returns The keyring. */ #getKeyringById(keyringId: string): EthKeyring { - const index = this.#keyringsMetadata.findIndex( + const index = this.state.keyringsMetadata.findIndex( (metadata) => metadata.id === keyringId, ); return this.#keyrings[index] as EthKeyring; @@ -2000,12 +2000,11 @@ export class KeyringController extends BaseController< ): Promise { const serializedKeyrings = await Promise.all( this.#keyrings.map(async (keyring, index) => { - const [type, data, metadata] = await Promise.all([ + const [type, data] = await Promise.all([ keyring.type, keyring.serialize(), - this.#keyringsMetadata[index], ]); - return { type, data, metadata }; + return { type, data }; }), ); @@ -2029,6 +2028,13 @@ export class KeyringController extends BaseController< for (const serializedKeyring of serializedKeyrings) { await this.#restoreKeyring(serializedKeyring); } + + if (this.state.keyringsMetadata.length > this.#keyrings.length) { + this.update((state) => { + // remove metadata from the end of the array to have the same length as the keyrings array + state.keyringsMetadata = state.keyringsMetadata.slice(0, -1 * (state.keyringsMetadata.length - this.#keyrings.length)); + }) + } } /** @@ -2107,11 +2113,9 @@ export class KeyringController extends BaseController< await this.#restoreSerializedKeyrings(vault); const updatedKeyrings = await this.#getUpdatedKeyrings(); - const updatedKeyringsMetadata = this.#keyringsMetadata; this.update((state) => { state.keyrings = updatedKeyrings; - state.keyringsMetadata = updatedKeyringsMetadata; if (updatedState.encryptionKey || updatedState.encryptionSalt) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = updatedState.encryptionSalt; @@ -2192,15 +2196,16 @@ export class KeyringController extends BaseController< } const updatedKeyrings = await this.#getUpdatedKeyrings(); - const updatedKeyringsMetadata = this.#keyringsMetadata; this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; - state.keyringsMetadata = updatedKeyringsMetadata; if (updatedState.encryptionKey) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; } + if (updatedKeyrings.length < state.keyringsMetadata.length) { + state.keyringsMetadata = state.keyringsMetadata.slice(0, -1 * (state.keyringsMetadata.length - updatedKeyrings.length)); + } }); return true; @@ -2309,10 +2314,11 @@ export class KeyringController extends BaseController< } this.#keyrings.push(keyring); - if (metadata && this.#keyringsMetadata.length < this.#keyrings.length) { - this.#keyringsMetadata = [...this.#keyringsMetadata, metadata]; + if (metadata && this.state.keyringsMetadata.length < this.#keyrings.length) { + this.update((state) => { + state.keyringsMetadata = [...state.keyringsMetadata, metadata]; + }); } - return keyring; } @@ -2341,8 +2347,8 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); try { - const { type, data, metadata } = serialized; - return await this.#newKeyring(type, data, metadata); + const { type, data } = serialized; + return await this.#newKeyring(type, data); } catch (_) { this.#unsupportedKeyrings.push(serialized); return undefined; @@ -2383,7 +2389,6 @@ export class KeyringController extends BaseController< validKeyrings.push(keyring); } else { await this.#destroyKeyring(keyring); - this.#keyringsMetadata = this.#keyringsMetadata.filter((_, i) => i !== index); } }), ); From ad3e9673b270e1c5f9ec8f790b3f897dfb75d0dd Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 22 Jan 2025 21:13:06 +0800 Subject: [PATCH 08/33] fix: revert keyringId arguments in getAccounts, and addNewAccount. Remove keyringMetadata options in #newKeyring --- .../src/KeyringController.ts | 89 ++++++++----------- 1 file changed, 39 insertions(+), 50 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 5ae51d326b9..ee9ca13c5e4 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -681,24 +681,14 @@ export class KeyringController extends BaseController< * Adds a new account to the default (first) HD seed phrase keyring. * * @param accountCount - Number of accounts before adding a new one, used to - * @param keyringId - The id of the keyring to add the account to. * make the method idempotent. * @returns Promise resolving to the added account address. */ - async addNewAccount( - accountCount?: number, - keyringId?: string, - ): Promise { + async addNewAccount(accountCount?: number): Promise { return this.#persistOrRollback(async () => { - let selectedKeyring = this.getKeyringsByType( + const selectedKeyring = this.getKeyringsByType( 'HD Key Tree', )[0] as EthKeyring; - if (keyringId) { - selectedKeyring = this.#getKeyringById(keyringId); - } - if (!selectedKeyring) { - throw new Error('No HD keyring found'); - } const oldAccounts = await selectedKeyring.getAccounts(); if (accountCount && oldAccounts.length !== accountCount) { @@ -716,7 +706,7 @@ export class KeyringController extends BaseController< } const [addedAccountAddress] = await selectedKeyring.addAccounts(1); - await this.#verifySeedPhrase(keyringId); + await this.#verifySeedPhrase(); return addedAccountAddress; }); @@ -824,9 +814,7 @@ export class KeyringController extends BaseController< return this.getOrAddQRKeyring(); } - return this.#persistOrRollback(async () => - this.#newKeyring(type, opts, { id: ulid(), name: '' }), - ); + return this.#persistOrRollback(async () => this.#newKeyring(type, opts)); } /** @@ -899,20 +887,9 @@ export class KeyringController extends BaseController< /** * Returns the public addresses of all accounts from every keyring. * - * @param keyringId - The id of the keyring to get the accounts from. * @returns A promise resolving to an array of addresses. */ - async getAccounts(keyringId?: string): Promise { - if (keyringId) { - const keyringIndex = this.state.keyringsMetadata.findIndex( - (keyring) => keyring.id === keyringId, - ); - const keyring = this.state.keyrings[keyringIndex]; - if (!keyring) { - throw new Error('Keyring not found'); - } - return keyring.accounts; - } + async getAccounts(): Promise { return this.state.keyrings.reduce( (accounts, keyring) => accounts.concat(keyring.accounts), [], @@ -1085,7 +1062,7 @@ export class KeyringController extends BaseController< } const newKeyring = (await this.#newKeyring(KeyringTypes.simple, [ privateKey, - ], { id: ulid(), name: '' })) as EthKeyring; + ])) as EthKeyring; const accounts = await newKeyring.getAccounts(); return accounts[0]; }); @@ -1413,6 +1390,15 @@ export class KeyringController extends BaseController< * @returns Promise resolving to the seed phrase as Uint8Array. */ async verifySeedPhrase(keyringId?: string): Promise { + const keyringIndex = this.state.keyringsMetadata.findIndex( + (keyring) => keyring.id === keyringId, + ); + const keyring = this.state.keyrings[keyringIndex]; + + if (keyring.type !== KeyringTypes.hd) { + throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); + } + return this.#withControllerLock(async () => this.#verifySeedPhrase(keyringId), ); @@ -1818,11 +1804,11 @@ export class KeyringController extends BaseController< * @param keyringId - The id of the keyring. * @returns The keyring. */ - #getKeyringById(keyringId: string): EthKeyring { + #getKeyringById(keyringId: string): EthKeyring | undefined { const index = this.state.keyringsMetadata.findIndex( (metadata) => metadata.id === keyringId, ); - return this.#keyrings[index] as EthKeyring; + return this.#keyrings[index]; } /** @@ -1850,10 +1836,10 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); // QRKeyring is not yet compatible with Keyring type from @metamask/utils - return (await this.#newKeyring(KeyringTypes.qr, undefined, { - id: ulid(), - name: '', - })) as unknown as QRKeyring; + return (await this.#newKeyring( + KeyringTypes.qr, + undefined, + )) as unknown as QRKeyring; } /** @@ -1932,13 +1918,18 @@ export class KeyringController extends BaseController< const keyring = keyringId ? this.#getKeyringById(keyringId) - : this.getKeyringsByType(KeyringTypes.hd)[0] as + : (this.getKeyringsByType(KeyringTypes.hd)[0] as | EthKeyring - | undefined; + | undefined); + if (!keyring) { throw new Error('No HD keyring found.'); } + if ((keyring.type as KeyringTypes) !== KeyringTypes.hd) { + throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); + } + assertHasUint8ArrayMnemonic(keyring); const seedWords = keyring.mnemonic; @@ -2244,10 +2235,7 @@ export class KeyringController extends BaseController< async #createKeyringWithFirstAccount(type: string, opts?: unknown) { this.#assertControllerMutexIsLocked(); - const keyring = (await this.#newKeyring(type, opts, { - id: ulid(), - name: '', - })) as EthKeyring; + const keyring = (await this.#newKeyring(type, opts)) as EthKeyring; const [firstAccount] = await keyring.getAccounts(); if (!firstAccount) { @@ -2261,19 +2249,14 @@ export class KeyringController extends BaseController< * using the given `opts`. The keyring is built using the keyring builder * registered for the given `type`. * - * * @param type - The type of keyring to add. * @param data - The data to restore a previously serialized keyring. - * @param metadata - The metadata to add to the keyring. * @returns The new keyring. * @throws If the keyring includes duplicated accounts. */ - async #newKeyring( - type: string, - data?: unknown, - metadata?: KeyringMetadata, - ): Promise> { + async #newKeyring(type: string, data?: unknown): Promise> { this.#assertControllerMutexIsLocked(); + const newKeyringMetadata = { id: ulid(), name: '' }; const keyringBuilder = this.#getKeyringBuilderForType(type); @@ -2314,9 +2297,15 @@ export class KeyringController extends BaseController< } this.#keyrings.push(keyring); - if (metadata && this.state.keyringsMetadata.length < this.#keyrings.length) { + if ( + newKeyringMetadata && + this.state.keyringsMetadata.length < this.#keyrings.length + ) { this.update((state) => { - state.keyringsMetadata = [...state.keyringsMetadata, metadata]; + state.keyringsMetadata = [ + ...state.keyringsMetadata, + newKeyringMetadata, + ]; }); } return keyring; From 3813a19e91604fc811e1f23a8a5732a5e0153343 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 22 Jan 2025 21:14:01 +0800 Subject: [PATCH 09/33] fix: handle keyringMetadata cleanup when removing emptyKeyrings and clearKeyrings --- packages/keyring-controller/src/KeyringController.ts | 9 +++++++++ packages/keyring-controller/src/constants.ts | 1 + 2 files changed, 10 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index ee9ca13c5e4..3a071f8463b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2321,6 +2321,9 @@ export class KeyringController extends BaseController< await this.#destroyKeyring(keyring); } this.#keyrings = []; + this.update((state) => { + state.keyringsMetadata = []; + }); } /** @@ -2366,6 +2369,7 @@ export class KeyringController extends BaseController< async #removeEmptyKeyrings(): Promise { this.#assertControllerMutexIsLocked(); const validKeyrings: EthKeyring[] = []; + const validKeyringMetadata: KeyringMetadata[] = []; // Since getAccounts returns a Promise // We need to wait to hear back form each keyring @@ -2376,12 +2380,17 @@ export class KeyringController extends BaseController< const accounts = await keyring.getAccounts(); if (accounts.length > 0) { validKeyrings.push(keyring); + validKeyringMetadata.push(this.state.keyringsMetadata[index]); } else { await this.#destroyKeyring(keyring); } }), ); this.#keyrings = validKeyrings; + + this.update((state) => { + state.keyringsMetadata = validKeyringMetadata; + }); } /** diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index fe58710cfaa..784eddb14c9 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -23,6 +23,7 @@ export enum KeyringControllerError { UnsupportedPrepareUserOperation = 'KeyringController - The keyring for the current address does not support the method prepareUserOperation.', UnsupportedPatchUserOperation = 'KeyringController - The keyring for the current address does not support the method patchUserOperation.', UnsupportedSignUserOperation = 'KeyringController - The keyring for the current address does not support the method signUserOperation.', + UnsupportedVerifySeedPhrase = 'KeyringController - The keyring does not support the method verifySeedPhrase.', NoAccountOnKeychain = "KeyringController - The keychain doesn't have accounts.", MissingCredentials = 'KeyringController - Cannot persist vault without password and encryption key', MissingVaultData = 'KeyringController - Cannot persist vault without vault information', From c1409bd7b6b3f6c472c8685db3a1c5668a40f939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 5 Feb 2025 14:50:09 +0100 Subject: [PATCH 10/33] fix: remove keyringmetadata misalignement logic --- .../src/KeyringController.test.ts | 173 ++++++------------ .../src/KeyringController.ts | 71 +++---- packages/keyring-controller/src/constants.ts | 1 + 3 files changed, 92 insertions(+), 153 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 314fc7cafbf..9c2affb96c9 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -55,10 +55,12 @@ jest.mock('uuid', () => { }; }); -jest.mock('ulid', () => ({ - ulid: () => 'ULID01234567890ABCDEFGHIJKLMN', - monotonicFactory: () => () => 'ULID01234567890ABCDEFGHIJKLMN', -})); +jest.mock('ulid', () => { + return { + ulid: () => 'ULID01234567890ABCDEFGHIJKLMN', + monotonicFactory: () => () => 'ULID01234567890ABCDEFGHIJKLMN', + }; +}); const input = '{"version":3,"id":"534e0199-53f6-41a9-a8fe-d504702ee5e8","address":"b97c80fab7a3793bbe746864db80d236f1345ea7",' + @@ -82,6 +84,7 @@ const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; describe('KeyringController', () => { afterEach(() => { sinon.restore(); + jest.clearAllMocks(); }); describe('constructor', () => { @@ -250,70 +253,6 @@ describe('KeyringController', () => { ); }); }); - - describe('when keyringId is provided', () => { - it('should add new account to specified HD keyring', async () => { - await withController(async ({ controller, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; - const addedAccountAddress = await controller.addNewAccount( - undefined, - keyringId, - ); - - expect(initialState.keyrings).toHaveLength(1); - expect(initialState.keyrings[0].accounts).not.toStrictEqual( - controller.state.keyrings[0].accounts, - ); - expect(controller.state.keyrings[0].accounts).toHaveLength(2); - expect(initialState.keyrings[0].accounts).not.toContain( - addedAccountAddress, - ); - expect(addedAccountAddress).toBe( - controller.state.keyrings[0].accounts[1], - ); - }); - }); - - it('should throw error if keyringId is invalid', async () => { - await withController(async ({ controller }) => { - await expect( - controller.addNewAccount(undefined, 'invalid-id'), - ).rejects.toThrow('No HD keyring found'); - }); - }); - - it('should throw error if accountCount is out of sequence for specified keyring', async () => { - await withController(async ({ controller, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; - const accountCount = initialState.keyrings[0].accounts.length; - - await expect( - controller.addNewAccount(accountCount + 1, keyringId), - ).rejects.toThrow('Account out of sequence'); - }); - }); - - it('should return existing account if called twice with same accountCount for specified keyring', async () => { - await withController(async ({ controller, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; - const accountCount = initialState.keyrings[0].accounts.length; - - const firstAccountAdded = await controller.addNewAccount( - accountCount, - keyringId, - ); - const secondAccountAdded = await controller.addNewAccount( - accountCount, - keyringId, - ); - - expect(firstAccountAdded).toBe(secondAccountAdded); - expect(controller.state.keyrings[0].accounts).toHaveLength( - accountCount + 1, - ); - }); - }); - }); }); describe('addNewAccountForKeyring', () => { @@ -477,7 +416,7 @@ describe('KeyringController', () => { password, currentSeedWord, ); - expect(initialState).toStrictEqual(controller.state); + expect(initialState.vault).toStrictEqual(controller.state.vault); }, ); }); @@ -726,7 +665,7 @@ describe('KeyringController', () => { expect(seed).not.toBe(''); }); }); - + it('should export seed phrase with valid keyringId', async () => { await withController(async ({ controller, initialState }) => { const keyringId = initialState.keyringsMetadata[0].id; @@ -734,7 +673,7 @@ describe('KeyringController', () => { expect(seed).not.toBe(''); }); }); - + it('should throw error if keyringId is invalid', async () => { await withController(async ({ controller }) => { await expect( @@ -758,13 +697,13 @@ describe('KeyringController', () => { it('should throw invalid password error with valid keyringId', async () => { await withController(async ({ controller, encryptor, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; - sinon - .stub(encryptor, 'decrypt') - .throws(new Error('Invalid password')); - await expect( - controller.exportSeedPhrase('', keyringId) - ).rejects.toThrow('Invalid password'); + const keyringId = initialState.keyringsMetadata[0].id; + sinon + .stub(encryptor, 'decrypt') + .throws(new Error('Invalid password')); + await expect( + controller.exportSeedPhrase('', keyringId) + ).rejects.toThrow('Invalid password'); }); }); }); @@ -802,26 +741,31 @@ describe('KeyringController', () => { describe('when wrong password is provided', () => { it('should throw error', async () => { - await withController( - async ({ controller, initialState, encryptor }) => { - const account = initialState.keyrings[0].accounts[0]; - sinon - .stub(encryptor, 'decrypt') - .rejects(new Error('Invalid password')); - - await expect( - controller.exportAccount('', account), - ).rejects.toThrow('Invalid password'); + await withController(async ({ controller, encryptor }) => { + jest + .spyOn(encryptor, 'decrypt') + .mockRejectedValueOnce(new Error('Invalid password')); + await expect(controller.exportSeedPhrase('')).rejects.toThrow( + 'Invalid password', + ); + }); + }); + it('should throw invalid password error with valid keyringId', async () => { + await withController( + async ({ controller, encryptor, initialState }) => { + const keyringId = initialState.keyringsMetadata[0].id; + jest + .spyOn(encryptor, 'decrypt') + .mockRejectedValueOnce(new Error('Invalid password')); await expect( - controller.exportAccount('JUNK_VALUE', account), + controller.exportSeedPhrase('', keyringId), ).rejects.toThrow('Invalid password'); }, ); }); }); }); - describe('when the keyring for the given address does not support exportAccount', () => { it('should throw error', async () => { const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19'; @@ -848,22 +792,6 @@ describe('KeyringController', () => { expect(accounts).toStrictEqual(initialAccount); }); }); - - it('should get accounts for specific keyring when keyringId is provided', async () => { - await withController(async ({ controller, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; - const keyringAccounts = await controller.getAccounts(keyringId); - expect(keyringAccounts).toStrictEqual(initialState.keyrings[0].accounts); - }); - }); - - it('should throw error if keyringId is invalid', async () => { - await withController(async ({ controller }) => { - await expect(controller.getAccounts('invalid-id')).rejects.toThrow( - 'Keyring not found', - ); - }); - }); }); describe('getEncryptionPublicKey', () => { @@ -2366,6 +2294,15 @@ describe('KeyringController', () => { }); }); + it('should return seedphrase for a specific keyring', async () => { + await withController(async ({ controller }) => { + const seedPhrase = await controller.verifySeedPhrase( + 'ULID01234567890ABCDEFGHIJKLMN', + ); + expect(seedPhrase).toBeDefined(); + }); + }); + it('should throw if mnemonic is not defined', async () => { await withController(async ({ controller }) => { const primaryKeyring = controller.getKeyringsByType( @@ -2385,7 +2322,7 @@ describe('KeyringController', () => { { skipVaultCreation: true }, async ({ controller }) => { await expect(controller.verifySeedPhrase()).rejects.toThrow( - 'No HD keyring found', + 'KeyringController - No HD Keyring found', ); }, ); @@ -2579,26 +2516,26 @@ describe('KeyringController', () => { const fn = jest.fn(); const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0]; const selector = { id: initialState.keyringsMetadata[0].id }; - + await controller.withKeyring(selector, fn); - + expect(fn).toHaveBeenCalledWith(keyring); }); }); - + it('should return the result of the function', async () => { await withController(async ({ controller, initialState }) => { const fn = async () => Promise.resolve('hello'); const selector = { id: initialState.keyringsMetadata[0].id }; - + expect(await controller.withKeyring(selector, fn)).toBe('hello'); }); }); - + it('should throw an error if the callback returns the selected keyring', async () => { - await withController(async ({ controller, initialState }) => { + await withController(async ({ controller, initialState }) => { const selector = { id: initialState.keyringsMetadata[0].id }; - + await expect( controller.withKeyring(selector, async (selectedKeyring) => { return selectedKeyring; @@ -2606,25 +2543,25 @@ describe('KeyringController', () => { ).rejects.toThrow(KeyringControllerError.UnsafeDirectKeyringAccess); }); }); - + describe('when the keyring is not found', () => { it('should throw an error if the keyring is not found and `createIfMissing` is false', async () => { await withController(async ({ controller, initialState }) => { const selector = { id: 'non-existent-id' }; const fn = jest.fn(); - + await expect(controller.withKeyring(selector, fn)).rejects.toThrow( KeyringControllerError.KeyringNotFound, ); expect(fn).not.toHaveBeenCalled(); }); }); - + it('should throw an error even if `createIfMissing` is true', async () => { await withController(async ({ controller, initialState }) => { const selector = { id: 'non-existent-id' }; const fn = jest.fn(); - + await expect( controller.withKeyring(selector, fn, { createIfMissing: true }), ).rejects.toThrow(KeyringControllerError.KeyringNotFound); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 3a071f8463b..d7ba01e8d4a 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -242,7 +242,7 @@ export type KeyringControllerMessenger = RestrictedMessenger< export type KeyringControllerOptions = { keyringBuilders?: { (): EthKeyring; type: string }[]; messenger: KeyringControllerMessenger; - state?: { vault?: string }; + state?: { vault?: string; keyringsMetadata?: KeyringMetadata[] }; } & ( | { cacheEncryptionKey: true; @@ -616,6 +616,8 @@ export class KeyringController extends BaseController< #keyrings: EthKeyring[]; + #keyringsMetadata: KeyringMetadata[]; + #unsupportedKeyrings: SerializedKeyring[]; #password?: string; @@ -665,6 +667,7 @@ export class KeyringController extends BaseController< this.#encryptor = encryptor; this.#keyrings = []; + this.#keyringsMetadata = state?.keyringsMetadata ?? []; this.#unsupportedKeyrings = []; // This option allows the controller to cache an exported key @@ -689,6 +692,9 @@ export class KeyringController extends BaseController< const selectedKeyring = this.getKeyringsByType( 'HD Key Tree', )[0] as EthKeyring; + if (!selectedKeyring) { + throw new Error('No HD keyring found'); + } const oldAccounts = await selectedKeyring.getAccounts(); if (accountCount && oldAccounts.length !== accountCount) { @@ -1390,13 +1396,19 @@ export class KeyringController extends BaseController< * @returns Promise resolving to the seed phrase as Uint8Array. */ async verifySeedPhrase(keyringId?: string): Promise { - const keyringIndex = this.state.keyringsMetadata.findIndex( - (keyring) => keyring.id === keyringId, - ); - const keyring = this.state.keyrings[keyringIndex]; + let keyring: EthKeyring; + if (!keyringId) { + keyring = this.#keyrings[0]; + } else { + keyring = this.#getKeyringById(keyringId) as EthKeyring; + + if (keyring.type !== KeyringTypes.hd) { + throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); + } + } - if (keyring.type !== KeyringTypes.hd) { - throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); + if (!keyring) { + throw new Error(KeyringControllerError.NoHdKeyring); } return this.#withControllerLock(async () => @@ -1483,11 +1495,9 @@ export class KeyringController extends BaseController< | undefined; if (!keyring && options.createIfMissing) { - const newMetadata = { id: ulid(), name: '' }; keyring = (await this.#newKeyring( selector.type, options.createWithData, - newMetadata, )) as SelectedKeyring; } } else if ('id' in selector) { @@ -1926,10 +1936,6 @@ export class KeyringController extends BaseController< throw new Error('No HD keyring found.'); } - if ((keyring.type as KeyringTypes) !== KeyringTypes.hd) { - throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); - } - assertHasUint8ArrayMnemonic(keyring); const seedWords = keyring.mnemonic; @@ -2020,11 +2026,8 @@ export class KeyringController extends BaseController< await this.#restoreKeyring(serializedKeyring); } - if (this.state.keyringsMetadata.length > this.#keyrings.length) { - this.update((state) => { - // remove metadata from the end of the array to have the same length as the keyrings array - state.keyringsMetadata = state.keyringsMetadata.slice(0, -1 * (state.keyringsMetadata.length - this.#keyrings.length)); - }) + if (this.#keyringsMetadata.length > this.#keyrings.length) { + throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); } } @@ -2190,12 +2193,13 @@ export class KeyringController extends BaseController< this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; + state.keyringsMetadata = this.#keyringsMetadata; if (updatedState.encryptionKey) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; } - if (updatedKeyrings.length < state.keyringsMetadata.length) { - state.keyringsMetadata = state.keyringsMetadata.slice(0, -1 * (state.keyringsMetadata.length - updatedKeyrings.length)); + if (updatedKeyrings.length < this.#keyringsMetadata.length) { + throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); } }); @@ -2256,7 +2260,11 @@ export class KeyringController extends BaseController< */ async #newKeyring(type: string, data?: unknown): Promise> { this.#assertControllerMutexIsLocked(); - const newKeyringMetadata = { id: ulid(), name: '' }; + + const newKeyringMetadata: KeyringMetadata = { + id: ulid().toString(), + name: '', + }; const keyringBuilder = this.#getKeyringBuilderForType(type); @@ -2299,14 +2307,9 @@ export class KeyringController extends BaseController< this.#keyrings.push(keyring); if ( newKeyringMetadata && - this.state.keyringsMetadata.length < this.#keyrings.length + this.#keyringsMetadata.length < this.#keyrings.length ) { - this.update((state) => { - state.keyringsMetadata = [ - ...state.keyringsMetadata, - newKeyringMetadata, - ]; - }); + this.#keyringsMetadata = [...this.#keyringsMetadata, newKeyringMetadata]; } return keyring; } @@ -2321,9 +2324,10 @@ export class KeyringController extends BaseController< await this.#destroyKeyring(keyring); } this.#keyrings = []; - this.update((state) => { - state.keyringsMetadata = []; - }); + this.#keyringsMetadata = []; + // this.update((state) => { + // state.keyringsMetadata = []; + // }); } /** @@ -2387,10 +2391,7 @@ export class KeyringController extends BaseController< }), ); this.#keyrings = validKeyrings; - - this.update((state) => { - state.keyringsMetadata = validKeyringMetadata; - }); + this.#keyringsMetadata = validKeyringMetadata; } /** diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index 784eddb14c9..c1030ae263f 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -32,4 +32,5 @@ export enum KeyringControllerError { DataType = 'KeyringController - Incorrect data type provided', NoHdKeyring = 'KeyringController - No HD Keyring found', ControllerLockRequired = 'KeyringController - attempt to update vault during a non mutually exclusive operation', + KeyringMetadataLengthMismatch = 'KeyringController - keyring metadata length mismatch', } From 8ba574c9918875c7419492091ae6571bc36c485e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:22:43 +0100 Subject: [PATCH 11/33] fix: build --- packages/keyring-controller/src/KeyringController.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index d7ba01e8d4a..8a63a8ddc0d 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -618,8 +618,6 @@ export class KeyringController extends BaseController< #keyringsMetadata: KeyringMetadata[]; - #unsupportedKeyrings: SerializedKeyring[]; - #password?: string; #qrKeyringStateListener?: ( From 955925849f15b636ae653a0ca437f85e69b801d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 5 Feb 2025 16:34:09 +0100 Subject: [PATCH 12/33] test(multi-srp): update unit tests coverage --- packages/keyring-controller/jest.config.js | 6 +- .../src/KeyringController.test.ts | 55 +++++++++++++++---- .../src/KeyringController.ts | 19 +++---- 3 files changed, 55 insertions(+), 25 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 3dbee998978..d0a3184e2e3 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 95.51, + branches: 95.4, functions: 100, - lines: 99.07, - statements: 99.08, + lines: 98.97, + statements: 98.98, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 9c2affb96c9..2ce6932d4f2 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -55,13 +55,6 @@ jest.mock('uuid', () => { }; }); -jest.mock('ulid', () => { - return { - ulid: () => 'ULID01234567890ABCDEFGHIJKLMN', - monotonicFactory: () => () => 'ULID01234567890ABCDEFGHIJKLMN', - }; -}); - const input = '{"version":3,"id":"534e0199-53f6-41a9-a8fe-d504702ee5e8","address":"b97c80fab7a3793bbe746864db80d236f1345ea7",' + '"crypto":{"ciphertext":"974fec42023c2d6340d9710863aa82a2961aa03b9d7e5dd19aa77ab4aab1f344",' + @@ -84,7 +77,8 @@ const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; describe('KeyringController', () => { afterEach(() => { sinon.restore(); - jest.clearAllMocks(); + // jest.clearAllMocks(); + jest.resetAllMocks(); }); describe('constructor', () => { @@ -255,6 +249,28 @@ describe('KeyringController', () => { }); }); + describe('behavior when keyring metadata length mismatch', () => { + it('should throw an error if the keyring metadata length mismatch', async () => { + // withController is already calling the #updateVault, which is throwing an error + await expect( + withController( + { + state: { + vault: undefined, + keyringsMetadata: [ + { id: '1', name: '' }, + { id: '2', name: '' }, + ], + }, + }, + async ({ controller }) => { + await controller.addNewAccount(); + }, + ), + ).rejects.toThrow('KeyringController - keyring metadata length mismatch'); + }); + }); + describe('addNewAccountForKeyring', () => { describe('when accountCount is not provided', () => { it('should add new account', async () => { @@ -1028,7 +1044,10 @@ describe('KeyringController', () => { const modifiedState = { ...initialState, keyrings: [initialState.keyrings[0], newKeyring], - keyringsMetadata: [initialState.keyringsMetadata[0], initialState.keyringsMetadata[0]], + keyringsMetadata: [ + initialState.keyringsMetadata[0], + controller.state.keyringsMetadata[1], + ], }; expect(controller.state).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); @@ -1102,7 +1121,10 @@ describe('KeyringController', () => { const modifiedState = { ...initialState, keyrings: [initialState.keyrings[0], newKeyring], - keyringsMetadata: [initialState.keyringsMetadata[0], initialState.keyringsMetadata[0]], + keyringsMetadata: [ + initialState.keyringsMetadata[0], + controller.state.keyringsMetadata[1], + ], }; expect(controller.state).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); @@ -2297,7 +2319,7 @@ describe('KeyringController', () => { it('should return seedphrase for a specific keyring', async () => { await withController(async ({ controller }) => { const seedPhrase = await controller.verifySeedPhrase( - 'ULID01234567890ABCDEFGHIJKLMN', + controller.state.keyringsMetadata[0].id, ); expect(seedPhrase).toBeDefined(); }); @@ -2327,6 +2349,17 @@ describe('KeyringController', () => { }, ); }); + + it('should throw unspported seed phrase error when keyring is not HD', async () => { + await withController(async ({ controller }) => { + await controller.addNewKeyring(KeyringTypes.simple, [privateKey]); + + const keyringId = controller.state.keyringsMetadata[1].id; + await expect(controller.verifySeedPhrase(keyringId)).rejects.toThrow( + 'KeyringController - The keyring does not support the method verifySeedPhrase.', + ); + }); + }); }); describe('verifyPassword', () => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8a63a8ddc0d..c888bef4a53 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -44,11 +44,10 @@ import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; // When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. -import { monotonicFactory } from 'ulid'; +import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; -const ulid = monotonicFactory(); const name = 'KeyringController'; /** @@ -1398,6 +1397,9 @@ export class KeyringController extends BaseController< if (!keyringId) { keyring = this.#keyrings[0]; } else { + console.log('keyrings: ', this.#keyrings); + console.log('keyringId: ', keyringId); + console.log('metadata: ', this.#keyringsMetadata); keyring = this.#getKeyringById(keyringId) as EthKeyring; if (keyring.type !== KeyringTypes.hd) { @@ -1930,6 +1932,7 @@ export class KeyringController extends BaseController< | EthKeyring | undefined); + // This will never going to be undefined because we are checking for it in all of the callers if (!keyring) { throw new Error('No HD keyring found.'); } @@ -2023,10 +2026,6 @@ export class KeyringController extends BaseController< for (const serializedKeyring of serializedKeyrings) { await this.#restoreKeyring(serializedKeyring); } - - if (this.#keyringsMetadata.length > this.#keyrings.length) { - throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); - } } /** @@ -2260,7 +2259,7 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); const newKeyringMetadata: KeyringMetadata = { - id: ulid().toString(), + id: ulid(), name: '', }; @@ -2322,10 +2321,6 @@ export class KeyringController extends BaseController< await this.#destroyKeyring(keyring); } this.#keyrings = []; - this.#keyringsMetadata = []; - // this.update((state) => { - // state.keyringsMetadata = []; - // }); } /** @@ -2479,6 +2474,7 @@ export class KeyringController extends BaseController< return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); const currentPassword = this.#password; + const currentKeyringsMetadata = this.#keyringsMetadata; try { return await callback({ releaseLock }); @@ -2486,6 +2482,7 @@ export class KeyringController extends BaseController< // Keyrings and password are restored to their previous state await this.#restoreSerializedKeyrings(currentSerializedKeyrings); this.#password = currentPassword; + this.#keyringsMetadata = currentKeyringsMetadata; throw e; } From 40fac06de554cf243a1d9bb02f348bee4a335dd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Thu, 6 Feb 2025 11:00:00 +0100 Subject: [PATCH 13/33] fix(multi-srp): clear keyringsMetadata on createNewVaultAndRestore --- .../src/KeyringController.test.ts | 58 ++++++++++++------- .../src/KeyringController.ts | 4 +- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 2ce6932d4f2..59ba1d037d8 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -77,7 +77,6 @@ const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; describe('KeyringController', () => { afterEach(() => { sinon.restore(); - // jest.clearAllMocks(); jest.resetAllMocks(); }); @@ -251,23 +250,29 @@ describe('KeyringController', () => { describe('behavior when keyring metadata length mismatch', () => { it('should throw an error if the keyring metadata length mismatch', async () => { - // withController is already calling the #updateVault, which is throwing an error - await expect( - withController( - { - state: { - vault: undefined, - keyringsMetadata: [ - { id: '1', name: '' }, - { id: '2', name: '' }, - ], - }, - }, - async ({ controller }) => { - await controller.addNewAccount(); + let vaultWithOneKeyring; + await withController(async ({ controller }) => { + vaultWithOneKeyring = controller.state.vault; + }); + + await withController( + { + skipVaultCreation: true, + state: { + vault: vaultWithOneKeyring, + keyringsMetadata: [ + { id: '1', name: '' }, + { id: '2', name: '' }, + ], }, - ), - ).rejects.toThrow('KeyringController - keyring metadata length mismatch'); + }, + async ({ controller }) => { + await controller.submitPassword(password); + await expect(controller.addNewAccount()).rejects.toThrow( + 'KeyringController - keyring metadata length mismatch', + ); + }, + ); }); }); @@ -410,6 +415,7 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { const initialVault = controller.state.vault; + const initialKeyringsMetadata = controller.state.keyringsMetadata; await controller.createNewVaultAndRestore( password, uint8ArraySeed, @@ -417,6 +423,13 @@ describe('KeyringController', () => { expect(controller.state).not.toBe(initialState); expect(controller.state.vault).toBeDefined(); expect(controller.state.vault).toStrictEqual(initialVault); + expect(controller.state.keyringsMetadata).toHaveLength( + initialKeyringsMetadata.length, + ); + // new keyring metadata should be generated + expect(controller.state.keyringsMetadata).not.toStrictEqual( + initialKeyringsMetadata, + ); }, ); }); @@ -714,13 +727,14 @@ describe('KeyringController', () => { it('should throw invalid password error with valid keyringId', async () => { await withController(async ({ controller, encryptor, initialState }) => { const keyringId = initialState.keyringsMetadata[0].id; - sinon - .stub(encryptor, 'decrypt') - .throws(new Error('Invalid password')); + jest + .spyOn(encryptor, 'decrypt') + .mockRejectedValueOnce(new Error('Invalid password')); await expect( - controller.exportSeedPhrase('', keyringId) + controller.exportSeedPhrase('', keyringId), ).rejects.toThrow('Invalid password'); - }); + }, + ); }); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index c888bef4a53..044bbe3494a 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1397,9 +1397,6 @@ export class KeyringController extends BaseController< if (!keyringId) { keyring = this.#keyrings[0]; } else { - console.log('keyrings: ', this.#keyrings); - console.log('keyringId: ', keyringId); - console.log('metadata: ', this.#keyringsMetadata); keyring = this.#getKeyringById(keyringId) as EthKeyring; if (keyring.type !== KeyringTypes.hd) { @@ -1913,6 +1910,7 @@ export class KeyringController extends BaseController< this.#password = password; await this.#clearKeyrings(); + this.#keyringsMetadata = []; await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); this.#setUnlocked(); } From bcaa0a3b3d1b9206182bd6fe634e1a6dba004a4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Fri, 7 Feb 2025 12:35:46 +0100 Subject: [PATCH 14/33] feat(multi-srp): add keyring metadta to AccountsController keyring object --- .../src/AccountsController.ts | 6 +- packages/keyring-controller/jest.config.js | 6 +- .../src/KeyringController.test.ts | 14 ++--- .../src/KeyringController.ts | 63 ++++++++----------- 4 files changed, 40 insertions(+), 49 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index f81f77565b1..91d7d8c48e5 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -24,6 +24,7 @@ import type { KeyringControllerGetKeyringsByTypeAction, KeyringControllerGetAccountsAction, KeyringControllerStateChangeEvent, + KeyringMetadata, } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; import type { @@ -211,6 +212,7 @@ export type AccountsControllerMessenger = RestrictedMessenger< type AddressAndKeyringTypeObject = { address: string; type: string; + metadata: KeyringMetadata; }; const accountsControllerMetadata = { @@ -773,13 +775,14 @@ export class AccountsController extends BaseController< const updatedNormalKeyringAddresses: AddressAndKeyringTypeObject[] = []; const updatedSnapKeyringAddresses: AddressAndKeyringTypeObject[] = []; - for (const keyring of keyringState.keyrings) { + for (const [index, keyring] of keyringState.keyrings.entries()) { if (keyring.type === KeyringTypes.snap) { updatedSnapKeyringAddresses.push( ...keyring.accounts.map((address) => { return { address, type: keyring.type, + metadata: keyringState.keyringsMetadata[index], }; }), ); @@ -789,6 +792,7 @@ export class AccountsController extends BaseController< return { address, type: keyring.type, + metadata: keyringState.keyringsMetadata[index], }; }), ); diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index d0a3184e2e3..dbabffdf940 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 95.4, + branches: 95.23, functions: 100, - lines: 98.97, - statements: 98.98, + lines: 98.96, + statements: 98.97, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 59ba1d037d8..68f570775cd 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3617,20 +3617,16 @@ describe('KeyringController', () => { it('should rollback the controller keyrings if the keyring creation fails', async () => { const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4'; stubKeyringClassWithAccount(MockKeyring, mockAddress); - // Mocking the serialize method to throw an error will - // halt the controller everytime it tries to persist the keyring, - // making it impossible to update the vault - jest - .spyOn(MockKeyring.prototype, 'serialize') - .mockImplementation(async () => { - throw new Error('You will never be able to persist me!'); - }); + await withController( { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, async ({ controller, initialState }) => { + jest.spyOn(controller, 'update' as never).mockImplementation(() => { + throw new Error('You will never be able to change me!'); + }); await expect( controller.addNewKeyring(MockKeyring.type), - ).rejects.toThrow('You will never be able to persist me!'); + ).rejects.toThrow('You will never be able to change me!'); expect(controller.state).toStrictEqual(initialState); await expect( diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 044bbe3494a..01291d6181e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -254,11 +254,7 @@ export type KeyringControllerOptions = { ); /** - * @type KeyringObject - * * Keyring object to return in fullUpdate - * @property type - Keyring type - * @property accounts - Associated accounts */ export type KeyringObject = { /** @@ -272,14 +268,16 @@ export type KeyringObject = { }; /** - * @type KeyringMetadata - * * Keyring metadata - * @property id - Keyring ID - * @property name - Keyring name */ export type KeyringMetadata = { + /** + * Keyring ID + */ id: string; + /** + * Keyring name + */ name: string; }; @@ -854,11 +852,7 @@ export class KeyringController extends BaseController< keyringId?: string, ): Promise { await this.verifyPassword(password); - if (!keyringId) { - assertHasUint8ArrayMnemonic(this.#keyrings[0]); - return this.#keyrings[0].mnemonic; - } - const selectedKeyring = this.#getKeyringById(keyringId); + const selectedKeyring = this.#getKeyringByIdOrDefault(keyringId); if (!selectedKeyring) { throw new Error('Keyring not found'); } @@ -1393,21 +1387,16 @@ export class KeyringController extends BaseController< * @returns Promise resolving to the seed phrase as Uint8Array. */ async verifySeedPhrase(keyringId?: string): Promise { - let keyring: EthKeyring; - if (!keyringId) { - keyring = this.#keyrings[0]; - } else { - keyring = this.#getKeyringById(keyringId) as EthKeyring; - - if (keyring.type !== KeyringTypes.hd) { - throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); - } - } + const keyring = this.#getKeyringByIdOrDefault(keyringId); if (!keyring) { throw new Error(KeyringControllerError.NoHdKeyring); } + if (keyring.type !== KeyringTypes.hd) { + throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); + } + return this.#withControllerLock(async () => this.#verifySeedPhrase(keyringId), ); @@ -1498,7 +1487,10 @@ export class KeyringController extends BaseController< )) as SelectedKeyring; } } else if ('id' in selector) { - keyring = this.#getKeyringById(selector.id) as SelectedKeyring; + const index = this.state.keyringsMetadata.findIndex( + (metadata) => metadata.id === selector.id, + ); + keyring = this.#keyrings[index] as SelectedKeyring; } if (!keyring) { @@ -1806,12 +1798,16 @@ export class KeyringController extends BaseController< } /** - * Get the keyring by id. + * Get the keyring by id or return the first keyring if the id is not found. * * @param keyringId - The id of the keyring. * @returns The keyring. */ - #getKeyringById(keyringId: string): EthKeyring | undefined { + #getKeyringByIdOrDefault(keyringId?: string): EthKeyring | undefined { + if (!keyringId) { + return this.#keyrings[0] as EthKeyring; + } + const index = this.state.keyringsMetadata.findIndex( (metadata) => metadata.id === keyringId, ); @@ -1924,11 +1920,7 @@ export class KeyringController extends BaseController< async #verifySeedPhrase(keyringId?: string): Promise { this.#assertControllerMutexIsLocked(); - const keyring = keyringId - ? this.#getKeyringById(keyringId) - : (this.getKeyringsByType(KeyringTypes.hd)[0] as - | EthKeyring - | undefined); + const keyring = this.#getKeyringByIdOrDefault(keyringId); // This will never going to be undefined because we are checking for it in all of the callers if (!keyring) { @@ -2300,12 +2292,11 @@ export class KeyringController extends BaseController< } this.#keyrings.push(keyring); - if ( - newKeyringMetadata && - this.#keyringsMetadata.length < this.#keyrings.length - ) { + if (this.#keyringsMetadata.length < this.#keyrings.length) { + // For some reason, metadata is not always an extensible array, so .push() is not working this.#keyringsMetadata = [...this.#keyringsMetadata, newKeyringMetadata]; } + return keyring; } @@ -2472,7 +2463,7 @@ export class KeyringController extends BaseController< return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); const currentPassword = this.#password; - const currentKeyringsMetadata = this.#keyringsMetadata; + const currentKeyringsMetadata = this.#keyringsMetadata.slice(); try { return await callback({ releaseLock }); From 0256b1e9856e2dcd39b6c27e010cc8a3de0959d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= Date: Fri, 7 Feb 2025 13:53:50 +0100 Subject: [PATCH 15/33] chore: add FIXME packages/keyring-controller/src/KeyringController.ts comment Co-authored-by: Charly Chevalier --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 01291d6181e..e5e6960dc93 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2293,7 +2293,7 @@ export class KeyringController extends BaseController< this.#keyrings.push(keyring); if (this.#keyringsMetadata.length < this.#keyrings.length) { - // For some reason, metadata is not always an extensible array, so .push() is not working + // FIXME: For some reason, metadata is not always an extensible array, so .push() is not working this.#keyringsMetadata = [...this.#keyringsMetadata, newKeyringMetadata]; } From 3265053ebd7c9e381cabc34ef033a0f4aa9a0d4d Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Fri, 7 Feb 2025 14:24:24 +0000 Subject: [PATCH 16/33] chore: fix lint issues --- eslint-warning-thresholds.json | 2 +- packages/keyring-controller/src/KeyringController.test.ts | 7 ++++--- packages/keyring-controller/src/KeyringController.ts | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 5658cfd8e1a..c0eb50398cf 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -250,7 +250,7 @@ "jest/no-conditional-in-test": 8 }, "packages/keyring-controller/src/KeyringController.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 5, + "@typescript-eslint/no-unsafe-enum-comparison": 4, "@typescript-eslint/no-unused-vars": 2, "jsdoc/tag-lines": 1 }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 68f570775cd..e4f0d0e7903 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -706,7 +706,7 @@ describe('KeyringController', () => { it('should throw error if keyringId is invalid', async () => { await withController(async ({ controller }) => { await expect( - controller.exportSeedPhrase(password, 'invalid-id') + controller.exportSeedPhrase(password, 'invalid-id'), ).rejects.toThrow('Keyring not found'); }); }); @@ -725,7 +725,8 @@ describe('KeyringController', () => { }); it('should throw invalid password error with valid keyringId', async () => { - await withController(async ({ controller, encryptor, initialState }) => { + await withController( + async ({ controller, encryptor, initialState }) => { const keyringId = initialState.keyringsMetadata[0].id; jest .spyOn(encryptor, 'decrypt') @@ -3601,7 +3602,7 @@ describe('KeyringController', () => { } }); // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-misused-promises + messenger.subscribe('KeyringController:stateChange', listener); await controller.submitPassword(password); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index e5e6960dc93..0179d0e4444 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1393,6 +1393,7 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.NoHdKeyring); } + // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison if (keyring.type !== KeyringTypes.hd) { throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); } @@ -1987,7 +1988,7 @@ export class KeyringController extends BaseController< }, ): Promise { const serializedKeyrings = await Promise.all( - this.#keyrings.map(async (keyring, index) => { + this.#keyrings.map(async (keyring) => { const [type, data] = await Promise.all([ keyring.type, keyring.serialize(), From 1d4430baa1b989fa1ec75269d97a073d3c6ef109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Fri, 7 Feb 2025 15:40:27 +0100 Subject: [PATCH 17/33] fix: lint warnings --- eslint-warning-thresholds.json | 2 +- packages/keyring-controller/src/KeyringController.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index c0eb50398cf..5658cfd8e1a 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -250,7 +250,7 @@ "jest/no-conditional-in-test": 8 }, "packages/keyring-controller/src/KeyringController.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 4, + "@typescript-eslint/no-unsafe-enum-comparison": 5, "@typescript-eslint/no-unused-vars": 2, "jsdoc/tag-lines": 1 }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index e4f0d0e7903..33a08f74d1b 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2594,7 +2594,7 @@ describe('KeyringController', () => { describe('when the keyring is not found', () => { it('should throw an error if the keyring is not found and `createIfMissing` is false', async () => { - await withController(async ({ controller, initialState }) => { + await withController(async ({ controller }) => { const selector = { id: 'non-existent-id' }; const fn = jest.fn(); @@ -2606,7 +2606,7 @@ describe('KeyringController', () => { }); it('should throw an error even if `createIfMissing` is true', async () => { - await withController(async ({ controller, initialState }) => { + await withController(async ({ controller }) => { const selector = { id: 'non-existent-id' }; const fn = jest.fn(); From 4645938c4a7e8fb8a39eb7f1c60e82175f30f723 Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Fri, 7 Feb 2025 14:42:23 +0000 Subject: [PATCH 18/33] chore: prettier fix --- .../src/KeyringController.test.ts | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 33a08f74d1b..ea8a56e1323 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2594,27 +2594,31 @@ describe('KeyringController', () => { describe('when the keyring is not found', () => { it('should throw an error if the keyring is not found and `createIfMissing` is false', async () => { - await withController(async ({ controller }) => { - const selector = { id: 'non-existent-id' }; - const fn = jest.fn(); + await withController( + async ({ controller, initialState: _initialState }) => { + const selector = { id: 'non-existent-id' }; + const fn = jest.fn(); - await expect(controller.withKeyring(selector, fn)).rejects.toThrow( - KeyringControllerError.KeyringNotFound, - ); - expect(fn).not.toHaveBeenCalled(); - }); + await expect( + controller.withKeyring(selector, fn), + ).rejects.toThrow(KeyringControllerError.KeyringNotFound); + expect(fn).not.toHaveBeenCalled(); + }, + ); }); it('should throw an error even if `createIfMissing` is true', async () => { - await withController(async ({ controller }) => { - const selector = { id: 'non-existent-id' }; - const fn = jest.fn(); + await withController( + async ({ controller, initialState: _initialState }) => { + const selector = { id: 'non-existent-id' }; + const fn = jest.fn(); - await expect( - controller.withKeyring(selector, fn, { createIfMissing: true }), - ).rejects.toThrow(KeyringControllerError.KeyringNotFound); - expect(fn).not.toHaveBeenCalled(); - }); + await expect( + controller.withKeyring(selector, fn, { createIfMissing: true }), + ).rejects.toThrow(KeyringControllerError.KeyringNotFound); + expect(fn).not.toHaveBeenCalled(); + }, + ); }); }); }); From 09c9430e7667e7fe0f5b5870fe0cdf01a19450fd Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Fri, 7 Feb 2025 15:02:27 +0000 Subject: [PATCH 19/33] chore: warning thresholds --- eslint-warning-thresholds.json | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 5658cfd8e1a..002481e0ebc 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -246,13 +246,12 @@ "n/no-unsupported-features/node-builtins": 1 }, "packages/keyring-controller/src/KeyringController.test.ts": { - "import-x/namespace": 16, + "import-x/namespace": 15, "jest/no-conditional-in-test": 8 }, "packages/keyring-controller/src/KeyringController.ts": { "@typescript-eslint/no-unsafe-enum-comparison": 5, - "@typescript-eslint/no-unused-vars": 2, - "jsdoc/tag-lines": 1 + "@typescript-eslint/no-unused-vars": 2 }, "packages/keyring-controller/tests/mocks/mockKeyring.ts": { "@typescript-eslint/prefer-readonly": 1 From d5981fae00855985fa1feb8d7b037e297693936d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= Date: Fri, 7 Feb 2025 16:28:07 +0100 Subject: [PATCH 20/33] fix(multi-srp): use internal variable instead of property for metadata update Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 0179d0e4444..658731b012b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2367,7 +2367,7 @@ export class KeyringController extends BaseController< const accounts = await keyring.getAccounts(); if (accounts.length > 0) { validKeyrings.push(keyring); - validKeyringMetadata.push(this.state.keyringsMetadata[index]); + validKeyringMetadata.push(this.#keyringsMetadata[index]); } else { await this.#destroyKeyring(keyring); } From 03c9731904fe6babba0c5a8c494eb93ce9c1fc39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Tue, 11 Feb 2025 11:14:49 +0100 Subject: [PATCH 21/33] chore(multi-srp): throw mismatch array outside of the update method --- packages/keyring-controller/src/KeyringController.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 658731b012b..b873dde5113 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2178,6 +2178,10 @@ export class KeyringController extends BaseController< } const updatedKeyrings = await this.#getUpdatedKeyrings(); + if (updatedKeyrings.length !== this.#keyringsMetadata.length) { + throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); + } + this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; @@ -2186,9 +2190,6 @@ export class KeyringController extends BaseController< state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; } - if (updatedKeyrings.length < this.#keyringsMetadata.length) { - throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); - } }); return true; From ddd87377b71cd4cd09fd6660d44f85155c1a1cab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Tue, 11 Feb 2025 16:30:28 +0100 Subject: [PATCH 22/33] fix(multi-srp): use push instead of spread operator for keyringsMetadata --- packages/keyring-controller/src/KeyringController.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index b873dde5113..5d97c0623b6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2185,7 +2185,7 @@ export class KeyringController extends BaseController< this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; - state.keyringsMetadata = this.#keyringsMetadata; + state.keyringsMetadata = this.#keyringsMetadata.slice(); if (updatedState.encryptionKey) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; @@ -2295,8 +2295,7 @@ export class KeyringController extends BaseController< this.#keyrings.push(keyring); if (this.#keyringsMetadata.length < this.#keyrings.length) { - // FIXME: For some reason, metadata is not always an extensible array, so .push() is not working - this.#keyringsMetadata = [...this.#keyringsMetadata, newKeyringMetadata]; + this.#keyringsMetadata.push(newKeyringMetadata); } return keyring; From 89746ba54a2c72b1572b55fd72e67c5f94223efa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= Date: Wed, 12 Feb 2025 11:32:35 +0100 Subject: [PATCH 23/33] Update packages/keyring-controller/src/KeyringController.ts Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 330cec0a3a5..faf3d94a6a5 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -254,7 +254,7 @@ export type KeyringControllerOptions = { ); /** - * Keyring object to return in fullUpdate + * A keyring object representation. */ export type KeyringObject = { /** From 03509bcfe70882ca2b017e0894d99eb50c3d880c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= Date: Wed, 12 Feb 2025 11:33:03 +0100 Subject: [PATCH 24/33] Update packages/keyring-controller/src/KeyringController.ts Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index faf3d94a6a5..298919f2d69 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -268,7 +268,7 @@ export type KeyringObject = { }; /** - * Keyring metadata + * Additional information related to a keyring. */ export type KeyringMetadata = { /** From ca5548b5fb33689976912241ccf65396ceab2705 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= Date: Wed, 12 Feb 2025 12:05:27 +0100 Subject: [PATCH 25/33] Update packages/keyring-controller/src/KeyringController.ts Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/src/KeyringController.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 298919f2d69..210dd2ecf77 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1894,10 +1894,7 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); // QRKeyring is not yet compatible with Keyring type from @metamask/utils - return (await this.#newKeyring( - KeyringTypes.qr, - undefined, - )) as unknown as QRKeyring; +return (await this.#newKeyring(KeyringTypes.qr)) as unknown as QRKeyring; } /** From aebc8a735776efea4b845a88f2cd37686d70c25f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 12 Feb 2025 13:52:21 +0100 Subject: [PATCH 26/33] test(multi-srp): add remove last account test --- packages/keyring-controller/jest.config.js | 6 +-- .../src/KeyringController.test.ts | 43 +++++++++++++++---- .../src/KeyringController.ts | 32 ++++++-------- 3 files changed, 50 insertions(+), 31 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index f8802626e11..c7cf31a665e 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.04, + branches: 94.61, functions: 100, - lines: 98.87, - statements: 98.88, + lines: 99.02, + statements: 99.03, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 12c6b844927..77034ad843b 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -261,12 +261,11 @@ describe('KeyringController', () => { }); }); - describe('behavior when keyring metadata length mismatch', () => { + describe('when the keyringMetadata length is different from the number of keyrings', () => { it('should throw an error if the keyring metadata length mismatch', async () => { - let vaultWithOneKeyring; - await withController(async ({ controller }) => { - vaultWithOneKeyring = controller.state.vault; - }); + const vaultWithOneKeyring = await withController( + async ({ controller }) => controller.state.vault, + ); await withController( { @@ -468,10 +467,15 @@ describe('KeyringController', () => { ); }); - it('should restore same vault if old seedWord is used', async () => { + it('should call encryptor.encrypt with the same keyrings if old seedWord is used', async () => { await withController( { cacheEncryptionKey }, - async ({ controller, initialState }) => { + async ({ controller, encryptor }) => { + const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + const serializedKeyring = await controller.withKeyring( + { type: 'HD Key Tree' }, + async (keyring) => keyring.serialize(), + ); const currentSeedWord = await controller.exportSeedPhrase(password); @@ -479,7 +483,13 @@ describe('KeyringController', () => { password, currentSeedWord, ); - expect(initialState.vault).toStrictEqual(controller.state.vault); + + expect(encryptSpy).toHaveBeenCalledWith(password, [ + { + data: serializedKeyring, + type: 'HD Key Tree', + }, + ]); }, ); }); @@ -1450,6 +1460,19 @@ describe('KeyringController', () => { ); }); }); + + it('should remove the keyring if last account is removed and its not primary keyring', async () => { + await withController(async ({ controller }) => { + await controller.addNewKeyring(KeyringTypes.hd); + expect(controller.state.keyrings).toHaveLength(2); + expect(controller.state.keyringsMetadata).toHaveLength(2); + await controller.removeAccount( + controller.state.keyrings[1].accounts[0], + ); + expect(controller.state.keyrings).toHaveLength(1); + expect(controller.state.keyringsMetadata).toHaveLength(1); + }); + }); }); describe('when the keyring for the given address does not support removeAccount', () => { @@ -2682,7 +2705,7 @@ describe('KeyringController', () => { await controller.submitPassword('123'); await expect(controller.verifySeedPhrase()).rejects.toThrow( - 'KeyringController - No HD Keyring found', + KeyringControllerError.KeyringNotFound, ); }); }); @@ -4050,6 +4073,8 @@ describe('KeyringController', () => { await withController( { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, async ({ controller, initialState }) => { + // We're mocking BaseController .update() to throw an error, as it's the last operation + // that is called before the function is rolled back. jest.spyOn(controller, 'update' as never).mockImplementation(() => { throw new Error('You will never be able to change me!'); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 210dd2ecf77..f28b20e0204 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -686,13 +686,13 @@ export class KeyringController extends BaseController< this.#assertIsUnlocked(); return this.#persistOrRollback(async () => { - const selectedKeyring = this.getKeyringsByType( - 'HD Key Tree', - )[0] as EthKeyring; - if (!selectedKeyring) { + const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as + | EthKeyring + | undefined; + if (!primaryKeyring) { throw new Error('No HD keyring found'); } - const oldAccounts = await selectedKeyring.getAccounts(); + const oldAccounts = await primaryKeyring.getAccounts(); if (accountCount && oldAccounts.length !== accountCount) { if (accountCount > oldAccounts.length) { @@ -708,7 +708,7 @@ export class KeyringController extends BaseController< return existingAccount; } - const [addedAccountAddress] = await selectedKeyring.addAccounts(1); + const [addedAccountAddress] = await primaryKeyring.addAccounts(1); await this.#verifySeedPhrase(); return addedAccountAddress; @@ -1412,16 +1412,6 @@ export class KeyringController extends BaseController< */ async verifySeedPhrase(keyringId?: string): Promise { this.#assertIsUnlocked(); - const keyring = this.#getKeyringByIdOrDefault(keyringId); - - if (!keyring) { - throw new Error(KeyringControllerError.NoHdKeyring); - } - - // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison - if (keyring.type !== KeyringTypes.hd) { - throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); - } return this.#withControllerLock(async () => this.#verifySeedPhrase(keyringId), @@ -1894,7 +1884,7 @@ export class KeyringController extends BaseController< this.#assertControllerMutexIsLocked(); // QRKeyring is not yet compatible with Keyring type from @metamask/utils -return (await this.#newKeyring(KeyringTypes.qr)) as unknown as QRKeyring; + return (await this.#newKeyring(KeyringTypes.qr)) as unknown as QRKeyring; } /** @@ -1974,9 +1964,13 @@ return (await this.#newKeyring(KeyringTypes.qr)) as unknown as QRKeyring; const keyring = this.#getKeyringByIdOrDefault(keyringId); - // This will never going to be undefined because we are checking for it in all of the callers if (!keyring) { - throw new Error('No HD keyring found.'); + throw new Error(KeyringControllerError.KeyringNotFound); + } + + // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison + if (keyring.type !== KeyringTypes.hd) { + throw new Error(KeyringControllerError.UnsupportedVerifySeedPhrase); } assertHasUint8ArrayMnemonic(keyring); From 5022d865c44a5ffd069488407214e8eac5ae9d71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Wed, 12 Feb 2025 16:25:05 +0100 Subject: [PATCH 27/33] chore(multi-srp): move primary keyring removal protection to removeAccount --- packages/keyring-controller/jest.config.js | 6 +++--- .../src/KeyringController.test.ts | 11 ++++++++++- .../keyring-controller/src/KeyringController.ts | 16 +++++++++++++--- packages/keyring-controller/src/constants.ts | 1 + 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index c7cf31a665e..f2c1b706c3b 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.61, + branches: 94.11, functions: 100, - lines: 99.02, - statements: 99.03, + lines: 98.87, + statements: 98.88, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 77034ad843b..647b3089320 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1402,13 +1402,22 @@ describe('KeyringController', () => { await withController(async ({ controller, initialState }) => { const account = initialState.keyrings[0].accounts[0] as Hex; await expect(controller.removeAccount(account)).rejects.toThrow( - KeyringControllerError.NoHdKeyring, + KeyringControllerError.LastAccountInPrimaryKeyring, ); expect(controller.state.keyrings).toHaveLength(1); expect(controller.state.keyrings[0].accounts).toHaveLength(1); }); }); + it('should not remove primary keyring if it has no accounts even if it has more than one HD keyring', async () => { + await withController(async ({ controller }) => { + await controller.addNewKeyring(KeyringTypes.hd); + await expect( + controller.removeAccount(controller.state.keyrings[0].accounts[0]), + ).rejects.toThrow(KeyringControllerError.LastAccountInPrimaryKeyring); + }); + }); + it('should remove account', async () => { await withController(async ({ controller, initialState }) => { await controller.importAccountWithStrategy( diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index f28b20e0204..576c9576087 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1094,6 +1094,18 @@ export class KeyringController extends BaseController< address, )) as EthKeyring; + const keyringIndex = this.state.keyrings.findIndex((kr) => + kr.accounts.includes(address), + ); + + const isPrimaryKeyring = keyringIndex === 0; + const shouldRemoveKeyring = (await keyring.getAccounts()).length === 1; + + // Primary keyring should never be removed, so we need to keep at least one account in it + if (isPrimaryKeyring && shouldRemoveKeyring) { + throw new Error(KeyringControllerError.LastAccountInPrimaryKeyring); + } + // Not all the keyrings support this, so we have to check if (!keyring.removeAccount) { throw new Error(KeyringControllerError.UnsupportedRemoveAccount); @@ -1106,9 +1118,7 @@ export class KeyringController extends BaseController< // type would need to be updated for a full non-EVM support. keyring.removeAccount(address as Hex); - const accounts = await keyring.getAccounts(); - // Check if this was the last/only account - if (accounts.length === 0) { + if (shouldRemoveKeyring) { await this.#removeEmptyKeyrings(); } }); diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index a94409c77df..364a9f4b346 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -34,4 +34,5 @@ export enum KeyringControllerError { NoHdKeyring = 'KeyringController - No HD Keyring found', ControllerLockRequired = 'KeyringController - attempt to update vault during a non mutually exclusive operation', KeyringMetadataLengthMismatch = 'KeyringController - keyring metadata length mismatch', + LastAccountInPrimaryKeyring = 'KeyringController - Last account in primary keyring cannot be removed', } From ae1a759aa9fa4b31ff2684b353e245b98dc4c212 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 12 Feb 2025 15:59:33 -0330 Subject: [PATCH 28/33] chore: Improve error handling related to keyring metadata This is a suggestion on how to improve error handling for keyring metadata in the PR #5112. --- .../src/KeyringController.ts | 66 +++++++++++++++---- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 576c9576087..cbf69342457 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2297,18 +2297,48 @@ export class KeyringController extends BaseController< * using the given `opts`. The keyring is built using the keyring builder * registered for the given `type`. * + * The internal keyring and keyring metadata arrays are updated with the new + * keyring as well. + * * @param type - The type of keyring to add. - * @param data - The data to restore a previously serialized keyring. + * @param data - Keyring initialization options. * @returns The new keyring. * @throws If the keyring includes duplicated accounts. */ async #newKeyring(type: string, data?: unknown): Promise> { - this.#assertControllerMutexIsLocked(); + const keyring = await this.#createKeyring(type, data); - const newKeyringMetadata: KeyringMetadata = { - id: ulid(), - name: '', - }; + this.#keyrings.push(keyring); + this.#keyringsMetadata.push(getDefaultKeyringMetadata()); + if (this.#keyrings.length !== this.#keyringsMetadata.length) { + throw new Error('Keyring metadata missing'); + } + + return keyring; + } + + /** + * Instantiate, initialize and return a keyring of the given `type` using the + * given `opts`. The keyring is built using the keyring builder registered + * for the given `type`. + * + * The keyring might be new, or it might be restored from the vault. This + * function should only be called from `#newKeyring` or `#restoreKeyring`, + * for the "new" and "restore" cases respectively. + * + * The internal keyring and keyring metadata arrays are *not* updated, the + * caller is expected to update them. + * + * @param type - The type of keyring to add. + * @param data - Keyring initialization options. + * @returns The new keyring. + * @throws If the keyring includes duplicated accounts. + */ + async #createKeyring( + type: string, + data?: unknown, + ): Promise> { + this.#assertControllerMutexIsLocked(); const keyringBuilder = this.#getKeyringBuilderForType(type); @@ -2348,11 +2378,6 @@ export class KeyringController extends BaseController< this.#subscribeToQRKeyringEvents(keyring as unknown as QRKeyring); } - this.#keyrings.push(keyring); - if (this.#keyringsMetadata.length < this.#keyrings.length) { - this.#keyringsMetadata.push(newKeyringMetadata); - } - return keyring; } @@ -2382,7 +2407,15 @@ export class KeyringController extends BaseController< try { const { type, data } = serialized; - return await this.#newKeyring(type, data); + const keyring = await this.#createKeyring(type, data); + this.#keyrings.push(keyring); + // If metadata is missing, assume the data is from an installation before + // we had keyring metadata. + if (this.#keyringsMetadata.length < this.#keyrings.length) { + console.log(`Adding missing metadata for '${type}' keyring`); + this.#keyringsMetadata.push(getDefaultKeyringMetadata()); + } + return keyring; } catch (_) { this.#unsupportedKeyrings.push(serialized); return undefined; @@ -2616,4 +2649,13 @@ async function withLock( } } +/** + * Generate a new keyring metadata object. + * + * @returns Keyring metadata. + */ +function getDefaultKeyringMetadata(): KeyringMetadata { + return { id: ulid(), name: '' }; +} + export default KeyringController; From b93bbdfb31c1db6273b74c20249a48df76e67661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Thu, 13 Feb 2025 13:57:40 +0100 Subject: [PATCH 29/33] test(multi-srp): update test coverage config --- packages/keyring-controller/jest.config.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index f2c1b706c3b..c174babbfd1 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.11, + branches: 93.56, functions: 100, - lines: 98.87, - statements: 98.88, + lines: 98.73, + statements: 98.74, }, }, From 7a6b427faac0cc1f97ab02ae72d39560ba395359 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Fri, 14 Feb 2025 16:37:43 +0100 Subject: [PATCH 30/33] test(multi-srp): fix length mismatch unit test --- .../src/KeyringController.test.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 647b3089320..9fffe73bd79 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -271,14 +271,27 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: vaultWithOneKeyring, + vault: vaultWithOneKeyring, // pass non-empty vault keyringsMetadata: [ { id: '1', name: '' }, { id: '2', name: '' }, ], }, }, - async ({ controller }) => { + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: 'HD Key Tree', + data: { + keyrings: [ + { + type: 'HD Key Tree', + accounts: ['0x123'], + }, + ], + }, + }, + ]); await controller.submitPassword(password); await expect(controller.addNewAccount()).rejects.toThrow( 'KeyringController - keyring metadata length mismatch', From a0a142222950a43f26f258324175daa9754ad0b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Mon, 17 Feb 2025 11:03:46 +0100 Subject: [PATCH 31/33] chore(multi-srp): review fixes --- eslint-warning-thresholds.json | 2 +- .../src/KeyringController.test.ts | 24 ++++--------------- .../src/KeyringController.ts | 4 ++-- 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index ced0b0db54f..3a421bda00c 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -243,7 +243,7 @@ "n/no-unsupported-features/node-builtins": 1 }, "packages/keyring-controller/src/KeyringController.test.ts": { - "import-x/namespace": 15, + "import-x/namespace": 14, "jest/no-conditional-in-test": 8 }, "packages/keyring-controller/src/KeyringController.ts": { diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 9fffe73bd79..946339965a6 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -294,7 +294,7 @@ describe('KeyringController', () => { ]); await controller.submitPassword(password); await expect(controller.addNewAccount()).rejects.toThrow( - 'KeyringController - keyring metadata length mismatch', + KeyringControllerError.KeyringMetadataLengthMismatch, ); }, ); @@ -782,9 +782,9 @@ describe('KeyringController', () => { describe('when wrong password is provided', () => { it('should export seed phrase', async () => { await withController(async ({ controller, encryptor }) => { - sinon - .stub(encryptor, 'decrypt') - .throws(new Error('Invalid password')); + jest + .spyOn(encryptor, 'decrypt') + .mockRejectedValueOnce(new Error('Invalid password')); await expect(controller.exportSeedPhrase('')).rejects.toThrow( 'Invalid password', ); @@ -858,20 +858,6 @@ describe('KeyringController', () => { ); }); }); - - it('should throw invalid password error with valid keyringId', async () => { - await withController( - async ({ controller, encryptor, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; - jest - .spyOn(encryptor, 'decrypt') - .mockRejectedValueOnce(new Error('Invalid password')); - await expect( - controller.exportSeedPhrase('', keyringId), - ).rejects.toThrow('Invalid password'); - }, - ); - }); }); }); describe('when the keyring for the given address does not support exportAccount', () => { @@ -2707,7 +2693,7 @@ describe('KeyringController', () => { ); }); - it('should throw unspported seed phrase error when keyring is not HD', async () => { + it('should throw unsupported seed phrase error when keyring is not HD', async () => { await withController(async ({ controller }) => { await controller.addNewKeyring(KeyringTypes.simple, [privateKey]); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 88c86c6a867..3727a54c942 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2309,11 +2309,11 @@ export class KeyringController extends BaseController< async #newKeyring(type: string, data?: unknown): Promise> { const keyring = await this.#createKeyring(type, data); - this.#keyrings.push(keyring); - this.#keyringsMetadata.push(getDefaultKeyringMetadata()); if (this.#keyrings.length !== this.#keyringsMetadata.length) { throw new Error('Keyring metadata missing'); } + this.#keyrings.push(keyring); + this.#keyringsMetadata.push(getDefaultKeyringMetadata()); return keyring; } From f823f91f0840780ba7b48875300dd2b272bc1352 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Mon, 17 Feb 2025 15:59:59 +0100 Subject: [PATCH 32/33] chore(multi-srp): revert AccountsController changes --- packages/accounts-controller/src/AccountsController.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 7bca3818b97..4c582b03ab1 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -23,7 +23,6 @@ import { type KeyringControllerGetKeyringsByTypeAction, type KeyringControllerGetAccountsAction, type KeyringControllerStateChangeEvent, - type KeyringMetadata, KeyringTypes, } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; @@ -214,7 +213,6 @@ export type AccountsControllerMessenger = RestrictedMessenger< type AddressAndKeyringTypeObject = { address: string; type: string; - metadata: KeyringMetadata; }; const accountsControllerMetadata = { @@ -741,14 +739,13 @@ export class AccountsController extends BaseController< const updatedNormalKeyringAddresses: AddressAndKeyringTypeObject[] = []; const updatedSnapKeyringAddresses: AddressAndKeyringTypeObject[] = []; - for (const [index, keyring] of keyringState.keyrings.entries()) { + for (const keyring of keyringState.keyrings) { if (keyring.type === KeyringTypes.snap) { updatedSnapKeyringAddresses.push( ...keyring.accounts.map((address) => { return { address, type: keyring.type, - metadata: keyringState.keyringsMetadata[index], }; }), ); @@ -758,7 +755,6 @@ export class AccountsController extends BaseController< return { address, type: keyring.type, - metadata: keyringState.keyringsMetadata[index], }; }), ); From 66c3f050a1b051ff59a4912b4cd68d742f3e0fab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20=C5=81ucka?= <5708018+PatrykLucka@users.noreply.github.com> Date: Mon, 17 Feb 2025 16:39:12 +0100 Subject: [PATCH 33/33] chore(multi-srp): review changes --- .../src/KeyringController.test.ts | 3 +-- .../src/KeyringController.ts | 23 ++++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 946339965a6..8a4b8c29e29 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2699,7 +2699,7 @@ describe('KeyringController', () => { const keyringId = controller.state.keyringsMetadata[1].id; await expect(controller.verifySeedPhrase(keyringId)).rejects.toThrow( - 'KeyringController - The keyring does not support the method verifySeedPhrase.', + KeyringControllerError.UnsupportedVerifySeedPhrase, ); }); }); @@ -4060,7 +4060,6 @@ describe('KeyringController', () => { await controller.persistAllKeyrings(); } }); - // TODO: Either fix this lint violation or explain why it's necessary to ignore. messenger.subscribe('KeyringController:stateChange', listener); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 3727a54c942..ec36a52d00b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1516,10 +1516,7 @@ export class KeyringController extends BaseController< )) as SelectedKeyring; } } else if ('id' in selector) { - const index = this.state.keyringsMetadata.findIndex( - (metadata) => metadata.id === selector.id, - ); - keyring = this.#keyrings[index] as SelectedKeyring; + keyring = this.#getKeyringById(selector.id) as SelectedKeyring; } if (!keyring) { @@ -1853,6 +1850,19 @@ export class KeyringController extends BaseController< ); } + /** + * Get the keyring by id. + * + * @param keyringId - The id of the keyring. + * @returns The keyring. + */ + #getKeyringById(keyringId: string): EthKeyring | undefined { + const index = this.state.keyringsMetadata.findIndex( + (metadata) => metadata.id === keyringId, + ); + return this.#keyrings[index]; + } + /** * Get the keyring by id or return the first keyring if the id is not found. * @@ -1864,10 +1874,7 @@ export class KeyringController extends BaseController< return this.#keyrings[0] as EthKeyring; } - const index = this.state.keyringsMetadata.findIndex( - (metadata) => metadata.id === keyringId, - ); - return this.#keyrings[index]; + return this.#getKeyringById(keyringId); } /**