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

Explain when Cryptofeed crashes during pairs retrieval #378

Merged
merged 3 commits into from Jan 6, 2021

Conversation

olibre
Copy link
Contributor

@olibre olibre commented Jan 6, 2021

During the pairs retrieval at startup time, some exchanges may reply an erroneous response.
Currently Cryptofeed does not always handle the exception, and crashes.

Some users think this is a bug in Cryptofeed: #371 (comment)

This PR improves the user experience by providing a clear explanation of the failure.

cryptofeed/pairs.py Outdated Show resolved Hide resolved
@bmoscon
Copy link
Owner

bmoscon commented Jan 6, 2021

looks good other than this one change. I can make the change myself it you do not have the time, just let me know

@olibre
Copy link
Contributor Author

olibre commented Jan 6, 2021

Hi Bryant
I have added a function to log and raise the shared failure explanation.
There is also a commit to normalize IOTA in CoinGecko.
I wish you the best for 2021:sparkles:

@bmoscon bmoscon merged commit 2fafa76 into bmoscon:master Jan 6, 2021
@bmoscon
Copy link
Owner

bmoscon commented Jan 6, 2021

thanks for the PR!

@olibre
Copy link
Contributor Author

olibre commented Jan 7, 2021

Hi Bryant
I have introduced a regression in that PR. Do not publish a new release, I will send you a fix.

The issue with CoinGecko is the huge number of supported pairs, and the difficulty to list all of them. The endpoint https://api.coingecko.com/api/v3/coins/list returns 5441 quote assets. I think there are about 15400 combinations of pairs {BaseAsset}-{QuoteAsset}. I will try to find a way to provide them. There is about 232 bases, 70 quotes and 2430 assets. The total of unique active assets is 2503.

@olibre olibre deleted the explain-pair-retrieval-failure branch January 7, 2021 10:04
@yohplala
Copy link
Contributor

yohplala commented Jan 7, 2021

Hi @olibre ,

I thought about that and had in mind to add to the dict of pairs a last _QuoteAsset key (specific to Coingecko pair retrieval) that would be the list of the quote currencies.

Then, in the function checking that pair exist,

  • either it proceeds as currently coded (checking existence of a pair in pair_dict keys)
  • or, within a new if to implement:
        if `_QuoteAsset`key exist in pair dict:
               then,
                     # checks are separate
                     check that `BaseAsset` exists in key
                     check that `QuoteAsset` exists in pair_dict['_QuoteAsset`] list

@bmoscon
Copy link
Owner

bmoscon commented Jan 7, 2021

the pairs functions need to return all legal symbols/pairs/etc, so however you want to do it, thats fine by me

@olibre
Copy link
Contributor Author

olibre commented Jan 7, 2021

Hi @yohplala Thank you for your ideas.
I have now some free time to investigate this issue.
I am currently checking we could get the assets and pairs from the CoinGecko API...

@yohplala
Copy link
Contributor

yohplala commented Jan 7, 2021

Hi @olibre
One thing I have uncorrectly read in your 1st message. The quote assets, you get them with
https://www.coingecko.com/api/documentations/v3#/simple/get_simple_supported_vs_currencies
But when you do all combinations, this remains a lot.
This is why instead of generating all possible pairs, only checking base_asset & quote_asset separately is more efficient.

@olibre
Copy link
Contributor Author

olibre commented Jan 7, 2021

Thanks @yohplala Yes I think we will do some thing in this way...

On my side I think we can use the endpoint supported_vs_currencies to get the list of the base QUOTE assets. This is my try using API v3:

curl https://api.coingecko.com/api/v3/simple/supported_vs_currencies > QuoteAssets.json
jq '.[]' < QuoteAssets.json | tr -d '"' | tr a-z A-Z > QuoteAssets.normalized

EDIT: replaced Base -> Quote

@yohplala
Copy link
Contributor

yohplala commented Jan 7, 2021

Thanks @yohplala Yes I think we will do some thing in this way...

On my side I think we can use the endpoint supported_vs_currencies to get the list of the base assets. This is my try using API v3:

curl https://api.coingecko.com/api/v3/simple/supported_vs_currencies > BaseAssets.json
jq '.[]' < BaseAssets.json | tr -d '"' | tr a-z A-Z > BaseAssets.normalized

supported vs currencies gives you quote assets, not base assets.

@olibre
Copy link
Contributor Author

olibre commented Jan 7, 2021

🤦

I sometimes confuse Quote and Base 😉

@olibre
Copy link
Contributor Author

olibre commented Jan 7, 2021

To clarify the situation, I will send another PR to fix the regression I have introduced with this PR. I see two solutions:

  1. I simply revert the heart of coingecko_pairs()
  2. I propose something different to better fit all pairs combinations

I am currently working on the second solution. But if I do not find anything better, I will just stick on the first solution.

@yohplala has also good ideas.

@olibre
Copy link
Contributor Author

olibre commented Jan 8, 2021

Hi @yohplala
For your information, I have just initiated a discussion about our subject: #387

@olibre
Copy link
Contributor Author

olibre commented Jan 8, 2021

Hi @bmoscon and @yohplala

Sorry, I thought I had introduced a bug (regression), but it was an earlier commit that changed the coingecko_pairs() body 8970107

def coingecko_pairs():
-    quote_c = requests.get('https://api.coingecko.com/api/v3/coins/list').json()
-    # Normalization
-    normalized = dict({'miota': 'iota'})
-    # Base currencies are defined manually (USD + BTC + ETH).
-    # The full list from Coingecko is not used, as the generated dict of pairs would be tremendous.
-    # base_c =  requests.get('https://api.coingecko.com/api/v3/simple/supported_vs_currencies').json()
-    base_c = (('USD', 'usd'), ('BTC', 'btc'), ('ETH', 'eth'))
-    return {(f"{q['symbol']}{PAIR_SEP}{bk}".upper() if q['symbol'] not in normalized else f"{normalized[q['symbol']]}{PAIR_SEP}{bk}".upper()): f"{q['id']}_{bv}" for q in quote_c for bk, bv in base_c}
+    data = requests.get('https://api.coingecko.com/api/v3/coins/list').json()
+    return {entry['symbol'].upper(): entry['id'] for entry in data}

The latter coingecko_pairs() has two issues that I will detail in the discussion #387

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.

None yet

3 participants