From 3fe49867b7bc08de64841b6b37f8531f4f38e355 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 5 Dec 2025 18:38:15 +0100 Subject: [PATCH 01/14] feat: add KeyringController:accountAdded event + use keyring events in AccountsController --- .../src/AccountsController.ts | 210 +++++------------- .../src/KeyringController.ts | 152 +++++++++---- 2 files changed, 161 insertions(+), 201 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index dd0bd3c6a9e..32b564c55ec 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -19,9 +19,10 @@ import { } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; import type { - KeyringControllerState, KeyringControllerGetKeyringsByTypeAction, KeyringControllerStateChangeEvent, + KeyringControllerAccountAddedEvent, + KeyringControllerAccountRemovedEvent, KeyringControllerGetStateAction, KeyringObject, } from '@metamask/keyring-controller'; @@ -202,6 +203,8 @@ export type AccountsControllerAccountAssetListUpdatedEvent = { export type AllowedEvents = | SnapStateChange | KeyringControllerStateChangeEvent + | KeyringControllerAccountRemovedEvent + | KeyringControllerAccountAddedEvent | SnapKeyringAccountAssetListUpdatedEvent | SnapKeyringAccountBalancesUpdatedEvent | SnapKeyringAccountTransactionsUpdatedEvent @@ -506,7 +509,10 @@ export class AccountsController extends BaseController< ); } - #assertAccountCanBeRenamed(account: InternalAccount, accountName: string) { + #assertAccountCanBeRenamed( + account: InternalAccount, + accountName: string, + ): void { if ( this.listMultichainAccounts().find( (internalAccount) => @@ -748,168 +754,59 @@ export class AccountsController extends BaseController< this.messenger.publish(event, ...payload); } - /** - * Handles changes in the keyring state, specifically when new accounts are added or removed. - * - * @param keyringState - The new state of the keyring controller. - * @param keyringState.isUnlocked - True if the keyrings are unlocked, false otherwise. - * @param keyringState.keyrings - List of all keyrings. - */ - #handleOnKeyringStateChange({ - isUnlocked, - keyrings, - }: KeyringControllerState): void { - // TODO: Change when accountAdded event is added to the keyring controller. - - // We check for keyrings length to be greater than 0 because the extension client may try execute - // submit password twice and clear the keyring state. - // https://github.com/MetaMask/KeyringController/blob/2d73a4deed8d013913f6ef0c9f5c0bb7c614f7d3/src/KeyringController.ts#L910 - if (!isUnlocked || keyrings.length === 0) { - return; - } - - // State patches. - const generatePatch = () => { - return { - previous: {} as Record, - added: [] as { - address: string; - keyring: KeyringObject; - }[], - updated: [] as InternalAccount[], - removed: [] as InternalAccount[], - }; - }; - const patches = { - snap: generatePatch(), - normal: generatePatch(), - }; - - // Gets the patch object based on the keyring type (since Snap accounts and other accounts - // are handled differently). - const patchOf = (type: string) => { - if (isSnapKeyringType(type)) { - return patches.snap; - } - return patches.normal; - }; - - // Create a map (with lower-cased addresses) of all existing accounts. - for (const account of this.listMultichainAccounts()) { - const address = account.address.toLowerCase(); - const patch = patchOf(account.metadata.keyring.type); + #handleOnKeyringAccountAdded(address: string, keyring: KeyringObject) { + let account: InternalAccount | undefined; - patch.previous[address] = account; - } + this.#update((state) => { + const { internalAccounts } = state; - // Go over all keyring changes and create patches out of it. - const addresses = new Set(); - for (const keyring of keyrings) { - const patch = patchOf(keyring.type); + account = this.#getInternalAccountFromAddressAndType(address, keyring); + if (account) { + // Re-compute the list of accounts everytime, so we can make sure new names + // are also considered. + const accounts = Object.values( + internalAccounts.accounts, + ) as InternalAccount[]; - for (const accountAddress of keyring.accounts) { - // Lower-case address to use it in the `previous` map. - const address = accountAddress.toLowerCase(); - const account = patch.previous[address]; + // Get next account name available for this given keyring. + const name = this.getNextAvailableAccountName( + account.metadata.keyring.type, + accounts, + ); - if (account) { - // If the account exists before, this might be an update. - patch.updated.push(account); - } else { - // Otherwise, that's a new account. - patch.added.push({ - address, - keyring, - }); - } + // If it's the first account, we need to select it. + const lastSelected = + accounts.length === 0 ? this.#getLastSelectedIndex() : 0; - // Keep track of those address to check for removed accounts later. - addresses.add(address); + internalAccounts.accounts[account.id] = { + ...account, + metadata: { + ...account.metadata, + name, + importTime: Date.now(), + lastSelected, + }, + }; } - } + }); - // We might have accounts associated with removed keyrings, so we iterate - // over all previous known accounts and check against the keyring addresses. - for (const patch of [patches.snap, patches.normal]) { - for (const [address, account] of Object.entries(patch.previous)) { - // If a previous address is not part of the new addesses, then it got removed. - if (!addresses.has(address)) { - patch.removed.push(account); - } - } + if (account) { + this.messenger.publish('AccountsController:accountAdded', account); } + } - // Diff that we will use to publish events afterward. - const diff = { - removed: [] as string[], - added: [] as InternalAccount[], - }; + #handleOnKeyringAccountRemoved(address: string) { + const account = this.listMultichainAccounts().find( + ({ address: accountAddress }) => accountAddress === address, + ); - this.#update( - (state) => { + if (account) { + this.#update((state) => { const { internalAccounts } = state; - for (const patch of [patches.snap, patches.normal]) { - for (const account of patch.removed) { - delete internalAccounts.accounts[account.id]; - - diff.removed.push(account.id); - } - - for (const added of patch.added) { - const account = this.#getInternalAccountFromAddressAndType( - added.address, - added.keyring, - ); - - if (account) { - // Re-compute the list of accounts everytime, so we can make sure new names - // are also considered. - const accounts = Object.values( - internalAccounts.accounts, - ) as InternalAccount[]; - - // Get next account name available for this given keyring. - const name = this.getNextAvailableAccountName( - account.metadata.keyring.type, - accounts, - ); - - // If it's the first account, we need to select it. - const lastSelected = - accounts.length === 0 ? this.#getLastSelectedIndex() : 0; - - internalAccounts.accounts[account.id] = { - ...account, - metadata: { - ...account.metadata, - name, - importTime: Date.now(), - lastSelected, - }, - }; - - diff.added.push(internalAccounts.accounts[account.id]); - } - } - } - }, - // Will get executed after the update, but before re-selecting an account in case - // the current one is not valid anymore. - () => { - // Now publish events - for (const id of diff.removed) { - this.messenger.publish('AccountsController:accountRemoved', id); - } - - for (const account of diff.added) { - this.messenger.publish('AccountsController:accountAdded', account); - } - }, - ); - - // NOTE: Since we also track "updated" accounts with our patches, we could fire a new event - // like `accountUpdated` (we would still need to check if anything really changed on the account). + delete internalAccounts.accounts[account.id]; + }); + } } /** @@ -1215,8 +1112,13 @@ export class AccountsController extends BaseController< this.#handleOnSnapStateChange(snapStateState), ); - this.messenger.subscribe('KeyringController:stateChange', (keyringState) => - this.#handleOnKeyringStateChange(keyringState), + this.messenger.subscribe('KeyringController:accountRemoved', (address) => + this.#handleOnKeyringAccountRemoved(address), + ); + + this.messenger.subscribe( + 'KeyringController:accountAdded', + (address, keyring) => this.#handleOnKeyringAccountAdded(address, keyring), ); this.messenger.subscribe( diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 590a10552fc..3ac876c4fd7 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -18,6 +18,7 @@ import type { Messenger } from '@metamask/messenger'; import type { Eip1024EncryptedData, Hex, Json } from '@metamask/utils'; import { add0x, + assert, assertIsStrictHexString, bytesToHex, hasProperty, @@ -209,9 +210,14 @@ export type KeyringControllerStateChangeEvent = { payload: [KeyringControllerState, Patch[]]; }; +export type KeyringControllerAccountAddedEvent = { + type: `${typeof name}:accountAdded`; + payload: [string, KeyringObject]; +}; + export type KeyringControllerAccountRemovedEvent = { type: `${typeof name}:accountRemoved`; - payload: [string]; + payload: [string, KeyringObject]; }; export type KeyringControllerLockEvent = { @@ -250,6 +256,7 @@ export type KeyringControllerEvents = | KeyringControllerStateChangeEvent | KeyringControllerLockEvent | KeyringControllerUnlockEvent + | KeyringControllerAccountAddedEvent | KeyringControllerAccountRemovedEvent; export type KeyringControllerMessenger = Messenger< @@ -297,11 +304,11 @@ export type KeyringObject = { */ export type KeyringMetadata = { /** - * Keyring ID + * Keyring ID. */ id: string; /** - * Keyring name + * Keyring name. */ name: string; }; @@ -1205,13 +1212,15 @@ export class KeyringController< async removeAccount(address: string): Promise { this.#assertIsUnlocked(); + const keyringIndex = this.state.keyrings.findIndex((kr) => + kr.accounts.includes(address), + ); + const keyringObject = this.state.keyrings[keyringIndex]; + assert(keyringObject, 'Keyring object not found'); + await this.#persistOrRollback(async () => { const keyring = (await this.getKeyringForAccount(address)) as EthKeyring; - const keyringIndex = this.state.keyrings.findIndex((kr) => - kr.accounts.includes(address), - ); - const isPrimaryKeyring = keyringIndex === 0; const shouldRemoveKeyring = (await keyring.getAccounts()).length === 1; @@ -1237,7 +1246,7 @@ export class KeyringController< } }); - this.messenger.publish(`${name}:accountRemoved`, address); + this.messenger.publish(`${name}:accountRemoved`, address, keyringObject); } /** @@ -2233,53 +2242,102 @@ export class KeyringController< * * @returns A promise resolving to `true` if the operation is successful. */ - #updateVault(): Promise { - return this.#withVaultLock(async () => { - // Ensure no duplicate accounts are persisted. - await this.#assertNoDuplicateAccounts(); + async #updateVault(): Promise { + const updateVault = async () => + this.#withVaultLock(async () => { + // Ensure no duplicate accounts are persisted. + await this.#assertNoDuplicateAccounts(); - if (!this.#encryptionKey) { - throw new Error(KeyringControllerError.MissingCredentials); - } + if (!this.#encryptionKey) { + throw new Error(KeyringControllerError.MissingCredentials); + } - const serializedKeyrings = await this.#getSerializedKeyrings(); + const serializedKeyrings = await this.#getSerializedKeyrings(); - if ( - !serializedKeyrings.some( - (keyring) => keyring.type === (KeyringTypes.hd as string), - ) - ) { - throw new Error(KeyringControllerError.NoHdKeyring); - } + if ( + !serializedKeyrings.some( + (keyring) => keyring.type === (KeyringTypes.hd as string), + ) + ) { + throw new Error(KeyringControllerError.NoHdKeyring); + } - const key = await this.#encryptor.importKey( - this.#encryptionKey.serialized, - ); - const encryptedVault = await this.#encryptor.encryptWithKey( - key, - serializedKeyrings, - ); - // We need to include the salt used to derive - // the encryption key, to be able to derive it - // from password again. - encryptedVault.salt = this.#encryptionKey.salt; - const updatedState: Partial = { - vault: JSON.stringify(encryptedVault), - encryptionKey: this.#encryptionKey.serialized, - encryptionSalt: this.#encryptionKey.salt, - }; + const key = await this.#encryptor.importKey( + this.#encryptionKey.serialized, + ); + const encryptedVault = await this.#encryptor.encryptWithKey( + key, + serializedKeyrings, + ); + // We need to include the salt used to derive + // the encryption key, to be able to derive it + // from password again. + encryptedVault.salt = this.#encryptionKey.salt; + const updatedState: Partial = { + vault: JSON.stringify(encryptedVault), + encryptionKey: this.#encryptionKey.serialized, + encryptionSalt: this.#encryptionKey.salt, + }; - const updatedKeyrings = await this.#getUpdatedKeyrings(); + const updatedKeyrings = await this.#getUpdatedKeyrings(); - this.update((state) => { - state.vault = updatedState.vault; - state.keyrings = updatedKeyrings; - state.encryptionKey = updatedState.encryptionKey; - state.encryptionSalt = updatedState.encryptionSalt; + this.update((state) => { + state.vault = updatedState.vault; + state.keyrings = updatedKeyrings; + state.encryptionKey = updatedState.encryptionKey; + state.encryptionSalt = updatedState.encryptionSalt; + }); + + return true; }); - return true; - }); + // Keep track of the old keyring states so we can make a diff of added/removed accounts. + const oldKeyrings = this.state.keyrings; + const updated = await updateVault(); + const newKeyrings = this.state.keyrings; + + const oldAccounts = new Map(); + for (const oldKeyring of oldKeyrings) { + for (const oldAccount of oldKeyring.accounts) { + oldAccounts.set(oldAccount, oldKeyring); + } + } + + const newAccounts = new Map(); + for (const newKeyring of newKeyrings) { + for (const newAccount of newKeyring.accounts) { + newAccounts.set(newAccount, newKeyring); + } + } + + for (const newAccount of newAccounts.keys()) { + if (oldAccounts.has(newAccount)) { + // We remove accounts that intersects, this way we'll now what are the removed + // accounts (from `oldAccounts`) and newly added accounts (from `newAccounts`). + oldAccounts.delete(newAccount); + newAccounts.delete(newAccount); + } + } + + // Those accounts got removed, since they are not part of the new set of accounts. + oldAccounts.forEach((keyringEventInfo, oldAccount) => + this.messenger.publish( + 'KeyringController:accountRemoved', + oldAccount, + keyringEventInfo, + ), + ); + + // Those accounts got added, since they were not part of the old set of accounts. + newAccounts.forEach((keyringEventInfo, newAccount) => + this.messenger.publish( + 'KeyringController:accountAdded', + newAccount, + keyringEventInfo, + ), + ); + + return updated; } /** From b4700b2a1d701c0625bbaf1976a1fcbc5c385c66 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 8 Dec 2025 15:01:15 +0100 Subject: [PATCH 02/14] fix: add missing return types --- 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 3ac876c4fd7..05b72ac02e6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2243,8 +2243,8 @@ export class KeyringController< * @returns A promise resolving to `true` if the operation is successful. */ async #updateVault(): Promise { - const updateVault = async () => - this.#withVaultLock(async () => { + const updateVault = async (): Promise => + this.#withVaultLock(async (): Promise => { // Ensure no duplicate accounts are persisted. await this.#assertNoDuplicateAccounts(); From 537e3180bccbe925eaeceef7e6fbd3dcc3c41c76 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 8 Dec 2025 15:01:25 +0100 Subject: [PATCH 03/14] refactor: more DRY --- .../src/KeyringController.ts | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 05b72ac02e6..5daae954d7e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2291,24 +2291,26 @@ export class KeyringController< return true; }); + const toAccountsMap = ( + keyrings: KeyringObject[], + ): Map => { + const accounts = new Map(); + for (const keyring of keyrings) { + for (const account of keyring.accounts) { + accounts.set(account, keyring); + } + } + + return accounts; + }; + // Keep track of the old keyring states so we can make a diff of added/removed accounts. const oldKeyrings = this.state.keyrings; const updated = await updateVault(); const newKeyrings = this.state.keyrings; - const oldAccounts = new Map(); - for (const oldKeyring of oldKeyrings) { - for (const oldAccount of oldKeyring.accounts) { - oldAccounts.set(oldAccount, oldKeyring); - } - } - - const newAccounts = new Map(); - for (const newKeyring of newKeyrings) { - for (const newAccount of newKeyring.accounts) { - newAccounts.set(newAccount, newKeyring); - } - } + const oldAccounts = toAccountsMap(oldKeyrings); + const newAccounts = toAccountsMap(newKeyrings); for (const newAccount of newAccounts.keys()) { if (oldAccounts.has(newAccount)) { From e137a0b0467bcff1bb4b452919c38995353be2e6 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 8 Dec 2025 16:30:20 +0100 Subject: [PATCH 04/14] fix: revert change to :accountRemoved --- .../src/KeyringController.ts | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 5daae954d7e..b0a8cfc94d3 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -18,7 +18,6 @@ import type { Messenger } from '@metamask/messenger'; import type { Eip1024EncryptedData, Hex, Json } from '@metamask/utils'; import { add0x, - assert, assertIsStrictHexString, bytesToHex, hasProperty, @@ -217,7 +216,7 @@ export type KeyringControllerAccountAddedEvent = { export type KeyringControllerAccountRemovedEvent = { type: `${typeof name}:accountRemoved`; - payload: [string, KeyringObject]; + payload: [string]; }; export type KeyringControllerLockEvent = { @@ -1212,15 +1211,13 @@ export class KeyringController< async removeAccount(address: string): Promise { this.#assertIsUnlocked(); - const keyringIndex = this.state.keyrings.findIndex((kr) => - kr.accounts.includes(address), - ); - const keyringObject = this.state.keyrings[keyringIndex]; - assert(keyringObject, 'Keyring object not found'); - await this.#persistOrRollback(async () => { const keyring = (await this.getKeyringForAccount(address)) as EthKeyring; + const keyringIndex = this.state.keyrings.findIndex((kr) => + kr.accounts.includes(address), + ); + const isPrimaryKeyring = keyringIndex === 0; const shouldRemoveKeyring = (await keyring.getAccounts()).length === 1; @@ -1246,7 +1243,7 @@ export class KeyringController< } }); - this.messenger.publish(`${name}:accountRemoved`, address, keyringObject); + this.messenger.publish(`${name}:accountRemoved`, address); } /** @@ -2322,12 +2319,8 @@ export class KeyringController< } // Those accounts got removed, since they are not part of the new set of accounts. - oldAccounts.forEach((keyringEventInfo, oldAccount) => - this.messenger.publish( - 'KeyringController:accountRemoved', - oldAccount, - keyringEventInfo, - ), + oldAccounts.forEach((_, oldAccount) => + this.messenger.publish('KeyringController:accountRemoved', oldAccount), ); // Those accounts got added, since they were not part of the old set of accounts. From 1a2a27cf436c04c73630ccb7fd894e0b0d7b93b2 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 8 Dec 2025 16:30:36 +0100 Subject: [PATCH 05/14] refactor: keyringEventInfo -> keyringObject --- 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 b0a8cfc94d3..d46ae51badf 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2324,11 +2324,11 @@ export class KeyringController< ); // Those accounts got added, since they were not part of the old set of accounts. - newAccounts.forEach((keyringEventInfo, newAccount) => + newAccounts.forEach((keyringObject, newAccount) => this.messenger.publish( 'KeyringController:accountAdded', newAccount, - keyringEventInfo, + keyringObject, ), ); From e6eaaa1246b50a8e8c3915f4711702df79f27d23 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 8 Dec 2025 16:30:45 +0100 Subject: [PATCH 06/14] test: add new tests --- .../src/KeyringController.test.ts | 188 +++++++++++++++++- 1 file changed, 185 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 220f315d899..bfca8087aa4 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1243,16 +1243,59 @@ describe('KeyringController', () => { }); describe('persistAllKeyrings', () => { + const getPrimaryKeyringFrom = (controller: KeyringController) => { + return controller.getKeyringsByType(KeyringTypes.hd)[0] as EthKeyring; + }; + it('should reflect changes made directly to a keyring into the KeyringController state', async () => { await withController(async ({ controller }) => { - const primaryKeyring = controller.getKeyringsByType( - KeyringTypes.hd, - )[0] as EthKeyring; + const primaryKeyring = getPrimaryKeyringFrom(controller); + const [addedAccount] = await primaryKeyring.addAccounts(1); + + await controller.persistAllKeyrings(); + + expect(controller.state.keyrings[0].accounts[1]).toBe(addedAccount); + }); + }); + + it('should fire `:accountAdded` if a keyring got updated outside of a withKeyring call', async () => { + await withController(async ({ controller, messenger }) => { + const primaryKeyring = getPrimaryKeyringFrom(controller); const [addedAccount] = await primaryKeyring.addAccounts(1); + const mockAccountAdded = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + await controller.persistAllKeyrings(); + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount, + keyringObject, + ); + }); + }); + + it('should fire `:accountRemoved` if a keyring got updated outside of a withKeyring call', async () => { + await withController(async ({ controller, messenger }) => { + const primaryKeyring = getPrimaryKeyringFrom(controller); + + const [addedAccount] = await primaryKeyring.addAccounts(1); + await controller.persistAllKeyrings(); expect(controller.state.keyrings[0].accounts[1]).toBe(addedAccount); + + const mockAccountRemoved = jest.fn(); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + primaryKeyring.removeAccount?.(addedAccount); + await controller.persistAllKeyrings(); + + const removedAccount = addedAccount; + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); }); }); @@ -3417,6 +3460,125 @@ describe('KeyringController', () => { }); }); + it('should fire `:accountAdded` if an account gets added', async () => { + await withController(async ({ controller, messenger }) => { + const mockAccountAdded = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount, + keyringObject, + ); + }); + }); + + it('should fire multiple `:accountAdded` if multiple accounts get added', async () => { + await withController(async ({ controller, messenger }) => { + const mockAccountAdded = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + + const [addedAccount1, addedAccount2] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(2), + ); + expect(addedAccount1).toBeDefined(); + expect(addedAccount2).toBeDefined(); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount1, + keyringObject, + ); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount2, + keyringObject, + ); + }); + }); + + it('should fire `:accountRemoved` if an account gets removed', async () => { + await withController(async ({ controller, messenger }) => { + const mockAccountRemoved = jest.fn(); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => keyring.removeAccount?.(addedAccount), + ); + + const removedAccount = addedAccount; + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + }); + }); + + it('should fire `:accountAdded` and `:accountRemoved` if multiple operations get executed', async () => { + await withController(async ({ controller, messenger }) => { + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + const mockAccountAdded = jest.fn(); + const mockAccountRemoved = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + const removedAccount = addedAccount; + const [addedAccount2, addedAccount3] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => { + // We create more than 1 account, otherwise the serialized state would be the same between + // the old state and new state, thus, preventing the vault update. + const addedAccounts = await keyring.addAccounts(2); + + // We shouldn't be allowed to remove accounts out-of-order with HD keyrings, but that's ok + // for test purposes. + keyring.removeAccount?.(removedAccount); + + return addedAccounts; + }, + ); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 1, + addedAccount2, + keyringObject, + ); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 2, + addedAccount3, + keyringObject, + ); + }); + }); + describe('when the keyring is selected by type', () => { it('should call the given function with the selected keyring', async () => { await withController(async ({ controller }) => { @@ -4050,6 +4212,26 @@ describe('KeyringController', () => { }, ); }); + + it('should call withKeyring', async () => { + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller, messenger }) => { + await controller.addNewKeyring(MockKeyring.type); + + const actionReturnValue = await messenger.call( + 'KeyringController:withKeyring', + { type: MockKeyring.type }, + async ({ keyring }) => { + expect(keyring.type).toBe(MockKeyring.type); + return keyring.type; + }, + ); + + expect(actionReturnValue).toBe(MockKeyring.type); + }, + ); + }); }); describe('addNewKeyring', () => { From fe85881cfd56bdcce7d8822775ef5dce3509b8df Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 09:37:47 +0100 Subject: [PATCH 07/14] test: add more test for persistAllKeyrings + lint --- .../src/KeyringController.test.ts | 72 +++++++++++++------ 1 file changed, 51 insertions(+), 21 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index cb749986d76..50e8d5bb5fa 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1246,7 +1246,9 @@ describe('KeyringController', () => { }); describe('persistAllKeyrings', () => { - const getPrimaryKeyringFrom = (controller: KeyringController) => { + const getPrimaryKeyringFrom = ( + controller: KeyringController, + ): EthKeyring => { return controller.getKeyringsByType(KeyringTypes.hd)[0] as EthKeyring; }; @@ -1302,6 +1304,54 @@ describe('KeyringController', () => { }); }); + it('should fire `:accountAdded` and `:accountRemoved` if multiple operations get executed', async () => { + await withController(async ({ controller, messenger }) => { + const primaryKeyring = getPrimaryKeyringFrom(controller); + + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + const mockAccountAdded = jest.fn(); + const mockAccountRemoved = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + const removedAccount = addedAccount; + + // We create more than 1 account, otherwise the serialized state would be the same between + // the old state and new state, thus, preventing the vault update. + const [addedAccount2, addedAccount3] = + await primaryKeyring.addAccounts(2); + + // We shouldn't be allowed to remove accounts out-of-order with HD keyrings, but that's ok + // for test purposes. + primaryKeyring.removeAccount?.(removedAccount); + + // Now trigger persistence and vault update. + await controller.persistAllKeyrings(); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 1, + addedAccount2, + keyringObject, + ); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 2, + addedAccount3, + keyringObject, + ); + }); + }); + it('should throw error when locked', async () => { await withController(async ({ controller }) => { await controller.setLocked(); @@ -4221,26 +4271,6 @@ describe('KeyringController', () => { }, ); }); - - it('should call withKeyring', async () => { - await withController( - { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, - async ({ controller, messenger }) => { - await controller.addNewKeyring(MockKeyring.type); - - const actionReturnValue = await messenger.call( - 'KeyringController:withKeyring', - { type: MockKeyring.type }, - async ({ keyring }) => { - expect(keyring.type).toBe(MockKeyring.type); - return keyring.type; - }, - ); - - expect(actionReturnValue).toBe(MockKeyring.type); - }, - ); - }); }); describe('addNewKeyring', () => { From cab3f121ba11efd963367a0fced8ac7006065df7 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 11:31:54 +0100 Subject: [PATCH 08/14] refactor: re-adapt accounts-controller tests --- eslint-suppressions.json | 2 +- .../src/AccountsController.test.ts | 744 +++++------------- 2 files changed, 188 insertions(+), 558 deletions(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 8b0b1951eaa..9663ded4186 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -110,7 +110,7 @@ }, "packages/accounts-controller/src/AccountsController.test.ts": { "@typescript-eslint/explicit-function-return-type": { - "count": 15 + "count": 14 }, "import-x/namespace": { "count": 1 diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 34de9f40bd2..83946457a3f 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -263,7 +263,8 @@ function buildAccountsControllerMessenger(rootMessenger = buildMessenger()) { ], events: [ 'SnapController:stateChange', - 'KeyringController:stateChange', + 'KeyringController:accountAdded', + 'KeyringController:accountRemoved', 'SnapKeyring:accountAssetListUpdated', 'SnapKeyring:accountBalancesUpdated', 'SnapKeyring:accountTransactionsUpdated', @@ -565,136 +566,18 @@ describe('AccountsController', () => { }); }); - describe('onKeyringStateChange', () => { - it('uses listMultichainAccounts', async () => { - const messenger = buildMessenger(); - - mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: {}, - selectedAccount: '', - }, - }, - messenger, - }); - - const listMultichainAccountsSpy = jest.spyOn( - accountsController, - 'listMultichainAccounts', - ); - - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); - - expect(listMultichainAccountsSpy).toHaveBeenCalled(); - }); - - it('does not update state when only keyring is unlocked without any keyrings', async () => { - const messenger = buildMessenger(); - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: {}, - selectedAccount: '', - }, - }, - messenger, - }); - - messenger.publish( - 'KeyringController:stateChange', - { isUnlocked: true, keyrings: [] }, - [], - ); - - const accounts = accountsController.listMultichainAccounts(); - - expect(accounts).toStrictEqual([]); - }); - - it('only update if the keyring is unlocked and when there are keyrings', async () => { - const messenger = buildMessenger(); - - const mockNewKeyringState = { - isUnlocked: false, - keyrings: [ - { - accounts: [mockAccount.address, mockAccount2.address], - type: KeyringTypes.hd, - id: '123', - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: {}, - selectedAccount: '', - }, - }, - messenger, - }); - - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); - - const accounts = accountsController.listMultichainAccounts(); - - expect(accounts).toStrictEqual([]); - }); - + describe('on KeyringController:accountAdded', () => { describe('adding accounts', () => { it('add new accounts', async () => { const messenger = buildMessenger(); - mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { accounts: { [mockAccount.id]: mockAccount, - [mockAccount3.id]: mockAccount3, }, selectedAccount: mockAccount.id, }, @@ -702,10 +585,18 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address, mockAccount2.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount2.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -736,28 +627,6 @@ describe('AccountsController', () => { ]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - accounts: [mockAccount3.address, mockAccount4.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -771,10 +640,18 @@ describe('AccountsController', () => { messenger, }); + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address, mockAccount4.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + snapKeyring.accounts[0], + snapKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -811,28 +688,6 @@ describe('AccountsController', () => { ]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - accounts: [mockAccount3.address, mockAccount4.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -846,10 +701,18 @@ describe('AccountsController', () => { messenger, }); + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address, mockAccount4.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + snapKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -870,31 +733,6 @@ describe('AccountsController', () => { mockGetKeyringByType.mockReturnValue([]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - // Since the Snap keyring will be mocked as "unavailable", this account won't be added - // to the state (like if the Snap did remove it right before the keyring controller - // state change got triggered). - accounts: [mockAccount3.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -907,10 +745,21 @@ describe('AccountsController', () => { messenger, }); + // Since the Snap keyring will be mocked as "unavailable", this account won't be added + // to the state (like if the Snap did remove it right before the keyring controller + // state change got triggered). + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + snapKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -925,23 +774,6 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockAccount.address, - mockAccount2.address, - mockAccount3.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -955,10 +787,22 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [ + mockAccount.address, + mockAccount2.address, + mockAccount3.address, + ], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -994,23 +838,6 @@ describe('AccountsController', () => { lastSelected: 1955565967656, }); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockAccount.address, - mockAccount2.address, - mockAccount3.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1024,10 +851,22 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [ + mockAccount.address, + mockAccount2WithCustomName.address, + mockAccount3.address, + ], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -1058,33 +897,11 @@ describe('AccountsController', () => { mockGetKeyringByType.mockReturnValue([ { type: KeyringTypes.snap, - getAccountByAddress: jest.fn().mockReturnValueOnce(null), + getAccountByAddress: jest.fn().mockReturnValueOnce(mockAccount3), }, ]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - accounts: [mockAccount3.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1095,15 +912,22 @@ describe('AccountsController', () => { messenger, }); + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address], + metadata: { id: 'mock-id2', name: 'mock-name2' }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + snapKeyring, ); const { selectedAccount } = accountsController.state.internalAccounts; - expect(selectedAccount).toBe(''); + // The 'missing' account was not found, so the selectedAccount has been set to the + // newly added account automatically. + expect(selectedAccount).toBe(mockAccount3.id); }); it('selectedAccount remains the same after adding a new account', async () => { @@ -1111,25 +935,11 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { accounts: { [mockAccount.id]: mockAccount, - [mockAccount3.id]: mockAccount3, }, selectedAccount: mockAccount.id, }, @@ -1137,10 +947,18 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address, mockAccount2.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount2.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -1155,7 +973,7 @@ describe('AccountsController', () => { expect(accountsController.getSelectedAccount().id).toBe(mockAccount.id); }); - it('publishes accountAdded event', async () => { + it('publishes :accountAdded event', async () => { const messenger = buildMessenger(); mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); @@ -1174,24 +992,18 @@ describe('AccountsController', () => { const messengerSpy = jest.spyOn(accountsControllerMessenger, 'publish'); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address, mockAccount2.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }; - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount2.address, + keyring, ); // First call is 'AccountsController:stateChange' @@ -1213,19 +1025,6 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount2]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1240,9 +1039,8 @@ describe('AccountsController', () => { }); messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountRemoved', + mockAccount.address, ); const accounts = accountsController.listMultichainAccounts(); @@ -1260,19 +1058,6 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1295,11 +1080,7 @@ describe('AccountsController', () => { messenger, }); - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); + messenger.publish('KeyringController:accountRemoved', '0x999'); const accounts = accountsController.listMultichainAccounts(); @@ -1324,19 +1105,7 @@ describe('AccountsController', () => { lastSelected: undefined, }, }; - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; + const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1359,11 +1128,7 @@ describe('AccountsController', () => { messenger, }); - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); + messenger.publish('KeyringController:accountRemoved', '0x999'); const accounts = accountsController.listMultichainAccounts(); @@ -1398,22 +1163,6 @@ describe('AccountsController', () => { }, }; - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockAccountWithoutLastSelected.address, - mockAccount2.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1431,11 +1180,7 @@ describe('AccountsController', () => { messenger, }); - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); + messenger.publish('KeyringController:accountRemoved', '0x999'); const accounts = accountsController.listMultichainAccounts(); @@ -1456,7 +1201,7 @@ describe('AccountsController', () => { ); }); - it('publishes accountRemoved event', async () => { + it('publishes :accountRemoved event', async () => { const messenger = buildMessenger(); mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); @@ -1476,23 +1221,9 @@ describe('AccountsController', () => { const messengerSpy = jest.spyOn(accountsControllerMessenger, 'publish'); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountRemoved', + mockAccount3.address, ); // First call is 'AccountsController:stateChange' @@ -1526,19 +1257,6 @@ describe('AccountsController', () => { mockReinitialisedAccount, ]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockReinitialisedAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1551,10 +1269,22 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockReinitialisedAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountRemoved', + mockInitialAccount.address, + ); + messenger.publish( + 'KeyringController:accountAdded', + mockReinitialisedAccount.address, + keyring, ); const selectedAccount = accountsController.getSelectedAccount(); @@ -1570,92 +1300,6 @@ describe('AccountsController', () => { expect(accounts).toStrictEqual([expectedAccount]); }); - it.each([ - { - lastSelectedForAccount1: 1111, - lastSelectedForAccount2: 9999, - expectedSelectedId: 'mock-id2', - }, - { - lastSelectedForAccount1: undefined, - lastSelectedForAccount2: 9999, - expectedSelectedId: 'mock-id2', - }, - { - lastSelectedForAccount1: 1111, - lastSelectedForAccount2: undefined, - expectedSelectedId: 'mock-id', - }, - { - lastSelectedForAccount1: 1111, - lastSelectedForAccount2: 0, - expectedSelectedId: 'mock-id', - }, - ])( - 'handle keyring reinitialization with multiple accounts. Account 1 lastSelected $lastSelectedForAccount1, Account 2 lastSelected $lastSelectedForAccount2. Expected selected account: $expectedSelectedId', - async ({ - lastSelectedForAccount1, - lastSelectedForAccount2, - expectedSelectedId, - }) => { - const messenger = buildMessenger(); - const mockExistingAccount1 = createMockInternalAccount({ - id: 'mock-id', - name: 'Account 1', - address: '0x123', - keyringType: KeyringTypes.hd, - }); - mockExistingAccount1.metadata.lastSelected = lastSelectedForAccount1; - const mockExistingAccount2 = createMockInternalAccount({ - id: 'mock-id2', - name: 'Account 2', - address: '0x456', - keyringType: KeyringTypes.hd, - }); - mockExistingAccount2.metadata.lastSelected = lastSelectedForAccount2; - - mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: { - [mockExistingAccount1.id]: mockExistingAccount1, - [mockExistingAccount2.id]: mockExistingAccount2, - }, - selectedAccount: 'unknown', - }, - }, - messenger, - }); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockExistingAccount1.address, - mockExistingAccount2.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); - - const selectedAccount = accountsController.getSelectedAccount(); - - expect(selectedAccount.id).toStrictEqual(expectedSelectedId); - }, - ); - it('fires :accountAdded before :selectedAccountChange', async () => { const messenger = buildMessenger(); @@ -1671,20 +1315,6 @@ describe('AccountsController', () => { messenger, }); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - const mockEventsOrder = jest.fn(); messenger.subscribe('AccountsController:accountAdded', () => { @@ -1696,10 +1326,18 @@ describe('AccountsController', () => { expect(accountsController.getSelectedAccount()).toBe(EMPTY_ACCOUNT); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount.address, + keyring, ); expect(mockEventsOrder).toHaveBeenNthCalledWith( @@ -3453,30 +3091,6 @@ describe('AccountsController', () => { keyringType: KeyringTypes.simple, }); - const mockNewKeyringStateWith = (simpleAddressess: string[]) => { - return { - isUnlocked: true, - keyrings: [ - { - type: 'HD Key Tree', - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: 'Simple Key Pair', - accounts: simpleAddressess, - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - }; - it('return the next account number', async () => { const messenger = buildMessenger(); @@ -3498,13 +3112,23 @@ describe('AccountsController', () => { messenger, }); + const simpleKeyring = { + type: 'Simple Key Pair', + accounts: [mockSimpleKeyring1.address, mockSimpleKeyring2.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([ - mockSimpleKeyring1.address, - mockSimpleKeyring2.address, - ]), - [], + 'KeyringController:accountAdded', + mockSimpleKeyring1.address, + simpleKeyring, + ); + messenger.publish( + 'KeyringController:accountAdded', + mockSimpleKeyring2.address, + simpleKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -3537,31 +3161,37 @@ describe('AccountsController', () => { messenger, }); + const simpleKeyring = { + type: 'Simple Key Pair', + accounts: [mockSimpleKeyring1.address, mockSimpleKeyring2.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([ - mockSimpleKeyring1.address, - mockSimpleKeyring2.address, - ]), - [], + 'KeyringController:accountAdded', + mockSimpleKeyring1.address, + simpleKeyring, + ); + messenger.publish( + 'KeyringController:accountAdded', + mockSimpleKeyring2.address, + simpleKeyring, ); // We then remove "Acccount 2" to create a gap messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([mockSimpleKeyring2.address]), - [], + 'KeyringController:accountRemoved', + mockSimpleKeyring1.address, ); // Finally we add a 3rd account, and it should be named "Account 4" (despite having a gap // since "Account 2" no longer exists) messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([ - mockSimpleKeyring2.address, - mockSimpleKeyring3.address, - ]), - [], + 'KeyringController:accountAdded', + mockSimpleKeyring3.address, + simpleKeyring, ); const accounts = accountsController.listMultichainAccounts(); From fda91cd6b797bbe0b9b1380ee86abcf8fe23e665 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 11:32:08 +0100 Subject: [PATCH 09/14] fix: fix accounts-controller new event handling --- eslint-suppressions.json | 4 +- .../src/AccountsController.ts | 80 ++++++++++--------- 2 files changed, 44 insertions(+), 40 deletions(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 9663ded4186..d214399647a 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -118,7 +118,7 @@ }, "packages/accounts-controller/src/AccountsController.ts": { "@typescript-eslint/explicit-function-return-type": { - "count": 10 + "count": 9 } }, "packages/accounts-controller/src/tests/mocks.ts": { @@ -3305,4 +3305,4 @@ "count": 2 } } -} +} \ No newline at end of file diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 32b564c55ec..5c64430fa31 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -755,43 +755,47 @@ export class AccountsController extends BaseController< } #handleOnKeyringAccountAdded(address: string, keyring: KeyringObject) { - let account: InternalAccount | undefined; - - this.#update((state) => { - const { internalAccounts } = state; - - account = this.#getInternalAccountFromAddressAndType(address, keyring); - if (account) { - // Re-compute the list of accounts everytime, so we can make sure new names - // are also considered. - const accounts = Object.values( - internalAccounts.accounts, - ) as InternalAccount[]; - - // Get next account name available for this given keyring. - const name = this.getNextAvailableAccountName( - account.metadata.keyring.type, - accounts, - ); - - // If it's the first account, we need to select it. - const lastSelected = - accounts.length === 0 ? this.#getLastSelectedIndex() : 0; + let account = this.#getInternalAccountFromAddressAndType(address, keyring); + if (account) { + // Re-compute the list of accounts everytime, so we can make sure new names + // are also considered. + const accounts = Object.values(this.state.internalAccounts.accounts); + + // Get next account name available for this given keyring. + const name = this.getNextAvailableAccountName( + account.metadata.keyring.type, + accounts, + ); - internalAccounts.accounts[account.id] = { - ...account, - metadata: { - ...account.metadata, - name, - importTime: Date.now(), - lastSelected, - }, - }; - } - }); + // If it's the first account, we need to select it. + const lastSelected = + accounts.length === 0 ? this.#getLastSelectedIndex() : 0; + + // Update account metadata. + account = { + ...account, + metadata: { + ...account.metadata, + name, + importTime: Date.now(), + lastSelected, + }, + }; - if (account) { - this.messenger.publish('AccountsController:accountAdded', account); + // Not sure why, but the compiler still infers `account` as possibly undefined. + const internalAccount: InternalAccount = account; + this.#update( + (state) => { + state.internalAccounts.accounts[internalAccount.id] = internalAccount; + }, + () => { + // Will be published before `:selectedAccountChange` event is published. + this.messenger.publish( + 'AccountsController:accountAdded', + internalAccount, + ); + }, + ); } } @@ -802,10 +806,10 @@ export class AccountsController extends BaseController< if (account) { this.#update((state) => { - const { internalAccounts } = state; - - delete internalAccounts.accounts[account.id]; + delete state.internalAccounts.accounts[account.id]; }); + + this.messenger.publish('AccountsController:accountRemoved', account.id); } } From f7f52ce35f120c5cf9e93f5a05e315cbb697dfef Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 11:47:50 +0100 Subject: [PATCH 10/14] chore: lint --- eslint-suppressions.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index d214399647a..8d7daf846c8 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -3305,4 +3305,4 @@ "count": 2 } } -} \ No newline at end of file +} From 5fee36ee2f36f15c104cfd1268c016d068136210 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 11:52:01 +0100 Subject: [PATCH 11/14] chore: changelogs --- packages/accounts-controller/CHANGELOG.md | 3 +++ packages/keyring-controller/CHANGELOG.md | 1 + 2 files changed, 4 insertions(+) diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 8ba444bbe2b..bbef70493cd 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Use new `KeyringController:account{Added,Removed}` events instead of the `KeyringController:stateChange` ([#7328](https://github.com/MetaMask/core/pull/7328)) + - This simplify the re-synchronization between keyrings and this controller. + - This should also improve performance slightly. - Move peer dependencies for controller and service packages to direct dependencies ([#7209](https://github.com/MetaMask/core/pull/7209), [#7258](https://github.com/MetaMask/core/pull/7258)) - The dependencies moved are: - `@metamask/keyring-controller` (^25.0.0) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 4c695436dbf..10e785745c5 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 ### Added +- Added new `KeyringController:account{Added,Removed}` events ([#7328](https://github.com/MetaMask/core/pull/7328)) - Added new `KeyringBuilder` type ([#7334](https://github.com/MetaMask/core/pull/7334)) - Added an action to call `removeAccount` ([#7241](https://github.com/MetaMask/core/pull/7241)) - This action is meant to be consumed by the `MultichainAccountService` to encapsulate the act of removing a wallet when seed phrase backup fails in the clients. From 54d6e16d2f6a56f6539c2c87937274afd3877076 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 15:06:31 +0100 Subject: [PATCH 12/14] refactor: override update to cover all possible use-cases --- .../src/KeyringController.ts | 103 +++++++++++------- 1 file changed, 61 insertions(+), 42 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index d46ae51badf..dc69659b0fb 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -689,6 +689,15 @@ function normalize(address: string): string | undefined { return isEthAddress(address) ? ethNormalize(address) : address; } +/** + * Local type used when override `BaseController` methods. + */ +type KeyringBaseController = BaseController< + typeof name, + KeyringControllerState, + KeyringControllerMessenger +>; + /** * Controller responsible for establishing and managing user identity. * @@ -2240,54 +2249,64 @@ export class KeyringController< * @returns A promise resolving to `true` if the operation is successful. */ async #updateVault(): Promise { - const updateVault = async (): Promise => - this.#withVaultLock(async (): Promise => { - // Ensure no duplicate accounts are persisted. - await this.#assertNoDuplicateAccounts(); - - if (!this.#encryptionKey) { - throw new Error(KeyringControllerError.MissingCredentials); - } + return this.#withVaultLock(async (): Promise => { + // Ensure no duplicate accounts are persisted. + await this.#assertNoDuplicateAccounts(); - const serializedKeyrings = await this.#getSerializedKeyrings(); + if (!this.#encryptionKey) { + throw new Error(KeyringControllerError.MissingCredentials); + } - if ( - !serializedKeyrings.some( - (keyring) => keyring.type === (KeyringTypes.hd as string), - ) - ) { - throw new Error(KeyringControllerError.NoHdKeyring); - } + const serializedKeyrings = await this.#getSerializedKeyrings(); - const key = await this.#encryptor.importKey( - this.#encryptionKey.serialized, - ); - const encryptedVault = await this.#encryptor.encryptWithKey( - key, - serializedKeyrings, - ); - // We need to include the salt used to derive - // the encryption key, to be able to derive it - // from password again. - encryptedVault.salt = this.#encryptionKey.salt; - const updatedState: Partial = { - vault: JSON.stringify(encryptedVault), - encryptionKey: this.#encryptionKey.serialized, - encryptionSalt: this.#encryptionKey.salt, - }; + if ( + !serializedKeyrings.some( + (keyring) => keyring.type === (KeyringTypes.hd as string), + ) + ) { + throw new Error(KeyringControllerError.NoHdKeyring); + } - const updatedKeyrings = await this.#getUpdatedKeyrings(); + const key = await this.#encryptor.importKey( + this.#encryptionKey.serialized, + ); + const encryptedVault = await this.#encryptor.encryptWithKey( + key, + serializedKeyrings, + ); + // We need to include the salt used to derive + // the encryption key, to be able to derive it + // from password again. + encryptedVault.salt = this.#encryptionKey.salt; + const updatedState: Partial = { + vault: JSON.stringify(encryptedVault), + encryptionKey: this.#encryptionKey.serialized, + encryptionSalt: this.#encryptionKey.salt, + }; - this.update((state) => { - state.vault = updatedState.vault; - state.keyrings = updatedKeyrings; - state.encryptionKey = updatedState.encryptionKey; - state.encryptionSalt = updatedState.encryptionSalt; - }); + const updatedKeyrings = await this.#getUpdatedKeyrings(); - return true; + this.update((state) => { + state.vault = updatedState.vault; + state.keyrings = updatedKeyrings; + state.encryptionKey = updatedState.encryptionKey; + state.encryptionSalt = updatedState.encryptionSalt; }); + return true; + }); + } + + /** + * Overriden `BaseController.update` method to automatically emit events + * when accounts are added or removed from the keyrings. + * + * @param callback - The state update callback. + * @returns The return value of the `super.update` call. + */ + override update( + callback: Parameters[0], + ): ReturnType { const toAccountsMap = ( keyrings: KeyringObject[], ): Map => { @@ -2303,7 +2322,7 @@ export class KeyringController< // Keep track of the old keyring states so we can make a diff of added/removed accounts. const oldKeyrings = this.state.keyrings; - const updated = await updateVault(); + const result = super.update(callback); // This is the real update call. const newKeyrings = this.state.keyrings; const oldAccounts = toAccountsMap(oldKeyrings); @@ -2332,7 +2351,7 @@ export class KeyringController< ), ); - return updated; + return result; } /** From 81198db37e0e512a9df15e8f6470290de64a9533 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 15:19:05 +0100 Subject: [PATCH 13/14] chore: add missing jsdocs --- eslint-suppressions.json | 5 ----- .../accounts-controller/src/AccountsController.ts | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 9cc6f85d051..a0680b43825 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -108,11 +108,6 @@ "count": 1 } }, - "packages/accounts-controller/src/AccountsController.ts": { - "@typescript-eslint/explicit-function-return-type": { - "count": 2 - } - }, "packages/address-book-controller/src/AddressBookController.test.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 1 diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 1904cdd9e09..a3ebe8d4e74 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -754,7 +754,13 @@ export class AccountsController extends BaseController< this.messenger.publish(event, ...payload); } - #handleOnKeyringAccountAdded(address: string, keyring: KeyringObject) { + /** + * Handle the addition of a new account in a keyring. + * + * @param address - The address of the new account. + * @param keyring - The keyring object of that new account. + */ + #handleOnKeyringAccountAdded(address: string, keyring: KeyringObject): void { let account = this.#getInternalAccountFromAddressAndType(address, keyring); if (account) { // Re-compute the list of accounts everytime, so we can make sure new names @@ -799,7 +805,12 @@ export class AccountsController extends BaseController< } } - #handleOnKeyringAccountRemoved(address: string) { + /** + * Handle the removal of an existing account from a keyring. + * + * @param address - The address of the new account. + */ + #handleOnKeyringAccountRemoved(address: string): void { const account = this.listMultichainAccounts().find( ({ address: accountAddress }) => accountAddress === address, ); From 5009b0aee6aa8c2822dea3266e5e4dd43657a6f3 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 12 Dec 2025 15:25:29 +0100 Subject: [PATCH 14/14] fix: remove extra :accountRemoved --- 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 8233ea2cd28..800ffad8dc8 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1260,7 +1260,7 @@ export class KeyringController< } }); - this.messenger.publish(`${name}:accountRemoved`, address); + // `:accountRemoved` event is automatically emitted during `update` calls. } /**