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

fix(limit-orders): don't override price from URL with initial value #4668

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

shoom3301
Copy link
Collaborator

Summary

Fixes #4666

In limit orders we have initial price which is a value derrived from /native_price API.
Recently I added SetupLimitOrderAmountsFromUrlUpdater which fills limit-orders state with amounts from URL.
Those two mechanisms were conflicting and it caused from price values.

After this fix:

  • if state was filled from URL
  • then don't updated it with an initial price

To Test

  1. Open Swap, set Sell 4000 WETH
  2. Remember the output amount
  3. Navigate to Limit orders
  4. ER: the form contains exactly the same sell/buy amounts as in Swap page
  5. ER: the limit price = buyAmount / selAmount

@shoom3301 shoom3301 requested review from a team July 4, 2024 09:23
@shoom3301 shoom3301 self-assigned this Jul 4, 2024
Copy link

vercel bot commented Jul 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos 🔄 Building (Inspect) Visit Preview Jul 16, 2024 4:21pm
cowfi 🔄 Building (Inspect) Visit Preview Jul 16, 2024 4:21pm
explorer-dev 🔄 Building (Inspect) Visit Preview Jul 16, 2024 4:21pm
swap-dev 🔄 Building (Inspect) Visit Preview Jul 16, 2024 4:21pm
widget-configurator 🔄 Building (Inspect) Visit Preview Jul 16, 2024 4:21pm

Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @shoom3301 , might this be possible to implement another logic for saving Sell amount only when navigating between forms?

Currently, I see different behaviors when:

  1. I open Limit orders page and specify a trade there:
    image
  2. When I navigate to Limit orders from Swap page, trade was prefilled there:
    image
  3. Navigate from TWAP orders to Limit, trade amounts prefilled:
    image

@alfetopito alfetopito changed the title fix(limit-orders): don't override price from IRL with initial value fix(limit-orders): don't override price from URL with initial value Jul 4, 2024
Copy link
Collaborator

@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.

Code looks good.

Has this been tested with recreate orders as well?
This change might affect that.

@shoom3301
Copy link
Collaborator Author

@anxolin about the comment from Elena: #4668 (review)

I consider it as an enhancement and think we should keep it as it is.
What is Elena's concern:

Before we used to have this behaviour:

  1. Open swap, select some trade
  2. For example: you got Sell 10 WETH for 326000 COW (price from /quote)
  3. Navigate to limit orders
  4. You will see: Sell 10 WETH for 285000 COW (price from /native_price)

In this PR:

  1. Open swap, select some trade
  2. For example: you got Sell 10 WETH for 326000 COW (price from /quote)
  3. Navigate to limit orders
  4. You will see: Sell 10 WETH for 326000 COW (exactly the same values as in Swap)

Why I think it's better.
When I saw some amount in Swap I usually have only one reason to go to Limit orders - I don't like the price from Swap and I want to edit it.
With this motivation, I would like to go to Limit orders with the exactly the same amounts and correct them a bit.
Having this, I think we should keep the same amounts while navigating from Swap/Twap to Limit orders.

@elena-zh
Copy link
Contributor

elena-zh commented Jul 4, 2024

Code looks good.

Has this been tested with recreate orders as well? This change might affect that.

@alfetopito , this does not affect recreate orders feature.
The only one issue is that when I cancel recreate request, the Limit orders form amounts are recalculated according to scenario 1 in my comment above (#4668 (review))

@anxolin
Copy link
Contributor

anxolin commented Jul 4, 2024

I personally like this behavior way more. It feels way more natural to me

Not sure if the friction of Elena is related to maybe the [Market price] button should give us the price of the SWAP (as market order swap) and not the market price (as the mid-point price). I don't know what is best for this button, but I'd be happy to merge the logic as suggested in this PR and maybe some other time in UX sessions we check if this button should behave one way or the other.

@elena-zh
Copy link
Contributor

elena-zh commented Jul 4, 2024

I personally like this behavior way more. It feels way more natural to me

Not sure if the friction of Elena is related to maybe the [Market price] button should give us the price of the SWAP (as market order swap) and not the market price (as the mid-point price). I don't know what is best for this button, but I'd be happy to merge the logic as suggested in this PR and maybe some other time in UX sessions we check if this button should behave one way or the other.

OK, convinced with the Swap.
But what about navigation from TWAP when an amount of 2 (n) parts is transferred into the Limit orders TO form?
image
It differs from the one we get on the Swap page.

@shoom3301
Copy link
Collaborator Author

@anxolin @elena-zh should we keep it as it is? Can I merge it?

@anxolin
Copy link
Contributor

anxolin commented Jul 16, 2024

OK, convinced with the Swap.
But what about navigation from TWAP when an amount of 2 (n) parts is transferred into the Limit orders TO form?

Sorry for this super slow response @elena-zh !
I think i still like this PR more than the status quo. I think is fine that if I go from TWAP to LIMIT i show the price of TWAP (same reasoning sasha made for SWAP and how I would go and place a limit price with the same price, applies here)

I don't think we need to make LIMIT order to have the same as SWAP, we need to make a nice transition from where we were. In this case we keep the buy amount constant. This is useful, cause in the case of

  • SWAP, you can easily adjust the SWAP price
  • TWAP you can place a partially executable order at the price of the TWAP (has less price impact)

Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

@anxolin , thank you for your reply!
Approving.
Could you please merge this PR into develop then?

@anxolin anxolin merged commit 1fee2da into develop Jul 16, 2024
4 of 10 checks passed
@anxolin anxolin deleted the fix/4666 branch July 16, 2024 16:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Price impact is not recalculated when switch between swap/limit forms
4 participants