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.87k stars 1.94k forks source link

[Kraken] adaptLimitOrder tradableAmount #1693

Closed MartinJ-Dev closed 7 years ago

MartinJ-Dev commented 7 years ago
public static LimitOrder adaptLimitOrder(KrakenOrder krakenOrder, String id) {

    KrakenOrderDescription orderDescription = krakenOrder.getOrderDescription();
    OrderType type = adaptOrderType(orderDescription.getType());
    BigDecimal tradableAmount = krakenOrder.getVolume().subtract(krakenOrder.getVolumeExecuted());
    CurrencyPair pair = adaptCurrencyPairOpenOrder(orderDescription.getAssetPair());
    Date timestamp = new Date((long) (krakenOrder.getOpenTimestamp() * 1000L));

    return new LimitOrder(type, tradableAmount, pair, id, timestamp,
        orderDescription.getPrice());
  }

Most exchanges define tradableAmount in a limit order to be the entire order size client placed. However this Kraken's adapter define it to be the order size minus the partially filled size.

It's not a bug accidentally introduced because the author intentionally implemented this way. However this is not consistent with most other exchanges' implementation. Should we change this?

npomfret commented 7 years ago

How can you see the amount remaining typically?

MartinJ-Dev commented 7 years ago

Do you mean the unfilled amount in an order? Then it's the order size minus partially filled size just like the above implementation. However the problem here is that exchanges are defining tradableAmount differently.

Achterhoeker commented 7 years ago

@MartianTech Which exchanges do not return the open amount for the order ?

As far as I known the exchanges Bitfinex, CexIO, Bitstamp, Kraken, Bittrex, Polo and Bleutrade return the amount still open for open open orders. (and not the original amount). This is also discussed a long time ago, then the decision was made that the amount still open needs te be returned.

MartinJ-Dev commented 7 years ago

@Achterhoeker Sorry if I incorrectly used the term "most exchanges" but to name a few, Gdax, Okcoin and Bitfinex, which you stated the opposite -

  public static OpenOrders adaptOrders(BitfinexOrderStatusResponse[] activeOrders) {

    List<LimitOrder> limitOrders = new ArrayList<>(activeOrders.length);

    for (BitfinexOrderStatusResponse order : activeOrders) {

      OrderType orderType = order.getSide().equalsIgnoreCase("buy") ? OrderType.BID : OrderType.ASK;
      OrderStatus status = adaptOrderStatus(order);
      CurrencyPair currencyPair = adaptCurrencyPair(order.getSymbol());
      Date timestamp = convertBigDecimalTimestampToDate(order.getTimestamp());

      limitOrders.add(new LimitOrder(orderType, order.getOriginalAmount(), currencyPair, String.valueOf(order.getId()), timestamp, order.getPrice(),
          order.getAvgExecutionPrice(), order.getExecutedAmount(), status));
    }

    return new OpenOrders(limitOrders);
  }

The issue I had here was not whether to use originalAmount or remainingAmount but the inconsistency across exchanges.

However, if I were to make a decision to choose between those, I prefer originalAmount. The reason is that in a limit order, originalAmount is the invariant and constant through its lifecycle. remainingAmount is inferred by subtracting executedAmount from originalAmount. This is also why Kraken as well as most other exchanges' API (I came across) for open orders always include the originalAmount but maybe the executedAmount or remainingAmount.

https://www.kraken.com/help/api#get-open-orders

Achterhoeker commented 7 years ago

@MartianTech Ai, thanks for pointing me to the fact that bitfinex isn't returning the left open amount. That means that that exchange is different from the rest i am using. But this discussion has been discussed before. I'll try to find the previous discussions. As you could expect, i should make a different decision ;-) Open orders do have to return the open order. (meaning, the order amount left open) For tracking the orders, the order id is unique and should be used a a handle to identify the order (so not the original amount) But there seems to inconsistency between xchanges in this behavior. Probably the best solution would be to add some configuration flag to pass to the open order query to give the behavior the caller wants ? But this would be a huge amount off work to make all xchanges supporting this. If a xchange isn't able to give the amount wanted, it could give the not supported exception.

MartinJ-Dev commented 7 years ago

@Achterhoeker Thanks for your info too.

Most exchanges API do return the originalAmount since it's the invariant. Although both remainingAmount and executedAmount are important, not every one return those because it's harder to implement from exchange's perspective. So it's hard to enforce every open order to contain those from our end.

I think the best implementation is Bitfinex's

private final BigDecimal originalAmount;
private final BigDecimal remainingAmount;
private final BigDecimal executedAmount;

It's clear, unambiguous and complete. It'd be great if LimitOrder can model like this. Then individual exchanges can adapt based on the information returned by its API.

Achterhoeker commented 7 years ago

@MartianTech I totally agree. But that results in an api change. Although, if we keep the current naming in the limit order class the same, and make as an addition field remainingAmount or executedAmount (only one is needed, the other can be calculated) then current users are not forced to adapt the api change. The value what they get in the (present) amount field is then always the original amount. This can be a change from the current implementation, but as already agreed, the current implementation is inconsistent. (so not very usable) If the new field is an null pointer, the behavior is not implemented or not given by the underlying exchange.

You think that this is an idea we can futher work on ? @timmolter Do you also think this can be a good change to get a consistent behavior ?

MartinJ-Dev commented 7 years ago

Yea that works for me too. Don't have to make API change. As long as we have consistent representation, I'm happy with it.

Achterhoeker commented 7 years ago

@MartianTech I've started with a first (small)kickoff for some exchanges, It this as you expected ? https://github.com/Achterhoeker/XChange/tree/openOrderRemainingAmount

MartinJ-Dev commented 7 years ago

@Achterhoeker Yea looks good to me. Left one comment. thanks for your prompt solution.