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

Making permission and approval controller methods idempotent #15848

Merged
merged 96 commits into from
Oct 31, 2022

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Sep 15, 2022

Fixes: #15714

Make these methods idempotent by adding exception handling in MetamaskController:

  • permissionController.revokePermissions
  • permissionController.rejectPermissionsRequest
  • permissionController.acceptPermissionsRequest
  • approvalController.accept
  • approvalController.reject

The methods above can throw error if they see same request again, for instance of we try to revoker a permission by calling permissionController.revokePermissions and the permission is already revoked it will throw error.

Catching these error like done in this PR will help making the methods behaviour idempotent when called multiple times from UI.

@github-actions
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.

@jpuri jpuri requested a review from rekmarks September 15, 2022 18:38
@jpuri jpuri marked this pull request as ready for review September 15, 2022 18:38
@jpuri jpuri requested a review from a team as a code owner September 15, 2022 18:38
@jpuri jpuri force-pushed the permission_approval_method_idempotent branch from fb5c96e to 9868acc Compare September 15, 2022 18:39
digiwand
digiwand previously approved these changes Sep 15, 2022
…taMask/metamask-extension into permission_approval_method_idempotent
@jpuri jpuri added the DO-NOT-MERGE Pull requests that should not be merged label Sep 16, 2022
@jpuri
Copy link
Contributor Author

jpuri commented Sep 16, 2022

The PR should be merged only after this change is included in extension code: MetaMask/core#909

segun
segun previously approved these changes Sep 16, 2022
@jpuri jpuri removed the DO-NOT-MERGE Pull requests that should not be merged label Sep 16, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [988f0ab]
Page Load Metrics (2128 ± 82 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint862234208465223
domContentLoaded15782314211817283
load15972372212817282
domInteractive15782314211817283

Base automatically changed from controller_update to develop October 25, 2022 04:54
@jpuri jpuri requested a review from digiwand October 25, 2022 08:27
@jpuri jpuri requested a review from digiwand October 26, 2022 07:42
@metamaskbot
Copy link
Collaborator

Builds ready [911116c]
Page Load Metrics (2636 ± 158 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1042766294587282
domContentLoaded199331112582287138
load199332882636329158
domInteractive199331112582287138

Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

LGTM!

@jpuri jpuri merged commit 17af3bb into develop Oct 31, 2022
@jpuri jpuri deleted the permission_approval_method_idempotent branch October 31, 2022 05:52
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making permission / approval controller actions idempotent
7 participants