From f603e0789f5c69315503b59719d4866711219cdd Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 30 Apr 2025 13:50:35 +0200 Subject: [PATCH 01/13] perf(keyring-controller): do not fire unecessary :stageChange with withKeyring --- packages/keyring-controller/CHANGELOG.md | 1 + packages/keyring-controller/package.json | 1 + .../src/KeyringController.test.ts | 66 +++++++++++++++++++ .../src/KeyringController.ts | 14 +++- .../tests/mocks/mockMutableKeyring.ts | 19 ++++++ yarn.lock | 1 + 6 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 packages/keyring-controller/tests/mocks/mockMutableKeyring.ts diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 450437656f3..528a146eff2 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Don't emit `:stateChange` from `withKeyring` unnecessarily ([#5732](https://github.com/MetaMask/core/pull/5732)) - Bump `@metamask/base-controller` from ^8.0.0 to ^8.0.1 ([#5722](https://github.com/MetaMask/core/pull/5722)) ## [21.0.4] diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index c163ececc63..86c7e2ab9ad 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -60,6 +60,7 @@ "async-mutex": "^0.5.0", "ethereumjs-wallet": "^1.0.1", "immer": "^9.0.6", + "lodash": "^4.17.21", "ulid": "^2.3.0" }, "devDependencies": { diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 29991fe2d6c..df4a83cb268 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -41,6 +41,7 @@ import MockEncryptor, { } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; import { MockKeyring } from '../tests/mocks/mockKeyring'; +import { MockMutableKeyring } from '../tests/mocks/mockMutableKeyring'; import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring'; import { buildMockTransaction } from '../tests/mocks/mockTransaction'; @@ -3080,6 +3081,71 @@ describe('KeyringController', () => { }, ); }); + + it('should update the vault if the keyring is being updated', async () => { + await withController( + { keyringBuilders: [keyringBuilderFactory(MockMutableKeyring)] }, + async ({ controller, messenger }) => { + const selector = { type: MockMutableKeyring.type }; + + const mockStateChange = jest.fn(); + messenger.subscribe( + 'KeyringController:stateChange', + mockStateChange, + ); + + await controller.withKeyring( + selector, + async ({ keyring }) => { + (keyring as MockMutableKeyring).update(); // Update the keyring state. + }, + { + createIfMissing: true, + }, + ); + + expect(mockStateChange).toHaveBeenCalled(); + expect(controller.state.keyrings).toHaveLength(2); + }, + ); + }); + + it('should not update the vault if the keyring has not been updated', async () => { + await withController( + { keyringBuilders: [keyringBuilderFactory(MockMutableKeyring)] }, + async ({ controller, messenger }) => { + const selector = { type: MockMutableKeyring.type }; + + const mockStateChange = jest.fn(); + messenger.subscribe( + 'KeyringController:stateChange', + mockStateChange, + ); + + // First create the reading before reading from it. + await controller.withKeyring( + selector, + async () => { + // No-op, but the keyring will still be persisted since we + // used `createIfMissing`. + }, + { + createIfMissing: true, + }, + ); + + expect(mockStateChange).toHaveBeenCalled(); + expect(controller.state.keyrings).toHaveLength(2); + + mockStateChange.mockReset(); + await controller.withKeyring(selector, async () => { + // No-op, keyring state won't be updated. + }); + + expect(mockStateChange).not.toHaveBeenCalled(); + }, + ); + }); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index f30a0a46387..f38dad4f83f 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -38,6 +38,7 @@ import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; // When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. +import { isEqual } from 'lodash'; import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; @@ -1551,8 +1552,9 @@ export class KeyringController extends BaseController< ): Promise { this.#assertIsUnlocked(); - return this.#persistOrRollback(async () => { + return this.#withRollback(async () => { let keyring: SelectedKeyring | undefined; + let forceUpdate = false; if ('address' in selector) { keyring = (await this.getKeyringForAccount(selector.address)) as @@ -1568,6 +1570,9 @@ export class KeyringController extends BaseController< selector.type, options.createWithData, )) as SelectedKeyring; + + // This is a new keyring, so we force the vault update in that case. + forceUpdate = true; } } else if ('id' in selector) { keyring = this.#getKeyringById(selector.id) as SelectedKeyring; @@ -1577,6 +1582,8 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.KeyringNotFound); } + const oldKeyringState = await keyring.serialize(); + const result = await operation({ keyring, metadata: this.#getKeyringMetadata(keyring), @@ -1590,6 +1597,11 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.UnsafeDirectKeyringAccess); } + if (forceUpdate || !isEqual(oldKeyringState, await keyring.serialize())) { + // Keyring has been updated, we need to update the vault. + await this.#updateVault(); + } + return result; }); } diff --git a/packages/keyring-controller/tests/mocks/mockMutableKeyring.ts b/packages/keyring-controller/tests/mocks/mockMutableKeyring.ts new file mode 100644 index 00000000000..ffcddd30e77 --- /dev/null +++ b/packages/keyring-controller/tests/mocks/mockMutableKeyring.ts @@ -0,0 +1,19 @@ +import { MockKeyring } from './mockKeyring'; + +export class MockMutableKeyring extends MockKeyring { + static type = 'Mock Mutable Keyring'; + + public type = 'Mock Mutable Keyring'; + + #updated: boolean = false; + + update() { + this.#updated = true; + } + + async serialize() { + return Promise.resolve({ + updated: this.#updated, + }); + } +} diff --git a/yarn.lock b/yarn.lock index 0c4a3632aba..d852c2d8a91 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3553,6 +3553,7 @@ __metadata: immer: "npm:^9.0.6" jest: "npm:^27.5.1" jest-environment-node: "npm:^27.5.1" + lodash: "npm:^4.17.21" sinon: "npm:^9.2.4" ts-jest: "npm:^27.1.4" typedoc: "npm:^0.24.8" From 676415f047cc21f5df81534aa4070e62e5775ccb Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 30 Apr 2025 18:18:47 +0200 Subject: [PATCH 02/13] test: refactor --- .../src/KeyringController.test.ts | 62 +++++++++---------- .../tests/mocks/mockMutableKeyring.ts | 19 ------ 2 files changed, 31 insertions(+), 50 deletions(-) delete mode 100644 packages/keyring-controller/tests/mocks/mockMutableKeyring.ts diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index df4a83cb268..c207d07b14d 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -41,7 +41,6 @@ import MockEncryptor, { } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; import { MockKeyring } from '../tests/mocks/mockKeyring'; -import { MockMutableKeyring } from '../tests/mocks/mockMutableKeyring'; import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring'; import { buildMockTransaction } from '../tests/mocks/mockTransaction'; @@ -3083,10 +3082,21 @@ describe('KeyringController', () => { }); it('should update the vault if the keyring is being updated', async () => { + const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4'; + stubKeyringClassWithAccount(MockKeyring, mockAddress); await withController( - { keyringBuilders: [keyringBuilderFactory(MockMutableKeyring)] }, + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, async ({ controller, messenger }) => { - const selector = { type: MockMutableKeyring.type }; + const selector = { type: MockKeyring.type }; + + await controller.addNewKeyring(MockKeyring.type); + const serializeSpy = jest.spyOn( + MockKeyring.prototype, + 'serialize', + ); + serializeSpy.mockResolvedValueOnce({ + foo: 'bar', // Initial keyring state. + }); const mockStateChange = jest.fn(); messenger.subscribe( @@ -3094,27 +3104,33 @@ describe('KeyringController', () => { mockStateChange, ); - await controller.withKeyring( - selector, - async ({ keyring }) => { - (keyring as MockMutableKeyring).update(); // Update the keyring state. - }, - { - createIfMissing: true, - }, - ); + await controller.withKeyring(selector, async () => { + serializeSpy.mockResolvedValueOnce({ + foo: 'zzz', // Mock keyring state change. + }); + }); expect(mockStateChange).toHaveBeenCalled(); - expect(controller.state.keyrings).toHaveLength(2); }, ); }); it('should not update the vault if the keyring has not been updated', async () => { + const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4'; + stubKeyringClassWithAccount(MockKeyring, mockAddress); await withController( - { keyringBuilders: [keyringBuilderFactory(MockMutableKeyring)] }, + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, async ({ controller, messenger }) => { - const selector = { type: MockMutableKeyring.type }; + const selector = { type: MockKeyring.type }; + + await controller.addNewKeyring(MockKeyring.type); + const serializeSpy = jest.spyOn( + MockKeyring.prototype, + 'serialize', + ); + serializeSpy.mockResolvedValue({ + foo: 'bar', // Initial keyring state. + }); const mockStateChange = jest.fn(); messenger.subscribe( @@ -3122,22 +3138,6 @@ describe('KeyringController', () => { mockStateChange, ); - // First create the reading before reading from it. - await controller.withKeyring( - selector, - async () => { - // No-op, but the keyring will still be persisted since we - // used `createIfMissing`. - }, - { - createIfMissing: true, - }, - ); - - expect(mockStateChange).toHaveBeenCalled(); - expect(controller.state.keyrings).toHaveLength(2); - - mockStateChange.mockReset(); await controller.withKeyring(selector, async () => { // No-op, keyring state won't be updated. }); diff --git a/packages/keyring-controller/tests/mocks/mockMutableKeyring.ts b/packages/keyring-controller/tests/mocks/mockMutableKeyring.ts deleted file mode 100644 index ffcddd30e77..00000000000 --- a/packages/keyring-controller/tests/mocks/mockMutableKeyring.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { MockKeyring } from './mockKeyring'; - -export class MockMutableKeyring extends MockKeyring { - static type = 'Mock Mutable Keyring'; - - public type = 'Mock Mutable Keyring'; - - #updated: boolean = false; - - update() { - this.#updated = true; - } - - async serialize() { - return Promise.resolve({ - updated: this.#updated, - }); - } -} From 1ce3039c154812b4b97cc6e93e14f18efde8b436 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 2 May 2025 15:04:06 +0200 Subject: [PATCH 03/13] refactor: move conditional logic to #persistOrRollback --- .../src/KeyringController.test.ts | 36 ++++-- .../src/KeyringController.ts | 103 +++++++++++++----- 2 files changed, 102 insertions(+), 37 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index c207d07b14d..2b88da4a88c 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -4,6 +4,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 { Messenger } from '@metamask/base-controller'; +import type { HDKeyringState } from '@metamask/eth-hd-keyring'; import { HdKeyring } from '@metamask/eth-hd-keyring'; import { normalize, @@ -262,14 +263,26 @@ describe('KeyringController', () => { }); it('should throw error if the account is duplicated', async () => { - jest - .spyOn(HdKeyring.prototype, 'addAccounts') - .mockResolvedValue(['0x123']); - jest.spyOn(HdKeyring.prototype, 'getAccounts').mockReturnValue(['0x123']); + const mockAddress = '0x123'; + const addAccountsSpy = jest.spyOn(HdKeyring.prototype, 'addAccounts'); + const getAccountsSpy = jest.spyOn(HdKeyring.prototype, 'getAccounts'); + const serializeSpy = jest.spyOn(HdKeyring.prototype, 'serialize'); + + addAccountsSpy.mockResolvedValue([mockAddress]); + getAccountsSpy.mockReturnValue([mockAddress]); await withController(async ({ controller }) => { - jest - .spyOn(HdKeyring.prototype, 'getAccounts') - .mockReturnValue(['0x123', '0x123']); + getAccountsSpy.mockReturnValue([mockAddress, mockAddress]); + serializeSpy + .mockResolvedValueOnce({ + mnemonic: '', + numberOfAccounts: 1, + hdPath: "m/44'/60'/0'/0", + }) + .mockResolvedValueOnce({ + mnemonic: '', + numberOfAccounts: 2, + hdPath: "m/44'/60'/0'/0", + }); await expect(controller.addNewAccount()).rejects.toThrow( KeyringControllerError.DuplicatedAccount, ); @@ -315,6 +328,11 @@ describe('KeyringController', () => { MockShallowGetAccountsKeyring.type, )[0] as EthKeyring; + jest + .spyOn(mockKeyring, 'serialize') + .mockResolvedValueOnce({ numberOfAccounts: 1 }) + .mockResolvedValueOnce({ numberOfAccounts: 2 }); + const addedAccountAddress = await controller.addNewAccountForKeyring(mockKeyring); @@ -3119,7 +3137,9 @@ describe('KeyringController', () => { const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4'; stubKeyringClassWithAccount(MockKeyring, mockAddress); await withController( - { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + { + keyringBuilders: [keyringBuilderFactory(MockKeyring)], + }, async ({ controller, messenger }) => { const selector = { type: MockKeyring.type }; diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index f38dad4f83f..26f39ad0fb7 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -321,6 +321,13 @@ export type SerializedKeyring = { data: Json; }; +/** + * A serialized keyring and metadata object. + */ +export type SerializedKeyringAndMetadata = SerializedKeyring & { + metadata?: KeyringMetadata; +}; + /** * A generic encryptor interface that supports encrypting and decrypting * serializable data with a password. @@ -1029,7 +1036,8 @@ export class KeyringController extends BaseController< */ async persistAllKeyrings(): Promise { this.#assertIsUnlocked(); - return this.#persistOrRollback(async () => true); + + return this.#persistOrRollback(async () => true, { forceUpdate: true }); } /** @@ -1400,20 +1408,24 @@ export class KeyringController extends BaseController< */ changePassword(password: string): Promise { this.#assertIsUnlocked(); - return this.#persistOrRollback(async () => { - assertIsValidPassword(password); - this.#password = password; - // We need to clear encryption key and salt from state - // to force the controller to re-encrypt the vault using - // the new password. - if (this.#cacheEncryptionKey) { - this.update((state) => { - delete state.encryptionKey; - delete state.encryptionSalt; - }); - } - }); + return this.#persistOrRollback( + async () => { + assertIsValidPassword(password); + + this.#password = password; + // We need to clear encryption key and salt from state + // to force the controller to re-encrypt the vault using + // the new password. + if (this.#cacheEncryptionKey) { + this.update((state) => { + delete state.encryptionKey; + delete state.encryptionSalt; + }); + } + }, + { forceUpdate: true }, + ); } /** @@ -1552,9 +1564,8 @@ export class KeyringController extends BaseController< ): Promise { this.#assertIsUnlocked(); - return this.#withRollback(async () => { + return this.#persistOrRollback(async () => { let keyring: SelectedKeyring | undefined; - let forceUpdate = false; if ('address' in selector) { keyring = (await this.getKeyringForAccount(selector.address)) as @@ -1570,9 +1581,6 @@ export class KeyringController extends BaseController< selector.type, options.createWithData, )) as SelectedKeyring; - - // This is a new keyring, so we force the vault update in that case. - forceUpdate = true; } } else if ('id' in selector) { keyring = this.#getKeyringById(selector.id) as SelectedKeyring; @@ -1582,8 +1590,6 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.KeyringNotFound); } - const oldKeyringState = await keyring.serialize(); - const result = await operation({ keyring, metadata: this.#getKeyringMetadata(keyring), @@ -1597,11 +1603,6 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.UnsafeDirectKeyringAccess); } - if (forceUpdate || !isEqual(oldKeyringState, await keyring.serialize())) { - // Keyring has been updated, we need to update the vault. - await this.#updateVault(); - } - return result; }); } @@ -2158,6 +2159,38 @@ export class KeyringController extends BaseController< return serializedKeyrings; } + /** + * Serialize the current array of keyring instances and their metadata, + * including unsupported keyrings by default. + * + * @param options - Method options. + * @param options.includeUnsupported - Whether to include unsupported keyrings. + * @returns The serialized keyrings. + */ + async #getSerializedKeyringsAndMetadata( + { includeUnsupported }: { includeUnsupported: boolean } = { + includeUnsupported: true, + }, + ): Promise { + const serializedKeyrings = await this.#getSerializedKeyrings({ + includeUnsupported, + }); + + const serializedKeyringsAndMetadata: SerializedKeyringAndMetadata[] = + serializedKeyrings.map((serialized, index) => { + return { + ...serialized, + metadata: this.#keyringsMetadata[index], + }; + }); + + if (includeUnsupported) { + serializedKeyringsAndMetadata.push(...this.#unsupportedKeyrings); + } + + return serializedKeyringsAndMetadata; + } + /** * Restore a serialized keyrings array. * @@ -2629,19 +2662,31 @@ export class KeyringController extends BaseController< /** * Execute the given function after acquiring the controller lock - * and save the keyrings to state after it, or rollback to their + * and save the vault to state after it (only if needed), or rollback to their * previous state in case of error. * * @param callback - The function to execute. + * @param options - Options. + * @param options.forceUpdate - Force the vault update. * @returns The result of the function. */ async #persistOrRollback( callback: MutuallyExclusiveCallback, + { forceUpdate }: { forceUpdate: boolean } = { forceUpdate: false }, ): Promise { return this.#withRollback(async ({ releaseLock }) => { + const oldState = !forceUpdate + ? await this.#getSerializedKeyringsAndMetadata() + : []; // No need to serialize anything when forcing the update. const callbackResult = await callback({ releaseLock }); - // State is committed only if the operation is successful - await this.#updateVault(); + const newState = !forceUpdate + ? await this.#getSerializedKeyringsAndMetadata() + : []; // Same. + + // State is committed only if the operation is successful and need to trigger a vault update. + if (forceUpdate || !isEqual(oldState, newState)) { + await this.#updateVault(); + } return callbackResult; }); From 8536233cc06f96f367e2d601c4b0cf106de2f18d Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 2 May 2025 15:08:25 +0200 Subject: [PATCH 04/13] chore: update changelog --- packages/keyring-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 528a146eff2..ede9816a5e6 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Don't emit `:stateChange` from `withKeyring` unnecessarily ([#5732](https://github.com/MetaMask/core/pull/5732)) +- Prevent emitting `:stateChange` from `withKeyring` unnecessarily ([#5732](https://github.com/MetaMask/core/pull/5732)) - Bump `@metamask/base-controller` from ^8.0.0 to ^8.0.1 ([#5722](https://github.com/MetaMask/core/pull/5722)) ## [21.0.4] From 84c04e3f4ea3b02e9feca72e41861f2aaca6c31a Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 2 May 2025 17:50:55 +0200 Subject: [PATCH 05/13] chore: lint --- packages/keyring-controller/src/KeyringController.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 2b88da4a88c..7e777df40fa 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -4,7 +4,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 { Messenger } from '@metamask/base-controller'; -import type { HDKeyringState } from '@metamask/eth-hd-keyring'; import { HdKeyring } from '@metamask/eth-hd-keyring'; import { normalize, From c034474d74875e3607c0c7a071a9f76b662bb90f Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 14:23:21 +0200 Subject: [PATCH 06/13] refactor: forceUpdate -> alwaysUpdate --- .../keyring-controller/src/KeyringController.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 26f39ad0fb7..99362e9ded7 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1037,7 +1037,7 @@ export class KeyringController extends BaseController< async persistAllKeyrings(): Promise { this.#assertIsUnlocked(); - return this.#persistOrRollback(async () => true, { forceUpdate: true }); + return this.#persistOrRollback(async () => true, { alwaysUpdate: true }); } /** @@ -1424,7 +1424,7 @@ export class KeyringController extends BaseController< }); } }, - { forceUpdate: true }, + { alwaysUpdate: true }, ); } @@ -2667,24 +2667,26 @@ export class KeyringController extends BaseController< * * @param callback - The function to execute. * @param options - Options. - * @param options.forceUpdate - Force the vault update. + * @param options.alwaysUpdate - Always trigger vault update. * @returns The result of the function. */ async #persistOrRollback( callback: MutuallyExclusiveCallback, - { forceUpdate }: { forceUpdate: boolean } = { forceUpdate: false }, + { alwaysUpdate }: { alwaysUpdate: boolean } = { + alwaysUpdate: false, + }, ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const oldState = !forceUpdate + const oldState = !alwaysUpdate ? await this.#getSerializedKeyringsAndMetadata() : []; // No need to serialize anything when forcing the update. const callbackResult = await callback({ releaseLock }); - const newState = !forceUpdate + const newState = !alwaysUpdate ? await this.#getSerializedKeyringsAndMetadata() : []; // Same. // State is committed only if the operation is successful and need to trigger a vault update. - if (forceUpdate || !isEqual(oldState, newState)) { + if (alwaysUpdate || !isEqual(oldState, newState)) { await this.#updateVault(); } From cc963c1a2a3298675c6392692f00a3b966c3a0f6 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 14:29:12 +0200 Subject: [PATCH 07/13] fix: add SerializedKeyringAndMetadata.keyring --- packages/keyring-controller/src/KeyringController.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 99362e9ded7..492a01aa198 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -324,7 +324,8 @@ export type SerializedKeyring = { /** * A serialized keyring and metadata object. */ -export type SerializedKeyringAndMetadata = SerializedKeyring & { +export type SerializedKeyringAndMetadata = { + keyring: SerializedKeyring; metadata?: KeyringMetadata; }; @@ -2179,13 +2180,17 @@ export class KeyringController extends BaseController< const serializedKeyringsAndMetadata: SerializedKeyringAndMetadata[] = serializedKeyrings.map((serialized, index) => { return { - ...serialized, + keyring: serialized, metadata: this.#keyringsMetadata[index], }; }); if (includeUnsupported) { - serializedKeyringsAndMetadata.push(...this.#unsupportedKeyrings); + for (const unsupportedKeyring of this.#unsupportedKeyrings) { + serializedKeyringsAndMetadata.push({ + keyring: unsupportedKeyring, + }); + } } return serializedKeyringsAndMetadata; From 53e461ab7bddf53a3c4e9f8096d411ebfc4a4bdf Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 14:44:30 +0200 Subject: [PATCH 08/13] refactor: do not use withKeyring in persistAllKeyrings --- packages/keyring-controller/src/KeyringController.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 492a01aa198..a44758e449e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1036,9 +1036,12 @@ export class KeyringController extends BaseController< * operation completes. */ async persistAllKeyrings(): Promise { - this.#assertIsUnlocked(); + return this.#withRollback(async () => { + this.#assertIsUnlocked(); - return this.#persistOrRollback(async () => true, { alwaysUpdate: true }); + await this.#updateVault(); + return true; + }); } /** From b8f40fe1b0fc0e15855815773a73be831badfa39 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 14:53:22 +0200 Subject: [PATCH 09/13] refactor: get rid of alwaysUpdate --- .../src/KeyringController.ts | 92 ++++++------------- 1 file changed, 29 insertions(+), 63 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index a44758e449e..83aaabc56ad 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -322,11 +322,12 @@ export type SerializedKeyring = { }; /** - * A serialized keyring and metadata object. + * State/data that can be updated during a `withKeyring` operation. */ -export type SerializedKeyringAndMetadata = { - keyring: SerializedKeyring; - metadata?: KeyringMetadata; +type UpdatableState = { + keyrings: SerializedKeyring[]; + keyringsMetadata: KeyringMetadata[]; + password?: string; }; /** @@ -1413,23 +1414,20 @@ export class KeyringController extends BaseController< changePassword(password: string): Promise { this.#assertIsUnlocked(); - return this.#persistOrRollback( - async () => { - assertIsValidPassword(password); + return this.#persistOrRollback(async () => { + assertIsValidPassword(password); - this.#password = password; - // We need to clear encryption key and salt from state - // to force the controller to re-encrypt the vault using - // the new password. - if (this.#cacheEncryptionKey) { - this.update((state) => { - delete state.encryptionKey; - delete state.encryptionSalt; - }); - } - }, - { alwaysUpdate: true }, - ); + this.#password = password; + // We need to clear encryption key and salt from state + // to force the controller to re-encrypt the vault using + // the new password. + if (this.#cacheEncryptionKey) { + this.update((state) => { + delete state.encryptionKey; + delete state.encryptionSalt; + }); + } + }); } /** @@ -2164,39 +2162,16 @@ export class KeyringController extends BaseController< } /** - * Serialize the current array of keyring instances and their metadata, - * including unsupported keyrings by default. + * Get a snapshot of data that can be updated during a `withKeyring` operation. * - * @param options - Method options. - * @param options.includeUnsupported - Whether to include unsupported keyrings. - * @returns The serialized keyrings. + * @returns Snapshot of updatable data. */ - async #getSerializedKeyringsAndMetadata( - { includeUnsupported }: { includeUnsupported: boolean } = { - includeUnsupported: true, - }, - ): Promise { - const serializedKeyrings = await this.#getSerializedKeyrings({ - includeUnsupported, - }); - - const serializedKeyringsAndMetadata: SerializedKeyringAndMetadata[] = - serializedKeyrings.map((serialized, index) => { - return { - keyring: serialized, - metadata: this.#keyringsMetadata[index], - }; - }); - - if (includeUnsupported) { - for (const unsupportedKeyring of this.#unsupportedKeyrings) { - serializedKeyringsAndMetadata.push({ - keyring: unsupportedKeyring, - }); - } - } - - return serializedKeyringsAndMetadata; + async #getUpdatableState(): Promise { + return { + keyrings: await this.#getSerializedKeyrings(), + keyringsMetadata: this.#keyringsMetadata.slice(), // Force copy. + password: this.#password, + }; } /** @@ -2674,27 +2649,18 @@ export class KeyringController extends BaseController< * previous state in case of error. * * @param callback - The function to execute. - * @param options - Options. - * @param options.alwaysUpdate - Always trigger vault update. * @returns The result of the function. */ async #persistOrRollback( callback: MutuallyExclusiveCallback, - { alwaysUpdate }: { alwaysUpdate: boolean } = { - alwaysUpdate: false, - }, ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const oldState = !alwaysUpdate - ? await this.#getSerializedKeyringsAndMetadata() - : []; // No need to serialize anything when forcing the update. + const oldState = await this.#getUpdatableState(); const callbackResult = await callback({ releaseLock }); - const newState = !alwaysUpdate - ? await this.#getSerializedKeyringsAndMetadata() - : []; // Same. + const newState = await this.#getUpdatableState(); // State is committed only if the operation is successful and need to trigger a vault update. - if (alwaysUpdate || !isEqual(oldState, newState)) { + if (!isEqual(oldState, newState)) { await this.#updateVault(); } From 62375b2045fca23f52871bb879ea95ce39b41e08 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 16:57:27 +0200 Subject: [PATCH 10/13] fix: move comment back to its place --- 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 83aaabc56ad..0f90f5bc013 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -37,8 +37,8 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; -// When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. import { isEqual } from 'lodash'; +// When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. import { ulid } from 'ulid'; import { KeyringControllerError } from './constants'; From 7643139961409901c3e9759313996ff000d08a33 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 17:02:12 +0200 Subject: [PATCH 11/13] chore: #getUpdatableState -> #getControllerSessionState Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/src/KeyringController.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 0f90f5bc013..ca757228830 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2162,18 +2162,18 @@ export class KeyringController extends BaseController< } /** - * Get a snapshot of data that can be updated during a `withKeyring` operation. + * Get a snapshot of session data held by class variables. * - * @returns Snapshot of updatable data. + * @returns An object with serialized keyrings, keyrings metadata, + * and the user password. */ - async #getUpdatableState(): Promise { + async #getControllerSessionState(): Promise { return { keyrings: await this.#getSerializedKeyrings(), keyringsMetadata: this.#keyringsMetadata.slice(), // Force copy. password: this.#password, }; } - /** * Restore a serialized keyrings array. * From b84ab30f5f7272172147479a31e396d15d4b0276 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 17:04:40 +0200 Subject: [PATCH 12/13] refactor: use #getSessionState --- packages/keyring-controller/src/KeyringController.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index ca757228830..a221cd8c51b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -324,7 +324,7 @@ export type SerializedKeyring = { /** * State/data that can be updated during a `withKeyring` operation. */ -type UpdatableState = { +type SessionState = { keyrings: SerializedKeyring[]; keyringsMetadata: KeyringMetadata[]; password?: string; @@ -2167,13 +2167,14 @@ export class KeyringController extends BaseController< * @returns An object with serialized keyrings, keyrings metadata, * and the user password. */ - async #getControllerSessionState(): Promise { + async #getSessionState(): Promise { return { keyrings: await this.#getSerializedKeyrings(), keyringsMetadata: this.#keyringsMetadata.slice(), // Force copy. password: this.#password, }; } + /** * Restore a serialized keyrings array. * @@ -2655,9 +2656,9 @@ export class KeyringController extends BaseController< callback: MutuallyExclusiveCallback, ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const oldState = await this.#getUpdatableState(); + const oldState = await this.#getSessionState(); const callbackResult = await callback({ releaseLock }); - const newState = await this.#getUpdatableState(); + const newState = await this.#getSessionState(); // State is committed only if the operation is successful and need to trigger a vault update. if (!isEqual(oldState, newState)) { From bfd28116acbb4118be452fb0f4caa647df8f1d0a Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 5 May 2025 20:17:16 +0200 Subject: [PATCH 13/13] fix: fix changelog --- packages/keyring-controller/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 2102c8b420f..e33b66aaccb 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,11 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Prevent emitting `:stateChange` from `withKeyring` unnecessarily ([#5732](https://github.com/MetaMask/core/pull/5732)) + ## [21.0.5] ### Changed -- Prevent emitting `:stateChange` from `withKeyring` unnecessarily ([#5732](https://github.com/MetaMask/core/pull/5732)) - Bump `@metamask/base-controller` from ^8.0.0 to ^8.0.1 ([#5722](https://github.com/MetaMask/core/pull/5722)) ### Fixed