Periodically update unrealized pnl for position in database #967

da-kami · 2023-07-20T08:31:24Z

stacked on top of #966

Note: I did not bother setting the unrealized_pnl to null when we close the position. Not sure that's actually necessary.

fixes: #932

coordinator/src/node/unrealized_pnl.rs

coordinator/src/bin/coordinator.rs

luckysori · 2023-07-21T05:28:55Z

coordinator/src/node/unrealized_pnl.rs

+        Direction::Short => (1.0_f32, position.leverage),
+    };
+
+    // the position in the database is the trader's position, our direction is opposite


luckysori · 2023-07-21T05:31:11Z

coordinator/src/node/unrealized_pnl.rs

+    let (long_leverage, short_leverage) = match position.direction {
+        Direction::Long => (position.leverage, 1.0_f32),
+        Direction::Short => (1.0_f32, position.leverage),
+    };


It's a bit scary that this kind of logic is located in this module. It's probably hard to define where this should be located atm. Maybe we can leave a comment talking about how the position is the trader's position and the coordinator is always leverage 1.

I did a refactoring where I moved the coordinator_pnl onto the Position struct and added tests.
This particular bit remains within that pnl calculation function. Eventually we might also want to extract this.

Instead of using 1.0 I opted for using the COORDINATOR_LEVERAGE constant that I made public in the coordinator's node module.

luckysori · 2023-07-21T05:32:37Z

coordinator/src/node/unrealized_pnl.rs

+    let current_price = match position.direction {
+        trade::Direction::Long => quote.bid_price,
+        trade::Direction::Short => quote.ask_price,
+    };


I can't reason about this. I assume the direction corresponds to the trader's direction. Given that this is the coordinator's perspective, does this affect which price it is. I would welcome a comment here.

Since it is the price that the trader will get, we have to use that price. We cannot argue from the side of the coordinator here, otherwise we would try to close the position with two different prices.
I'll see to extract a function that we can use throughout the code - it's not the first time this code triggers a lengthy thinking process :)

coordinator/src/node/unrealized_pnl.rs

bonomat · 2023-07-24T09:31:03Z

coordinator/src/node/unrealized_pnl.rs

+        Direction::Short => (1.0_f32, position.leverage),
+    };
+
+    // the position in the database is the trader's position, our direction is opposite


This is related: #941

coordinator/migrations/2023-07-20-055142_unrealized_pnl_positions/up.sql

coordinator/src/position/models.rs

bonomat · 2023-07-24T10:03:25Z

coordinator/src/position/models.rs

+
+        let coordinator_pnl = position.calculate_coordinator_pnl(quote).unwrap();
+
+        assert_eq!(coordinator_pnl, -9_090_909);


It's hard for me to follow these amounts :(

Ok, it took me a while to understand what is happening and why this value is correct:

In my words expressed: In this example, the person who went long, bought $20,000 worth of BTC at the price of 20,000, i.e. 1 BTC
At the price of $22,000 he sells $20,000 worth of BTC, i.e. he sells it for 0.909090909 BTC.
The difference is his profit, i.e.

1 BTC - 0.909090909 BTC = 0.09090909 BTC profit

I agree with both your statements: It is hard to follow the amounts and it's hard to understand why it's correct.
Any idea how to make it better? 😬

I tried to come up with better numbers, but I feel the numbers are not the problem but more the model and the way we structure and express things. I could not come up with something better so far though. We already had a hard time with such kind of tests in ItchySats as far as I remember.

I don't know... Maybe add the calculation from my comment and add asserts for that?

Eitherway... if we have to touch this again, we very likely will need to go through this thought process again 🙈

I think it's worth adding a comment like what Philipp said above each test, because I agree that the thought process is not intuitive.

coordinator/src/position/models.rs

bonomat · 2023-07-24T11:34:18Z

coordinator/src/node/expired_positions.rs

@@ -57,10 +58,7 @@ pub async fn close(node: Node) {
        };

        let closing_price = match BitmexClient::get_quote(&position.expiry_timestamp).await {


I know it's not your code, but this price should not be from bitmex but from our orderbook/counterparty.

I see where you are going, but for me using a price from the orderbook only makes sense if there is also an order that is eventually executed. We don't do this at the moment.
The coordinator can still use an orderbook price and just simulate the match - but I find this a bit odd.
I opened #988 to capture this; feel free to change the description

I see where you are going, but for me using a price from the orderbook only makes sense if there is also an order that is eventually executed. We don't do this at the moment.

Totally agree. We should be doing this though.

bonomat · 2023-07-24T11:37:56Z

coordinator/src/node/expired_positions.rs

-                trade::Direction::Long => quote.bid_price,
-                trade::Direction::Short => quote.ask_price,
-            },
+            Ok(quote) => closing_price(position.direction, quote),


