feat(272): add chainId to Keyring API requests (transaction/typed message)

src/SnapKeyring.test.ts

@@ -540,14 +540,14 @@ describe('SnapKeyring', () => {
   describe('signTransaction', () => {
     it('signs a ethereum transaction synchronously', async () => {
       const mockTx = {
-        data: '0x0',
+        data: '0x00',
         gasLimit: '0x26259fe',
         gasPrice: '0x1',
         nonce: '0xfffffffe',
         to: '0xccccccccccccd000000000000000000000000000',
         value: '0x1869e',
         chainId: '0x1',
-        type: '0x00',
+        type: '0x0',
       };
       const mockSignedTx = {
         ...mockTx,
@@ -557,11 +557,38 @@ describe('SnapKeyring', () => {
       };
       const tx = TransactionFactory.fromTxData(mockTx);
       const expectedSignedTx = TransactionFactory.fromTxData(mockSignedTx);
+      const expectedScope = 'eip155:1';
+
       mockSnapController.handleRequest.mockResolvedValue({
         pending: false,
         result: mockSignedTx,
       });
+
       const signature = await keyring.signTransaction(accounts[0].address, tx);
+      expect(mockSnapController.handleRequest).toHaveBeenCalledWith({
+        snapId,
+        handler: 'onKeyringRequest',
+        origin: 'metamask',
+        request: {
+          id: expect.any(String),
+          jsonrpc: '2.0',
+          method: 'keyring_submitRequest',
+          params: {
+            id: expect.any(String),
+            scope: expectedScope,
+            account: accounts[0].id,
+            request: {
+              method: 'eth_signTransaction',
+              params: [
+                {
+                  ...mockTx,
+                  from: accounts[0].address,
+                },
+              ],
+            },
+          },
+        },
+      });
       expect(signature).toStrictEqual(expectedSignedTx);
     });
   });
@@ -605,6 +632,7 @@ describe('SnapKeyring', () => {
       },
     };
 
+    const expectedScope = 'eip155:1';
     const expectedSignature =
       '0x4355c47d63924e8a72e509b65029052eb6c299d53a04e167c5775fd466751c9d07299936d304c153f6443dfa05f40ff007d72911b6f72307f996231605b915621c';
 
@@ -628,7 +656,7 @@ describe('SnapKeyring', () => {
           method: 'keyring_submitRequest',
           params: {
             id: expect.any(String),
-            scope: expect.any(String),
+            scope: expectedScope,
             account: accounts[0].id,
             request: {
               method: 'eth_signTypedData_v1',
@@ -661,7 +689,7 @@ describe('SnapKeyring', () => {
           method: 'keyring_submitRequest',
           params: {
             id: expect.any(String),
-            scope: expect.any(String),
+            scope: expectedScope,
             account: accounts[0].id,
             request: {
               method: 'eth_signTypedData_v4',
@@ -694,7 +722,7 @@ describe('SnapKeyring', () => {
           method: 'keyring_submitRequest',
           params: {
             id: expect.any(String),
-            scope: expect.any(String),
+            scope: expectedScope,
             account: accounts[0].id,
             request: {
               method: 'eth_signTypedData_v1',
@@ -706,6 +734,53 @@ describe('SnapKeyring', () => {
       expect(signature).toStrictEqual(expectedSignature);
     });
 
+    it('signs typed data without domain chainId has no scope', async () => {
+      mockSnapController.handleRequest.mockResolvedValue({
+        pending: false,
+        result: expectedSignature,
+      });
+
+      const dataToSignWithoutDomainChainId = {
+        ...dataToSign,
+        domain: {
+          name: dataToSign.domain.name,
+          version: dataToSign.domain.version,
+          verifyingContract: dataToSign.domain.verifyingContract,
+          // We do not defined the chainId (it's currently marked as
+          // optional in the current type declaration).
+          // chainId: 1,
+        },
+      };
+
+      const signature = await keyring.signTypedData(
+        accounts[0].address,
+        dataToSignWithoutDomainChainId,
+        { version: SignTypedDataVersion.V4 },
+      );
+      expect(mockSnapController.handleRequest).toHaveBeenCalledWith({
+        snapId,
+        handler: 'onKeyringRequest',
+        origin: 'metamask',
+        request: {
+          id: expect.any(String),
+          jsonrpc: '2.0',
+          method: 'keyring_submitRequest',
+          params: {
+            id: expect.any(String),
+            // Without chainId alongside the typed message, we cannot
+            // compute the scope for this request!
+            scope: '', // Default value for `signTypedTransaction`
+            account: accounts[0].id,
+            request: {
+              method: 'eth_signTypedData_v4',
+              params: [accounts[0].address, dataToSignWithoutDomainChainId],
+            },
+          },
+        },
+      });
+      expect(signature).toStrictEqual(expectedSignature);
+    });
+
     it('calls eth_prepareUserOperation', async () => {
       const baseTxs = [
         {

src/SnapKeyring.ts

@@ -30,6 +30,7 @@ import { EventEmitter } from 'events';
 import { assert, mask, object, string } from 'superstruct';
 import { v4 as uuid } from 'uuid';
 
+import { toCaipChainId, CaipNamespaces } from './caip';
 import { DeferredPromise } from './DeferredPromise';
 import { KeyringSnapControllerClient } from './KeyringSnapControllerClient';
 import { SnapIdMap } from './SnapIdMap';
@@ -454,17 +455,19 @@ export class SnapKeyring extends EventEmitter {
     transaction: TypedTransaction,
     _opts = {},
   ): Promise<Json | TypedTransaction> {
+    const chainId = transaction.common.chainId();
     const tx = toJson({
       ...transaction.toJSON(),
       from: address,
       type: `0x${transaction.type.toString(16)}`,
-      chainId: bigIntToHex(transaction.common.chainId()),
+      chainId: bigIntToHex(chainId),
     });
 
     const signedTx = await this.#submitRequest({
       address,
       method: EthMethod.SignTransaction,
       params: [tx],
+      chainId: toCaipChainId(CaipNamespaces.Eip155, `${chainId}`),
     });
 
     // ! It's *** CRITICAL *** that we mask the signature here, otherwise the
@@ -509,11 +512,18 @@ export class SnapKeyring extends EventEmitter {
     // used if the version is not specified or not supported.
     const method = methods[opts.version] || EthMethod.SignTypedDataV1;
 
+    // Extract chain ID as if it was a typed message (as defined by EIP-712), if
+    // input is not a typed message, then chain ID will be undefined!
+    const chainId = (data as TypedMessage<any>).domain?.chainId;
+
     return strictMask(
       await this.#submitRequest({
         address,
         method,
         params: toJson<Json[]>([address, data]),
+        ...(chainId === undefined
+          ? {}
+          : { chainId: toCaipChainId(CaipNamespaces.Eip155, `${chainId}`) }),
       }),
       EthBytesStruct,
     );

src/caip.test.ts

@@ -0,0 +1,46 @@
+import { CAIP_NAMESPACE_REGEX, CAIP_REFERENCE_REGEX } from '@metamask/utils';
+
+import { toCaipChainId, CaipNamespaces } from './caip';
+
+describe('toCaipChainId', () => {
+  // This function relies on @metamask/utils CAIP helpers. Those are being
+  // tested with a variety of inputs.
+  // Here we mainly focus on our own wrapper around those:
+
+  it('returns a valid CAIP-2 chain ID', () => {
+    const namespace = CaipNamespaces.Eip155;
+    const reference = '1';
+    expect(toCaipChainId(namespace, reference)).toBe(
+      `${namespace}:${reference}`,
+    );
+  });
+
+  it.each([
+    // Too short, must have 3 chars at least
+    '',
+    'xs',
+    // Not matching
+    '!@#$%^&*()',
+    // Too long
+    'namespacetoolong',
+  ])('throws for invalid namespaces: %s', (namespace) => {
+    const reference = '1';
+    expect(() => toCaipChainId(namespace, reference)).toThrow(
+      `Invalid "namespace", must match: ${CAIP_NAMESPACE_REGEX.toString()}`,
+    );
+  });
+
+  it.each([
+    // Too short, must have 1 char at least
+    '',
+    // Not matching
+    '!@#$%^&*()',
+    // Too long
+    '012345678901234567890123456789012', // 33 chars
+  ])('throws for invalid reference: %s', (reference) => {
+    const namespace = CaipNamespaces.Eip155;
+    expect(() => toCaipChainId(namespace, reference)).toThrow(
+      `Invalid "reference", must match: ${CAIP_REFERENCE_REGEX.toString()}`,
+    );
+  });
+});

src/caip.ts

@@ -0,0 +1,51 @@
+import {
+  CAIP_NAMESPACE_REGEX,
+  CAIP_REFERENCE_REGEX,
+  isCaipNamespace,
+  isCaipReference,
+} from '@metamask/utils';
+import type {
+  CaipNamespace,
+  CaipReference,
+  CaipChainId,
+} from '@metamask/utils';
+
+/** Supported CAIP namespaces. */
+export const CaipNamespaces = {
+  /** Namespace for EIP-155 compatible chains. */
+  Eip155: 'eip155' as CaipNamespace,
+} as const;
+
+/**
+ * Chain ID as defined per the CAIP-2
+ * {@link https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md}.
+ *
+ * It defines a way to uniquely identify any blockchain in a human-readable
+ * way.
+ *
+ * @param namespace - The standard (ecosystem) of similar blockchains.
+ * @param reference - Identify of a blockchain within a given namespace.
+ * @throws {@link Error}
+ * This exception is thrown if the inputs does not comply with the CAIP-2
+ * syntax specification
+ * {@link https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md#syntax}.
+ * @returns A CAIP chain ID.
+ */
+export function toCaipChainId(
+  namespace: CaipNamespace,
+  reference: CaipReference,
+): CaipChainId {
+  if (!isCaipNamespace(namespace)) {
+    throw new Error(
+      `Invalid "namespace", must match: ${CAIP_NAMESPACE_REGEX.toString()}`,
+    );
+  }
+
+  if (!isCaipReference(reference)) {
+    throw new Error(
+      `Invalid "reference", must match: ${CAIP_REFERENCE_REGEX.toString()}`,
+    );
+  }
+
+  return `${namespace}:${reference}`;
+}