Bootstrap margin fee exch configuration #320

2021-01-31T08:40:38-08:00

dazito commented

2021-01-31 08:40:38 -08:00

(Migrated from github.com)

In this pull request we have support for margin fee configuration per exchange,

scionaltera (Migrated from github.com) requested changes

2021-02-10 00:36:22 -08:00

scionaltera (Migrated from github.com) left a comment

Just a few things I spotted reading through. I haven't had a chance to run the branch yet and try it out for myself. This will be nice to have.

application.example.yaml

					
				@ -170,2 +170,4 @@

				      # will use that value instead. If not, it will fall back to the configured value.

				      fee: 0.0026

				      # The amount of fees Kraken charges for margin trades. This fee is only needed if this exchange has margin set to true

				      marginFee: 0.0002

"The amount of additional fees Kraken charges for margin trades." ?

"The amount of **additional** fees Kraken charges for margin trades." ?

src/main/java/com/r307/arbitrader/config/ExchangeConfiguration.java

I think you've got an extra 'r' in there.

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

exchang -> exchange

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +5,4 @@

				public class ExchangeFee {

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

The names of these things seems inconsistent. In the YAML it's fee and marginFee, which appear to map to longFee and shortFee respectively. Since this is what getExchangeFee() returns now, it should have the same names in it as the configuration values from the YAML. In most cases it's either pulling updated versions of those values from the exchange API, or the values directly from the configuration.

I'd be happy to go with tradeFee and marginFee because fee by itself is now ambiguous. It would be backwards incompatible in the YAML unfortunately but we can make that change internally.

Also in the same vein, comments would be helpful. :)

