fix(keyring-controller): Only drop target keyring on removeAccount

packages/keyring-controller/CHANGELOG.md

@@ -19,6 +19,12 @@ 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))
+- 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]
 

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);
@@ -1623,15 +1638,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');
         });
       });
 
@@ -1645,6 +1656,88 @@ 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([]);
+        });
+      });
+
+      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', () => {

packages/keyring-controller/src/KeyringController.ts

@@ -1094,35 +1094,34 @@ export class KeyringController<
    */
   async getKeyringForAccount(account: string): Promise<unknown> {
     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(
       `${KeyringControllerErrorMessage.NoKeyring}. Error info: ${errorInfo}`,
     );
   }
 
+  async #findKeyringIndexForAccount(account: string): Promise<number> {
+    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;
@@ -1260,15 +1263,18 @@ 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<State>.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<State>.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) {
-        await this.#removeEmptyKeyrings();
+        this.#keyrings.splice(keyringIndex, 1);
+        await this.#destroyKeyring(keyring);
       }
     });
 
@@ -2576,34 +2582,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<void> {
-    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.
    *