The closing_price is not wrong but for me it adds to the confusion in this whole story and I had to do some mental gymnastics to follow what is happening here 🙈

What about we do something like this:

// 1. get counter direction, i.e. he was long, now he needs to go short, or he was short, now needs to go long let direction = position.direction.opposite(); // 2. TODO: get latest quote from the orderbook/counter party let quote = BitmexClient::get_quote(&position.expiry_timestamp) .await .unwrap(); // 3. get closing price based on order direction let closing_price = match direction { Direction::Long => quote.ask_price, Direction::Short => quote.bid_price, };

bonomat · 2023-07-24T11:39:30Z

crates/trade/src/cfd.rs

+/// is effectively going short to close a long order.
+/// If the trader closes a short position then the trader gets the best ask price because the trader
+/// is effectively going long to close a short order.
+pub fn closing_price(trader_direction: Direction, quote: Quote) -> Decimal {


What do you think of implementing this on Quote?

I would then not call it closing_price but price_for_direction or so.

It's hard to fit this into my mental model, but I can see how it could make the code easier :)

For me the Quote does not care about any underlying financial instrument, it just depicts the best bid and ask price in the market (weighted by volume).
For me "closing" is always related to the position, not to the quote - the quote does not care about closing.
Given that the quote defines the closing price I can see how it could somehow make sense to add it there, but if I would just look at the quote out of context I would be confused.

Maybe you can share your mental model - it might help to make all this code better :)

For me "closing" is always related to the position, not to the quote - the quote does not care about closing.

Exactly, that's why I want to get rid of the term closing :D

I would add something like:

impl Quote { fn get_price_for_direction(&self, direction: Direction) -> Decimal { match direction { Direction::Long => self.ask_price, Direction::Short => self.bid_price, } } }

and then from the outside, i.e. where you currently call closing_price you do:

