fix!: Keyring.signTypedData accepts types for V1

packages/keyring-eth-hd/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **BREAKING:** The method signature for `signTypedData` has been changed ([#224](https://github.com/MetaMask/accounts/pull/224))
+  - The method now accepts a `TypedDataV1` object when `SignTypedDataVersion.V1` is passed in the options, and `TypedMessage<Types>` when other versions are requested.
+
 ## [12.1.0]
 
 ### Added

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

@@ -26,7 +26,7 @@ module.exports = merge(baseConfig, {
       branches: 83.87,
       functions: 100,
       lines: 95,
-      statements: 97.91,
+      statements: 97.27,
     },
   },
 });

packages/keyring-eth-hd/src/hd-keyring.ts

@@ -341,27 +341,31 @@ export class HdKeyring {
    * Sign a typed message using the private key of the specified account.
    * This method is compatible with the `eth_signTypedData` RPC method.
    *
-   * @param withAccount - The address of the account.
-   * @param typedData - The typed data to sign.
-   * @param opts - The options for signing the message.
+   * @param address - The address of the account.
+   * @param data - The typed data to sign.
+   * @param options - The options for signing the message.
    * @returns The signature of the message.
    */
-  async signTypedData<Types extends MessageTypes>(
-    withAccount: Hex,
-    typedData: TypedDataV1 | TypedMessage<Types>,
-    opts: HDKeyringAccountSelectionOptions & {
-      version: SignTypedDataVersion;
-    } = { version: SignTypedDataVersion.V1 },
+  async signTypedData<
+    Version extends SignTypedDataVersion,
+    Types extends MessageTypes,
+    Options extends { version?: Version },
+  >(
+    address: Hex,
+    data: Version extends 'V1' ? TypedDataV1 : TypedMessage<Types>,
+    options?: HDKeyringAccountSelectionOptions & Options,
   ): Promise<string> {
+    let { version } = options ?? { version: SignTypedDataVersion.V1 };
+
     // Treat invalid versions as "V1"
-    const version = Object.keys(SignTypedDataVersion).includes(opts.version)
-      ? opts.version
-      : SignTypedDataVersion.V1;
+    if (!version || !Object.keys(SignTypedDataVersion).includes(version)) {
+      version = SignTypedDataVersion.V1;
+    }
 
-    const privateKey = this.#getPrivateKeyFor(withAccount, opts);
+    const privateKey = this.#getPrivateKeyFor(address, options);
     return signTypedData({
       privateKey: Buffer.from(privateKey),
-      data: typedData,
+      data,
       version,
     });
   }

packages/keyring-eth-ledger-bridge/CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **BREAKING:** The `signTypedData` method now requires `SignTypedDataVersion.V4` as version for the `options` argument ([#224](https://github.com/MetaMask/accounts/pull/224))
+
 ## [10.0.0]
 
 ### Changed

packages/keyring-eth-ledger-bridge/jest.config.js

@@ -23,10 +23,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 90.78,
+      branches: 90.97,
       functions: 96,
-      lines: 95.02,
-      statements: 95.07,
+      lines: 95.03,
+      statements: 95.09,
     },
   },
 });

packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts

@@ -968,7 +968,6 @@ describe('LedgerKeyring', function () {
           ],
         },
       };
