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

Increase default slippage from 2% to 3%, show Advanced Options by default #10842

Merged
merged 5 commits into from
Apr 8, 2021

Conversation

dan437
Copy link
Contributor

@dan437 dan437 commented Apr 7, 2021

Explanation:

  • Increase default slippage from 2% to 3%
  • Increase slippage from 1% to 2%
  • Always show Advanced Options

Manual testing steps:

  • Click on the Swap button in the MetaMask extension
  • See the Swap page with Advanced Options visible and 3% slippage tolerance selected by default

Screenshots:

Swap page: Advanced Options visible and 3% slippage tolerance selected by default
image

Verifying that the 3% slippage is send to the server:
image

Pre-popullating previously set custom value when going back to the Swap page:
image

All unit tests are green:
image

@dan437 dan437 requested a review from a team as a code owner April 7, 2021 17:03
@dan437 dan437 requested a review from Gudahtt April 7, 2021 17:03
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@danjm danjm self-requested a review April 7, 2021 17:32
@danjm danjm self-assigned this Apr 7, 2021
@danjm
Copy link
Contributor

danjm commented Apr 7, 2021

This change looks good, but actually reveals a pre-existing bug that I think should be addressed within this PR.

To see this bug on this branch:

  • set the slippage to 2
  • click "Review Swap"
  • once on the "View Quote" screen click "Back"
  • the selection will be defaulted back to 3, even though the fetchParams.slippage still equals 2

I've attached a file that shows a change that resolves that problem:
slippage-patch.txt

However, there will still be a similar problem if a custom amount is entered. That should be resolved too, I think.

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

One small issue to address, otherwise this code looks good

return 1;
} else if (currentSlippage === 2) {
return 0;
} else if (currentSlippage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be best not to call setCustomValue in the callback to useState, as this is incidental to the purpose of this function. I think the intent of the code would be clearer if we added a callback to the useState call that sets up the customValue which would set the initial custom value to currentSlippage if currentSlippage is truthy but not equal to 2 or 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

LGTM!

@danjm danjm merged commit a814f8e into MetaMask:develop Apr 8, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2021
@dan437 dan437 deleted the swaps-slippage branch July 24, 2023 11:28
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.

2 participants