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 AssetsContractController from BaseControllerV1 to BaseControllerV2 #4072

Closed
Tracked by #4432
desi opened this issue Mar 18, 2024 · 3 comments · Fixed by #4397 or #4564
Closed
Tracked by #4432

Convert AssetsContractController from BaseControllerV1 to BaseControllerV2 #4072

desi opened this issue Mar 18, 2024 · 3 comments · Fixed by #4397 or #4564
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 AssetsContractController.

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) AssetsContractController Convert AssetsContractController from BaseControllerV1 to BaseControllerV2 Mar 18, 2024
@MajorLift
Copy link
Contributor

MajorLift commented May 23, 2024

  • Define internal messenger actions/events types.

    • Define messenger actions for all public methods of AssetsContractController class.
    • Define AssetsContractControllerActions, AssetsContractControllerEvents, AssetsContractControllerGetStateAction (using ControllerGetStateAction type), AssetsContractControllerStateChangeEvent (using ControllerStateChangeEvent type)
  • Replace constructor option callbacks with messenger actions/events

    • onPreferencesStateChange
      • PreferencesController:stateChange
      • Use PreferencesControllerStateChangeEvent
    • onNetworkDidChange
      • NetworkController:networkDidChange
      • Use NetworkControllerNetworkDidChangeEvent
    • getNetworkClientById
      • NetworkController:getNetworkClientById
      • Use NetworkControllerGetNetworkClientByIdAction
    • Define AllowedActions union type and add the above as its members.
      • Export this type so it's usable by tests and other internal modules, but exclude it as a package-level export.
    • Replace the callback invocations in the constructor with subscribe() calls, using the existing listener logic.
  • Define AssetsContractControllerMessenger using the types defined in the above bullet points.

  • AssetsContractController extends BaseControllerV2.

  • AssetsContractConfig

    • Move properties of this type to private class fields of the AssetsContractController class.
    • Set default values from this.defaultConfig object.
    • Replace this.configure calls with assignments to corresponding class fields.
  • constructor

    • Replace super call so that it conforms to V2 requirements.
    • Remove this.initialize()
  • Remove override name class field.

  • Replace all private keywords for class fields and methods with # prefix.

  • Refactor tests. Use withController pattern.

  • Consider replacing type annotation for SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID with satisfies keyword.

  • Note: This controller has an empty state object

    • May not need to be a controller class at all (i.e. should not extend from BaseControllerV2).
    • Could be modeled as collection of utility functions or as a service class instead.
      • Public class methods could be static methods instead of messenger actions.
      • Devise new API for downstream invocation of methods (currently using constructor option callbacks), or define messenger actions even if they don't interact with state?

@desi
Copy link
Contributor Author

desi commented May 23, 2024

All of the steps above are correct. Rename to not be called Controller and remove it from using the BaseController class. Whoever picks this up should decide how to make this class have the best api.

Acceptance criteria is basically we are not using BaseControllerV1 here.

@cryptodev-2s
Copy link
Contributor

cryptodev-2s commented May 23, 2024

Some functions seems to be just helper function used within the controller only for example getERC20Standard and getChainId which can be private instead

@MajorLift MajorLift self-assigned this Jun 5, 2024
@MajorLift MajorLift reopened this Jul 26, 2024
@MajorLift MajorLift linked a pull request Jul 26, 2024 that will close this issue
3 tasks
MajorLift added a commit that referenced this issue Jul 30, 2024
## References

- See #4072

## Changelog

```md
### Added

- Add public readonly property `name` to `AssetsContractController` and assign "AssetsContractController" as its value.
```

## 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
AugmentedMode pushed a commit that referenced this issue Jul 30, 2024
## References

- See #4072

## Changelog

```md
### Added

- Add public readonly property `name` to `AssetsContractController` and assign "AssetsContractController" as its value.
```

## 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