core icon indicating copy to clipboard operation
core copied to clipboard

Convert AssetsContractController from BaseControllerV1 to BaseControllerV2

Open desi opened this issue 1 year ago • 3 comments

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 avatar Mar 18 '24 18:03 desi

  • 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?

MajorLift avatar May 23 '24 04:05 MajorLift

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.

desi avatar May 23 '24 16:05 desi

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

cryptodev-2s avatar May 23 '24 16:05 cryptodev-2s