Fix fallback gas estimation by Gudahtt · Pull Request #19746 · MetaMask/metamask-extension

Gudahtt · 2023-06-23T21:26:00Z

Explanation

Our fallback gas estimation was failing due to a bug in the @metamask/controller-utils package. This was causing gas estimation to fail completely on certain networks (those not supported by our gas estimation APIs and non EIP-1559 compatibile), and it was causing the fallback gas estimation operation (in case our API was down) to fail across all networks.

Additionally, the legacy gas API has been disabled as part of this PR. This was required in order to cover this functionality with e2e tests (if the legacy gas API was enabled, that would take precedence over the code this PR is fixing).

I tried removing the legacy gas API in a separate PR, but that depends upon this bug fix, so they need to be changed together.

A few test IDs were added for the purpose of helping with writing the e2e tests.

Fixes #19735

Manual Testing Steps

Switch to the BNB network, and start the send or swaps flow. You should see an error like the one shown in the referenced issue if you're testing with develop or the v10.33.0 release candidate. With this PR, the error no longer appears.

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

Gudahtt · 2023-06-23T21:29:12Z

@metamaskbot update-policies

metamaskbot · 2023-06-23T21:45:31Z

No policy changes

metamaskbot · 2023-06-23T21:58:56Z

Builds ready [7126883]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1750 ± 70 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	113	231	159	35	17
		domContentLoaded	1526	2115	1750	145	70
		load	1527	2115	1750	145	70
		domInteractive	1526	2115	1750	145	70

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 bytes
ui: 0 bytes
common: -16 bytes

codecov · 2023-06-23T21:59:00Z

Codecov Report

Merging #19746 (faa1753) into develop (3df690b) will increase coverage by 0.01%.
The diff coverage is 88.89%.

@@             Coverage Diff             @@
##           develop   #19746      +/-   ##
===========================================
+ Coverage    69.85%   69.86%   +0.01%     
===========================================
  Files          980      980              
  Lines        36911    36913       +2     
  Branches      9909     9909              
===========================================
+ Hits         25782    25786       +4     
+ Misses       11129    11127       -2

Impacted Files	Coverage Δ
app/scripts/metamask-controller.js	`65.75% <0.00%> (+0.12%)`	⬆️
...n-detail-item/transaction-detail-item.component.js	`100.00% <ø> (ø)`
...nts/app/confirm-gas-display/confirm-gas-display.js	`100.00% <100.00%> (ø)`
...m-legacy-gas-display/confirm-legacy-gas-display.js	`93.55% <100.00%> (+0.22%)`	⬆️
...omponents/app/gas-details-item/gas-details-item.js	`100.00% <100.00%> (ø)`

metamaskbot · 2023-06-26T13:05:02Z

Builds ready [962fec1]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1435 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	105	170	124	17	8
		domContentLoaded	1357	1585	1435	57	27
		load	1357	1585	1435	57	27
		domInteractive	1357	1585	1435	57	27

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 bytes
ui: 0 bytes
common: -16 bytes

Our fallback gas estimation was failing due to a bug in the `@metamask/controller-utils` package. This was causing gas estimation to fail completely on certain networks (those not supported by our gas estimation APIs and non EIP-1559 compatibile), and it was causing the fallback gas estimation operation (in case our API was down) to fail across all networks. Fixes #19735

E2E tests have been added to capture gas estimation. Cases are added for our API, for the fallback estimate, and for non-EIP-1559 estimates. As part of this work, the legacy gas API had to be disabled. This was being used in e2e tests but was dead code in production. It needed to be disabled to ensure the code under test was reachable.

Gudahtt · 2023-06-26T17:46:21Z

