Plan consolidated API for TokenDetectionController + DetectTokensController

[mcmire]

Look into how TokenDetectionController (from this repo) is being used in the mobile app (including the patches for this controller in mobile) and how DetectTokensController is being used in the extension, and determine an API for a combined controller that satisfies the requirements that extension and mobile have.

[MajorLift]

Consolidated TokenDetectionController API

• Note: remove all properties from TokenDetection{Config,State} and use private class fields instead.

Constructor

• deferPollingStart (optional)
• interval (optional)
• chainId (required)
• selectedAddress (optional)
• messenger (required)

Fields

• super.#intervalLength

• #intervalId

• #chainId

• #selectedAddress

• #isDetectionEnabledFromPreference

• #isDetectionEnabledForNetwork

• Callbacks for communicating with BaseControllerV1 controllers:

  • #onPreferenceStateChange
  • #getBalancesInSingleCall
  • #addDetectedTokens
  • #getTokensState
  • #getPreferencesState

Methods

polling-controller:

• super.{g,s}etIntervalLength
• startPolling()
• stopPolling()
• _executePoll()
• restartTokenDetection()

(remove start, stop)

core:

• detectTokens (merge with detectNewTokens())
  • Refactor - extract methods:
    • updateTokens()
    • getTokensToDetectSlices()
    • operationToSafelyExecute()
  • @throws: MetaMask - DetectTokensController single call balance fetch failed
  • Add this.trackMetaMetricsEvent call
• getCorrectChainId() (merge with getChainIdFromNetworkStore())

extension:

• #registerKeyringHandlers()
• isActive()
• trackMetaMetricsEvent()

mobile:

• updateTokensName(): (tokenList) => tokensController.updateTokensName(tokenList)

const { tokens } = this.getTokensState();
...
if (tokens.length && !tokens[0].name) {
    this.updateTokensName(tokenList);
}

remove

• extension:
  • interval accessors
  • tokenList setter

[mcmire]

@MajorLift Is this essentially blocked by https://github.com/MetaMask/core/issues/3625? Would you need to revisit this once that's merged?

[MajorLift]

Any changes to the TokenDetectionController API that are made in #3625 will need to be reflected here as well, but it doesn't look like there will be any significant departures from what I have right now.

Mostly I need a sign-off on the plan in the comment above before I can get started on #3626. That can wait until after #3625 is merged, though.

[mcmire]

It is good that you've noted some of the ways that each implementation captures data internally, but the list is fairly large for what is a small controller. I wonder if it would be better to list the public API that we want to have rather than all of the private properties and methods that are present. How we end up storing data internally or refactoring the logic, we can decide when we create the consolidated version, but I don't know if that's important for now.

Here's what I see as the public API:

• Everything from BaseController and StaticIntervalPollingController (we can't change this, so it stays)
  • name instance property
  • metadata instance property
  • state getter
  • setIntervalLength
  • getIntervalLength
  • startPollingByNetworkClientId
  • stopAllPolling
  • stopPollingByPollingToken
  • onPollingCompleteByNetworkClientId
  • _executePoll (not part of the public API, but something that needs to be implemented)
• detectTokens (or detectNewTokens — whatever we want to call it)

That said, I know I'm sort of changing the purpose of this ticket, but in addition to reviewing the public API, perhaps we should also take this time to audit the behavior between the three implementations as well, and capture a list of differences there. When creating a consolidated version of the controller I imagine we'll want to do some refactoring, so we'll want to make sure we preserve any nuances.

I see you've done this a bit already: you've alluded to the fact that DetectTokensController (extension implementation) keeps track of the locked/unlocked state of the wallet, and calls the method to detect tokens when the wallet is unlocked, and refuses to detect tokens when it's locked.

Besides this, it looks like there may be other slight differences in the constructor in TokenDetectionController vs. DetectTokensController. Specifically, in TokenDetectionController, when TokenListController state changes, the method to detect tokens is called only when the tokens collection is non-empty; but in DetectTokensController, it's always called.

Also, detectTokens in TokenDetectionController is slightly different from detectNewTokens in DetectTokensController:

• Whereas TokenDetectionController caches the selected chain and whether or not it supports token detection, DetectTokensController reads the selected chain every time it's called
• Whereas TokenDetectionController aborts if token detection has been disabled in preferences, DetectTokensController does not abort if token detection has been disabled in preferences but the current chain is mainnet; instead, a static list of tokens is used as the base (so as not to make any additional network requests?).
• Whereas TokenDetectionController calls TokensController.addDetectedTokens with the tokens from TokenListController's tokenList collection which are not in TokensController's tokens collection, not in TokensController's ignoredTokens collection, and have a non-zero balance, DetectTokensController goes one step further and excludes tokens which are already in TokensController's detectedTokens collection.

[mcmire]

Between what you've noted and what I've noted, I imagine we probably have enough to go on, but if you find any other differences feel free to add them here.

I'll let you close this ticket when you feel you have enough information to proceed to #3626.

[MajorLift]

Notes:

• Constructor
  • Wherever DetectTokensController uses restartTokenDetection, replace detectTokens
  • Subscribe to AccountsController:selectedAccountChange event
  • Avoid triggering TokenListController:stateChange for empty tokens list (TokenDetectionController)
• Replace #disabled checks with isActive
• detectTokens:
  • tokens-controller: apply messenger pattern
  • Subscribe to tokens-controller: maintain cache of ignoredTokens, detectedTokens
  • Exclude tokens already in detectedTokens list from detectTokens logic
  • Replace isTokenDetectionInactiveInMainnet is true -> early exit with detectTokens check for #isDetectionEnabledFromPreferences.
  • OR copy logic for generating static mainnet token list into core? reach out to mobile team on importance of handling this case explicitly.
  • Accept _trackMetaMetricsEvent callback in constructor
  • Generate and pass eventTokensDetails
  • try-catch block is handled by _safelyExecute()
• restartTokenDetection
  • triggers detectTokens and resets interval length to default
  • all other logic moved into detectTokens