Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

fix: disconnect error during transport reset (iOS devices only) #231

Merged
merged 5 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ module.exports = {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 87.94,
branches: 89.28,
functions: 95.91,
lines: 91.13,
statements: 91.22,
lines: 91.32,
statements: 91.41,
},
},

Expand Down
38 changes: 34 additions & 4 deletions src/ledger-keyring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ describe('LedgerKeyring', function () {
expect(serialized.deviceId).toBe('some-device');
});

it('should migrate accountIndexes to accountDetails', async function () {
it('migrates accountIndexes to accountDetails', async function () {
const someHdPath = `m/44'/60'/0'/0/0`;
const account = fakeAccounts[1];
const checksum = ethUtil.toChecksumAddress(account);
Expand All @@ -211,7 +211,7 @@ describe('LedgerKeyring', function () {
});
});

it('should migrate non-bip44 accounts to accountDetails', async function () {
it('migrates non-bip44 accounts to accountDetails', async function () {
const someHdPath = `m/44'/60'/0'`;
const account = fakeAccounts[1];
const checksum = ethUtil.toChecksumAddress(account);
Expand All @@ -229,6 +229,28 @@ describe('LedgerKeyring', function () {
hdPath: `m/44'/60'/0'/1`,
});
});

it('throws errors when address is not found', async function () {
dawnseeker8 marked this conversation as resolved.
Show resolved Hide resolved
const hdPath = `m/44'/60'/0'`;
const account = '0x90A5b70d94418d6c25C19071e5b8170607f6302D';

let thrownError;
const accountIndexes: Record<string, number> = {};
accountIndexes['0x90a'] = 1;

try {
await keyring.deserialize({
hdPath,
accounts: [account],
deviceId: 'some-device',
implementFullBIP44: true,
accountIndexes,
});
} catch (error) {
thrownError = error;
}
expect(thrownError).toStrictEqual(new Error('Unknown address'));
});
dawnseeker8 marked this conversation as resolved.
Show resolved Hide resolved
});

describe('setDeviceId', function () {
Expand Down Expand Up @@ -440,12 +462,12 @@ describe('LedgerKeyring', function () {
});

describe('getFirstPage', function () {
it('should set the currentPage to 1', async function () {
it('sets the currentPage to 1', async function () {
await keyring.getFirstPage();
expect(keyring.page).toBe(1);
});

it('should return the list of accounts for current page', async function () {
it('returns the list of accounts for current page', async function () {
const accounts = await keyring.getFirstPage();

expect(accounts).toHaveLength(keyring.perPage);
Expand Down Expand Up @@ -978,6 +1000,14 @@ describe('LedgerKeyring', function () {
);
});

it('throws an error if the version is not provided', async function () {
await expect(
keyring.signTypedData(fakeAccounts[0], fixtureData),
).rejects.toThrow(
'Ledger: Only version 4 of typed data signing is supported',
);
});

it('throws an error if the hdPath is not found', async function () {
jest
.spyOn(keyring, 'unlockAccountByAddress')
Expand Down
1 change: 1 addition & 0 deletions src/ledger-keyring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const pathBase = 'm';
const hdPathString = `${pathBase}/44'/60'/0'`;
const keyringType = 'Ledger Hardware';

// This number make one of our failure test very slow which for loop need to run 1000 times.
dawnseeker8 marked this conversation as resolved.
Show resolved Hide resolved
const MAX_INDEX = 1000;

enum NetworkApiUrls {
Expand Down
7 changes: 7 additions & 0 deletions src/ledger-transport-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,12 @@ describe('LedgerTransportMiddleware', function () {
const app = transportMiddleware.getEthApp();
expect(app).toBeDefined();
});

it('throw error when transport not set', async function () {
dawnseeker8 marked this conversation as resolved.
Show resolved Hide resolved
transportMiddleware = new LedgerTransportMiddleware();
expect(() => transportMiddleware.getEthApp()).toThrow(
'Instance `transport` is not initialized.',
);
});
});
});
7 changes: 1 addition & 6 deletions src/ledger-transport-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ export class LedgerTransportMiddleware implements TransportMiddleware {

readonly transportEncoding = 'ascii';

#app?: MetaMaskLedgerHwAppEth;

#transport?: Transport;

/**
Expand Down Expand Up @@ -60,9 +58,6 @@ export class LedgerTransportMiddleware implements TransportMiddleware {
* @returns An generic interface for communicating with a Ledger hardware wallet to perform operation.
*/
getEthApp(): MetaMaskLedgerHwAppEth {
if (!this.#app) {
Copy link
Member

Choose a reason for hiding this comment

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

The comment above is now inaccurate: "it create a new eth app instance if not exist."

We should fix that in a follow up PR

this.#app = new MetaMaskLedgerHwAppEth(this.getTransport());
}
return this.#app;
return new MetaMaskLedgerHwAppEth(this.getTransport());
}
}
Loading