Bootstrap margin fee exchange configuration #351

2021-02-24T15:59:27-08:00

dazito commented

2021-02-24 15:59:27 -08:00

(Migrated from github.com)

This pull request replaces https://github.com/agonyforge/arbitrader/pull/320

dazito commented

2021-02-24 16:26:11 -08:00

(Migrated from github.com)

@AllirionX please have a look at my changes in the PaperTradeService. I had a merge conflict there. Direct link to the diff: https://github.com/agonyforge/arbitrader/pull/351/files#diff-6b843429246f24a5598b1910f6350f8e1cd50476dec55d1f4ddcec2b695cc0a4

@AllirionX please have a look at my changes in the `PaperTradeService`. I had a merge conflict there. Direct link to the diff: https://github.com/agonyforge/arbitrader/pull/351/files#diff-6b843429246f24a5598b1910f6350f8e1cd50476dec55d1f4ddcec2b695cc0a4

lilianchiassai (Migrated from github.com) requested changes

2021-02-24 19:25:35 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

A few things here:

about the logs, the paper exchange is just an exchange, it does not need to know if it's a long or short exchange, only if it's a BID or ASK order. Also the logs say exchange:{}|exchange:{} instead of exchange:{}|margin:{} I also try to start the paper trading logs by {} paper exchange: so we can identify logs coming from the paper trading internal logic easily
the way the fees are calculated, the margin fees will be used for all orders on the short exchange, even if margin is not needed (if we have sufficient funds to sell, or buy I think we should only pay the trade fees)

I don't now if it's how it's supposed to work: if we set the leverage when buying on the short exchange, are we supposed to pay margin fees? I would think so, but then the setLeverage(2) in the TradingService.executeOrderPair method should not be set for the short exchange BUY orders - as we don't need the leverage.

Maybe better way to do this would be to check if a margin trade is required or not depending on the balances and leverage?
Create

boolean isMarginRequired (LimitOrder order) {
        BigDecimal counterDelta = getCounterDelta(order);
        BigDecimal baseDelta = getBaseDelta(order);
 
        //Margin required because we don't have enough cash
        if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().counter).add(counterDelta).compareTo(BigDecimal.ZERO) <0)
            return true;

        //Margin required because we dont have enough crypto 
        if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().base).add(baseDelta).compareTo(BigDecimal.ZERO) <0)
            return true;

        //Margin required because the leverage was set
        //This can bit omitted, or moved the the checkBalance method, if we decide setting the order leverage does not automatically means we want to do a margin trade
        if(order.getLeverage() != null)
            return true;

        return false;
    }

Change checkBalance (LimitOrder order) to

private void checkBalance(LimitOrder order) {
        if(isMarginRequired(order) && !exchangeService.getExchangeMetadata(exchange).getMargin())
            throw new FundsExceededException();
    }

And use isMarginRequired to see if we should use the margin fees.

fee = isMarginRequired(order) && exchangeFee.getMarginFee().isPresent() ? exchangeFee.getMarginFee().get() : exchangeFee.getTradeFee();
LOGGER.info("{} paper exchange: margin enabled:{}|margin required:{}| order using {} fee", exchangeName, exchangeConfiguration.getMargin(), isMarginRequired(order), fee);

A few things here: - about the logs, the paper exchange is just an exchange, it does not need to know if it's a long or short exchange, only if it's a BID or ASK order. Also the logs say `exchange:{}|exchange:{}` instead of `exchange:{}|margin:{}` I also try to start the paper trading logs by `{} paper exchange:` so we can identify logs coming from the paper trading internal logic easily - the way the fees are calculated, the margin fees will be used for all orders on the short exchange, even if margin is not needed (if we have sufficient funds to sell, or buy I think we should only pay the trade fees) I don't now if it's how it's supposed to work: if we set the leverage when buying on the short exchange, are we supposed to pay margin fees? I would think so, but then the setLeverage(2) in the TradingService.executeOrderPair method should not be set for the short exchange BUY orders - as we don't need the leverage. Maybe better way to do this would be to check if a margin trade is required or not depending on the balances and leverage? Create ``` boolean isMarginRequired (LimitOrder order) { BigDecimal counterDelta = getCounterDelta(order); BigDecimal baseDelta = getBaseDelta(order); //Margin required because we don't have enough cash if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().counter).add(counterDelta).compareTo(BigDecimal.ZERO) <0) return true; //Margin required because we dont have enough crypto if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().base).add(baseDelta).compareTo(BigDecimal.ZERO) <0) return true; //Margin required because the leverage was set //This can bit omitted, or moved the the checkBalance method, if we decide setting the order leverage does not automatically means we want to do a margin trade if(order.getLeverage() != null) return true; return false; } ``` Change checkBalance (LimitOrder order) to ``` private void checkBalance(LimitOrder order) { if(isMarginRequired(order) && !exchangeService.getExchangeMetadata(exchange).getMargin()) throw new FundsExceededException(); } ``` And use `isMarginRequired` to see if we should use the margin fees. ``` fee = isMarginRequired(order) && exchangeFee.getMarginFee().isPresent() ? exchangeFee.getMarginFee().get() : exchangeFee.getTradeFee(); LOGGER.info("{} paper exchange: margin enabled:{}|margin required:{}| order using {} fee", exchangeName, exchangeConfiguration.getMargin(), isMarginRequired(order), fee); ```

