Skip to content

Commit

Permalink
fix: resolve PR comment
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@stanleyyconsensys
stanleyyconsensys committed Dec 5, 2023
1 parent 045a4f4 commit c689469
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 107 deletions.
19 changes: 9 additions & 10 deletions src/ledger-bridge.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,7 @@ export type LedgerSignTypedDataResponse = Awaited<
ReturnType<LedgerHwAppEth['signEIP712HashedMessage']>
>;

export type LedgerBridgeOptions = Record<string, string | number | object>;

export type LedgerBridgeSerializeData = Record<
string,
string | number | object
>;
export type LedgerBridgeOptions = Record<string, unknown>;

// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export interface LedgerBridge<T extends LedgerBridgeOptions> {
Expand All @@ -41,12 +36,16 @@ export interface LedgerBridge<T extends LedgerBridgeOptions> {

destroy(): Promise<void>;

serializeData(): Promise<LedgerBridgeSerializeData>;

deserializeData(opts: LedgerBridgeSerializeData): Promise<void>;

/**
* Method to get the current configuration of the ledger bridge keyring.
*/
getOptions(): Promise<T>;

/**
* Method to set the current configuration of the ledger bridge keyring.
*
* @param opts - An object contains configuration of the bridge.
*/
setOptions(opts: T): Promise<void>;

attemptMakeApp(): Promise<boolean>;
Expand Down
96 changes: 62 additions & 34 deletions src/ledger-iframe-bridge.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
29 changes: 9 additions & 20 deletions src/ledger-iframe-bridge.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
LedgerSignTransactionResponse,
LedgerSignTypedDataParams,
LedgerSignTypedDataResponse,
LedgerBridgeSerializeData,
} from './ledger-bridge';

const LEDGER_IFRAME_ID = 'LEDGER-IFRAME';
Expand Down Expand Up @@ -99,7 +98,7 @@ export class LedgerIframeBridge

iframeLoaded = false;

bridgeUrl = '';
#opts: LedgerIframeBridgeOptions = { bridgeUrl: BRIDGE_URL };

eventListener?: (eventMessage: {
origin: string;
Expand All @@ -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);
}
Expand All @@ -137,25 +138,13 @@ export class LedgerIframeBridge
}
}

async deserializeData(
serializeData: LedgerBridgeSerializeData,
): Promise<void> {
this.bridgeUrl = (serializeData.bridgeUrl as string) ?? this.bridgeUrl;
}

async serializeData(): Promise<LedgerBridgeSerializeData> {
return {
bridgeUrl: this.bridgeUrl,
};
}

async getOptions(): Promise<LedgerIframeBridgeOptions> {
return { bridgeUrl: this.bridgeUrl };
return this.#opts;
}

async setOptions(opts: LedgerIframeBridgeOptions): Promise<void> {
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();
}
Expand Down
26 changes: 0 additions & 26 deletions src/ledger-keyring.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<string, AccountDetails> = {};
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);
});
Expand Down
18 changes: 1 addition & 17 deletions src/ledger-keyring.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'`;
Expand Down Expand Up @@ -50,8 +46,6 @@ export type LedgerBridgeKeyringOptions = {
accountDetails: Readonly<Record<string, AccountDetails>>;
accountIndexes: Readonly<Record<string, number>>;
implementFullBIP44: boolean;
bridgeUrl?: string;
bridgeOptions?: LedgerBridgeSerializeData;
};

/**
Expand Down Expand Up @@ -124,7 +118,6 @@ export class LedgerKeyring extends EventEmitter {
accounts: this.accounts,
accountDetails: this.accountDetails,
implementFullBIP44: false,
bridgeOptions: await this.bridge.serializeData(),
};
}

Expand All @@ -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);
}
Expand Down

0 comments on commit c689469

Please sign in to comment.