Fix contradictory typing of IFrameMessageResponse and refactor into…

… modular types (#207)

## Motivation

`IFrameMessageResponse` is currently defined with a generic parameter `TAction`. This parameter doesn't usefully narrow or widen the type. 
- The `action: TAction` property appears to *widen* `IFrameMessageResponse` to accept any action type, but `TAction` is also constrained as a subtype of `IFrameMessageActions`.
- The `TAction` generic param appears to *narrow* `IFrameMessageResponse` to select individual union members, but it interferes with the inference-based type narrowing we can get if `IFrameMessageResponse` is an exhaustively enumerated discriminated union. 

This causes two problems:

1. Type errors on assignment operations that should be valid.
- e.g. [`as` type casting](https://github.com/MetaMask/eth-ledger-bridge-keyring/pull/207/files#diff-c901ebd8641651c9156a86894574a6d9567f2ae79495de0edff2cbef8ae958f0L302-L304) is necessary here because supertype is being assigned to a subtype i.e. `(response: IFrameMessageResponse<TAction>) => void` to `(response: IFrameMessageResponse<IFrameMessageActions>) => void`.
  - In general, `(x: SuperType) => T` is a *subtype* of `(x: SubType) => T`.
- **This error indicates an underlying issue with the typing and shouldn't be suppressed.**
- Removing `TAction` puts `(response: SomeIFrameMessageResponse) => void` on the LHS (assignee, supertype) and `(response: IFrameMessageResponse) => void` on the RHS (assigned, subtype), resolving this logical error.

2. `TAction` is also silencing type errors we should be getting from type narrowing based on `action` value. 
- e.g. Some union members of `IFrameMessageResponse` do not include a `success`, `error`, `payload`, or `payload.error` property, but because of `TAction`, TypeScript doesn't alert us that we should be performing `in` checks on them in addition to null checks. 
- This can cause unexpected failure at runtime, especially from the destructuring assignments that are being used.

Constraining `IFrameMessageResponse['action']` to `IFrameMessageActions` both resolves these issues and guides us towards writing type-safe logic about these actions that conform to their specific type signatures. It appears that this was the original intention of writing `IFrameMessageActions` as a discriminated union instead of a wider type encompassing all possible actions.

## Changes

- **BREAKING** Narrows `IFrameMessageResponse` type definition

## Explanation

- To resolve these issues, This PR makes `IFrameMessageResponse` non-generic and removes `as` casting.
- For improved readability and maintainability, `IFrameMessageResponse` is also refactored into a union of named types.
- A `IFrameMessageResponseBase` type is defined for modularity and better visibility of `IFrameMessageResponse` types with atypical shapes.

## References

- Closes #208
- More info on function subtyping: MetaMask/core#1890 (comment)

jest.config.js

@@ -41,10 +41,10 @@ module.exports = {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 65.42,
+      branches: 68.06,
       functions: 88.57,
-      lines: 81.63,
-      statements: 81.55,
+      lines: 81.29,
+      statements: 81.21,
     },
   },
 

src/ledger-iframe-bridge.test.ts

@@ -1,6 +1,7 @@
 import { hasProperty } from '@metamask/utils';
 
 import {
+  type IFrameMessageResponse,
   IFrameMessageAction,
   LedgerIframeBridge,
 } from './ledger-iframe-bridge';
@@ -62,7 +63,7 @@ describe('LedgerIframeBridge', function () {
    */
   function stubKeyringIFramePostMessage(
     bridgeInstance: LedgerIframeBridge,
-    fn: (message: any) => void,
+    fn: (message: IFrameMessageResponse) => void,
   ) {
     if (!isIFrameValid(bridgeInstance.iframe)) {
       throw new Error('the iframe is not valid');

src/ledger-iframe-bridge.ts

@@ -22,46 +22,58 @@ export enum IFrameMessageAction {
   LedgerSignTypedData = 'ledger-sign-typed-data',
 }
 
-type IFrameMessageResponse<TAction extends IFrameMessageAction> = {
-  action: TAction;
+type IFrameMessageResponseStub<
+  SuccessResult extends Record<string, unknown>,
+  FailureResult = Error,
+> = {
   messageId: number;
 } & (
-  | {
-      action: IFrameMessageAction.LedgerConnectionChange;
-      payload: { connected: boolean };
-    }
-  | ({
-      action: IFrameMessageAction.LedgerMakeApp;
-    } & ({ success: true } | { success: false; error?: unknown }))
-  | {
-      action: IFrameMessageAction.LedgerUpdateTransport;
-      success: boolean;
-    }
-  | ({
-      action: IFrameMessageAction.LedgerUnlock;
-    } & (
-      | { success: true; payload: GetPublicKeyResponse }
-      | { success: false; payload: { error: Error } }
-    ))
-  | ({
-      action: IFrameMessageAction.LedgerSignTransaction;
-    } & (
-      | { success: true; payload: LedgerSignTransactionResponse }
-      | { success: false; payload: { error: Error } }
-    ))
-  | ({
-      action:
-        | IFrameMessageAction.LedgerSignPersonalMessage
-        | IFrameMessageAction.LedgerSignTypedData;
-    } & (
-      | {
-          success: true;
-          payload: LedgerSignMessageResponse | LedgerSignTypedDataResponse;
-        }
-      | { success: false; payload: { error: Error } }
-    ))
+  | { success: true; payload: SuccessResult }
+  | { success: false; payload: { error: FailureResult } }
 );
 
+type LedgerConnectionChangeActionResponse = {
+  messageId: number;
+  action: IFrameMessageAction.LedgerConnectionChange;
+  payload: { connected: boolean };
+};
+
+type LedgerMakeAppActionResponse = {
+  messageId: number;
+  action: IFrameMessageAction.LedgerMakeApp;
+} & ({ success: true } | { success: false; error?: unknown });
+
+type LedgerUpdateTransportActionResponse = {
+  messageId: number;
+  action: IFrameMessageAction.LedgerUpdateTransport;
+  success: boolean;
+};
+
+type LedgerUnlockActionResponse = {
+  action: IFrameMessageAction.LedgerUnlock;
+} & IFrameMessageResponseStub<GetPublicKeyResponse>;
+
+type LedgerSignTransactionActionResponse = {
+  action: IFrameMessageAction.LedgerSignTransaction;
+} & IFrameMessageResponseStub<LedgerSignTransactionResponse>;
+
+type LedgerSignPersonalMessageActionResponse = {
+  action: IFrameMessageAction.LedgerSignPersonalMessage;
+} & IFrameMessageResponseStub<LedgerSignMessageResponse>;
+
+type LedgerSignTypedDataActionResponse = {
+  action: IFrameMessageAction.LedgerSignTypedData;
+} & IFrameMessageResponseStub<LedgerSignTypedDataResponse>;
+
+export type IFrameMessageResponse =
+  | LedgerConnectionChangeActionResponse
+  | LedgerMakeAppActionResponse
+  | LedgerUpdateTransportActionResponse
+  | LedgerUnlockActionResponse
+  | LedgerSignTransactionActionResponse
+  | LedgerSignPersonalMessageActionResponse
+  | LedgerSignTypedDataActionResponse;
+
 type IFrameMessage<TAction extends IFrameMessageAction> = {
   action: TAction;
   params?: Readonly<Record<string, unknown>>;
@@ -80,17 +92,15 @@ export class LedgerIframeBridge implements LedgerBridge {
 
   eventListener?: (eventMessage: {
     origin: string;
-    data: IFrameMessageResponse<IFrameMessageAction>;
+    data: IFrameMessageResponse;
   }) => void;
 
   isDeviceConnected = false;
 
   currentMessageId = 0;
 
-  messageCallbacks: Record<
-    number,
-    (response: IFrameMessageResponse<IFrameMessageAction>) => void
-  > = {};
+  messageCallbacks: Record<number, (response: IFrameMessageResponse) => void> =
+    {};
 
   delayedPromise?: {
     resolve: (value: boolean) => void;
@@ -119,10 +129,12 @@ export class LedgerIframeBridge implements LedgerBridge {
           action: IFrameMessageAction.LedgerMakeApp,
         },
         (response) => {
-          if (response.success) {
+          if ('success' in response && response.success) {
             resolve(true);
-          } else {
+          } else if ('error' in response) {
             reject(response.error);
+          } else {
+            reject(new Error('Unknown error occurred'));
           }
         },
       );
@@ -147,8 +159,8 @@ export class LedgerIframeBridge implements LedgerBridge {
           action: IFrameMessageAction.LedgerUpdateTransport,
           params: { transportType },
         },
-        ({ success }) => {
-          if (success) {
+        (response) => {
+          if ('success' in response && response.success) {
             return resolve(true);
           }
           return reject(new Error('Ledger transport could not be updated'));
@@ -223,11 +235,16 @@ export class LedgerIframeBridge implements LedgerBridge {
           action,
           params,
         },
-        ({ success, payload }) => {
-          if (success) {
-            return resolve(payload);
+        (response) => {
+          if ('payload' in response && response.payload) {
+            if ('success' in response && response.success) {
+              return resolve(response.payload);
+            }
+            if ('error' in response.payload) {
+              return reject(response.payload.error);
+            }
           }
-          return reject(payload.error);
+          return reject(new Error('Unknown error occurred'));
         },
       );
     });
@@ -267,7 +284,7 @@ export class LedgerIframeBridge implements LedgerBridge {
     bridgeUrl: string,
     eventMessage: {
       origin: string;
-      data: IFrameMessageResponse<IFrameMessageAction>;
+      data: IFrameMessageResponse;
     },
   ) {
     if (eventMessage.origin !== this.#getOrigin(bridgeUrl)) {
@@ -289,7 +306,7 @@ export class LedgerIframeBridge implements LedgerBridge {
 
   #sendMessage<TAction extends IFrameMessageAction>(
     message: IFrameMessage<TAction>,
-    callback: (response: IFrameMessageResponse<TAction>) => void,
+    callback: (response: IFrameMessageResponse) => void,
   ) {
     this.currentMessageId += 1;
 
@@ -299,9 +316,7 @@ export class LedgerIframeBridge implements LedgerBridge {
       target: LEDGER_IFRAME_ID,
     };
 
-    this.messageCallbacks[this.currentMessageId] = callback as (
-      response: IFrameMessageResponse<IFrameMessageAction>,
-    ) => void;
+    this.messageCallbacks[this.currentMessageId] = callback;
 
     if (!this.iframeLoaded || !this.iframe || !this.iframe.contentWindow) {
       throw new Error('The iframe is not loaded yet');