From c522a4d97b14dd8568373e85d7cb91db66def5b3 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 20 Aug 2024 13:04:20 -0400 Subject: [PATCH] [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` (https://github.com/MetaMask/core/pull/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 https://github.com/MetaMask/core/issues/4448 - Contributes to https://github.com/MetaMask/core/issues/4213 - Next steps https://github.com/MetaMask/metamask-mobile/issues/10073 - See https://github.com/MetaMask/metamask-mobile/pull/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](https://github.com/MetaMask/core/pull/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/composable-controller@6.0.0` 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`. ([#4467](https://github.com/MetaMask/core/pull/4467)) - V1 controllers define their state types using the `interface` keyword, which are incompatible with `Record` 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 | 1 + .../src/ComposableController.test.ts | 117 ++++++++--- .../src/ComposableController.ts | 189 ++++++++++++------ yarn.lock | 1 + 4 files changed, 220 insertions(+), 88 deletions(-) diff --git a/packages/composable-controller/package.json b/packages/composable-controller/package.json index e69e0b4e8b5..2a4cb20d413 100644 --- a/packages/composable-controller/package.json +++ b/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", diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index 1967d59f687..e8765590821 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/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 { 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 { defaultState = { @@ -138,12 +156,30 @@ class BazController extends BaseControllerV1 { 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 + | ComposableControllerEvents + | ChildControllerStateChangeEvents >().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().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({ - 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({ + new ComposableController< + ComposableControllerState, + ControllersMap[keyof ComposableControllerState] + >({ controllers: [fooController], messenger: composableControllerMessenger, }); @@ -336,11 +394,13 @@ describe('ComposableController', () => { allowedActions: [], allowedEvents: ['FooController:stateChange'], }); - const composableController = - new ComposableController({ - 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({ + new ComposableController< + ComposableControllerState, + ControllersMap[keyof ComposableControllerState] + >({ controllers: [barController, fooController], messenger: composableControllerMessenger, }); @@ -421,7 +484,10 @@ describe('ComposableController', () => { allowedActions: [], allowedEvents: ['FooController:stateChange'], }); - new ComposableController({ + 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); }); }); }); diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index e710efa32c4..684dffdda9b 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -1,7 +1,7 @@ -import { BaseController, BaseControllerV1 } from '@metamask/base-controller'; import type { ActionConstraint, BaseConfig, + BaseControllerV1, BaseState, EventConstraint, RestrictedControllerMessenger, @@ -9,10 +9,25 @@ import type { StateMetadata, ControllerStateChangeEvent, } from '@metamask/base-controller'; +import { BaseController } from '@metamask/base-controller'; +import type { Json, PublicInterface } from '@metamask/utils'; import type { Patch } from 'immer'; export const controllerName = 'ComposableController'; +export const INVALID_CONTROLLER_ERROR = + 'Invalid controller: controller must have a `messagingSystem` or be a class inheriting from `BaseControllerV1`.'; + +/** + * A universal supertype for the `BaseControllerV1` state object. + */ +type ConfigConstraintV1 = BaseConfig & object; + +/** + * A universal supertype for the `BaseControllerV1` state object. + */ +type StateConstraintV1 = BaseState & object; + /** * A universal subtype of all controller instances that extend from `BaseControllerV1`. * Any `BaseControllerV1` instance can be assigned to this type. @@ -20,10 +35,26 @@ export const controllerName = 'ComposableController'; * Note that this type is not the widest subtype or narrowest supertype of all `BaseControllerV1` instances. * This type is therefore unsuitable for general use as a type constraint, and is only intended for use within the ComposableController. */ -export type BaseControllerV1Instance = - // `any` is used so that all `BaseControllerV1` instances are assignable to this type. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - BaseControllerV1; +type BaseControllerV1Instance = PublicInterface< + BaseControllerV1 +>; + +/** + * A universal supertype of functions that accept a piece of controller state and return some derivation of that state. + */ +type StateDeriverConstraint = (value: never) => Json; + +/** + * A universal supertype of metadata objects for individual state properties. + */ +type StatePropertyMetadataConstraint = { + [P in 'anonymous' | 'persist']: boolean | StateDeriverConstraint; +}; + +/** + * A universal supertype of state metadata objects. + */ +type StateMetadataConstraint = Record; /** * A universal subtype of all controller instances that extend from `BaseController` (formerly `BaseControllerV2`). @@ -34,9 +65,17 @@ export type BaseControllerV1Instance = * * For this reason, we only look for `BaseController` properties that we use in the ComposableController (name and state). */ -export type BaseControllerInstance = { - name: string; - state: StateConstraint; +type BaseControllerInstance = Omit< + PublicInterface< + BaseController< + string, + StateConstraint, + RestrictedControllerMessengerConstraint + > + >, + 'metadata' +> & { + metadata: StateMetadataConstraint; }; /** @@ -46,21 +85,23 @@ export type BaseControllerInstance = { * Note that this type is not the widest subtype or narrowest supertype of all `BaseController` and `BaseControllerV1` instances. * This type is therefore unsuitable for general use as a type constraint, and is only intended for use within the ComposableController. */ -export type ControllerInstance = - | BaseControllerV1Instance - | BaseControllerInstance; +type ControllerInstance = BaseControllerV1Instance | BaseControllerInstance; /** * The narrowest supertype of all `RestrictedControllerMessenger` instances. + * + * @template ControllerName - Name of the controller. + * Optionally can be used to narrow the type to a specific controller. */ -export type RestrictedControllerMessengerConstraint = - RestrictedControllerMessenger< - string, - ActionConstraint, - EventConstraint, - string, - string - >; +export type RestrictedControllerMessengerConstraint< + ControllerName extends string = string, +> = RestrictedControllerMessenger< + ControllerName, + ActionConstraint, + EventConstraint, + string, + string +>; /** * Determines if the given controller is an instance of `BaseControllerV1` @@ -69,20 +110,22 @@ export type RestrictedControllerMessengerConstraint = */ export function isBaseControllerV1( controller: ControllerInstance, -): controller is BaseControllerV1< - BaseConfig & Record, - BaseState & Record -> { +): controller is BaseControllerV1Instance { return ( 'name' in controller && typeof controller.name === 'string' && + 'config' in controller && + typeof controller.config === 'object' && 'defaultConfig' in controller && typeof controller.defaultConfig === 'object' && + 'state' in controller && + typeof controller.state === 'object' && 'defaultState' in controller && typeof controller.defaultState === 'object' && 'disabled' in controller && typeof controller.disabled === 'boolean' && - controller instanceof BaseControllerV1 + 'subscribe' in controller && + typeof controller.subscribe === 'function' ); } @@ -93,24 +136,23 @@ export function isBaseControllerV1( */ export function isBaseController( controller: ControllerInstance, -): controller is BaseController< - string, - StateConstraint, - RestrictedControllerMessengerConstraint -> { +): controller is BaseControllerInstance { return ( 'name' in controller && typeof controller.name === 'string' && 'state' in controller && typeof controller.state === 'object' && - controller instanceof BaseController + 'metadata' in controller && + typeof controller.metadata === 'object' ); } /** * A universal supertype for the controller state object, encompassing both `BaseControllerV1` and `BaseControllerV2` state. */ -export type LegacyControllerStateConstraint = BaseState | StateConstraint; +export type LegacyControllerStateConstraint = + | StateConstraintV1 + | StateConstraint; /** * A universal supertype for the composable controller state object. @@ -136,17 +178,22 @@ export type ComposableControllerStateConstraint = { }; /** - * A controller state change event for any controller instance that extends from either `BaseControllerV1` or `BaseControllerV2`. + * A `stateChange` event for any controller instance that extends from either `BaseControllerV1` or `BaseControllerV2`. */ // TODO: Replace all instances with `ControllerStateChangeEvent` once `BaseControllerV2` migrations are completed for all controllers. type LegacyControllerStateChangeEvent< ControllerName extends string, - ControllerState extends LegacyControllerStateConstraint, + ControllerState extends StateConstraintV1, > = { type: `${ControllerName}:stateChange`; payload: [ControllerState, Patch[]]; }; +/** + * The `stateChange` event type for the {@link ComposableControllerMessenger}. + * + * @template ComposableControllerState - A type object that maps controller names to their state types. + */ export type ComposableControllerStateChangeEvent< ComposableControllerState extends ComposableControllerStateConstraint, > = LegacyControllerStateChangeEvent< @@ -154,11 +201,24 @@ export type ComposableControllerStateChangeEvent< ComposableControllerState >; +/** + * A union type of internal event types available to the {@link ComposableControllerMessenger}. + * + * @template ComposableControllerState - A type object that maps controller names to their state types. + */ export type ComposableControllerEvents< ComposableControllerState extends ComposableControllerStateConstraint, > = ComposableControllerStateChangeEvent; -type ChildControllerStateChangeEvents< +/** + * A utility type that extracts controllers from the {@link ComposableControllerState} type, + * and derives a union type of all of their corresponding `stateChange` events. + * + * This type can handle both `BaseController` and `BaseControllerV1` controller instances. + * + * @template ComposableControllerState - A type object that maps controller names to their state types. + */ +export type ChildControllerStateChangeEvents< ComposableControllerState extends ComposableControllerStateConstraint, > = ComposableControllerState extends Record< infer ControllerName extends string, @@ -166,15 +226,26 @@ type ChildControllerStateChangeEvents< > ? ControllerState extends StateConstraint ? ControllerStateChangeEvent - : ControllerState extends Record + : // TODO: Remove this conditional branch once `BaseControllerV2` migrations are completed for all controllers. + ControllerState extends StateConstraintV1 ? LegacyControllerStateChangeEvent : never : never; +/** + * A union type of external event types available to the {@link ComposableControllerMessenger}. + * + * @template ComposableControllerState - A type object that maps controller names to their state types. + */ type AllowedEvents< ComposableControllerState extends ComposableControllerStateConstraint, > = ChildControllerStateChangeEvents; +/** + * The messenger of the {@link ComposableController}. + * + * @template ComposableControllerState - A type object that maps controller names to their state types. + */ export type ComposableControllerMessenger< ComposableControllerState extends ComposableControllerStateConstraint, > = RestrictedControllerMessenger< @@ -186,25 +257,15 @@ export type ComposableControllerMessenger< AllowedEvents['type'] >; -type GetChildControllers< - ComposableControllerState, - ControllerName extends keyof ComposableControllerState = keyof ComposableControllerState, -> = ControllerName extends string - ? ComposableControllerState[ControllerName] extends StateConstraint - ? { name: ControllerName; state: ComposableControllerState[ControllerName] } - : BaseControllerV1< - BaseConfig & Record, - BaseState & ComposableControllerState[ControllerName] - > - : never; - /** - * Controller that can be used to compose multiple controllers together. - * @template ChildControllerState - The composed state of the child controllers that are being used to instantiate the composable controller. + * Controller that composes multiple child controllers and maintains up-to-date composed state. + * + * @template ComposableControllerState - A type object containing the names and state types of the child controllers. + * @template ChildControllers - A union type of the child controllers being used to instantiate the {@link ComposableController}. */ export class ComposableController< ComposableControllerState extends LegacyComposableControllerStateConstraint, - ChildControllers extends ControllerInstance = GetChildControllers, + ChildControllers extends ControllerInstance, > extends BaseController< typeof controllerName, ComposableControllerState, @@ -256,32 +317,34 @@ export class ComposableController< /** * Constructor helper that subscribes to child controller state changes. + * * @param controller - Controller instance to update */ #updateChildController(controller: ControllerInstance): void { + const { name } = controller; if (!isBaseController(controller) && !isBaseControllerV1(controller)) { - throw new Error( - 'Invalid controller: controller must extend from BaseController or BaseControllerV1', - ); + // False negative. `name` is a string type. + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + throw new Error(`${name} - ${INVALID_CONTROLLER_ERROR}`); } - - const { name } = controller; - if ( - (isBaseControllerV1(controller) && 'messagingSystem' in controller) || - isBaseController(controller) - ) { + try { this.messagingSystem.subscribe( - // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // False negative. `name` is a string type. // eslint-disable-next-line @typescript-eslint/restrict-template-expressions `${name}:stateChange`, - (childState: Record) => { + (childState: LegacyControllerStateConstraint) => { this.update((state) => { Object.assign(state, { [name]: childState }); }); }, ); - } else if (isBaseControllerV1(controller)) { - controller.subscribe((childState) => { + } catch (error: unknown) { + // False negative. `name` is a string type. + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + console.error(`${name} - ${String(error)}`); + } + if (isBaseControllerV1(controller)) { + controller.subscribe((childState: StateConstraintV1) => { this.update((state) => { Object.assign(state, { [name]: childState }); }); diff --git a/yarn.lock b/yarn.lock index 15eac553fc3..7352add7818 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2487,6 +2487,7 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^6.0.2" "@metamask/json-rpc-engine": "npm:^9.0.2" + "@metamask/utils": "npm:^9.1.0" "@types/jest": "npm:^27.4.1" deepmerge: "npm:^4.2.2" immer: "npm:^9.0.6"