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

Split AssetsController into TokensController and CollectiblesController #2881

Merged
merged 4 commits into from
Jul 26, 2021

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Jul 9, 2021

Description
Required changes to land the split AssetsController work (currently in PR) in the controllers repo.

Issue
MetaMask/core#516

Comment on lines 190 to 194
},
...tokens
}
];
if (tokens && Array.isArray(tokens)) {
assets = [...assets, ...tokens];
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a case where tokens can be other than array, null, undefined?
If not this would not be necessary and we can just change ...tokens to ...(tokens || [])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this wasn't meant to be included but your suggestion is safer than how it is now

@adonesky1 adonesky1 force-pushed the split-assets-controller branch 2 times, most recently from 20bb5ba to 4698667 Compare July 9, 2021 17:23
@adonesky1 adonesky1 force-pushed the split-assets-controller branch 2 times, most recently from 45bbf86 to 1113350 Compare July 23, 2021 15:24
@adonesky1 adonesky1 marked this pull request as ready for review July 23, 2021 15:56
@adonesky1 adonesky1 requested a review from a team as a code owner July 23, 2021 15:56
@adonesky1 adonesky1 requested a review from wachunei July 23, 2021 15:56
Copy link
Member

@wachunei wachunei left a comment

Choose a reason for hiding this comment

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

LGTM

@wachunei wachunei added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Jul 23, 2021
@adonesky1 adonesky1 force-pushed the split-assets-controller branch from acc05c0 to 44f0fc9 Compare July 23, 2021 17:47
@adonesky1 adonesky1 force-pushed the split-assets-controller branch from 44f0fc9 to eb9ba92 Compare July 23, 2021 18:09
@adonesky1 adonesky1 changed the title [WIP] Split AssetsController into TokensController and CollectiblesController Split AssetsController into TokensController and CollectiblesController Jul 23, 2021
@adonesky1 adonesky1 force-pushed the split-assets-controller branch from eb9ba92 to 15c8055 Compare July 23, 2021 20:34
@adonesky1 adonesky1 requested review from andrepimenta and Cal-L July 23, 2021 22:16
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@adonesky1 adonesky1 merged commit 89e2d41 into develop Jul 26, 2021
@adonesky1 adonesky1 deleted the split-assets-controller branch July 26, 2021 19:57
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
…er (#2881)

* Split AssetsController into TokensController and CollectiblesController

* add back split in AssetDetectionController instantiation

* add config to tokensController

Co-authored-by: Ibrahim Taveras <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants