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.

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

refactor: Replace DetectTokensController with core monorepo's TokenDetectionController #22928

Merged
merged 42 commits into from
Mar 14, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Feb 13, 2024

Description

This commit replaces DetectTokensController with the core repo's TokenDetectionController.

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

Related issues

Manual testing steps

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

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • 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.

This comment was marked as resolved.

@MajorLift MajorLift changed the title Apply consolidated TokenDetectionController from core Replace DetectTokensController with consolidated TokenDetectionController from core Feb 16, 2024
@bergeron bergeron self-requested a review February 16, 2024 17:18
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 21, 2024
@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch from 6081fd9 to 9bf4d14 Compare February 22, 2024 22:59
@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch from 9bf4d14 to da7270a Compare March 8, 2024 22:41

This comment was marked as resolved.

Copy link

socket-security bot commented Mar 8, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package Note
New author npm/rfdc@1.3.1

Ignoring: npm/@metamask/accounts-controller@11.0.0, npm/@metamask/assets-controllers@26.0.0, npm/@metamask/eth-json-rpc-infura@9.1.0, npm/@metamask/keyring-controller@13.0.0, npm/@metamask/preferences-controller@8.0.0

View full report↗︎

Next steps

What is new author?

A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package.

Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/rfdc@1.3.1

@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch from da7270a to 96e8576 Compare March 8, 2024 22:44
@metamaskbot metamaskbot removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Mar 8, 2024
@MajorLift MajorLift added the needs-qa Label will automate into QA workspace label Mar 8, 2024
@MajorLift MajorLift self-assigned this Mar 8, 2024
@MajorLift MajorLift changed the title Replace DetectTokensController with consolidated TokenDetectionController from core refactor: Replace DetectTokensController with consolidated TokenDetectionController from core repo Mar 8, 2024
@MajorLift MajorLift added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 8, 2024
@MajorLift

This comment was marked as resolved.

@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch from 5ce8858 to 4dc3ab4 Compare March 8, 2024 23:22
@MajorLift MajorLift removed needs-qa Label will automate into QA workspace needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 8, 2024
@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch 3 times, most recently from c6a3a05 to 5d43932 Compare March 9, 2024 03:18
@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch 4 times, most recently from 5c99b14 to 4f6a160 Compare March 13, 2024 01:51
@metamaskbot

This comment has been minimized.

@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch from b669ccf to b825ed4 Compare March 13, 2024 16:02
@metamaskbot

This comment was marked as duplicate.

@metamaskbot

This comment was marked as duplicate.

mcmire
mcmire previously approved these changes Mar 13, 2024
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bergeron
Copy link
Contributor

I'm not super familiar with the controller messaging patterns to determine if they're all correct. But I don't see anything that looks wrong! 🚀

bergeron
bergeron previously approved these changes Mar 13, 2024
@metamaskbot

This comment was marked as duplicate.

@MajorLift MajorLift dismissed stale reviews from bergeron and mcmire via d038ff9 March 13, 2024 22:47
@MajorLift MajorLift force-pushed the 240213-apply-token-detection-controller branch from 3957631 to d038ff9 Compare March 13, 2024 22:47
@metamaskbot
Copy link
Collaborator

Builds ready [17dde59]
Page Load Metrics (1048 ± 461 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint692231284019
domContentLoaded989372311
load6222521048960461
domInteractive989372311
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -73.33 KiB (-2.00%)
  • ui: 48 Bytes (0.00%)
  • common: -3.47 KiB (-0.07%)

@MajorLift MajorLift requested a review from mcmire March 14, 2024 15:22
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@MajorLift MajorLift merged commit 4f2d6a4 into develop Mar 14, 2024
64 of 65 checks passed
@MajorLift MajorLift deleted the 240213-apply-token-detection-controller branch March 14, 2024 16:42
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 14, 2024
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-balances area-tokens release-11.14.0 Issue or pull request that will be included in release 11.14.0 team-wallet-framework type-refactor
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Replace DetectTokensController with consolidated TokenDetectionController from core repo
6 participants