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: Update lavamoat to a version with more diff-friendly policy ordering #29311

Merged
merged 9 commits into from
Dec 18, 2024

Conversation

naugtur
Copy link
Contributor

@naugtur naugtur commented Dec 18, 2024

Description

This lavamoat update brings a different sorting comparator for policy.json files that will produce more readable diffs.
This PR has 2 commits - one that reorders the policy without making any changes and another that updates lavamoat packages on top of that.

For better clarity on the policy.json files, inspect each commit separately.

This is going to be hard to review and merge because conflict resolution requires a redo. Gonna have to schedule it carefully.

Open in GitHub Codespaces

Related issues

Manual testing steps

  1. run a local policy update and verify the diff is not ridiculously large (that would mean the update failed to apply properly after policy got reordered here)

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.

@naugtur naugtur self-assigned this Dec 18, 2024
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.

@naugtur
Copy link
Contributor Author

naugtur commented Dec 18, 2024

@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

@@ -2693,7 +2693,7 @@
"@zxing/library>ts-custom-error": true
}
},
"extension-port-stream>readable-stream>abort-controller": {
"@lavamoat/lavapack>readable-stream>abort-controller": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of these browserify policies should have changed, so let's explore why these changes are trivial:

  • the path to abort-controller changed because packages got rearranged in the tree and this is now the shortest path to it.
  • readable-stream got deduplicated into a newer version that uses AbortSignal directly etc.

@@ -1225,14 +1171,21 @@
"assert": true,
"path": true
},
"globals": {
"afterAll": true,
"process.cwd": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like we're detecting new globals we didn't detect before

"globals": {
"Event": true,
"EventTarget": true,
"console": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could restrict these in overrides later as I don't think this shim has any work left to do.

"globals": {
"process": true
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is silly, but it comes with the ecosystem of streams reimplementations

@naugtur
Copy link
Contributor Author

naugtur commented Dec 18, 2024

Policy review:
There should be no meningful changes to policy in the webapp as we didn't touch anything there. the changes present are due to changes in the shape of the dependency tree.

Changes to the build policy indicate the lavamoat update (the readable-stream is a new copy of readable-stream (lavamoat updated to v4 and it's the newest version in the dependencies here according to yarn why)

cc @MetaMask/policy-reviewers

@naugtur naugtur marked this pull request as ready for review December 18, 2024 09:37
@naugtur naugtur requested review from a team as code owners December 18, 2024 09:37
Mrtenz
Mrtenz previously approved these changes Dec 18, 2024
Copy link
Member

@Mrtenz Mrtenz 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!

weizman
weizman previously approved these changes Dec 18, 2024
Copy link
Member

@weizman weizman left a comment

Choose a reason for hiding this comment

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

Lg

@metamaskbot
Copy link
Collaborator

Builds ready [5a46b93]
Page Load Metrics (1808 ± 130 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34922701712403194
domContentLoaded141922271780258124
load142422661808270130
domInteractive26154513115
backgroundConnect8112302713
firstReactRender1688502411
getState595202512
initialActions01000
loadScripts100117781314241116
setupStore75116147
uiStartup160829572093354170
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 3.5 KiB (0.04%)

@naugtur
Copy link
Contributor Author

naugtur commented Dec 18, 2024

Bundle size diffs [🚨 Warning! Bundle size has increased!]

same exact commits on main 3d ago said decreased 🤷

@naugtur naugtur added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@naugtur naugtur added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [e510fde]
Page Load Metrics (1606 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint13712019161418890
domContentLoaded13651941158817383
load13731977160618086
domInteractive22117543115
backgroundConnect88222189
firstReactRender1579442512
getState45112126
initialActions01000
loadScripts9781485116315072
setupStore625963
uiStartup15702277182720096
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 3.5 KiB (0.04%)

@naugtur naugtur dismissed stale reviews from Mrtenz and weizman via 1714c40 December 18, 2024 11:43
@naugtur
Copy link
Contributor Author

naugtur commented Dec 18, 2024

@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

@naugtur naugtur enabled auto-merge December 18, 2024 12:05
@metamaskbot
Copy link
Collaborator

Builds ready [215cc04]
Page Load Metrics (1534 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1424172715387536
domContentLoaded1419171615177436
load1423172715347636
domInteractive226632136
backgroundConnect75318157
firstReactRender1599423115
getState56416189
initialActions01000
loadScripts1035133411236732
setupStore66215199
uiStartup16092172181817584
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 3.5 KiB (0.04%)

@naugtur naugtur added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit 5aa2962 Dec 18, 2024
77 checks passed
@naugtur naugtur deleted the naugtur/lavamoat-policy-diff-friendly branch December 18, 2024 13:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2024
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-lavamoat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants