fix: Revert "refactor: use withKeyring method (#25435)" (#26041)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

[Revert "refactor: use
withKeyring](#25435)


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26041?quickstart=1)

## **Related issues**

Fixes: ci failures for Create Snap accounts

## **Manual testing steps**

1. check ci for ff and ff flask 


https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/92931/workflows/0e8fb7fe-e5bb-4cec-816c-eed6a97c0d45/jobs/3461246/steps


https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/92931/workflows/0e8fb7fe-e5bb-4cec-816c-eed6a97c0d45/jobs/3461243

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

Author: seaona

app/scripts/metamask-controller.js

@@ -142,7 +142,6 @@ import {
 import { Interface } from '@ethersproject/abi';
 import { abiERC1155, abiERC721 } from '@metamask/metamask-eth-abis';
 import { isEvmAccountType } from '@metamask/keyring-api';
-import { normalize } from '@metamask/eth-sig-util';
 import {
   methodsRequiringNetworkSwitch,
   methodsWithConfirmation,
@@ -4046,10 +4045,7 @@ export default class MetamaskController extends EventEmitter {
         // keyring's iframe and have the setting initialized properly
         // Optimistically called to not block MetaMask login due to
         // Ledger Keyring GitHub downtime
-        this.withKeyringForDevice(
-          { name: HardwareDeviceNames.ledger },
-          async (keyring) => this.setLedgerTransportPreference(keyring),
-        );
+        this.setLedgerTransportPreference();
       }
     } finally {
       releaseLock();
@@ -4179,10 +4175,7 @@ export default class MetamaskController extends EventEmitter {
     // Optimistically called to not block MetaMask login due to
     // Ledger Keyring GitHub downtime
     if (completedOnboarding) {
-      this.withKeyringForDevice(
-        { name: HardwareDeviceNames.ledger },
-        async (keyring) => this.setLedgerTransportPreference(keyring),
-      );
+      this.setLedgerTransportPreference();
     }
   }
 
@@ -4286,74 +4279,50 @@ export default class MetamaskController extends EventEmitter {
   // Hardware
   //
 
-  /**
-   * Select a hardware wallet device and execute a
-   * callback with the keyring for that device.
-   *
-   * Note that KeyringController state is not updated before
-   * the end of the callback execution, and calls to KeyringController
-   * methods within the callback can lead to deadlocks.
-   *
-   * @param {object} options - The options for the device
-   * @param {string} options.name - The device name to select
-   * @param {string} options.hdPath - An optional hd path to be set on the device
-   * keyring
-   * @param {*} callback - The callback to execute with the keyring
-   * @returns {*} The result of the callback
-   */
-  async withKeyringForDevice(options, callback) {
+  async getKeyringForDevice(deviceName, hdPath = null) {
     const keyringOverrides = this.opts.overrides?.keyrings;
-    let keyringType = null;
-    switch (options.name) {
+    let keyringName = null;
+    switch (deviceName) {
       case HardwareDeviceNames.trezor:
-        keyringType = keyringOverrides?.trezor?.type || TrezorKeyring.type;
+        keyringName = keyringOverrides?.trezor?.type || TrezorKeyring.type;
         break;
       case HardwareDeviceNames.ledger:
-        keyringType = keyringOverrides?.ledger?.type || LedgerKeyring.type;
+        keyringName = keyringOverrides?.ledger?.type || LedgerKeyring.type;
         break;
       case HardwareDeviceNames.qr:
-        keyringType = QRHardwareKeyring.type;
+        keyringName = QRHardwareKeyring.type;
         break;
       case HardwareDeviceNames.lattice:
-        keyringType = keyringOverrides?.lattice?.type || LatticeKeyring.type;
+        keyringName = keyringOverrides?.lattice?.type || LatticeKeyring.type;
         break;
       default:
         throw new Error(
-          'MetamaskController:withKeyringForDevice - Unknown device',
+          'MetamaskController:getKeyringForDevice - Unknown device',
         );
     }
+    let [keyring] = await this.keyringController.getKeyringsByType(keyringName);
+    if (!keyring) {
+      keyring = await this.keyringController.addNewKeyring(keyringName);
+    }
+    if (hdPath && keyring.setHdPath) {
+      keyring.setHdPath(hdPath);
+    }
+    if (deviceName === HardwareDeviceNames.lattice) {
+      keyring.appName = 'MetaMask';
+    }
+    if (deviceName === HardwareDeviceNames.trezor) {
+      const model = keyring.getModel();
+      this.appStateController.setTrezorModel(model);
+    }
 
-    return this.keyringController.withKeyring(
-      { type: keyringType },
-      async (keyring) => {
-        if (options.hdPath && keyring.setHdPath) {
-          keyring.setHdPath(options.hdPath);
-        }
-
-        if (options.name === HardwareDeviceNames.lattice) {
-          keyring.appName = 'MetaMask';
-        }
-
-        if (options.name === HardwareDeviceNames.trezor) {
-          const model = keyring.getModel();
-          this.appStateController.setTrezorModel(model);
-        }
-
-        keyring.network = this.networkController.state.providerConfig.type;
+    keyring.network = this.networkController.state.providerConfig.type;
 
-        return await callback(keyring);
-      },
-      {
-        createIfMissing: true,
-      },
-    );
+    return keyring;
   }
 
   async attemptLedgerTransportCreation() {
-    return await this.withKeyringForDevice(
-      HardwareDeviceNames.ledger,
-      async (keyring) => keyring.attemptMakeApp(),
-    );
+    const keyring = await this.getKeyringForDevice(HardwareDeviceNames.ledger);
+    return await keyring.attemptMakeApp();
   }
 
   /**
@@ -4365,38 +4334,35 @@ export default class MetamaskController extends EventEmitter {
    * @returns [] accounts
    */
   async connectHardware(deviceName, page, hdPath) {
-    return this.withKeyringForDevice(
-      { name: deviceName, hdPath },
-      async (keyring) => {
-        if (deviceName === HardwareDeviceNames.ledger) {
-          await this.setLedgerTransportPreference(keyring);
-        }
+    const keyring = await this.getKeyringForDevice(deviceName, hdPath);
 
-        let accounts = [];
-        switch (page) {
-          case -1:
-            accounts = await keyring.getPreviousPage();
-            break;
-          case 1:
-            accounts = await keyring.getNextPage();
-            break;
-          default:
-            accounts = await keyring.getFirstPage();
-        }
+    if (deviceName === HardwareDeviceNames.ledger) {
+      await this.setLedgerTransportPreference(keyring);
+    }
 
-        // Merge with existing accounts
-        // and make sure addresses are not repeated
-        const oldAccounts = await this.keyringController.getAccounts();
+    let accounts = [];
+    switch (page) {
+      case -1:
+        accounts = await keyring.getPreviousPage();
+        break;
+      case 1:
+        accounts = await keyring.getNextPage();
+        break;
+      default:
+        accounts = await keyring.getFirstPage();
+    }
 
-        const accountsToTrack = [
-          ...new Set(
-            oldAccounts.concat(accounts.map((a) => a.address.toLowerCase())),
-          ),
-        ];
-        this.accountTracker.syncWithAddresses(accountsToTrack);
-        return accounts;
-      },
-    );
+    // Merge with existing accounts
+    // and make sure addresses are not repeated
+    const oldAccounts = await this.keyringController.getAccounts();
+
+    const accountsToTrack = [
+      ...new Set(
+        oldAccounts.concat(accounts.map((a) => a.address.toLowerCase())),
+      ),
+    ];
+    this.accountTracker.syncWithAddresses(accountsToTrack);
+    return accounts;
   }
 
   /**
@@ -4407,12 +4373,8 @@ export default class MetamaskController extends EventEmitter {
    * @returns {Promise<boolean>}
    */
   async checkHardwareStatus(deviceName, hdPath) {
-    return this.withKeyringForDevice(
-      { name: deviceName, hdPath },
-      async (keyring) => {
-        return keyring.isUnlocked();
-      },
-    );
+    const keyring = await this.getKeyringForDevice(deviceName, hdPath);
+    return keyring.isUnlocked();
   }
 
   /**
@@ -4422,15 +4384,14 @@ export default class MetamaskController extends EventEmitter {
    * @returns {Promise<boolean>}
    */
   async forgetDevice(deviceName) {
-    return this.withKeyringForDevice({ name: deviceName }, async (keyring) => {
-      for (const address of keyring.accounts) {
-        await this._onAccountRemoved(address);
-      }
+    const keyring = await this.getKeyringForDevice(deviceName);
 
-      keyring.forgetDevice();
+    for (const address of keyring.accounts) {
+      await this.removeAccount(address);
+    }
 
-      return true;
-    });
+    keyring.forgetDevice();
+    return true;
   }
 
   /**
@@ -4468,22 +4429,21 @@ export default class MetamaskController extends EventEmitter {
    * @returns {'ledger' | 'lattice' | string | undefined}
    */
   async getDeviceModel(address) {
-    return this.keyringController.withKeyring({ address }, async (keyring) => {
-      switch (keyring.type) {
-        case KeyringType.trezor:
-          return keyring.getModel();
-        case KeyringType.qr:
-          return keyring.getName();
-        case KeyringType.ledger:
-          // TODO: get model after ledger keyring exposes method
-          return HardwareDeviceNames.ledger;
-        case KeyringType.lattice:
-          // TODO: get model after lattice keyring exposes method
-          return HardwareDeviceNames.lattice;
-        default:
-          return undefined;
-      }
-    });
+    const keyring = await this.keyringController.getKeyringForAccount(address);
+    switch (keyring.type) {
+      case KeyringType.trezor:
+        return keyring.getModel();
+      case KeyringType.qr:
+        return keyring.getName();
+      case KeyringType.ledger:
+        // TODO: get model after ledger keyring exposes method
+        return HardwareDeviceNames.ledger;
+      case KeyringType.lattice:
+        // TODO: get model after lattice keyring exposes method
+        return HardwareDeviceNames.lattice;
+      default:
+        return undefined;
+    }
   }
 
   /**
@@ -4513,24 +4473,16 @@ export default class MetamaskController extends EventEmitter {
     hdPath,
     hdPathDescription,
   ) {
-    const { address: unlockedAccount, label } = await this.withKeyringForDevice(
-      { name: deviceName, hdPath },
-      async (keyring) => {
-        keyring.setAccountToUnlock(index);
-        const [address] = await keyring.addAccounts(1);
-        return {
-          address: normalize(address),
-          label: this.getAccountLabel(
-            deviceName === HardwareDeviceNames.qr
-              ? keyring.getName()
-              : deviceName,
-            index,
-            hdPathDescription,
-          ),
-        };
-      },
+    const keyring = await this.getKeyringForDevice(deviceName, hdPath);
+
+    keyring.setAccountToUnlock(index);
+    const unlockedAccount =
+      await this.keyringController.addNewAccountForKeyring(keyring);
+    const label = this.getAccountLabel(
+      deviceName === HardwareDeviceNames.qr ? keyring.getName() : deviceName,
+      index,
+      hdPathDescription,
     );
-
     // Set the account label to Trezor 1 / Ledger 1 / QR Hardware 1, etc
     this.preferencesController.setAccountLabel(unlockedAccount, label);
     // Select the account
@@ -4706,8 +4658,20 @@ export default class MetamaskController extends EventEmitter {
    * @param {string[]} address - A hex address
    */
   async removeAccount(address) {
-    await this._onAccountRemoved(address);
+    // Remove all associated permissions
+    this.removeAllAccountPermissions(address);
+
+    ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
+    this.custodyController.removeAccount(address);
+    ///: END:ONLY_INCLUDE_IF(build-mmi)
+
+    const keyring = await this.keyringController.getKeyringForAccount(address);
+    // Remove account from the keyring
     await this.keyringController.removeAccount(address);
+    const updatedKeyringAccounts = keyring ? await keyring.getAccounts() : {};
+    if (updatedKeyringAccounts?.length === 0) {
+      keyring.destroy?.();
+    }
 
     return address;
   }
@@ -5920,21 +5884,6 @@ export default class MetamaskController extends EventEmitter {
     this._notifyChainChange();
   }
 
-  /**
-   * Execute side effects of a removed account.
-   *
-   * @param {string} address - The address of the account to remove.
-   * @returns {Promise<void>}
-   */
-  async _onAccountRemoved(address) {
-    // Remove all associated permissions
-    this.removeAllAccountPermissions(address);
-
-    ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
-    this.custodyController.removeAccount(address);
-    ///: END:ONLY_INCLUDE_IF(build-mmi)
-  }
-
   // misc
 
   /**
@@ -6209,15 +6158,16 @@ export default class MetamaskController extends EventEmitter {
   /**
    * Sets the Ledger Live preference to use for Ledger hardware wallet support
    *
-   * @param keyring
+   * @param _keyring
    * @deprecated This method is deprecated and will be removed in the future.
    * Only webhid connections are supported in chrome and u2f in firefox.
    */
-  async setLedgerTransportPreference(keyring) {
+  async setLedgerTransportPreference(_keyring) {
     const transportType = window.navigator.hid
       ? LedgerTransportTypes.webhid
       : LedgerTransportTypes.u2f;
-
+    const keyring =
+      _keyring || (await this.getKeyringForDevice(HardwareDeviceNames.ledger));
     if (keyring?.updateTransportMethod) {
       return keyring.updateTransportMethod(transportType).catch((e) => {
         throw e;