Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: Replace eth-sig-util@^2 with @metamask/eth-sig-util@^6 #157

Merged
merged 14 commits into from
Oct 19, 2023
Merged

deps: Replace eth-sig-util@^2 with @metamask/eth-sig-util@^6 #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6ec437d
update eth-sig-util version to latest
adonesky1 Dec 20, 2022
4c48fc3
bump minimum node version
adonesky1 Dec 20, 2022
cc1973c
cleanup
adonesky1 Dec 20, 2022
860f7bd
lint
adonesky1 Dec 20, 2022
6398b18
Merge branch main into update-eth-sig-util-version
legobeat Oct 3, 2023
6923199
deps: @metamask/eth-sig-util@^5.0.2->^6.0.1
legobeat Oct 3, 2023
44fc2d5
devDeps: add missing @ledgerhq/types implicit peerDeps
legobeat Oct 3, 2023
4c032c5
lint:fix
legobeat Oct 3, 2023
b89f0be
test: lint:fix
legobeat Oct 3, 2023
f5c2c98
fixups after eth-sig-util update
legobeat Oct 3, 2023
36d2ab4
test: fix jest.spyOn of @metamask/eth-sig-util by mocking es module
legobeat Oct 3, 2023
88625cd
Merge branch main into update-eth-sig-util-version
legobeat Oct 10, 2023
d10891f
Merge branch 'main' into update-eth-sig-util-version
legobeat Oct 19, 2023
f9457eb
yarn dedupe
legobeat Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .depcheckrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"ignores": [
"@lavamoat/allow-scripts",
"@ledgerhq/types-cryptoassets",
"@ledgerhq/types-devices",
"@metamask/auto-changelog",
"@types/*",
"prettier-plugin-packagejson",
Expand Down
9 changes: 5 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,22 @@
"@ethereumjs/rlp": "^4.0.0",
"@ethereumjs/tx": "^4.1.1",
"@ethereumjs/util": "^8.0.0",
"@metamask/eth-sig-util": "^6.0.1",
"@metamask/utils": "^5.0.0",
"eth-sig-util": "^2.0.0",
"hdkey": "0.8.0"
},
"devDependencies": {
"@ethereumjs/common": "^3.1.1",
"@lavamoat/allow-scripts": "^2.3.0",
"@ledgerhq/hw-app-eth": "^6.32.0",
"@ledgerhq/types-cryptoassets": "^7.6.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these extra dependencies necessary? I don't see them being used in this PR.

Copy link
Contributor

@legobeat legobeat Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're used by sibling modules: https://github.com/MetaMask/eth-ledger-bridge-keyring/actions/runs/6577714417/job/17869959528?pr=201

Error: node_modules/@ledgerhq/domain-service/lib/types.d.ts(1,26): error TS2307: Cannot find module '@ledgerhq/types-cryptoassets' or its corresponding type declarations.
Error: node_modules/@ledgerhq/types-live/lib/account.d.ts(2,58): error TS2307: Cannot find module '@ledgerhq/types-cryptoassets' or its corresponding type declarations.
Error: node_modules/@ledgerhq/types-live/lib/bridge.d.ts(3,37): error TS2307: Cannot find module '@ledgerhq/types-cryptoassets' or its corresponding type declarations.
Error: node_modules/@ledgerhq/types-live/lib/manager.d.ts(3,37): error TS2307: Cannot find module '@ledgerhq/types-cryptoassets' or its corresponding type declarations.
Error: node_modules/@ledgerhq/types-live/lib/portfolio.d.ts(2,52): error TS2307: Cannot find module '@ledgerhq/types-cryptoassets' or its corresponding type declarations.
Error: node_modules/@ledgerhq/types-live/lib/transaction.d.ts(2,27): error TS2307: Cannot find module '@ledgerhq/types-cryptoassets' or its corresponding type declarations.
Error: Process completed with exit code 2.

"@ledgerhq/types-devices": "^6.22.4",
"@metamask/auto-changelog": "^3.1.0",
"@metamask/eslint-config": "^11.0.1",
"@metamask/eslint-config-browser": "^11.0.0",
"@metamask/eslint-config-jest": "^11.0.0",
"@metamask/eslint-config-nodejs": "^11.0.0",
"@metamask/eslint-config-typescript": "^11.0.0",
"@types/eth-sig-util": "^2.1.1",
"@types/ethereumjs-tx": "^1.0.1",
"@types/hdkey": "^2.0.1",
"@types/jest": "^28.1.6",
Expand Down Expand Up @@ -91,8 +92,8 @@
"lavamoat": {
"allowScripts": {
"@lavamoat/preinstall-always-fail": false,
"eth-sig-util>ethereumjs-util>ethereum-cryptography>keccak": false,
"eth-sig-util>ethereumjs-util>ethereum-cryptography>secp256k1": false,
"@ledgerhq/hw-app-eth>@ledgerhq/domain-service>eip55>keccak": false,
"ethereumjs-tx>ethereumjs-util>keccak": false,
"hdkey>secp256k1": false
}
}
Expand Down
12 changes: 10 additions & 2 deletions src/ledger-keyring.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,22 @@ import { Common, Chain, Hardfork } from '@ethereumjs/common';
import { RLP } from '@ethereumjs/rlp';
import { TransactionFactory } from '@ethereumjs/tx';
import * as ethUtil from '@ethereumjs/util';
import sigUtil from 'eth-sig-util';
import * as sigUtil from '@metamask/eth-sig-util';
import EthereumTx from 'ethereumjs-tx';
import HDKey from 'hdkey';

import { LedgerBridge } from './ledger-bridge';
import { LedgerIframeBridge } from './ledger-iframe-bridge';
import { AccountDetails, LedgerKeyring } from './ledger-keyring';

jest.mock('@metamask/eth-sig-util', () => {
return {
// eslint-disable-next-line @typescript-eslint/naming-convention
__esModule: true,
...jest.requireActual('@metamask/eth-sig-util'),
};
});

Comment on lines +13 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legobeat is this necessary. Don't understand what, if anything, this is doing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in order to mock the new version (see error this is addressing here:

FAIL src/ledger-keyring.test.ts (14.651 s)
  ● LedgerKeyring › signPersonalMessage › should call create a listener waiting for the iframe response

    Cannot spyOn on a primitive value; undefined given

      616 |
      617 |       jest
    > 618 |         .spyOn(sigUtil, 'recoverPersonalSignature')
          |          ^
      619 |         .mockReturnValue(fakeAccounts[0]);
      620 |
      621 |       await keyring.signPersonalMessage(fakeAccounts[0], 'some message');

      at ModuleMocker.spyOn (node_modules/jest-mock/build/index.js:774:13)
      at Object.<anonymous> (src/ledger-keyring.test.ts:618:10)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a common approach when bundling all exports from an import into one variable and then spying on an individual export. Basically we're making a copy of sigUtil so that all properties are writable (jest.spyOn reassigns the property you spy on).

const fakeAccounts = [
'0xF30952A1c534CDE7bC471380065726fa8686dfB3',
'0x44fe3Cf56CaF651C4bD34Ae6dbcffa34e9e3b84B',
Expand Down Expand Up @@ -700,7 +708,7 @@ describe('LedgerKeyring', function () {
},
],
},
primaryType: 'Mail',
primaryType: 'Mail' as const,
types: {
EIP712Domain: [
{ name: 'name', type: 'string' },
Expand Down
42 changes: 20 additions & 22 deletions src/ledger-keyring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { RLP } from '@ethereumjs/rlp';
import { TransactionFactory, TxData, TypedTransaction } from '@ethereumjs/tx';
import * as ethUtil from '@ethereumjs/util';
import type { MessageTypes, TypedMessage } from '@metamask/eth-sig-util';
import {
recoverPersonalSignature,
recoverTypedSignature,
SignTypedDataVersion,
TypedDataUtils,
} from '@metamask/eth-sig-util';
// eslint-disable-next-line import/no-nodejs-modules
import { Buffer } from 'buffer';
import * as sigUtil from 'eth-sig-util';
import type OldEthJsTransaction from 'ethereumjs-tx';
// eslint-disable-next-line import/no-nodejs-modules
import { EventEmitter } from 'events';
Expand Down Expand Up @@ -418,10 +424,9 @@ export class LedgerKeyring extends EventEmitter {
recoveryId = `0${recoveryId}`;
}
const signature = `0x${payload.r}${payload.s}${recoveryId}`;
const addressSignedWith = sigUtil.recoverPersonalSignature({
const addressSignedWith = recoverPersonalSignature({
data: message,
// eslint-disable-next-line id-denylist
sig: signature,
signature,
});
if (
ethUtil.toChecksumAddress(addressSignedWith) !==
Expand Down Expand Up @@ -453,9 +458,9 @@ export class LedgerKeyring extends EventEmitter {
return hdPath;
}

async signTypedData(
async signTypedData<T extends MessageTypes>(
withAccount: string,
data: sigUtil.EIP712TypedData,
data: TypedMessage<T>,
options: { version?: string } = {},
) {
const isV4 = options.version === 'V4';
Expand All @@ -466,22 +471,18 @@ export class LedgerKeyring extends EventEmitter {
}

const { domain, types, primaryType, message } =
sigUtil.TypedDataUtils.sanitizeData(data);
const domainSeparatorHex = sigUtil.TypedDataUtils.hashStruct(
TypedDataUtils.sanitizeData(data);
const domainSeparatorHex = TypedDataUtils.hashStruct(
'EIP712Domain',
domain,
types,
// @ts-expect-error @types/eth-sig-util documents this function
// as taking three arguments, but it actually takes four.
// See: https://github.com/MetaMask/eth-sig-util/blob/v2.5.4/index.js#L174
isV4,
SignTypedDataVersion.V4,
).toString('hex');
const hashStructMessageHex = sigUtil.TypedDataUtils.hashStruct(
primaryType,
const hashStructMessageHex = TypedDataUtils.hashStruct(
primaryType.toString(),
message,
types,
// @ts-expect-error see comment above
isV4,
SignTypedDataVersion.V4,
).toString('hex');

const hdPath = await this.unlockAccountByAddress(withAccount);
Expand All @@ -508,13 +509,10 @@ export class LedgerKeyring extends EventEmitter {
recoveryId = `0${recoveryId}`;
}
const signature = `0x${payload.r}${payload.s}${recoveryId}`;
// @ts-expect-error recoverTypedSignature_v4 is missing from
// @types/eth-sig-util.
// See: https://github.com/MetaMask/eth-sig-util/blob/v2.5.4/index.js#L464
const addressSignedWith = sigUtil.recoverTypedSignature_v4({
const addressSignedWith = recoverTypedSignature({
data,
// eslint-disable-next-line id-denylist
sig: signature,
signature,
version: SignTypedDataVersion.V4,
});
if (
ethUtil.toChecksumAddress(addressSignedWith) !==
Expand Down
Loading