-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[FLASK] Fix an issue with installing and updating snaps with 0 permissions #15796
Conversation
Builds ready [8a6c49e]
Page Load Metrics (2360 ± 99 ms)
highlights:storybook
|
Builds ready [783a654]
Page Load Metrics (1281 ± 43 ms)
highlights:storybook
|
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.
LGTM
@@ -0,0 +1,62 @@ | |||
diff --git a/node_modules/@metamask/snap-controllers/dist/snaps/SnapController.js b/node_modules/@metamask/snap-controllers/dist/snaps/SnapController.js |
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.
Can you explain why we're using a patch instead of latest snap-controllers version?
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.
Its this fix applied: MetaMask/snaps#754
Will remove once we cut another release of skunkworks.
const revokedPermissions = request.unusedPermissions ?? {}; | ||
const newPermissions = request.newPermissions ?? {}; | ||
const hasPermissions = | ||
Object.keys(approvedPermissions).length + |
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.
please add parentheses around arithmetic to avoid ambiguity.
(yes I know that +
has higher precedence over >
, but it makes the intentions clearer)
(
Object.keys(approvedPermissions).length +
Object.keys(revokedPermissions).length +
Object.keys(newPermissions).length
) > 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.
Not possible due to linting
rejectSnapInstall={(requestId) => { | ||
rejectPendingApproval( | ||
requestId, | ||
serializeError(ethErrors.provider.userRejectedRequest()), |
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 feel iffy that we're serialising errors in the UI and using rpc errors in general.
export function getFirstSnapUpdateRequest(state) { | ||
const requests = getSnapUpdateRequests(state); | ||
export function getFirstSnapInstallOrUpdateRequest(state) { | ||
const requests = getSnapInstallOrUpdateRequests(state); |
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.
return getSnapInstallOrUpdateRequests(state)?.[0] ?? null
@ritave Will improve this in the upcoming skunkworks PR. Merging this as is. |
Explanation
Fixes an issue with installing snaps that didn't request any permissions.
Manual Testing Steps