Feat/track pnl #966

da-kami · 2023-07-20T07:25:54Z

Especially the first commit will be interesting for everybody to see - we are finally associating the positions with the contracts - Thanks for the great pairing session @luckysori :)

note: This PR only handles the realized pnl, I will see to add tracking for the unrealized pnl in a follow up.

related ticket: #932
(I'll add the fixes to the follow up :)

da-kami · 2023-07-20T07:30:03Z

Note: I should probably extend the e2e tests with asserts that the position is actually Closed and the realized_pnl set. Maybe @klochowicz you can help me with that tomorrow :)

da-kami · 2023-07-20T07:46:11Z

Note: It would potentially be better to change the insert to be an upsert and remove the specific update methods. But that would require some more model changes. I decided to get the feature done without more refactoring. Let me know what you think.

da-kami · 2023-07-20T07:48:05Z

coordinator/migrations/2023-07-19-055140_associate_position_with_dlc/up.sql

+ALTER TABLE
+    positions
+ADD
+    COLUMN "temporary_contract_id" TEXT NOT NULL;


I think this could actually be UNIQUE... CC @luckysori

bonomat · 2023-07-20T08:02:59Z

coordinator/migrations/2023-07-19-055143_closed_positions/up.sql

+-- In order to allow re-running this migration we thus have to make sure to only add the value if it does not exist yet.
+ALTER TYPE "PositionState_Type"
+ADD
+    VALUE IF NOT EXISTS 'Closed';


Feel good state ☺️

bonomat · 2023-07-20T08:06:08Z

coordinator/src/node/expired_positions.rs

+    let positions = positions
+        .into_iter()
+        .filter(|p| {
+            p.position_state == PositionState::Open
+                && OffsetDateTime::now_utc().ge(&p.expiry_timestamp)
+        })
+        .collect::<Vec<Position>>();


Nit:

positions should already contain only open positions.

it might be more efficient to include the expiry check in the sql query

I completely agree, but I would clean this up in a follow up. in this PR I just moved the code because I found having this code inline in main quite horrible for readability :)

bonomat · 2023-07-20T08:12:08Z

coordinator/src/db/positions.rs

@@ -67,6 +67,26 @@ impl Position {
        Ok(positions)
    }

+    #[autometrics]


Why do we need autometrics for this call?

copy paste... we have this on all the functions inside of this Node impl block at the moment. I'm happy to remove it (or add it to the function below as well where I did not add it 😅)

bonomat

I'm wondering how this will look like once we support position resizing 🤔

holzeis

I think this is good to merge, I left a couple of non-blocking remarks that you might want to address.

But before releasing that to production I guess we will need a migration script for existing positions without a temporary_contract_id

coordinator/migrations/2023-07-19-055140_associate_position_with_dlc/up.sql

coordinator/src/node.rs

coordinator/src/position/models.rs

crates/ln-dlc-node/src/node/dlc_channel.rs

holzeis · 2023-07-20T08:07:35Z

coordinator/migrations/2023-07-19-055143_closed_positions/up.sql

+ALTER TYPE "PositionState_Type"
+ADD
+    VALUE IF NOT EXISTS 'Closed';
+ALTER TABLE


🙃 Maybe not 100% related to your change, but it would be great if we'd also save the closing price on the position and not just the pnl.

I will see to add the closing price in a follow up, see #967 (comment)

holzeis · 2023-07-20T08:09:20Z

coordinator/src/db/positions.rs

            PositionState::Open => crate::position::models::PositionState::Open,
            PositionState::Closing => crate::position::models::PositionState::Closing,
