Handle expired orders in the app #923

klochowicz · 2023-07-07T07:52:34Z

prune expired orders in the app when maker or coordinator is offline
add a test that ensures we detect lack of orders

TODO:

Disable open/close buttons when there's no 'best price' for a given direction
disable 'close position' button if there's no price in that direction

klochowicz · 2023-07-10T03:56:26Z

Two remarks:

killing a process and spawning it again from the integration test is not great, but does the job - potentially we could have an endpoint on maker and coordinator to "disable" them for test purposes without killing the processes. This would simplify the current workaround.
@da-kami suggested that it might be good to add an event to the orderbook stream that informs subscribers about the pruning. I was leaning towards this as well, so let me know whether it would be useful to add it now or not.

da-kami

Happy to see this merged, but I think just disabling the buttons might be quite confusing.
The user does not know why the buttons are disabled - we should add some form of indicator that shows the connection status to orderbook / coordinator that makes it obvious that the buttons are disabled because there is not connection.

Alternatively we could also display a toast for now so the user know why when the user enters the screen.

da-kami · 2023-07-11T05:33:22Z

crates/tests-e2e/tests/zzz_stop_orders.rs

+    kill_process("maker").expect("to be able to kill maker process");
+    assert!(!is_maker_running(), "maker should be stopped by now");
+
+    let wait_time = std::time::Duration::from_secs(70);


❓ Would it be possible to make the expiry time configurable so we can set it to e.g. 5 seconds instead so this test does not have to wait for 70 seconds? 😅

it would be nice indeed, I will create a ticket for this!

I'm against having a test which runs for 70+ seconds :/

Moved the test into a separate PR #937

I also realised that I don't need to wait for that long, because maker is likely to have already sent an order (and wait_until! covers the rest).

this of course in pessimistic scenario will still go for way too long, but having #935 would be a good remedy.

mobile/native/src/orderbook.rs

da-kami · 2023-07-11T05:40:40Z

mobile/test/trade_test.dart

+    PositionChangeNotifier positionChangeNotifier = PositionChangeNotifier(positionService);
+
+    // We have to have current price, otherwise we can't take order
+    positionChangeNotifier.price = Price(bid: 30000.0, ask: 30000.0);


nit: I think could potentially solve this cleaner by adding an explicit Extension to the PositionChangeNotifier in the tests that adds a setter so we can set the price; then we don't have to expose the private _price.

I don't think this adds anything, a direct getter and setter == a public member....

mobile/lib/features/trade/position_list_item.dart

klochowicz · 2023-07-11T05:57:44Z

Happy to see this merged, but I think just disabling the buttons might be quite confusing. The user does not know why the buttons are disabled - we should add some form of indicator that shows the connection status to orderbook / coordinator that makes it obvious that the buttons are disabled because there is not connection.

Alternatively we could also display a toast for now so the user know why when the user enters the screen.

I agree with you that this would be ideal, there's a ticket scheduled to do this for the upcoming iteration #915.

I typically try to split the plumbing / backend support for a feature from the ideal UX, to avoid painful rebasing and to be able to concentrate on flutter in the follow-ups. YMMV though.

bonomat · 2023-07-11T06:04:30Z

crates/tests-e2e/tests/zzz_stop_orders.rs

+
+#[tokio::test]
+#[ignore = "need to be run with 'just e2e' command"]
+async fn no_orders_when_counterparty_is_offline() {


The test name says that you should not expect an order if the counterparty is offline. This makes sense of course, but I guess what you are testing here is that outdated orders (older than 1m) are not beeing forwarded anymore?

I don't think there is any use in testing if a terminated binary is not sending orders anymore.

If you want to test expired orders, you can do a simpler test:

Only start the coordinator and the app

Manually post a NewOrder to the coordinator using a reqwest client. Here you can set the expiry to in Xsec.

Assert that no new order arrives past this few seconds.

But even that test might not be that useful...

I wanted to test that we detect lack of orders when the counterparty goes offline. As it turns out, we didn't. So I wrote the code that prunes outdated orders in the app so the feature would be working.

I like your idea with 'faking' maker; we could have created one with different expiry.
I don't think that the test would be simpler, because we would need to kill the maker anyway, and start using a "fake" one, but it certainly would be faster!

I don't mind stripping out the test if you see no value in it.

I don't mind stripping out the test if you see no value in it.

Please treat me as an equal engineer, not as a your superior :)

Thinking about this more, can we write unit tests for this at the places where it matters? e.g.

here we write a test that no expired order is being forwarded

in the frontend we check if the order is still up2date when reading it.

Perhaps let's rephrase it:

I don't mind stripping out the test if you see no value in it.

What I meant was that I'm OK with stripping out the test if it causes contention for any of the reviewers, as it's not strictly necessary for addressing the issue.

I wrote the test because things were supposed to work, and didn't (normal part of engineering process). IMHO it would be great if we keep the test as it's proven to be useful.

I believe integration tests like this are needed; we need to know how we behave when the other party is not available and have that tested.

The criticism for waiting 70s is very much valid, and there's an open ticket that would address it - the maker doesn't have HTTP API to control orders for now, so it was not easy to address in this PR.

🤔Could I have started with an unit test instead? I don't think so - unit tests were there, it's just this case was missed during the implementation phase.

If you would like to add additional unit tests, or schedule them for the iteration, I think it's a good idea (the more unit tests the better!).

@bonomat I have extracted the test into a separate PR, so it's not blocking the functionality: #937

holzeis · 2023-07-11T20:24:20Z

🤔 i am bit late to the party, but is it so bad that we let an order fail if there is no match?

klochowicz · 2023-07-12T01:15:43Z

🤔 i am bit late to the party, but is it so bad that we let an order fail if there is no match?

I'm not sure what are you referring to. IMHO you should disable the buttons where there is no orders in the orderbook and you cannot do anything.
of course one could hide it in the hope that no-one notices, but that's not good UX.

Monitor prices and position close events. These events can be later asserted on in tests.

Until now the expired orders were never pruned in case the other party did not explicitly deleted it. This would have happened

The package version changed recently, and it's only apparent after one runs the project.

If there's no bid/ask price, disable the buttons that would react to it.

holzeis · 2023-07-12T04:50:47Z

🤔 i am bit late to the party, but is it so bad that we let an order fail if there is no match?

I'm not sure what are you referring to. IMHO you should disable the buttons where there is no orders in the orderbook and you cannot do anything.

of course one could hide it in the hope that no-one notices, but that's not good UX.

Well, I guess the issue is that we are using the order price in the first place. That made it look odd as there is no price.

But we could also simply use the actual price (e.g. from bitmex) as reference, hit order and if we can't find a match, we fail the order.

I tend to prefer not disabling the buttons, but no need to action it. We might want to revisit that another time. 🙂

klochowicz · 2023-07-12T04:57:48Z

🤔 i am bit late to the party, but is it so bad that we let an order fail if there is no match?

I'm not sure what are you referring to. IMHO you should disable the buttons where there is no orders in the orderbook and you cannot do anything.
of course one could hide it in the hope that no-one notices, but that's not good UX.

Well, I guess the issue is that we are using the order price in the first place. That made it look odd as there is no price.

I don't see it as an issue. If there is no order that can be taken, why would not show this fact?

But we could also simply use the actual price (e.g. from bitmex) as reference, hit order and if we can't find a match, we fail the order.

I tend to prefer not disabling the buttons, but no need to action it. We might want to revisit that another time. 🙂

again, it's poor UX to not disable the buttons when we know for a fact that they cannot succeed.
I agree with @da-kami that just disabling the buttons is not enough, and we should be more clear why they are disabled, but this is just a start.

holzeis · 2023-07-12T05:00:31Z

Again, I do not agree on the poor UX argument. But as mentioned we don't have to action it now.

klochowicz · 2023-07-12T05:10:14Z

Again, I do not agree on the poor UX argument. But as mentioned we don't have to action it now.

Please state your reasons, as I would love to understand your POV.

There's very little more frustrating experiences on the web that needing to submit a form to check whether we can submit a form.
if I understood your proposal correctly - what you are proposing, is needing to submit an order to see whether we can submit an order.

klochowicz · 2023-07-12T05:13:43Z

I'm merging this, as the unsolved issues are captured in the follow-up PR #937 or the follow-up ticket scheduled for this iteration: #915

klochowicz · 2023-07-12T05:13:58Z

bors r+

bors · 2023-07-12T05:35:39Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

holzeis · 2023-07-12T17:56:25Z

Again, I do not agree on the poor UX argument. But as mentioned we don't have to action it now.

Please state your reasons, as I would love to understand your POV.

There's very little more frustrating experiences on the web that needing to submit a form to check whether we can submit a form.

if I understood your proposal correctly - what you are proposing, is needing to submit an order to see whether we can submit an order.

Disabling the button presumes that we either have an immediate match or fail.
Enabled buttons will not guarantee that the order will not fail.
The order matching is the responsibility of the coordinator.

Bottom line, by disabling the buttons we already imply that there is no match. This might be the case now as we have a very simplified matching logic, but this will change.

In other words, I think it's perfectly fine to fail an order if there is no match. The App should not implement business logic which is in the responsibility of the coordinator.

holzeis · 2023-07-12T20:50:11Z

Additionally, we will not learn about an trading attempt, indicating more demand.

klochowicz self-assigned this Jul 7, 2023

klochowicz linked an issue Jul 7, 2023 that may be closed by this pull request

Deal with order unavailability in the 10101 app #860

Closed

4 tasks

klochowicz marked this pull request as draft July 7, 2023 08:19

This comment was marked as resolved.

Sign in to view

klochowicz force-pushed the feat/notice-expired-orders-in-app branch 2 times, most recently from 51e54b0 to 710319e Compare July 10, 2023 03:51

klochowicz requested review from da-kami, bonomat and holzeis July 10, 2023 03:51

klochowicz marked this pull request as ready for review July 10, 2023 03:51

klochowicz force-pushed the feat/notice-expired-orders-in-app branch 3 times, most recently from 444eaa3 to 05189e5 Compare July 10, 2023 06:12

This comment was marked as resolved.

Sign in to view

klochowicz force-pushed the feat/notice-expired-orders-in-app branch from 05189e5 to c14c168 Compare July 11, 2023 01:45

da-kami approved these changes Jul 11, 2023

View reviewed changes

klochowicz mentioned this pull request Jul 11, 2023

Make the order expiry time configurable #935

Closed

bonomat reviewed Jul 11, 2023

View reviewed changes

klochowicz added 7 commits July 12, 2023 14:05

chore(tests-e2e): Subscribe to all notifications in TestSubscriber

7e17580

Monitor prices and position close events. These events can be later asserted on in tests.

chore(tests-e2e): Add more debug logs

793d1c8

chore(coordinator): Move websocket handling into own module

f1aa9a4

fix(app): Prune expired orders

f0d4278

Until now the expired orders were never pruned in case the other party did not explicitly deleted it. This would have happened

chore: Commit latest Podfile.lock

956d26b

The package version changed recently, and it's only apparent after one runs the project.

chore: Capture backtraces when running the tests

f54a5ab

feat: Disable taking orders when the prices are not available

3c4c848

If there's no bid/ask price, disable the buttons that would react to it.

klochowicz force-pushed the feat/notice-expired-orders-in-app branch from 1cc6c5d to f27f80d Compare July 12, 2023 04:36

klochowicz mentioned this pull request Jul 12, 2023

chore(tests-e2e): Add a test to verify that backends detects lack of orders #937

Closed

feat: Disable close position button when no orders in orderbook

a2f71b2

klochowicz force-pushed the feat/notice-expired-orders-in-app branch from f27f80d to a2f71b2 Compare July 12, 2023 05:11

bors bot merged commit 71c8237 into main Jul 12, 2023
7 checks passed

bors bot deleted the feat/notice-expired-orders-in-app branch July 12, 2023 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle expired orders in the app #923

Handle expired orders in the app #923

klochowicz commented Jul 7, 2023 •

edited

Loading

This comment was marked as resolved.

klochowicz commented Jul 10, 2023

This comment was marked as resolved.

da-kami left a comment

da-kami Jul 11, 2023

klochowicz Jul 11, 2023

bonomat Jul 11, 2023

klochowicz Jul 12, 2023 •

edited

Loading

da-kami Jul 11, 2023

klochowicz Jul 11, 2023

klochowicz commented Jul 11, 2023

bonomat Jul 11, 2023

klochowicz Jul 11, 2023

bonomat Jul 11, 2023

klochowicz Jul 12, 2023 •

edited

Loading

klochowicz Jul 12, 2023

holzeis commented Jul 11, 2023

klochowicz commented Jul 12, 2023

holzeis commented Jul 12, 2023

klochowicz commented Jul 12, 2023

holzeis commented Jul 12, 2023 •

edited

Loading

klochowicz commented Jul 12, 2023

klochowicz commented Jul 12, 2023

klochowicz commented Jul 12, 2023

bors bot commented Jul 12, 2023

holzeis commented Jul 12, 2023 •

edited

Loading

holzeis commented Jul 12, 2023

Handle expired orders in the app #923

Handle expired orders in the app #923

Conversation

klochowicz commented Jul 7, 2023 • edited Loading

This comment was marked as resolved.

klochowicz commented Jul 10, 2023

This comment was marked as resolved.

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

Choose a reason for hiding this comment

klochowicz Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis commented Jul 11, 2023

klochowicz commented Jul 12, 2023

holzeis commented Jul 12, 2023

klochowicz commented Jul 12, 2023

holzeis commented Jul 12, 2023 • edited Loading

klochowicz commented Jul 12, 2023

klochowicz commented Jul 12, 2023

klochowicz commented Jul 12, 2023

bors bot commented Jul 12, 2023

holzeis commented Jul 12, 2023 • edited Loading

holzeis commented Jul 12, 2023

klochowicz commented Jul 7, 2023 •

edited

Loading

klochowicz Jul 12, 2023 •

edited

Loading

klochowicz Jul 12, 2023 •

edited

Loading

holzeis commented Jul 12, 2023 •

edited

Loading

holzeis commented Jul 12, 2023 •

edited

Loading