[token-detection-controller] Apply recent DetectTokensController updates
#3923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
from
bbbea84
to
6be2657
Compare
MajorLift
changed the base branch from
main
to
jl/mmp-3905/fix-detect-tokens-controller-token-list-polling
MajorLift
changed the title
DetectTokensControllerDetectTokensController
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
2 times, most recently
from
e03832d
to
d6e35d1
Compare
MajorLift
changed the base branch from
jl/mmp-3905/fix-detect-tokens-controller-token-list-polling
to
main
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
from
d6e35d1
to
7abf52f
Compare
13 tasks
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
3 times, most recently
from
e139f86
to
cd16db6
Compare
MajorLift
changed the title
DetectTokensControllerDetectTokensController, refactor detectTokens method
MajorLift
changed the base branch from
main
to
jl/mmp-3905/fix-detect-tokens-controller-token-list-polling
MajorLift
changed the base branch from
jl/mmp-3905/fix-detect-tokens-controller-token-list-polling
to
main
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
6 times, most recently
from
6baa047
to
3425c89
Compare
MajorLift
changed the title
DetectTokensController, refactor detectTokens methodDetectTokensController updates
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
from
0a13346
to
de5043b
Compare
3 tasks
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
from
fdd94ce
to
c9d88b6
Compare
bergeron
reviewed
Feb 22, 2024
bergeron
reviewed
Feb 22, 2024
| } | ||
|
|
||
| for (const tokensSlice of sliceOfTokensToDetect) { | ||
| for (const tokensSlice of slicesOfTokensToDetect) { |
There was a problem hiding this comment.
Seems like the slices could be queried in parallel as a future improvement? What do you you think?
There was a problem hiding this comment.
I don't see a reason why we couldn't do that, yeah.
mcmire
reviewed
Feb 23, 2024
Comment on lines
+514
to
+524
| const [tokensAddresses, detectedTokensAddresses, ignoredTokensAddresses] = [ | ||
| allTokens, | ||
| allDetectedTokens, | ||
| allIgnoredTokens, | ||
| ].map((tokens) => | ||
| ( | ||
| tokens[chainIdAgainstWhichToDetect]?.[addressAgainstWhichToDetect] ?? [] | ||
| ).map((value) => (typeof value === 'string' ? value : value.address)), | ||
| ); |
There was a problem hiding this comment.
TIL that allIgnoredTokens doesn't match the others 😩
| } | ||
|
|
||
| for (const tokensSlice of sliceOfTokensToDetect) { | ||
| for (const tokensSlice of slicesOfTokensToDetect) { |
There was a problem hiding this comment.
I don't see a reason why we couldn't do that, yeah.
Comment on lines
-508
to
-522
| let ignored; | ||
| for (const tokenAddress of Object.keys(balances)) { | ||
| if (ignoredTokens.length) { | ||
| ignored = ignoredTokens.find( | ||
| (ignoredTokenAddress) => | ||
| ignoredTokenAddress === toChecksumHexAddress(tokenAddress), | ||
| ); | ||
| } | ||
| const caseInsensitiveTokenKey = | ||
| findCaseInsensitiveMatch( | ||
| Object.keys(tokenListUsed), | ||
| tokenAddress, | ||
| ) ?? ''; | ||
|
|
||
| if (ignored === undefined) { |
There was a problem hiding this comment.
This is so much easier to read! 😮💨
| }) => { | ||
| mockGetProviderConfig({ | ||
| chainId: '0x89', | ||
| } as unknown as ProviderConfig); |
…count` action - constructor option `selectedAddress` no longer defaults to ''
…lientId` and passing it to `getBalancesInSingleCall` - See MetaMask/metamask-extension#22898 - Uses `NetworkController:getNetworkConfigurationByNetworkClientId` instead of `NetworkController:getNetworkClientById`
…ed to wrong network, and detection using `chainId` keyed states - See MetaMask/metamask-extension#22814 - See #3914
…ClientIdByChainId`, `getProviderConfig`
Co-authored-by: jiexi <jiexiluan@gmail.com>
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
from
eccf79c
to
8a73dde
Compare
…for consistency
…kClientId`, so that the method only relies on the network-controller, not the cached `this.#chainId` that could be stale
MajorLift
force-pushed
the
240215-TokenDetectionController-sync-with-extension
branch
from
8a73dde
to
e9bbf1d
Compare
3 tasks
MajorLift
deleted the
240215-TokenDetectionController-sync-with-extension
branch
MajorLift
added a commit
that referenced
this pull request
Mar 4, 2024
## Explanation Adds refactors and cosmetic, typing fixes to #3923. - Extracts three methods from `TokenDetectionController`'s `detectTokens` method: - `#getCorrectChainIdAndNetworkClientId` - `#getTokenListAndSlicesOfTokensToDetect` - `#addDetectedTokens` - Maintains distinction between class fields `#selectedAddress`, `#networkClientId` and corresponding parameters used in `detectTokens` and its helper methods, so that `detectTokens` method can be used independently of polling/passive detection. - [Refactor `#getCorrectChainIdAndNetworkClientId`](c75fb3b) to remove `findNetworkClientIdByChainId` which might return inconsistent/unexpected results, and replace it with `getState`, `getNetworkClientById` - [Add missing method return types](5c2e887) - [Type networkClientId as `NetworkClientId`](05be5d2) - [Fix excess properties from legacy token list, define `LegacyToken`, `TokenDetectionMap`](ea4c5b8) - Removes `#chainId` class field. ## References - Closes #1614 - Fixes #3661 - Blocked by (Follows from) #3923 - Blocking #3916 - Blocking #3918 ## Changelog ## 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: Brian Bergeron <brian.e.bergeron@gmail.com> Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
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 pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
As a preparatory step for fully replacing the extension
DetectTokensControllerwith the consolidated core repoTokenDetectionController,TokenDetectionControllerneeds to be updated with changes made to the extensionDetectTokensControllersince #1813 was closed.Diff of
DetectTokensControllerstate5d285f7be5f7be981995dfa725aad97d81cc990a..85cd1c89039e900b452edb704ec37e9ccbd3e76a#diff-323d0cf464Differences from extension
DetectTokensControllerRefactors logic for retrieving
chainId,networkClientIdintothis.#getCorrectChainIdAndNetworkClientIdgetNetworkConfigurationByNetworkClientIdaction instead ofgetNetworkClientByIdto retrievechainId.networkClientIdis not supplied to the method, or it's supplied butgetNetworkConfigurationByNetworkClientIdreturnsundefined, findschainIdfromproviderConfig.detectTokensreplacesdetectNewTokensdetectTokensaccepts options object{ selectedAddress, networkClientId }instead of{ selectedAddress, chainId, networkClientId }.getBalancesInSingleCallfails. Also does not exit early -- continues looping.Tokentypes toTokensController:addDetectedTokensinstead of objects containing only{ address, decimals, symbol }.#trackMetaMetricsEventsis a private method instead of protected._trackMetaMetricsEvent.References
TokenDetectionControllerup-to-date and release new version ofassets-controllers#3916detectTokensmethod #3938Changelog
@metamask/assets-controllersChecklist