Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/use etherjs specific imports #15461

Merged
merged 6 commits into from
Jan 24, 2023
Merged

Fix/use etherjs specific imports #15461

merged 6 commits into from
Jan 24, 2023

Conversation

amerkadicE
Copy link
Contributor

@amerkadicE amerkadicE commented Aug 4, 2022 •
edited
Loading

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

  • PR template is filled out
  • IF this PR fixes a bug, a test that would have caught the bug has been added
  • 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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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.

@amerkadicE amerkadicE force-pushed the fix/use-etherjs-specific-imports branch 2 times, most recently from f485c62 to ec73c85 Compare August 4, 2022 19:48
@adonesky1 adonesky1 mentioned this pull request Aug 9, 2022
Open
@danjm danjm added the MV3 label Aug 10, 2022
@hilvmason hilvmason added the PR - P1 identifies PRs deemed priority for Extension team label Aug 15, 2022
@amerkadicE amerkadicE force-pushed the fix/use-etherjs-specific-imports branch 3 times, most recently from 8986e80 to 89a1ff0 Compare August 18, 2022 10:57
@metamaskbot
Copy link
Collaborator

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

@amerkadicE amerkadicE marked this pull request as ready for review August 18, 2022 13:50
@amerkadicE amerkadicE requested a review from a team as a code owner August 18, 2022 13:50
adonesky1
adonesky1 reviewed Aug 18, 2022
View reviewed changes
app/scripts/controllers/swaps.test.js
@@ -377,7 +377,7 @@ describe('SwapsController', function () {
assert.strictEqual(gasEstimate, bufferedGasLimit);
assert.strictEqual(
gasEstimateWithRefund,
`0x${new BigNumber(maxGas, 10)
`0x${new BigNumberjs(maxGas, 10)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just use the ethers version of BigNumber here too? And get rid of bignumber.js here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will cause a test to fail because ethers BigNumber adds 0 when we covert to hex value.
image

adonesky1
adonesky1 previously approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@adonesky1 adonesky1 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question, otherwise LGTM

@amerkadicE amerkadicE force-pushed the fix/use-etherjs-specific-imports branch from 5a9f2df to 921da2c Compare August 19, 2022 10:28
@metamaskbot
Copy link
Collaborator

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

danjm
danjm requested changes Aug 19, 2022
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... why does this make the bundle size bigger? We should investigate that

@brad-decker
Copy link
Contributor

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.

@adonesky1
Copy link
Contributor

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?

@amerkadicE amerkadicE force-pushed the fix/use-etherjs-specific-imports branch from 2092919 to b17d8a9 Compare September 5, 2022 06:11
@metamaskbot
Copy link
Collaborator

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

@amerkadicE
Copy link
Contributor Author

@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.

@danjm danjm self-assigned this Sep 6, 2022
@adonesky1
Copy link
Contributor

@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
Copy link
Contributor

adonesky1 commented Sep 6, 2022 •
edited
Loading

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
Copy link
Contributor

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

@adonesky1
Copy link
Contributor

adonesky1 commented Sep 6, 2022 •
edited
Loading

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
Copy link
Contributor

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 adonesky1 force-pushed the fix/use-etherjs-specific-imports branch 2 times, most recently from 0e0b22c to f34f903 Compare January 18, 2023 17:53
@metamaskbot
Copy link
Collaborator

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

@adonesky1 adonesky1 force-pushed the fix/use-etherjs-specific-imports branch from 98092de to cb30ace Compare January 19, 2023 15:50
@metamaskbot
Copy link
Collaborator

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

brad-decker
brad-decker previously approved these changes Jan 19, 2023
View reviewed changes
@adonesky1
Copy link
Contributor

adonesky1 commented Jan 19, 2023 •
edited
Loading

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: #17298

@adonesky1 adonesky1 force-pushed the fix/use-etherjs-specific-imports branch from cb30ace to d3bdcf3 Compare January 23, 2023 16:30
@metamaskbot
Copy link
Collaborator

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

danjm
danjm approved these changes Jan 23, 2023
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

danjm
danjm requested changes Jan 23, 2023
View reviewed changes
app/scripts/controllers/detect-tokens.js Outdated
@@ -114,6 +115,7 @@ export default class DetectTokensController {
: tokenList;

const tokensToDetect = [];
this.ethersProvider = new Web3Provider(this._network._provider);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I don't know if this file needs a provider any more?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you're right! Will remove

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here

danjm
danjm reviewed Jan 23, 2023
View reviewed changes
app/scripts/controllers/preferences.js Outdated
@@ -73,7 +73,7 @@ export default class PreferencesController {
};

this.network = opts.network;
this.ethersProvider = new ethers.providers.Web3Provider(opts.provider);
this.ethersProvider = new Web3Provider(opts.provider);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment as above, what is this provider used for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here

@adonesky1 adonesky1 requested a review from danjm January 23, 2023 18:38
@metamaskbot
Copy link
Collaborator

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

@brad-decker
Copy link
Contributor

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

@adonesky1 adonesky1 merged commit dd09245 into develop Jan 24, 2023
@adonesky1 adonesky1 deleted the fix/use-etherjs-specific-imports branch January 24, 2023 14:10
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@brad-decker brad-decker brad-decker approved these changes

@danjm danjm danjm approved these changes

@Gtonizuka Gtonizuka Awaiting requested review from Gtonizuka Gtonizuka is a code owner automatically assigned from MetaMask/extension-devs

@adonesky1 adonesky1 Awaiting requested review from adonesky1

Assignees

@danjm danjm

Labels
PR - P1 identifies PRs deemed priority for Extension team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace general imports of ethers with specific package imports
6 participants
@amerkadicE @metamaskbot @brad-decker @adonesky1 @danjm @hilvmason