chore: Sync position with dlc channel state #1298

holzeis · 2023-09-14T12:25:45Z

This change will sync the position to the dlc channel state in the following scenarios

DLC Channel in state Signed but no position: Create position from filling order.
DLC Channel in state OffChainClosed and a position exists. Delete the position.
DLC Channel in state CloseOffered or CloseAccepted: Inform the UI that the dlc channel is recovering.
DLC Channel in state Offered, Accepted or Finalized: Inform the UI that the dlc channel is recovering.
DLC Channel in any other state but with position: Delete position the channel might have been force closed.

Recover from scenarios 1 and 2.

I have commented the part of the code that should update the position state, so that the spinner will go forever when opening or closing a position.

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

Recover from scenarios 3 and 4.

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

Interestingly this takes quite a long time to complete. I did at a 5 seconds sleep for the channel to get ready with 4a5e66d, but it still takes far too long.

Manually calling the period_check to process any pending actions immediately "fixed" that issue, but should be fixed in rust-dlc. There is no reason why we should check every 30 seconds if pending actions need to be processed as pending action can only ever happen after channel reestablishment.

Recover from scenario 5 (force closure)

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

fixes #1273
fixes #1271

mobile/lib/features/trade/async_order_change_notifier.dart

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

crates/ln-dlc-node/src/node/dlc_channel.rs

crates/ln-dlc-node/src/node/mod.rs

klochowicz · 2023-09-14T19:05:47Z

mobile/lib/common/recover_dlc_change_notifier.dart

+          builder: (context) {
+            TaskStatus status = context.watch<RecoverDlcChangeNotifier>().taskStatus;
+
+            // todo(holzeis): Reusing the order submission status dialog is not nice, but it's actually suitable for any task execution that has pending,


is there a particular reason you're not just keeping this as normal TODOs ? e.g. do you have a commit hook that checks for these specifically?

on my end is worse than normal TODO as it's harder to spot it both with the naked eye when reading the code and also doesn't get picked up by my IDE which e.g. automatically shows me TODOs that are not on the main branch.

also, one can see the author of a TODO with regular git blame.

if you don't mind either way, please consider the points above 🙏

I think I've seen it on other projects (rust-dlc?) done like that - and I like it. It shows ownership of the todo. git blame is not always trustworthy as moving code will easily change that.

I like doing it like that and I don't think it hurts, but if you are feeling strongly about it I can change it to a regular todo.

fyi: my IDE is highlighting these todos the same way its highlighting any other todo.

if you are using this plugin https://github.com/alphapapa/magit-todos, I guess the only issue was that I did not capitalized the keyword.

Items in KEYWORD(username): format are also read

I have capitalized all fixmes and todos, let me know if this is working for you now.

mobile/native/src/ln_dlc/node.rs

mobile/lib/common/domain/background_task.dart

mobile/native/src/ln_dlc/mod.rs

mobile/native/src/orderbook.rs

luckysori

LGTM, but I would really appreciate some tests. I think we can fairly simply define a pure function that can be unit tested. Then it wouldn't be so important to review the tricky logic.

crates/ln-dlc-node/src/node/dlc_channel.rs

luckysori · 2023-09-15T08:36:28Z

mobile/native/src/event/api.rs

@@ -32,8 +32,14 @@ pub enum Event {
 #[frb]
 #[derive(Clone)]
 pub enum BackgroundTask {


❓ The name BackgroundTask doesn't make sense to me atm. These are just messages, right?

well, they are messages tasks executed in the background 😅. Any suggestion for a better name? 🙂

I think it's fine looking at it again.

mobile/native/src/ln_dlc/node.rs

mobile/native/src/ln_dlc/recover.rs

holzeis · 2023-09-18T06:35:48Z

@luckysori @klochowicz I have addressed your feedback and also added unit tests. de37a34#diff-817ef51e9e3a8d899433cfad50ab3e46d729def3e1bd663f5d5614a8a468e174

Please have another look.

klochowicz

LGTM!

klochowicz · 2023-09-18T16:55:10Z

mobile/lib/common/task_status_dialog.dart

-        switch (widget.type) {
-          case OrderSubmissionStatusDialogType.pendingSubmit:
-          case OrderSubmissionStatusDialogType.successfulSubmit:
+        switch (widget.status) {
+          case TaskStatus.pending:
            return "Pending";
-          case OrderSubmissionStatusDialogType.filled:
+          case TaskStatus.success:
            return "Success";
-          case OrderSubmissionStatusDialogType.failedSubmit:
-          case OrderSubmissionStatusDialogType.failedFill:
+          case TaskStatus.failed:
            return "Failure";


the way how it made the widget simpler confirms your initial intuition that it could be a more general-purpose widget ❤️

luckysori

Thank you for adding the tests!

luckysori · 2023-09-19T13:14:10Z

mobile/native/src/event/api.rs

@@ -32,8 +32,14 @@ pub enum Event {
 #[frb]
 #[derive(Clone)]
 pub enum BackgroundTask {


I think it's fine looking at it again.

mobile/native/src/event/api.rs

Before we got confusing log messages with "received unexpected event".

This change will do the following things depending on the dlc channel state and position. - DLC Channel in state `Signed` but no position: Create position from `filling` order. - DLC Channel in state `OffChainClosed` and a position exists. Delete the position. - DLC Channel in state `CloseOffered` or `CloseAccepted`: Inform the UI that the dlc channel is recovering. - DLC Channel in state `Offered`, `Accepted` or `Finalized`: Inform the UI that the dlc channel is recovering. - DLC Channel in any other state but with position: Delete position the channel might have been force closed.

The `OrderSubmissionStatusDialog` is actually useful for any kind of task showing a pending, failed or success state.

holzeis requested review from klochowicz and luckysori September 14, 2023 12:25

holzeis self-assigned this Sep 14, 2023

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from 4a5e66d to a285949 Compare September 14, 2023 12:26

holzeis changed the title ~~chore: Ignore unrelated background notification tasks~~ chore: Sync position with dlc channel state Sep 14, 2023

This was linked to issues Sep 14, 2023

Sync position with dlc channel state #1273

Closed

Show order filling dialog when app is restarted in an intermediate dlc channel state #1271

Closed

holzeis marked this pull request as draft September 14, 2023 15:08

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from a285949 to 9db92f8 Compare September 14, 2023 15:23

holzeis marked this pull request as ready for review September 14, 2023 15:23

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from 9db92f8 to 38bf276 Compare September 14, 2023 15:25

klochowicz reviewed Sep 14, 2023

View reviewed changes

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from 38bf276 to 28224b1 Compare September 15, 2023 07:27

holzeis mentioned this pull request Sep 15, 2023

Periodic check is called only every 30 seconds #1302

Closed

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from 28224b1 to e0c021e Compare September 15, 2023 08:12

holzeis requested a review from klochowicz September 15, 2023 08:16

holzeis force-pushed the feat/display-rollover-option-to-user branch 2 times, most recently from 6ae6404 to 7fbb639 Compare September 15, 2023 08:45

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from e0c021e to 26cd809 Compare September 15, 2023 08:51

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

luckysori reviewed Sep 15, 2023

View reviewed changes

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch 4 times, most recently from bc131ad to fe955b8 Compare September 18, 2023 06:34

holzeis requested a review from luckysori September 18, 2023 06:35

klochowicz approved these changes Sep 18, 2023

View reviewed changes

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

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from fe955b8 to e37bf12 Compare September 19, 2023 12:51

luckysori approved these changes Sep 19, 2023

View reviewed changes

chore: Ignore unrelated background notification tasks

e9af101

Before we got confusing log messages with "received unexpected event".

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from e37bf12 to 0067166 Compare September 19, 2023 14:03

holzeis added 4 commits September 19, 2023 16:06

refactor: OrderSubmissionStatusDialog to TaskStatusDialog

ec39bcf

The `OrderSubmissionStatusDialog` is actually useful for any kind of task showing a pending, failed or success state.

chore: Set rollover and recover dialog to failed after timeout

270f6c0

chore: Capitalize todos and fixmes

f33faa5

holzeis force-pushed the chore/sync-position-with-dlc-channel-state branch from 0067166 to f33faa5 Compare September 19, 2023 14:06

Base automatically changed from feat/display-rollover-option-to-user to main September 19, 2023 14:35

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

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

holzeis deleted the chore/sync-position-with-dlc-channel-state branch September 19, 2023 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Sync position with dlc channel state #1298

chore: Sync position with dlc channel state #1298

holzeis commented Sep 14, 2023 •

edited

Loading

klochowicz Sep 14, 2023

holzeis Sep 15, 2023

holzeis Sep 15, 2023

luckysori left a comment

luckysori Sep 15, 2023

holzeis Sep 17, 2023

luckysori Sep 19, 2023

holzeis commented Sep 18, 2023 •

edited

Loading

klochowicz left a comment

klochowicz Sep 18, 2023

luckysori left a comment

luckysori Sep 19, 2023

chore: Sync position with dlc channel state #1298

chore: Sync position with dlc channel state #1298

Conversation

holzeis commented Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

holzeis commented Sep 18, 2023 • edited Loading

klochowicz left a comment

Choose a reason for hiding this 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

holzeis commented Sep 14, 2023 •

edited

Loading

holzeis commented Sep 18, 2023 •

edited

Loading