From 1319d5df7d04cf263f8766e805d27892a14d0061 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 13:29:55 +0100 Subject: [PATCH 01/15] merge eth-keyring-controller logic --- packages/keyring-controller/jest.config.js | 8 +- packages/keyring-controller/package.json | 4 + .../src/KeyringController.test.ts | 2 +- .../src/KeyringController.ts | 927 +++++++++++++++--- packages/keyring-controller/src/constants.ts | 34 + packages/keyring-controller/src/utils.ts | 8 + types/@metamask/eth-hd-keyring.d.ts | 2 + types/@metamask/eth-simple-keyring.d.ts | 2 + yarn.lock | 3 + 9 files changed, 869 insertions(+), 121 deletions(-) create mode 100644 packages/keyring-controller/src/constants.ts create mode 100644 packages/keyring-controller/src/utils.ts create mode 100644 types/@metamask/eth-hd-keyring.d.ts create mode 100644 types/@metamask/eth-simple-keyring.d.ts diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 0e525e1f76..708bbc752a 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: 100, - functions: 100, - lines: 100, - statements: 100, + branches: 74.14, + functions: 98.95, + lines: 93.84, + statements: 93.88, }, }, diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index b2d0e4f124..c9f9949802 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -33,7 +33,11 @@ "dependencies": { "@keystonehq/metamask-airgapped-keyring": "^0.13.1", "@metamask/base-controller": "^4.1.1", + "@metamask/browser-passworder": "^4.3.0", + "@metamask/eth-hd-keyring": "^7.0.1", "@metamask/eth-keyring-controller": "^17.0.1", + "@metamask/eth-sig-util": "^7.0.0", + "@metamask/eth-simple-keyring": "^6.0.1", "@metamask/keyring-api": "^3.0.0", "@metamask/message-manager": "^7.3.8", "@metamask/utils": "^8.3.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index cacbc57121..6c52da206a 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1214,7 +1214,7 @@ describe('KeyringController', () => { data: '', from: initialState.keyrings[0].accounts[0], }), - ).toThrow("Can't sign an empty message"); + ).rejects.toThrow("Can't sign an empty message"); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 94cbee33ff..00036bd6e2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -5,14 +5,15 @@ import type { } from '@keystonehq/metamask-airgapped-keyring'; import type { RestrictedControllerMessenger } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; -import { - KeyringController as EthKeyringController, - KeyringType, -} from '@metamask/eth-keyring-controller'; +import * as encryptorUtils from '@metamask/browser-passworder'; +import HDKeyring from '@metamask/eth-hd-keyring'; +import { KeyringType } from '@metamask/eth-keyring-controller'; import type { ExportableKeyEncryptor, GenericEncryptor, } from '@metamask/eth-keyring-controller/dist/types'; +import { normalize } from '@metamask/eth-sig-util'; +import SimpleKeyring from '@metamask/eth-simple-keyring'; import type { EthBaseTransaction, EthBaseUserOperation, @@ -24,8 +25,19 @@ import type { PersonalMessageParams, TypedMessageParams, } from '@metamask/message-manager'; -import type { Eip1024EncryptedData, Hex, Json } from '@metamask/utils'; -import { assertIsStrictHexString, hasProperty } from '@metamask/utils'; +import type { + Eip1024EncryptedData, + Hex, + Json, + KeyringClass, +} from '@metamask/utils'; +import { + assertIsStrictHexString, + hasProperty, + isObject, + isValidHexAddress, + remove0x, +} from '@metamask/utils'; import { Mutex } from 'async-mutex'; import { addHexPrefix, @@ -38,6 +50,9 @@ import { import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; +import { KeyringControllerError } from './constants'; +import { throwError } from './utils'; + const name = 'KeyringController'; /** @@ -248,6 +263,32 @@ export enum SignTypedDataVersion { V4 = 'V4', } +export type SerializedKeyring = { + type: string; + data: Json; +}; + +/** + * Get builder function for `Keyring` + * + * Returns a builder function for `Keyring` with a `type` property. + * + * @param KeyringConstructor - The Keyring class for the builder. + * @returns A builder function for the given Keyring. + */ +export function keyringBuilderFactory(KeyringConstructor: KeyringClass) { + const builder = () => new KeyringConstructor(); + + builder.type = KeyringConstructor.type; + + return builder; +} + +const defaultKeyringBuilders = [ + keyringBuilderFactory(SimpleKeyring), + keyringBuilderFactory(HDKeyring), +]; + export const getDefaultKeyringState = (): KeyringControllerState => { return { isUnlocked: false, @@ -274,6 +315,67 @@ function assertHasUint8ArrayMnemonic( } } +/** + * Assert that the provided encryptor supports + * encryption and encryption key export. + * + * @param encryptor - The encryptor to check. + * @throws If the encryptor does not support key encryption. + */ +function assertIsExportableKeyEncryptor( + encryptor: GenericEncryptor | ExportableKeyEncryptor, +): asserts encryptor is ExportableKeyEncryptor { + if ( + !( + 'importKey' in encryptor && + typeof encryptor.importKey === 'function' && + 'decryptWithKey' in encryptor && + typeof encryptor.decryptWithKey === 'function' && + 'encryptWithKey' in encryptor && + typeof encryptor.encryptWithKey === 'function' + ) + ) { + throw new Error(KeyringControllerError.UnsupportedEncryptionKeyExport); + } +} + +/** + * Checks if the provided value is a serialized keyrings array. + * + * @param array - The value to check. + * @returns True if the value is a serialized keyrings array. + */ +function isSerializedKeyringsArray( + array: unknown, +): array is SerializedKeyring[] { + return ( + typeof array === 'object' && + Array.isArray(array) && + array.every((value) => value.type && value.data) + ); +} + +/** + * Display For Keyring + * + * Is used for adding the current keyrings to the state object. + * + * @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[] }> { + const accounts = await keyring.getAccounts(); + + return { + type: keyring.type, + // Cast to `Hex[]` here is safe here because `addresses` has no nullish + // values, and `normalizeToHex` returns `Hex` unless given a nullish value + accounts: accounts.map(normalize) as Hex[], + }; +} + /** * Controller responsible for establishing and managing user identity. * @@ -298,7 +400,17 @@ export class KeyringController extends BaseController< private readonly setAccountLabel?: (address: string, label: string) => void; - #keyring: EthKeyringController; + #keyringBuilders: { (): EthKeyring; type: string }[]; + + #keyrings: EthKeyring[]; + + #unsupportedKeyrings: SerializedKeyring[]; + + #password?: string; + + #encryptor: GenericEncryptor | ExportableKeyEncryptor; + + #cacheEncryptionKey: boolean; #qrKeyringStateListener?: ( state: ReturnType, @@ -324,6 +436,7 @@ export class KeyringController extends BaseController< updateIdentities, setSelectedAddress, setAccountLabel, + encryptor = encryptorUtils, keyringBuilders, messenger, state, @@ -345,25 +458,20 @@ export class KeyringController extends BaseController< }, }); - if (options.cacheEncryptionKey) { - this.#keyring = new EthKeyringController({ - initState: state, - encryptor: options.encryptor, - keyringBuilders, - cacheEncryptionKey: options.cacheEncryptionKey, - }); - } else { - this.#keyring = new EthKeyringController({ - initState: state, - encryptor: options.encryptor, - keyringBuilders, - cacheEncryptionKey: options.cacheEncryptionKey ?? false, - }); + this.#keyringBuilders = keyringBuilders + ? defaultKeyringBuilders.concat(keyringBuilders) + : defaultKeyringBuilders; + + this.#encryptor = encryptor; + this.#keyrings = []; + this.#unsupportedKeyrings = []; + + // This option allows the controller to cache an exported key + // for use in decrypting and encrypting data without password + this.#cacheEncryptionKey = Boolean(options.cacheEncryptionKey); + if (this.#cacheEncryptionKey) { + assertIsExportableKeyEncryptor(encryptor); } - this.#keyring.memStore.subscribe(this.#fullUpdate.bind(this)); - this.#keyring.store.subscribe(this.#fullUpdate.bind(this)); - this.#keyring.on('lock', this.#handleLock.bind(this)); - this.#keyring.on('unlock', this.#handleUnlock.bind(this)); this.syncIdentities = syncIdentities; this.updateIdentities = updateIdentities; @@ -385,11 +493,13 @@ export class KeyringController extends BaseController< keyringState: KeyringControllerMemState; addedAccountAddress: string; }> { - const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; + const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as + | EthKeyring + | undefined; if (!primaryKeyring) { throw new Error('No HD keyring found'); } - const oldAccounts = await this.#keyring.getAccounts(); + const oldAccounts = await this.getAccounts(); if (accountCount && oldAccounts.length !== accountCount) { if (accountCount > oldAccounts.length) { @@ -403,8 +513,8 @@ export class KeyringController extends BaseController< }; } - await this.#keyring.addNewAccount(primaryKeyring); - const newAccounts = await this.#keyring.getAccounts(); + await this.addNewAccountForKeyring(primaryKeyring); + const newAccounts = await this.getAccounts(); await this.verifySeedPhrase(); @@ -444,13 +554,15 @@ export class KeyringController extends BaseController< return existingAccount; } - await this.#keyring.addNewAccount(keyring); + await keyring.addAccounts(1); + await this.persistAllKeyrings(); + const addedAccountAddress = (await this.getAccounts()).find( (selectedAddress) => !oldAccounts.includes(selectedAddress), ); assertIsStrictHexString(addedAccountAddress); - this.updateIdentities(await this.#keyring.getAccounts()); + this.updateIdentities(await this.getAccounts()); return addedAccountAddress; } @@ -461,15 +573,50 @@ export class KeyringController extends BaseController< * @returns Promise resolving to current state when the account is added. */ async addNewAccountWithoutUpdate(): Promise { - const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; + const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as + | EthKeyring + | undefined; if (!primaryKeyring) { throw new Error('No HD keyring found'); } - await this.#keyring.addNewAccount(primaryKeyring); + await primaryKeyring.addAccounts(1); + await this.persistAllKeyrings(); await this.verifySeedPhrase(); return this.#getMemState(); } + /** + * Create new vault And with a specific keyring + * + * Destroys any old encrypted storage, + * creates a new encrypted store with the given password, + * creates a new wallet with 1 account. + * + * @fires KeyringController#unlock + * @param password - The password to encrypt the vault with. + * @param keyring - A object containing the params to instantiate a new keyring. + * @param keyring.type - The keyring type. + * @param keyring.opts - Optional parameters required to instantiate the keyring. + * @returns A promise that resolves to the state. + */ + async createNewVaultWithKeyring( + password: string, + keyring: { + type: string; + opts?: unknown; + }, + ): Promise { + if (typeof password !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + this.#password = password; + + await this.#clearKeyrings(); + await this.#createKeyring(keyring.type, keyring.opts); + this.#setUnlocked(); + return this.#getMemState(); + } + /** * Effectively the same as creating a new keychain then populating it * using the given seed phrase. @@ -490,14 +637,14 @@ export class KeyringController extends BaseController< try { this.updateIdentities([]); - await this.#keyring.createNewVaultWithKeyring(password, { + await this.createNewVaultWithKeyring(password, { type: KeyringType.HD, opts: { mnemonic: seed, numberOfAccounts: 1, }, }); - this.updateIdentities(await this.#keyring.getAccounts()); + this.updateIdentities(await this.getAccounts()); return this.#getMemState(); } finally { releaseLock(); @@ -515,7 +662,7 @@ export class KeyringController extends BaseController< try { const accounts = await this.getAccounts(); if (!accounts.length) { - await this.#keyring.createNewVaultWithKeyring(password, { + await this.createNewVaultWithKeyring(password, { type: KeyringType.HD, }); this.updateIdentities(await this.getAccounts()); @@ -542,7 +689,30 @@ export class KeyringController extends BaseController< return this.getOrAddQRKeyring(); } - return this.#keyring.addNewKeyring(type, opts); + const keyring = await this.#newKeyring(type, opts); + + if (!keyring) { + throw new Error(KeyringControllerError.NoKeyring); + } + + if (type === KeyringType.HD && (!isObject(opts) || !opts.mnemonic)) { + if (!keyring.generateRandomMnemonic) { + throw new Error( + KeyringControllerError.UnsupportedGenerateRandomMnemonic, + ); + } + + keyring.generateRandomMnemonic(); + await keyring.addAccounts(1); + } + + const accounts = await keyring.getAccounts(); + await this.#checkForDuplicate(type, accounts); + + this.#keyrings.push(keyring); + await this.persistAllKeyrings(); + + return keyring; } /** @@ -552,7 +722,10 @@ export class KeyringController extends BaseController< * @param password - Password of the keyring. */ async verifyPassword(password: string) { - await this.#keyring.verifyPassword(password); + if (!this.state.vault) { + throw new Error(KeyringControllerError.VaultError); + } + await this.#encryptor.decrypt(password, this.state.vault); } /** @@ -572,8 +745,8 @@ export class KeyringController extends BaseController< */ async exportSeedPhrase(password: string): Promise { await this.verifyPassword(password); - assertHasUint8ArrayMnemonic(this.#keyring.keyrings[0]); - return this.#keyring.keyrings[0].mnemonic; + assertHasUint8ArrayMnemonic(this.#keyrings[0]); + return this.#keyrings[0].mnemonic; } /** @@ -585,7 +758,15 @@ export class KeyringController extends BaseController< */ async exportAccount(password: string, address: string): Promise { await this.verifyPassword(password); - return this.#keyring.exportAccount(address); + + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + if (!keyring.exportAccount) { + throw new Error(KeyringControllerError.UnsupportedExportAccount); + } + + return await keyring.exportAccount(normalize(address) as Hex); } /** @@ -593,8 +774,19 @@ export class KeyringController extends BaseController< * * @returns A promise resolving to an array of addresses. */ - getAccounts(): Promise { - return this.#keyring.getAccounts(); + async getAccounts(): Promise { + const keyrings = this.#keyrings; + + const keyringArrays = await Promise.all( + keyrings.map(async (keyring) => keyring.getAccounts()), + ); + const addresses = keyringArrays.reduce((res, arr) => { + return res.concat(arr); + }, []); + + // Cast to `Hex[]` here is safe here because `addresses` has no nullish + // values, and `normalize` returns `Hex` unless given a nullish value + return addresses.map(normalize) as Hex[]; } /** @@ -609,7 +801,15 @@ export class KeyringController extends BaseController< account: string, opts?: Record, ): Promise { - return this.#keyring.getEncryptionPublicKey(account, opts); + const normalizedAddress = normalize(account) as Hex; + const keyring = (await this.getKeyringForAccount( + account, + )) as EthKeyring; + if (!keyring.getEncryptionPublicKey) { + throw new Error(KeyringControllerError.UnsupportedGetEncryptionPublicKey); + } + + return await keyring.getEncryptionPublicKey(normalizedAddress, opts); } /** @@ -624,7 +824,15 @@ export class KeyringController extends BaseController< from: string; data: Eip1024EncryptedData; }): Promise { - return this.#keyring.decryptMessage(messageParams); + const address = normalize(messageParams.from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + if (!keyring.decryptMessage) { + throw new Error(KeyringControllerError.UnsupportedDecryptMessage); + } + + return keyring.decryptMessage(address, messageParams.data); } /** @@ -638,7 +846,37 @@ export class KeyringController extends BaseController< * @returns Promise resolving to keyring of the `account` if one exists. */ async getKeyringForAccount(account: string): Promise { - return this.#keyring.getKeyringForAccount(account); + // Cast to `Hex` here is safe here because `address` is not nullish. + // `normalizeToHex` returns `Hex` unless given a nullish value. + const hexed = normalize(account) as Hex; + + const candidates = await Promise.all( + this.#keyrings.map(async (keyring) => { + return Promise.all([keyring, keyring.getAccounts()]); + }), + ); + + const winners = candidates.filter((candidate) => { + const accounts = candidate[1].map(normalize); + return accounts.includes(hexed); + }); + + if (winners.length && winners[0]?.length) { + return winners[0][0]; + } + + // Adding more info to the error + let errorInfo = ''; + if (!isValidHexAddress(hexed)) { + errorInfo = 'The address passed in is invalid/empty'; + } else if (!candidates.length) { + errorInfo = 'There are no keyrings'; + } else if (!winners.length) { + errorInfo = 'There are keyrings, but none match the address'; + } + throw new Error( + `${KeyringControllerError.NoKeyring}. Error info: ${errorInfo}`, + ); } /** @@ -651,7 +889,7 @@ export class KeyringController extends BaseController< * @returns An array of keyrings of the given type. */ getKeyringsByType(type: KeyringTypes | string): unknown[] { - return this.#keyring.getKeyringsByType(type); + return this.#keyrings.filter((keyring) => keyring.type === type); } /** @@ -661,7 +899,76 @@ export class KeyringController extends BaseController< * operation completes. */ async persistAllKeyrings(): Promise { - return this.#keyring.persistAllKeyrings(); + const { encryptionKey, encryptionSalt } = this.state; + + if (!this.#password && !encryptionKey) { + throw new Error(KeyringControllerError.MissingCredentials); + } + + const serializedKeyrings = await Promise.all( + this.#keyrings.map(async (keyring) => { + const [type, data] = await Promise.all([ + keyring.type, + keyring.serialize(), + ]); + return { type, data }; + }), + ); + + serializedKeyrings.push(...this.#unsupportedKeyrings); + + let vault: string | undefined; + let newEncryptionKey: string | undefined; + + if (this.#cacheEncryptionKey) { + assertIsExportableKeyEncryptor(this.#encryptor); + + if (encryptionKey) { + const key = await this.#encryptor.importKey(encryptionKey); + const vaultJSON = await this.#encryptor.encryptWithKey( + key, + serializedKeyrings, + ); + vaultJSON.salt = encryptionSalt; + vault = JSON.stringify(vaultJSON); + } else if (this.#password) { + const { vault: newVault, exportedKeyString } = + await this.#encryptor.encryptWithDetail( + this.#password, + serializedKeyrings, + ); + + vault = newVault; + newEncryptionKey = exportedKeyString; + } + } else { + if (typeof this.#password !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + vault = await this.#encryptor.encrypt(this.#password, serializedKeyrings); + } + + if (!vault) { + throw new Error(KeyringControllerError.MissingVaultData); + } + + this.update((state) => { + state.vault = vault; + }); + + // The keyring updates need to be announced before updating the encryptionKey + // so that the updated keyring gets propagated to the extension first. + // Not calling {@link updateMemStoreKeyrings} results in the wrong account being selected + // in the extension. + await this.#updateMemStoreKeyrings(); + if (newEncryptionKey) { + this.update((state) => { + state.encryptionKey = newEncryptionKey; + state.encryptionSalt = JSON.parse(vault as string).salt; + }); + } + + return true; } /** @@ -721,11 +1028,11 @@ export class KeyringController extends BaseController< default: throw new Error(`Unexpected import strategy: '${strategy}'`); } - const newKeyring = await this.#keyring.addNewKeyring(KeyringTypes.simple, [ + const newKeyring = (await this.addNewKeyring(KeyringTypes.simple, [ privateKey, - ]); + ])) as EthKeyring; const accounts = await newKeyring.getAccounts(); - const allAccounts = await this.#keyring.getAccounts(); + const allAccounts = await this.getAccounts(); this.updateIdentities(allAccounts); return { keyringState: this.#getMemState(), @@ -741,7 +1048,28 @@ export class KeyringController extends BaseController< * @returns Promise resolving current state when this account removal completes. */ async removeAccount(address: Hex): Promise { - await this.#keyring.removeAccount(address); + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + + // Not all the keyrings support this, so we have to check + if (!keyring.removeAccount) { + throw new Error(KeyringControllerError.UnsupportedRemoveAccount); + } + + // The `removeAccount` method of snaps keyring is async. We have to update + // the interface of the other keyrings to be async as well. + // eslint-disable-next-line @typescript-eslint/await-thenable + await keyring.removeAccount(address); + + const accounts = await keyring.getAccounts(); + // Check if this was the last/only account + if (accounts.length === 0) { + await this.#removeEmptyKeyrings(); + } + + await this.persistAllKeyrings(); + this.messagingSystem.publish(`${name}:accountRemoved`, address); return this.#getMemState(); } @@ -753,7 +1081,16 @@ export class KeyringController extends BaseController< */ async setLocked(): Promise { this.#unsubscribeFromQRKeyringsEvents(); - await this.#keyring.setLocked(); + + this.#password = undefined; + this.update((state) => { + state.isUnlocked = false; + state.keyrings = []; + }); + await this.#clearKeyrings(); + + this.messagingSystem.publish(`${name}:lock`); + return this.#getMemState(); } @@ -763,11 +1100,20 @@ export class KeyringController extends BaseController< * @param messageParams - PersonalMessageParams object to sign. * @returns Promise resolving to a signed message string. */ - signMessage(messageParams: PersonalMessageParams) { + async signMessage(messageParams: PersonalMessageParams): Promise { if (!messageParams.data) { throw new Error("Can't sign an empty message"); } - return this.#keyring.signMessage(messageParams); + + const address = normalize(messageParams.from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + if (!keyring.signMessage) { + throw new Error(KeyringControllerError.UnsupportedSignMessage); + } + + return await keyring.signMessage(address, messageParams.data); } /** @@ -776,8 +1122,18 @@ export class KeyringController extends BaseController< * @param messageParams - PersonalMessageParams object to sign. * @returns Promise resolving to a signed message string. */ - signPersonalMessage(messageParams: PersonalMessageParams) { - return this.#keyring.signPersonalMessage(messageParams); + async signPersonalMessage(messageParams: PersonalMessageParams) { + const address = normalize(messageParams.from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + if (!keyring.signPersonalMessage) { + throw new Error(KeyringControllerError.UnsupportedSignPersonalMessage); + } + + const normalizedData = normalize(messageParams.data) as Hex; + + return await keyring.signPersonalMessage(address, normalizedData); } /** @@ -803,15 +1159,22 @@ export class KeyringController extends BaseController< throw new Error(`Unexpected signTypedMessage version: '${version}'`); } - return await this.#keyring.signTypedMessage( - { - from: messageParams.from, - data: - version !== SignTypedDataVersion.V1 && - typeof messageParams.data === 'string' - ? JSON.parse(messageParams.data) - : messageParams.data, - }, + // Cast to `Hex` here is safe here because `messageParams.from` is not nullish. + // `normalize` returns `Hex` unless given a nullish value. + const address = normalize(messageParams.from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + if (!keyring.signTypedData) { + throw new Error(KeyringControllerError.UnsupportedSignTypedMessage); + } + + return await keyring.signTypedData( + address, + version !== SignTypedDataVersion.V1 && + typeof messageParams.data === 'string' + ? JSON.parse(messageParams.data) + : messageParams.data, { version }, ); } catch (error) { @@ -827,12 +1190,20 @@ export class KeyringController extends BaseController< * @param opts - An optional options object. * @returns Promise resolving to a signed transaction string. */ - signTransaction( + async signTransaction( transaction: TypedTransaction, from: string, opts?: Record, ): Promise { - return this.#keyring.signTransaction(transaction, from, opts); + const address = normalize(from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + if (!keyring.signTransaction) { + throw new Error(KeyringControllerError.UnsupportedSignTransaction); + } + + return await keyring.signTransaction(address, transaction, opts); } /** @@ -846,7 +1217,14 @@ export class KeyringController extends BaseController< from: string, transactions: EthBaseTransaction[], ): Promise { - return await this.#keyring.prepareUserOperation(from, transactions); + const address = normalize(from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + + return keyring.prepareUserOperation + ? await keyring.prepareUserOperation(address, transactions) + : throwError(KeyringControllerError.UnsupportedPrepareUserOperation); } /** @@ -861,7 +1239,14 @@ export class KeyringController extends BaseController< from: string, userOp: EthUserOperation, ): Promise { - return await this.#keyring.patchUserOperation(from, userOp); + const address = normalize(from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + + return keyring.patchUserOperation + ? await keyring.patchUserOperation(address, userOp) + : throwError(KeyringControllerError.UnsupportedPatchUserOperation); } /** @@ -875,7 +1260,14 @@ export class KeyringController extends BaseController< from: string, userOp: EthUserOperation, ): Promise { - return await this.#keyring.signUserOperation(from, userOp); + const address = normalize(from) as Hex; + const keyring = (await this.getKeyringForAccount( + address, + )) as EthKeyring; + + return keyring.signUserOperation + ? await keyring.signUserOperation(address, userOp) + : throwError(KeyringControllerError.UnsupportedSignUserOperation); } /** @@ -890,7 +1282,12 @@ export class KeyringController extends BaseController< encryptionKey: string, encryptionSalt: string, ): Promise { - await this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); + this.#keyrings = await this.#unlockKeyrings( + undefined, + encryptionKey, + encryptionSalt, + ); + this.#setUnlocked(); const qrKeyring = this.getQRKeyring(); if (qrKeyring) { @@ -910,8 +1307,10 @@ export class KeyringController extends BaseController< * @returns Promise resolving to the current state. */ async submitPassword(password: string): Promise { - await this.#keyring.submitPassword(password); - const accounts = await this.#keyring.getAccounts(); + this.#keyrings = await this.#unlockKeyrings(password); + this.#setUnlocked(); + + const accounts = await this.getAccounts(); const qrKeyring = this.getQRKeyring(); if (qrKeyring) { @@ -930,7 +1329,9 @@ export class KeyringController extends BaseController< * @returns Promise resolving to the seed phrase as Uint8Array. */ async verifySeedPhrase(): Promise { - const primaryKeyring = this.#keyring.getKeyringsByType(KeyringTypes.hd)[0]; + const primaryKeyring = this.getKeyringsByType(KeyringTypes.hd)[0] as + | EthKeyring + | undefined; if (!primaryKeyring) { throw new Error('No HD keyring found.'); } @@ -946,9 +1347,7 @@ export class KeyringController extends BaseController< // The HD Keyring Builder is a default keyring builder // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const hdKeyringBuilder = this.#keyring.getKeyringBuilderForType( - KeyringTypes.hd, - )!; + const hdKeyringBuilder = this.#getKeyringBuilderForType(KeyringTypes.hd)!; const hdKeyring = hdKeyringBuilder(); // @ts-expect-error @metamask/eth-hd-keyring correctly handles @@ -982,9 +1381,7 @@ export class KeyringController extends BaseController< */ getQRKeyring(): QRKeyring | undefined { // QRKeyring is not yet compatible with Keyring type from @metamask/utils - return this.#keyring.getKeyringsByType( - KeyringTypes.qr, - )[0] as unknown as QRKeyring; + return this.getKeyringsByType(KeyringTypes.qr)[0] as unknown as QRKeyring; } /** @@ -1000,8 +1397,8 @@ export class KeyringController extends BaseController< // eslint-disable-next-line @typescript-eslint/no-explicit-any async restoreQRKeyring(serialized: any): Promise { (await this.getOrAddQRKeyring()).deserialize(serialized); - await this.#keyring.persistAllKeyrings(); - this.updateIdentities(await this.#keyring.getAccounts()); + await this.persistAllKeyrings(); + this.updateIdentities(await this.getAccounts()); } async resetQRKeyringState(): Promise { @@ -1074,13 +1471,13 @@ export class KeyringController extends BaseController< const keyring = await this.getOrAddQRKeyring(); keyring.setAccountToUnlock(index); - const oldAccounts = await this.#keyring.getAccounts(); + const oldAccounts = await this.getAccounts(); // QRKeyring is not yet compatible with Keyring from // @metamask/utils, but we can use the `addNewAccount` method // as it internally calls `addAccounts` from on the keyring instance, // which is supported by QRKeyring API. - await this.#keyring.addNewAccount(keyring as unknown as EthKeyring); - const newAccounts = await this.#keyring.getAccounts(); + await this.addNewAccountForKeyring(keyring as unknown as EthKeyring); + const newAccounts = await this.getAccounts(); this.updateIdentities(newAccounts); newAccounts.forEach((address: string) => { if (!oldAccounts.includes(address)) { @@ -1090,11 +1487,14 @@ export class KeyringController extends BaseController< this.setSelectedAddress(address); } }); - await this.#keyring.persistAllKeyrings(); + await this.persistAllKeyrings(); } async getAccountKeyringType(account: string): Promise { - return (await this.#keyring.getKeyringForAccount(account)).type; + const keyring = (await this.getKeyringForAccount( + account, + )) as EthKeyring; + return keyring.type; } async forgetQRDevice(): Promise<{ @@ -1102,14 +1502,14 @@ export class KeyringController extends BaseController< remainingAccounts: string[]; }> { const keyring = await this.getOrAddQRKeyring(); - const allAccounts = (await this.#keyring.getAccounts()) as string[]; + const allAccounts = (await this.getAccounts()) as string[]; keyring.forgetDevice(); - const remainingAccounts = (await this.#keyring.getAccounts()) as string[]; + const remainingAccounts = (await this.getAccounts()) as string[]; const removedAccounts = allAccounts.filter( (address: string) => !remainingAccounts.includes(address), ); this.updateIdentities(remainingAccounts); - await this.#keyring.persistAllKeyrings(); + await this.persistAllKeyrings(); return { removedAccounts, remainingAccounts }; } @@ -1179,6 +1579,25 @@ export class KeyringController extends BaseController< ); } + /** + * Get Keyring Class For Type + * + * Searches the current `keyringBuilders` array + * for a Keyring builder whose unique `type` property + * matches the provided `type`, + * returning it if it exists. + * + * @param type - The type whose class to get. + * @returns The class, if it exists. + */ + #getKeyringBuilderForType( + type: string, + ): { (): EthKeyring; type: string } | undefined { + return this.#keyringBuilders.find( + (keyringBuilder) => keyringBuilder.type === type, + ); + } + /** * Add qr hardware keyring. * @@ -1188,9 +1607,19 @@ export class KeyringController extends BaseController< */ async #addQRKeyring(): Promise { // QRKeyring is not yet compatible with Keyring type from @metamask/utils - const qrKeyring = (await this.#keyring.addNewKeyring( - KeyringTypes.qr, - )) as unknown as QRKeyring; + const qrKeyring = (await this.#newKeyring(KeyringTypes.qr, { + accounts: [], + })) as unknown as QRKeyring; + + if (!qrKeyring) { + throw new Error(KeyringControllerError.NoKeyring); + } + + const accounts = await qrKeyring.getAccounts(); + await this.#checkForDuplicate(KeyringTypes.qr, accounts); + + this.#keyrings.push(qrKeyring as unknown as EthKeyring); + await this.persistAllKeyrings(); this.#subscribeToQRKeyringEvents(qrKeyring); @@ -1212,7 +1641,7 @@ export class KeyringController extends BaseController< } #unsubscribeFromQRKeyringsEvents() { - const qrKeyrings = this.#keyring.getKeyringsByType( + const qrKeyrings = this.getKeyringsByType( KeyringTypes.qr, ) as unknown as QRKeyring[]; @@ -1224,40 +1653,306 @@ export class KeyringController extends BaseController< } /** - * Sync controller state with current keyring store - * and memStore states. + * Update memStore Keyrings * - * @fires KeyringController:stateChange + * Updates the in-memory keyrings, without persisting. */ - #fullUpdate() { - const { vault } = this.#keyring.store.getState(); - const { keyrings, isUnlocked, encryptionKey, encryptionSalt } = - this.#keyring.memStore.getState(); - - this.update(() => ({ - vault, - keyrings, - isUnlocked, - encryptionKey, - encryptionSalt, - })); + async #updateMemStoreKeyrings(): Promise { + const keyrings = await Promise.all(this.#keyrings.map(displayForKeyring)); + this.update((state) => { + state.keyrings = keyrings; + }); } /** - * Handle keyring lock event. + * Unlock Keyrings. + * + * Attempts to unlock the persisted encrypted storage, + * initializing the persisted keyrings to RAM. * - * @fires KeyringController:lock + * @param password - The keyring controller password. + * @param encryptionKey - An exported key string to unlock keyrings with. + * @param encryptionSalt - The salt used to encrypt the vault. + * @returns The keyrings array. */ - #handleLock() { - this.messagingSystem.publish(`${name}:lock`); + async #unlockKeyrings( + password: string | undefined, + encryptionKey?: string, + encryptionSalt?: string, + ): Promise[]> { + const encryptedVault = this.state.vault; + if (!encryptedVault) { + throw new Error(KeyringControllerError.VaultError); + } + + await this.#clearKeyrings(); + + let vault; + + if (this.#cacheEncryptionKey) { + assertIsExportableKeyEncryptor(this.#encryptor); + + if (password) { + const result = await this.#encryptor.decryptWithDetail( + password, + encryptedVault, + ); + vault = result.vault; + this.#password = password; + + this.update((state) => { + state.encryptionKey = result.exportedKeyString; + state.encryptionSalt = result.salt; + }); + } else { + const parsedEncryptedVault = JSON.parse(encryptedVault); + + if (encryptionSalt !== parsedEncryptedVault.salt) { + throw new Error(KeyringControllerError.ExpiredCredentials); + } + + if (typeof encryptionKey !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + + const key = await this.#encryptor.importKey(encryptionKey); + vault = await this.#encryptor.decryptWithKey(key, parsedEncryptedVault); + + // This call is required on the first call because encryptionKey + // is not yet inside the memStore + this.update((state) => { + state.encryptionKey = encryptionKey; + // we can safely assume that encryptionSalt is defined here + // because we compare it with the salt from the vault + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + state.encryptionSalt = encryptionSalt!; + }); + } + } else { + if (typeof password !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + + vault = await this.#encryptor.decrypt(password, encryptedVault); + this.#password = password; + } + + if (!isSerializedKeyringsArray(vault)) { + throw new Error(KeyringControllerError.VaultDataError); + } + + await Promise.all(vault.map(this.#restoreKeyring.bind(this))); + await this.#updateMemStoreKeyrings(); + + if ( + this.#password && + (!this.#cacheEncryptionKey || !encryptionKey) && + this.#encryptor.isVaultUpdated && + !this.#encryptor.isVaultUpdated(encryptedVault) + ) { + // Re-encrypt the vault with safer method if one is available + await this.persistAllKeyrings(); + } + + return this.#keyrings; } /** - * Handle keyring unlock event. + * Create keyring. + * + * - Creates a new vault. + * - Creates a new keyring with at least one account. + * - Makes the first account the selected account. + * @param type - Keyring type to instantiate. + * @param opts - Optional parameters required to instantiate the keyring. + * @returns A promise that resolves if the operation was successful. + */ + async #createKeyring(type: string, opts?: unknown) { + const keyring = (await this.addNewKeyring(type, opts)) as + | EthKeyring + | undefined; + if (!keyring) { + throw new Error(KeyringControllerError.NoKeyring); + } + + const [firstAccount] = await keyring.getAccounts(); + if (!firstAccount) { + throw new Error(KeyringControllerError.NoFirstAccount); + } + } + + /** + * Instantiate, initialize and return a new keyring + * + * The keyring instantiated is of the given `type`. + * + * @param type - The type of keyring to add. + * @param data - The data to restore a previously serialized keyring. + * @returns The new keyring. + */ + async #newKeyring(type: string, data: unknown): Promise> { + const keyringBuilder = this.#getKeyringBuilderForType(type); + + if (!keyringBuilder) { + throw new Error( + `${KeyringControllerError.NoKeyringBuilder}. Keyring type: ${type}`, + ); + } + + const keyring = keyringBuilder(); + + // @ts-expect-error Enforce data type after updating clients + await keyring.deserialize(data); + + if (keyring.init) { + await keyring.init(); + } + + return keyring; + } + + /** + * Clear Keyrings + * + * Deallocates all currently managed keyrings and accounts. + * Used before initializing a new vault and after locking + * MetaMask. + */ + async #clearKeyrings() { + // clear keyrings from memory + for (const keyring of this.#keyrings) { + await this.#destroyKeyring(keyring); + } + this.#keyrings = []; + this.update((state) => { + state.keyrings = []; + }); + } + + /** + * Restore Keyring Helper + * + * Attempts to initialize a new keyring from the provided serialized payload. + * On success, returns the resulting keyring instance. + * + * @param serialized - The serialized keyring. + * @param serialized.type - Keyring type. + * @param serialized.data - Keyring data. + * @returns The deserialized keyring or undefined if the keyring type is unsupported. + */ + async #restoreKeyring( + serialized: SerializedKeyring, + ): Promise | undefined> { + const { type, data } = serialized; + + let keyring: EthKeyring | undefined; + try { + keyring = await this.#newKeyring(type, data); + } catch (error) { + // Ignore error. + console.error(error); + } + + if (!keyring) { + this.#unsupportedKeyrings.push(serialized); + return undefined; + } + + // getAccounts also validates the accounts for some keyrings + await keyring.getAccounts(); + this.#keyrings.push(keyring); + return keyring; + } + + /** + * Destroy Keyring + * + * Some keyrings support a method called `destroy`, that destroys the + * keyring along with removing all its event listeners and, in some cases, + * clears the keyring bridge iframe from the DOM. + * + * @param keyring - The keyring to destroy. + */ + async #destroyKeyring(keyring: EthKeyring) { + await keyring.destroy?.(); + } + + /** + * Remove empty keyrings. + * + * Loops through the keyrings and removes the ones with empty accounts + * (usually after removing the last / only account) from a keyring. + */ + async #removeEmptyKeyrings(): Promise { + const validKeyrings: EthKeyring[] = []; + + // Since getAccounts returns a Promise + // We need to wait to hear back form each keyring + // in order to decide which ones are now valid (accounts.length > 0) + + await Promise.all( + this.#keyrings.map(async (keyring: EthKeyring) => { + const accounts = await keyring.getAccounts(); + if (accounts.length > 0) { + validKeyrings.push(keyring); + } else { + await this.#destroyKeyring(keyring); + } + }), + ); + this.#keyrings = validKeyrings; + } + + /** + * Checks for duplicate keypairs, using the the first account in the given + * array. Rejects if a duplicate is found. + * + * Only supports 'Simple Key Pair'. + * + * @param type - The key pair type to check for. + * @param newAccountArray - Array of new accounts. + * @returns The account, if no duplicate is found. + */ + async #checkForDuplicate( + type: string, + newAccountArray: string[], + ): Promise { + const accounts = await this.getAccounts(); + + switch (type) { + case KeyringType.Simple: { + const isIncluded = Boolean( + accounts.find( + (key) => + newAccountArray[0] && + (key === newAccountArray[0] || + key === remove0x(newAccountArray[0])), + ), + ); + + if (isIncluded) { + throw new Error(KeyringControllerError.DuplicatedAccount); + } + return newAccountArray; + } + + default: { + return newAccountArray; + } + } + } + + /** + * Unlock Keyrings + * + * Unlocks the keyrings. * * @fires KeyringController:unlock */ - #handleUnlock() { + #setUnlocked(): void { + this.update((state) => { + state.isUnlocked = true; + }); this.messagingSystem.publish(`${name}:unlock`); } diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts new file mode 100644 index 0000000000..71992514f5 --- /dev/null +++ b/packages/keyring-controller/src/constants.ts @@ -0,0 +1,34 @@ +export enum KeyringType { + HD = 'HD Key Tree', + Simple = 'Simple Key Pair', +} + +export enum KeyringControllerError { + NoKeyring = 'KeyringController - No keyring found', + WrongPasswordType = 'KeyringController - Password must be of type string.', + NoFirstAccount = 'KeyringController - First Account not found.', + DuplicatedAccount = 'KeyringController - The account you are trying to import is a duplicate', + VaultError = 'KeyringController - Cannot unlock without a previous vault.', + VaultDataError = 'KeyringController - The decrypted vault has an unexpected shape.', + UnsupportedEncryptionKeyExport = 'KeyringController - The encryptor does not support encryption key export.', + UnsupportedGenerateRandomMnemonic = 'KeyringController - The current keyring does not support the method generateRandomMnemonic.', + UnsupportedExportAccount = '`KeyringController - The keyring for the current address does not support the method exportAccount', + UnsupportedRemoveAccount = '`KeyringController - The keyring for the current address does not support the method removeAccount', + UnsupportedSignTransaction = 'KeyringController - The keyring for the current address does not support the method signTransaction.', + UnsupportedSignMessage = 'KeyringController - The keyring for the current address does not support the method signMessage.', + UnsupportedSignPersonalMessage = 'KeyringController - The keyring for the current address does not support the method signPersonalMessage.', + UnsupportedGetEncryptionPublicKey = 'KeyringController - The keyring for the current address does not support the method getEncryptionPublicKey.', + UnsupportedDecryptMessage = 'KeyringController - The keyring for the current address does not support the method decryptMessage.', + UnsupportedSignTypedMessage = 'KeyringController - The keyring for the current address does not support the method signTypedMessage.', + UnsupportedGetAppKeyAddress = 'KeyringController - The keyring for the current address does not support the method getAppKeyAddress.', + UnsupportedExportAppKeyForAddress = 'KeyringController - The keyring for the current address does not support the method exportAppKeyForAddress.', + 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.', + 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', + ExpiredCredentials = 'KeyringController - Encryption key and salt provided are expired', + NoKeyringBuilder = 'KeyringController - No keyringBuilder found for keyring', + DataType = 'KeyringController - Incorrect data type provided', +} diff --git a/packages/keyring-controller/src/utils.ts b/packages/keyring-controller/src/utils.ts new file mode 100644 index 0000000000..ea1e41287a --- /dev/null +++ b/packages/keyring-controller/src/utils.ts @@ -0,0 +1,8 @@ +/** + * Throws an error. + * + * @param error - Error message or Error object to throw. + */ +export function throwError(error: string | Error): never { + throw typeof error === 'string' ? new Error(error) : error; +} diff --git a/types/@metamask/eth-hd-keyring.d.ts b/types/@metamask/eth-hd-keyring.d.ts new file mode 100644 index 0000000000..957e066392 --- /dev/null +++ b/types/@metamask/eth-hd-keyring.d.ts @@ -0,0 +1,2 @@ +// eslint-disable-next-line import/unambiguous +declare module '@metamask/eth-hd-keyring'; diff --git a/types/@metamask/eth-simple-keyring.d.ts b/types/@metamask/eth-simple-keyring.d.ts new file mode 100644 index 0000000000..a44ddd1ecf --- /dev/null +++ b/types/@metamask/eth-simple-keyring.d.ts @@ -0,0 +1,2 @@ +// eslint-disable-next-line import/unambiguous +declare module '@metamask/eth-simple-keyring'; diff --git a/yarn.lock b/yarn.lock index d8115ffc6b..8701b1097b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2243,8 +2243,11 @@ __metadata: "@lavamoat/allow-scripts": ^2.3.1 "@metamask/auto-changelog": ^3.4.4 "@metamask/base-controller": ^4.1.1 + "@metamask/browser-passworder": ^4.3.0 + "@metamask/eth-hd-keyring": ^7.0.1 "@metamask/eth-keyring-controller": ^17.0.1 "@metamask/eth-sig-util": ^7.0.1 + "@metamask/eth-simple-keyring": ^6.0.1 "@metamask/keyring-api": ^3.0.0 "@metamask/message-manager": ^7.3.8 "@metamask/scure-bip39": ^2.1.1 From dd08f83d45b2727baeb72d9c09b1217c1cdc306c Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 13:35:03 +0100 Subject: [PATCH 02/15] refactor: rename updateMemStoreKeyrings --- .../src/KeyringController.ts | 74 +++++++++---------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 00036bd6e2..94dc26b9a6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -585,38 +585,6 @@ export class KeyringController extends BaseController< return this.#getMemState(); } - /** - * Create new vault And with a specific keyring - * - * Destroys any old encrypted storage, - * creates a new encrypted store with the given password, - * creates a new wallet with 1 account. - * - * @fires KeyringController#unlock - * @param password - The password to encrypt the vault with. - * @param keyring - A object containing the params to instantiate a new keyring. - * @param keyring.type - The keyring type. - * @param keyring.opts - Optional parameters required to instantiate the keyring. - * @returns A promise that resolves to the state. - */ - async createNewVaultWithKeyring( - password: string, - keyring: { - type: string; - opts?: unknown; - }, - ): Promise { - if (typeof password !== 'string') { - throw new TypeError(KeyringControllerError.WrongPasswordType); - } - this.#password = password; - - await this.#clearKeyrings(); - await this.#createKeyring(keyring.type, keyring.opts); - this.#setUnlocked(); - return this.#getMemState(); - } - /** * Effectively the same as creating a new keychain then populating it * using the given seed phrase. @@ -958,9 +926,9 @@ export class KeyringController extends BaseController< // The keyring updates need to be announced before updating the encryptionKey // so that the updated keyring gets propagated to the extension first. - // Not calling {@link updateMemStoreKeyrings} results in the wrong account being selected + // Not calling {@link updateKeyringsInState} results in the wrong account being selected // in the extension. - await this.#updateMemStoreKeyrings(); + await this.#updateKeyringsInState(); if (newEncryptionKey) { this.update((state) => { state.encryptionKey = newEncryptionKey; @@ -1653,11 +1621,41 @@ export class KeyringController extends BaseController< } /** - * Update memStore Keyrings + * Create new vault And with a specific keyring + * + * Destroys any old encrypted storage, + * creates a new encrypted store with the given password, + * creates a new wallet with 1 account. * - * Updates the in-memory keyrings, without persisting. + * @fires KeyringController#unlock + * @param password - The password to encrypt the vault with. + * @param keyring - A object containing the params to instantiate a new keyring. + * @param keyring.type - The keyring type. + * @param keyring.opts - Optional parameters required to instantiate the keyring. + * @returns A promise that resolves to the state. + */ + async createNewVaultWithKeyring( + password: string, + keyring: { + type: string; + opts?: unknown; + }, + ): Promise { + if (typeof password !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + this.#password = password; + + await this.#clearKeyrings(); + await this.#createKeyring(keyring.type, keyring.opts); + this.#setUnlocked(); + return this.#getMemState(); + } + + /** + * Update the controller state with its current keyrings. */ - async #updateMemStoreKeyrings(): Promise { + async #updateKeyringsInState(): Promise { const keyrings = await Promise.all(this.#keyrings.map(displayForKeyring)); this.update((state) => { state.keyrings = keyrings; @@ -1742,7 +1740,7 @@ export class KeyringController extends BaseController< } await Promise.all(vault.map(this.#restoreKeyring.bind(this))); - await this.#updateMemStoreKeyrings(); + await this.#updateKeyringsInState(); if ( this.#password && From 0950f20243ca4729805476f77c220c4a3374ed0d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 13:38:10 +0100 Subject: [PATCH 03/15] chore: use eth-sig-util ^7.0.1 --- packages/keyring-controller/package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index c9f9949802..1bcb221e7b 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -36,7 +36,7 @@ "@metamask/browser-passworder": "^4.3.0", "@metamask/eth-hd-keyring": "^7.0.1", "@metamask/eth-keyring-controller": "^17.0.1", - "@metamask/eth-sig-util": "^7.0.0", + "@metamask/eth-sig-util": "^7.0.1", "@metamask/eth-simple-keyring": "^6.0.1", "@metamask/keyring-api": "^3.0.0", "@metamask/message-manager": "^7.3.8", @@ -52,7 +52,6 @@ "@keystonehq/bc-ur-registry-eth": "^0.9.0", "@lavamoat/allow-scripts": "^2.3.1", "@metamask/auto-changelog": "^3.4.4", - "@metamask/eth-sig-util": "^7.0.1", "@metamask/scure-bip39": "^2.1.1", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", From f5e4632c3490c972faadf3d5e5ef55fd6fd312b0 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 13:45:29 +0100 Subject: [PATCH 04/15] refactor: make createNewVaultWithKeyring private --- packages/keyring-controller/jest.config.js | 2 +- packages/keyring-controller/src/KeyringController.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 708bbc752a..a7a7dca158 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -18,7 +18,7 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 74.14, - functions: 98.95, + functions: 98.94, lines: 93.84, statements: 93.88, }, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 94dc26b9a6..4bb8d26dfc 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -605,7 +605,7 @@ export class KeyringController extends BaseController< try { this.updateIdentities([]); - await this.createNewVaultWithKeyring(password, { + await this.#createNewVaultWithKeyring(password, { type: KeyringType.HD, opts: { mnemonic: seed, @@ -630,7 +630,7 @@ export class KeyringController extends BaseController< try { const accounts = await this.getAccounts(); if (!accounts.length) { - await this.createNewVaultWithKeyring(password, { + await this.#createNewVaultWithKeyring(password, { type: KeyringType.HD, }); this.updateIdentities(await this.getAccounts()); @@ -1627,14 +1627,14 @@ export class KeyringController extends BaseController< * creates a new encrypted store with the given password, * creates a new wallet with 1 account. * - * @fires KeyringController#unlock + * @fires KeyringController:unlock * @param password - The password to encrypt the vault with. * @param keyring - A object containing the params to instantiate a new keyring. * @param keyring.type - The keyring type. * @param keyring.opts - Optional parameters required to instantiate the keyring. * @returns A promise that resolves to the state. */ - async createNewVaultWithKeyring( + async #createNewVaultWithKeyring( password: string, keyring: { type: string; From 57116b9f06553fc31d91cad3b27f0b89b895633f Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 16:50:14 +0100 Subject: [PATCH 05/15] refactor: drop eth-keyring-controller dependency --- packages/keyring-controller/package.json | 1 - .../src/KeyringController.test.ts | 6 +- .../src/KeyringController.ts | 109 ++++++++++++++++-- packages/keyring-controller/src/constants.ts | 5 - .../tests/mocks/mockEncryptor.ts | 3 +- yarn.lock | 27 ----- 6 files changed, 104 insertions(+), 47 deletions(-) diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 1bcb221e7b..eb1315ceb2 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -35,7 +35,6 @@ "@metamask/base-controller": "^4.1.1", "@metamask/browser-passworder": "^4.3.0", "@metamask/eth-hd-keyring": "^7.0.1", - "@metamask/eth-keyring-controller": "^17.0.1", "@metamask/eth-sig-util": "^7.0.1", "@metamask/eth-simple-keyring": "^6.0.1", "@metamask/keyring-api": "^3.0.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 6c52da206a..8480bb8d42 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3,10 +3,6 @@ import { TransactionFactory } from '@ethereumjs/tx'; import { CryptoHDKey, ETHSignature } from '@keystonehq/bc-ur-registry-eth'; import { MetaMaskKeyring as QRKeyring } from '@keystonehq/metamask-airgapped-keyring'; import { ControllerMessenger } from '@metamask/base-controller'; -import { - KeyringControllerError, - keyringBuilderFactory, -} from '@metamask/eth-keyring-controller'; import { normalize, recoverPersonalSignature, @@ -34,6 +30,7 @@ import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; import { MockKeyring } from '../tests/mocks/mockKeyring'; import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring'; import { buildMockTransaction } from '../tests/mocks/mockTransaction'; +import { KeyringControllerError } from './constants'; import type { KeyringControllerEvents, KeyringControllerMessenger, @@ -45,6 +42,7 @@ import { AccountImportStrategy, KeyringController, KeyringTypes, + keyringBuilderFactory, } from './KeyringController'; jest.mock('uuid', () => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 4bb8d26dfc..3468eb9b3f 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -7,11 +7,6 @@ import type { RestrictedControllerMessenger } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; import * as encryptorUtils from '@metamask/browser-passworder'; import HDKeyring from '@metamask/eth-hd-keyring'; -import { KeyringType } from '@metamask/eth-keyring-controller'; -import type { - ExportableKeyEncryptor, - GenericEncryptor, -} from '@metamask/eth-keyring-controller/dist/types'; import { normalize } from '@metamask/eth-sig-util'; import SimpleKeyring from '@metamask/eth-simple-keyring'; import type { @@ -268,6 +263,102 @@ export type SerializedKeyring = { data: Json; }; +/** + * A generic encryptor interface that supports encrypting and decrypting + * serializable data with a password. + */ +export type GenericEncryptor = { + /** + * Encrypts the given object with the given password. + * + * @param password - The password to encrypt with. + * @param object - The object to encrypt. + * @returns The encrypted string. + */ + encrypt: (password: string, object: Json) => Promise; + /** + * Decrypts the given encrypted string with the given password. + * + * @param password - The password to decrypt with. + * @param encryptedString - The encrypted string to decrypt. + * @returns The decrypted object. + */ + decrypt: (password: string, encryptedString: string) => Promise; + /** + * Optional vault migration helper. Checks if the provided vault is up to date + * with the desired encryption algorithm. + * + * @param vault - The encrypted string to check. + * @param targetDerivationParams - The desired target derivation params. + * @returns The updated encrypted string. + */ + isVaultUpdated?: ( + vault: string, + targetDerivationParams?: encryptorUtils.KeyDerivationOptions, + ) => boolean; +}; + +/** + * An encryptor interface that supports encrypting and decrypting + * serializable data with a password, and exporting and importing keys. + */ +export type ExportableKeyEncryptor = GenericEncryptor & { + /** + * Encrypts the given object with the given encryption key. + * + * @param key - The encryption key to encrypt with. + * @param object - The object to encrypt. + * @returns The encryption result. + */ + encryptWithKey: ( + key: unknown, + object: Json, + ) => Promise; + /** + * Encrypts the given object with the given password, and returns the + * encryption result and the exported key string. + * + * @param password - The password to encrypt with. + * @param object - The object to encrypt. + * @param salt - The optional salt to use for encryption. + * @returns The encrypted string and the exported key string. + */ + encryptWithDetail: ( + password: string, + object: Json, + salt?: string, + ) => Promise; + /** + * Decrypts the given encrypted string with the given encryption key. + * + * @param key - The encryption key to decrypt with. + * @param encryptedString - The encrypted string to decrypt. + * @returns The decrypted object. + */ + decryptWithKey: (key: unknown, encryptedString: string) => Promise; + /** + * Decrypts the given encrypted string with the given password, and returns + * the decrypted object and the salt and exported key string used for + * encryption. + * + * @param password - The password to decrypt with. + * @param encryptedString - The encrypted string to decrypt. + * @returns The decrypted object and the salt and exported key string used for + * encryption. + */ + decryptWithDetail: ( + password: string, + encryptedString: string, + ) => Promise; + /** + * Generates an encryption key from exported key string. + * + * @param key - The exported key string. + * @returns The encryption key. + */ + importKey: (key: string) => Promise; +}; + /** * Get builder function for `Keyring` * @@ -606,7 +697,7 @@ export class KeyringController extends BaseController< try { this.updateIdentities([]); await this.#createNewVaultWithKeyring(password, { - type: KeyringType.HD, + type: KeyringTypes.hd, opts: { mnemonic: seed, numberOfAccounts: 1, @@ -631,7 +722,7 @@ export class KeyringController extends BaseController< const accounts = await this.getAccounts(); if (!accounts.length) { await this.#createNewVaultWithKeyring(password, { - type: KeyringType.HD, + type: KeyringTypes.hd, }); this.updateIdentities(await this.getAccounts()); } @@ -663,7 +754,7 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.NoKeyring); } - if (type === KeyringType.HD && (!isObject(opts) || !opts.mnemonic)) { + if (type === KeyringTypes.hd && (!isObject(opts) || !opts.mnemonic)) { if (!keyring.generateRandomMnemonic) { throw new Error( KeyringControllerError.UnsupportedGenerateRandomMnemonic, @@ -1918,7 +2009,7 @@ export class KeyringController extends BaseController< const accounts = await this.getAccounts(); switch (type) { - case KeyringType.Simple: { + case KeyringTypes.simple: { const isIncluded = Boolean( accounts.find( (key) => diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index 71992514f5..f9f06dd0f9 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -1,8 +1,3 @@ -export enum KeyringType { - HD = 'HD Key Tree', - Simple = 'Simple Key Pair', -} - export enum KeyringControllerError { NoKeyring = 'KeyringController - No keyring found', WrongPasswordType = 'KeyringController - Password must be of type string.', diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 9eaa82b305..39e74c6f28 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -1,6 +1,7 @@ -import type { ExportableKeyEncryptor } from '@metamask/eth-keyring-controller/dist/types'; import type { Json } from '@metamask/utils'; +import type { ExportableKeyEncryptor } from '../../src/KeyringController'; + export const PASSWORD = 'password123'; export const MOCK_ENCRYPTION_KEY = JSON.stringify({ alg: 'A256GCM', diff --git a/yarn.lock b/yarn.lock index 8701b1097b..e3612c3c94 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1942,22 +1942,6 @@ __metadata: languageName: unknown linkType: soft -"@metamask/eth-keyring-controller@npm:^17.0.1": - version: 17.0.1 - resolution: "@metamask/eth-keyring-controller@npm:17.0.1" - dependencies: - "@ethereumjs/tx": ^4.2.0 - "@metamask/browser-passworder": ^4.3.0 - "@metamask/eth-hd-keyring": ^7.0.1 - "@metamask/eth-sig-util": ^7.0.0 - "@metamask/eth-simple-keyring": ^6.0.1 - "@metamask/keyring-api": ^3.0.0 - "@metamask/obs-store": ^9.0.0 - "@metamask/utils": ^8.2.0 - checksum: ddaeeb15510fd1896bbe94ca3c47c5867730a1370ec19b7ba4206b1048e303a846c67592a64efbfed7f19d82eaa782f84e802d0d66e0d4764420684199e47c33 - languageName: node - linkType: hard - "@metamask/eth-query@npm:^4.0.0": version: 4.0.0 resolution: "@metamask/eth-query@npm:4.0.0" @@ -2245,7 +2229,6 @@ __metadata: "@metamask/base-controller": ^4.1.1 "@metamask/browser-passworder": ^4.3.0 "@metamask/eth-hd-keyring": ^7.0.1 - "@metamask/eth-keyring-controller": ^17.0.1 "@metamask/eth-sig-util": ^7.0.1 "@metamask/eth-simple-keyring": ^6.0.1 "@metamask/keyring-api": ^3.0.0 @@ -2409,16 +2392,6 @@ __metadata: languageName: node linkType: hard -"@metamask/obs-store@npm:^9.0.0": - version: 9.0.0 - resolution: "@metamask/obs-store@npm:9.0.0" - dependencies: - "@metamask/safe-event-emitter": ^3.0.0 - readable-stream: ^3.6.2 - checksum: 1c202a5bbdc79a6b8b3fba946c09dc5521e87260956d30db6543e7bf3d95bd44ebd958f509e3e7332041845176487fe78d3b40bdedbc213061ba849fd978e468 - languageName: node - linkType: hard - "@metamask/permission-controller@npm:^7.0.0, @metamask/permission-controller@npm:^7.1.0": version: 7.1.0 resolution: "@metamask/permission-controller@npm:7.1.0" From a375ad5ad097cd89602cb85b32df6ba93eb676e0 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 17:39:17 +0100 Subject: [PATCH 06/15] fix: port isSerializedKeyringsArray function --- packages/keyring-controller/src/KeyringController.ts | 3 ++- packages/keyring-controller/tests/mocks/mockEncryptor.ts | 8 +++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 3468eb9b3f..8d41343a46 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -31,6 +31,7 @@ import { hasProperty, isObject, isValidHexAddress, + isValidJson, remove0x, } from '@metamask/utils'; import { Mutex } from 'async-mutex'; @@ -442,7 +443,7 @@ function isSerializedKeyringsArray( return ( typeof array === 'object' && Array.isArray(array) && - array.every((value) => value.type && value.data) + array.every((value) => value.type && isValidJson(value.data)) ); } diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 39e74c6f28..30a40b97ab 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -1,5 +1,3 @@ -import type { Json } from '@metamask/utils'; - import type { ExportableKeyEncryptor } from '../../src/KeyringController'; export const PASSWORD = 'password123'; @@ -19,7 +17,7 @@ export const MOCK_HEX = '0xabcdef0123456789'; const MOCK_KEY = Buffer.alloc(32); const INVALID_PASSWORD_ERROR = 'Incorrect password.'; -let cacheVal: Json; +let cacheVal: string; export default class MockEncryptor implements ExportableKeyEncryptor { // TODO: Replace `any` with type @@ -36,13 +34,13 @@ export default class MockEncryptor implements ExportableKeyEncryptor { throw new Error(INVALID_PASSWORD_ERROR); } - return cacheVal ?? {}; + return JSON.parse(cacheVal) ?? {}; } // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any async encryptWithKey(_key: unknown, dataObj: any) { - cacheVal = dataObj; + cacheVal = JSON.stringify(dataObj); return { data: MOCK_HEX, iv: 'anIv', From d38c7f8738191bb6152738fae92f8252cfd67206 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jan 2024 21:26:14 +0100 Subject: [PATCH 07/15] test: adjust coverage --- packages/keyring-controller/jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index a7a7dca158..fbdf23cadf 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: 74.14, - functions: 98.94, - lines: 93.84, - statements: 93.88, + branches: 80, + functions: 98.93, + lines: 95.55, + statements: 95.58, }, }, From 0856709f9167bfc47f7f422fd70752a2f60e80b2 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jan 2024 15:03:13 +0100 Subject: [PATCH 08/15] refactor: #restoreKeyring function --- packages/keyring-controller/jest.config.js | 6 ++-- .../src/KeyringController.ts | 28 ++++++------------- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index fbdf23cadf..e7f1972755 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: 80, + branches: 80.55, functions: 98.93, - lines: 95.55, - statements: 95.58, + lines: 96.11, + statements: 96.14, }, }, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8d41343a46..39e26261b6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1920,38 +1920,28 @@ export class KeyringController extends BaseController< } /** - * Restore Keyring Helper - * - * Attempts to initialize a new keyring from the provided serialized payload. + * Restore a Keyring from a provided serialized payload. * On success, returns the resulting keyring instance. * * @param serialized - The serialized keyring. - * @param serialized.type - Keyring type. - * @param serialized.data - Keyring data. * @returns The deserialized keyring or undefined if the keyring type is unsupported. */ async #restoreKeyring( serialized: SerializedKeyring, ): Promise | undefined> { - const { type, data } = serialized; - - let keyring: EthKeyring | undefined; try { - keyring = await this.#newKeyring(type, data); - } catch (error) { - // Ignore error. - console.error(error); - } + const { type, data } = serialized; + const keyring = await this.#newKeyring(type, data); - if (!keyring) { + // getAccounts also validates the accounts for some keyrings + await keyring.getAccounts(); + this.#keyrings.push(keyring); + + return keyring; + } catch (_) { this.#unsupportedKeyrings.push(serialized); return undefined; } - - // getAccounts also validates the accounts for some keyrings - await keyring.getAccounts(); - this.#keyrings.push(keyring); - return keyring; } /** From a4b7eab3e954daa5f3cd3bb082c995b25a18ecbf Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jan 2024 16:57:28 +0100 Subject: [PATCH 09/15] refactor: remove unreachable branches --- .../keyring-controller/src/KeyringController.ts | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 39e26261b6..dce0f6d254 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -751,10 +751,6 @@ export class KeyringController extends BaseController< const keyring = await this.#newKeyring(type, opts); - if (!keyring) { - throw new Error(KeyringControllerError.NoKeyring); - } - if (type === KeyringTypes.hd && (!isObject(opts) || !opts.mnemonic)) { if (!keyring.generateRandomMnemonic) { throw new Error( @@ -1671,10 +1667,6 @@ export class KeyringController extends BaseController< accounts: [], })) as unknown as QRKeyring; - if (!qrKeyring) { - throw new Error(KeyringControllerError.NoKeyring); - } - const accounts = await qrKeyring.getAccounts(); await this.#checkForDuplicate(KeyringTypes.qr, accounts); @@ -1858,12 +1850,7 @@ export class KeyringController extends BaseController< * @returns A promise that resolves if the operation was successful. */ async #createKeyring(type: string, opts?: unknown) { - const keyring = (await this.addNewKeyring(type, opts)) as - | EthKeyring - | undefined; - if (!keyring) { - throw new Error(KeyringControllerError.NoKeyring); - } + const keyring = (await this.addNewKeyring(type, opts)) as EthKeyring; const [firstAccount] = await keyring.getAccounts(); if (!firstAccount) { From 29e7e47e686733711e5910d0ea34a6483f034b5c Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 26 Jan 2024 11:17:02 +0100 Subject: [PATCH 10/15] test: add hdkeyring test case --- packages/keyring-controller/jest.config.js | 8 ++++---- .../src/KeyringController.test.ts | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index e7f1972755..10db8a1983 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: 80.55, - functions: 98.93, - lines: 96.11, - statements: 96.14, + branches: 95.17, + functions: 100, + lines: 99.22, + statements: 99.22, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 8480bb8d42..9c5118b37e 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3,6 +3,7 @@ import { TransactionFactory } from '@ethereumjs/tx'; import { CryptoHDKey, ETHSignature } from '@keystonehq/bc-ur-registry-eth'; import { MetaMaskKeyring as QRKeyring } from '@keystonehq/metamask-airgapped-keyring'; import { ControllerMessenger } from '@metamask/base-controller'; +import HDKeyring from '@metamask/eth-hd-keyring'; import { normalize, recoverPersonalSignature, @@ -525,6 +526,20 @@ describe('KeyringController', () => { }, ); }); + + it('should throw error if the first account is not found on the keyring', async () => { + jest + .spyOn(HDKeyring.prototype, 'getAccounts') + .mockResolvedValue([]); + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.createNewVaultAndKeychain(password), + ).rejects.toThrow(KeyringControllerError.NoFirstAccount); + }, + ); + }); }); describe('when there is an existing vault', () => { From ee61a046761abf0841671cba1a51f0ee18cbf6ad Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 26 Jan 2024 11:42:03 +0100 Subject: [PATCH 11/15] refactor: remove `addNewAccount` code repetition --- packages/keyring-controller/jest.config.js | 2 +- packages/keyring-controller/src/KeyringController.ts | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 10db8a1983..57b73715b5 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -19,7 +19,7 @@ module.exports = merge(baseConfig, { global: { branches: 95.17, functions: 100, - lines: 99.22, + lines: 99.21, statements: 99.22, }, }, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index dce0f6d254..e2fc5b7d6e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -605,17 +605,13 @@ export class KeyringController extends BaseController< }; } - await this.addNewAccountForKeyring(primaryKeyring); - const newAccounts = await this.getAccounts(); - + const addedAccountAddress = await this.addNewAccountForKeyring( + primaryKeyring, + ); await this.verifySeedPhrase(); - this.updateIdentities(newAccounts); - const addedAccountAddress = newAccounts.find( - (selectedAddress: string) => !oldAccounts.includes(selectedAddress), - ); + this.updateIdentities(await this.getAccounts()); - assertIsStrictHexString(addedAccountAddress); return { keyringState: this.#getMemState(), addedAccountAddress, From 2139fbe8bc45296d46f7f165c7071d11f3b0c7e1 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 26 Jan 2024 13:28:44 +0100 Subject: [PATCH 12/15] docs: update jsdoc --- .../src/KeyringController.ts | 55 ++++++++----------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index e2fc5b7d6e..44beffdf27 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -259,6 +259,9 @@ export enum SignTypedDataVersion { V4 = 'V4', } +/** + * A serialized keyring object. + */ export type SerializedKeyring = { type: string; data: Json; @@ -1632,15 +1635,10 @@ export class KeyringController extends BaseController< } /** - * Get Keyring Class For Type + * Get the keyring builder for the given `type`. * - * Searches the current `keyringBuilders` array - * for a Keyring builder whose unique `type` property - * matches the provided `type`, - * returning it if it exists. - * - * @param type - The type whose class to get. - * @returns The class, if it exists. + * @param type - The type of keyring to get the builder for. + * @returns The keyring builder, or undefined if none exists. */ #getKeyringBuilderForType( type: string, @@ -1701,7 +1699,7 @@ export class KeyringController extends BaseController< } /** - * Create new vault And with a specific keyring + * Create new vault with an initial keyring * * Destroys any old encrypted storage, * creates a new encrypted store with the given password, @@ -1727,7 +1725,7 @@ export class KeyringController extends BaseController< this.#password = password; await this.#clearKeyrings(); - await this.#createKeyring(keyring.type, keyring.opts); + await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); this.#setUnlocked(); return this.#getMemState(); } @@ -1743,15 +1741,13 @@ export class KeyringController extends BaseController< } /** - * Unlock Keyrings. - * - * Attempts to unlock the persisted encrypted storage, - * initializing the persisted keyrings to RAM. + * Unlock Keyrings, decrypting the vault and deserializing all + * keyrings contained in it, using a password or an encryption key with salt. * * @param password - The keyring controller password. * @param encryptionKey - An exported key string to unlock keyrings with. * @param encryptionSalt - The salt used to encrypt the vault. - * @returns The keyrings array. + * @returns A promise resolving to the deserialized keyrings array. */ async #unlockKeyrings( password: string | undefined, @@ -1836,16 +1832,14 @@ export class KeyringController extends BaseController< } /** - * Create keyring. + * Create a new keyring, ensuring that the first account is + * also created. * - * - Creates a new vault. - * - Creates a new keyring with at least one account. - * - Makes the first account the selected account. * @param type - Keyring type to instantiate. * @param opts - Optional parameters required to instantiate the keyring. - * @returns A promise that resolves if the operation was successful. + * @returns A promise that resolves if the operation is successful. */ - async #createKeyring(type: string, opts?: unknown) { + async #createKeyringWithFirstAccount(type: string, opts?: unknown) { const keyring = (await this.addNewKeyring(type, opts)) as EthKeyring; const [firstAccount] = await keyring.getAccounts(); @@ -1855,9 +1849,9 @@ export class KeyringController extends BaseController< } /** - * Instantiate, initialize and return a new keyring - * - * The keyring instantiated is of the given `type`. + * Instantiate, initialize and return a new keyring of the given `type`, + * 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. @@ -1885,14 +1879,10 @@ export class KeyringController extends BaseController< } /** - * Clear Keyrings - * - * Deallocates all currently managed keyrings and accounts. - * Used before initializing a new vault and after locking - * MetaMask. + * Remove all managed keyrings, destroying all their + * instances in memory. */ async #clearKeyrings() { - // clear keyrings from memory for (const keyring of this.#keyrings) { await this.#destroyKeyring(keyring); } @@ -2006,9 +1996,8 @@ export class KeyringController extends BaseController< } /** - * Unlock Keyrings - * - * Unlocks the keyrings. + * Set the `isUnlocked` to true and notify listeners + * through the messenger. * * @fires KeyringController:unlock */ From 8022c167ccd25d46b69c69d7d697afaf22339f8a Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 26 Jan 2024 14:31:55 +0100 Subject: [PATCH 13/15] refactor: remove useless eslint-disable --- types/@metamask/eth-hd-keyring.d.ts | 1 - types/@metamask/eth-simple-keyring.d.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/types/@metamask/eth-hd-keyring.d.ts b/types/@metamask/eth-hd-keyring.d.ts index 957e066392..650803e985 100644 --- a/types/@metamask/eth-hd-keyring.d.ts +++ b/types/@metamask/eth-hd-keyring.d.ts @@ -1,2 +1 @@ -// eslint-disable-next-line import/unambiguous declare module '@metamask/eth-hd-keyring'; diff --git a/types/@metamask/eth-simple-keyring.d.ts b/types/@metamask/eth-simple-keyring.d.ts index a44ddd1ecf..3778872a78 100644 --- a/types/@metamask/eth-simple-keyring.d.ts +++ b/types/@metamask/eth-simple-keyring.d.ts @@ -1,2 +1 @@ -// eslint-disable-next-line import/unambiguous declare module '@metamask/eth-simple-keyring'; From 03dde4418eb6d189256b51d1bd69d5e3d4656377 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 29 Jan 2024 12:07:40 +0100 Subject: [PATCH 14/15] docs: fix jsdoc --- packages/keyring-controller/src/KeyringController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 44beffdf27..53ea86171a 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -465,8 +465,8 @@ async function displayForKeyring( return { type: keyring.type, - // Cast to `Hex[]` here is safe here because `addresses` has no nullish - // values, and `normalizeToHex` returns `Hex` unless given a nullish value + // Cast to `Hex[]` here is safe here because `accounts` has no nullish + // values, and `normalize` returns `Hex` unless given a nullish value accounts: accounts.map(normalize) as Hex[], }; } From dd28464836e0ffa3c967f129a668fa3b34a58b0f Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 29 Jan 2024 14:20:28 +0100 Subject: [PATCH 15/15] refactor: remove `throwError` util --- packages/keyring-controller/jest.config.js | 2 +- .../src/KeyringController.ts | 25 +++++++++++-------- packages/keyring-controller/src/utils.ts | 8 ------ 3 files changed, 16 insertions(+), 19 deletions(-) delete mode 100644 packages/keyring-controller/src/utils.ts diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 57b73715b5..14946730e8 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,7 +17,7 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 95.17, + branches: 95.71, functions: 100, lines: 99.21, statements: 99.22, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 53ea86171a..2ead832949 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -47,7 +47,6 @@ import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; import { KeyringControllerError } from './constants'; -import { throwError } from './utils'; const name = 'KeyringController'; @@ -1277,9 +1276,11 @@ export class KeyringController extends BaseController< address, )) as EthKeyring; - return keyring.prepareUserOperation - ? await keyring.prepareUserOperation(address, transactions) - : throwError(KeyringControllerError.UnsupportedPrepareUserOperation); + if (!keyring.prepareUserOperation) { + throw new Error(KeyringControllerError.UnsupportedPrepareUserOperation); + } + + return await keyring.prepareUserOperation(address, transactions); } /** @@ -1299,9 +1300,11 @@ export class KeyringController extends BaseController< address, )) as EthKeyring; - return keyring.patchUserOperation - ? await keyring.patchUserOperation(address, userOp) - : throwError(KeyringControllerError.UnsupportedPatchUserOperation); + if (!keyring.patchUserOperation) { + throw new Error(KeyringControllerError.UnsupportedPatchUserOperation); + } + + return await keyring.patchUserOperation(address, userOp); } /** @@ -1320,9 +1323,11 @@ export class KeyringController extends BaseController< address, )) as EthKeyring; - return keyring.signUserOperation - ? await keyring.signUserOperation(address, userOp) - : throwError(KeyringControllerError.UnsupportedSignUserOperation); + if (!keyring.signUserOperation) { + throw new Error(KeyringControllerError.UnsupportedSignUserOperation); + } + + return await keyring.signUserOperation(address, userOp); } /** diff --git a/packages/keyring-controller/src/utils.ts b/packages/keyring-controller/src/utils.ts deleted file mode 100644 index ea1e41287a..0000000000 --- a/packages/keyring-controller/src/utils.ts +++ /dev/null @@ -1,8 +0,0 @@ -/** - * Throws an error. - * - * @param error - Error message or Error object to throw. - */ -export function throwError(error: string | Error): never { - throw typeof error === 'string' ? new Error(error) : error; -}