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

feat: migrate token balances controller to base controller v2 #3750

Merged

Conversation

cryptodev-2s
Copy link
Contributor

@cryptodev-2s cryptodev-2s commented Jan 9, 2024

Explanation

The TokenBalancesController has been migrated to BaseControllerV2.

References

Closes #1808

Changelog

@metamask/token-balances-controller

Changed

  • BREAKING: Convert to BaseControllerV2
    • The constructor parameters have changed; rather than accepting a "config" parameter for interval and tokens we now pass both values as controller options, and a "state" parameter, there is now just a single object for all constructor arguments. This object has a mandatory messenger and an optional state, tokens, interval properties a disabled property has also been added.
    • State now saves tokens balances as strings and not as a BNs.
    • Additional BN export has been removed as it was intended to be removed in the next major release.

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

@cryptodev-2s cryptodev-2s requested a review from a team as a code owner January 9, 2024 16:06
@cryptodev-2s cryptodev-2s force-pushed the migrate-token-balances-controller-to-base-controller-v2 branch from 5cdeec0 to e125c97 Compare January 10, 2024 13:57
@cryptodev-2s cryptodev-2s force-pushed the migrate-token-balances-controller-to-base-controller-v2 branch from 2f9305f to 8b2f042 Compare January 10, 2024 15:12
@MajorLift
Copy link
Contributor

BaseControllerV2 controllers now have a :getState action and :stateChange event by default, so we'll need to add TokenBalancesControllerGetStateAction and TokenBalancesControllerStateChangeEvent.

For reference (from #3707):

  • Introduce TransactionControllerGetStateAction type
    • You can use the ControllerGetStateAction utility type to create this; see TokenDetectionController for an example
  • Introduce TransactionControllerStateChangeEvent type
    • You can use the ControllerStateChangeEvent utility type to create the type; see TokenDetectionController for an example
  • Introduce TransactionControllerControllerEvents type, set to TransactionControllerStateChangeEvent
  • Introduce TransactionControllerControllerActions type, set to TransactionControllerGetStateAction
  • Update TransactionControllerMessenger to allowlist the above events and actions

@MajorLift
Copy link
Contributor

Also, it seems like this controller should extend from StaticIntervalPollingController, not BaseControllerV2. The .{g,s}etIntervalLength() methods could be used instead of this.#interval. Although maybe this could be done in a separate PR?

@cryptodev-2s
Copy link
Contributor Author

BaseControllerV2 controllers now have a :getState action and :stateChange event by default, so we'll need to add TokenBalancesControllerGetStateAction and TokenBalancesControllerStateChangeEvent.

For reference (from #3707):

  • Introduce TransactionControllerGetStateAction type

    • You can use the ControllerGetStateAction utility type to create this; see TokenDetectionController for an example
  • Introduce TransactionControllerStateChangeEvent type

    • You can use the ControllerStateChangeEvent utility type to create the type; see TokenDetectionController for an example
  • Introduce TransactionControllerControllerEvents type, set to TransactionControllerStateChangeEvent

  • Introduce TransactionControllerControllerActions type, set to TransactionControllerGetStateAction

  • Update TransactionControllerMessenger to allowlist the above events and actions

missing types added

@cryptodev-2s
Copy link
Contributor Author

cryptodev-2s commented Jan 10, 2024

Also, it seems like this controller should extend from StaticIntervalPollingController, not BaseControllerV2. The .{g,s}etIntervalLength() methods could be used instead of this.#interval. Although maybe this could be done in a separate PR?

Currently TokenBalancesController is not tied to NetworkController and if I correctly understood StaticIntervalPollingController require having networkClientId

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.

Made a first pass on this. Nice work overall. I'll check back a bit later but this should cover most of it for now.

packages/assets-controllers/src/TokenBalancesController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/TokenBalancesController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/TokenBalancesController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/index.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/TokenBalancesController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/TokenBalancesController.ts Outdated Show resolved Hide resolved
@mcmire
Copy link
Contributor

mcmire commented Jan 10, 2024

@cryptodev-2s @MajorLift Re: inheriting from StaticIntervalPollingController, I believe the purpose of that superclass is to ensure that polling can be performed in a multichain-compatible way. I don't think this controller has been converted to be multichain-compatible yet — I think it's still on the DevEx team's board to do that. So perhaps we stick with BaseControllerV2 for now and then if they see the need to migrate this class to StaticIntervalPollingController, they can do that, or we can if we end up taking it over from them.

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.

Nice! Just a few more suggestions.

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!

@cryptodev-2s cryptodev-2s merged commit 5b34a40 into main Jan 11, 2024
136 checks passed
@cryptodev-2s cryptodev-2s deleted the migrate-token-balances-controller-to-base-controller-v2 branch January 11, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update TokenBalancesController to BaseControllerV2
3 participants