Update SelectedNetworkController to use chainId #4670
Conversation
| } | ||
| } | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| // TODO also monitor the defaultRpcEndpoint for a chainId changing |
There was a problem hiding this comment.
Is this something that needs to be done? Feels like the only thing that needs to be reacted to is the whole network/chain being deleted.
There was a problem hiding this comment.
If moving to chain ids is too much effort in the short term, a similar effect can be achieved by redirecting domains to the default endpoint when it changes: #4679
There was a problem hiding this comment.
Hey @bergeron yeah Jiexi and I strategized around this and decided to pursue both options.
"redirecting domains to the default endpoint when it changes" per your and my conversation at first - in case the work in this PR takes a bit longer to merge.
There was a problem hiding this comment.
Oh but I'm just seeing your PR #4679. We were thinking we could add this logic using the SelectedNetworkController's public API in the extension client in your PR first
There was a problem hiding this comment.
in the extension
Do you have more precise idea for how this would work?
Here's the 2 reasons I implemented it via #4679 instead of through the extension.
(1) The SelectedNetworkController will need changes either way, since the current NetworkController:stateChange subscriber doesn't do what we want. Deleting a single endpoint currently gets it defaulted to the global network client instead of the network's default. So we'll be editing that subscriber anyway.
(2) This case where you both delete the default endpoint, and replace it with a new default in 1 operation to updateNetwork:
Screen.Recording.2024-09-10.at.8.12.19.AM.mov
The extension doesn't know the new endpoint's network client id until the controller autogenerates it. So domain edits have to occur in response to the state update, and the SelectedNetworkController's seemed most convenient as it has everything to handle both removes and edits in 1 place.
There was a problem hiding this comment.
(1) The SelectedNetworkController will need changes either way, since the current NetworkController:stateChange subscriber doesn't do what we want. Deleting a single endpoint currently gets it defaulted to the global network client instead of the network's default. So we'll be editing that subscriber anyway.
I suppose this does make it tricky. I was thinking we could subscribe to this event in the MetaMask-Controller and call setNetworkClientDomain using the SelectedNetworkController's public API (similar pattern to what we do in several ways here) But if we're competing with the subscription already in the controller we could have some funky race conditions...
|
@metamask-bot publish-preview |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| chainIdForDomain === patch.path[1] | ||
| ) { | ||
| // If the network was updated, redirect to the network's default endpoint | ||
| this.setChainIdForDomain(domain, chainIdForDomain); |
There was a problem hiding this comment.
We don't have a good way to inspect what rpc endpoint the network client proxy for the domain is currently pointed at, so we repoint the network client proxy for the domain at the default network client for the chainId any time the network configuration for that chainId changes
| const originNetworkClientId = this.messagingSystem.call( | ||
| 'SelectedNetworkController:getNetworkClientIdForDomain', | ||
| const originChainId = this.messagingSystem.call( | ||
| 'SelectedNetworkController:getChainIdForDomain', | ||
| this.#originOfCurrentBatch, | ||
| ); | ||
| const originNetworkClientId = this.messagingSystem.call( | ||
| 'NetworkController:getDefaultNetworkClientIdForChainId', | ||
| originChainId, | ||
| ); |
There was a problem hiding this comment.
Is this the bit you were saying was awkward? Yeah I see that. Not a big deal though. And we're going to be removing the need for these calls altogether very soon!
Explanation
DRAFT
References
Changelog
@metamask/package-a@metamask/package-bChecklist