Dispose or destroy keyrings on reference drop (#233)

* feat: dispose or destroy keyrings on removal

* refactor: use for instead of while cycle

* rollback refactor

* test: refactor test case

* refactor: make dispose async

* test: add destroy method to keyring mock

* ci: adjust jest coverage threshold

Author: mikesposito

jest.config.js

@@ -41,10 +41,10 @@ module.exports = {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 70.37,
-      functions: 92.72,
-      lines: 90.72,
-      statements: 90.93,
+      branches: 70.45,
+      functions: 92.85,
+      lines: 90.57,
+      statements: 90.78,
     },
   },
   preset: 'ts-jest',

src/KeyringController.test.ts

@@ -78,6 +78,15 @@ describe('KeyringController', () => {
 
       expect(lockSpy.calledOnce).toBe(true);
     });
+
+    it('calls keyring optional destroy function', async () => {
+      const destroy = sinon.spy(KeyringMockWithInit.prototype, 'destroy');
+      await keyringController.addNewKeyring('Keyring Mock With Init');
+
+      await keyringController.setLocked();
+
+      expect(destroy.calledOnce).toBe(true);
+    });
   });
 
   describe('submitPassword', () => {
@@ -544,6 +553,18 @@ describe('KeyringController', () => {
       expect(keyringController.keyrings).toHaveLength(1);
     });
 
+    it('calls keyring optional destroy function', async () => {
+      const destroy = sinon.spy(KeyringMockWithInit.prototype, 'destroy');
+      const keyring = await keyringController.addNewKeyring(
+        'Keyring Mock With Init',
+      );
+      sinon.stub(keyringController, 'getKeyringForAccount').resolves(keyring);
+
+      await keyringController.removeAccount('0x0');
+
+      expect(destroy.calledOnce).toBe(true);
+    });
+
     it('does not remove the keyring if there are accounts remaining after removing one from the keyring', async () => {
       // Add a new keyring with two accounts
       await keyringController.addNewKeyring(KeyringType.HD, {

src/KeyringController.ts

@@ -175,8 +175,7 @@ class KeyringController extends EventEmitter {
     });
 
     // remove keyrings
-    this.keyrings = [];
-    await this.updateMemStoreKeyrings();
+    await this.#clearKeyrings();
     this.emit('lock');
     return this.fullUpdate();
   }
@@ -641,6 +640,8 @@ class KeyringController extends EventEmitter {
         const accounts = await keyring.getAccounts();
         if (accounts.length > 0) {
           validKeyrings.push(keyring);
+        } else {
+          await this.#disposeKeyring(keyring);
         }
       }),
     );
@@ -984,16 +985,40 @@ class KeyringController extends EventEmitter {
    * Clear Keyrings
    *
    * Deallocates all currently managed keyrings and accounts.
-   * Used before initializing a new vault.
+   * Used before initializing a new vault and after locking
+   * MetaMask.
    */
-  async #clearKeyrings(): Promise<void> {
+  async #clearKeyrings() {
     // clear keyrings from memory
+    for (const keyring of this.keyrings) {
+      await this.#disposeKeyring(keyring);
+    }
     this.keyrings = [];
     this.memStore.updateState({
       keyrings: [],
     });
   }
 
+  /**
+   * Dispose Keyring
+   *
+   * The Trezor Keyring has a method called `dispose` that removes the
+   * iframe from the DOM. The Ledger Keyring has a method called `destroy`
+   * that clears the keyring event listeners. This method checks if the provided
+   * keyring has either of these methods and calls them if they exist.
+   *
+   * @param keyring - The keyring to dispose or destroy.
+   */
+  async #disposeKeyring(keyring: Keyring<Json>) {
+    if ('dispose' in keyring && typeof keyring.dispose === 'function') {
+      await keyring.dispose();
+    }
+
+    if ('destroy' in keyring && typeof keyring.destroy === 'function') {
+      await keyring.destroy();
+    }
+  }
+
   /**
    * Unlock Keyrings
    *

src/test/keyring.mock.ts

@@ -33,6 +33,14 @@ class KeyringMockWithInit implements Keyring<Json> {
   async deserialize(_: any) {
     return Promise.resolve();
   }
+
+  async removeAccount(_: any) {
+    return Promise.resolve();
+  }
+
+  async destroy() {
+    return Promise.resolve();
+  }
 }
 
 export default KeyringMockWithInit;