[composable-controller] Make class and messenger generic upon child controllers

[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.

However, there's no simple way to achieve this using BaseController, as controller-messenger allow lists are defined for the controller class singleton, not controller instances.

Instead, ComposableController and ComposableControllerMessenger need to be made polymorphic upon the controllers constructor property, which represents the list of child controllers to be composed.

Requirements

• A generic parameter ControllerList (or maybe ChildControllers) needs to be added to the ComposableController class and ComposableControllerMessenger type.
• ControllerList should be constrained, but not defined by the least supertype for all base-controller instances (currently ControllerInstance).
• ~ControllerList should be used to derive the return type of flatState() at the type level.~
• ControllerList should be used to derive the state type for ComposableController instances.
• ControllerList should be used to derive the allow lists of ComposableControllerMessenger.
  • The allow lists should at minimum include the stateChange events for all child controllers that are subscribed to in the #updateChildController() method.

References

• See https://github.com/MetaMask/core/pull/3590#discussion_r1414460107

[desi]

Hey team! Please add your planning poker estimate with Zenhub @Gudahtt @kanthesha @MajorLift @mcmire @mikesposito

[MajorLift]

One approach we could take is to add the types listed in requirements as generic parameters to the ComposableController class and compute them in place:

export class ComposableController<
  ChildControllers extends ControllerInstance[],
    
  ComposedState extends ComposableControllerState = T1<ChildControllers>,
    
  ComposedStateChangeEvent = ControllerStateChangeEvent<'ComposableController', ComposedState>,
    
  ChildStateChangeEvents extends EventConstraint & { type: `${string}:stateChange` } = T2<ChildControllers>,
    
  ComposedControllerMessenger = RestrictedControllerMessenger<
    'ComposableController',
    never,
    ComposedStateChangeEvent | ChildStateChangeEvents,
    never,
    ChildStateChangeEvents['type']
  >,
> extends BaseController<
  'ComposableController',
  ComposedState,
  ComposedControllerMessenger
> {
   constructor({
    controllers,
    messenger,
  }: {
    controllers: ControllerList;
    messenger: ComposedControllerMessenger;
  }) {}
}

• Reduces the only independent variable to the list of child controllers.
• The generic constraints validate the constructor inputs.
  • Probably the most important feature to be introduced by this ticket.
• Implementing T1 and T2 would be a good starting point for tackling this ticket.

Another approach we could take is to write a factory class for ComposableControllers:

abstract class ComposableControllerFactory {
  static compose<
    ChildControllers extends ControllerInstance[],
    
    ComposedState extends ComposableControllerState = T1<ChildControllers>,
    
    ComposedStateChangeEvent = ControllerStateChangeEvent<'ComposableController', ComposedState>
    
    ChildStateChangeEvents extends EventConstraint & { type: `${string}:stateChange` } = T2<ChildControllers>,
    
    ComposedControllerMessenger = RestrictedControllerMessenger<
      'ComposableController',
      never,
      ComposedStateChangeEvent | ChildStateChangeEvents,
      never,
      ChildStateChangeEvents['type']
    >,
  >({
    controllers: ChildControllers, 
    messenger: ComposedControllerMessenger, 
  }){}
}

• Decouples "factory" and type-level logic vs. internal methods and BaseController-inherited properties of each controller instance.
• ComposableController arguably already functions as a factory that creates an arbitrary number of unique controller instances, which is distinct from the behavior of other singleton controllers. We need a new abstraction to encapsulate this distinction.
• Narrows the scope of the problem to typing each ComposableController instance only once -- upon instantiation.
  • Preserves current ComposableController API which doesn't allow users to update child controller list state using side effects (i.e. this.update()).

The API of the ComposableController class would change as follows:

• Before:

const composableControllerMessenger = controllerMessenger.getRestricted({
  name: 'ComposableController',
  allowedEvents: ['FooController:stateChange'],
});
const composableController = new ComposableController({
  controllers: [fooController],
  messenger: composableControllerMessenger,
});

• After:

const composableControllerMessenger = controllerMessenger.getRestricted({
  name: 'ComposableController',
  allowedEvents: ['FooController:stateChange', 'FooController:stateChange'],
});
const { composedController } = ComposableControllerFactory.compose<[fooController, barController]>({
  controllers: [fooController, barController], 
  messenger: composableControllerMessenger,
});

This would require the consumer to supply a controllerMessenger instance with the stateChange events of all child controllers specified as allowed events. This seems unavoidable with the current ControllerMessenger implementation, which does not support allow lists to be altered after instantiation.

• ComposableController does need to be typed according to its child controllers list, but it will still be used as a singleton. Applying the factory pattern will therefore be unnecessary.

[desi]

@MajorLift How would you feel about changing the estimate on this reflect the actual complexity? And just wondering what the next steps on this one are?