feat: Automatically rollover in rollover weekend #1287

holzeis · 2023-09-12T13:07:26Z

This change will automatically push the users dlc expiry to the next Sunday 15pm UTC if the user comes online within the rollover weekend (Friday, 15pm UTC - Sunday, 15pm UTC).

The video was taken with an adapted get_expiry_timestamp and is_in_rollover_weekend logic to provoke the rollover.

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-09-12.at.14.27.54.mp4

resolves #1123

luckysori

I'm confused about rollovers being driven by the orderbook.

Even though the coordinator circumstantially contains the orderbook, I don't think the orderbook has anything to do with rollovers. This is the flow that I see:

Trader creates market order.
Orderbook matches against limit order.
Coordinator executes order with trader (and maker), creating a position with expiry (because of technical limitations).
As the position expiry approaches, the trader has a chance to extend the lifetime of the position by rolling over. If they don't, the coordinator places an order on their behalf. If they do opt to roll over, nothing happens on the orderbook.

crates/orderbook-commons/src/lib.rs

coordinator/src/orderbook/trading.rs

crates/orderbook-commons/src/lib.rs

holzeis · 2023-09-17T10:06:52Z

@luckysori I've split up the responsibilities of the trading module in to separate modules. Please have a look at b6cd302 and let me know what you think!

Without that fix we were updating all positions of the user!

Before everything was handled by the trading module. Now the following responsibilities have been split up. - Rollover: Moved to the coordinator component and responsible for proposing a rollover. - Async Match: Moved into a dedicated component to check if an async match needs to be executed by the app. - Notification: Responsible for the users and sending messages to them. Note 1, the websocket communication is still in one component and not separated between coordinator and orderbook. It would have been correct to split that up as well, but the effort and additional complexity was IMHO not worth it. Note 2, we have multiple commons crates, which are very much related to each ohter. I think we should combine all of those into a single one, to simplify things.

luckysori

I appreciate the refactor!

luckysori · 2023-09-19T13:04:33Z

coordinator/src/bin/coordinator.rs

+    let (_handle, notifier) = notification::start(tx_user_feed.clone());
+
+    let (_handle, trading_sender) =
+        trading::start(pool.clone(), tx_price_feed.clone(), notifier.clone());
+
+    let _handle = async_match::monitor(pool.clone(), tx_user_feed.clone(), notifier.clone());
+    let _handle = rollover::monitor(pool.clone(), tx_user_feed.clone(), notifier);


❓ Doesn't the fact that you shadow _handle cause the previous RemoteHandles to be dropped stopping the corresponding tasks?

Btw, I think there is not much benefit to just returning the RemoteHandle to "ignore" it. What you usually do with a RemoteHandle is store in some other struct so that when you drop that one the corresponding task is cancelled.

❓ Doesn't the fact that you shadow _handle cause the previous RemoteHandles to be dropped stopping the corresponding tasks?

Doesn't look like it. At least it worked during testing.

Btw, I think there is not much benefit to just returning the RemoteHandle to "ignore" it. What you usually do with a RemoteHandle is store in some other struct so that when you drop that one the corresponding task is cancelled.

Well the reason I return it to the top level is so that the tasks live as long as the coordinator. That's why I am doing this, do you think that is not needed? Happy to remove it, but I think when I did, the compiler warned me about it.

❓ Doesn't the fact that you shadow _handle cause the previous RemoteHandles to be dropped stopping the corresponding tasks?

Doesn't look like it. At least it worked during testing.

Confirmed it does not drop it: https://www.reddit.com/r/rust/comments/qpekne/what_actually_happens_when_ypu_shadow_a_variable/.

Btw, I think there is not much benefit to just returning the RemoteHandle to "ignore" it. What you usually do with a RemoteHandle is store in some other struct so that when you drop that one the corresponding task is cancelled.

Well the reason I return it to the top level is so that the tasks live as long as the coordinator. That's why I am doing this, do you think that is not needed?

I guess because the coordinator is the whole program it isn't actually useful. It would be handy if we could remake the coordinator in case of catastrophic failure (without restarting the binary). Then you would want to cancel all the old tasks and restart them with a fresh coordinator struct.

Happy to remove it, but I think when I did, the compiler warned me about it.

That's probably because you called remote_handle on the future and returned the RemoteHandle which is marked as must_use. You can obviously just spawn the future without doing any of that, but I don't mind if you wanna keep the current version.

got ya. Since this PR is now open for quite some time. I prefer to go ahead with the current change, and adapt the remote_handle stuff in a follow up PR.

We need to provide the current timestamp instead of the position expiry, as it would have been otherwise never eligible for rollover.

