From e0f18781d1dd9b060b0e4b5c45e2b51dd6269f97 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 8 May 2023 16:36:08 +0200 Subject: [PATCH 01/18] feat: supp cacheEncryptionKey and submitEncryptionKey method --- packages/keyring-controller/src/KeyringController.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8173b42e4a..fa31e61bc8 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -858,6 +858,13 @@ export class KeyringController extends BaseController< await this.#keyring.persistAllKeyrings(); await this.fullUpdate(); } + + async submitEncryptionKey( + encryptionKey: string, + encryptionSalt: string, + ): Promise { + return this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); + } } export default KeyringController; From 5f9e16a56aedffad6286403c8c7953f731735f67 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 8 May 2023 16:43:27 +0200 Subject: [PATCH 02/18] docs: update jsdoc --- packages/keyring-controller/src/KeyringController.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index fa31e61bc8..8173b42e4a 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -858,13 +858,6 @@ export class KeyringController extends BaseController< await this.#keyring.persistAllKeyrings(); await this.fullUpdate(); } - - async submitEncryptionKey( - encryptionKey: string, - encryptionSalt: string, - ): Promise { - return this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); - } } export default KeyringController; From 0b1cba76f429ef5db05d09457db722beef8dd380 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 16 May 2023 15:59:18 +0200 Subject: [PATCH 03/18] refactor: migrate KeyringController to BaseControllerV2 --- packages/keyring-controller/package.json | 1 + .../src/KeyringController.test.ts | 69 +++++-- .../src/KeyringController.ts | 180 ++++++++++-------- yarn.lock | 8 + 4 files changed, 155 insertions(+), 103 deletions(-) diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 4a1e7ac3e7..e9a9fb865d 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -48,6 +48,7 @@ "@metamask/scure-bip39": "^2.1.0", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", + "immer": "^10.0.2", "jest": "^27.5.1", "sinon": "^9.2.4", "ts-jest": "^27.1.4", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 552642cf42..064efa17a2 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -14,13 +14,16 @@ import * as uuid from 'uuid'; import { isValidHexAddress, NetworkType } from '@metamask/controller-utils'; import { keyringBuilderFactory } from '@metamask/eth-keyring-controller'; import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; +import { ControllerMessenger } from '@metamask/base-controller'; import MockEncryptor, { mockKey } from '../tests/mocks/mockEncryptor'; import { AccountImportStrategy, - KeyringConfig, KeyringController, - KeyringMemState, KeyringObject, + KeyringControllerEvents, + KeyringControllerMessenger, + KeyringControllerOptions, + KeyringState, KeyringTypes, } from './KeyringController'; @@ -252,10 +255,12 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState, preferences, encryptor }) => { - const cleanKeyringController = new KeyringController( + const cleanKeyringController = new KeyringController({ preferences, - { cacheEncryptionKey, encryptor }, - ); + cacheEncryptionKey, + encryptor, + messenger: buildKeyringControllerMessenger(), + }); const initialSeedWord = await controller.exportSeedPhrase( password, ); @@ -1106,20 +1111,14 @@ describe('KeyringController', () => { }); describe('subscribe and unsubscribe', () => { - it('should subscribe and unsubscribe', async () => { - await withController(async ({ controller }) => { + it('should fire stateChange event', async () => { + await withController(async ({ controller, messenger }) => { const listener = sinon.stub(); - controller.subscribe(listener); - await controller.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], - ); + messenger.subscribe('KeyringController:stateChange', listener); + + await controller.fullUpdate(); + expect(listener.called).toBe(true); - controller.unsubscribe(listener); - await controller.removeAccount( - '0x51253087e6f8358b5f10c0a94315d69db3357859', - ); - expect(listener.calledTwice).toBe(false); }); }); }); @@ -1657,6 +1656,7 @@ type WithControllerCallback = ({ preferences, initialState, encryptor, + messenger, }: { controller: KeyringController; preferences: { @@ -1667,15 +1667,40 @@ type WithControllerCallback = ({ setSelectedAddress: sinon.SinonStub; }; encryptor: MockEncryptor; - initialState: KeyringMemState; + initialState: KeyringState; + messenger: KeyringControllerMessenger; }) => Promise | ReturnValue; -type WithControllerOptions = Partial; +type WithControllerOptions = Partial; type WithControllerArgs = | [WithControllerCallback] | [WithControllerOptions, WithControllerCallback]; +/** + * Build a controller messenger that includes all events used by the keyring + * controller. + * + * @returns The controller messenger. + */ +function buildMessenger() { + return new ControllerMessenger(); +} + +/** + * Build a restricted controller messenger for the keyring controller. + * + * @param messenger - A controller messenger. + * @returns The keyring controller restricted messenger. + */ +function buildKeyringControllerMessenger(messenger = buildMessenger()) { + return messenger.getRestricted({ + name: 'KeyringController', + allowedActions: [], + allowedEvents: ['KeyringController:stateChange'], + }); +} + /** * Builds a controller based on the given options and creates a new vault * and keychain, then calls the given function with that controller. @@ -1698,9 +1723,12 @@ async function withController( updateIdentities: sinon.stub(), setSelectedAddress: sinon.stub(), }; - const controller = new KeyringController(preferences, { + const messenger = buildKeyringControllerMessenger(); + const controller = new KeyringController({ + preferences, encryptor, cacheEncryptionKey: false, + messenger, ...rest, }); const initialState = await controller.createNewVaultAndKeychain(password); @@ -1709,5 +1737,6 @@ async function withController( preferences, encryptor, initialState, + messenger, }); } diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8173b42e4a..9cc5aa51f2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -22,16 +22,17 @@ import { IKeyringState as IQRKeyringState, } from '@keystonehq/metamask-airgapped-keyring'; import { - BaseController, - BaseConfig, - BaseState, - Listener, + BaseControllerV2, + RestrictedControllerMessenger, } from '@metamask/base-controller'; import { PreferencesController } from '@metamask/preferences-controller'; import { PersonalMessageParams, TypedMessageParams, } from '@metamask/message-manager'; +import type { Patch } from 'immer'; + +const name = 'KeyringController'; /** * Available keyring types @@ -61,40 +62,54 @@ export interface KeyringObject { * * Keyring controller state * @property vault - Encrypted string representing keyring data - * @property keyrings - Group of accounts - */ -export interface KeyringState extends BaseState { - vault?: string; - keyrings: Keyring[]; -} - -/** - * @type KeyringMemState - * - * Keyring mem controller state * @property isUnlocked - Whether vault is unlocked * @property keyringTypes - Account types * @property keyrings - Group of accounts */ -export interface KeyringMemState extends BaseState { +export type KeyringState = { + vault?: string; isUnlocked: boolean; keyringTypes: string[]; keyrings: Keyring[]; encryptionKey?: string; encryptionSalt?: string; -} +}; + +export type KeyringControllerStateChangeEvent = { + type: `KeyringController:stateChange`; + payload: [KeyringState, Patch[]]; +}; + +export type KeyringControllerEvents = KeyringControllerStateChangeEvent; + +export type KeyringControllerMessenger = RestrictedControllerMessenger< + typeof name, + never, + KeyringControllerEvents, + string, + string +>; /** - * @type KeyringConfig + * @type KeyringControllerOptions * * Keyring controller configuration * @property encryptor - Keyring encryptor */ -export interface KeyringConfig extends BaseConfig { +export type KeyringControllerOptions = { + preferences: { + removeIdentity: PreferencesController['removeIdentity']; + syncIdentities: PreferencesController['syncIdentities']; + updateIdentities: PreferencesController['updateIdentities']; + setSelectedAddress: PreferencesController['setSelectedAddress']; + setAccountLabel?: PreferencesController['setAccountLabel']; + }; + messenger: KeyringControllerMessenger; encryptor?: any; keyringBuilders?: any[]; cacheEncryptionKey?: boolean; -} + state?: Partial; +}; /** * @type Keyring @@ -104,11 +119,11 @@ export interface KeyringConfig extends BaseConfig { * @property accounts - Associated accounts * @property index - Associated index */ -export interface Keyring { +export type Keyring = { accounts: string[]; type: string; index?: number; -} +}; /** * A strategy for importing an account @@ -129,6 +144,12 @@ export enum SignTypedDataVersion { V4 = 'V4', } +const defaultState: KeyringState = { + isUnlocked: false, + keyringTypes: [], + keyrings: [], +}; + /** * Controller responsible for establishing and managing user identity. * @@ -138,17 +159,13 @@ export enum SignTypedDataVersion { * with the internal keyring controller and handling certain complex operations that involve the * keyrings. */ -export class KeyringController extends BaseController< - KeyringConfig, - KeyringState +export class KeyringController extends BaseControllerV2< + typeof name, + KeyringState, + KeyringControllerMessenger > { private mutex = new Mutex(); - /** - * Name of this controller used during composition - */ - override name = 'KeyringController'; - private removeIdentity: PreferencesController['removeIdentity']; private syncIdentities: PreferencesController['syncIdentities']; @@ -165,49 +182,62 @@ export class KeyringController extends BaseController< * Creates a KeyringController instance. * * @param options - The controller options. - * @param options.removeIdentity - Remove the identity with the given address. - * @param options.syncIdentities - Sync identities with the given list of addresses. - * @param options.updateIdentities - Generate an identity for each address given that doesn't already have an identity. - * @param options.setSelectedAddress - Set the selected address. - * @param options.setAccountLabel - Set a new name for account. - * @param config - Initial options used to configure this controller. - * @param state - Initial state to set on this controller. + * @param options.preferences - The preferences controller. + * @param options.preferences.removeIdentity - Remove the identity with the given address. + * @param options.preferences.syncIdentities - Sync identities with the given list of addresses. + * @param options.preferences.updateIdentities - Generate an identity for each address given that doesn't already have an identity. + * @param options.preferences.setSelectedAddress - Set the selected address. + * @param options.preferences.setAccountLabel - Set a new name for account. + * @param options.encryptor - The encryptor for encrypting/decrypting keyrings. + * @param options.keyringBuilders - The keyring builders for initializing the keyring controller. + * @param options.cacheEncryptionKey - Whether to cache the encryption key in memory. + * @param options.state - Initial state to set on this controller. + * @param options.messenger - A restricted controller messenger. */ - constructor( - { + constructor({ + preferences: { removeIdentity, syncIdentities, updateIdentities, setSelectedAddress, setAccountLabel, - }: { - removeIdentity: PreferencesController['removeIdentity']; - syncIdentities: PreferencesController['syncIdentities']; - updateIdentities: PreferencesController['updateIdentities']; - setSelectedAddress: PreferencesController['setSelectedAddress']; - setAccountLabel?: PreferencesController['setAccountLabel']; }, - config?: Partial, - state?: Partial, - ) { - super(config, state); + encryptor, + keyringBuilders, + cacheEncryptionKey, + state, + messenger, + }: KeyringControllerOptions) { + super({ + name, + metadata: { + vault: { persist: true, anonymous: false }, + isUnlocked: { persist: false, anonymous: false }, + keyringTypes: { persist: false, anonymous: false }, + keyrings: { persist: false, anonymous: false }, + }, + messenger, + state: { + ...defaultState, + ...state, + }, + }); + this.#keyring = new EthKeyringController( - Object.assign({ initState: state }, config), + Object.assign( + { initState: state }, + { encryptor, keyringBuilders, cacheEncryptionKey }, + ), ); this.#keyring.store.subscribe(() => { this.update({ vault: this.#keyring.store.getState().vault }); }); - this.defaultState = { - ...this.#keyring.store.getState(), - keyrings: [], - }; this.removeIdentity = removeIdentity; this.syncIdentities = syncIdentities; this.updateIdentities = updateIdentities; this.setSelectedAddress = setSelectedAddress; this.setAccountLabel = setAccountLabel; - this.initialize(); this.fullUpdate(); } @@ -220,7 +250,7 @@ export class KeyringController extends BaseController< * address. */ async addNewAccount(accountCount?: number): Promise<{ - keyringState: KeyringMemState; + keyringState: KeyringState; addedAccountAddress: string; }> { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; @@ -262,7 +292,7 @@ export class KeyringController extends BaseController< * * @returns Promise resolving to current state when the account is added. */ - async addNewAccountWithoutUpdate(): Promise { + async addNewAccountWithoutUpdate(): Promise { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; /* istanbul ignore if */ if (!primaryKeyring) { @@ -418,7 +448,7 @@ export class KeyringController extends BaseController< strategy: AccountImportStrategy, args: any[], ): Promise<{ - keyringState: KeyringMemState; + keyringState: KeyringState; importedAccountAddress: string; }> { let privateKey; @@ -479,7 +509,7 @@ export class KeyringController extends BaseController< * @param address - Address of the account to remove. * @returns Promise resolving current state when this account removal completes. */ - async removeAccount(address: string): Promise { + async removeAccount(address: string): Promise { this.removeIdentity(address); await this.#keyring.removeAccount(address); return this.fullUpdate(); @@ -490,7 +520,7 @@ export class KeyringController extends BaseController< * * @returns Promise resolving to current state. */ - setLocked(): Promise { + setLocked(): Promise { return this.#keyring.setLocked(); } @@ -606,7 +636,7 @@ export class KeyringController extends BaseController< async submitEncryptionKey( encryptionKey: string, encryptionSalt: string, - ): Promise { + ): Promise { return this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); } @@ -617,32 +647,13 @@ export class KeyringController extends BaseController< * @param password - Password to unlock the keychain. * @returns Promise resolving to the current state. */ - async submitPassword(password: string): Promise { + async submitPassword(password: string): Promise { await this.#keyring.submitPassword(password); const accounts = await this.#keyring.getAccounts(); await this.syncIdentities(accounts); return this.fullUpdate(); } - /** - * Adds new listener to be notified of state changes. - * - * @param listener - Callback triggered when state changes. - */ - override subscribe(listener: Listener) { - this.#keyring.store.subscribe(listener); - } - - /** - * Removes existing listener from receiving state changes. - * - * @param listener - Callback to remove. - * @returns True if a listener is found and unsubscribed. - */ - override unsubscribe(listener: Listener) { - return this.#keyring.store.unsubscribe(listener); - } - /** * Adds new listener to be notified when the wallet is locked. * @@ -711,7 +722,7 @@ export class KeyringController extends BaseController< * * @returns The current state. */ - async fullUpdate(): Promise { + async fullUpdate(): Promise { const keyrings: Keyring[] = await Promise.all( this.#keyring.keyrings.map( async (keyring: KeyringObject, index: number): Promise => { @@ -727,7 +738,10 @@ export class KeyringController extends BaseController< }, ), ); - this.update({ keyrings: [...keyrings] }); + this.update((state) => { + state.vault = this.#keyring.vault; + state.keyrings = [...keyrings]; + }); return this.#keyring.fullUpdate(); } diff --git a/yarn.lock b/yarn.lock index 322b90704b..048b4ed5f3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1725,6 +1725,7 @@ __metadata: deepmerge: ^4.2.2 ethereumjs-util: ^7.0.10 ethereumjs-wallet: ^1.0.1 + immer: ^10.0.2 jest: ^27.5.1 sinon: ^9.2.4 ts-jest: ^27.1.4 @@ -5819,6 +5820,13 @@ __metadata: languageName: node linkType: hard +"immer@npm:^10.0.2": + version: 10.0.2 + resolution: "immer@npm:10.0.2" + checksum: 525a3b14210d02ae420c3b9f6ca14f7e9bcf625611d1356e773e7739f14c7c8de50dac442e6c7de3a6e24a782f7b792b6b8666bc0b3f00269d21a95f8f68ca84 + languageName: node + linkType: hard + "immer@npm:^9.0.6": version: 9.0.6 resolution: "immer@npm:9.0.6" From afc2e66677e5d4aea0991b1d006bfb3f6659a274 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 18 May 2023 11:32:05 +0200 Subject: [PATCH 04/18] fix: use immer v9 --- packages/keyring-controller/package.json | 2 +- yarn.lock | 9 +-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index e9a9fb865d..4105c030bd 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -48,7 +48,7 @@ "@metamask/scure-bip39": "^2.1.0", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", - "immer": "^10.0.2", + "immer": "^9.0.6", "jest": "^27.5.1", "sinon": "^9.2.4", "ts-jest": "^27.1.4", diff --git a/yarn.lock b/yarn.lock index 048b4ed5f3..ce61665a0d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1725,7 +1725,7 @@ __metadata: deepmerge: ^4.2.2 ethereumjs-util: ^7.0.10 ethereumjs-wallet: ^1.0.1 - immer: ^10.0.2 + immer: ^9.0.6 jest: ^27.5.1 sinon: ^9.2.4 ts-jest: ^27.1.4 @@ -5820,13 +5820,6 @@ __metadata: languageName: node linkType: hard -"immer@npm:^10.0.2": - version: 10.0.2 - resolution: "immer@npm:10.0.2" - checksum: 525a3b14210d02ae420c3b9f6ca14f7e9bcf625611d1356e773e7739f14c7c8de50dac442e6c7de3a6e24a782f7b792b6b8666bc0b3f00269d21a95f8f68ca84 - languageName: node - linkType: hard - "immer@npm:^9.0.6": version: 9.0.6 resolution: "immer@npm:9.0.6" From 5ad25f733e4f6aba4b0798a9fa6b4e632875d254 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 18 May 2023 14:13:58 +0200 Subject: [PATCH 05/18] rollback: controller parameters change --- .../src/KeyringController.test.ts | 17 +++--- .../src/KeyringController.ts | 59 ++++++++----------- 2 files changed, 32 insertions(+), 44 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 064efa17a2..437f533751 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -22,7 +22,7 @@ import { KeyringObject, KeyringControllerEvents, KeyringControllerMessenger, - KeyringControllerOptions, + KeyringControllerConfig, KeyringState, KeyringTypes, } from './KeyringController'; @@ -255,12 +255,11 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState, preferences, encryptor }) => { - const cleanKeyringController = new KeyringController({ + const cleanKeyringController = new KeyringController( preferences, - cacheEncryptionKey, - encryptor, - messenger: buildKeyringControllerMessenger(), - }); + buildKeyringControllerMessenger(), + { cacheEncryptionKey, encryptor }, + ); const initialSeedWord = await controller.exportSeedPhrase( password, ); @@ -1671,7 +1670,7 @@ type WithControllerCallback = ({ messenger: KeyringControllerMessenger; }) => Promise | ReturnValue; -type WithControllerOptions = Partial; +type WithControllerOptions = Partial; type WithControllerArgs = | [WithControllerCallback] @@ -1724,11 +1723,9 @@ async function withController( setSelectedAddress: sinon.stub(), }; const messenger = buildKeyringControllerMessenger(); - const controller = new KeyringController({ - preferences, + const controller = new KeyringController(preferences, messenger, { encryptor, cacheEncryptionKey: false, - messenger, ...rest, }); const initialState = await controller.createNewVaultAndKeychain(password); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9cc5aa51f2..840dde73b8 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -65,6 +65,8 @@ export interface KeyringObject { * @property isUnlocked - Whether vault is unlocked * @property keyringTypes - Account types * @property keyrings - Group of accounts + * @property encryptionKey - Keyring encryption key + * @property encryptionSalt - Keyring encryption salt */ export type KeyringState = { vault?: string; @@ -91,24 +93,15 @@ export type KeyringControllerMessenger = RestrictedControllerMessenger< >; /** - * @type KeyringControllerOptions + * @type KeyringControllerConfig * * Keyring controller configuration * @property encryptor - Keyring encryptor */ -export type KeyringControllerOptions = { - preferences: { - removeIdentity: PreferencesController['removeIdentity']; - syncIdentities: PreferencesController['syncIdentities']; - updateIdentities: PreferencesController['updateIdentities']; - setSelectedAddress: PreferencesController['setSelectedAddress']; - setAccountLabel?: PreferencesController['setAccountLabel']; - }; - messenger: KeyringControllerMessenger; +export type KeyringControllerConfig = { encryptor?: any; keyringBuilders?: any[]; cacheEncryptionKey?: boolean; - state?: Partial; }; /** @@ -182,32 +175,33 @@ export class KeyringController extends BaseControllerV2< * Creates a KeyringController instance. * * @param options - The controller options. - * @param options.preferences - The preferences controller. - * @param options.preferences.removeIdentity - Remove the identity with the given address. - * @param options.preferences.syncIdentities - Sync identities with the given list of addresses. - * @param options.preferences.updateIdentities - Generate an identity for each address given that doesn't already have an identity. - * @param options.preferences.setSelectedAddress - Set the selected address. - * @param options.preferences.setAccountLabel - Set a new name for account. - * @param options.encryptor - The encryptor for encrypting/decrypting keyrings. - * @param options.keyringBuilders - The keyring builders for initializing the keyring controller. - * @param options.cacheEncryptionKey - Whether to cache the encryption key in memory. - * @param options.state - Initial state to set on this controller. - * @param options.messenger - A restricted controller messenger. + * @param options.removeIdentity - Remove the identity with the given address. + * @param options.syncIdentities - Sync identities with the given list of addresses. + * @param options.updateIdentities - Generate an identity for each address given that doesn't already have an identity. + * @param options.setSelectedAddress - Set the selected address. + * @param options.setAccountLabel - Set a new name for account. + * @param messenger - A restricted controller messenger. + * @param config - Initial options used to configure this controller. + * @param state - Initial state to set on this controller. */ - constructor({ - preferences: { + constructor( + { removeIdentity, syncIdentities, updateIdentities, setSelectedAddress, setAccountLabel, + }: { + removeIdentity: PreferencesController['removeIdentity']; + syncIdentities: PreferencesController['syncIdentities']; + updateIdentities: PreferencesController['updateIdentities']; + setSelectedAddress: PreferencesController['setSelectedAddress']; + setAccountLabel?: PreferencesController['setAccountLabel']; }, - encryptor, - keyringBuilders, - cacheEncryptionKey, - state, - messenger, - }: KeyringControllerOptions) { + messenger: KeyringControllerMessenger, + config?: KeyringControllerConfig, + state?: Partial, + ) { super({ name, metadata: { @@ -224,10 +218,7 @@ export class KeyringController extends BaseControllerV2< }); this.#keyring = new EthKeyringController( - Object.assign( - { initState: state }, - { encryptor, keyringBuilders, cacheEncryptionKey }, - ), + Object.assign({ initState: state }, config), ); this.#keyring.store.subscribe(() => { this.update({ vault: this.#keyring.store.getState().vault }); From f1a8f8f598cb573485119ae61662407eccbe0623 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 18 May 2023 18:42:19 +0200 Subject: [PATCH 06/18] refactor: change KeyringConfig to KeyringControllerConfig --- .../src/KeyringController.test.ts | 4 +- .../src/KeyringController.ts | 40 +++++++++++-------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 437f533751..dc437546a7 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -23,7 +23,7 @@ import { KeyringControllerEvents, KeyringControllerMessenger, KeyringControllerConfig, - KeyringState, + KeyringControllerState, KeyringTypes, } from './KeyringController'; @@ -1666,7 +1666,7 @@ type WithControllerCallback = ({ setSelectedAddress: sinon.SinonStub; }; encryptor: MockEncryptor; - initialState: KeyringState; + initialState: KeyringControllerState; messenger: KeyringControllerMessenger; }) => Promise | ReturnValue; diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 840dde73b8..976fb3f7c4 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -58,7 +58,7 @@ export interface KeyringObject { } /** - * @type KeyringState + * @type KeyringControllerState * * Keyring controller state * @property vault - Encrypted string representing keyring data @@ -68,7 +68,7 @@ export interface KeyringObject { * @property encryptionKey - Keyring encryption key * @property encryptionSalt - Keyring encryption salt */ -export type KeyringState = { +export type KeyringControllerState = { vault?: string; isUnlocked: boolean; keyringTypes: string[]; @@ -77,16 +77,23 @@ export type KeyringState = { encryptionSalt?: string; }; +export type KeyringControllerGetStateAction = { + type: `${typeof name}:getState`; + handler: () => KeyringControllerState; +}; + export type KeyringControllerStateChangeEvent = { - type: `KeyringController:stateChange`; - payload: [KeyringState, Patch[]]; + type: `${typeof name}:stateChange`; + payload: [KeyringControllerState, Patch[]]; }; +export type KeyringControllerActions = KeyringControllerGetStateAction; + export type KeyringControllerEvents = KeyringControllerStateChangeEvent; export type KeyringControllerMessenger = RestrictedControllerMessenger< typeof name, - never, + KeyringControllerActions, KeyringControllerEvents, string, string @@ -137,7 +144,7 @@ export enum SignTypedDataVersion { V4 = 'V4', } -const defaultState: KeyringState = { +const defaultState: KeyringControllerState = { isUnlocked: false, keyringTypes: [], keyrings: [], @@ -154,7 +161,7 @@ const defaultState: KeyringState = { */ export class KeyringController extends BaseControllerV2< typeof name, - KeyringState, + KeyringControllerState, KeyringControllerMessenger > { private mutex = new Mutex(); @@ -200,7 +207,7 @@ export class KeyringController extends BaseControllerV2< }, messenger: KeyringControllerMessenger, config?: KeyringControllerConfig, - state?: Partial, + state?: Partial, ) { super({ name, @@ -241,7 +248,7 @@ export class KeyringController extends BaseControllerV2< * address. */ async addNewAccount(accountCount?: number): Promise<{ - keyringState: KeyringState; + keyringState: KeyringControllerState; addedAccountAddress: string; }> { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; @@ -283,7 +290,7 @@ export class KeyringController extends BaseControllerV2< * * @returns Promise resolving to current state when the account is added. */ - async addNewAccountWithoutUpdate(): Promise { + async addNewAccountWithoutUpdate(): Promise { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; /* istanbul ignore if */ if (!primaryKeyring) { @@ -439,7 +446,7 @@ export class KeyringController extends BaseControllerV2< strategy: AccountImportStrategy, args: any[], ): Promise<{ - keyringState: KeyringState; + keyringState: KeyringControllerState; importedAccountAddress: string; }> { let privateKey; @@ -500,7 +507,7 @@ export class KeyringController extends BaseControllerV2< * @param address - Address of the account to remove. * @returns Promise resolving current state when this account removal completes. */ - async removeAccount(address: string): Promise { + async removeAccount(address: string): Promise { this.removeIdentity(address); await this.#keyring.removeAccount(address); return this.fullUpdate(); @@ -511,7 +518,7 @@ export class KeyringController extends BaseControllerV2< * * @returns Promise resolving to current state. */ - setLocked(): Promise { + setLocked(): Promise { return this.#keyring.setLocked(); } @@ -627,7 +634,7 @@ export class KeyringController extends BaseControllerV2< async submitEncryptionKey( encryptionKey: string, encryptionSalt: string, - ): Promise { + ): Promise { return this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); } @@ -638,7 +645,7 @@ export class KeyringController extends BaseControllerV2< * @param password - Password to unlock the keychain. * @returns Promise resolving to the current state. */ - async submitPassword(password: string): Promise { + async submitPassword(password: string): Promise { await this.#keyring.submitPassword(password); const accounts = await this.#keyring.getAccounts(); await this.syncIdentities(accounts); @@ -713,7 +720,7 @@ export class KeyringController extends BaseControllerV2< * * @returns The current state. */ - async fullUpdate(): Promise { + async fullUpdate(): Promise { const keyrings: Keyring[] = await Promise.all( this.#keyring.keyrings.map( async (keyring: KeyringObject, index: number): Promise => { @@ -730,7 +737,6 @@ export class KeyringController extends BaseControllerV2< ), ); this.update((state) => { - state.vault = this.#keyring.vault; state.keyrings = [...keyrings]; }); return this.#keyring.fullUpdate(); From fb1a96cc268e93177bf46049d96650f9623598ff Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 19 May 2023 14:18:11 +0200 Subject: [PATCH 07/18] refactor: sync controller state with keyring state --- .../src/KeyringController.test.ts | 14 --- .../src/KeyringController.ts | 93 +++++++------------ 2 files changed, 31 insertions(+), 76 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index dc437546a7..f0189e4d4a 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -286,7 +286,6 @@ describe('KeyringController', () => { expect(controller.state.keyrings).not.toStrictEqual([]); const keyring = controller.state.keyrings[0]; expect(keyring.accounts).not.toStrictEqual([]); - expect(keyring.index).toStrictEqual(0); expect(keyring.type).toStrictEqual('HD Key Tree'); expect(controller.state.vault).toBeDefined(); }); @@ -1109,19 +1108,6 @@ describe('KeyringController', () => { }); }); - describe('subscribe and unsubscribe', () => { - it('should fire stateChange event', async () => { - await withController(async ({ controller, messenger }) => { - const listener = sinon.stub(); - messenger.subscribe('KeyringController:stateChange', listener); - - await controller.fullUpdate(); - - expect(listener.called).toBe(true); - }); - }); - }); - describe('onLock', () => { it('should receive lock event', async () => { await withController(async ({ controller }) => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 976fb3f7c4..d2017efd5d 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -6,10 +6,7 @@ import { stripHexPrefix, getBinarySize, } from 'ethereumjs-util'; -import { - isValidHexAddress, - toChecksumHexAddress, -} from '@metamask/controller-utils'; +import { isValidHexAddress } from '@metamask/controller-utils'; import { normalize as normalizeAddress, signTypedData, @@ -117,12 +114,10 @@ export type KeyringControllerConfig = { * Keyring object to return in fullUpdate * @property type - Keyring type * @property accounts - Associated accounts - * @property index - Associated index */ export type Keyring = { accounts: string[]; type: string; - index?: number; }; /** @@ -212,7 +207,7 @@ export class KeyringController extends BaseControllerV2< super({ name, metadata: { - vault: { persist: true, anonymous: false }, + vault: { persist: false, anonymous: false }, isUnlocked: { persist: false, anonymous: false }, keyringTypes: { persist: false, anonymous: false }, keyrings: { persist: false, anonymous: false }, @@ -227,16 +222,14 @@ export class KeyringController extends BaseControllerV2< this.#keyring = new EthKeyringController( Object.assign({ initState: state }, config), ); - this.#keyring.store.subscribe(() => { - this.update({ vault: this.#keyring.store.getState().vault }); - }); + this.#keyring.store.subscribe(this.#fullUpdate.bind(this)); + this.#keyring.memStore.subscribe(this.#fullUpdate.bind(this)); this.removeIdentity = removeIdentity; this.syncIdentities = syncIdentities; this.updateIdentities = updateIdentities; this.setSelectedAddress = setSelectedAddress; this.setAccountLabel = setAccountLabel; - this.fullUpdate(); } /** @@ -265,7 +258,7 @@ export class KeyringController extends BaseControllerV2< // we return the account already existing at index `accountCount` const primaryKeyringAccounts = await primaryKeyring.getAccounts(); return { - keyringState: await this.fullUpdate(), + keyringState: this.state, addedAccountAddress: primaryKeyringAccounts[accountCount], }; } @@ -280,7 +273,7 @@ export class KeyringController extends BaseControllerV2< (selectedAddress: string) => !oldAccounts.includes(selectedAddress), ); return { - keyringState: await this.fullUpdate(), + keyringState: this.state, addedAccountAddress, }; } @@ -298,7 +291,7 @@ export class KeyringController extends BaseControllerV2< } await this.#keyring.addNewAccount(primaryKeyring); await this.verifySeedPhrase(); - return this.fullUpdate(); + return this.state; } /** @@ -310,7 +303,10 @@ export class KeyringController extends BaseControllerV2< * either as a string or an array of UTF-8 bytes that represent the string. * @returns Promise resolving to the restored keychain object. */ - async createNewVaultAndRestore(password: string, seed: Uint8Array) { + async createNewVaultAndRestore( + password: string, + seed: Uint8Array, + ): Promise { const releaseLock = await this.mutex.acquire(); if (!password || !password.length) { throw new Error('Invalid password'); @@ -318,13 +314,9 @@ export class KeyringController extends BaseControllerV2< try { this.updateIdentities([]); - const vault = await this.#keyring.createNewVaultAndRestore( - password, - seed, - ); + await this.#keyring.createNewVaultAndRestore(password, seed); this.updateIdentities(await this.#keyring.getAccounts()); - this.fullUpdate(); - return vault; + return this.state; } finally { releaseLock(); } @@ -339,17 +331,12 @@ export class KeyringController extends BaseControllerV2< async createNewVaultAndKeychain(password: string) { const releaseLock = await this.mutex.acquire(); try { - let vault; const accounts = await this.getAccounts(); - if (accounts.length > 0) { - vault = await this.fullUpdate(); - } else { - vault = await this.#keyring.createNewVaultAndKeychain(password); + if (!accounts.length) { + await this.#keyring.createNewVaultAndKeychain(password); this.updateIdentities(await this.getAccounts()); - await this.fullUpdate(); } - - return vault; + return this.state; } finally { releaseLock(); } @@ -496,7 +483,7 @@ export class KeyringController extends BaseControllerV2< const allAccounts = await this.#keyring.getAccounts(); this.updateIdentities(allAccounts); return { - keyringState: await this.fullUpdate(), + keyringState: this.state, importedAccountAddress: accounts[0], }; } @@ -510,7 +497,7 @@ export class KeyringController extends BaseControllerV2< async removeAccount(address: string): Promise { this.removeIdentity(address); await this.#keyring.removeAccount(address); - return this.fullUpdate(); + return this.state; } /** @@ -518,7 +505,7 @@ export class KeyringController extends BaseControllerV2< * * @returns Promise resolving to current state. */ - setLocked(): Promise { + setLocked(): Promise> { return this.#keyring.setLocked(); } @@ -635,7 +622,8 @@ export class KeyringController extends BaseControllerV2< encryptionKey: string, encryptionSalt: string, ): Promise { - return this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); + await this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); + return this.state; } /** @@ -649,7 +637,7 @@ export class KeyringController extends BaseControllerV2< await this.#keyring.submitPassword(password); const accounts = await this.#keyring.getAccounts(); await this.syncIdentities(accounts); - return this.fullUpdate(); + return this.state; } /** @@ -716,30 +704,15 @@ export class KeyringController extends BaseControllerV2< } /** - * Update keyrings in state and calls KeyringController fullUpdate method returning current state. + * Sync controller state with current keyring + * store and memStore state. * - * @returns The current state. */ - async fullUpdate(): Promise { - const keyrings: Keyring[] = await Promise.all( - this.#keyring.keyrings.map( - async (keyring: KeyringObject, index: number): Promise => { - const keyringAccounts = await keyring.getAccounts(); - const accounts = Array.isArray(keyringAccounts) - ? keyringAccounts.map((address) => toChecksumHexAddress(address)) - : /* istanbul ignore next */ []; - return { - accounts, - index, - type: keyring.type, - }; - }, - ), - ); - this.update((state) => { - state.keyrings = [...keyrings]; - }); - return this.#keyring.fullUpdate(); + #fullUpdate() { + this.update(() => ({ + ...this.#keyring.store.getState(), + ...this.#keyring.memStore.getState(), + })); } // QR Hardware related methods @@ -750,9 +723,7 @@ export class KeyringController extends BaseControllerV2< * @returns The added keyring */ private async addQRKeyring(): Promise { - const keyring = await this.#keyring.addNewKeyring(KeyringTypes.qr); - await this.fullUpdate(); - return keyring; + return this.#keyring.addNewKeyring(KeyringTypes.qr); } /** @@ -767,8 +738,8 @@ export class KeyringController extends BaseControllerV2< async restoreQRKeyring(serialized: any): Promise { (await this.getOrAddQRKeyring()).deserialize(serialized); + await this.#keyring.persistAllKeyrings(); this.updateIdentities(await this.#keyring.getAccounts()); - await this.fullUpdate(); } async resetQRKeyringState(): Promise { @@ -852,7 +823,6 @@ export class KeyringController extends BaseControllerV2< } }); await this.#keyring.persistAllKeyrings(); - await this.fullUpdate(); } async getAccountKeyringType(account: string): Promise { @@ -867,7 +837,6 @@ export class KeyringController extends BaseControllerV2< this.setSelectedAddress(account); }); await this.#keyring.persistAllKeyrings(); - await this.fullUpdate(); } } From f303c0b973af44b8646a93bdebdd3eb096ac1e44 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 19 May 2023 17:09:08 +0200 Subject: [PATCH 08/18] refactor: no vault sync --- 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 d2017efd5d..eeee33bc43 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -222,7 +222,6 @@ export class KeyringController extends BaseControllerV2< this.#keyring = new EthKeyringController( Object.assign({ initState: state }, config), ); - this.#keyring.store.subscribe(this.#fullUpdate.bind(this)); this.#keyring.memStore.subscribe(this.#fullUpdate.bind(this)); this.removeIdentity = removeIdentity; @@ -704,13 +703,11 @@ export class KeyringController extends BaseControllerV2< } /** - * Sync controller state with current keyring - * store and memStore state. + * Sync controller state with current keyring memStore state. * */ #fullUpdate() { this.update(() => ({ - ...this.#keyring.store.getState(), ...this.#keyring.memStore.getState(), })); } From f9e837cbff0272febfa7ef2cddafe85e90e733df Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 19 May 2023 17:09:41 +0200 Subject: [PATCH 09/18] refactor: use state value for isUnlocked --- 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 eeee33bc43..5e5e440768 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -357,7 +357,7 @@ export class KeyringController extends BaseControllerV2< * @returns Boolean returning true if the vault is unlocked. */ isUnlocked(): boolean { - return this.#keyring.memStore.getState().isUnlocked; + return this.state.isUnlocked; } /** From 3efbd4f985c6e628e932e45cba9b4ad41b5c2157 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 19 May 2023 17:54:11 +0200 Subject: [PATCH 10/18] refactor: use controller state for test assertions --- .../src/KeyringController.test.ts | 94 +++++++++---------- 1 file changed, 44 insertions(+), 50 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index f0189e4d4a..57f9c7cee5 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -63,24 +63,21 @@ describe('KeyringController', () => { it('should add new account', async () => { await withController( async ({ controller, initialState, preferences }) => { - const { - keyringState: currentKeyringMemState, - addedAccountAddress, - } = await controller.addNewAccount(); + const { addedAccountAddress } = await controller.addNewAccount(); expect(initialState.keyrings).toHaveLength(1); expect(initialState.keyrings[0].accounts).not.toStrictEqual( - currentKeyringMemState.keyrings[0].accounts, + controller.state.keyrings[0].accounts, ); - expect(currentKeyringMemState.keyrings[0].accounts).toHaveLength(2); + expect(controller.state.keyrings[0].accounts).toHaveLength(2); expect(initialState.keyrings[0].accounts).not.toContain( addedAccountAddress, ); expect(addedAccountAddress).toBe( - currentKeyringMemState.keyrings[0].accounts[1], + controller.state.keyrings[0].accounts[1], ); expect( preferences.updateIdentities.calledWith( - currentKeyringMemState.keyrings[0].accounts, + controller.state.keyrings[0].accounts, ), ).toBe(true); expect(preferences.setSelectedAddress.called).toBe(false); @@ -93,26 +90,23 @@ describe('KeyringController', () => { it('should add new account if accountCount is in sequence', async () => { await withController( async ({ controller, initialState, preferences }) => { - const { - keyringState: currentKeyringMemState, - addedAccountAddress, - } = await controller.addNewAccount( + const { addedAccountAddress } = await controller.addNewAccount( initialState.keyrings[0].accounts.length, ); expect(initialState.keyrings).toHaveLength(1); expect(initialState.keyrings[0].accounts).not.toStrictEqual( - currentKeyringMemState.keyrings[0].accounts, + controller.state.keyrings[0].accounts, ); - expect(currentKeyringMemState.keyrings[0].accounts).toHaveLength(2); + expect(controller.state.keyrings[0].accounts).toHaveLength(2); expect(initialState.keyrings[0].accounts).not.toContain( addedAccountAddress, ); expect(addedAccountAddress).toBe( - currentKeyringMemState.keyrings[0].accounts[1], + controller.state.keyrings[0].accounts[1], ); expect( preferences.updateIdentities.calledWith( - currentKeyringMemState.keyrings[0].accounts, + controller.state.keyrings[0].accounts, ), ).toBe(true); expect(preferences.setSelectedAddress.called).toBe(false); @@ -134,10 +128,10 @@ describe('KeyringController', () => { const accountCount = initialState.keyrings[0].accounts.length; const { addedAccountAddress: firstAccountAdded } = await controller.addNewAccount(accountCount); - const { keyringState, addedAccountAddress: secondAccountAdded } = + const { addedAccountAddress: secondAccountAdded } = await controller.addNewAccount(accountCount); expect(firstAccountAdded).toBe(secondAccountAdded); - expect(keyringState.keyrings[0].accounts).toHaveLength( + expect(controller.state.keyrings[0].accounts).toHaveLength( accountCount + 1, ); }); @@ -151,13 +145,12 @@ describe('KeyringController', () => { async ({ controller, initialState, preferences }) => { const initialUpdateIdentitiesCallCount = preferences.updateIdentities.callCount; - const currentKeyringMemState = - await controller.addNewAccountWithoutUpdate(); + await controller.addNewAccountWithoutUpdate(); expect(initialState.keyrings).toHaveLength(1); expect(initialState.keyrings[0].accounts).not.toStrictEqual( - currentKeyringMemState.keyrings[0].accounts, + controller.state.keyrings[0].accounts, ); - expect(currentKeyringMemState.keyrings[0].accounts).toHaveLength(2); + expect(controller.state.keyrings[0].accounts).toHaveLength(2); // we make sure that updateIdentities is not called // during this test expect(preferences.updateIdentities.callCount).toBe( @@ -176,11 +169,11 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { const initialVault = controller.state.vault; - const currentState = await controller.createNewVaultAndRestore( + await controller.createNewVaultAndRestore( password, uint8ArraySeed, ); - expect(initialState).not.toBe(currentState); + expect(controller.state).not.toBe(initialState); expect(controller.state.vault).toBeDefined(); expect(controller.state.vault).toStrictEqual(initialVault); }, @@ -195,11 +188,11 @@ describe('KeyringController', () => { password, ); - const currentState = await controller.createNewVaultAndRestore( + await controller.createNewVaultAndRestore( password, currentSeedWord, ); - expect(initialState).toStrictEqual(currentState); + expect(initialState).toStrictEqual(controller.state); }, ); }); @@ -235,12 +228,12 @@ describe('KeyringController', () => { cacheEncryptionKey && it('should set encryptionKey and encryptionSalt in state', async () => { withController({ cacheEncryptionKey }, async ({ controller }) => { - const currentState = await controller.createNewVaultAndRestore( + await controller.createNewVaultAndRestore( password, uint8ArraySeed, ); - expect(currentState.encryptionKey).toBeDefined(); - expect(currentState.encryptionSalt).toBeDefined(); + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptionSalt).toBeDefined(); }); }); }), @@ -263,18 +256,19 @@ describe('KeyringController', () => { const initialSeedWord = await controller.exportSeedPhrase( password, ); - const currentState = - await cleanKeyringController.createNewVaultAndKeychain( - password, - ); + await cleanKeyringController.createNewVaultAndKeychain( + password, + ); const currentSeedWord = await cleanKeyringController.exportSeedPhrase(password); expect(initialSeedWord).toBeDefined(); - expect(initialState).not.toBe(currentState); + expect(initialState).not.toBe(cleanKeyringController.state); expect(currentSeedWord).toBeDefined(); expect(initialSeedWord).not.toBe(currentSeedWord); expect( - isValidHexAddress(currentState.keyrings[0].accounts[0]), + isValidHexAddress( + cleanKeyringController.state.keyrings[0].accounts[0], + ), ).toBe(true); expect(controller.state.vault).toBeDefined(); }, @@ -301,14 +295,12 @@ describe('KeyringController', () => { password, ); const initialVault = controller.state.vault; - const currentState = await controller.createNewVaultAndKeychain( - password, - ); + await controller.createNewVaultAndKeychain(password); const currentSeedWord = await controller.exportSeedPhrase( password, ); expect(initialSeedWord).toBeDefined(); - expect(initialState).toBe(currentState); + expect(initialState).toBe(controller.state); expect(currentSeedWord).toBeDefined(); expect(initialSeedWord).toBe(currentSeedWord); expect(initialVault).toStrictEqual(controller.state.vault); @@ -332,8 +324,10 @@ describe('KeyringController', () => { it('should set locked correctly', async () => { await withController(async ({ controller }) => { expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); controller.setLocked(); expect(controller.isUnlocked()).toBe(false); + expect(controller.state.isUnlocked).toBe(false); }); }); }); @@ -484,7 +478,7 @@ describe('KeyringController', () => { accounts: [address], type: 'Simple Key Pair', }; - const { keyringState, importedAccountAddress } = + const { importedAccountAddress } = await controller.importAccountWithStrategy( AccountImportStrategy.privateKey, [privateKey], @@ -493,7 +487,7 @@ describe('KeyringController', () => { ...initialState, keyrings: [initialState.keyrings[0], newKeyring], }; - expect(keyringState).toStrictEqual(modifiedState); + expect(controller.state).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); }); }); @@ -553,7 +547,7 @@ describe('KeyringController', () => { const somePassword = 'holachao123'; const address = '0xb97c80fab7a3793bbe746864db80d236f1345ea7'; - const { keyringState, importedAccountAddress } = + const { importedAccountAddress } = await controller.importAccountWithStrategy( AccountImportStrategy.json, [input, somePassword], @@ -567,7 +561,7 @@ describe('KeyringController', () => { ...initialState, keyrings: [initialState.keyrings[0], newKeyring], }; - expect(keyringState).toStrictEqual(modifiedState); + expect(controller.state).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); }); }); @@ -651,8 +645,8 @@ describe('KeyringController', () => { it('should remove HD Key Tree keyring from state when single account associated with it is deleted', async () => { await withController(async ({ controller, initialState }) => { const account = initialState.keyrings[0].accounts[0]; - const finalState = await controller.removeAccount(account); - expect(finalState.keyrings).toHaveLength(0); + await controller.removeAccount(account); + expect(controller.state.keyrings).toHaveLength(0); }); }); @@ -662,10 +656,10 @@ describe('KeyringController', () => { AccountImportStrategy.privateKey, [privateKey], ); - const finalState = await controller.removeAccount( + await controller.removeAccount( '0x51253087e6f8358b5f10c0a94315d69db3357859', ); - expect(finalState).toStrictEqual(initialState); + expect(controller.state).toStrictEqual(initialState); }); }); @@ -1084,9 +1078,9 @@ describe('KeyringController', () => { cacheEncryptionKey && it('should set encryptionKey and encryptionSalt in state', async () => { withController({ cacheEncryptionKey }, async ({ controller }) => { - const recoveredState = await controller.submitPassword(password); - expect(recoveredState.encryptionKey).toBeDefined(); - expect(recoveredState.encryptionSalt).toBeDefined(); + await controller.submitPassword(password); + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptionSalt).toBeDefined(); }); }); }), From 21f02526a407505dd47a2974d9018ece94732f13 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 19 May 2023 19:16:23 +0200 Subject: [PATCH 11/18] refactor: remove onLock and onUnlock in favor of events --- .../src/KeyringController.test.ts | 49 ++++++------ .../src/KeyringController.ts | 76 +++++++++++-------- 2 files changed, 71 insertions(+), 54 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 57f9c7cee5..066092fa81 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -330,6 +330,15 @@ describe('KeyringController', () => { expect(controller.state.isUnlocked).toBe(false); }); }); + + it('should emit KeyringController:lock event', async () => { + await withController(async ({ controller, messenger }) => { + const listener = sinon.spy(); + messenger.subscribe('KeyringController:lock', listener); + await controller.setLocked(); + expect(listener.called).toBe(true); + }); + }); }); describe('exportSeedPhrase', () => { @@ -1075,6 +1084,18 @@ describe('KeyringController', () => { ); }); + it('should emit KeyringController:unlock event', async () => { + await withController( + { cacheEncryptionKey }, + async ({ controller, messenger }) => { + const listener = sinon.spy(); + messenger.subscribe('KeyringController:unlock', listener); + await controller.submitPassword(password); + expect(listener.called).toBe(true); + }, + ); + }); + cacheEncryptionKey && it('should set encryptionKey and encryptionSalt in state', async () => { withController({ cacheEncryptionKey }, async ({ controller }) => { @@ -1102,28 +1123,6 @@ describe('KeyringController', () => { }); }); - describe('onLock', () => { - it('should receive lock event', async () => { - await withController(async ({ controller }) => { - const listenerLock = sinon.stub(); - controller.onLock(listenerLock); - await controller.setLocked(); - expect(listenerLock.called).toBe(true); - }); - }); - }); - - describe('onUnlock', () => { - it('should receive unlock event', async () => { - await withController(async ({ controller }) => { - const listenerUnlock = sinon.stub(); - controller.onUnlock(listenerUnlock); - await controller.submitPassword(password); - expect(listenerUnlock.called).toBe(true); - }); - }); - }); - describe('verifySeedPhrase', () => { it('should return current seedphrase', async () => { await withController(async ({ controller }) => { @@ -1676,7 +1675,11 @@ function buildKeyringControllerMessenger(messenger = buildMessenger()) { return messenger.getRestricted({ name: 'KeyringController', allowedActions: [], - allowedEvents: ['KeyringController:stateChange'], + allowedEvents: [ + 'KeyringController:stateChange', + 'KeyringController:lock', + 'KeyringController:unlock', + ], }); } diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 5e5e440768..9e1553ed06 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -84,9 +84,22 @@ export type KeyringControllerStateChangeEvent = { payload: [KeyringControllerState, Patch[]]; }; +export type KeyringControllerLockEvent = { + type: `${typeof name}:lock`; + payload: []; +}; + +export type KeyringControllerUnlockEvent = { + type: `${typeof name}:unlock`; + payload: []; +}; + export type KeyringControllerActions = KeyringControllerGetStateAction; -export type KeyringControllerEvents = KeyringControllerStateChangeEvent; +export type KeyringControllerEvents = + | KeyringControllerStateChangeEvent + | KeyringControllerLockEvent + | KeyringControllerUnlockEvent; export type KeyringControllerMessenger = RestrictedControllerMessenger< typeof name, @@ -223,6 +236,8 @@ export class KeyringController extends BaseControllerV2< Object.assign({ initState: state }, config), ); this.#keyring.memStore.subscribe(this.#fullUpdate.bind(this)); + this.#keyring.on('lock', this.#handleLock.bind(this)); + this.#keyring.on('unlock', this.#handleUnlock.bind(this)); this.removeIdentity = removeIdentity; this.syncIdentities = syncIdentities; @@ -639,26 +654,6 @@ export class KeyringController extends BaseControllerV2< return this.state; } - /** - * Adds new listener to be notified when the wallet is locked. - * - * @param listener - Callback triggered when wallet is locked. - * @returns EventEmitter if listener added. - */ - onLock(listener: () => void) { - return this.#keyring.on('lock', listener); - } - - /** - * Adds new listener to be notified when the wallet is unlocked. - * - * @param listener - Callback triggered when wallet is unlocked. - * @returns EventEmitter if listener added. - */ - onUnlock(listener: () => void) { - return this.#keyring.on('unlock', listener); - } - /** * Verifies the that the seed phrase restores the current keychain's accounts. * @@ -702,16 +697,6 @@ export class KeyringController extends BaseControllerV2< return seedWords; } - /** - * Sync controller state with current keyring memStore state. - * - */ - #fullUpdate() { - this.update(() => ({ - ...this.#keyring.memStore.getState(), - })); - } - // QR Hardware related methods /** @@ -835,6 +820,35 @@ export class KeyringController extends BaseControllerV2< }); await this.#keyring.persistAllKeyrings(); } + + /** + * Sync controller state with current keyring memStore state. + * + * @fires KeyringController:stateChange + */ + #fullUpdate() { + this.update(() => ({ + ...this.#keyring.memStore.getState(), + })); + } + + /** + * Handle keyring lock event. + * + * @fires KeyringController:lock + */ + #handleLock() { + this.messagingSystem.publish(`${name}:lock`); + } + + /** + * Handle keyring unlock event. + * + * @fires KeyringController:unlock + */ + #handleUnlock() { + this.messagingSystem.publish(`${name}:unlock`); + } } export default KeyringController; From e42f23ed3760baa6dd71cd501bd020a948e93440 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 May 2023 10:08:41 +0200 Subject: [PATCH 12/18] refactor: sync mem and persistent store --- packages/keyring-controller/src/KeyringController.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9e1553ed06..9367d496b6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -236,6 +236,7 @@ export class KeyringController extends BaseControllerV2< Object.assign({ initState: state }, config), ); 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)); @@ -822,12 +823,14 @@ export class KeyringController extends BaseControllerV2< } /** - * Sync controller state with current keyring memStore state. + * Sync controller state with current keyring store + * and memStore states. * * @fires KeyringController:stateChange */ #fullUpdate() { this.update(() => ({ + ...this.#keyring.store.getState(), ...this.#keyring.memStore.getState(), })); } From 4c86fb506900a5e804d65ae7700d8ee91c3f1850 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 29 May 2023 13:10:04 +0200 Subject: [PATCH 13/18] refactor: apply review feedback --- 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 9367d496b6..64e785b7ae 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -220,8 +220,8 @@ export class KeyringController extends BaseControllerV2< super({ name, metadata: { - vault: { persist: false, anonymous: false }, - isUnlocked: { persist: false, anonymous: false }, + vault: { persist: true, anonymous: false }, + isUnlocked: { persist: false, anonymous: true }, keyringTypes: { persist: false, anonymous: false }, keyrings: { persist: false, anonymous: false }, }, From 2dc7a3c21c88a66d04e53cbea4f389e304912a0d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 29 May 2023 13:33:33 +0200 Subject: [PATCH 14/18] refactor: use option bag for constructor --- .../src/KeyringController.test.ts | 19 ++--- .../src/KeyringController.ts | 70 +++++++++---------- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 066092fa81..c2f180a452 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -22,9 +22,9 @@ import { KeyringObject, KeyringControllerEvents, KeyringControllerMessenger, - KeyringControllerConfig, KeyringControllerState, KeyringTypes, + KeyringControllerOptions, } from './KeyringController'; jest.mock('uuid', () => { @@ -248,11 +248,12 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState, preferences, encryptor }) => { - const cleanKeyringController = new KeyringController( - preferences, - buildKeyringControllerMessenger(), - { cacheEncryptionKey, encryptor }, - ); + const cleanKeyringController = new KeyringController({ + ...preferences, + messenger: buildKeyringControllerMessenger(), + cacheEncryptionKey, + encryptor, + }); const initialSeedWord = await controller.exportSeedPhrase( password, ); @@ -1649,7 +1650,7 @@ type WithControllerCallback = ({ messenger: KeyringControllerMessenger; }) => Promise | ReturnValue; -type WithControllerOptions = Partial; +type WithControllerOptions = Partial; type WithControllerArgs = | [WithControllerCallback] @@ -1706,9 +1707,11 @@ async function withController( setSelectedAddress: sinon.stub(), }; const messenger = buildKeyringControllerMessenger(); - const controller = new KeyringController(preferences, messenger, { + const controller = new KeyringController({ encryptor, cacheEncryptionKey: false, + messenger, + ...preferences, ...rest, }); const initialState = await controller.createNewVaultAndKeychain(password); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 64e785b7ae..44d3af3de2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -109,16 +109,17 @@ export type KeyringControllerMessenger = RestrictedControllerMessenger< string >; -/** - * @type KeyringControllerConfig - * - * Keyring controller configuration - * @property encryptor - Keyring encryptor - */ -export type KeyringControllerConfig = { +export type KeyringControllerOptions = { + removeIdentity: PreferencesController['removeIdentity']; + syncIdentities: PreferencesController['syncIdentities']; + updateIdentities: PreferencesController['updateIdentities']; + setSelectedAddress: PreferencesController['setSelectedAddress']; + setAccountLabel?: PreferencesController['setAccountLabel']; encryptor?: any; keyringBuilders?: any[]; cacheEncryptionKey?: boolean; + messenger: KeyringControllerMessenger; + state?: Partial; }; /** @@ -189,34 +190,30 @@ export class KeyringController extends BaseControllerV2< /** * Creates a KeyringController instance. * - * @param options - The controller options. - * @param options.removeIdentity - Remove the identity with the given address. - * @param options.syncIdentities - Sync identities with the given list of addresses. - * @param options.updateIdentities - Generate an identity for each address given that doesn't already have an identity. - * @param options.setSelectedAddress - Set the selected address. - * @param options.setAccountLabel - Set a new name for account. - * @param messenger - A restricted controller messenger. - * @param config - Initial options used to configure this controller. - * @param state - Initial state to set on this controller. + * @param opts - Initial options used to configure this controller + * @param opts.removeIdentity - Remove the identity with the given address. + * @param opts.syncIdentities - Sync identities with the given list of addresses. + * @param opts.updateIdentities - Generate an identity for each address given that doesn't already have an identity. + * @param opts.setSelectedAddress - Set the selected address. + * @param opts.setAccountLabel - Set a new name for account. + * @param opts.encryptor - An optional object for defining encryption schemes. + * @param opts.keyringBuilders - Set a new name for account. + * @param opts.cacheEncryptionKey - Whether to cache or not encryption key. + * @param opts.messenger - A restricted controller messenger. + * @param opts.state - Initial state to set on this controller. */ - constructor( - { - removeIdentity, - syncIdentities, - updateIdentities, - setSelectedAddress, - setAccountLabel, - }: { - removeIdentity: PreferencesController['removeIdentity']; - syncIdentities: PreferencesController['syncIdentities']; - updateIdentities: PreferencesController['updateIdentities']; - setSelectedAddress: PreferencesController['setSelectedAddress']; - setAccountLabel?: PreferencesController['setAccountLabel']; - }, - messenger: KeyringControllerMessenger, - config?: KeyringControllerConfig, - state?: Partial, - ) { + constructor({ + removeIdentity, + syncIdentities, + updateIdentities, + setSelectedAddress, + setAccountLabel, + encryptor, + keyringBuilders, + cacheEncryptionKey, + messenger, + state, + }: KeyringControllerOptions) { super({ name, metadata: { @@ -233,7 +230,10 @@ export class KeyringController extends BaseControllerV2< }); this.#keyring = new EthKeyringController( - Object.assign({ initState: state }, config), + Object.assign( + { initState: state }, + { encryptor, keyringBuilders, cacheEncryptionKey }, + ), ); this.#keyring.memStore.subscribe(this.#fullUpdate.bind(this)); this.#keyring.store.subscribe(this.#fullUpdate.bind(this)); From 7d1d2653a528f31fc9bb1e61d7f91c188c534290 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 30 May 2023 11:09:02 +0200 Subject: [PATCH 15/18] fix: set immer as prod dependency --- packages/keyring-controller/package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 4105c030bd..0f7a0eeda9 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -38,7 +38,8 @@ "@metamask/preferences-controller": "workspace:^", "async-mutex": "^0.2.6", "ethereumjs-util": "^7.0.10", - "ethereumjs-wallet": "^1.0.1" + "ethereumjs-wallet": "^1.0.1", + "immer": "^9.0.6" }, "devDependencies": { "@ethereumjs/common": "^2.6.1", @@ -48,7 +49,6 @@ "@metamask/scure-bip39": "^2.1.0", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", - "immer": "^9.0.6", "jest": "^27.5.1", "sinon": "^9.2.4", "ts-jest": "^27.1.4", From 2b785196e2823405c7758b25afe66a683bc3fb61 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 30 May 2023 11:27:16 +0200 Subject: [PATCH 16/18] fix: add encryption key and salt to memstore --- packages/keyring-controller/src/KeyringController.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 44d3af3de2..1822cf0427 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -221,6 +221,8 @@ export class KeyringController extends BaseControllerV2< isUnlocked: { persist: false, anonymous: true }, keyringTypes: { persist: false, anonymous: false }, keyrings: { persist: false, anonymous: false }, + encryptionKey: { persist: false, anonymous: false }, + encryptionSalt: { persist: false, anonymous: false }, }, messenger, state: { From f3f0807eeec74ee06af6cdefed9ef47ba6b7c4b5 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 1 Jun 2023 14:34:55 +0200 Subject: [PATCH 17/18] refactor: remove vault encryptionKey and encryptionSalt from returns --- .../src/KeyringController.test.ts | 12 ++--- .../src/KeyringController.ts | 45 ++++++++++++------- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index c2f180a452..b0761d5cec 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1079,8 +1079,8 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState }) => { - const recoveredState = await controller.submitPassword(password); - expect(recoveredState).toStrictEqual(initialState); + await controller.submitPassword(password); + expect(controller.state).toStrictEqual(initialState); }, ); }); @@ -1114,11 +1114,11 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey: true }, async ({ controller, initialState }) => { - const recoveredState = await controller.submitEncryptionKey( + await controller.submitEncryptionKey( mockKey.toString('hex'), initialState.encryptionSalt as string, ); - expect(recoveredState).toStrictEqual(initialState); + expect(controller.state).toStrictEqual(initialState); }, ); }); @@ -1714,12 +1714,12 @@ async function withController( ...preferences, ...rest, }); - const initialState = await controller.createNewVaultAndKeychain(password); + await controller.createNewVaultAndKeychain(password); return await fn({ controller, preferences, encryptor, - initialState, + initialState: controller.state, messenger, }); } diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 1822cf0427..ce347f69eb 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -74,6 +74,11 @@ export type KeyringControllerState = { encryptionSalt?: string; }; +export type KeyringControllerMemState = Omit< + KeyringControllerState, + 'vault' | 'encryptionKey' | 'encryptionSalt' +>; + export type KeyringControllerGetStateAction = { type: `${typeof name}:getState`; handler: () => KeyringControllerState; @@ -258,7 +263,7 @@ export class KeyringController extends BaseControllerV2< * address. */ async addNewAccount(accountCount?: number): Promise<{ - keyringState: KeyringControllerState; + keyringState: KeyringControllerMemState; addedAccountAddress: string; }> { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; @@ -290,7 +295,7 @@ export class KeyringController extends BaseControllerV2< (selectedAddress: string) => !oldAccounts.includes(selectedAddress), ); return { - keyringState: this.state, + keyringState: this.#getMemState(), addedAccountAddress, }; } @@ -300,7 +305,7 @@ export class KeyringController extends BaseControllerV2< * * @returns Promise resolving to current state when the account is added. */ - async addNewAccountWithoutUpdate(): Promise { + async addNewAccountWithoutUpdate(): Promise { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; /* istanbul ignore if */ if (!primaryKeyring) { @@ -308,7 +313,7 @@ export class KeyringController extends BaseControllerV2< } await this.#keyring.addNewAccount(primaryKeyring); await this.verifySeedPhrase(); - return this.state; + return this.#getMemState(); } /** @@ -323,7 +328,7 @@ export class KeyringController extends BaseControllerV2< async createNewVaultAndRestore( password: string, seed: Uint8Array, - ): Promise { + ): Promise { const releaseLock = await this.mutex.acquire(); if (!password || !password.length) { throw new Error('Invalid password'); @@ -333,7 +338,7 @@ export class KeyringController extends BaseControllerV2< this.updateIdentities([]); await this.#keyring.createNewVaultAndRestore(password, seed); this.updateIdentities(await this.#keyring.getAccounts()); - return this.state; + return this.#getMemState(); } finally { releaseLock(); } @@ -353,7 +358,7 @@ export class KeyringController extends BaseControllerV2< await this.#keyring.createNewVaultAndKeychain(password); this.updateIdentities(await this.getAccounts()); } - return this.state; + return this.#getMemState(); } finally { releaseLock(); } @@ -450,7 +455,7 @@ export class KeyringController extends BaseControllerV2< strategy: AccountImportStrategy, args: any[], ): Promise<{ - keyringState: KeyringControllerState; + keyringState: KeyringControllerMemState; importedAccountAddress: string; }> { let privateKey; @@ -500,7 +505,7 @@ export class KeyringController extends BaseControllerV2< const allAccounts = await this.#keyring.getAccounts(); this.updateIdentities(allAccounts); return { - keyringState: this.state, + keyringState: this.#getMemState(), importedAccountAddress: accounts[0], }; } @@ -511,10 +516,10 @@ export class KeyringController extends BaseControllerV2< * @param address - Address of the account to remove. * @returns Promise resolving current state when this account removal completes. */ - async removeAccount(address: string): Promise { + async removeAccount(address: string): Promise { this.removeIdentity(address); await this.#keyring.removeAccount(address); - return this.state; + return this.#getMemState(); } /** @@ -522,7 +527,7 @@ export class KeyringController extends BaseControllerV2< * * @returns Promise resolving to current state. */ - setLocked(): Promise> { + setLocked(): Promise { return this.#keyring.setLocked(); } @@ -638,9 +643,9 @@ export class KeyringController extends BaseControllerV2< async submitEncryptionKey( encryptionKey: string, encryptionSalt: string, - ): Promise { + ): Promise { await this.#keyring.submitEncryptionKey(encryptionKey, encryptionSalt); - return this.state; + return this.#getMemState(); } /** @@ -650,11 +655,11 @@ export class KeyringController extends BaseControllerV2< * @param password - Password to unlock the keychain. * @returns Promise resolving to the current state. */ - async submitPassword(password: string): Promise { + async submitPassword(password: string): Promise { await this.#keyring.submitPassword(password); const accounts = await this.#keyring.getAccounts(); await this.syncIdentities(accounts); - return this.state; + return this.#getMemState(); } /** @@ -854,6 +859,14 @@ export class KeyringController extends BaseControllerV2< #handleUnlock() { this.messagingSystem.publish(`${name}:unlock`); } + + #getMemState(): KeyringControllerMemState { + return { + isUnlocked: this.state.isUnlocked, + keyrings: this.state.keyrings, + keyringTypes: this.state.keyringTypes, + }; + } } export default KeyringController; From a0d8096c6b0a36fc0e7b5464c0a5ae447515f272 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 5 Jun 2023 10:24:04 +0200 Subject: [PATCH 18/18] fix: remove forgotten state return --- 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 ce347f69eb..59950e861a 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -280,7 +280,7 @@ export class KeyringController extends BaseControllerV2< // we return the account already existing at index `accountCount` const primaryKeyringAccounts = await primaryKeyring.getAccounts(); return { - keyringState: this.state, + keyringState: this.#getMemState(), addedAccountAddress: primaryKeyringAccounts[accountCount], }; }