core icon indicating copy to clipboard operation
core copied to clipboard

[composable-controller] Non-controllers with empty state should be rejected or excluded from composed state

Open MajorLift opened this issue 1 year ago • 0 comments

Explanation

Some of our classes that were previously labeled as controllers have empty state, making them non-controllers.

As part of Wallet Framework team's Q2 2024 OKRs (O3KR1), we're currently in the process of upgrading these non-controllers to use the messenger pattern without inheriting from BaseController.

As downstream usage of the ComposableController includes passing in these non-controllers as child controllers, we should define a consistent behavior for handling non-controller input.

  • a) We could simply reject non-controllers both at the type-level and at runtime, or
  • b) we could accept non-controllers as child controllers but handle them so that the ComposableController instance doesn't subscribe to their non-existent stateChange events (i.e. The following message should not be logged to console for non-controllers: "Error: Event missing from allow list: ExampleController:stateChange"), and
    • b-1) exclude the non-controllers from the composed state object entirely, or
    • b-2) include the non-controllers as properties in the composed state object, with empty objects assigned to the names of the non-controllers.

Proof of concept

  • https://github.com/MetaMask/metamask-mobile/pull/10374/commits/a9204a54069c439853b525539541506dc4755111
    • https://github.com/MetaMask/metamask-mobile/pull/10374#discussion_r1700941985
  • https://github.com/MetaMask/metamask-mobile/pull/10374/commits/6ba9da2278532539ee2005cd55696d7f0f1947e8
    • https://github.com/MetaMask/metamask-mobile/pull/10374#discussion_r1701131210

The following logic could be moved into the ComposableController constructor:

    new ComposableController<
      Exclude<EngineState, keyof NonControllers>,
      Controllers[Exclude<keyof Controllers, keyof NonControllers>]
    >({
      controllers: controllers.filter(
        (
          controller,
        ): controller is Controllers[Exclude<
          keyof Controllers,
          keyof NonControllers
        >] =>
          'state' in controller &&
          controller.state !== undefined &&
          Object.keys(controller.state).length > 0,
      ),
      messenger: ...
    });

References

  • See https://github.com/MetaMask/core/issues/4432

MajorLift avatar Jun 21 '24 15:06 MajorLift