[token-detection-controller] Consolidate with `DetectTokensController` from extension #3775

MajorLift · 2024-01-11T23:59:13Z

Explanation

Merges the core TokenDetectionController with DetectTokensController from the extension and patches for this controller from mobile.

Aims to replace the extension and mobile versions of this controller and implement a unified interface that consolidates the functionality of all three iterations.
Generally assumes DetectTokensController to be more up-to-date, and prioritizes preserving extension and mobile behavior to minimize disruption.

Significant API changes: see Changelog for details.

References

Changelog

`@metamask/assets-controllers`

Added

BREAKING: Adds @metamask/accounts-controller ^8.0.0 and @metamask/keyring-controller ^12.0.0 as dependencies and peer dependencies. (#3775).
BREAKING: TokenDetectionController newly subscribes to the PreferencesController:stateChange, AccountsController:selectedAccountChange, KeyringController:lock, KeyringController:unlock events, and allows the PreferencesController:getState messenger action. (#3775)

Changed

BREAKING: TokenDetectionController is merged with DetectTokensController from the metamask-extension repo. (#3775)
- BREAKING: TokenDetectionController now resets its polling interval to the default value of 3 minutes when token detection is triggered by external controller events KeyringController:unlock, TokenListController:stateChange, PreferencesController:stateChange, AccountsController:selectedAccountChange.
- BREAKING: TokenDetectionController now refetches tokens on NetworkController:networkDidChange if the networkClientId is changed instead of chainId.
- BREAKING: TokenDetectionController cannot initiate polling or token detection if KeyringController state is locked.
- BREAKING: The detectTokens method now excludes tokens that are already included in the TokensController's detectedTokens list from the batch of incoming tokens it sends to the TokensController addDetectedTokens method.
- BREAKING: The constructor for TokenDetectionController expects a new required proprerty trackMetaMetricsEvent, which defines the callback that is called in the detectTokens method.
- BREAKING: In Mainnet, even if the PreferenceController's useTokenDetection option is set to false, automatic token detection is performed on the legacy token list (token data from the contract-metadata repo).

Removed

BREAKING: TokenDetectionController constructor no longer accepts options onPreferencesStateChange, getPreferencesState. (#3775)
BREAKING: TokenDetectionController no longer allows the NetworkController:stateChange event. The NetworkController:networkDidChange event can be used instead. (#3775)

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

packages/assets-controllers/CHANGELOG.md

MajorLift · 2024-01-22T15:10:12Z

@SocketSecurity ignore npm/@metamask/object-multiplex@1.3.0 (Internal tooling)

mcmire

Nice job sorting all of these differences out. I did a first pass and made some notes. I noticed that some tests are missing for the added behavior. I know we have another ticket to ensure that there are comprehensive tests for this controller, but do we want to add tests for the changes added here? Then that other tickets would cover behavior that is out of scope of this consolidation work.

packages/assets-controllers/src/TokenDetectionController.ts

MajorLift · 2024-01-26T16:31:25Z

Hmm CI changelog:validate is not running on the current state of the changelog file:
https://github.com/MetaMask/core/actions/runs/7670677089/job/20907426326?pr=3775

mcmire · 2024-01-26T17:54:05Z

@MajorLift That's strange. yarn changelog:validate works for me locally. I also tried running it on Node 20 instead of 16 and it still works. I wonder if this is due to bumping create-release-branch a few days ago. Although a new version of auto-changelog was released a month ago. There must be some difference between the CI and local environments. I'll have to investigate this.

mcmire · 2024-01-26T19:52:11Z

@MajorLift Is this commit present in this branch? f4d4047

MajorLift · 2024-01-26T21:38:01Z

@MajorLift Is this commit present in this branch? f4d4047

It looks like changelog:validate was correctly alerting us that my changelog wasn't conforming to prettier rules. Adding newlines after the unreleased category headings fixed the issue.

The current way changelog:validate shows diffs is a bit confusing. Would it make sense to reverse the diff source and target so that suggested additions show up as positive diffs and removals as negative diffs?

mcmire · 2024-01-29T16:13:34Z

The current way changelog:validate shows diffs is a bit confusing. Would it make sense to reverse the diff source and target so that suggested additions show up as positive diffs and removals as negative diffs?

Absolutely, yes. This is an issue that we've been meaning to fix for a while.

…s` versions used by `@metamask/accounts-controller`

…tests)

- **BREAKING**: use `NetworkControllerNetworkDidChangeEvent` instead

…e` action and `stateChange` event

…Messenger`

…rCallback` arguments

…e yarn resolutions entry for `@metamask/providers`

Update packages/assets-controllers/CHANGELOG.md

…` is changed instead of `chainId`, and avoid resetting polling interval

…ange` listener

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

…DetectTokensController

mcmire

Looks good to me! 🎉

…roller-messenger pattern (#3690) ## Motivation Remove `TokenDetectionController` constructor callbacks for `TokensController`. - `TokensController` features should be exposed as messenger actions/events so that consumers don't need to import the entire class. - `TokensController` should use messenger action/events to consume external controller features instead of callbacks. ## Explanation - Replaces constructor options callbacks `onPreferencesStateChange`, `onNetworkDidChange`, `onTokenListStateChange`, `getNetworkClientById` with messenger actions (`NetworkController:getNetworkClientById`) and events (`PreferencesController:stateChange`, `NetworkController:networkDidChange`, `TokenListController:stateChange`). - Replaces tokens-controller callbacks in token-detection-controller and token-balances-controller with `TokensController:getState`, `TokensController:addDetectedTokens` actions and `TokensController:stateChange` event. ## References - Extracted from #3775 - Contributes to #3626 ## Changelog ### [`@metamask/assets-controllers`](https://github.com/MetaMask/core/pull/3690/files#diff-ee47d03d53776b8dd530799a8047f5e32e36e35765620aeb50b294adc3339fab) ### Added - `TokensController` now exports `TokensControllerActions`, `TokensControllerGetStateAction`, `TokensControllerAddDetectedTokensAction`, `TokensControllerEvents`, `TokensControllerStateChangeEvent`. ([#3690](#3690)) ### Changed - **BREAKING:** The `TokensState` type is now defined as a type alias rather than an interface. ([#3690](#3690)) - This is breaking because it could affect how this type is used with other types, such as `Json`, which does not support TypeScript interfaces. ### Removed - **BREAKING:** `TokenDetectionController` constructor no longer accepts options `onPreferencesStateChange`, `getPreferencesState`, `getTokensState`, `addDetectedTokens`. ([#3690](#3690), [#3775](#3775)) - **BREAKING:** `TokensController` constructor no longer accepts options `onPreferencesStateChange`, `onNetworkDidChange`, `onTokenListStateChange`, `getNetworkClientById`. ([#3690](#3690)) - **BREAKING:** `TokenBalancesController` constructor no longer accepts options `onTokensStateChange`, `getSelectedAddress`. ([#3690](#3690)) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

…h `DetectTokensController` (#3867) ## Explanation - This PR adds unit tests for the token-detection-controller API changes implemented in #3775. - The following changes are covered in the new tests: - Don't detect if keyring-controller `isUnlocked` state is false. - Subscribe to `KeyringController:unlock`, `KeyringController:lock` events - Subscribe to `AccountsController:selectedAccountChange` event - Detect tokens using `@metamask/contract-metadata` static token list if on mainnet and `useTokenDetection` is false. - Call `trackMetaMetricsEvent` for every detected token. - The aim of this PR isn't to achieve 100% test coverage for token-detection-controller. That will be the goal of follow-up ticket #1615 ## References - Closes #3626 - Follows from: - #3775 - #3690 - Followed by #1615 ## Changelog N/A ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate

MajorLift self-assigned this Jan 12, 2024

MajorLift added the team-wallet-framework label Jan 12, 2024

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch 6 times, most recently from 2444b95 to db6974c Compare January 20, 2024 02:40

MajorLift marked this pull request as ready for review January 20, 2024 03:13

MajorLift requested a review from a team as a code owner January 20, 2024 03:13

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch from e8b624c to 209effd Compare January 21, 2024 21:44

MajorLift mentioned this pull request Jan 22, 2024

TokenDetectionController: Write comprehensive unit tests #1615

Open

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch from 209effd to e9e4a4e Compare January 22, 2024 01:14

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch from db13e83 to f8df3d5 Compare January 22, 2024 15:05

MajorLift commented Jan 22, 2024

View reviewed changes

packages/assets-controllers/CHANGELOG.md Outdated Show resolved Hide resolved

packages/assets-controllers/CHANGELOG.md Outdated Show resolved Hide resolved

MajorLift mentioned this pull request Jan 22, 2024

[assets-controllers] Upgrade TokensController to fully utilize controller-messenger pattern #3690

Merged

3 tasks

MajorLift changed the title ~~Consolidate core TokenDetectionController and extension DetectTokensController~~ [token-detection-controller] Consolidate with DetectTokensController from extension Jan 22, 2024

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch from 197e379 to 8231533 Compare January 24, 2024 05:39

mcmire reviewed Jan 26, 2024

View reviewed changes

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch 2 times, most recently from 0ae2ffd to fbc1a83 Compare January 26, 2024 16:25

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch from 1957a33 to ca19f99 Compare January 26, 2024 21:25

MajorLift added 20 commits February 6, 2024 16:45

Define #registerEventListeners constructor helper private method

7016835

Add yarn resolutions entry to resolve conflicting `@metamask/provider…

a44ccf7

…s` versions used by `@metamask/accounts-controller`

Add @metamask/keyring-api as devDep (for InternalAccount type in …

3fbdffa

…tests)

Remove unused NetworkControllerStateChange event

b5bd0e8

- **BREAKING**: use `NetworkControllerNetworkDidChangeEvent` instead

Replace preferences controller callbacks in constructor with `getStat…

beacf9b

…e` action and `stateChange` event

Minor refactors to event listeners for consistency

4b303b4

test: adjust action,event allowlist in `buildTokenDetectionController…

ad11ac7

…Messenger`

test: add callbacks for all allowed actions, events as `WithControlle…

c90de3e

…rCallback` arguments

test: adjust for removal of getPreferencesState callback

1173eb9

Add CHANGELOG entries

2be2e24

[assets-controllers] Bump @metamask/kerying-api to ^3.0.0 and remov…

968c0c0

…e yarn resolutions entry for `@metamask/providers`

Update packages/assets-controllers/CHANGELOG.md

2db0081

Update packages/assets-controllers/CHANGELOG.md

Extract findCaseInsensitiveMatch helper function

7c25f3c

Use networkClientId instead of chainId in #restartTokenDetection

f1af71a

test: remove unnecessary action handler unregisters

907b4df

Changelog linter fix

04df278

In networkDidChange event listener, detect tokens if `networkClientId…

312e450

…` is changed instead of `chainId`, and avoid resetting polling interval

In changelog, move new actions/events under "Added" heading

9a4ae93

Include #stopPolling() call in KeyringController:lock listener

91be385

Replace detectTokens with #restartTokenDetection in `networkDidCh…

88eb7ca

…ange` listener

MajorLift force-pushed the 230113-consolidate-TokenDetectionController-DetectTokensController branch from 754b46f to 88eb7ca Compare February 6, 2024 21:46

test: adjust coverage thresholds

e01f993

MajorLift mentioned this pull request Feb 6, 2024

[token-detection-controller] Add tests for API changes from merge with DetectTokensController #3867

Merged

3 tasks

MajorLift and others added 2 commits February 7, 2024 13:44

Apply suggestions to CHANGELOG

4a944d8

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

Merge branch 'main' into 230113-consolidate-TokenDetectionController-…

582b370

…DetectTokensController

mcmire approved these changes Feb 8, 2024

View reviewed changes

MajorLift merged commit 00e4a9c into main Feb 8, 2024
136 checks passed

MajorLift deleted the 230113-consolidate-TokenDetectionController-DetectTokensController branch February 8, 2024 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[token-detection-controller] Consolidate with `DetectTokensController` from extension #3775

[token-detection-controller] Consolidate with `DetectTokensController` from extension #3775

MajorLift commented Jan 11, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as resolved.

MajorLift commented Jan 22, 2024

mcmire left a comment

MajorLift commented Jan 26, 2024

mcmire commented Jan 26, 2024 •

edited

Loading

mcmire commented Jan 26, 2024

MajorLift commented Jan 26, 2024

mcmire commented Jan 29, 2024

mcmire left a comment

[token-detection-controller] Consolidate with DetectTokensController from extension #3775

[token-detection-controller] Consolidate with DetectTokensController from extension #3775

Conversation

MajorLift commented Jan 11, 2024 • edited Loading

Explanation

References

Changelog

@metamask/assets-controllers

Added

Changed

Removed

Checklist

This comment was marked as outdated.

This comment was marked as resolved.

MajorLift commented Jan 22, 2024

mcmire left a comment

Choose a reason for hiding this comment

MajorLift commented Jan 26, 2024

mcmire commented Jan 26, 2024 • edited Loading

mcmire commented Jan 26, 2024

MajorLift commented Jan 26, 2024

mcmire commented Jan 29, 2024

mcmire left a comment

Choose a reason for hiding this comment

[token-detection-controller] Consolidate with `DetectTokensController` from extension #3775

[token-detection-controller] Consolidate with `DetectTokensController` from extension #3775

MajorLift commented Jan 11, 2024 •

edited

Loading

`@metamask/assets-controllers`

mcmire commented Jan 26, 2024 •

edited

Loading