+            PositionState::Closed => crate::position::models::PositionState::Closed {
+                pnl: realized_pnl.expect("realized pnl to be set when position is closed"),


🤔 Not sure if I like putting the pnl into the enum. Why not simply keep it as option on the position itself? I think that would also make the transformation from and to the database easier?

For me this is more idiomatic in rust code. This makes it clear that this pnl is only there in Closed state. I think this is more accurate for the model in the business logic. The database cannot easily depict that so I flattened it out in the db. But that's an implementation detail for me :)

Yeah, I think this is exactly what you want from Rust.

holzeis · 2023-07-20T08:12:46Z

coordinator/src/bin/coordinator.rs

+        let node = node.clone();
+        async move {
+            loop {
+                tokio::time::sleep(CLOSED_POSITION_SYNC_INTERVAL).await;


❓ Whats the rule of when to put an interval setting into the config file and when static on top? 😅

huh, I did not think about putting it into the config file. My rule of thumb would be: if we think we want to configure it we configure it. Is this relevant for the tests? If so then I'd add it to the config file so we can potentially use shorter intervals for the tests 🤔

If we need it in the config I'll address it in a follow up!

holzeis · 2023-07-20T08:15:00Z

coordinator/src/node/closed_positions.rs

+            .context("Failed to load open and closing positions")?;
+
+    for position in open_and_closing_positions {
+        let contract = match node


🤔 do we need a similar check for open contracts? I think there are scenarios where we also move backwards. cc @luckysori

If needed please ping and I'll address it in a follow up!

I suppose we could generalise this function to look at also consider the possibility that closing positions could go back to open. I don't think it's currently possible to go backwards in the protocol, but we did talk about it in p2pderivatives/rust-dlc#120.

Is this what you are referring to, @holzeis?

yes, that was what I was referring, thanks for adding the ticket 🙂

holzeis · 2023-07-20T08:17:11Z

coordinator/src/node/closed_positions.rs

+        {
+            Ok(Some(closed_contract)) => closed_contract,
+            Ok(None) => {
+                tracing::trace!(position_id=%position.id, "Position not closed yet, skipping");


🔧 we might want to add a timeout here and set the position to failed if its still in closing. No need to repeat that sync if the position is in closing for days.

I agree that this feature might be good to have, but I think this should be solved outside of this PR.
I captured #969 - feel free to add more information there!

da-kami · 2023-07-20T11:13:58Z

I think this is good to merge, I left a couple of non-blocking remarks that you might want to address.

But before releasing that to production I guess we will need a migration script for existing positions without a temporary_contract_id

That would require an in-code migration - but I'm questioning if it's even possible to deterministically know which position belongs to which contract so far. Is it worth investing time into this?

I propose:

Ignore not having a temporary contract id in the old ones; potentially add a dummy value that can be ignored when loading 🤔
taken a snapshot of all positions before upgrade
upgrade (we have a Closed state now)
Manually run a query that sets all Closing positions to Closed.

This is simpler, and - at least in my opinion - this tiny amount of pnl we lost/won so far should not matter 😅

da-kami · 2023-07-21T01:03:56Z

I'm wondering how this will look like once we support position resizing 🤔

TLDR; I don't think this model requires changes to introduce resizing :)

A few more thoughts - might be good food for thoughts for later :)

In my mind the position is always an aggregate over contracts. Orders define the execution that will lead to a contract (or resize of a contract or no contract anymore).
Eventually more than two orders will define that aggregate, but the position still only exists (is "open") or does not exist (is "closed"). The current position model already depicts an average_entry_price which would be updated when it gets resized (extended or reduced).
I think the closing scenario will be the same; eventually there will be a closing_price (execution price that reduces the position's quantity to 0, effectively wiping out the position) and, once the execution succeeded and the contract was "paid out", a payout and realized_pnl (= payout - collateral).

One thing that is still missing is associating orders with the positions. But I don't think this association changes the behavior of "closing" a position if we opt to stick to this model in the database.

Below are a few thoughts that discuss how the model could be change. I mostly talk about changing it in the database - the business logic model could be changed as well, but does not have to :)

I think we could model the position as a purely virtual aggregate in code (without depicting the position in the database); the position would then be an aggregate of it's orders, and the orders would store more information (e.g. execution price, ...). The position would then not exist in the database, but in order to decide if there is a position (i.e. the position quantity is not 0) we would aggregate all the successfully executed orders. This could be similar to the aggregates we had in ItchySats; the position would be purely virtual, and there is no "open" and "close" position, but just a position exists or not based on the executed order history.
I am not sure it's worth going for this though; the experience with aggregates in ItchySats was sometimes cumbersome. It was a (quite) concious decision to save the position into the database and manage it's state. Note that we could quite easily change it - the business logic could be pretty much the same, i.e. the way the positions::models::Position is constructed from the database would change - we could opt for only storing orders there that are aggregated into defining the values in positions::models::Position.

We could also do an in-between and delete the position in the database if it does not exist instead of setting it to Closed. This in-between solution would still require some form of aggregate over all orders though, so we can come up with the overall pnl over time. So, I don't think there is much value in this in between solution (might as well keep the historic positions as closed then...).

We could also opt to depict the aggregate differently in the database; at the moment we go for a per user per "cycle" model (where a cycle is basically a contract at the moment). This has the advantage that we can drive the position from the contract state easily.
However, we could depict the database position as a different aggregate. We could do a complete aggregate (spanning over all cycles) per user and set the quantity to 0 as well in the database instead of "closing" the position; or even do a complete combined position over all users with a combined pnl. Not sure if it's worth doing this in the database though.

da-kami · 2023-07-21T03:01:59Z

bors r+

966: Feat/track pnl r=da-kami a=da-kami Especially the first commit will be interesting for everybody to see - we are finally associating the `positions` with the contracts - Thanks for the great pairing session `@luckysori` :) note: This PR only handles the realized pnl, I will see to add tracking for the unrealized pnl in a follow up. related ticket: #932 (I'll add the *fixes* to the follow up :) Co-authored-by: Daniel Karzel <daniel@comit.network>

da-kami · 2023-07-21T03:02:49Z

bors r-

bors · 2023-07-21T03:02:50Z

Canceled.

Co-authored-by: Lucas Soriano del Pino <lucas_soriano@fastmail.com>

da-kami · 2023-07-21T03:06:15Z

bors r+

bors · 2023-07-21T03:19:21Z

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

LGTM!

970: Fix outdated schema and correctly handle temporary contract id r=da-kami a=da-kami Unfortunately #966 merged with an outdated `schema.rs` version in the coordinator. This PR fixes this! Co-authored-by: Daniel Karzel <daniel@comit.network>

967: Periodically update unrealized pnl for position in database r=da-kami a=da-kami 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 Co-authored-by: Daniel Karzel <daniel@comit.network>

da-kami requested review from bonomat, holzeis, klochowicz and luckysori July 20, 2023 07:25

da-kami force-pushed the feat/track-pnl branch from 1ad5e4f to 95d4ca8 Compare July 20, 2023 07:34

da-kami commented Jul 20, 2023

View reviewed changes

da-kami force-pushed the feat/track-pnl branch from 4364614 to 385eb5b Compare July 20, 2023 08:01

bonomat reviewed Jul 20, 2023

View reviewed changes

bonomat approved these changes Jul 20, 2023

View reviewed changes

holzeis approved these changes Jul 20, 2023

View reviewed changes

da-kami mentioned this pull request Jul 20, 2023

Periodically update unrealized pnl for position in database #967

Merged

da-kami mentioned this pull request Jul 21, 2023

Handle positions that have been hanging in Closing for too long #969

Closed

da-kami force-pushed the feat/track-pnl branch from 63b73cc to ac754fc Compare July 21, 2023 02:29

da-kami and others added 5 commits July 21, 2023 13:05

Associate position with temporary contract id

f1bbe1a

Co-authored-by: Lucas Soriano del Pino <lucas_soriano@fastmail.com>

Add Closed state to position including realized pnl

7c7bed7

Move closing expired position into dedicated module

72b46f9

Periodically update positions to Closed based on DLC

9e01166

Update positions.update_timestamp upon updated

f6030a8

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

bors bot merged commit 6b05827 into main Jul 21, 2023
7 checks passed

bors bot deleted the feat/track-pnl branch July 21, 2023 03:19

da-kami mentioned this pull request Jul 21, 2023

Fix outdated schema and correctly handle temporary contract id #970

Merged

luckysori reviewed Jul 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/track pnl #966

Feat/track pnl #966

da-kami commented Jul 20, 2023

da-kami commented Jul 20, 2023

da-kami commented Jul 20, 2023

da-kami Jul 20, 2023

bonomat Jul 20, 2023

bonomat Jul 20, 2023 •

edited

Loading

da-kami Jul 20, 2023 •

edited

Loading

bonomat Jul 20, 2023

da-kami Jul 20, 2023

bonomat left a comment

holzeis left a comment •

edited

Loading

holzeis Jul 20, 2023

da-kami Jul 20, 2023 •

edited

Loading

holzeis Jul 20, 2023

da-kami Jul 20, 2023 •

edited

Loading

luckysori Jul 21, 2023

holzeis Jul 20, 2023

da-kami Jul 20, 2023

da-kami Jul 21, 2023

holzeis Jul 20, 2023

da-kami Jul 21, 2023

luckysori Jul 21, 2023

holzeis Jul 21, 2023

holzeis Jul 20, 2023

da-kami Jul 21, 2023 •

edited

Loading

da-kami commented Jul 20, 2023 •

edited

Loading

da-kami commented Jul 21, 2023 •

edited

Loading

da-kami commented Jul 21, 2023

da-kami commented Jul 21, 2023

bors bot commented Jul 21, 2023

da-kami commented Jul 21, 2023

bors bot commented Jul 21, 2023

luckysori left a comment

Feat/track pnl #966

Feat/track pnl #966

Conversation

da-kami commented Jul 20, 2023

da-kami commented Jul 20, 2023

da-kami commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonomat Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

da-kami 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

bonomat left a comment

Choose a reason for hiding this comment

holzeis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami 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

da-kami Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

da-kami commented Jul 20, 2023 • edited Loading

da-kami commented Jul 21, 2023 • edited Loading

da-kami commented Jul 21, 2023

da-kami commented Jul 21, 2023

bors bot commented Jul 21, 2023

da-kami commented Jul 21, 2023

bors bot commented Jul 21, 2023

luckysori left a comment

Choose a reason for hiding this comment

bonomat Jul 20, 2023 •

edited

Loading

da-kami Jul 20, 2023 •

edited

Loading

holzeis left a comment •

edited

Loading

da-kami Jul 20, 2023 •

edited

Loading

da-kami Jul 20, 2023 •

edited

Loading

da-kami Jul 21, 2023 •

edited

Loading

da-kami commented Jul 20, 2023 •

edited

Loading

da-kami commented Jul 21, 2023 •

edited

Loading