Merge TokenDetectionController with DetectTokensController (from extension), and migrate to this repo #1812
Labels
Comments
9 tasks
MajorLift
added a commit
to MetaMask/metamask-extension
that referenced
this issue
Mar 14, 2024
…nDetectionController` (#22928) ## **Description** This commit replaces [`DetectTokensController`](https://github.com/MetaMask/metamask-extension/blob/68cc610976485d0071400cb2d4c3ea18cc6b15d9/app/scripts/controllers/detect-tokens.js) with the core repo's [`TokenDetectionController`](https://github.com/MetaMask/core/blob/main/packages/assets-controllers/src/TokenDetectionController.ts). This represents the final step of Shared Libraries' initiative to a) consolidate the core repo's `TokenDetectionController`, the extension's `DetectTokensController`, and relevant mobile patches to `@metamask/assets-controllers`, with the goal of b) migrating both extension and mobile to use the consolidated controller in core. This also represents a full conversion to TypeScript for `DetectTokensController` and its unit tests. [](https://codespaces.new/MetaMask/metamask-extension/pull/22928?quickstart=1) ## **Related issues** - Contributes to: - MetaMask/core#700 - MetaMask/core#1812 - Closes: - #23127 - #23322 - Blocked by: - MetaMask/core#3916 - MetaMask/core#3923 - MetaMask/core#4007 (`@metamask/assets-controllers` v26) ## **Manual testing steps** - Check whether issue no. 4 in this comment #23010 (comment) can be reproed in this branch. ## Changelog ### `metamask-extension` - `MetamaskController` class: - Define a `preferencesControllerMessenger` instance which only allows the `PreferencesController:getState` action and `PreferencesController:stateChange` event. - Add a subscription to the preferences-controller observable store, with a listener that publishes a `PreferencesController:stateChange` event. - `PreferencesController`: - Add `messenger` as an optional constructor options object property. - Register a `PreferencesController:getState` action handler. - **BREAKING:** Replace all imports of `toChecksumHexAddress` from `@metamask/controller-utils` with imports from the internal `shared/modules/hexstring-utils` module. - **BREAKING:** Bump `@metamask/assets-controllers` to `^26.0.0`. - Remove patch. - **BREAKING:** Bump `@metamask/accounts-controller` to `^11.0.0`. - Remove unused diff in patch. - **BREAKING:** Bump `@metamask/keyring-controller` to `^13.0.0`. - Remove patch. - **BREAKING:** Remove `@metamask/polling-controller` as a dependency. - Required by `test-yarn-dedupe` CI run. - Bump `@metamask/controller-utils` to `^8.0.4`. ### `TokenDetectionController` (compared to `DetectTokensController`) - **BREAKING:** Inherit from `StaticIntervalPollingController` instead of `StaticIntervalPollingControllerOnly` - Constructor and class fields: - **BREAKING:** Remove `preferences`, `network`, `tokenList`, `tokensController`, `assetsContractController`, `getCurrentSelectedAccount`, `getNetworkClientById` as constructor options, and add required option `getBalancesInSingleCall`. - **BREAKING:** Remove `disableLegacyInterval` class field and constructor option. - `#restartTokenDetection` always resets polling interval to default regardless of whether legacy or new polling is being used. - **BREAKING:** Add a `#disabled` private class field, which blocks all network requests if set to true, and add `disabled` as an optional constructor option, which defaults to 'true' if omitted. - **BREAKING:** Remove `isOpen` class field, and replace by adding `enable`, `disable` public methods. - Add optional constructor option `selectedAddress`. If omitted, its value is populated by calling the `AccountsController:getSelectedAccount` action. - Messenger: - **BREAKING:** Newly subscribe to the `PreferencesController:stateChange`, `AccountsController:selectedAccountChange`, `KeyringController:lock`, `KeyringController:unlock` events. - **BREAKING:** Newly allow messenger actions `AccountsController:getSelectedAccount`, `NetworkController:getNetworkClientById`, `NetworkController:getNetworkConfigurationByNetworkClientId`, `NetworkController:getState`, `KeyringController:getState`, `PreferencesController:getState`, `TokenListController:getState`, `TokensController:getState`, and `TokensController:addDetectedTokens`. - **BREAKING:** `detectTokens` replaces the `detectNewTokens` method. - **BREAKING:** Now expects an options object with optional properties `selectedAddress`, `networkClientId`, removing the `chainId` option. - **BREAKING:** Passes lists of full `Token` types to `TokensController:addDetectedTokens` instead of objects containing only `{ address, decimals, symbol }`. - Processes an arbitrary number of tokens in batches of 1000. - Previously, `detectTokens` was limited to two batches, with the first batch being limited to 1000 tokens. - If the `getBalancesInSingleCall` callback fails, it does not throw an error or exit early, and the method continues processing the next batch of tokens. - **BREAKING:** `#restartTokenDetection` is a private method instead of public. - **BREAKING:** Replace the `getChainIdFromNetworkStore` method with the private method `#getCorrectChainIdAndNetworkClientId`. - **BREAKING:** `#trackMetaMetricsEvents` is a private method instead of protected. - Passes string literals instead of extension shared constants into `_trackMetaMetricsEvent`. ### `TokensController` - **BREAKING:** Newly allows `NetworkController:getNetworkClientById` messenger action. - **BREAKING:** Newly subscribes to `NetworkController:networkDidChange`, `PreferencesController:stateChange`, `TokenListController:stateChange` events. - **BREAKING:** Unsubscribes from `NetworkController:stateChange` event. ## **Screenshots/Recordings** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
davidmurdoch
pushed a commit
to MetaMask/metamask-extension
that referenced
this issue
Mar 14, 2024
](https://codespaces.new/MetaMask/metamask-extension/pull/22928?quickstart=1){kind=link}
…nDetectionController` (#22928) ## **Description** This commit replaces [`DetectTokensController`](https://github.com/MetaMask/metamask-extension/blob/68cc610976485d0071400cb2d4c3ea18cc6b15d9/app/scripts/controllers/detect-tokens.js) with the core repo's [`TokenDetectionController`](https://github.com/MetaMask/core/blob/main/packages/assets-controllers/src/TokenDetectionController.ts). This represents the final step of Shared Libraries' initiative to a) consolidate the core repo's `TokenDetectionController`, the extension's `DetectTokensController`, and relevant mobile patches to `@metamask/assets-controllers`, with the goal of b) migrating both extension and mobile to use the consolidated controller in core. This also represents a full conversion to TypeScript for `DetectTokensController` and its unit tests. [](https://codespaces.new/MetaMask/metamask-extension/pull/22928?quickstart=1) ## **Related issues** - Contributes to: - MetaMask/core#700 - MetaMask/core#1812 - Closes: - #23127 - #23322 - Blocked by: - MetaMask/core#3916 - MetaMask/core#3923 - MetaMask/core#4007 (`@metamask/assets-controllers` v26) ## **Manual testing steps** - Check whether issue no. 4 in this comment #23010 (comment) can be reproed in this branch. ## Changelog ### `metamask-extension` - `MetamaskController` class: - Define a `preferencesControllerMessenger` instance which only allows the `PreferencesController:getState` action and `PreferencesController:stateChange` event. - Add a subscription to the preferences-controller observable store, with a listener that publishes a `PreferencesController:stateChange` event. - `PreferencesController`: - Add `messenger` as an optional constructor options object property. - Register a `PreferencesController:getState` action handler. - **BREAKING:** Replace all imports of `toChecksumHexAddress` from `@metamask/controller-utils` with imports from the internal `shared/modules/hexstring-utils` module. - **BREAKING:** Bump `@metamask/assets-controllers` to `^26.0.0`. - Remove patch. - **BREAKING:** Bump `@metamask/accounts-controller` to `^11.0.0`. - Remove unused diff in patch. - **BREAKING:** Bump `@metamask/keyring-controller` to `^13.0.0`. - Remove patch. - **BREAKING:** Remove `@metamask/polling-controller` as a dependency. - Required by `test-yarn-dedupe` CI run. - Bump `@metamask/controller-utils` to `^8.0.4`. ### `TokenDetectionController` (compared to `DetectTokensController`) - **BREAKING:** Inherit from `StaticIntervalPollingController` instead of `StaticIntervalPollingControllerOnly` - Constructor and class fields: - **BREAKING:** Remove `preferences`, `network`, `tokenList`, `tokensController`, `assetsContractController`, `getCurrentSelectedAccount`, `getNetworkClientById` as constructor options, and add required option `getBalancesInSingleCall`. - **BREAKING:** Remove `disableLegacyInterval` class field and constructor option. - `#restartTokenDetection` always resets polling interval to default regardless of whether legacy or new polling is being used. - **BREAKING:** Add a `#disabled` private class field, which blocks all network requests if set to true, and add `disabled` as an optional constructor option, which defaults to 'true' if omitted. - **BREAKING:** Remove `isOpen` class field, and replace by adding `enable`, `disable` public methods. - Add optional constructor option `selectedAddress`. If omitted, its value is populated by calling the `AccountsController:getSelectedAccount` action. - Messenger: - **BREAKING:** Newly subscribe to the `PreferencesController:stateChange`, `AccountsController:selectedAccountChange`, `KeyringController:lock`, `KeyringController:unlock` events. - **BREAKING:** Newly allow messenger actions `AccountsController:getSelectedAccount`, `NetworkController:getNetworkClientById`, `NetworkController:getNetworkConfigurationByNetworkClientId`, `NetworkController:getState`, `KeyringController:getState`, `PreferencesController:getState`, `TokenListController:getState`, `TokensController:getState`, and `TokensController:addDetectedTokens`. - **BREAKING:** `detectTokens` replaces the `detectNewTokens` method. - **BREAKING:** Now expects an options object with optional properties `selectedAddress`, `networkClientId`, removing the `chainId` option. - **BREAKING:** Passes lists of full `Token` types to `TokensController:addDetectedTokens` instead of objects containing only `{ address, decimals, symbol }`. - Processes an arbitrary number of tokens in batches of 1000. - Previously, `detectTokens` was limited to two batches, with the first batch being limited to 1000 tokens. - If the `getBalancesInSingleCall` callback fails, it does not throw an error or exit early, and the method continues processing the next batch of tokens. - **BREAKING:** `#restartTokenDetection` is a private method instead of public. - **BREAKING:** Replace the `getChainIdFromNetworkStore` method with the private method `#getCorrectChainIdAndNetworkClientId`. - **BREAKING:** `#trackMetaMetricsEvents` is a private method instead of protected. - Passes string literals instead of extension shared constants into `_trackMetaMetricsEvent`. ### `TokensController` - **BREAKING:** Newly allows `NetworkController:getNetworkClientById` messenger action. - **BREAKING:** Newly subscribes to `NetworkController:networkDidChange`, `PreferencesController:stateChange`, `TokenListController:stateChange` events. - **BREAKING:** Unsubscribes from `NetworkController:stateChange` event. ## **Screenshots/Recordings** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
These controllers are similar but have separate implementations. We will need to dive into how they are used in the extension vs. mobile app to understand what a combined version should look like.
While we are doing this, it may be worth it to complete the following items before completing the merge:
detectTokens/detectNewTokensmethod (as it is quite complex): TokenDetectionController: Refactor #1614Additionally, there are patches in mobile around TokenDetectionController under
@metamask/assets-controllers, and we will want to make sure those are integrated as well.The text was updated successfully, but these errors were encountered: