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.

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

Remove un-necessary word lists from bundle - replace ethers with submodules #15022

Open
jpuri opened this issue Jun 23, 2022 · 4 comments
Open
Assignees
Labels
MV3 team-extension-platform Extension Platform team

Comments

@jpuri
Copy link
Contributor

jpuri commented Jun 23, 2022

@adonesky1
Copy link
Contributor

The wordlists that are added to the bundle from ethers (via ethersproject/wordlist) should be removed with this work currently in progress:
Screen Shot 2022-08-09 at 2 23 47 PM

If they aren't removed in this work we may need to patch or create our own versions of @ethersproject/wallet and @ethersproject/json-wallet that don't depend on these wordlists to remove them from from our bundle.

There are other wordlists from bip39 that this ticket may also refer to.

@adonesky1
Copy link
Contributor

This PR to our fork of BIP39 should remove another ~160-180kb from our common.js files:
Screen Shot 2022-08-09 at 3 05 23 PM

After that we will still have the extra word-lists from @scure/bip39 another version of bip39 that we use in @metamask/key-tree (which is in the extension dependency tree). We may want to simply change that package to use our bip39 fork. Will investigate.

@adonesky1
Copy link
Contributor

Actually I vaguely remember @rekmarks expressing that we should switch over to using @scure/bip39 rather than our fork of bitcoinjs/bip39? Maybe I'm imagining or misattributing that but... Erik and @kumavis do you think its worth swapping over to using a fork of @scure/bip39 with our same patches and then removing the unused wordlists as I am suggesting we do in this PR to our fork, and then consolidate around using that in @metamask/key-tree, @metamask/eth-hd-keyring and @metamask/keyringcontroller?

@rekmarks
Copy link
Member

rekmarks commented Aug 10, 2022

I think we should switch over to @scure, fork it, apply our patches, and then consolidate around that. Per its readme, the benefits of @scure/bip39 over bip39 are:

  • Is 491KB all-bundled instead of 1.3MB
  • Uses 2 dependencies instead of 15
  • Wordlists are 157KB instead of 315KB
  • Had an external security audit by Cure53 on Jan 5, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MV3 team-extension-platform Extension Platform team
Projects
None yet
Development

No branches or pull requests

6 participants