feat: reuse paging-derived accounts to avoid redundant device I/O at unlock

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

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<void> {
+        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);

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<string, PagingCacheEntry> = new Map();
+
   page = 0;
 
   perPage = 5;
@@ -138,6 +154,7 @@ export class LedgerKeyring implements Keyring {
   }
 
   async destroy(): Promise<void> {
+    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,15 +279,27 @@ export class LedgerKeyring implements Keyring {
   async addAccounts(amount: number): Promise<Hex[]> {
     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[] = [];
           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 +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 = [];