Optimism Bedrock Fix + New Gas Displays #19960

segun · 2023-07-11T09:35:38Z

Explanation

This PR has lots of changes, but most of them are i18n messages and fixes to existing e2e and unit tests. So it's not all that daunting. Thanks for the review ;)

Problem: the Optimism Bedrock upgrade, which adds support to EIP1559, happened on June 6th.
Now, we see some issues with the current gas displays

whenever we send a transaction Optimism network we often display 0 on any the suggested gas fees options. This seems to be a rounding issue - fees are so low in the L2 that we end up displaying a 0 for all the options

The bug in code is that we always try to display the values in ETH, But the Optimism fees are sent from the backend in GWEI, the fix is to check if the ETH conversion is zero and then use GWEI conversion, if that is also zero, we then use WEI conversion.

See: [Bug]: Optimism Bedrock upgrade - issues with gas display for new EIP1559 transactions #19561 for more information and design considerations.
Fixes: [Bug]: Optimism Bedrock upgrade - issues with gas display for new EIP1559 transactions #19561

Screenshots/Screencaps

Before

After

Manual Testing Steps

Select Optimism network
Send EIP1559
Click Edit fees
See recommended fees are 0

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.

github-actions · 2023-07-11T09:35:51Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2023-07-12T11:26:24Z

