From fadb1c39456d34d8a2ee3320cebcc2c9d017b8ef Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:22:20 -0400 Subject: [PATCH 1/4] feat: add paging cache for Ledger accounts --- .../keyring-eth-ledger-bridge/CHANGELOG.md | 4 + .../src/ledger-keyring.test.ts | 230 ++++++++++++++++++ .../src/ledger-keyring.ts | 81 +++++- 3 files changed, 312 insertions(+), 3 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index 4506510ca..dceaacba7 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] +### Added + +- Add a paging cache to the Ledger keyring that stores BIP-44 derived entries (`hdPath`, `address`, `publicKey`, `chainCode`) keyed by derivation path. In Ledger Live mode, `addAccounts` now reuses a cached address for an already-paged path and skips the device round-trip (`unlock(path)` → `bridge.getPublicKey`). The cache is invalidated on `forgetDevice`, `setHdPath` (when the path changes), `destroy`, and on device disconnect, and is also exposed via `invalidatePagingCache()` for external disconnect handlers ([#TODO](https://github.com/MetaMask/accounts/pull/TODO)) + ## [12.2.0] ### Added 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 7b37cb3d8..71a7793a0 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts @@ -489,6 +489,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); @@ -585,6 +592,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 172ceba3d..3b96c0835 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -56,6 +56,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 = { @@ -101,6 +115,8 @@ export class LedgerKeyring implements Keyring { readonly type: string = keyringType; + readonly #pagingCache: Map = new Map(); + page = 0; perPage = 5; @@ -136,6 +152,7 @@ export class LedgerKeyring implements Keyring { } async destroy(): Promise { + this.#invalidatePagingCache(); return this.bridge.destroy(); } @@ -221,6 +238,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; } @@ -259,15 +277,28 @@ 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[] = []; + // eslint-disable-next-line promise/always-return for (let i = from; i < to; i++) { 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 +685,7 @@ export class LedgerKeyring implements Keyring { this.paths = {}; this.accountDetails = {}; this.hdk = new HDKey(); + this.#invalidatePagingCache(); } /* PRIVATE METHODS */ @@ -681,7 +713,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 +761,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 = []; From f01b39037c647d3ea8379545fcd8ac7973607cde Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:04:25 -0400 Subject: [PATCH 2/4] fix: remove inline surpression --- packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 165a6ee13..67d6f4a0d 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -288,7 +288,6 @@ export class LedgerKeyring implements Keyring { const from = this.unlockedAccount; const to = from + amount; const newAccounts: Hex[] = []; - // eslint-disable-next-line promise/always-return for (let i = from; i < to; i++) { const path = this.#getPathForIndex(i); let address: Hex; From cbc0f570a499b286faffe1332b56022475b0587a Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:07:51 -0400 Subject: [PATCH 3/4] chore: update CHANGELOG --- packages/keyring-eth-ledger-bridge/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index c5c9a72e9..066b096c2 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/packages/keyring-eth-ledger-bridge/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add a paging cache to the Ledger keyring that stores BIP-44 derived entries (`hdPath`, `address`, `publicKey`, `chainCode`) keyed by derivation path. In Ledger Live mode, `addAccounts` now reuses a cached address for an already-paged path and skips the device round-trip (`unlock(path)` → `bridge.getPublicKey`). The cache is invalidated on `forgetDevice`, `setHdPath` (when the path changes), `destroy`, and on device disconnect, and is also exposed via `invalidatePagingCache()` for external disconnect handlers ([#TODO](https://github.com/MetaMask/accounts/pull/TODO)) +- Add a paging cache to the Ledger keyring that stores BIP-44 derived entries (`hdPath`, `address`, `publicKey`, `chainCode`) keyed by derivation path ([#613](https://github.com/MetaMask/accounts/pull/613)). ## [13.0.1] From 963e020d4f15c8f7d2ae751bd1f58f2eda4aa232 Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:23:22 -0400 Subject: [PATCH 4/4] feat: add defensive mismatch check --- .../keyring-eth-ledger-bridge/CHANGELOG.md | 4 +- .../src/v2/ledger-keyring.test.ts | 67 +++++++++++++++++++ .../src/v2/ledger-keyring.ts | 8 +++ 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index 066b096c2..2ee43ae4f 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/packages/keyring-eth-ledger-bridge/CHANGELOG.md @@ -7,9 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added +### Changed -- Add a paging cache to the Ledger keyring that stores BIP-44 derived entries (`hdPath`, `address`, `publicKey`, `chainCode`) keyed by derivation path ([#613](https://github.com/MetaMask/accounts/pull/613)). +- Reuse paging-derived accounts to avoid redundant device I/O at unlock ([#613](https://github.com/MetaMask/accounts/pull/613)) ## [13.0.1] 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];