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

Fix #9872 - Show price difference warning on swaps price quote #9899

Merged
merged 12 commits into from
Dec 2, 2020

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Nov 17, 2020

Fixes: #9872

Explanation: Implements the price difference warning for swaps quotes (https://www.figma.com/file/fDtda1cs3MmPXw1MgKswZc?embed_host=notion&kind=&node-id=1533%3A1064&viewer=1)

Market

WarningYellow

WarningRed

@darkwing darkwing requested a review from danjm November 17, 2020 21:46
@darkwing darkwing requested a review from a team as a code owner November 17, 2020 21:46
@darkwing darkwing changed the title Show price difference warning on swaps price quote Fix #9872 - Show price difference warning on swaps price quote Nov 17, 2020
@github-actions
Copy link
Contributor

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
Copy link
Contributor

danjm commented Nov 18, 2020

I think that if there is a balance warning being shown, we should not show the price difference warning. Because the price difference warning will not matter if the user can't actually submit the swap.

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.

Code looks good overall. Two requested changes. Also, I am going to check with Jake about the copy.

@darkwing darkwing force-pushed the 9872-price-difference branch from 6c67e78 to 5671855 Compare November 20, 2020 17:09
@danjm
Copy link
Contributor

danjm commented Nov 20, 2020

Need to incorporate the copy changes discussed here https://consensys.slack.com/archives/C01CZRZ9H0T/p1605800606242600

@darkwing darkwing force-pushed the 9872-price-difference branch from 5671855 to 3e0ab86 Compare November 20, 2020 17:14
@darkwing
Copy link
Contributor Author

Per @jakehaugen, we need to update the following:

Warning message:
Swapping 1.2 ETH (~$500) for 0.08 WBTC (~$450).

Tooltip:
The difference in market prices can be affected by fees taken by intermediaries, size of market, size of trade, or market inefficiencies.

@darkwing
Copy link
Contributor Author

@jakehaugen @danjm I believe I've found a blocker to this: we are not given a value for the destination token in the quotes we get returned from the API (as far as I can tell). For example, if I want to swap 5 $ETH for {x} $LINK, we do get told how many $LINK we'd get in return, but we don't get the value of 1 $LINK, nor do we get a total dollar value.

We do know the conversionRate of the origin token; can we do some wild math to figure out the total destination value? Is that safe? Please let me know if I'm missing anything obvious with dcd8eea to get the dollar amounts. Ideas very much welcome!

@darkwing darkwing force-pushed the 9872-price-difference branch from dcd8eea to c9d3065 Compare December 1, 2020 15:48
@danjm
Copy link
Contributor

danjm commented Dec 1, 2020

@darkwing bumping up a previous comment I made:

I think that if there is a balance warning being shown, we should not show the price difference warning. Because the price difference warning will not matter if the user can't actually submit the swap.

although, looking at the code, I am not sure why the price difference warning is showing at the same time as the insufficient eth warning...

@darkwing
Copy link
Contributor Author

darkwing commented Dec 1, 2020

although, looking at the code, I am not sure why the price difference warning is showing at the same time as the insufficient eth warning...

If you're referring to the screenshots I posted, I set shouldShowPriceDifferenceWarning explicitly to true to illustrate how this new block could be used. Is there a reliable swap pairing that would often trigger this warning?

@danjm
Copy link
Contributor

danjm commented Dec 1, 2020

This is how it will look when no insufficient warning is present on full screen:

Screenshot from 2020-12-01 14-11-00

Edit: to recreate, you can disable the display of the insufficient warning but leave its containing div where it is, and also disable the logic that hides the price difference warning when there is insufficient balance

This is because of the space that the insufficient eth warning container takes up. It would be best for the price difference warning to be centered.

let priceDifferenceMessage = ''
if (priceSlippage.calculationError) {
// An error was returned by the API; show it to the user
priceDifferenceMessage = priceSlippage.calculationError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be a bad assumption, but if there is a calculation error, we show it to the user directly. Thoughts?

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 in this case we should just show the "unknown impact" message.

@darkwing darkwing force-pushed the 9872-price-difference branch 3 times, most recently from 8c28cf9 to 8b606b8 Compare December 2, 2020 02:53
@darkwing darkwing force-pushed the 9872-price-difference branch from 8b606b8 to 7bf44da Compare December 2, 2020 03:35
@darkwing darkwing force-pushed the 9872-price-difference branch from 97ddfa0 to e56281d Compare December 2, 2020 18:49
@danjm
Copy link
Contributor

danjm commented Dec 2, 2020

One other change that is needed here: the places where we set a --thin modifier class based on showInsufficientWarning need to be updated to show that class based on showInsufficientWarning || shouldShowPriceDifferenceWarning

@darkwing darkwing force-pushed the 9872-price-difference branch from f4532ef to 0918248 Compare December 2, 2020 21:06
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

@darkwing darkwing merged commit 673371d into MetaMask:develop Dec 2, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2020
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.

Add price difference warnings to metaswap view quotes screen
3 participants