Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

Cancel order: Sync statuses in Explorer and CowSwap #612

Closed
elena-zh opened this issue Jul 19, 2021 · 15 comments · Fixed by #999
Closed

Cancel order: Sync statuses in Explorer and CowSwap #612

elena-zh opened this issue Jul 19, 2021 · 15 comments · Fixed by #999
Assignees
Labels
app:Explorer Explorer App Bug Something isn't working Medium Protofire task to the protofire team

Comments

@elena-zh
Copy link

elena-zh commented Jul 19, 2021

The issue is related to the 'Cancel' order transactions.

When I cancel an order, it is immediately changes its status as 'Cancelled' in the protocol explorer.
However, as we use a 'soft' cancellation, the order might be executed, hence, the status is changed to 'Filled' in the explorer, and it seems to be strange a bit.
I think it would be nice to add 'Cancelling' status in the protocol explorer (link it is in the CowSwap app) for an order and show it until an order is successfully cancelled/filled.
https://drive.google.com/file/d/1j92oV8O2LLPMGZgCY4FSUnFZrXXhjpVL/view

@alfetopito
Copy link
Contributor

There are two ways of thinking about this.

  1. The explorer is an Etherscan like interface for GP orders and what you see (on the backend) is what you get.
  2. The explorer should have the same behaviour as CowSwap, to have the same information on both.

The reason why CowSwap does not show the cancelled update straight away, is that due to race conditions, the order can still be matched even after the request to cancel it was successful.
So we show it as pending cancellation until we has reasonably certain it has not been matched in the mean time.

We have though discussed internally to display the intermediate cancellation requested state.
We'll probably work on it when the orders history is re-worked on CowSwap gnosis/cowswap#892

@elena-zh
Copy link
Author

I'd like to add another case.
Sometimes, I see the notification message in the CowSwap app that my transaction is successfully cancelled.
However, in a few seconds, I may receive a notification about the successful order placement.
Looks a bit confusing to me..

@alfetopito
Copy link
Contributor

I'd like to add another case.
Sometimes, I see the notification message in the CowSwap app that my transaction is successfully cancelled.
However, in a few seconds, I may receive a notification about the successful order placement.
Looks a bit confusing to me..

You do?
Well, I thought I fixed that, apparently I didn't :/

I can play around with the time we wait for this, but in the end this is a race condition.
We could wait even longer to consider the order cancelled to reduce the frequency it happens, at the cost of the user having to wait more for a feedback.

@elena-zh
Copy link
Author

@alfetopito , at least, I faced this issue twice.
I will keep my eye on it further..

@elena-zh
Copy link
Author

@alfetopito ,

You do?
Well, I thought I fixed that, apparently I didn't :/

I can play around with the time we wait for this, but in the end this is a race condition.
We could wait even longer to consider the order cancelled to reduce the frequency it happens, at the cost of the user having to wait more for a feedback.

video proof:

Untitled_.Jul.28.2021.7_05.PM.mp4

@alfetopito
Copy link
Contributor

Never doubted you!
Just thought I fixed it 😓

@elena-zh
Copy link
Author

One more issue related to statuses in Explorer: my transaction time has expired, however, status is still 'Open"..
0 min

@alfetopito
Copy link
Contributor

It "should" update in the background every 10s until it's expired, thus updating the state.
If it didn't, probably need to keep checking for awhile longer.
I believe a refresh would fix it?

@elena-zh
Copy link
Author

@alfetopito , statuses were updated in the end. But it took more than 10 sec.
Refreshing helps.

@elena-zh elena-zh changed the title Sync statuses in the protocol explorer and in the CowSwap app Cancel order: Sync statuses in Explorer and CowSwap + double confirmation 'Cancel-Filled' Aug 4, 2021
@elena-zh
Copy link
Author

With a new sidebar implementation now it looks odd when 'valid to' time is less than a current one, but the order is still in 'Open' status..
image

@alfetopito alfetopito removed their assignment Aug 26, 2021
@alfetopito alfetopito transferred this issue from gnosis/cowswap Aug 26, 2021
@elena-zh elena-zh added app:Explorer Explorer App Medium Bug Something isn't working labels Aug 27, 2021
@anxolin
Copy link
Contributor

anxolin commented Aug 30, 2021

@elena-zh u are super good at testing edge cases 💪

With a new sidebar implementation now it looks odd when 'valid to' time is less than a current one, but the order is still in 'Open' status..

I think this should only happen for a few seconds, then should expire. This is because we only update periodically. It can be improved if we actually set a timer instead of periodically update. However, i would consider it low prio. If we want to keep track of that we can make an independent issue for it.

One more issue related to statuses in Explorer: my transaction time has expired, however, status is still 'Open"..

I think is similar to the previous case. We could also do a similar solution. Also, im not too concerned with this one for now. But true that it might be a good idea to track it.

Sometimes, I see the notification message in the CowSwap app that my transaction is successfully cancelled.
However, in a few seconds, I may receive a notification about the successful order placement.
Looks a bit confusing to me..

This one is more annoying 😅
I think we should fix it, not too urgent, but I think this one should be in it's own issue because is unrelated to the others. Is likely that we already have it, or we have one very similar.

@anxolin
Copy link
Contributor

anxolin commented Aug 30, 2021

#612 I think could potentially be 4 issues. 1 i think is important to solve eventually. Then there’s issues related to the canceled state having a small delay (that is fine for now imo, but can be improved).

Then there’s the issue of CowSwap / Explorer discrepancy on CANCEL workflow. CowSwap has “canceling state”. I understand why we did it differently, but I would argue that is more important to be coherent.

@elena-zh
Copy link
Author

I think this should only happen for a few seconds, then should expire.

If only for a few seconds...
This happens for a minute or so. That's why I'm reporting this issue=)

@anxolin
Copy link
Contributor

anxolin commented Aug 30, 2021

This happens for a minute or so. That's why I'm reporting this issue=)

ohh, ok! then we should definitely review. Thanks for raising it. At least we could make the check more frequent, that would be a quick fix

@elena-zh
Copy link
Author

elena-zh commented Aug 30, 2021

Yes, For convenience, here is a separate issue for this case #1362

@alfetopito alfetopito mentioned this issue Oct 19, 2021
4 tasks
@elena-zh elena-zh changed the title Cancel order: Sync statuses in Explorer and CowSwap + double confirmation 'Cancel-Filled' Cancel order: Sync statuses in Explorer and CowSwap Nov 9, 2021
mergify bot pushed a commit that referenced this issue Nov 24, 2021
@henrypalacios henrypalacios added the Protofire task to the protofire team label Dec 3, 2021
@mergify mergify bot closed this as completed in #999 Feb 1, 2022
mergify bot pushed a commit that referenced this issue Feb 1, 2022
# Summary

Closes #612 

**Proposal:**
Add logic to check if the order is cancelled, [cowswap reference](https://github.com/gnosis/cowswap/blob/2165-adding-wallet-supports/src/custom/state/orders/utils.ts#L38)

# To Test

1. Create a new swap on [dev cowswap environment](https://cowswap.dev.gnosisdev.com/#/swap).
2. Go to pending button and clicked, it will open `account modal` try to cancel order.
3. Go to explorer https://pr999--gpui.review.gnosisdev.com/rinkeby/address/<address>    (:bulb:  replace your wallet `address`)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:Explorer Explorer App Bug Something isn't working Medium Protofire task to the protofire team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants