Add chainId support to TokenRatesController #476

wachunei · 2021-05-24T17:00:30Z

No description provided.

src/assets/TokenRatesController.ts

wachunei · 2021-05-25T00:58:20Z

src/assets/TokenRatesController.ts

+  set chainId(_chainId: string) {
+    !this.disabled && safelyExecute(() => this.updateExchangeRates());
+  }
+
+  get chainId() {
+    throw new Error('Property only used for setting');
+  }
+


When chainId changes, run update if not disabled.

wachunei · 2021-05-25T00:59:35Z

src/assets/TokenRatesController.ts

@@ -170,21 +214,43 @@ export class TokenRatesController extends BaseController<
   * @returns Promise resolving when this operation completes
   */
  async updateExchangeRates() {
-    if (this.tokenList.length === 0) {
+    if (this.tokenList.length === 0 || this.disabled) {


If this was called directly, nothing was stopping it from being run. What is the intended behavior originally: should disable just disable tokenList/chainId changes?

I don't think the disabled property was used in this controller 🤔

Like many aspects of the BaseController, some controllers are written to use it and some ignore it, or break when you try to use it. The config is similar - it's built to be changeable at runtime, but many controllers are written assuming that never happens, and they break if you try it.

It doesn't hurt to add a check for disabled here but I'm not sure it helps either, unless I'm missing something. This awkwardness with the API is one of the things BaseControllerV2 is meant to address.

wachunei · 2021-05-25T01:04:47Z

src/assets/TokenRatesController.ts

+    let chainSlug;
+
+    if (!chainSlugIdentifier) {
+      try {
+        chainSlug = await this.getChainSlugIdentifier(chainId);
+        if (!chainSlug) {
+          this.update({ chainSlugIdentifier: null });
+        }
+      } catch {
+        this.update({ chainSlugIdentifier: undefined });
+      }
+    }


Here's the main question.

Since now there is an extra chainId parameter, we need to hit two endpoints

Get the chain slug given the chain id

Use the chain slug to recover the prices, like it was done before

Should we "remember" the slug? There are three cases:

The request fails

The request is successful but the requested chain id is not listed on the api

The request is successful and we have a slug we can use

If we don't have the slug stored, this will always make two request when fetching prices.
If we store the slug, we must be able to define when to invalidate it in case the API changes, for example: it supports a new chain which we stored as unsupported

Maybe we store the slug when we are able to get a valid response and then wrap the fetchExchangeRate in a try/catch in case the slug becomes invalid and attempt to refetch it if that's the case?

How would we manage the case the slug does not exist and later it does? Example: We would store it as null, so we don't even attempt to fetch the rates, but if the slug is supported in the future, how would we invalidate that null?

Maybe I'm not understanding but if the chainSlugIdentifier is null we try will try to fetch it again as the code stands? Its not ideal because we are not able to cut out that extra network request by storing it as unsupported but I can't think of a way around that right now - unless they only update the api when they change the endpoint (v3 -> v4)?

What we are doing on swaps-controller to hold values for a certain amount of time is to store pairs (value, timestamp) and compare timestamp, which represents the last time value was fetched, to a time threshold.

This way if value is null but Date.now() - timestamp is less than a threshold, we consider the network is not supported, otherwise we attempt to fetch the slug and update the pair (value, timestamp = Date.now()).

The same works when value is a-network-slug that might become unsupported, but we check only after we are over the time threshold.

Does it make sense?

Yes that makes sense. So we're weighing the trade-offs of optimizing for fewer network requests and, if we choose to go that route, possibly not fetching exchange rates when they are available until the unsupported flag expires according to whatever time threshold we set. I guess I don't have a strong intuition of which way to go with this. What do you think @Gudahtt?

wachunei · 2021-05-25T14:47:09Z

Another issue is that updateExchangeRates will run twice: when chainId changes and when the token list changes.

This could be avoided if assets had the chain_id as property, which would also work when listing assets from different chains in the same view.

Gudahtt

I left some initial comments here and on Notion. If we decide to pursue caching the API response, some of these comments might end up being obsolete though.

Gudahtt · 2021-06-09T15:36:13Z

src/assets/TokenRatesController.ts

+  },
+};
+
+function getUpdatedSupportedChains(


This function seems.... odd. It just combines the two parameters? Seems to have limited utility at first glance.

Yeah, it actually merges a nested property of state because BaseController.update is shallow merge, would override other chainIds values.

Gudahtt · 2021-06-09T15:37:18Z

src/assets/TokenRatesController.ts

@@ -109,9 +153,16 @@ export class TokenRatesController extends BaseController<
      disabled: true,
      interval: 180000,
      nativeCurrency: 'eth',
+      chainId: '1',


We should not have a hard-coded default network - we should find out for sure what the initial network is. This could be disastrous, if this controller thinks the user is on a different network until he first switch.

Kinda just as bad to do this with the native currency as well to be honest, though that's a pre-existing issue.

This was done in order to provide backwards compatibility, since the controller was implicitly always hard coded to chainId: 1 (because ethereum was the hardcoded endpoint).

Will look into how we should handle this properly then.

I've handled this elsewhere by passing in the current network state as a constructor parameter, or passing in a "getNetworkState" function which can be used to check the initial state.

Cool, will look it up!

Gudahtt · 2021-06-09T15:39:53Z

src/assets/TokenRatesController.ts

@@ -137,6 +192,22 @@ export class TokenRatesController extends BaseController<
    }, this.config.interval);
  }

