[assets-contract-controller] Apply messenger pattern and remove inheritance from `BaseControllerV1` #4397

MajorLift · 2024-06-10T18:03:23Z

Motivation

As part of the Wallet Framework Team's Q2 2024 OKRs (O3KR1: "100% completion of all core controllers to BaseControllerV2"), the AssetsContractController needs to be converted to a V2 controller.

However, because AssetsContractController has an empty state object, it is a non-controller, which must be able to communicate with other controllers using a messagingSystem, but should not inherit from the BaseControllerV2 class.

Explanation

This commit introduces and applies the messenger pattern to the AssetsContractController class:

Convert its public methods into messenger actions.
Define the AssetsContractControllerMessenger and associated types.
Remove elements associated with BaseControllerV1.
Avoid creating a new inheritance relationship with BaseControllerV2.

The newly defined messenger actions/events are applied downstream in NftController and TokenBalancesController.

References

Closes Convert AssetsContractController from BaseControllerV1 to BaseControllerV2 #4072

Changelog

`@metamask/assets-controllers`

Added

BREAKING: TokenBalancesControllerMessenger must allow the AssetsContractController:getERC20BalanceOf action in addition to its previous allowed actions. (#4397)
BREAKING: NftControllerMessenger must allow the following actions in addition to its previous allowed actions: AssetsContractController:getERC721AssetName, AssetsContractController:getERC721AssetSymbol, AssetsContractController:getERC721TokenURI, AssetsContractController:getERC721OwnerOf, AssetsContractController:getERC1155BalanceOf, AssetsContractController:getERC1155TokenURI. (#4397)
BREAKING: Add elements to the AssetsContractController class: (#4397)
- BREAKING: Add required constructor option messenger.
- Add class field messagingSystem.
- Add getters for ipfsGateway and chainId. As corresponding setters have not been defined, these properties are not externally mutable.
Add and export the AssetsContractControllerMessenger type (#4397)
- AssetsContractControllerMessenger must allow the external actions NetworkController:getNetworkClientById, NetworkController:getNetworkConfigurationByNetworkClientId, NetworkController:getSelectedNetworkClient, NetworkController:getState.
- AssetsContractControllerMessenger must allow the external events PreferencesController:stateChange, NetworkController:networkDidChange.
Add and export new types: AssetsContractControllerActions, AssetsContractControllerEvents, AssetsContractControllerGetERC20StandardAction, AssetsContractControllerGetERC721StandardAction, AssetsContractControllerGetERC1155StandardAction, AssetsContractControllerGetERC20BalanceOfAction, AssetsContractControllerGetERC20TokenDecimalsAction, AssetsContractControllerGetERC20TokenNameAction, AssetsContractControllerGetERC721NftTokenIdAction, AssetsContractControllerGetERC721TokenURIAction, AssetsContractControllerGetERC721AssetNameAction, AssetsContractControllerGetERC721AssetSymbolAction, AssetsContractControllerGetERC721OwnerOfAction, AssetsContractControllerGetERC1155TokenURIAction, AssetsContractControllerGetERC1155BalanceOfAction, AssetsContractControllerTransferSingleERC1155Action, AssetsContractControllerGetTokenStandardAndDetailsAction, AssetsContractControllerGetBalancesInSingleCallAction. (#4397)
Add a new setProvider method to AssetsContractController. (#4397)
- Replaces the removed provider setter method, and widens the provider function parameter type from Provider to Provider | undefined.

Changed

BREAKING: The type of SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID is narrowed from Record<Hex, string> to the const-asserted literal properties of the SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID object. (#4397)
- The index signature is restricted to the union of the enum keys of SupportedTokenDetectionNetworks.
- The property value type is restricted to the type union of the addresses defined in the object.
- The object type is constrained by Record<Hex, string> using the satisfies keyword.
BREAKING: Convert the BalanceMap type from an interface into a type alias. (#4397)
- Type aliases have an index signature of string by default, and are compatible with the StateConstraint type defined in the @metamask/base-controller package.

Removed

BREAKING: Remove elements from the AssetsContractController class: (#4397)
- BREAKING: AssetsContractController no longer inherits from BaseControllerV1.
- BREAKING: Remove constructor option callbacks onPreferencesStateChange, onNetworkDidChange, getNetworkClientById, and replace with corresponding messenger actions and events.
- BREAKING: Remove class fields: name, config (along with its properties provider, ipfsGateway, chainId).
- BREAKING: Remove methods: getProvider, getChainId.
- BREAKING: Remove the provider setter method.
BREAKING: Remove the getERC20BalanceOf constructor option callback from the TokenBalancesControllerOptions type and the TokenBalancesController constructor. (#4397)
BREAKING: Remove NftController constructor option callbacks: getERC721AssetName, getERC721AssetSymbol, getERC721TokenURI, getERC721OwnerOf, getERC1155BalanceOf, getERC1155TokenURI. (#4397)
BREAKING: Remove the AssetsContractConfig type. (#4397)
BREAKING: Remove export for MISSING_PROVIDER_ERROR. (#4397)

Fixed

BREAKING: Convert the getERC721NftTokenId method of the AssetsContractController into an async function. (#4397)

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

MajorLift

Notes for reviewers - AssetsContractController:

MajorLift · 2024-07-16T17:03:57Z

packages/assets-controllers/src/AssetsContractController.ts

-}
+};
+
+const name = 'AssetsContractController';


Left as name instead of updating to controllerName in anticipation of the class being renamed as a non-controller class.

packages/assets-controllers/src/AssetsContractController.ts

MajorLift · 2024-07-16T17:04:37Z

packages/assets-controllers/src/AssetsContractController.ts

+  get ipfsGateway() {
+    return this.#ipfsGateway;
+  }
+
+  get chainId() {
+    return this.#chainId;
+  }


Added getters for use in tests. Since these class fields are now private and have no setters, they are not externally mutable.

Hmm, why not make the properties public instead of adding getters?

I want these fields to be immutable from the outside (since they affect the result of methods e.g. #getCorrect{ChainId,Provider}), but freely assignable from inside of the same class. Private + getter seems to be the only straightforward way of achieving this.

Making them public and readonly doesn't work because it restricts assignments from internal class methods (i.e. this.chainId = chainId doesn't work if this.chainId is readonly).

MajorLift · 2024-07-16T17:04:42Z

packages/assets-controllers/src/AssetsContractController.ts

+  #getCorrectProvider(networkClientId?: NetworkClientId): Web3Provider {
    const provider = networkClientId
-      ? this.getNetworkClientById(networkClientId).provider
-      : this._provider;
+      ? this.messagingSystem.call(
+          `NetworkController:getNetworkClientById`,
+          networkClientId,
+        ).provider
+      : this.messagingSystem.call('NetworkController:getSelectedNetworkClient')
+          ?.provider ?? this.#provider;


Retrieves the currently selected provider instance even if the networkClientId argument is not provided (fixes breaking tests).

MajorLift · 2024-07-16T17:04:45Z

packages/assets-controllers/src/AssetsContractController.ts

+  #getCorrectChainId(networkClientId?: NetworkClientId): Hex {
+    if (networkClientId) {
+      const networkClientConfiguration = this.messagingSystem.call(
+        'NetworkController:getNetworkConfigurationByNetworkClientId',
+        networkClientId,
+      );
+      if (networkClientConfiguration) {
+        return networkClientConfiguration.chainId;
+      }
+    }
+    const { selectedNetworkClientId } = this.messagingSystem.call(
+      'NetworkController:getState',
+    );
+    const networkClient = this.messagingSystem.call(
+      'NetworkController:getNetworkClientById',
+      selectedNetworkClientId,
+    );
+    return networkClient.configuration?.chainId ?? this.#chainId;


Retrieves the currently selected chainId even if the networkClientId argument is not provided (fixes breaking tests).

MajorLift · 2024-07-16T17:04:50Z

packages/assets-controllers/src/AssetsContractController.ts

@@ -25,7 +30,7 @@ import { ERC721Standard } from './Standards/NftStandards/ERC721/ERC721Standard';
 * @param chainId - ChainID of network
 * @returns Whether the current network supports token detection
 */
-export const SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID: Record<Hex, string> = {
+export const SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID = {


Narrows type to const-asserted literal properties. See changelog "Changed" section for details.

MajorLift · 2024-07-16T17:04:53Z

packages/assets-controllers/src/AssetsContractController.ts

+    if (
+      !((id): id is keyof typeof SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID =>
+        id in SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID)(chainId)
+    ) {


Type guard narrows chainId to the index signature of SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID. Without this, attempting to use chainId as a property accessor for SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID will result in the following error:

Element implicitly has an 'any' type because expression of type '`0x${string}`' can't be used to index type '{ readonly "0x1": "0xb1f8e55c7f64d203c1400b9d8555d050f94adf39"; readonly "0x38": "0x2352c63A83f9Fd126af8676146721Fa00924d7e4"; readonly "0x89": "0x2352c63A83f9Fd126af8676146721Fa00924d7e4"; ... 14 more ...; readonly "0x505": "0x6aa75276052d96696134252587894ef5ffa520af"; }'.ts(7053)

Hmm. The hasProperty function from @metamask/utils is supposed to get around this. Strange.

It looks like hasProperty only serves as a type guard for objectToCheck, and not the property key. The adhoc type guard here narrows chainId, not SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID.

hasProperty(SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID, chainId) does not is-assert that chainId is any narrower than Hex.

Er, right. That makes sense.

I feel like there must be a more concise way to have TypeScript infer that id is keyof typeof SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID. But my brain isn't working right now, and this seems readable enough.

Yes it seems like just the in check should be sufficient to narrow chainId down to keyof.

It looks like this update in v5.5 microsoft/TypeScript#57847 might be related.

Hmm! Nice, I'll follow that PR.

MajorLift · 2024-07-16T17:06:16Z

packages/assets-controllers/src/AssetsContractController.ts

   *
-   * TODO: Replace this wth a method.
-   *
-   * @property provider - Provider used to create a new underlying Web3 instance
+   * @param provider - Provider used to create a new underlying Web3 instance
   */
-  set provider(provider: Provider) {
-    this._provider = provider;
+  setProvider(provider: Provider | undefined) {
+    this.#provider = provider;
  }



Replaces provider setter. Allows undefined literal to be passed in and assigned, which is a frequent pattern in the tests.

MajorLift

Notes for reviewers - tests:

MajorLift · 2024-07-16T17:35:53Z

packages/assets-controllers/src/AssetsContractController.test.ts

@@ -56,31 +61,39 @@ const TEST_ACCOUNT_PUBLIC_ADDRESS =
 */
 async function setupAssetContractControllers({
  options,
-  useNetworkControllerProvider,
+  useNetworkControllerProvider = false,


Previously, the controller was relying on useNetworkControllerProvider being evaluated as a falsy value when set to undefined.

MajorLift · 2024-07-16T17:35:57Z

packages/assets-controllers/src/AssetsContractController.test.ts

+  options?: Partial<
+    Omit<ConstructorParameters<typeof AssetsContractController>[0], 'messenger'>
+  >;


All constructor options except for messenger, which is defined by this function.

MajorLift · 2024-07-16T17:36:00Z

packages/assets-controllers/src/AssetsContractController.test.ts

+  const controllerMessenger = new ControllerMessenger<
+    | NetworkControllerActions
+    | AssetsContractControllerActions
+    | AssetsContractAllowedActions,
+    | NetworkControllerEvents
+    | AssetsContractControllerEvents
+    | AssetsContractAllowedEvents
+  >();


The function is updated to use a shared, unrestricted controller messenger for communications between NetworkController and AssetsContractController.

MajorLift · 2024-07-16T17:36:05Z

packages/assets-controllers/src/AssetsContractControllerWithNetworkClientId.test.ts

+    ).rejects.toThrow(
+      `No custom network client was found with the ID "invalidNetworkClientId".`,
+    );
+    messenger.clearEventSubscriptions('NetworkController:networkDidChange');


Fix outdated reference to NetworkController:stateChange.

MajorLift · 2024-07-16T17:36:27Z

packages/assets-controllers/src/NftController.test.ts

      const callActionSpy = jest
        .spyOn(messenger, 'call')
+        // 1. `AccountsController:getAccount`
        .mockReturnValueOnce(OWNER_ACCOUNT)
+        // 2. `AssetsContractController:getERC721OwnerOf`
+        .mockResolvedValueOnce(OWNER_ADDRESS)
+        // 3. `AssetsContractController:getERC721TokenURI`
+        .mockResolvedValueOnce('https://testtokenuri.com')
+        // 4. `ApprovalController:addRequest`
        .mockResolvedValueOnce({})
-        .mockReturnValueOnce(OWNER_ACCOUNT);
+        // 5. `AccountsController:getAccount`
+        .mockReturnValueOnce(OWNER_ACCOUNT)
+        // 6. `AssetsContractController:getERC721AssetName`
+        .mockResolvedValueOnce('testERC721Name')
+        // 7. `AssetsContractController:getERC721AssetSymbol`
+        .mockResolvedValueOnce('testERC721Symbol');

      await nftController.watchNft(ERC721_NFT, ERC721, 'https://test-dapp.com');
-      // First call is getInternalAccount. Second call is the approval request.
-      expect(callActionSpy).toHaveBeenCalledTimes(3);
+      expect(callActionSpy).toHaveBeenCalledTimes(7);
      expect(callActionSpy).toHaveBeenNthCalledWith(
-        2,
+        4,
        'ApprovalController:addRequest',


This is the flow of NftController method calls and messenger call invocations triggered by a watchNft call on a ERC721 asset.

- `#getAddressOrSelectedAddress` 1. `AccountsController:getAccount` - `#validateWatchNft` > `isNftOwner` 2. `AssetsContractController:getERC721OwnerOf` - `#getNftInformation` > `#getNftInformationFromTokenURI` > `#getNftURIAndStandard` 3. `AssetsContractController:getERC721TokenURI` - `_requestApproval` 4. `ApprovalController:addRequest` - `addNft` > `#getAddressOrSelectedAddress` 5. `AccountsController:getAccount` - `#addNftContract` > `#getNftContractInformation` > `#getNftContractInformationFromContract` 6. `AssetsContractController:getERC721AssetName` 7. `AssetsContractController:getERC721AssetSymbol`

MajorLift · 2024-07-16T17:36:33Z

packages/assets-controllers/src/NftController.test.ts

      const callActionSpy = jest
        .spyOn(messenger, 'call')
+        // 1. `AccountsController:getAccount`
        .mockReturnValueOnce(OWNER_ACCOUNT)
+        // 2. `AssetsContractController:getERC721OwnerOf`
+        .mockRejectedValueOnce(new Error('Not an ERC721 contract'))
+        // 3. `AssetsContractController:getERC1155BalanceOf`
+        .mockResolvedValueOnce(new BN(1))
+        // 4. `AssetsContractController:getERC721TokenURI`
+        .mockRejectedValueOnce(new Error('Not an ERC721 contract'))
+        // 5. `AssetsContractController:getERC1155TokenURI`
+        .mockResolvedValueOnce('https://testtokenuri.com')
+        // 6. `ApprovalController:addRequest`
        .mockResolvedValueOnce({})
-        .mockReturnValueOnce(OWNER_ACCOUNT);
+        // 7. `AccountsController:getAccount`
+        .mockReturnValueOnce(OWNER_ACCOUNT)
+        // 8. `AssetsContractController:getERC1155AssetName`
+        .mockResolvedValueOnce('testERC1155Name')
+        // 9. `AssetsContractController:getERC1155AssetSymbol`
+        .mockResolvedValueOnce('testERC1155Symbol');

      await nftController.watchNft(
        ERC1155_NFT,
        ERC1155,
        'https://etherscan.io',
      );
-      // First call is getInternalAccount. Second call is the approval request.
-      expect(callActionSpy).toHaveBeenCalledTimes(3);
+      expect(callActionSpy).toHaveBeenCalledTimes(9);
      expect(callActionSpy).toHaveBeenNthCalledWith(
-        2,
+        6,
        'ApprovalController:addRequest',


This is the flow of NftController method calls and messenger call invocations triggered by a watchNft call on a ERC1155 asset.

- `#getAddressOrSelectedAddress` 1. `AccountsController:getAccount` - `#validateWatchNft` > `isNftOwner` 2. `AssetsContractController:getERC721OwnerOf` (rejects) 3. `AssetsContractController:getERC1155BalanceOf` - `#getNftInformation` > `#getNftInformationFromTokenURI` > `#getNftURIAndStandard` 4. `AssetsContractController:getERC721TokenURI` (rejects) 5. `AssetsContractController:getERC1155TokenURI` - `_requestApproval` 6. `ApprovalController:addRequest` - `addNft` > `#getAddressOrSelectedAddress` 7. `AccountsController:getAccount` - `#addNftContract` > `#getNftContractInformation` > `#getNftContractInformationFromContract` 8. `AssetsContractController:getERC721AssetName` (rejects) 9. `AssetsContractController:getERC721AssetSymbol` (rejects)

MajorLift · 2024-07-16T17:36:37Z

packages/assets-controllers/src/NftController.test.ts

+              description: 'testERC721Description',
+              image: 'testERC721Image',
+              name: 'testERC721Name',


The result for this test changed with all messenger calls being fully mocked. Are we perhaps now getting the correct result, or is this unexpected behavior?

MajorLift · 2024-07-16T17:36:43Z

packages/assets-controllers/src/NftController.test.ts

+        getERC721TokenURI: jest
+          .fn()
+          .mockImplementation(() => 'https://testtokenuri.com'),
+        getERC721OwnerOf: jest.fn().mockImplementation(() => OWNER_ADDRESS),


These mocks are redundant with the return and resolved values being fully mocked for the callActionSpy. Should we rely on fully mocking the action handlers instead?

- Restore usage of `Object.getPrototypeOf` - add `setProvider` to exceptions

MajorLift · 2024-07-16T17:51:26Z

packages/assets-controllers/src/AssetsContractController.ts

+/**
+ * A utility type that derives the public method names of a given messenger consumer class,
+ * and uses it to generate the class's internal messenger action types.
+ * @template Controller - A messenger consumer class.
+ */
+// TODO: Figure out generic constraint and move to base-controller
+type ControllerActionsMap<Controller> = {
+  [ClassMethod in keyof Controller as Controller[ClassMethod] extends ActionConstraint['handler']
+    ? ClassMethod
+    : never]: {
+    type: `${typeof name}:${ClassMethod & string}`;
+    handler: Controller[ClassMethod];
+  };
+};
+
+type AssetsContractControllerActionsMap =
+  ControllerActionsMap<AssetsContractController>;
+
+/**
+ * The union of all public class method names of {@link AssetsContractController}.
+ */
+type AssetsContractControllerMethodName =
+  keyof AssetsContractControllerActionsMap;
+
+/**
+ * The union of all internal messenger actions available to the {@link AssetsContractControllerMessenger}.
+ */
+export type AssetsContractControllerActions =
+  AssetsContractControllerActionsMap[AssetsContractControllerMethodName];


Proof of concept for new base-controller utility type that should be useful when creating new controllers, or adding a large number of new actions/methods.

MajorLift · 2024-07-16T17:51:29Z

packages/assets-controllers/src/AssetsContractController.ts

+  // TODO: Expand into base-controller utility function that batch registers action handlers.
+  #registerActionHandlers() {
+    const nonMethodClassProperties = [
+      'constructor',
+      'messagingSystem',
+      'setProvider',
+      'provider',
+      'ipfsGateway',
+      'chainId',
+    ];
+
+    getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(
+      (method) => {
+        if (
+          ((key: keyof this): key is AssetsContractControllerMethodName =>
+            !nonMethodClassProperties.find((e) => e === key) &&
+            typeof this[key] === 'function')(method)
+        ) {
+          this.messagingSystem.registerActionHandler(
+            `${name}:${method}`,
+            // TODO: Write a generic for-loop implementation that iterates over an input union type in tandem with the input array.
+            // @ts-expect-error Both assigned argument and assignee parameter are using the entire union type for `method` instead of the type for the current element
+            this[method].bind(this),
+          );
+        }
+      },
+    );
+  }


Proof of concept for new base-controller utility function that can batch register action handlers to a given messenger.

mcmire

Makes sense overall. Just had some comments/suggestions.

mcmire · 2024-07-16T22:47:58Z

packages/assets-controllers/src/AssetsContractController.ts

+  };
+};
+
+type AssetsContractControllerActionsMap =


Food for thought: I'm curious whether in the future we should deprecate separate types for actions in favor of one type that holds all actions, like this. For cases like this it seems like it would save time for both the author (because we wouldn't have to repeat the name of the action in the name of the type) and the maintainer (because we wouldn't have to check that names of types have the correct form).

One overkill solution we could try is to use the TypeScript compiler API. We could define a function that would auto-generate the individual types by manipulating the AST directly, and then output a module containing those types.

But I agree that consolidating the type exports into lookup tables would be just as effective and much simpler.

mcmire · 2024-07-16T22:54:36Z

packages/assets-controllers/src/AssetsContractController.ts

+    const nonMethodClassProperties = [
+      'constructor',
+      'messagingSystem',
+      'setProvider',
+      'provider',
+      'ipfsGateway',
+      'chainId',
+    ];
+
+    getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(
+      (method) => {
+        if (
+          ((key: keyof this): key is AssetsContractControllerMethodName =>
+            !nonMethodClassProperties.find((e) => e === key) &&


Doesn't the typeof this[key] === 'function' check already exclude non-methods? Would it be enough to just exclude setProvider here?

Suggested change

const nonMethodClassProperties = [

'constructor',

'messagingSystem',

'setProvider',

'provider',

'ipfsGateway',

'chainId',

];

getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(

(method) => {

if (

((key: keyof this): key is AssetsContractControllerMethodName =>

!nonMethodClassProperties.find((e) => e === key) &&

const methodsExcludedFromMessenger = [

'setProvider',

];

getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(

(method) => {

if (

((key: keyof this): key is AssetsContractControllerMethodName =>

!methodsExcludedFromMessenger.find((e) => e === key) &&

Alternatively, why exclude setProvider? Would it be easier to assume that all methods will become actions?

Suggested change

const nonMethodClassProperties = [

'constructor',

'messagingSystem',

'setProvider',

'provider',

'ipfsGateway',

'chainId',

];

getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(

(method) => {

if (

((key: keyof this): key is AssetsContractControllerMethodName =>

!nonMethodClassProperties.find((e) => e === key) &&

getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(

(method) => {

if (

((key: keyof this): key is AssetsContractControllerMethodName =>

Also why use Object.getPrototypeOf(this), why not use this directly?

Suggested change

const nonMethodClassProperties = [

'constructor',

'messagingSystem',

'setProvider',

'provider',

'ipfsGateway',

'chainId',

];

getKnownPropertyNames<keyof this>(Object.getPrototypeOf(this)).forEach(

(method) => {

if (

((key: keyof this): key is AssetsContractControllerMethodName =>

!nonMethodClassProperties.find((e) => e === key) &&

getKnownPropertyNames<keyof this>(this).forEach(

(method) => {

if (

((key: keyof this): key is AssetsContractControllerMethodName =>

Doesn't the typeof this[key] === 'function' check already exclude non-methods? Would it be enough to just exclude setProvider here?

All public methods that are not actions should be explicitly excluded here. Otherwise, the forEach loop will register actions like AssetsContractController:constructor. While in TypeScript environments our types will prevent these from being used, in downstream JavaScript environments, these actions will be accessible at runtime.

This is both an unintended and undesirable extension of the messenger's API, as well as a potential security risk in the form of a ready-made dynamic property access vulnerability (see #4041 -- especially constructor key as attack vector), since the action handler this['constructor'].bind(this) is bound to the class instance in use.

That said, the variable name methodsExcludedFromMessenger is much clearer. Renamed here: f50e5f4

Also why use Object.getPrototypeOf(this), why not use this directly?

This results in errors (e.g "A handler for AssetsContractController:getERC1155BalanceOf has not been registered") showing that none of the methods are being registered as action handlers.

This is because public class methods are defined as sub-properties of the class's prototype property, not top-level properties of the class itself, meaning we need to iterate over the prototype object here, not this.

This is because public class methods are defined as sub-properties of the class's prototype property, not top-level properties of the class itself, meaning we need to iterate over the prototype object, not this.

Oh, I forgot that getKnownPropertyNames doesn't loop over all properties of an object, it only loops over owned properties. Okay, that makes sense now.

mcmire · 2024-07-16T23:12:18Z

packages/assets-controllers/src/AssetsContractController.ts

+  get ipfsGateway() {
+    return this.#ipfsGateway;
+  }
+
+  get chainId() {
+    return this.#chainId;
+  }


Hmm, why not make the properties public instead of adding getters?

mcmire · 2024-07-16T23:14:18Z

packages/assets-controllers/src/AssetsContractController.ts

  /**
   * Get the relevant provider instance.
   *
   * @param networkClientId - Network Client ID.
   * @returns Web3Provider instance.
   */
-  getProvider(networkClientId?: NetworkClientId): Web3Provider {


Is it necessary to make this method private and rename it in this PR? It's weird that it exists, but it seems separate from making this class a non-controller.

Since we're introducing the messagingSystem into this class, it seems reasonable to enforce a norm that public methods should be messenger actions in principle (except for constructor, getters/setters etc.). The implementation of ControllerActionsMap and #registerActionHandlers (at least in their current, simple form) also rely on this norm being in place.

Gotcha, makes sense.

packages/assets-controllers/src/AssetsContractController.ts

mcmire · 2024-07-16T23:20:11Z

packages/assets-controllers/src/AssetsContractController.ts

+    if (
+      !((id): id is keyof typeof SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID =>
+        id in SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID)(chainId)
+    ) {


Hmm. The hasProperty function from @metamask/utils is supposed to get around this. Strange.

packages/assets-controllers/src/AssetsContractController.ts

mcmire · 2024-07-17T15:40:22Z

packages/assets-controllers/src/AssetsContractController.test.ts

    await expect(
-      assetsContract.getERC20BalanceOf(
+      messenger.call(


I see that you've changed all of the instances where we are calling a method on the class to call the corresponding messenger action. That makes sense — but it does create some inconsistency with the guidance we have for writing tests. The describes for these tests use the name of the method, but here we are calling the messenger action. I worry that could be a bit confusing when reading these tests.

Do we want to change the describes, or how do you think is the best way to resolve this discrepancy?

Hmm a little confused about this. It looks like describe is only used once at the top level, and the it descriptions don't use method names directly in all affected test files except NftController.test.ts? For NftController it seems like the methods are being tested directly.

Ah, you're right. We don't follow that describe convention here anyway.

Then I guess I will ask a slightly different question. Fast-forwarding, if we were to automatically register messenger actions for all public methods in all controllers, how should we go about writing tests? Would we ensure that the tests that currently center around method now center around the messenger actions (i.e. instead of calling the method we now call the messenger action in the test)? Would we test both the method and the messenger action by wrapping existing tests for a public method in a loop so that we can easily run the same tests for that method and the messenger action without having to copy those tests?

I wonder if we could omit method tests if a messenger action handler has a single corresponding class method because it is defined using this[method].bind(this)?

If we're only testing one of the two, it should be safer to test the messenger action, since the constraints for setting up a messenger action call (e.g. instantiating messenger and enumerating available actions) is a superset of the same for invoking its corresponding class method.

Otherwise, the messenger action and the method should be equivalent, but to ensure that they don't become inconsistent due to code drift, we could add an expect call to these tests verifying that the action handler is a bound class method with no additional logic.

For other cases where a method has no corresponding messenger action (e.g. NftController.watchNft), or a messenger action handler is not directly derived from a class method (e.g. ApprovalController:addRequest), we will need to test, respectively, only the method, or both the method and the messenger separately.

What are your thoughts on testing only the messenger actions where the action and the method are equivalent? I feel like I might be missing some of the implications here e.g. how would this affect coverage?

Testing just the messenger in the cases where it's a bound method could work. The coverage tool would still be happy because we would effectively be killing two birds with one stone. The one thing I'm interested to know is whether it is truly possible to test that the messenger action handler is equivalent to the bound version of the method. I found a Stack Overflow answer here but it seems a bit too good to be true. But we could definitely try it out!

Nice that's our exact use case and it looks like it's a simple solution. We should definitely try applying this.

Co-authored-by: Elliot Winkler <ellliot.winkler@gmail.com>

mcmire

Had one more suggestion, and I still have an open question about the tests. But once we resolve those then I should be good.

I know we talked about this before but I'm a bit hesitant about the keeping the existing name "AssetsContractController". It would probably mess up the diff to rename it considering how many changes there are, so maybe that's something we can do in a separate PR. But my thought here is that if we have something that's not a controller, it seems confusing to name it with a "Controller" suffix. So, I'll think about some alternate names.

mcmire · 2024-07-17T20:17:49Z

packages/assets-controllers/src/AssetsContractController.test.ts

+  const controllerMessenger = new ControllerMessenger<
+    | NetworkControllerActions
+    | AssetsContractControllerActions
+    | AssetsContractAllowedActions,
+    | NetworkControllerEvents
+    | AssetsContractControllerEvents
+    | AssetsContractAllowedEvents
+  >();


I believe we can do this more easily without having to export AllowedActions and AllowedEvents now. A while back I added a couple of utility types for extracting the action and event types from a controller messenger type. So what if we say this instead:

Suggested change

const controllerMessenger = new ControllerMessenger<

| NetworkControllerActions

| AssetsContractControllerActions

| AssetsContractAllowedActions,

| NetworkControllerEvents

| AssetsContractControllerEvents

| AssetsContractAllowedEvents

>();

const controllerMessenger = new ControllerMessenger<

| ExtractAvailableAction<AssetsContractControllerMessenger>

| NetworkControllerActions,

| ExtractAvailableEvent<AssetsContractControllerMessenger>

| NetworkControllerEvents

>();

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

mcmire

This looks great. Very thorough!

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 2 times, most recently from e019088 to 6ea5485 Compare June 10, 2024 18:12

MajorLift self-assigned this Jun 10, 2024

MajorLift added the team-wallet-framework label Jun 10, 2024

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 6 times, most recently from 9d30de1 to ad69fb3 Compare June 10, 2024 21:32

desi removed the team-wallet-framework label Jun 13, 2024

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 4 times, most recently from 64095c7 to a1d5fa9 Compare June 18, 2024 18:27

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 4 times, most recently from 5da97d5 to 871960d Compare June 26, 2024 21:26

MajorLift added the team-wallet-framework label Jun 26, 2024

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 3 times, most recently from ee38d05 to e9f1916 Compare July 3, 2024 03:12

MajorLift changed the title ~~[assets-controllers] Convert AssetsContractController from BaseControllerV1 to service class utilizing messenger pattern~~ [AssetsContractController] Remove inheritance from BaseControllerV1 and convert to use messenger pattern Jul 8, 2024

MajorLift changed the title ~~[AssetsContractController] Remove inheritance from BaseControllerV1 and convert to use messenger pattern~~ [assets-contract-controller] Remove inheritance from BaseControllerV1 and convert to use messenger pattern Jul 8, 2024

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 2 times, most recently from 4c6b87d to b54d0bb Compare July 10, 2024 17:50

MajorLift requested a review from a team July 16, 2024 01:56

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 2 times, most recently from 81fcaf2 to 57a71ae Compare July 16, 2024 05:45

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch from 774741b to 99fc5b9 Compare July 16, 2024 16:04

Convert provider setter into setProvider method

6c6f6e5

MajorLift commented Jul 16, 2024

View reviewed changes

Add jsdoc for AssetsContractController types

20e5451

MajorLift commented Jul 16, 2024

View reviewed changes

Fix #registerActionHandlers

1d5ea66

- Restore usage of `Object.getPrototypeOf` - add `setProvider` to exceptions

MajorLift commented Jul 16, 2024

View reviewed changes

test(nft-controller): fix messenger call mocks

236844a

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch 2 times, most recently from fd1e1d1 to b758d93 Compare July 16, 2024 22:48

MajorLift added 2 commits July 16, 2024 22:49

test(nft-controller): expose mock constructors for allowedActions

a40f0d1

Merge branch 'main' into 240610-upgrade-AssetsContractController

161b7ce

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch from 96db928 to 161b7ce Compare July 17, 2024 02:50

mcmire reviewed Jul 17, 2024

View reviewed changes

MajorLift and others added 2 commits July 17, 2024 12:48

Rename variable for clarity

f50e5f4

Co-authored-by: Elliot Winkler <ellliot.winkler@gmail.com>

Remove getter for provider

418df79

MajorLift requested a review from a team July 17, 2024 17:19

Merge branch 'main' into 240610-upgrade-AssetsContractController

c35943c

mcmire reviewed Jul 17, 2024

View reviewed changes

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch from ab6681e to d488ce8 Compare July 17, 2024 20:53

Apply ExtractAvailable{Action,Event} test utility types

49831d9

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

MajorLift force-pushed the 240610-upgrade-AssetsContractController branch from d488ce8 to 49831d9 Compare July 17, 2024 20:53

MajorLift requested a review from a team July 18, 2024 12:21

Merge branch 'main' into 240610-upgrade-AssetsContractController

e6324d9

mcmire approved these changes Jul 19, 2024

View reviewed changes

Merge branch 'main' into 240610-upgrade-AssetsContractController

b496b69

MajorLift merged commit 4485e8a into main Jul 19, 2024
116 checks passed

MajorLift deleted the 240610-upgrade-AssetsContractController branch July 19, 2024 22:42

MajorLift mentioned this pull request Aug 2, 2024

Bump typescript from ~5.4.5 to ~5.5.4 #4588

Draft

3 tasks

[assets-contract-controller] Apply messenger pattern and remove inheritance from BaseControllerV1 #4397

[assets-contract-controller] Apply messenger pattern and remove inheritance from BaseControllerV1 #4397

Conversation

MajorLift commented Jun 10, 2024 • edited Loading

Motivation

Explanation

References

Changelog

@metamask/assets-controllers

Added

Changed

Removed

Fixed

Checklist

MajorLift left a comment

Choose a reason for hiding this comment

MajorLift Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

MajorLift Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

[assets-contract-controller] Apply messenger pattern and remove inheritance from `BaseControllerV1` #4397

[assets-contract-controller] Apply messenger pattern and remove inheritance from `BaseControllerV1` #4397

MajorLift commented Jun 10, 2024 •

edited

Loading

`@metamask/assets-controllers`

MajorLift Jul 16, 2024 •

edited

Loading

MajorLift Jul 17, 2024 •

edited

Loading

MajorLift Jul 17, 2024 •

edited

Loading

MajorLift left a comment •

edited

Loading

MajorLift Jul 16, 2024 •

edited

Loading

MajorLift Jul 16, 2024 •

edited

Loading

MajorLift Jul 17, 2024 •

edited

Loading

MajorLift Jul 17, 2024 •

edited

Loading

MajorLift Jul 17, 2024 •

edited

Loading

mcmire left a comment •

edited

Loading