Skip to content
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

Merged
merged 11 commits into from
Aug 27, 2018
Merged

Implement GTC Orders #481

merged 11 commits into from
Aug 27, 2018

Conversation

lukechilds
Copy link
Member

@lukechilds lukechilds commented Aug 18, 2018

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:

screen shot 2018-08-18 at 6 14 03 pm

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:

screen shot 2018-08-18 at 6 14 33 pm

At 1480px we add the trading pair back in:

screen shot 2018-08-18 at 6 15 00 pm

This allows the cancel button to fit in fine. Even with long tickers such as SUPERNET.


Fixes #262

@sindresorhus
Copy link
Contributor

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 Swap 1/5 it suddenly showed as Failed for 5 seconds.

@sindresorhus
Copy link
Contributor

So after clicking the "Cancel" button a second time on the first swap and waiting 3 more minutes, it now shows as "Failed".

@sindresorhus
Copy link
Contributor

I did a third swap, and the "Cancel" button is disabled even when "Pending".

@lukechilds
Copy link
Member Author

lukechilds commented Aug 19, 2018

The cancel behaviour is still slightly buggy but we need it to be able to cancel orders.

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"?

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.

I did another swap, and now the "Cancel" button on the first swap is enabled again.

I did a third swap, and the "Cancel" button is disabled even when "Pending".

Yup, can re-create this occasionally, seems like a small state bug in <CancelButton />, will look into that.

So after clicking the "Cancel" button a second time on the first swap and waiting 3 more minutes, it now shows as "Failed".

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 cancelled instead of failed for clarity.

Also at some point when the second swap was at Swap 1/5 it suddenly showed as Failed for 5 seconds.

Did it complete successfully? Can you share the debug data for that swap?

@sindresorhus
Copy link
Contributor

Did it complete successfully? Can you share the debug data for that swap?

https://gist.github.com/sindresorhus/b2332c940459aa6c590678854fe1e869

Notice the failed event at line 341.

@lukechilds
Copy link
Member Author

Error code -9997 is user cancelled trade: https://github.com/jl777/SuperNET/blob/2203c06680b3b62ea0608ac1c1c40cda25832115/iguana/exchanges/LP_ordermatch.c#L755-L757

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:

  • Maybe you clicked cancel when it was pending
  • The cancel command got blocked internally in mm
  • The trade matched
  • The cancel command somehow responded successfully (because it was issued before match)
  • Trade was already matched so couldn't actually be cancelled

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.

@sindresorhus
Copy link
Contributor

I clicked "Cancel" while it was "Pending".

@lukechilds
Copy link
Member Author

Thanks, reported to James: jl777/SuperNET#956

@lukechilds
Copy link
Member Author

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.
@lukechilds
Copy link
Member Author

lukechilds commented Aug 22, 2018

@sindresorhus

I did another swap, and now the "Cancel" button on the first swap is enabled again.

I did a third swap, and the "Cancel" button is disabled even when "Pending".

Yup, can re-create this occasionally, seems like a small state bug in <CancelButton />, will look into that.

I've been looking into this, it's definitely nothing wrong with the <CancelButton /> component, it's super simple, just has a default state of enabled and can only be set to disabled by being clicked.

I'm fairly confident this is actually unrelated to this PR and is caused by the recent introduction of react-virtualized to the <SwapList /> component.

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 react-virtualized is trying to do some clever optimisations to reduce component re-render and is somehow confusing the state when new items are pushed.

Would someone be able to take a look into this? @kevva do you have prior experience with react-virtualized?

@sindresorhus
Copy link
Contributor

I'm fairly confident this is actually unrelated to this PR and is caused by the recent introduction of react-virtualized to the component.

Seems so. If you click "Cancel", then scroll it out of view and then back again, the button is now longer disabled.

@sindresorhus
Copy link
Contributor

I'm fairly confident this is actually unrelated to this PR and is caused by the recent introduction of react-virtualized to the component.

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

@sindresorhus
Copy link
Contributor

I have been extensively testing this for many days now and it seems to work well in general.

@sindresorhus sindresorhus merged commit 0040d04 into master Aug 27, 2018
@sindresorhus
Copy link
Contributor

Merging to unblock other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Cancel" button to the trades in the Exchange view
2 participants