refactor: remove qr assertion

MetaMask · Jul 18, 2023 · af77e34 · af77e34
1 parent 4c5e4af
commit af77e34
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 103 deletions.
diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
@@ -11,19 +11,22 @@ import {
   SignTypedDataVersion,
 } from '@metamask/eth-sig-util';
 import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english';
-import { isValidHexAddress, type Hex } from '@metamask/utils';
+import {
+  isValidHexAddress,
+  type Hex,
+  type Keyring,
+  type Json,
+} from '@metamask/utils';
 import { bufferToHex } from 'ethereumjs-util';
 import * as sinon from 'sinon';
 import * as uuid from 'uuid';
 
-import {
-  type KeyringControllerEvents,
-  type KeyringControllerMessenger,
-  type KeyringControllerState,
-  type KeyringControllerOptions,
-  type KeyringControllerActions,
-  assertHasUint8ArrayMnemonic,
-  assertIsQRKeyring,
+import type {
+  KeyringControllerEvents,
+  KeyringControllerMessenger,
+  KeyringControllerState,
+  KeyringControllerOptions,
+  KeyringControllerActions,
 } from './KeyringController';
 import {
   AccountImportStrategy,
@@ -348,24 +351,42 @@ describe('KeyringController', () => {
   });
 
   describe('exportSeedPhrase', () => {
-    describe('when correct password is provided', () => {
-      it('should export seed phrase', async () => {
+    describe('when mnemonic is not exportable', () => {
+      it('should throw error', async () => {
         await withController(async ({ controller }) => {
-          const seed = await controller.exportSeedPhrase(password);
-          expect(seed).not.toBe('');
+          const primaryKeyring = controller.getKeyringsByType(
+            KeyringTypes.hd,
+          )[0] as Keyring<Json> & { mnemonic: string };
+
+          primaryKeyring.mnemonic = '';
+
+          await expect(controller.exportSeedPhrase(password)).rejects.toThrow(
+            "Can't get mnemonic bytes from keyring",
+          );
         });
       });
     });
 
-    describe('when wrong password is provided', () => {
-      it('should export seed phrase', async () => {
-        await withController(async ({ controller, encryptor }) => {
-          sinon
-            .stub(encryptor, 'decrypt')
-            .throws(new Error('Invalid password'));
-          await expect(controller.exportSeedPhrase('')).rejects.toThrow(
-            'Invalid password',
-          );
+    describe('when mnemonic is exportable', () => {
+      describe('when correct password is provided', () => {
+        it('should export seed phrase', async () => {
+          await withController(async ({ controller }) => {
+            const seed = await controller.exportSeedPhrase(password);
+            expect(seed).not.toBe('');
+          });
+        });
+      });
+
+      describe('when wrong password is provided', () => {
+        it('should export seed phrase', async () => {
+          await withController(async ({ controller, encryptor }) => {
+            sinon
+              .stub(encryptor, 'decrypt')
+              .throws(new Error('Invalid password'));
+            await expect(controller.exportSeedPhrase('')).rejects.toThrow(
+              'Invalid password',
+            );
+          });
         });
       });
     });
@@ -437,10 +458,10 @@ describe('KeyringController', () => {
         await withController(async ({ controller }) => {
           const normalizedInitialAccounts =
             controller.state.keyrings[0].accounts.map(normalize);
-          const keyring = await controller.getKeyringForAccount(
+          const keyring = (await controller.getKeyringForAccount(
             // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
             normalizedInitialAccounts[0]!,
-          );
+          )) as Keyring<Json>;
           expect(keyring.type).toBe('HD Key Tree');
           expect(keyring.getAccounts()).toStrictEqual(
             normalizedInitialAccounts,
@@ -468,7 +489,9 @@ describe('KeyringController', () => {
     describe('when existing type is provided', () => {
       it('should return keyrings of the right type', async () => {
         await withController(async ({ controller }) => {
-          const keyrings = controller.getKeyringsByType(KeyringTypes.hd);
+          const keyrings = controller.getKeyringsByType(
+            KeyringTypes.hd,
+          ) as Keyring<Json>[];
           expect(keyrings).toHaveLength(1);
           expect(keyrings[0].type).toBe(KeyringTypes.hd);
           expect(keyrings[0].getAccounts()).toStrictEqual(
@@ -1206,6 +1229,20 @@ describe('KeyringController', () => {
         expect(seedPhrase).toBeInstanceOf(Uint8Array);
       });
     });
+
+    it('should throw if mnemonic is not defined', async () => {
+      await withController(async ({ controller }) => {
+        const primaryKeyring = controller.getKeyringsByType(
+          KeyringTypes.hd,
+        )[0] as Keyring<Json> & { mnemonic: string };
+
+        primaryKeyring.mnemonic = '';
+
+        await expect(controller.verifySeedPhrase()).rejects.toThrow(
+          "Can't get mnemonic bytes from keyring",
+        );
+      });
+    });
   });
 
   describe('verifyPassword', () => {
@@ -1696,58 +1733,6 @@ describe('KeyringController', () => {
   });
 });
 
-describe('Type guards and assertions', () => {
-  describe('assertHasUint8ArrayMnemonic', () => {
-    it('should not throw if mnemonic is defined', async () => {
-      await withController(async ({ controller }) => {
-        const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
-        expect(() => assertHasUint8ArrayMnemonic(keyring)).not.toThrow();
-      });
-    });
-
-    it('should throw if mnemonic is not defined', async () => {
-      await withController(async ({ controller }) => {
-        await controller.importAccountWithStrategy(
-          AccountImportStrategy.privateKey,
-          [privateKey],
-        );
-
-        const keyring = await controller.getKeyringForAccount(
-          '0x51253087e6f8358b5f10c0a94315d69db3357859',
-        );
-
-        expect(() => assertHasUint8ArrayMnemonic(keyring)).toThrow(
-          "Can't get mnemonic bytes from keyring",
-        );
-      });
-    });
-  });
-
-  describe('assertIsQRKeyring', () => {
-    it('should not throw if keyring is a QRKeyring', async () => {
-      const keyring = new QRKeyring();
-      expect(() => assertIsQRKeyring(keyring)).not.toThrow();
-    });
-
-    it('should throw if keyring is not QRKeyring', async () => {
-      await withController(async ({ controller }) => {
-        await controller.importAccountWithStrategy(
-          AccountImportStrategy.privateKey,
-          [privateKey],
-        );
-
-        const keyring = await controller.getKeyringForAccount(
-          '0x51253087e6f8358b5f10c0a94315d69db3357859',
-        );
-
-        expect(() => assertIsQRKeyring(keyring)).toThrow(
-          'Expected QRKeyring instance',
-        );
-      });
-    });
-  });
-});
-
 type WithControllerCallback<ReturnValue> = ({
   controller,
   preferences,

diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
@@ -1,6 +1,6 @@
 import type { TxData, TypedTransaction } from '@ethereumjs/tx';
 import {
-  MetaMaskKeyring as QRKeyring,
+  type MetaMaskKeyring as QRKeyring,
   type IKeyringState as IQRKeyringState,
 } from '@keystonehq/metamask-airgapped-keyring';
 import type { RestrictedControllerMessenger } from '@metamask/base-controller';
@@ -170,7 +170,7 @@ const defaultState: KeyringControllerState = {
  * @param keyring - The keyring to check
  * @throws When the keyring does not have a mnemonic
  */
-export function assertHasUint8ArrayMnemonic(
+function assertHasUint8ArrayMnemonic(
   keyring: Keyring<Json>,
 ): asserts keyring is Keyring<Json> & { mnemonic: Uint8Array } {
   if (
@@ -182,22 +182,6 @@ export function assertHasUint8ArrayMnemonic(
   }
 }
 
-/**
- * Assert that the passed keyring is a QRKeyring.
- * This is needed as currently `@keystonehq/metamask-airgapped-keyring`
- * is not compatible with the `Keyring` type from `@metamask/utils`.
- *
- * @param keyring - The keyring to check
- * @throws When the keyring is not an instance of QRKeyring
- */
-export function assertIsQRKeyring(
-  keyring: unknown,
-): asserts keyring is QRKeyring {
-  if (!(keyring instanceof QRKeyring)) {
-    throw new Error('Expected QRKeyring instance');
-  }
-}
-
 /**
  * Controller responsible for establishing and managing user identity.
  *
@@ -465,7 +449,7 @@ export class KeyringController extends BaseControllerV2<
    * @param account - An account address.
    * @returns Promise resolving to keyring of the `account` if one exists.
    */
-  async getKeyringForAccount(account: string): Promise<Keyring<Json>> {
+  async getKeyringForAccount(account: string): Promise<unknown> {
     return this.#keyring.getKeyringForAccount(account);
   }
 
@@ -478,7 +462,7 @@ export class KeyringController extends BaseControllerV2<
    * @param type - Keyring type name.
    * @returns An array of keyrings of the given type.
    */
-  getKeyringsByType(type: KeyringTypes | string): Keyring<Json>[] {
+  getKeyringsByType(type: KeyringTypes | string): unknown[] {
     return this.#keyring.getKeyringsByType(type);
   }
 
@@ -793,12 +777,8 @@ export class KeyringController extends BaseControllerV2<
    * @returns The added keyring
    */
   async getOrAddQRKeyring(): Promise<QRKeyring> {
-    const keyring =
-      this.#keyring.getKeyringsByType(KeyringTypes.qr)[0] ||
-      (await this.addQRKeyring());
-
-    assertIsQRKeyring(keyring);
-
+    const keyring = (this.#keyring.getKeyringsByType(KeyringTypes.qr)[0] ||
+      (await this.addQRKeyring())) as unknown as QRKeyring;
     return keyring;
   }