feat: add @metamask/network-connection-banner-controller#9041
Open
cryptodev-2s wants to merge 50 commits into
Open
feat: add @metamask/network-connection-banner-controller#9041cryptodev-2s wants to merge 50 commits into
cryptodev-2s wants to merge 50 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
cryptodev-2s
force-pushed
the
wpc-1014-network-connection-banner-controller
branch
2 times, most recently
from
9111156 to
d1fa945
Compare
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
cryptodev-2s
force-pushed
the
wpc-1014-network-connection-banner-controller
branch
5 times, most recently
from
cb4b1af to
ab5cc87
Compare
Contributor
Author
|
@metamaskbot publish-preview |
cryptodev-2s
temporarily deployed
to
default-branch
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
This comment was marked as outdated.
This comment was marked as outdated.
cryptodev-2s
force-pushed
the
wpc-1014-network-connection-banner-controller
branch
from
6d2ab14 to
2fc626b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
First pass collects enabled networks that have a default rpc endpoint and metadata; second pass filters failing ones and enriches into FailedNetwork. Drops the manual `totalNetworksWithMetadata` counter in favor of `networksWithMetadata.length`.
Extracted: - `#getEnabledEvmChainIds` — reads enabled EIP-155 chain ids - `#collectNetworksWithMetadata` — first pass, joins config and metadata - `#buildFailedNetwork` — enrichment for a single failing network - `#pickBannerNetwork` — applies the rule to pick which one to surface Also introduces a `NetworkWithMetadata` intermediate type.
Follows the controller guidelines: instead of reacting to every `stateChange` on the three upstream controllers, subscribe with narrow selectors so we only re-evaluate when a field we care about (`networksMetadata`, `networkConfigurationsByChainId`, `enabledNetworkMap`, `connectivityStatus`) actually changes. Each subscription needs its own handler reference since the messenger keys subscribers by handler identity — inline arrows wrap `#onUpstreamChange` to keep them distinct.
Reverts the type based check from the prior commit. Match on the
`{infuraProjectId}` placeholder that lives on stored network
configurations (the placeholder is substituted with the real project
id at request time). URLs that already carry a substituted key are
treated as custom since we do not have the project id in this package.
Follows the pattern in docs/code-guidelines/controller-guidelines.md: one subscribe per peer with a composed selector. - NetworkController: `createSelector` over `networksMetadata` and `networkConfigurationsByChainId`, memoized so the projected object is reference stable while unrelated fields change. - NetworkEnablementController: peer exposed `selectEnabledNetworkMap`. - ConnectivityController: peer exposed `connectivityControllerSelectors.selectConnectivityStatus`.
The degraded and unavailable timer callbacks no longer re run `#findFailedNetwork`. Instead they use the `FailedNetwork` captured at schedule time: our upstream selector subscriptions cancel or replace the pending timer whenever peer state changes in a way that matters, so at fire time the captured failure is still the right one. Removes the two silent recovery tests and the `setNetworkStateSilently` helper, which simulated a scenario the messenger contract disallows.
…ection-banner-controller
|
Warning MetaMask internal reviewing guidelines:
|
`RpcEndpoint` is defined inside `@metamask/network-controller` but not re-exported from its barrel. Use `NetworkConfiguration['rpcEndpoints'][number]` so we don't rely on a symbol that isn't part of the peer's public API.
Sync JSDoc for `dismissBanner` after the earlier trim.
The inline arrow selectors triggered `jsdoc/require-param`, `jsdoc/require-returns`, and `@typescript-eslint/explicit-function-return-type`. Extracting them as named consts with proper JSDoc and explicit return types fixes all six errors and matches the pattern in connectivity-controller.
Contributor
Author
|
@metamaskbot publish-preview |
mcmire
reviewed
Jul 2, 2026
| /packages/wallet @MetaMask/core-platform | ||
| /packages/wallet-cli @MetaMask/core-platform @MetaMask/ocap-kernel | ||
| /packages/wallet-framework-docs @MetaMask/core-platform | ||
| /packages/network-connection-banner-controller @MetaMask/core-platform |
Contributor
There was a problem hiding this comment.
Should we keep this list alphabetized and place this after multichain-api-middleware?
Contributor
Author
There was a problem hiding this comment.
Alphabetized in 699b24f5f.
| * the controller's actions and events and to namespace the controller's state | ||
| * data when composed with other controllers. | ||
| */ | ||
| const controllerName = 'NetworkConnectionBannerController'; |
Contributor
There was a problem hiding this comment.
Can we use CONTROLLER_NAME instead of controllerName? I recently updated the guidelines and sample controllers so that the name of this variable uses the same format as other constants.
Suggested change
| const controllerName = 'NetworkConnectionBannerController'; | |
| const CONTROLLER_NAME = 'NetworkConnectionBannerController'; |
Also what do you think about defining this constant before selectNetworkControllerFields?
Contributor
Author
There was a problem hiding this comment.
Renamed to CONTROLLER_NAME and moved above the module scope selectors in eff0473be.
| * Details of a failing network the banner describes. | ||
| */ | ||
| export type FailedNetwork = { | ||
| chainId: Hex; |
Contributor
There was a problem hiding this comment.
Should we go ahead and define JSDoc for all of these properties too while we are at it?
Contributor
Author
There was a problem hiding this comment.
Added in eff0473be.
Comment on lines
+271
to
+273
| // Scoped selectors per controller guideline so unrelated upstream | ||
| // `stateChange` events (e.g. a `NetworkController` selected client id | ||
| // update) do not trigger a re-evaluation. |
Contributor
There was a problem hiding this comment.
Do we need this comment?
Suggested change
| // Scoped selectors per controller guideline so unrelated upstream | |
| // `stateChange` events (e.g. a `NetworkController` selected client id | |
| // update) do not trigger a re-evaluation. |
Contributor
Author
There was a problem hiding this comment.
Removed in eff0473be.
Comment on lines
+301
to
+304
| * Starts evaluating network connection state. | ||
| * | ||
| * This method should be called after the upstream network, network | ||
| * enablement, and connectivity controllers have been initialized. |
Contributor
There was a problem hiding this comment.
This is technically true but doesn't talk about what happens after the network connection is evaluated.
What about:
Suggested change
| * Starts evaluating network connection state. | |
| * | |
| * This method should be called after the upstream network, network | |
| * enablement, and connectivity controllers have been initialized. | |
| * Look for a failed network, if any, and populate the initial state of the | |
| * banner. | |
| * | |
| * This method should be called after NetworkController, | |
| * NetworkEnablementController, and ConnectivityController have been | |
| * initialized. |
Contributor
Author
There was a problem hiding this comment.
Rewritten in eff0473be to describe what start() does (populate initial state, react from there) alongside when to call it.
Comment on lines
+289
to
+290
| () => this.#onUpstreamChange(), | ||
| connectivityControllerSelectors.selectConnectivityStatus, |
Contributor
There was a problem hiding this comment.
Similar as above, except that the selector should return an object:
Suggested change
| () => this.#onUpstreamChange(), | |
| connectivityControllerSelectors.selectConnectivityStatus, | |
| this.#refreshState.bind(this), | |
| ({ connectivityStatus }: ConnectivityControllerState) => ({ | |
| connectivityController: { | |
| connectivityStatus, | |
| }, | |
| }), |
Contributor
Author
There was a problem hiding this comment.
Added selectConnectivityControllerFields in 7dcb3d974.
Comment on lines
+279
to
+280
| () => this.#onUpstreamChange(), | ||
| selectNetworkControllerFields, |
Contributor
There was a problem hiding this comment.
Since we have the network configurations and network metadata can we pass them along to #refreshState? Maybe #refreshState can take options for each of the pieces of data it needs, only calling <controller>:getState if it's missing a piece of data.
Suggested change
| () => this.#onUpstreamChange(), | |
| selectNetworkControllerFields, | |
| this.#refreshState.bind(this), | |
| (state: NetworkState) => ({ | |
| networkControllerState: { | |
| networkConfigurationsByChainId: state.networkConfigurationsByChainId, | |
| networksMetadata: state.networksMetadata, | |
| }, | |
| }), |
Contributor
Author
There was a problem hiding this comment.
selectNetworkControllerFields already returns { networksMetadata, networkConfigurationsByChainId }; subscription handlers now pass the projection straight into #refreshState as networkControllerState in 7dcb3d974.
Comment on lines
+518
to
+523
| const otherInfura = rpcEndpoints.find( | ||
| (endpoint, index) => | ||
| index !== defaultRpcEndpointIndex && | ||
| getIsInfuraEndpoint(endpoint.url), | ||
| ); | ||
| switchableInfuraNetworkClientId = otherInfura?.networkClientId ?? null; |
Contributor
There was a problem hiding this comment.
There should only be one Infura network (this name is misleading):
Suggested change
| const otherInfura = rpcEndpoints.find( | |
| (endpoint, index) => | |
| index !== defaultRpcEndpointIndex && | |
| getIsInfuraEndpoint(endpoint.url), | |
| ); | |
| switchableInfuraNetworkClientId = otherInfura?.networkClientId ?? null; | |
| const infuraEndpoint = rpcEndpoints.find( | |
| (endpoint, index) => | |
| index !== defaultRpcEndpointIndex && | |
| getIsInfuraEndpoint(endpoint.url), | |
| ); | |
| switchableInfuraNetworkClientId = infuraEndpoint?.networkClientId ?? null; |
Contributor
Author
There was a problem hiding this comment.
Renamed in eff0473be.
| } | ||
| } | ||
|
|
||
| type NetworkWithMetadata = { |
Contributor
There was a problem hiding this comment.
What do you think about putting this type at the top of the file?
Contributor
Author
There was a problem hiding this comment.
Moved in eff0473be.
| * `{infuraProjectId}` placeholder form that lives on stored network | ||
| * configurations before the wallet substitutes the real project id at | ||
| * request time. URLs that already carry a substituted key are treated as | ||
| * custom (we do not have the project id in this package). |
Contributor
There was a problem hiding this comment.
Should this controller take an Infura project ID so that we can match the custom endpoints as well?
Contributor
Author
There was a problem hiding this comment.
NetworkConfiguration.rpcEndpoints[i].url is always stored with the {infuraProjectId} placeholder in MetaMask (substitution happens at request time in the network client). The placeholder match already covers every real case, so threading a project id through the constructor would add state the controller does not otherwise care about. Happy to revisit if a client ever starts persisting substituted URLs.
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
BREAKING CHANGE: `init()` is gone. Consumers of the banner controller now drive its lifecycle via `start()` and `stop()`. The old `init()` model ran the controller from the moment the wallet Engine constructed it, which meant the 5s / 30s escalation timers could complete while the user was still on the lock screen. A first look after unlock could show `unavailable` even though nothing was actually wrong. `start()` and `stop()` tie the controller to the UI that renders the banner. Both are idempotent. `start()` runs the initial evaluation and enables reactions to upstream state changes. `stop()` cancels pending timers, resets banner state to `available`, and ignores upstream changes until the next `start()`. - Rename messenger action `NetworkConnectionBannerController:init` to `NetworkConnectionBannerController:start` and add `NetworkConnectionBannerController:stop`. - Rewrite the README lifecycle section. - Reorganize tests into a dedicated `start / stop` block with coverage for stop cancelling a pending banner, ignoring upstream changes after stop, resuming on start after stop, and idempotence when never started.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 44d1ed7. Configure here.
A synchronous listener on `NetworkConnectionBannerController:stateChanged` could call `stop()` from inside `#refreshState`, either during the pre timer reset `update` or during the degraded timer's own `update`. The enclosing `setTimeout` was still scheduled, so the banner could escalate to `degraded` or `unavailable` after the UI had stopped the controller. Two new guards check `#started` after each `update` before scheduling the next timer. New regression tests exercise both re entry points.
- Rename `controllerName` to `CONTROLLER_NAME` per updated guidelines and move it above the module scope selectors. - Move the `NetworkWithMetadata` intermediate type to the top of the file with the other type definitions. - Add JSDoc to the previously undocumented properties of the `FailedNetwork` type. - Drop the redundant scoped selectors comment above the subscribe block, and the extra blank line before the `failedNetwork` guard in `#refreshState`. - Rewrite the `start` JSDoc to describe what it does, not just when it runs. - Rename `switchToDefaultInfuraRpc` to `switchToDefaultInfuraRpcEndpoint` (RPC endpoints are the term we use) and take `chainId` as a positional argument since there is only one. - Rename `otherInfura` to `infuraEndpoint` in `#buildFailedNetwork` since there is only one Infura endpoint per chain.
- `StubbedState` type becomes `ExternalState` with controller name keys (`NetworkController`, `NetworkEnablementController`, `ConnectivityController`) that match the messenger namespace. - Rework `BuildExternalStateArgs` to reference peer state types directly and use property names that match `NetworkController` state (`networkConfigurationsByChainId`, `networksMetadata`, `enabledEvmChainIds` since we are strictly on the EIP 155 namespace). - `buildEnablementState` becomes `buildNetworkEnablementControllerState` to match the actual controller name. - `makeMetadata` becomes `buildNetworkMetadata`, and `buildConfiguration` becomes `buildNetworkConfiguration` for consistency with the other `build*` helpers. - `buildNetworkState` becomes `buildExternalState`, `setNetworkState` becomes `publishNetworkStateChanges`, and the `initialState` argument of `withController` becomes `externalState`. - Rename `describe` blocks to match the event they exercise: `on NetworkController:stateChange` and `on ConnectivityController:stateChange`.
Moves `network-connection-banner-controller` to its alphabetically sorted position after `multichain-api-middleware`.
Adds `setNetworkControllerState` and `setNetworkEnablementControllerState` so tests that specifically want to exercise one peer's `stateChange` event can publish only that event, matching the code path they claim to cover. `publishNetworkStateChanges` stays as a setup convenience for tests that want to seed both at once. The `start / stop` describe now uses `setNetworkControllerState` directly, moving the enablement setup to the `externalState` argument of `withController`. Adds an `on NetworkEnablementController:stateChange` describe block with coverage for enabling a failing chain and disabling one whose banner is already showing.
`buildInfuraEndpoint` and `buildCustomEndpoint` now live with the other test helpers at the bottom of the file and take options bags so the network client id is called out by name at every call site.
Neither field influences the banner rule and no test asserts on them. Falling back to the `buildNetworkConfiguration` defaults keeps the Polygon test setups smaller without changing behavior.
`#refreshState` now takes optional `networkControllerState`, `networkEnablementControllerState`, and `connectivityControllerState` parameters (each `Pick`d down to the fields the rule uses), falling back to the respective `<controller>:getState` call when omitted. Subscription handlers pass their memoized projection straight in, so event driven re evaluations reuse the value the selector already computed instead of calling `getState` again. `#findFailedNetwork`, `#collectNetworksWithMetadata`, and `#getEnabledEvmChainIds` take state as arguments too. The `#isStarted` gate lives at the top of `#refreshState` now, so the old `#onUpstreamChange` shim goes away. Adds two composed selectors (`selectNetworkEnablementControllerFields`, `selectConnectivityControllerFields`) that return object projections matching the `Pick` shape.
Options bag first, then test function. Matches the pattern in `sample-petnames-controller.test.ts`. Existing shorthand `withController(fn)` for the no options case keeps working via the variadic overload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
The RPC connection banner ("Still connecting" / "Unable to connect") is currently rendered by both
metamask-extensionandmetamask-mobilebased on duplicated logic in each repo:pslso a single provider's wide outage doesn't pop the banner.{ chainId, networkName, rpcUrl, isInfuraEndpoint, infuraNetworkClientId }payload shape.This PR introduces
@metamask/network-connection-banner-controllerso there's a single source of truth. The controller:NetworkController:stateChanged,NetworkEnablementController:stateChanged, andConnectivityController:stateChangedvia the messenger.{ status: 'available' | 'degraded' | 'unavailable', network: FailedNetwork | null }that clients subscribe to.dismissBanner()andswitchToDefaultInfuraRpc({ chainId })— the latter looks up the chain's first Infura endpoint and callsNetworkController:updateNetworkwithreplacementSelectedRpcEndpointIndex.psl1.15 ships itstypesfield outside itsexportsmap so TypeScript'snode16module resolution can't find it; a small ambientsrc/psl.d.tsshim mirrors the workaround already in use in extension and mobile.ip-regexis pinned to^4.3.0(last CJS release) because the monorepo's Jest config doesn't transform ESM-only deps.References
Checklist
Note
Medium Risk
switchToDefaultInfuraRpcEndpointmutates default RPC via NetworkController; banner logic is intricate but isolated in a new package with broad test coverage.Overview
Adds
@metamask/network-connection-banner-controller, a new core package that centralizes the RPC connection banner logic previously duplicated in extension and mobile.NetworkConnectionBannerControllerwatches enabled EVM networks viaNetworkController,NetworkEnablementController, andConnectivityController, applies the shared show/hide rules (custom-RPC override, multi-domain failures, all-enabled-down escape hatch, single-provider Infura suppression via eTLD+1), and drives 5s → degraded and 30s → unavailable escalation timers. UI callsstart()/stop()so timers do not run while the wallet surface is inactive;dismissBanner()andswitchToDefaultInfuraRpcEndpoint(chainId)(viaNetworkController:updateNetwork) handle user actions.Monorepo wiring includes README/CODEOWNERS/teams/tsconfig, a root
types/psl.d.tsshim forpsltyping, and package depspsl/ip-regex.Reviewed by Cursor Bugbot for commit cadd612. Bugbot is set up for automated code reviews on this repo. Configure here.