feat: Initiate the dlc channel setup from the coordinator #244

holzeis · 2023-03-09T14:48:46Z

Checklist:

Submit order to orderbook
Refactor TradeParams to reflect a Match
Initiate dlc channel proposal from coordiantor
Accept dlc channel proposal from the taker
Accept dlc channel proposal from the maker
Manually broadcast confirm message once both taker and maker have accepted the dlc channel
Publish order filling and order filled updates
Testing

resolves #243

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

da-kami

Thanks for driving this forward, I left some comments

da-kami · 2023-03-10T00:15:11Z

crates/trade/src/lib.rs

-    /// This has to correspond to our order's leverage.
-    pub leverage: f64,
+    /// Note, this needs to be reversed for the counter party.
+    pub direction: Direction,


❓Do we need the Direction here?

TLDR: Since the orderbooks communicates the execution_price (and potentially funding rate in the future), we should not need the Direction in this model.

In the TradeParams model the Orderbook would communicate the price to be used as execution_price for both parties. If the execution_price is know then the Direction is not needed unless I missed something. My understanding is, that the Direction is only needed to decide which price / funding rate is to be used.

If we can I would try to avoid adding the Direction here; I think this was one thing that was confusing in our previous model in ItchySats - and at this point (after the execution price was decided) it should be irrelevant.

I think you are right, the direction might not be needed here.

Hmmm.. I've seen that it was not added in the TradeParams model, but figured it was simply forgotten.

If it's not needed, how will we be able to know if the trader wants to buy or sell?

Or in other words who will profit from a price change up or down if we don't know the direction.

mobile/native/src/trade/order/handler.rs

da-kami · 2023-03-10T00:54:54Z

mobile/native/src/trade/order/handler.rs

+    client
+        .post(format!(
+            "http://{}/api/orderbook/orders",
+            config::get_http_endpoint()


Suggested change

config::get_http_endpoint()

config::get_orderbook_http_endpoint()

I suppose this is the one for the orderbook; even if we configure it as the same initially, I would add a separate configuration entry :)

we don't have that config yet on the mobile app. I am going to add it later.

da-kami · 2023-03-10T01:01:12Z

mobile/native/src/ln_dlc/mod.rs

+
+                    let channel_id = sub_channel.channel_id;
+
+                    // todo: the app should validate if the offered dlc channel matches it's submitted order.


❓How do we associate incoming DLC requests with orders?

In the previous flow the client was in charge of triggering the trade based on the orderbook reporting a match. That match was for an order, and thus the client could identify the order.
The client then triggerd the execution, and only allowed one trade execution at the time - so if multiple match events from the orderbook would have come in, the client was in charge of scheduling the execution.

Now, the coordinator is in charge of scheduling the execution. If the client submitted multiple order to the orderbook, how would the client know which order is being executed here and update the order state accordingly?

By adapting this, the order / position lifecycle updates were removed, I think we should add them again so we can find an answer to the question above :)

Possibilities:

The client asks the coordinator what is being executed

The coordinator communicates the order id that is being executed to the client

~~The client asks the orderbook what was matched~~ -> I think this does not work, because the orderbook does not execute, and several oders could have been matched, so we run into the same problem; the orderbook does not know what's being executed by the coordinator

My conclusion is that the information what order is in execution has to come from the coordinator. We can easily validate that the trade parameters that the coordinator proposes match the order in question, so I think this is fine.

Yesterday we discussed the possibility of the coordinator communicating the order-id as part of the DLC prposal. As far as I remember this was not possible out of the box, so we will have to come up with a different way.

I still think that the orderbook should notify the trader as well and tell them:
hey, your order X,Y,Z have been matched, expect a DLC channel of size Q coming from coordinator c.

I still think that the orderbook should notify the trader as well and tell them:
hey, your order X,Y,Z have been matched, expect a DLC channel of size Q coming from coordinator c.

We can do that, but I think we additionally need a way to get information from the coordinator about which execution (i.e. DLC) is for which order. I don't think the orderbook telling us is enough - especially for a maker that potentially has several takers trading with him at the same time. We have to be able to clearly associate one order with one DLC, and I don't think the orderbook has this information in this flow.

bonomat · 2023-03-10T02:12:02Z

