diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index 23e1cb51a..2ee43ae4f 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/packages/keyring-eth-ledger-bridge/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Reuse paging-derived accounts to avoid redundant device I/O at unlock ([#613](https://github.com/MetaMask/accounts/pull/613)) + ## [13.0.1] ### Changed 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 a06d1c042..290d883af 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts @@ -490,6 +490,13 @@ describe('LedgerKeyring', function () { it('returns the list of accounts when isLedgerLiveHdPath is true', async function () { keyring.setHdPath(`m/44'/60'/0'/0/0`); jest.spyOn(keyring, 'unlock').mockResolvedValue(fakeAccounts[0]); + jest.spyOn(bridge, 'getPublicKey').mockResolvedValue({ + publicKey: + '04197ced33b63059074b90ddecb9400c45cbc86210a20317b539b8cae84e573342149c3384ae45f27db68e75823323e97e03504b73ecbc47f5922b9b8144345e5a', + chainCode: + 'ba0fb16e01c463d1635ec36f5adeb93a838adcd1526656c55f828f1e34002a8b', + address: fakeAccounts[0], + }); const accounts = await keyring.getFirstPage(); expect(accounts).toHaveLength(keyring.perPage); @@ -586,6 +593,229 @@ describe('LedgerKeyring', function () { }); }); + describe('pagingCache', function () { + /** + * Extracts the account index from a Ledger Live BIP-44 path of the form + * `m/44'/60'/{index}'/0/0`. + * + * @param hdPath - The HD path, or undefined for the root unlock. + * @returns The account index, defaulting to 0 for the root path. + */ + function accountIndexFromPath(hdPath?: string): number { + return Number(hdPath?.split('/')[3]?.replace(/'/gu, '') ?? '0'); + } + + /** + * Builds a `getPublicKey` payload for the given Ledger Live path, deriving + * the address from the matching fake account. + * + * @param hdPath - The Ledger (m/-stripped) HD path. + * @returns A fake `getPublicKey` response payload. + */ + function fakePublicKeyPayload(hdPath: string): { + publicKey: string; + chainCode: string; + address: string; + } { + const idx = accountIndexFromPath(hdPath); + return { + publicKey: + '04197ced33b63059074b90ddecb9400c45cbc86210a20317b539b8cae84e573342149c3384ae45f27db68e75823323e97e03504b73ecbc47f5922b9b8144345e5a', + chainCode: + 'ba0fb16e01c463d1635ec36f5adeb93a838adcd1526656c55f828f1e34002a8b', + address: (fakeAccounts[idx] ?? fakeAccounts[0]) as string, + }; + } + + /** + * Populates the paging cache by paging through the first page of + * accounts in Ledger Live mode, with the device marked as connected. + * + * Both the root `unlock` and the per-path `bridge.getPublicKey` calls are + * mocked and restored before returning, leaving the keyring with a + * populated cache and no active spies. + * + * @param options - Configuration. + * @param options.connected - Whether the device is connected (default true). + * @returns The keyring with its paging cache populated for paths 0..perPage-1. + */ + async function populateCache({ + connected = true, + }: { connected?: boolean } = {}): Promise { + keyring.setHdPath(`m/44'/60'/0'/0/0`); + bridge.isDeviceConnected = connected; + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + const getPublicKeySpy = jest + .spyOn(bridge, 'getPublicKey') + .mockImplementation(async ({ hdPath }) => + fakePublicKeyPayload(hdPath), + ); + await keyring.getFirstPage(); + getPublicKeySpy.mockRestore(); + unlockSpy.mockRestore(); + } + + it('paging then addAccounts for the same path skips the second getPublicKey bridge call', async function () { + await populateCache(); + + keyring.setAccountToUnlock(0); + // Fresh spies so we can assert call counts for addAccounts only. + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + const getPublicKeySpy = jest + .spyOn(bridge, 'getPublicKey') + .mockResolvedValue({ + publicKey: '04', + chainCode: '00', + address: fakeAccounts[0], + }); + + const accounts = await keyring.addAccounts(1); + + expect(accounts).toStrictEqual([fakeAccounts[0]]); + // The cached path must not trigger a per-index unlock or bridge call. + expect(unlockSpy).not.toHaveBeenCalledWith(`m/44'/60'/0'/0/0`); + expect(getPublicKeySpy).not.toHaveBeenCalled(); + }); + + it('stores publicKey and chainCode from the bridge payload in the cache', async function () { + await populateCache(); + + // The cache holds the payload values verbatim for the paged path, so + // addAccounts reuses the cached address instead of re-deriving it. + keyring.setAccountToUnlock(0); + jest.spyOn(keyring, 'unlock').mockResolvedValue(fakeAccounts[0] as Hex); + jest.spyOn(bridge, 'getPublicKey').mockResolvedValue({ + publicKey: '04', + chainCode: '00', + address: fakeAccounts[0], + }); + + const accounts = await keyring.addAccounts(1); + + expect(accounts).toStrictEqual([fakeAccounts[0]]); + }); + + it('falls back to unlock(path) when the path is not in the cache', async function () { + keyring.setHdPath(`m/44'/60'/0'/0/0`); + bridge.isDeviceConnected = true; + jest + .spyOn(keyring, 'unlock') + .mockImplementation(async (hdPath?: string) => { + if (!hdPath) { + return fakeAccounts[0] as Hex; + } + return fakeAccounts[accountIndexFromPath(hdPath)] as Hex; + }); + + keyring.setAccountToUnlock(7); + const accounts = await keyring.addAccounts(1); + + expect(accounts).toStrictEqual([fakeAccounts[7]]); + }); + + it('forgetDevice clears the cache so the next addAccounts re-derives', async function () { + await populateCache(); + keyring.forgetDevice(); + + keyring.setHdPath(`m/44'/60'/0'/0/0`); + keyring.setAccountToUnlock(0); + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + + await keyring.addAccounts(1); + + expect(unlockSpy).toHaveBeenCalledWith(`m/44'/60'/0'/0/0`); + }); + + it('setHdPath to a different path clears the cache', async function () { + await populateCache(); + + // Changing to a different path clears the cache and resets the HDKey. + keyring.setHdPath(`m/44'/60'/0'/1/0`); + // Switch back to Ledger Live mode to exercise the cache-miss branch. + keyring.setHdPath(`m/44'/60'/0'/0/0`); + keyring.setAccountToUnlock(0); + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + + await keyring.addAccounts(1); + + expect(unlockSpy).toHaveBeenCalledWith(`m/44'/60'/0'/0/0`); + }); + + it('setHdPath to the same path does not clear the cache', async function () { + await populateCache(); + + keyring.setHdPath(`m/44'/60'/0'/0/0`); + keyring.setAccountToUnlock(0); + const getPublicKeySpy = jest + .spyOn(bridge, 'getPublicKey') + .mockResolvedValue({ + publicKey: '04', + chainCode: '00', + address: fakeAccounts[0], + }); + jest.spyOn(keyring, 'unlock').mockResolvedValue(fakeAccounts[0] as Hex); + + await keyring.addAccounts(1); + + expect(getPublicKeySpy).not.toHaveBeenCalled(); + }); + + it('destroy clears the cache so the next addAccounts re-derives', async function () { + await populateCache(); + + jest.spyOn(bridge, 'destroy').mockResolvedValue(undefined); + await keyring.destroy(); + + keyring.setAccountToUnlock(0); + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + + await keyring.addAccounts(1); + + expect(unlockSpy).toHaveBeenCalledWith(`m/44'/60'/0'/0/0`); + }); + + it('invalidates the cache when the device is disconnected during addAccounts', async function () { + await populateCache(); + + // Simulate a disconnected device. + bridge.isDeviceConnected = false; + keyring.setAccountToUnlock(0); + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + + await keyring.addAccounts(1); + + // Cache is invalidated due to disconnect, so the per-index path is + // unlocked again (mocked, so no device round-trip is required). + expect(unlockSpy).toHaveBeenCalledWith(`m/44'/60'/0'/0/0`); + }); + + it('exposes invalidatePagingCache() for external disconnect handlers', async function () { + await populateCache(); + + keyring.invalidatePagingCache(); + keyring.setAccountToUnlock(0); + const unlockSpy = jest + .spyOn(keyring, 'unlock') + .mockResolvedValue(fakeAccounts[0] as Hex); + + await keyring.addAccounts(1); + + expect(unlockSpy).toHaveBeenCalledWith(`m/44'/60'/0'/0/0`); + }); + }); + describe('attemptMakeApp', function () { it('calls the bridge attemptMakeApp method', async function () { jest.spyOn(bridge, 'attemptMakeApp').mockResolvedValue(true); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index d66b8826e..67d6f4a0d 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -58,6 +58,20 @@ export type AccountPageEntry = { index: number; }; +/** + * A cached BIP-44 derived account entry, keyed by derivation path. + * + * Populated by `#getAccountsBIP44` so that subsequent `addAccounts` calls in + * Ledger Live mode can skip the device round-trip (`unlock` → + * `bridge.getPublicKey`) for paths that have already been paged. + */ +type PagingCacheEntry = { + hdPath: string; + address: Hex; + publicKey: string; + chainCode?: string; +}; + export type AccountPage = AccountPageEntry[]; export type AccountDetails = { @@ -103,6 +117,8 @@ export class LedgerKeyring implements Keyring { readonly type: string = keyringType; + readonly #pagingCache: Map = new Map(); + page = 0; perPage = 5; @@ -138,6 +154,7 @@ export class LedgerKeyring implements Keyring { } async destroy(): Promise { + this.#invalidatePagingCache(); return this.bridge.destroy(); } @@ -223,6 +240,7 @@ export class LedgerKeyring implements Keyring { // Reset HDKey if the path changes if (this.hdPath !== hdPath) { this.hdk = new HDKey(); + this.#invalidatePagingCache(); } this.hdPath = hdPath; } @@ -261,7 +279,12 @@ export class LedgerKeyring implements Keyring { async addAccounts(amount: number): Promise { return new Promise((resolve, reject) => { this.unlock() - .then(async (_) => { + .then(async () => { + // If the device disconnected since the paging cache was populated, + // the cached entries may no longer be trustworthy; drop them. + if (!this.isConnected()) { + this.#invalidatePagingCache(); + } const from = this.unlockedAccount; const to = from + amount; const newAccounts: Hex[] = []; @@ -269,7 +292,14 @@ export class LedgerKeyring implements Keyring { const path = this.#getPathForIndex(i); let address: Hex; if (this.#isLedgerLiveHdPath()) { - address = await this.unlock(path); + const cached = this.#pagingCache.get(path); + if (cached) { + // Reuse the previously paged address and skip the device + // round-trip (unlock → bridge.getPublicKey). + address = cached.address; + } else { + address = await this.unlock(path); + } } else { address = this.#addressFromIndex(pathBase, i); } @@ -654,6 +684,7 @@ export class LedgerKeyring implements Keyring { this.paths = {}; this.accountDetails = {}; this.hdk = new HDKey(); + this.#invalidatePagingCache(); } /* PRIVATE METHODS */ @@ -681,7 +712,34 @@ export class LedgerKeyring implements Keyring { for (let i = from; i < to; i++) { const path = this.#getPathForIndex(i); - const address = await this.unlock(path); + // Call the bridge directly (rather than via `unlock`) so we can capture + // the full payload (publicKey/chainCode) for the paging cache. + let payload; + try { + payload = await this.bridge.getPublicKey({ + hdPath: this.#toLedgerPath(path), + }); + } catch (error: unknown) { + handleLedgerTransportError( + error, + 'Ledger: Unknown error while unlocking account', + ); + } + + const address = add0x(payload.address); + if (payload.chainCode) { + this.hdk.publicKey = Buffer.from(payload.publicKey, 'hex'); + this.hdk.chainCode = Buffer.from(payload.chainCode, 'hex'); + } + + // Stash the derived entry so subsequent addAccounts calls for this + // path can skip the device round-trip. + this.#pagingCache.set(path, { + hdPath: path, + address, + publicKey: payload.publicKey, + chainCode: payload.chainCode, + }); const valid = this.implementFullBIP44 ? await this.#hasPreviousTransactions(address) : true; @@ -702,6 +760,22 @@ export class LedgerKeyring implements Keyring { return accounts; } + /** + * Clears the BIP-44 paging cache. + * + * This is called internally when the HD path changes, the device is + * forgotten, the keyring is destroyed, or the device disconnects. It is + * also exposed publicly so that consumers can invalidate the cache in + * response to a device disconnect event. + */ + invalidatePagingCache(): void { + this.#invalidatePagingCache(); + } + + #invalidatePagingCache(): void { + this.#pagingCache.clear(); + } + #getAccountsLegacy(from: number, to: number): AccountPage { const accounts: AccountPage = []; diff --git a/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.test.ts b/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.test.ts index ee663d435..ac5a60ca8 100644 --- a/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.test.ts @@ -8,6 +8,8 @@ import { import type { KeyringAccount } from '@metamask/keyring-api'; import { KeyringType } from '@metamask/keyring-api/v2'; import { EthKeyringMethod } from '@metamask/keyring-sdk/v2'; +import { add0x, getChecksumAddress } from '@metamask/utils'; +import type { Hex } from '@metamask/utils'; import HDKey from 'hdkey'; import type { LedgerBridge, LedgerBridgeOptions } from '../ledger-bridge'; @@ -712,6 +714,71 @@ describe('LedgerKeyring', () => { const accounts = await wrapper.getAccounts(); expect(accounts).toHaveLength(2); }); + + it('returns the cached address for a paged Ledger Live index with no device I/O', async () => { + const bridge = getMockBridge(); + bridge.isDeviceConnected = true; + const inner = new LegacyLedgerKeyring({ bridge }); + inner.setHdPath(`m/44'/60'/0'/0/0`); + // Pre-seed the root HDKey so the root `unlock()` is a no-op. + inner.hdk = fakeHdKey; + + // Populate the paging cache by paging the first page in Ledger Live mode. + const populateSpy = jest + .spyOn(bridge, 'getPublicKey') + .mockImplementation(async ({ hdPath }) => { + // `#toLedgerPath` strips the leading `m/` before calling the bridge, + // so the path arrives as `44'/60'/{index}'/0/0`. + const match = hdPath?.match(/^44'\/60'\/(\d+)'\/0\/0$/u); + const idx = match?.[1] ? parseInt(match[1], 10) : 0; + return { + publicKey: + '04197ced33b63059074b90ddecb9400c45cbc86210a20317b539b8cae84e573342149c3384ae45f27db68e75823323e97e03504b73ecbc47f5922b9b8144345e5a', + chainCode: + 'ba0fb16e01c463d1635ec36f5adeb93a838adcd1526656c55f828f1e34002a8b', + address: (EXPECTED_ACCOUNTS[idx] ?? EXPECTED_ACCOUNTS[0]) as string, + }; + }); + await inner.getFirstPage(); + populateSpy.mockRestore(); + + // Fresh spy: createAccounts for an already-paged path must NOT hit the device. + const deviceSpy = jest.spyOn(bridge, 'getPublicKey'); + + const wrapper = new LedgerKeyring({ + legacyKeyring: inner, + entropySource, + }); + const newAccounts = await wrapper.createAccounts( + derivePathOptions(`m/44'/60'/2'/0/0`), + ); + const account = getFirstAccount(newAccounts); + + expect(account.address).toBe(EXPECTED_ACCOUNTS[2]); + expect(account.options.entropy.derivationPath).toBe(`m/44'/60'/2'/0/0`); + expect(deviceSpy).not.toHaveBeenCalled(); + deviceSpy.mockRestore(); + }); + + it('throws Account derivation mismatch when the returned address is registered under a different path', async () => { + const { wrapper, inner } = createEmptyWrapper(); + + // Pre-register an address under a different path than the one we will request. + const staleAddress = EXPECTED_ACCOUNTS[0] as Hex; + const checksummed = getChecksumAddress(add0x(staleAddress)); + inner.accountDetails[checksummed] = { + bip44: false, + hdPath: `m/44'/60'/0'/9`, + }; + + // Mock addAccounts to return the stale address without updating accountDetails, + // simulating a stale cache returning an address bound to the wrong path. + jest.spyOn(inner, 'addAccounts').mockResolvedValue([staleAddress]); + + await expect( + wrapper.createAccounts(derivePathOptions(`m/44'/60'/0'/0/5`)), + ).rejects.toThrow('Account derivation mismatch'); + }); }); describe('deleteAccount', () => { diff --git a/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.ts index d0e13ab95..a86c2178c 100644 --- a/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.ts @@ -336,6 +336,14 @@ export class LedgerKeyring throw new Error('Failed to create new account'); } + // Verify the returned address was registered under the requested derivation path. + // A stale paging cache could otherwise silently register the wrong address for the requested path. + const checksummedNewAddress = this.#getChecksumHexAddress(newAddress); + const details = this.inner.accountDetails[checksummedNewAddress]; + if (details && details.hdPath !== derivationPath) { + throw new Error('Account derivation mismatch'); + } + const newAccount = this.#createKeyringAccount(newAddress, targetIndex); return [newAccount];