[composable-controller] Narrow class and messenger types by parameterizing over child controllers list

[MajorLift]

Explanation

Currently, the allow lists of ComposableControllerMessenger are set to string, which is too permissive. Each ComposableController instance needs typing and allow lists that are specific to its set of input child controllers.

To achieve this, the ComposableController class and its messenger are made polymorphic upon the controllers constructor option, which represents the list of child controllers to be composed.

References

• Closes #3627

Changelog

@metamask/composable-controller

Added

• Adds and exports new types: (#3952)
  • RestrictedControllerMessengerConstraint, which is the narrowest supertype of all controller-messenger instances.
  • LegacyControllerStateConstraint, a universal supertype for the controller state object, encompassing both BaseControllerV1 and BaseControllerV2 state.
  • ComposableControllerStateConstraint, the narrowest supertype for the composable controller state object.

Changed

• BREAKING: The ComposableController class is now a generic class that expects one generic argument ComposableControllerState (#3952).
  • BREAKING: Child controllers that extend BaseControllerV1 must have an overridden name property that is defined using the as const assertion for the ComposableController class to be typed correctly.

Checklist

• [x] I've updated the test suite for new or updated code as appropriate
• [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

Hmm, that's a lot of generic type parameters. Have you considered consolidating them into just one - the ComposableControllerState? I think everything else can be derived from that. We don't need to have access to the full controller types, just the state.

Example here: https://github.com/MetaMask/core/pull/new/example-composable-controller-reduced-type-params (I just quickly prototyped this, it's still a bit messy and totally undocumented/untested)

[MajorLift]

@Gudahtt The only user-facing generic parameter in ComposableController is ChildControllers. The other five parameters are automatically derived from ChildControllers without any further user input required, using the expressions in their generic parameter defaults.

These "optional" generic parameters are essentially internal variables of a "ComposableController" type-level function that are evaluated when the function is called with a "ChildControllers" argument.

This gives us exactly accurate types for ComposedControllerState and ComposedControllerMessenger that (as "scoped/internal variables") are accessible from within each composable-controller instance. Using types that are wider than the current generic params would be imo an unnecessary regression in terms of type safety.

I'll try to find a way in the jsdoc to make it immediately clear that ComposableController exposes only one generic parameter. I'm thinking only leaving ChildControllers in the jsdoc, and moving information about the "private" variables to in-line comments should work.

[Gudahtt]

The prototype branch I linked also has exactly accurate types. I was not suggesting using wider types, just deriving things the other way (deriving controller types from state, rather than state from controller). It seems simpler to me to have fewer generic type parameters, even if they are optional.

[Gudahtt]

Oh wait, I maybe see what you mean, the types for the controllers parameter aren't exact in the branch I shared. Just the state+messenger is. Hmm, OK. I wasn't sure that that mattered, since the wider type we used for that parameter did seem sufficient for ensuring they were handled correctly.

I asked about this because I was considering how this would fit into the wallet library idea, but I don't have a complete picture of how that would work yet. I'll think about it a bit more first.

[MajorLift]

• The controllers constructor option not being constrained shouldn't be an issue if the controllers input is used as the source of truth. Even if a generic param ChildControllers is assigned to controllers, the generic param will be inferred from controllers if left omitted.

• The biggest difference between our approaches seems to be the types deriving from ChildControllers vs. ComposableControllerState. Needing to supply the state object everywhere might make the API a bit awkward (example below).

But if the composed state type is expected to be readily available (without needing to be derived from ChildControllers or something else), I think either way should work.

const controllerMessenger = new ControllerMessenger<
        never,
        | ComposableControllerEvents<{
            FooController: { foo: 'foo' };
            QuzController: { quz: 'quz' };
          }>
        | FooControllerEvent
        | QuzControllerEvent
      >();

• One clear advantage of the prototype branch is that it lets us export controller instance-specific types. If this is important, we should definitely go with this approach.

[Gudahtt]

But if the composed state type is expected to be readily available (without needing to be derived from ChildControllers or something else), I think either way should work.

Yes, this is what I would expect. Controller state should be known ahead of time, statically. We can see this on mobile which uses TypeScript for constructing controllers, the state is declared upfront in one big object: https://github.com/MetaMask/metamask-mobile/blob/3e1fd671b936abf6175508ff8479bc3d171ef1fc/app/core/Engine.ts#L256