Reintroduce position resizing #2359

luckysori · 2024-04-05T07:30:04Z

Fixes #1791.

position-resizing-v2.mp4

I fixed calculation bugs in our original implementation of position resizing that we used for LN-DLC channels. This is why some assert values changed.
I reintroduced the resizing e2e tests, which are now much more helpful because they assert on the specific values of position and trade for the app. This can be further improved by asserting on the coordinator side. The same pattern should eventually be applied to most e2e tests.
I had to add an is_complete column to the coordinator's trade table to be able to record the trader_realized_pnl_sat ahead of time.

Limitations:

The app must have an order in Filling to be able to update the position after we get a resizing channel renewal offer. This pattern has caused problems in the past. We will probably fix this my sending extra metadata with the rust-dlc offer message.

holzeis

LGTM 👍

coordinator/migrations/2024-03-18-083352_add_order_matching_fees_to_position/up.sql

coordinator/src/position/models.rs

holzeis · 2024-04-05T19:28:24Z

mobile/lib/features/welcome/welcome_screen.dart

@@ -218,6 +219,11 @@ class _WelcomeScreenState extends State<WelcomeScreen> {
  void initState() {
    super.initState();

+    if (Environment.parse().network == "regtest") {


mobile/lib/features/trade/domain/trade_values.dart

holzeis · 2024-04-05T19:36:11Z

coordinator/src/trade/mod.rs

+            .await
+            .context("Could not propose DLC channel update")?;
+
+        // TODO: Shouldn't we include actually starting the DLC protocol in the DB transaction in


Since we use different transactional systems, postgres and sled I think it doesn't matter. But once we are also using postgres that would be great. As we could atomically execute both rust dlc and 10101 changes.

Fair point. I've removed the TODO.

I think what confuses me is the name. It is surprising that start_dlc_protocol, called on the ProtocolExecutor, only interfaces with the database.

coordinator/src/trade/mod.rs

holzeis · 2024-04-05T19:40:25Z

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

-        event::publish(&EventInternal::PositionUpdateNotification(position));
+        // Assume that if there is an order in filling we are dealing with position resizing.
+        //
+        // TODO: This is garbage as we very well know. Any other ideas? We could generate


@bonomat and I were talking before, that it would probably make sense to add a custom message to wrap the renew protocol for rollover, resizing and async matching.. and enrich the message with some 10101 meta data. that would also allow us to get rid of the ugly websocket communication to send over some additional data.

That sounds good, but I think that belongs in a different PR. Given your approval, I assume you are okay with not tackling this TODO right now.

At this stage it is only informative[^1], and `docker-compose` gives me a warning about it. [^1]: https://docs.docker.com/compose/compose-file/04-version-and-name/#version-top-level-element-optional.

The order-matching fee will accumulate all the order-matching fees for a given position: opening order-matching fee; arbitrarily many resize order-matching fees; and closing order-matching fee. This information can be used to compute the collateral reserve when resizing the position. The accumulated order-matching fee will be the lower bound for the coordinator's collateral reserve.

The production coordinator does not have any instances of `PositionState::ResizeProposed` and `PositionState::Resizing`, since they correspond to _subchannel_ resizing protocols. We still keep `PositionState::Resizing` around since we can reuse the type for the new DLC channel resizing protocol.

This saves me a few seconds every time I start a fresh `regtest` app.

By default, we now use a fixed price of $50k. This applies to CI too. This is useful if we want to assert on specific values in the e2e tests. Instructions to use the binary ------------------------------ 1. Use a $50k fixed price: ``` $ cargo run --bin dev-maker ``` or ``` $ just maker ``` 2. Use a historic price feed: ``` $ cargo run --bin dev-maker -- historic ``` or ``` $ just maker historic ``` 3. Use a custom fixed price e.g $100k: ``` $ cargo run --bin dev-maker -- fixed 100000 ``` or ``` $ just maker "fixed 100000" ```

luckysori · 2024-04-09T11:32:05Z

Queuing this up because I can't face another rebase that breaks another test. I will tackle any review comments in a separate PR.

- I fixed calculation bugs in our original implementation of position resizing that we used for LN-DLC channels. This is why some assert values changed. - I reintroduced the resizing e2e tests, which are now much more helpful because they assert on the specific values of position and trade for the app. This can be further improved by asserting on the coordinator side. The same pattern should eventually be applied to most e2e tests. - I had to add an `is_complete` column to the coordinator's `trade` table to be able to record the `trader_realized_pnl_sat` ahead of time. Limitations: - The app must have an order in `Filling` to be able to update the position after we get a resizing channel renewal offer. This pattern has caused problems in the past. We will probably fix this my sending extra metadata with the `rust-dlc` offer message.

A trade only has a collateral associated with it once it's applied to a position. Since it's not used for anything, we remove it.

Already fixed.

holzeis · 2024-04-09T18:21:34Z

coordinator/src/trade/mod.rs

+                    .expect("to fit"),
+                order_matching_fee,
+                trader_realized_pnl_sat: realized_pnl.map(SignedAmount::to_sat),
+                is_complete: false,


🤔 It looks to me the only reason why you'd have to create the trade already here is because of the trader_realized_pnl_sat. Why not simply add that to the trade_params table which is anyways just temporary and intended to remember the state from when the protocol starts to when it ends.

I dislike the is_complete flag because

It introduces a state on the trade.

It is not consistent with the rest of the implementation. e.g. if we want to keep the is_complete flag there is no reason for a technical trade_params table. We could remove it and create the trade immediately with the required data and set it to "incomplete"

I am not saying we have to stay with the trade_params but mixing the two concepts feels wrong to me.

You might be right.

It seemed wrong to use the trade_params table because the PNL is not a trade parameter. It's the result of applying the trade to a position.

I agree that it is inconsistent. I can try to fix it, but the priority for me is fairly low.

Did it anyway: #2394.

Thanks

It seemed wrong to use the trade_params table because the PNL is not a trade parameter. It's the result of applying the trade to a position.

Maybe renaming the table would help, but I can't think of a better name 🙈

luckysori requested review from bonomat and holzeis April 5, 2024 07:30

luckysori self-assigned this Apr 5, 2024

luckysori force-pushed the feat/position-resizing branch from b7d2e15 to 0c4a9c5 Compare April 5, 2024 07:49

luckysori force-pushed the fix/issue-2288 branch from 5e6dd19 to 10e0cee Compare April 5, 2024 12:02

luckysori force-pushed the feat/position-resizing branch from 0c4a9c5 to c377373 Compare April 5, 2024 12:03

holzeis approved these changes Apr 5, 2024

View reviewed changes

Base automatically changed from fix/issue-2288 to main April 7, 2024 08:53

luckysori force-pushed the feat/position-resizing branch 3 times, most recently from 0f0ae67 to b3b29bd Compare April 9, 2024 09:33

luckysori added 8 commits April 9, 2024 20:32

chore(docker-compose): Remove version

073890d

At this stage it is only informative[^1], and `docker-compose` gives me a warning about it. [^1]: https://docs.docker.com/compose/compose-file/04-version-and-name/#version-top-level-element-optional.

chore(coordinator): Clean up Position struct

85b987c

chore(app): Remove a couple of mentions of Lightning

5d67d83

chore(coordinator): Delete unused files

68b94a3

chore(app): Pre-accept conditions in welcome screen for regtest

18d3b09

This saves me a few seconds every time I start a fresh `regtest` app.

luckysori force-pushed the feat/position-resizing branch 3 times, most recently from 385db6e to 11b0cfc Compare April 9, 2024 11:26

luckysori enabled auto-merge April 9, 2024 11:31

luckysori force-pushed the feat/position-resizing branch from 11b0cfc to a19cab9 Compare April 9, 2024 11:52

luckysori added 3 commits April 9, 2024 22:01

chore(coordinator): Remove collateral from trades table

ed30023

A trade only has a collateral associated with it once it's applied to a position. Since it's not used for anything, we remove it.

chore(ln-dlc-node): Remove outdated TODO and hack

7681b79

Already fixed.

luckysori force-pushed the feat/position-resizing branch from a19cab9 to 7681b79 Compare April 9, 2024 12:01

luckysori added this pull request to the merge queue Apr 9, 2024

Merged via the queue into main with commit 59922f9 Apr 9, 2024
22 checks passed

luckysori deleted the feat/position-resizing branch April 9, 2024 12:36

holzeis reviewed Apr 9, 2024

View reviewed changes

luckysori mentioned this pull request Apr 10, 2024

Add trader_pnl to trade_params table #2394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce position resizing #2359

Reintroduce position resizing #2359

luckysori commented Apr 5, 2024 •

edited

Loading

holzeis left a comment

holzeis Apr 5, 2024

holzeis Apr 5, 2024

luckysori Apr 8, 2024 •

edited

Loading

holzeis Apr 5, 2024

luckysori Apr 8, 2024

luckysori commented Apr 9, 2024

holzeis Apr 9, 2024

luckysori Apr 9, 2024

luckysori Apr 10, 2024

holzeis Apr 10, 2024 •

edited

Loading

Reintroduce position resizing #2359

Reintroduce position resizing #2359

Conversation

luckysori commented Apr 5, 2024 • edited Loading

holzeis left a comment

Choose a reason for hiding this comment

holzeis Apr 5, 2024

Choose a reason for hiding this comment

holzeis Apr 5, 2024

Choose a reason for hiding this comment

luckysori Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

holzeis Apr 5, 2024

Choose a reason for hiding this comment

luckysori Apr 8, 2024

Choose a reason for hiding this comment

luckysori commented Apr 9, 2024

holzeis Apr 9, 2024

Choose a reason for hiding this comment

luckysori Apr 9, 2024

Choose a reason for hiding this comment

luckysori Apr 10, 2024

Choose a reason for hiding this comment

holzeis Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

luckysori commented Apr 5, 2024 •

edited

Loading

luckysori Apr 8, 2024 •

edited

Loading

holzeis Apr 10, 2024 •

edited

Loading