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

fix: ComposableController instantiation and functionality is broken #10073

Open
9 tasks
MajorLift opened this issue Jun 21, 2024 · 0 comments · May be fixed by #10441
Open
9 tasks

fix: ComposableController instantiation and functionality is broken #10073

MajorLift opened this issue Jun 21, 2024 · 0 comments · May be fixed by #10441
Assignees
Labels
Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-wallet-framework type-bug Something isn't working

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Jun 21, 2024

What is this about?

Explanation

ComposableController usage in the mobile Engine is currently broken in two ways.

  1. The first argument of the ComposableController's constructor has a type constraint that is incompatible with V1 members of the controllers input array.
Argument of type '(PhishingController | AccountsController | AccountTrackerController | AddressBookController | ... 22 more ... | SwapsController)[]' is not assignable to parameter of type 'ControllerList'.
  Type 'PhishingController | AccountsController | AccountTrackerController | AddressBookController | ... 22 more ... | SwapsController' is not assignable to type 'BaseController<any, any> | { name: string; state: Record<string, unknown>; }'.
    Type 'AccountTrackerController' is not assignable to type 'BaseController<any, any> | { name: string; state: Record<string, unknown>; }'.
      Type 'AccountTrackerController' is not assignable to type 'BaseController<any, any>'.
        Types have separate declarations of a private property 'initialConfig'.ts(2345)

(This error is also encountered with later versions of ComposableController. See MetaMask/core#4448)

Up until now, this error has been suppressed with the following comment, which is incorrect:

// @ts-expect-error The ComposableController needs to be updated to support BaseControllerV2
  1. The second argument passed into the ComposableController constructor has always been a ControllerMessenger class instance, when it should have been a RestrictedControllerMessenger.
    this.datamodel = new ComposableController(
      controllers,
-     this.controllerMessenger,
+     this.controllerMessenger.getRestricted({
+       name: 'ComposableController',
+       allowedActions: [],
+       allowedEvents: [
...
+       ],
+     )},
    );

This grants the ComposableController wider permissions than intended (we only want to allow stateChange events for all child controllers), and generally goes against our recommended usage pattern for messengers.

(However, it shouldn't prevent the controllerMessenger or ComposableController state from subscribing to state updates from child controllers)

Solution

  1. Add tests to Engine.test.js file verifying that the datamodel has subscribed to state updates from all child controllers (both those with a messagingSystem, and V1 controllers with no messagingSystem).

  2. Bump @metamask/composable-controller to the latest version which offers better type safety and quality-of-life improvements.

@metamask/[email protected] features widespread any usage, which would unnecessarily complicate any debugging efforts.

  1. For the issue of defining a type constraint that can accept V1 controllers for the first argument controllers, attempt to resolve by bumping V1 controllers from mobile.

If unsuccessful, wait until the issue is fixed from the ComposableController side: MetaMask/core#4448

Scenario

No response

Design

No response

Technical Details

No response

Threat Modeling Framework

No response

Acceptance Criteria

No response

Stakeholder review needed before the work gets merged

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References

No response

@MajorLift MajorLift added team-wallet-framework type-bug Something isn't working labels Jun 21, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Jun 21, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Jun 21, 2024
@MajorLift MajorLift self-assigned this Jun 27, 2024
@gauthierpetetin gauthierpetetin added the Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking label Jun 28, 2024
MajorLift added a commit to MetaMask/core 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/[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
@desi desi assigned cryptodev-2s and unassigned MajorLift Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-wallet-framework type-bug Something isn't working
Projects
Status: To be fixed
Status: To be fixed
3 participants