Repository: ccxt/ccxt
Pull request: Fix Coinegg loadMarkets and fetchTickers
This fixes #5086 . Note that this is a breaking change since IDs are now using the identifiers returned by CoinEgg (`base_quote` as opposed to `basequote`).

Problem statement:
CoinEgg LoadMarkets Unknown Markets
OS: Ubuntu 18.03
Programming Language version: Node v10
CCXT version: Latest
Exchange: CoinEgg
Method: LoadMarkets

```
Exchange Name:  coinegg
Exchange Markets: [
             '0/BTC',            '1/BTC',            '2/BTC',            '3/BTC',
             '4/BTC',            '5/BTC',            '6/BTC',            '7/BTC',
             '8/BTC',            '9/BTC',           '10/BTC',           '11/BTC',
            '12/BTC',           '13/BTC',           '14/BTC',           '15/BTC',
            '16/BTC',           '17/BTC',           '18/BTC',           '19/BTC',
            '20/BTC',           '21/BTC',           '22/BTC',           '23/BTC',
            '24/BTC',           '25/BTC',           '26/BTC',           '27/BTC',
            '28/BTC',           '29/BTC',           '30/BTC',           '31/BTC',
            '32/BTC',           '33/BTC',           '34/BTC',           '35/BTC',
            '36/BTC',           '37/BTC',           '38/BTC',           '39/BTC',
            '40/BTC',           '41/BTC',           '42/BTC',           '43/BTC',
            '44/BTC',           '45/BTC',           '46/BTC',           '47/BTC',
            '48/BTC',           '49/BTC',           '50/BTC',           '51/BTC',
            '52/BTC',           '53/BTC',           '54/BTC',           '55/BTC',
            '56/BTC',           '57/BTC',           '58/BTC',           '59/BTC',
            '60/BTC',           '61/BTC',           '62/BTC',           '63/BTC',
            '64/BTC',           '65/BTC',           '66/BTC',           '67/BTC',
            '68/BTC',           '69/BTC',           '70/BTC',           '71/BTC',
            '72/BTC',           '73/BTC',           '74/BTC',           '75/BTC',
            '76/BTC',           '77/BTC',           '78/BTC',           '79/BTC',
            '80/BTC',            '0/ETH',            '1/ETH',            '2/ETH',
             '3/ETH',            '4/ETH',            '5/ETH',            '6/ETH',
             '7/ETH',            '8/ETH',            '9/ETH',           '10/ETH',
            '11/ETH',           '12/ETH',           '13/ETH',           '14/ETH',
            '15/ETH',           '16/ETH',           '17/ETH',           '18/ETH',
  ... 224 more items
]
coinegg fetchTicker failed due to exchange error: coinegg coin does not exist 0/BTC


```

Hints from issue discussion:
If you check their verbose responses, it looks like they have changed the way they serve markets.

```
mbp:ccxt igorkroitor$ node examples/js/cli coinegg symbols --verbose
fetch:
 coinegg GET https://www.coinegg.com/coin/btc/allcoin?t=1557407713046
Request:
 {}
 undefined

handleRestResponse:
 coinegg GET https://www.coinegg.com/coin/btc/allcoin?t=1557407713046 200 OK
Response:
 { Date: 'Thu, 09 May 2019 13:15:14 GMT',
  'Content-Type': 'text/html; charset=utf-8',
  'Transfer-Encoding': 'chunked',
  Connection: 'close',
  'Set-Cookie': 'lang=en_US; expires=Sun, 06-May-2029 13:15:14 GMT; Max-Age=315360000; path=/; domain=.coinegg.com',
  Vary: 'Accept-Encoding',
  'Expect-Ct': 'max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"',
  Server: 'cloudflare',
  'Cf-Ray': '4d43f0e01d9e63a7-FRA',
  'Content-Encoding': 'gzip' }
 <script type="text/javascript">alert('Page Not Found');self.location='/'</script>
```

So, consider it broken at this point.
OK, Considered broken
@kroitor This is the same error as we saw before where a string is treated as an array and iterated over character by character - still set on supporting that behavior? I would much rather see the `fetchMarkets` function throwing an error here than trying to interpret the characters in an HTML body as elements in a deserialized JSON result...
@Legogris 

> This is the same error as we saw before where a string is treated as an array and iterated over character by character - still set on supporting that behavior?

It's more like a side-effect, we just don't check `safeValue (response, 'markets', [])` in `fetchMarkets`, because an exchange isn't expected to break down like that too often. So, yeah, you're absolutely, right, we just need to sanitize the input as described [here](https://github.com/ccxt/ccxt/blob/master/CONTRIBUTING.md#sanitizing-input-with-safe-methods) before looping over markets...

> I would much rather see the fetchMarkets function throwing an error here than trying to interpret the characters in an HTML body as elements in a deserialized JSON result...

Yes, we should throw in this case. And we should generalize it and throw in all such cases throughout the lib. I just didn't have the time to get to it, sorry for the recent delays on my side, I'll get to it soon after I resolve the backlog of PRs and existing contributions (currently working on okex v3).

Commit message: Change URL for fetching markets

Patch to review:
diff --git a/js/coinegg.js b/js/coinegg.js
index e67bc429c771e..80fac31eae7cc 100644
--- a/js/coinegg.js
+++ b/js/coinegg.js
@@ -23,7 +23,7 @@ module.exports = class coinegg extends Exchange {
             'urls': {
                 'logo': 'https://user-images.githubusercontent.com/1294454/36770310-adfa764e-1c5a-11e8-8e09-449daac3d2fb.jpg',
                 'api': {
-                    'web': 'https://www.coinegg.com/coin',
+                    'web': 'https://trade.coinegg.com/web',
                     'rest': 'https://api.coinegg.com/api/v1',
                 },
                 'www': 'https://www.coinegg.com',
@@ -34,7 +34,7 @@ module.exports = class coinegg extends Exchange {
             'api': {
                 'web': {
                     'get': [
-                        '{quote}/allcoin',
+                        'symbol/ticker?right_coin={quote}',
                         '{quote}/trends',
                         '{quote}/{base}/order',
                         '{quote}/{base}/trades',
@@ -156,23 +156,20 @@ module.exports = class coinegg extends Exchange {
         let result = [];
         for (let b = 0; b < quoteIds.length; b++) {
             let quoteId = quoteIds[b];
-            let bases = await this.webGetQuoteAllcoin ({
+            let response = await this.webGetSymbolTickerRightCoinQuote ({
                 'quote': quoteId,
             });
-            if (bases === undefined)
+            let tickers = response.data;
+            if (tickers === undefined)
                 throw new ExchangeNotAvailable (this.id + ' fetchMarkets() for "' + quoteId + '" returned: "' + this.json (bases) + '"');
-            let baseIds = Object.keys (bases);
-            let numBaseIds = baseIds.length;
-            if (numBaseIds < 1)
-                throw new ExchangeNotAvailable (this.id + ' fetchMarkets() for "' + quoteId + '" returned: "' + this.json (bases) + '"');
-            for (let i = 0; i < baseIds.length; i++) {
-                let baseId = baseIds[i];
-                let market = bases[baseId];
+            for (let i = 0; i < tickers.length; i++) {
+                let ticker = tickers[i];
+                let id = ticker['symbol'];
+                let baseId = id.split ('_')[0];
                 let base = baseId.toUpperCase ();
                 let quote = quoteId.toUpperCase ();
                 base = this.commonCurrencyCode (base);
                 quote = this.commonCurrencyCode (quote);
-                let id = baseId + quoteId;
                 let symbol = base + '/' + quote;
                 let precision = {
                     'amount': 8,
@@ -201,7 +198,7 @@ module.exports = class coinegg extends Exchange {
                             'max': undefined,
                         },
                     },
-                    'info': market,
+                    'info': ticker,
                 });
             }
         }
@@ -477,8 +474,6 @@ module.exports = class coinegg extends Exchange {
         let url = this.urls['api'][apiType] + '/' + this.implodeParams (path, params);
         let query = this.omit (params, this.extractParams (path));
         if (api === 'public' || api === 'web') {
-            if (api === 'web')
-                query['t'] = this.nonce ();
             if (Object.keys (query).length)
                 url += '?' + this.urlencode (query);
         } else {


Code review comments:
diff_hunk: @@ -258,36 +255,14 @@ module.exports = class coinegg extends Exchange {
 
     async fetchTickers (symbols = undefined, params = {}) {
         await this.loadMarkets ();
-        let quoteIds = this.options['quoteIds'];
+        const markets = Object.values (this.markets_by_id);
         let result = {};
-        for (let b = 0; b < quoteIds.length; b++) {
-            let quoteId = quoteIds[b];
-            let tickers = await this.webGetQuoteAllcoin ({
-                'quote': quoteId,
-            });
-            let baseIds = Object.keys (tickers);
-            if (!baseIds.length) {
-                throw new ExchangeError ('fetchTickers failed');
-            }
-            for (let i = 0; i < baseIds.length; i++) {
-                let baseId = baseIds[i];
-                let ticker = tickers[baseId];
-                let id = baseId + quoteId;
-                if (id in this.markets_by_id) {
-                    let market = this.marketsById[id];
-                    let symbol = market['symbol'];
-                    result[symbol] = this.parseTicker ({
-                        'high': ticker[4],
-                        'low': ticker[5],
-                        'buy': ticker[2],
-                        'sell': ticker[3],
-                        'last': ticker[1],
-                        'change': ticker[8],
-                        'vol': ticker[6],
-                        'quoteVol': ticker[7],
-                    }, market);
-                }
+        for (let i = 0; i < markets.length; i++) {
+            const market = markets[i];
+            if (symbols !== undefined && symbols.indexOf (market['symbol']) < 0) {
+                continue;
             }
+            result[market['symbol']] = await this.fetchTicker (market['symbol'], params);
path: js/coinegg.js
text: @user1:
Oops )

I suggest to drop `fetchTickers`, because we stick to one https request per unified call (so methods can be combined in userland, and always behave in a more-or-less unified fashion). We very rarely send more than one call per method, only if we absolutely have to and it cannot be done otherwise. 

Also, we usually don't invent `fetchTickers`, if the exchange in question does not have it. So, in this case the [exchange metainfo](https://github.com/ccxt/ccxt/wiki/Manual#exchange-metadata) would contain `has['fetchTickers'] == 'emulated'`. 

However, due to rate-limiting and other factors, we don't emulate `fetchTickers` with multiple consequent calls looping over all markets with `fetchTicker`. It would be hard to account for all possible use-cases, rate-limits, retry-strategies and so on...  

Instead, we just tell the user the actual truth, which is: `exchange.has['fetchTickers'] == false && exchange.has['fetchTicker'] == true` and let the user wrap `fetchTicker` how he wants )

@author:
Makes sense, updating with `fetchTickers` removed as suggested.