feat: Rework task status dialog #2502

holzeis · 2024-05-04T11:25:25Z

I simplified a lot in this patch and addressed almost all shortcomings I noticed.

Dialogs will not overlap in case another dialog is already shown.
Errors will transported via the TaskStatus.failed type allowing us to also show errors on rollover, etc.
Errors are also reported if some intermediate protocol step fails.
Aligned background task handling to reduce the amount of boilerplate code required to add a new background task.
Simplified the dialog content. It will only show a text, but no margin, fee or pnl anymore. I opted to remove that in favor of simplicity and re-usability.
No use of global keys to access the current context.
Dialogs can now also be shown on the settings screen.
The trade dialog is created through an async trade task event (equal to an expired or liquidated order). Note, it is still created optimistically assuming we will immediately find a match or fail. Changing that though will be very easy now.
Removed the confetti in favor of a more clean success dialog. (There is still room for improvement though)

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-05-04.at.13.23.10.mp4

fixes #2381
fixes #2089
fixes #1995

This will allow us to have root context, which can manage dialogs.

First step towards aggregating all dialogs to a single root context (xxi screen)

… notifier initialization I don't know anymore why this was added in the first place, but it doesn't make much sense anymore.

Before we showed the trade dialog optimistically assuming we will get a dlc offer instantly, even though this is coming asynchronously. Although there was no issue with this yet, I think moving this to the xxi screen as well makes sense, because we might end up with orders not getting immediately matched. Also gets rid of a redundant show dialog.

This simplifies the handling of background tasks. Next we will adapt the async order change notifier to fit into the background task pattern. This will eventually allow us to prevent multiple dialogs to be shown at once.

This patch simplifies a lot of the logic that has grown historically. The key changes are the following: - The AsyncTrade does not differentiate between manual or other order reasons. - The TaskStatus.failed enum transports any error that happens during the trade. Replacing the `submitOrderError` and `failureReason`. - The `PendingOrder` has been removed in favor of the `TaskStatus`. This gives us a little bit less information for the dialog, but this is in alignment with simplifying the task dialog in general. - If the protocol fails at any point during the trade, the error will be reported to the task dialog. Before we had only a few scenarios where an error was actually reported back to the UI, depending on the dialog to time out. Note, if the protocol fails on the coordinator side the dialog would still have to time out since we do not yet send a `TradeError` message in that case.

…he protocol.

mobile/lib/common/task_status_dialog.dart

It feels more logical to show the new task event event if the previous event was of a different type.

bonomat

Very much appreciated :)

Disclaimer: I didn't review the flutter code 🙈

This will allow us to clearly differentiate between a reopen/resize and a rollover protocol. We need this in order to be able to add the order id to all trade related messages. BREAKING change due to a changed protocol message

holzeis · 2024-05-06T14:25:44Z

@bonomat @luckysori I've additionally addressed the issue that errors on the coordinator side are not reported back to the app, thus the dialog timing out. These errors are now reported back to the app. This is still done using the websocket, we should probably also think about refactoring the TadeError and RolloverError websocket messages to p2p protocol messages.

The last changes also wraps all dlc renew protocol steps into dedicated Rollover messages so we can distinguish properly between a reopen | resize and a rollover.

Additionally I've added the order_id to all trade messages (finally fixing #2136) and the order_reason to the settle messages so we can differentiate between a regular settlement and irregular settlements like liquidation and expired positions.

Please have a look, this is not just a frontend change.

luckysori

LGTM overall. The scope is a bit out of control, so it's hard to review thoroughly, but I think the changes should not be very risky.

mobile/lib/common/xxi_screen.dart

mobile/native/src/dlc/node.rs

mobile/assets/error.gif

luckysori · 2024-05-07T01:42:04Z

mobile/lib/common/xxi_screen.dart

+              // update the active task to the last event received on the stack.
+              activeTask = task;


🤔 I'm not sure. I think it's just weird to receive a different DLC message when you are in the middle of another protocol.

Maybe we should not be bubbling up events that don't make sense?

In general that should never happen except for the following two scenarios.

The user did not close the dialog and another event happens. I think in this case its fine to simple overwrite the current active task. (Note, the active task will only be set to null if the dialog is closed.

The user reconnects in the middle of the protocol, that would trigger a RecoverDlc background task, which will change the task type. However the change is only maginal as only the text will be changed, stating that we lost connection and trying to recover.

All other scenarios would be bugs and I think it would be better to show them so they can be fixed instead of hiding them.

coordinator/src/emergency_kit.rs

crates/xxi-node/src/message_handler.rs

mobile/native/src/orderbook.rs

mobile/lib/common/background_task_dialog_screen.dart

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

This will allow us to uniquely identify the affected order BREAKING change due to a change protocol message.

This is useful during development when we reload the dart files to automatically re-render the dialog. However, the logic is only intended to get executed when there is an actual event, so this will fail if we reload manually. Adding this catch makes development a bit easier.

Note, the app could have crashed after the coordinator not being online for while since the round value grew indefinitely.

holzeis requested review from bonomat and luckysori May 4, 2024 11:25

holzeis self-assigned this May 4, 2024

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from 44893d5 to cd406c2 Compare May 4, 2024 11:27

holzeis added 15 commits May 4, 2024 13:35

chore: Wrap settings, trade and wallet screen into shell route

5edd90c

This will allow us to have root context, which can manage dialogs.

chore: Move compare coordinator version check to xxi screen

db8e105

First step towards aggregating all dialogs to a single root context (xxi screen)

chore: Move recover dialog to xxi screen

75dc01f

chore: Move rollover dialog to xxi screen

58e75f4

chore: Move async order dialog to xxi screen

5a13516

chore: Move full sync dialog to xxi screen

e52851b

chore: Move collab revert dialog to xxi screen

ee11bb1

chore: Remove unnecessary fetch async order during async order change…

16e5f2b

… notifier initialization I don't know anymore why this was added in the first place, but it doesn't make much sense anymore.

chore: Remove unused event

3afd6a9

feat: Add error message to failed task

1293dc4

chore: Combine background task change notifier

52515db

This simplifies the handling of background tasks. Next we will adapt the async order change notifier to fit into the background task pattern. This will eventually allow us to prevent multiple dialogs to be shown at once.

chore: Report failed rollover to the ui

b5b95d9

chore: Remove unused receive usdp components

3043849

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from cd406c2 to 891fc3f Compare May 4, 2024 11:35

holzeis added 3 commits May 4, 2024 13:48

feat: Redesign task status dialog

3f41d1e

chore: Move error details to task dialog

33a18a1

docs: Add todo to send trade error in case the coordinator fails in t…

3f06a74

…he protocol.

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from 891fc3f to 3f06a74 Compare May 4, 2024 11:48

feat: Make error details scrollable

5cd2288

holzeis commented May 5, 2024

View reviewed changes

mobile/lib/common/task_status_dialog.dart Outdated Show resolved Hide resolved

feat: Override active task if types are different

47f9329

It feels more logical to show the new task event event if the previous event was of a different type.

bonomat approved these changes May 5, 2024

View reviewed changes

holzeis added 2 commits May 6, 2024 08:25

fix: Add missing force reject case for rollover offer

dd93510

chore!: Add remaining rollover dlc messages

5eed091

This will allow us to clearly differentiate between a reopen/resize and a rollover protocol. We need this in order to be able to add the order id to all trade related messages. BREAKING change due to a changed protocol message

holzeis requested a review from bonomat May 6, 2024 14:35

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from 5bc7cd3 to ffb49d2 Compare May 6, 2024 18:26

luckysori approved these changes May 7, 2024

View reviewed changes

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from ffb49d2 to e6a5e7c Compare May 7, 2024 10:33

holzeis added 6 commits May 7, 2024 13:10

chore!: Add order id to trade messages

17e77dd

This will allow us to uniquely identify the affected order BREAKING change due to a change protocol message.

feat: Report trade error from coordinator if trade or rollover fails

70c79d2

chore: Delay websocket connection retry at most 1 second

5efc04b

chore: Rename xxi screen to background task dialog screen

760d805

chore: Don't wrap original error message in another error message

5fbb258

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from e6a5e7c to 4fe862b Compare May 7, 2024 11:10

holzeis added 2 commits May 7, 2024 14:37

feat: Add dedicated background tasks for expiry and liquidation

eb0b1a7

fix: Only return trade errors to the user in case of a manual order

6168453

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch 2 times, most recently from 4f93e32 to 9de0610 Compare May 7, 2024 12:41

holzeis added 2 commits May 7, 2024 14:53

chore: Do not loop gifs

77dd96a

fix: Align p2p and websocket connection reconnect timeouts

67f7592

Note, the app could have crashed after the coordinator not being online for while since the round value grew indefinitely.

holzeis force-pushed the chore/rework-trade-and-recover-dialog branch from 9de0610 to 67f7592 Compare May 7, 2024 12:53

holzeis added this pull request to the merge queue May 7, 2024

Merged via the queue into main with commit f0f2710 May 7, 2024
22 checks passed

holzeis deleted the chore/rework-trade-and-recover-dialog branch May 7, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rework task status dialog #2502

feat: Rework task status dialog #2502

holzeis commented May 4, 2024 •

edited

Loading

bonomat left a comment •

edited

Loading

holzeis commented May 6, 2024

luckysori left a comment

luckysori May 7, 2024

holzeis May 7, 2024

		// update the active task to the last event received on the stack.
		activeTask = task;

feat: Rework task status dialog #2502

feat: Rework task status dialog #2502

Conversation

holzeis commented May 4, 2024 • edited Loading

bonomat left a comment • edited Loading

Choose a reason for hiding this comment

holzeis commented May 6, 2024

luckysori left a comment

Choose a reason for hiding this comment

luckysori May 7, 2024

Choose a reason for hiding this comment

holzeis May 7, 2024

Choose a reason for hiding this comment

holzeis commented May 4, 2024 •

edited

Loading

bonomat left a comment •

edited

Loading