Deal with expired position the right way #1246

holzeis · 2023-09-07T13:39:38Z

This PR got quite large and addresses several issues we had with closing expired positions.

Expired positions are now automatically matched against the orderbook

Since we currently have only one maker running which is publishing short and long orders this means the order for the expired position will get matched with the maker again, basically reducing his position with the coordinator.

Once the user comes online (connects to our websocket) we check for any pending matches (e.g. due to an expired position) and send a AsyncMatch message which the app is immediately executing.

For the following video, I reduced the position expiry to 30 seconds and the regular interval check for expired positions to 5 seconds. What happened in the background is that the orderbook found that the position expired found a match and saved that for later when the user is coming back online. Once online the match is executed with the user.

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-09-07.at.15.27.55.mp4

resolves #1173
resolves #1071

bonomat

Overall good work.
It's not a good idea to change migration files, this makes commit not atomic and we could run into weird problems.

I don't like the concept of a forced order yet. You seem to define every order as forced which was not manual. Maybe we can describe the concept differently?

Otherwise, I think this looks good.

crates/orderbook-commons/src/lib.rs

coordinator/migrations/2023-09-04-090152_add_order_state_and_matches/up.sql

coordinator/src/node/expired_orders.rs

coordinator/src/orderbook/db/orders.rs

coordinator/migrations/2023-09-04-090152_add_order_state_and_matches/up.sql

bonomat · 2023-09-08T11:38:03Z

coordinator/src/orderbook/db/custom_types.rs

@@ -130,6 +131,51 @@ impl FromSql<OrderStateType, Pg> for OrderState {
    }
 }

