Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[composable-controller] Fix BaseControllerV1Instance type and make ChildControllers a required type parameter #4448

Closed
MajorLift opened this issue Jun 21, 2024 · 0 comments · Fixed by #4467
Assignees
Labels
bug Something isn't working team-wallet-framework

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Jun 21, 2024

References

Motivation

The ComposableController API is currently not working as expected in downstream consumers (mobile).

Explanation

  • The BaseControllerV1Instance type needs to be fixed to accommodate controller versions used in mobile that inherit from BaseControllerV1.
    Type 'PhishingController | AccountsController | AccountTrackerController | AddressBookController | ... 22 more ... | SwapsController' does not satisfy the constraint 'ControllerInstance'.
    Type 'AccountTrackerController' is not assignable to type 'ControllerInstance'.
      Type 'AccountTrackerController' is not assignable to type 'BaseControllerV1Instance'.
        Types have separate declarations of a private property 'initialConfig'.ts(2344)
  • The ChildControllers optional generic parameter should either be converted into a required parameter, or annotated in jsdoc to make it clear that a full list of controllers can be supplied by the consumer as a type argument.
  • Fix LegacyComposableControllerStateConstraint to { [name: string]: Record<any, any> }, since index signature of interface types used for V1 state objects is not fixed to string.
  • Test on mobile engine whether runtime checks isBaseController() and isBaseControllerV1() are functioning as intended.
@MajorLift MajorLift added bug Something isn't working team-wallet-framework labels Jun 21, 2024
@MajorLift MajorLift self-assigned this Jun 21, 2024
MajorLift added a commit that referenced this issue Aug 20, 2024
… 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/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<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
@MajorLift MajorLift reopened this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-wallet-framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant