-
Notifications
You must be signed in to change notification settings - Fork 639
feat: Validate platform version against production #3417
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
feat: Validate platform version against production #3417
Conversation
packages/snaps-utils/src/manifest/validators/production-platform-version.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| const determineProductionVersion = useFileSystemCache( | ||
| 'snaps-production-version', | ||
| inMilliseconds(7, Duration.Day), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that if there's a new release in-between the 7 days of caching the CLI will miss it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. We can reduce the TTL if we think 7 days is too long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT @Mrtenz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to be a particularly demanding request, so maybe one day or a couple days is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced to 3 days
0cb6fb9 to
767aa66
Compare
packages/snaps-utils/src/manifest/validators/production-platform-version.test.ts
Outdated
Show resolved
Hide resolved
packages/snaps-utils/src/manifest/validators/production-platform-version.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3417 +/- ##
=======================================
Coverage 98.15% 98.16%
=======================================
Files 401 402 +1
Lines 11067 11109 +42
Branches 1748 1753 +5
=======================================
+ Hits 10863 10905 +42
Misses 204 204 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| inMilliseconds(7, Duration.Day), | ||
| async () => { | ||
| try { | ||
| const latestRelease = await fetch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with this is that there can be a delay between the release being created on GitHub and it actually being live in Chrome, Firefox, etc., especially given the slow rollout sometimes.
Not something we need to resolve right now as this is much better than not having any validation, but it's something to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's not ideal, but way better than no warning at all IMO
Validate
platformVersionagainst the rough version used in production for MetaMask (by inspecting GitHub) and produce a warning if the Snap is using a newer version.Closes https://github.com/MetaMask/MetaMask-planning/issues/4980