Create and expose collection of network clients #1439

mcmire · 2023-06-20T23:53:06Z

Explanation

In order for users to make requests to multiple networks simultaneously, we need to phase out the concept of a single active network in the network controller in favor of a collection of networks (active or otherwise). This commit is the first step in making this happen.

To this end, this commit:

Adds the concept of an "auto-managed network client". This is an object that has the same exact interface as a network client — that is, it exposes a provider and block tracker — except that the network client doesn't spring into existence until an attempt is made to use the provider or block tracker in some way.
Adds a registry to hold auto-managed network clients. This registry will be prepopulated with networks we've built in to MetaMask as well as any custom networks that already exist in state under networkConfigurations.
Adds a new method getNetworkClients which returns the contents of the registry.
Update the logic that is executed when initializeProvider, setProviderType, setActiveNetwork, or resetConnection is called so that the "active network client" — i.e. the targets of the provider and block tracker proxy — is merely set based on some existing network client in the registry instead of being recreated each time.
Updates upsertNetworkConfiguration and removeNetworkConfiguration to either a) create a new network client in the event that a new network configuration is added and add it to the network client registry or b) destroy and remove an existing network client from the registry then create a new one in the event that the chain ID for an existing network configuration is updated.

References

Fixes #1045.

Changelog

`@metamask/network-controller`

ADDED: Add getNetworkClientsById method
- This returns a registry of built-in and custom networks that the network controller is cognizant of, allowing consumers to access multiple networks simultaneously if desired
CHANGED: Update the controller so that a network client is no longer recreated each time the network is initialized or switched; rather, a network client which has been created ahead of time will be utilized
CHANGED: Update upsertNetworkConfiguration to keep the network client registry up to date with changes to the set of network configurations
- If a new network configuration is added, the information in it will be used to create and register a new network client
- If an existing network configuration is updated, its information will be used to recreate the client for the corresponding network

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

In order for users to make requests to multiple networks simultaneously, we need to phase out the concept of a single active network in the network controller in favor of a collection of networks (active or otherwise). This commit is the first step in making this happen. To this end, this commit: - Adds the concept of an "auto-managed network client". This is an object that has the same exact interface as a network client — that is, it exposes a provider and block tracker — except that the network client doesn't spring into existence until an attempt is made to use the provider or block tracker in some way. This prevents network requests from occurring prematurely. - Adds a registry to hold auto-managed network clients. This registry will be prepopulated with networks we've built in to MetaMask as well as any custom networks that already exist in state under `networkConfigurations`. - Adds a new method `getNetworkClients` which returns the contents of the registry. - Update the logic that is executed when `initializeProvider`, `setProviderType`, `setActiveNetwork`, or `resetConnection` is called so that the "active network client" — i.e. the targets of the provider and block tracker proxy — is merely set based on some existing network client in the registry instead of being recreated each time. - Updates `upsertNetworkConfiguration` and `removeNetworkConfiguration` to either a) create a new network client in the event that a new network configuration is added and add it to the network client registry or b) destroy and remove an existing network client from the registry then create a new one in the event that the chain ID for an existing network configuration is updated.

packages/network-controller/src/NetworkController.ts

mcmire · 2023-06-21T22:03:16Z

packages/network-controller/src/NetworkController.ts

@@ -353,42 +420,57 @@ export class NetworkController extends BaseControllerV2<
    this.#previousProviderConfig = this.state.providerConfig;
  }

-  #configureProvider(


This method effectively turned into #createAutoManagedNetworkClientRegistry.

mcmire · 2023-06-21T22:03:52Z

packages/network-controller/src/NetworkController.ts

    this.messagingSystem.publish('NetworkController:networkDidChange');
    await this.lookupNetwork();
  }

-  #registerProvider() {


I consolidated this method into #selectNetwork.

mcmire · 2023-06-21T22:04:28Z

packages/network-controller/src/NetworkController.ts

-    }
-  }
-
-  #setupInfuraProvider(type: InfuraNetworkType) {


The code in this and the next method is now split up into two methods. The code to create the network client is now in #createAutoManagedNetworkClientRegistry and the code to update the provider is now in #selectNetwork.

mcmire · 2023-06-21T22:07:51Z

