From ad125b1359b0c7c561a74bd7d50bfe7684382d2d Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Thu, 9 Apr 2026 15:01:43 +0200 Subject: [PATCH 1/7] feat(keyring-controller): persist vault when keyring state changes during unlock Introduces `hasStateChanged` helper and uses it to detect when a keyring's serialized state differs from what was stored in the vault. If any keyring changed during deserialization (e.g. ran a migration, or was missing metadata), the vault is re-persisted via the existing upgrade path in `submitPassword`/`submitEncryptionKey`. Also removes the now-redundant `isEqual` import from lodash. --- .../src/KeyringController.ts | 66 ++++++++++++------- 1 file changed, 42 insertions(+), 24 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9e2dc7212d..c4c6f6e1af 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -32,7 +32,7 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; -import { cloneDeep, isEqual } from 'lodash'; +import { cloneDeep } from 'lodash'; // When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. import { ulid } from 'ulid'; @@ -1526,7 +1526,7 @@ export class KeyringController< encryptionKey: string, encryptionSalt?: string, ): Promise { - const { newMetadata } = await this.#withRollback(async () => { + const { hasChanged } = await this.#withRollback(async () => { const result = await this.#unlockKeyrings({ encryptionKey, encryptionSalt, @@ -1539,7 +1539,7 @@ export class KeyringController< // if new metadata has been generated during login, we // can attempt to upgrade the vault. await this.#withRollback(async () => { - if (newMetadata) { + if (hasChanged) { await this.#updateVault(); } }); @@ -1572,7 +1572,7 @@ export class KeyringController< * @returns Promise resolving when the operation completes. */ async submitPassword(password: string): Promise { - const { newMetadata } = await this.#withRollback(async () => { + const { hasChanged } = await this.#withRollback(async () => { const result = await this.#unlockKeyrings({ password }); this.#setUnlocked(); return result; @@ -1580,10 +1580,10 @@ export class KeyringController< try { // If there are stronger encryption params available, or - // if new metadata has been generated during login, we + // if the keyring state has changed during deserialization, we // can attempt to upgrade the vault. await this.#withRollback(async () => { - if (newMetadata || this.#isNewEncryptionAvailable()) { + if (hasChanged || this.#isNewEncryptionAvailable()) { await this.#deriveAndSetEncryptionKey(password, { // If the vault is being upgraded, we want to ignore the metadata // that is already in the vault, so we can effectively @@ -2179,24 +2179,24 @@ export class KeyringController< serializedKeyrings: SerializedKeyring[], ): Promise<{ keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; - newMetadata: boolean; + hasChanged: boolean; }> { await this.#clearKeyrings(); const keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[] = []; - let newMetadata = false; + let hasChanged = false; for (const serializedKeyring of serializedKeyrings) { const result = await this.#restoreKeyring(serializedKeyring); if (result) { const { keyring, metadata } = result; keyrings.push({ keyring, metadata }); - if (result.newMetadata) { - newMetadata = true; + if (result.hasChanged) { + hasChanged = true; } } } - return { keyrings, newMetadata }; + return { keyrings, hasChanged }; } /** @@ -2217,7 +2217,7 @@ export class KeyringController< }, ): Promise<{ keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; - newMetadata: boolean; + hasChanged: boolean; }> { return this.#withVaultLock(async () => { if (!this.state.vault) { @@ -2255,7 +2255,7 @@ export class KeyringController< ); } - const { keyrings, newMetadata } = + const { keyrings, hasChanged } = await this.#restoreSerializedKeyrings(vault); const updatedKeyrings = await this.#getUpdatedKeyrings(); @@ -2266,7 +2266,7 @@ export class KeyringController< state.encryptionSalt = this.#encryptionKey?.salt; }); - return { keyrings, newMetadata }; + return { keyrings, hasChanged }; }); } @@ -2496,30 +2496,36 @@ export class KeyringController< async #restoreKeyring( serialized: SerializedKeyring, ): Promise< - | { keyring: EthKeyring; metadata: KeyringMetadata; newMetadata: boolean } + | { keyring: EthKeyring; metadata: KeyringMetadata; hasChanged: boolean } | undefined > { this.#assertControllerMutexIsLocked(); try { const { type, data, metadata: serializedMetadata } = serialized; - let newMetadata = false; - let metadata = serializedMetadata; + const keyring = await this.#createKeyring(type, data); + const restoredData = await keyring.serialize(); + let hasChanged = hasStateChanged(data, restoredData); + await this.#assertNoDuplicateAccounts([keyring]); - // If metadata is missing, assume the data is from an installation before - // we had keyring metadata. + + // If metadata is missing, assume the data is from an installation before we had + // keyring metadata. + let metadata = serializedMetadata; if (!metadata) { - newMetadata = true; + hasChanged = true; metadata = getDefaultKeyringMetadata(); } + // The keyring is added to the keyrings array only if it's successfully restored // and the metadata is successfully added to the controller this.#keyrings.push({ keyring, metadata, }); - return { keyring, metadata, newMetadata }; + + return { keyring, metadata, hasChanged }; } catch (error) { console.error(error); this.#unsupportedKeyrings.push(serialized); @@ -2598,12 +2604,12 @@ export class KeyringController< callback: MutuallyExclusiveCallback, ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const oldState = JSON.stringify(await this.#getSessionState()); + const prevState = await this.#getSessionState(); const callbackResult = await callback({ releaseLock }); - const newState = JSON.stringify(await this.#getSessionState()); + 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)) { + if (hasStateChanged(prevState, newState)) { await this.#updateVault(); } @@ -2719,4 +2725,16 @@ function getDefaultKeyringMetadata(): KeyringMetadata { return { id: ulid(), name: '' }; } +/** + * Check if the state of a keyring has changed by comparing the serialized state before + * and after an operation. + * + * @param before - The state of the keyring before the operation. + * @param after - The state of the keyring after the operation. + * @returns True if the state has changed, false otherwise. + */ +function hasStateChanged(before: Json, after: Json): boolean { + return JSON.stringify(before) !== JSON.stringify(after); +} + export default KeyringController; From 04147b29985ec4de3dee0323a4c4adf318a3d716 Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Thu, 9 Apr 2026 15:06:48 +0200 Subject: [PATCH 2/7] chore: update keyring-controller changelog --- packages/keyring-controller/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 4356dbe4b3..64f28db580 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Expose `KeyringController:signTransaction` method through `KeyringController` messenger ([#8408](https://github.com/MetaMask/core/pull/8408)) +- Persist vault when keyring state changes during unlock ([#8415](https://github.com/MetaMask/core/pull/8415)) + - If a keyring's serialized state differs after deserialization (e.g. a migration ran, or metadata was missing), the vault is now re-persisted so the change is not lost on the next unlock. ### Changed From ddf7bb6035dbfdcb4a854a5d913f9d3ed48cab64 Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Thu, 9 Apr 2026 15:12:36 +0200 Subject: [PATCH 3/7] chore: rename prevState -> oldState --- 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 c4c6f6e1af..6384d8d8b4 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2604,12 +2604,12 @@ export class KeyringController< callback: MutuallyExclusiveCallback, ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const prevState = await this.#getSessionState(); + const oldState = await this.#getSessionState(); const callbackResult = await callback({ releaseLock }); const newState = await this.#getSessionState(); // State is committed only if the operation is successful and need to trigger a vault update. - if (hasStateChanged(prevState, newState)) { + if (hasStateChanged(oldState, newState)) { await this.#updateVault(); } From 4f9cb62c33dc1d4846054fe242745152a7e615bf Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Thu, 9 Apr 2026 15:31:55 +0200 Subject: [PATCH 4/7] fix(keyring-controller): detect state changes from keyrings with shallow serialize() Eagerly stringify session state before the operation so that subsequent mutations to keyring-internal arrays (e.g. MockShallowKeyring) cannot retroactively change the snapshot used for comparison. Also removes the now-unnecessary `hasStateChanged` helper. --- .../src/KeyringController.ts | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 6384d8d8b4..7f7af5e952 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2506,7 +2506,7 @@ export class KeyringController< const keyring = await this.#createKeyring(type, data); const restoredData = await keyring.serialize(); - let hasChanged = hasStateChanged(data, restoredData); + let hasChanged = JSON.stringify(data) !== JSON.stringify(restoredData); await this.#assertNoDuplicateAccounts([keyring]); @@ -2604,12 +2604,12 @@ export class KeyringController< callback: MutuallyExclusiveCallback, ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const oldState = await this.#getSessionState(); + const oldState = JSON.stringify(await this.#getSessionState()); const callbackResult = await callback({ releaseLock }); - const newState = await this.#getSessionState(); + const newState = JSON.stringify(await this.#getSessionState()); // State is committed only if the operation is successful and need to trigger a vault update. - if (hasStateChanged(oldState, newState)) { + if (oldState !== newState) { await this.#updateVault(); } @@ -2725,16 +2725,5 @@ function getDefaultKeyringMetadata(): KeyringMetadata { return { id: ulid(), name: '' }; } -/** - * Check if the state of a keyring has changed by comparing the serialized state before - * and after an operation. - * - * @param before - The state of the keyring before the operation. - * @param after - The state of the keyring after the operation. - * @returns True if the state has changed, false otherwise. - */ -function hasStateChanged(before: Json, after: Json): boolean { - return JSON.stringify(before) !== JSON.stringify(after); -} export default KeyringController; From 0fd1697beb30602df191d889fbb2d0d4db61da73 Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Thu, 9 Apr 2026 15:32:54 +0200 Subject: [PATCH 5/7] chore: remove extra empty line --- packages/keyring-controller/src/KeyringController.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 7f7af5e952..26a8aae3e2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2725,5 +2725,4 @@ function getDefaultKeyringMetadata(): KeyringMetadata { return { id: ulid(), name: '' }; } - export default KeyringController; From 010a2d83b95fa650154ae1c30d65631f8c095a7d Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Fri, 10 Apr 2026 12:08:16 +0200 Subject: [PATCH 6/7] test(keyring-controller): use inline migrating keyring to test deserialization state change --- .../src/KeyringController.test.ts | 70 +++++++++++++++++++ tsconfig.packages.json | 1 + 2 files changed, 71 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 5ead061329..77937f1a57 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2945,6 +2945,76 @@ describe('KeyringController', () => { ); }); + it('should update the vault if the keyring state changes during deserialization', async () => { + const oldState = { version: 1, foo: 'bar' }; + const newState = { version: 2, foo: 'bar' }; + + // A keyring that migrates its own state during deserialization: + // after deserialize(oldState), serialize() returns newState. + class MigratingKeyring { + static type = 'Migrating Keyring'; + + type = 'Migrating Keyring'; + + #state: Record = {}; + + async serialize(): Promise> { + return this.#state; + } + + async deserialize(data: Record): Promise { + this.#state = data.version === 1 ? { ...data, version: 2 } : data; + } + + async getAccounts(): Promise { + return []; + } + + async addAccounts(): Promise { + return []; + } + } + + await withController( + { + skipVaultCreation: true, + state: { + vault: createVault([ + // #updateVault requires at least one HD keyring to be present. + { + type: KeyringTypes.hd, + data: {}, + metadata: { id: '0', name: '' }, + }, + { + type: MigratingKeyring.type, + data: oldState, + metadata: { id: '1', name: '' }, + }, + ]), + }, + keyringBuilders: [ + keyringBuilderFactory(MigratingKeyring as unknown as KeyringClass), + ], + }, + async ({ controller, encryptor }) => { + const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); + + await controller.submitPassword(password); + + expect(encryptWithKeySpy).toHaveBeenCalledWith( + defaultCredentials, + expect.arrayContaining([ + expect.objectContaining({ + type: MigratingKeyring.type, + data: newState, + }), + ]), + ); + }, + ); + }); + it('should unlock the wallet if the state has a duplicate account and the encryption parameters are outdated', async () => { stubKeyringClassWithAccount(MockKeyring, '0x123'); stubKeyringClassWithAccount(HdKeyring, '0x123'); diff --git a/tsconfig.packages.json b/tsconfig.packages.json index fbde19a598..f1b12a5d44 100644 --- a/tsconfig.packages.json +++ b/tsconfig.packages.json @@ -4,6 +4,7 @@ */ "extends": "./tsconfig.base.json", "compilerOptions": { + "types": ["jest"], /** * Here we ensure that TypeScript resolves `@metamask/*` imports to the * uncompiled source code for packages that live in this repo. From 6e7502642ba7c8f5a7cc2b674ff8bf88cd88869b Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Fri, 10 Apr 2026 13:13:30 +0200 Subject: [PATCH 7/7] fix(keyring-controller): snapshot data before deserialize to detect in-place mutations --- .../keyring-controller/src/KeyringController.test.ts | 11 ++++++++--- packages/keyring-controller/src/KeyringController.ts | 5 +++-- tsconfig.packages.json | 1 - 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 77937f1a57..b3977e6a4e 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2949,8 +2949,9 @@ describe('KeyringController', () => { const oldState = { version: 1, foo: 'bar' }; const newState = { version: 2, foo: 'bar' }; - // A keyring that migrates its own state during deserialization: - // after deserialize(oldState), serialize() returns newState. + // A keyring that migrates its own state in-place during deserialization: after + // deserialize(oldState), the original `data` object is mutated and serialize() + // returns the updated state. class MigratingKeyring { static type = 'Migrating Keyring'; @@ -2963,7 +2964,9 @@ describe('KeyringController', () => { } async deserialize(data: Record): Promise { - this.#state = data.version === 1 ? { ...data, version: 2 } : data; + // Mutate in-place to simulate a keyring that upgrades its own format. + data.version = 2; + this.#state = data; } async getAccounts(): Promise { @@ -3002,6 +3005,8 @@ describe('KeyringController', () => { await controller.submitPassword(password); + // Migration should have triggered a new vault update that we need to + // re-encrypt: expect(encryptWithKeySpy).toHaveBeenCalledWith( defaultCredentials, expect.arrayContaining([ diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 26a8aae3e2..49ff236709 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2504,9 +2504,10 @@ export class KeyringController< try { const { type, data, metadata: serializedMetadata } = serialized; + const oldState = JSON.stringify(data); const keyring = await this.#createKeyring(type, data); - const restoredData = await keyring.serialize(); - let hasChanged = JSON.stringify(data) !== JSON.stringify(restoredData); + const newState = JSON.stringify(await keyring.serialize()); + let hasChanged = oldState !== newState; await this.#assertNoDuplicateAccounts([keyring]); diff --git a/tsconfig.packages.json b/tsconfig.packages.json index f1b12a5d44..fbde19a598 100644 --- a/tsconfig.packages.json +++ b/tsconfig.packages.json @@ -4,7 +4,6 @@ */ "extends": "./tsconfig.base.json", "compilerOptions": { - "types": ["jest"], /** * Here we ensure that TypeScript resolves `@metamask/*` imports to the * uncompiled source code for packages that live in this repo.