-
-
Notifications
You must be signed in to change notification settings - Fork 234
feat: Add NetworkVisibilityController
#5939
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
"dependencies": { | ||
"@metamask/base-controller": "^8.0.1", | ||
"@metamask/keyring-api": "^18.0.0", | ||
"@metamask/keyring-snap-client": "^5.0.0", |
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.
Do we use this dep, I don't see it when I search for it?
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.
We are inheriting the BaseController
in NetworkVisibilityController
so that we can extend from it. I saw this pattern elsewhere in the app. Did I miss something here?
"@metamask/multichain-network-controller": "^0.8.0" | ||
}, | ||
"peerDependencies": { | ||
"@metamask/error-reporting-service": "^1.0.0", |
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.
Same here, searching for this, I don't see if in out src.
"peerDependencies": { | ||
"@metamask/error-reporting-service": "^1.0.0", | ||
"@metamask/network-controller": "^23.0.0", | ||
"@metamask/providers": "^22.0.0" |
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.
Same here, I think we can remove it since it is not used.
@gambinish Thanks for pinging me on this PR. I know we just had a discussion where we came to the conclusion that we could integrate the network enablement and network ordering features into one controller. However, I'm not satisfied with the name "NetworkVisibilityController", and I can't come up with a good alternative. And that's telling me that perhaps these are two features that don't belong together after all. The network ordering is strictly UI-related, but the network enablement seems more than that, and so perhaps mixing them together would make things confusing. Maybe we just want a NetworkEnablementController or EnabledNetworksController for now and we can port NetworkOrderController later? |
I think that this is a fair point. To be honest, I thought about this as well. I'll bring this back to the team, though I don't anticipate much pushback. I'll remove the @mcmire do you have any idea why the test files aren't able to I remember we were troubleshooting this locally, unsure why this is failing in CI when it's passing on my machine. |
@gambinish I think I figured it out. It looks like you have an extra |
The
NetworkVisibilityController
is a core implementation of network ordering and enablement functionality that was previously only available in the extension. This PR moves and expands this functionality from the extension'sNetworkOrder
controller to core, enabling network ordering and visibility features across both extension and mobile platforms. It also renames it toNetworkVisibilityController
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 theNetworkController
Key motivations for this change:
Platform Parity: Network ordering was previously only implemented in the extension via
NetworkOrder
controller, leaving mobile without this functionality. Moving this to core will unlock the ability to have this feature built on mobile.Network Enablement Feature: This work is part of the broader Network Enablement initiative (Network Controller: Multi Networks Enablement feature #5737). The controller has been expanded to include
enabledNetworkMap
state, allowing us to:Code Quality Improvements: The move to core has provided an opportunity to:
The controller now handles three key aspects:
References
Checklist