packages/network-controller/src/NetworkController.ts

  ): Promise<string> {
-    assertIsStrictHexString(chainId);
+    const sanitizedNetworkConfiguration = (


We were technically doing this before by destructuring the object that this method takes, but that approach meant that properties of the network configuration object that were not technically provided ended up as undefined when put into networkConfigurations. The code here is essentially equivalent to Lodash's pick function.

Makes sense! Potentially simpler to use the argument as-is and throw if extra properties are there (safer too, as we'd be informed about these properties being ignored rather than them being silently dropped). But that would be better as a separate PR, if we did want to go that route.

packages/network-controller/src/NetworkController.ts

packages/network-controller/tests/NetworkController.test.ts

mcmire · 2023-06-21T22:23:19Z

packages/network-controller/src/create-auto-managed-network-client.test.ts

+/**
+ * Handles mocking JSON-requests sent to the network.
+ */
+class MockedNetwork {


This may look familiar; there have been a couple of different variations on this theme elsewhere. That said, it doesn't do anything by default, so it's simpler than what we've done before.

packages/network-controller/src/NetworkController.ts

Gudahtt · 2023-06-28T14:25:05Z

packages/network-controller/src/NetworkController.ts

+   *
+   * @returns The list of known network clients.
+   */
+  getNetworkClients(): AutoManagedNetworkClient<NetworkClientConfiguration>[] {


Hmm, do you think a list is the right data structure to use here? I'm just wondering how this is likely to be used. Maybe having them keyed by something (e.g. network name for built-in, or id for custom) would be better.

Presumably, in most places where this is called, the caller is going to be looking for a specific network client. Keying it by whatever unique identifier the caller is likely to have would probably make this easier to use.

Right. The first version of this method was called getNetworkClientsById and returned an object keyed by an identifier, which for Infura networks would look like infura||<network> and for custom networks would look like custom||<chainId>||<lowercased rpcUrl> or custom||<network configuration ID>, depending on where it came from ¹.

Ultimately I wasn't sure how we will end up using this collection and figured that once we had more information we could tweak this. So I went with an array for now. But, an object would definitely be more versatile. I'll play around with this some more.

Footnotes

Custom networks aren't always associated with a network configuration ID, so we can't always use that in the identifier (because the network controller can be instantiated with a providerConfig that has nothing to do with a built-in network or a network in networkConfigurations). ↩

Custom networks aren't always associated with a network configuration ID

They are if you consider just how extension and mobile are using this controller. We can consider it a breaking change of this API, but "supporting custom networks specified via provider config that don't correspond with network configurations" is not a feature we're supporting intentionally. If anything it's a bug.

Okay, I've changed this method to return an object rather than an array: 96c4229

Co-authored-by: Mark Stacey <[email protected]>

packages/network-controller/src/NetworkController.ts

packages/network-controller/src/create-auto-managed-network-client.ts

packages/network-controller/src/create-network-client.ts

packages/network-controller/src/create-auto-managed-network-client.test.ts

packages/network-controller/src/NetworkController.ts

This reverts commit edd4a8bb1940558e5f0ebda743c92eb8b41fa8e2.

- Extract code that builds a network client ID - Refactor `#createAutoManagedNetworkClientRegistry` to avoid loop - Use full network client IDs in registry so that we can change `getNetworkClients()`

…t if applicable

mcmire · 2023-06-29T22:55:26Z

packages/network-controller/tests/NetworkController.test.ts

-                networkConfigurations: {
-                  testNetworkConfiguration: {
-                    id: 'testNetworkConfiguration',
+    describe('if a provider has not been set', () => {


The functionality tested here already exists, but as I was updating the JSDocs for rollbackToPreviousProvider, I noticed that they referred to this behavior. I wanted to ensure that I wasn't breaking anything, so I backfilled them. I also grouped the existing tests under a describe — that's what's causing the large diff here. I didn't change any of the existing tests, though, so if you remove this describe and then ungroup the tests within the next one, then you shouldn't see any differences in the diff.

mcmire · 2023-06-29T23:17:28Z

@Gudahtt I believe I've addressed all of your comments above.

packages/network-controller/src/NetworkController.ts

Gudahtt · 2023-06-30T01:48:03Z

packages/network-controller/src/NetworkController.ts

+  if (typeof infuraNetworkOrProviderConfig === 'string') {
+    return `infura||${infuraNetworkOrProviderConfig}` as const;
+  }
+  return `infura||${infuraNetworkOrProviderConfig.type}` as const;


How important is it that we use a composite ID?

I'm wondering because I'm hoping that we'll eventually migrate the built-in networks to be more similar to the custom networks (one type of network client, one middleware stack, etc., see #1279), at which point we presumably won't have a need for this custom|| prefix on each ID.

And until then, we don't have to worry about ID conflicts. The UUIDs we create for custom networks will never conflict with the short built-in network names.

Perhaps dropping the composite ID now would save us the effort of removing it in a migration later?

It'd also make it a bit simpler to access network clients in cases where we don't already have the network client ID. We could do soemthing like networkClients[providerConfig.type || providerConfig.id] , no need to construct the ID each time.

We don't really need the prefix to be able to tell what type of network client it is, that's true. One benefit to using a prefix that I can think of is that it allows the type of a built-in network client ID to be clearly different from the type of a custom network client ID — and actually, it allows a type to be assigned to the network client ID at all. If we drop the prefix, then the type of a custom network client ID is just string. At that point, any kind of network client ID itself, at least from a consumer perspective, will end up being string, whether Infura or not.

Maybe that's fine though. Network configuration IDs are just strings and we're okay with that. I've made this change here: 9e67774

Right... yeah this does make type narrowing more challenging. It's still possible for us to narrow from string to InfuraNetworkType though at least. We'd need to compare with each entry rather than just look at the prefix. This seems OK though.

Gudahtt

Looks good! All that's left are nits, plus that one question about the ID. I'd like to at least hear your thoughts about the ID before proceeding but otherwise I'm ready to approve.

…en value

…onfig

Gudahtt

LGTM!

adonesky1 · 2023-06-30T18:54:44Z

packages/network-controller/src/NetworkController.ts

@@ -296,8 +530,12 @@ export class NetworkController extends BaseControllerV2<

  #providerProxy: ProviderProxy | undefined;

+  #provider: ProxyWithAccessibleTarget<Provider> | undefined;


What is this doing? It looks like we set it but never use it? And its private so it couldn't used externally? cc @mcmire

Create and expose collection of network clients In order for users to make requests to multiple networks simultaneously, we need to phase out the concept of a single active network in the network controller in favor of a collection of networks (active or otherwise). This commit is the first step in making this happen. To this end, this commit: - Adds the concept of an "auto-managed network client". This is an object that has the same exact interface as a network client — that is, it exposes a provider and block tracker — except that the network client doesn't spring into existence until an attempt is made to use the provider or block tracker in some way. - Adds a registry to hold auto-managed network clients. This registry will be prepopulated with networks we've built in to MetaMask as well as any custom networks that already exist in state under `networkConfigurations`. - Adds a new method `getNetworkClientsById` which returns the contents of the registry, keyed by identifier. - Update the logic that is executed when `initializeProvider`, `setProviderType`, `setActiveNetwork`, or `resetConnection` is called so that the "active network client" — i.e. the targets of the provider and block tracker proxy — is merely set based on some existing network client in the registry instead of being recreated each time. - Updates `upsertNetworkConfiguration` and `removeNetworkConfiguration` to either a) create a new network client in the event that a new network configuration is added and add it to the network client registry or b) destroy and remove an existing network client from the registry then create a new one in the event that the chain ID for an existing network configuration is updated. Co-authored-by: Mark Stacey <[email protected]>

mcmire force-pushed the expose-network-clients branch from ff0feb3 to afebca5 Compare June 21, 2023 15:54

mcmire mentioned this pull request Jun 21, 2023

Simplify network types accepted by TransactionController #1443

Merged

3 tasks

mcmire force-pushed the expose-network-clients branch from afebca5 to fbde98b Compare June 21, 2023 18:32

mcmire marked this pull request as ready for review June 21, 2023 20:26

mcmire requested a review from a team as a code owner June 21, 2023 20:26

mcmire force-pushed the expose-network-clients branch 4 times, most recently from 38df17a to 6d11b2f Compare June 21, 2023 22:01

mcmire force-pushed the expose-network-clients branch from 6d11b2f to 1e2ba98 Compare June 21, 2023 22:25

mcmire commented Jun 21, 2023

View reviewed changes

mcmire added 2 commits June 27, 2023 12:33

Merge branch 'main' into expose-network-clients

577f21c

No need to pass provider into request methods anymore

df1f5f8

Gudahtt reviewed Jun 28, 2023

View reviewed changes

mcmire and others added 3 commits June 28, 2023 10:38

Merge branch 'main' into expose-network-clients

7991c79

Remove unnecessary tests

fa07c1f

Fix type parameter for knownKeysOf

d561088

Co-authored-by: Mark Stacey <[email protected]>

Gudahtt reviewed Jun 28, 2023

View reviewed changes

packages/network-controller/src/NetworkController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jun 28, 2023

View reviewed changes

packages/network-controller/src/NetworkController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jun 28, 2023

View reviewed changes

packages/network-controller/src/NetworkController.ts Outdated Show resolved Hide resolved

packages/network-controller/src/NetworkController.ts Outdated Show resolved Hide resolved

packages/network-controller/src/NetworkController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jun 28, 2023

View reviewed changes

packages/network-controller/src/create-auto-managed-network-client.test.ts Show resolved Hide resolved

packages/network-controller/src/NetworkController.ts Outdated Show resolved Hide resolved

mcmire added 7 commits June 28, 2023 17:08

Refactor code to build network clients

680d9eb

Revert "Refactor code to build network clients"

8256cb0

This reverts commit edd4a8bb1940558e5f0ebda743c92eb8b41fa8e2.

Refactor some things

c1dfb6b

- Extract code that builds a network client ID - Refactor `#createAutoManagedNetworkClientRegistry` to avoid loop - Use full network client IDs in registry so that we can change `getNetworkClients()`

Reword JSDocs for BlockTrackerProxy and ProviderProxy

45ba9cb

Rename 'nc' to 'networkConfiguration'

85c7e9c

Fix formatting

f01a57d

Use networkConfig rather than networkConfiguration

e19f87b

mcmire added 11 commits June 28, 2023 23:20

Make JSDocs for createAutoManagedNetworkClient more honest

f2e5549

Fix lint violation

03eee76

Make functions to build IDs more straightforward

ec75958

Fix TypeScript errors

577b0ff

Fix formatting

1cdc7c8

Prioritize network config over provider config in network clients lis…

24aa829

…t if applicable

Fix tests

513edbd

Simplify reduce in upsertNetworkConfiguration

90f44c7

Extract pick to a typecast-free version

e15525e

Test .configuration and .destroy for auto-managed network client

ad08e04

Merge branch 'main' into expose-network-clients

0fa287e

mcmire commented Jun 29, 2023

View reviewed changes

Gudahtt reviewed Jun 30, 2023

View reviewed changes

mcmire added 7 commits June 29, 2023 22:12

Simplify network client IDs

9e67774

Use 'registry' rather than 'obj'

6977c44

Update description for 'pick'

163e8e7

Undo description change to 'pick'

84a9f12

Update 'assertOfType' to rename 'test' to 'validate' and pass the giv…

72d6d6e

…en value

No need to construct an Infura network client config for a provider c…

d6d1dbd

…onfig

Fix lint violation

1a827fc

Gudahtt approved these changes Jun 30, 2023

View reviewed changes

mcmire merged commit f70430e into main Jun 30, 2023

mcmire deleted the expose-network-clients branch June 30, 2023 15:25

adonesky1 reviewed Jun 30, 2023

View reviewed changes

adonesky1 mentioned this pull request Jul 6, 2023

Use MultiChain NetworkController in ActiveNetworkController #1472

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create and expose collection of network clients #1439

Create and expose collection of network clients #1439

mcmire commented Jun 20, 2023 •

edited

Loading

mcmire Jun 21, 2023

mcmire Jun 21, 2023

mcmire Jun 21, 2023

mcmire Jun 21, 2023

Gudahtt Jun 28, 2023

mcmire Jun 21, 2023 •

edited

Loading

Gudahtt Jun 28, 2023

mcmire Jun 28, 2023 •

edited

Loading

Gudahtt Jun 28, 2023

mcmire Jun 29, 2023

mcmire Jun 29, 2023

mcmire commented Jun 29, 2023

Gudahtt Jun 30, 2023

Gudahtt Jun 30, 2023

mcmire Jun 30, 2023

Gudahtt Jun 30, 2023

Gudahtt left a comment

Gudahtt left a comment

adonesky1 Jun 30, 2023

		@@ -296,8 +530,12 @@ export class NetworkController extends BaseControllerV2<

		#providerProxy: ProviderProxy \| undefined;

		#provider: ProxyWithAccessibleTarget<Provider> \| undefined;

Create and expose collection of network clients #1439

Create and expose collection of network clients #1439

Conversation

mcmire commented Jun 20, 2023 • edited Loading

Explanation

References

Changelog

@metamask/network-controller

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire commented Jun 20, 2023 •

edited

Loading

`@metamask/network-controller`

mcmire Jun 21, 2023 •

edited

Loading

mcmire Jun 28, 2023 •

edited

Loading