Replace NetworkController w/ core version #19486
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
mcmire
force-pushed
the
upgrade-assets-controllers
branch
5 times, most recently
from
388c7f0 to
64a1b56
Compare
|
Blocked by MetaMask/core#1421 |
mcmire
force-pushed
the
replace-network-controller
branch
3 times, most recently
from
9599182 to
cf5c74f
Compare
| @@ -208,8 +218,8 @@ export default class DetectTokensController { | |||
| this.interval = DEFAULT_INTERVAL; | |||
| } | |||
|
|
|||
| getChainIdFromNetworkStore(network) { | |||
| return network?.store.getState().providerConfig.chainId; | |||
| getChainIdFromNetworkStore() { | |||
There was a problem hiding this comment.
The argument for this method was removed because the thing it is trying to access is already accessible as a property.
| }, | ||
| getNetwork: () => | ||
| this.networkController.store.getState().networkId ?? 'loading', | ||
| onNetworkStateChange: networkControllerMessenger.subscribe.bind( |
There was a problem hiding this comment.
It seemed that previously we were copying the state but making no other changes to the state. I've replaced this with a direct bind to the stateChange event.
There was a problem hiding this comment.
Looks like this was originally done to work around the need to rename provider to providerConfig (see e0ff039), but I must have missed that it was no longer necessary after that was finally renamed in d1cea85#diff-6fbff2cfe97ac01b77296ef2122c7e0a5b3ff6a84b584b4d1a87482f35eea3d6
mcmire
force-pushed
the
replace-network-controller
branch
from
cf5c74f to
c658295
Compare
|
New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: @json-rpc-specification/meta-schema@1.0.6, @types/jest-when@3.5.2, jest-when@3.5.2 |
mcmire
force-pushed
the
replace-network-controller
branch
3 times, most recently
from
e176f77 to
fc8f266
Compare
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Next stepsTake a deeper look at the dependencyTake 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 packageIf 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 riskTo ignore an alert, reply with a comment starting with |
|
@SocketSecurity ignore @httptoolkit/subscriptions-transport-ws@0.9.19 |
mcmire
force-pushed
the
replace-network-controller
branch
from
d0b3bce to
d5f9bdf
Compare
Builds ready [609712c]
Page Load Metrics (2014 ± 79 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov Report
@@ Coverage Diff @@
## develop #19486 +/- ##
===========================================
- Coverage 70.80% 69.83% -0.97%
===========================================
Files 988 979 -9
Lines 38375 36905 -1470
Branches 10046 9909 -137
===========================================
- Hits 27168 25769 -1399
+ Misses 11207 11136 -71
|
mcmire
force-pushed
the
replace-network-controller
branch
3 times, most recently
from
812db62 to
ff73422
Compare
Builds ready [ff73422]
Page Load Metrics (1512 ± 28 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This commit fulfills a long-standing desire to get the extension using the same network controller as mobile by removing NetworkController from this repo and replacing it with NetworkController from the `@metamask/network-controller` package. The new version of NetworkController is different the old one in a few ways: - The new controller inherits from BaseControllerV2, so the `state` property is used to access the state instead of `store.getState()`. All references of the latter have been replaced with the former. - As the new controller no longer has a `store` property, it cannot be subscribed to; the controller takes a messenger which can be subscribed to instead. There were various places within MetamaskController where the old way of subscribing has been replaced with the new way. In addition, DetectTokensController has been updated to take a messenger object so that it can listen for NetworkController state changes. - The state of the new controller is not updatable from the outside. This affected BackupController, which dumps state from NetworkController (among other controllers), but also loads the same state into NetworkController on import. A method `loadBackup` has been added to NetworkController to facilitate this use case, and BackupController is now using this method instead of attempting to call `update` on NetworkController. - The new controller does not have a `getCurrentChainId` method; instead, the chain ID can be read from the provider config in state. This affected MmiController. (MmiController was also updated to read custom networks from the new network controller instead of the preferences controller). - The default network that the new controller is set to is always Mainnet (previously it could be either localhost or Goerli in test mode, depending on environment variables).
mcmire
force-pushed
the
replace-network-controller
branch
from
ff73422 to
8766ef1
Compare
Builds ready [8766ef1]
Page Load Metrics (1780 ± 75 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mcmire
changed the title
| @@ -31,7 +31,7 @@ export default class BackupController { | |||
| } | |||
|
|
|||
| if (network) { | |||
| this.networkController.store.updateState(network); | |||
There was a problem hiding this comment.
Because the core NetworkController inherits from BaseControllerV2, it doesn't allow consumers to update its state directly like this. I added a loadBackup method to NetworkController to achieve the same thing as this code here: MetaMask/core#1421
Builds ready [99a2a9b]
Page Load Metrics (1533 ± 66 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-10.34.0
Explanation
This commit fulfills a long-standing desire to get the extension using the same network controller as mobile by removing NetworkController from this repo and replacing it with NetworkController from the
@metamask/network-controllerpackage.The new version of NetworkController is different the old one in a few ways:
stateproperty is used to access the state instead ofstore.getState(). All references of the latter have been replaced with the former.storeproperty, it cannot be subscribed to; the controller takes a messenger which can be subscribed to instead. There were various places within MetamaskController where the old way of subscribing has been replaced with the new way. In addition, DetectTokensController has been updated to take a messenger object so that it can listen for NetworkController state changes.loadBackuphas been added to NetworkController to facilitate this use case, and BackupController is now using this method instead of attempting to callupdateon NetworkController.getCurrentChainIdmethod; instead, the chain ID can be read from the provider config in state. This affected MmiController. (MmiController was also updated to read custom networks from the new network controller instead of the preferences controller).The default network that the new controller is set to is always Mainnet (previously it could be either localhost or Goerli in test mode, depending on environment variables).Technically this is true from thecoreperspective, but I've brought back the old logic so there should be no functional difference on this side of things.Fixes #17931.
Manual Testing Steps
The new NetworkController should be functionally equivalent to the old one, so there should be no user-facing changes. That said, it would probably worth testing anything that involves the network (which, I know, is basically everything) to ensure that it works as before.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.