OpenBB-finance / OpenBB

Investment Research for Everyone, Everywhere.
https://openbb.co
Other
33.81k stars 3.08k forks source link

[Bug] Unable to load data for some cryptocurrencies (LTC-USD@Coinbase for example) #1618

Closed marcelonyc closed 2 years ago

marcelonyc commented 2 years ago

Describe the bug When I try to load data for LTC-USD from Coinbase I get an error

Error: 'float' object has no attribute 'upper'

To Reproduce

/crypto 
load --coin ltc --source cb --vs USD  -i 1day

Desktop (please complete the following information):

Additional context I traced the issue to how the crypto maps get loaded and how the Crypto name is retrieved. It seems that, in the case of LTC, some entries from the Yahofinance map add too many nan rows. I tried to figure out a way to load this, but I am having some trouble understanding how / why the crypto maps are loaded.

image

If I change this line, I can load LTC-USD

cryptocurrency_helpers.py: 347 coins_map_df = prepare_all_coins_df().set_index("Symbol").dropna()

The error occurs when you get to this line cryptocurrency_helpers.py:927 coin, currency = symbol_coinbase.upper(), currency.upper() symbol_coinbase is empty because this code line returns nan cryptocurrency_helpers.py:487

coin_map_df.iloc[0]

image

Proposed solution If I understand the map loading process, the goal is to get a list of the coins available on each exchange and link them to other exchanges by the coin symbol. I believe the problem is that at least one list from Yahoo Finance, has the same Symbol for many pairs. This will cause duplicate records and later break the symbol lookup but return nan when a value actually exists.

I propose the following

bolshoytoster commented 2 years ago

@marcelonyc I believe it's just a type mismatch (calling upper on a float).

Could you send a link to a dataset that causes this error?

marcelonyc commented 2 years ago

@bolshoytoster I am loading from Coinbase directly. The error happens before the data is loaded. It works for btc or eth, but not for ltc.

bolshoytoster commented 2 years ago

@marcelonyc Looks like this occurs when a openbb_terminal/cryptocurrency/data/*_gecko_map.json file is missing the specified currency.

The best solution is probably to either add a warning and/or use a default value.

bolshoytoster commented 2 years ago

I've implemented a warning:

diff --git a/openbb_terminal/cryptocurrency/cryptocurrency_helpers.py b/openbb_terminal/cryptocurrency/cryptocurrency_helpers.py
index b6b6ab1..ed870fa 100644
--- a/openbb_terminal/cryptocurrency/cryptocurrency_helpers.py
+++ b/openbb_terminal/cryptocurrency/cryptocurrency_helpers.py
@@ -832,6 +832,16 @@ def load_ta_data(
         quoted currency
     """

+    if pd.isna(coin_map_df[{
+        "bin": "Binance",
+        "cp": "CoinPaprika",
+        "cg": "CoinGecko",
+        "cb": "Coinbase",
+        "yf": "YahooFinance",
+    }[source]]):
+        console.print(f"{source} doesn't support the specified --coin")
+        return pd.DataFrame(), ""
+
     limit = kwargs.get("limit", 100)
     interval = kwargs.get("interval", "1day")
     days = kwargs.get("days", 30)

I can open a PR if you want.

marcelonyc commented 2 years ago

@bolshoytoster Your change does not fix the problem. LTC-USD is a valid Coinbase pair.

bolshoytoster commented 2 years ago

@marcelonyc looks like the issue is in openbb_terminal/cryptocurrency/data/coingecko_coins.json, this fixed it for me:

diff --git a/openbb_terminal/cryptocurrency/data/coingecko_coins.json b/openbb_terminal/cryptocurrency/data/coingecko_coins.json
index e24c71c..93ec98a 100644
--- a/openbb_terminal/cryptocurrency/data/coingecko_coins.json
+++ b/openbb_terminal/cryptocurrency/data/coingecko_coins.json
@@ -7205,62 +7205,62 @@
     "name": "BIDR"
   },
   {
-    "id": "binance-peg-avalanche",
+    "id": "avalanche",
     "symbol": "avax",
     "name": "Binance-Peg Avalanche"
   },
   {
-    "id": "binance-peg-bitcoin-cash",
+    "id": "bitcoin-cash",
     "symbol": "bch",
     "name": "Binance-Peg Bitcoin Cash"
   },
   {
-    "id": "binance-peg-cardano",
+    "id": "cardano",
     "symbol": "ada",
     "name": "Binance-Peg Cardano"
   },
   {
-    "id": "binance-peg-dogecoin",
+    "id": "dogecoin",
     "symbol": "doge",
     "name": "Binance-Peg Dogecoin"
   },
   {
-    "id": "binance-peg-eos",
+    "id": "eos",
     "symbol": "eos",
     "name": "Binance-Peg EOS"
   },
   {
-    "id": "binance-peg-filecoin",
+    "id": "filecoin",
     "symbol": "fil",
     "name": "Binance-Peg Filecoin"
   },
   {
-    "id": "binance-peg-firo",
+    "id": "firo",
     "symbol": "firo",
     "name": "Binance-Peg Firo"
   },
   {
-    "id": "binance-peg-iotex",
+    "id": "iotex",
     "symbol": "iotx",
     "name": "Binance-Peg IoTeX"
   },
   {
-    "id": "binance-peg-litecoin",
+    "id": "litecoin",
     "symbol": "ltc",
     "name": "Binance-Peg Litecoin"
   },
   {
-    "id": "binance-peg-ontology",
+    "id": "ontology",
     "symbol": "ont",
     "name": "Binance-Peg Ontology"
   },
   {
-    "id": "binance-peg-polkadot",
+    "id": "polkadot",
     "symbol": "dot",
     "name": "Binance-Peg Polkadot"
   },
   {
-    "id": "binance-peg-xrp",
+    "id": "xrp",
     "symbol": "xrp",
     "name": "Binance-Peg XRP"
   },
marcelonyc commented 2 years ago

@bolshoytoster you can't alter that file. The file is the output of this URL https://api.coingecko.com/api/v3/coins/list

Take a look at the README.md in the data directory.

The change that I am asking for needs to be implemented in code.

bolshoytoster commented 2 years ago

@marcelonyc in that case the only real fix would be quite hacky.

minhhoang1023 commented 2 years ago

@marcelonyc Thank you for the excellent analysis! ✨

You're correct here that the error is due to the nan, where there's no equivalent symbol for coinbase. This is because CoinGecko, which is the primary exchange where we develop the mapping, has the same symbols for two assets (e.g. litecoin, and binance-pegged-litecoin). So we when merged the df, there's no equivalent mapping of binance-pegged-litecoin for Coinbase.

I will be implementing your proposal, which is to dropna before doing iloc[0].