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

feat: upgrade @metamask/eth-ledger-bridge-keyring #11836

Merged
merged 28 commits into from
Nov 13, 2024
Merged

feat: upgrade @metamask/eth-ledger-bridge-keyring #11836

merged 28 commits into from
Nov 13, 2024
+204 −48

Conversation

dawnseeker8
Copy link
Contributor

@dawnseeker8 dawnseeker8 commented Oct 17, 2024 •
edited
Loading

Description

This PR contain the upgrade of @metamask/eth-ledger-bridge-keyring library to latest 6.0.0, which contain the latest @ledgerhq/hw-eth-app from ledger team,

the latest @ledgerhq/hw-eth-app will provide following:

  1. clear-signing feature support
  2. replace @ledgerhq/cryptoassets with lighter package @ledgerhq/cryptoassets-evm-signature

Regarding the package size anlysis from requirement https://github.com/MetaMask/accounts-planning/issues/567
after the upgrade of @ledgerhq/hw-eth-app to 6.39.0, the whole @LedgerHQ library bundle did increase a little bit than before. (due to some new features support), but for clear-signing support, we may need to take the hit.

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/567

Manual testing steps

Need a full regression on all ledger features to make sure all ledger features not broken by this library upgrade.

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.

@dawnseeker8 dawnseeker8 added team-hardware-wallets Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 17, 2024
@dawnseeker8 dawnseeker8 requested review from a team as code owners October 17, 2024 12:42
@github-actions 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.

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@dawnseeker8 dawnseeker8 changed the title Feat/upgrade eth ledger bridge keyring feat: upgrade @metamask/eth-ledger-keyring-bridge Oct 17, 2024
@dawnseeker8 dawnseeker8 changed the title feat: upgrade @metamask/eth-ledger-keyring-bridge feat: upgrade @metamask/eth-ledger-bridge-keyring Oct 17, 2024
@socket-security Socket Security
Copy link

socket-security bot commented Oct 17, 2024 •
edited
Loading

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

Package New capabilities Transitives Size Publisher
npm/@ledgerhq/cryptoassets-evm-signatures@13.5.0 None 0 3.1 MB ldg-github-ci
npm/@ledgerhq/devices@8.4.4 None +1 431 kB ldg-github-ci
npm/@ledgerhq/domain-service@1.2.6 None 0 142 kB ldg-github-ci
npm/@ledgerhq/evm-tools@1.2.3 None 0 135 kB ldg-github-ci
npm/@ledgerhq/hw-app-eth@6.39.0 None 0 2.21 MB ldg-github-ci
npm/@ledgerhq/hw-transport-mocker@6.29.4 None 0 108 kB ldg-github-ci
npm/@ledgerhq/hw-transport@6.31.4 None 0 133 kB ldg-github-ci
npm/@ledgerhq/live-env@2.3.0 None 0 266 kB aboissiere, gbrahm-ledger, ldg-github-ci, ...5 more
npm/@ledgerhq/types-live@6.52.0 None 0 377 kB ldg-github-ci
npm/@metamask/eth-ledger-bridge-keyring@6.0.0 None +2 423 kB metamaskbot
npm/eip55@2.1.1 None 0 5.55 kB junderw
npm/keccak@3.0.4 None 0 779 kB fanatid
npm/utility-types@3.11.0 None 0 64.3 kB piotrwitek

🚮 Removed packages: npm/@ledgerhq/cryptoassets@6.37.0, npm/@ledgerhq/devices@8.4.2, npm/@ledgerhq/errors@6.18.0, npm/@ledgerhq/hw-app-eth@6.26.1, npm/@ledgerhq/hw-transport@6.31.2, npm/@metamask/eth-ledger-bridge-keyring@4.1.0

View full report↗︎

@dawnseeker8 dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 17, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@dawnseeker8 dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 17, 2024
@dawnseeker8 dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 18, 2024
@dawnseeker8 dawnseeker8 removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Oct 18, 2024
@dawnseeker8 dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

https://bitrise.io/ Bitrise

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

Commit hash: 0830919
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/997abcaa-88ee-4eb6-9a1f-95c28a15f350

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

@dawnseeker8 dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: c9acb37
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/50922f7d-bcce-4a6d-b54b-2251b2fd1fd5

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

dawnseeker8
dawnseeker8 commented Nov 12, 2024
View reviewed changes
ios/Podfile.lock Outdated Show resolved Hide resolved
@dawnseeker8 dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 2bb51e9
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/7e7f50c2-bc87-4b13-8696-da71d7d94491

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

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@dawnseeker8 dawnseeker8 removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 12, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

tommasini
tommasini approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

@tommasini
Copy link
Contributor

@metamask/eth-ledger-bridge-keyring brings a library that downloads a big file during bundle, do you know if this is still happening?

@dawnseeker8
Copy link
Contributor Author

@metamask/eth-ledger-bridge-keyring brings a library that downloads a big file during bundle, do you know if this is still happening?

No really, the reason we upgrade is ledger team has do a upgrade on their lib to reduce the size, however, i have checked the whole @LedgerHQ library, the size didn't change too much compared to previous version.
however, Ledgerhq team plan to release a new version soon. we may need to upgrade that lib again to further reduce the size. anyway we need this PR to be merged so that we can start a clear signing feature

@dawnseeker8 dawnseeker8 added this pull request to the merge queue Nov 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 13, 2024
@dawnseeker8 dawnseeker8 added this pull request to the merge queue Nov 13, 2024
Merged via the queue into main with commit 5ff0b2e Nov 13, 2024
40 checks passed
@dawnseeker8 dawnseeker8 deleted the feat/upgrade-eth-ledger-bridge-keyring branch November 13, 2024 10:03
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2024
@metamaskbot metamaskbot added the release-7.36.0 Issue or pull request that will be included in release 7.36.0 label Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tommasini tommasini tommasini approved these changes

@mikesposito mikesposito mikesposito approved these changes

@Akaryatrh Akaryatrh Akaryatrh approved these changes

Assignees

@dawnseeker8 dawnseeker8

Labels
QA Passed A successful QA run through has been done release-7.36.0 Issue or pull request that will be included in release 7.36.0 team-accounts team-hardware-wallets
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dawnseeker8 @vivek-consensys @tommasini @Akaryatrh @mikesposito @metamaskbot @legobeat