Enable editing L2 gas on optimism #18217

jpuri · 2023-03-17T12:02:18Z

github-actions · 2023-03-17T12:02:34Z

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-03-17T14:29:25Z

Builds ready [a72f053]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1597 ± 54 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	95	143	111	14	7
		domContentLoaded	1453	1933	1575	109	52
		load	1474	1941	1597	112	54
		domInteractive	1453	1933	1575	109	52

Bundle size diffs

background: 0 bytes
ui: -519 bytes
common: 0 bytes

metamaskbot · 2023-03-17T14:55:49Z

Builds ready [b0be2e8]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1714 ± 64 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	108	154	126	14	7
		domContentLoaded	1518	1961	1697	131	63
		load	1518	1962	1714	134	64
		domInteractive	1518	1961	1697	131	63

Bundle size diffs

background: 0 bytes
ui: -519 bytes
common: 0 bytes

codecov · 2023-03-17T14:56:05Z

Codecov Report

Merging #18217 (0c12b15) into develop (f92e463) will decrease coverage by 1.13%.
The diff coverage is 78.57%.

❗ Current head 0c12b15 differs from pull request most recent head d51f446. Consider uploading reports for the commit d51f446 to get more accurate results

@@             Coverage Diff             @@
##           develop   #18217      +/-   ##
===========================================
- Coverage    65.15%   64.02%   -1.13%     
===========================================
  Files          936      914      -22     
  Lines        35965    35619     -346     
  Branches      9231     9032     -199     
===========================================
- Hits         23432    22804     -628     
- Misses       12533    12815     +282

Impacted Files	Coverage Δ
...ed-gas-controls/advanced-gas-controls.component.js	`42.86% <ø> (-3.81%)`	⬇️
...vanced-gas-inputs/advanced-gas-inputs.component.js	`90.77% <ø> (+1.22%)`	⬆️
...vanced-gas-inputs/advanced-gas-inputs.container.js	`100.00% <ø> (ø)`
.../pages/send/send-content/send-content.container.js	`85.71% <ø> (ø)`
ui/selectors/selectors.js	`70.35% <ø> (-0.87%)`	⬇️
.../multilayer-fee-message/multi-layer-fee-message.js	`88.00% <66.67%> (+84.00%)`	⬆️
...saction-base/confirm-transaction-base.component.js	`50.00% <80.00%> (+1.24%)`	⬆️
.../pages/send/send-content/send-content.component.js	`97.37% <100.00%> (+0.07%)`	⬆️

... and 227 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jpuri · 2023-03-20T04:20:38Z

@seaona : can you plz test optimism transactions on this PR.

metamaskbot · 2023-03-20T04:41:50Z

Builds ready [0c12b15]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1741 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	103	143	123	10	5
		domContentLoaded	1538	1978	1717	113	54
		load	1538	1997	1741	106	51
		domInteractive	1538	1978	1717	113	54

Bundle size diffs

background: 0 bytes
ui: -519 bytes
common: 0 bytes

NiranjanaBinoy

LGTM

segun · 2023-04-05T23:49:53Z

ui/components/app/multilayer-fee-message/multi-layer-fee-message.js

@@ -21,19 +21,21 @@ export default function MultilayerFeeMessage({
  const [fetchedLayer1Total, setLayer1Total] = useState(null);

  let layer1Total = 'unknown';


Any reason we're not using null or undefined here instead of the string unknown?

Unknown is string displayed to the user is L1 gas estimates are not available:

I improved it to use internationalisation.

seaona · 2023-04-06T09:47:22Z

QA findings and comments:

Whenever I try to depoy a contract, I get an error Insufficient funds for gas. It seems that gas is not correctly calculated making the transaction always fail

optimism-insufficient-gas.mp4

Updating the gas on any dapp transaction, crashes MetaMask with the error
Message: Invariant failed: A state mutation was detected between dispatches, in the path confirmTransaction.txData.simulationFails. This may cause incorrect behavior.

optimism-gas-update-crash.mp4

Approve ERC20 Custom screen displays the Layer 2 Gas Fees is always 0. For testing this you can go to this contract and click approve

approve-erc20-optimism.mp4

Layer 2 gas styling is different from Layer 1 gas styling. This happens to any dapp interaction. Seems Layer 2 should follow the same style as we have for Layer 1? @bschorchit

Whenever I set the gas limit lower than 21000 I get duplicated warnings/messages. I see a warning advising to set gas limit above 21000. This seems a recommendation, but at the same time, I see an input validation which errors and says value must be grater than 21000 and the button is disabled. Could we consolidate this messages so they are not duplicated/ indicating different things (one recommends, the other enforces)? No need to be in this PR, but something to consider. What do you think? @bschorchit @jpuri

Layer 2 gas fee does not display Fiat conversion. Should we also display it in the same way we do for Layer 1 fees and Total fields? @bschorchit

Whenever I deactivate the price checker, I still see the fiat conversion on the Send Confirmation screen. I think this is a pre-existing problem and can be solved separately

disable-fiat-conversion.mp4

Whenever a transaction is going to fail and MM knows beforehand, we display a gas value of unkown for the Layer 1 fees. I think this could be treated as a separate issue, and it's related to how MetaMask handles failing transactions. [Bug]: Failing Transactions don't display a Warning anymore and set an insane amount of Gas fees #17379 . Also could be tackled in a different issue? @bschorchit @jpuri

optimism-failing-tx-gas-unknown.mp4

seaona

Some of the issues in my comment might need to be addressed in this PR and some we can open separate issues

…sk-extension into optimism_gas_editing_fix

jpuri · 2023-04-06T14:59:07Z

Hello @seaona : I fixed these issues:

display fiat value for L2 gas value
Fix L2 on approval pages

But I am not able to replicate the errors reported above. Also, I could deploy contract successfully.

jpuri · 2023-04-18T09:08:02Z

@seaona : thanks for the finding, I fixed the issue. There is a lot happening in develop simultaneously and updating the PR brings about more issues 😄

metamaskbot · 2023-04-18T09:46:19Z

Builds ready [edefce9]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1528 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	90	295	114	46	22
		domContentLoaded	1318	1725	1502	103	49
		load	1334	1725	1528	105	50
		domInteractive	1318	1725	1502	103	50

Bundle size diffs

background: 0 bytes
ui: 2055 bytes
common: 0 bytes

seaona · 2023-04-18T16:22:55Z

@jpuri thank you for the fix!! Indeed there are a lot of changes that impact and it's a complex PR.
I see the above issue fixed. Unfortunately I found another issue, which happens whenever we trigger x2 transactions from a Dapp, the second one has default Layer 2 fees as 0.

Repro:

Go to test dapp https://metamask.github.io/test-dapp/
Click Send twice
Navigate to the second trx
See layer 2 fees are set to 0

optimism-fees-multiple-tx.webm

… optimism_gas_editing_fix

jpuri · 2023-04-19T04:58:40Z

Hey @seaona : this seems to be issue in develop, may be related to recent gas fee display PR merged. The last transaction is showing 0 estimated gas even on goreli:

metamaskbot · 2023-04-19T05:23:33Z

Builds ready [6149836]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1697 ± 53 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	166	125	19	9
		domContentLoaded	1543	1968	1693	109	52
		load	1543	1968	1697	110	53
		domInteractive	1543	1968	1693	109	52

Bundle size diffs

background: 0 bytes
ui: 2055 bytes
common: 0 bytes

seaona · 2023-04-19T15:42:55Z

Great finding @jpuri ! I've opened a separate issue here [Bug]: Triggering a 2nd tx from a Dapp sets the gas Limit to 0

This PR looks good from QA 💯 great job!!

metamaskbot · 2023-04-20T00:40:13Z

Builds ready [78c766b]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1683 ± 77 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	162	122	18	9
		domContentLoaded	1443	2177	1661	164	79
		load	1443	2177	1683	160	77
		domInteractive	1443	2177	1661	164	79

Bundle size diffs

background: 0 bytes
ui: 2055 bytes
common: 0 bytes

metamaskbot · 2023-04-24T08:15:59Z

Builds ready [91524fd]

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

Page Load Metrics (1732 ± 67 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	161	123	15	7
		domContentLoaded	1446	2016	1707	136	65
		load	1531	2037	1732	140	67
		domInteractive	1446	2016	1707	136	65

Bundle size diffs

background: 0 bytes
ui: 2055 bytes
common: 0 bytes

metamaskbot · 2023-04-25T07:02:52Z

Builds ready [d51f446]

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

Page Load Metrics (1487 ± 44 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	90	134	105	10	5
		domContentLoaded	1384	1690	1466	70	34
		load	1384	1770	1487	91	44
		domInteractive	1383	1690	1466	70	34

Bundle size diffs

background: 0 bytes
ui: 2275 bytes
common: 0 bytes

NiranjanaBinoy

LGTM

Change has been implemented.

Enable editing L2 gas on optimism

2cff25b

jpuri added 3 commits March 17, 2023 16:09

fix lint issue

657421c

fix build

da8f584

fix lint issue

a72f053

Merge branch 'develop' into optimism_gas_editing_fix

b0be2e8

Merge branch 'develop' into optimism_gas_editing_fix

0c12b15

jpuri marked this pull request as ready for review March 20, 2023 04:16

jpuri requested a review from a team as a code owner March 20, 2023 04:16

jpuri requested a review from garrettbear March 20, 2023 04:16

jpuri requested review from NiranjanaBinoy and digiwand and removed request for garrettbear March 20, 2023 12:40

segun previously approved these changes Mar 22, 2023

View reviewed changes

NiranjanaBinoy previously approved these changes Apr 5, 2023

View reviewed changes

segun reviewed Apr 5, 2023

View reviewed changes

jpuri added the needs-qa Label will automate into QA workspace label Apr 6, 2023

seaona previously requested changes Apr 6, 2023

View reviewed changes

jpuri added 3 commits April 6, 2023 20:25

Fix issues

a0f1086

merge

adf3b95

Merge branch 'optimism_gas_editing_fix' of github.com:MetaMask/metama…

35da64c

…sk-extension into optimism_gas_editing_fix

jpuri dismissed stale reviews from NiranjanaBinoy and segun via 35da64c April 6, 2023 14:57

fix

71c151c

fix build

edefce9

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

6149836

… optimism_gas_editing_fix

Merge branch 'develop' into optimism_gas_editing_fix

78c766b

jpuri added 2 commits April 21, 2023 11:16

Merge branch 'develop' into optimism_gas_editing_fix

75fbc31

Merge branch 'develop' into optimism_gas_editing_fix

8afca20

danjm previously approved these changes Apr 21, 2023

View reviewed changes

Merge branch 'develop' into optimism_gas_editing_fix

91524fd

seaona removed the needs-qa Label will automate into QA workspace label Apr 24, 2023

merge

d51f446

jpuri dismissed danjm’s stale review via d51f446 April 25, 2023 06:30

jpuri requested review from danjm and segun April 25, 2023 07:28

danjm approved these changes Apr 25, 2023

View reviewed changes

NiranjanaBinoy approved these changes Apr 25, 2023

View reviewed changes

jpuri merged commit 82a6419 into develop Apr 26, 2023

jpuri deleted the optimism_gas_editing_fix branch April 26, 2023 00:13

github-actions bot locked and limited conversation to collaborators Apr 26, 2023

danjm added release-10.30.0 Issue or pull request that will be included in release 10.30.0 release-blocker This bug is blocking the next release labels May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable editing L2 gas on optimism #18217

Enable editing L2 gas on optimism #18217

jpuri commented Mar 17, 2023

github-actions bot commented Mar 17, 2023

metamaskbot commented Mar 17, 2023

metamaskbot commented Mar 17, 2023

codecov bot commented Mar 17, 2023 •

edited

Loading

jpuri commented Mar 20, 2023

metamaskbot commented Mar 20, 2023

NiranjanaBinoy left a comment

segun Apr 5, 2023

jpuri Apr 10, 2023

seaona commented Apr 6, 2023

seaona left a comment

jpuri commented Apr 6, 2023

jpuri commented Apr 18, 2023

metamaskbot commented Apr 18, 2023

seaona commented Apr 18, 2023

jpuri commented Apr 19, 2023

metamaskbot commented Apr 19, 2023

seaona commented Apr 19, 2023

metamaskbot commented Apr 20, 2023

metamaskbot commented Apr 24, 2023

metamaskbot commented Apr 25, 2023

NiranjanaBinoy left a comment

		@@ -21,19 +21,21 @@ export default function MultilayerFeeMessage({
		const [fetchedLayer1Total, setLayer1Total] = useState(null);

		let layer1Total = 'unknown';

Enable editing L2 gas on optimism #18217

Enable editing L2 gas on optimism #18217

Conversation

jpuri commented Mar 17, 2023

github-actions bot commented Mar 17, 2023

metamaskbot commented Mar 17, 2023

metamaskbot commented Mar 17, 2023

codecov bot commented Mar 17, 2023 • edited Loading

Codecov Report

jpuri commented Mar 20, 2023

metamaskbot commented Mar 20, 2023

NiranjanaBinoy left a comment

Choose a reason for hiding this comment

segun Apr 5, 2023

Choose a reason for hiding this comment

jpuri Apr 10, 2023

Choose a reason for hiding this comment

seaona commented Apr 6, 2023

seaona left a comment

Choose a reason for hiding this comment

jpuri commented Apr 6, 2023

jpuri commented Apr 18, 2023

metamaskbot commented Apr 18, 2023

seaona commented Apr 18, 2023

jpuri commented Apr 19, 2023

metamaskbot commented Apr 19, 2023

seaona commented Apr 19, 2023

metamaskbot commented Apr 20, 2023

metamaskbot commented Apr 24, 2023

metamaskbot commented Apr 25, 2023

NiranjanaBinoy left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 17, 2023 •

edited

Loading