Skip to content

fix(deps): unpin [email protected],x [email protected]#11972

Merged
NicolasMassart merged 22 commits intomainfrom
deps-ethereumjs-6
Mar 13, 2025
Merged

fix(deps): unpin [email protected],x [email protected]#11972
NicolasMassart merged 22 commits intomainfrom
deps-ethereumjs-6

Conversation

@legobeat
Copy link
Contributor

@legobeat legobeat commented Oct 23, 2024

Description

  • Unpin and dedupe ethereumjs-util and ethereumjs-abi while staying on legacy v6

  • Replace import { BN } from 'ethereumjs-util' with importing BN explicitly from either bn.js@4 or bn.js@5

    • Mixing the two is not safe and can cause breakage.
    • We don't get complete and correct typings with importing the old re-export
    • This adds both v4 and v5 as explicit dependencies - ideally v4 will be removed as transition to v5 concludes
    • Add EIP-1191 address checksum algorithm support for toChecksumAddress()
    • Unblock relying on the ancient and deprecated [email protected]
  • ethereumjs-util changelog

  • [email protected]

Related issues

Blocking

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.

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

@legobeat legobeat added dependencies Pull requests that update a dependency file team-application-security Application security team Run Smoke E2E and removed team-lavamoat labels Oct 23, 2024
@github-actions

This comment was marked as outdated.

@socket-security
Copy link

socket-security bot commented Oct 23, 2024

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@lavamoat/[email protected]0.2.1 Transitive: environment, filesystem +6 2.56 MB
npm/@types/[email protected] None 0 0 B
npm/@types/[email protected] None 0 0 B
npm/[email protected] None 0 0 B
npm/[email protected] None 0 0 B
npm/[email protected]0.6.8 None +2 214 kB holgerd77

🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

@github-actions

This comment was marked as outdated.

@legobeat legobeat marked this pull request as ready for review October 23, 2024 10:41
@legobeat legobeat requested a review from a team as a code owner October 23, 2024 10:41
@legobeat legobeat requested a review from a team October 23, 2024 10:41
@legobeat legobeat requested a review from a team as a code owner October 23, 2024 10:41
@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: b708bac
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/619aa4be-9711-4726-9e62-08968d39e035

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@legobeat legobeat marked this pull request as draft October 23, 2024 10:52
@legobeat legobeat force-pushed the deps-ethereumjs-6 branch 5 times, most recently from 6256e6b to 33737d5 Compare October 23, 2024 13:27
@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 75.60976% with 10 lines in your changes missing coverage. Please review.

Project coverage is 63.88%. Comparing base (d1d22d7) to head (67d4e4b).
Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
app/util/dappTransactions/index.ts 81.25% 2 Missing and 1 partial ⚠️
...components/UI/Ramp/Views/BuildQuote/BuildQuote.tsx 33.33% 0 Missing and 2 partials ⚠️
app/components/hooks/useTokenBalance.tsx 0.00% 2 Missing ⚠️
...Stake/components/StakingBalance/StakingBalance.tsx 0.00% 0 Missing and 1 partial ⚠️
...nents/hooks/useAddressBalance/useAddressBalance.ts 0.00% 0 Missing and 1 partial ⚠️
app/util/number/index.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11972      +/-   ##
==========================================
+ Coverage   63.42%   63.88%   +0.46%     
==========================================
  Files        2093     2141      +48     
  Lines       45277    45707     +430     
  Branches     6234     6320      +86     
==========================================
+ Hits        28717    29201     +484     
+ Misses      14676    14616      -60     
- Partials     1884     1890       +6     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@NicolasMassart
Copy link
Contributor

NicolasMassart commented Mar 7, 2025

Aside from yarn git-safe-dependencies failing on CI (but passes locally), I think this PR is ready to be merged.
Note that this work is done to explicitly assign BN to either v4 or v5 but it will require to complete the migration to v5 eventually.
This only prevents misunderstanding and mixing BN versions in the code.
All the changes migrated to v5 are because they were ready to migrate as is (mostly)
But for the remaining v4, some work has to be done as migrating simply without changes to the v5 will return different results than expected. Hopefully we have unit tests on this.

Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

this looks good to me but I don't approve it as I spent a lot of time working on it as if I was the author and I want to make sure we have at least another review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR unpins and dedupes the legacy ethereumjs dependencies while switching from re-exported BN types to explicit imports from bn.js libraries. Key changes include:

  • Removing BN imports from ethereumjs-util and replacing them with explicit imports from bnjs4 (and bnjs5 for some tests)
  • Updating type annotations and instantiation across production and test files
  • Adjusting balance calculations to use the new BN implementations

Reviewed Changes

File Description
app/components/Views/confirmations/SendFlow/Amount/index.js Removed ethereumjs-util BN import and updated max value calculation
app/components/UI/Ramp/hooks/useIntentAmount.ts Switched to explicit BN4 import
app/components/UI/Ramp/Views/BuildQuote/BuildQuote.tsx Replaced BN with BN4 for amount handling and calculations
app/components/UI/Stake/* Updated BN usage to BN4 in input and gas fee handling logic
app/components/UI/ConfirmAddAsset/ConfirmAddAsset.test.tsx Updated BN usage in tests to BN4
app/components/UI/Tokens/index.test.tsx Updated BN usage in tests to BN5
app/components/Views/confirmations/SendFlow/Confirm/validation.ts Updated BN types to BN4 for balance validations
other files Numerous similar updates to enforce explicit BN library usage across production and test code

Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.

@naugtur
Copy link

naugtur commented Mar 10, 2025

Fix for failing git-safe-dependencies: LavaMoat/LavaMoat#1556
The tool incorrectly recognized a package specifier as a git dependency.

@naugtur
Copy link

naugtur commented Mar 11, 2025

I copied the lockfile from this PR to our repo as a fixture and Socket shouted at me. There's a bunch of serious stuff ignored in this repo. Are we fine with that?

@github-actions

This comment was marked as outdated.

@metamaskbot

This comment was marked as outdated.

@github-actions
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 9f3f322
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/3bb3b0f3-47c9-4fd6-8408-704568d69485

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 9f3f322
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/3bb3b0f3-47c9-4fd6-8408-704568d69485

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link
Member

@wachunei wachunei left a comment

Choose a reason for hiding this comment

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

Ramp changes LGTM.

Copy link
Contributor

@nickewansmith nickewansmith left a comment

Choose a reason for hiding this comment

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

Stake changes lgtm

Copy link
Contributor

@salimtb salimtb left a comment

Choose a reason for hiding this comment

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

Assets changes LGTM

@sonarqubecloud
Copy link

Copy link
Member

@OGPoyraz OGPoyraz left a comment

Choose a reason for hiding this comment

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

Confirmation changes LGTM

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-7.43.0 Issue or pull request that will be included in release 7.43.0 team-application-security Application security team team-mobile-platform Mobile Platform team team-ramp issues related to Ramp features

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.