From 417b9e962a5af3cf221cef9c7f8c8a4db0c96af3 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 12:15:16 +0100 Subject: [PATCH 01/10] test: do not clear other empty keyrings --- .../src/KeyringController.test.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 7cf33dff3f8..1d4b9cbfd33 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1645,6 +1645,35 @@ describe('KeyringController', () => { expect(controller.state.keyrings).toHaveLength(1); }); }); + + it('should not remove other empty keyrings when removing an account', async () => { + await withController(async ({ controller }) => { + // Import an account, creating a Simple keyring with 1 account + const importedAccount = await controller.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + + // Add an empty Simple keyring (no accounts) + await controller.addNewKeyring(KeyringTypes.simple); + + // We now have: 1 HD keyring + 1 Simple keyring (with account) + 1 empty Simple keyring = 3 keyrings + expect(controller.state.keyrings).toHaveLength(3); + expect(controller.state.keyrings[1].accounts).toStrictEqual([ + importedAccount, + ]); + expect(controller.state.keyrings[2].accounts).toStrictEqual([]); + + // Remove the imported account (empties the first Simple keyring) + await controller.removeAccount(importedAccount); + + // Only the targeted keyring should be removed, the other empty Simple keyring should remain + expect(controller.state.keyrings).toHaveLength(2); + expect(controller.state.keyrings[0].type).toBe(KeyringTypes.hd); + expect(controller.state.keyrings[1].type).toBe(KeyringTypes.simple); + expect(controller.state.keyrings[1].accounts).toStrictEqual([]); + }); + }); }); describe('when the keyring for the given address does not support removeAccount', () => { From d7eecedec79c5905f730a2628e90f107b051c50d Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 12:02:40 +0100 Subject: [PATCH 02/10] remove empty keyring instead of removing all empty keyrings --- .../src/KeyringController.ts | 31 ++----------------- 1 file changed, 2 insertions(+), 29 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 1c9e8ac5cd7..23f1943d374 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1268,7 +1268,8 @@ export class KeyringController< keyring.removeAccount(address as Hex); if (shouldRemoveKeyring) { - await this.#removeEmptyKeyrings(); + this.#keyrings.splice(keyringIndex, 1); + await this.#destroyKeyring(keyring); } }); @@ -2576,34 +2577,6 @@ export class KeyringController< await keyring.destroy?.(); } - /** - * Remove empty keyrings. - * - * Loops through the keyrings and removes the ones with empty accounts - * (usually after removing the last / only account) from a keyring. - */ - async #removeEmptyKeyrings(): Promise { - this.#assertControllerMutexIsLocked(); - 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, metadata }) => { - const accounts = await keyring.getAccounts(); - if (accounts.length > 0) { - validKeyrings.push({ keyring, metadata }); - } else { - await this.#destroyKeyring(keyring); - } - }), - ); - this.#keyrings = validKeyrings; - } - /** * Assert that there are no duplicate accounts in the keyrings. * From 696cf58333e9574083fc6f918cd0914bd003cd10 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 12:25:19 +0100 Subject: [PATCH 03/10] test: await async keyring.removeAccount --- .../src/KeyringController.test.ts | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 1d4b9cbfd33..2c2ad6cc169 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1674,6 +1674,59 @@ describe('KeyringController', () => { expect(controller.state.keyrings[1].accounts).toStrictEqual([]); }); }); + + it('should await an async removeAccount method before removing the keyring', async () => { + const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19'; + + // Track async operation state + let removeAccountCompleted = false; + let keyringCountDuringRemove: number | undefined; + + // Create a mock keyring class with an async removeAccount + class AsyncRemoveAccountKeyring extends MockKeyring { + static override type = 'Async Remove Account Keyring'; + + override type = 'Async Remove Account Keyring'; + + removeAccount = jest.fn(async () => { + // Simulate async operation with a delay + await new Promise((resolve) => setTimeout(resolve, 10)); + removeAccountCompleted = true; + }); + } + + stubKeyringClassWithAccount(AsyncRemoveAccountKeyring, address); + + await withController( + { + keyringBuilders: [keyringBuilderFactory(AsyncRemoveAccountKeyring)], + }, + async ({ controller, messenger }) => { + await controller.addNewKeyring(AsyncRemoveAccountKeyring.type); + expect(controller.state.keyrings).toHaveLength(2); + + // Subscribe to state changes to capture timing + messenger.subscribe('KeyringController:stateChange', () => { + // Record keyring count when state changes and removeAccount hasn't completed yet + if ( + !removeAccountCompleted && + keyringCountDuringRemove === undefined + ) { + keyringCountDuringRemove = controller.state.keyrings.length; + } + }); + + await controller.removeAccount(address); + + // Verify removeAccount completed before the keyring was removed + expect(removeAccountCompleted).toBe(true); + // The keyring should only be removed after removeAccount completes, + // so the first state change should still have 2 keyrings (or be undefined if no change occurred before completion) + // After completion, keyring count should be 1 + expect(controller.state.keyrings).toHaveLength(1); + }, + ); + }); }); describe('when the keyring for the given address does not support removeAccount', () => { From af757151e1b80d086d36d4d1b2f2ba9110b2ed65 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 12:04:43 +0100 Subject: [PATCH 04/10] await remove account --- .../keyring-controller/src/KeyringController.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 23f1943d374..8526aa5205b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1260,12 +1260,14 @@ export class KeyringController< ); } - // The `removeAccount` method of snaps keyring is async. We have to update - // the interface of the other keyrings to be async as well. - // FIXME: We do cast to `Hex` to makes the type checker happy here, and - // because `Keyring.removeAccount` requires address to be `Hex`. Those - // type would need to be updated for a full non-EVM support. - keyring.removeAccount(address as Hex); + // FIXME #1: We do cast to `Hex` to make the type checker happy here, and + // because `Keyring.removeAccount` requires address to be `Hex`. + // Those types would need to be updated for a full non-EVM support. + // + // FIXME #2: The `removeAccount` method of snaps keyring is async. We have + // to update the interface of the other keyrings to be async as well. + // eslint-disable-next-line @typescript-eslint/await-thenable + await keyring.removeAccount(address as Hex); if (shouldRemoveKeyring) { this.#keyrings.splice(keyringIndex, 1); From b96e9e18b9656bc5b5c4f498bd5c512d3ce55fae Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 12:25:25 +0100 Subject: [PATCH 05/10] update CHANGELOG --- packages/keyring-controller/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index b906dab6170..7954e727712 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Upgrade `@metamask/utils` from `^11.8.1` to `^11.9.0` ([#7511](https://github.com/MetaMask/core/pull/7511)) +- Do not remove other empty keyrings when removing an account ([#7670](https://github.com/MetaMask/core/pull/7670)) +- Await the `removeAccount` method of the keyring ([#7670](https://github.com/MetaMask/core/pull/7670)) ## [25.0.0] From 512cde3a2114a7f3b3de9e135bd86a5dfcde9944 Mon Sep 17 00:00:00 2001 From: matthiasgeihs <62935430+matthiasgeihs@users.noreply.github.com> Date: Tue, 20 Jan 2026 17:00:33 +0100 Subject: [PATCH 06/10] Update packages/keyring-controller/CHANGELOG.md Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 7954e727712..7e9095e53ca 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -20,7 +20,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Upgrade `@metamask/utils` from `^11.8.1` to `^11.9.0` ([#7511](https://github.com/MetaMask/core/pull/7511)) - Do not remove other empty keyrings when removing an account ([#7670](https://github.com/MetaMask/core/pull/7670)) -- Await the `removeAccount` method of the keyring ([#7670](https://github.com/MetaMask/core/pull/7670)) ## [25.0.0] From e2c6fbf0276f134b8486176193d8c1b6fe7cbdef Mon Sep 17 00:00:00 2001 From: matthiasgeihs <62935430+matthiasgeihs@users.noreply.github.com> Date: Tue, 20 Jan 2026 17:00:45 +0100 Subject: [PATCH 07/10] Update packages/keyring-controller/CHANGELOG.md Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- packages/keyring-controller/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 7e9095e53ca..e35f143edae 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -19,7 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Upgrade `@metamask/utils` from `^11.8.1` to `^11.9.0` ([#7511](https://github.com/MetaMask/core/pull/7511)) -- Do not remove other empty keyrings when removing an account ([#7670](https://github.com/MetaMask/core/pull/7670)) +- Prevent the accidental removal of unrelated empty keyrings when deleting an account ([#7670](https://github.com/MetaMask/core/pull/7670)) + - Empty keyrings were removed during account deletion, regardless of which account was being targeted. ## [25.0.0] From cac08b0b6b0a4da1e1d959eb53b12e2c67a73aab Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 17:34:10 +0100 Subject: [PATCH 08/10] fix missing address normalization --- .../src/KeyringController.test.ts | 8 +--- .../src/KeyringController.ts | 45 ++++++++++--------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 2c2ad6cc169..93b255825d0 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1623,15 +1623,11 @@ describe('KeyringController', () => { controller.removeAccount( '0x0000000000000000000000000000000000000000', ), - ).rejects.toThrow( - 'KeyringController - No keyring found. Error info: There are keyrings, but none match the address', - ); + ).rejects.toThrow('KeyringController - No keyring found'); await expect( controller.removeAccount('0xDUMMY_INPUT'), - ).rejects.toThrow( - 'KeyringController - No keyring found. Error info: There are keyrings, but none match the address', - ); + ).rejects.toThrow('KeyringController - No keyring found'); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8526aa5205b..add11e8183b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1094,28 +1094,16 @@ export class KeyringController< */ async getKeyringForAccount(account: string): Promise { this.#assertIsUnlocked(); - const address = normalize(account); - - const candidates = await Promise.all( - this.#keyrings.map(async ({ keyring }) => { - return [keyring, await keyring.getAccounts()] as const; - }), - ); - - const winners = candidates.filter((candidate) => { - const accounts = candidate[1].map(normalize); - return accounts.includes(address); - }); - - if (winners.length && winners[0]?.length) { - return winners[0][0]; + const keyringIndex = await this.#findKeyringIndexForAccount(account); + if (keyringIndex > -1) { + return this.#keyrings[keyringIndex].keyring; } // Adding more info to the error let errorInfo = ''; - if (!candidates.length) { + if (this.#keyrings.length === 0) { errorInfo = 'There are no keyrings'; - } else if (!winners.length) { + } else { errorInfo = 'There are keyrings, but none match the address'; } throw new KeyringControllerError( @@ -1123,6 +1111,17 @@ export class KeyringController< ); } + async #findKeyringIndexForAccount(account: string): Promise { + this.#assertIsUnlocked(); + const address = account.toLowerCase(); + const accountsPerKeyring = await Promise.all( + this.#keyrings.map(({ keyring }) => keyring.getAccounts()), + ); + return accountsPerKeyring.findIndex((accounts) => + accounts.map((a) => a.toLowerCase()).includes(address), + ); + } + /** * Returns all keyrings of the given type. * @@ -1237,11 +1236,15 @@ export class KeyringController< this.#assertIsUnlocked(); await this.#persistOrRollback(async () => { - const keyring = (await this.getKeyringForAccount(address)) as EthKeyring; + const keyringIndex = await this.#findKeyringIndexForAccount(address); - const keyringIndex = this.state.keyrings.findIndex((kr) => - kr.accounts.includes(address), - ); + if (keyringIndex === -1) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.NoKeyring, + ); + } + + const { keyring } = this.#keyrings[keyringIndex]; const isPrimaryKeyring = keyringIndex === 0; const shouldRemoveKeyring = (await keyring.getAccounts()).length === 1; From a3d58aba78536b98847b633d2668677e2957396b Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 18:24:25 +0100 Subject: [PATCH 09/10] test: do not remove primary keyring --- .../src/KeyringController.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 93b255825d0..e0bd8947e71 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1572,6 +1572,21 @@ describe('KeyringController', () => { }); }); + it('should not remove primary keyring when address is not normalized', async () => { + await withController(async ({ controller, initialState }) => { + const account = initialState.keyrings[0].accounts[0] as Hex; + // Convert to checksummed/uppercase address (non-normalized), keeping 0x prefix lowercase + const nonNormalizedAccount = `0x${account.slice(2).toUpperCase()}`; + await expect( + controller.removeAccount(nonNormalizedAccount), + ).rejects.toThrow( + KeyringControllerErrorMessage.LastAccountInPrimaryKeyring, + ); + expect(controller.state.keyrings).toHaveLength(1); + expect(controller.state.keyrings[0].accounts).toHaveLength(1); + }); + }); + it('should not remove primary keyring if it has no accounts even if it has more than one HD keyring', async () => { await withController(async ({ controller }) => { await controller.addNewKeyring(KeyringTypes.hd); From 074a6c110224302fa26da68ba8d385d6ea1696c1 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 20 Jan 2026 18:27:38 +0100 Subject: [PATCH 10/10] update CHANGELOG --- packages/keyring-controller/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index e35f143edae..9529566e42b 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -22,6 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Prevent the accidental removal of unrelated empty keyrings when deleting an account ([#7670](https://github.com/MetaMask/core/pull/7670)) - Empty keyrings were removed during account deletion, regardless of which account was being targeted. +### Fixed + +- Fixed a bug where `removeAccount` would not prevent deletion of the primary keyring because of missing address normalization ([#7670](https://github.com/MetaMask/core/pull/7670)) + ## [25.0.0] ### Added