Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Update orders faster #1893

Merged
merged 1 commit into from
Nov 25, 2021
Merged

Update orders faster #1893

merged 1 commit into from
Nov 25, 2021

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Nov 19, 2021

Summary

Tries to improve the UX around when we detect an order has been fullfileed.

This PR is a bit sensitive because changes:

  • the update logic for pending orders
  • the update logic for recently cancelled orders
  • the update logic for the check that tells the user if the order is still with a good price (or if they are out of market)

However, the changes try to be scoped to only:

  • Update faster (5X) the pending orders --> 2s
  • Also the check where we verify if orders are still within the market price (2x) --> 15s
  • Additionally, there's a flag to prevent concurrent execution of checks. If there's a previous check onging, the check will be skipped. This rarely occur since checks are fast (way less than 100ms)

For reviewers

I left some commented console logs, was helpful while analysing, but i think is too much output. I left it there cause it's handy imo, but if you totally hate it, i would remove

To Test

  1. Make sure pending orders work as expected
    2.Make sure cancelling orders work as expected
  2. Ideally, try to reproduce a situation where market conditions change. I think this is hard to test. I guess one way is trying to move the market price (big trade in Uniswap v2) while you are creating a CowSwap order with a very tight slippage tolerance (0.00001%)

context

We need to make sure we detect the trades as soon as possible. Maybe we can research in hybrid approach using (time, bock, events, or a combination of the three). Related #1892

@anxolin anxolin changed the base branch from develop to release/1.5 November 19, 2021 11:34
@anxolin anxolin requested review from a team November 19, 2021 11:34
@anxolin anxolin added the RELEASE Included in the release that is being closed label Nov 19, 2021
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@elena-zh
Copy link

Hey @anxolin , great!
'Open' and 'Filled' statuses are updated faster even than in Order list in Explorer (30 sec there).
'Cancelled' status works as expected.

Orders' status also looks good to me when market conditions change. A nitpick there is that sometimes 'Price out of range' warning may appear for 1 sec on 'Open' order before changing to 'Filled' . See the video (start from 30 sec): https://watch.screencastify.com/v/LlNji6ah7w9oxAfcBWgh

The only issue I found with 'Expired' status: it is still updated in a minute or so
expired

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good code wise.
I'm not too keen on lot's of logs but we can leave them on and disable if it gets too annoying.

Base automatically changed from release/1.5 to master November 25, 2021 09:43
@anxolin
Copy link
Contributor Author

anxolin commented Nov 25, 2021

I'm not too keen on lot's of logs but we can leave them on and disable if it gets too annoying.

I think we log too much, probably we need a better strategy in terms of logs. I feel is more important to make the app be more fluid.

Thanks for the tests @elena-zh, yes, that can happen if the price moves, probably the price moves because of my own trade, so i agree is improvable but it just happens for a few seconds, so we can leave it as it is for now imo.

@anxolin anxolin changed the base branch from master to develop November 25, 2021 19:26
@anxolin anxolin merged commit de6e1fa into develop Nov 25, 2021
@elena-zh
Copy link

Hey @anxolin , thank you for explanation!
But still 'Expired' status is still updated in a minute or so. I reported a separate issue for this status #1928

The only issue I found with 'Expired' status: it is still updated in a minute or so
expired

@alfetopito alfetopito deleted the update-orders-faster branch November 26, 2021 16:08
@ramirotw ramirotw mentioned this pull request Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
RELEASE Included in the release that is being closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants