NetworkController: Remove providerConfigChange event #1329
Conversation
Despite its name, the `providerConfigChange` event in NetworkController
doesn't get emitted whenever the `providerConfig` property in state is
changed. Rather, it gets updated as the very last step in
`lookupNetwork`. This method is called:
- When the provider is initialized
- When the network is switched
- When the network is rolled back to the previous setting
- When `resetConnection` is called
`lookupNetwork` can also be called on its own.
If this were not enough, we already provide a way to listen for changes
to a particular property in state: by passing a selector function to the
`subscribe` method on the controller messenger.
So, this commit removes the `providerConfigChange` event from
NetworkController. Instead of the following code:
```
messenger.subscribe(
'NetworkController:providerConfigChange',
(providerConfig) => {
// do something with the providerConfig
},
)
```
consumers should be able to say:
```
messenger.subscribe(
'NetworkController:stateChange',
(providerConfig) => {
// do something with the providerConfig
},
(networkControllerState) => networkControllerState.providerConfig,
)
```
mcmire
force-pushed
the
remove-provider-config-change-event
branch
from
14d2598
to
c11fb0a
Compare
| ); | ||
| } | ||
| onNetworkStateChange(async (networkControllerState) => { | ||
| await this.#onNetworkControllerStateChange(networkControllerState); |
There was a problem hiding this comment.
We can simulate a network controller state object in tests, so we don't need to make this check.
| this.currentChainId = newProviderConfig.chainId; | ||
| await this.resetPolling(); | ||
| } | ||
| 'NetworkController:stateChange', |
There was a problem hiding this comment.
Note that we have a similar pattern of using onNetworkStateChange or subscribing to an event through the messenger in TokenListController, so I made them match each other.
| this.EIP1559APIEndpoint = EIP1559APIEndpoint; | ||
| this.legacyAPIEndpoint = legacyAPIEndpoint; | ||
| this.clientId = clientId; | ||
|
|
||
| this.ethQuery = new EthQuery(this.#getProvider()); |
There was a problem hiding this comment.
We don't need to call getProvider only if onNetworkStateChange and getChainId are given; this is a required option, so we can always get the provider this way. So I chose to use getProvider in the state change callback below to refresh the EthQuery instance. However, if we want to continue using the getEthQuery action we can — I just didn't know if we planned on getting rid of that or not.
There was a problem hiding this comment.
LGTM! I was concerned this would affect more controllers, but 2 is not bad. Nice that the gas fee controller and token list controller network change handling is now more concise and standardized.
I would suggest that the PR description be updated to include a note on the gas fee controller though; that change to the restricted messenger type is a breaking change as well.
|
PR description updated and conflicts resolved. |
There was a problem hiding this comment.
LGTM!
Though for:
BREAKING: [@metamask/assets-controllers] Update type of messenger option for TokenListController to remove NetworkController:providerConfigChange from the set of allowed events
We should mention that the NetworkController:stateChange event is now required as well, since that's the breaking part. Removing an event is non-breaking.
Despite its name, the `providerConfigChange` event in NetworkController
doesn't get emitted whenever the `providerConfig` property in state is
changed. Rather, it gets updated as the very last step in
`lookupNetwork`. This method is called:
- When the provider is initialized
- When the network is switched
- When the network is rolled back to the previous setting
- When `resetConnection` is called
`lookupNetwork` can also be called on its own.
If this were not enough, we already provide a way to listen for changes
to a particular property in state: by passing a selector function to the
`subscribe` method on the controller messenger.
So, this commit removes the `providerConfigChange` event from
NetworkController. Instead of the following code:
```
messenger.subscribe(
'NetworkController:providerConfigChange',
(providerConfig) => {
// do something with the providerConfig
},
)
```
consumers should be able to say:
```
messenger.subscribe(
'NetworkController:stateChange',
(providerConfig) => {
// do something with the providerConfig
},
(networkControllerState) => networkControllerState.providerConfig,
)
```
Despite its name, the `providerConfigChange` event in NetworkController
doesn't get emitted whenever the `providerConfig` property in state is
changed. Rather, it gets updated as the very last step in
`lookupNetwork`. This method is called:
- When the provider is initialized
- When the network is switched
- When the network is rolled back to the previous setting
- When `resetConnection` is called
`lookupNetwork` can also be called on its own.
If this were not enough, we already provide a way to listen for changes
to a particular property in state: by passing a selector function to the
`subscribe` method on the controller messenger.
So, this commit removes the `providerConfigChange` event from
NetworkController. Instead of the following code:
```
messenger.subscribe(
'NetworkController:providerConfigChange',
(providerConfig) => {
// do something with the providerConfig
},
)
```
consumers should be able to say:
```
messenger.subscribe(
'NetworkController:stateChange',
(providerConfig) => {
// do something with the providerConfig
},
(networkControllerState) => networkControllerState.providerConfig,
)
```
Description
Despite its name, the
providerConfigChangeevent in NetworkController doesn't get emitted whenever theproviderConfigproperty in state is changed. Rather, it gets updated as the very last step inlookupNetwork. This method is called:resetConnectionis calledlookupNetworkcan also be called on its own.If this were not enough, we already provide a way to listen for changes to a particular property in state: by passing a selector function to the
subscribemethod on the controller messenger.So, this commit removes the
providerConfigChangeevent from NetworkController. Instead of the following code:consumers should be able to say:
Changes
@metamask/network-controller] UpdateinitializeProvider,lookupNetwork,setProviderType,setActiveNetwork,rollbackToPreviousProvider, andresetConnectionso thatNetworkController:providerConfigChangeis no longer emitted; remove this event entirely from the set of allowed events along with theNetworkControllerProviderConfigChangeEventtypeNetworkController:stateChangewith a selector function that returnsproviderConfigif they want to perform an action whenproviderConfigchanges.@metamask/network-controller] AddNetworkController:getStateto the set of allowed messenger actions, along with aNetworkControllerGetStateActiontype@metamask/assets-controllers] Update signature ofonNetworkStateChangecallback for TokenListController so that it must be given a NetworkController state change object; it will no longer also accept a provider config object@metamask/assets-controllers] Update type of TokenListController messenger to requireNetworkController:stateChangeas an allowed event@metamask/gas-fee-controller] Update type of GasFeeMessenger to requireNetworkController:getStateas an allowed eventReferences
Fixes #1205.
Checklist