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

Get token prices from our API instead of CoinGecko #3600

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Dec 1, 2023

Explanation

CoinGecko is planning on retiring their /simple/token_price endpoint soon. This endpoint is critical for us, as we use to fetch prices for tokens. This commit removes all code related to CoinGecko in TokenRatesController so that it hits an internal API instead.

This is set up in such a way that we can upgrade to newer versions of the API (or even other services) in the future without breaking backward compatibility. However, at the moment it does introduce breaking changes to TokenRatesController.

References

Fixes #3574.

Changelog

(Updated in PR)

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

@mcmire mcmire requested a review from a team as a code owner December 1, 2023 05:39
@mcmire mcmire force-pushed the switch-to-codefi-price-api branch 2 times, most recently from 1969e05 to 1302cc3 Compare December 1, 2023 05:41
@mcmire mcmire marked this pull request as draft December 1, 2023 16:57
@mcmire
Copy link
Contributor Author

mcmire commented Dec 1, 2023

Putting into draft again because I'm going to make sure to incorporate changes in MetaMask/metamask-mobile#7957.

@mcmire mcmire force-pushed the switch-to-codefi-price-api branch from 6e9c8d1 to 0a9c0a7 Compare December 1, 2023 20:47
@mcmire mcmire marked this pull request as ready for review December 1, 2023 20:52
CoinGecko is planning on retiring their `/simple/token_price` endpoint
soon. This endpoint is critical for us, as we use to fetch prices for
tokens. This commit removes all code related to CoinGecko in
TokenRatesController so that it hits an internal API instead.

This is set up in such a way that we can upgrade to newer versions of
the API (or even other services) in the future without breaking backward
compatibility. However, at the moment it does introduce breaking
changes to TokenRatesController.
@mcmire mcmire force-pushed the switch-to-codefi-price-api branch from 0a9c0a7 to c61add8 Compare December 1, 2023 20:57
// Base
'0x2105',
// Shiden
// NOTE: This is the wrong chain ID, this should be 0x150
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm asking Tomas about this: MetaMask/metamask-mobile#7957 (comment)

* The list of currencies that can be supplied as the `vsCurrency` parameter to
* the `/spot-prices` endpoint, in lowercase form.
*/
export const SUPPORTED_CURRENCIES = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list comes from MetaMask/metamask-mobile#7957.

* the `/spot-prices` endpoint, but in hexadecimal form (for consistency with
* how we represent chain IDs in other places).
*/
export const SUPPORTED_CHAIN_IDS = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list comes from MetaMask/metamask-mobile#7957.

mikesposito
mikesposito previously approved these changes Dec 4, 2023
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! Probably just some changes to do on inline docs

@mikesposito
Copy link
Member

I am thinking about how/if this relates to this issue.

Do we handle the case where a list of tokens overflows the service rate limit? And in case not, what would be the best way to do that after these changes?

@mcmire
Copy link
Contributor Author

mcmire commented Dec 5, 2023

Do we handle the case where a list of tokens overflows the service rate limit? And in case not, what would be the best way to do that after these changes?

I think in a future PR, we can wrap the call to this.#tokenPricesService.fetchTokenPrices so that we divide the list of tokens into batches, and then call that method for each batch to get the total set of token prices. Does that answer your question?

@mcmire mcmire merged commit 2e8afb3 into main Dec 5, 2023
132 checks passed
@mcmire mcmire deleted the switch-to-codefi-price-api branch December 5, 2023 22:08
Gudahtt added a commit that referenced this pull request Dec 14, 2023
PR #3600 included a non-breaking change to `controller-utils` that was
required by the `assets-controllers` changes.
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] Switch from CoinGecko to MetaMask price API
2 participants