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

Convert TokensController from BaseControllerV1 to BaseControllerV2 #4075

Closed
desi opened this issue Mar 18, 2024 · 2 comments · Fixed by #4304
Closed

Convert TokensController from BaseControllerV1 to BaseControllerV2 #4075

desi opened this issue Mar 18, 2024 · 2 comments · Fixed by #4304
Assignees

Comments

@desi
Copy link
Contributor

desi commented Mar 18, 2024

Considering that converting TokenDetectionController to BaseController v2 took longer than expected due to unforeseen changes, we want to be more conscientious about converting TokensController.

Figure out all of the changes we would want to make in upgrading this controller, outline those changes here and create new tickets if appropriate.

@desi desi changed the title (assets-controllers) TokensController Convert TokensController from BaseControllerV1 to BaseControllerV2 Mar 18, 2024
@mikesposito
Copy link
Member

mikesposito commented Apr 17, 2024

TokensController is mostly in good shape. It already uses a messenger, it already broadcasts events through it and it already handles actions properly.

Along with the main change of extending BaseControllerV2 class instead of V1, we can:

  • Rename TokenState type to TokensControllerState, to match other controllers.
  • Same for TokensConfig -> TokensControllerConfig
  • We should be able to use the already declared allowed events and actions
  • We should lower the visibility of these functions to #:
    • _createEthersContract -> #createEthersContract
    • _detectIsERC721 -> #detectIsERC721
    • _generateRandomId -> #generateRandomId
    • _getNewAllTokensState -> #getNewAllTokensState
    • _getProvider -> #getProvider
    • _requestApproval -> #requestApproval
  • We should be able to remove the hub property, since it appears unused (TODO: verify this statement)
  • Tests should be refactored to support the new controller version
    • In addition to that, we can create a withController helper to make tests more readable and easier to be maintained

@desi
Copy link
Contributor Author

desi commented Apr 17, 2024

hub is used in Mobile in one place.

@cryptodev-2s cryptodev-2s self-assigned this May 22, 2024
cryptodev-2s added a commit that referenced this issue May 29, 2024
## Explanation

The TokensController has been migrated to BaseControllerV2. As part of
this migration, the deprecated config property has been removed and has
been replaced with flatten properties on the controller constructor
(`chainId`, `selectedAddress` and `provider`).

PS: A migration is needed when using a new release of this controller.

## References

* Fixes #4075 

## Changelog

### `@metamask/assets-controllers`

#### Changed
- **BREAKING:** Convert TokensController to `BaseControllerV2`
- The constructor parameters have changed; rather than accepting a
"config" parameter, it takes`selectedAddress` and `provider` parameters.
- **BREAKING:** Convert Token object in TokenBalancesController to a
`type` instead of `interface` and replace the `balanceError` property
with `hasBalanceError` flag.

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

Successfully merging a pull request may close this issue.

3 participants