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.

Sign up for GitHub

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

Jump to bottom

[Token Detection V2] 6 of 7 - Introduce Detected Tokens #808

Merged
merged 52 commits into from
Jun 7, 2022
Merged

[Token Detection V2] 6 of 7 - Introduce Detected Tokens #808

merged 52 commits into from
Jun 7, 2022

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Apr 27, 2022 •
edited
Loading

Description

This PR is intended to introduce the concept of detectedTokens and are separate from the existing tokens, which we display in the wallet today.

What Changed

TokenBalancesController (non-breaking)

  • Expose detectedTokens during onTokensStateChange to support showing balances for detected tokens.

TokenDetectionController (breaking)

  • Change addTokens from constructor to addDetectedTokens since we're no longer automatically adding detected tokens directly to wallet. Since the action is to add detected tokens, the updated terminology makes more sense.
  • Introduce onTokenListStateChange to constructor, which will be triggered when token list updates from TokenListController. This was needed because there was a scenario where detection was triggering against an empty token list after switching networks (Since fetching token list may not be finished before detection happens). By introducing this listener, we're making it so that detection can be triggered by the token list update action.
  • Introduce getNetworkState in constructor to gets initial state of the network controller.
  • Introduce getPreferencesState in constructor to get initial state of the preferences controller.
  • Deprecated onTokensStateChange in constructor. Do not need this anymore since tokens are managed internally by TokensController.

TokenRatesController (non-breaking)

  • Expose detectedTokens during onTokensStateChange to support showing fiat balances for detected tokens.

TokensController (non-breaking)

  • Introduce concept of detectedTokens
  • Add addDetectedTokens method, which is used by TokenDetectionController when tokens are detected.
  • Update addTokens methods to account for detected tokens.
  • Add fetchTokenMetadata method to fetch aggregator info when adding a single token.
  • [BREAKING] Deprecated removeAndIgnoreToken method. In the case of ignoring a single token, use ignoreTokens and pass in an array with a single token address as the input.

PreferencesController (breaking)

  • Change useStaticTokenList to useTokenDetection and set to true by default. This means that token detection will be auto detected on TD supported networks by default.

utils (breaking)

  • Update method convertPriceToDecimal to convertHexToDecimal for better naming.

6 of 7 PRs for #763

@Cal-L Cal-L requested a review from a team as a code owner April 27, 2022 22:58
@Cal-L Cal-L changed the title [Token Detection V2] Introduce Detected Tokens [Token Detection V2] 6 of 7 - Introduce Detected Tokens Apr 27, 2022
@Cal-L Cal-L mentioned this pull request Apr 27, 2022
Closed
2 tasks
@Cal-L Cal-L mentioned this pull request May 18, 2022
Closed
@Cal-L Cal-L marked this pull request as draft May 20, 2022 17:16
@Cal-L Cal-L marked this pull request as ready for review May 24, 2022 18:46
mcmire
mcmire reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

A few things, but this all looks pretty reasonable to me.

src/assets/TokenDetectionController.ts Outdated Show resolved Hide resolved
src/assets/TokensController.ts Outdated Show resolved Hide resolved
src/assets/TokensController.ts Outdated Show resolved Hide resolved
src/assets/TokensController.ts Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented Jun 3, 2022

Jest: "global" coverage threshold for branches (90%) not met: 89.94%

Looks like the unit test job is failing because coverage is now slightly lower. Interesting. Since this is mostly new, tested code, I would not expect this. Must have missed something with the tests.

@Gudahtt
Copy link
Member

Gudahtt commented Jun 3, 2022

Ugh. I just tried running locally and it exceeded the coverage threshold. It looks like errors encountered on CI (related to network failures) are causing it to take different paths which is lowering the coverage, even though the actual tests still pass.

@Gudahtt
Copy link
Member

Gudahtt commented Jun 6, 2022 •
edited
Loading

Thanks @Cal-L, coverage looks a lot better now. There was one comment that I think should be addressed, but I only had minor (non-blocking) suggestions apart from that.

Gudahtt
Gudahtt previously approved these changes Jun 6, 2022
View reviewed changes
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, great work!

@Cal-L
Copy link
Contributor Author

Cal-L commented Jun 7, 2022 •
edited
Loading

Continued convo about occurrence criteria - https://consensys.slack.com/archives/G1L7H42BT/p1654560442953019?thread_ts=1650897225.469639&cid=G1L7H42BT

@Cal-L Cal-L merged commit 4c1df4f into main Jun 7, 2022
@Cal-L Cal-L deleted the token-detection/introduce-detected-tokens branch June 7, 2022 00:40
@adonesky1 adonesky1 mentioned this pull request Jun 7, 2022
Merged
@adonesky1 adonesky1 mentioned this pull request Jun 21, 2022
Merged
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@Cal-L @MajorLift
[Token Detection V2] 6 of 7 - Introduce Detected Tokens (#808)
07b1e2b 
* Introduce detected tokens and update other relevant controllers.

* Update preferences controller with useTokenDetection

* Set token detection to be true by default

* Fix linting issues

* Fix ComposableController test

* Update ComposableController test

* Use named export with AbortController

* Remove typecasting as any on error

* Reintroduce override keyword. Remove typecast any on error.

* Clean up token list messenger in token detection controller.

* Refine token detection tests

* Remove unnecessary abort controller from TokenDetectionController

* Change importTokens back to addTokens. Rename hex to decimal util function.

* Remove redundant comment

* Do not mutate token states directly on TokensController. Update fetchTokenMetadata return type with null.

* Remove check for hexadecimal chainId on TokensController

* Rename isTokenDetectionEnabledForNetwork to isTokenDetectionSupportedForNetwork

* Throw error for fetchTokenMetadata when fetching from non supported network

* Update imports on TokenListController

* Make fetchTokenMetadata on TokensController private. Only return null on unsupported network. Throw any other errors. Fix TokensController tests.

* Update TokenDetectionController import + clean up tests.

* Add TokensController test for switching networks while adding token

* Clean up token service tests

* Fix linting

* Return undefined instead of throwing for token metadata when no response. Clean up TokensController tests.

* Fix TokensController test

* Make token detection restart method private

* Remove unused networkType from TokenDetectionController

* Populate tokens with image property, which uses proxied url

* Remove removeAndIgnore method from TokensController. Update TokensController related tests.

* Update fetchTokenMetadata @returns doc

* Update TokenListMessenger type

* Test onTokensStateChange after introducing detected tokens for TokenRatesController

* Test onTokensStateChange for detected tokens on TokenBalancesController

* Clean up addToken doc

* Update TokenDetectionController to respect disable property from config

* Fix TokensController tes

* Deprecate onTokensStateChange from TokenDetectionController

* Fix formatting proxy token images. Re-introduce ability to provide image to addToken in TokensController

* Fix tests

* Update formatIconUrlWithProxy tokenAddress input to state mixed or lowercase

* Increase test coverage for AssetsContractController

* Increase branch coverage for AssetsContractController

* Increase test coverage for TokensController and TokenDetectionController

* Improve token detection test to check for getBalancesInSingle call

* Update comments on TokenDetectionController

* Update TokenDetectionController tests to check for getBalancesInSingleCall
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@Cal-L @MajorLift
[Token Detection V2] 6 of 7 - Introduce Detected Tokens (#808)
99ec7bd 
* Introduce detected tokens and update other relevant controllers.

* Update preferences controller with useTokenDetection

* Set token detection to be true by default

* Fix linting issues

* Fix ComposableController test

* Update ComposableController test

* Use named export with AbortController

* Remove typecasting as any on error

* Reintroduce override keyword. Remove typecast any on error.

* Clean up token list messenger in token detection controller.

* Refine token detection tests

* Remove unnecessary abort controller from TokenDetectionController

* Change importTokens back to addTokens. Rename hex to decimal util function.

* Remove redundant comment

* Do not mutate token states directly on TokensController. Update fetchTokenMetadata return type with null.

* Remove check for hexadecimal chainId on TokensController

* Rename isTokenDetectionEnabledForNetwork to isTokenDetectionSupportedForNetwork

* Throw error for fetchTokenMetadata when fetching from non supported network

* Update imports on TokenListController

* Make fetchTokenMetadata on TokensController private. Only return null on unsupported network. Throw any other errors. Fix TokensController tests.

* Update TokenDetectionController import + clean up tests.

* Add TokensController test for switching networks while adding token

* Clean up token service tests

* Fix linting

* Return undefined instead of throwing for token metadata when no response. Clean up TokensController tests.

* Fix TokensController test

* Make token detection restart method private

* Remove unused networkType from TokenDetectionController

* Populate tokens with image property, which uses proxied url

* Remove removeAndIgnore method from TokensController. Update TokensController related tests.

* Update fetchTokenMetadata @returns doc

* Update TokenListMessenger type

* Test onTokensStateChange after introducing detected tokens for TokenRatesController

* Test onTokensStateChange for detected tokens on TokenBalancesController

* Clean up addToken doc

* Update TokenDetectionController to respect disable property from config

* Fix TokensController tes

* Deprecate onTokensStateChange from TokenDetectionController

* Fix formatting proxy token images. Re-introduce ability to provide image to addToken in TokensController

* Fix tests

* Update formatIconUrlWithProxy tokenAddress input to state mixed or lowercase

* Increase test coverage for AssetsContractController

* Increase branch coverage for AssetsContractController

* Increase test coverage for TokensController and TokenDetectionController

* Improve token detection test to check for getBalancesInSingle call

* Update comments on TokenDetectionController

* Update TokenDetectionController tests to check for getBalancesInSingleCall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire mcmire left review comments

@NiranjanaBinoy NiranjanaBinoy NiranjanaBinoy left review comments

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Cal-L @Gudahtt @mcmire @NiranjanaBinoy