upgrade TokenRatesController to BaseControllerV2 #4314

kanthesha · 2024-05-23T20:45:36Z

Explanation

In this, the TokenRatesController has been upgraded to BaseControllerV2. The upgrade includes TokenRatesController inheriting BaseController v2 instead of BaseController v1. This affects the constructor and the also the way state is updated inside the controller, but it also prompts other changes:

This also includes the changes to the unit tests, so that all the tests uses withController pattern.

References

Fixes #4076

Changelog

@metamask/assets-controller

Added

New types for TokenRatesController messenger actions
- TokenRatesControllerGetStateAction
New types for TokenRatesController messenger events
- TokenRatesControllerStateChangeEvent

Changed

BREAKING: Changed superclass of TokenRatesController from BaseController v1 to BaseController v2
BREAKING: Renamed TokenRatesState to TokenRatesControllerState

Removed

BREAKING: Removed TokenRatesConfig type

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

kanthesha · 2024-05-23T21:25:56Z

packages/assets-controllers/src/TokenRatesController.ts


 // This interface was created before this ESLint rule was added.
 // Convert to a `type` in a future major version.
 // eslint-disable-next-line @typescript-eslint/consistent-type-definitions
 export interface ContractExchangeRates {
-  [address: string]: number | undefined;
+  [address: string]: number;


undefined is removed, because it is not a json type! We need to fix on the consumer side, in case this will cause an error, where it is consumed.

We could either delete keys that have an undefined value, or accept null instead of undefined

also you can consider update interface to type

mikesposito · 2024-05-29T11:25:25Z

packages/assets-controllers/src/TokenRatesController.ts

+    currentChainId,
+    currentTicker,
+    currentAddress,


Nit: Would it be possible to substitute these with messenger calls? I believe we can obtain the current chain id from NetworkController, and the current selected address from AccountsController

Make sense.
I've removed the currentChainId, currentTicker and currentAddress and now currentChainId and currentTicker are assigned form messenger network state and selectedAddress from PreferencesController state.

mikesposito · 2024-05-29T11:26:51Z

packages/assets-controllers/src/TokenRatesController.ts

+    this.#interval = interval;
+    this.#chainId = currentChainId;
+    this.#ticker = currentTicker;
+    this.#selectedAddress = currentAddress;


Having the selected address as a class variable here means that we also need to keep this in sync. Perhaps it would be better to always take the fresh value from AccountsController?

Make sense.
TokenRatesController uses PreferencesController state to manage the selectedAddress. So for now, initial selected address will be assigned from the same to be consistent. But I believe that the selectedAddress has to be pointing to AccountsController rather than PreferencesController. I'll open up a separate ticket to make those changes.

It looks like using AccountsController to get the selected address will be addressed in #4219.

mikesposito · 2024-05-29T11:27:34Z

packages/assets-controllers/src/TokenRatesController.ts

    this.#tokenPricesService = tokenPricesService;
+    this.#disabled = disabled;
+    this.#interval = interval;
+    this.#chainId = currentChainId;


Similarly to #selectedAddress, would it be easier to take this value from NetworkController instead of storing it in a class variable?

I think, rather than referencing outside variables each time when we need selectedAddress within the class, accessing private class / instance variable will be easier and faster.

And I think, global truth for selectedAddress has to be from AccountsController.

This pattern seems okay to me. In fact it may be advantageous to use private properties to cache values like this to mitigate synchronization issues. Like, if we have to pull a bunch of values from a bunch of controllers at the start of a polling iteration it might be better to do that all in one go and then cache those values rather than fetch them at various points during that iteration (which is what we'd have to do if we didn't use private properties, I believe).

kanthesha · 2024-06-04T14:58:07Z

Now the default value for allTokens and allDetectedTokens has been assigned from TokensController:getState (messaging system).

…oller-v2

cryptodev-2s · 2024-05-28T12:06:10Z

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

-
-            // @ts-expect-error Intentionally incomplete state
-            controllerEvents.tokensStateChange({
+            triggerTokensStateChange({


in this test we want to Intentionally test incomplete state ? in this case we shouldn't be adding getDefaultTokensState ? same for the above test where it has been removed ?

I think, the messenger publish expects complete state of the TokensControllerState!

cryptodev-2s · 2024-06-05T15:41:43Z

packages/assets-controllers/src/TokenRatesController.ts

@@ -152,13 +183,25 @@ async function getCurrencyConversionRate({
  }
 }

+const metadata = {


to align with other controllers this can be tokenRatesControllerMetadata

packages/assets-controllers/src/index.ts

cryptodev-2s · 2024-06-05T15:43:20Z

packages/assets-controllers/src/TokenRatesController.ts


 // This interface was created before this ESLint rule was added.
 // Convert to a `type` in a future major version.
 // eslint-disable-next-line @typescript-eslint/consistent-type-definitions
 export interface ContractExchangeRates {
-  [address: string]: number | undefined;
+  [address: string]: number;


also you can consider update interface to type

cryptodev-2s · 2024-06-05T15:44:02Z

packages/assets-controllers/src/TokenRatesController.ts

@@ -111,9 +120,31 @@ enum PollState {
 // This interface was created before this ESLint rule was added.
 // Convert to a `type` in a future major version.
 // eslint-disable-next-line @typescript-eslint/consistent-type-definitions


the eslint disable can be removed?

cryptodev-2s · 2024-06-05T15:44:30Z

packages/assets-controllers/src/TokenRatesController.ts

+  AllowedEvents['type']
+>;
+
+export type TokenRatesControllerStateChangeEvent = ControllerStateChangeEvent<


maybe this should be above TokenRatesControllerMessenger

packages/assets-controllers/src/TokenRatesController.ts

cryptodev-2s · 2024-06-05T15:55:06Z

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

-        chainId: '0x1',
-        selectedAddress: defaultSelectedAddress,
+    it('should set default state', async () => {
+      await withController({}, async ({ controller }) => {


can be await withController(async ({ controller }) => {

packages/assets-controllers/src/TokenRatesController.ts

mcmire · 2024-06-05T17:58:56Z

packages/assets-controllers/src/TokenRatesController.ts

+    this.#interval = interval;
+    this.#chainId = currentChainId;
+    this.#ticker = currentTicker;
+    this.#selectedAddress = currentAddress;


It looks like using AccountsController to get the selected address will be addressed in #4219.

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

mcmire · 2024-06-05T18:10:20Z

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

+        mockGetNetworkClientById.mockImplementation(handler);
+      },
+      callActionSpy,
+      triggerPreferencesStateChange: (state: PreferencesState) => {


Nit: What are your thoughts on having tests use the messenger directly to do whatever they want rather than wrapping each of the actions in a function? It is already pretty short to say messenger.publish('whatever') in a test, so it doesn't seem that we are saving that many characters here. Dropping these wrappers would also simplify withController so that it is more copy-and-pasteable across files.

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

packages/assets-controllers/src/TokenRatesController.ts

mcmire · 2024-06-05T19:13:21Z

Oof I didn't mean to request changes, sorry.

…oller-v2

packages/assets-controllers/src/index.ts

MajorLift · 2024-06-06T15:30:45Z

packages/assets-controllers/src/TokenRatesController.ts

+  TokensControllerGetStateAction,
+  TokensControllerStateChangeEvent,
+  TokensControllerState,
+} from './TokensController';

 /**
 * @type Token


Looks like we don't have jsdoc for aggregators, hasBalanceError, isErc721, name. This isn't due to any change introduced by this PR, but it would be nice to address this while we're doing this upgrade.

I have noticed many types are missing some properties definition, I don't know if can force this with an eslint rule

As a starting point:

* @property aggregators - An array containing the token's aggregators * @property hasBalanceError - 'true' if getting the balance of an ERC-20 token resulted in an error * @property isErc721 - 'true' if the token is an NFT * @property name - Name of the token

@cryptodev-2s I'm sure there are jsdoc eslint rules that are supposed to do that. I'll write up a ticket for enabling them.

It looks like we're moving away from jsdoc comments for types, since annotating each property individually gives the user better access to the comments, especially when hovering on the property.

(e.g. see https://github.com/MetaMask/core/pull/4286/files#diff-ea0aa37793484e278bd59970b7a6ce8380b75b723b34612081d402fb66261cfdR152-R196, and hover over any search result of "networkConfiguration." (networkConfiguration\. for regex) in the same file.

For functions, objects etc., we might still want to enforce jsdoc lint rules, so I'll still take a look into configuring this.

Adding an ESLint rule to check for JSDoc'ing properties would be great! I did some initial research into this a while back and didn't find any existing rules but maybe someone has come up with something by now.

…f github.com:MetaMask/core into upgrade-token-rates-controller-to-base-controller-v2

…oller-v2

cryptodev-2s · 2024-06-06T16:16:52Z

packages/assets-controllers/src/TokenRatesController.ts

 * @property image - Image of the token, url or bit32 image
+ * @property hasBalanceError - 'true' if there is an error while updating the token balance
+ * @property isERC721 - 'true' if the token is a ERC721 token
+ * @property name - Name of the token
 */



Nit:
we can remove this empty space

packages/assets-controllers/src/TokenRatesController.ts

mcmire

A few more suggestions but this is looking pretty good to me.

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

packages/assets-controllers/src/TokenRatesController.ts

Didn't mean to request changes :)

cryptodev-2s

LGTM!

mcmire

LGTM!

…oller-v2

kanthesha added 3 commits May 23, 2024 21:16

upgrade TokenRatesController to BaseControllerV2

39ea680

Rename TokenRatesState to TokenRatesControllerState

44d8c64

TokenRatesConfig removed and TokenRatesState renamed in index.ts

1868331

kanthesha marked this pull request as ready for review May 23, 2024 21:00

kanthesha requested a review from a team as a code owner May 23, 2024 21:00

desi requested a review from a team May 23, 2024 21:18

kanthesha commented May 23, 2024

View reviewed changes

controller state added back to constructor param

a701310

mcmire self-requested a review May 28, 2024 15:58

mikesposito reviewed May 29, 2024

View reviewed changes

kanthesha added 2 commits May 30, 2024 12:17

chain id and selected address are assigned from messenger system

d8e8c5e

resolving conflicts and fixing tests

d838c78

kanthesha added 4 commits June 4, 2024 16:51

clean-up: removed unwanted commented code

0a550b6

Merge branch 'main' into upgrade-token-rates-controller-to-base-contr…

d8f9059

…oller-v2

Merge branch 'main' into upgrade-token-rates-controller-to-base-contr…

7e43deb

…oller-v2

resolving conflicts and fixing tests

b715343

cryptodev-2s reviewed Jun 5, 2024

View reviewed changes

mcmire previously requested changes Jun 5, 2024

View reviewed changes

kanthesha added 2 commits June 6, 2024 12:27

added suggested changes

a3fdb9e

Merge branch 'main' into upgrade-token-rates-controller-to-base-contr…

09e8f90

…oller-v2

MajorLift reviewed Jun 6, 2024

View reviewed changes

packages/assets-controllers/src/index.ts Outdated Show resolved Hide resolved

MajorLift reviewed Jun 6, 2024

View reviewed changes

kanthesha added 4 commits June 6, 2024 16:58

Included PR suggestions

f52d94f

Merge branch 'upgrade-token-rates-controller-to-base-controller-v2' o…

d43b2e6

…f github.com:MetaMask/core into upgrade-token-rates-controller-to-base-controller-v2

Jsdoc comments updated

d2146d5

Merge branch 'main' into upgrade-token-rates-controller-to-base-contr…

ae0a290

…oller-v2

cryptodev-2s reviewed Jun 6, 2024

View reviewed changes

packages/assets-controllers/src/TokenRatesController.ts Outdated Show resolved Hide resolved

kanthesha added 4 commits June 6, 2024 17:45

private handle to #handle

dd09e2e

remove unnecessary space before Token type

87f74c1

The missing jsdoc comments added

71d960e

clean up

f96fcb7

mcmire reviewed Jun 7, 2024

View reviewed changes

packages/assets-controllers/src/TokenRatesController.test.ts Outdated Show resolved Hide resolved

packages/assets-controllers/src/TokenRatesController.test.ts Outdated Show resolved Hide resolved

packages/assets-controllers/src/TokenRatesController.ts Show resolved Hide resolved

include suggestions

db8654d

cryptodev-2s approved these changes Jun 7, 2024

View reviewed changes

mcmire approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'main' into upgrade-token-rates-controller-to-base-contr…

ecb218c

…oller-v2

kanthesha merged commit 4d77dde into main Jun 8, 2024
113 checks passed

kanthesha deleted the upgrade-token-rates-controller-to-base-controller-v2 branch June 8, 2024 12:27

ccharly added a commit that referenced this pull request Jun 12, 2024

chore: update entries for #4314

c9a6139

upgrade TokenRatesController to BaseControllerV2 #4314

upgrade TokenRatesController to BaseControllerV2 #4314

Conversation

kanthesha commented May 23, 2024

Explanation

References

Changelog

Added

Changed

Removed

Checklist

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

Choose a reason for hiding this comment

mcmire Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

kanthesha commented Jun 4, 2024

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

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 commented Jun 5, 2024

Choose a reason for hiding this comment

cryptodev-2s Jun 6, 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 Jun 6, 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

cryptodev-2s left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

mcmire Jun 7, 2024 •

edited

Loading

cryptodev-2s Jun 6, 2024 •

edited

Loading

MajorLift Jun 6, 2024 •

edited

Loading