Change the volume calculation so the trade stays market neutral #326

2021-02-09T06:34:53-08:00

lilianchiassai commented

2021-02-09 06:34:53 -08:00

(Migrated from github.com)

The goal is to have a profit independent from the market moves.

scionaltera (Migrated from github.com) reviewed

2021-02-10 14:41:06 -08:00

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

I realize this is being super nitpicky and has no effect on how the code runs but I've tried really hard in the code to always do long then short so it's always in the same order. I think that kind of consistency helps make the code easier to read and more predictable.

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

Is this still a TODO? It looks like the lines following it are making the adjustment.

lilianchiassai (Migrated from github.com) reviewed

2021-02-10 14:50:18 -08:00

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

This is still a TODO. The next line only adjust the orders to the echange steps, they don't ensure that shortVolume = longVolume * feeFactor

lilianchiassai (Migrated from github.com) reviewed

2021-02-10 14:55:14 -08:00

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

No problem. I intend to refactor the volume calculation according to the maths in this comment: https://github.com/scionaltera/arbitrader/issues/325#issuecomment-776331413

scionaltera (Migrated from github.com) reviewed

2021-02-12 00:13:18 -08:00

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

					
				@ -577,0 +612,4 @@

				                tradeVolume.getShortOrderVolume(),

				                shortLimitPrice,

				                spread.getShortTicker().getBid().toPlainString(),

				                spread.getCurrencyPair().counter.getSymbol(),

Would it make sense to make a new object like TradeVolumes with two BigDecimals in it so you could have a method getTradeVolumes(...) that just figures it all out in one shot and hands it to you?

I do like having the lower level methods available to get long from short and short from long, but in the higher level trade logic it might be nice to hide the details.

Would it make sense to make a new object like `TradeVolumes` with two `BigDecimal`s in it so you could have a method `getTradeVolumes(...)` that just figures it all out in one shot and hands it to you? I do like having the lower level methods available to get long from short and short from long, but in the higher level trade logic it might be nice to hide the details.

I like how you broke these up. It would make it pretty easy to make them package private and write unit tests to guarantee they're doing the math correctly. ;)

lilianchiassai (Migrated from github.com) reviewed

2021-02-12 03:54:41 -08:00

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

					
				@ -577,0 +612,4 @@

				                tradeVolume.getShortOrderVolume(),

				                shortLimitPrice,

				                spread.getShortTicker().getBid().toPlainString(),

				                spread.getCurrencyPair().counter.getSymbol(),

I like the idea, a TradeVolumes could hold the 4 volumes (long, long adjusted for FeeComputation.CLIENT, short, short adjusted for FeeComputation.CLIENT) and hold the logic to adjust the volumes according to the fees/step size etc.

lilianchiassai (Migrated from github.com) reviewed

2021-02-12 03:55:54 -08:00

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

					
				@ -577,0 +612,4 @@

				                tradeVolume.getShortOrderVolume(),

				                shortLimitPrice,

				                spread.getShortTicker().getBid().toPlainString(),

				                spread.getCurrencyPair().counter.getSymbol(),

I will move the methods to a TradeVolumes class, and each public method of the class could be unit tested to make sure our volume related maths is correct.

scionaltera (Migrated from github.com) reviewed

2021-02-16 12:55:53 -08:00

scionaltera (Migrated from github.com) left a comment

No description provided.

Looks good to me. Let me know if/when you're ready to merge.

lilianchiassai commented

2021-03-01 08:11:20 -08:00

(Migrated from github.com)

Instead of calculating the volumes - that was leading to rounding issues for FeeComputation.CLIENT exchanges - we now calculate the fees directly and we make sure everything is rounded properly.

As we decided to remove support for certain configuration, I had to add a couple of IllegalArgumentException, but ideally the exchange that would trigger those exceptions should be disabled during start up (this should be part of a future PR).

The configuration not supported are:

FeeComputation.CLIENT exchanges with and amount step size
FeeComputation.CLIENT exchanges with fees >= 1%. Calculating the volume to order is only an approximation that could have an error between -scale/2 and scale/2. For the fee calculation, that leads to an error margin of scale/2 * fee and to make sure that the approximation is not causing a rounding issue, we limit the fee to 1%

Also the exit trade does not check the amount step size or scale as the entry trade should have taken care of that, and will throw an exception if it happens.

Instead of calculating the volumes - that was leading to rounding issues for `FeeComputation.CLIENT` exchanges - we now calculate the fees directly and we make sure everything is rounded properly. As we decided to remove support for certain configuration, I had to add a couple of `IllegalArgumentException`, but ideally the exchange that would trigger those exceptions should be disabled during start up (this should be part of a future PR). The configuration not supported are: - `FeeComputation.CLIENT` exchanges with and amount step size - `FeeComputation.CLIENT` exchanges with fees >= 1%. Calculating the volume to order is only an approximation that could have an error between -scale/2 and scale/2. For the fee calculation, that leads to an error margin of `scale/2 * fee` and to make sure that the approximation is not causing a rounding issue, we limit the fee to 1% - Also the exit trade does not check the amount step size or scale as the entry trade should have taken care of that, and will throw an exception if it happens.

scionaltera (Migrated from github.com) reviewed

2021-03-02 23:34:00 -08:00

scionaltera (Migrated from github.com) reviewed

2021-03-03 10:54:46 -08:00

dazito commented

2021-03-03 11:46:23 -08:00

(Migrated from github.com)

Can you merge master into this branch?

dazito (Migrated from github.com) requested changes

2021-03-03 12:01:56 -08:00

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

					
				@ -232,0 +257,4 @@

				            return;

				        }

				        logEntryTrade(spread, shortExchangeName, longExchangeName, exitTarget, tradeVolume, longFeeComputation, shortFeeComputation, longLimitPrice, shortLimitPrice, isForcedOpenCondition);

Can you pass in here the minimum profit and the market neutrality value and then format the email and instant notification (telegram message) to include them?

First you need to merge master into this branch otherwise you won't have this new instant message code.

Can you pass in here the minimum profit and the market neutrality value and then format the email and instant notification (telegram message) to include them? First you need to merge master into this branch otherwise you won't have this new instant message code.