From 2cc856a45d1916922c95b0e553c392edea905995 Mon Sep 17 00:00:00 2001 From: Stanley Yuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Tue, 27 Feb 2024 15:47:13 +0800 Subject: [PATCH] feat!: refactor to enable additional bridge (#210) - BREAKING: `LedgerIframeBridge` class constructor now takes an options object with `bridgeUrl`. - BREAKING: `LedgerBridge` `init` function now takes no parameters. - BREAKING: `LedgerBridgeKeyringOptions` no longer contain `bridgeUrl`. - BREAKING: `LedgerBridge` interface is now parameterized over its option type - Add `getOptions` and `setOptions` methods to `LedgerBridge` interface --- jest.config.js | 8 +-- src/ledger-bridge.ts | 21 ++++-- src/ledger-iframe-bridge.test.ts | 117 +++++++++++++++++++++++++++++-- src/ledger-iframe-bridge.ts | 46 ++++++++++-- src/ledger-keyring.test.ts | 17 ++--- src/ledger-keyring.ts | 18 ++--- 6 files changed, 189 insertions(+), 38 deletions(-) diff --git a/jest.config.js b/jest.config.js index 6b29cc7c..95af46b4 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.33, - functions: 88.23, - lines: 81.46, - statements: 81.38, + branches: 69.76, + functions: 88.73, + lines: 81.93, + statements: 81.84, }, }, diff --git a/src/ledger-bridge.ts b/src/ledger-bridge.ts index bcee7afe..f90af2ec 100644 --- a/src/ledger-bridge.ts +++ b/src/ledger-bridge.ts @@ -26,14 +26,27 @@ export type LedgerSignTypedDataResponse = Awaited< ReturnType >; -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface LedgerBridge { +export type LedgerBridgeOptions = Record; + +export type LedgerBridge = { isDeviceConnected: boolean; - init(bridgeUrl: string): Promise; + init(): Promise; destroy(): 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; updateTransportMethod(transportType: string): Promise; @@ -51,4 +64,4 @@ export interface LedgerBridge { deviceSignTypedData( params: LedgerSignTypedDataParams, ): Promise; -} +}; diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index c2c69dae..9c12b628 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -51,7 +51,8 @@ async function simulateIFrameLoad(iframe?: HTMLIFrameElementShim) { } const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; - +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; @@ -75,8 +76,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); }); @@ -84,13 +87,46 @@ 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(); const addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); - await bridge.init('bridgeUrl'); + await bridge.init(); expect(addEventListenerSpy).toHaveBeenCalledTimes(1); expect(bridge.iframeLoaded).toBe(false); @@ -480,4 +516,77 @@ describe('LedgerIframeBridge', function () { expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled(); }); }); + + describe('setOption', function () { + let removeEventListenerSpy: jest.SpyInstance; + let addEventListenerSpy: jest.SpyInstance; + const defaultIframeLoadedCounter = 1; + + beforeEach(async () => { + removeEventListenerSpy = jest.spyOn(global.window, 'removeEventListener'); + addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); + bridge = new LedgerIframeBridge(); + await bridge.init(); + await simulateIFrameLoad(bridge.iframe); + }); + + describe('when configurate bridge url', function () { + describe('when given bridge url is different with current', function () { + beforeEach(async () => { + await bridge.setOptions({ bridgeUrl: 'https://metamask.io' }); + }); + + it('should set bridgeUrl correctly', async function () { + expect(await bridge.getOptions()).toHaveProperty( + 'bridgeUrl', + 'https://metamask.io', + ); + }); + + 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 () { + it('should throw error', async function () { + await expect(bridge.setOptions({ bridgeUrl: '' })).rejects.toThrow( + INVALID_URL_ERROR, + ); + }); + }); + }); + }); + + describe('getOption', function () { + it('return instance options', async function () { + 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 efdb9b83..f87aca5c 100644 --- a/src/ledger-iframe-bridge.ts +++ b/src/ledger-iframe-bridge.ts @@ -85,11 +85,19 @@ type IFramePostMessage = target: typeof LEDGER_IFRAME_ID; }; -export class LedgerIframeBridge implements LedgerBridge { +export type LedgerIframeBridgeOptions = { + bridgeUrl: string; +}; + +export class LedgerIframeBridge + implements LedgerBridge +{ iframe?: HTMLIFrameElement; iframeLoaded = false; + #opts: LedgerIframeBridgeOptions; + eventListener?: (eventMessage: { origin: string; data: IFrameMessageResponse; @@ -108,10 +116,21 @@ export class LedgerIframeBridge implements LedgerBridge { transportType: string; }; - async init(bridgeUrl: string) { - this.#setupIframe(bridgeUrl); + constructor( + opts: LedgerIframeBridgeOptions = { + bridgeUrl: 'https://metamask.github.io/eth-ledger-bridge-keyring', + }, + ) { + this.#validateConfiguration(opts); + this.#opts = { + bridgeUrl: opts?.bridgeUrl, + }; + } + + async init() { + this.#setupIframe(this.#opts.bridgeUrl); - this.eventListener = this.#eventListener.bind(this, bridgeUrl); + this.eventListener = this.#eventListener.bind(this, this.#opts.bridgeUrl); window.addEventListener('message', this.eventListener); } @@ -122,6 +141,19 @@ export class LedgerIframeBridge implements LedgerBridge { } } + async getOptions(): Promise { + return this.#opts; + } + + async setOptions(opts: LedgerIframeBridgeOptions): Promise { + this.#validateConfiguration(opts); + if (this.#opts?.bridgeUrl !== opts.bridgeUrl) { + this.#opts.bridgeUrl = opts.bridgeUrl; + await this.destroy(); + await this.init(); + } + } + async attemptMakeApp(): Promise { return new Promise((resolve, reject) => { this.#sendMessage( @@ -324,4 +356,10 @@ export class LedgerIframeBridge implements LedgerBridge { 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 5c0ca773..82c18018 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', () => { @@ -85,8 +88,7 @@ const fakeTypeTwoTx = TransactionFactory.fromTxData( describe('LedgerKeyring', function () { let keyring: LedgerKeyring; - let bridge: LedgerBridge; - + let bridge: LedgerBridge; /** * Sets up the keyring to unlock one account. * @@ -140,7 +142,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 +164,6 @@ 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.hdPath).toBe(`m/44'/60'/0'`); expect(Array.isArray(output.accounts)).toBe(true); expect(output.accounts).toHaveLength(0); @@ -188,9 +188,6 @@ describe('LedgerKeyring', function () { const serialized = await keyring.serialize(); expect(serialized.accounts).toHaveLength(1); - expect(serialized.bridgeUrl).toBe( - 'https://metamask.github.io/eth-ledger-bridge-keyring', - ); expect(serialized.hdPath).toBe(someHdPath); expect(serialized.accountDetails).toStrictEqual(accountDetails); }); diff --git a/src/ledger-keyring.ts b/src/ledger-keyring.ts index 0ae4dd32..c875a854 100644 --- a/src/ledger-keyring.ts +++ b/src/ledger-keyring.ts @@ -15,14 +15,12 @@ import type OldEthJsTransaction from 'ethereumjs-tx'; import { EventEmitter } from 'events'; import HDKey from 'hdkey'; -import { LedgerBridge } from './ledger-bridge'; +import { LedgerBridge, LedgerBridgeOptions } 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 +31,7 @@ enum NetworkApiUrls { } type SignTransactionPayload = Awaited< - ReturnType + ReturnType['deviceSignTransaction']> >; export type AccountDetails = { @@ -47,7 +45,6 @@ export type LedgerBridgeKeyringOptions = { accounts: readonly string[]; accountDetails: Readonly>; accountIndexes: Readonly>; - bridgeUrl: string; implementFullBIP44: boolean; }; @@ -95,11 +92,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 +105,7 @@ export class LedgerKeyring extends EventEmitter { } async init() { - return this.bridge.init(this.bridgeUrl); + return this.bridge.init(); } async destroy() { @@ -122,16 +117,15 @@ export class LedgerKeyring extends EventEmitter { hdPath: this.hdPath, accounts: this.accounts, accountDetails: this.accountDetails, - bridgeUrl: this.bridgeUrl, implementFullBIP44: false, }; } 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.accountDetails) { this.#migrateAccountDetails(opts); }