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 assets controllers to latest #24360

Merged
merged 10 commits into from May 7, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented May 3, 2024

Description

Bumps assets-controllers to latest.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@sahar-fehri sahar-fehri requested a review from a team as a code owner May 3, 2024 09:27
Copy link
Contributor

github-actions bot commented May 3, 2024

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 May 3, 2024

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

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] Transitive: environment +15 5.9 MB metamaskbot
npm/@metamask/[email protected] None +5 1.25 MB metamaskbot
npm/@types/[email protected] None 0 3.2 kB types
npm/[email protected] None 0 409 kB connor.peet
npm/[email protected] None 0 81.8 kB acubed

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

View full report↗︎

@sahar-fehri sahar-fehri self-assigned this May 3, 2024
@sahar-fehri sahar-fehri added needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. team-assets labels May 3, 2024
@sahar-fehri
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@sahar-fehri sahar-fehri requested review from a team as code owners May 3, 2024 09:59
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.38%. Comparing base (4519cac) to head (d13aff2).

❗ Current head d13aff2 differs from pull request most recent head e87e242. Consider uploading reports for the commit e87e242 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24360      +/-   ##
===========================================
+ Coverage    67.36%   67.38%   +0.02%     
===========================================
  Files         1285     1285              
  Lines        50088    50052      -36     
  Branches     12996    12992       -4     
===========================================
- Hits         33738    33726      -12     
+ Misses       16350    16326      -24     

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

@metamaskbot
Copy link
Collaborator

Builds ready [a8e77c6]
Page Load Metrics (953 ± 653 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62150942210
domContentLoaded9381573
load5034889531359653
domInteractive9381573
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -30 Bytes (-0.00%)
  • ui: -2 Bytes (-0.00%)
  • common: -41.45 KiB (-0.70%)

bergeron
bergeron previously approved these changes May 3, 2024
@legobeat
Copy link
Contributor

legobeat commented May 4, 2024

@metamask/assets-controllers has a peerDependency on @metamask/account-controllers.

Before this change, the direct dependency version of @metamask/account-controllers satisfies the peerDependency requirement. This change will break it. Also upgrading @metamask/account-controllers from ^11.0.0 to ^14.0.0 would make them align again.

I haven't looked closer than this, but I would think that there is something behind the peerDependency bumps?

Is it actually safe, or should @metamask/account-controllers also be upgraded alongside?

salimtb
salimtb previously approved these changes May 6, 2024
@sahar-fehri sahar-fehri dismissed stale reviews from salimtb and bergeron via 99c3db1 May 6, 2024 21:41
@metamaskbot
Copy link
Collaborator

Builds ready [99c3db1]
Page Load Metrics (1256 ± 636 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint68224953818
domContentLoaded95316105
load56293512561325636
domInteractive95316105
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -30 Bytes (-0.00%)
  • ui: -2 Bytes (-0.00%)
  • common: -41.45 KiB (-0.70%)

@sahar-fehri
Copy link
Contributor Author

sahar-fehri commented May 6, 2024

@metamask/assets-controllers has a peerDependency on @metamask/account-controllers.

Before this change, the direct dependency version of @metamask/account-controllers satisfies the peerDependency requirement. This change will break it. Also upgrading @metamask/account-controllers from ^11.0.0 to ^14.0.0 would make them align again.

I haven't looked closer than this, but I would think that there is something behind the peerDependency bumps?

Is it actually safe, or should @metamask/account-controllers also be upgraded alongside?

Hey @legobeat! that's a good point! I think i expected to see build errors or errors in console extension/background if a peer dependency needed an upgrade, i have tested this PR locally and i do not see any errors.
I see the changelog in core marked accounts-controller v14.0.0 as breaking but maybe it does not necessarily need an update?

@sahar-fehri sahar-fehri force-pushed the chore/bump-assets-controller branch from 99c3db1 to a23a64a Compare May 7, 2024 12:27
@metamaskbot
Copy link
Collaborator

Builds ready [a23a64a]
Page Load Metrics (1302 ± 660 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint622251053919
domContentLoaded99418189
load50325013021375660
domInteractive99418189
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -30 Bytes (-0.00%)
  • ui: -2 Bytes (-0.00%)
  • common: -41.45 KiB (-0.67%)

@metamaskbot
Copy link
Collaborator

Builds ready [a23a64a]
Page Load Metrics (1302 ± 660 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint622251053919
domContentLoaded99418189
load50325013021375660
domInteractive99418189
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -30 Bytes (-0.00%)
  • ui: -2 Bytes (-0.00%)
  • common: -41.45 KiB (-0.67%)

@sahar-fehri sahar-fehri force-pushed the chore/bump-assets-controller branch from a23a64a to c2ebbfb Compare May 7, 2024 18:53
@sahar-fehri
Copy link
Contributor Author

@metamaskbot update-policies

Copy link

socket-security bot commented May 7, 2024

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

View full report↗︎

@metamaskbot
Copy link
Collaborator

Policies updated

@mcmire
Copy link
Contributor

mcmire commented May 7, 2024

I just checked through the changes in the peer dependencies that assets-controllers has, and most of them are type-related, so they don't have an impact here. It seems that we can address those upgrades in a separate PR.

@metamaskbot
Copy link
Collaborator

Builds ready [d13aff2]
Page Load Metrics (1247 ± 703 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint70181942613
domContentLoaded9221231
load58375412471463703
domInteractive9221231
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -30 Bytes (-0.00%)
  • ui: -2 Bytes (-0.00%)
  • common: -41.45 KiB (-0.67%)

@metamaskbot
Copy link
Collaborator

Builds ready [e87e242]
Page Load Metrics (687 ± 522 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6013980168
domContentLoaded9181121
load4928756871088522
domInteractive9181121
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -30 Bytes (-0.00%)
  • ui: -2 Bytes (-0.00%)
  • common: -41.45 KiB (-0.67%)

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

@sahar-fehri sahar-fehri merged commit 39e7512 into develop May 7, 2024
72 checks passed
@sahar-fehri sahar-fehri deleted the chore/bump-assets-controller branch May 7, 2024 23:24
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2024
@metamaskbot metamaskbot added the release-11.18.0 Issue or pull request that will be included in release 11.18.0 label May 7, 2024
@metamaskbot metamaskbot added release-11.17.0 Issue or pull request that will be included in release 11.17.0 and removed release-11.18.0 Issue or pull request that will be included in release 11.18.0 labels May 23, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.17.0 on PR. Adding release label release-11.17.0 on PR and removing other release labels(release-11.18.0), as PR was added to branch 11.17.0 when release was cut.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-11.17.0 Issue or pull request that will be included in release 11.17.0 team-assets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants