knowm / XChange

XChange is a Java library providing a streamlined API for interacting with 60+ Bitcoin and Altcoin exchanges providing a consistent interface for trading and accessing market data.
http://knowm.org/open-source/xchange/
MIT License
3.88k stars 1.94k forks source link

Bitmex exchange metadata and use of currency pairs is inconsistent #2886

Closed badgerwithagun closed 5 years ago

badgerwithagun commented 5 years ago

If I do the following:

 Exchange exchange = ExchangeFactory.INSTANCE.createExchange(BitmexExchange.class.getName());
 System.out.println(exchange.getExchangeMetaData().getCurrencyPairs().keySet());

I get:

[BTC/BTC, EOS/BTC, BCH/BTC, ADA/BTC, ETH/USD, ZEC/BTC, TRX/BTC, XLM/BTC, LTC/BTC, \
XRP/BTC, BTC/USD, ETH/BTC, DASH/BTC, XMR/BTC, ETC/BTC]

There a a few problems here

This last bit is interesting. The code in org.knowm.xchange.bitmex.BitmexAdapters.adaptToExchangeMetaData seems to do this deliberately, but...

System.out.println(exchange.getMarketDataService().getTicker(CurrencyPair.BTC_USD));

... returns null, whereas...

System.out.println(exchange.getMarketDataService().getTicker(new CurrencyPair("XBT", "USD")));

... works (as I would expect, to be honest!)

How should this work? Should Bitmex accept XBT as one might expect (in which case it's the exchange metadata implementation that's wrong), or should it translate BTC to XBT internally (in which case the first of those two examples should work, but not the second)?

My inclination is to remove all the cleverness in org.knowm.xchange.bitmex.BitmexAdapters.adaptToExchangeMetaData and make it use Bitmex-native currency codes throughout.

badgerwithagun commented 5 years ago

It seems to be a bit worse than I thought. Here's what I have to do to get the ticker and trades for ADA:

System.out.println(exchange.getMarketDataService().getTicker(new CurrencyPair("ADA", "Z18")).getLast());
System.out.println(exchange.getMarketDataService().getTrades(new CurrencyPair("ADA", "BTC"), BitmexPrompt.QUARTERLY));

So I can't use the same currently pair to get trades as I do for tickers. Anyone know why? Why not just make it ADA/Z18, XBT/Z18, XBT/USD across the board, in line with Bitmex' own representation, and get rid of BitmexPrompt?

It seems doomed to failure to try and make the pairs line up with other exchanges, given that Mex's swap contracts aren't even sold/priced in line with other exchanges (when buying 100 on XBT/USD you're actually buying 100 USD's worth, not 100 BTC).

timmolter commented 5 years ago

This is probably the biggest issue with XChange right now. Clients should use the symbols that the exchanges defined for each currency and XChange should stay out of the way and not do any conversions. Conversions at a global level to introduce consistency is a feature that doesn't yet exist, but there is demand for it.

Regarding the json file if there is incorrect metadata, it simply needs to be updated and merged. :)

badgerwithagun commented 5 years ago

OK, when I get some time soon, I'm going to put together a pull request that moves this mapping out of the main API and into an additional Bitmex-specific conversion API so that long-lived code can still correctly determine the current active contract.

So instead of:

exchange.getMarketDataService().getTicker(new CurrencyPair("BTC", "USD"))

The developer should do something like this:

// Returns XBT/H19 (at the moment)
CurrencyPair activeContract = ((BitmexExchange) exchange).determineActiveContract("BTC", "USD", QUARTERLY);
exchange.getMarketDataService().getTicker(activeContract);

Then the normal XChange API methods should work in the way one would expect. I'll also try and clean up the metadata.

jakubkalicki commented 5 years ago

Hey, @badgerwithagun I could take care of this issue, if you like.

badgerwithagun commented 5 years ago

That'd be amazing, @jakubkalicki. It never seems to make it to the top of my priority list!

jakubkalicki commented 5 years ago

Ok, i'm starting then 😃

badgerwithagun commented 5 years ago

Dzięki! Happy to review/help test/etc.

badgerwithagun commented 5 years ago

A few test cases based on my thinking here:

assertEquals(new CurrencyPair("XBT", "USD"), exchange.determineActiveContract("BTC", "USD", PERPETUAL);
assertEquals(new CurrencyPair("LTC", "H19"), exchange.determineActiveContract("LTC", "BTC", QUARTERLY);
assertEquals(new CurrencyPair("ETH", "H19"), exchange.determineActiveContract("ETH", "BTC", QUARTERLY);
jakubkalicki commented 5 years ago

Ok, now it's implemented in the way you suggested.

I'm wondering what should be done with this adaptToExchangeMetaData method. Some options here:

  1. Just remove it, it's seems to be useless
  2. Load currency pairs and currencies in the same way as in determineActiveContract method
  3. Something else?
badgerwithagun commented 5 years ago

Fantastic! Two remaining parts to this:

We still want to be able to query the exchange metadata via XChange and get a list of the currently active contracts using:

Exchange exchange = ExchangeFactory.INSTANCE.createExchange(BitmexExchange.class.getName());
 System.out.println(exchange.getExchangeMetaData().getCurrencyPairs());

So the results of the API query have to be merged into the ExchangeMetaData. What we want to remove is all the conversion from the Bitmex instrument symbols, and just return them directly converted into CurrencyPair. So I don't think the method should be removed, just simplified - no more symbol conversions, just augment the static method data from bitmex.json, just as all the other exchange implementations do (see Binance or Bitfinex for examples).

The second issue is that several methods like MarketDataService.getTrades and getOrderBook (and presumably others on TradeService and AccountService) are actually expecting the converted symbols. It's inconsistent. One must fetch tickers like this:

exchange.getMarketDataService().getTicker(new CurrencyPair("XBT", "USD"));

But one must fetch trades and order books like this:

exchange.getMarketDataService().getTrades(new CurrencyPair("BTC", "USD"), PERPETUAL);
exchange.getMarketDataService().getOrderBook(new CurrencyPair("BTC", "USD"), PERPETUAL);

Instead, all API methods should just accept new CurrencyPair("XBT", "USD"). The only API method where BitmexPrompt should be accepted is your new determineActiveContract method.

jakubkalicki commented 5 years ago

Second issue is solved, now BitmexMarketDataService is consistent with other services. I have also added few integration tests. Probably tomorrow i will take care of meta data.

jakubkalicki commented 5 years ago

Done