[base-controller] Fix all any usage, apply universal supertype for functions (#1890)

## Motivation

The `ControllerMessenger` pattern that is used in all of our controllers
currently relies on unsafe typing, which includes `any` usage. A more
rigorous overhaul of these definitions will help us implement features
with more type-safe and robust code.

## Explanation

### Main

- Removes all `any` usage from `Action`, `Event` typing in
`ControllerMessenger`.
- Types `Action['handler']` with correct universal supertype for
functions.
- Fixes `SelectorFunction`, `EventSubscriptionMap` typing that was
causing errors.
- Improves `BaseController` typing by defining `NamespacedName` type:
the narrowest supertype of all names defined within a given namespace.
- `BaseController` now expects a "getState" action type and
"stateChange" event type in all controllers.

### Auxiliary

- Improved typing for `deriveStateFromMetadata`
- ~Rename all single-letter type param names to be more descriptive.~
(moved here: #2030)

### Notes on the "universal function supertype"

#### A. `(...args: never[]) => unknown`

`Action['handler'] extends ActionConstraint['handler']`, and in general
`Subtype extends Supertype`. Therefore, in order to assign any function
to `Action['handler']` without restrictions, we need
`ActionConstraint['handler']` to be the widest possible function i.e.
the supertype of all functions.

To achieve this, the return type of this supertype function needs to be
`unknown` (universal supertype), and the parameters need to be `never`
(universal subtype). This is because in general `(x: SuperType) => T` is
a *subtype* of `(x: SubType) => T`. So the params need to be the
narrowest type (i.e. `never`) for the function type to be widened into
the universal function supertype.

- There's an example of this in the [TypeScript
docs](https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#inferring-within-conditional-types).
- And here's a more detailed [reddit
comment](https://www.reddit.com/r/typescript/comments/muyl55/comment/gv9ndij/?utm_source=share&utm_medium=web2x&context=3)
on this topic.

#### B. `(...args: never) => unknown`

In general, array types of arbitrary length are not assignable to tuple
types that have fixed types designated to some or all of its index
positions (the array is wider than the tuple since it has less
constraints).

This means that there's a whole subcategory of functions that `(...args:
never[]) => unknown` doesn't cover: functions that have a finite number
of parameters that are strictly ordered and typed, making their spread
argument list evaluate as a fixed-length or variadic **tuple** type,
instead of a freely expandable (or contractable) **array** type.

That is, `(...args: never[]) => unknown` covers the cases where all of
the params can be typed together as a group e.g. `(...args: (string |
number)[]) => T`. But to cover cases like `(a: string, b: number,
...rest: unknown[]) => T`, we need another top type with an even
narrower type for the params: `(...args: never) => unknown`.

#### C. `((...args: never) => unknown) | ((...args: never[]) =>
unknown)`

In short, the above represents the top type that includes both of these
function categories.

- `| never` in general is analogous to `+ 0`, so `(...args: (never[] |
never)) => unknown` doesn't work.
- `(...args: never) => unknown` doesn't work with the error message
`Type 'T[]' is not assignable to type 'never'`.

## References

- Closes #2026

## Changelog

### `@metamask/base-controller`
- **CHANGED**: `ActionConstraint['handler']` has been fixed to remove
`any` usage, and is now defined as the universal supertype of all
functions, meaning any function can be safely assigned to it, regardless
of argument types, number of arguments, or return value type.
- **CHANGED** : `ExtractActionParameters` and `ExtractActionResponse`
have also been fixed to remove `any` usage, and to use the new typing of
`ActionConstraint['handler']`.
- **CHANGED**: **BREAKING** Alters `SelectorFunction` type, replacing
its `Args` generic parameter with an event payload type that is defined
with a new required generic parameter `Event`.
- **ADDED**: Exports `NamespacedName` type, which is the narrowest
supertype of all names defined within a given namespace.

### `@metamask/rate-limit-controller`
- **CHANGED**: **BREAKING** `RateLimitedApi['method']` is now
constrained by the `ActionConstraint['handler']` type from the
`@metamask/base-controller` module.

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

---------

Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>

Author: 3 people

packages/base-controller/src/BaseControllerV2.ts

@@ -3,8 +3,9 @@ import { enablePatches, produceWithPatches, applyPatches } from 'immer';
 import type { Draft, Patch } from 'immer';
 
 import type {
+  ActionConstraint,
+  EventConstraint,
   RestrictedControllerMessenger,
-  Namespaced,
 } from './ControllerMessenger';
 
 enablePatches();
@@ -62,13 +63,45 @@ export interface StatePropertyMetadata<T extends Json> {
   anonymous: boolean | StateDeriver<T>;
 }
 
+export type ControllerGetStateAction<
+  ControllerName extends string,
+  ControllerState extends Record<string, unknown>,
+> = {
+  type: `${ControllerName}:getState`;
+  handler: () => ControllerState;
+};
+
+export type ControllerStateChangeEvent<
+  ControllerName extends string,
+  ControllerState extends Record<string, unknown>,
+> = {
+  type: `${ControllerName}:stateChange`;
+  payload: [ControllerState, Patch[]];
+};
+
+export type ControllerActions<
+  ControllerName extends string,
+  ControllerState extends Record<string, unknown>,
+> = ControllerGetStateAction<ControllerName, ControllerState>;
+
+export type ControllerEvents<
+  ControllerName extends string,
+  ControllerState extends Record<string, unknown>,
+> = ControllerStateChangeEvent<ControllerName, ControllerState>;
+
 /**
  * Controller class that provides state management, subscriptions, and state metadata
  */
 export class BaseController<
   N extends string,
   S extends Record<string, Json>,
-  messenger extends RestrictedControllerMessenger<N, any, any, string, string>,
+  messenger extends RestrictedControllerMessenger<
+    N,
+    ActionConstraint | ControllerActions<N, S>,
+    EventConstraint | ControllerEvents<N, S>,
+    string,
+    string
+  >,
 > {
   private internalState: S;
 
@@ -164,7 +197,7 @@ export class BaseController<
 
     this.internalState = nextState;
     this.messagingSystem.publish(
-      `${this.name}:stateChange` as Namespaced<N, any>,
+      `${this.name}:stateChange`,
       nextState,
       patches,
     );
@@ -183,7 +216,7 @@ export class BaseController<
     const nextState = applyPatches(this.internalState, patches);
     this.internalState = nextState;
     this.messagingSystem.publish(
-      `${this.name}:stateChange` as Namespaced<N, any>,
+      `${this.name}:stateChange`,
       nextState,
       patches,
     );
@@ -199,9 +232,7 @@ export class BaseController<
    * listeners from being garbage collected.
    */
   protected destroy() {
-    this.messagingSystem.clearEventSubscriptions(
-      `${this.name}:stateChange` as Namespaced<N, any>,
-    );
+    this.messagingSystem.clearEventSubscriptions(`${this.name}:stateChange`);
   }
 }
 
@@ -250,20 +281,20 @@ function deriveStateFromMetadata<S extends Record<string, Json>>(
   metadata: StateMetadata<S>,
   metadataProperty: 'anonymous' | 'persist',
 ): Record<string, Json> {
-  return Object.keys(state).reduce((persistedState, key) => {
+  return (Object.keys(state) as (keyof S)[]).reduce<
+    Partial<Record<keyof S, Json>>
+  >((persistedState, key) => {
     try {
-      const stateMetadata = metadata[key as keyof S];
+      const stateMetadata = metadata[key];
       if (!stateMetadata) {
-        throw new Error(`No metadata found for '${key}'`);
+        throw new Error(`No metadata found for '${String(key)}'`);
       }
       const propertyMetadata = stateMetadata[metadataProperty];
       const stateProperty = state[key];
       if (typeof propertyMetadata === 'function') {
-        persistedState[key as string] = propertyMetadata(
-          stateProperty as S[keyof S],
-        );
+        persistedState[key] = propertyMetadata(stateProperty);
       } else if (propertyMetadata) {
-        persistedState[key as string] = stateProperty;
+        persistedState[key] = stateProperty;
       }
       return persistedState;
     } catch (error) {
@@ -274,5 +305,5 @@ function deriveStateFromMetadata<S extends Record<string, Json>>(
       });
       return persistedState;
     }
-  }, {} as Record<string, Json>);
+  }, {}) as Record<keyof S, Json>;
 }

packages/base-controller/src/ControllerMessenger.ts

@@ -1,38 +1,58 @@
-export type ActionHandler<Action, ActionType> = (
+export type ActionHandler<
+  Action extends ActionConstraint,
+  ActionType = Action['type'],
+> = (
   ...args: ExtractActionParameters<Action, ActionType>
 ) => ExtractActionResponse<Action, ActionType>;
-export type ExtractActionParameters<Action, T> = Action extends {
+
+export type ExtractActionParameters<
+  Action extends ActionConstraint,
+  T = Action['type'],
+> = Action extends {
   type: T;
-  handler: (...args: infer H) => any;
+  handler: (...args: infer H) => unknown;
 }
   ? H
   : never;
-export type ExtractActionResponse<Action, T> = Action extends {
+
+export type ExtractActionResponse<
+  Action extends ActionConstraint,
+  T = Action['type'],
+> = Action extends {
   type: T;
-  handler: (...args: any) => infer H;
+  handler: (...args: infer _) => infer H;
 }
   ? H
   : never;
 
-export type ExtractEventHandler<Event, T> = Event extends {
+export type ExtractEventHandler<
+  Event extends EventConstraint,
+  T = Event['type'],
+> = Event extends {
   type: T;
   payload: infer P;
 }
   ? P extends unknown[]
     ? (...payload: P) => void
     : never
   : never;
-export type ExtractEventPayload<Event, T> = Event extends {
+
+export type ExtractEventPayload<
+  Event extends EventConstraint,
+  T = Event['type'],
+> = Event extends {
   type: T;
   payload: infer P;
 }
-  ? P
+  ? P extends unknown[]
+    ? P
+    : never
   : never;
 
 export type GenericEventHandler = (...args: unknown[]) => void;
 
-export type SelectorFunction<Args extends unknown[], ReturnValue> = (
-  ...args: Args
+export type SelectorFunction<Event extends EventConstraint, ReturnValue> = (
+  ...args: ExtractEventPayload<Event>
 ) => ReturnValue;
 export type SelectorEventHandler<SelectorReturnValue> = (
   newValue: SelectorReturnValue,
@@ -41,13 +61,19 @@ export type SelectorEventHandler<SelectorReturnValue> = (
 
 export type ActionConstraint = {
   type: string;
-  handler: (...args: any) => unknown;
+  handler: ((...args: never) => unknown) | ((...args: never[]) => unknown);
+};
+export type EventConstraint = {
+  type: string;
+  payload: unknown[];
 };
-export type EventConstraint = { type: string; payload: unknown[] };
 
-type EventSubscriptionMap = Map<
-  GenericEventHandler | SelectorEventHandler<unknown>,
-  SelectorFunction<any, unknown> | undefined
+type EventSubscriptionMap<
+  Event extends EventConstraint,
+  ReturnValue = unknown,
+> = Map<
+  GenericEventHandler | SelectorEventHandler<ReturnValue>,
+  SelectorFunction<ExtractEventPayload<Event>, ReturnValue> | undefined
 >;
 
 /**
@@ -62,6 +88,9 @@ export type Namespaced<Name extends string, T> = T extends `${Name}:${string}`
   ? T
   : never;
 
+export type NamespacedName<Namespace extends string = string> =
+  `${Namespace}:${string}`;
+
 type NarrowToNamespace<T, Namespace extends string> = T extends {
   type: `${Namespace}:${string}`;
 }
@@ -151,10 +180,10 @@ export class RestrictedControllerMessenger<
    * @param action - The action type. This is a unqiue identifier for this action.
    * @param handler - The action handler. This function gets called when the `call` method is
    * invoked with the given action type.
-   * @throws Will throw when a handler has been registered for this action type already.
+   * @throws Will throw if an action handler that is not in the current namespace is being registered.
    * @template T - A type union of Action type strings that are namespaced by N.
    */
-  registerActionHandler<T extends Namespaced<N, Action['type']>>(
+  registerActionHandler<T extends NamespacedName<N>>(
     action: T,
     handler: ActionHandler<Action, T>,
   ) {
@@ -177,7 +206,7 @@ export class RestrictedControllerMessenger<
    * @param action - The action type. This is a unqiue identifier for this action.
    * @template T - A type union of Action type strings that are namespaced by N.
    */
-  unregisterActionHandler<T extends Namespaced<N, Action['type']>>(action: T) {
+  unregisterActionHandler<T extends NamespacedName<N>>(action: T) {
     /* istanbul ignore if */ // Branch unreachable with valid types
     if (!action.startsWith(`${this.controllerName}:`)) {
       throw new Error(
@@ -202,7 +231,7 @@ export class RestrictedControllerMessenger<
    * @template T - A type union of allowed Action type strings.
    * @returns The action return value.
    */
-  call<T extends AllowedAction & string>(
+  call<T extends AllowedAction & NamespacedName>(
     action: T,
     ...params: ExtractActionParameters<Action, T>
   ): ExtractActionResponse<Action, T> {
@@ -227,7 +256,7 @@ export class RestrictedControllerMessenger<
    * match the type of this payload.
    * @template E - A type union of Event type strings that are namespaced by N.
    */
-  publish<E extends Namespaced<N, Event['type']>>(
+  publish<E extends NamespacedName<N>>(
     event: E,
     ...payload: ExtractEventPayload<Event, E>
   ) {
@@ -252,7 +281,7 @@ export class RestrictedControllerMessenger<
    * match the type of the payload for this event type.
    * @template E - A type union of Event type strings.
    */
-  subscribe<E extends AllowedEvent & string>(
+  subscribe<E extends AllowedEvent & NamespacedName>(
     eventType: E,
     handler: ExtractEventHandler<Event, E>,
   ): void;
@@ -276,13 +305,13 @@ export class RestrictedControllerMessenger<
    * @template E - A type union of Event type strings.
    * @template V - The selector return value.
    */
-  subscribe<E extends AllowedEvent & string, V>(
+  subscribe<E extends AllowedEvent & NamespacedName, V>(
     eventType: E,
     handler: SelectorEventHandler<V>,
     selector: SelectorFunction<ExtractEventPayload<Event, E>, V>,
   ): void;
 
-  subscribe<E extends AllowedEvent & string, V>(
+  subscribe<E extends AllowedEvent & NamespacedName, V>(
     event: E,
     handler: ExtractEventHandler<Event, E>,
     selector?: SelectorFunction<ExtractEventPayload<Event, E>, V>,
@@ -312,7 +341,7 @@ export class RestrictedControllerMessenger<
    * @throws Will throw when the given event handler is not registered for this event.
    * @template T - A type union of allowed Event type strings.
    */
-  unsubscribe<E extends AllowedEvent & string>(
+  unsubscribe<E extends AllowedEvent & NamespacedName>(
     event: E,
     handler: ExtractEventHandler<Event, E>,
   ) {
@@ -335,7 +364,7 @@ export class RestrictedControllerMessenger<
    * @param event - The event type. This is a unique identifier for this event.
    * @template E - A type union of Event type strings that are namespaced by N.
    */
-  clearEventSubscriptions<E extends Namespaced<N, Event['type']>>(event: E) {
+  clearEventSubscriptions<E extends NamespacedName<N>>(event: E) {
     /* istanbul ignore if */ // Branch unreachable with valid types
     if (!event.startsWith(`${this.controllerName}:`)) {
       throw new Error(
@@ -362,7 +391,10 @@ export class ControllerMessenger<
 > {
   private readonly actions = new Map<Action['type'], unknown>();
 
-  private readonly events = new Map<Event['type'], EventSubscriptionMap>();
+  private readonly events = new Map<
+    Event['type'],
+    EventSubscriptionMap<Event>
+  >();
 
   /**
    * A cache of selector return values for their respective handlers.

packages/rate-limit-controller/src/RateLimitController.ts

@@ -1,4 +1,7 @@
-import type { RestrictedControllerMessenger } from '@metamask/base-controller';
+import type {
+  ActionConstraint,
+  RestrictedControllerMessenger,
+} from '@metamask/base-controller';
 import { BaseControllerV2 as BaseController } from '@metamask/base-controller';
 import { rpcErrors } from '@metamask/rpc-errors';
 import type { Patch } from 'immer';
@@ -10,7 +13,7 @@ import type { Patch } from 'immer';
  * @property rateLimitCount - The amount of calls an origin can make in the rate limit time window.
  */
 export type RateLimitedApi = {
-  method: (...args: any[]) => any;
+  method: ActionConstraint['handler'];
   rateLimitTimeout?: number;
   rateLimitCount?: number;
 };
@@ -121,11 +124,11 @@ export class RateLimitController<
 
     this.messagingSystem.registerActionHandler(
       `${name}:call` as const,
-      ((
+      (
         origin: string,
         type: keyof RateLimitedApis,
         ...args: Parameters<RateLimitedApis[keyof RateLimitedApis]['method']>
-      ) => this.call(origin, type, ...args)) as any,
+      ) => this.call(origin, type, ...args),
     );
   }
 
@@ -135,7 +138,6 @@ export class RateLimitController<
    * @param origin - The requesting origin.
    * @param type - The type of API call to make.
    * @param args - Arguments for the API call.
-   * @returns `false` if rate-limited, and `true` otherwise.
    */
   async call<ApiType extends keyof RateLimitedApis>(
     origin: string,
@@ -155,7 +157,9 @@ export class RateLimitController<
       throw new Error('Invalid api type');
     }
 
-    return implementation(...args);
+    return implementation(...args) as ReturnType<
+      RateLimitedApis[ApiType]['method']
+    >;
   }
 
   /**