fix state conversion rate issue

[adonesky1]

per this thread I think we've got a pretty nasty papercut here:

Screen.Recording.2021-05-12.at.2.27.44.PM.mov

Partially fixes: extension issue #9492

When we fetch invalid conversion rates either because the user hasn't entered a ticker symbol or the symbol ticker is not recognized by the API we use for conversions, we are currently throwing an error and leaving a stale conversion rate in state. This PR changes this failed fetch condition to result in a null conversion rate. This extension repo PR (WIP -> unit tests), deals with the ramifications of potentially null conversion rates.

[adonesky1]

@brad-decker and @Gudahtt should this PR be set against a v8.0.0 branch since develop isn't ready to roll yet?

[Gudahtt]

I think develop is pretty close to ready - maybe within a day or two. Plus we'll want this change on develop anyway.

It might be best to target develop for now, and backport if necessary?

[Gudahtt]

Thanks for looking into this! I'm not sure about this approach though 🤔

If I'm understanding this correctly, we're still sending a request for the symbol undefined to CryptoCompare, and handling a failed response by setting the rate to null. Instead it would be better to never make the request at all if we're certain it will fail (e.g. if we don't have a valid symbol). Faster, less traffic, etc.

Second, we do still want to know if something goes severely wrong here with fetchExchangeRate. This will result in any error after the network request, including unexpected ones, getting flattened to a null response. Kind of a minor problem in this case maybe because the most common error here is a bad/unrecognized symbol, and that is expected and not useful to log to Sentry. But if we get a result that has an unexpected format, that's worth logging as it might indicate a parsing issue, or an API change that we didn't prepare for.

Preserving the exact reason for the error lets us have smarter polling logic too. For example, we could keep polling if it's a network error, but stop polling completely if we discover that CryptoCompare doesn't support the symbol (and never start polling to begin with if we don't know the symbol).

One way to preserve the error information would be to leave the parsing logic as-is, but add a check to see if the response was successful or not. In testing this out just now I discovered that CryptoCompare's API returns a HTTP 200 response with an error object/message in this scenario, which isn't anticipated here. We could look for that and throw an error ahead of this parsing logic.

Then in the CurrencyRateController, we could catch all errors, and re-throw any that we don't know we want to suppress. That's one suggestion anyway - let me know what you think.

[adonesky1]

Yeah this makes sense! I'll take a stab at your recommendation and ping you when I've pushed! Thanks @Gudahtt

[adonesky1]

@Gudahtt I think I may have an issue with my sequencing and try/catch/finally logic. I'm just trying to make sure that the currencyRate state is updated no matter what and that the right errors still make it through. Let me know what you think? and if you're free to jump on a call and take a look together that would be great.

[Gudahtt]

This looks correct to me! Still room for further improvements to the currency rate controller, to stop polling in certain conditions. But that can wait - this solves the core issue.

[adonesky1]

@Gudahtt I banged my head against the desk for a bit here trying to figure out how to get full test coverage... Not sure why it won't count both conditional branches on this line in my tests? Do you know where I'm going wrong here?

[Gudahtt]

There's an HTML coverage report generated when you run yarn test locally that makes it a lot easier to spot gaps in coverage. You can find it in coverage/lcov-report/index.html.

[adonesky1]

There's an HTML coverage report generated when you run yarn test locally that makes it a lot easier to spot gaps in coverage. You can find it in coverage/lcov-report/index.html.

Thanks for the tip!

[Gudahtt]

Left a couple of suggestions; let me know what you think!

[Gudahtt]

LGTM, great work!