-      const options = { version: 'V4' };
 
       beforeEach(async function () {
         jest
@@ -989,7 +988,7 @@ describe('LedgerKeyring', function () {
         const result = await keyring.signTypedData(
           fakeAccounts[15],
           fixtureData,
-          options,
+          { version: sigUtil.SignTypedDataVersion.V4 },
         );
         expect(result).toBe(
           '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b',
@@ -1016,7 +1015,7 @@ describe('LedgerKeyring', function () {
         const result = await keyring.signTypedData(
           fakeAccounts[15],
           fixtureDataWithoutSalt,
-          options,
+          { version: sigUtil.SignTypedDataVersion.V4 },
         );
         expect(result).toBe(
           '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b',
@@ -1034,7 +1033,9 @@ describe('LedgerKeyring', function () {
           }));
 
         await expect(
-          keyring.signTypedData(fakeAccounts[15], fixtureData, options),
+          keyring.signTypedData(fakeAccounts[15], fixtureData, {
+            version: sigUtil.SignTypedDataVersion.V4,
+          }),
         ).rejects.toThrow(
           'Ledger: The signature doesnt match the right address',
         );
@@ -1043,7 +1044,8 @@ describe('LedgerKeyring', function () {
       it('throws an error if the signTypedData version is not v4', async function () {
         await expect(
           keyring.signTypedData(fakeAccounts[0], fixtureData, {
-            version: 'V3',
+            // @ts-expect-error we want to test an invalid version
+            version: sigUtil.SignTypedDataVersion.V3,
           }),
         ).rejects.toThrow(
           'Ledger: Only version 4 of typed data signing is supported',
@@ -1063,7 +1065,9 @@ describe('LedgerKeyring', function () {
           .spyOn(keyring, 'unlockAccountByAddress')
           .mockResolvedValue(undefined);
         await expect(
-          keyring.signTypedData(fakeAccounts[0], fixtureData, options),
+          keyring.signTypedData(fakeAccounts[0], fixtureData, {
+            version: sigUtil.SignTypedDataVersion.V4,
+          }),
         ).rejects.toThrow('Ledger: Unknown error while signing message');
       });
 
@@ -1073,7 +1077,9 @@ describe('LedgerKeyring', function () {
           .mockRejectedValue(new Error('some error'));
 
         await expect(
-          keyring.signTypedData(fakeAccounts[15], fixtureData, options),
+          keyring.signTypedData(fakeAccounts[15], fixtureData, {
+            version: sigUtil.SignTypedDataVersion.V4,
+          }),
         ).rejects.toThrow('some error');
       });
 
@@ -1083,7 +1089,9 @@ describe('LedgerKeyring', function () {
           .mockRejectedValue('some error');
 
         await expect(
-          keyring.signTypedData(fakeAccounts[15], fixtureData, options),
+          keyring.signTypedData(fakeAccounts[15], fixtureData, {
+            version: sigUtil.SignTypedDataVersion.V4,
+          }),
         ).rejects.toThrow('Ledger: Unknown error while signing message');
       });
 
@@ -1096,7 +1104,9 @@ describe('LedgerKeyring', function () {
         const result = await keyring.signTypedData(
           fakeAccounts[15],
           fixtureData,
-          options,
+          {
+            version: sigUtil.SignTypedDataVersion.V4,
+          },
         );
         expect(result).toBe(
           '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e3200',

packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts

@@ -493,12 +493,17 @@ export class LedgerKeyring implements Keyring {
     return hdPath;
   }
 
-  async signTypedData<T extends MessageTypes>(
+  async signTypedData<
+    Version extends SignTypedDataVersion.V4,
+    Types extends MessageTypes,
+    Options extends { version?: Version },
+  >(
     withAccount: Hex,
-    data: TypedMessage<T>,
-    options: { version?: string } = {},
+    data: TypedMessage<Types>,
+    options?: Options,
   ): Promise<string> {
-    const isV4 = options.version === 'V4';
+    const { version } = options ?? {};
+    const isV4 = version === 'V4';
     if (!isV4) {
       throw new Error(
         'Ledger: Only version 4 of typed data signing is supported',

packages/keyring-eth-simple/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **BREAKING:** The method signature for `signTypedData` has been changed ([#224](https://github.com/MetaMask/accounts/pull/224))
+  - The method now accepts a `TypedDataV1` object when `SignTypedDataVersion.V1` is passed in the options, and `TypedMessage<Types>` when other versions are requested.
+  - The `options` argument type has been changed to `{ version: SignTypedDataVersion } | undefined`.
+
 ## [10.0.0]
 
 ### Changed

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

@@ -359,6 +359,7 @@ describe('simple-keyring', function () {
     it('returns the expected value if invalid version is given', async function () {
       await keyring.deserialize([privKeyHex]);
       const signature = await keyring.signTypedData(address, typedData, {
+        // @ts-expect-error: intentionally passing invalid version
         version: 'FOO',
       });
       expect(signature).toBe(expectedSignature);
@@ -389,7 +390,7 @@ describe('simple-keyring', function () {
     it('works via `V1` string', async function () {
       await keyring.deserialize([privKeyHex]);
       const signature = await keyring.signTypedData(address, typedData, {
-        version: 'V1',
+        version: SignTypedDataVersion.V1,
       });
       expect(signature).toBe(expectedSignature);
       const restored = recoverTypedSignature({
@@ -445,7 +446,7 @@ describe('simple-keyring', function () {
 
       await keyring.deserialize([privKeyHex]);
       const signature = await keyring.signTypedData(address, typedData, {
-        version: 'V3',
+        version: SignTypedDataVersion.V3,
       });
       const restored = recoverTypedSignature({
         data: typedData,
@@ -505,7 +506,7 @@ describe('simple-keyring', function () {
       const address = (await keyring.getAccounts())[0];
       assert(address, 'address is undefined');
       const signature = await keyring.signTypedData(address, typedData, {
-        version: 'V3',
+        version: SignTypedDataVersion.V3,
       });
       expect(signature).toBe(expectedSignature);
       const restored = recoverTypedSignature({
@@ -534,7 +535,7 @@ describe('simple-keyring', function () {
 
       await keyring.deserialize([privKeyHex]);
       const signature = await keyring.signTypedData(address, typedData, {
-        version: 'V4',
+        version: SignTypedDataVersion.V4,
       });
       const restored = recoverTypedSignature({
         data: typedData,
@@ -725,7 +726,7 @@ describe('simple-keyring', function () {
       const address = (await keyring.getAccounts())[0];
       assert(address, 'address is undefined');
       const signature = await keyring.signTypedData(address, typedData, {
-        version: 'V4',
+        version: SignTypedDataVersion.V4,
       });
       expect(signature).toBe(expectedSignature);
       const restored = recoverTypedSignature({

packages/keyring-eth-simple/src/simple-keyring.ts

@@ -7,6 +7,9 @@ import {
   stripHexPrefix,
 } from '@ethereumjs/util';
 import {
+  type TypedDataV1,
+  type MessageTypes,
+  type TypedMessage,
   concatSig,
   decrypt,
   EIP7702Authorization,
@@ -154,21 +157,24 @@ export default class SimpleKeyring implements Keyring {
     return decrypt({ privateKey, encryptedData });
   }
 
-  // personal_signTypedData, signs data along with the schema
-  async signTypedData(
+  async signTypedData<
+    Version extends SignTypedDataVersion,
+    Types extends MessageTypes,
+    Options extends { version?: Version } & KeyringOpt,
+  >(
     address: Hex,
-    typedData: any,
-    opts: KeyringOpt = { version: SignTypedDataVersion.V1 },
+    data: Version extends 'V1' ? TypedDataV1 : TypedMessage<Types>,
+    options?: Options,
   ): Promise<string> {
-    // Treat invalid versions as "V1"
-    let version = SignTypedDataVersion.V1;
+    let { version } = options ?? { version: SignTypedDataVersion.V1 };
 
-    if (opts.version && isSignTypedDataVersion(opts.version)) {
-      version = SignTypedDataVersion[opts.version];
+    // Treat invalid versions as "V1"
+    if (!version || !isSignTypedDataVersion(version)) {
+      version = SignTypedDataVersion.V1;
     }
 
-    const privateKey = this.#getPrivateKeyFor(address, opts);
-    return signTypedData({ privateKey, data: typedData, version });
+    const privateKey = this.#getPrivateKeyFor(address, options);
+    return signTypedData({ privateKey, data, version });
   }
 
   // get public key for nacl

packages/keyring-eth-trezor/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **BREAKING:** The method signature for `signTypedData` has been changed ([#224](https://github.com/MetaMask/accounts/pull/224))
+  - The `options` argument type has been changed to `{ version: SignTypedDataVersion.V3 | SignTypedDataVersion.V4 } | undefined`.
+  - The `options.version` argument type has been restricted to accept `SignTypedDataVersion.V3 | SignTypedDataVersion.V4` ([#224](https://github.com/MetaMask/accounts/pull/224))
+
 ## [8.0.0]
 
 ### Changed

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

@@ -23,10 +23,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 51.63,
+      branches: 52.38,
       functions: 91.22,
-      lines: 90.1,
-      statements: 90.3,
+      lines: 90.15,
+      statements: 90.35,
     },
   },
 });

packages/keyring-eth-trezor/src/trezor-keyring.ts

@@ -459,11 +459,17 @@ export class TrezorKeyring implements Keyring {
   }
 
   // EIP-712 Sign Typed Data
-  async signTypedData<T extends MessageTypes>(
+  async signTypedData<
+    Version extends SignTypedDataVersion.V3 | SignTypedDataVersion.V4,
+    Types extends MessageTypes,
+    Options extends { version?: Version },
+  >(
     address: Hex,
-    data: TypedMessage<T>,
-    { version }: { version: SignTypedDataVersion },
+    data: TypedMessage<Types>,
+    options?: Options,
   ): Promise<string> {
+    const { version } = options ?? { version: SignTypedDataVersion.V4 };
+
     const dataWithHashes = transformTypedData(
       data,
       version === SignTypedDataVersion.V4,

packages/keyring-utils/src/keyring.ts

@@ -229,7 +229,7 @@ export type Keyring = {
    */
   signTypedData?(
     address: Hex,
-    typedData: Record<string, unknown>,
+    typedData: unknown[] | Record<string, unknown>,
     options?: Record<string, unknown>,
   ): Promise<string>;