Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` #3609

MajorLift · 2023-12-01T20:30:41Z

Explanation

This upgrades TokenDetectionController to extend BaseControllerV2 and StaticIntervalPollingController as a preparation step for merging TokenDetectionController with DetectTokensController.

References

Changelog

Added

TokenListController now exports a TokenListControllerMessenger type (#3609).
TokenDetectionController exports types TokenDetectionControllerMessenger, TokenDetectionControllerActions, TokenDetectionControllerGetStateAction, TokenDetectionControllerEvents, TokenDetectionControllerStateChangeEvent (#3609).
Add enable and disable methods to TokenDetectionController, which control whether the controller is able to make polling requests or all of its network calls are blocked. (#3609).
- Note that if the controller is initiated without the disabled constructor option set to false, the enable method will need to be called before the controller can make polling requests in response to subscribed events.

Changed

BREAKING: TokenDetectionController is upgraded to extend BaseControllerV2 and StaticIntervalPollingController (#3609).
- The constructor now expects an options object as its only argument, with required properties messenger, networkClientId, required callbacks onPreferencesStateChange, getBalancesInSingleCall, addDetectedTokens, getTokenState, getPreferencesState, and optional properties disabled, interval, selectedAddress.

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

- Add types`TokenDetectionState`, `TokenDetectionControllerGetStateAction`, `TokenDetectionControllerActions`, `TokenDetectionControllerStateChangeEvent`, `TokenDetectionControllerEvents`, `TokenDetectionControllerMessenger`

- Remove methods that can be replaced with actions/events - Replace references to `this.config` with `this.state` - Replace `this.configure()` calls with `this.update()`

…ine `AllowedEvents`

…kenDetectionController`

…e `getNetworkState`

…twork client

…llerv2-migration

…s` and event `stateChange`" This reverts commit 757ab04.

…DetectedTokens` and event `stateChange`"

MajorLift · 2023-12-20T16:46:00Z

I extracted the tokens-controller changes to a new PR: #3690.

I left the token list controller-messenger export since it's a small, isolated change. (related: should token-list-controller be subscribed to networkDidChange now?)

All tests are passing on this PR now with all suggestions reflected. I think it's ready for a final pass review!

mcmire

Just a couple more comments, but I'm otherwise good with this.

packages/assets-controllers/src/TokenDetectionController.ts

mcmire · 2023-12-21T05:25:20Z

packages/assets-controllers/CHANGELOG.md

+### Changed
+- **BREAKING:** `TokenDetectionController` is upgraded to extend `BaseControllerV2` and `StaticIntervalPollingController` ([#3609](https://github.com/MetaMask/core/pull/3609)).
+  - The constructor now expects an options object as its only argument, with required properties `messenger`, `networkClientId`, required callbacks `onPreferencesStateChange`, `getBalancesInSingleCall`, `getPreferencesState`, and optional properties `interval`, `selectedAddress`. Note that the `config` object is no longer used by the constructor.
+  - Polling can only be initiated by calling `start` or `startPollingByNetworkClientId` from a controller client. There is no longer an option to automatically start polling when the controller is instantiated.


Which option was this?

Ah, I used "polling" here to include passively calling detectTokens (i.e. executePoll), which is definitely confusing. Also, the disabled option was previously true by default, which makes the current API a breaking change. Thoughts on the following?

Suggested change

- Polling can only be initiated by calling `start` or `startPollingByNetworkClientId` from a controller client. There is no longer an option to automatically start polling when the controller is instantiated.

- **BREAKING:** By default, the controller may now passively call the `detectTokens` method in response to subscribed events. To disable all network requests, initialize the controller with the `disabled` option set to true, or call the `stop` method.

I guess whether to keep this change would depend on the expected behavior of polling controllers upon initialization. If we're going to use a dedicated service for network calls and let it handle blocking outgoing requests, perhaps it makes sense to enable passive updates in the polling controller itself?

But if this is disruptive to the current implementation I can revert the default value for disabled. It will require some changes in the tests, probably including restoring these start calls: 73eccbf.

I'd prefer that we keep the current default, and consider changing this independently even if we do want to. Better to minimize scope of we can.

One issue with doing that is that the new startPollingByNetworkClientId method is now blocked by default in this controller, and requires the disabled flag to be flipped before it can work.

Making the #disabled class field public or adding a setter method seems unsafe and not in keeping with the idea of having this be controlled by a network service,

requiring clients or the tests to reinstantiate the entire controller with disabled: false prior to making startPollingByNetworkClientId calls seems like overkill and bad design,

requiring start to be called before startPollingByNetworkClientId calls seems redundant and unintuitive,

and none of the v2 polling controllers seem to have a disabled property, so lifting the disabled property itself to StaticIntervalPollingController would require more changes that are out of scope.

I'm not sure how to proceed with this off the top of my head. I'll give it some more thought.

I see what you mean, this is a bit tricky. I guess this dilemma was introduced with these new polling functions, but made worse with this change because runtime config modification is no longer possible.

For the CurrencyRateController, the DevEx team decided to deal with this by calling stopAllPolling when the UI closes. So they didn't need to use the disabled state for the newer polling API. But this is problematic because now those polls won't be going anymore when the UI re-opens, even if they might still be needed.

The easiest solution would be to make it possible to update disabled at runtime. I don't know what you meant about that seeming unsafe, nor do I understand what you're referring to by "not in keeping with the idea of having this be controlled by a network service".

Longer-term I'd like us to take these services out of the controller altogether, so that there is never a need to disable the controller. @mcmire and I have also discussed adding a "pause" method to the polling controller, so that they can be paused and unpaused without being deregistered (this idea was from before the service refactor was discussed, I am unsure if we still need this). But both of those ideas seem fairly substantial, I am guessing that we'd be better off trying to land this first, now that this work is already done.

I was referring to the longer-term solution you mentioned, but I can see that trying to anticipate those changes is premature at this point.

I added a disable and enable method for now: 05cf2bd. Does this seem like a good approach?

Looks reasonable to me!

Could use additional tests for those two new methods though

…llerv2-migration

packages/assets-controllers/src/TokenDetectionController.ts

…ClientById`

…llerv2-migration

This reverts commit 73eccbf.

…llerv2-migration

Gudahtt

LGTM!

Some missing tests still, but to get this test suite in great shape would require a lot of work, so I'm not sure how high a bar we want to set at this time. I'll leave that for you to decide

mcmire

Great job keeping up with all of the requests 😅 This looks great to me!

MajorLift · 2023-12-22T19:05:03Z

Note on tests for disable and enable:
The last four tests cover the cases disabled: false and disabled: true -> enable().
TODO: add two tests for disabled: false -> disable() -> enable() with active and passive polls in between.

* origin/main: Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` (#3609)

MajorLift added enhancement New feature or request team-wallet-framework labels Dec 1, 2023

MajorLift self-assigned this Dec 1, 2023

MajorLift force-pushed the 231201-token-detection-controller-basecontrollerv2-migration branch 9 times, most recently from dd7ea6c to 4992b15 Compare December 7, 2023 16:24

MajorLift marked this pull request as ready for review December 7, 2023 16:31

MajorLift requested a review from a team as a code owner December 7, 2023 16:31

MajorLift force-pushed the 231201-token-detection-controller-basecontrollerv2-migration branch 9 times, most recently from 64c5308 to b039535 Compare December 11, 2023 16:51

MajorLift added 7 commits December 11, 2023 08:51

Define types for BaseControllerV2 migration

d595d0f

- Add types`TokenDetectionState`, `TokenDetectionControllerGetStateAction`, `TokenDetectionControllerActions`, `TokenDetectionControllerStateChangeEvent`, `TokenDetectionControllerEvents`, `TokenDetectionControllerMessenger`

Upgrade TokenDetectionController to extend from PollingController

56ca692

- Remove methods that can be replaced with actions/events - Replace references to `this.config` with `this.state` - Replace `this.configure()` calls with `this.update()`

Replace event handlers with controller-messenger pattern

25ae501

[token-list-controller] Export TokenListControllerMessenger and def…

d42cc0f

…ine `AllowedEvents`

Explicitly enumerate package-level exports

0a66c50

Move TokenDetectionConfig properties to private class fields in `To…

1b5b635

…kenDetectionController`

Define controllerName, TokenDetectionState as empty object, remov…

94448cd

…e `getNetworkState`

MajorLift force-pushed the 231201-token-detection-controller-basecontrollerv2-migration branch from 097bcd4 to acd022f Compare December 20, 2023 15:24

MajorLift added 5 commits December 20, 2023 07:47

test: Restore publishes for networkDidChange event, mock polygon ne…

ee2154d

…twork client

Merge branch 'main' into 231201-token-detection-controller-basecontro…

cfbd787

…llerv2-migration

Adjust jest coverage thresholds

21f9b8c

Revert "[tokens-controller] Add actions getState, `addDetectedToken…

de94761

…s` and event `stateChange`" This reverts commit 757ab04.

test: Apply 'Revert "[tokens-controller] Add actions getState, `add…

4b6c1d2

…DetectedTokens` and event `stateChange`"

MajorLift requested review from mcmire and Gudahtt December 20, 2023 17:24

mcmire reviewed Dec 21, 2023

View reviewed changes

MajorLift added 2 commits December 20, 2023 22:02

Merge branch 'main' into 231201-token-detection-controller-basecontro…

a951492

…llerv2-migration

Adjust jest coverage threshold

541f8f4

Gudahtt reviewed Dec 21, 2023

View reviewed changes

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

MajorLift added 9 commits December 21, 2023 09:51

Use getNetworkConfigurationByNetworkClientId to replace `getNetwork…

798bb09

…ClientById`

Remove throw error in _executePoll

1552041

Merge branch 'main' into 231201-token-detection-controller-basecontro…

2451982

…llerv2-migration

Adjust jest coverage threshold

6d020bc

Set disabled to true by default and add enable, disable methods

05cf2bd

Revert "test: remove unnecessary .start() calls"

d8ced01

This reverts commit 73eccbf.

test: Adjust tests to disabled being true by default

16e8bad

Update changelog

f67bead

Merge branch 'main' into 231201-token-detection-controller-basecontro…

8e1cdf5

…llerv2-migration

Gudahtt approved these changes Dec 21, 2023

View reviewed changes

mcmire approved these changes Dec 22, 2023

View reviewed changes

MajorLift merged commit 247890e into main Dec 22, 2023
136 checks passed

MajorLift deleted the 231201-token-detection-controller-basecontrollerv2-migration branch December 22, 2023 19:05

Gudahtt added a commit that referenced this pull request Dec 22, 2023

Merge remote-tracking branch 'origin/main' into release/104.0.0

59c2f80

* origin/main: Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` (#3609)

Gudahtt added a commit that referenced this pull request Dec 22, 2023

Incorporate changelogs from #3609

9b3f3f5

MajorLift mentioned this pull request Oct 3, 2024

refactor: Typescript conversion of log-web3-shim-usage.js MetaMask/metamask-extension#23732

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` #3609

Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` #3609

MajorLift commented Dec 1, 2023 •

edited

Loading

MajorLift commented Dec 20, 2023 •

edited

Loading

mcmire left a comment

mcmire Dec 21, 2023

MajorLift Dec 21, 2023 •

edited

Loading

Gudahtt Dec 21, 2023

MajorLift Dec 21, 2023 •

edited

Loading

Gudahtt Dec 21, 2023

MajorLift Dec 21, 2023

Gudahtt Dec 21, 2023

Gudahtt Dec 21, 2023

Gudahtt left a comment

mcmire left a comment

MajorLift commented Dec 22, 2023

	- Polling can only be initiated by calling `start` or `startPollingByNetworkClientId` from a controller client. There is no longer an option to automatically start polling when the controller is instantiated.
	- BREAKING: By default, the controller may now passively call the `detectTokens` method in response to subscribed events. To disable all network requests, initialize the controller with the `disabled` option set to true, or call the `stop` method.

Upgrade TokenDetectionController to extend BaseControllerV2, StaticIntervalPollingController #3609

Upgrade TokenDetectionController to extend BaseControllerV2, StaticIntervalPollingController #3609

Conversation

MajorLift commented Dec 1, 2023 • edited Loading

Explanation

References

Changelog

Added

Changed

Checklist

MajorLift commented Dec 20, 2023 • edited Loading

mcmire left a comment

Choose a reason for hiding this comment

mcmire Dec 21, 2023

Choose a reason for hiding this comment

MajorLift Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Dec 21, 2023

Choose a reason for hiding this comment

MajorLift Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Dec 21, 2023

Choose a reason for hiding this comment

MajorLift Dec 21, 2023

Choose a reason for hiding this comment

Gudahtt Dec 21, 2023

Choose a reason for hiding this comment

Gudahtt Dec 21, 2023

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

MajorLift commented Dec 22, 2023

Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` #3609

Upgrade `TokenDetectionController` to extend `BaseControllerV2`, `StaticIntervalPollingController` #3609

MajorLift commented Dec 1, 2023 •

edited

Loading

MajorLift commented Dec 20, 2023 •

edited

Loading

MajorLift Dec 21, 2023 •

edited

Loading

MajorLift Dec 21, 2023 •

edited

Loading