MetaMask / eth-ledger-bridge-keyring

A wrapper around LedgerJS libraries, to support the KeyringController protocol used by MetaMask
ISC License
80 stars 93 forks source link

Fix typing of `IFrameMessageResponse` and refactor into modular types #208

Closed MajorLift closed 11 months ago

MajorLift commented 12 months ago

IFrameMessageResponse is currently defined with a generic parameter TAction, which is constrained as a subtype of IFrameMessageActions. This parameter doesn't usefully narrow or widen the type, and causes two problems:

  1. Type errors on assignment operations that should be valid.

    • e.g. as type casting: failing because it's trying to assign a supertype to a subtype i.e. (response: IFrameMessageResponse<TAction>) => void to (response: IFrameMessageResponse<IFrameMessageActions>) => void.
    • In general, (x: SuperType) => void is a subtype of (x: SubType) => void.
    • This error indicates an underlying issue with the typing and shouldn't be silenced.
    • Removing TAction puts (response: SomeSpecificIFrameMessageResponse) => void on the LHS (assignee, supertype) and (response: IFrameMessageResponse) => void on the RHS (assigned, subtype), resolving this logical error.
  2. TAction also interferes with type narrowing based on action value, and is silencing type errors. Some union members of IFrameMessageResponse do not include a success, error, payload, or payload.error property, but because of the action: TAction property, TypeScript doesn't alert us that we should be performing in checks on them in addition to null checks. This can cause some of the existing destructuring expressions to unexpectedly fail at runtime.

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 of the actions.

Tasks