metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard
verified publisher icon MetaMask

Fix/use etherjs specific imports

Open amerkadicE opened this issue 3 years ago • 19 comments

Explanation

We don't need to import both whole ethers npm package. This PR replaces all imports of ethers npm umbrella package with a specific ethers package.

More Information

Fixes #15232

Screenshots/Screencaps

Before

Current bundle size on develop "background": { "name": "background", "size": 76085955

"ui": { "name": "ui", "size": 94503010,

Google Chrome build size after removing web3 npm package.

"background": { "name": "background", "size": 10236142,

"ui": { "name": "ui", "size": 12504705,

image

After

Google Chrome build size after removing ethers umbrella npm package. "background": { "name": "background", "size": 13145874,

"ui": { "name": "ui", "size": 16133810,

image

Manual Testing Steps

Check if the extension works correctly. There are a lot of places where we changed ethers package.

Pre-Merge Checklist

  • [x] PR template is filled out
  • [ ] IF this PR fixes a bug, a test that would have caught the bug has been added
  • [x] PR is linked to the appropriate GitHub issue
  • [ ] PR has been added to the appropriate release Milestone

+ If there are functional changes:

  • [ ] Manual testing complete & passed
  • [ ] "Extension QA Board" label has been applied

amerkadicE avatar Aug 04 '22 13:08 amerkadicE

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 Aug 04 '22 13:08 github-actions[bot]

Builds ready [89a1ff0]
Page Load Metrics (2107 ± 126 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint912351193015
domContentLoaded179127832075254122
load182329182107262126
domInteractive179127832075254122

highlights:

storybook

metamaskbot avatar Aug 18 '22 11:08 metamaskbot

Builds ready [0367001]
Page Load Metrics (1947 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint992651373919
domContentLoaded17712214193010349
load17712214194711857
domInteractive17712214193010349

highlights:

storybook

metamaskbot avatar Aug 19 '22 11:08 metamaskbot

seconding @danjm 's message above, in the before and after all the numbers you posted look good (reduction in bundle size) but the image of the zip files show an increase in total size. We should at least understand that portion and where the size is coming from.

brad-decker avatar Aug 19 '22 14:08 brad-decker

I was trying this approach to measuring size increase as well - looking at the zip file size - but I'm not sure if its a valid way to measure...? @brad-decker @Gudahtt what do you think?

adonesky1 avatar Aug 22 '22 16:08 adonesky1

Builds ready [492f884]
Page Load Metrics (2139 ± 92 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1071731342110
domContentLoaded19002557210917986
load19002557213919292
domInteractive19002557210917986

highlights:

storybook

metamaskbot avatar Sep 05 '22 07:09 metamaskbot

@danjm I investigated why we have bundle sizes bigger after using specific ethers packages.

It looks like every specific ethers package will install node modules that he needs, so it will cause multiple installations of the same package. Like in the picture below.

image

When we install umbrella ethers, the size of that node module is 8,5 MB. When we install only specific packages that we use, the size of that node module is 25,5MB.

amerkadicE avatar Sep 06 '22 10:09 amerkadicE

@danjm I investigated why we have bundle sizes bigger after using specific ethers packages.

It looks like every specific ethers package will install node modules that he needs, so it will cause multiple installations of the same package. Like in the picture below.

image

When we install umbrella ethers, the size of that node module is 8,5 MB. When we install only specific packages that we use, the size of that node module is 25,5MB.

looking into this.

adonesky1 avatar Sep 06 '22 20:09 adonesky1

So at least part of the problem is that any places in our dependency tree that ethers is imported rather than the component @ethersproject/<subfolder> it makes sure that we will have atleast one fixed version import of each @ethersproject/ dependency, since each version of ethers depends on the same fixed equivalent version of each of the @ethersproject/ components: 5.7.0, 5.6.0, etc. So we should try to get rid of all uses of the "umbrella" ethers package in our dependency tree if possible, otherwise de-duping our way out of this will become harder. Yes de-duping means we only end up with one fixed (no ^ or >) version of all the @ethersproject/ packages but still.

adonesky1 avatar Sep 06 '22 21:09 adonesky1

Actually not sure if we can do this since @ethersproject/hardware-wallets depends on ethers itself...

adonesky1 avatar Sep 06 '22 21:09 adonesky1

Looks like upgrading our version of @eth-optimism/contracts could help... @danjm do you know if this will require complex adaptations, seems like we're pretty far behind: currently .0.0-2021919175625 -> latest 0.5.33

adonesky1 avatar Sep 06 '22 21:09 adonesky1

Looks like upgrading our version of @eth-optimism/contracts could help... @danjm do you know if this will require complex adaptations, seems like we're pretty far behind: currently .0.0-2021919175625 -> latest 0.5.33

Looks like the interfaces haven't changed for the functions/objects we use

adonesky1 avatar Sep 06 '22 21:09 adonesky1

Looks like the interfaces haven't changed for the functions/objects we use @adonesky1 I agree. I don't think updating will be breaking for us

danjm avatar Sep 07 '22 18:09 danjm

So despite consolidating all of the @ethersproject/ components to a single version, i'm still seeing the size increase about the same amount. Looking at the remaining instances of ethers in our dependency graph: Screen Shot 2022-09-07 at 2 19 20 PM

we can easily move @metamask/controllers and @metamask/smart-transactions-controller to use the specific imports ourselves, but will need eth-optimism team to update @eth-optimism/core-utils to use specific imports instead of ethers if we want to get it out of our dependency tree altogether.

Do we think its worth asking them to make that update for us @danjm? almost certainly doesn't make sense to make our own fork just for this...

adonesky1 avatar Sep 07 '22 19:09 adonesky1

I've pinged the Optimism team about this here: https://consensys.slack.com/archives/C01KXHPU3C6/p1662579353556779

adonesky1 avatar Sep 07 '22 19:09 adonesky1

Builds ready [52ec9c6]
Page Load Metrics (1895 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint95164128189
domContentLoaded16972351186514368
load16972482189516479
domInteractive16972351186514368

highlights:

storybook

metamaskbot avatar Sep 07 '22 22:09 metamaskbot

the Optimism team has graciously opened PR that will help us remove the umbrella package ethers from our dependency tree: https://github.com/ethereum-optimism/optimism/pull/3363. Once they publish we can pull that in and update here.

adonesky1 avatar Sep 08 '22 21:09 adonesky1

Working with eth-optimism team to get their new build right.

adonesky1 avatar Sep 13 '22 14:09 adonesky1

Currently waiting for updated build from Optimism: thread

adonesky1 avatar Sep 20 '22 18:09 adonesky1

@adonesky1 are there any updates relevant to this PR that can be shared?

brad-decker avatar Nov 04 '22 17:11 brad-decker

Builds ready [98092de]
Page Load Metrics (1333 ± 105 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97135113115
domContentLoaded98617191305220105
load104817931333219105
domInteractive98617191305220105
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 76238 bytes
  • ui: 197263 bytes
  • common: -183927 bytes

highlights:

storybook

metamaskbot avatar Jan 19 '23 00:01 metamaskbot

Builds ready [cb30ace]
Page Load Metrics (1225 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint94181122199
domContentLoaded10341579121417584
load10341579122516981
domInteractive10341579121417584
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 2992 bytes
  • ui: 218941 bytes
  • common: -217287 bytes

highlights:

storybook

metamaskbot avatar Jan 19 '23 16:01 metamaskbot

The only outstanding instance of ethers in the extensions's dependency graph is from @ensdomains/ensjs v2.1.0 via @truffle/encoder / @truffle/decoder. @truffle/decoder is used by the transactions-decoding component, and probably can't be easily replaced but @ensdomains/ensjs has an alpha version (3.0.0) which removes it's dependency on ethers so perhaps we ask Truffle to update this dependency to use the alpha version of @ensdomains/ensjs? cc @danjm ~I will write up a separate ticket to follow up on this~ Here is a ticket tracking this: https://github.com/MetaMask/metamask-extension/issues/17298

adonesky1 avatar Jan 19 '23 17:01 adonesky1

Builds ready [d3bdcf3]
Page Load Metrics (1404 ± 137 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1002221412713
domContentLoaded103918841388278133
load103919641404286137
domInteractive103918841387278133
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 3205 bytes
  • ui: 225326 bytes
  • common: -223672 bytes

metamaskbot avatar Jan 23 '23 16:01 metamaskbot

Builds ready [eb4d944]
Page Load Metrics (1338 ± 96 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97144118157
domContentLoaded10461715131118086
load10461782133819996
domInteractive10461715131018086
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 2768 bytes
  • ui: 225326 bytes
  • common: -223643 bytes

metamaskbot avatar Jan 23 '23 20:01 metamaskbot

Is replacing bignumber imports an important part of this? It doesnt seem like all were changed

brad-decker avatar Jan 24 '23 03:01 brad-decker