Add getNetworkClientById

[adonesky1]

1. Add getNetworkClientById, a simple getter for the networkClient registry that takes a networkClientId and returns the corresponding networkClient.

• TODO Add tests for this method (if we agree it makes sense)

2. Rename getNetworkClientsById: this name is/was confusing. To me the ById implies you pass in an Id as an arg rather than that the return will be keyed by id. Open to discussion on this.

[mcmire]

I left a couple of comments.

Would you mind adding tests for getNetworkClientById? (edit: Oh, I see you already marked that as a TODO — great :) )

[mcmire]

Since we currently have a split between Infura and custom network clients, I think it would be a good idea for this method to return the right type depending on the ID given so that networkClient.configuration will return a correctly typed configuration object. Instead of relying on getNetworkClients, we can access the registry directly. Also, considering that CustomNetworkClientId is a string, this method can technically return undefined if that string isn't present as a property in the custom network client registry. So consumers don't have to deal with undefined, I think it would be a good idea to throw in that case.

With that in mind, what about this?

Suggested change

	/**
	* Returns the network client for a given network client id.
	*
	* @param networkClientId - The network client id for which to retrieve the network client.
	* @returns The network client.
	*/
	getNetworkClientById(
	networkClientId: NetworkClientId,
	): AutoManagedNetworkClient<NetworkClientConfiguration> {
	const networkClients = this.getNetworkClients();
	return networkClients[networkClientId];
	}
	/**
	* Returns the Infura network client with the given ID.
	*
	* @param infuraNetworkClientId - An Infura network client ID.
	* @returns The Infura network client.
	* @throws If an Infura network client does not exist with the given ID.
	*/
	getNetworkClientById(
	infuraNetworkClientId: BuiltInNetworkClientId,
	): AutoManagedNetworkClient<InfuraNetworkClientConfiguration>;
	/**
	* Returns the custom network client with the given ID.
	*
	* @param customNetworkClientId - A custom network client ID.
	* @returns The custom network client.
	* @throws If a custom network client does not exist with the given ID.
	*/
	getNetworkClientById(
	customNetworkClientId: CustomNetworkClientId,
	): AutoManagedNetworkClient<CustomNetworkClientConfiguration>;
	getNetworkClientById(
	networkClientId: NetworkClientId,
	): AutoManagedNetworkClient<NetworkClientConfiguration> {
	const autoManagedNetworkClientRegistry =
	this.#ensureAutoManagedNetworkClientRegistryPopulated();
	if (isInfuraProviderType(networkClientId)) {
	const infuraNetworkClient =
	autoManagedNetworkClientRegistry[NetworkClientType.Infura][
	networkClientId
	];
	if (!infuraNetworkClient) {
	throw new Error(
	`No Infura network client was found with the ID "${networkClientId}".`,
	);
	}
	return infuraNetworkClient;
	}
	const customNetworkClient =
	autoManagedNetworkClientRegistry[NetworkClientType.Custom][
	networkClientId
	];
	if (!customNetworkClient) {
	throw new Error(
	`No custom network client was found with the ID "${networkClientId}".`,
	);
	}
	return customNetworkClient;
	}

With these changes, this should be the case:

const goerliNetworkClient = networkController.getNetworkClientById(InfuraNetworkType.goerli);
goerliNetworkClient.configuration  //=> InfuraNetworkClientConfiguration
// Assuming that 'AAAA-AAAA-AAAA-AAAA' exists in the registry
const customNetworkClient = networkController.getNetworkClientById('AAAA-AAAA-AAAA-AAAA');
customNetworkClient.configuration  //=> CustomNetworkClientConfiguration
// Assuming that 'BBBB-BBBB-BBBB-BBBB' does not exist in the registry
networkController.getNetworkClientById('BBBB-BBBB-BBBB-BBBB');  // throws

[adonesky1]

if (!infuraNetworkClient) {
throw new Error(
No Infura network client was found with the ID "${networkClientId}".,
);
}

I'm pretty sure this couldn't ever be hit the way the Controller is written today since if it entered the if block

if (isInfuraProviderType(networkClientId))

it must be a valid infura clientId and must therefore be included in the registry.

I suppose this could be refactored at some point such that this wouldn't be the case so I don't mind leaving the error in.

[adonesky1]

cc @mcmire

[mcmire]

That's true. I've only added that check because eventually we want to enable TypeScript's exactOptionalPropertyTypes option, and once that happen it'll add | undefined to all index access, so it's good to keep that in mind now.

[mcmire]

I agree with removing ById here since philosophically it conflicts with how we are naming getNetworkClientById.

As discussed on Slack, if you assign this to a variable, but then you want an array version of this, the new name here forces you to name that array variable something else other than networkClients, which might be the more obvious choice. To save an extra decision from having to be made, what do we think about naming this something like getNetworkClientRegistry?

[adonesky1]

sure I think that makes some sense!

[adonesky1]

done here: c7be0e7

[mcmire]

Marking this as "changes requested" just as a reminder to add tests.

[legobeat]

Even if it's redundant, it'd be helpful to call this getNetworkClientByClientId to avoid confusion with networkID or even chainID.

(Or more subjective preference: getNetworkClientByClientID - both variants exist and Imo acronyms are easier sall-caps)

[adonesky1]

Even if it's redundant, it'd be helpful to call this getNetworkClientByClientId to avoid confusion with networkID or even chainID.

If we were gonna go this route wouldn't we want to do getNetworkClientByNetworkClientId?

Personally I think getNetworkClientById works. I think its reasonable to infer to which id we're referring.

[mcmire]

It's true that there are a few different concepts that have IDs, so I understand how confusion would arise. I am generally a fan of being explicit where needed, but in this case I also believe that it's reasonable to assume that the ID in this case refers to the thing we're getting rather than some other object, as it's the simplest possibility. If there is confusion, then the method signature includes infuraNetworkClientId or customNetworkClientId in the arguments, so one should be able to hover over the method in one's editor and get a sense of how it's intended to be used quickly.

[mcmire]

Looks good!