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

feat: add tooltip and change message #4819

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Aug 22, 2024

Summary

Changes the text for the smart slippage and adds a tooltip

image

image

Implementation details

This PR will use BFF for estimating the slippage tolerance. Specifically uses this: cowprotocol/bff#84

This implementation has been deployed to BARN, so it should have #84 applied:
https://bff.barn.cow.fi/1/markets/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2-0x2260fac5e5542a773aa44fbcfedf7c193bc2c599/slippageTolerance

Test

Select different markets, and check how it change the default slippage suggested by the app.

Make sure the trades go trough even for illiquid markets.

It might be helpful to check coingecko to understand why sometimes is the number higher, like in AAVE token (1.84%)

image image image image image

Copy link

vercel bot commented Aug 22, 2024

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

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Aug 23, 2024 1:38pm
cowfi ✅ Ready (Inspect) Visit Preview Aug 23, 2024 1:38pm
explorer-dev ✅ Ready (Inspect) Visit Preview Aug 23, 2024 1:38pm
swap-dev ✅ Ready (Inspect) Visit Preview Aug 23, 2024 1:38pm
widget-configurator ✅ Ready (Inspect) Visit Preview Aug 23, 2024 1:38pm

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.

I know it's not part of this PR, but since we are now actually setting the slippage dynamically, we should at least inform about that IMO.

image

I see the value when I set it myself
image

Another point is that, once I manually edited it, I don't know there's a proposed value.
Would be great to show the option to change it if different from selection.

Also in the confirmation screen would be a good place to point out this value was dynamically suggested
image

Anyway, looks good as is, but would like to add those changes on top

@elena-zh
Copy link
Contributor

Hey @anxolin , in additions to @alfetopito comments, I'd like to add one more tooltip where it would be nice to change wording:
image
At least, remove 'your' from the text

@elena-zh
Copy link
Contributor

elena-zh commented Aug 23, 2024

Hey @anxolin , I noticed that a dynamic slippage is not applied to ETH orders. See: weth-xai token pair in Arb has 1.38% slippage while eth-xai has 0.5. Is this an expected behavior for now?
0 5
1 38

Bff returns same value for both requests, but on the FE is it not overwritten
image
image

@anxolin
Copy link
Contributor Author

anxolin commented Aug 23, 2024

Yes @alfetopito I agree with your points. I also thought this was not nicely communicated. I guess we will need to follow up and add it.

I think we should always show the suggested slippage, even when it is overridden, so the user knows how their slippage compares to the suggested one. Also could provide a quick way to select the suggested one. All these things need to be a bit reviewed, maybe with @fairlighteth supervision

@elena-zh for the tooltip, no problem.
For ETH Flow, I think we shouldn't override it anymore if something we can improve the BFF slippage calculation so orders selling ETH would add some extra buffer. So we will need to:

  • FE: Stop overriding this when we have a slippage suggestion
  • BFF: Potentially, take into account ETH Flow needs more time to settle, so this is taken into account in the estimation

@anxolin
Copy link
Contributor Author

anxolin commented Aug 23, 2024

This is a follow up I don't think I will manage to get in time, but I just kick started it #4826

@alfetopito alfetopito merged commit 75a16ac into develop Aug 26, 2024
11 of 12 checks passed
@alfetopito alfetopito deleted the add-slippage-tooltip branch August 26, 2024 14:00
@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2024
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.

3 participants