From 515dec164559579d683c31cc747bd7762359ade7 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Wed, 20 Nov 2024 15:32:23 +0800 Subject: [PATCH 01/15] feat: add clear signing feature to ledger. --- .../keyring-eth-ledger-bridge/package.json | 1 + .../src/ledger-bridge.ts | 4 +- .../src/ledger-iframe-bridge.test.ts | 28 ++++++-- .../src/ledger-keyring.ts | 28 ++++---- .../src/ledger-mobile-bridge.test.ts | 66 +++++++++++-------- .../src/ledger-mobile-bridge.ts | 22 +++---- yarn.lock | 11 ++++ 7 files changed, 99 insertions(+), 61 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/package.json b/packages/keyring-eth-ledger-bridge/package.json index 81245bc2..7efdd269 100644 --- a/packages/keyring-eth-ledger-bridge/package.json +++ b/packages/keyring-eth-ledger-bridge/package.json @@ -61,6 +61,7 @@ "@ledgerhq/hw-transport": "^6.31.3", "@ledgerhq/types-cryptoassets": "^7.15.1", "@ledgerhq/types-devices": "^6.25.3", + "@ledgerhq/types-live": "^6.53.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/utils": "^9.3.0", "@ts-bridge/cli": "^0.6.0", diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts index 1a66bc46..01388584 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts @@ -1,5 +1,6 @@ import type LedgerHwAppEth from '@ledgerhq/hw-app-eth'; import type Transport from '@ledgerhq/hw-transport'; +import { EIP712Message } from '@ledgerhq/types-live'; export type GetPublicKeyParams = { hdPath: string }; export type GetPublicKeyResponse = Awaited< @@ -18,8 +19,7 @@ export type LedgerSignMessageResponse = Awaited< export type LedgerSignTypedDataParams = { hdPath: string; - domainSeparatorHex: string; - hashStructMessageHex: string; + message: EIP712Message; }; export type LedgerSignTypedDataResponse = Awaited< ReturnType diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts index d5053ec0..f74150f0 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts @@ -443,8 +443,18 @@ describe('LedgerIframeBridge', function () { }; const params = { hdPath: "m/44'/60'/0'/0", - domainSeparatorHex: '', - hashStructMessageHex: '', + message: { + domain: { + chainId: 1, + verifyingContract: '0xdsf', + }, + primaryType: 'string', + types: { + EIP712Domain: [], + string: [], + }, + message: { test: 'test' }, + }, }; stubKeyringIFramePostMessage(bridge, (message) => { @@ -475,8 +485,18 @@ describe('LedgerIframeBridge', function () { const errorMessage = 'Ledger Error'; const params = { hdPath: "m/44'/60'/0'/0", - domainSeparatorHex: '', - hashStructMessageHex: '', + message: { + domain: { + chainId: 1, + verifyingContract: '0xdsf', + }, + primaryType: 'string', + types: { + EIP712Domain: [], + string: [], + }, + message: { test: 'test' }, + }, }; stubKeyringIFramePostMessage(bridge, (message) => { diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index eeeeb494..e5caaa8e 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -494,18 +494,6 @@ export class LedgerKeyring extends EventEmitter { const { domain, types, primaryType, message } = TypedDataUtils.sanitizeData(data); - const domainSeparatorHex = TypedDataUtils.hashStruct( - 'EIP712Domain', - domain, - types, - SignTypedDataVersion.V4, - ).toString('hex'); - const hashStructMessageHex = TypedDataUtils.hashStruct( - primaryType.toString(), - message, - types, - SignTypedDataVersion.V4, - ).toString('hex'); const hdPath = await this.unlockAccountByAddress(withAccount); @@ -513,12 +501,24 @@ export class LedgerKeyring extends EventEmitter { throw new Error('Ledger: Unknown error while signing message'); } + const pt = primaryType.toString(); + let payload; try { payload = await this.bridge.deviceSignTypedData({ hdPath, - domainSeparatorHex, - hashStructMessageHex, + message: { + domain: { + name: domain.name, + chainId: domain.chainId, + version: domain.version, + verifyingContract: domain.verifyingContract, + salt: domain.salt? new TextDecoder().decode(domain.salt): undefined, + }, + types, + primaryType: pt, + message, + }, }); } catch (error) { throw error instanceof Error diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts index a26165bf..dded130b 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts @@ -1,5 +1,6 @@ import ledgerService from '@ledgerhq/hw-app-eth/lib/services/ledger'; import Transport from '@ledgerhq/hw-transport'; +import { EIP712Message } from '@ledgerhq/types-live'; import { MetaMaskLedgerHwAppEth } from './ledger-hw-app'; import { LedgerMobileBridge } from './ledger-mobile-bridge'; @@ -19,8 +20,8 @@ describe('LedgerMobileBridge', function () { let transportMiddlewareGetEthAppSpy: jest.SpyInstance; const mockEthApp = { - signEIP712HashedMessage: jest.fn(), - signTransaction: jest.fn(), + signEIP712Message: jest.fn(), + clearSignTransaction : jest.fn(), getAddress: jest.fn(), signPersonalMessage: jest.fn(), openEthApp: jest.fn(), @@ -155,50 +156,57 @@ describe('LedgerMobileBridge', function () { const hdPath = "m/44'/60'/0'/0/0"; const tx = 'f86d8202b38477359400825208944592d8f8d7b001e72cb26a73e4fa1806a51ac79d880de0b6b3a7640000802ba0699ff162205967ccbabae13e07cdd4284258d46ec1051a70a51be51ec2bc69f3a04e6944d508244ea54a62ebf9a72683eeadacb73ad7c373ee542f1998147b220e'; - const resolution = await ledgerService.resolveTransaction(tx, {}, {}); await bridge.deviceSignTransaction({ hdPath, tx, }); expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(1); - expect(mockEthApp.signTransaction).toHaveBeenCalledTimes(1); - expect(mockEthApp.signTransaction).toHaveBeenCalledWith( - hdPath, - tx, - resolution, - ); + expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1); + expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, { + externalPlugins: true, + erc20: true, + nft: true, + }); }); - it('throws an error when tx format is not correct', async function () { - const hdPath = "m/44'/60'/0'/0/0"; - const tx = ''; - await expect( - bridge.deviceSignTransaction({ - hdPath, - tx, - }), - ).rejects.toThrow(Error); - expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(0); - expect(mockEthApp.signTransaction).toHaveBeenCalledTimes(0); - }); + // it('throws an error when tx format is not correct', async function () { + // const hdPath = "m/44'/60'/0'/0/0"; + // const tx = ''; + // await expect( + // bridge.deviceSignTransaction({ + // hdPath, + // tx, + // }), + // ).rejects.toThrow(Error); + // expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(0); + // expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(0); + // }); }); describe('deviceSignTypedData', function () { it('sends and processes a successful ledger-sign-typed-data message', async function () { const hdPath = "m/44'/60'/0'/0/0"; - const domainSeparatorHex = 'domainSeparatorHex'; - const hashStructMessageHex = 'hashStructMessageHex'; + const message: EIP712Message = { + domain: { + chainId: 1, + verifyingContract: '0xdsf', + }, + primaryType: 'string', + types: { + EIP712Domain: [], + string: [], + }, + message: { test: 'test' }, + }; await bridge.deviceSignTypedData({ hdPath, - domainSeparatorHex, - hashStructMessageHex, + message, }); expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(1); - expect(mockEthApp.signEIP712HashedMessage).toHaveBeenCalledTimes(1); - expect(mockEthApp.signEIP712HashedMessage).toHaveBeenCalledWith( + expect(mockEthApp.signEIP712Message).toHaveBeenCalledTimes(1); + expect(mockEthApp.signEIP712Message).toHaveBeenCalledWith( hdPath, - domainSeparatorHex, - hashStructMessageHex, + message, ); }); }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts index 80109ad9..b72976a6 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts @@ -1,4 +1,3 @@ -import ledgerService from '@ledgerhq/hw-app-eth/lib/services/ledger'; import type Transport from '@ledgerhq/hw-transport'; import { @@ -90,20 +89,14 @@ export class LedgerMobileBridge implements MobileBridge { * * @param params - An object contains hdPath, domainSeparatorHex and hashStructMessageHex. * @param params.hdPath - The BIP 32 path of the account. - * @param params.domainSeparatorHex - The domain separator. - * @param params.hashStructMessageHex - The hashed struct message. + * @param params.message - The EIP712 message to sign. * @returns Retrieve v, r, s from the signed message. */ async deviceSignTypedData({ hdPath, - domainSeparatorHex, - hashStructMessageHex, + message, }: LedgerSignTypedDataParams): Promise { - return this.#getEthApp().signEIP712HashedMessage( - hdPath, - domainSeparatorHex, - hashStructMessageHex, - ); + return this.#getEthApp().signEIP712Message(hdPath, message); } /** @@ -119,8 +112,13 @@ export class LedgerMobileBridge implements MobileBridge { tx, hdPath, }: LedgerSignTransactionParams): Promise { - const resolution = await ledgerService.resolveTransaction(tx, {}, {}); - return this.#getEthApp().signTransaction(hdPath, tx, resolution); + const ethApp = this.#getEthApp(); + + return ethApp.clearSignTransaction(hdPath, tx, { + externalPlugins: true, + erc20: true, + nft: true, + }); } /** diff --git a/yarn.lock b/yarn.lock index 655303ac..9115aacb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1626,6 +1626,16 @@ __metadata: languageName: node linkType: hard +"@ledgerhq/types-live@npm:^6.53.0": + version: 6.53.0 + resolution: "@ledgerhq/types-live@npm:6.53.0" + dependencies: + bignumber.js: "npm:^9.1.2" + rxjs: "npm:^7.8.1" + checksum: 10/aeb4881f994b86ff5e284c8b24b81ad2fae21b87769639fd55b266f3979f6d66f0690ef1b76c51aeebb6238c3c780062d0b8e2e6d91a1860fc4827a3c4194b55 + languageName: node + linkType: hard + "@metamask/abi-utils@npm:^2.0.4": version: 2.0.4 resolution: "@metamask/abi-utils@npm:2.0.4" @@ -1900,6 +1910,7 @@ __metadata: "@ledgerhq/hw-transport": "npm:^6.31.3" "@ledgerhq/types-cryptoassets": "npm:^7.15.1" "@ledgerhq/types-devices": "npm:^6.25.3" + "@ledgerhq/types-live": "npm:^6.53.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/eth-sig-util": "npm:^8.0.0" "@metamask/utils": "npm:^9.3.0" From 49e6c80dd2236ff9afac8f90e7ff7423133838ba Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Wed, 20 Nov 2024 15:34:24 +0800 Subject: [PATCH 02/15] feat: Fix formatting issue. --- packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts | 4 +++- .../src/ledger-mobile-bridge.test.ts | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index e5caaa8e..e223086f 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -513,7 +513,9 @@ export class LedgerKeyring extends EventEmitter { chainId: domain.chainId, version: domain.version, verifyingContract: domain.verifyingContract, - salt: domain.salt? new TextDecoder().decode(domain.salt): undefined, + salt: domain.salt + ? new TextDecoder().decode(domain.salt) + : undefined, }, types, primaryType: pt, diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts index dded130b..152aaa56 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts @@ -1,4 +1,3 @@ -import ledgerService from '@ledgerhq/hw-app-eth/lib/services/ledger'; import Transport from '@ledgerhq/hw-transport'; import { EIP712Message } from '@ledgerhq/types-live'; @@ -21,7 +20,7 @@ describe('LedgerMobileBridge', function () { const mockEthApp = { signEIP712Message: jest.fn(), - clearSignTransaction : jest.fn(), + clearSignTransaction: jest.fn(), getAddress: jest.fn(), signPersonalMessage: jest.fn(), openEthApp: jest.fn(), From 1017fc2c77eb7965f6df2552337939d508e7c328 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Fri, 22 Nov 2024 16:33:05 +0800 Subject: [PATCH 03/15] feat: change not valid unit tests to valid case after using clearSignTransaction api for clear signing. --- .../src/ledger-mobile-bridge.test.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts index 152aaa56..68338091 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts @@ -168,18 +168,18 @@ describe('LedgerMobileBridge', function () { }); }); - // it('throws an error when tx format is not correct', async function () { - // const hdPath = "m/44'/60'/0'/0/0"; - // const tx = ''; - // await expect( - // bridge.deviceSignTransaction({ - // hdPath, - // tx, - // }), - // ).rejects.toThrow(Error); - // expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(0); - // expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(0); - // }); + it('returns undefined when tx format is not correct', async function () { + const hdPath = "m/44'/60'/0'/0/0"; + const tx = ''; + expect( + await bridge.deviceSignTransaction({ + hdPath, + tx, + }), + ).toBeUndefined(); + expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(1); + expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1); + }); }); describe('deviceSignTypedData', function () { From 78079b5ca5674086eebb8f8ff121d3fde22c57b2 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Fri, 22 Nov 2024 16:46:24 +0800 Subject: [PATCH 04/15] feat: Refactory the code in deviceSignTransaction. --- .../keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts index b72976a6..9e538159 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts @@ -112,9 +112,7 @@ export class LedgerMobileBridge implements MobileBridge { tx, hdPath, }: LedgerSignTransactionParams): Promise { - const ethApp = this.#getEthApp(); - - return ethApp.clearSignTransaction(hdPath, tx, { + return this.#getEthApp().clearSignTransaction(hdPath, tx, { externalPlugins: true, erc20: true, nft: true, From de0c84e060f56835d7a0a87d51ea5151da5051a2 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Fri, 22 Nov 2024 16:56:28 +0800 Subject: [PATCH 05/15] feat: Fix the `yarn lint` error in CI pipeline --- packages/keyring-eth-ledger-bridge/package.json | 2 +- yarn.lock | 12 +----------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/package.json b/packages/keyring-eth-ledger-bridge/package.json index 7efdd269..ebbc00b1 100644 --- a/packages/keyring-eth-ledger-bridge/package.json +++ b/packages/keyring-eth-ledger-bridge/package.json @@ -61,7 +61,7 @@ "@ledgerhq/hw-transport": "^6.31.3", "@ledgerhq/types-cryptoassets": "^7.15.1", "@ledgerhq/types-devices": "^6.25.3", - "@ledgerhq/types-live": "^6.53.0", + "@ledgerhq/types-live": "^6.52.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/utils": "^9.3.0", "@ts-bridge/cli": "^0.6.0", diff --git a/yarn.lock b/yarn.lock index 9115aacb..3df20179 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1626,16 +1626,6 @@ __metadata: languageName: node linkType: hard -"@ledgerhq/types-live@npm:^6.53.0": - version: 6.53.0 - resolution: "@ledgerhq/types-live@npm:6.53.0" - dependencies: - bignumber.js: "npm:^9.1.2" - rxjs: "npm:^7.8.1" - checksum: 10/aeb4881f994b86ff5e284c8b24b81ad2fae21b87769639fd55b266f3979f6d66f0690ef1b76c51aeebb6238c3c780062d0b8e2e6d91a1860fc4827a3c4194b55 - languageName: node - linkType: hard - "@metamask/abi-utils@npm:^2.0.4": version: 2.0.4 resolution: "@metamask/abi-utils@npm:2.0.4" @@ -1910,7 +1900,7 @@ __metadata: "@ledgerhq/hw-transport": "npm:^6.31.3" "@ledgerhq/types-cryptoassets": "npm:^7.15.1" "@ledgerhq/types-devices": "npm:^6.25.3" - "@ledgerhq/types-live": "npm:^6.53.0" + "@ledgerhq/types-live": "npm:^6.52.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/eth-sig-util": "npm:^8.0.0" "@metamask/utils": "npm:^9.3.0" From 5ac629ccb42ee98e000c3e70a32b73255025c03b Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Fri, 22 Nov 2024 22:13:13 +0800 Subject: [PATCH 06/15] feat: improve the unit test coverage, and replace TextDecoder with native compatible approach for mobile. --- .../src/ledger-keyring.test.ts | 28 +++++++++++++++++++ .../src/ledger-keyring.ts | 7 ++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts index 5695d055..3b060d47 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts @@ -928,6 +928,7 @@ describe('LedgerKeyring', function () { name: 'Ether Mail', verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC', version: '1', + salt: new TextEncoder().encode('hello'), }, message: { contents: 'Hello, Bob!', @@ -1000,6 +1001,33 @@ describe('LedgerKeyring', function () { ); }); + it('resolves properly when message domain salt is undefined', async function () { + const fixtureDataWithoutSalt = { + ...fixtureData, + domain: { + ...fixtureData.domain, + salt: undefined, + }, + }; + + jest + .spyOn(keyring.bridge, 'deviceSignTypedData') + .mockImplementation(async () => ({ + v: 27, + r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9', + s: '46759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e32', + })); + + const result = await keyring.signTypedData( + fakeAccounts[15], + fixtureDataWithoutSalt, + options, + ); + expect(result).toBe( + '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b', + ); + }); + it('throws error when address does not match', async function () { jest .spyOn(keyring.bridge, 'deviceSignTypedData') diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index e223086f..67ccbc77 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -514,7 +514,7 @@ export class LedgerKeyring extends EventEmitter { version: domain.version, verifyingContract: domain.verifyingContract, salt: domain.salt - ? new TextDecoder().decode(domain.salt) + ? this.#arrayBufferToString(domain.salt) : undefined, }, types, @@ -562,6 +562,11 @@ export class LedgerKeyring extends EventEmitter { this.hdk = new HDKey(); } + #arrayBufferToString(buffer: ArrayBuffer): string { + const uint8Array = new Uint8Array(buffer); + return String.fromCharCode(...uint8Array); + } + /* PRIVATE METHODS */ async #getPage(increment: number): Promise { this.page += increment; From 47b3edd3ff46f7ab89c1b64eff5dab1c9aae36a9 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Tue, 26 Nov 2024 17:02:33 +0800 Subject: [PATCH 07/15] feat: try to improve the unit tests for code coverage in ledger-iframe-bridge.ts --- .../keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts index efb57bb8..e8f31168 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts @@ -117,7 +117,7 @@ export class LedgerIframeBridge ) { this.#validateConfiguration(opts); this.#opts = { - bridgeUrl: opts?.bridgeUrl, + bridgeUrl: opts.bridgeUrl, }; } @@ -141,7 +141,7 @@ export class LedgerIframeBridge async setOptions(opts: LedgerIframeBridgeOptions): Promise { this.#validateConfiguration(opts); - if (this.#opts?.bridgeUrl !== opts.bridgeUrl) { + if (this.#opts.bridgeUrl !== opts.bridgeUrl) { this.#opts.bridgeUrl = opts.bridgeUrl; await this.destroy(); await this.init(); From 2e17328563ff18a88f70e3134b7ccfd714ec7106 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Tue, 26 Nov 2024 17:34:06 +0800 Subject: [PATCH 08/15] feat: try to improve the unit tests for code coverage in ledger-iframe-bridge.ts --- .../keyring-eth-ledger-bridge/jest.config.js | 8 +++---- .../src/ledger-iframe-bridge.test.ts | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index 4d5b3200..95879d6c 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/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.14, - functions: 95.95, - lines: 94.77, - statements: 94.83, + branches: 91.91, + functions: 96, + lines: 95.02, + statements: 95.07, }, }, }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts index f74150f0..4d47230c 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts @@ -177,6 +177,29 @@ describe('LedgerIframeBridge', function () { // eslint-disable-next-line @typescript-eslint/unbound-method expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled(); }); + + it('throws an error when unknown error occur', async function () { + const errorMessage = 'Unknown error occurred'; + + stubKeyringIFramePostMessage(bridge, (message) => { + expect(message).toStrictEqual({ + action: IFrameMessageAction.LedgerMakeApp, + messageId: 1, + target: LEDGER_IFRAME_ID, + }); + + bridge.messageCallbacks[message.messageId]?.({ + action: IFrameMessageAction.LedgerMakeApp, + messageId: 1, + success: false, + }); + }); + + await expect(bridge.attemptMakeApp()).rejects.toThrow(errorMessage); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled(); + }); }); describe('updateTransportMethod', function () { From 385a63df4bba3295fb44f43f0b58f88ce8d6d2b8 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Tue, 26 Nov 2024 17:42:36 +0800 Subject: [PATCH 09/15] feat: Revert the ledger-iframe-bridge.ts to main version. --- .../keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts index e8f31168..efb57bb8 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts @@ -117,7 +117,7 @@ export class LedgerIframeBridge ) { this.#validateConfiguration(opts); this.#opts = { - bridgeUrl: opts.bridgeUrl, + bridgeUrl: opts?.bridgeUrl, }; } @@ -141,7 +141,7 @@ export class LedgerIframeBridge async setOptions(opts: LedgerIframeBridgeOptions): Promise { this.#validateConfiguration(opts); - if (this.#opts.bridgeUrl !== opts.bridgeUrl) { + if (this.#opts?.bridgeUrl !== opts.bridgeUrl) { this.#opts.bridgeUrl = opts.bridgeUrl; await this.destroy(); await this.init(); From 4e3a6e1c3033be34c4427d7ecdaf43db39dc2884 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Tue, 26 Nov 2024 18:21:20 +0800 Subject: [PATCH 10/15] feat: Revert the jest.config.js to previous version and rerun the tests to update the coverage. --- packages/keyring-eth-ledger-bridge/jest.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index 95879d6c..c02ba14d 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/packages/keyring-eth-ledger-bridge/jest.config.js @@ -23,7 +23,7 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 91.91, + branches: 90.97, functions: 96, lines: 95.02, statements: 95.07, From aa81a41f19054ce29b5816284b88e38ac6015033 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang <7315988+dawnseeker8@users.noreply.github.com> Date: Wed, 4 Dec 2024 18:54:32 +0800 Subject: [PATCH 11/15] Apply suggestions from code review Co-authored-by: Charly Chevalier --- packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 67ccbc77..c4dbec75 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -514,6 +514,8 @@ export class LedgerKeyring extends EventEmitter { version: domain.version, verifyingContract: domain.verifyingContract, salt: domain.salt + // We convert this to a plain string to avoid encoding issue on the + // mobile side (to avoid using `TextDecoder`). ? this.#arrayBufferToString(domain.salt) : undefined, }, From 2ea4e7fc39a41fe7e7c91ff29c55d6ffb57a81ab Mon Sep 17 00:00:00 2001 From: Xiaoming Wang <7315988+dawnseeker8@users.noreply.github.com> Date: Wed, 4 Dec 2024 20:45:02 +0800 Subject: [PATCH 12/15] Apply suggestions from code review Co-authored-by: Charly Chevalier --- packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts | 2 +- .../src/ledger-mobile-bridge.test.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index c4dbec75..74ebff85 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -520,7 +520,7 @@ export class LedgerKeyring extends EventEmitter { : undefined, }, types, - primaryType: pt, + primaryType: primaryType.toString(), message, }, }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts index 68338091..6af71ed9 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts @@ -179,6 +179,11 @@ describe('LedgerMobileBridge', function () { ).toBeUndefined(); expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(1); expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1); + expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, { + externalPlugins: true, + erc20: true, + nft: true, + }); }); }); From ebd580305798db171298df6dd29db55e104c2621 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Wed, 4 Dec 2024 20:57:45 +0800 Subject: [PATCH 13/15] feat: Change base on dev review. --- .../keyring-eth-ledger-bridge/jest.config.js | 6 +-- .../keyring-eth-ledger-bridge/package.json | 2 +- .../src/ledger-iframe-bridge.test.ts | 46 +++++++------------ .../src/ledger-keyring.ts | 20 ++++---- yarn.lock | 12 ++++- 5 files changed, 41 insertions(+), 45 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index c02ba14d..b4262844 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/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.97, + branches: 90.9, functions: 96, - lines: 95.02, - statements: 95.07, + lines: 95.03, + statements: 95.09, }, }, }); diff --git a/packages/keyring-eth-ledger-bridge/package.json b/packages/keyring-eth-ledger-bridge/package.json index ebbc00b1..50b47f0c 100644 --- a/packages/keyring-eth-ledger-bridge/package.json +++ b/packages/keyring-eth-ledger-bridge/package.json @@ -61,7 +61,7 @@ "@ledgerhq/hw-transport": "^6.31.3", "@ledgerhq/types-cryptoassets": "^7.15.1", "@ledgerhq/types-devices": "^6.25.3", - "@ledgerhq/types-live": "^6.52.0", + "@ledgerhq/types-live": "^6.54.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/utils": "^9.3.0", "@ts-bridge/cli": "^0.6.0", diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts index 4d47230c..5ab159a4 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts @@ -458,27 +458,28 @@ describe('LedgerIframeBridge', function () { }); describe('deviceSignTypedData', function () { + const params = { + hdPath: "m/44'/60'/0'/0", + message: { + domain: { + chainId: 1, + verifyingContract: '0xdsf', + }, + primaryType: 'string', + types: { + EIP712Domain: [], + string: [], + }, + message: { test: 'test' }, + }, + }; + it('sends and processes a successful ledger-sign-typed-data message', async function () { const payload = { v: 0, r: '', s: '', }; - const params = { - hdPath: "m/44'/60'/0'/0", - message: { - domain: { - chainId: 1, - verifyingContract: '0xdsf', - }, - primaryType: 'string', - types: { - EIP712Domain: [], - string: [], - }, - message: { test: 'test' }, - }, - }; stubKeyringIFramePostMessage(bridge, (message) => { expect(message).toStrictEqual({ @@ -506,21 +507,6 @@ describe('LedgerIframeBridge', function () { it('throws an error when a ledger-sign-typed-data message is not successful', async function () { const errorMessage = 'Ledger Error'; - const params = { - hdPath: "m/44'/60'/0'/0", - message: { - domain: { - chainId: 1, - verifyingContract: '0xdsf', - }, - primaryType: 'string', - types: { - EIP712Domain: [], - string: [], - }, - message: { test: 'test' }, - }, - }; stubKeyringIFramePostMessage(bridge, (message) => { expect(message).toStrictEqual({ diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 74ebff85..f277270c 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -501,8 +501,6 @@ export class LedgerKeyring extends EventEmitter { throw new Error('Ledger: Unknown error while signing message'); } - const pt = primaryType.toString(); - let payload; try { payload = await this.bridge.deviceSignTypedData({ @@ -513,11 +511,7 @@ export class LedgerKeyring extends EventEmitter { chainId: domain.chainId, version: domain.version, verifyingContract: domain.verifyingContract, - salt: domain.salt - // We convert this to a plain string to avoid encoding issue on the - // mobile side (to avoid using `TextDecoder`). - ? this.#arrayBufferToString(domain.salt) - : undefined, + salt: this.#convertSaltIfAny(domain.salt), }, types, primaryType: primaryType.toString(), @@ -564,9 +558,15 @@ export class LedgerKeyring extends EventEmitter { this.hdk = new HDKey(); } - #arrayBufferToString(buffer: ArrayBuffer): string { - const uint8Array = new Uint8Array(buffer); - return String.fromCharCode(...uint8Array); + #convertSaltIfAny(salt: ArrayBuffer | undefined): string | undefined { + if (!salt) { + return undefined; + } + + // We convert this to a plain string to avoid encoding issue on the + // mobile side (to avoid using `TextDecoder`). + const saltBytes = new Uint8Array(salt); + return String.fromCharCode(...saltBytes); } /* PRIVATE METHODS */ diff --git a/yarn.lock b/yarn.lock index 3df20179..3b85dcde 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1626,6 +1626,16 @@ __metadata: languageName: node linkType: hard +"@ledgerhq/types-live@npm:^6.54.0": + version: 6.54.0 + resolution: "@ledgerhq/types-live@npm:6.54.0" + dependencies: + bignumber.js: "npm:^9.1.2" + rxjs: "npm:^7.8.1" + checksum: 10/90cf2207d10ab4f8db8461670fe53b74d56b19eb67b9f868e3159aba2fc77ca3b015a6f67af316cf3bd0711db395af11218dc845194c05f193a106a44730fcda + languageName: node + linkType: hard + "@metamask/abi-utils@npm:^2.0.4": version: 2.0.4 resolution: "@metamask/abi-utils@npm:2.0.4" @@ -1900,7 +1910,7 @@ __metadata: "@ledgerhq/hw-transport": "npm:^6.31.3" "@ledgerhq/types-cryptoassets": "npm:^7.15.1" "@ledgerhq/types-devices": "npm:^6.25.3" - "@ledgerhq/types-live": "npm:^6.52.0" + "@ledgerhq/types-live": "npm:^6.54.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/eth-sig-util": "npm:^8.0.0" "@metamask/utils": "npm:^9.3.0" From 35aeca9df36e2a3dd17ebef73ff4921a5132dbbf Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Wed, 4 Dec 2024 21:45:15 +0800 Subject: [PATCH 14/15] feat: fix the lint issue. --- yarn.lock | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/yarn.lock b/yarn.lock index 3b85dcde..843f9ea8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1616,17 +1616,7 @@ __metadata: languageName: node linkType: hard -"@ledgerhq/types-live@npm:^6.52.0": - version: 6.52.0 - resolution: "@ledgerhq/types-live@npm:6.52.0" - dependencies: - bignumber.js: "npm:^9.1.2" - rxjs: "npm:^7.8.1" - checksum: 10/c410f02159538d66f59956512fc5bab2cb17edee7f6a15a517c31d89d6c730e52691666bb2e1d98718c701e94306dd7498544ea3d7772ff0b5ad6522fb2c335c - languageName: node - linkType: hard - -"@ledgerhq/types-live@npm:^6.54.0": +"@ledgerhq/types-live@npm:^6.52.0, @ledgerhq/types-live@npm:^6.54.0": version: 6.54.0 resolution: "@ledgerhq/types-live@npm:6.54.0" dependencies: From 47e7cbfefd94e9822cf7f4056f7f782a160c67e7 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Thu, 5 Dec 2024 20:13:17 +0800 Subject: [PATCH 15/15] feat: Revert the types-live version based on Charly comments. --- packages/keyring-eth-ledger-bridge/package.json | 2 +- yarn.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/package.json b/packages/keyring-eth-ledger-bridge/package.json index 50b47f0c..ebbc00b1 100644 --- a/packages/keyring-eth-ledger-bridge/package.json +++ b/packages/keyring-eth-ledger-bridge/package.json @@ -61,7 +61,7 @@ "@ledgerhq/hw-transport": "^6.31.3", "@ledgerhq/types-cryptoassets": "^7.15.1", "@ledgerhq/types-devices": "^6.25.3", - "@ledgerhq/types-live": "^6.54.0", + "@ledgerhq/types-live": "^6.52.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/utils": "^9.3.0", "@ts-bridge/cli": "^0.6.0", diff --git a/yarn.lock b/yarn.lock index 843f9ea8..3df20179 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1616,13 +1616,13 @@ __metadata: languageName: node linkType: hard -"@ledgerhq/types-live@npm:^6.52.0, @ledgerhq/types-live@npm:^6.54.0": - version: 6.54.0 - resolution: "@ledgerhq/types-live@npm:6.54.0" +"@ledgerhq/types-live@npm:^6.52.0": + version: 6.52.0 + resolution: "@ledgerhq/types-live@npm:6.52.0" dependencies: bignumber.js: "npm:^9.1.2" rxjs: "npm:^7.8.1" - checksum: 10/90cf2207d10ab4f8db8461670fe53b74d56b19eb67b9f868e3159aba2fc77ca3b015a6f67af316cf3bd0711db395af11218dc845194c05f193a106a44730fcda + checksum: 10/c410f02159538d66f59956512fc5bab2cb17edee7f6a15a517c31d89d6c730e52691666bb2e1d98718c701e94306dd7498544ea3d7772ff0b5ad6522fb2c335c languageName: node linkType: hard @@ -1900,7 +1900,7 @@ __metadata: "@ledgerhq/hw-transport": "npm:^6.31.3" "@ledgerhq/types-cryptoassets": "npm:^7.15.1" "@ledgerhq/types-devices": "npm:^6.25.3" - "@ledgerhq/types-live": "npm:^6.54.0" + "@ledgerhq/types-live": "npm:^6.52.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/eth-sig-util": "npm:^8.0.0" "@metamask/utils": "npm:^9.3.0"