feat: NetworkController changes to support Network Syncing #4939
Conversation
this is to support network syncing, where we add a new network to a synced device
Prithpal-Sooriya
changed the title
Prithpal-Sooriya
force-pushed
the
feat/add-network-controller-changes-to-support-network-syncing
branch
from
417566b
to
6e65961
Compare
…rkConfiguration this is used for network syncing to override local state with the remove state.
| * This will subsequently update the network client registry; state.networksMetadata, and state.selectedNetworkClientId | ||
| * @param networkConfiguration - the network configuration to override | ||
| */ | ||
| async dangerouslySetNetworkConfiguration( |
There was a problem hiding this comment.
Yep, I understand that this is a lot of logic here. I've ensured that we correctly update controller state; as well as internal data structures used inside the controller.
Fundamentally we need this method as we are unable to sync/override networks due to the random (uuidV4()) networkClientIds assigned to different devices (hence we were not able to use the existing updatedNetwork method).
There was a problem hiding this comment.
The name for this is a bit of a throwback to Reacts dangerouslySetInnerHTML. I want to signify to developers that we should not use this method, and we need to be very cautious when using this method.
There was a problem hiding this comment.
@Prithpal-Sooriya Should this method be private then? Is this one used programmatically in the extension via this.networkController.dangerouslySetNetworkConfiguration or only called through the messaging system?
There was a problem hiding this comment.
I will need to investigate, can private methods be used in the messaging system.
There was a problem hiding this comment.
Updated this method to private as it is only accessible through the messaging system - good callout!
| @@ -11410,6 +11436,205 @@ describe('NetworkController', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('dangerouslySetNetworkConfiguration', () => { | |||
There was a problem hiding this comment.
This test file is large. Understandably since the network controller is responsible for MANY things and logic.
I've tried to keep these tests isolated and lean.
…-controller-changes-to-support-network-syncing
|
Hello! I have some concerns here and would definitely love to understand the use case for the new method you're adding. As you've mentioned elsewhere, there is a lot going on already in the network controller and I just want to make sure that we absolutely need a different way of updating a network configuration. |
|
@mcmire yep I agree, this is a very big change. As mentioned in the PR description, I'm planning on having a 1:1 with the assets team and also include a code walkthrough. Happy to include you and discuss in DMs the context for this change. |
|
@Prithpal-Sooriya That would be great, thanks! |
| * This will subsequently update the network client registry; state.networksMetadata, and state.selectedNetworkClientId | ||
| * @param networkConfiguration - the network configuration to override | ||
| */ | ||
| async dangerouslySetNetworkConfiguration( |
There was a problem hiding this comment.
@Prithpal-Sooriya Should this method be private then? Is this one used programmatically in the extension via this.networkController.dangerouslySetNetworkConfiguration or only called through the messaging system?
This is now only accessible through the messaging systems
…-controller-changes-to-support-network-syncing
|
@Gudahtt yep sure, will add an ADR. I'm not planning on merging this work until we get team-wide acceptance. |
|
@mcmire @Gudahtt This PR can actually be closed. We have managed to get the underlying feature (network syncing) to work using the existing I will close this PR, and append #4698 to expose the So the only PRs that touch the network controller are: |
Explanation
A set of changes to support network syncing. This is a big scary change, so will have a 1:1 with assets team and pair review this.
I'll also record a loom vid for a quick code walkthrough.
References
N/A
Changelog
@metamask/network-controllernetworkClientIdif provided instead ofuuidV4()dangerouslySetNetworkConfigurationto cautiously overwrite a given network configuration. Used for network syncing, where we need to override local state with the synced remote state.Checklist