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

Throw on CryptoCompare API error #486

Merged
merged 3 commits into from
Nov 29, 2017
Merged

Conversation

wbobeirne
Copy link
Contributor

This is a small bandaid for #462, CryptoCompare price error responses come back with valid json and a 200 status code so they were running through the rest of the code as though it were a success response. This caused an uncaught error to bubble up. This just throws if the API comes back with an error, causing the failure case for this action to trigger.

@wbobeirne wbobeirne added the status: ready for review Open PR's that are ready for technical review. Replaced with "changes needed" or "ready to merge". label Nov 28, 2017
@dternyak
Copy link
Contributor

dternyak commented Nov 29, 2017

I'm inclined to wait until vlad updates CC backend as specified in MyEtherWallet/MyEtherWallet#462 and implement paged token requests.

@wbobeirne
Copy link
Contributor Author

We'll definitely want to implement that, but this change handles any class of error from their API, not just this one. The current behavior is to very incorrectly end up with rates that are undefined, which throws uncaught killing the site in dev and could have a pretty bad user experience in production. I think we should keep this, but not close out #462 and finish fixing that once the change is live.

@dternyak dternyak merged commit 5b07395 into develop Nov 29, 2017
@dternyak dternyak deleted the throw-cryptocompare-error branch November 29, 2017 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready for review Open PR's that are ready for technical review. Replaced with "changes needed" or "ready to merge".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants