feat: use crypto compare multi api for currency rate controller #4852
Conversation
| error instanceof Error && | ||
| error.message.includes('market does not exist for this coin pair') |
There was a problem hiding this comment.
Unlike the single endpoint, the multi endpoint does not appear to throw if the symbol does not exist. It just ignores that particular symbol and returns the rest. So I don't see anything to catch.
| conversionRate: number; | ||
| conversionDate: number; | ||
| usdConversionRate?: string; | ||
| usdConversionRate?: number; |
There was a problem hiding this comment.
This is sort of extra, but these seemed wrongly typed to me. I see everything as numbers. I mainly wanted to change fetchMultiExchangeRate to be correctly typed as numbers, but it spilled over to here as well.
-
Crypto compare returns everything as numbers:
https://min-api.cryptocompare.com/data/pricemulti?fsyms=BTC,ETH&tsyms=USD,EUR
{"BTC":{"USD":65834.86,"EUR":60972.28},"ETH":{"USD":2415.39,"EUR":2232.75}} -
The
fetchMultiExchangeRatefunction keeps them as numbers:
- They're still numbers when
RatesControlleris saving them to state:
- And they're still numbers when I export state from MM extension.
There was a problem hiding this comment.
The reason why these are strings is that converting to number loses precision and can change the value in the conversion to an IEEE 754 float.
JSON itself allows numbers with arbitrary precision and does not consider the specifics of JavaScript.
Downcasting to JS number (which is done internally by JSON.parse) is in general not safe and can produce incorrect results, where in particular for exchange rates loss of precision compound as values get multiplied and result in inaccuracies.
This is the reason why we should:
- use some decimal/bignumber implementation when calculating on these values
- not serialize them as
number- prefer
stringwhen serializing for public APIs
- prefer
- when deserializing JSON containing numbers, either:
- (preferred) use a different implementation than
JSON.parsewhich does not destroy precision - transform the raw JSON string before desiralization, wrapping numbers to strings with
"so they can be safely deserialized usingJSON.parse
- (preferred) use a different implementation than
- In general avoid treating them as
numberever
Even if at some point we end up being forced to use number at some API endpoint for whatever reason, those should be contained.
There was a problem hiding this comment.
More on the same:
https://stackoverflow.com/questions/47916160/alternative-to-json-parse-for-maintaining-decimal-precision/47916876#47916876
https://0.30000000000000004.com/
Why the PayPal API uses strings:
https://stackoverflow.com/questions/35709595/why-would-you-use-a-string-in-json-to-represent-a-decimal-number
https://blog.phakorn.com/jsons-numeric-boundaries-the-lesser-known-reality-of-inaccurate-figures
There was a problem hiding this comment.
That all makes sense. I know that was the intent of typing these as string. But it's not what actually happens today given we're using Response.json() which puts them into numbers. I think the types should accurately reflect that in the meantime.
Filed this to track it. #4857. The wallets consuming this state would also need to stop expecting numbers.
|
Would it be viable to make the public API less tightly coupled to Crypto Compare API? Would make it less involved to make future changes and make it more flexible and robust at the same time. Related: https://github.com/MetaMask/core/pull/4852/files#r1817508642 |
Can you explain what you're thinking? The crypto compare stuff is already behind a separate service with generic interfaces for |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
## **Description** Leverages MetaMask/core#4852 to fetch native currency prices across all evm chains, instead of just the current chain. Metamask will now hit the `/pricemulti` endpoint of cryptocompare to fetch an array of currencies in 1 request. [](https://codespaces.new/MetaMask/metamask-extension/pull/28196?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** No user facing changes. 1. Add some networks with native currencies other than ETH like polygon, bnb 2. Verify native tokens have the correct fiat price 3. Export state and verify `currencyRates` has entries for each native currency ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
](https://codespaces.new/MetaMask/metamask-extension/pull/28196?quickstart=1){kind=link}
Explanation
Updates the currency rate controller to use the
pricemultiendpoint of crypto compare. This allows us to fetch exchange rates for multiple native currencies in a single http request.References
Changelog
@metamask/assets-controllersCurrencyRateControllernow accepts an array of native currencies as its polling input.RatesControllernow types theconversionRateandusdConversionRatein its state asnumberinstead ofstring, to match what it was actually storing.Checklist