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

Correct use of useTokenTracker in viewQuote to ensure token data is not disrupted by faulty token in user account #10456

Merged
merged 3 commits into from
Feb 17, 2021

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Feb 16, 2021

This PR fixes the following bug which exists on develop and prod:

  1. Get exactly 5 UNI in your account
  2. Use MetaMask's "Add token" feature to add a token that always returns an error when the eth-token-tracker tries to call either the symbol, balance or balanceOf method on the token contract. For example, add 0x7a250d5630b4cf539739df2c5dacb4c659f2488d and use a symbol name like "Test"
  3. Then send most of your ETH out of your account. Leave $1-$10 USD of eth in your account
  4. Then go to the home screen, select the UNI token, clicked the "Swap" button once on that token's asset screen
  5. Once on the swap build quote screen, click the "Max" button, so that you are swapping away all 5 of your UNI.
  6. Select ETH and the token to swap to, and request quotes
    7. You will arrive at the view quote screen and see a message that says "You need 5 more UNI to complete this swap", even though you have exactly 5 UNI in your account. Instead you should see a "You need ## more ETH to complete this swap"

Cause of this bug:

The useTokenTracker hook is how we get all balance information for users tokens. The useTokenTracker hook gets it balances via the eth-token-tracker library. That library receives data on all of a users tokens, and attempts to fetch balance data for all of them within a Promise.all call. If an error is thrown when attempting to fetch the balance data for any single token, it ultimately causes the useTokenTracker to return an empty array of token balances, even if balances for token other than the erroneous one would have succeeded.

If the user arrived at the view quote screen, and the above happened AND they had insufficient ETH for the token swap, then they see an error that says they have insufficient tokens. The useTokenTracker has no data on the token, so the view-quote screen logic evaluates the users balance of the token to be 0. Meanwhile, because the the user has insufficient ETH, the swaps.state.balanceError property is set to true. This causes the logic that sets insufficientTokens to evaluate to true, and tokenBalanceNeeded to evaluate to the amount of tokens being sent.

Solution:

This PR solves the issue in two ways, each of which would work on their own, but together they handle possible erroneous cases that could arise in the future as well.

First, the second parameter passed to useTokenTracker in view-quote is set to true. This is the includeFailedTokens parameter. It ensures that balances for all tokens that can be successfully retrieved are returned, even if attempting to get the balance of one or more tokens encounters and error. (This functionality was added in #9896). This alone prevents the bug as described above as it ensures that the token balance reflects the user's actual token balance and is not incorrectly defaulted to 0. As a result, the user will now correctly see an eth balance error (instead of a token balance error).

Second, if for whatever reason the useTokenTracker fails to get the balance for the currently selected token, we should not assume the balance is 0 and then show the user an error message that suggests that. Instead we should tell the user that we were unable to get information on their token balance, while also disabling submission of swaps in this case. A tokenBalanceUnavailable variable and associated error message has been added to handle this case.

@danjm danjm requested a review from a team as a code owner February 16, 2021 10:34
@danjm danjm requested a review from brad-decker February 16, 2021 10:34
@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.

@metamaskbot
Copy link
Collaborator

Builds ready [acb4756]
Page Load Metrics (642 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint519663136
domContentLoaded34797764014771
load34897964214771
domInteractive34697764014771

brad-decker
brad-decker previously approved these changes Feb 16, 2021
@danjm
Copy link
Contributor Author

danjm commented Feb 17, 2021

@Gudahtt I added your suggestion copy change

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [21fd018]
Page Load Metrics (582 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint47766073
domContentLoaded3556895817938
load3576905827938
domInteractive3556895807938

@danjm danjm merged commit b056867 into develop Feb 17, 2021
@danjm danjm deleted the use-failed-tokens-view-quote branch February 17, 2021 14:49
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2021
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.

4 participants