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

fix: resolve secp256k1 to latest version #28000

Closed
wants to merge 3 commits into from

Conversation

NicholasEllul
Copy link
Contributor

@NicholasEllul NicholasEllul commented Oct 21, 2024

Description

This pull request resolves the secp256k1 library to the latest version. Currently we use a variety of versions under 4.0.x, this would unify them under 4.0.4.

This is the state of the library before this resolution - as you can see, only patch changes will apply.

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

@NicholasEllul NicholasEllul requested a review from a team as a code owner October 21, 2024 21:04
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.

Copy link

socket-security bot commented Oct 21, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/secp256k1@4.0.3

View full report↗︎

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Oct 21, 2024
@NicholasEllul NicholasEllul added the team-extension-platform Extension Platform team label Oct 21, 2024
@NicholasEllul NicholasEllul changed the title Resolve secp256k1 to latest version fix: resolve secp256k1 to latest version Oct 21, 2024
legobeat
legobeat previously approved these changes Oct 21, 2024
@legobeat
Copy link
Contributor

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@metamaskbot metamaskbot requested a review from a team as a code owner October 21, 2024 21:51
@legobeat
Copy link
Contributor

@NicholasEllul related:

@@ -264,7 +264,8 @@
"@metamask/network-controller@npm:^17.0.0": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch",
"@metamask/network-controller@npm:^19.0.0": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch",
"@metamask/network-controller@npm:^20.0.0": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch",
"path-to-regexp": "1.9.0"
"path-to-regexp": "1.9.0",
"secp256k1": "4.0.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this also tries to pin the 3.x versions, which should go to 3.8.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@legobeat yarn why didn't highlight any cases of 3.x versions for extension. Do you recommend being more explicit about the resolution for each sub-dependency to avoid too wide of a scope in the event that other deps using v3 are introduced?

@metamaskbot
Copy link
Collaborator

Builds ready [6ff4f08]
Page Load Metrics (1864 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17262353186713364
domContentLoaded16782198182811053
load17292289186412158
domInteractive278148178
backgroundConnect1199372411
firstReactRender46286975124
getState578262512
initialActions01000
loadScripts1204165313479545
setupStore117025199
uiStartup190529542093227109
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -91.73 KiB (-1.84%)
  • ui: 0 Bytes (0.00%)
  • common: 222 Bytes (0.00%)

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.

LGTM (unless the answer to the question from Nic to lego changes my understanding of what exactly needs to be pinned here)

@legobeat
Copy link
Contributor

legobeat commented Oct 22, 2024

@danjm Maybe we can consider superseding this by #27973?

@NicholasEllul
Copy link
Contributor Author

@legobeat yes, looks like your pull request covers all the relevant changes here. Thanks!

auto-merge was automatically disabled October 22, 2024 12:33

Pull request was closed

@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template team-extension-platform Extension Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants