chore: create network enablement controller

[salimtb]

Explanation

The NetworkEnablementController is a core implementation of network enablement functionality that was previously only available in the extension. This PR moves and expands this functionality from the extension's NetworkOrder controller to core, enabling network visibility features across both extension and mobile platforms. It also renames it to NetworkEnablementController to more accurately represent what it does. Maybe in the future this can be expanded to include other UX enhancements that don't need to be tightly coupled with the NetworkController

Key motivations for this change:

Network Enablement Feature: This work is part of the broader Network Enablement initiative (#5737). The controller has been expanded to include enabledNetworkMap state, allowing us to:

Track which networks are enabled/disabled
Support the new network enablement feature
Code Quality Improvements: The move to core has provided an opportunity to:

Improve test coverage of the existing network ordering logic
The controller now handles three key aspects:

Network enablement (new functionality. recently introduced on extension)

References

• Moves functionality from NetworkOrder controller in extension to core
• Part of Network Enablement initiative (Network Controller: Multi Networks Enablement feature #5737)
• Related to ongoing work related to Global Network Selector Removal

Changelog

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 communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
• I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

[cursor]

Bug: Network Controller Errors: Type Handling & Nested Updates

The NetworkEnablementController has two bugs:

1. TypeError when disabling networks: The #toggleNetwork method can throw a TypeError when attempting to disable a network. This occurs because selectAllEnabledNetworks(this.state)[namespace] can return undefined if the namespace has no enabled networks, leading to Object.values(undefined). Additionally, the logic for counting enabled networks is unnecessarily complex and redundant, as selectAllEnabledNetworks(...)[namespace] already returns a string array whose length should be used directly.
2. Nested state updates: The #toggleNetwork method calls this.#ensureNetworkEntry(chainId) within its this.update() callback. Since #ensureNetworkEntry also calls this.update(), this creates nested state updates, which can lead to state inconsistencies or unexpected behavior.

packages/network-enablement-controller/src/NetworkEnablementController.ts#L455-L466

core/packages/network-enablement-controller/src/NetworkEnablementController.ts

Lines 455 to 466 in 303ac2a

	// Don't disable the last remaining enabled network
	if (
	!enable &&
	Object.values(selectAllEnabledNetworks(this.state)[namespace]).flat()
	.length <= 1
	) {
	return;
	}
	this.update((s) => {
	// Ensure entry exists first
	this.#ensureNetworkEntry(chainId);

Fix in Cursor • Fix in Web

---

Was this report helpful? Give feedback by reacting with 👍 or 👎

[Prithpal-Sooriya]

Nice! Should we allow an array, to allow enabling multiple networks. Just considering for the "select all" networks flow.

We can have this in a future version if performance is a concern.

[Prithpal-Sooriya]

Hmmm, open to discussion, but I don't think we should include a manually written API reference.

I'm not against this - it is really great for open source devs to view. But I am wondering if the full API spec can be auto-generated through types (we do include a typedoc file).

E.g. our README would look like:

## Usage

... our useage section for controller/selectors (amazing!)

## API Reference

For full API reference, look at [path to auto generated type doc].

[mcmire]

Unfortunately typedoc generation is currently broken due to the way that we build the repo 😞 I've been slowly working on fixing this (#1845 (comment)) but I'm not sure when that will be complete. Whatever path you want to take here is fine with me, but I just wanted to let you know about that.

Having some kind of usage examples at the very least would be great though, we don't do that often enough.

[Prithpal-Sooriya]

I was wondering why we needed this dep. Looks like it is to provide us the SolScope property. Shame, it would be nice if this was provided through other utils.

import { SolScope } from '@metamask/keyring-api';

[mcmire]

If you don't want the dependency, could you copy the type? Sometimes we've done that before. Just a thought.

[Prithpal-Sooriya]

We don't really need this comment do we?

[Prithpal-Sooriya]

I think it would be good to align the default networks with what is done in the NetworkController?
Just to avoid having multiple places to update.

Only mentioning since we will soon add Arbitrum as another default network.

[mcmire]

So that you don't have to maintain a list here as well, what are your thoughts on adding an init method to this controller that intelligently populates the initial state from whatever network configurations are present in NetworkController state if no initial state was passed to the constructor? That is, the state of this controller would be set to {} initially but would be populated in init.

[Prithpal-Sooriya]

Because the CaipNamespace can be deleted, can we make the value optional? Also thoughts on expanding this type so we can insert some JSDoc comments?

interface EnabledMap {
    /**
     * caipNamespace
     * @example eip155 or solana
     * NOTE - a namespace may be deleted, hence indexing can result in undefined.
     */
    [caipNamespace: string]: {
        /**
         * chainId, provided as hex or CAIP-2
         * @example 0x1 or solana:mainnet
         */
        [chainId: string]: boolean
    } | undefined
}

[mcmire]

I know what you mean, however, this type says that all keys in this object must be a CaipNamespace and all values must be a Record<string, boolean>. If you delete a key, wouldn't that still hold true?

Regardless, should chain IDs be CAIP chain IDs?

Suggested change

	type EnabledMap = Record<CaipNamespace, Record<string, boolean>>;
	type EnabledMap = Record<CaipNamespace, Record<CaipChainId, boolean>>;

[Prithpal-Sooriya]

Q - will this hasOneEnabled prevent a enabledNetworkMap entry from being deleted?

[mcmire]

Took a pass at this. Had a lot of comments so I decided to stop. :) I will do another pass later.

In general it seems like there is more complexity in this controller than I would expect even though we are tracking whether networks are enabled or disabled and organizing them by group. It seems that we are constantly having to convert CAIP chain IDs so I'm curious if there is a more succinct way we can represent them so we don't have to do that. I will take a closer look later.

[mcmire]

This property is already set in BaseController, do we need this here?

Suggested change

this.messagingSystem = messenger;

[mcmire]

This method seems to either set a network to enabled or disabled. Yet there is a method that explicitly sets a network to disabled below. That means there are two ways to disable a network using this controller. You can also set a network to disabled despite the name being setEnabledNetwork.

To make the API more consistent and reduce the number of code paths, what are your thoughts on making this method only enable a network? Then, what are your thoughts on calling this enableNetwork rather than setEnabledNetwork? I think that it would be more clear what this method does.

[mcmire]

It looks like you already have an isNetworkEnabled selector. To make this API lighter what are your thoughts on removing this method and having consumers always use the selector instead?

[mcmire]

Does this need to be an instance method or can it be a function outside of the class? This way the selectors can use it.

[mcmire]

It looks like each of these lines makes a state update. It's generally good practice to batch state updates together in the same operation. What are your thoughts on refactoring this so there is only one state update?

[mcmire]

Similar here. If this is something that's not allowed, should we throw an error so that we push the responsibility to prevent this from happening on to the UI layer in the client?

[mcmire]

We added something a while back to BaseController that prevents redundant updates: c580f85. So I am not sure that using this method to prevent unnecessary state updates would be necessary. However, I can see how this might prevent the last network in a namespace from being disabled. Perhaps we should update this wording?

Suggested change

	* This method is used to prevent unnecessary state updates when trying to enable
	* a network that is already the only enabled network in its namespace.
	* This method is used to prevent the last network in a namespace from being removed.

[mcmire]

What are your thoughts on having this method/function return reference as well? It seems that this would prevent us from needing to re-parse caipId any time we want the reference (which we are doing a lot in this file).

[mcmire]

Do we need to do anything if #hasOneEnabled returns true?

Suggested change

	this.update((s) => {
	// Ensure entry exists first
	this.#ensureNetworkEntry(chainId);
	if (this.#hasOneEnabled(s, namespace, chainId)) {
	return;
	}
	// If enabling a non-popular network, disable all networks in the same namespace
	if (enable && !this.#isPopularNetwork(caipId)) {
	Object.keys(s.enabledNetworkMap[namespace]).forEach((key) => {
	s.enabledNetworkMap[namespace][key] = false;
	});
	}
	s.enabledNetworkMap[namespace][storageKey] = enable;
	});
	this.update((s) => {
	// Ensure entry exists first
	this.#ensureNetworkEntry(chainId);
	// If enabling a non-popular network, disable all networks in the same namespace
	if (enable && !this.#isPopularNetwork(caipId)) {
	Object.keys(s.enabledNetworkMap[namespace]).forEach((key) => {
	s.enabledNetworkMap[namespace][key] = false;
	});
	}
	s.enabledNetworkMap[namespace][storageKey] = enable;
	});

[mcmire]

Should this say:

Suggested change

	// Don't disable the last remaining enabled network
	// Don't update the last remaining enabled network

But also, do we need this? Is this covered already by the #hasOneEnabled check below?