don't update currency rates on transient errors #4662
Conversation
| @@ -196,23 +197,27 @@ export class CurrencyRateController extends StaticIntervalPollingController< | |||
| error.message.includes('market does not exist for this coin pair') | |||
| ) | |||
| ) { | |||
| // Don't update state on transient / unexpected errors | |||
| shouldUpdateState = false; | |||
| throw error; | |||
| } | |||
| } finally { | |||
| try { | |||
There was a problem hiding this comment.
Just out of curiosity, what is the reason for this try - finally block inside of the finally block?
There was a problem hiding this comment.
to be sure the mutex always gets released, even if this.update were to throw
There was a problem hiding this comment.
Yeah, I was wondering if the update function, could indeed throw an error
| conversionDate, | ||
| conversionRate, | ||
| usdConversionRate, | ||
| if (shouldUpdateState) { |
There was a problem hiding this comment.
Just wondering if it wouldn't be a more common pattern, if we just want to update the state when an error is not thrown, we can update the state as the last line of the try block, and have the finally just to release the mutex
That way we wouldn't need the new let variable shouldUpdateState
There was a problem hiding this comment.
we want the state updated on some kids of errors though. If we get market does not exist for this coin pair, then we want to update it to null because we know crypto compare doesn't support it. Trying to maintain that existing behavior, because the apps may depend on it.
There was a problem hiding this comment.
Ah I see! Thanks for the explanation!
|
|
||
| it('should not update state on unexpected / transient errors', async () => { | ||
| const cryptoCompareHost = 'https://min-api.cryptocompare.com'; | ||
| nock(cryptoCompareHost) |
There was a problem hiding this comment.
|
Wondering your thoughts here: MetaMask/metamask-mobile#11087 (comment) |
Explanation
We think in MetaMask/metamask-mobile#11035, the intermittent 0 balances were caused by transient errors in requests to crypto compare. Instead of setting the conversion rate to null in this case, we'll omit the state update and keep the conversion rate from the previous successful fetch.
References
Changelog
@metamask/assets-controllersCurrencyRateControllerif unexpected errors occur during requests to crypto compare, the conversion rate in state will remain unchanged instead of being set to null.Checklist