+  set chainId(chainId: string) {


We've been trying to move away from using setters for this kinda thing, and instead preferring methods. If this was a method (like setChainId) it's clear that setting it has a side-effect, and the caller can look to the method definition to see what it does. It might even have a nice JSDoc comment (which gets shown inline via intellisense on some editors).

But with a setter like this... it's ambiguous to the caller what effect setting this has. It might have a setter, or it might be referenced anywhere in this controller or in other modules, or both.

Is there an example of the right way to do this?
I've been using it as a side-effect handler of controller.config({...})

Ah, right, I forgot configure worked that way. Ah well. Maybe this is fine until switching to the new BaseControllerV2 API then. Not a huge deal.

Gudahtt · 2021-06-09T15:47:03Z

src/assets/TokenRatesController.ts

+
+    const chainCache = supportedChains[chainId];
+    // supportedChain has not been created, we skip this execution
+    if (!chainCache) {


This branch seems like it should be impossible to reach 🤔 Since supportedChains[chainId] is set immediately after switching networks.

The race condition I was thinking of is:
Given this state:

chainCache.slug is null

tokens is a list of tokens

Since we have now two listeners (on assets change and on network change), could it be the case this method executes when the assets change (because of a network change) ~~instead of~~ before the network change itself?

There is an open question on the technical proposal about this

The thing is, if the assets change before the chain ID does, this would still be referencing the old chain ID. I don't believe it's possible for this to have a new chain ID without the setter for chainId running first.

I see it now, this check is unnecessary 👍

adonesky1 · 2021-06-23T19:59:37Z

src/assets/TokenRatesController.ts

+        newContractExchangeRates[address] = undefined;
+      });
+    } else {
+      const pairs = this.tokenList.map((token) => token.address).join(',');


[nit] @wachunei I know you didn't name this const but it seems slightly misnamed to me? It's a list of tokens that are each one side of a pair with the native currency right?

No, this is just a concatenation of token addresses joined by a comma, from something like [{ address: '0x1...', ...restOfTokenObject1 }, { address: '0x2...', ...restOfTokenObject2 }] into "0x1...,0x2...", which is what CoinGecko takes in as query param for the API call.

I agree is not a great name! But you know, naming stuff is hard hahahaha

pairs should be addresses, sorry for necroposting

adonesky1 · 2021-06-24T15:18:04Z

@wachunei have you tested this at all wired up in the extension?

wachunei · 2021-06-24T15:20:14Z

@wachunei have you tested this at all wired up in the extension?

No, I could test it with the mobile app, have not done it yet tho.

adonesky1 · 2021-06-24T15:33:10Z

No, I could test it with the mobile app, have not done it yet tho.

Ok I'm going to give it a go (on the extension) if there are no objections.

wachunei · 2021-06-24T15:34:11Z

That would be great! Thank you @adonesky1

adonesky1 · 2021-06-24T19:28:12Z

@Gudahtt, @brad-decker is there any roadmap for migrating the extension towards use of the AssetsController? Or, if not, in order to integrate the TokenRatesController in the extension will we need to modify this controller to expect alternative listeners that correspond with where/how token data is in the extension?

Gudahtt · 2021-06-24T20:02:54Z

It is planned, but only in our imaginations at this point. No concrete plan yet.

wachunei · 2021-07-01T14:56:38Z

@adonesky1 I tested in on the app and it is working, opened a draft PR of the integration here MetaMask/metamask-mobile#2863

brad-decker

Looks good to me!One non blocking Q

brad-decker · 2021-07-02T16:36:41Z

src/assets/TokenRatesController.ts

+  supportedChains: SupportedChainsCache;
+}
+
+const COINGECKO_API = {


[Q] Is this a common pattern in the controller code base, having the UPPER_SNAKE_CASE (which I read as a constant) have methods on it? Non blocking just curious

Great question, actually I don't know. It has landed to this point because it started as a constant plain object but then it mutated to an object with methods for convenience. Happy to adopt convention/pattern before moving on.

Maybe just CoinGeckoApi or some such for now. I could see it living in its own platforms file when there are more than one platform we use for this. For now its fine here. Its also fine to let it in like this.

Changed on 1aa1bfb (#476)

adonesky1 · 2021-07-02T18:34:30Z

Ok I'm going to give it a go (on the extension) if there are no objections.

@wachunei sorry I never followed up on this. This work is blocked by a need to migrating the extension to use of the AssetController. I began to plan this work and then got pulled in another direction. This obviously shouldn't be a blocker though. I will probably try to pick that work up again soon after this has merged.

wachunei · 2021-07-02T20:19:10Z

@adonesky1 No worries.

This is also in a weird state because of develop moving on to v11. Do we maintain minor versions of different major versions in parallel?

If I revert e4bc0d1 on this branch, this is 10.x compatible (our current mobile target version) and this could be released as 10.3
If I merge as is, this would be 11 compatible and won't work with current mobile code, so this is blocked until we upgrade to 11

Thoughts?

Gudahtt · 2021-07-02T20:30:44Z

Generally I'd recommend always targeting the main branch with all bug fixes/features/etc. and separately backporting if necessary. If you're going to be backporting, you'll typically want the change in both places anyway, and it's just easier to land it on main first.

There are some basic instructions for how to prepare a backport release in the README. Basically you'd create a 10.x branch off of v10.1 (the most recent non-deprecated v10 release), create a release branch based there, cherry-pick the change you want to backport, and do the usual release bump and changelog update. Whether you want to bother with backporting is up to you/the mobile team. It's a tradeoff between taking the extra effort to backport v.s. waiting for v11 to land. It's probably better to land v11 and save yourself the effort unless time constraints make that difficult.

wachunei · 2021-07-02T20:40:29Z

Thanks @Gudahtt, I'll merge this and ask the mobile team about having this included in v11 or backporting.

wachunei · 2021-07-05T13:57:52Z

Just a reminder this would be a major version bump since TokenRatesController constructor now expects a onNetworkStateChange listener.

wachunei added 2 commits May 24, 2021 13:00

Initial proposal for chainId support of TokenRatesController

84ac2fd

Default to ethereum mainnet chain

b720274

wachunei commented May 24, 2021

View reviewed changes

src/assets/TokenRatesController.ts Outdated Show resolved Hide resolved

Get chainSlugIdentifier on update

efddcdf

wachunei commented May 25, 2021

View reviewed changes

Merge branch 'main' into feature/token-rate-chainid

8ad4be4

mobularay mentioned this pull request Jun 3, 2021

View the FIAT conversion of my tokens MetaMask/metamask-mobile#2440

Closed

Add supportedChains and chainCache

f3524f6

Gudahtt reviewed Jun 9, 2021

View reviewed changes

wachunei added 4 commits June 11, 2021 16:11

Cache response instead of individual slugs

4830c33

Merge branch 'main' into feature/token-rate-chainid

1774012

Update interval test

0db47bd

Merge branch 'main' into feature/token-rate-chainid

204ff43

wachunei marked this pull request as ready for review June 23, 2021 19:46

wachunei requested a review from a team as a code owner June 23, 2021 19:46

adonesky1 reviewed Jun 23, 2021

View reviewed changes

Use 6 hours threshold

bca002a

Merge branch 'main' into feature/token-rate-chainid

e4bc0d1

wachunei mentioned this pull request Jul 1, 2021

Allow custom network fiat values MetaMask/metamask-mobile#2863

Merged

brad-decker previously approved these changes Jul 2, 2021

View reviewed changes

Rename COINGECKO_API to CoinGeckoApi

1aa1bfb

wachunei dismissed brad-decker’s stale review via 1aa1bfb July 2, 2021 18:20

brad-decker approved these changes Jul 2, 2021

View reviewed changes

Merge branch 'main' into feature/token-rate-chainid

120f654

wachunei merged commit f23f71e into main Jul 5, 2021

wachunei deleted the feature/token-rate-chainid branch July 5, 2021 13:54

wachunei mentioned this pull request Jul 7, 2021

v11.1.0 release #514

Closed

brad-decker mentioned this pull request Jul 7, 2021

v12.0.0 release #515

Merged

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Add chainId support to TokenRatesController (#476)

ff52fd7

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Add chainId support to TokenRatesController (#476)

4e4e9a0

Add chainId support to TokenRatesController #476

Add chainId support to TokenRatesController #476

Conversation

wachunei commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wachunei May 25, 2021 • edited Loading

Choose a reason for hiding this comment

adonesky1 May 28, 2021 • 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

adonesky1 May 28, 2021 • edited Loading

Choose a reason for hiding this comment

wachunei commented May 25, 2021

Gudahtt left a 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

Gudahtt Jun 9, 2021 • 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

wachunei Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 Jun 23, 2021 • 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

adonesky1 commented Jun 24, 2021

wachunei commented Jun 24, 2021

adonesky1 commented Jun 24, 2021

wachunei commented Jun 24, 2021

adonesky1 commented Jun 24, 2021

Gudahtt commented Jun 24, 2021

wachunei commented Jul 1, 2021

brad-decker left a 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

adonesky1 commented Jul 2, 2021

wachunei commented Jul 2, 2021

Gudahtt commented Jul 2, 2021

wachunei commented Jul 2, 2021

wachunei commented Jul 5, 2021

wachunei May 25, 2021 •

edited

Loading

adonesky1 May 28, 2021 •

edited

Loading

adonesky1 May 28, 2021 •

edited

Loading

Gudahtt Jun 9, 2021 •

edited

Loading

wachunei Jun 9, 2021 •

edited

Loading

adonesky1 Jun 23, 2021 •

edited

Loading