-
-
Notifications
You must be signed in to change notification settings - Fork 242
chore: create network enablement controller #6028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/tests/NetworkEnablementController.test.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, just had some comments. Most are minor but there are a few that would be good to address.
packages/network-enablement-controller/src/NetworkEnablementController.ts
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
packages/network-enablement-controller/src/NetworkEnablementController.ts
Outdated
Show resolved
Hide resolved
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Nested Update Calls Cause State Inconsistencies
Nested this.update()
calls occur when #toggleNetwork
calls #ensureNetworkEntry
within its this.update()
callback, as #ensureNetworkEntry
also performs a this.update()
operation. This anti-pattern can lead to state inconsistencies, race conditions, and unpredictable behavior in the BaseController's state management. The logic from #ensureNetworkEntry
should be inlined into the outer update callback.
packages/network-enablement-controller/src/NetworkEnablementController.ts#L467-L485
core/packages/network-enablement-controller/src/NetworkEnablementController.ts
Lines 467 to 485 in 38f4ca9
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; | |
}); | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
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 toNetworkEnablementController
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 NetworkControllerKey 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
Changelog
Checklist