-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Implement Cryptocompare API to fetch exchange rates #658
base: main
Are you sure you want to change the base?
Conversation
An option could be added for the user to select the preference, but cryptocompare does work better for most users.
4728b7e
to
27f6a6c
Compare
@schildbach Any feedback about this? This change is very important for us to promote bitcoin (and crypto in general) in Argentina (and I suppose that also in other countries without free access to USD). |
@schildbach any feedback about this would be appreciated. I know the code is not great, but I've just learned Java to write this. :) |
This looks like a good addition, I'd like to see it merged (being from Argentina too) :) |
Any feedback on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally the parsing code looks good. However we're missing some kind of source selection, or merging of result data. I don't want to forcibly disable CoinGecko for everyone.
* @author Eloy Espinaco | ||
*/ | ||
public final class CryptoCompare { | ||
private static final HttpUrl URL = HttpUrl.parse("https://min-api.cryptocompare.com/data/price?fsym=BTC&tsyms=usd,aed,ars,aud,bdt,bhd,bmd,brl,cad,chf,clp,cny,czk,dkk,eur,gbp,hkd,huf,idr,ils,inr,jpy,krw,kwd,lkr,mmk,mxn,myr,ngn,nok,nzd,php,pkr,pln,rub,sar,sek,sgd,thb,try,twd,uah,vef,vnd,zar,xdr&extraParams=bitcoinwallet"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of the extraParams=bitcoinwallet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just an optional param to let cryptocompare make stats.
for (Map.Entry<String, String> entry : rates.entrySet()) { | ||
final String symbol = entry.getKey().toUpperCase(Locale.US); | ||
try { | ||
final Fiat rate = Fiat.parseFiatInexact(symbol, entry.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting is off a bit here. Please use 4 spaces per indent level.
import static org.junit.Assert.assertEquals; | ||
|
||
/** | ||
* @author Andreas Schildbach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you want to use yourself as the author here, rather than me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this code is actually yours, as I've used a removed version as the source and modified it... but I can add myself if you think so.
Seems like they want to phase out their public API (without API key): https://cryptocompare.zendesk.com/hc/en-gb/articles/360013348454-What-will-happen-to-the-existing-free-public-API- |
No, they already made clear that they will not drop support, just IP rate limit it, that will have no impact on the wallet, as the wallet make requests from different IPs for each user.
So it seems that cryptocompare API can be used without issues. |
It would be great to have a selector, but I'm not able to implement that myself. |
This should close #627 and might also help to close #613, because cryptocompare does not use cloudflare.
This implementation requires hard-coding the fiat currencies (the api cannot send you all the fiat rates) and it does require all the rates every time to keep the compatibility with CoinGecko implementation.
Also, the second commit does replace the implementation without adding any selector for the user, but I'm not sure how to implement that. This is my first commit on java, so there might be a lot of things to fix, so comments and improvements are welcome.
That said, I've tested it for a while, and I'm very happy with it. And the exchange rate for ARS is fixed (no more the ridiculous rate provided by coingecko).