refactor: remove timeout for messages requiring user actions (#279)

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues or other links reviewers should consult to
understand this pull request better? For instance:

* Fixes #12345
* See: #67890
-->

#271 introduced timeouts for
all messages between the Ledger bridge and the iframe. Though a
20-second timeout may not be enough for the user to grab the device and
accept the request - especially if the user wants to look closely at the
signature request on the hardware device.

For this reason, the timeout is being removed for messages requiring
user intervention, while keeping it for messages that do not expect user
actions (i.e. `updateTransportMethod` and `attemptMakeApp`)

Author: mikesposito

packages/keyring-eth-ledger-bridge/jest.config.js

@@ -23,7 +23,7 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 91.83,
+      branches: 91.89,
       functions: 97.87,
       lines: 97.16,
       statements: 97.19,

packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.test.ts

@@ -433,18 +433,6 @@ describe('LedgerIframeBridge', function () {
       // eslint-disable-next-line @typescript-eslint/unbound-method
       expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled();
     });
-
-    it('throws a timeout error when the message does not receive a response', async function () {
-      const params = { hdPath: "m/44'/60'/0'/0", tx: '' };
-
-      stubKeyringIFramePostMessage(bridge, () => {
-        jest.advanceTimersByTime(20_000);
-      });
-
-      await expect(bridge.deviceSignTransaction(params)).rejects.toThrow(
-        'Ledger iframe message timeout',
-      );
-    });
   });
 
   describe('deviceSignMessage', function () {
@@ -507,18 +495,6 @@ describe('LedgerIframeBridge', function () {
       // eslint-disable-next-line @typescript-eslint/unbound-method
       expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled();
     });
-
-    it('throws a timeout error when the message does not receive a response', async function () {
-      const params = { hdPath: "m/44'/60'/0'/0", message: '' };
-
-      stubKeyringIFramePostMessage(bridge, () => {
-        jest.advanceTimersByTime(20_000);
-      });
-
-      await expect(bridge.deviceSignMessage(params)).rejects.toThrow(
-        'Ledger iframe message timeout',
-      );
-    });
   });
 
   describe('deviceSignTypedData', function () {
@@ -595,16 +571,6 @@ describe('LedgerIframeBridge', function () {
       // eslint-disable-next-line @typescript-eslint/unbound-method
       expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled();
     });
-
-    it('throws a timeout error when the message does not receive a response', async function () {
-      stubKeyringIFramePostMessage(bridge, () => {
-        jest.advanceTimersByTime(20_000);
-      });
-
-      await expect(bridge.deviceSignTypedData(params)).rejects.toThrow(
-        'Ledger iframe message timeout',
-      );
-    });
   });
 
   describe('setOption', function () {

packages/keyring-eth-ledger-bridge/src/ledger-iframe-bridge.ts

@@ -16,8 +16,6 @@ const LEDGER_IFRAME_ID = 'LEDGER-IFRAME';
 
 const IFRAME_MESSAGE_TIMEOUT = 4000;
 
-const USER_ACTION_REQUEST_TIMEOUT = 20_000;
-
 export enum IFrameMessageAction {
   LedgerIsIframeReady = 'ledger-is-iframe-ready',
   LedgerConnectionChange = 'ledger-connection-change',
@@ -158,9 +156,12 @@ export class LedgerIframeBridge
   }
 
   async attemptMakeApp(): Promise<boolean> {
-    const response = await this.#sendMessage({
-      action: IFrameMessageAction.LedgerMakeApp,
-    });
+    const response = await this.#sendMessage(
+      {
+        action: IFrameMessageAction.LedgerMakeApp,
+      },
+      { timeout: IFRAME_MESSAGE_TIMEOUT },
+    );
 
     if ('success' in response && response.success) {
       return true;
@@ -177,10 +178,13 @@ export class LedgerIframeBridge
       throw new Error('The iframe is not loaded yet');
     }
 
-    const response = await this.#sendMessage({
-      action: IFrameMessageAction.LedgerUpdateTransport,
-      params: { transportType },
-    });
+    const response = await this.#sendMessage(
+      {
+        action: IFrameMessageAction.LedgerUpdateTransport,
+        params: { transportType },
+      },
+      { timeout: IFRAME_MESSAGE_TIMEOUT },
+    );
 
     if ('success' in response && response.success) {
       return true;
@@ -192,11 +196,7 @@ export class LedgerIframeBridge
   async getPublicKey(
     params: GetPublicKeyParams,
   ): Promise<GetPublicKeyResponse> {
-    return this.#deviceActionMessage(
-      IFrameMessageAction.LedgerUnlock,
-      params,
-      USER_ACTION_REQUEST_TIMEOUT,
-    );
+    return this.#deviceActionMessage(IFrameMessageAction.LedgerUnlock, params);
   }
 
   async deviceSignTransaction(
@@ -205,7 +205,6 @@ export class LedgerIframeBridge
     return this.#deviceActionMessage(
       IFrameMessageAction.LedgerSignTransaction,
       params,
-      USER_ACTION_REQUEST_TIMEOUT,
     );
   }
 
@@ -215,7 +214,6 @@ export class LedgerIframeBridge
     return this.#deviceActionMessage(
       IFrameMessageAction.LedgerSignPersonalMessage,
       params,
-      USER_ACTION_REQUEST_TIMEOUT,
     );
   }
 
@@ -225,59 +223,42 @@ export class LedgerIframeBridge
     return this.#deviceActionMessage(
       IFrameMessageAction.LedgerSignTypedData,
       params,
-      USER_ACTION_REQUEST_TIMEOUT,
     );
   }
 
   async #deviceActionMessage(
     action: IFrameMessageAction.LedgerUnlock,
     params: GetPublicKeyParams,
-    timeout: number,
   ): Promise<GetPublicKeyResponse>;
 
   async #deviceActionMessage(
     action: IFrameMessageAction.LedgerSignTransaction,
     params: LedgerSignTransactionParams,
-    timeout: number,
   ): Promise<LedgerSignTransactionResponse>;
 
   async #deviceActionMessage(
     action: IFrameMessageAction.LedgerSignPersonalMessage,
     params: LedgerSignMessageParams,
-    timeout: number,
   ): Promise<LedgerSignMessageResponse>;
 
   async #deviceActionMessage(
     action: IFrameMessageAction.LedgerSignTypedData,
     params: LedgerSignTypedDataParams,
-    timeout: number,
   ): Promise<LedgerSignTypedDataResponse>;
 
   async #deviceActionMessage(
-    ...[action, params, timeout]:
-      | [IFrameMessageAction.LedgerUnlock, GetPublicKeyParams, number]
-      | [
-          IFrameMessageAction.LedgerSignTransaction,
-          LedgerSignTransactionParams,
-          number,
-        ]
-      | [
-          IFrameMessageAction.LedgerSignPersonalMessage,
-          LedgerSignMessageParams,
-          number,
-        ]
-      | [
-          IFrameMessageAction.LedgerSignTypedData,
-          LedgerSignTypedDataParams,
-          number,
-        ]
+    ...[action, params]:
+      | [IFrameMessageAction.LedgerUnlock, GetPublicKeyParams]
+      | [IFrameMessageAction.LedgerSignTransaction, LedgerSignTransactionParams]
+      | [IFrameMessageAction.LedgerSignPersonalMessage, LedgerSignMessageParams]
+      | [IFrameMessageAction.LedgerSignTypedData, LedgerSignTypedDataParams]
   ): Promise<
     | GetPublicKeyResponse
     | LedgerSignTransactionResponse
     | LedgerSignMessageResponse
     | LedgerSignTypedDataResponse
   > {
-    const response = await this.#sendMessage({ action, params }, timeout);
+    const response = await this.#sendMessage({ action, params });
 
     if ('payload' in response && response.payload) {
       if ('success' in response && response.success) {
@@ -338,7 +319,7 @@ export class LedgerIframeBridge
 
   async #sendMessage<TAction extends IFrameMessageAction>(
     message: IFrameMessage<TAction>,
-    timeout = IFRAME_MESSAGE_TIMEOUT,
+    { timeout }: { timeout?: number } = {},
   ): Promise<IFrameMessageResponse> {
     this.currentMessageId += 1;
 
@@ -360,14 +341,16 @@ export class LedgerIframeBridge
       throw new Error('The iframe is not loaded yet');
     }
 
-    setTimeout(() => {
-      if (this.#messageResponseHandles.has(postMsg.messageId)) {
-        this.#messageResponseHandles.delete(postMsg.messageId);
-        messageResponseHandle.reject(
-          new Error('Ledger iframe message timeout'),
-        );
-      }
-    }, timeout);
+    if (timeout) {
+      setTimeout(() => {
+        if (this.#messageResponseHandles.has(postMsg.messageId)) {
+          this.#messageResponseHandles.delete(postMsg.messageId);
+          messageResponseHandle.reject(
+            new Error('Ledger iframe message timeout'),
+          );
+        }
+      }, timeout);
+    }
 
     this.iframe.contentWindow.postMessage(postMsg, '*');