Fix NaN conversionRate values#194
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #194 +/- ##
======================================
Coverage 100% 100%
======================================
Files 23 23
Lines 1518 1518
Branches 211 211
======================================
Hits 1518 1518Continue to review full report at Codecov.
|
ccb4f40 to
5b7eff7
Compare
| const conversionRate = Number(json[currency.toUpperCase()]); | ||
|
|
||
| if (!Number.isFinite(conversionRate)) { | ||
| throw new Error(`Invalid response for ${currency.toUpperCase()}: ${json[currency.toUpperCase()]}`); |
There was a problem hiding this comment.
I see a potential bug throwing an error here. While ago we had an issue with concurrency in calls to the API happening asynchronously in a bad way. the solution was to add a semaphore to take care of the current state of the controller while doing calls. So, this method is used here
https://github.com/MetaMask/gaba/blob/2dc732ff1b13be27fc5f83a197f379fb8669be18/src/assets/CurrencyRateController.ts#L144
and is being called from other places with safelyExecute so throwing an error is fine. But throwing an error without releasing
https://github.com/MetaMask/gaba/blob/2dc732ff1b13be27fc5f83a197f379fb8669be18/src/assets/CurrencyRateController.ts#L148
could block this method.
There was a problem hiding this comment.
Good point, though this seems to be a pre-existing problem (one I introduced a while back 😬 ). handleFetch already throws an error in some cases, so it could already block.
I guess the solution would be to put a try block around the code between the mutex.aquire() and releaseLock(), and put releaseLock() in the finally clause.
There was a problem hiding this comment.
You're right, that's why we have this method called safelyExecute to handle errors, all async methods are called with that if it throws it just keeps polling.
For the same reason having a try catch inside safelyExecute doesn't look good from my point of view. As you mentioned before, going with 0 as value would be good instead of throwing.
There was a problem hiding this comment.
It would solve the problem you identified (the mutex never being released). It should be perfectly fine. 🤷♂ Whether it's being called from safelyExecute or not shouldn't make any difference.
Though it was try ... finally that I suggested, not try ... catch. try ... finally would let us release the lock in the finally block regardless whether an error was thrown or not, then it would let the error continue to bubble upwards if it was thrown, to be caught by safelyExecute or wherever else. It's a pretty standard way of dealing with cases like this where you need to ensure something is released.
There was a problem hiding this comment.
It's a pretty standard way of dealing with cases like this yes, I know.
I'm fine whatever handles the issue I brought 👍
There was a problem hiding this comment.
I have created a separate PR to address this problem: #195
estebanmino
left a comment
There was a problem hiding this comment.
all right let's merge this first?
The two provider construction functions have been removed. These functions are instead availabile from the new package `eth-json-rpc-provider`.
This PR fixes[1] an issue in the extension where the
conversionRatein state isNaN.How this manifested itself in the extension
The
CurrencyRateControllerwas introduced in v6.6.0[2][3] (v6.5.3...v6.6.0) and we saw MetaMask/metamask-extension#6725 shortly after that went out. TheNaNin the background state ends up asnullin the UI, because JSON, and is then used to format balances in theCurrencyDisplayContainercomponent:This has reared its head in a few GH issues:
Missing symbols in the API
The root cause here would be the exchange API response missing the symbol. This is at least possible if the selected symbol errors, as the API still returns a "successful" response. Note the
200status code:[1] In part, we'll need to update the extension with this.