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: custom slippage improvements for UI and cowswap swapper #5908

Merged
merged 4 commits into from
Dec 21, 2023

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Dec 20, 2023

Description

Some types of swappers (thorchain streaming swaps) dont support the ability to specify the slippage when getting a quote, and we need a way to propagate this to users. Other swappers (0x) only provide slippage protection for certain pairs.

This PR implements a UI change that shows users what quotes are affected.

  • Includes custom slippage capability for cowsap
  • 0x only provides slippage protection for specific pairs, so this PR addresses that
  • Fixes trade execution UI showing the default slippage instead of custom slippage

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #5779

Risk

  • Risk of broken trades
  • Risk of incorrect custom slippage for cow swap

Testing

  • Check trades are working as expected
  • Check cowswap custom slippage is correct

Engineering

Carefully check the implementation of custom slippage for cowswap.

Operations

Screenshots (if applicable)

Cowswap default slippage value - see matching payload
image

Cowswap custom slippage value - see matching payload
image

Completed cow trade with custom slippage (note bottom right of UI
image

Video showing slippage UI behaviour

slippage.mov

@woodenfurniture woodenfurniture requested a review from a team as a code owner December 20, 2023 02:21
@woodenfurniture
Copy link
Member Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@woodenfurniture woodenfurniture changed the title feat: handle quotes that dont support custom slippage feat: custom slippage improvements for UI and cowswap swapper Dec 20, 2023
if (!quote) return

const isError =
userSlippageDecimal !== undefined && quote.slippageTolerancePercentage !== userSlippageDecimal
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but can we use a consistent vernacular for both of these? i.e slippageDecimal or slipperPercentage (perhaps percentageDecimal).
Reading this is a bit confusing

Copy link
Contributor

@gomesalexandre gomesalexandre Dec 20, 2023

Choose a reason for hiding this comment

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

Also, can we use a different terminology than error here? While we do display it as an error for users, not being able to set custom slippage is not an actual runtime error, it's part of the flow for swappers that don't support it, so this adds to the confusion

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah agreed, I've pushed a commit to align things a bit better 8996988

Copy link
Member Author

Choose a reason for hiding this comment

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

also a88fa6b

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally. Conceptual stamp except for one question.

1inch

  • Default slippage
image
  • Custom slippage is honored at final quote time
image image image

CoW

  • Default slippage
image
  • Custom slippage is honored
image image image

According to this implementation:

https://github.com/cowprotocol/cowswap/blob/071ce71b22d25a7767547f39254a5845132622a4/libs/common-utils/src/misc.ts#L176-L182

100 indeed is correct as 1% slippage ✅
image

THOR non-streaming swaps

  • Default slippage

  • Custom slippage

image image

@woodenfurniture the default 1% doesn't seem to be reflected at quote time here, though I believe this is on purpose because of our implementation, as we use limit over slippage? Memo for reference: =:THOR.RUNE:thor125dwsa39yeylqc7pn59l079dur502nsleyrgup:194410385:ss:0

https://viewblock.io/thorchain/tx/c8ceabdf8aad8f680a79c27ee1625942d103c06a17144dc7d09776c3d0ad68bb

Streaming swaps

  • Default slippage bps
image image
  • Custom slippage
image image image

Custom slippage is unable to get set for streaming swaps, and auto bps are used as they should ✅

@woodenfurniture woodenfurniture force-pushed the feat-handle-slippage branch 2 times, most recently from 0a250ea to 8996988 Compare December 20, 2023 23:36
@woodenfurniture
Copy link
Member Author

@woodenfurniture the default 1% doesn't seem to be reflected at quote time here, though I believe this is on purpose because of our implementation, as we use limit over slippage? Memo for reference: =:THOR.RUNE:thor125dwsa39yeylqc7pn59l079dur502nsleyrgup:194410385:ss:0

correct, we're manually implementing slippage through the limit mechanism here:
src/lib/swapper/swappers/ThorchainSwapper/utils/addSlippageToMemo.ts
image

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Akschual stamp after q. has been answered.

@gomesalexandre gomesalexandre enabled auto-merge (squash) December 21, 2023 08:27
@woodenfurniture woodenfurniture merged commit 0f05cf8 into develop Dec 21, 2023
2 checks passed
@woodenfurniture woodenfurniture deleted the feat-handle-slippage branch December 21, 2023 21:52
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.

Handle quotes that dont support custom slippage
2 participants