refactor: move Device initialization to constructor

Author: mikesposito

packages/keyring-eth-qr/jest.config.js

@@ -20,10 +20,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 85.71,
+      branches: 91.8,
       functions: 100,
-      lines: 93.26,
-      statements: 93.39,
+      lines: 95.04,
+      statements: 95.14,
     },
   },
 });

packages/keyring-eth-qr/src/device.ts

@@ -121,6 +121,21 @@ export type IndexedAddress = {
 export type DeviceDetails = CommonDeviceDetails &
   (HDModeDeviceDetails | AccountModeDeviceDetails);
 
+export type DeviceOptions = {
+  /**
+   * The requestScan function to scan the QR code
+   */
+  requestScan: QrKeyringBridge['requestScan'];
+  /**
+   * The source of the device, which can be of type `DeviceDetails`,
+   * `string`, or `SerializedUR`.
+   *
+   * When a `string` or `SerializedUR` is provided, the Device will
+   * initialize itself from the UR.
+   */
+  source: DeviceDetails | string | SerializedUR;
+};
+
 /**
  * Get the fingerprint of the source CryptoAccount or CryptoHDKey
  *
@@ -189,32 +204,23 @@ function readCryptoAccountOutputDescriptors(source: CryptoAccount): {
 export class Device {
   readonly #requestScan: QrKeyringBridge['requestScan'];
 
-  #pairedDevice?: DeviceDetails | undefined;
-
-  constructor(requestScan: QrKeyringBridge['requestScan']) {
-    this.#requestScan = requestScan;
-  }
+  readonly #pairedDevice: DeviceDetails;
 
   /**
-   * Initialize the Device with a source.
+   * Create a new Device instance.
    *
-   * @param source - The account source, which can be a CryptoAccount, CryptoHDKey, or a UR string.
+   * @param options - The options for the Device, including the requestScan function
+   * and the source of the device details.
+   * @param options.requestScan - The function to request a scan of the QR code.
+   * @param options.source - The source of the device details, which can be a
+   * UR string, a `SerializedUR`, or a `DeviceDetails` object.
    */
-  init(source: DeviceDetails | string | SerializedUR): void {
-    if (typeof source === 'string' || 'cbor' in source) {
-      this.#initFromUR(source);
-    } else {
-      this.#pairedDevice = source;
-    }
-  }
-
-  /**
-   * Check if this class instance is paired with a device
-   *
-   * @returns True if the Device is initialized, false otherwise
-   */
-  isInitialized(): boolean {
-    return this.#pairedDevice !== undefined;
+  constructor({ requestScan, source }: DeviceOptions) {
+    this.#requestScan = requestScan;
+    this.#pairedDevice =
+      typeof source === 'string' || 'cbor' in source
+        ? this.#deviceDetailsFromUR(source)
+        : source;
   }
 
   /**
@@ -225,10 +231,6 @@ export class Device {
    * @throws Will throw an error if the source is not initialized
    */
   addressFromIndex(index: number): Hex {
-    if (!this.#pairedDevice) {
-      throw new Error('Device not paired');
-    }
-
     if (this.#pairedDevice.keyringMode === DeviceMode.ACCOUNT) {
       const address = Object.keys(this.#pairedDevice.paths)[index];
       if (!address) {
@@ -262,10 +264,6 @@ export class Device {
    * @returns The path of the address
    */
   pathFromAddress(address: Hex): string {
-    if (!this.#pairedDevice) {
-      throw new Error('UR not initialized');
-    }
-
     const normalizedAddress = getChecksumAddress(add0x(address));
 
     if (this.#pairedDevice.keyringMode === DeviceMode.ACCOUNT) {
@@ -289,10 +287,6 @@ export class Device {
    * @returns The index of the address
    */
   indexFromAddress(address: Hex): number {
-    if (!this.#pairedDevice) {
-      throw new Error('UR not initialized');
-    }
-
     const cachedIndex = this.#pairedDevice.indexes[address];
     if (cachedIndex !== undefined) {
       return Number(cachedIndex);
@@ -344,21 +338,14 @@ export class Device {
   }
 
   /**
-   * Retrieve the details of the paired device, if any.
+   * Retrieve the details of the paired device.
    *
-   * @returns Thea paired device details, or undefined if not initialized
+   * @returns Thea paired device details
    */
-  getDeviceDetails(): DeviceDetails | undefined {
+  getDeviceDetails(): DeviceDetails {
     return this.#pairedDevice;
   }
 
-  /**
-   * Clear the paired device details.
-   */
-  clear(): void {
-    this.#pairedDevice = undefined;
-  }
-
   /**
    * Sign a transaction. This is equivalent to the `eth_signTransaction`
    * Ethereum JSON-RPC method. See the Ethereum JSON-RPC API documentation for
@@ -372,10 +359,6 @@ export class Device {
     address: Hex,
     transaction: TypedTransaction,
   ): Promise<TypedTxData> {
-    if (!this.#pairedDevice?.xfp) {
-      throw new Error('No device paired.');
-    }
-
     const dataType =
       transaction.type === TransactionType.Legacy
         ? DataType.transaction
@@ -433,10 +416,6 @@ export class Device {
     address: Hex,
     data: TypedMessage<Types>,
   ): Promise<string> {
-    if (!this.#pairedDevice?.xfp) {
-      throw new Error('No device paired.');
-    }
-
     const requestId = uuidv4();
     const ethSignRequestUR = EthSignRequest.constructETHRequest(
       Buffer.from(JSON.stringify(data), 'utf8'),
@@ -478,10 +457,6 @@ export class Device {
    * @returns The signed message.
    */
   async signPersonalMessage(address: Hex, message: Hex): Promise<string> {
-    if (!this.#pairedDevice?.xfp) {
-      throw new Error('No device paired.');
-    }
-
     const requestId = uuidv4();
     const ethSignRequestUR = EthSignRequest.constructETHRequest(
       Buffer.from(remove0x(message), 'hex'),
@@ -511,38 +486,39 @@ export class Device {
   }
 
   /**
-   * Set the paired device from a UR string
+   * Derive the device details from a UR string
    *
    * @param ur - The UR string to set the root account from
+   * @returns The device details derived from the UR
    */
-  #initFromUR(ur: string | SerializedUR): void {
+  #deviceDetailsFromUR(ur: string | SerializedUR): DeviceDetails {
     const source = this.#decodeUR(ur);
     const fingerprint = getFingerprintFromSource(source);
 
     if (source instanceof CryptoAccount) {
       const { name, xfp, paths, keyringAccount } =
         readCryptoAccountOutputDescriptors(source);
-      this.#pairedDevice = {
+      return {
         keyringMode: DeviceMode.ACCOUNT,
         keyringAccount,
         name,
         xfp,
         paths,
         indexes: {},
       };
-    } else {
-      const { getBip32Key, getOrigin, getChildren, getName, getNote } = source;
-      this.#pairedDevice = {
-        keyringMode: DeviceMode.HD,
-        keyringAccount: getNote(),
-        name: getName(),
-        xfp: fingerprint,
-        hdPath: `m/${getOrigin().getPath()}`,
-        childrenPath: getChildren()?.getPath() || DEFAULT_CHILDREN_PATH,
-        xpub: getBip32Key(),
-        indexes: {},
-      };
     }
+
+    const { getBip32Key, getOrigin, getChildren, getName, getNote } = source;
+    return {
+      keyringMode: DeviceMode.HD,
+      keyringAccount: getNote(),
+      name: getName(),
+      xfp: fingerprint,
+      hdPath: `m/${getOrigin().getPath()}`,
+      childrenPath: getChildren()?.getPath() || DEFAULT_CHILDREN_PATH,
+      xpub: getBip32Key(),
+      indexes: {},
+    };
   }
 
   /**

packages/keyring-eth-qr/src/qr-keyring.test.ts

@@ -63,17 +63,23 @@ function getMockBridge(): QrKeyringBridge {
  *
  * @param bridge - The QrKeyringBridge instance to mock.
  * @param signature - The signature to return in the mocked scan resolution.
+ * @param requestId - Optional request ID to use in the signature UR.
  */
 function mockBridgeScanResolution(
   bridge: QrKeyringBridge,
   signature: string,
+  requestId?: string | null,
 ): void {
   jest.spyOn(bridge, 'requestScan').mockImplementationOnce(async (request) => {
     const signatureUR = new ETHSignature(
       Buffer.from(signature, 'hex'),
-      Buffer.from(
-        Uint8Array.from(uuid.parse(request.request?.requestId ?? '')),
-      ),
+      requestId === null
+        ? undefined
+        : Buffer.from(
+            Uint8Array.from(
+              uuid.parse(requestId ?? request.request?.requestId ?? ''),
+            ),
+          ),
     ).toUR();
     return {
       type: signatureUR.type,
@@ -332,7 +338,7 @@ describe('QrKeyring', () => {
         });
 
         await expect(keyring.addAccounts(1)).rejects.toThrow(
-          'Device not paired',
+          'No device paired.',
         );
       });
     });
@@ -440,13 +446,13 @@ describe('QrKeyring', () => {
     });
   });
 
-  describe('submitUR', () => {
-    it('initializes the QrKeyring with a UR', async () => {
+  describe('pairDevice', () => {
+    it('pairs a device to the QrKeyring with a UR', async () => {
       const keyring = new QrKeyring({
         bridge: getMockBridge(),
       });
 
-      keyring.submitUR(KNOWN_HDKEY_UR);
+      keyring.pairDevice(KNOWN_HDKEY_UR);
 
       expect(await getXPUBFromKeyring(keyring)).toStrictEqual(
         SERIALIZED_KEYSTONE_KEYRING.xpub,
@@ -458,7 +464,7 @@ describe('QrKeyring', () => {
         bridge: getMockBridge(),
       });
 
-      expect(() => keyring.submitUR('invalid-ur')).toThrow('Invalid Scheme');
+      expect(() => keyring.pairDevice('invalid-ur')).toThrow('Invalid Scheme');
     });
 
     it('throws an error if the UR is not of type `crypto-hdkey` or `crypto-account`', () => {
@@ -467,7 +473,7 @@ describe('QrKeyring', () => {
       });
 
       expect(() =>
-        keyring.submitUR({
+        keyring.pairDevice({
           type: 'invalid-type',
           cbor: 'a0',
         }),
@@ -711,6 +717,38 @@ describe('QrKeyring', () => {
           expect(signedTx.to).toStrictEqual(LEGACY_TRANSACTION.to);
           expect(signedTx.nonce).toBe(LEGACY_TRANSACTION.nonce);
         });
+
+        it('throws an error if the signature scan `requestId` is wrong', async () => {
+          const keyring = new QrKeyring({
+            bridge: getMockBridge(),
+            ur,
+          });
+          mockBridgeScanResolution(
+            keyring.bridge,
+            '33ea4c1dc4b201ad1b1feaf172aadf60dcf2f8bd76d941396bfaebfc3b2868b0340d5689341925c99cdea39e3c5daf7fe2776f220e5b018e85d3b1df19c7bc4701',
+            '92fdd736-e03e-4650-9992-ff3c70f16f4e',
+          );
+
+          await expect(
+            keyring.signTransaction(from, TRANSACTION),
+          ).rejects.toThrow(/Signature request ID mismatch./u);
+        });
+
+        it('throws an error if the signature scan `requestId` is missing', async () => {
+          const keyring = new QrKeyring({
+            bridge: getMockBridge(),
+            ur,
+          });
+          mockBridgeScanResolution(
+            keyring.bridge,
+            '33ea4c1dc4b201ad1b1feaf172aadf60dcf2f8bd76d941396bfaebfc3b2868b0340d5689341925c99cdea39e3c5daf7fe2776f220e5b018e85d3b1df19c7bc4701',
+            null,
+          );
+
+          await expect(
+            keyring.signTransaction(from, TRANSACTION),
+          ).rejects.toThrow('Signature request ID is missing.');
+        });
       },
     );
   });