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

Prevent redundant token rate updates #3647

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Dec 8, 2023

Explanation

The token rates controller fetches rates for certain collections of tokens obtained via the tokens controller. If any of these collections change, then new rates need to be fetched.

Unfortunately, because the check to determine whether these collections have changed is simplistic, new rates are fetched every single time the tokens controller state is changed.

To address this, we can consider two things:

  • The token rates controller only uses the addresses of tokens in these collections to fetch rates, and it also normalizes those addresses when updating state.
  • These collections of tokens may contain duplicates.

So to reduce the number of times rates are fetched and to optimize each request, this commit updates the tokens state callback to assemble the aforementioned token collections into a deduplicated, normalized list of token addresses, and then it checks to see if this list has any changes to know whether or not to re-fetch rates.

References

Fixes #3605.

Changelog

@metamask/assets-controllers

  • FIXED: Fix TokenRatesController to prevent redundant token rate updates when tokens change
    • Previously, token rates would be re-fetched for the globally selected network on all TokensController state changes, but now token rates are always performed for a deduplicated and normalized set of addresses, and changes to this set determine whether rates should be re-fetched.

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

The token rates controller fetches rates for certain collections of
tokens obtained via the tokens controller. If any of these collections
change, then new rates need to be fetched.

Unfortunately, because the check to determine whether these collections
have changed is simplistic, new rates are fetched every single time the
tokens controller state is changed.

To address this, we can consider two things:

- The token rates controller only uses the addresses of tokens in these
  collections to fetch rates, and it also normalizes those addresses
  when updating state.
- These collections of tokens may contain duplicates.

So to reduce the number of times rates are fetched and to optimize each
request, this commit updates the tokens state callback to assemble the
aforementioned token collections into a deduplicated, normalized list of
token addresses, and then it checks to see if this list has any changes
to know whether or not to re-fetch rates.
@mcmire mcmire requested a review from a team as a code owner December 8, 2023 22:30
@mcmire mcmire force-pushed the prevent-redundant-token-rate-updates-2 branch from 4867c37 to a20836b Compare December 8, 2023 22:30
@@ -482,7 +490,7 @@ export class TokenRatesController extends PollingControllerV1<
(obj, [tokenContractAddress, tokenPrice]) => {
return {
...obj,
[toChecksumHexAddress(tokenContractAddress)]: tokenPrice.value,
[tokenContractAddress]: tokenPrice.value,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the time we get here, tokenContractAddress is already normalized (thanks to #getTokenAddresses).

Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

return [
...new Set(
[...tokens, ...detectedTokens].map((token) =>
toHex(toChecksumHexAddress(token.address)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the toHex call redundant alongside toChecksumHexAddress?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not — unfortunately toChecksumHexAddress (located in controller-utils) doesn't use a type predicate to assert that its return value is a Hex. toHex (which is from @metamask/utils) does, though. So it's redundant in the sense that it doesn't do anything, but it does convert this to a Hex.

@mcmire mcmire merged commit c106e64 into main Dec 11, 2023
136 checks passed
@mcmire mcmire deleted the prevent-redundant-token-rate-updates-2 branch December 11, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[assets-controllers] Prevent redundant token rate updates
4 participants