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

Reduce the default slippage from 3% to 2% #14863

Merged
merged 12 commits into from
Jun 7, 2022

Conversation

dan437
Copy link
Contributor

@dan437 dan437 commented Jun 6, 2022

Explanation

Reduce the default slippage from 3% to 2%.

Screenshots

Before:
image

After:
image

@dan437 dan437 requested a review from a team as a code owner June 6, 2022 12:06
@dan437 dan437 requested a review from mcmire June 6, 2022 12:06
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2022

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.

darkwing
darkwing previously approved these changes Jun 6, 2022
@@ -206,3 +206,5 @@ export const TOKEN_BUCKET_PRIORITY = {
OWNED: 'owned',
TOP: 'top',
};

export const DEFAULT_SLIPPAGE = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!

@@ -40,16 +41,16 @@ export default function SlippageButtons({
const [enteringCustomValue, setEnteringCustomValue] = useState(false);
const [activeButtonIndex, setActiveButtonIndex] = useState(() => {
if (currentSlippage === 3) {
return 1;
return 1; // 3% slippage.
} else if (currentSlippage === 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can use the constant in this?

darkwing
darkwing previously approved these changes Jun 7, 2022
}}
>
2%
{`${SLIPPAGE.DEFAULT}%`}
Copy link
Contributor

@adonesky1 adonesky1 Jun 7, 2022

Choose a reason for hiding this comment

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

Should we localize this? It seems like in persian and turkish the percentage sign comes first.

Comment on lines 34 to 35
currentSlippage !== SLIPPAGE.DEFAULT &&
currentSlippage !== SLIPPAGE.HIGH
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
currentSlippage !== SLIPPAGE.DEFAULT &&
currentSlippage !== SLIPPAGE.HIGH
!Object.values(SLIPPAGE).includes(currentSlippage)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh, I like this!

adonesky1
adonesky1 previously approved these changes Jun 7, 2022
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

small nit otherwise LGTM!

@dan437 dan437 merged commit 9705531 into MetaMask:develop Jun 7, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2022
@dan437 dan437 deleted the swaps-default-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.

3 participants