mobile/native/src/trade/order/handler.rs

-        // in 24h
-        expiry: Duration::from_secs(60 * 60 * 24),
-        oracle_pk: ln_dlc::get_oracle_pubkey()?,
+    // todo: this model should include the contract_symbol


Either this or you make the contract_symbol part of the route, e.g.
.../api/orderbook/orders/btcusd

I'm in favor of adding it as field - this allows us a more data driven approach, where the context is clear from the data that was sent.

bonomat · 2023-03-10T02:12:26Z

mobile/native/src/trade/order/handler.rs

-        expiry: Duration::from_secs(60 * 60 * 24),
-        oracle_pk: ln_dlc::get_oracle_pubkey()?,
+    // todo: this model should include the contract_symbol
+    let new_order = trade::NewOrder {


We will need some more information here, e.g. will need to know if this is a market order or a limit order

This is how the NewOrder is represented on the API for the app; I think this should be complete. Likely we will need a similar representation when sending an order to the orderbook.

#[frb] #[derive(Debug, Clone)] pub struct NewOrder { #[frb(non_final)] pub leverage: f64, #[frb(non_final)] pub quantity: f64, #[frb(non_final)] pub contract_symbol: ContractSymbol, #[frb(non_final)] pub direction: Direction, // Box needed for complex enum, otherwise generated Rust code complains about Default impl // missing #[frb(non_final)] pub order_type: Box<OrderType>, }

what about the pub_key of the trader?

True, the pubkey of the trader should be added. On the app it's our own so we know it internally and don't send it to the API; for the orderbook we will have to add it.

bonomat · 2023-03-10T02:13:23Z

mobile/native/src/trade/order/handler.rs

+    let new_order = trade::NewOrder {
+        price: dec!(22_000), // todo: replace with actual btcusd rate price
+        quantity: Decimal::try_from(order.quantity)?,
+        maker_id: "".to_string(), // todo: what should be provided on the maker_id?


This was just a dummy field. Imho this should be the sender's pk, e.g. if maker posts the limit order, then this is the maker's pk, if it is a market order by the trader, then this is the trader's pk.

I see.. Then we should probably name it trader_id or something.

bonomat · 2023-03-10T02:15:29Z

coordinator/src/node.rs

+
+                let margin_trader = calculate_margin(
+                    match_params.execution_price,
+                    match_params.quantity as f64, // todo: the quantity should not be f64


What else should it be?
In BTC/USD the quantity is equal to USD value of the position. As talked in the past : it would be cool to support fraction of a cent as position size, hence this needs to be float.

fair enough, I wasn't aware of that requirement. In the past we used u64 for the quantity.

What precision do we want here though? I would avoid floats as much as possible.

bonomat · 2023-03-10T02:16:25Z

coordinator/src/node.rs

+                let margin_trader = calculate_margin(
+                    match_params.execution_price,
+                    match_params.quantity as f64, // todo: the quantity should not be f64
+                    trade.leverage as f64,        // todo: the leverage should not be f64


Unless we want to rule out fractional leverages this can be something else, otherwise this has to be a float.

We need fractional leverage for a combined position, where multiple different orders (with multiple leverages) are combined in one position for one trading pair.
This has to stay f64 from my point of view.

thanks for the explanation.

Can we use Decimal instead?

Can we use Decimal instead?

I think that's a good idea!

Note: So far when using Decimal, we always use float as representation when serializing/deserializing it. It will still be better to use Decimal when performing calculations; but just for the sake of communicating information f64 might actually be fine. We can however still use the Decimal type here, because then it becomes an implementation detail how it is serialized / desierialized.

In the orderbook I've used Decimal in most places.

crates/trade/src/lib.rs

bonomat · 2023-03-10T02:22:24Z

crates/trade/src/lib.rs

+/// Emitted by the orderbook to the coordinator for execution when a match is found.
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct MatchParams {
+    /// The taker trade information


I think we need to be able to identify which order was matched because the trader might have multiple orders pending.

Sorry, the match params ended up in the wrong commit, the trade model (introduced in the next commit) includes the order id.

maker/src/bin/maker.rs

bonomat · 2023-03-10T02:24:20Z

maker/src/bin/maker.rs

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


Imho the sleep should come at the end, otherwise we wait unnecessarily for the first attempt.

Having the sleep in the beginning has the advantage, that the continues below will not skip the sleep.

bonomat

I think the flow is pretty much what we discussed. One critical question remains open though: 2f2d435 (#244)

This way we will get a reliable pub key of the maker.

Setup gets a bit complex at this point as the maker is now also included in the test. We need to find a way to do this automated as there a so many moving parts currently. Manuell testing failed with either the taker or the maker channel getting closed during dlc channel proposal. see message below. ``` 2023-03-10 19:12:58 DEBUG lightning::ln::peer_handler: Got Err message from 02b103838b4fc38a423342e2d187de6da76dde13e7a0271c8247e19c91027140f7: Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 1 confs, now have 0 confs. 2023-03-10 19:12:58 ERROR lightning::ln::channelmanager: Force-closing channel dbab3910068bf81fd7c0148cc5ef48686528036493e0a43324d54c7b4839d43b 2023-03-10 19:12:58 DEBUG lightning::ln::channelmanager: Finishing force-closure of channel with 0 HTLCs to fail ``

da-kami · 2023-03-13T02:51:50Z

I tried to sum up holistically what is the advantage and disadvantage of this new flow against the previous version where the clients were in charge of triggering the trade execution after a match.
This does not mean we have to change anything, but I wanted to document this somewhere, so I started here.
Eventually this should be documented in #181.

Advantages:

The coordinator can fail a trade setup faster if one of the parties is not reachable, because the coordinator reaches out to the parties for trade setup instead of waiting for both parties to reach out.
If there is a bug in "how the trade is being triggered" we can fix it faster, because the coordinator is in control and we don't have to roll out a new version on the client(s). This is only an advantage for the first message; as soon as something from the client is involved this advantage becomes irrelevant.

Disadvantages:

The orderbook coming up with a match is decoupled from the execution on the client side. If the orderbook contacts the client for a match, and the client triggers the execution it is clear that this execution is for that match. If the coordinator triggers the execution there is additional validation needed on the client, to ensure the execution is triggered according to the match of the orderbook. This can be overcome by e.g. the client querying the orderbook to retrieve match information.
Similarly, the client may not be able to match the order_id that is being executed with the client's orders. Further information from the orderbook (if we assume that execution = match) or coordinator (if the execution can be different than the match(es)) is required (which has to be validated against the orderbook, see above).

Thinking about this made me come to this question: Who is in charge of optimizing the "trades" so that we don't execute everything sequentially - the orderbook or the coordinator?

If we assume that the orderbook, eventually, should be smart enough to batch execution together, i.e. the orderbook should ensure that not all orders are handled sequentially if there are multiple matches, this would mean that the coordinator should actually not question the authority of the orderbook. That could mean that the coordinator does not do any optimizations - in such a case we might as well let the client trigger the execution to overcome the disadvantages listed above.
If we, however, expect the coordinator to do some form of trade execution optimization then the flow as proposed in this PR will make more sense.

assume that there is only one open order in the app and load the open order from the db so we can update it.

248: feat: add order matching function r=1010Tom a=1010Tom follow-up PR from #246 Next: - [x] fill in correct types: needs types from #244 - [ ] subscribe to match event in maker/taker/coordinator Co-authored-by: 1010Tom <1010Tom@mailx.site> Co-authored-by: Richard Holzeis <richard@holzeis.me>

luckysori · 2023-03-17T01:37:04Z

@holzeis, @da-kami: Shall we close this PR now that we have dealt with these changes in other PRs?

holzeis · 2023-03-19T19:33:14Z

Yes let's close it. I kept it open so we could finish our discussions, but we have moved on already.

holzeis requested review from bonomat, da-kami and luckysori March 9, 2023 14:48

holzeis self-assigned this Mar 9, 2023

holzeis marked this pull request as draft March 9, 2023 14:48

chore: Move trade API to coordinator

497eb41

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

holzeis force-pushed the feat/initiate-the-dlc-channel-setup-from-the-coordinator branch from 8eb3d24 to ab06660 Compare March 9, 2023 14:50

da-kami reviewed Mar 10, 2023

View reviewed changes

bonomat reviewed Mar 10, 2023

View reviewed changes

crates/trade/src/lib.rs Outdated Show resolved Hide resolved

bonomat reviewed Mar 10, 2023

View reviewed changes

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

bonomat reviewed Mar 10, 2023

View reviewed changes

holzeis force-pushed the feat/initiate-the-dlc-channel-setup-from-the-coordinator branch 3 times, most recently from 978bdee to 614c3ad Compare March 10, 2023 12:07

holzeis added 4 commits March 10, 2023 13:30

feat: Submit order to orderbook

ea67dd1

feat: Initiate dlc channel proposal from the coordinator

9b50d6d

feat: Accept dlc channel offers from trader

312eb83

feat: Confirm matched dlcs

af0ccaf

holzeis force-pushed the feat/initiate-the-dlc-channel-setup-from-the-coordinator branch from 614c3ad to af0ccaf Compare March 10, 2023 12:31

holzeis added 3 commits March 10, 2023 16:50

chore: Add maker seed as exception to gitignore

7472efd

This way we will get a reliable pub key of the maker.

docs: Fix typo

78b7deb

fixup! WIP: Publish order filling event / testing

cfd0492

assume that there is only one open order in the app and load the open order from the db so we can update it.

1010Tom mentioned this pull request Mar 13, 2023

feat: add order matching function #248

Merged

2 tasks

holzeis closed this Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Initiate the dlc channel setup from the coordinator #244

feat: Initiate the dlc channel setup from the coordinator #244

holzeis commented Mar 9, 2023 •

edited

da-kami left a comment

da-kami Mar 10, 2023

bonomat Mar 10, 2023

holzeis Mar 10, 2023 •

edited

da-kami Mar 10, 2023

holzeis Mar 10, 2023

da-kami Mar 10, 2023 •

edited

bonomat Mar 10, 2023

da-kami Mar 10, 2023

bonomat Mar 10, 2023

da-kami Mar 10, 2023

bonomat Mar 10, 2023 •

edited

da-kami Mar 10, 2023 •

edited

holzeis Mar 10, 2023

da-kami Mar 12, 2023

bonomat Mar 10, 2023

holzeis Mar 10, 2023

bonomat Mar 10, 2023 •

edited

holzeis Mar 10, 2023 •

edited

luckysori Mar 13, 2023

bonomat Mar 10, 2023 •

edited

da-kami Mar 10, 2023 •

edited

holzeis Mar 10, 2023

luckysori Mar 13, 2023

da-kami Mar 13, 2023 •

edited

bonomat Mar 13, 2023

bonomat Mar 10, 2023

holzeis Mar 10, 2023

bonomat Mar 10, 2023

holzeis Mar 10, 2023

bonomat left a comment

da-kami commented Mar 13, 2023 •

edited

luckysori commented Mar 17, 2023

holzeis commented Mar 19, 2023

	config::get_http_endpoint()
	config::get_orderbook_http_endpoint()


		let channel_id = sub_channel.channel_id;

		// todo: the app should validate if the offered dlc channel matches it's submitted order.

feat: Initiate the dlc channel setup from the coordinator #244

feat: Initiate the dlc channel setup from the coordinator #244

Conversation

holzeis commented Mar 9, 2023 • edited

da-kami 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 Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami Mar 10, 2023 • edited

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 Mar 10, 2023 • edited

Choose a reason for hiding this comment

da-kami Mar 10, 2023 • edited

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 Mar 10, 2023 • edited

Choose a reason for hiding this comment

holzeis Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonomat Mar 10, 2023 • edited

Choose a reason for hiding this comment

da-kami Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami Mar 13, 2023 • edited

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

Choose a reason for hiding this comment

da-kami commented Mar 13, 2023 • edited

luckysori commented Mar 17, 2023

holzeis commented Mar 19, 2023

holzeis commented Mar 9, 2023 •

edited

holzeis Mar 10, 2023 •

edited

da-kami Mar 10, 2023 •

edited

bonomat Mar 10, 2023 •

edited

da-kami Mar 10, 2023 •

edited

bonomat Mar 10, 2023 •

edited

holzeis Mar 10, 2023 •

edited

bonomat Mar 10, 2023 •

edited

da-kami Mar 10, 2023 •

edited

da-kami Mar 13, 2023 •

edited

da-kami commented Mar 13, 2023 •

edited