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

Improve CoinGecko stability, add attribution #188

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

JSKitty
Copy link
Member

@JSKitty JSKitty commented Aug 30, 2023

Abstract

This simple PR improves the stability of our CoinGecko integration:

  • MPW now quietly catches any errors on data refreshes.
  • MPW can still display the correct Currency even if CoinGecko is unavailable.
  • MPW can fallback to in-memory cache, if CoinGecko suddenly goes unresponsive.

Previously, even a single CoinGecko call failing would still display a generic "Uncaught Network Error" for the user, CoinGecko calls sometimes randomly fail, so this was causing users that kept MPW open for long periods of time, to randomly return to errors on their app, this should be less now that CoinGecko errors are handled perfectly.

Additionally, the PR adds i18n'd CoinGecko attribution, which is required by their documentation terms.

image


Testing

This PR is fairly behind-the-scenes, so the best tests a user can do, is to simply check that their currency (USD, GBP, etc) is loading correctly, displaying correctly in all areas, and that the Settings show the currency options and CoinGecko attribution.

For folks that are confident in messing with the Browser Developer Console, however, you can follow this to thoroughly test the PR:

  • Load up MPW as normal, login.
  • Open the console, hit "Network".
  • Find a "CoinGecko" request, and hit "Block URL" or similar depending on your browser.
  • Once you've blocked CoinGecko's requests from MPW, watch for any errors, the GUI should NOT show any errors, and prices should continue displaying nicely from local cache, any CoinGecko errors will be silently placed in the Console Logs.

If any errors are found, the PR works unexpectedly, or you have viable suggestions to improve the UX or functionality of the PR, let me know!


@JSKitty JSKitty added Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request Awaiting Review This PR and/or issue is awaiting reviews before continuing. Review Reward: 10 PIV Reviewers of this Pull Request will receive a 10 PIV reward labels Aug 30, 2023
@JSKitty JSKitty self-assigned this Aug 30, 2023
Copy link

@BreadJS BreadJS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot better than the annoying notifications on fetch error!
Works as expected! Good job Kitty!

LGTM!

@JSKitty
Copy link
Member Author

JSKitty commented Aug 30, 2023

Tested by another ~2 rewarded Quality Control members, merging PR!

@JSKitty JSKitty merged commit ac90c85 into master Aug 30, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request Review Reward: 10 PIV Reviewers of this Pull Request will receive a 10 PIV reward
Projects
Development

Successfully merging this pull request may close these issues.

2 participants