Skip to content
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

Replace DetectTokensController with consolidated TokenDetectionController from core repo #23127

Closed
1 of 9 tasks
MajorLift opened this issue Feb 22, 2024 · 0 comments · Fixed by #22928
Closed
1 of 9 tasks

Replace DetectTokensController with consolidated TokenDetectionController from core repo #23127

MajorLift opened this issue Feb 22, 2024 · 0 comments · Fixed by #22928
Assignees
@MajorLift
Labels
release-11.14.0 Issue or pull request that will be included in release 11.14.0 team-wallet-framework

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Feb 22, 2024 •
edited
Loading

What is this about?

The core repo TokenDetectionController has been consolidated with the extension DetectTokensController as of MetaMask/core#1812.

Once these changes are released, the extension-side DetectTokensController will be replaced.

Breaking Changes to be introduced by TokenDetectionController

  • Extends StaticIntervalPollingController instead of StaticIntervalPollingControllerOnly

  • Constructor options:

    • Removes getCurrentSelectedAccount, getNetworkClientById callbacks as constructor options. Uses corresponding messenger actions instead.
    • Removes disableLegacyInterval class field and constructor option.
      • Instead has disabled option, which blocks all network requests.

  • Adds enable, disable public methods.

  • #restartTokenDetection is a private method.

    • #restartTokenDetection accepts options object { selectedAddress, networkClientId } instead of { selectedAddress, chainId }
    • #restartTokenDetection always resets polling interval to default regardless of whether legacy or new polling is being used.
    • Removes the getChainIdFromNetworkStore public method and replaces internally with #getCorrectChainIdAndNetworkClientId.

  • Uses NetworkController:getNetworkConfigurationByNetworkClientId action instead of NetworkController:getNetworkClientById action to get correct 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 { accountAddress, 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.

Scenario

No response

Design

No response

Technical Details

No response

Threat Modeling Framework

No response

Acceptance Criteria

No response

Stakeholder review needed before the work gets merged

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References
@MajorLift MajorLift self-assigned this Feb 22, 2024
@MajorLift MajorLift linked a pull request Feb 22, 2024 that will close this issue
refactor: Replace DetectTokensController with core monorepo's TokenDetectionController #22928
Merged
13 tasks
@MajorLift MajorLift mentioned this issue Feb 22, 2024
Merged
13 tasks
MajorLift added a commit that referenced this issue Mar 14, 2024
@MajorLift
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.
@metamaskbot metamaskbot added the release-11.14.0 Issue or pull request that will be included in release 11.14.0 label Mar 14, 2024
davidmurdoch pushed a commit that referenced this issue Mar 14, 2024
@MajorLift @davidmurdoch
refactor: Replace DetectTokensController with core monorepo's `Toke…
53003b4 
…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
Assignees

@MajorLift MajorLift

Labels
release-11.14.0 Issue or pull request that will be included in release 11.14.0 team-wallet-framework
Projects
None yet
Milestone
No milestone
2 participants
@MajorLift @metamaskbot