From ccd406e3330a571210ba0efbb346d03a908142ef Mon Sep 17 00:00:00 2001
From: Michele Esposito <michele@esposito.codes>
Date: Fri, 17 Jan 2025 12:21:44 +0100
Subject: [PATCH 1/6] chore: fix lint warnings in `KeyringController`

---
 .../src/KeyringController.test.ts             |  79 ++++++------
 .../src/KeyringController.ts                  | 112 +++++++-----------
 2 files changed, 74 insertions(+), 117 deletions(-)

diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
index 18a0c2319d1..7dfa9fe8bf2 100644
--- a/packages/keyring-controller/src/KeyringController.test.ts
+++ b/packages/keyring-controller/src/KeyringController.test.ts
@@ -25,13 +25,6 @@ import {
 import * as sinon from 'sinon';
 import * as uuid from 'uuid';
 
-import MockEncryptor, {
-  MOCK_ENCRYPTION_KEY,
-} from '../tests/mocks/mockEncryptor';
-import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring';
-import { MockKeyring } from '../tests/mocks/mockKeyring';
-import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring';
-import { buildMockTransaction } from '../tests/mocks/mockTransaction';
 import { KeyringControllerError } from './constants';
 import type {
   KeyringControllerEvents,
@@ -47,6 +40,13 @@ import {
   isCustodyKeyring,
   keyringBuilderFactory,
 } from './KeyringController';
+import MockEncryptor, {
+  MOCK_ENCRYPTION_KEY,
+} from '../tests/mocks/mockEncryptor';
+import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring';
+import { MockKeyring } from '../tests/mocks/mockKeyring';
+import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring';
+import { buildMockTransaction } from '../tests/mocks/mockTransaction';
 
 jest.mock('uuid', () => {
   return {
@@ -181,12 +181,10 @@ describe('KeyringController', () => {
       it('should not add a new account if called twice with the same accountCount param', async () => {
         await withController(async ({ controller, initialState }) => {
           const accountCount = initialState.keyrings[0].accounts.length;
-          const firstAccountAdded = await controller.addNewAccount(
-            accountCount,
-          );
-          const secondAccountAdded = await controller.addNewAccount(
-            accountCount,
-          );
+          const firstAccountAdded =
+            await controller.addNewAccount(accountCount);
+          const secondAccountAdded =
+            await controller.addNewAccount(accountCount);
           expect(firstAccountAdded).toBe(secondAccountAdded);
           expect(controller.state.keyrings[0].accounts).toHaveLength(
             accountCount + 1,
@@ -220,13 +218,11 @@ describe('KeyringController', () => {
 
           const accountCount = initialState.keyrings[0].accounts.length;
           // We add a new account for "index 1" (not existing yet)
-          const firstAccountAdded = await controller.addNewAccount(
-            accountCount,
-          );
+          const firstAccountAdded =
+            await controller.addNewAccount(accountCount);
           // Adding an account for an existing index will return the existing account's address
-          const secondAccountAdded = await controller.addNewAccount(
-            accountCount,
-          );
+          const secondAccountAdded =
+            await controller.addNewAccount(accountCount);
           expect(firstAccountAdded).toBe(secondAccountAdded);
           expect(controller.state.keyrings[0].accounts).toHaveLength(
             accountCount + 1,
@@ -258,9 +254,8 @@ describe('KeyringController', () => {
           const [primaryKeyring] = controller.getKeyringsByType(
             KeyringTypes.hd,
           ) as Keyring<Json>[];
-          const addedAccountAddress = await controller.addNewAccountForKeyring(
-            primaryKeyring,
-          );
+          const addedAccountAddress =
+            await controller.addNewAccountForKeyring(primaryKeyring);
           expect(initialState.keyrings).toHaveLength(1);
           expect(initialState.keyrings[0].accounts).not.toStrictEqual(
             controller.state.keyrings[0].accounts,
@@ -306,9 +301,8 @@ describe('KeyringController', () => {
           const [primaryKeyring] = controller.getKeyringsByType(
             KeyringTypes.hd,
           ) as Keyring<Json>[];
-          const addedAccountAddress = await controller.addNewAccountForKeyring(
-            primaryKeyring,
-          );
+          const addedAccountAddress =
+            await controller.addNewAccountForKeyring(primaryKeyring);
           expect(initialState.keyrings).toHaveLength(1);
           expect(initialState.keyrings[0].accounts).not.toStrictEqual(
             controller.state.keyrings[0].accounts,
@@ -407,9 +401,8 @@ describe('KeyringController', () => {
           await withController(
             { cacheEncryptionKey },
             async ({ controller, initialState }) => {
-              const currentSeedWord = await controller.exportSeedPhrase(
-                password,
-              );
+              const currentSeedWord =
+                await controller.exportSeedPhrase(password);
 
               await controller.createNewVaultAndRestore(
                 password,
@@ -475,9 +468,8 @@ describe('KeyringController', () => {
               async ({ controller }) => {
                 await controller.createNewVaultAndKeychain(password);
 
-                const currentSeedPhrase = await controller.exportSeedPhrase(
-                  password,
-                );
+                const currentSeedPhrase =
+                  await controller.exportSeedPhrase(password);
 
                 expect(currentSeedPhrase.length).toBeGreaterThan(0);
                 expect(
@@ -565,17 +557,15 @@ describe('KeyringController', () => {
             await withController(
               { cacheEncryptionKey },
               async ({ controller, initialState }) => {
-                const initialSeedWord = await controller.exportSeedPhrase(
-                  password,
-                );
+                const initialSeedWord =
+                  await controller.exportSeedPhrase(password);
                 expect(initialSeedWord).toBeDefined();
                 const initialVault = controller.state.vault;
 
                 await controller.createNewVaultAndKeychain(password);
 
-                const currentSeedWord = await controller.exportSeedPhrase(
-                  password,
-                );
+                const currentSeedWord =
+                  await controller.exportSeedPhrase(password);
                 expect(initialState).toStrictEqual(controller.state);
                 expect(initialSeedWord).toBe(currentSeedWord);
                 expect(initialVault).toStrictEqual(controller.state.vault);
@@ -2556,21 +2546,18 @@ describe('KeyringController', () => {
           ),
         );
 
-        const firstPage = await signProcessKeyringController.connectQRHardware(
-          0,
-        );
+        const firstPage =
+          await signProcessKeyringController.connectQRHardware(0);
         expect(firstPage).toHaveLength(5);
         expect(firstPage[0].index).toBe(0);
 
-        const secondPage = await signProcessKeyringController.connectQRHardware(
-          1,
-        );
+        const secondPage =
+          await signProcessKeyringController.connectQRHardware(1);
         expect(secondPage).toHaveLength(5);
         expect(secondPage[0].index).toBe(5);
 
-        const goBackPage = await signProcessKeyringController.connectQRHardware(
-          -1,
-        );
+        const goBackPage =
+          await signProcessKeyringController.connectQRHardware(-1);
         expect(goBackPage).toStrictEqual(firstPage);
 
         await signProcessKeyringController.unlockQRHardwareWalletAccount(0);
diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
index bf5b853aeeb..729cbc52464 100644
--- a/packages/keyring-controller/src/KeyringController.ts
+++ b/packages/keyring-controller/src/KeyringController.ts
@@ -52,32 +52,19 @@ const name = 'KeyringController';
  * Available keyring types
  */
 export enum KeyringTypes {
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   simple = 'Simple Key Pair',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   hd = 'HD Key Tree',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   qr = 'QR Hardware Wallet Device',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   trezor = 'Trezor Hardware',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   ledger = 'Ledger Hardware',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   lattice = 'Lattice Hardware',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   snap = 'Snap Keyring',
 }
 
 /**
  * Custody keyring types are a special case, as they are not a single type
  * but they all start with the prefix "Custody".
+ *
  * @param keyringType - The type of the keyring.
  * @returns Whether the keyring type is a custody keyring.
  */
@@ -86,15 +73,9 @@ export const isCustodyKeyring = (keyringType: string): boolean => {
 };
 
 /**
- * @type KeyringControllerState
+ * KeyringControllerState
  *
- * Keyring controller state
- * @property vault - Encrypted string representing keyring data
- * @property isUnlocked - Whether vault is unlocked
- * @property keyringTypes - Account types
- * @property keyrings - Group of accounts
- * @property encryptionKey - Keyring encryption key
- * @property encryptionSalt - Keyring encryption salt
+ * The KeyringController state
  */
 export type KeyringControllerState = {
   vault?: string;
@@ -251,11 +232,9 @@ export type KeyringControllerOptions = {
 );
 
 /**
- * @type KeyringObject
+ * KeyringObject
  *
  * Keyring object to return in fullUpdate
- * @property type - Keyring type
- * @property accounts - Associated accounts
  */
 export type KeyringObject = {
   accounts: string[];
@@ -266,11 +245,7 @@ export type KeyringObject = {
  * A strategy for importing an account
  */
 export enum AccountImportStrategy {
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   privateKey = 'privateKey',
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
   json = 'json',
 }
 
@@ -404,13 +379,11 @@ export type KeyringSelector =
  *
  * @param releaseLock - A function to release the lock.
  */
-// TODO: Either fix this lint violation or explain why it's necessary to ignore.
-// eslint-disable-next-line @typescript-eslint/naming-convention
-type MutuallyExclusiveCallback<T> = ({
+type MutuallyExclusiveCallback<Result> = ({
   releaseLock,
 }: {
   releaseLock: MutexInterface.Releaser;
-}) => Promise<T>;
+}) => Promise<Result>;
 
 /**
  * Get builder function for `Keyring`
@@ -586,17 +559,17 @@ export class KeyringController extends BaseController<
 
   readonly #vaultOperationMutex = new Mutex();
 
-  #keyringBuilders: { (): EthKeyring<Json>; type: string }[];
+  readonly #keyringBuilders: { (): EthKeyring<Json>; type: string }[];
 
-  #keyrings: EthKeyring<Json>[];
+  readonly #unsupportedKeyrings: SerializedKeyring[];
 
-  #unsupportedKeyrings: SerializedKeyring[];
+  readonly #encryptor: GenericEncryptor | ExportableKeyEncryptor;
 
-  #password?: string;
+  readonly #cacheEncryptionKey: boolean;
 
-  #encryptor: GenericEncryptor | ExportableKeyEncryptor;
+  #keyrings: EthKeyring<Json>[];
 
-  #cacheEncryptionKey: boolean;
+  #password?: string;
 
   #qrKeyringStateListener?: (
     state: ReturnType<IQRKeyringState['getState']>,
@@ -765,6 +738,7 @@ export class KeyringController extends BaseController<
    * If there is a pre-existing locked vault, it will be replaced.
    *
    * @param password - Password to unlock the new vault.
+   * @returns Promise resolving when the operation ends successfully.
    */
   async createNewVaultAndKeychain(password: string) {
     return this.#persistOrRollback(async () => {
@@ -989,7 +963,7 @@ export class KeyringController extends BaseController<
     return this.#persistOrRollback(async () => {
       let privateKey;
       switch (strategy) {
-        case 'privateKey':
+        case AccountImportStrategy.privateKey:
           const [importedKey] = args;
           if (!importedKey) {
             throw new Error('Cannot import an empty key.');
@@ -1013,7 +987,7 @@ export class KeyringController extends BaseController<
 
           privateKey = remove0x(prefixed);
           break;
-        case 'json':
+        case AccountImportStrategy.json:
           let wallet;
           const [input, password] = args;
           try {
@@ -1024,7 +998,7 @@ export class KeyringController extends BaseController<
           privateKey = bytesToHex(wallet.getPrivateKey());
           break;
         default:
-          throw new Error(`Unexpected import strategy: '${strategy}'`);
+          throw new Error(`Unexpected import strategy: '${String(strategy)}'`);
       }
       const newKeyring = (await this.#newKeyring(KeyringTypes.simple, [
         privateKey,
@@ -1054,7 +1028,6 @@ export class KeyringController extends BaseController<
 
       // 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
       // 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.
@@ -2203,7 +2176,6 @@ export class KeyringController extends BaseController<
       // cases and allow retro-compatibility too.
       // FIXME: For some reason, it seems that eslint is complaining about this call being non-thenable
       // even though it is... For now, we just disable it:
-      // eslint-disable-next-line @typescript-eslint/await-thenable
       await keyring.generateRandomMnemonic();
       await keyring.addAccounts(1);
     }
@@ -2353,14 +2325,14 @@ export class KeyringController extends BaseController<
    * and save the keyrings to state after it, or rollback to their
    * previous state in case of error.
    *
-   * @param fn - The function to execute.
+   * @param callback - The function to execute.
    * @returns The result of the function.
    */
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
-  async #persistOrRollback<T>(fn: MutuallyExclusiveCallback<T>): Promise<T> {
+  async #persistOrRollback<Result>(
+    callback: MutuallyExclusiveCallback<Result>,
+  ): Promise<Result> {
     return this.#withRollback(async ({ releaseLock }) => {
-      const callbackResult = await fn({ releaseLock });
+      const callbackResult = await callback({ releaseLock });
       // State is committed only if the operation is successful
       await this.#updateVault();
 
@@ -2372,18 +2344,18 @@ export class KeyringController extends BaseController<
    * Execute the given function after acquiring the controller lock
    * and rollback keyrings and password states in case of error.
    *
-   * @param fn - The function to execute atomically.
+   * @param callback - The function to execute atomically.
    * @returns The result of the function.
    */
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
-  async #withRollback<T>(fn: MutuallyExclusiveCallback<T>): Promise<T> {
+  async #withRollback<Result>(
+    callback: MutuallyExclusiveCallback<Result>,
+  ): Promise<Result> {
     return this.#withControllerLock(async ({ releaseLock }) => {
       const currentSerializedKeyrings = await this.#getSerializedKeyrings();
       const currentPassword = this.#password;
 
       try {
-        return await fn({ releaseLock });
+        return await callback({ releaseLock });
       } catch (e) {
         // Keyrings and password are restored to their previous state
         await this.#restoreSerializedKeyrings(currentSerializedKeyrings);
@@ -2414,13 +2386,13 @@ export class KeyringController extends BaseController<
    * controller and that changes its state is executed in a mutually exclusive way,
    * preventing unsafe concurrent access that could lead to unpredictable behavior.
    *
-   * @param fn - The function to execute while the controller mutex is locked.
+   * @param callback - The function to execute while the controller mutex is locked.
    * @returns The result of the function.
    */
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
-  async #withControllerLock<T>(fn: MutuallyExclusiveCallback<T>): Promise<T> {
-    return withLock(this.#controllerOperationMutex, fn);
+  async #withControllerLock<Result>(
+    callback: MutuallyExclusiveCallback<Result>,
+  ): Promise<Result> {
+    return withLock(this.#controllerOperationMutex, callback);
   }
 
   /**
@@ -2431,15 +2403,15 @@ export class KeyringController extends BaseController<
    * This ensures that each operation that interacts with the vault
    * is executed in a mutually exclusive way.
    *
-   * @param fn - The function to execute while the vault mutex is locked.
+   * @param callback - The function to execute while the vault mutex is locked.
    * @returns The result of the function.
    */
-  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-  // eslint-disable-next-line @typescript-eslint/naming-convention
-  async #withVaultLock<T>(fn: MutuallyExclusiveCallback<T>): Promise<T> {
+  async #withVaultLock<Result>(
+    callback: MutuallyExclusiveCallback<Result>,
+  ): Promise<Result> {
     this.#assertControllerMutexIsLocked();
 
-    return withLock(this.#vaultOperationMutex, fn);
+    return withLock(this.#vaultOperationMutex, callback);
   }
 }
 
@@ -2449,19 +2421,17 @@ export class KeyringController extends BaseController<
  * error is thrown.
  *
  * @param mutex - The mutex to lock.
- * @param fn - The function to execute while the mutex is locked.
+ * @param callback - The function to execute while the mutex is locked.
  * @returns The result of the function.
  */
-// TODO: Either fix this lint violation or explain why it's necessary to ignore.
-// eslint-disable-next-line @typescript-eslint/naming-convention
-async function withLock<T>(
+async function withLock<Result>(
   mutex: Mutex,
-  fn: MutuallyExclusiveCallback<T>,
-): Promise<T> {
+  callback: MutuallyExclusiveCallback<Result>,
+): Promise<Result> {
   const releaseLock = await mutex.acquire();
 
   try {
-    return await fn({ releaseLock });
+    return await callback({ releaseLock });
   } finally {
     releaseLock();
   }

From e8e41090db4d7084f4e21de821d063b662991b05 Mon Sep 17 00:00:00 2001
From: Michele Esposito <michele@esposito.codes>
Date: Fri, 17 Jan 2025 13:00:46 +0100
Subject: [PATCH 2/6] fix: throw explicit error when `KeyringController` is
 locked

---
 .../src/KeyringController.test.ts             |  39 +++---
 .../src/KeyringController.ts                  | 113 +++++++++++++-----
 packages/keyring-controller/src/constants.ts  |   1 +
 3 files changed, 105 insertions(+), 48 deletions(-)

diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
index bbba030eee3..5a545e6d843 100644
--- a/packages/keyring-controller/src/KeyringController.test.ts
+++ b/packages/keyring-controller/src/KeyringController.test.ts
@@ -193,12 +193,12 @@ describe('KeyringController', () => {
       });
     });
 
-    it('should throw error with no HD keyring', async () => {
+    it('should throw error when the controller is locked', async () => {
       await withController(
         { skipVaultCreation: true },
         async ({ controller }) => {
           await expect(controller.addNewAccount()).rejects.toThrow(
-            'No HD keyring found',
+            KeyringControllerError.ControllerLocked,
           );
         },
       );
@@ -901,19 +901,16 @@ describe('KeyringController', () => {
         });
       });
 
-      it('should throw an error if there are no keyrings', async () => {
-        await withController(
-          { skipVaultCreation: true },
-          async ({ controller }) => {
-            await expect(
-              controller.getKeyringForAccount(
-                '0x51253087e6f8358b5f10c0a94315d69db3357859',
-              ),
-            ).rejects.toThrow(
-              'KeyringController - No keyring found. Error info: There are no keyrings',
-            );
-          },
-        );
+      it('should throw an error if the controller is locked', async () => {
+        await withController(async ({ controller }) => {
+          await controller.setLocked();
+
+          await expect(
+            controller.getKeyringForAccount(
+              '0x51253087e6f8358b5f10c0a94315d69db3357859',
+            ),
+          ).rejects.toThrow(KeyringControllerError.ControllerLocked);
+        });
       });
     });
   });
@@ -963,7 +960,7 @@ describe('KeyringController', () => {
         await controller.setLocked();
 
         await expect(controller.persistAllKeyrings()).rejects.toThrow(
-          KeyringControllerError.MissingCredentials,
+          KeyringControllerError.ControllerLocked,
         );
       });
     });
@@ -2018,9 +2015,9 @@ describe('KeyringController', () => {
             async ({ controller }) => {
               await controller.setLocked();
 
-              await expect(controller.changePassword('')).rejects.toThrow(
-                KeyringControllerError.MissingCredentials,
-              );
+              await expect(async () =>
+                controller.changePassword(''),
+              ).rejects.toThrow(KeyringControllerError.ControllerLocked);
             },
           );
         });
@@ -2265,12 +2262,12 @@ describe('KeyringController', () => {
       });
     });
 
-    it('should throw error with no HD keyring', async () => {
+    it('should throw error if the controller is locked', async () => {
       await withController(
         { skipVaultCreation: true },
         async ({ controller }) => {
           await expect(controller.verifySeedPhrase()).rejects.toThrow(
-            'No HD keyring found',
+            KeyringControllerError.ControllerLocked,
           );
         },
       );
diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
index 138fb6e6e4f..244c4165a1e 100644
--- a/packages/keyring-controller/src/KeyringController.ts
+++ b/packages/keyring-controller/src/KeyringController.ts
@@ -655,34 +655,31 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to the added account address.
    */
   async addNewAccount(accountCount?: number): Promise<string> {
-    return this.#persistOrRollback(async () => {
-      const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as
-        | EthKeyring<Json>
-        | undefined;
-      if (!primaryKeyring) {
-        throw new Error('No HD keyring found');
-      }
-      const oldAccounts = await primaryKeyring.getAccounts();
+    return this.withKeyring(
+      { type: KeyringTypes.hd },
+      async (primaryKeyring) => {
+        const oldAccounts = await primaryKeyring.getAccounts();
+
+        if (accountCount && oldAccounts.length !== accountCount) {
+          if (accountCount > oldAccounts.length) {
+            throw new Error('Account out of sequence');
+          }
+          // we return the account already existing at index `accountCount`
+          const existingAccount = oldAccounts[accountCount];
 
-      if (accountCount && oldAccounts.length !== accountCount) {
-        if (accountCount > oldAccounts.length) {
-          throw new Error('Account out of sequence');
-        }
-        // we return the account already existing at index `accountCount`
-        const existingAccount = oldAccounts[accountCount];
+          if (!existingAccount) {
+            throw new Error(`Can't find account at index ${accountCount}`);
+          }
 
-        if (!existingAccount) {
-          throw new Error(`Can't find account at index ${accountCount}`);
+          return existingAccount;
         }
 
-        return existingAccount;
-      }
-
-      const [addedAccountAddress] = await primaryKeyring.addAccounts(1);
-      await this.#verifySeedPhrase();
+        const [addedAccountAddress] = await primaryKeyring.addAccounts(1);
+        await this.#verifySeedPhrase();
 
-      return addedAccountAddress;
-    });
+        return addedAccountAddress;
+      },
+    );
   }
 
   /**
@@ -700,6 +697,8 @@ export class KeyringController extends BaseController<
     // We still uses `Hex` here, since we are not using this method when creating
     // and account using a "Snap Keyring". This function assume the `keyring` is
     // ethereum compatible, but "Snap Keyring" might not be.
+    this.#assertIsUnlocked();
+
     return this.#persistOrRollback(async () => {
       const oldAccounts = await this.#getAccountsFromKeyrings();
 
@@ -783,6 +782,8 @@ export class KeyringController extends BaseController<
     type: KeyringTypes | string,
     opts?: unknown,
   ): Promise<unknown> {
+    this.#assertIsUnlocked();
+
     if (type === KeyringTypes.qr) {
       return this.getOrAddQRKeyring();
     }
@@ -850,6 +851,7 @@ export class KeyringController extends BaseController<
    * @returns A promise resolving to an array of addresses.
    */
   async getAccounts(): Promise<string[]> {
+    this.#assertIsUnlocked();
     return this.state.keyrings.reduce<string[]>(
       (accounts, keyring) => accounts.concat(keyring.accounts),
       [],
@@ -868,6 +870,7 @@ export class KeyringController extends BaseController<
     account: string,
     opts?: Record<string, unknown>,
   ): Promise<string> {
+    this.#assertIsUnlocked();
     const address = ethNormalize(account) as Hex;
     const keyring = (await this.getKeyringForAccount(
       account,
@@ -891,6 +894,7 @@ export class KeyringController extends BaseController<
     from: string;
     data: Eip1024EncryptedData;
   }): Promise<string> {
+    this.#assertIsUnlocked();
     const address = ethNormalize(messageParams.from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
@@ -913,6 +917,7 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to keyring of the `account` if one exists.
    */
   async getKeyringForAccount(account: string): Promise<unknown> {
+    this.#assertIsUnlocked();
     const address = normalize(account);
 
     const candidates = await Promise.all(
@@ -952,6 +957,7 @@ export class KeyringController extends BaseController<
    * @returns An array of keyrings of the given type.
    */
   getKeyringsByType(type: KeyringTypes | string): unknown[] {
+    this.#assertIsUnlocked();
     return this.#keyrings.filter((keyring) => keyring.type === type);
   }
 
@@ -963,6 +969,7 @@ export class KeyringController extends BaseController<
    * operation completes.
    */
   async persistAllKeyrings(): Promise<boolean> {
+    this.#assertIsUnlocked();
     return this.#persistOrRollback(async () => true);
   }
 
@@ -980,6 +987,7 @@ export class KeyringController extends BaseController<
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     args: any[],
   ): Promise<string> {
+    this.#assertIsUnlocked();
     return this.#persistOrRollback(async () => {
       let privateKey;
       switch (strategy) {
@@ -1036,6 +1044,8 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving when the account is removed.
    */
   async removeAccount(address: string): Promise<void> {
+    this.#assertIsUnlocked();
+
     await this.#persistOrRollback(async () => {
       const keyring = (await this.getKeyringForAccount(
         address,
@@ -1069,6 +1079,8 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving when the operation completes.
    */
   async setLocked(): Promise<void> {
+    this.#assertIsUnlocked();
+
     return this.#withRollback(async () => {
       this.#unsubscribeFromQRKeyringsEvents();
 
@@ -1093,6 +1105,8 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to a signed message string.
    */
   async signMessage(messageParams: PersonalMessageParams): Promise<string> {
+    this.#assertIsUnlocked();
+
     if (!messageParams.data) {
       throw new Error("Can't sign an empty message");
     }
@@ -1115,6 +1129,7 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to a signed message string.
    */
   async signPersonalMessage(messageParams: PersonalMessageParams) {
+    this.#assertIsUnlocked();
     const address = ethNormalize(messageParams.from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
@@ -1140,6 +1155,8 @@ export class KeyringController extends BaseController<
     messageParams: TypedMessageParams,
     version: SignTypedDataVersion,
   ): Promise<string> {
+    this.#assertIsUnlocked();
+
     try {
       if (
         ![
@@ -1189,6 +1206,7 @@ export class KeyringController extends BaseController<
     from: string,
     opts?: Record<string, unknown>,
   ): Promise<TxData> {
+    this.#assertIsUnlocked();
     const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
@@ -1213,6 +1231,7 @@ export class KeyringController extends BaseController<
     transactions: EthBaseTransaction[],
     executionContext: KeyringExecutionContext,
   ): Promise<EthBaseUserOperation> {
+    this.#assertIsUnlocked();
     const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
@@ -1243,6 +1262,7 @@ export class KeyringController extends BaseController<
     userOp: EthUserOperation,
     executionContext: KeyringExecutionContext,
   ): Promise<EthUserOperationPatch> {
+    this.#assertIsUnlocked();
     const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
@@ -1268,6 +1288,7 @@ export class KeyringController extends BaseController<
     userOp: EthUserOperation,
     executionContext: KeyringExecutionContext,
   ): Promise<string> {
+    this.#assertIsUnlocked();
     const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
@@ -1287,11 +1308,8 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving when the operation completes.
    */
   changePassword(password: string): Promise<void> {
+    this.#assertIsUnlocked();
     return this.#persistOrRollback(async () => {
-      if (!this.state.isUnlocked) {
-        throw new Error(KeyringControllerError.MissingCredentials);
-      }
-
       assertIsValidPassword(password);
 
       this.#password = password;
@@ -1349,6 +1367,7 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to the seed phrase as Uint8Array.
    */
   async verifySeedPhrase(): Promise<Uint8Array> {
+    this.#assertIsUnlocked();
     return this.#withControllerLock(async () => this.#verifySeedPhrase());
   }
 
@@ -1418,6 +1437,8 @@ export class KeyringController extends BaseController<
       createIfMissing: false,
     },
   ): Promise<CallbackResult> {
+    this.#assertIsUnlocked();
+
     return this.#persistOrRollback(async () => {
       let keyring: SelectedKeyring | undefined;
 
@@ -1465,6 +1486,7 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   getQRKeyring(): QRKeyring | undefined {
+    this.#assertIsUnlocked();
     // QRKeyring is not yet compatible with Keyring type from @metamask/utils
     return this.getKeyringsByType(KeyringTypes.qr)[0] as unknown as QRKeyring;
   }
@@ -1476,6 +1498,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `addNewKeyring` and `withKeyring` instead.
    */
   async getOrAddQRKeyring(): Promise<QRKeyring> {
+    this.#assertIsUnlocked();
+
     return (
       this.getQRKeyring() ||
       (await this.#persistOrRollback(async () => this.#addQRKeyring()))
@@ -1492,6 +1516,8 @@ export class KeyringController extends BaseController<
   // TODO: Replace `any` with type
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   async restoreQRKeyring(serialized: any): Promise<void> {
+    this.#assertIsUnlocked();
+
     return this.#persistOrRollback(async () => {
       const keyring = this.getQRKeyring() || (await this.#addQRKeyring());
       keyring.deserialize(serialized);
@@ -1505,6 +1531,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async resetQRKeyringState(): Promise<void> {
+    this.#assertIsUnlocked();
+
     (await this.getOrAddQRKeyring()).resetStore();
   }
 
@@ -1516,6 +1544,8 @@ export class KeyringController extends BaseController<
    * instead.
    */
   async getQRKeyringState(): Promise<IQRKeyringState> {
+    this.#assertIsUnlocked();
+
     return (await this.getOrAddQRKeyring()).getMemStore();
   }
 
@@ -1527,6 +1557,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async submitQRCryptoHDKey(cryptoHDKey: string): Promise<void> {
+    this.#assertIsUnlocked();
+
     (await this.getOrAddQRKeyring()).submitCryptoHDKey(cryptoHDKey);
   }
 
@@ -1538,6 +1570,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async submitQRCryptoAccount(cryptoAccount: string): Promise<void> {
+    this.#assertIsUnlocked();
+
     (await this.getOrAddQRKeyring()).submitCryptoAccount(cryptoAccount);
   }
 
@@ -1553,6 +1587,8 @@ export class KeyringController extends BaseController<
     requestId: string,
     ethSignature: string,
   ): Promise<void> {
+    this.#assertIsUnlocked();
+
     (await this.getOrAddQRKeyring()).submitSignature(requestId, ethSignature);
   }
 
@@ -1563,6 +1599,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async cancelQRSignRequest(): Promise<void> {
+    this.#assertIsUnlocked();
+
     (await this.getOrAddQRKeyring()).cancelSignRequest();
   }
 
@@ -1573,6 +1611,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async cancelQRSynchronization(): Promise<void> {
+    this.#assertIsUnlocked();
+
     (await this.getOrAddQRKeyring()).cancelSync();
   }
 
@@ -1588,6 +1628,8 @@ export class KeyringController extends BaseController<
   async connectQRHardware(
     page: number,
   ): Promise<{ balance: string; address: string; index: number }[]> {
+    this.#assertIsUnlocked();
+
     return this.#persistOrRollback(async () => {
       try {
         const keyring = this.getQRKeyring() || (await this.#addQRKeyring());
@@ -1628,6 +1670,8 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async unlockQRHardwareWalletAccount(index: number): Promise<void> {
+    this.#assertIsUnlocked();
+
     return this.#persistOrRollback(async () => {
       const keyring = this.getQRKeyring() || (await this.#addQRKeyring());
 
@@ -1637,6 +1681,8 @@ export class KeyringController extends BaseController<
   }
 
   async getAccountKeyringType(account: string): Promise<string> {
+    this.#assertIsUnlocked();
+
     const keyring = (await this.getKeyringForAccount(
       account,
     )) as EthKeyring<Json>;
@@ -1653,6 +1699,8 @@ export class KeyringController extends BaseController<
     removedAccounts: string[];
     remainingAccounts: string[];
   }> {
+    this.#assertIsUnlocked();
+
     return this.#persistOrRollback(async () => {
       const keyring = this.getQRKeyring();
 
@@ -2338,6 +2386,17 @@ export class KeyringController extends BaseController<
     this.messagingSystem.publish(`${name}:unlock`);
   }
 
+  /**
+   * Assert that the controller is unlocked.
+   *
+   * @throws If the controller is locked.
+   */
+  #assertIsUnlocked(): void {
+    if (!this.state.isUnlocked) {
+      throw new Error(KeyringControllerError.ControllerLocked);
+    }
+  }
+
   /**
    * Execute the given function after acquiring the controller lock
    * and save the keyrings to state after it, or rollback to their
diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts
index fe58710cfaa..4da2115a417 100644
--- a/packages/keyring-controller/src/constants.ts
+++ b/packages/keyring-controller/src/constants.ts
@@ -24,6 +24,7 @@ export enum KeyringControllerError {
   UnsupportedPatchUserOperation = 'KeyringController - The keyring for the current address does not support the method patchUserOperation.',
   UnsupportedSignUserOperation = 'KeyringController - The keyring for the current address does not support the method signUserOperation.',
   NoAccountOnKeychain = "KeyringController - The keychain doesn't have accounts.",
+  ControllerLocked = 'KeyringController - The operation cannot be completed while the controller is locked.',
   MissingCredentials = 'KeyringController - Cannot persist vault without password and encryption key',
   MissingVaultData = 'KeyringController - Cannot persist vault without vault information',
   ExpiredCredentials = 'KeyringController - Encryption key and salt provided are expired',

From 18d72a8e5429671ee5783d058ceae6bb943e5dea Mon Sep 17 00:00:00 2001
From: Michele Esposito <michele@esposito.codes>
Date: Fri, 17 Jan 2025 13:01:58 +0100
Subject: [PATCH 3/6] adjust coverage

---
 packages/keyring-controller/jest.config.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js
index 3dbee998978..81217ca1206 100644
--- a/packages/keyring-controller/jest.config.js
+++ b/packages/keyring-controller/jest.config.js
@@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 95.51,
+      branches: 92.94,
       functions: 100,
-      lines: 99.07,
-      statements: 99.08,
+      lines: 98.61,
+      statements: 98.63,
     },
   },
 

From 8794120c9a987053bce109554773bfb97953a54e Mon Sep 17 00:00:00 2001
From: Michele Esposito <michele@esposito.codes>
Date: Fri, 17 Jan 2025 14:38:42 +0100
Subject: [PATCH 4/6] refactor: rewrite `getKeyringForAddress`

---
 packages/keyring-controller/jest.config.js    |  6 +--
 .../src/KeyringController.test.ts             | 25 ++++++------
 .../src/KeyringController.ts                  | 39 +++++++------------
 3 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js
index 81217ca1206..d28a7cf5cad 100644
--- a/packages/keyring-controller/jest.config.js
+++ b/packages/keyring-controller/jest.config.js
@@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 92.94,
+      branches: 94.07,
       functions: 100,
-      lines: 98.61,
-      statements: 98.63,
+      lines: 98.94,
+      statements: 98.96,
     },
   },
 
diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
index 5a545e6d843..260aa4df30a 100644
--- a/packages/keyring-controller/src/KeyringController.test.ts
+++ b/packages/keyring-controller/src/KeyringController.test.ts
@@ -694,9 +694,12 @@ describe('KeyringController', () => {
           it('should throw error', async () => {
             await withController(async ({ controller }) => {
               await expect(
-                controller.exportAccount(password, ''),
+                controller.exportAccount(
+                  password,
+                  '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045',
+                ),
               ).rejects.toThrow(
-                'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+                `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
               );
             });
           });
@@ -838,7 +841,7 @@ describe('KeyringController', () => {
           await expect(
             controller.decryptMessage(messageParams),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
           );
         });
       });
@@ -896,7 +899,7 @@ describe('KeyringController', () => {
               '0x0000000000000000000000000000000000000000',
             ),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
           );
         });
       });
@@ -1215,13 +1218,13 @@ describe('KeyringController', () => {
               '0x0000000000000000000000000000000000000000',
             ),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            'KeyringController - No keyring found. Error info: There are 2 keyrings available, but none match the address',
           );
 
           await expect(
             controller.removeAccount('0xDUMMY_INPUT'),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            'KeyringController - No keyring found. Error info: There are 2 keyrings available, but none match the address',
           );
         });
       });
@@ -1279,7 +1282,7 @@ describe('KeyringController', () => {
               from: '',
             }),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
           );
         });
       });
@@ -1347,7 +1350,7 @@ describe('KeyringController', () => {
               from: '',
             }),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
           );
         });
       });
@@ -1699,7 +1702,7 @@ describe('KeyringController', () => {
             expect(unsignedEthTx.v).toBeUndefined();
             await controller.signTransaction(unsignedEthTx, '');
           }).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
           );
         });
       });
@@ -2446,7 +2449,7 @@ describe('KeyringController', () => {
               await expect(
                 controller.withKeyring(selector, fn),
               ).rejects.toThrow(
-                'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+                `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
               );
               expect(fn).not.toHaveBeenCalled();
             });
@@ -3473,7 +3476,7 @@ describe('KeyringController', () => {
             await expect(
               controller.exportAccount(password, mockAddress),
             ).rejects.toThrow(
-              'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+              'KeyringController - No keyring found. Error info: There are 1 keyrings available, but none match the address',
             );
           },
         );
diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
index 244c4165a1e..455d15d03af 100644
--- a/packages/keyring-controller/src/KeyringController.ts
+++ b/packages/keyring-controller/src/KeyringController.ts
@@ -30,6 +30,7 @@ import type {
 } from '@metamask/utils';
 import {
   add0x,
+  assert,
   assertIsStrictHexString,
   bytesToHex,
   hasProperty,
@@ -918,33 +919,24 @@ export class KeyringController extends BaseController<
    */
   async getKeyringForAccount(account: string): Promise<unknown> {
     this.#assertIsUnlocked();
-    const address = normalize(account);
 
-    const candidates = await Promise.all(
-      this.#keyrings.map(async (keyring) => {
-        return Promise.all([keyring, keyring.getAccounts()]);
-      }),
+    const address = normalize(account);
+    assert(
+      address,
+      `${KeyringControllerError.NoKeyring}. Error infor: Invalid account address`,
     );
 
-    const winners = candidates.filter((candidate) => {
-      const accounts = candidate[1].map(normalize);
-      return accounts.includes(address);
-    });
+    const keyringIndex = this.state.keyrings.findIndex((keyring) =>
+      keyring.accounts.includes(address),
+    );
 
-    if (winners.length && winners[0]?.length) {
-      return winners[0][0];
+    if (keyringIndex === -1 || !this.#keyrings[keyringIndex]) {
+      throw new Error(
+        `${KeyringControllerError.NoKeyring}. Error info: There are ${this.#keyrings.length} keyrings available, but none match the address`,
+      );
     }
 
-    // Adding more info to the error
-    let errorInfo = '';
-    if (!candidates.length) {
-      errorInfo = 'There are no keyrings';
-    } else if (!winners.length) {
-      errorInfo = 'There are keyrings, but none match the address';
-    }
-    throw new Error(
-      `${KeyringControllerError.NoKeyring}. Error info: ${errorInfo}`,
-    );
+    return this.#keyrings[keyringIndex];
   }
 
   /**
@@ -1894,10 +1886,7 @@ export class KeyringController extends BaseController<
     const primaryKeyring = this.getKeyringsByType(KeyringTypes.hd)[0] as
       | EthKeyring<Json>
       | undefined;
-    if (!primaryKeyring) {
-      throw new Error('No HD keyring found.');
-    }
-
+    assert(primaryKeyring, 'No HD keyring found.');
     assertHasUint8ArrayMnemonic(primaryKeyring);
 
     const seedWords = primaryKeyring.mnemonic;

From f712653004736f38789c09c2151758e60da2b0bb Mon Sep 17 00:00:00 2001
From: Michele Esposito <michele@esposito.codes>
Date: Fri, 17 Jan 2025 14:45:42 +0100
Subject: [PATCH 5/6] revert: use `withKeyring` in `addNewAccount`

---
 packages/keyring-controller/jest.config.js    |  2 +-
 .../src/KeyringController.ts                  | 44 ++++++++++---------
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js
index d28a7cf5cad..a913a46ade1 100644
--- a/packages/keyring-controller/jest.config.js
+++ b/packages/keyring-controller/jest.config.js
@@ -19,7 +19,7 @@ module.exports = merge(baseConfig, {
     global: {
       branches: 94.07,
       functions: 100,
-      lines: 98.94,
+      lines: 98.95,
       statements: 98.96,
     },
   },
diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
index 455d15d03af..e12caa01e28 100644
--- a/packages/keyring-controller/src/KeyringController.ts
+++ b/packages/keyring-controller/src/KeyringController.ts
@@ -656,31 +656,35 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to the added account address.
    */
   async addNewAccount(accountCount?: number): Promise<string> {
-    return this.withKeyring(
-      { type: KeyringTypes.hd },
-      async (primaryKeyring) => {
-        const oldAccounts = await primaryKeyring.getAccounts();
-
-        if (accountCount && oldAccounts.length !== accountCount) {
-          if (accountCount > oldAccounts.length) {
-            throw new Error('Account out of sequence');
-          }
-          // we return the account already existing at index `accountCount`
-          const existingAccount = oldAccounts[accountCount];
+    this.#assertIsUnlocked();
 
-          if (!existingAccount) {
-            throw new Error(`Can't find account at index ${accountCount}`);
-          }
+    return this.#persistOrRollback(async () => {
+      const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as
+        | EthKeyring<Json>
+        | undefined;
+      assert(primaryKeyring, 'No HD Keyring found');
+
+      const oldAccounts = await primaryKeyring.getAccounts();
+
+      if (accountCount && oldAccounts.length !== accountCount) {
+        if (accountCount > oldAccounts.length) {
+          throw new Error('Account out of sequence');
+        }
+        // we return the account already existing at index `accountCount`
+        const existingAccount = oldAccounts[accountCount];
 
-          return existingAccount;
+        if (!existingAccount) {
+          throw new Error(`Can't find account at index ${accountCount}`);
         }
 
-        const [addedAccountAddress] = await primaryKeyring.addAccounts(1);
-        await this.#verifySeedPhrase();
+        return existingAccount;
+      }
 
-        return addedAccountAddress;
-      },
-    );
+      const [addedAccountAddress] = await primaryKeyring.addAccounts(1);
+      await this.#verifySeedPhrase();
+
+      return addedAccountAddress;
+    });
   }
 
   /**

From c60cc5a4c94eb8d88b0085b37cf7a36d38d7b9dd Mon Sep 17 00:00:00 2001
From: Michele Esposito <michele@esposito.codes>
Date: Fri, 17 Jan 2025 15:21:35 +0100
Subject: [PATCH 6/6] refactor: test primary keyring absence

---
 packages/keyring-controller/jest.config.js    |  6 +-
 .../src/KeyringController.test.ts             | 73 +++++++++++++++----
 .../src/KeyringController.ts                  | 43 +++++++----
 3 files changed, 89 insertions(+), 33 deletions(-)

diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js
index a913a46ade1..53a583464ab 100644
--- a/packages/keyring-controller/jest.config.js
+++ b/packages/keyring-controller/jest.config.js
@@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 94.07,
+      branches: 94.26,
       functions: 100,
-      lines: 98.95,
-      statements: 98.96,
+      lines: 98.96,
+      statements: 98.98,
     },
   },
 
diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
index 260aa4df30a..e39c4703299 100644
--- a/packages/keyring-controller/src/KeyringController.test.ts
+++ b/packages/keyring-controller/src/KeyringController.test.ts
@@ -191,6 +191,20 @@ describe('KeyringController', () => {
           );
         });
       });
+
+      it('should throw an error if there is no primary keyring', async () => {
+        await withController(async ({ controller, encryptor }) => {
+          await controller.setLocked();
+          jest
+            .spyOn(encryptor, 'decrypt')
+            .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
+          await controller.submitPassword('123');
+
+          await expect(controller.addNewAccount()).rejects.toThrow(
+            'No HD keyring found',
+          );
+        });
+      });
     });
 
     it('should throw error when the controller is locked', async () => {
@@ -694,12 +708,9 @@ describe('KeyringController', () => {
           it('should throw error', async () => {
             await withController(async ({ controller }) => {
               await expect(
-                controller.exportAccount(
-                  password,
-                  '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045',
-                ),
+                controller.exportAccount(password, ''),
               ).rejects.toThrow(
-                `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+                'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
               );
             });
           });
@@ -841,7 +852,7 @@ describe('KeyringController', () => {
           await expect(
             controller.decryptMessage(messageParams),
           ).rejects.toThrow(
-            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
@@ -892,14 +903,32 @@ describe('KeyringController', () => {
     });
 
     describe('when non-existing account is provided', () => {
-      it('should throw error', async () => {
+      it('should throw error if no account matches the address', async () => {
         await withController(async ({ controller }) => {
           await expect(
             controller.getKeyringForAccount(
               '0x0000000000000000000000000000000000000000',
             ),
           ).rejects.toThrow(
-            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
+          );
+        });
+      });
+
+      it('should throw an error if there is no keyring', async () => {
+        await withController(async ({ controller, encryptor }) => {
+          await controller.setLocked();
+          jest
+            .spyOn(encryptor, 'decrypt')
+            .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
+          await controller.submitPassword('123');
+
+          await expect(
+            controller.getKeyringForAccount(
+              '0x0000000000000000000000000000000000000000',
+            ),
+          ).rejects.toThrow(
+            'KeyringController - No keyring found. Error info: There are no keyrings',
           );
         });
       });
@@ -1218,13 +1247,13 @@ describe('KeyringController', () => {
               '0x0000000000000000000000000000000000000000',
             ),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are 2 keyrings available, but none match the address',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
 
           await expect(
             controller.removeAccount('0xDUMMY_INPUT'),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: There are 2 keyrings available, but none match the address',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
@@ -1282,7 +1311,7 @@ describe('KeyringController', () => {
               from: '',
             }),
           ).rejects.toThrow(
-            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
@@ -1350,7 +1379,7 @@ describe('KeyringController', () => {
               from: '',
             }),
           ).rejects.toThrow(
-            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
@@ -1702,7 +1731,7 @@ describe('KeyringController', () => {
             expect(unsignedEthTx.v).toBeUndefined();
             await controller.signTransaction(unsignedEthTx, '');
           }).rejects.toThrow(
-            `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
@@ -2275,6 +2304,20 @@ describe('KeyringController', () => {
         },
       );
     });
+
+    it('should throw an error if there is no primary keyring', async () => {
+      await withController(async ({ controller, encryptor }) => {
+        await controller.setLocked();
+        jest
+          .spyOn(encryptor, 'decrypt')
+          .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
+        await controller.submitPassword('123');
+
+        await expect(controller.verifySeedPhrase()).rejects.toThrow(
+          'No HD keyring found',
+        );
+      });
+    });
   });
 
   describe('verifyPassword', () => {
@@ -2449,7 +2492,7 @@ describe('KeyringController', () => {
               await expect(
                 controller.withKeyring(selector, fn),
               ).rejects.toThrow(
-                `KeyringController - No keyring found. Error info: There are ${controller.state.keyrings.length} keyrings available, but none match the address`,
+                'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
               );
               expect(fn).not.toHaveBeenCalled();
             });
@@ -3476,7 +3519,7 @@ describe('KeyringController', () => {
             await expect(
               controller.exportAccount(password, mockAddress),
             ).rejects.toThrow(
-              'KeyringController - No keyring found. Error info: There are 1 keyrings available, but none match the address',
+              'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
             );
           },
         );
diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
index e12caa01e28..d6f79370c37 100644
--- a/packages/keyring-controller/src/KeyringController.ts
+++ b/packages/keyring-controller/src/KeyringController.ts
@@ -30,7 +30,6 @@ import type {
 } from '@metamask/utils';
 import {
   add0x,
-  assert,
   assertIsStrictHexString,
   bytesToHex,
   hasProperty,
@@ -662,7 +661,9 @@ export class KeyringController extends BaseController<
       const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as
         | EthKeyring<Json>
         | undefined;
-      assert(primaryKeyring, 'No HD Keyring found');
+      if (!primaryKeyring) {
+        throw new Error('No HD keyring found');
+      }
 
       const oldAccounts = await primaryKeyring.getAccounts();
 
@@ -923,24 +924,33 @@ export class KeyringController extends BaseController<
    */
   async getKeyringForAccount(account: string): Promise<unknown> {
     this.#assertIsUnlocked();
-
     const address = normalize(account);
-    assert(
-      address,
-      `${KeyringControllerError.NoKeyring}. Error infor: Invalid account address`,
-    );
 
-    const keyringIndex = this.state.keyrings.findIndex((keyring) =>
-      keyring.accounts.includes(address),
+    const candidates = await Promise.all(
+      this.#keyrings.map(async (keyring) => {
+        return Promise.all([keyring, keyring.getAccounts()]);
+      }),
     );
 
-    if (keyringIndex === -1 || !this.#keyrings[keyringIndex]) {
-      throw new Error(
-        `${KeyringControllerError.NoKeyring}. Error info: There are ${this.#keyrings.length} keyrings available, but none match the address`,
-      );
+    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];
     }
 
-    return this.#keyrings[keyringIndex];
+    // Adding more info to the error
+    let errorInfo = '';
+    if (!candidates.length) {
+      errorInfo = 'There are no keyrings';
+    } else if (!winners.length) {
+      errorInfo = 'There are keyrings, but none match the address';
+    }
+    throw new Error(
+      `${KeyringControllerError.NoKeyring}. Error info: ${errorInfo}`,
+    );
   }
 
   /**
@@ -1890,7 +1900,10 @@ export class KeyringController extends BaseController<
     const primaryKeyring = this.getKeyringsByType(KeyringTypes.hd)[0] as
       | EthKeyring<Json>
       | undefined;
-    assert(primaryKeyring, 'No HD keyring found.');
+    if (!primaryKeyring) {
+      throw new Error('No HD keyring found.');
+    }
+
     assertHasUint8ArrayMnemonic(primaryKeyring);
 
     const seedWords = primaryKeyring.mnemonic;