+  });
+
+  describe('Send on a network that is not EIP-1559 compatible', function () {
+    it('show expected gas defaults', async function () {


This test was failing prior to this PR

Gudahtt · 2023-06-26T17:46:57Z

+    hardfork: 'london',
+  };
+
+  describe('Send on a network that is EIP-1559 compatible', function () {


These tests were already passing (both before and after this PR), but I included them here just to have all four code paths covered in the same file.

Gudahtt · 2023-06-26T17:54:08Z

This has been updated with e2e tests

metamaskbot · 2023-06-26T18:28:46Z

Builds ready [faa1753]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1675 ± 64 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	114	318	153	45	21
		domContentLoaded	1421	1955	1675	134	64
		load	1422	1955	1675	134	64
		domInteractive	1421	1954	1675	134	64

Bundle size diffs [🚀 Bundle size reduced!]

background: -124 bytes
ui: 384 bytes
common: -16 bytes

danjm

LGTM, nice work on the e2e tests

Gudahtt · 2023-06-26T18:42:27Z

      EIP1559APIEndpoint: `${gasApiBaseUrl}/networks/<chain_id>/suggestedGasFees`,
-      getCurrentNetworkLegacyGasAPICompatibility: () => {
-        const { chainId } = this.networkController.state.providerConfig;
-        return process.env.IN_TEST || chainId === CHAIN_IDS.MAINNET;


I've since discovered that this legacy gas endpoint remains useful for non-EIP1559 chains that our APIs support (e.g. BSC and Polygon). But we can restore this for those chains in a later PR, they weren't using it yet anyway.

Gudahtt requested a review from a team as a code owner June 23, 2023 21:26

Gudahtt requested a review from hmalik88 June 23, 2023 21:26

Gudahtt force-pushed the fix-non-EIP-1559-gas-estimates branch from 8478d3c to 7126883 Compare June 23, 2023 21:28

Gudahtt force-pushed the fix-non-EIP-1559-gas-estimates branch 2 times, most recently from 569c56b to 962fec1 Compare June 26, 2023 12:33

brad-decker previously approved these changes Jun 26, 2023

View reviewed changes

Gudahtt dismissed brad-decker’s stale review via 142f264 June 26, 2023 17:41

Gudahtt force-pushed the fix-non-EIP-1559-gas-estimates branch from 962fec1 to 142f264 Compare June 26, 2023 17:41

Gudahtt added 2 commits June 26, 2023 15:14

Gudahtt force-pushed the fix-non-EIP-1559-gas-estimates branch from 142f264 to cde2190 Compare June 26, 2023 17:44

Gudahtt commented Jun 26, 2023

View reviewed changes

Fix gas API referenced in e2e test

46e3559

Update unit test snapshots

faa1753

darkwing approved these changes Jun 26, 2023

View reviewed changes

danjm approved these changes Jun 26, 2023

View reviewed changes

Gudahtt commented Jun 26, 2023

View reviewed changes

Gudahtt merged commit ec7e7fd into develop Jun 26, 2023

Gudahtt deleted the fix-non-EIP-1559-gas-estimates branch June 26, 2023 18:43

github-actions Bot locked and limited conversation to collaborators Jun 26, 2023

metamaskbot added the release-10.34.0 Issue or pull request that will be included in release 10.34.0 label Jun 26, 2023

danjm removed the release-10.34.0 Issue or pull request that will be included in release 10.34.0 label Jun 27, 2023

danjm added release-blocker This bug is blocking the next release release-10.33.0 Issue or pull request that will be included in release 10.33.0 labels Jun 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fallback gas estimation#19746

Fix fallback gas estimation#19746
Gudahtt merged 4 commits intodevelopfrom
fix-non-EIP-1559-gas-estimates

Gudahtt commented Jun 23, 2023 •

edited

Loading

Uh oh!

Gudahtt commented Jun 23, 2023

Uh oh!

metamaskbot commented Jun 23, 2023

Uh oh!

metamaskbot commented Jun 23, 2023

Uh oh!

codecov Bot commented Jun 23, 2023 •

edited

Loading

Uh oh!

metamaskbot commented Jun 26, 2023

Uh oh!

Gudahtt Jun 26, 2023

Uh oh!

Gudahtt Jun 26, 2023

Uh oh!

Gudahtt commented Jun 26, 2023

Uh oh!

metamaskbot commented Jun 26, 2023

Uh oh!

danjm left a comment

Uh oh!

Gudahtt Jun 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Gudahtt commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

Gudahtt commented Jun 23, 2023

Uh oh!

metamaskbot commented Jun 23, 2023

Uh oh!

metamaskbot commented Jun 23, 2023

Uh oh!

codecov Bot commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Jun 26, 2023

Uh oh!

Gudahtt Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

Gudahtt Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

Gudahtt commented Jun 26, 2023

Uh oh!

metamaskbot commented Jun 26, 2023

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Gudahtt commented Jun 23, 2023 •

edited

Loading

codecov Bot commented Jun 23, 2023 •

edited

Loading