metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Include MultiLayer Gas Fee information for Scroll

Open dghelm opened this issue 1 year ago • 9 comments

Description

Motivation

The changes in this PR add MultiLayer Gas Fee information for Scroll. Its goal is to meet users' expectations that have used Op stack networks like Optimism and Base. In those networks, the multi-dimensional gas (L2 Fee for normal gas stuff and L1 Fee for data availablility) is presented to the user for legacy txs, so they have a clear expectation of transaction fees. Arbitrum and Linea take another approach -- fully hide the fee breakdown from users and just call it "gas," which gives an equally consistent user experience.

For Scroll, the L2 fee (which works nearly identically to Op Stack's) is also not included in estimate_gas calls, but users have been reporting poor experiences using MetaMask with Scroll because it's missing much of the fee paid for transactions. This PR will go a long way to helping users understand and have a consistent experience across Op and Scroll.

Note: Currently, Scroll does not support type-2 (EIP-1559) transactions, so Optimism's type-0 tx code works sufficiently for the needs of Scroll users.

Implementation

Key changes:

  • Adds Scroll and Scroll Sepolia as networks in shared/constants/network.ts
  • Creates a new "multiLayerFee" directory to organize L2 gas functionality (and moves optimism code into a sub dir of it), and updates imports referencing these files
  • Abstracts "multilayerfeenetwork" across networks, and invokes (minimally) different logic for OpStack v Scroll chains. This also lets other chains integrate logic (or, eventually I imagine, other OpStack logic as chain designs diversify), without having to touch core logic files.
  • Removes several methods from ui/selectors/selectors.js, moving the functionality to ui/helpers/utils/multiLayerFee/networks.js
    • This avoids making Hook calls against the redux state for every chainId (would be costly if OpStack reaches their 1M rollup vision, also just bloats the list of selectors)
    • Allows this list of chainIds and helper methods to be called from places that don't have access to the redux state.
    • NOTE: These methods were exported but not used elsewhere in the codebase. New methods only exported if used elsewhere.

(Thanks to @pajicf for the initial work on this PR during Scroll Alpha testnet)

Related issues

None identified. Tangentially related to #21139 , but doesn't fix it. (This PR adds current legacy tx support to Scroll, doesn't extend the feature to 1559 txs.)

Manual testing steps

  1. Add Scroll or Scroll Sepolia networks using Chainlist.
  2. If needing Scroll Sepolia tokens, use a faucet, or bridge Sepolia to Scroll Sepolia here. (Or reply with a wallet address and I'll send some over.)
  3. Start a tx. "Send" is easiest, but the bridge linked above works, or try a swap on a testnet Uniswap showcase.

Screenshots/Recordings

NOTE: Please see this comment for latest screenshots.

Before

image

Send: image image image

After

image

Send: image image image

Pre-merge author checklist

  • [x] I’ve followed MetaMask Coding Standards.
  • [x] I've clearly explained what problem this PR is solving and how it is solved.
  • [x] I've linked related issues
  • [x] I've included manual testing steps
  • [x] I've included screenshots/recordings if applicable
  • [x] I’ve included tests if applicable
  • [x] I’ve documented my code using JSDoc format if applicable
  • [ ] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • [x] I’ve properly set the pull request status:
    • [x] In case it's not yet "ready for review", I've set it to "draft".
    • [x] In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

dghelm avatar Nov 08 '23 03:11 dghelm

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.

github-actions[bot] avatar Nov 08 '23 03:11 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

dghelm avatar Nov 08 '23 03:11 dghelm

Codecov Report

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

Comparison is base (2f598dc) 67.70% compared to head (c78e46e) 68.51%.

:exclamation: Current head c78e46e differs from pull request most recent head 3f457ca. Consider uploading reports for the commit 3f457ca to get more accurate results

Files Patch % Lines
.../utils/multiLayerFee/scroll/fetchEstimatedL1Fee.js 74.07% 7 Missing :warning:
...helpers/utils/multiLayerFee/fetchEstimatedL1Fee.js 75.00% 4 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #21733      +/-   ##
===========================================
+ Coverage    67.70%   68.51%   +0.82%     
===========================================
  Files         1044     1049       +5     
  Lines        40478    41696    +1218     
  Branches     10880    11112     +232     
===========================================
+ Hits         27403    28568    +1165     
- Misses       13075    13128      +53     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 14 '23 01:11 codecov[bot]

Hi @bschorchit -- thanks for tagging the issue. As a new contributor to MetaMask, I'm unsure of the process at this point. Is there anything I can do to help bring attention to this PR? Happy to do a pair session or something else if it helps a reviewer get context more quickly.

dghelm avatar Jan 17 '24 14:01 dghelm

Hi @dghelm, sorry for the delay in getting to this PR. We'll be prioritizing this soon (expect 1-2 weeks).

bschorchit avatar Feb 05 '24 22:02 bschorchit

Excited to have a review, but when I went to pull in the develop branch, I guess there are changes that have broken my implementation -- new view: image

I'll try to get a fix submitted quickly.

dghelm avatar Feb 06 '24 03:02 dghelm

Okay, I started to make minimal changes, but the last PR for Op Bedrock support seems to be pretty regressive for legacy txs. Here's what a legacy tx currently looks like on the develop branch: image

Note: states aren't shared between "details" pane and the main pane, labels are mismatched, and different fields don't add up correctly. Also, the information is displayed twice which is odd in itself.

My suggested changes, which I'm not sure how intrusive would need to be:

  • Change "Optimism Fee" label to "Execution Fee". We could say "Layer 2 Fee," but this seemed like a nice generic middle ground before gas fees get more complex to include additional dimensions. Similarly, "Layer 1 Fee" might be better labeled "L1 Data Fee".
  • Change the fees above "details" to only state "estimated fee" (sum of Execution Fee and L1 Data Fee")
  • Explore making "Details" component update its state as L2 gas fees or L1 data fees are updated.

dghelm avatar Feb 06 '24 05:02 dghelm

A number of updates made to adjust for the new UI changes.

Fixes:

  • in type2 transactions, Total gas now includes L1 & L2 fees.

Changes:

  • renamed "Optimism Fee" to "Execution Fee"
  • dropped "multilayer fee message" and instead let the details pane show this information. Now showing a single "estimate" just like type-2 transactions

Follow-Up:

  • state management is fairly messy across components for feature, and could likely be better optimized by treating it like tx gas estimates, but I imagine this is outside the scope of an outside contribution (or at least the ability of this outside contributor :) )

Example Screenshots (Before is above):

Initial confirmation window (legacy tx): image

Showing details: image

Showing OP type-2 tx, with correct final "Total" calculated including Layer 1 fee: image

dghelm avatar Feb 11 '24 06:02 dghelm

Thank you for the latest addition and contributions, the team will be wrapping up a work this week that is currently blocking moving on to this. Sorry for the delay, but we're planning to get to this soon.

bschorchit avatar Feb 19 '24 03:02 bschorchit

@bschorchit Curious if you have any update on the blocking work?

icemelon avatar Mar 06 '24 00:03 icemelon

New push to resolve merge conflicts (updating for Op Sepolia testnet support).

@bschorchit please let me know if there's anything else on my end to keep an eye on with Dencun upgrades :)

dghelm avatar Mar 06 '24 18:03 dghelm

Hello @dghelm,

First and foremost, thank you very much for your valuable contribution!

I believe @bschorchit will provide an update shortly. However, I wanted to inform you that the team has decided to pause this PR temporarily. This decision comes as we are in the midst of transitioning to incorporate Layer1GasFeeFlows into the transaction controller, as seen here. I am currently doing the implementation of this feature in the extension, starting with the Optimism flow.

To clarify the sequence of implementation, it will proceed as follows:

  1. https://github.com/MetaMask/core/pull/4055 - Implementation of OptimismGasFeeFlow in core.
  2. https://github.com/MetaMask/metamask-extension/pull/23521 - Extension PR which is updating the transaction-controller package to include the OptimismGasFeeFlow implementation.
  3. A PR link will be provided soon - Implementation of ScrollLayer1GasFeeFlow in core.
  4. A PR link will be provided soon - Extension PR which is updating the transaction-controller package to include the ScrollLayer1GasFeeFlow implementation.

Upon reviewing this PR, I noticed only minor differences compared to the Optimism flow. Therefore, I will be preparing the upcoming PRs for Scroll, and your initial PR will be instrumental in this process (thank you once again).

As soon as the final PR is prepared, I will reach out to you, @dghelm, to conduct the final manual QA.

OGPoyraz avatar Mar 14 '24 13:03 OGPoyraz

Upon reviewing this PR, I noticed only minor differences compared to the Optimism flow. Therefore, I will be preparing the upcoming PRs for Scroll, and your initial PR will be instrumental in this process (thank you once again). As soon as the final PR is prepared, I will reach out to you, @dghelm, to conduct the final manual QA.

Excited to see the progress @OGPoyraz ! Would it be helpful (or speed things up) if I or someone from our team used your Optimism code changes to start PR drafts to core and extension for the adjustments needed for Scroll?

dghelm avatar Mar 26 '24 19:03 dghelm

Update: addition of Scroll to the new Transaction Controller behavior is implemented in #23991

dghelm avatar May 01 '24 16:05 dghelm