get_price_for_direction(position.direction.oposite())`

Then we know, we want the price for the opposite direction.

If we know the `closing_price` then it is used for updating the unrealized pnl. Note that the realized pnl is *only* set once we update it based on the closed contract. The realized pnl is not updated based on the execution price, because there is no execution guarantee.

We use the same logic in three different places already, better extract it into a function to make this clearer.

Additionally adds more tests to `trade::cfd` that reflect the pnl calculation on the trader side. This reflects that the traders losses are the coordinator's profits and vice versa.

da-kami · 2023-07-25T02:35:12Z

bors r+

bors · 2023-07-25T02:48:26Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

luckysori

Thank you, I like it!

coordinator/src/db/positions.rs

coordinator/src/position/models.rs

luckysori · 2023-07-25T01:54:07Z

coordinator/src/position/models.rs

+
+        let coordinator_pnl = position.calculate_coordinator_pnl(quote).unwrap();
+
+        assert_eq!(coordinator_pnl, -9_090_909);


I think it's worth adding a comment like what Philipp said above each test, because I agree that the thought process is not intuitive.

coordinator/src/position/models.rs

994: Some cleanup and alignment r=da-kami a=da-kami A `Quote` is basically a more detailed `Price`; the `Quote` uses the `Price`'s `get_price_for_direction` logic to get the price by direction. --- `@luckysori` comments from #967 should all be worked in Co-authored-by: Daniel Karzel <[email protected]>

da-kami requested review from bonomat, holzeis and luckysori July 20, 2023 08:31

holzeis reviewed Jul 20, 2023

View reviewed changes

coordinator/src/node/unrealized_pnl.rs Show resolved Hide resolved

coordinator/src/bin/coordinator.rs Outdated Show resolved Hide resolved

coordinator/src/bin/coordinator.rs Outdated Show resolved Hide resolved

da-kami force-pushed the feat/track_unrealized_pnl branch 2 times, most recently from 7dbfaaa to c6bbf8a Compare July 20, 2023 11:35

da-kami mentioned this pull request Jul 21, 2023

Feat/track pnl #966

Merged

da-kami force-pushed the feat/track-pnl branch 2 times, most recently from ac754fc to f6030a8 Compare July 21, 2023 03:05

da-kami force-pushed the feat/track_unrealized_pnl branch from c6bbf8a to 7f7b17a Compare July 21, 2023 03:08

Base automatically changed from feat/track-pnl to main July 21, 2023 03:19

Periodically update unrealized pnl for position in database

6d15d74

holzeis approved these changes Jul 21, 2023

View reviewed changes

da-kami force-pushed the feat/track_unrealized_pnl branch from 7f7b17a to 0691230 Compare July 21, 2023 05:32

da-kami changed the base branch from main to fix/optional-temporary-contract-id-in-position July 21, 2023 05:33

Base automatically changed from fix/optional-temporary-contract-id-in-position to main July 21, 2023 05:33

luckysori approved these changes Jul 21, 2023

View reviewed changes

da-kami force-pushed the feat/track_unrealized_pnl branch from 0691230 to 3ec9b25 Compare July 24, 2023 03:47

holzeis assigned da-kami Jul 24, 2023

da-kami requested a review from luckysori July 24, 2023 06:09

bonomat reviewed Jul 24, 2023

View reviewed changes

coordinator/migrations/2023-07-20-055142_unrealized_pnl_positions/up.sql Outdated Show resolved Hide resolved

bonomat reviewed Jul 24, 2023

View reviewed changes

coordinator/src/position/models.rs Outdated Show resolved Hide resolved

bonomat reviewed Jul 24, 2023

View reviewed changes

coordinator/src/position/models.rs Show resolved Hide resolved

bonomat reviewed Jul 24, 2023

View reviewed changes

da-kami mentioned this pull request Jul 24, 2023

Re-think how we can handle collaboratively closing expired positions #988

Closed

da-kami force-pushed the feat/track_unrealized_pnl branch from be1206d to cf3f06f Compare July 25, 2023 01:10

da-kami added 2 commits July 25, 2023 12:11

Extract closing_price into a function

0ba98eb

We use the same logic in three different places already, better extract it into a function to make this clearer.

Add tests for coordinator pnl calculation

2479fcf

Additionally adds more tests to `trade::cfd` that reflect the pnl calculation on the trader side. This reflects that the traders losses are the coordinator's profits and vice versa.

da-kami force-pushed the feat/track_unrealized_pnl branch from cf3f06f to 2479fcf Compare July 25, 2023 02:15

bors bot merged commit 039f0c3 into main Jul 25, 2023

bors bot deleted the feat/track_unrealized_pnl branch July 25, 2023 02:48

luckysori approved these changes Jul 25, 2023

View reviewed changes

da-kami mentioned this pull request Jul 25, 2023

Some cleanup and alignment #994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Periodically update unrealized pnl for position in database #967

Periodically update unrealized pnl for position in database #967

da-kami commented Jul 20, 2023 •

edited

Loading

luckysori Jul 21, 2023

luckysori Jul 21, 2023

da-kami Jul 24, 2023

luckysori Jul 21, 2023

da-kami Jul 24, 2023

bonomat Jul 24, 2023

bonomat Jul 24, 2023

bonomat Jul 24, 2023

da-kami Jul 24, 2023

bonomat Jul 24, 2023

luckysori Jul 25, 2023

luckysori Jul 25, 2023

bonomat Jul 24, 2023 •

edited

Loading

da-kami Jul 24, 2023

bonomat Jul 24, 2023

bonomat Jul 24, 2023

bonomat Jul 24, 2023

bonomat Jul 24, 2023

da-kami Jul 24, 2023

da-kami Jul 24, 2023 •

edited

Loading

bonomat Jul 24, 2023

da-kami commented Jul 25, 2023

bors bot commented Jul 25, 2023

luckysori left a comment

luckysori Jul 25, 2023


		let coordinator_pnl = position.calculate_coordinator_pnl(quote).unwrap();

		assert_eq!(coordinator_pnl, -9_090_909);

		@@ -57,10 +58,7 @@ pub async fn close(node: Node) {
		};

		let closing_price = match BitmexClient::get_quote(&position.expiry_timestamp).await {

Periodically update unrealized pnl for position in database #967

Periodically update unrealized pnl for position in database #967

Conversation

da-kami commented Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonomat Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami commented Jul 25, 2023

bors bot commented Jul 25, 2023

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami commented Jul 20, 2023 •

edited

Loading

bonomat Jul 24, 2023 •

edited

Loading

da-kami Jul 24, 2023 •

edited

Loading