[composable-controller] Fix incorrect behavior and improve type-level…

… safeguards (#4467)

## Overview

This commit fixes issues with the `ComposableController` class's
interface, and its logic for validating V1 and V2 controllers.

These changes will enable `ComposableController` to function correctly
downstream in the Mobile Engine, and eventually the Wallet Framework
POC.

## Explanation

The previous approach of generating mock controller classes from the
`ComposableControllerState` input using the `GetChildControllers` was
flawed, because the mock controllers would always be incomplete,
complicating attempts at validation.

Instead, we now rely on the downstream consumer to provide both a
composed type of state schemas (`ComposableControllerState`) and a type
union of the child controller instances (`ChildControllers`). For
example, in mobile, we can use (with some adjustments) `EngineState` for
the former, and `Controllers[keyof Controllers]` for the latter.

The validation logic for V1 controllers has also been updated. Due to
breaking changes made to the private properties of `BaseControllerV1`
(#3959), mobile V1 controllers
relying on versions prior to these changes were introduced were
incompatible with the up-to-date `BaseControllerV1` version that the
composable-controller package references.

In this commit, the validator type `BaseControllerV1Instance` filters
out the problematic private properties by using the `PublicInterface`
type. Because the public API of `BaseControllerV1` has been relatively
constant, this removes the type errors that previously occurred in
mobile when passing V1 controllers into `ComposableController`.

## References

- Closes #4448
- Contributes to #4213
- Next steps MetaMask/metamask-mobile#10073
  - See MetaMask/metamask-mobile#10011

## Changelog

### `@metamask/composable-controller` (major)

### Changed

- **BREAKING:** Add two required generic parameters to the
`ComposableController` class: `ComposedControllerState` (constrained by
`LegacyComposableControllerStateConstraint`) and `ChildControllers`
(constrained by `ControllerInstance`)
([#4467](#4467)).
- **BREAKING:** The type guard `isBaseController` now validates that the
input has an object-type property named `metadata` in addition to its
existing checks.
- **BREAKING:** The type guard `isBaseControllerV1` now validates that
the input has object-type properties `config`, `state`, and
function-type property `subscribe`, in addition to its existing checks.
- **BREAKING:** Narrow `LegacyControllerStateConstraint` type from
`BaseState | StateConstraint` to `BaseState & object | StateConstraint`.
- Add an optional generic parameter `ControllerName` to the
`RestrictedControllerMessengerConstraint` type, which extends `string`
and defaults to `string`.
 
### Fixed

- **BREAKING:** The `ComposableController` class raises a type error if
a non-controller with no `state` property is passed into the
`ChildControllers` generic parameter or the `controllers` constructor
option.
- Previously, a runtime error was thrown at class instantiation with no
type-level enforcement.
- When the `ComposableController` class is instantiated, its messenger
now attempts to subscribe to all child controller `stateChange` events
that are included in the messenger's events allowlist.
- This was always the expected behavior, but a bug introduced in
`@metamask/[email protected]` caused `stateChange` event
subscriptions to fail.
- `isBaseController` and `isBaseControllerV1` no longer return false
negatives.
- The `instanceof` operator is no longer used to validate that the input
is a subclass of `BaseController` or `BaseControllerV1`.
- The `ChildControllerStateChangeEvents` type checks that the child
controller's state extends from the `StateConstraintV1` type instead of
from `Record<string, unknown>`.
([#4467](#4467))
- V1 controllers define their state types using the `interface` keyword,
which are incompatible with `Record<string, unknown>` by default. This
resulted in `ChildControllerStateChangeEvents` failing to generate
`stateChange` events for V1 controllers and returning `never`.

## 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

packages/composable-controller/package.json

@@ -46,6 +46,7 @@
   "devDependencies": {
     "@metamask/auto-changelog": "^3.4.4",
     "@metamask/json-rpc-engine": "^9.0.2",
+    "@metamask/utils": "^9.1.0",
     "@types/jest": "^27.4.1",
     "deepmerge": "^4.2.2",
     "immer": "^9.0.6",

packages/composable-controller/src/ComposableController.test.ts

@@ -11,8 +11,14 @@ import { JsonRpcEngine } from '@metamask/json-rpc-engine';
 import type { Patch } from 'immer';
 import * as sinon from 'sinon';
 
-import type { ComposableControllerEvents } from './ComposableController';
-import { ComposableController } from './ComposableController';
+import type {
+  ChildControllerStateChangeEvents,
+  ComposableControllerEvents,
+} from './ComposableController';
+import {
+  ComposableController,
+  INVALID_CONTROLLER_ERROR,
+} from './ComposableController';
 
 // Mock BaseController classes
 
@@ -130,6 +136,18 @@ class BarController extends BaseControllerV1<never, BarControllerState> {
 type BazControllerState = BaseState & {
   baz: string;
 };
+type BazControllerEvent = {
+  type: `BazController:stateChange`;
+  payload: [BazControllerState, Patch[]];
+};
+
+type BazMessenger = RestrictedControllerMessenger<
+  'BazController',
+  never,
+  BazControllerEvent,
+  never,
+  never
+>;
 
 class BazController extends BaseControllerV1<never, BazControllerState> {
   defaultState = {
@@ -138,12 +156,30 @@ class BazController extends BaseControllerV1<never, BazControllerState> {
 
   override name = 'BazController' as const;
 
-  constructor() {
+  protected messagingSystem: BazMessenger;
+
+  constructor({ messenger }: { messenger: BazMessenger }) {
     super();
     this.initialize();
+    this.messagingSystem = messenger;
   }
 }
 
+type ControllersMap = {
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  FooController: FooController;
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  QuzController: QuzController;
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  BarController: BarController;
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  BazController: BazController;
+};
+
 describe('ComposableController', () => {
   afterEach(() => {
     sinon.restore();
@@ -159,16 +195,30 @@ describe('ComposableController', () => {
         // eslint-disable-next-line @typescript-eslint/naming-convention
         BazController: BazControllerState;
       };
+
       const composableMessenger = new ControllerMessenger<
         never,
-        ComposableControllerEvents<ComposableControllerState>
+        | ComposableControllerEvents<ComposableControllerState>
+        | ChildControllerStateChangeEvents<ComposableControllerState>
       >().getRestricted({
         name: 'ComposableController',
         allowedActions: [],
-        allowedEvents: [],
+        allowedEvents: ['BazController:stateChange'],
       });
-      const controller = new ComposableController({
-        controllers: [new BarController(), new BazController()],
+      const controller = new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
+        controllers: [
+          new BarController(),
+          new BazController({
+            messenger: new ControllerMessenger<never, never>().getRestricted({
+              name: 'BazController',
+              allowedActions: [],
+              allowedEvents: [],
+            }),
+          }),
+        ],
         messenger: composableMessenger,
       });
 
@@ -194,7 +244,10 @@ describe('ComposableController', () => {
         allowedEvents: [],
       });
       const barController = new BarController();
-      new ComposableController({
+      new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
         controllers: [barController],
         messenger: composableMessenger,
       });
@@ -255,11 +308,13 @@ describe('ComposableController', () => {
           'QuzController:stateChange',
         ],
       });
-      const composableController =
-        new ComposableController<ComposableControllerState>({
-          controllers: [fooController, quzController],
-          messenger: composableControllerMessenger,
-        });
+      const composableController = new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
+        controllers: [fooController, quzController],
+        messenger: composableControllerMessenger,
+      });
       expect(composableController.state).toStrictEqual({
         FooController: { foo: 'foo' },
         QuzController: { quz: 'quz' },
@@ -288,7 +343,10 @@ describe('ComposableController', () => {
         allowedActions: [],
         allowedEvents: ['FooController:stateChange'],
       });
-      new ComposableController<ComposableControllerState>({
+      new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
         controllers: [fooController],
         messenger: composableControllerMessenger,
       });
@@ -336,11 +394,13 @@ describe('ComposableController', () => {
         allowedActions: [],
         allowedEvents: ['FooController:stateChange'],
       });
-      const composableController =
-        new ComposableController<ComposableControllerState>({
-          controllers: [barController, fooController],
-          messenger: composableControllerMessenger,
-        });
+      const composableController = new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
+        controllers: [barController, fooController],
+        messenger: composableControllerMessenger,
+      });
       expect(composableController.state).toStrictEqual({
         BarController: { bar: 'bar' },
         FooController: { foo: 'foo' },
@@ -373,7 +433,10 @@ describe('ComposableController', () => {
         allowedActions: [],
         allowedEvents: ['FooController:stateChange'],
       });
-      new ComposableController<ComposableControllerState>({
+      new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
         controllers: [barController, fooController],
         messenger: composableControllerMessenger,
       });
@@ -421,7 +484,10 @@ describe('ComposableController', () => {
         allowedActions: [],
         allowedEvents: ['FooController:stateChange'],
       });
-      new ComposableController<ComposableControllerState>({
+      new ComposableController<
+        ComposableControllerState,
+        ControllersMap[keyof ComposableControllerState]
+      >({
         controllers: [barController, fooController],
         messenger: composableControllerMessenger,
       });
@@ -490,14 +556,15 @@ describe('ComposableController', () => {
       });
       expect(
         () =>
-          new ComposableController({
+          new ComposableController<
+            ComposableControllerState,
+            ControllersMap[keyof ComposableControllerState]
+          >({
             // @ts-expect-error - Suppressing type error to test for runtime error handling
             controllers: [notController, fooController],
             messenger: composableControllerMessenger,
           }),
-      ).toThrow(
-        'Invalid controller: controller must extend from BaseController or BaseControllerV1',
-      );
+      ).toThrow(INVALID_CONTROLLER_ERROR);
     });
   });
 });