Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

Fix calculation for Trade.minimumAmountOut #44

Closed

Conversation

Jdecristi
Copy link

Fixes: #43

Description

The function had a math mistake, but the tests did not catch it because the tests were only checking for small slippage rates. When the slippage rate is 5% or less, the tests did not fail because the result was almost correct when rounded to the nearest whole number. However, when the slippage rate increased, the problem became more obvious.

Additionally, I made a change to the function to prevent a slippage rate of over 100%. It's impossible to lose more than 100% of your money, so setting a slippage rate of over 100% should generate an error.

I had to verify this change, so I modified the tests to reflect these updates.

Screen capture

Here is a manual test on the Uniswap Interface

Before After
Screenshot Screenshot

Test plan

Reproducing the error

  1. Go to app.uniswap.org
  2. Add two tokens to swap (Easier to test with two stablecoins like USDC & DAI)
  3. Add Custom Slippage (Easier to test with a round number like 50%)
  4. Open SwapDetailsDropdown see the incorrect number in the "Minimum output" row

Automated testing

  • Run yarn test in your local env to make sure all test pass

@tinaszheng
Copy link
Contributor

hey @Jdecristi thanks for your contribution!

the current behavior is correct but it is confusing, so apologies for that. we've chosen a spec where slippage is symmetric wrt exact in vs exact out, so if 100% slippage for exact out = double your input amount, then 100% slippage for exact in = half your output amount.

@tinaszheng tinaszheng closed this Nov 2, 2023
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.

Incorrect Calculation for Trade.minimumAmountOut
2 participants