Have commission as part of the OrderFillEvent and Position

[scoriiu]

I think it would be pretty useful to incorporate commission in the order fills and position. For position the commission should be part of the realised PNL as well.

If you agree on this, I can open a PR to support the above.

[cjdsellers]

I agree with this. For example BitMEX does supply the commission in the execution events, not every broker does however. Although I still think this isn't a problem as that can just be set to zero where the amount isn't available directly.

The ongoing challenge is to allow the platform to integrate with as many exchanges and brokers as possible. Not all options will be utilized, some data won't be available everywhere. So we just need appropriate controls and alerts to the users so that everything is communicated explicitly.

You're most welcome to open a PR.

[scoriiu]

I think most of the brokers supply the commission one way or another.

I took a look at the code of BacktestExecClient and I see 2 possible options for passing on the commission to the fill event. One would be to send it via the constructor, but this would mean to create the fill event after the commission is calculated, so code has to be moved around a bit. Second would be to set the commission value in the event once it is calculated. I prefer the first one, but have to think to an elegant way of doing it.

Do you have any thoughts on this?

[cjdsellers]

I also prefer the first one - we should ensure event objects remain immutable.

I'd probably add it in the OrderFillEvent including OrderPartiallyFilled and OrderFilled constructors just after average_price.

Make sure its a Money object too.

There's a stub event method in the tesk_kit module, however there's alot of places it will need to be added including the serialization module... non-trivial.

You're most welcome to have a go.

[scoriiu]

Yes, I will use a Money object for sure. Will open a PR.

[scoriiu]

See https://github.com/nautechsystems/nautilus_trader/pull/38. Subsequently I would like to take into account the commission when calculating the realized pnl in the position, as a separate PR.

[cjdsellers]

Good job on the PR. That sounds like a plan.

[scoriiu]

I see now that Position is represented in quote currency. I'm more used to seeing position represented in base currency. This representation in quote currency I've seen only for the inverse contracts (e.g. Bitmex XBTUSD).

I wonder what is the reason behind this design choice?

[cjdsellers]

This is also an issue for the OrderFill events, where currently there's a quote_currency property. Its probably more correct to use the settlement currency which should always be the account base currency? Not certain on this.

The design decision was probably a naive attempt to calculate PNL given the price difference between open and close prices which are in the quote currency - without knowledge of what the account base currency actually is.

All of this logic can and needs to be tightened up.

It would be straight forward to pick up the base currency where the event is generated in the adapter client, some API's even provide the settlement currency (BitMEX).

[scoriiu]

In my opinion is more intuitive to use the base currency, it is more inline with the real execution flow of the exchanges. But as I said above, it can be both, quote and base. For example Bybit exchange offers direct and inverse contract for BTCUSD. For the direct contract, the position/order fills are denominated in BTC (base), as opposed to the inverse contract where the position/order are denominated in USD (quote).

So I prefer the default to be base currency. This is also inline with other trading platforms I've worked with so far (e.g. backtrader, zipline and others).

With the current design, I will still have to convert the commission to quote currency in order to subtract it from the realised PNL. I opened a WIP to illustrate this: https://github.com/nautechsystems/nautilus_trader/pull/42/files#diff-c04b7634b30840e137a489cf532c55e3R464

The problem I currently see with tis approach, is when writing tests and trying to reverse calculate the position commission, it will not match the initial commission -> https://github.com/nautechsystems/nautilus_trader/blob/59f874017173fb834ff6907814ef54fa1c8dde4c/tests/unit_tests/backtest/test_backtest_simulated_broker.py#L470

I will have to see if it's because of the arithmetical precision or some other factor.

[cjdsellers]

Well in the above case if its rounded to a precision of 2 it would indeed be equal to 180.01

I did a quick refactoring, didn't realize you were still working on it. I'll push and then hand that back over to you.

[cjdsellers]

Just thinking ahead to bases denominated in crypto such as BTC, the precision will not always be 2...

[scoriiu]

Haha, no problem. Usually I place a WIP: in front to mark the PR as work in progress. This is a good point regarding the precision.

[cjdsellers]

That's a good convention. Noted.

[scoriiu]

True, usually the precision is exchange specific. Every exchange provide trading rules through a REST endpoint. The rules specify how each instrument should be traded (precision, min-max size/price, rounding types, ticker size and more). Thankfully this is all taken care by CCXT when dealing with the exchange in live mode.

The rules can be fetched also via CCXT, by calling ccxt.fetch_markets() which returns the same format for all the exchanges. This is for the case when we might be interested in parsing the rules.

[cjdsellers]

Ok this is good. Note the existence of the Instrument object which contains the precision, tick size, min-max trade sizes etc. I see the potential for a CryptoInstrument object which inherits Instrument. We can add all of the properties the CCXT API has, possibly the base instrument will need refactoring also moving some of the FOREX specific properties up to ForexInstrument.

So Instrument is originally where the precision value for a Price would come from.

[scoriiu]

Sounds great.

[scoriiu]

Closing the PR as this was addressed.