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

chore: bump eth-ledger-bridge-keyring to ^3.0.1 #26498

Merged
merged 12 commits into from
Sep 9, 2024

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Aug 19, 2024

Description

This PR updates the eth-ledger-bridge-keyring package to ^3.0.1. See CHANGELOG.md for more details on the changes we are bringing in.

Note that we are not updating to the latest major version (4), because a change in the package dependencies in the last major version caused a bundle size increase of ~1.2MB

Open in GitHub Codespaces

Related issues

Progresses #26840

Manual testing steps

All interactions with the Ledger device should be tested manually.

  1. Connect a Ledger account
  2. Sign transaction
  3. Sign personal message
  4. Sign typed message
  5. Remove Ledger account

Screenshots/Recordings

N/A

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.

@mikesposito
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

Copy link
Member Author

Choose a reason for hiding this comment

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

The patch has been ported upstream

Copy link
Contributor

@legobeat legobeat Sep 8, 2024

Choose a reason for hiding this comment

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

@mikesposito Looks like the change is not present anymore after downgrading this PR to 3.0.1 as it was merged after release of v4? Should the patch be brought back for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it all good it was merged without the patch?

package.json Outdated Show resolved Hide resolved
@mikesposito mikesposito marked this pull request as ready for review August 20, 2024 13:52
@mikesposito mikesposito requested review from a team as code owners August 20, 2024 13:52
@metamaskbot
Copy link
Collaborator

Builds ready [230d7c4]
Page Load Metrics (73 ± 8 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint71161982211
domContentLoaded4511468199
load5111473178
domInteractive96124115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.17 MiB (35.82%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.16%. Comparing base (416d024) to head (b2892a7).

Files with missing lines Patch % Lines
...ts/lib/offscreen-bridge/ledger-offscreen-bridge.ts 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26498      +/-   ##
===========================================
- Coverage    70.17%   70.16%   -0.01%     
===========================================
  Files         1425     1425              
  Lines        49659    49663       +4     
  Branches     13891    13891              
===========================================
  Hits         34846    34846              
- Misses       14813    14817       +4     

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

@mikesposito mikesposito requested a review from a team August 20, 2024 13:55
@mikesposito
Copy link
Member Author

Bundle size diffs [🚨 Warning! Bundle size has increased!]
background: 1.17 MiB (35.82%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

Hmm, this is alarming. Looking into it now

@mikesposito
Copy link
Member Author

mikesposito commented Aug 22, 2024

We decided to create a backport release of the patch we need to bring to the extension, while we investigate for a long term solution to the package size issue

yarn.lock Outdated Show resolved Hide resolved
@mikesposito
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [a0bfadc]
Page Load Metrics (1710 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint20920501365571274
domContentLoaded14392119169219393
load14592123171019795
domInteractive208332147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 885 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@mikesposito mikesposito changed the title chore: update eth-ledger-bridge-keyring deps: bump eth-ledger-bridge-keyring Sep 3, 2024
@mikesposito mikesposito changed the title deps: bump eth-ledger-bridge-keyring chore: bump eth-ledger-bridge-keyring Sep 3, 2024
@mikesposito mikesposito changed the title chore: bump eth-ledger-bridge-keyring chore: bump eth-ledger-bridge-keyring to ^3.0.1 Sep 3, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [21fd490]
Page Load Metrics (1867 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24120441709452217
domContentLoaded17032186184810852
load17172188186711053
domInteractive14228444321
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 885 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

cryptodev-2s
cryptodev-2s previously approved these changes Sep 4, 2024
Copy link
Contributor

@cryptodev-2s cryptodev-2s 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 [5136c2e]
Page Load Metrics (1955 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24524111759527253
domContentLoaded16032368192119694
load161323981955208100
domInteractive188236168
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 885 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

sonarcloud bot commented Sep 9, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [b2892a7]
Page Load Metrics (1720 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33119561651322155
domContentLoaded15601938169811354
load15681958172011354
domInteractive14188393718
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 885 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@kanthesha kanthesha left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@desi desi left a comment

Choose a reason for hiding this comment

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

LGTM

@mikesposito mikesposito merged commit d6d43eb into develop Sep 9, 2024
78 checks passed
@mikesposito mikesposito deleted the chore/update-ledger-keyring branch September 9, 2024 15:09
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 9, 2024
@gauthierpetetin gauthierpetetin added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-wallet-framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants