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.

Sign up for GitHub

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

Jump to bottom

[composable-controller] Subscribe to stateChange events of V1 controllers with messenger, type fixes #3964

Merged
merged 12 commits into from
Mar 7, 2024
Merged

[composable-controller] Subscribe to stateChange events of V1 controllers with messenger, type fixes #3964

merged 12 commits into from
Mar 7, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Feb 23, 2024 •
edited
Loading

Explanation

  • If a controller is V1 but it has a messaging system, subscribe to its stateChange event.
  • Remove @metamask/utils as a dependency.
  • Remove package-level exports for BaseControllerV{1,2}Instance, ControllerInstance, as these are incomplete and unsuited for general usage. isBaseController{,V1} are not removed.
    • Rename BaseControllerV2Instance as BaseControllerInstance for consistency with isBaseController.

References

Changelog

@metamask/composable-controller

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 self-assigned this Feb 23, 2024
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from cac2559 to 0c830ba Compare February 23, 2024 04:39
@MajorLift MajorLift linked an issue Feb 23, 2024 that may be closed by this pull request
[composable-controller] Fix incorrect #updateChildController behavior and improve typing #3907
Closed
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0c830ba to 599fd44 Compare February 23, 2024 04:48
@MajorLift MajorLift removed a link to an issue Feb 23, 2024
[composable-controller] Fix incorrect #updateChildController behavior and improve typing #3907
Closed
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from bdc167c to 0882483 Compare February 23, 2024 19:30
@MajorLift MajorLift changed the title [composable-controller] Type fixes, cleanup [composable-controller] Subscribe to stateChange events of V1 controllers with messenger, type fixes Feb 23, 2024
@MajorLift MajorLift marked this pull request as ready for review February 23, 2024 19:37
@MajorLift MajorLift requested a review from a team as a code owner February 23, 2024 19:37
@MajorLift MajorLift linked an issue Feb 23, 2024 that may be closed by this pull request
[composable-controller] Subscribe to stateChange events of V1 controllers with messenger #3966
Closed
@cryptodev-2s
Copy link
Contributor

@MajorLift in case of BaseControllerV1 without messagingSystem shouldn't we throw an error ? Or in this case I guess we would want only to save the initial state within the ComposableController ?
Other ways you PR looks good

@MajorLift
Copy link
Contributor Author

@cryptodev-2s Hey thanks for taking a look at this! A BaseControllerV1 controller without a messagingSystem would be the default expectation and not an error case, since the controller-messenger pattern was only introduced in BaseControllerV2.

This PR enables composable-controller to handle V1 controllers like NftController, TokensController, TransactionsController that do have a messagingSystem because they are halfway there to upgrading to V2, but for regular V1 controllers where that's not the case, composable-controller's existing logic for merging state and subscribing a listener to the controller is preserved.

cryptodev-2s
cryptodev-2s previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 6ef730e to 0fa97c3 Compare February 27, 2024 23:33
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0ab242a to 0fa97c3 Compare March 6, 2024 16:34
Base automatically changed from 240221-base-controller-type-fixes to main March 7, 2024 18:24
@MajorLift MajorLift dismissed cryptodev-2s’s stale review March 7, 2024 18:24

The base branch was changed.

@MajorLift MajorLift requested a review from a team as a code owner March 7, 2024 18:24
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0fa97c3 to 7d46b06 Compare March 7, 2024 18:26
@MajorLift MajorLift removed the request for review from a team March 7, 2024 18:36
mcmire
mcmire reviewed Mar 7, 2024
View reviewed changes
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.

Looks mostly good! Just had one question.

packages/composable-controller/src/ComposableController.ts
*
* The `BaseController` class itself can't be used directly as a type representing all of its subclasses,
* because the generic parameters it expects require knowing the exact shape of the controller's state and messenger.
* Note that this type is not the greatest subtype or narrowest supertype of all `BaseController` instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

packages/composable-controller/src/ComposableController.ts
if (
(isBaseControllerV1(controller) && 'messagingSystem' in controller) ||
isBaseController(controller)
) {
this.messagingSystem.subscribe(
Copy link
Contributor

Choose a reason for hiding this comment

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

If a BaseControllerV1 controller has a messagingSystem property, could the state of this controller could be updated twice per update? If so, do we still need an else if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! Fixed here: 811a94a

MajorLift and others added 2 commits March 7, 2024 16:13
mcmire
mcmire approved these changes Mar 7, 2024
View reviewed changes
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.

Looks good!

@MajorLift MajorLift merged commit 964e389 into main Mar 7, 2024
139 checks passed
@MajorLift MajorLift deleted the 240222-composable-controller-type-fixes branch March 7, 2024 21:24
@MajorLift MajorLift mentioned this pull request Apr 25, 2024
Merged
3 tasks
@MajorLift MajorLift mentioned this pull request Jun 15, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire mcmire approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s left review comments

Assignees

@MajorLift MajorLift

Labels
base-controller-breaking-changes team-wallet-framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[composable-controller] Subscribe to stateChange events of V1 controllers with messenger
3 participants
@MajorLift @cryptodev-2s @mcmire