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

Pricenode crashes on startup due to CoinMarketCap retired API #4032

Merged

Conversation

wiz
Copy link
Member

@wiz wiz commented Mar 6, 2020

Background

CoinMarketCap isn't actively used as a pricenode provider, because the only coins that used it have been delisted.

Issue

CoinMarketCap has retired their v1 API, and the resulting HTTP 410 Gone causes the pricenode to crash on startup

Summary

This PR simply removes CoinMarketCap as a PriceNodeProvider since it's apparently no longer used and the API has been retired

@wiz wiz requested a review from cbeams as a code owner March 6, 2020 17:37
@wiz wiz changed the title Remove CoinMarketCap as dead code Pricenode crashes on startup due to CoinMarketCap dead code Mar 6, 2020
@sqrrm
Copy link
Member

sqrrm commented Mar 6, 2020

From reading the comments on how the Providers are instantiated, this looks fine. I don't understand exactly how that's done though, would really prefer the review of @cbeams

@wiz wiz changed the title Pricenode crashes on startup due to CoinMarketCap dead code Pricenode crashes on startup due to CoinMarketCap retired API Mar 6, 2020
With the removal of CoinMarketCap as an exchange rate provider in the
prior commit, the @order values of the remaining three provider
implementations are non-contiguous. CoinMarketCap was @order(3) and with
it gone, the remaining order values became 1, 2, 4. This is not a
problem in practice, but could be confusing for future maintainers, so
this commit simply decrements the Poloniex implementation from 4 to 3 in
order to make the values contiguous once again.

This commit also fixes a typo in ExchangeRateProvider's Javadoc.
@wiz
Copy link
Member Author

wiz commented Mar 6, 2020

Again please note that this PR is an emergency measure since currently pricenodes crash on startup. Obviously we need to consider implementing one of the new CMC authenticated / paid APIs to replace the API that has been retired

Copy link
Member

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

utACK, see my additional review commit.

@sqrrm sqrrm merged commit 72bcc4a into bisq-network:master Mar 6, 2020
@wiz wiz deleted the remove-coinmarketcap-as-pricenode-provider branch March 6, 2020 18:29
@ripcurlx ripcurlx added this to the v1.2.8 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants