Fix 10517 - Prevent tokens without addresses from being added to token list by darkwing · Pull Request #10593 · MetaMask/metamask-extension

darkwing · 2021-03-04T22:32:16Z

Explanation:

ToDo:

Add migration
Throw error in addToken to prevent this from happening again
Explore ui/app/ducks/swaps/swaps.js to see if this is truly the place it's happening, and if selectedFromToken and selectedToToken are the true culprits.

github-actions · 2021-03-04T22:32:33Z

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 · 2021-03-05T17:56:07Z

ui/app/ducks/swaps/swaps.js

This is necessary because the next two lines would allow const selectedToToken = {} to add a token, which we definitely don't want.

This looks like a fine change to protect against errors. However, we should never be in a situation where there is no toTokenAddress. If so, requesting quotes should fail.

In the issue description, it was described how editing the code so that toTokenAddress is undefined results in the described bug, which occurs after confirming the swap. So it sounds like requesting quotes succeeded (otherwise there wouldn't be a quote to submit on the view quote screen).

So that leads me to believe that there is some other value that is being used for the token address in the request for quote. Or swapsTokens contains a token that has an undefined address.

const destinationTokenInfo = swapsTokens?.find(({ address }) => address === toTokenAddress) || selectedToToken;

It would be good if we could verify exactly how the request for quotes succeeds when const selectedToToken = {} and what the address is in that case.

We could remove the {} fallback from selectedFromToken and selectedToToken that existed from the initial swaps merge, and remove the toTokenAddress and fromTokenAddress checks in this PR.

If we should never get there without these properties, we should probably throw an error here, right?

If we should never get there without these properties, we should probably throw an error here, right?

Right, but what I want to know is how was it possible to request quotes without these properties? Requesting quotes when from or two are empty strings or undefined would return 0 quotes.

I think this can be addressed in another PR, as the change to actions.js handles the objectives of this PR.

ui/app/ducks/swaps/swaps.js

ui/app/store/actions.js

darkwing · 2021-03-08T15:04:19Z

@danjm Since you have the most knowledge about the swaps codebase, could you take a look at this?

app/scripts/migrations/053.js

danjm · 2021-03-10T10:56:53Z

To fully address #10593, should a change also be made on the 'Add Token' screen so that typing a single character does not result in an error state when there is a flawed token in the users data?

Gudahtt · 2021-03-10T18:05:02Z

when there is a flawed token in the users data?

Doesn't this PR prevent this circumstance? I don't quite understand what situation you're referring to.

danjm · 2021-03-10T18:09:03Z

Doesn't this PR prevent this circumstance?

Right, but suppose such a token does somehow get added. The add token screen shouldn't error during typing right? I mean, add token functionality should continue to work regardless of what the data is like for already added tokens.

ui/app/store/actions.js

Gudahtt · 2021-03-10T18:24:08Z

Right, but suppose such a token does somehow get added. The add token screen shouldn't error during typing right? I mean, add token functionality should continue to work regardless of what the data is like for already added tokens.

I don't think we should expect UI to be tolerant of invalid state, no. Handling that everywhere would add a huge amount of complexity to our UI, and might obscure what the real expectations of the component are.

darkwing · 2021-03-11T21:25:24Z

app/scripts/migrations/054.js

This if is required because neither PreferencesController.tokens or PreferencesController. hiddenTokens exists in the migration data to this point.

What do you mean by "neither PreferencesController.tokens or PreferencesController. hiddenTokens exists in the migration data to this point"? If they don't exist, is this forEach necessary? Will the code inside the if block ever run?

Perhaps you meant that it is possible for PreferencesController.tokens or PreferencesController.hiddenTokens to not exist at this point.

darkwing · 2021-03-11T21:25:43Z

app/scripts/migrations/054.js

This if is required because neither PreferencesController. accountHiddenTokens or PreferencesController. accountTokens exists in the migration data to this point.

darkwing · 2021-03-11T21:26:04Z

app/scripts/migrations/054.js

assetImages isn't in our migration data to this point.

darkwing · 2021-03-13T03:30:28Z

@danjm One thing I notice is:

Open the extension in full screen mode
Click "Swap" from the home screen -- "ETH" is the selected "from" token
Refresh the page
No "From" token is selected.

We should probably default to ETH, since that's what the home screen does. Maybe we do this in a separate PR?

brad-decker

LGTM, though would like @danjm to revisit his comment and subsequent changes before merge.

app/scripts/migrations/054.js

danjm

I left a couple comments on the migration that should be addressed

brad-decker

changes LGTM @danjm can you give this another pass, please 🙏🏻

brad-decker · 2021-03-16T21:02:49Z

@darkwing you'll need to move your migration test into the migrations folder alongside 054.js, and then apply your changes to test/unit/ui/app/ations.test.js to ui/app/store/actions.test.js. Let me know if I can help.

darkwing · 2021-03-17T00:58:49Z

Rebased per @brad-decker's directives. I also removed a test that I had added, as since ....

const addTokenStub = sinon
        .stub()
        .callsFake((_, __, ___, ____, cb) => cb(new Error('error')));

... makes addToken fail regardless of arguments, it doesn't make sense to test that not providing a token address and pretend it represents anything.

darkwing · 2021-03-17T01:17:45Z

@Gudahtt @danjm @brad-decker Since this is my first migration, I'd love a deep dive on it, especially test data approval.

brad-decker

I walked through and ran this test a few times, I only have one minor NIT. I think this is a fine migration and test/data set.

app/scripts/migrations/054.js

darkwing · 2021-03-17T16:49:33Z

Good idea Brad, updated! I also added Array.isArray checks for increased type checking security.

brad-decker

LGTM

…n list

…affected

darkwing · 2021-03-25T14:25:03Z

In the interest of keeping migrations as tight as possible, I've removed checks on hiddenTokens and accountHiddenTokens, as a review of the affected user's state logs show that those properties weren't affected.

Gudahtt

LGTM!

danjm

LGTM

darkwing requested a review from a team as a code owner March 4, 2021 22:32

darkwing requested a review from danjm March 4, 2021 22:32

darkwing marked this pull request as draft March 4, 2021 23:43

darkwing force-pushed the 10517-no-address branch 3 times, most recently from 255bb08 to 4a36fd0 Compare March 5, 2021 03:18

darkwing marked this pull request as ready for review March 5, 2021 17:11

darkwing commented Mar 5, 2021

View reviewed changes

ui/app/ducks/swaps/swaps.js Outdated Show resolved Hide resolved

darkwing commented Mar 5, 2021

View reviewed changes

ui/app/store/actions.js Outdated Show resolved Hide resolved

darkwing force-pushed the 10517-no-address branch from 7695da0 to 8e7acad Compare March 5, 2021 21:05

danjm reviewed Mar 10, 2021

View reviewed changes

app/scripts/migrations/053.js Outdated Show resolved Hide resolved

Gudahtt reviewed Mar 10, 2021

View reviewed changes

ui/app/store/actions.js Outdated Show resolved Hide resolved

darkwing force-pushed the 10517-no-address branch 8 times, most recently from 30ec483 to 6a0cf31 Compare March 11, 2021 19:11

darkwing commented Mar 11, 2021

View reviewed changes

app/scripts/migrations/054.js Outdated

Copy link

Contributor Author

darkwing Mar 11, 2021

Choose a reason for hiding this comment

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

assetImages isn't in our migration data to this point.

brad-decker mentioned this pull request Mar 15, 2021

refactor incoming tx controller #10639

Merged

brad-decker reviewed Mar 15, 2021

View reviewed changes

danjm reviewed Mar 16, 2021

View reviewed changes

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

danjm reviewed Mar 16, 2021

View reviewed changes

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

danjm suggested changes Mar 16, 2021

View reviewed changes

darkwing force-pushed the 10517-no-address branch from 054834f to 6084b75 Compare March 16, 2021 16:08

brad-decker previously approved these changes Mar 16, 2021

View reviewed changes

danjm previously approved these changes Mar 16, 2021

View reviewed changes

darkwing dismissed stale reviews from danjm and brad-decker via 608bb0e March 17, 2021 00:28

darkwing force-pushed the 10517-no-address branch from 608bb0e to 3804b91 Compare March 17, 2021 00:52

darkwing force-pushed the 10517-no-address branch from c6bf32e to 7ed2444 Compare March 17, 2021 01:16

brad-decker suggested changes Mar 17, 2021

View reviewed changes

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

brad-decker previously approved these changes Mar 17, 2021

View reviewed changes

darkwing dismissed brad-decker’s stale review via 18c4538 March 23, 2021 17:59

darkwing force-pushed the 10517-no-address branch 2 times, most recently from 18c4538 to 66ca5ea Compare March 23, 2021 18:01

Fix 10517 - Prevent tokens without addresses from being added to toke…

89c1e0d

…n list

darkwing force-pushed the 10517-no-address branch from 66ca5ea to 89c1e0d Compare March 23, 2021 18:19

Avoid touching hiddenTokens and accountHiddenTokens as they were not …

de915da

…affected

Gudahtt approved these changes Mar 25, 2021

View reviewed changes

danjm approved these changes Mar 29, 2021

View reviewed changes

darkwing merged commit 29cc31a into MetaMask:develop Mar 29, 2021

github-actions bot locked and limited conversation to collaborators Mar 29, 2021

Conversation

darkwing commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

darkwing commented Mar 8, 2021

Uh oh!

Uh oh!

danjm commented Mar 10, 2021

Uh oh!

Gudahtt commented Mar 10, 2021

Uh oh!

danjm commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Gudahtt commented Mar 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkwing commented Mar 13, 2021

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

brad-decker commented Mar 16, 2021

Uh oh!

darkwing commented Mar 17, 2021

Uh oh!

darkwing commented Mar 17, 2021

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darkwing commented Mar 17, 2021

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

darkwing commented Mar 25, 2021

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

darkwing commented Mar 4, 2021 •

edited

Loading

danjm commented Mar 10, 2021 •

edited

Loading