dazito (Migrated from github.com) reviewed

2021-02-25 12:15:17 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

Thanks for the nice feedback 👍

I think we always pay margin fees even if we have enough balance for the borrowed coins (short), @scionaltera can you please confirm?

As far as I know setting value2 to order.getLeverage() is only needed for Liquid exchange and has no impact in the remaining exchanges. @scionaltera can you confirm this as well?

Thanks for the nice feedback :+1: I think we always pay margin fees even if we have enough balance for the borrowed coins (short), @scionaltera can you please confirm? As far as I know setting value`2` to `order.getLeverage()` is only needed for Liquid exchange and has no impact in the remaining exchanges. @scionaltera can you confirm this as well?

scionaltera (Migrated from github.com) reviewed

2021-02-25 15:17:11 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

The exchange should decide whether it's a margin or non-margin trade by looking at LimitOrder.getLeverage(). If it's not null, then it's a margin trade (regardless of long or short) and margin fees should be charged. If we really wanted to do it right for margin orders we wouldn't take the full amount out of the balance either. On a 2:1 trade (getLeverage() == 2) we'd only take half of the value away from the balance. It's not that important for Arbitrader at this time because we only ever set it to 2 and we never trade above our actual balance.

The exchange should decide whether it's a margin or non-margin trade by looking at `LimitOrder.getLeverage()`. If it's not null, then it's a margin trade (regardless of long or short) and margin fees should be charged. If we really wanted to do it right for margin orders we wouldn't take the full amount out of the balance either. On a 2:1 trade (`getLeverage() == 2`) we'd only take half of the value away from the balance. It's not that important for Arbitrader at this time because we only ever set it to 2 and we never trade above our actual balance.

lilianchiassai (Migrated from github.com) reviewed

2021-02-25 16:36:30 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

In that case, we can have a method useMargin to determine if we charge the margin fees or not.

private boolean useMargin(LimitOrder order) {
    return order.getLeverage() != null && exchangeService.getExchangeMetadata(exchange).getMargin();
}

The fees could be:

fee = useMargin(order)  && exchangeFee.getMarginFee().isPresent() ? exchangeFee.getMarginFee().get() : exchangeFee.getTradeFee();
LOGGER.info("{} paper exchange: margin enabled:{}|margin required:{}| order using {} fee", exchangeName, exchangeConfiguration.getMargin(), useMargin(order), fee);

and change the checkBalance method accordingly

private void checkBalance(LimitOrder order) {
    if(!useMargin(order)) {
        BigDecimal counterDelta = getCounterDelta(order);
        BigDecimal baseDelta = getBaseDelta(order);
 
        //we don't have enough cash
        if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().counter).add(counterDelta).compareTo(BigDecimal.ZERO) <0)
            throw new FundsExceededException();

        //we dont have enough crypto 
        if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().base).add(baseDelta).compareTo(BigDecimal.ZERO) <0)
            throw new FundsExceededException();
    } else {
         //TODO after implementing leverage in the paper exchange, we should check here if we have sufficient funds to cover the leverage
         // For now we can consider that we have unlimited funds for margin orders
    }
}

