-
Notifications
You must be signed in to change notification settings - Fork 642
feat!: Use StorageService to store source code
#3777
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
base: main
Are you sure you want to change the base?
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
57095fe to
6a20e47
Compare
202b635 to
801cc12
Compare
655f32e to
85b792f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3777 +/- ##
=======================================
Coverage 98.35% 98.35%
=======================================
Files 430 430
Lines 12340 12362 +22
Branches 1919 1920 +1
=======================================
+ Hits 12137 12159 +22
Misses 203 203 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
85b792f to
e3515be
Compare
| "@metamask/snaps-rpc-methods": "workspace:^", | ||
| "@metamask/snaps-sdk": "workspace:^", | ||
| "@metamask/snaps-utils": "workspace:^", | ||
| "@metamask/storage-service": "^0.0.1", |
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.
This should really be >=1.0.0 before we use this in production 😵
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.
I know but I was never released as 1.0.0 😅
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.
If Andre considers it stable, we could ask for a bump to v1
b5235de to
31b6881
Compare
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
df10375 to
197f300
Compare
|
|
||
| export type PersistedSnap = Snap; | ||
|
|
||
| export type StoredSnap = Snap & StorageServiceSnapData; |
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.
| export type StoredSnap = Snap & StorageServiceSnapData; | |
| export type StoredSnap = PersistedSnap & StorageServiceSnapData; |
Nit
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.
lol: #3777 (comment)
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.
welp
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.
fight with @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.
I meant that we can remove the StoredSnap type 😅
| export type StoredSnap = Snap & StorageServiceSnapData; | |
| export type PersistedSnap = Snap & StorageServiceSnapData; |
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.
Well, not entirely since that is used for typing out the persisted state of the controller itself
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.
Having both PersistedSnap and StoredSnap seems confusing in my opinion, since they mean very similar things.
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, I don't disagree. But unsure what to propose
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.
Would it be really a problem ? I looked at the usage of PersistedSnap and it's not used anywhere outside of SnapController 🤔
| return getSnapObject({ id: snapId as SnapId }); | ||
| }); | ||
|
|
||
| messenger.registerActionHandler( |
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.
I don't follow why we are changing the default mocks for permissions 🤔
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.
We're changing this to avoid throwing a lot of errors due to the fact that now, because we init the controller in the tests, we call callLifecycleHooks. In this path we call hasPermission and getPermissions, if hasPermission returns true but getPermissions doesn't return the correct handler permissions we throw an error and that was happening for each test.
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 makes a lot of sense. Can we document that in the PR description?
| messenger.registerActionHandler('PermissionController:hasPermission', () => { | ||
| return true; | ||
| }); | ||
| messenger.registerActionHandler( |
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.
| fetchMock.mockImplementation(async () => { | ||
| throw new AssertionError({ message: 'Unmocked access to internet.' }); | ||
| }); | ||
|
|
||
| // Hydrate the storage service with the default snap data. | ||
| await hydrateStorageService(); |
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.
Shouldn't this be dependent on the test? Not all tests need the default Snap to be in the storage service
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.
| @@ -10216,7 +10381,12 @@ describe('SnapController', () => { | |||
| origin: 'bar.io', | |||
| }); | |||
|
|
|||
| const snapController = getSnapController( | |||
| await hydrateStorageService({ | |||
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.
I don't fully understand when we need to do this and when we don't. Could getSnapController handle this instead? 🤔
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.
we hydrate the defaults in beforeEach so if the test only uses the default values there's no need to call hydrateStorageService. However if the test uses different values in the snaps state we need to set the corresponding source codes in the StorageService
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.
Couldn't each call to getSnapController set it up instead? So we avoid the beforeEach
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.
Yes !
This PR adds the use of
StorageServiceto store the Snap source codes. Due to this storage being async, theSnapControllerinitialization is now async and the preinstalled snaps setup now lives ininit()Note
Adds
@metamask/storage-serviceas a new dependency inpackages/snaps-controllers/package.json.dependenciesto include@metamask/storage-service@^0.0.1Written by Cursor Bugbot for commit 197f300. This will update automatically on new commits. Configure here.