MyCryptoHQ / MyCrypto

MyCrypto is an open-source tool that allows you to manage your Ethereum accounts privately and securely. Developed by and for the community since 2015, we’re focused on building awesome products that put the power in people’s hands.
https://mycrypto.com
MIT License
1.37k stars 650 forks source link

CryptoCompare equivalent values API needs to be batch-requested #462

Closed wbobeirne closed 6 years ago

wbobeirne commented 6 years ago

Updated issue

Now that the API is more resilient, we'll need to handle the 500+ characters use case if you request an obscene number of tokens. We'll simply need to split the request into multiple groups, and combine all of the results at the end.

Old issue

If you load a wallet that has tokens that CryptoCompare doesn't know about, the price API call fails:

https://min-api.cryptocompare.com/data/price?fsym=ETH&tsyms=USD,EUR,GBP,BTC,CHF,REP,ETH,ANT,BeerCoin%20%F0%9F%8D%BA%20,CryptoCarbon,DAO,DGD,DGX%201.0,EDG,EMV,GNT,GUP,HMQ,ICN,JET,JetCoins,MCAP,MCO,MDA,MGO,MKR,MLN,NxC,OMG,PAY,PLU,PTOY,QRL,REP,RLC,RLT,ROUND,SGT,SHIT,SNGLS,SNT,STORJ,SWT,TIME,TRST,TaaS,Unicorn%20%F0%9F%A6%84%20,VERI,VRS,VSL,WINGS,XAUR

{"Response":"Error","Message":"tsyms param seems to be invalid.","Type":1,"Aggregated":false,"Data":[]}

The immediate problem is that the fetch call doesn't catch this (It comes back as a 200 response) The bigger problem is that one bad token kills this feature entirely. I have a few possible solutions:

1) Get a white-list of CryptoCompare tokens. This kinda sucks, and would have to be updated frequently. 2) Make the request, and if it fails, make the request again with only ETH. Make EquivalentValues show only ETH, and only allow them to select per token, not all tokens + ETH. 3) Try to work out better handling from CryptoCompare's side. Either an OPTIONS endpoint that describes which tokens it acknowledges, or have it return even with bad tokens, just omit them from the response.

cc @tayvano on number 3, do we have a relationship with them?

vcealicu commented 6 years ago

I'll fix it on our side. We'll just omit the tokens that don't trade. That's what we do for the pricemultifull path. I'll implement the same logic on this.

vcealicu commented 6 years ago

I found the bug.... it's just a limit that is being hit and I return the wrong error. The actual error should be: the tsyms length is higher than 100 (that's the current limit).

I'm pushing out a change that will return the correct error and increase the limit to 500 characters, so you can request on average around 80-100 assets per call (depending on the ticker length of each asset).

If a user has more than 80 assets, I would say split it into separate calls and this way you can handle unlimited assets, they will just take a few calls to get all the data.

vcealicu commented 6 years ago

It's running on dev overnight to make sure I'm not breaking anything and will deploy it live tomorrow morning. We just added 3 more vms on the API side, we are doing 16M calls per hour so I really need to test even the slightest changes properly. https://www.cryptocompare.com/coins/btc/post/p_301483

dternyak commented 6 years ago

Thank you for the extremely quick response and turn-around @vcealicu 😁

vcealicu commented 6 years ago

Fixed now, feel free to close the issue. No problem, just ping me on skype or email if you need anything else from us.

wbobeirne commented 6 years ago

Awesome, works great. I'll be updating the issue to reflect the work we need to do to do batch requests, but I'm going to re-label it as low. I can't imagine there are too many instances of 80+ coin requests.

SharonManrique commented 6 years ago

Added to Asana