In that case, we can have a method `useMargin` to determine if we charge the margin fees or not. ``` private boolean useMargin(LimitOrder order) { return order.getLeverage() != null && exchangeService.getExchangeMetadata(exchange).getMargin(); } ``` The fees could be: ``` fee = useMargin(order) && exchangeFee.getMarginFee().isPresent() ? exchangeFee.getMarginFee().get() : exchangeFee.getTradeFee(); LOGGER.info("{} paper exchange: margin enabled:{}|margin required:{}| order using {} fee", exchangeName, exchangeConfiguration.getMargin(), useMargin(order), fee); ``` and change the checkBalance method accordingly ``` private void checkBalance(LimitOrder order) { if(!useMargin(order)) { BigDecimal counterDelta = getCounterDelta(order); BigDecimal baseDelta = getBaseDelta(order); //we don't have enough cash if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().counter).add(counterDelta).compareTo(BigDecimal.ZERO) <0) throw new FundsExceededException(); //we dont have enough crypto if(exchange.getPaperAccountService().getBalance(order.getCurrencyPair().base).add(baseDelta).compareTo(BigDecimal.ZERO) <0) throw new FundsExceededException(); } else { //TODO after implementing leverage in the paper exchange, we should check here if we have sufficient funds to cover the leverage // For now we can consider that we have unlimited funds for margin orders } } ```

scionaltera (Migrated from github.com) reviewed

2021-02-26 12:11:10 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

One other wrinkle I just thought of - the PaperExchange should probably check whether it's configured with margin: true or not and reject margin orders if it is not supposed to be a margin-capable exchange. It would be a bug if we tried to submit a margin order to a non-margin exchange, but stranger things have happened.

One other wrinkle I just thought of - the PaperExchange should probably check whether it's configured with `margin: true` or not and reject margin orders if it is not supposed to be a margin-capable exchange. It would be a bug if we tried to submit a margin order to a non-margin exchange, but stranger things have happened.

scionaltera (Migrated from github.com) requested changes

2021-02-26 12:25:08 -08:00

scionaltera (Migrated from github.com) left a comment

Lots of comments but only one actual problem. The way this code is written we might set tradeFee: 0.0026 and marginFee: 0.0002 based on the examples in the example configuration and the bot would return 0.0002 as the fee for the entire short trade which is incorrect. It should be 0.0028. I have pointed out all the places I saw where this needs to be fixed.

Lots of comments but only one actual problem. The way this code is written we might set `tradeFee: 0.0026` and `marginFee: 0.0002` based on the examples in the example configuration and the bot would return 0.0002 as the fee for the _entire short trade_ which is incorrect. It should be 0.0028. I have pointed out all the places I saw where this needs to be fixed.

application.example.yaml

This change to the comment is not accurate. The trade fee is applied to every trade. Long or short, margin or not.

src/main/java/com/r307/arbitrader/service/TradingService.java

This should be shortExchangeFee.getTradeFee().add(shortExchangeFee.getMarginFee().get()). The trade fee and margin fees need to get added together.

This should be `shortExchangeFee.getTradeFee().add(shortExchangeFee.getMarginFee().get())`. The trade fee and margin fees need to get added together.

src/main/java/com/r307/arbitrader/service/TradingService.java

Again if it's a short trade the trade fee and margin fee need to be added together.

src/main/java/com/r307/arbitrader/service/TradingService.java

If it's a margin trade the trade fee and margin fee need to be added together.

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

The fee on a margin trade needs to be the trade fee + the margin fee.

lilianchiassai (Migrated from github.com) reviewed

2021-02-26 13:04:18 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

It does check it with exchangeService.getExchangeMetadata(exchange).getMargin(). Currently, it rejects orders that would make any balance negative unless margin is enabled, via the check balance method

dazito (Migrated from github.com) reviewed

2021-02-26 13:25:35 -08:00

application.example.yaml

Aha, I didn't know that. I will fix it.

dazito (Migrated from github.com) reviewed

2021-02-26 13:31:16 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

Thanks @AllirionX for your great feedback!
@scionaltera yeah, I have implemented a check condition for that use case.

Thanks @AllirionX for your great feedback! @scionaltera yeah, I have implemented a check condition for that use case.

scionaltera (Migrated from github.com) reviewed

2021-02-26 14:20:54 -08:00

lilianchiassai (Migrated from github.com) reviewed

2021-02-28 05:10:55 -08:00

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

This check is not necessary here if the useMargin method throws a MarginNotSupportedException when the leverage is not null but the exchange is not configured for it.

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

If this checks for funds, for margin enabled, etc. should we rename both checkBalance to verifyOrder ?

src/main/java/com/r307/arbitrader/service/paper/PaperTradeService.java

					
				@ -260,3 +285,4 @@

				        BigDecimal feePercentage = getFee(order);

				        //Calculate fees in crypto currency

				        BigDecimal baseFee = exchangeService.getExchangeMetadata(exchange).getFeeComputation() == SERVER ? BigDecimal.ZERO : order.getOriginalAmount().multiply(feePercentage).setScale(BTC_SCALE,RoundingMode.HALF_EVEN);

This should probably be removed: it's logic is already included in checkBalance and useMargin.

