Skip to content

Commit

Permalink
refactor: Replace DetectTokensController with core monorepo's `Toke…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
MajorLift authored and davidmurdoch committed Mar 14, 2024
1 parent a873c1b commit 53003b4
Show file tree
Hide file tree
Showing 35 changed files with 398 additions and 2,049 deletions.
4 changes: 1 addition & 3 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,7 @@ module.exports = {
],
excludedFiles: [
'app/scripts/controllers/app-state.test.js',
'app/scripts/controllers/mmi-controller.test.ts',
'app/scripts/controllers/detect-tokens.test.js',
'app/scripts/controllers/mmi-controller.test.js',
'app/scripts/controllers/permissions/**/*.test.js',
'app/scripts/controllers/preferences.test.js',
'app/scripts/lib/**/*.test.js',
Expand Down Expand Up @@ -299,7 +298,6 @@ module.exports = {
'app/scripts/controllers/permissions/**/*.test.js',
'app/scripts/controllers/preferences.test.js',
'app/scripts/lib/**/*.test.js',
'app/scripts/controllers/detect-tokens.test.js',
'app/scripts/metamask-controller.test.js',
'app/scripts/migrations/*.test.js',
'app/scripts/platforms/*.test.js',
Expand Down
1 change: 0 additions & 1 deletion .mocharc.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ module.exports = {
'./app/scripts/lib/**/*.test.js',
'./app/scripts/migrations/*.test.js',
'./app/scripts/platforms/*.test.js',
'./app/scripts/controllers/detect-tokens.test.js',
'./app/scripts/controllers/app-state.test.js',
'./app/scripts/controllers/permissions/**/*.test.js',
'./app/scripts/controllers/mmi-controller.test.ts',
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

Loading

0 comments on commit 53003b4

Please sign in to comment.