Skip to content

Commit 27a1e7e

Browse files
mikespositoccharly
mikesposito
and
ccharly
authored
fix!: Keyring.signTypedData accepts types for V1 (#224)
<!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues or other links reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> * Fixes #225 ## Examples <!-- Are there any examples of this change being used in another repository? When considering changes to the MetaMask module template, it's strongly preferred that the change be experimented with in another repository first. This gives reviewers a better sense of how the change works, making it less likely the change will need to be reverted or adjusted later. --> --------- Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
1 parent 9678b47 commit 27a1e7e

File tree

14 files changed

+106
-53
lines changed

14 files changed

+106
-53
lines changed

‎packages/keyring-eth-hd/CHANGELOG.md

+5
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Changed
11+
12+
- **BREAKING:** The method signature for `signTypedData` has been changed ([#224](https://github.com/MetaMask/accounts/pull/224))
13+
- The method now accepts a `TypedDataV1` object when `SignTypedDataVersion.V1` is passed in the options, and `TypedMessage<Types>` when other versions are requested.
14+
1015
## [12.1.0]
1116

1217
### Added

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ module.exports = merge(baseConfig, {
2626
branches: 83.87,
2727
functions: 100,
2828
lines: 95,
29-
statements: 97.91,
29+
statements: 97.27,
3030
},
3131
},
3232
});

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

+18-14
Original file line numberDiff line numberDiff line change
@@ -341,27 +341,31 @@ export class HdKeyring {
341341
* Sign a typed message using the private key of the specified account.
342342
* This method is compatible with the `eth_signTypedData` RPC method.
343343
*
344-
* @param withAccount - The address of the account.
345-
* @param typedData - The typed data to sign.
346-
* @param opts - The options for signing the message.
344+
* @param address - The address of the account.
345+
* @param data - The typed data to sign.
346+
* @param options - The options for signing the message.
347347
* @returns The signature of the message.
348348
*/
349-
async signTypedData<Types extends MessageTypes>(
350-
withAccount: Hex,
351-
typedData: TypedDataV1 | TypedMessage<Types>,
352-
opts: HDKeyringAccountSelectionOptions & {
353-
version: SignTypedDataVersion;
354-
} = { version: SignTypedDataVersion.V1 },
349+
async signTypedData<
350+
Version extends SignTypedDataVersion,
351+
Types extends MessageTypes,
352+
Options extends { version?: Version },
353+
>(
354+
address: Hex,
355+
data: Version extends 'V1' ? TypedDataV1 : TypedMessage<Types>,
356+
options?: HDKeyringAccountSelectionOptions & Options,
355357
): Promise<string> {
358+
let { version } = options ?? { version: SignTypedDataVersion.V1 };
359+
356360
// Treat invalid versions as "V1"
357-
const version = Object.keys(SignTypedDataVersion).includes(opts.version)
358-
? opts.version
359-
: SignTypedDataVersion.V1;
361+
if (!version || !Object.keys(SignTypedDataVersion).includes(version)) {
362+
version = SignTypedDataVersion.V1;
363+
}
360364

361-
const privateKey = this.#getPrivateKeyFor(withAccount, opts);
365+
const privateKey = this.#getPrivateKeyFor(address, options);
362366
return signTypedData({
363367
privateKey: Buffer.from(privateKey),
364-
data: typedData,
368+
data,
365369
version,
366370
});
367371
}

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

+4
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Changed
11+
12+
- **BREAKING:** The `signTypedData` method now requires `SignTypedDataVersion.V4` as version for the `options` argument ([#224](https://github.com/MetaMask/accounts/pull/224))
13+
1014
## [10.0.0]
1115

1216
### Changed

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@ module.exports = merge(baseConfig, {
2323
// An object that configures minimum threshold enforcement for coverage results
2424
coverageThreshold: {
2525
global: {
26-
branches: 90.78,
26+
branches: 90.97,
2727
functions: 96,
28-
lines: 95.02,
29-
statements: 95.07,
28+
lines: 95.03,
29+
statements: 95.09,
3030
},
3131
},
3232
});

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

+19-9
Original file line numberDiff line numberDiff line change
@@ -968,7 +968,6 @@ describe('LedgerKeyring', function () {
968968
],
969969
},
970970
};
971-
const options = { version: 'V4' };
972971

973972
beforeEach(async function () {
974973
jest
@@ -989,7 +988,7 @@ describe('LedgerKeyring', function () {
989988
const result = await keyring.signTypedData(
990989
fakeAccounts[15],
991990
fixtureData,
992-
options,
991+
{ version: sigUtil.SignTypedDataVersion.V4 },
993992
);
994993
expect(result).toBe(
995994
'0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b',
@@ -1016,7 +1015,7 @@ describe('LedgerKeyring', function () {
10161015
const result = await keyring.signTypedData(
10171016
fakeAccounts[15],
10181017
fixtureDataWithoutSalt,
1019-
options,
1018+
{ version: sigUtil.SignTypedDataVersion.V4 },
10201019
);
10211020
expect(result).toBe(
10221021
'0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b',
@@ -1034,7 +1033,9 @@ describe('LedgerKeyring', function () {
10341033
}));
10351034

10361035
await expect(
1037-
keyring.signTypedData(fakeAccounts[15], fixtureData, options),
1036+
keyring.signTypedData(fakeAccounts[15], fixtureData, {
1037+
version: sigUtil.SignTypedDataVersion.V4,
1038+
}),
10381039
).rejects.toThrow(
10391040
'Ledger: The signature doesnt match the right address',
10401041
);
@@ -1043,7 +1044,8 @@ describe('LedgerKeyring', function () {
10431044
it('throws an error if the signTypedData version is not v4', async function () {
10441045
await expect(
10451046
keyring.signTypedData(fakeAccounts[0], fixtureData, {
1046-
version: 'V3',
1047+
// @ts-expect-error we want to test an invalid version
1048+
version: sigUtil.SignTypedDataVersion.V3,
10471049
}),
10481050
).rejects.toThrow(
10491051
'Ledger: Only version 4 of typed data signing is supported',
@@ -1063,7 +1065,9 @@ describe('LedgerKeyring', function () {
10631065
.spyOn(keyring, 'unlockAccountByAddress')
10641066
.mockResolvedValue(undefined);
10651067
await expect(
1066-
keyring.signTypedData(fakeAccounts[0], fixtureData, options),
1068+
keyring.signTypedData(fakeAccounts[0], fixtureData, {
1069+
version: sigUtil.SignTypedDataVersion.V4,
1070+
}),
10671071
).rejects.toThrow('Ledger: Unknown error while signing message');
10681072
});
10691073

@@ -1073,7 +1077,9 @@ describe('LedgerKeyring', function () {
10731077
.mockRejectedValue(new Error('some error'));
10741078

10751079
await expect(
1076-
keyring.signTypedData(fakeAccounts[15], fixtureData, options),
1080+
keyring.signTypedData(fakeAccounts[15], fixtureData, {
1081+
version: sigUtil.SignTypedDataVersion.V4,
1082+
}),
10771083
).rejects.toThrow('some error');
10781084
});
10791085

@@ -1083,7 +1089,9 @@ describe('LedgerKeyring', function () {
10831089
.mockRejectedValue('some error');
10841090

10851091
await expect(
1086-
keyring.signTypedData(fakeAccounts[15], fixtureData, options),
1092+
keyring.signTypedData(fakeAccounts[15], fixtureData, {
1093+
version: sigUtil.SignTypedDataVersion.V4,
1094+
}),
10871095
).rejects.toThrow('Ledger: Unknown error while signing message');
10881096
});
10891097

@@ -1096,7 +1104,9 @@ describe('LedgerKeyring', function () {
10961104
const result = await keyring.signTypedData(
10971105
fakeAccounts[15],
10981106
fixtureData,
1099-
options,
1107+
{
1108+
version: sigUtil.SignTypedDataVersion.V4,
1109+
},
11001110
);
11011111
expect(result).toBe(
11021112
'0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e3200',

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

+9-4
Original file line numberDiff line numberDiff line change
@@ -493,12 +493,17 @@ export class LedgerKeyring implements Keyring {
493493
return hdPath;
494494
}
495495

496-
async signTypedData<T extends MessageTypes>(
496+
async signTypedData<
497+
Version extends SignTypedDataVersion.V4,
498+
Types extends MessageTypes,
499+
Options extends { version?: Version },
500+
>(
497501
withAccount: Hex,
498-
data: TypedMessage<T>,
499-
options: { version?: string } = {},
502+
data: TypedMessage<Types>,
503+
options?: Options,
500504
): Promise<string> {
501-
const isV4 = options.version === 'V4';
505+
const { version } = options ?? {};
506+
const isV4 = version === 'V4';
502507
if (!isV4) {
503508
throw new Error(
504509
'Ledger: Only version 4 of typed data signing is supported',

‎packages/keyring-eth-simple/CHANGELOG.md

+6
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Changed
11+
12+
- **BREAKING:** The method signature for `signTypedData` has been changed ([#224](https://github.com/MetaMask/accounts/pull/224))
13+
- The method now accepts a `TypedDataV1` object when `SignTypedDataVersion.V1` is passed in the options, and `TypedMessage<Types>` when other versions are requested.
14+
- The `options` argument type has been changed to `{ version: SignTypedDataVersion } | undefined`.
15+
1016
## [10.0.0]
1117

1218
### Changed

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

+6-5
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,7 @@ describe('simple-keyring', function () {
359359
it('returns the expected value if invalid version is given', async function () {
360360
await keyring.deserialize([privKeyHex]);
361361
const signature = await keyring.signTypedData(address, typedData, {
362+
// @ts-expect-error: intentionally passing invalid version
362363
version: 'FOO',
363364
});
364365
expect(signature).toBe(expectedSignature);
@@ -389,7 +390,7 @@ describe('simple-keyring', function () {
389390
it('works via `V1` string', async function () {
390391
await keyring.deserialize([privKeyHex]);
391392
const signature = await keyring.signTypedData(address, typedData, {
392-
version: 'V1',
393+
version: SignTypedDataVersion.V1,
393394
});
394395
expect(signature).toBe(expectedSignature);
395396
const restored = recoverTypedSignature({
@@ -445,7 +446,7 @@ describe('simple-keyring', function () {
445446

446447
await keyring.deserialize([privKeyHex]);
447448
const signature = await keyring.signTypedData(address, typedData, {
448-
version: 'V3',
449+
version: SignTypedDataVersion.V3,
449450
});
450451
const restored = recoverTypedSignature({
451452
data: typedData,
@@ -505,7 +506,7 @@ describe('simple-keyring', function () {
505506
const address = (await keyring.getAccounts())[0];
506507
assert(address, 'address is undefined');
507508
const signature = await keyring.signTypedData(address, typedData, {
508-
version: 'V3',
509+
version: SignTypedDataVersion.V3,
509510
});
510511
expect(signature).toBe(expectedSignature);
511512
const restored = recoverTypedSignature({
@@ -534,7 +535,7 @@ describe('simple-keyring', function () {
534535

535536
await keyring.deserialize([privKeyHex]);
536537
const signature = await keyring.signTypedData(address, typedData, {
537-
version: 'V4',
538+
version: SignTypedDataVersion.V4,
538539
});
539540
const restored = recoverTypedSignature({
540541
data: typedData,
@@ -725,7 +726,7 @@ describe('simple-keyring', function () {
725726
const address = (await keyring.getAccounts())[0];
726727
assert(address, 'address is undefined');
727728
const signature = await keyring.signTypedData(address, typedData, {
728-
version: 'V4',
729+
version: SignTypedDataVersion.V4,
729730
});
730731
expect(signature).toBe(expectedSignature);
731732
const restored = recoverTypedSignature({

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

+16-10
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ import {
77
stripHexPrefix,
88
} from '@ethereumjs/util';
99
import {
10+
type TypedDataV1,
11+
type MessageTypes,
12+
type TypedMessage,
1013
concatSig,
1114
decrypt,
1215
EIP7702Authorization,
@@ -154,21 +157,24 @@ export default class SimpleKeyring implements Keyring {
154157
return decrypt({ privateKey, encryptedData });
155158
}
156159

157-
// personal_signTypedData, signs data along with the schema
158-
async signTypedData(
160+
async signTypedData<
161+
Version extends SignTypedDataVersion,
162+
Types extends MessageTypes,
163+
Options extends { version?: Version } & KeyringOpt,
164+
>(
159165
address: Hex,
160-
typedData: any,
161-
opts: KeyringOpt = { version: SignTypedDataVersion.V1 },
166+
data: Version extends 'V1' ? TypedDataV1 : TypedMessage<Types>,
167+
options?: Options,
162168
): Promise<string> {
163-
// Treat invalid versions as "V1"
164-
let version = SignTypedDataVersion.V1;
169+
let { version } = options ?? { version: SignTypedDataVersion.V1 };
165170

166-
if (opts.version && isSignTypedDataVersion(opts.version)) {
167-
version = SignTypedDataVersion[opts.version];
171+
// Treat invalid versions as "V1"
172+
if (!version || !isSignTypedDataVersion(version)) {
173+
version = SignTypedDataVersion.V1;
168174
}
169175

170-
const privateKey = this.#getPrivateKeyFor(address, opts);
171-
return signTypedData({ privateKey, data: typedData, version });
176+
const privateKey = this.#getPrivateKeyFor(address, options);
177+
return signTypedData({ privateKey, data, version });
172178
}
173179

174180
// get public key for nacl

‎packages/keyring-eth-trezor/CHANGELOG.md

+6
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Changed
11+
12+
- **BREAKING:** The method signature for `signTypedData` has been changed ([#224](https://github.com/MetaMask/accounts/pull/224))
13+
- The `options` argument type has been changed to `{ version: SignTypedDataVersion.V3 | SignTypedDataVersion.V4 } | undefined`.
14+
- The `options.version` argument type has been restricted to accept `SignTypedDataVersion.V3 | SignTypedDataVersion.V4` ([#224](https://github.com/MetaMask/accounts/pull/224))
15+
1016
## [8.0.0]
1117

1218
### Changed

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@ module.exports = merge(baseConfig, {
2323
// An object that configures minimum threshold enforcement for coverage results
2424
coverageThreshold: {
2525
global: {
26-
branches: 51.63,
26+
branches: 52.38,
2727
functions: 91.22,
28-
lines: 90.1,
29-
statements: 90.3,
28+
lines: 90.15,
29+
statements: 90.35,
3030
},
3131
},
3232
});

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

+9-3
Original file line numberDiff line numberDiff line change
@@ -459,11 +459,17 @@ export class TrezorKeyring implements Keyring {
459459
}
460460

461461
// EIP-712 Sign Typed Data
462-
async signTypedData<T extends MessageTypes>(
462+
async signTypedData<
463+
Version extends SignTypedDataVersion.V3 | SignTypedDataVersion.V4,
464+
Types extends MessageTypes,
465+
Options extends { version?: Version },
466+
>(
463467
address: Hex,
464-
data: TypedMessage<T>,
465-
{ version }: { version: SignTypedDataVersion },
468+
data: TypedMessage<Types>,
469+
options?: Options,
466470
): Promise<string> {
471+
const { version } = options ?? { version: SignTypedDataVersion.V4 };
472+
467473
const dataWithHashes = transformTypedData(
468474
data,
469475
version === SignTypedDataVersion.V4,

‎packages/keyring-utils/src/keyring.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ export type Keyring = {
229229
*/
230230
signTypedData?(
231231
address: Hex,
232-
typedData: Record<string, unknown>,
232+
typedData: unknown[] | Record<string, unknown>,
233233
options?: Record<string, unknown>,
234234
): Promise<string>;
235235

0 commit comments

Comments
 (0)