This should throw a MarginNotSupportedException if the order leverage is not null and the exchange does not have margin enabled, basically moving the check on line 70 to this method

lilianchiassai (Migrated from github.com) reviewed

2021-02-28 16:30:15 -08:00

src/test/java/com/r307/arbitrader/service/paper/PaperTradeServiceTest.java

This test throws an exception because we did not set the order leverage.
We should have another test for the happy scenario (with order.getLeverage() != null ) that does not throw an exception

This test throws an exception because we did not set the order leverage. We should have another test for the happy scenario (with `order.getLeverage() != null` ) that does not throw an exception

dazito (Migrated from github.com) reviewed

2021-03-01 14:49:21 -08:00

src/test/java/com/r307/arbitrader/service/paper/PaperTradeServiceTest.java

Thanks, you were right. We were missing copying the leverage value and I also created an unit test to assert on that

scionaltera (Migrated from github.com) requested changes

2021-03-09 12:23:41 -08:00

src/main/java/com/r307/arbitrader/service/ExchangeService.java

This comment and the next one will be confusing now. It's "trade fee" and "margin fee".

src/main/java/com/r307/arbitrader/service/ExchangeService.java

Need to rename shortFee to marginFee here and 'longFee' to 'tradeFee' below both in variable names and comments. They're both the individual fee rates and not the combined fees.

Need to rename `shortFee` to `marginFee` here and 'longFee' to 'tradeFee' below both in variable names and comments. They're both the individual fee rates and not the combined fees.

src/main/java/com/r307/arbitrader/service/ExchangeService.java

This is incorrectly named. The method returns the margin fee, not the combined fees for a short trade.

src/main/java/com/r307/arbitrader/service/TradingService.java

Let's be as clear as we can be in these comments and say "Long exchange fee percent" on 183 and "Short exchange fee percent" on 185.

Between long and short exchanges and long and short trades and long and short fees there is a lot of room for ambiguity. That's why I'm trying so hard to make sure we use our terminology consistently.

Let's be as clear as we can be in these comments and say "Long exchange fee percent" on 183 and "Short exchange fee percent" on 185. Between long and short exchanges and long and short trades and long and short fees there is a lot of room for ambiguity. That's why I'm trying so hard to make sure we use our terminology consistently.

scionaltera (Migrated from github.com) reviewed

2021-03-12 10:03:34 -08:00

scionaltera (Migrated from github.com) left a comment

No description provided.

Once master is merged and conflicts resolved I think this should be OK to merge.

lilianchiassai (Migrated from github.com) reviewed

2021-03-21 16:19:50 -07:00

src/main/java/com/r307/arbitrader/service/TradingService.java

The variable names confused me:
ExchangeFee has two fees: tradeFee and marginFee
But here:
tradeFee is the long exchange tradeFee
marginFee is the sum of tradeFee and marginFee for the longExchange

To avoid confusion we could use different variable names:
longTradeFee (= tradeFee)
shortTotalFee (= tradeFee + marginFee)

The variable names confused me: ExchangeFee has two fees: tradeFee and marginFee But here: tradeFee is the long exchange tradeFee marginFee is the sum of tradeFee and marginFee for the longExchange To avoid confusion we could use different variable names: longTradeFee (= tradeFee) shortTotalFee (= tradeFee + marginFee)

src/main/java/com/r307/arbitrader/service/TradingService.java

See other comment.
tradeFee is the long exchange trade fee, and marginFee is the sum of the short exchange tradeFee and marginFee.
They should be logged as such:
LOGGER.debug("Long trade fee percent: {}", longTradeFee);
LOGGER.debug("Short trade and margin fee percent: {} + {} = {}", shortTradeFee, shortMarginFee, shortTotalFee);

See other comment. tradeFee is the long exchange trade fee, and marginFee is the sum of the short exchange tradeFee and marginFee. They should be logged as such: LOGGER.debug("Long trade fee percent: {}", longTradeFee); LOGGER.debug("Short trade and margin fee percent: {} + {} = {}", shortTradeFee, shortMarginFee, shortTotalFee);

src/main/java/com/r307/arbitrader/service/TradingService.java

See other comment, the same naming convention should apply here as well.

scionaltera (Migrated from github.com) reviewed

2021-03-21 16:32:41 -07:00

src/main/java/com/r307/arbitrader/service/TradingService.java

Agreed. Once you take the marginFee and add the tradeFee to it, it is specific to the short trade and it is the total of both the fees that apply to that trade.

Agreed. Once you take the `marginFee` and add the `tradeFee` to it, it is specific to the short trade and it is the total of both the fees that apply to that trade.