holzeis requested review from bonomat, klochowicz and luckysori September 12, 2023 13:07

holzeis self-assigned this Sep 12, 2023

holzeis force-pushed the feat/update-maker-on-expired-positions branch from c6facee to c8471c5 Compare September 12, 2023 13:21

holzeis force-pushed the feat/display-rollover-option-to-user branch from b3e651e to 55ba6e5 Compare September 12, 2023 13:24

holzeis changed the title ~~chore: Set position expiry only in one place~~ feat: Automatically rollover in rollover weekend Sep 12, 2023

holzeis linked an issue Sep 12, 2023 that may be closed by this pull request

Display rollover option to user #1123

Closed

holzeis force-pushed the feat/display-rollover-option-to-user branch from 55ba6e5 to ccccf3b Compare September 12, 2023 14:51

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

holzeis force-pushed the feat/display-rollover-option-to-user branch 2 times, most recently from e300836 to 501372e Compare September 12, 2023 19:47

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

holzeis force-pushed the feat/display-rollover-option-to-user branch from 93fe42c to 1fc5e84 Compare September 14, 2023 08:38

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

holzeis force-pushed the feat/display-rollover-option-to-user branch 2 times, most recently from 9658824 to d90d44e Compare September 14, 2023 08:46

Base automatically changed from feat/update-maker-on-expired-positions to main September 14, 2023 10:21

luckysori reviewed Sep 15, 2023

View reviewed changes

holzeis force-pushed the feat/display-rollover-option-to-user branch 3 times, most recently from 7fbb639 to bc18260 Compare September 15, 2023 09:02

luckysori reviewed Sep 15, 2023

View reviewed changes

crates/orderbook-commons/src/lib.rs Outdated Show resolved Hide resolved

crates/orderbook-commons/src/lib.rs Outdated Show resolved Hide resolved

holzeis force-pushed the feat/display-rollover-option-to-user branch from b6cd302 to bbd5ca2 Compare September 17, 2023 10:05

holzeis requested a review from luckysori September 17, 2023 10:06

holzeis added 3 commits September 19, 2023 14:48

chore: Set position expiry only in one place

d4b14ca

feat: Calculate expiry timestamp to Sunday 3pm

cdd708e

feat: Automatically rollover if user opens app during rollover weekend.

48e857a

holzeis added 2 commits September 19, 2023 14:49

fix: Only update open or rollover positions

63932dc

Without that fix we were updating all positions of the user!

holzeis force-pushed the feat/display-rollover-option-to-user branch from bbd5ca2 to f3fa8db Compare September 19, 2023 12:49

luckysori approved these changes Sep 19, 2023

View reviewed changes

fix: Do not rollover an expired position

322ab39

We need to provide the current timestamp instead of the position expiry, as it would have been otherwise never eligible for rollover.

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

holzeis removed this pull request from the merge queue due to a manual request Sep 19, 2023

holzeis enabled auto-merge September 19, 2023 14:07

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

Merged via the queue into main with commit 0b30115 Sep 19, 2023
7 checks passed

holzeis deleted the feat/display-rollover-option-to-user branch September 19, 2023 14:35

holzeis mentioned this pull request Sep 21, 2023

Coordinator panicked #1338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Automatically rollover in rollover weekend #1287

feat: Automatically rollover in rollover weekend #1287

holzeis commented Sep 12, 2023 •

edited

Loading

luckysori left a comment

holzeis commented Sep 17, 2023

luckysori left a comment

luckysori Sep 19, 2023

luckysori Sep 19, 2023

holzeis Sep 19, 2023

holzeis Sep 19, 2023

luckysori Sep 19, 2023

luckysori Sep 19, 2023

holzeis Sep 19, 2023

feat: Automatically rollover in rollover weekend #1287

feat: Automatically rollover in rollover weekend #1287

Conversation

holzeis commented Sep 12, 2023 • edited Loading

luckysori left a comment

Choose a reason for hiding this comment

holzeis commented Sep 17, 2023

luckysori left a comment

Choose a reason for hiding this comment

luckysori Sep 19, 2023

Choose a reason for hiding this comment

luckysori Sep 19, 2023

Choose a reason for hiding this comment

holzeis Sep 19, 2023

Choose a reason for hiding this comment

holzeis Sep 19, 2023

Choose a reason for hiding this comment

luckysori Sep 19, 2023

Choose a reason for hiding this comment

luckysori Sep 19, 2023

Choose a reason for hiding this comment

holzeis Sep 19, 2023

Choose a reason for hiding this comment

holzeis commented Sep 12, 2023 •

edited

Loading