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(deps): bump @metamask/composable-controller from ^3.0.0 to ^6.0.2 #10011

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Jun 15, 2024

Description

Required follow-up changes

  • ComposableController:

    • 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)
    • Add jsdoc for the ChildControllers optional generic parameter, making it clearer that a full list of controllers can be supplied by the consumer.
    • 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.
  • On mobile, upgrade the affected controllers to V2:

    • V2 upgrades are available for all of the following:
      • AccountTrackerController
      • NftController
      • TokenRatesController
      • TransactionController
    • Exceptions:
      • AssetsContractController: set to be upgraded to non-controller that uses messenger but doesn't inherit from BaseControllerV2. ComposableController will need to be updated to accommodated non-controllers
      • NftDetectionController: current release inherits from BaseControllerV2, but should be upgraded to non-controller.
      • SwapsController: V2 upgrade unavailable

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

this.datamodel = new ComposableController({
this.datamodel = new ComposableController<
EngineState,
Controllers[keyof Controllers]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're seeing an error on this line that requires the following hotfix:
Remove the ControllerInstance generic constraint from the ChildControllers generic parameter of ComposableController.

Fundamentally, the ControllerInstance type needs to be fixed so that BaseControllerV1 instances are assignable.

@MajorLift MajorLift changed the title fix: Apply API changes necessary for ComposableController 6.0.2 upgrade fix(deps): bump @metamask/composable-controller from ^3.0.0 to ^6.0.2 Jun 15, 2024
@MajorLift MajorLift force-pushed the jongsun/composable-controller-6.0.2-upgrade branch from 3fd34e5 to bc1392b Compare June 18, 2024 19:29
…trollers` type members and `BaseControllerV1Instance` type
@MajorLift MajorLift closed this Jul 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant