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
| * However Custom RPC endpoints, that are synchronized between devices, | ||
| * can contain a `networkClientId` set on both devices. |
There was a problem hiding this comment.
For custom RPCs, the networkClientId is a random UUID.
To make it easier for syncing, if a device is adding new networks from remote, we can reuse the UUID.
This was there is less chance of having some weird state where 2 devices use the same networks + RPCs, but they are different UUIDs.
There was a problem hiding this comment.
This is mostly optional (to keep uuid eventual consistency) across devices.
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.
| @@ -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! |
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