[token-detection-controller] Apply recent `DetectTokensController` updates #3923

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

Merged

MajorLift merged 18 commits into main from 240215-TokenDetectionController-sync-with-extension

Feb 27, 2024

Contributor

MajorLift commented Feb 15, 2024 •

edited

Loading

Explanation

As a preparatory step for fully replacing the extension DetectTokensController with the consolidated core repo TokenDetectionController, TokenDetectionController needs to be updated with changes made to the extension DetectTokensController since #1813 was closed.

Diff of `DetectTokensController` state

MetaMask/metamask-extension@5d285f7be5f7be981995dfa725aad97d81cc990a..85cd1c89039e900b452edb704ec37e9ccbd3e76a#diff-323d0cf464

Differences from extension `DetectTokensController`

Refactors logic for retrieving chainId, networkClientId into this.#getCorrectChainIdAndNetworkClientId
- Uses getNetworkConfigurationByNetworkClientId action instead of getNetworkClientById to retrieve chainId.
- If networkClientId is not supplied to the method, or it's supplied but getNetworkConfigurationByNetworkClientId returns undefined, finds chainId from providerConfig.
detectTokens replaces detectNewTokens
- detectTokens accepts options object { selectedAddress, networkClientId } instead of { selectedAddress, chainId, networkClientId }.
- Does not throw error if getBalancesInSingleCall fails. Also does not exit early -- continues looping.
- Passes lists of full Token types to TokensController:addDetectedTokens instead of objects containing only { address, decimals, symbol }.
#trackMetaMetricsEvents is a private method instead of protected.
- Passes string literals instead of extension shared constants into _trackMetaMetricsEvent.

References

Partially implements Bring TokenDetectionController up-to-date and release new version of assets-controllers #3916
Blocking Release 118.0.0 #3918
Changes adopted from:
Blocking (Followed by) [token-detection-controller] Refactor detectTokens method #3938

Changelog

`@metamask/assets-controllers`

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

MajorLift mentioned this pull request

Release 118.0.0 #3918

Closed

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from bbbea84 to 6be2657 Compare

February 16, 2024 05:13

MajorLift changed the base branch from main to jl/mmp-3905/fix-detect-tokens-controller-token-list-polling

February 16, 2024 06:00

MajorLift changed the title ~~[token-detection-controller] Sync with updates made in extension DetectTokensController~~ [token-detection-controller] Apply updates made in extension DetectTokensController

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch 2 times, most recently from e03832d to d6e35d1 Compare

February 16, 2024 06:02

MajorLift changed the base branch from jl/mmp-3905/fix-detect-tokens-controller-token-list-polling to main

February 16, 2024 06:03

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from d6e35d1 to 7abf52f Compare

February 16, 2024 06:05

MajorLift mentioned this pull request

refactor: Replace DetectTokensController with core monorepo's TokenDetectionController MetaMask/metamask-extension#22928

Merged

13 tasks

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch 3 times, most recently from e139f86 to cd16db6 Compare

February 16, 2024 20:29

MajorLift changed the title ~~[token-detection-controller] Apply updates made in extension DetectTokensController~~ [token-detection-controller] Update for parity with extension DetectTokensController, refactor detectTokens method

MajorLift changed the base branch from main to jl/mmp-3905/fix-detect-tokens-controller-token-list-polling

February 16, 2024 20:33

MajorLift changed the base branch from jl/mmp-3905/fix-detect-tokens-controller-token-list-polling to main

February 16, 2024 20:33

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch 6 times, most recently from 6baa047 to 3425c89 Compare

February 17, 2024 03:36

MajorLift changed the title ~~[token-detection-controller] Update for parity with extension DetectTokensController, refactor detectTokens method~~ [token-detection-controller] Apply recent DetectTokensController updates

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from 0a13346 to de5043b Compare

February 17, 2024 04:17

MajorLift mentioned this pull request

[token-detection-controller] Refactor detectTokens method #3928

Closed

3 tasks

MajorLift marked this pull request as ready for review

February 20, 2024 15:55

MajorLift requested a review from a team as a code owner

February 20, 2024 15:55

MajorLift self-assigned this

MajorLift added the team-wallet-framework label

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from fdd94ce to c9d88b6 Compare

February 20, 2024 18:13

MajorLift requested a review from jiexi

February 20, 2024 21:48

bergeron reviewed

View reviewed changes

packages/assets-controllers/src/TokenDetectionController.ts Show resolved Hide resolved

bergeron reviewed

View reviewed changes

packages/assets-controllers/src/TokenDetectionController.ts

    
                  }

                  for (const tokensSlice of sliceOfTokensToDetect) {

                  for (const tokensSlice of slicesOfTokensToDetect) {

Contributor

bergeron Feb 22, 2024 •

edited

Loading

Seems like the slices could be queried in parallel as a future improvement? What do you you think?

Contributor

mcmire Feb 23, 2024

I don't see a reason why we couldn't do that, yeah.

mcmire reviewed

View reviewed changes

packages/assets-controllers/src/TokenDetectionController.ts Show resolved Hide resolved

packages/assets-controllers/src/TokenDetectionController.ts Show resolved Hide resolved

packages/assets-controllers/src/TokenDetectionController.ts Outdated Show resolved Hide resolved

packages/assets-controllers/src/TokenDetectionController.ts

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)),

                  );

Contributor

mcmire Feb 23, 2024

TIL that allIgnoredTokens doesn't match the others 😩

packages/assets-controllers/src/TokenDetectionController.ts

    
                  }

                  for (const tokensSlice of sliceOfTokensToDetect) {

                  for (const tokensSlice of slicesOfTokensToDetect) {

Contributor

mcmire Feb 23, 2024

I don't see a reason why we couldn't do that, yeah.

packages/assets-controllers/src/TokenDetectionController.ts

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) {

Contributor

mcmire Feb 23, 2024

This is so much easier to read! 😮‍💨

packages/assets-controllers/src/TokenDetectionController.test.ts

    
                      }) => {

                        mockGetProviderConfig({

                          chainId: '0x89',

                        } as unknown as ProviderConfig);

Contributor

mcmire Feb 23, 2024

👍🏻

packages/assets-controllers/src/TokenDetectionController.test.ts Show resolved Hide resolved

MajorLift and others added 12 commits

February 23, 2024 13:50


          Initialize #selectedAddress using `AccountsController:getSelectedAc…

0fa46b3

…count` action

- constructor option `selectedAddress` no longer defaults to ''


          [detectTokens] Align variable names with extension detectNewTokens …

e40ae46

…method


          [detectTokens] Apply extension-side fix for getting correct `networkC…

a10e4e0

…lientId` and passing it to `getBalancesInSingleCall`

- See MetaMask/metamask-extension#22898
- Uses `NetworkController:getNetworkConfigurationByNetworkClientId` instead of `NetworkController:getNetworkClientById`


          [detectTokens] Apply and refactor fixes for detected tokens being add…

a26ea0f

…ed to wrong network, and detection using `chainId` keyed states

- See MetaMask/metamask-extension#22814
- See #3914


          Replace #getCorrectChainId with #getCorrectChainIdAndNetworkClientId

99e01da


          test: register action handlers for getSelectedAccount, `findNetwork…

904e4b7

…ClientIdByChainId`, `getProviderConfig`


          Fix erroneous reference to tokenList


          Fix typo

e2be829


          Update changelog

a3b5c55


          test: adjust coverage thresholds

f452964


          test: Fix unmocked ProviderConfig for changed chainId

366e56c


          Update packages/assets-controllers/CHANGELOG.md

857142b

Co-authored-by: jiexi <jiexiluan@gmail.com>

MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from eccf79c to 8a73dde Compare

February 23, 2024 18:55

MajorLift added 3 commits

February 23, 2024 14:04


          Rename accoutAddress option of detectTokens to selectedAddress …

2b88d05

…for consistency


          Update this.#chainId on networkDidChange event


          Remove reference to this.#chainId from `#getCorrectChainIdAndNetwor…

e9bbf1d

…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

February 23, 2024 19:04

bergeron mentioned this pull request

Make findNetworkClientIdByChainId prefer the current client if multiple match #3927

Closed

3 tasks

mcmire approved these changes

View reviewed changes

Contributor

mcmire left a comment

Looks good!

MajorLift added 3 commits

February 26, 2024 18:56


          Merge branch 'main' into 240215-TokenDetectionController-sync-with-ex…

3f9de16

…tension


          Merge branch 'main' into 240215-TokenDetectionController-sync-with-ex…

7d15225

…tension


          Merge branch 'main' into 240215-TokenDetectionController-sync-with-ex…

2b4a6f7

…tension

MajorLift merged commit 1d78bb5 into main

MajorLift deleted the 240215-TokenDetectionController-sync-with-extension branch

February 27, 2024 00:13

MajorLift added a commit that referenced this pull request


          [token-detection-controller] Refactor detectTokens method (#3938)

34bf8f0

## 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


          refactor: Replace DetectTokensController with core monorepo's `Toke…

4f2d6a4

…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.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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

Labels

team-wallet-framework