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

Bump @metamask/providers to v13.0.0 #20917

Merged
merged 24 commits into from
Oct 6, 2023
Merged

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Sep 15, 2023

Driven by a need to get rid of window.ethereum.networkVersion as a part of removing networkId work. This PR bumps @metamask/providers from v11.1.0 to v13.0.0, which introduces a deprecation warning when accessing window.ethereum.networkVersion (as well as chainId and selectedAddress). The goal is to get this into the wild, alert devs, and to finally remove these properties in the future.

Additionally, this PR bumps browserify from v16.5.1 to v17.0.0 which is required for polyfilling streams.pipeline which is now used in @metamask/providers Completed in a separate PR already

MetaMask/providers@v11.1.0...v13.0.0

Explanation

Screenshots/Screencaps

Before

Screenshot 2023-09-19 at 9 36 46 AM

After

Screenshot 2023-09-19 at 9 31 30 AM

Manual Testing Steps

  1. Open browser console in any page
  2. Enter window.ethereum.chainId
  3. You should get the value for the current selected chain ID and a deprecation warning
  4. Enter window.ethereum.chainId again
  5. You should get the value for the current selected chain ID, but no deprecation warning this time
  6. Repeat 2-6 for window.ethereum.networkVersion and window.ethereum.selectedAddress

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@github-actions
Copy link
Contributor

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.

@socket-security
Copy link

socket-security bot commented Sep 15, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@socket-security
Copy link

socket-security bot commented Sep 18, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@jiexi
Copy link
Contributor Author

jiexi commented Sep 18, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@jiexi jiexi mentioned this pull request Sep 19, 2023
@jiexi jiexi changed the title Bump @metamask/providers to v13.0.0 Bump @metamask/providers to v13.0.0, browserify to v17.0.0 Sep 19, 2023
@jiexi jiexi marked this pull request as ready for review September 19, 2023 16:39
@jiexi jiexi requested review from a team as code owners September 19, 2023 16:39
@jiexi
Copy link
Contributor Author

jiexi commented Sep 19, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [9873ef8]
Page Load Metrics (1590 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint112191142189
domContentLoaded14412009159011857
load14422009159011857
domInteractive14412009159011857
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 340 Bytes (0.01%)
  • ui: -13.88 KiB (-0.17%)
  • common: 64.57 KiB (1.41%)

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (c32d063) 68.63% compared to head (806e33b) 68.63%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #20917   +/-   ##
========================================
  Coverage    68.63%   68.63%           
========================================
  Files         1017     1017           
  Lines        40790    40790           
  Branches     10896    10896           
========================================
  Hits         27996    27996           
  Misses       12794    12794           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adonesky1
Copy link
Contributor

Can we only have one version of browserify in our dependency graph? Otherwise shouldn't @metamask/providers bring the different version requirement with it?

@legobeat legobeat added the dependencies Pull requests that update a dependency file label Sep 20, 2023
@jiexi
Copy link
Contributor Author

jiexi commented Sep 20, 2023

One thought on making this upgrades as safe and smooth as we can: If browserify@16->17 can be broken out separately and merged prior to upgrading providers to 13, that would be preferred

Resolved by #20946 . Thank you for taking care of that!

@metamask/[email protected]

Can bump this in a separate PR
MetaMask/eth-snap-keyring#102

MetaMask/snaps#1748

Looks like this may require some manual attention. I'll try to take a look today.

But yes, if this actually relies on a polyfill that browserify pulls in, sounds like something that should be done wither by providers or directly in metamask-extension itself?

Hard to say. It's awkward because the provider is almost always used in the browser context, but not strictly. Because it's totally valid to be used in nodejs, I think this is a concern of metamask-extension and not providers.

@legobeat legobeat changed the title Bump @metamask/providers to v13.0.0, browserify to v17.0.0 Bump @metamask/providers to v13.0.0 Sep 22, 2023
@jiexi
Copy link
Contributor Author

jiexi commented Oct 4, 2023

Current provider alignment status. Still have v11 floating around

  • bump provider to v13 in @metamask/snaps-execution-environments
    • consume in @metamask/snaps-controllers
    • consume in @metamask/eth-snap-keyring
    • consume in @metamask/keyring-api
      • consume in @metamask/eth-snap-keyring

bump provider to v13 in @metamask/snaps-execution-environments: MetaMask/snaps#1748

@@ -1136,6 +1136,21 @@
"string.prototype.matchall>side-channel": true
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure we don't want to commit these changes? I believe these are OS specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i believe this comes from the auto generated lavamoat policies

@jiexi
Copy link
Contributor Author

jiexi commented Oct 5, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [37f4f50]
Page Load Metrics (1324 ± 344 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint8213899157
domContentLoaded6913293188
load8319961324717344
domInteractive6913293188
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@jiexi
Copy link
Contributor Author

jiexi commented Oct 6, 2023

No longer blocked by having to align snaps (provider v11) and this change

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Collaborator

Builds ready [806e33b]
Page Load Metrics (1045 ± 377 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint81128100147
domContentLoaded6912093147
load8021731045786377
domInteractive6912093147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@jiexi jiexi merged commit 253b962 into develop Oct 6, 2023
@jiexi jiexi deleted the jl/deprecate-provider-properties branch October 6, 2023 20:20
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2023
@metamaskbot metamaskbot added the release-11.4.0 Issue or pull request that will be included in release 11.4.0 label Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file release-11.4.0 Issue or pull request that will be included in release 11.4.0 team-wallet-api-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants