core icon indicating copy to clipboard operation
core copied to clipboard

[composable-controller] Implement type validation of constructor options

Open MajorLift opened this issue 10 months ago • 4 comments

Requirements

When initializing a ComposableController class, given a ComposableControllerState type:

  1. A type error should be raised if the list of child controllers specified in the controllers array constructor option does not exactly match the list of controllers included in the ComposableControllerState type.

  2. A type error should be raised if the controller-messenger instance passed into the messenger constructor option has an event allowlist that does not include all of the stateChange events for the list of controllers in the ComposableControllerState type.

References

  • Follows from https://github.com/MetaMask/core/issues/3627

MajorLift avatar Apr 25 '24 15:04 MajorLift

@MajorLift Can you update the ticket to include the solutions you have tried for number 2 above and the roadblocks you hit so that others have that context?

desi avatar Apr 25 '24 15:04 desi

Seems like we probably need some spikes to figure out how to solve or approach. So waiting to estimate until we have a chance to do that.

desi avatar Apr 25 '24 15:04 desi

  • controllers:

    • Validating with a union type consisting of the child controllers allows controllers lists that are incomplete.
    • Validating with a tuple type consisting of the child controllers forces us to pass in a controllers list that matches the order of the tuple type, not just the contents.
      • Because the tuple type is likely to be derived from a union type, its order cannot necessarily be determined based on the order of controllers presented in the ComposableControllerState type.
  • messenger:

    • Given an event allowlist T, and U s.t. U <: T, TypeScript does not recognize that RestrictedControllerMessenger<..., U> <: RestrictedControllerMessenger<..., T>.
    • Wrap class constructor into a factory function that takes the event allowlist as a parameter, and validate that parameter?

MajorLift avatar May 01 '24 15:05 MajorLift

Following up on the last bullet point in the above comment, the wallet framework proposal also suggests refactoring the ComposableController into a compose function that returns a class instance that composes the input child controllers.

This should make it easier to validate the input params of the compose function, as we can let TypeScript use the input arguments as sources of inference for generic params, and then apply generic constraints.

Here's an example of this concept in action.

MajorLift avatar May 31 '24 15:05 MajorLift

Priority analysis:

  • If ComposableController is only used internally: Nice-to-have.
  • If ComposableController is included in the Wallet Framework: Should at least aim for partial improvements.

MajorLift avatar Sep 11 '24 15:09 MajorLift