+#[derive(Debug, Clone, Copy, PartialEq, FromSqlRow, AsExpression)]
+#[diesel(sql_type = OrderReasonType)]
+pub(crate) enum OrderReason {


OrderReason sounds weird to me. Is it more an OrderType?

We already have an OrderType 🙈

How about SubOrderType?

OrderCreationType? OrderMethod?

coordinator/src/orderbook/trading.rs

coordinator/src/orderbook/db/custom_types.rs

mobile/lib/features/trade/force_order_change_notifier.dart

mobile/native/src/db/models.rs

holzeis · 2023-09-08T16:37:33Z

It's not a good idea to change migration files, this makes commit not atomic and we could run into weird problems.

I've split up the migration scripts into dedicated folders per commit.

I don't like the concept of a forced order yet. You seem to define every order as forced which was not manual. Maybe we can describe the concept differently?

As discussed offline I renamed it to async order and removed types that aren't part of this PR.

An async order is an order that has not been created by the user on the app, but automatically by the orderbook. For now this is just the market order created by the coordinator because the position expired, but this concept can also be used for stop loss, take profit, liquidation and limit orders.

bonomat

Looks good, but please wait for a second pair of eyes

luckysori

Thanks for working on this long and complicated task!

Whilst I do like the concept of letting the coordinator post an order on behalf of the trader, who would most likely choose this over settling their position on-chain, the implementation is quite complicated and I am not convinced we are modelling things correctly with the introduction of Matches and OrderReason.

At this stage I don't really want to block the merge since other teammates have already approved this. My main worry is that the orderbook is becoming harder to understand and maintain, and I think it was already in a bad spot before this PR which adds more features.

crates/orderbook-commons/src/lib.rs

luckysori · 2023-09-13T07:36:02Z

crates/orderbook-commons/src/lib.rs

+    if matches.len() == 1 {
+        return matches.first().expect("to be exactly one").execution_price;
+    }


🙃 I think this can be solved nicely with subslice patterns. No need to address this, I just think you might be interested.

coordinator/migrations/2023-09-08-090152_add_order_state/up.sql

coordinator/src/orderbook/trading.rs

luckysori · 2023-09-13T12:39:00Z

coordinator/src/orderbook/trading.rs

-                }
+            let match_params = matched_orders.matches();
+            for match_param in match_params {
+                matches::insert(&mut conn, match_param)?;


🤔 It's kind of weird that we need to save a match for each trader involved in the trade. Why not just one entry with all the information?

Because I want to track if a match got executed on per trader. The model would have gotten to complex otherwise.

Also considering that we could have multiple matches per order. By having a single match record we can easily reflect 1:n and m:n relations.

crates/orderbook-commons/src/lib.rs

luckysori · 2023-09-13T12:44:59Z

crates/trade/src/cfd.rs

+        let uncapped_pnl = match opening_price != Decimal::ZERO && closing_price != Decimal::ZERO {
+            true => {
+                let uncapped_pnl = (quantity / opening_price) - (quantity / closing_price);
+                let uncapped_pnl = uncapped_pnl.round_dp_with_strategy(
+                    8,
+                    rust_decimal::RoundingStrategy::MidpointAwayFromZero,
+                );
+                uncapped_pnl
+                    .to_f64()
+                    .context("Could not convert Decimal to f64")?
+            }
+            false => 0.0,
+        };


🔧 Can't we just use checked_divs and .unwrap_or_defaults? Also matching on a boolean is weird. if-else is the way to go.

Same thing applies above.

I only touched that code to prevent the divide by zero panic (unblocking me during testing). Don't want to touch it further as it is not related to this PR.

Also happy to drop that commit, if you think that should not happen.

crates/orderbook-commons/src/lib.rs

Having the order matching logic in the route layer made it difficult to utilize existing functionality. By moving it to the trading module it's now cleaner to interact with it from a different component.

Extracting the average execution price calculation from the `FilledWith` model makes things easier to reuse.

Keeping an order state on the coordinator side (similar to how it is done on the app side) allows us to depict more complex states of the order needed for handling expired positions.

If a position expires the coordinator will automatically create a closing order and look for a match. Since the user is not to be expected to be online the coordinator saves the match into the database.

`TradeValues` are either created from the quantity or from the margin. The new name now reflects that much better.

Apparently it can happen that we get a 0 bid or 0 ask price. That can lead to a panic: Aborting after panic in task info=panicked at 'Division by zero'

That makes the navigator keys accessible from anywhere.

This commit introduces the order reason and the force match. Basically the idea is that we have certain orders that will have to get created by the orderbook (on behalf of the user like expiry, liquidation, take profit and stop loss). Those orders are matched automatically without the users intervention. Once matched we inform the user about the match for execution. If the user is online the trade will be automatically executed if not, the execution is delayed until the user comes online. Note, this match can still expire.

The taken flag is not needed anymore as it is replaced by a more elaborate order state.

Spawning a new tokio task when processing a message prevents us from potentially blocking trading requests as the trading module might still be busy processing the last request.

holzeis requested review from bonomat, klochowicz and luckysori September 7, 2023 13:39

holzeis self-assigned this Sep 7, 2023

holzeis mentioned this pull request Sep 7, 2023

Simulate position between maker and coordinator #1216

Merged

holzeis force-pushed the feat/update-maker-on-expired-positions branch from f889032 to e9b264e Compare September 8, 2023 05:59

bonomat reviewed Sep 8, 2023

View reviewed changes

holzeis mentioned this pull request Sep 8, 2023

Can't close stable position due to not enough inbound capacity #1257

Closed

holzeis force-pushed the feat/update-maker-on-expired-positions branch from e9b264e to c08589b Compare September 8, 2023 15:41

holzeis force-pushed the feat/update-maker-on-expired-positions branch 3 times, most recently from 689b799 to 39d8abb Compare September 9, 2023 09:56

holzeis requested a review from bonomat September 9, 2023 09:58

bonomat approved these changes Sep 10, 2023

View reviewed changes

holzeis force-pushed the feat/update-maker-on-expired-positions branch 3 times, most recently from c8471c5 to 27b3cc3 Compare September 12, 2023 18:08

luckysori reviewed Sep 13, 2023

View reviewed changes

holzeis force-pushed the feat/update-maker-on-expired-positions branch from 27b3cc3 to 325e32f Compare September 14, 2023 08:11

holzeis added 10 commits September 14, 2023 10:11

chore: fix typo in function name

4e999a7

chore: Move order matching logic to trading module

80ab4a4

Having the order matching logic in the route layer made it difficult to utilize existing functionality. By moving it to the trading module it's now cleaner to interact with it from a different component.

chore: Extract average execution price calcualtion

10bd436

Extracting the average execution price calculation from the `FilledWith` model makes things easier to reuse.

feat: Add order state to order

8658be4

Keeping an order state on the coordinator side (similar to how it is done on the app side) allows us to depict more complex states of the order needed for handling expired positions.

feat: Find match on an expired position

bb890c7

If a position expires the coordinator will automatically create a closing order and look for a match. Since the user is not to be expected to be online the coordinator saves the match into the database.

chore: Fix typo in log message

69b1f3e

chore: Add missing leverage and contract symbol to order

3258972

chore: Improve trade values create method name

3d50aa6

`TradeValues` are either created from the quantity or from the margin. The new name now reflects that much better.

fix: Don't divide by zero

be535d3

Apparently it can happen that we get a 0 bid or 0 ask price. That can lead to a panic: Aborting after panic in task info=panicked at 'Division by zero'

chore: Move global keys into dedicated file

841a0c2

That makes the navigator keys accessible from anywhere.

holzeis force-pushed the feat/update-maker-on-expired-positions branch from 325e32f to 08ce864 Compare September 14, 2023 08:13

holzeis added 4 commits September 14, 2023 10:40

chore: Remove taken from orders

e326854

The taken flag is not needed anymore as it is replaced by a more elaborate order state.

chore: Set expired limit orders to failed

2be5881

chore: Spawn tokio tasks for each received trading message

f21358c

Spawning a new tokio task when processing a message prevents us from potentially blocking trading requests as the trading module might still be busy processing the last request.

holzeis force-pushed the feat/update-maker-on-expired-positions branch from 08ce864 to f21358c Compare September 14, 2023 08:40

holzeis enabled auto-merge September 14, 2023 09:05

holzeis mentioned this pull request Sep 14, 2023

feat: Send push notifications for expired and expiring positions #1297

Merged

holzeis added this pull request to the merge queue Sep 14, 2023

Merged via the queue into main with commit 8bd6afc Sep 14, 2023
7 checks passed

holzeis deleted the feat/update-maker-on-expired-positions branch September 14, 2023 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deal with expired position the right way #1246

Deal with expired position the right way #1246

holzeis commented Sep 7, 2023 •

edited

Loading

bonomat left a comment

bonomat Sep 8, 2023

holzeis Sep 8, 2023

holzeis Sep 9, 2023 •

edited

Loading

Restioson Sep 13, 2023

holzeis commented Sep 8, 2023 •

edited

Loading

bonomat left a comment

luckysori left a comment

luckysori Sep 13, 2023

luckysori Sep 13, 2023

holzeis Sep 13, 2023

luckysori Sep 13, 2023

luckysori Sep 13, 2023

holzeis Sep 14, 2023

Deal with expired position the right way #1246

Deal with expired position the right way #1246

Conversation

holzeis commented Sep 7, 2023 • edited Loading

bonomat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis commented Sep 8, 2023 • edited Loading

bonomat left a comment

Choose a reason for hiding this comment

luckysori left a 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

holzeis commented Sep 7, 2023 •

edited

Loading

holzeis Sep 9, 2023 •

edited

Loading

holzeis commented Sep 8, 2023 •

edited

Loading