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

Upgrade TokenDetectionController to extend BaseControllerV2, StaticIntervalPollingController #3609

Merged
merged 95 commits into from
Dec 22, 2023
Merged

Upgrade TokenDetectionController to extend BaseControllerV2, StaticIntervalPollingController #3609

Show file tree
Hide file tree
Changes from 84 commits
Commits
Show all changes
95 commits
Select commit Hold shift + click to select a range
d595d0f
Define types for BaseControllerV2 migration
MajorLift Dec 1, 2023
56ca692
Upgrade `TokenDetectionController` to extend from `PollingController`
MajorLift Dec 1, 2023
25ae501
Replace event handlers with controller-messenger pattern
MajorLift Dec 1, 2023
d42cc0f
[token-list-controller] Export `TokenListControllerMessenger` and def…
MajorLift Dec 7, 2023
0a66c50
Explicitly enumerate package-level exports
MajorLift Dec 7, 2023
1b5b635
Move `TokenDetectionConfig` properties to private class fields in `To…
MajorLift Dec 7, 2023
94448cd
Define `controllerName`, `TokenDetectionState` as empty object, remov…
MajorLift Dec 7, 2023
bcfa0a1
Fix: unregister internal `getState` action handler in constructor as …
MajorLift Dec 7, 2023
b8dcf32
Fix `sliceOfTokensToDetect` logic to handle arbitrary number of `toke…
MajorLift Dec 7, 2023
a5ef650
Various fixes for controller upgrade
MajorLift Dec 8, 2023
72b3618
Adapt tests to align with upgraded `TokenDetectionController` API
MajorLift Dec 8, 2023
2f74472
Fix test to avoid using deprecated aurora network
MajorLift Dec 11, 2023
720dc69
Assume that `getTokensState` never returns `undefined`
MajorLift Dec 11, 2023
74f74ba
Remove unnecessary cleanup function calls
MajorLift Dec 11, 2023
7b3d4e4
Add jsdoc for test utility functions
MajorLift Dec 11, 2023
2bef9b8
Fix unnecessary `await`
MajorLift Dec 11, 2023
9c8491a
Use new network-controller action/event
MajorLift Dec 11, 2023
05ed86b
Fix tests
MajorLift Dec 11, 2023
8964e0d
Move `unregisterActionhandler` calls for `TokenDetectionController:ge…
MajorLift Dec 11, 2023
26509c1
Remove `disabled` class field entirely
MajorLift Dec 11, 2023
cb2bd08
Remove unnecessary `tokenDetect.start()` calls from tests
MajorLift Dec 11, 2023
9244803
Record CHANGELOG entries
MajorLift Dec 12, 2023
1274137
Reorder constructor assignments
MajorLift Dec 12, 2023
0c0476c
Await all `detectTokens()` calls
MajorLift Dec 12, 2023
8dc1fcd
Test fixes
MajorLift Dec 12, 2023
bb43ebb
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 12, 2023
fdb70d9
Add `deferPollingStart` property
MajorLift Dec 12, 2023
98e8d17
Fix test
MajorLift Dec 12, 2023
8ad12b5
Fix test to avoid implicitly calling `detectTokens()`
MajorLift Dec 12, 2023
5e94d13
Fix `#getCorrectChainId` and polygon network test to pull network con…
MajorLift Dec 12, 2023
a5ebc52
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 12, 2023
a2007f6
Update jest coverage thresholds
MajorLift Dec 12, 2023
3532bbc
Remove `deferPollingStart` constructor option
MajorLift Dec 12, 2023
6601dcc
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 12, 2023
322d074
Add controller types to package-level exports
MajorLift Dec 12, 2023
b60ada5
Update CHANGELOG records
MajorLift Dec 12, 2023
599da4d
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 12, 2023
6c22a3a
Update jest coverage thresholds
MajorLift Dec 12, 2023
a26d568
Mock action handler return value instead of registering multiple hand…
MajorLift Dec 13, 2023
85e3f10
Remove unregister action handler call in constructor and define `befo…
MajorLift Dec 13, 2023
e1a061a
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 13, 2023
b9cbcba
Revert "Fix `sliceOfTokensToDetect` logic to handle arbitrary number …
MajorLift Dec 13, 2023
4322102
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 13, 2023
fb7cbe2
set polygon network as rpc in test
MajorLift Dec 14, 2023
5d602a3
Fix test so that `preferencesStateChangeListener` triggers `detectTok…
MajorLift Dec 14, 2023
a41a218
Add `#networkClientId` class field and update using `seletedNetworkCl…
MajorLift Dec 14, 2023
7e52c7c
Update CHANGELOG to reflect 'Revert "Fix `sliceOfTokensToDetect` logi…
MajorLift Dec 14, 2023
e7dac31
Update jsdoc for class fields
MajorLift Dec 14, 2023
15f8922
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 14, 2023
15ea19c
Fix mistaken assignment of chainId to networkClientId property
MajorLift Dec 14, 2023
3e1416d
Revert "Add `#networkClientId` class field and update using `seletedN…
MajorLift Dec 14, 2023
123aff5
Revert "Fix test so that `preferencesStateChangeListener` triggers `d…
MajorLift Dec 14, 2023
7f2e58b
Fix test so that `preferencesStateChangeListener` triggers `detectTok…
MajorLift Dec 14, 2023
d32cdf4
Add `#networkClientId` class field and update using `seletedNetworkCl…
MajorLift Dec 14, 2023
9167cac
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 14, 2023
71d9967
Fix test by publishing `networkDidChange` event in `changeNetwork` he…
MajorLift Dec 14, 2023
0dda603
Add `#disabled` class field to ensure that passive token detection is…
MajorLift Dec 14, 2023
c5eec25
Adjust jest coverage thresholds
MajorLift Dec 14, 2023
1337d19
Update packages/assets-controllers/src/TokenDetectionController.test.ts
MajorLift Dec 15, 2023
41f85a1
Update packages/assets-controllers/CHANGELOG.md
MajorLift Dec 15, 2023
8c331fa
Edit test descriptions so that `getBalancesInSingleMock` is subject
MajorLift Dec 15, 2023
d40247e
Fix incorrect package-level export
MajorLift Dec 15, 2023
757ab04
[tokens-controller] Add actions `getState`, `addDetectedTokens` and e…
MajorLift Dec 15, 2023
44e92cb
Replace `getTokensState`, `addDetectedTokens` callbacks with messenge…
MajorLift Dec 15, 2023
4b0bda3
Make `detectTokens` take empty options object as default argument
MajorLift Dec 15, 2023
a007d15
Remove assignment to `#chainId` in `#getCorrectChainId`
MajorLift Dec 15, 2023
f5e6611
Edit test description
MajorLift Dec 15, 2023
b2a5af5
Update changelog
MajorLift Dec 15, 2023
eadf3ba
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 15, 2023
aa9e3ba
Adjust jest coverage thresholds
MajorLift Dec 15, 2023
f636e25
Revert "Fix incorrect package-level export"
MajorLift Dec 15, 2023
173168f
Use `getNetworkClientById` action in `#getCorrectChainId` method
MajorLift Dec 16, 2023
d81c23d
Replace `chainId` with `networkClientId` in constructor options.
MajorLift Dec 16, 2023
d9bd4b1
Remove `getNetworkConfigurationByClientId` handler from tests
MajorLift Dec 18, 2023
fefc64b
test: adjust `getNetworkClientById` handler to return polygon chainId
MajorLift Dec 18, 2023
73eccbf
test: remove unnecessary `.start()` calls
MajorLift Dec 18, 2023
5fe7674
Add `disabled` as constructor option and set it to block `#startPolli…
MajorLift Dec 18, 2023
116a227
Adjust jest coverage thresholds
MajorLift Dec 18, 2023
acd022f
Remove unused comment
MajorLift Dec 19, 2023
ee2154d
test: Restore publishes for `networkDidChange` event, mock polygon ne…
MajorLift Dec 20, 2023
cfbd787
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 20, 2023
21f9b8c
Adjust jest coverage thresholds
MajorLift Dec 20, 2023
de94761
Revert "[tokens-controller] Add actions `getState`, `addDetectedToken…
MajorLift Dec 20, 2023
4b6c1d2
test: Apply 'Revert "[tokens-controller] Add actions `getState`, `add…
MajorLift Dec 20, 2023
a951492
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 21, 2023
541f8f4
Adjust jest coverage threshold
MajorLift Dec 21, 2023
798bb09
Use `getNetworkConfigurationByNetworkClientId` to replace `getNetwork…
MajorLift Dec 21, 2023
1552041
Remove throw error in `_executePoll`
MajorLift Dec 21, 2023
2451982
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 21, 2023
6d020bc
Adjust jest coverage threshold
MajorLift Dec 21, 2023
05cf2bd
Set `disabled` to true by default and add `enable`, `disable` methods
MajorLift Dec 21, 2023
d8ced01
Revert "test: remove unnecessary `.start()` calls"
MajorLift Dec 21, 2023
16e8bad
test: Adjust tests to `disabled` being true by default
MajorLift Dec 21, 2023
f67bead
Update changelog
MajorLift Dec 21, 2023
8e1cdf5
Merge branch 'main' into 231201-token-detection-controller-basecontro…
MajorLift Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/assets-controllers/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Added
- `TokenListController` now exports a `TokenListControllerMessenger` type ([#3609](https://github.com/MetaMask/core/pull/3609)).
- `TokenDetectionController` exports types `TokenDetectionControllerMessenger`, `TokenDetectionControllerActions`, `TokenDetectionControllerGetStateAction`, `TokenDetectionControllerEvents`, `TokenDetectionControllerStateChangeEvent` ([#3609](https://github.com/MetaMask/core/pull/3609)).

### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Which option was this?

Copy link
Contributor Author

@MajorLift MajorLift Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MajorLift MajorLift Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Looks reasonable to me!

Copy link
Member

Choose a reason for hiding this comment

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

Could use additional tests for those two new methods though


## [22.0.0]
### Changed
Expand Down
8 changes: 4 additions & 4 deletions packages/assets-controllers/jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 88.2,
functions: 95.95,
lines: 96.25,
statements: 96.5,
branches: 88.8,
functions: 97.08,
lines: 97.23,
statements: 97.28,
},
},

Expand Down
Loading
Loading