From cce452ffdbd2950642fb18c4370bfff809ddc277 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Tue, 13 May 2025 11:09:55 +0200 Subject: [PATCH 1/3] fix: remove metadata for unsupported keyrings (#5725) ## Explanation When the user vault is decrypted and there is an attempt to restore an unsupported/deprecated/faulty keyring there's no mechanism to remove related metadata, which leads to a situation where no further action can be made on the controller, because checks for keyrings and metadata length will fail. We could remove the related metadata object when the keyring restore fails, but then we would lose the original ID generated for the keyring. We can, instead, change the place where the metadata is stored from a state property to the encrypted vault: by placing the metadata along with its serialised keyring in the vault we can guarantee a 1:1 link between them while being able to keep metadata for unsupported keyrings. Given that we don't need to use the KeyringController state to persist metadata anymore (as it is persisted along with the vault), we can also remove `keyringsMetadata` completely, and add a `metadata` attribute to each keyring in `state.keyrings` instead - which won't be persisted, as it will be recreated at runtime every time the vault is decrypted and the keyrings are deserialised. ## References * Fixes https://github.com/MetaMask/core/issues/5701 ## Changelog ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Mark Stacey --- eslint-warning-thresholds.json | 3 - .../src/AccountsController.test.ts | 335 +++++++------- .../src/AccountsController.ts | 6 +- packages/keyring-controller/CHANGELOG.md | 6 + packages/keyring-controller/jest.config.js | 6 +- .../src/KeyringController.test.ts | 415 +++++++++++++++--- .../src/KeyringController.ts | 214 +++++---- packages/keyring-controller/src/constants.ts | 1 - .../NotificationServicesController.test.ts | 26 +- .../src/PreferencesController.test.ts | 71 ++- .../UserStorageController.test.ts | 2 - .../__fixtures__/mockMessenger.ts | 1 - 12 files changed, 730 insertions(+), 356 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index e7eb5143ae5..feceaddb4ed 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -437,9 +437,6 @@ "packages/polling-controller/src/AbstractPollingController.ts": { "@typescript-eslint/prefer-readonly": 1 }, - "packages/preferences-controller/src/PreferencesController.test.ts": { - "prettier/prettier": 4 - }, "packages/queued-request-controller/src/QueuedRequestController.ts": { "@typescript-eslint/prefer-readonly": 2 }, diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 8a5fd21cbca..d0241c9be5e 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -501,12 +501,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -534,7 +532,7 @@ describe('AccountsController', () => { messenger.publish( 'KeyringController:stateChange', - { isUnlocked: true, keyrings: [], keyringsMetadata: [] }, + { isUnlocked: true, keyrings: [] }, [], ); @@ -553,12 +551,10 @@ describe('AccountsController', () => { accounts: [mockAccount.address, mockAccount2.address], type: KeyringTypes.hd, id: '123', - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -595,12 +591,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -654,20 +648,18 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', - }, - { - id: 'mock-id2', - name: 'mock-name2', + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, }, ], }; @@ -731,20 +723,18 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, { type: KeyringTypes.snap, accounts: [mockAccount3.address, mockAccount4.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', - }, - { - id: 'mock-id2', - name: 'mock-name2', + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, }, ], }; @@ -792,6 +782,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, { type: KeyringTypes.snap, @@ -799,16 +793,10 @@ describe('AccountsController', () => { // to the state (like if the Snap did remove it right before the keyring controller // state change got triggered). accounts: [mockAccount3.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', - }, - { - id: 'mock-id2', - name: 'mock-name2', + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, }, ], }; @@ -851,12 +839,10 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -919,12 +905,10 @@ describe('AccountsController', () => { mockAccount2.address, mockAccount3.address, ], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -982,20 +966,18 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, { type: KeyringTypes.snap, accounts: [mockAccount3.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', - }, - { - id: 'mock-id2', - name: 'mock-name2', + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, }, ], }; @@ -1032,12 +1014,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1093,12 +1073,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1132,12 +1110,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1179,12 +1155,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1245,12 +1219,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1324,12 +1296,10 @@ describe('AccountsController', () => { mockAccountWithoutLastSelected.address, mockAccount2.address, ], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1400,12 +1370,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAccount.address, mockAccount2.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1452,12 +1420,10 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockReinitialisedAccount.address], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1554,12 +1520,10 @@ describe('AccountsController', () => { mockExistingAccount1.address, mockExistingAccount2.address, ], - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, ], }; @@ -1799,12 +1763,13 @@ describe('AccountsController', () => { 'KeyringController:getState', mockGetState.mockReturnValue({ keyrings: [ - { type: KeyringTypes.hd, accounts: [mockAddress1, mockAddress2] }, - ], - keyringsMetadata: [ { - id: 'mock-keyring-id-0', - name: 'mock-keyring-id-name', + type: KeyringTypes.hd, + accounts: [mockAddress1, mockAddress2], + metadata: { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, }, ], }), @@ -1863,12 +1828,10 @@ describe('AccountsController', () => { { type: KeyringTypes.snap, accounts: [mockSnapAccount, mockSnapAccount2], - }, - ], - keyringsMetadata: [ - { - id: 'mock-keyring-id-1', - name: 'mock-keyring-id-name', + metadata: { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name', + }, }, ], }), @@ -1962,12 +1925,13 @@ describe('AccountsController', () => { 'KeyringController:getState', mockGetState.mockReturnValue({ keyrings: [ - { type: KeyringTypes.hd, accounts: [mockAddress1, mockAddress2] }, - ], - keyringsMetadata: [ { - id: 'mock-keyring-id-0', - name: 'mock-keyring-id-name', + type: KeyringTypes.hd, + accounts: [mockAddress1, mockAddress2], + metadata: { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, }, ], }), @@ -2035,17 +1999,21 @@ describe('AccountsController', () => { 'KeyringController:getState', mockGetState.mockReturnValue({ keyrings: [ - { type: KeyringTypes.hd, accounts: [mockAddress1] }, - { type: KeyringTypes.snap, accounts: ['0x1234'] }, - ], - keyringsMetadata: [ { - id: 'mock-keyring-id-0', - name: 'mock-keyring-id-name', + type: KeyringTypes.hd, + accounts: [mockAddress1], + metadata: { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, }, { - id: 'mock-keyring-id-1', - name: 'mock-keyring-id-name2', + type: KeyringTypes.snap, + accounts: ['0x1234'], + metadata: { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name2', + }, }, ], }), @@ -2103,17 +2071,21 @@ describe('AccountsController', () => { 'KeyringController:getState', mockGetState.mockReturnValue({ keyrings: [ - { type: KeyringTypes.snap, accounts: ['0x1234'] }, - { type: KeyringTypes.hd, accounts: [mockAddress1] }, - ], - keyringsMetadata: [ { - id: 'mock-keyring-id-0', - name: 'mock-keyring-id-name', + type: KeyringTypes.snap, + accounts: ['0x1234'], + metadata: { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, }, { - id: 'mock-keyring-id-1', - name: 'mock-keyring-id-name2', + type: KeyringTypes.hd, + accounts: [mockAddress1], + metadata: { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name2', + }, }, ], }), @@ -2168,11 +2140,14 @@ describe('AccountsController', () => { messenger.registerActionHandler( 'KeyringController:getState', mockGetState.mockReturnValue({ - keyrings: [{ type: keyringType, accounts: [mockAddress1] }], - keyringsMetadata: [ + keyrings: [ { - id: 'mock-keyring-id-0', - name: 'mock-keyring-id-name', + type: keyringType, + accounts: [mockAddress1], + metadata: { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, }, ], }), @@ -2312,17 +2287,21 @@ describe('AccountsController', () => { 'KeyringController:getState', mockGetState.mockReturnValue({ keyrings: [ - { type: KeyringTypes.snap, accounts: ['0x1234'] }, - { type: KeyringTypes.hd, accounts: [mockAddress1] }, - ], - keyringsMetadata: [ { - id: 'mock-keyring-id-1', - name: 'mock-keyring-id-name', + type: KeyringTypes.snap, + accounts: ['0x1234'], + metadata: { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name', + }, }, { - id: 'mock-keyring-id-2', - name: 'mock-keyring-id-name2', + type: KeyringTypes.hd, + accounts: [mockAddress1], + metadata: { + id: 'mock-keyring-id-2', + name: 'mock-keyring-id-name2', + }, }, ], }), @@ -3076,20 +3055,18 @@ describe('AccountsController', () => { { type: 'HD Key Tree', accounts: [mockAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }, { type: 'Simple Key Pair', accounts: simpleAddressess, - }, - ], - keyringsMetadata: [ - { - id: 'mock-id', - name: 'mock-name', - }, - { - id: 'mock-id2', - name: 'mock-name2', + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, }, ], }; diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 1abb03a64c9..7bf59e80e65 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -684,11 +684,11 @@ export class AccountsController extends BaseController< */ async #listNormalAccounts(): Promise { const internalAccounts: InternalAccount[] = []; - const { keyrings, keyringsMetadata } = this.messagingSystem.call( + const { keyrings } = this.messagingSystem.call( 'KeyringController:getState', ); - for (const [keyringIndex, keyring] of keyrings.entries()) { + for (const keyring of keyrings) { const keyringType = keyring.type; if (!isNormalKeyringType(keyringType as KeyringTypes)) { // We only consider "normal accounts" here, so keep looping @@ -702,7 +702,7 @@ export class AccountsController extends BaseController< if (isHdKeyringType(keyring.type as KeyringTypes)) { options = { - entropySource: keyringsMetadata[keyringIndex].id, + entropySource: keyring.metadata.id, // NOTE: We are not using the `hdPath` from the associated keyring here and // getting the keyring instance here feels a bit overkill. // This will be naturally fixed once every keyring start using `KeyringAccount` and implement the keyring API. diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index d6def954e41..fdd56be0a67 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING** `keyringsMetadata` has been removed from the controller state ([#5725](https://github.com/MetaMask/core/pull/5725)) + - The metadata is now stored in each keyring object in the `state.keyrings` array. + - When updating to this version, we recommend removing the `keyringsMetadata` state and all state referencing a keyring ID with a migration. New metadata will be generated for each keyring automatically after the update. + ## [21.0.6] ### Changed diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index d8355e87e91..0d930e38202 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 93.64, + branches: 94.25, functions: 100, - lines: 98.92, - statements: 98.93, + lines: 98.79, + statements: 98.8, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 26f09defe41..97be3333362 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -127,6 +127,95 @@ describe('KeyringController', () => { }, ); }); + + it('allows removing a keyring builder without bricking the wallet when metadata was already generated', async () => { + await withController( + { + skipVaultCreation: true, + state: { + vault: 'my vault', + }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: '', + metadata: { id: 'hd', name: '' }, + }, + { + type: 'Unsupported', + data: '', + metadata: { id: 'unsupported', name: '' }, + }, + { + type: KeyringTypes.hd, + data: '', + metadata: { id: 'hd2', name: '' }, + }, + ]); + + await controller.submitPassword(password); + + expect(controller.state.keyrings).toHaveLength(2); + expect(controller.state.keyrings[0].type).toBe(KeyringTypes.hd); + expect(controller.state.keyrings[0].metadata).toStrictEqual({ + id: 'hd', + name: '', + }); + expect(controller.state.keyrings[1].type).toBe(KeyringTypes.hd); + expect(controller.state.keyrings[1].metadata).toStrictEqual({ + id: 'hd2', + name: '', + }); + }, + ); + }); + + it('allows removing a keyring builder without bricking the wallet when metadata was not yet generated', async () => { + await withController( + { + skipVaultCreation: true, + state: { + vault: 'my vault', + }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: 'HD Key Tree', + data: '', + metadata: { id: 'hd', name: '' }, + }, + { + type: 'HD Key Tree', + data: '', + metadata: { id: 'hd2', name: '' }, + }, + // This keyring was already unsupported + // (no metadata, and is at the end of the array) + { + type: MockKeyring.type, + data: 'unsupported', + }, + ]); + + await controller.submitPassword(password); + + expect(controller.state.keyrings).toHaveLength(2); + expect(controller.state.keyrings[0].type).toBe(KeyringTypes.hd); + expect(controller.state.keyrings[0].metadata).toStrictEqual({ + id: 'hd', + name: '', + }); + expect(controller.state.keyrings[1].type).toBe(KeyringTypes.hd); + expect(controller.state.keyrings[1].metadata).toStrictEqual({ + id: 'hd2', + name: '', + }); + }, + ); + }); }); describe('addNewAccount', () => { @@ -467,7 +556,7 @@ describe('KeyringController', () => { { cacheEncryptionKey }, async ({ controller, initialState }) => { const initialVault = controller.state.vault; - const initialKeyringsMetadata = controller.state.keyringsMetadata; + const initialKeyrings = controller.state.keyrings; await controller.createNewVaultAndRestore( password, uint8ArraySeed, @@ -475,12 +564,12 @@ describe('KeyringController', () => { expect(controller.state).not.toBe(initialState); expect(controller.state.vault).toBeDefined(); expect(controller.state.vault).toStrictEqual(initialVault); - expect(controller.state.keyringsMetadata).toHaveLength( - initialKeyringsMetadata.length, + expect(controller.state.keyrings).toHaveLength( + initialKeyrings.length, ); // new keyring metadata should be generated - expect(controller.state.keyringsMetadata).not.toStrictEqual( - initialKeyringsMetadata, + expect(controller.state.keyrings).not.toStrictEqual( + initialKeyrings, ); }, ); @@ -507,6 +596,10 @@ describe('KeyringController', () => { { data: serializedKeyring, type: 'HD Key Tree', + metadata: { + id: expect.any(String), + name: '', + }, }, ]); }, @@ -769,7 +862,7 @@ describe('KeyringController', () => { it('should export seed phrase with valid keyringId', async () => { await withController(async ({ controller, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; + const keyringId = initialState.keyrings[0].metadata.id; const seed = await controller.exportSeedPhrase(password, keyringId); expect(seed).not.toBe(''); }); @@ -799,7 +892,7 @@ describe('KeyringController', () => { it('should throw invalid password error with valid keyringId', async () => { await withController( async ({ controller, encryptor, initialState }) => { - const keyringId = initialState.keyringsMetadata[0].id; + const keyringId = initialState.keyrings[0].metadata.id; jest .spyOn(encryptor, 'decrypt') .mockRejectedValueOnce(new Error('Invalid password')); @@ -1203,10 +1296,12 @@ describe('KeyringController', () => { ); const modifiedState = { ...initialState, - keyrings: [initialState.keyrings[0], newKeyring], - keyringsMetadata: [ - initialState.keyringsMetadata[0], - controller.state.keyringsMetadata[1], + keyrings: [ + initialState.keyrings[0], + { + ...newKeyring, + metadata: controller.state.keyrings[1].metadata, + }, ], }; expect(controller.state).toStrictEqual(modifiedState); @@ -1280,10 +1375,12 @@ describe('KeyringController', () => { }; const modifiedState = { ...initialState, - keyrings: [initialState.keyrings[0], newKeyring], - keyringsMetadata: [ - initialState.keyringsMetadata[0], - controller.state.keyringsMetadata[1], + keyrings: [ + initialState.keyrings[0], + { + ...newKeyring, + metadata: controller.state.keyrings[1].metadata, + }, ], }; expect(controller.state).toStrictEqual(modifiedState); @@ -1480,12 +1577,10 @@ describe('KeyringController', () => { await withController(async ({ controller }) => { await controller.addNewKeyring(KeyringTypes.hd); expect(controller.state.keyrings).toHaveLength(2); - expect(controller.state.keyringsMetadata).toHaveLength(2); await controller.removeAccount( controller.state.keyrings[1].accounts[0], ); expect(controller.state.keyrings).toHaveLength(1); - expect(controller.state.keyringsMetadata).toHaveLength(1); }); }); }); @@ -2635,10 +2730,12 @@ describe('KeyringController', () => { }); it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => { + // @ts-expect-error HdKeyring is not yet compatible with Keyring type. + stubKeyringClassWithAccount(HdKeyring, '0x123'); await withController( { cacheEncryptionKey, - state: { keyringsMetadata: [], vault: 'my vault' }, + state: { vault: 'my vault' }, skipVaultCreation: true, }, async ({ controller, encryptor }) => { @@ -2648,46 +2745,150 @@ describe('KeyringController', () => { data: { accounts: ['0x123'], }, + metadata: { + id: '123', + name: '', + }, }, ]); await controller.submitPassword(password); - expect(controller.state.keyringsMetadata).toHaveLength(1); - }, - ); - }); - - it('should throw an error when the controller is instantiated with an existing `keyringsMetadata` with too many objects', async () => { - await withController( - { - cacheEncryptionKey, - state: { - keyringsMetadata: [ - { id: '123', name: '' }, - { id: '456', name: '' }, - ], - vault: 'my vault', - }, - skipVaultCreation: true, - }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + expect(controller.state.keyrings).toStrictEqual([ { type: KeyringTypes.hd, - data: { - accounts: ['0x123'], + accounts: ['0x123'], + metadata: { + id: '123', + name: '', }, }, ]); - - await expect(controller.submitPassword(password)).rejects.toThrow( - KeyringControllerError.KeyringMetadataLengthMismatch, - ); }, ); }); + cacheEncryptionKey && + it('should generate new metadata when there is no metadata in the vault and cacheEncryptionKey is enabled', async () => { + const hdKeyringSerializeSpy = jest.spyOn( + HdKeyring.prototype, + 'serialize', + ); + await withController( + { + cacheEncryptionKey: true, + state: { + vault: 'my vault', + }, + skipVaultCreation: true, + }, + async ({ controller, encryptor }) => { + const encryptWithKeySpy = jest.spyOn( + encryptor, + 'encryptWithKey', + ); + jest + .spyOn(encryptor, 'importKey') + // @ts-expect-error we are assigning a mock value + .mockResolvedValue('imported key'); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]); + hdKeyringSerializeSpy.mockResolvedValue({ + // @ts-expect-error we are assigning a mock value + accounts: ['0x123'], + }); + + await controller.submitPassword(password); + + expect(controller.state.keyrings).toStrictEqual([ + { + type: KeyringTypes.hd, + accounts: expect.any(Array), + metadata: { + id: expect.any(String), + name: '', + }, + }, + ]); + expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: expect.any(String), + name: '', + }, + }, + ]); + }, + ); + }); + + !cacheEncryptionKey && + it('should generate new metadata when there is no metadata in the vault and cacheEncryptionKey is disabled', async () => { + const hdKeyringSerializeSpy = jest.spyOn( + HdKeyring.prototype, + 'serialize', + ); + await withController( + { + cacheEncryptionKey: false, + state: { + vault: 'my vault', + }, + skipVaultCreation: true, + }, + async ({ controller, encryptor }) => { + const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]); + hdKeyringSerializeSpy.mockResolvedValue({ + // @ts-expect-error we are assigning a mock value + accounts: ['0x123'], + }); + + await controller.submitPassword(password); + + expect(controller.state.keyrings).toStrictEqual([ + { + type: KeyringTypes.hd, + accounts: expect.any(Array), + metadata: { + id: expect.any(String), + name: '', + }, + }, + ]); + expect(encryptSpy).toHaveBeenCalledWith(password, [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: expect.any(String), + name: '', + }, + }, + ]); + }, + ); + }); + it('should unlock the wallet if the state has a duplicate account and the encryption parameters are outdated', async () => { stubKeyringClassWithAccount(MockKeyring, '0x123'); // @ts-expect-error HdKeyring is not yet compatible with Keyring type. @@ -2750,6 +2951,33 @@ describe('KeyringController', () => { ); }); + cacheEncryptionKey && + it('should not upgrade the vault encryption if the key encryptor has the same parameters', async () => { + await withController( + { + skipVaultCreation: true, + cacheEncryptionKey, + state: { vault: 'my vault' }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); + const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]); + + await controller.submitPassword(password); + + expect(encryptSpy).not.toHaveBeenCalled(); + }, + ); + }); + !cacheEncryptionKey && it('should upgrade the vault encryption if the generic encryptor has different parameters', async () => { await withController( @@ -2777,6 +3005,36 @@ describe('KeyringController', () => { ); }); + it('should not upgrade the vault encryption if the encryptor has the same parameters and the keyring has metadata', async () => { + await withController( + { + skipVaultCreation: true, + cacheEncryptionKey, + state: { vault: 'my vault' }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); + const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: '123', + name: '', + }, + }, + ]); + + await controller.submitPassword(password); + + expect(encryptSpy).not.toHaveBeenCalled(); + }, + ); + }); + !cacheEncryptionKey && it('should throw error if password is of wrong type', async () => { await withController( @@ -2864,6 +3122,57 @@ describe('KeyringController', () => { ); }); + it('should update the vault if new metadata is created while unlocking', async () => { + jest.spyOn(HdKeyring.prototype, 'serialize').mockResolvedValue({ + // @ts-expect-error we are assigning a mock value + accounts: ['0x123'], + }); + await withController( + { + cacheEncryptionKey: true, + skipVaultCreation: true, + state: { + vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + // @ts-expect-error we want to force the controller to have an + // encryption salt equal to the one in the vault + encryptionSalt: 'my salt', + }, + }, + async ({ controller, initialState, encryptor }) => { + const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest + .spyOn(encryptor, 'importKey') + // @ts-expect-error we are assigning a mock value + .mockResolvedValue('imported key'); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: '0x123', + }, + ]); + + await controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + initialState.encryptionSalt as string, + ); + + expect(controller.state.isUnlocked).toBe(true); + expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: expect.any(String), + name: '', + }, + }, + ]); + }, + ); + }); + it('should throw error if vault unlocked has an unexpected shape', async () => { await withController( { cacheEncryptionKey: true }, @@ -2930,7 +3239,7 @@ describe('KeyringController', () => { it('should return seedphrase for a specific keyring', async () => { await withController(async ({ controller }) => { const seedPhrase = await controller.verifySeedPhrase( - controller.state.keyringsMetadata[0].id, + controller.state.keyrings[0].metadata.id, ); expect(seedPhrase).toBeDefined(); }); @@ -2965,7 +3274,7 @@ describe('KeyringController', () => { await withController(async ({ controller }) => { await controller.addNewKeyring(KeyringTypes.simple, [privateKey]); - const keyringId = controller.state.keyringsMetadata[1].id; + const keyringId = controller.state.keyrings[1].metadata.id; await expect(controller.verifySeedPhrase(keyringId)).rejects.toThrow( KeyringControllerError.UnsupportedVerifySeedPhrase, ); @@ -3073,7 +3382,7 @@ describe('KeyringController', () => { const fn = jest.fn(); const selector = { type: KeyringTypes.hd }; const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0]; - const metadata = controller.state.keyringsMetadata[0]; + const { metadata } = controller.state.keyrings[0]; await controller.withKeyring(selector, fn); @@ -3211,7 +3520,7 @@ describe('KeyringController', () => { address: initialState.keyrings[0].accounts[0] as Hex, }; const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0]; - const metadata = controller.state.keyringsMetadata[0]; + const { metadata } = controller.state.keyrings[0]; await controller.withKeyring(selector, fn); @@ -3253,11 +3562,11 @@ describe('KeyringController', () => { describe('when the keyring is selected by id', () => { it('should call the given function with the selected keyring', async () => { - await withController(async ({ controller, initialState }) => { + await withController(async ({ controller }) => { const fn = jest.fn(); const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0]; - const selector = { id: initialState.keyringsMetadata[0].id }; - const metadata = controller.state.keyringsMetadata[0]; + const { metadata } = controller.state.keyrings[0]; + const selector = { id: metadata.id }; await controller.withKeyring(selector, fn); @@ -3268,7 +3577,7 @@ describe('KeyringController', () => { it('should return the result of the function', async () => { await withController(async ({ controller, initialState }) => { const fn = async () => Promise.resolve('hello'); - const selector = { id: initialState.keyringsMetadata[0].id }; + const selector = { id: initialState.keyrings[0].metadata.id }; expect(await controller.withKeyring(selector, fn)).toBe('hello'); }); @@ -3276,7 +3585,7 @@ describe('KeyringController', () => { it('should throw an error if the callback returns the selected keyring', async () => { await withController(async ({ controller, initialState }) => { - const selector = { id: initialState.keyringsMetadata[0].id }; + const selector = { id: initialState.keyrings[0].metadata.id }; await expect( controller.withKeyring(selector, async ({ keyring }) => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 97f95b90b09..b4147ae26b2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -22,7 +22,6 @@ import type { KeyringClass } from '@metamask/keyring-utils'; import type { Eip1024EncryptedData, Hex, Json } from '@metamask/utils'; import { add0x, - assert, assertIsStrictHexString, bytesToHex, hasProperty, @@ -92,10 +91,6 @@ export type KeyringControllerState = { * Representations of managed keyrings. */ keyrings: KeyringObject[]; - /** - * Metadata for each keyring. - */ - keyringsMetadata: KeyringMetadata[]; /** * The encryption key derived from the password and used to encrypt * the vault. This is only stored if the `cacheEncryptionKey` option @@ -278,6 +273,10 @@ export type KeyringObject = { * Keyring type. */ type: string; + /** + * Additional data associated with the keyring. + */ + metadata: KeyringMetadata; }; /** @@ -319,6 +318,7 @@ export enum SignTypedDataVersion { export type SerializedKeyring = { type: string; data: Json; + metadata?: KeyringMetadata; }; /** @@ -326,7 +326,6 @@ export type SerializedKeyring = { */ type SessionState = { keyrings: SerializedKeyring[]; - keyringsMetadata: KeyringMetadata[]; password?: string; }; @@ -482,7 +481,6 @@ export const getDefaultKeyringState = (): KeyringControllerState => { return { isUnlocked: false, keyrings: [], - keyringsMetadata: [], }; }; @@ -566,12 +564,18 @@ function isSerializedKeyringsArray( * * Is used for adding the current keyrings to the state object. * - * @param keyring - The keyring to display. + * @param keyringWithMetadata - The keyring and its metadata. + * @param keyringWithMetadata.keyring - The keyring to display. + * @param keyringWithMetadata.metadata - The metadata of the keyring. * @returns A keyring display object, with type and accounts properties. */ -async function displayForKeyring( - keyring: EthKeyring, -): Promise<{ type: string; accounts: string[] }> { +async function displayForKeyring({ + keyring, + metadata, +}: { + keyring: EthKeyring; + metadata: KeyringMetadata; +}): Promise { const accounts = await keyring.getAccounts(); return { @@ -579,6 +583,7 @@ async function displayForKeyring( // Cast to `string[]` here is safe here because `accounts` has no nullish // values, and `normalize` returns `string` unless given a nullish value accounts: accounts.map(normalize) as string[], + metadata, }; } @@ -638,12 +643,10 @@ export class KeyringController extends BaseController< readonly #cacheEncryptionKey: boolean; - #keyrings: EthKeyring[]; + #keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; #unsupportedKeyrings: SerializedKeyring[]; - #keyringsMetadata: KeyringMetadata[]; - #password?: string; #qrKeyringStateListener?: ( @@ -674,7 +677,6 @@ export class KeyringController extends BaseController< vault: { persist: true, anonymous: false }, isUnlocked: { persist: false, anonymous: true }, keyrings: { persist: false, anonymous: false }, - keyringsMetadata: { persist: true, anonymous: false }, encryptionKey: { persist: false, anonymous: false }, encryptionSalt: { persist: false, anonymous: false }, }, @@ -691,7 +693,6 @@ export class KeyringController extends BaseController< this.#encryptor = encryptor; this.#keyrings = []; - this.#keyringsMetadata = state?.keyringsMetadata?.slice() ?? []; this.#unsupportedKeyrings = []; // This option allows the controller to cache an exported key @@ -989,7 +990,7 @@ export class KeyringController extends BaseController< const address = normalize(account); const candidates = await Promise.all( - this.#keyrings.map(async (keyring) => { + this.#keyrings.map(async ({ keyring }) => { return Promise.all([keyring, keyring.getAccounts()]); }), ); @@ -1026,7 +1027,9 @@ export class KeyringController extends BaseController< */ getKeyringsByType(type: KeyringTypes | string): unknown[] { this.#assertIsUnlocked(); - return this.#keyrings.filter((keyring) => keyring.type === type); + return this.#keyrings + .filter(({ keyring }) => keyring.type === type) + .map(({ keyring }) => keyring); } /** @@ -1442,14 +1445,29 @@ export class KeyringController extends BaseController< encryptionKey: string, encryptionSalt: string, ): Promise { - return this.#withRollback(async () => { - this.#keyrings = await this.#unlockKeyrings( + const { newMetadata } = await this.#withRollback(async () => { + const result = await this.#unlockKeyrings( undefined, encryptionKey, encryptionSalt, ); this.#setUnlocked(); + return result; }); + + try { + // if new metadata has been generated during login, we + // can attempt to upgrade the vault. + await this.#withRollback(async () => { + if (newMetadata) { + await this.#updateVault(); + } + }); + } catch (error) { + // We don't want to throw an error if the upgrade fails + // since the controller is already unlocked. + console.error('Failed to update vault during login:', error); + } } /** @@ -1460,21 +1478,25 @@ export class KeyringController extends BaseController< * @returns Promise resolving when the operation completes. */ async submitPassword(password: string): Promise { - await this.#withRollback(async () => { - this.#keyrings = await this.#unlockKeyrings(password); + const { newMetadata } = await this.#withRollback(async () => { + const result = await this.#unlockKeyrings(password); this.#setUnlocked(); + return result; }); try { - // If there are stronger encryption params available, we + // If there are stronger encryption params available, or + // if new metadata has been generated during login, we // can attempt to upgrade the vault. - await this.#withRollback(async () => - this.#upgradeVaultEncryptionParams(), - ); + await this.#withRollback(async () => { + if (newMetadata || this.#isNewEncryptionAvailable()) { + await this.#updateVault(); + } + }); } catch (error) { // We don't want to throw an error if the upgrade fails // since the controller is already unlocked. - console.error('Failed to upgrade vault encryption params:', error); + console.error('Failed to update vault during login:', error); } } @@ -1949,10 +1971,8 @@ export class KeyringController extends BaseController< * @returns The keyring. */ #getKeyringById(keyringId: string): EthKeyring | undefined { - const index = this.state.keyringsMetadata.findIndex( - (metadata) => metadata.id === keyringId, - ); - return this.#keyrings[index]; + return this.#keyrings.find(({ metadata }) => metadata.id === keyringId) + ?.keyring; } /** @@ -1963,7 +1983,7 @@ export class KeyringController extends BaseController< */ #getKeyringByIdOrDefault(keyringId?: string): EthKeyring | undefined { if (!keyringId) { - return this.#keyrings[0] as EthKeyring; + return this.#keyrings[0]?.keyring; } return this.#getKeyringById(keyringId); @@ -1976,11 +1996,13 @@ export class KeyringController extends BaseController< * @returns The keyring metadata. */ #getKeyringMetadata(keyring: unknown): KeyringMetadata { - const index = this.#keyrings.findIndex( - (keyringCandidate) => keyringCandidate === keyring, + const keyringWithMetadata = this.#keyrings.find( + (candidate) => candidate.keyring === keyring, ); - assert(index !== -1, KeyringControllerError.KeyringNotFound); - return this.#keyringsMetadata[index]; + if (!keyringWithMetadata) { + throw new Error(KeyringControllerError.KeyringNotFound); + } + return keyringWithMetadata.metadata; } /** @@ -2072,7 +2094,6 @@ export class KeyringController extends BaseController< this.#password = password; await this.#clearKeyrings(); - this.#keyringsMetadata = []; await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); this.#setUnlocked(); } @@ -2156,13 +2177,13 @@ export class KeyringController extends BaseController< includeUnsupported: true, }, ): Promise { - const serializedKeyrings = await Promise.all( - this.#keyrings.map(async (keyring) => { - const [type, data] = await Promise.all([ - keyring.type, - keyring.serialize(), - ]); - return { type, data }; + const serializedKeyrings: SerializedKeyring[] = await Promise.all( + this.#keyrings.map(async ({ keyring, metadata }) => { + return { + type: keyring.type, + data: await keyring.serialize(), + metadata, + }; }), ); @@ -2182,7 +2203,6 @@ export class KeyringController extends BaseController< async #getSessionState(): Promise { return { keyrings: await this.#getSerializedKeyrings(), - keyringsMetadata: this.#keyringsMetadata.slice(), // Force copy. password: this.#password, }; } @@ -2191,15 +2211,30 @@ export class KeyringController extends BaseController< * Restore a serialized keyrings array. * * @param serializedKeyrings - The serialized keyrings array. + * @returns The restored keyrings. */ async #restoreSerializedKeyrings( serializedKeyrings: SerializedKeyring[], - ): Promise { + ): Promise<{ + keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; + newMetadata: boolean; + }> { await this.#clearKeyrings(); + const keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[] = []; + let newMetadata = false; for (const serializedKeyring of serializedKeyrings) { - await this.#restoreKeyring(serializedKeyring); + const result = await this.#restoreKeyring(serializedKeyring); + if (result) { + const { keyring, metadata } = result; + keyrings.push({ keyring, metadata }); + if (result.newMetadata) { + newMetadata = true; + } + } } + + return { keyrings, newMetadata }; } /** @@ -2215,7 +2250,10 @@ export class KeyringController extends BaseController< password: string | undefined, encryptionKey?: string, encryptionSalt?: string, - ): Promise { + ): Promise<{ + keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; + newMetadata: boolean; + }> { return this.#withVaultLock(async () => { const encryptedVault = this.state.vault; if (!encryptedVault) { @@ -2276,13 +2314,8 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.VaultDataError); } - await this.#restoreSerializedKeyrings(vault); - - // The keyrings array and the keyringsMetadata array should - // always have the same length while the controller is unlocked. - if (this.#keyrings.length !== this.#keyringsMetadata.length) { - throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); - } + const { keyrings, newMetadata } = + await this.#restoreSerializedKeyrings(vault); const updatedKeyrings = await this.#getUpdatedKeyrings(); @@ -2294,7 +2327,7 @@ export class KeyringController extends BaseController< } }); - return this.#keyrings; + return { keyrings, newMetadata }; }); } @@ -2366,14 +2399,10 @@ export class KeyringController extends BaseController< } const updatedKeyrings = await this.#getUpdatedKeyrings(); - if (updatedKeyrings.length !== this.#keyringsMetadata.length) { - throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); - } this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; - state.keyringsMetadata = this.#keyringsMetadata.slice(); if (updatedState.encryptionKey) { state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; @@ -2385,22 +2414,18 @@ export class KeyringController extends BaseController< } /** - * Upgrade the vault encryption parameters if needed. + * Check if there are new encryption parameters available. * * @returns A promise resolving to `void`. */ - async #upgradeVaultEncryptionParams(): Promise { - this.#assertControllerMutexIsLocked(); + #isNewEncryptionAvailable(): boolean { const { vault } = this.state; - if ( - vault && - this.#password && - this.#encryptor.isVaultUpdated && - !this.#encryptor.isVaultUpdated(vault) - ) { - await this.#updateVault(); + if (!vault || !this.#password || !this.#encryptor.isVaultUpdated) { + return false; } + + return !this.#encryptor.isVaultUpdated(vault); } /** @@ -2413,7 +2438,7 @@ export class KeyringController extends BaseController< const keyrings = this.#keyrings; const keyringArrays = await Promise.all( - keyrings.map(async (keyring) => keyring.getAccounts()), + keyrings.map(async ({ keyring }) => keyring.getAccounts()), ); const addresses = keyringArrays.reduce((res, arr) => { return res.concat(arr); @@ -2460,11 +2485,7 @@ export class KeyringController extends BaseController< async #newKeyring(type: string, data?: unknown): Promise { const keyring = await this.#createKeyring(type, data); - if (this.#keyrings.length !== this.#keyringsMetadata.length) { - throw new Error('Keyring metadata missing'); - } - this.#keyrings.push(keyring); - this.#keyringsMetadata.push(getDefaultKeyringMetadata()); + this.#keyrings.push({ keyring, metadata: getDefaultKeyringMetadata() }); return keyring; } @@ -2536,7 +2557,7 @@ export class KeyringController extends BaseController< */ async #clearKeyrings() { this.#assertControllerMutexIsLocked(); - for (const keyring of this.#keyrings) { + for (const { keyring } of this.#keyrings) { await this.#destroyKeyring(keyring); } this.#keyrings = []; @@ -2552,22 +2573,30 @@ export class KeyringController extends BaseController< */ async #restoreKeyring( serialized: SerializedKeyring, - ): Promise { + ): Promise< + | { keyring: EthKeyring; metadata: KeyringMetadata; newMetadata: boolean } + | undefined + > { this.#assertControllerMutexIsLocked(); try { - const { type, data } = serialized; + const { type, data, metadata: serializedMetadata } = serialized; + let newMetadata = false; + let metadata = serializedMetadata; const keyring = await this.#createKeyring(type, data); // If metadata is missing, assume the data is from an installation before // we had keyring metadata. - if (this.#keyringsMetadata.length <= this.#keyrings.length) { - console.log(`Adding missing metadata for '${type}' keyring`); - this.#keyringsMetadata.push(getDefaultKeyringMetadata()); + if (!metadata) { + newMetadata = 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); - return keyring; + this.#keyrings.push({ + keyring, + metadata, + }); + return { keyring, metadata, newMetadata }; } catch (error) { console.error(error); this.#unsupportedKeyrings.push(serialized); @@ -2596,26 +2625,24 @@ export class KeyringController extends BaseController< */ async #removeEmptyKeyrings(): Promise { this.#assertControllerMutexIsLocked(); - const validKeyrings: EthKeyring[] = []; - const validKeyringMetadata: KeyringMetadata[] = []; + const validKeyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[] = + []; // Since getAccounts returns a Promise // We need to wait to hear back form each keyring // in order to decide which ones are now valid (accounts.length > 0) await Promise.all( - this.#keyrings.map(async (keyring: EthKeyring, index: number) => { + this.#keyrings.map(async ({ keyring, metadata }) => { const accounts = await keyring.getAccounts(); if (accounts.length > 0) { - validKeyrings.push(keyring); - validKeyringMetadata.push(this.#keyringsMetadata[index]); + validKeyrings.push({ keyring, metadata }); } else { await this.#destroyKeyring(keyring); } }), ); this.#keyrings = validKeyrings; - this.#keyringsMetadata = validKeyringMetadata; } /** @@ -2642,11 +2669,6 @@ export class KeyringController extends BaseController< this.update((state) => { state.isUnlocked = true; - // If new keyringsMetadata was generated during the unlock operation, - // we'll have to update the state with the new array - if (this.#keyringsMetadata.length > state.keyringsMetadata.length) { - state.keyringsMetadata = this.#keyringsMetadata.slice(); - } }); this.messagingSystem.publish(`${name}:unlock`); } @@ -2700,13 +2722,11 @@ export class KeyringController extends BaseController< return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); const currentPassword = this.#password; - const currentKeyringsMetadata = this.#keyringsMetadata.slice(); try { return await callback({ releaseLock }); } catch (e) { // Keyrings and password are restored to their previous state - this.#keyringsMetadata = currentKeyringsMetadata; this.#password = currentPassword; await this.#restoreSerializedKeyrings(currentSerializedKeyrings); diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index abf89373050..d914a3d6f74 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -35,6 +35,5 @@ export enum KeyringControllerError { DataType = 'KeyringController - Incorrect data type provided', NoHdKeyring = 'KeyringController - No HD Keyring found', ControllerLockRequired = 'KeyringController - attempt to update vault during a non mutually exclusive operation', - KeyringMetadataLengthMismatch = 'KeyringController - keyring metadata length mismatch', LastAccountInPrimaryKeyring = 'KeyringController - Last account in primary keyring cannot be removed', } diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts index 7445390462b..66e037c08a1 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts @@ -141,8 +141,16 @@ describe('metamask-notifications - init()', () => { const act = async (addresses: string[], assertion: () => void) => { mockKeyringControllerGetState.mockReturnValue({ isUnlocked: true, - keyrings: [{ accounts: addresses, type: KeyringTypes.hd }], - keyringsMetadata: [], + keyrings: [ + { + accounts: addresses, + type: KeyringTypes.hd, + metadata: { + id: '123', + name: '', + }, + }, + ], }); await actPublishKeyringStateChange(globalMessenger, addresses); @@ -270,7 +278,6 @@ describe('metamask-notifications - init()', () => { mocks.mockKeyringControllerGetState.mockReturnValue({ isUnlocked: false, // Wallet Locked keyrings: [], - keyringsMetadata: [], }); }); @@ -357,7 +364,6 @@ describe('metamask-notifications - init()', () => { mocks.mockKeyringControllerGetState.mockReturnValue({ isUnlocked: false, keyrings: [], - keyringsMetadata: [], }); }); expect(mockKeyringControllerGetState).toHaveBeenCalledTimes(1); @@ -952,8 +958,16 @@ describe('metamask-notifications - enableMetamaskNotifications()', () => { messengerMocks.mockKeyringControllerGetState.mockReturnValue({ isUnlocked: true, - keyrings: [{ accounts: [ADDRESS_1], type: KeyringTypes.hd }], - keyringsMetadata: [], + keyrings: [ + { + accounts: [ADDRESS_1], + type: KeyringTypes.hd, + metadata: { + id: '123', + name: '', + }, + }, + ], }); return { ...messengerMocks, mockCreateOnChainTriggers }; diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index 30ff3ea64e8..002d5adefbe 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -31,10 +31,13 @@ describe('PreferencesController', () => { useMultiRpcMigration: true, showIncomingTransactions: Object.values( ETHERSCAN_SUPPORTED_CHAIN_IDS, - ).reduce((acc, curr) => { - acc[curr] = true; - return acc; - }, {} as { [chainId in EtherscanSupportedHexChainId]: boolean }), + ).reduce( + (acc, curr) => { + acc[curr] = true; + return acc; + }, + {} as { [chainId in EtherscanSupportedHexChainId]: boolean }, + ), smartTransactionsOptInStatus: true, useSafeChainsListValidation: true, tokenSortConfig: { @@ -69,6 +72,10 @@ describe('PreferencesController', () => { { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, }, ], }, @@ -111,7 +118,16 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00'], + type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, + }, + ], }, [], ); @@ -141,7 +157,16 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00'], + type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, + }, + ], }, [], ); @@ -170,7 +195,16 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: [], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: [], + type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, + }, + ], }, [], ); @@ -203,6 +237,10 @@ describe('PreferencesController', () => { { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, }, ], }, @@ -237,10 +275,18 @@ describe('PreferencesController', () => { { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, }, { accounts: ['0x00', '0x01', '0x02'], type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, }, ], }, @@ -271,7 +317,16 @@ describe('PreferencesController', () => { 'KeyringController:stateChange', { ...getDefaultKeyringState(), - keyrings: [{ accounts: ['0x00', '0x01'], type: 'CustomKeyring' }], + keyrings: [ + { + accounts: ['0x00', '0x01'], + type: 'CustomKeyring', + metadata: { + id: 'mock-id', + name: '', + }, + }, + ], }, [], ); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index eb01a865423..c8b7aefeaf9 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -995,7 +995,6 @@ describe('user-storage/user-storage-controller - snap handling', () => { messengerMocks.mockKeyringGetState.mockReturnValue({ isUnlocked: false, keyrings: [], - keyringsMetadata: [], }); const controller = new UserStorageController({ messenger: messengerMocks.messenger, @@ -1012,7 +1011,6 @@ describe('user-storage/user-storage-controller - snap handling', () => { messengerMocks.mockKeyringGetState.mockReturnValue({ isUnlocked: true, keyrings: [], - keyringsMetadata: [], }); const controller = new UserStorageController({ diff --git a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts index 4a1a3a606b6..eeb9f4861f6 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts @@ -132,7 +132,6 @@ export function mockUserStorageMessenger( ).mockReturnValue({ isUnlocked: true, keyrings: [], - keyringsMetadata: [], }); const mockAccountsListAccounts = jest.fn(); From 42e8f5cccc3a763af066fd89e361684907294569 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 13 May 2025 11:48:05 +0200 Subject: [PATCH 2/3] fix: misplaced changelog entry for `@metamask/profile-sync-controller` (#5788) ## Explanation This PR moves a changelog entry from **13.0.0** to **Unreleased** for `@metamask/profile-sync-controller`. This entry was mistakenly placed in an already released version's changelog. ## References ## Changelog ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- packages/profile-sync-controller/CHANGELOG.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index bf6e1148b01..b110058a6a0 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,12 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## [13.0.0] - ### Changed - **BREAKING:** Replace all "Profile Syncing" mentions to "Backup & Sync" ([#5686](https://github.com/MetaMask/core/pull/5686)) - Replaces state properties `isProfileSyncingEnabled` to `isBackupAndSyncEnabled`, and `isProfileSyncingUpdateLoading` to `isBackupAndSyncUpdateLoading` + +## [13.0.0] + +### Changed + - **BREAKING:** Bump `@metamask/accounts-controller` peer dependency from `^27.0.0` to `^28.0.0` ([#5763](https://github.com/MetaMask/core/pull/5763)) - **BREAKING:** Bump `@metamask/snaps-controllers` peer dependency from `^9.19.0` to `^11.0.0` ([#5639](https://github.com/MetaMask/core/pull/5639)) - **BREAKING:** Bump `@metamask/providers` peer dependency from `^18.1.1` to `^21.0.0` ([#5639](https://github.com/MetaMask/core/pull/5639)) From 2136c32e0a381c2f8a96cf8bff125346bc584bb6 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 13 May 2025 13:03:37 +0200 Subject: [PATCH 3/3] Release 393.0.0 (#5789) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Explanation This is a RC for v393.0.0. See changelog for more details - `@metamask/profile-sync-controller@14.0.0` ## References Instructions for client migration are in these test drive PRs: - ✅ Extension test drive PR: https://github.com/MetaMask/metamask-extension/pull/32572 - ✅ Mobile test drive PR: https://github.com/MetaMask/metamask-mobile/pull/15211 ## Changelog ```ms ### Changed - Bump `@metamask/profile-sync-controller` from `^13.0.0` to `^14.0.0` ([#5789](https://github.com/MetaMask/core/pull/5789)) ``` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- package.json | 2 +- packages/notification-services-controller/CHANGELOG.md | 2 ++ packages/notification-services-controller/package.json | 4 ++-- packages/profile-sync-controller/CHANGELOG.md | 9 ++++++++- packages/profile-sync-controller/package.json | 2 +- yarn.lock | 6 +++--- 6 files changed, 17 insertions(+), 8 deletions(-) diff --git a/package.json b/package.json index b9f75c1df6f..405e47a1f32 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/core-monorepo", - "version": "392.0.0", + "version": "393.0.0", "private": true, "description": "Monorepo for packages shared between MetaMask clients", "repository": { diff --git a/packages/notification-services-controller/CHANGELOG.md b/packages/notification-services-controller/CHANGELOG.md index 05fa1da69c5..a022d672ae0 100644 --- a/packages/notification-services-controller/CHANGELOG.md +++ b/packages/notification-services-controller/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Bump peer dependency `@metamask/profile-sync-controller` to `^14.0.0` ([#5789](https://github.com/MetaMask/core/pull/5789)) + - While `@metamask/profile-sync-controller@14.0.0` contains breaking changes for clients, they are not breaking as a peer dependency here as the changes do not impact `@metamask/notification-services-controller` - replaced `KeyringController:withKeyring` with `KeyringController:getState` to get the first HD keyring for notifications ([#5764](https://github.com/MetaMask/core/pull/5764)) - Bump `@metamask/controller-utils` to `^11.8.0` ([#5765](https://github.com/MetaMask/core/pull/5765)) diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index fe24169af4c..8265b598a22 100644 --- a/packages/notification-services-controller/package.json +++ b/packages/notification-services-controller/package.json @@ -123,7 +123,7 @@ "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/keyring-controller": "^21.0.6", - "@metamask/profile-sync-controller": "^13.0.0", + "@metamask/profile-sync-controller": "^14.0.0", "@types/jest": "^27.4.1", "@types/readable-stream": "^2.3.0", "contentful": "^10.15.0", @@ -138,7 +138,7 @@ }, "peerDependencies": { "@metamask/keyring-controller": "^21.0.0", - "@metamask/profile-sync-controller": "^13.0.0" + "@metamask/profile-sync-controller": "^14.0.0" }, "engines": { "node": "^18.18 || >=20" diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index b110058a6a0..276d196210b 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,11 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [14.0.0] + ### Changed - **BREAKING:** Replace all "Profile Syncing" mentions to "Backup & Sync" ([#5686](https://github.com/MetaMask/core/pull/5686)) - Replaces state properties `isProfileSyncingEnabled` to `isBackupAndSyncEnabled`, and `isProfileSyncingUpdateLoading` to `isBackupAndSyncUpdateLoading` +### Fixed + +- Remove metadata for unsupported keyrings ([#5725](https://github.com/MetaMask/core/pull/5725)) + ## [13.0.0] ### Changed @@ -580,7 +586,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial release -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@13.0.0...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@14.0.0...HEAD +[14.0.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@13.0.0...@metamask/profile-sync-controller@14.0.0 [13.0.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@12.0.0...@metamask/profile-sync-controller@13.0.0 [12.0.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@11.0.1...@metamask/profile-sync-controller@12.0.0 [11.0.1]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@11.0.0...@metamask/profile-sync-controller@11.0.1 diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index 5ab32c29695..cc85a94cb4f 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/profile-sync-controller", - "version": "13.0.0", + "version": "14.0.0", "description": "The profile sync helps developers synchronize data across multiple clients and devices in a privacy-preserving way. All data saved in the user storage database is encrypted client-side to preserve privacy. The user storage provides a modular design, giving developers the flexibility to construct and manage their storage spaces in a way that best suits their needs", "keywords": [ "MetaMask", diff --git a/yarn.lock b/yarn.lock index a8f68979192..ca8679c48bc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3873,7 +3873,7 @@ __metadata: "@metamask/base-controller": "npm:^8.0.1" "@metamask/controller-utils": "npm:^11.8.0" "@metamask/keyring-controller": "npm:^21.0.6" - "@metamask/profile-sync-controller": "npm:^13.0.0" + "@metamask/profile-sync-controller": "npm:^14.0.0" "@metamask/utils": "npm:^11.2.0" "@types/jest": "npm:^27.4.1" "@types/readable-stream": "npm:^2.3.0" @@ -3892,7 +3892,7 @@ __metadata: uuid: "npm:^8.3.2" peerDependencies: "@metamask/keyring-controller": ^21.0.0 - "@metamask/profile-sync-controller": ^13.0.0 + "@metamask/profile-sync-controller": ^14.0.0 languageName: unknown linkType: soft @@ -4054,7 +4054,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/profile-sync-controller@npm:^13.0.0, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": +"@metamask/profile-sync-controller@npm:^14.0.0, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": version: 0.0.0-use.local resolution: "@metamask/profile-sync-controller@workspace:packages/profile-sync-controller" dependencies: