-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement GTC Orders #481
Implement GTC Orders #481
Conversation
I did a swap (CHIPS/KMD), and then clicked Cancel right away while it's "Pending" and it's been showing as "Pending" for 10 min now. When is it supposed to show as "Unmatched"? I did another swap, and now the "Cancel" button on the first swap is enabled again. Also at some point when the second swap was at |
So after clicking the "Cancel" button a second time on the first swap and waiting 3 more minutes, it now shows as "Failed". |
I did a third swap, and the "Cancel" button is disabled even when "Pending". |
The cancel behaviour is still slightly buggy but we need it to be able to cancel orders.
Unmatched isn't really used anymore, the order will remain open until it's matched or cancelled. Cancelling the trade will emit a failed event. It should cancel instantly (or whenever the cancel RPC method responds). I haven't been able to re-create this but if you notice it again can you check the network tab in dev tools and see if the cancel request is stuck as pending and if not what the response is.
Yup, can re-create this occasionally, seems like a small state bug in
Yeah, that's correct behaviour. I think we get a consistent error code for user-initiated cancels so will try and detect that and show the status as
Did it complete successfully? Can you share the debug data for that swap? |
https://gist.github.com/sindresorhus/b2332c940459aa6c590678854fe1e869 Notice the |
Error code You shouldn't be allowed to do that once it's matched. Did you click the cancel button at some point? If so can you remember if it was before or after it was matched? Seems possibly like an internal mm bug where:
Honestly, I don't think we can really fix this. It's just due to the buggyness of the cancel command and the fact it doesn't do proper swap management. It just handles things one at a time with global variables. Will report the issue to James though becuase that state should not be possible. Also, although obviously pretty confusing to the user, the updated message handling code from the stuck swaps PR will handle this fine, so functionality wise it doesn't break anything. After a cancel message we will still accept and update the UI with further progress events. |
I clicked "Cancel" while it was "Pending". |
Thanks, reported to James: jl777/SuperNET#956 |
Just realised, as an aside, this also closes #262 |
With GTC orders this wording seems more familair with the idea of an open order on a centralised exchange as opposed to a pending atomic swap.
I've been looking into this, it's definitely nothing wrong with the I'm fairly confident this is actually unrelated to this PR and is caused by the recent introduction of You can recreate it by cancelling some trades and then creating new ones. The button state seems to get shifted up a row when a new item is pushed. If you filter and then unfilter it displays properly. Or if you scroll the item out of view and then back in view it displays properly. This seems to me like Would someone be able to take a look into this? @kevva do you have prior experience with |
Seems so. If you click "Cancel", then scroll it out of view and then back again, the button is now longer disabled. |
Fixed in #486 |
I have been extensively testing this for many days now and it seems to work well in general. |
Merging to unblock other PRs. |
This PR makes all orders Good 'Till Cancelled (GTC) orders. If you place an order it will stay pending and keep re-broadcasting the order every 10 minutes until it either matches or the user cancels it.
If a user closes HyperDEX while an order is open it will show as unmatched the next time they open HyperDEX. We can look at trying to auto-restart these orders in the futire but it might be a good idea because the UTXOs required for the trade may have been spent externally while the app was closed and so the restart would fail.
I also found a few bugs in the GTC orders while implementing this. I've worked with James to fix these but the behaviour is still sometimes odd. If you create an order that can't be matched instantly, then later an order becomes available to match with it, it often has a lot of difficulty connecting and starting the swap. When cancelling the order and re opening it will match right away. However if you leave it for long enough it does seem to match eventually. More details here: jl777/SuperNET#954
I don't think we should let this issue block merging this PR. The our iplementation is correct and if this is resolved in
marketmaker
we can just drop in a new binary and it will be resolved in HyperDEX. It also doesn't really degrade the current usage because at the moment if there is no order available it will just fail. With this PR, even with inconsistent matching, it will stay open and eventually match, or the user can just cancel if it takes too long.So now the cancel button actually does something useful 🎉.
The cancel behaviour is still a bit buggy (#262), but way less noticeable in GTC mode as trades can remain in a pending and cancellable state for a much longer period of time. Once a trade is matched and starts the swap it can no longer be cancelled (from the UI).
I'm planning on updating some of the UI and terminology in a follow up PR to better reflect the new behaviour. "Recent Swaps" in the exchange view will become "Open Orders" and only show open orders, once completed they will dissapear from this view. I also think we should change "pending" => "open" to equate better to centralised exchange terminology of an open order as opposed to a pending atomic swap.
Because of this we need the cancel button in the Exchange view so I had to play around a bit with the styling to fit everything in.
If all currencies used sensible ticker lengths this would be a lot easier but we need to account for some asset chains which have super long names like SUPERNETwhich is a pain. I think I've managed to smush everything round to make good use of the available space now.
The new styles use the existing spacing when the window is small:
Then at 1280px everything goes on one line and we drop the trading pair to make it fit. I think it's fine removing the trading pair because we still list the base and quote currencies in the amount:
At 1480px we add the trading pair back in:
This allows the cancel button to fit in fine. Even with long tickers such as SUPERNET.
Fixes #262