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

Add all manifest validation logic as separate rules #2572

Merged
merged 23 commits into from
Jul 25, 2024
Merged

Add all manifest validation logic as separate rules #2572

merged 23 commits into from
Jul 25, 2024

Conversation

ritave
Copy link
Contributor

@ritave ritave commented Jul 11, 2024 •
edited
Loading

fixes #2571

@ritave ritave changed the title Add all manifest validation logic as seperate rules Add all manifest validation logic as separate rules Jul 11, 2024
@ritave ritave force-pushed the ritave/validators branch from 78dbce1 to b446d32 Compare July 22, 2024 15:00
@ritave ritave marked this pull request as ready for review July 22, 2024 15:00
@ritave ritave requested a review from a team as a code owner July 22, 2024 15:00
@codecov Codecov
Copy link

codecov bot commented Jul 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (12eaf23) to head (c5b114d).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2572      +/-   ##
==========================================
+ Coverage   94.01%   94.17%   +0.15%     
==========================================
  Files         461      476      +15     
  Lines        9678     9813     +135     
  Branches     1486     1496      +10     
==========================================
+ Hits         9099     9241     +142     
+ Misses        579      572       -7

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

Mrtenz
Mrtenz requested changes Jul 23, 2024
View reviewed changes
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.

Nice! This seems like a good improvement over the previous manifest logic. I like the approach of different validation rules you've used.

I left some clarifying questions, nits, and some suggestions.

packages/snaps-cli/src/commands/build/implementation.test.ts Show resolved Hide resolved
packages/snaps-cli/src/webpack/plugins.test.ts Show resolved Hide resolved
packages/snaps-cli/src/commands/manifest/implementation.ts Outdated Show resolved Hide resolved
packages/snaps-utils/src/manifest/manifest.ts Show resolved Hide resolved
packages/snaps-utils/src/manifest/manifest.ts Outdated Show resolved Hide resolved
packages/snaps-utils/src/manifest/validators/is-snap-manifest.ts Outdated Show resolved Hide resolved
packages/snaps-utils/src/manifest/validators/manifest-localization.ts Outdated Show resolved Hide resolved
packages/snaps-utils/src/manifest/validators/package-json-recommended-fields.ts Show resolved Hide resolved
packages/snaps-utils/src/manifest/validators/package-json-recommended-fields.ts Outdated Show resolved Hide resolved
packages/snaps-utils/src/manifest/validators/package-name-match.ts Outdated Show resolved Hide resolved
Mrtenz
Mrtenz reviewed Jul 25, 2024
View reviewed changes
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.

Maybe we should update the build command to have better formatted warnings and errors too, instead of just throwing an error?

$ yarn build
✖ Error: Manifest Error: The manifest is invalid.
"snap.manifest.json" "shasum" field does not match computed shasum. Got "+iXB4ursqJMaWaR3fHJYNCxM8di9xvLeYoTv9jjMfjI=", expected "+iXB4ursqJMaWWR3fHJYNCxM8di9xvLeYoTv9jjMfjI=".
    at ./MetaMask/snaps/packages/snaps-webpack-plugin/dist/chunk-5IQTHQ6D.js:206106:17

Feel free to do that in a separate PR though if you prefer.

@ritave
Copy link
Contributor Author

ritave commented Jul 25, 2024

Improving the dev experience will be part of #2565

ritave added 21 commits July 25, 2024 12:39
@ritave
[WIP] Add all manifest validation logic as seperate rules
257a4e1
@ritave
Refactored checkManifest
a19e1d3
@ritave
Fix snap-utils tests
8a8ee89
@ritave
Remove double jsonParse
ee06e30
@ritave
Update test checksum
c11d029
@ritave
Revert manifest test checksum update
ccc9aab 
For some reason checkManifest calculates the checksum wrongly
instead of the checksum changing
@ritave
Fix checksum validator ignoring svgIcon file for calculation
b4d09a1
@ritave
Building a snap now shows a list of fixed problems
733061c
@ritave
Fix snaps-utils tests
0205e0c
@ritave
Fix snaps-browserify-plugin tests
cb22db9
@ritave
Fix snap-rollup-plugin tests
703e79c
@ritave
Fix snaps-webpack-plugin tests
ba217ae
@ritave
Fix snaps-cli tests
6474967
@ritave
Update coverage in snaps-utils
6355860
@ritave
Added partial tests for validators
21c32da
@ritave
Extracted parsing localization files from validator
f19125e
@ritave
Added more tests to validators
86ecc3f
@ritave
Add tests to all validators
f38e5be
@ritave
Fix after rebase
4dab92e 
Moved superstruct to @metamask/superstruct
@ritave
Changes after code review
458bfdb
@ritave
Update coverage
9c41cb2
@ritave ritave force-pushed the ritave/validators branch from 6d81688 to c5b114d Compare July 25, 2024 10:39
@ritave ritave merged commit 0788b52 into main Jul 25, 2024
156 checks passed
@ritave ritave deleted the ritave/validators branch July 25, 2024 10:56
@ritave ritave mentioned this pull request Jul 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mrtenz Mrtenz Mrtenz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor bundle and manifest validation logic
2 participants
@ritave @Mrtenz