From a4a77f90f04b6744fd3c2f1a960ba934bf33d0dc Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Wed, 29 Nov 2023 15:08:43 +0800 Subject: [PATCH 1/7] feat: refactor to enable addtional bridge --- jest.config.js | 8 ++-- src/ledger-bridge.ts | 19 ++++++++- src/ledger-iframe-bridge.test.ts | 68 ++++++++++++++++++++++++++++++-- src/ledger-iframe-bridge.ts | 47 ++++++++++++++++++++-- src/ledger-keyring.test.ts | 50 +++++++++++++++++------ src/ledger-keyring.ts | 34 ++++++++++------ 6 files changed, 189 insertions(+), 37 deletions(-) diff --git a/jest.config.js b/jest.config.js index 6fc5cddf..c636ebbc 100644 --- a/jest.config.js +++ b/jest.config.js @@ -41,10 +41,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 65.42, - functions: 88.57, - lines: 81.63, - statements: 81.55, + branches: 69.42, + functions: 89.18, + lines: 82.27, + statements: 82.18, }, }, diff --git a/src/ledger-bridge.ts b/src/ledger-bridge.ts index bcee7afe..94fa2c5c 100644 --- a/src/ledger-bridge.ts +++ b/src/ledger-bridge.ts @@ -26,14 +26,29 @@ export type LedgerSignTypedDataResponse = Awaited< ReturnType >; +export type LedgerBridgeOptions = Record; + +export type LedgerBridgeSerializeData = Record< + string, + string | number | object +>; + // eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface LedgerBridge { +export interface LedgerBridge { isDeviceConnected: boolean; - init(bridgeUrl: string): Promise; + init(): Promise; destroy(): Promise; + serializeData(): Promise; + + deserializeData(opts: LedgerBridgeSerializeData): Promise; + + getOptions(): Promise; + + setOptions(opts: T): Promise; + attemptMakeApp(): Promise; updateTransportMethod(transportType: string): Promise; diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index 161eaf69..3aaffacb 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -50,6 +50,7 @@ async function simulateIFrameLoad(iframe?: HTMLIFrameElementShim) { } const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; +const BRIDGE_URL = 'BRIDGE_URL'; describe('LedgerIframeBridge', function () { let bridge: LedgerIframeBridge; @@ -74,8 +75,10 @@ describe('LedgerIframeBridge', function () { } beforeEach(async function () { - bridge = new LedgerIframeBridge(); - await bridge.init('bridgeUrl'); + bridge = new LedgerIframeBridge({ + bridgeUrl: BRIDGE_URL, + }); + await bridge.init(); await simulateIFrameLoad(bridge.iframe); }); @@ -89,7 +92,7 @@ describe('LedgerIframeBridge', function () { const addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); - await bridge.init('bridgeUrl'); + await bridge.init(); expect(addEventListenerSpy).toHaveBeenCalledTimes(1); expect(bridge.iframeLoaded).toBe(false); @@ -479,4 +482,63 @@ describe('LedgerIframeBridge', function () { expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled(); }); }); + + describe('deserializeData', function () { + it('data has assigned to instance', async function () { + bridge.bridgeUrl = ''; + await bridge.deserializeData({ bridgeUrl: BRIDGE_URL }); + expect(bridge.bridgeUrl).toBe(BRIDGE_URL); + }); + }); + + describe('serializeData', function () { + it('return struct data', async function () { + await bridge.deserializeData({ bridgeUrl: BRIDGE_URL }); + const result = await bridge.serializeData(); + expect(result).toStrictEqual({ + bridgeUrl: BRIDGE_URL, + }); + }); + }); + + describe('setOption', function () { + let removeEventListenerSpy: jest.SpyInstance; + let addEventListenerSpy: jest.SpyInstance; + + beforeEach(() => { + removeEventListenerSpy = jest.spyOn(global.window, 'removeEventListener'); + addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); + bridge = new LedgerIframeBridge({ + bridgeUrl: BRIDGE_URL, + }); + }); + + it('option set correctly', async function () { + await bridge.init(); + await bridge.setOptions({ bridgeUrl: 'another url' }); + await simulateIFrameLoad(bridge.iframe); + expect(bridge.bridgeUrl).toBe('another url'); + expect(addEventListenerSpy).toHaveBeenCalledTimes(2); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(1); + }); + + it('option will not set when given bridgeUrl is same', async function () { + await bridge.init(); + await bridge.setOptions({ bridgeUrl: BRIDGE_URL }); + await simulateIFrameLoad(bridge.iframe); + expect(bridge.bridgeUrl).toBe(BRIDGE_URL); + expect(addEventListenerSpy).toHaveBeenCalledTimes(1); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(0); + }); + }); + + describe('getOption', function () { + it('return instance options', async function () { + bridge.bridgeUrl = BRIDGE_URL; + const result = await bridge.getOptions(); + expect(result).toStrictEqual({ + bridgeUrl: BRIDGE_URL, + }); + }); + }); }); diff --git a/src/ledger-iframe-bridge.ts b/src/ledger-iframe-bridge.ts index a5ffe5f8..766a467b 100644 --- a/src/ledger-iframe-bridge.ts +++ b/src/ledger-iframe-bridge.ts @@ -8,6 +8,7 @@ import { LedgerSignTransactionResponse, LedgerSignTypedDataParams, LedgerSignTypedDataResponse, + LedgerBridgeSerializeData, } from './ledger-bridge'; const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; @@ -73,11 +74,21 @@ type IFramePostMessage = target: typeof LEDGER_IFRAME_ID; }; -export class LedgerIframeBridge implements LedgerBridge { +const BRIDGE_URL = 'https://metamask.github.io/eth-ledger-bridge-keyring'; + +export type LedgerIframeBridgeOptions = { + bridgeUrl: string; +}; + +export class LedgerIframeBridge + implements LedgerBridge +{ iframe?: HTMLIFrameElement; iframeLoaded = false; + bridgeUrl = ''; + eventListener?: (eventMessage: { origin: string; data: IFrameMessageResponse; @@ -98,10 +109,14 @@ export class LedgerIframeBridge implements LedgerBridge { transportType: string; }; - async init(bridgeUrl: string) { - this.#setupIframe(bridgeUrl); + constructor(opts?: LedgerIframeBridgeOptions) { + this.bridgeUrl = opts?.bridgeUrl ?? BRIDGE_URL; + } + + async init() { + this.#setupIframe(this.bridgeUrl); - this.eventListener = this.#eventListener.bind(this, bridgeUrl); + this.eventListener = this.#eventListener.bind(this, this.bridgeUrl); window.addEventListener('message', this.eventListener); } @@ -112,6 +127,30 @@ export class LedgerIframeBridge implements LedgerBridge { } } + async deserializeData( + serializeData: LedgerBridgeSerializeData, + ): Promise { + this.bridgeUrl = (serializeData.bridgeUrl as string) ?? this.bridgeUrl; + } + + async serializeData(): Promise { + return { + bridgeUrl: this.bridgeUrl, + }; + } + + async getOptions(): Promise { + return { bridgeUrl: this.bridgeUrl }; + } + + async setOptions(opts: LedgerIframeBridgeOptions): Promise { + if (opts.bridgeUrl && this.bridgeUrl !== opts.bridgeUrl) { + this.bridgeUrl = opts.bridgeUrl; + await this.destroy(); + await this.init(); + } + } + async attemptMakeApp(): Promise { return new Promise((resolve, reject) => { this.#sendMessage( diff --git a/src/ledger-keyring.test.ts b/src/ledger-keyring.test.ts index f457001f..0f9665a4 100644 --- a/src/ledger-keyring.test.ts +++ b/src/ledger-keyring.test.ts @@ -7,7 +7,10 @@ import EthereumTx from 'ethereumjs-tx'; import HDKey from 'hdkey'; import { LedgerBridge } from './ledger-bridge'; -import { LedgerIframeBridge } from './ledger-iframe-bridge'; +import { + LedgerIframeBridge, + LedgerIframeBridgeOptions, +} from './ledger-iframe-bridge'; import { AccountDetails, LedgerKeyring } from './ledger-keyring'; jest.mock('@metamask/eth-sig-util', () => { @@ -83,10 +86,12 @@ const fakeTypeTwoTx = TransactionFactory.fromTxData( { common: commonEIP1559, freeze: false }, ); +const BRIDGE_URL = 'BRIDGE_URL'; + describe('LedgerKeyring', function () { let keyring: LedgerKeyring; - let bridge: LedgerBridge; - + let bridge: LedgerBridge; + const opts = { bridgeUrl: BRIDGE_URL }; /** * Sets up the keyring to unlock one account. * @@ -102,7 +107,7 @@ describe('LedgerKeyring', function () { } beforeEach(async function () { - bridge = new LedgerIframeBridge(); + bridge = new LedgerIframeBridge(opts); keyring = new LedgerKeyring({ bridge }); keyring.hdk = fakeHdKey; await keyring.deserialize(); @@ -128,7 +133,7 @@ describe('LedgerKeyring', function () { describe('constructor', function () { it('constructs', async function () { const ledgerKeyring = new LedgerKeyring({ - bridge: new LedgerIframeBridge(), + bridge: new LedgerIframeBridge(opts), }); expect(typeof ledgerKeyring).toBe('object'); @@ -140,7 +145,8 @@ describe('LedgerKeyring', function () { expect( () => new LedgerKeyring({ - bridge: undefined as unknown as LedgerBridge, + bridge: + undefined as unknown as LedgerBridge, }), ).toThrow('Bridge is a required dependency for the keyring'); }); @@ -161,9 +167,7 @@ describe('LedgerKeyring', function () { it('serializes an instance', async function () { const output = await keyring.serialize(); - expect(output.bridgeUrl).toBe( - 'https://metamask.github.io/eth-ledger-bridge-keyring', - ); + expect(output.bridgeOptions).toHaveProperty('bridgeUrl', BRIDGE_URL); expect(output.hdPath).toBe(`m/44'/60'/0'`); expect(Array.isArray(output.accounts)).toBe(true); expect(output.accounts).toHaveLength(0); @@ -184,13 +188,35 @@ describe('LedgerKeyring', function () { hdPath: someHdPath, accounts: [account], accountDetails, + bridgeOptions: { bridgeUrl: BRIDGE_URL }, }); const serialized = await keyring.serialize(); expect(serialized.accounts).toHaveLength(1); - expect(serialized.bridgeUrl).toBe( - 'https://metamask.github.io/eth-ledger-bridge-keyring', - ); + expect(serialized.bridgeOptions).toHaveProperty('bridgeUrl', BRIDGE_URL); + expect(serialized.hdPath).toBe(someHdPath); + expect(serialized.accountDetails).toStrictEqual(accountDetails); + }); + + it('should deserializes with bridgeUrl', async function () { + const account = fakeAccounts[0]; + const checksum = ethUtil.toChecksumAddress(account); + const someHdPath = `m/44'/60'/0'/1`; + const accountDetails: Record = {}; + accountDetails[checksum] = { + index: 0, + hdPath: someHdPath, + }; + await keyring.deserialize({ + hdPath: someHdPath, + accounts: [account], + accountDetails, + bridgeUrl: 'URL2', + }); + const serialized = await keyring.serialize(); + + expect(serialized.accounts).toHaveLength(1); + expect(serialized.bridgeOptions).toHaveProperty('bridgeUrl', 'URL2'); expect(serialized.hdPath).toBe(someHdPath); expect(serialized.accountDetails).toStrictEqual(accountDetails); }); diff --git a/src/ledger-keyring.ts b/src/ledger-keyring.ts index 77392c5b..10eb48e2 100644 --- a/src/ledger-keyring.ts +++ b/src/ledger-keyring.ts @@ -15,14 +15,16 @@ import type OldEthJsTransaction from 'ethereumjs-tx'; import { EventEmitter } from 'events'; import HDKey from 'hdkey'; -import { LedgerBridge } from './ledger-bridge'; +import { + LedgerBridge, + LedgerBridgeOptions, + LedgerBridgeSerializeData, +} from './ledger-bridge'; const pathBase = 'm'; const hdPathString = `${pathBase}/44'/60'/0'`; const keyringType = 'Ledger Hardware'; -const BRIDGE_URL = 'https://metamask.github.io/eth-ledger-bridge-keyring'; - const MAX_INDEX = 1000; enum NetworkApiUrls { @@ -33,7 +35,7 @@ enum NetworkApiUrls { } type SignTransactionPayload = Awaited< - ReturnType + ReturnType['deviceSignTransaction']> >; export type AccountDetails = { @@ -47,8 +49,9 @@ export type LedgerBridgeKeyringOptions = { accounts: readonly string[]; accountDetails: Readonly>; accountIndexes: Readonly>; - bridgeUrl: string; implementFullBIP44: boolean; + bridgeUrl?: string; + bridgeOptions?: LedgerBridgeSerializeData; }; /** @@ -95,11 +98,9 @@ export class LedgerKeyring extends EventEmitter { implementFullBIP44 = false; - bridgeUrl: string = BRIDGE_URL; + bridge: LedgerBridge>; - bridge: LedgerBridge; - - constructor({ bridge }: { bridge: LedgerBridge }) { + constructor({ bridge }: { bridge: LedgerBridge }) { super(); if (!bridge) { @@ -110,7 +111,7 @@ export class LedgerKeyring extends EventEmitter { } async init() { - return this.bridge.init(this.bridgeUrl); + return this.bridge.init(); } async destroy() { @@ -122,16 +123,25 @@ export class LedgerKeyring extends EventEmitter { hdPath: this.hdPath, accounts: this.accounts, accountDetails: this.accountDetails, - bridgeUrl: this.bridgeUrl, implementFullBIP44: false, + bridgeOptions: await this.bridge.serializeData(), }; } async deserialize(opts: Partial = {}) { this.hdPath = opts.hdPath ?? hdPathString; - this.bridgeUrl = opts.bridgeUrl ?? BRIDGE_URL; this.accounts = opts.accounts ?? []; this.accountDetails = opts.accountDetails ?? {}; + + if (opts.bridgeOptions) { + await this.bridge.deserializeData(opts.bridgeOptions); + } else if (opts.bridgeUrl) { + // allow legacy options bridgeUrl to be passed in + await this.bridge.deserializeData({ + bridgeUrl: opts.bridgeUrl, + }); + } + if (!opts.accountDetails) { this.#migrateAccountDetails(opts); } From 5dde0ba39106d81e9d101565e3a1d131bb3dd041 Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Thu, 30 Nov 2023 14:31:55 +0800 Subject: [PATCH 2/7] feat: update jest score --- jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jest.config.js b/jest.config.js index 95acdb90..3c58cff8 100644 --- a/jest.config.js +++ b/jest.config.js @@ -41,10 +41,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 68.06, - functions: 88.57, - lines: 81.29, - statements: 81.21, + branches: 71.42, + functions: 89.18, + lines: 81.93, + statements: 81.84, }, }, From 045a4f453c61005b4bb743c68ee20a6e3938830a Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Thu, 30 Nov 2023 14:40:30 +0800 Subject: [PATCH 3/7] fix: incorrect LedgerBridge type --- src/ledger-keyring.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ledger-keyring.ts b/src/ledger-keyring.ts index 10eb48e2..48c0e02a 100644 --- a/src/ledger-keyring.ts +++ b/src/ledger-keyring.ts @@ -98,7 +98,7 @@ export class LedgerKeyring extends EventEmitter { implementFullBIP44 = false; - bridge: LedgerBridge>; + bridge: LedgerBridge; constructor({ bridge }: { bridge: LedgerBridge }) { super(); From c689469010ed23fd1c8840e9999334908e53ad37 Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Tue, 5 Dec 2023 11:26:41 +0800 Subject: [PATCH 4/7] fix: resolve PR comment Due to either bridgeOptions or bridgeUrl is not part of account data, it should be removed to remain the bridge configurated by itself but not depend on account Remove deserializeData and serializeData due to same reason Change bridgeUrl to be a object of configuration --- src/ledger-bridge.ts | 19 +++---- src/ledger-iframe-bridge.test.ts | 96 +++++++++++++++++++++----------- src/ledger-iframe-bridge.ts | 29 +++------- src/ledger-keyring.test.ts | 26 --------- src/ledger-keyring.ts | 18 +----- 5 files changed, 81 insertions(+), 107 deletions(-) diff --git a/src/ledger-bridge.ts b/src/ledger-bridge.ts index 94fa2c5c..fc32c319 100644 --- a/src/ledger-bridge.ts +++ b/src/ledger-bridge.ts @@ -26,12 +26,7 @@ export type LedgerSignTypedDataResponse = Awaited< ReturnType >; -export type LedgerBridgeOptions = Record; - -export type LedgerBridgeSerializeData = Record< - string, - string | number | object ->; +export type LedgerBridgeOptions = Record; // eslint-disable-next-line @typescript-eslint/consistent-type-definitions export interface LedgerBridge { @@ -41,12 +36,16 @@ export interface LedgerBridge { destroy(): Promise; - serializeData(): Promise; - - deserializeData(opts: LedgerBridgeSerializeData): Promise; - + /** + * Method to get the current configuration of the ledger bridge keyring. + */ getOptions(): Promise; + /** + * Method to set the current configuration of the ledger bridge keyring. + * + * @param opts - An object contains configuration of the bridge. + */ setOptions(opts: T): Promise; attemptMakeApp(): Promise; diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index 8695ad92..6f6b29cd 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -484,58 +484,86 @@ describe('LedgerIframeBridge', function () { }); }); - describe('deserializeData', function () { - it('data has assigned to instance', async function () { - bridge.bridgeUrl = ''; - await bridge.deserializeData({ bridgeUrl: BRIDGE_URL }); - expect(bridge.bridgeUrl).toBe(BRIDGE_URL); - }); - }); - - describe('serializeData', function () { - it('return struct data', async function () { - await bridge.deserializeData({ bridgeUrl: BRIDGE_URL }); - const result = await bridge.serializeData(); - expect(result).toStrictEqual({ - bridgeUrl: BRIDGE_URL, - }); - }); - }); - describe('setOption', function () { let removeEventListenerSpy: jest.SpyInstance; let addEventListenerSpy: jest.SpyInstance; + const defaultIframeLoadedCounter = 1; - beforeEach(() => { + beforeEach(async () => { removeEventListenerSpy = jest.spyOn(global.window, 'removeEventListener'); addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); bridge = new LedgerIframeBridge({ bridgeUrl: BRIDGE_URL, }); - }); - - it('option set correctly', async function () { await bridge.init(); - await bridge.setOptions({ bridgeUrl: 'another url' }); await simulateIFrameLoad(bridge.iframe); - expect(bridge.bridgeUrl).toBe('another url'); - expect(addEventListenerSpy).toHaveBeenCalledTimes(2); - expect(removeEventListenerSpy).toHaveBeenCalledTimes(1); }); - it('option will not set when given bridgeUrl is same', async function () { - await bridge.init(); - await bridge.setOptions({ bridgeUrl: BRIDGE_URL }); - await simulateIFrameLoad(bridge.iframe); - expect(bridge.bridgeUrl).toBe(BRIDGE_URL); - expect(addEventListenerSpy).toHaveBeenCalledTimes(1); - expect(removeEventListenerSpy).toHaveBeenCalledTimes(0); + describe('when configurate bridge url', function () { + describe('when given bridge url is different with current', function () { + beforeEach(async () => { + await bridge.setOptions({ bridgeUrl: 'another url' }); + }); + + it('should set bridgeUrl correctly', async function () { + expect(await bridge.getOptions()).toHaveProperty( + 'bridgeUrl', + 'another url', + ); + }); + + it('should reload the iframe', async function () { + expect(addEventListenerSpy).toHaveBeenCalledTimes( + defaultIframeLoadedCounter + 1, + ); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe('when given bridge url is same as current', function () { + beforeEach(async () => { + await bridge.setOptions({ bridgeUrl: BRIDGE_URL }); + }); + + it('should not set bridgeUrl', async function () { + expect(await bridge.getOptions()).toHaveProperty( + 'bridgeUrl', + BRIDGE_URL, + ); + }); + + it('should not reload the iframe', async function () { + expect(addEventListenerSpy).toHaveBeenCalledTimes( + defaultIframeLoadedCounter, + ); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(0); + }); + }); + + describe('when given bridge url is empty', function () { + beforeEach(async () => { + await bridge.setOptions({ bridgeUrl: '' }); + }); + + it('should not set bridgeUrl', async function () { + expect(await bridge.getOptions()).toHaveProperty( + 'bridgeUrl', + BRIDGE_URL, + ); + }); + + it('should not reload the iframe', async function () { + expect(addEventListenerSpy).toHaveBeenCalledTimes( + defaultIframeLoadedCounter, + ); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(0); + }); + }); }); }); describe('getOption', function () { it('return instance options', async function () { - bridge.bridgeUrl = BRIDGE_URL; const result = await bridge.getOptions(); expect(result).toStrictEqual({ bridgeUrl: BRIDGE_URL, diff --git a/src/ledger-iframe-bridge.ts b/src/ledger-iframe-bridge.ts index 76ce6af7..b3510c12 100644 --- a/src/ledger-iframe-bridge.ts +++ b/src/ledger-iframe-bridge.ts @@ -8,7 +8,6 @@ import { LedgerSignTransactionResponse, LedgerSignTypedDataParams, LedgerSignTypedDataResponse, - LedgerBridgeSerializeData, } from './ledger-bridge'; const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; @@ -99,7 +98,7 @@ export class LedgerIframeBridge iframeLoaded = false; - bridgeUrl = ''; + #opts: LedgerIframeBridgeOptions = { bridgeUrl: BRIDGE_URL }; eventListener?: (eventMessage: { origin: string; @@ -120,13 +119,15 @@ export class LedgerIframeBridge }; constructor(opts?: LedgerIframeBridgeOptions) { - this.bridgeUrl = opts?.bridgeUrl ?? BRIDGE_URL; + if (opts?.bridgeUrl) { + this.#opts.bridgeUrl = opts.bridgeUrl; + } } async init() { - this.#setupIframe(this.bridgeUrl); + this.#setupIframe(this.#opts.bridgeUrl); - this.eventListener = this.#eventListener.bind(this, this.bridgeUrl); + this.eventListener = this.#eventListener.bind(this, this.#opts.bridgeUrl); window.addEventListener('message', this.eventListener); } @@ -137,25 +138,13 @@ export class LedgerIframeBridge } } - async deserializeData( - serializeData: LedgerBridgeSerializeData, - ): Promise { - this.bridgeUrl = (serializeData.bridgeUrl as string) ?? this.bridgeUrl; - } - - async serializeData(): Promise { - return { - bridgeUrl: this.bridgeUrl, - }; - } - async getOptions(): Promise { - return { bridgeUrl: this.bridgeUrl }; + return this.#opts; } async setOptions(opts: LedgerIframeBridgeOptions): Promise { - if (opts.bridgeUrl && this.bridgeUrl !== opts.bridgeUrl) { - this.bridgeUrl = opts.bridgeUrl; + if (opts.bridgeUrl && this.#opts.bridgeUrl !== opts.bridgeUrl) { + this.#opts.bridgeUrl = opts.bridgeUrl; await this.destroy(); await this.init(); } diff --git a/src/ledger-keyring.test.ts b/src/ledger-keyring.test.ts index 0f9665a4..e05629cd 100644 --- a/src/ledger-keyring.test.ts +++ b/src/ledger-keyring.test.ts @@ -167,7 +167,6 @@ describe('LedgerKeyring', function () { it('serializes an instance', async function () { const output = await keyring.serialize(); - expect(output.bridgeOptions).toHaveProperty('bridgeUrl', BRIDGE_URL); expect(output.hdPath).toBe(`m/44'/60'/0'`); expect(Array.isArray(output.accounts)).toBe(true); expect(output.accounts).toHaveLength(0); @@ -188,35 +187,10 @@ describe('LedgerKeyring', function () { hdPath: someHdPath, accounts: [account], accountDetails, - bridgeOptions: { bridgeUrl: BRIDGE_URL }, }); const serialized = await keyring.serialize(); expect(serialized.accounts).toHaveLength(1); - expect(serialized.bridgeOptions).toHaveProperty('bridgeUrl', BRIDGE_URL); - expect(serialized.hdPath).toBe(someHdPath); - expect(serialized.accountDetails).toStrictEqual(accountDetails); - }); - - it('should deserializes with bridgeUrl', async function () { - const account = fakeAccounts[0]; - const checksum = ethUtil.toChecksumAddress(account); - const someHdPath = `m/44'/60'/0'/1`; - const accountDetails: Record = {}; - accountDetails[checksum] = { - index: 0, - hdPath: someHdPath, - }; - await keyring.deserialize({ - hdPath: someHdPath, - accounts: [account], - accountDetails, - bridgeUrl: 'URL2', - }); - const serialized = await keyring.serialize(); - - expect(serialized.accounts).toHaveLength(1); - expect(serialized.bridgeOptions).toHaveProperty('bridgeUrl', 'URL2'); expect(serialized.hdPath).toBe(someHdPath); expect(serialized.accountDetails).toStrictEqual(accountDetails); }); diff --git a/src/ledger-keyring.ts b/src/ledger-keyring.ts index 48c0e02a..28f89780 100644 --- a/src/ledger-keyring.ts +++ b/src/ledger-keyring.ts @@ -15,11 +15,7 @@ import type OldEthJsTransaction from 'ethereumjs-tx'; import { EventEmitter } from 'events'; import HDKey from 'hdkey'; -import { - LedgerBridge, - LedgerBridgeOptions, - LedgerBridgeSerializeData, -} from './ledger-bridge'; +import { LedgerBridge, LedgerBridgeOptions } from './ledger-bridge'; const pathBase = 'm'; const hdPathString = `${pathBase}/44'/60'/0'`; @@ -50,8 +46,6 @@ export type LedgerBridgeKeyringOptions = { accountDetails: Readonly>; accountIndexes: Readonly>; implementFullBIP44: boolean; - bridgeUrl?: string; - bridgeOptions?: LedgerBridgeSerializeData; }; /** @@ -124,7 +118,6 @@ export class LedgerKeyring extends EventEmitter { accounts: this.accounts, accountDetails: this.accountDetails, implementFullBIP44: false, - bridgeOptions: await this.bridge.serializeData(), }; } @@ -133,15 +126,6 @@ export class LedgerKeyring extends EventEmitter { this.accounts = opts.accounts ?? []; this.accountDetails = opts.accountDetails ?? {}; - if (opts.bridgeOptions) { - await this.bridge.deserializeData(opts.bridgeOptions); - } else if (opts.bridgeUrl) { - // allow legacy options bridgeUrl to be passed in - await this.bridge.deserializeData({ - bridgeUrl: opts.bridgeUrl, - }); - } - if (!opts.accountDetails) { this.#migrateAccountDetails(opts); } From e8b9088742601960033f3e826f2dbfc18855768c Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Tue, 5 Dec 2023 11:27:57 +0800 Subject: [PATCH 5/7] fix: update jest score --- jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jest.config.js b/jest.config.js index 3c58cff8..1a6782c5 100644 --- a/jest.config.js +++ b/jest.config.js @@ -41,10 +41,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 71.42, - functions: 89.18, - lines: 81.93, - statements: 81.84, + branches: 69.91, + functions: 88.88, + lines: 81.64, + statements: 81.56, }, }, From a373bce03a83cf260d3ec14a53c61ea9c29a34fb Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Tue, 5 Dec 2023 13:10:46 +0800 Subject: [PATCH 6/7] fix: resolve PR comment --- jest.config.js | 8 ++-- src/ledger-iframe-bridge.test.ts | 63 +++++++++++++++++++++----------- src/ledger-iframe-bridge.ts | 26 +++++++++---- src/ledger-keyring.test.ts | 7 +--- 4 files changed, 65 insertions(+), 39 deletions(-) diff --git a/jest.config.js b/jest.config.js index 1a6782c5..7bd25942 100644 --- a/jest.config.js +++ b/jest.config.js @@ -41,10 +41,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 69.91, - functions: 88.88, - lines: 81.64, - statements: 81.56, + branches: 69.53, + functions: 89.04, + lines: 81.76, + statements: 81.67, }, }, diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index 6f6b29cd..9c12b628 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -51,8 +51,8 @@ async function simulateIFrameLoad(iframe?: HTMLIFrameElementShim) { } const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; -const BRIDGE_URL = 'BRIDGE_URL'; - +const BRIDGE_URL = 'https://metamask.github.io/eth-ledger-bridge-keyring'; +const INVALID_URL_ERROR = 'bridgeURL is not a valid URL'; describe('LedgerIframeBridge', function () { let bridge: LedgerIframeBridge; @@ -87,6 +87,39 @@ describe('LedgerIframeBridge', function () { jest.clearAllMocks(); }); + describe('constructor', function () { + describe('when configurate not given', function () { + it('should use the default bridgeUrl', async function () { + bridge = new LedgerIframeBridge(); + expect(await bridge.getOptions()).toHaveProperty( + 'bridgeUrl', + BRIDGE_URL, + ); + }); + }); + + describe('when configurate given', function () { + it('should set the given bridgeUrl', async function () { + bridge = new LedgerIframeBridge({ + bridgeUrl: 'https://metamask.io', + }); + expect(await bridge.getOptions()).toHaveProperty( + 'bridgeUrl', + 'https://metamask.io', + ); + }); + + it('should throw error if given url is empty', async function () { + expect( + () => + new LedgerIframeBridge({ + bridgeUrl: '', + }), + ).toThrow(INVALID_URL_ERROR); + }); + }); + }); + describe('init', function () { it('sets up the listener and iframe', async function () { bridge = new LedgerIframeBridge(); @@ -492,9 +525,7 @@ describe('LedgerIframeBridge', function () { beforeEach(async () => { removeEventListenerSpy = jest.spyOn(global.window, 'removeEventListener'); addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); - bridge = new LedgerIframeBridge({ - bridgeUrl: BRIDGE_URL, - }); + bridge = new LedgerIframeBridge(); await bridge.init(); await simulateIFrameLoad(bridge.iframe); }); @@ -502,13 +533,13 @@ describe('LedgerIframeBridge', function () { describe('when configurate bridge url', function () { describe('when given bridge url is different with current', function () { beforeEach(async () => { - await bridge.setOptions({ bridgeUrl: 'another url' }); + await bridge.setOptions({ bridgeUrl: 'https://metamask.io' }); }); it('should set bridgeUrl correctly', async function () { expect(await bridge.getOptions()).toHaveProperty( 'bridgeUrl', - 'another url', + 'https://metamask.io', ); }); @@ -541,23 +572,11 @@ describe('LedgerIframeBridge', function () { }); describe('when given bridge url is empty', function () { - beforeEach(async () => { - await bridge.setOptions({ bridgeUrl: '' }); - }); - - it('should not set bridgeUrl', async function () { - expect(await bridge.getOptions()).toHaveProperty( - 'bridgeUrl', - BRIDGE_URL, + it('should throw error', async function () { + await expect(bridge.setOptions({ bridgeUrl: '' })).rejects.toThrow( + INVALID_URL_ERROR, ); }); - - it('should not reload the iframe', async function () { - expect(addEventListenerSpy).toHaveBeenCalledTimes( - defaultIframeLoadedCounter, - ); - expect(removeEventListenerSpy).toHaveBeenCalledTimes(0); - }); }); }); }); diff --git a/src/ledger-iframe-bridge.ts b/src/ledger-iframe-bridge.ts index b3510c12..f87aca5c 100644 --- a/src/ledger-iframe-bridge.ts +++ b/src/ledger-iframe-bridge.ts @@ -85,8 +85,6 @@ type IFramePostMessage = target: typeof LEDGER_IFRAME_ID; }; -const BRIDGE_URL = 'https://metamask.github.io/eth-ledger-bridge-keyring'; - export type LedgerIframeBridgeOptions = { bridgeUrl: string; }; @@ -98,7 +96,7 @@ export class LedgerIframeBridge iframeLoaded = false; - #opts: LedgerIframeBridgeOptions = { bridgeUrl: BRIDGE_URL }; + #opts: LedgerIframeBridgeOptions; eventListener?: (eventMessage: { origin: string; @@ -118,10 +116,15 @@ export class LedgerIframeBridge transportType: string; }; - constructor(opts?: LedgerIframeBridgeOptions) { - if (opts?.bridgeUrl) { - this.#opts.bridgeUrl = opts.bridgeUrl; - } + constructor( + opts: LedgerIframeBridgeOptions = { + bridgeUrl: 'https://metamask.github.io/eth-ledger-bridge-keyring', + }, + ) { + this.#validateConfiguration(opts); + this.#opts = { + bridgeUrl: opts?.bridgeUrl, + }; } async init() { @@ -143,7 +146,8 @@ export class LedgerIframeBridge } async setOptions(opts: LedgerIframeBridgeOptions): Promise { - if (opts.bridgeUrl && this.#opts.bridgeUrl !== opts.bridgeUrl) { + this.#validateConfiguration(opts); + if (this.#opts?.bridgeUrl !== opts.bridgeUrl) { this.#opts.bridgeUrl = opts.bridgeUrl; await this.destroy(); await this.init(); @@ -352,4 +356,10 @@ export class LedgerIframeBridge this.iframe.contentWindow.postMessage(postMsg, '*'); } + + #validateConfiguration(opts: LedgerIframeBridgeOptions): void { + if (typeof opts.bridgeUrl !== 'string' || opts.bridgeUrl.length === 0) { + throw new Error('bridgeURL is not a valid URL'); + } + } } diff --git a/src/ledger-keyring.test.ts b/src/ledger-keyring.test.ts index e05629cd..36a0d54d 100644 --- a/src/ledger-keyring.test.ts +++ b/src/ledger-keyring.test.ts @@ -86,12 +86,9 @@ const fakeTypeTwoTx = TransactionFactory.fromTxData( { common: commonEIP1559, freeze: false }, ); -const BRIDGE_URL = 'BRIDGE_URL'; - describe('LedgerKeyring', function () { let keyring: LedgerKeyring; let bridge: LedgerBridge; - const opts = { bridgeUrl: BRIDGE_URL }; /** * Sets up the keyring to unlock one account. * @@ -107,7 +104,7 @@ describe('LedgerKeyring', function () { } beforeEach(async function () { - bridge = new LedgerIframeBridge(opts); + bridge = new LedgerIframeBridge(); keyring = new LedgerKeyring({ bridge }); keyring.hdk = fakeHdKey; await keyring.deserialize(); @@ -133,7 +130,7 @@ describe('LedgerKeyring', function () { describe('constructor', function () { it('constructs', async function () { const ledgerKeyring = new LedgerKeyring({ - bridge: new LedgerIframeBridge(opts), + bridge: new LedgerIframeBridge(), }); expect(typeof ledgerKeyring).toBe('object'); From 69b72216bb3600ef87802b7877470b0843b3510c Mon Sep 17 00:00:00 2001 From: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Mon, 26 Feb 2024 18:21:38 +0800 Subject: [PATCH 7/7] Update ledger-bridge.ts --- src/ledger-bridge.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ledger-bridge.ts b/src/ledger-bridge.ts index fc32c319..f90af2ec 100644 --- a/src/ledger-bridge.ts +++ b/src/ledger-bridge.ts @@ -28,8 +28,7 @@ export type LedgerSignTypedDataResponse = Awaited< export type LedgerBridgeOptions = Record; -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface LedgerBridge { +export type LedgerBridge = { isDeviceConnected: boolean; init(): Promise; @@ -65,4 +64,4 @@ export interface LedgerBridge { deviceSignTypedData( params: LedgerSignTypedDataParams, ): Promise; -} +};