The names of these things seems inconsistent. In the YAML it's `fee` and `marginFee`, which appear to map to `longFee` and `shortFee` respectively. Since this is what `getExchangeFee()` returns now, it should have the same names in it as the configuration values from the YAML. In most cases it's either pulling updated versions of those values from the exchange API, or the values directly from the configuration. I'd be happy to go with `tradeFee` and `marginFee` because `fee` by itself is now ambiguous. It would be backwards incompatible in the YAML unfortunately but we can make that change internally. Also in the same vein, comments would be helpful. :)

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +7,4 @@

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

				    public ExchangeFee(BigDecimal shortFee, BigDecimal longFee) {

I hate to nitpick but everywhere else in the code I've been careful to always do long, then short. Since both these arguments are the same type it's going to be an easy mistake to make if everything else is the other way around.

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +20,4 @@

				        this.shortFee = shortFee;

				    }

				    public BigDecimal getLongFee() {

Not super important but from a defensive code perspective this strikes me as a little strange, that the short one has Optional and this one doesn't. There isn't any protection in the setter or the constructor against setting this to null. I know it should never be null because there's always a fee, but it's totally possible to set it null by accident or malice and crash the program.

This object seems like a good candidate to be immutable anyway since the fees don't change after the instance is created. Then it would make more sense to not have Optional because there's no way you could set it null, and you could throw an exception if you try to make one with a null longFee.

Not super important but from a defensive code perspective this strikes me as a little strange, that the short one has `Optional` and this one doesn't. There isn't any protection in the setter or the constructor against setting this to `null`. I know it should never be null because there's always a fee, but it's totally possible to set it null by accident or malice and crash the program. This object seems like a good candidate to be immutable anyway since the fees don't change after the instance is created. Then it would make more sense to not have `Optional` because there's no way you could set it null, and you could throw an exception if you try to make one with a null `longFee`.

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

					
				@ -155,3 +178,3 @@

				        order.setCumulativeAmount(order.getOriginalAmount());

				        order.setFee(order.getCumulativeCounterAmount().multiply(exchangeService.getExchangeFee(exchange, (CurrencyPair)order.getInstrument(),false)));

				        order.setFee(order.getCumulativeCounterAmount().multiply(fee));

				        LOGGER.info("{} paper exchange: Order {} filled for {}{}, with {} fees.",

There are a lot of these messages. I'm a little concerned that since they're all the same it might be difficult to determine which code it's coming from. It's not that big of a deal since the remedy is to set the missing configuration setting. Just something that I've done in the past that has bitten me before - I always try to make each log message unique so it's easy to grep for. :)

dazito (Migrated from github.com) reviewed

2021-02-10 13:54:47 -08:00

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

					
				@ -155,3 +178,3 @@

				        order.setCumulativeAmount(order.getOriginalAmount());

				        order.setFee(order.getCumulativeCounterAmount().multiply(exchangeService.getExchangeFee(exchange, (CurrencyPair)order.getInstrument(),false)));

				        order.setFee(order.getCumulativeCounterAmount().multiply(fee));

				        LOGGER.info("{} paper exchange: Order {} filled for {}{}, with {} fees.",

I did not notice, I will make them unique by specifying what the method was trying to do. Adding or subtracting fees.

dazito (Migrated from github.com) reviewed

2021-02-10 14:57:53 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +5,4 @@

				public class ExchangeFee {

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

Yeah I tried to be backwards compatible while at the same time explicit what is what and follow the same naming logic we have in the code where we have named things like tradeFee and marginFee for consistency reasons because we tend to use shortSomething and longSomthing in other parts of the code like shortExchange, longExchange, shortLimitOrder, longLimitOrder, longTrade, shortTrade, longOpenOrders, shortOpenOrders and so on.

So I think in the code shortFee and longFee keep the consistency and follow the naming we have been using,

But then on the .xml I am not sure how to name them because we have margin and fee.

What do you think?

Yeah I tried to be backwards compatible while at the same time explicit what is what and follow the same naming logic we have in the code where we have named things like `tradeFee` and `marginFee` for consistency reasons because we tend to use `shortSomething` and `longSomthing` in other parts of the code like `shortExchange`, `longExchange`, `shortLimitOrder`, `longLimitOrder`, `longTrade`, `shortTrade`, `longOpenOrders`, `shortOpenOrders` and so on. So I think in the code `shortFee` and `longFee` keep the consistency and follow the naming we have been using, But then on the `.xml` I am not sure how to name them because we have `margin` and `fee`. What do you think?

dazito (Migrated from github.com) reviewed

2021-02-10 14:58:19 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +7,4 @@

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

				    public ExchangeFee(BigDecimal shortFee, BigDecimal longFee) {

Good point 👍

Good point :+1:

dazito (Migrated from github.com) reviewed

2021-02-10 15:30:55 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +20,4 @@

				        this.shortFee = shortFee;

				    }

				    public BigDecimal getLongFee() {

I agree with the object being immutable and I am usually all the way for immutable objects. I don't remember why I did not set it immutable neither I see a good reason to not be immutable so I will make the fields final.

The public Optional<BigDecimal> getShortFee() is optional because not all exchanges are margin exchanges so for some exchanges if we call getShortFee() we may actually get a null object and if we don't do a null check then we will get a NPE. Making getShortFee() return an Optional is a sign that this value may be null and it will force the developer to take that in consideration.

getLongFee() is does not return Optional because it should never be null. If it is null then the program should crash.
If I got you right, you would also let the program crash if shortFee is also null?

I agree with the object being immutable and I am usually all the way for immutable objects. I don't remember why I did not set it immutable neither I see a good reason to not be immutable so I will make the fields final. The `public Optional<BigDecimal> getShortFee()` is optional because not all exchanges are margin exchanges so for some exchanges if we call `getShortFee()` we may actually get a null object and if we don't do a null check then we will get a NPE. Making `getShortFee()` return an `Optional` is a sign that this value may be null and it will force the developer to take that in consideration. `getLongFee()` is does not return `Optional` because it should never be null. If it is null then the program should crash. If I got you right, you would also let the program crash if `shortFee` is also null?

scionaltera (Migrated from github.com) reviewed

2021-02-10 15:42:46 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +5,4 @@

				public class ExchangeFee {

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

I don't think they fit neatly into the "long" and "short" buckets like the other things do. The longFee isn't really that, it's also used for short trades in addition to the marginFee. It's a fee applied to every trade. The marginFee is only applied to margin trades and only when they open. Those happen to all be "short" so far in our model but I believe you'd still pay the marginFee if you opened a long margin trade on Kraken?

I don't think they fit neatly into the "long" and "short" buckets like the other things do. The `longFee` isn't really that, it's also used for short trades in addition to the `marginFee`. It's a fee applied to **every** trade. The `marginFee` is only applied to margin trades and only when they open. Those happen to all be "short" so far in our model but I believe you'd still pay the `marginFee` if you opened a _long_ margin trade on Kraken?

scionaltera (Migrated from github.com) reviewed

2021-02-10 15:46:54 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +20,4 @@

				        this.shortFee = shortFee;

				    }

				    public BigDecimal getLongFee() {

Don't get me wrong, I'm all for using Optional. I just have an irrational need for things to be symmetrical. 😂

If there's no reason not to make it immutable, let's do that. Then there wouldn't be a way to make getLongFee() return null (by setting it null after construction or passing it into the constructor) and break the contract of the object. Let's also make it blow up if you pass it a null longFee through the constructor, or at least annotate it @NotNull so the IDE warns you not to do that.

Don't get me wrong, I'm all for using `Optional`. I just have an irrational need for things to be symmetrical. 😂 If there's no reason _not_ to make it immutable, let's do that. Then there wouldn't be a way to make `getLongFee()` return null (by setting it null after construction or passing it into the constructor) and break the contract of the object. Let's also make it blow up if you pass it a null `longFee` through the constructor, or at least annotate it `@NotNull` so the IDE warns you not to do that.

dazito (Migrated from github.com) reviewed

2021-02-14 05:41:12 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +5,4 @@

				public class ExchangeFee {

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

I renamed them to tradeFee and marginFee and also renamed on the .yml file

I renamed them to `tradeFee` and `marginFee` and also renamed on the `.yml` file

dazito (Migrated from github.com) reviewed

2021-02-14 05:50:54 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +20,4 @@

				        this.shortFee = shortFee;

				    }

				    public BigDecimal getLongFee() {

I changed the constructor to include @NotNull BigDecimal tradeFee and inside the constructor I added Objects.requireNonNull(tradeFee);

Regarding the Optional, are you fine with?

I changed the constructor to include `@NotNull BigDecimal tradeFee` and inside the constructor I added `Objects.requireNonNull(tradeFee);` Regarding the `Optional`, are you fine with?

scionaltera (Migrated from github.com) requested changes

2021-02-23 13:04:56 -08:00

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

Still a couple extra 'r's :)

src/test/java/com/r307/arbitrader/service/cache/ExchangeFeeCacheTest.java

I don't see a test for the short fees, and I have a concern there. I see in the example that you set the "fee" to 0.0026 and the "marginFee" to 0.0002. In getShortFee() you retrieve the marginFee but I can't seem to find where you add fee to it. The way I understand it, fee is applied to every trade while marginFee is added to fee for margin trades, for a total of 0.0028.

I don't see a test for the short fees, and I have a concern there. I see in the example that you set the "fee" to 0.0026 and the "marginFee" to 0.0002. In `getShortFee()` you retrieve the `marginFee` but I can't seem to find where you add `fee` to it. The way I understand it, `fee` is applied to **every** trade while `marginFee` is **added** to `fee` for margin trades, for a total of 0.0028.

dazito (Migrated from github.com) reviewed

2021-02-23 15:38:36 -08:00

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

Rrrrrrr! :D

dazito (Migrated from github.com) reviewed

2021-02-23 15:42:28 -08:00

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

Hmm something is wrong here. I don't have that extra 'r' in my code and I have pushed everything 🤔
Maybe you are seeing an old commit?

This is how the code is on my side:
`@Nullable
private BigDecimal getShortFee(ExchangeConfiguration exchangeMetadata) {
final BigDecimal shortFee;

// If exchange is set for margin trade then get the configured margin (short) fee
if (exchangeMetadata.getMargin()) {
    if (exchangeMetadata.getMarginFeeOverride() != null) {
        shortFee = exchangeMetadata.getMarginFeeOverride();
    }
    else {
        shortFee = exchangeMetadata.getMarginFee();
    }
}
else {
    shortFee = null;
}

return shortFee;

}`

Searching for getMarginFeeOverrride returns no results.

Hmm something is wrong here. I don't have that extra 'r' in my code and I have pushed everything :thinking: Maybe you are seeing an old commit? This is how the code is on my side: `@Nullable private BigDecimal getShortFee(ExchangeConfiguration exchangeMetadata) { final BigDecimal shortFee; // If exchange is set for margin trade then get the configured margin (short) fee if (exchangeMetadata.getMargin()) { if (exchangeMetadata.getMarginFeeOverride() != null) { shortFee = exchangeMetadata.getMarginFeeOverride(); } else { shortFee = exchangeMetadata.getMarginFee(); } } else { shortFee = null; } return shortFee; }` Searching for `getMarginFeeOverrride` returns no results.

dazito (Migrated from github.com) reviewed

2021-02-23 15:50:22 -08:00

src/test/java/com/r307/arbitrader/service/cache/ExchangeFeeCacheTest.java

Here I am only testing if the ExchangeFeeCache works as expected. I test only adding and retrieving to/from the cache and assert that I retrieve what I was expecting to.

I do not test trading logic here, I don't think testing trading logic belong here because ExchangeFeeCache has no logic regarding calculating fees but only acting as a fee caching mechanism. It only has two methods:

setCachedFee
computeCacheKey

Here I am only testing if the `ExchangeFeeCache` works as expected. I test only adding and retrieving to/from the cache and assert that I retrieve what I was expecting to. I do not test trading logic here, I don't think testing trading logic belong here because `ExchangeFeeCache` has no logic regarding calculating fees but only acting as a fee caching mechanism. It only has two methods: - setCachedFee - computeCacheKey

scionaltera (Migrated from github.com) reviewed

2021-02-23 18:08:53 -08:00

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

It's on your branch on GitHub. It's possible you have changes on your computer that haven't been pushed?

https://github.com/dazito/arbitrader/blob/bootstap-margin-fee-exch-configuration/src/main/java/com/r307/arbitrader/service/ExchangeService.java#L331

Be sure to double check your remotes given that we changed the name from scionaltera to agonyforge. Maybe it's getting pushed but not to where you want? 🤷‍♂️

It's on your branch on GitHub. It's possible you have changes on your computer that haven't been pushed? https://github.com/dazito/arbitrader/blob/bootstap-margin-fee-exch-configuration/src/main/java/com/r307/arbitrader/service/ExchangeService.java#L331 Be sure to double check your remotes given that we changed the name from scionaltera to agonyforge. Maybe it's getting pushed but not to where you want? 🤷‍♂️

scionaltera (Migrated from github.com) reviewed

2021-02-23 18:19:23 -08:00

src/test/java/com/r307/arbitrader/service/cache/ExchangeFeeCacheTest.java

Right but I don't see any such test anywhere in your PR and I couldn't find the code where the fees are added together. It's totally possible that I just didn't see it and if that's the case please point it out.

I've read through a few times now and I'm still not seeing how we don't get a fee of 0.0002 for short trades because getShortFee() returns getMarginFee() and I don't see any place where we add getLongFee() to it.

That's why I was complaining earlier about "long" and "short" fees. It's not accurate. There's a "trade" fee on every trade and an additional "margin" fee on margin trades. Any trade should have fees equal to the "trade" fee. Any margin trade, long or short, should have fees equal to "trade fee + margin fee". It's not related to long or short, it's related to whether you're trading on margin or not.

Right but I don't see any such test anywhere in your PR and I couldn't find the code where the fees are added together. It's totally possible that I just didn't see it and if that's the case please point it out. I've read through a few times now and I'm still not seeing how we don't get a fee of 0.0002 for short trades because `getShortFee()` returns `getMarginFee()` and I don't see any place where we add `getLongFee()` to it. That's why I was complaining earlier about "long" and "short" fees. It's not accurate. There's a "trade" fee on every trade and an additional "margin" fee on margin trades. _Any_ trade should have fees equal to the "trade" fee. _Any_ margin trade, long or short, should have fees equal to "trade fee + margin fee". It's not related to long or short, it's related to whether you're trading on margin or not.

scionaltera (Migrated from github.com) reviewed

2021-02-23 18:27:09 -08:00

application.example.yaml

					
				@ -168,3 +168,3 @@

				      # The amount of fees Kraken charges. If an exchange's API supports requesting fees at runtime, the bot

				      # The amount of fees Kraken charges for long trades. If an exchange's API supports requesting fees at runtime, the bot

				      # will use that value instead. If not, it will fall back to the configured value.

This comment change is not accurate. The 0.0026 fee value is applied to all trades. The 0.0002 marginFee is applied in addition to fee to any margin trades.

This comment change is not accurate. The 0.0026 `fee` value is applied to all trades. The 0.0002 `marginFee` is applied _in addition to_ `fee` to any margin trades.

scionaltera (Migrated from github.com) reviewed

2021-02-23 18:37:40 -08:00

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

This will choose either the long or the short fee. What we want here, and in subtractFees(), and in the regular feeComputation == SERVER flow, is to choose whether we use the trade fee alone or the trade fee plus the margin fee. Right now that's based on long or short but it's important to remember that we're making a big assumption there. Short trades must be on margin, but even though it's not possible (yet) in Arbitrader it is possible to make a long trade on margin. That's why I want the names to be right.

This will choose either the long or the short fee. What we want here, and in `subtractFees()`, and in the regular `feeComputation == SERVER` flow, is to choose whether we use the trade fee alone or the trade fee plus the margin fee. Right now that's based on long or short but it's important to remember that we're making a big assumption there. Short trades _must_ be on margin, but even though it's not possible (yet) in Arbitrader it is possible to make a long trade on margin. That's why I want the names to be right.

scionaltera (Migrated from github.com) reviewed

2021-02-23 18:38:48 -08:00

src/main/java/com/r307/arbitrader/service/model/ExchangeFee.java

					
				@ -0,0 +5,4 @@

				public class ExchangeFee {

				    private BigDecimal shortFee;

				    private BigDecimal longFee;

Probably related to the 'rrr' thing but I don't see this fixed in this PR. There must be some changes that didn't get pushed correctly?

dazito commented

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

(Migrated from github.com)

This PR is replaced by https://github.com/agonyforge/arbitrader/pull/351

Commenting is not possible because the repository is archived.

No reviewers

scionaltera

No labels

No milestone

No project

No assignees

1 participant

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

scion/arbitrader!320

Rows
Columns