Builds ready [4291c53]

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 (1537 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	109	226	131	25	12
		domContentLoaded	1378	1812	1537	108	52
		load	1378	1813	1537	108	52
		domInteractive	1378	1812	1537	108	52

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 bytes
ui: 3015 bytes
common: -322 bytes

codecov · 2023-07-12T11:26:48Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (1e60159) 67.78% compared to head (da9663c) 67.78%.
Report is 6 commits behind head on develop.

Files	Patch %	Lines
...app/fee-details-component/fee-details-component.js	59.52%	17 Missing ⚠️
.../components/app/gas-timing/gas-timing.component.js	78.12%	7 Missing ⚠️
...omponents/app/gas-details-item/gas-details-item.js	87.50%	2 Missing ⚠️
...s/app/approve-content-card/approve-content-card.js	50.00%	1 Missing ⚠️
...ponents/app/edit-gas-fee-icon/edit-gas-fee-icon.js	91.67%	1 Missing ⚠️
...saction-base/confirm-transaction-base.component.js	91.67%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #19960      +/-   ##
===========================================
- Coverage    67.78%   67.78%   -0.00%     
===========================================
  Files         1052     1053       +1     
  Lines        40816    40876      +60     
  Branches     10943    10955      +12     
===========================================
+ Hits         27665    27704      +39     
- Misses       13151    13172      +21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2023-07-13T11:39:21Z

Builds ready [89e9ef2]

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 (1672 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	118	310	156	42	20
		domContentLoaded	1496	1845	1671	88	42
		load	1496	1845	1672	88	42
		domInteractive	1496	1845	1671	88	42

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 bytes
ui: 3354 bytes
common: -322 bytes

metamaskbot · 2023-07-14T11:05:47Z

Builds ready [eec3426]

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 (1502 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	103	162	122	16	8
		domContentLoaded	1350	1717	1501	76	37
		load	1351	1717	1502	76	37
		domInteractive	1350	1717	1501	77	37

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 bytes
ui: 4220 bytes
common: -322 bytes

metamaskbot · 2023-07-18T08:53:31Z

Builds ready [8ab1b8a]

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 (1491 ± 32 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	100	170	120	17	8
		domContentLoaded	1363	1593	1491	67	32
		load	1363	1593	1491	67	32
		domInteractive	1363	1593	1491	67	32

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 bytes
ui: 4172 bytes
common: -322 bytes

darkwing · 2023-07-24T19:46:02Z

ui/hooks/useCurrencyDisplay.js

@@ -53,18 +53,26 @@ export function useCurrencyDisplay(
  const isUserPreferredCurrency = currency === currentCurrency;

  const value = useMemo(() => {
+    let ethDisplayValue;
+    console.log('Using display value: ', displayValue);


Console needs removal

georgewrmarshall

Hey @segun, Great start! I've left a few suggestions regarding UI and using the component library components instead of writing more CSS

ui/components/app/edit-gas-fee-icon/edit-gas-fee-icon.js

ui/components/app/edit-gas-fee-icon/index.scss

ui/components/app/gas-details-item/gas-details-item-title/gas-details-item-title.js

ui/components/app/gas-details-item/gas-details-item-title/index.scss

ui/components/app/gas-timing/gas-timing.component.js

ui/components/app/gas-timing/index.scss

ui/components/app/confirm-page-container/confirm-page-container-content/index.scss

test/e2e/tests/address-book.spec.js

metamaskbot · 2023-12-05T09:04:27Z

Builds ready [d83ac11]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-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, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1289 ± 86 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	86	171	117	27	13
		domContentLoaded	9	51	19	11	5
		load	988	1797	1289	179	86
		domInteractive	9	51	19	11	5

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: 4.27 KiB (0.06%)
common: -322 Bytes (-0.01%)

cryptotavares · 2023-12-05T10:58:53Z

test/e2e/tests/address-book.spec.js


+        await driver.clickElement({ text: 'Edit', tag: 'button' });
+
+        const [gasLimitInput, gasPriceInput] = await driver.findElements(
+          'input[type="number"]',
+        );
+        await gasPriceInput.fill('8');
+        await gasLimitInput.fill('100000');
+        await driver.clickElement({ text: 'Save', tag: 'button' });


Why is this needed? Can we not send from address book without editing gas? If much this should be a test case in the send spec, no? Edit gas price and limit before sending

Yes we can, but I've noticed instances in the CI when this fails. I am not able to replicate these failures locally, but @seaona helped me troubleshoot CI and we found out that in the cases when this fails, the confirm button below is not enabled because the fee is too small.

So the tests passed locally but intermittently fail on CI. But with this fix it passes everytime on CI.

humm ok that is interesting.. it feels that there's an issue with that test. We should try to get to the root cause of it instead of fixing by editing the gas values during the test (that seems to be a different test case). If the test is passing for now, I would recommend tackling the flaky test in different PR.
My suggestion is, do not change the test in this PR and create a task to address the specific test later and add it to our board.
what do you think?

Sure done that

test/e2e/tests/send-edit.spec.js

cryptotavares · 2023-12-05T11:01:53Z

test/e2e/tests/send-eth.spec.js

-          css: '.transaction-detail-item:nth-of-type(1) h6:nth-of-type(2)',
-          text: '0.04503836 ETH',
+          css: '.currency-display-component__text',
+          text: '0.05684869',


Is there a reason for the different amount?

Not sure, but tests are not passing with old value

ui/components/app/fee-details-component/fee-details-component.js

ui/components/multichain/pages/send/send.test.js

ui/pages/confirm-transaction-base/confirm-transaction-base.test.js

This is the 1st commit message: Add border and padding to gas component. Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> refine css Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> Base Accordion component Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> Show fee details if supports eip 1559 Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> Show maxFeePerGas as optimism fee. Show L1 fee where available Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> Display L2 values in GWEI Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> Max fee per gas no longer needed in opts Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> remove new line Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> Add new edit gas icon component. Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com> fix some UI alignment issues show fee details before transaction details. Fix Optimism edge case. Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com> Fix snapshop Rebase Fix snapshots fix snaphot Fix conflicts Fix snapshot Fixed snapshot fix snapshot Fix e2e fix e2e Fix snapshots. Fix account token list spec fix flaky tests. Fix snapshot Fix snapshots

metamaskbot · 2023-12-07T08:45:06Z

Builds ready [da9663c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-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, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1395 ± 54 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	144	114	13	6
		domContentLoaded	10	48	20	10	5
		load	1159	1600	1395	112	54
		domInteractive	10	48	20	10	5

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: 4.27 KiB (0.06%)
common: -322 Bytes (-0.01%)

metamaskbot · 2024-01-02T13:45:46Z

Missing release label release-11.8.0 on PR. Adding release label release-11.8.0 on PR and removing other release labels(release-11.9.0), as PR was added to branch 11.8.0 when release was cut.

segun added team-confirmations-secure-ux-PR PRs from the confirmations team needs-ux-ds-review needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jul 11, 2023

segun requested a review from a team as a code owner July 11, 2023 09:35

segun self-assigned this Jul 11, 2023

segun marked this pull request as draft July 11, 2023 11:30

segun force-pushed the dev-segun-new-gas-component branch 3 times, most recently from d686c19 to 4291c53 Compare July 12, 2023 10:54

segun force-pushed the dev-segun-new-gas-component branch 3 times, most recently from d5aeb78 to bcba2ef Compare July 13, 2023 09:19

segun force-pushed the dev-segun-new-gas-component branch from 89e9ef2 to a9f2262 Compare July 13, 2023 16:19

segun marked this pull request as ready for review July 13, 2023 16:31

segun force-pushed the dev-segun-new-gas-component branch from a9f2262 to 88ca79f Compare July 13, 2023 16:35

bschorchit mentioned this pull request Jul 13, 2023

[Extension] Display L1 and L2 gas fees for eip-1559 transactions on Polygon zkevm, zkSync Era and Linea #20012

Open

segun force-pushed the dev-segun-new-gas-component branch from eec3426 to 8ab1b8a Compare July 18, 2023 08:19

bschorchit requested a review from georgewrmarshall July 20, 2023 12:14

darkwing reviewed Jul 24, 2023

View reviewed changes

georgewrmarshall reviewed Jul 24, 2023

View reviewed changes

jpuri reviewed Jul 25, 2023

View reviewed changes

test/e2e/tests/address-book.spec.js Outdated Show resolved Hide resolved

segun requested review from kumavis, brad-decker and a team as code owners August 14, 2023 15:51

bschorchit mentioned this pull request Nov 21, 2023

[Bug]: Inaccurate gas estimate shown from actual transaction sent on Optimism OP Optimistic L2 #21898

Closed

segun force-pushed the dev-segun-new-gas-component branch from baf82ac to b11f098 Compare November 22, 2023 11:48

bschorchit mentioned this pull request Nov 22, 2023

[Bug]: Not enough gas blocks user transaction despite having enough funds on Optimism OP Optimistic L2 #21899

Closed

segun force-pushed the dev-segun-new-gas-component branch 2 times, most recently from 859de2c to 727ef5e Compare November 28, 2023 06:56

segun force-pushed the dev-segun-new-gas-component branch 2 times, most recently from 66ca5b4 to d83ac11 Compare December 4, 2023 13:39

cryptotavares reviewed Dec 5, 2023

View reviewed changes

test/e2e/tests/send-edit.spec.js Show resolved Hide resolved

cryptotavares reviewed Dec 5, 2023

View reviewed changes

test/e2e/tests/send-edit.spec.js Show resolved Hide resolved

cryptotavares reviewed Dec 5, 2023

View reviewed changes

ui/components/app/fee-details-component/fee-details-component.js Outdated Show resolved Hide resolved

cryptotavares reviewed Dec 5, 2023

View reviewed changes

ui/components/multichain/pages/send/send.test.js Show resolved Hide resolved

cryptotavares reviewed Dec 5, 2023

View reviewed changes

ui/pages/confirm-transaction-base/confirm-transaction-base.test.js Outdated Show resolved Hide resolved

segun added 2 commits December 5, 2023 16:34

Fix PR Comments.

961b1b4

segun force-pushed the dev-segun-new-gas-component branch from 9878402 to 961b1b4 Compare December 5, 2023 13:35

Merge branch 'develop' into dev-segun-new-gas-component

da9663c

cryptotavares approved these changes Dec 7, 2023

View reviewed changes

blackdevelopa approved these changes Dec 8, 2023

View reviewed changes

segun merged commit f2cf947 into develop Dec 11, 2023
67 checks passed

segun deleted the dev-segun-new-gas-component branch December 11, 2023 12:48

github-actions bot locked and limited conversation to collaborators Dec 11, 2023

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 11, 2023

metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Dec 11, 2023

gauthierpetetin added release-11.8.0 Issue or pull request that will be included in release 11.8.0 and removed release-11.9.0 Issue or pull request that will be included in release 11.9.0 labels Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimism Bedrock Fix + New Gas Displays #19960

Optimism Bedrock Fix + New Gas Displays #19960

segun commented Jul 11, 2023 •

edited

github-actions bot commented Jul 11, 2023

metamaskbot commented Jul 12, 2023

codecov bot commented Jul 12, 2023 •

edited

metamaskbot commented Jul 13, 2023

metamaskbot commented Jul 14, 2023

metamaskbot commented Jul 18, 2023

darkwing Jul 24, 2023

georgewrmarshall left a comment

metamaskbot commented Dec 5, 2023

cryptotavares Dec 5, 2023

segun Dec 5, 2023

cryptotavares Dec 5, 2023

segun Dec 7, 2023

cryptotavares Dec 5, 2023

segun Dec 5, 2023

metamaskbot commented Dec 7, 2023

metamaskbot commented Jan 2, 2024

Optimism Bedrock Fix + New Gas Displays #19960

Optimism Bedrock Fix + New Gas Displays #19960

Conversation

segun commented Jul 11, 2023 • edited

Explanation

This PR has lots of changes, but most of them are i18n messages and fixes to existing e2e and unit tests. So it's not all that daunting. Thanks for the review ;)

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jul 11, 2023

metamaskbot commented Jul 12, 2023

codecov bot commented Jul 12, 2023 • edited

Codecov Report

metamaskbot commented Jul 13, 2023

metamaskbot commented Jul 14, 2023

metamaskbot commented Jul 18, 2023

darkwing Jul 24, 2023

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

metamaskbot commented Dec 5, 2023

cryptotavares Dec 5, 2023

Choose a reason for hiding this comment

segun Dec 5, 2023

Choose a reason for hiding this comment

cryptotavares Dec 5, 2023

Choose a reason for hiding this comment

segun Dec 7, 2023

Choose a reason for hiding this comment

cryptotavares Dec 5, 2023

Choose a reason for hiding this comment

segun Dec 5, 2023

Choose a reason for hiding this comment

metamaskbot commented Dec 7, 2023

metamaskbot commented Jan 2, 2024

segun commented Jul 11, 2023 •

edited

codecov bot commented Jul 12, 2023 •

edited