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

Upgrade ComposableController to extend BaseControllerV2 #3590

Merged

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 29, 2023

Explanation

As part of the upcoming core wallet library initiative, we plan to migrate all controllers to BaseControllerV2. This commit upgrades ComposableController.

References

Changelog

@metamask/composable-controller

Added

  • Add types ComposableControllerState, ComposableControllerStateChangeEvent, ComposableControllerEvents, ComposableControllerMessenger.

Changed

  • BREAKING: ComposableController is upgraded to extend BaseControllerV2.
    • The constructor now expects an options object with required properties controllers and messenger as its only argument.
    • ComposableController no longer has a subscribe method. Instead, listeners for ComposableController events must be registered to the controller messenger that generated the restricted messenger assigned to the instance's messagingSystem class field.
    • Any getters for ComposableController state that access the internal class field directly should be refactored to instead use listeners that are subscribed to ComposableControllerStateChangeEvent.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift force-pushed the 231128-upgrade-ComposableController-to-BaseControllerV2 branch 6 times, most recently from 6ecd249 to 75f3d62 Compare November 30, 2023 19:46
@MajorLift MajorLift changed the title Update ComposableController to extend BaseControllerV2 Upgrade ComposableController to extend BaseControllerV2 Nov 30, 2023
@MajorLift MajorLift self-assigned this Nov 30, 2023
@MajorLift MajorLift force-pushed the 231128-upgrade-ComposableController-to-BaseControllerV2 branch from c58856b to 7eee820 Compare November 30, 2023 19:55
@MajorLift MajorLift marked this pull request as ready for review November 30, 2023 20:50
@MajorLift MajorLift requested a review from a team as a code owner November 30, 2023 20:50
@MajorLift MajorLift force-pushed the 231128-upgrade-ComposableController-to-BaseControllerV2 branch from 1d953dd to 1959df1 Compare December 1, 2023 21:05
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't finished a full review of this, but I had some questions about making this controller more type safe. They might open a can of worms but I thought I'd pose them anyway.

packages/composable-controller/src/ComposableController.ts Outdated Show resolved Hide resolved
packages/composable-controller/src/ComposableController.ts Outdated Show resolved Hide resolved
packages/composable-controller/src/ComposableController.ts Outdated Show resolved Hide resolved
packages/composable-controller/src/ComposableController.ts Outdated Show resolved Hide resolved
export type ComposableControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
ControllerGetStateAction<string, Record<string, unknown>>,
ControllerStateChangeEvent<string, Record<string, unknown>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This would seem to indicate that this controller can listen to any controller anywhere, but that's not true: it can only listen to the controllers that are passed. Additionally, the set of possible state objects that stateChange emits are based on the controllers given.

We have the same issue with the state: each key in the state is the name of a controller, and the valid set of names comes from the controllers given to this controller.

It seems that the ControllerInstance ought to be a type parameter here and everywhere else. However, I tried to make this change and I was running into issues. :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess my question here is — is it worth it to make this type more specific?

Copy link
Contributor Author

@MajorLift MajorLift Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely is, but we'll need new logic that isn't used anywhere else.

For other controllers, their allow lists are tied to the controller class singleton, but for composable-controller, the allow lists need to be different for every class instance, depending on the list of child controllers that are passed into the constructor.

To accommodate this difference, we would need to make ComposableControllerMessenger polymorphic upon the list of child controllers e.g.

type AllControllersStateChangeEvents = ControllerStateChangeEvent<
  string,
  Record<string, unknown>
>;

export type ComposableControllerMessenger<
  ChildControllers extends ControllerInstance = ControllerList[number],
  ChildControllersStateChangeEvents extends AllControllersStateChangeEvents = ChildControllers extends never
    ? AllControllersStateChangeEvents
    : ControllerStateChangeEvent<
        ChildControllers['name'],
        ChildControllers['state']
      >,
> = RestrictedControllerMessenger<
  typeof controllerName,
  never,
  ChildControllersStateChangeEvents,
  string,
  string
>;

We don't have any existing logic to ensure that ControllerInstance and ControllerList actually reflect the child controllers being used, though. They're "least upper bound" constraints, not "greatest lower bound" definitions, so to speak. We'd need additional generic logic in the ComposableController class and .getRestricted() to align all of those with the ChildControllers generic parameter I'm using in the above example. I'll have to give this some thought...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, okay. It sounds like making that change could cause this PR to balloon, so perhaps I will review this PR with a minimal scope in mind and then we can follow it up with another PR that improves upon the types. Does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like the idea of narrowing these types in a separate PR. For now, enabling all state change events should let us get to a composable-controller that is fully functional, if not fully type-safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a new ticket for the typing improvements: #3627

MajorLift and others added 5 commits December 4, 2023 15:15
Co-authored-by: Ellilot Winkler <[email protected]>
- No other way to handle `super({ messenger })` call now that `ComposableController` extends from `BaseControllerV2`
@MajorLift MajorLift force-pushed the 231128-upgrade-ComposableController-to-BaseControllerV2 branch 2 times, most recently from 757e790 to 0a3d642 Compare December 5, 2023 16:10
@MajorLift MajorLift force-pushed the 231128-upgrade-ComposableController-to-BaseControllerV2 branch from 0a3d642 to bd22f1c Compare December 5, 2023 16:14
@MajorLift MajorLift force-pushed the 231128-upgrade-ComposableController-to-BaseControllerV2 branch from 4301ee8 to 3d8a220 Compare December 7, 2023 19:50
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

@MajorLift MajorLift merged commit 9ea6b88 into main Dec 11, 2023
136 checks passed
@MajorLift MajorLift deleted the 231128-upgrade-ComposableController-to-BaseControllerV2 branch December 11, 2023 22:47
Gudahtt added a commit that referenced this pull request Dec 22, 2023
The `subscribe` property was removed in #3590 because it is no longer
used. It has been restored here so that we can release the
`@metamask/base-controller` without any breaking changes.

This property's description has been updated to clarify that it's no
longer used, and it's now marked as deprecated.
Gudahtt added a commit that referenced this pull request Dec 22, 2023
The `subscribe` property was removed in #3590 because it is no longer
used. It has been restored here so that we can release the
`@metamask/base-controller` without any breaking changes.

This property's description has been updated to clarify that it's no
longer used, and it's now marked as deprecated.
Gudahtt added a commit that referenced this pull request Dec 22, 2023
## Explanation

The `subscribe` property was removed in #3590 because it is no longer
used. It has been restored here so that we can release the
`@metamask/base-controller` without any breaking changes.

This property's description has been updated to clarify that it's no
longer used, and it's now marked as deprecated.

## References

Discussed in the v104 release candidate PR:
#3695 (comment)

## Changelog

### `@metamask/base-controller`

- Changed: Deprecate the `subscribe` property of `BaseControllerV2`
- This property was used to differentiate between `BaseControllerV1` and
`BaseControllerV2` controllers. It is no longer used, so it has been
marked as deprecated.

## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ComposableController to BaseController v2
2 participants