Skip to content

fix(gas-fee-controller): make GasFeeController initialization-order-agnostic - #9569

Merged
sirtimid merged 6 commits into
mainfrom
sirtimid/gas-fee-controller-init-order-agnostic
Jul 21, 2026
Merged

fix(gas-fee-controller): make GasFeeController initialization-order-agnostic#9569
sirtimid merged 6 commits into
mainfrom
sirtimid/gas-fee-controller-init-order-agnostic

Conversation

@sirtimid

@sirtimid sirtimid commented Jul 21, 2026

Copy link
Copy Markdown
Member

Explanation

The GasFeeController constructor was the only currently-wired-or-wireable controller that performed eager, construction-time reads of NetworkController and the network provider:

  • new EthQuery(this.#getProvider()) — unconditional.
  • Seeding currentChainId — either via an eager getChainId() call or via NetworkController:getState + getNetworkClientById.

This forced any shared initialization (e.g. @metamask/wallet) to guarantee NetworkController is constructed before GasFeeController, which is exactly the construction-order coupling we want to avoid. Every other order-agnostic controller resolves its collaborators lazily.

This PR defers those reads to first use so the controller can be constructed in any order, matching the pattern already used elsewhere. No public API change — the constructor signature and gas-fee-fetching behavior are unchanged.

Changes

  • Removed the eager EthQuery build and the eager currentChainId seeding from the constructor. Network-change subscription wiring is kept as-is (subscribing is safe regardless of construction order).
  • Added lazy, memoized resolution:
    • #getEthQuery() — builds the EthQuery from getProvider on first use.
    • #getCurrentChainId() — resolves the chain ID from the getChainId callback when provided (only when paired with onNetworkDidChange, preserving the original coupling), otherwise from NetworkController.
  • #onNetworkControllerDidChange now resets ethQuery to undefined (rebuilt lazily on the next fetch) instead of rebuilding it eagerly per network change, and still updates currentChainId.

Testing

  • New tests assert construction performs no NetworkController:getState / getNetworkClientById / getProvider / getChainId calls — even when those handlers throw — for both the callback and no-callback constructor branches.
  • New test covers network-change handling via both the onNetworkDidChange callback and the NetworkController:networkDidChange messenger subscription (chain ID + eth query update on the next fetch).
  • All existing tests pass unchanged (53 total). 100% coverage on the changed lines; remaining uncovered lines are pre-existing.

References

Splits out the root-cause fix from the wiring PR #9527, so that GasFeeController can be migrated into the shared @metamask/wallet init set without a dependency-ordering mechanism.

🤖 Generated with Claude Code


Note

Medium Risk
Touches core gas-estimation initialization and network-switch caching; behavior is intended to stay the same but timing of first network reads shifts to first fetch.

Overview
GasFeeController no longer touches NetworkController or the RPC provider during construction, so it can be instantiated before the network stack is ready (e.g. shared @metamask/wallet init) without changing the public constructor or fetch semantics.

The constructor drops eager EthQuery creation and immediate chain-ID resolution (getChainId() / getState + getNetworkClientById). Instead, #getEthQuery() and #getCurrentChainId() memoize provider and chain ID on first use inside _fetchGasFeeEstimateData. When onNetworkDidChange + getChainId are supplied, the callback is stored without calling getChainId at construct time.

On network change, #onNetworkControllerDidChange clears the cached ethQuery (rebuilt on the next fetch) and still updates currentChainId and polling. Tests lock in zero constructor-time network/provider/getChainId calls and correct chain ID + provider refresh after network switches via callback or NetworkController:networkDidChange.

Reviewed by Cursor Bugbot for commit 66c249d. Bugbot is set up for automated code reviews on this repo. Configure here.

@sirtimid
sirtimid force-pushed the sirtimid/gas-fee-controller-init-order-agnostic branch from 6aedfa1 to b0a9c28 Compare July 21, 2026 10:40
@sirtimid
sirtimid marked this pull request as ready for review July 21, 2026 10:40
@sirtimid
sirtimid requested review from a team as code owners July 21, 2026 10:40
@sirtimid
sirtimid requested a review from matthewwalsh0 July 21, 2026 10:40
@sirtimid
sirtimid temporarily deployed to default-branch July 21, 2026 10:40 — with GitHub Actions Inactive

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 b0a9c28. Configure here.

Comment thread packages/gas-fee-controller/src/GasFeeController.ts
sirtimid and others added 6 commits July 21, 2026 16:25
…gnostic

Defer the constructor's NetworkController and provider reads until the
first gas fee fetch. The constructor no longer eagerly calls
NetworkController:getState, NetworkController:getNetworkClientById, the
getChainId option, or the getProvider option; the provider (EthQuery) and
current chain ID are now resolved lazily on first use and memoized, kept
fresh by the networkDidChange handler.

This lets the controller be constructed before NetworkController is ready,
removing the need for dependency ordering when wiring it into the shared
@metamask/wallet init set. The constructor signature and gas fee fetching
behavior are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… eth query rebuild

Resolve the current chain ID into a local before entering the update()
recipe so the Immer producer stays side-effect free, and extract
#getChainIdForNetworkClient to dedupe the network-client chain-ID lookup
shared by the network-change handler and the lazy resolver.

Add a test asserting the provider is read once, the eth query is cached,
then rebuilt from the provider after a network change (mutation-verified),
and reuse setupGasFeeController for the messenger-event test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The order-agnostic refactor removed two no-restricted-syntax
violations in GasFeeController.ts, leaving stale suppression counts
that fail lint:eslint in CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sirtimid
sirtimid force-pushed the sirtimid/gas-fee-controller-init-order-agnostic branch from 5d65742 to 66c249d Compare July 21, 2026 13:43

@matthewwalsh0 matthewwalsh0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Absolutely out of scope for this PR but we can probably simplify this loads by removing the global / selected chain support as that predates the multi-chain / PollingController support which should be the only route needed now

@sirtimid
sirtimid added this pull request to the merge queue Jul 21, 2026
Merged via the queue into main with commit dd0096f Jul 21, 2026
427 checks passed
@sirtimid
sirtimid deleted the sirtimid/gas-fee-controller-init-order-agnostic branch July 21, 2026 16:47
sirtimid added a commit that referenced this pull request Jul 21, 2026
Wire `GasFeeController` into `@metamask/wallet`'s default initialization
set as its own `InitializationConfiguration`, mirroring the
`transaction-controller` / `network-controller` instances. A wired
`TransactionController` delegates `GasFeeController:fetchGasFeeEstimates`
lazily, so the wallet boots today but throws the first time a transaction
flow needs gas estimates; wiring `GasFeeController` supplies that handler.

Adds a required `instanceOptions.gasFeeController` option whose `clientId`
(sent as `X-Client-Id` to the gas API) is required, so every client
identifies itself; all other fields are optional and fall back to
platform-agnostic defaults. wallet-cli passes `clientId: 'cli'`.

The instance builds `getProvider` / `getCurrentNetworkEIP1559Compatibility`
as lazy closures over `NetworkController`, and `GasFeeController` was made
initialization-order-agnostic upstream (#9569), so no dependency-ordering
machinery is needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sirtimid added a commit that referenced this pull request Jul 21, 2026
Wire `GasFeeController` into `@metamask/wallet`'s default initialization
set as its own `InitializationConfiguration`, mirroring the
`transaction-controller` / `network-controller` instances. A wired
`TransactionController` delegates `GasFeeController:fetchGasFeeEstimates`
lazily, so the wallet boots today but throws the first time a transaction
flow needs gas estimates; wiring `GasFeeController` supplies that handler.

Adds a required `instanceOptions.gasFeeController` option whose `clientId`
(sent as `X-Client-Id` to the gas API) is required, so every client
identifies itself; all other fields are optional and fall back to
platform-agnostic defaults. wallet-cli passes `clientId: 'cli'`.

The instance builds `getProvider` / `getCurrentNetworkEIP1559Compatibility`
as lazy closures over `NetworkController`, and `GasFeeController` was made
initialization-order-agnostic upstream (#9569), so no dependency-ordering
machinery is needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pull Bot pushed a commit to dmrazzy/core that referenced this pull request Jul 22, 2026
…etaMask#9527)

## Explanation

Wires `GasFeeController` into `@metamask/wallet`'s default
initialization set as its own `InitializationConfiguration` under
`src/initialization/instances/gas-fee-controller/`, mirroring the most
recently merged instances (`transaction-controller`,
`network-controller`).

`TransactionController` is already wired and its messenger delegates
`GasFeeController:fetchGasFeeEstimates`. Delegation registers a *lazy*
handler, so the wallet boots fine today — but the first transaction flow
that needs gas estimates throws `A handler for
GasFeeController:fetchGasFeeEstimates has not been registered`. Wiring
`GasFeeController` is the missing piece that lets a wired
`TransactionController` actually estimate gas.

`@metamask/wallet` is the shared controller-integration layer that
`metamask-extension`, `metamask-mobile`, and `@metamask/wallet-cli` all
adopt, so every value that differs between clients is an injectable
`instanceOptions.gasFeeController` slot with a platform-agnostic default
— nothing is baked to one client.

### Constructor callbacks

`GasFeeController` takes direct callbacks rather than pure messenger
delegation. The instance builds them from the wired `NetworkController`:

- `getProvider` → `NetworkController:getState`
(`selectedNetworkClientId`) then
`NetworkController:getNetworkClientById(id).provider`
- `getCurrentNetworkEIP1559Compatibility` →
`NetworkController:getEIP1559Compatibility` (coerced to a defined
`boolean`)

`onNetworkDidChange` / `getChainId` are omitted — the constructor
already has a messenger-based network-tracking fallback when both are
absent (subscribes `NetworkController:networkDidChange`).

### Injectable options + per-environment values

| Option | Extension | Mobile | Default (wallet-cli / package) |
| ------ | --------- | ------ | ------------------------------ |
| `EIP1559APIEndpoint` | `.../networks/<chain_id>/suggestedGasFees` (dev
override → `gas.uat-api.cx.metamask.io`) | same prod URL | prod URL;
injectable |
| `legacyAPIEndpoint` | `${GAS_API}/networks/<chain_id>/gasPrices` |
same prod URL | prod URL; injectable |
| `clientId` | `'extension'` | `'mobile'` | `'cli'` (sent as
`X-Client-Id`); injectable |
| `interval` | `10_000` | `15_000` | controller default `15_000`;
injectable |
| `getCurrentNetworkLegacyGasAPICompatibility` | `chainId === BSC` |
`mainnet \|\| BSC \|\| POLYGON` | `() => false`; injectable |
| `getCurrentAccountEIP1559Compatibility` | `() => true` | omitted | `()
=> true`; injectable |

### Initialization ordering

No construction-ordering machinery is needed. `GasFeeController` was
made initialization-order-agnostic upstream
([MetaMask#9569](MetaMask#9569)): the constructor
no longer eagerly resolves `NetworkController` (the `getProvider` /
`getCurrentNetworkEIP1559Compatibility` callbacks resolve it lazily at
call time), so it can be constructed before `NetworkController`.
`initialize` therefore constructs the default set in its natural order,
and the `dependencies` field / `orderByDependencies` sort that an
earlier revision of this PR added were dropped in favor of that upstream
refactor.

### `fetch` decision

`GasFeeController` fetches gas estimates via the **global `fetch`**
inside `@metamask/gas-fee-controller` (it has no injectable `fetch`
option). This PR takes **path (a): accept the global `fetch`** — no
upstream change; works on Node 18+ (wallet-cli daemon), modern browsers,
and React Native. The global-`fetch` usage lives inside the controller
package, not inside `@metamask/wallet` code, and both clients already
rely on this today. Adding an injectable `fetch` option to
`@metamask/gas-fee-controller` is tracked as a possible follow-up if the
convention is to be enforced strictly.

## Client adoption PRs

- Extension — MetaMask/metamask-extension#44534
- Mobile — MetaMask/metamask-mobile#33430

## References

- Upstream controller:
[`packages/gas-fee-controller/src/GasFeeController.ts`](https://github.com/MetaMask/core/blob/main/packages/gas-fee-controller/src/GasFeeController.ts)
- Mirror template:
[`packages/wallet/src/initialization/instances/transaction-controller/`](https://github.com/MetaMask/core/tree/main/packages/wallet/src/initialization/instances/transaction-controller)
- Extension init:
[`app/scripts/messenger-client-init/confirmations/gas-fee-controller-init.ts`](https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/messenger-client-init/confirmations/gas-fee-controller-init.ts)
- Mobile init:
[`app/core/Engine/controllers/gas-fee-controller/gas-fee-controller-init.ts`](https://github.com/MetaMask/metamask-mobile/blob/main/app/core/Engine/controllers/gas-fee-controller/gas-fee-controller-init.ts)

## Related

- Closes MetaMask#9510

## Checklist

- [x] I've read the [contributing
guidelines](https://github.com/MetaMask/core/blob/main/docs/contributing.md).
- [x] I've completed the PR template to the best of my ability.
- [x] I've included tests if applicable (100% coverage; colocated
instance test).
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable.
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/core/blob/main/docs/contributing.md#labeling-guidelines)).
Not required for external contributors.

🤖 Generated with [Claude Code](https://claude.com/claude-code)



<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Breaking API for all Wallet consumers and changes the transaction
gas-estimation path, but the integration follows existing controller
init patterns and includes dedicated tests.
> 
> **Overview**
> **Breaking:** `@metamask/wallet` now boots `GasFeeController` as part
of default initialization, so wired `TransactionController` flows can
call `GasFeeController:fetchGasFeeEstimates` instead of failing with a
missing handler.
> 
> Consumers must pass **`instanceOptions.gasFeeController`** with a
required **`clientId`** (sent as `X-Client-Id` to the gas API); other
gas settings stay optional with shared defaults (production API URLs,
network callbacks built from `NetworkController`).
> 
> `@metamask/wallet-cli` sets **`clientId: 'cli'`** on the daemon
wallet. Extension/mobile adoption is expected in separate PRs.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
baa4472. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants