Simulate position between maker and coordinator #1216

luckysori · 2023-09-05T04:11:40Z

Fixes #973.

bonomat · 2023-09-11T14:06:27Z

Cargo.toml

@@ -20,3 +20,6 @@ lightning-net-tokio = { git = "https://github.com/p2pderivatives/rust-lightning/
 lightning-persister = { git = "https://github.com/p2pderivatives/rust-lightning/", rev = "3b69cecf" }

 rust-bitcoin-coin-selection = { git = "https://github.com/p2pderivatives/rust-bitcoin-coin-selection" }
+
+# Waiting for the next release
+xtra = { git = "https://github.com/Restioson/xtra/", rev = "d98393a" }


Do we really want to introduce xtra?

Since xtra is such a small dependency and the actor pattern is useful here, I am keen to introduce it.

That reminds me to cut a release from master on xtra

luckysori · 2023-09-14T06:33:45Z

@bonomat @holzeis Please review this again.

I have not been able to test it locally because my new environment is not letting me start the app locally atm. I will only merge once I am able to resolve those problems and verify that the order matches are propagated correctly.

holzeis

The change is definitely simpler than before. I like it. However it is certainly not as powerful as your other proposal.

While before the coordinator was relying entirely on the dlc states (which is the most reliable source of truth) with this change the maker solely relies on the updates from the coordinator / orderbook.

I think that is the way to go, but it will also put us at the risk that the position reflected to the maker may not be entirely correct. However, I think we need to find those scenarios and have to update the maker accordingly.

@luckysori @bonomat what about existing positions? Would it make sense to add an api to the coordinator returning all orders where the maker has been matched / is matched to an active position? That way the maker is not relying solely on getting updates in deltas.

maker/src/orderbook_ws.rs

maker/src/position.rs

luckysori · 2023-09-18T10:33:50Z

@holzeis: I've updated the PR with a proposal to ensure that the maker's simulated position is resilient to restarts.

Unfortunately I still cannot test this e2e locally because my app won't build.

holzeis

I don't think we can rely on the AllOrders event here, as they won't include expired orders.

holzeis · 2023-09-18T11:23:39Z

maker/src/orderbook_ws.rs

 ) -> Result<()> {
    tracing::trace!(%msg, "New message from orderbook");

    let msg = serde_json::from_str::<OrderbookMsg>(&msg).context("Deserialization failed")?;

    match msg {
+        OrderbookMsg::AllOrders(orders) => {


🚨You will only received non-expired orders here. You won't get consistently the taken orders here over time as the makers orders expire after 60 seconds. (unless configured differently)

Thanks for the tip. The name definitely does not convey this.

I'm going to add an API to the WebSocket to get all the matched/taken orders for a trader.

It turns out none of the current orderbook messages work well for this purpose, so I just ended up adding a new one and letting the maker ask for it via a new orderbook request.

It would be much nicer if the orderbook could drive these updates instead, but it's not easy to implement, specially since we want to avoid adding maker-specific logic to the orderbook/coordinator.

bonomat

I have a hard time imagine how this evolves once we have a bitmex position as well. But let's get this in if it works :)

bonomat · 2023-09-19T08:09:04Z

maker/src/orderbook_ws.rs

+            let _ = position_manager.send(PositionUpdateTenTenOne(orders)).await;
+        }
+        OrderbookMsg::Match(FilledWith { order_id, .. }) => {
+            // We cannot rely directly on this message because the match does not specify the


We cannot rely directly on this message because the match does not specify the direction.

Why not? We know the order_id and given we created the order, we know the direction.

I left the comment because I tried it and it didn't work. The problem stems from the fact that the quantity in the Match is absolute, so the sign is always positive and there is no way to figure out the direction. I didn't want to change the semantics of quantity because it was created for a different purpose and the fact that it is absolute might be important.

If we remembered the orders we created as the maker we could figure it out, but that information is not persisted on the maker.

If we remembered the orders we created as the maker we could figure it out, but that information is not persisted on the maker.

Right ... there is this ticket for that #227

Good point!

I think it wouldn't hurt to persist the limit orders and it might be handy in this scenario, but it's still pretty limited as we only learn about the match and we cannot guarantee that the trade is executed. At least once the order is filled we know that the protocol was successfully started, so it's very likely that the trade will be executed.

The issue also mentions persisting the position, but in the current version where the coordinator is the only source of truth for this I actually think relying solely on the orderbook/coordinator is best, as there can't be disagreements. Once we introduce maker-coordinator DLCs we can get rid of all this, and then persisting some position metadata will be more interesting.

bonomat · 2023-09-19T08:35:12Z

coordinator/src/orderbook/db/orders.rs

+            let quantity = match direction_maker {
+                Direction::Long => quantity,
+                Direction::Short => -quantity,
+            };


I like this logic, maybe we should start using this accross the board.

How do you think we should action your comment? I assume we are already mapping like this elsewhere.

Oh, maybe we are already doing this :)

bonomat · 2023-09-19T08:37:38Z

maker/src/position.rs

+    }
+
+    #[cfg(test)]
+    fn get_tentenone(&self, contract_symbol: &ContractSymbol) -> Decimal {


Should we add an HTTP Api to query this ? Could come handy in our production setup to know what is going on .

bonomat · 2023-09-19T08:40:47Z

maker/src/position.rs

@@ -0,0 +1,220 @@
+use async_trait::async_trait;


Honest rust question: why is this not in ./position/mod.rs?

It's optional to use mod.rs since Rust 2018, although I prefer not to use it: https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs.

At some point @holzeis wanted to try out introducing mod.rs, but it's a bit annoying when you have multiple files open with the same mod.rs name. I would be keen to align it and remove all mod.rs files in favour of the newer standard.

Thanks for the clarification. I'm happy to follow this best practice :)

holzeis · 2023-09-19T14:12:53Z

Nice work! Thank you for reworking it! 🚀

luckysori requested review from bonomat, holzeis and klochowicz September 5, 2023 04:11

luckysori self-assigned this Sep 5, 2023

luckysori force-pushed the feat/maker-simulated-position branch 2 times, most recently from 33c1fd5 to a298aae Compare September 6, 2023 02:05

This comment was marked as resolved.

Sign in to view

bonomat reviewed Sep 11, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

luckysori force-pushed the feat/maker-simulated-position branch from a298aae to 89ed143 Compare September 14, 2023 06:31

luckysori requested review from bonomat and holzeis September 14, 2023 06:31

holzeis reviewed Sep 14, 2023

View reviewed changes

maker/src/orderbook_ws.rs Outdated Show resolved Hide resolved

maker/src/position.rs Show resolved Hide resolved

luckysori force-pushed the feat/maker-simulated-position branch from 89ed143 to c291d58 Compare September 18, 2023 10:30

luckysori requested a review from holzeis September 18, 2023 10:30

holzeis requested changes Sep 18, 2023

View reviewed changes

luckysori requested a review from holzeis September 18, 2023 12:59

luckysori force-pushed the feat/maker-simulated-position branch from 83a34f9 to 3c84706 Compare September 19, 2023 00:22

bonomat approved these changes Sep 19, 2023

View reviewed changes

holzeis approved these changes Sep 19, 2023

View reviewed changes

feat(maker): Simulate position with 10101 coordinator

3e8b310

luckysori force-pushed the feat/maker-simulated-position branch from 3c84706 to 3e8b310 Compare September 19, 2023 14:43

luckysori mentioned this pull request Sep 19, 2023

Query maker position via HTTP API #1328

Closed

luckysori enabled auto-merge September 19, 2023 14:46

luckysori mentioned this pull request Sep 19, 2023

Remove all instances of mod.rs #1329

Closed

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

Merged via the queue into main with commit b543f43 Sep 19, 2023

luckysori deleted the feat/maker-simulated-position branch September 19, 2023 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simulate position between maker and coordinator #1216

Simulate position between maker and coordinator #1216

luckysori commented Sep 5, 2023

This comment was marked as resolved.

bonomat Sep 11, 2023

luckysori Sep 14, 2023

Restioson Sep 14, 2023

This comment was marked as resolved.

luckysori commented Sep 14, 2023

holzeis left a comment

luckysori commented Sep 18, 2023

holzeis left a comment

holzeis Sep 18, 2023

luckysori Sep 18, 2023

luckysori Sep 19, 2023

bonomat left a comment

bonomat Sep 19, 2023

luckysori Sep 19, 2023 •

edited

Loading

bonomat Sep 19, 2023

luckysori Sep 19, 2023 •

edited

Loading

bonomat Sep 19, 2023

luckysori Sep 19, 2023

bonomat Sep 20, 2023

bonomat Sep 19, 2023

bonomat Sep 19, 2023

luckysori Sep 19, 2023 •

edited

Loading

bonomat Sep 19, 2023

holzeis commented Sep 19, 2023

Simulate position between maker and coordinator #1216

Simulate position between maker and coordinator #1216

Conversation

luckysori commented Sep 5, 2023

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

luckysori commented Sep 14, 2023

holzeis left a comment

Choose a reason for hiding this comment

luckysori commented Sep 18, 2023

holzeis 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

bonomat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori Sep 19, 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

luckysori Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis commented Sep 19, 2023

luckysori Sep 19, 2023 •

edited

Loading

luckysori Sep 19, 2023 •

edited

Loading

luckysori Sep 19, 2023 •

edited

Loading