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

Use approval controller for permissions requests #9401

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Use approval controller for permissions requests #9401

merged 1 commit into from
Dec 14, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Sep 14, 2020 •
edited
Loading

This PR introduces the new approval controller to the extension codebase. We use it for the permissions controller's pending approval functionality.

The approval controller sets us up for a new pattern of requesting and managing user confirmations in RPC methods. Along with the generic RPC method middleware, the approval controller will allow us to eliminate our message managers, and decouple these method handlers from our provider stack, making the implementations more portable between the extension and mobile.

@rekmarks rekmarks force-pushed the approval-controller branch 3 times, most recently from 65605ce to 08aa491 Compare September 14, 2020 17:02
@metamaskbot
Copy link
Collaborator

Builds ready [08aa491]
Page Load Metrics (578 ± 94 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298644199
domContentLoaded320101357719694
load321101457819694
domInteractive319101257619694

@rekmarks rekmarks mentioned this pull request Sep 14, 2020
Closed
@rekmarks rekmarks force-pushed the approval-controller branch 2 times, most recently from 44a9977 to d4557da Compare October 9, 2020 20:05
@rekmarks rekmarks changed the title RFC: Add approval controller Add approval controller Oct 9, 2020
@metamaskbot
Copy link
Collaborator

Builds ready [648c7c2]
Page Load Metrics (426 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30433432
domContentLoaded29162542410249
load29262742610249
domInteractive29162542410249

@rekmarks rekmarks force-pushed the approval-controller branch from 648c7c2 to b01bb6c Compare October 9, 2020 21:56
@metamaskbot
Copy link
Collaborator

Builds ready [b01bb6c]
Page Load Metrics (445 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318543157
domContentLoaded31087744412761
load31187844512761
domInteractive31087644312761

@rekmarks rekmarks force-pushed the approval-controller branch from b01bb6c to 274765e Compare October 13, 2020 20:53
@rekmarks rekmarks marked this pull request as ready for review October 13, 2020 20:53
@rekmarks rekmarks requested a review from a team as a code owner October 13, 2020 20:53
@rekmarks rekmarks requested a review from whymarrh October 13, 2020 20:53
@metamaskbot
Copy link
Collaborator

Builds ready [274765e]
Page Load Metrics (468 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33115552512
domContentLoaded34477846612761
load34678046812761
domInteractive34377846612761

@rekmarks rekmarks mentioned this pull request Oct 20, 2020
Merged
@rekmarks
Copy link
Member Author

rekmarks commented Oct 20, 2020 •
edited
Loading

I'm gonna do this on in TypeScript, so reverting to draft until MetaMask/core#289 is merged and we can update @metamask/controllers.

Edit: This was done.

@rekmarks rekmarks marked this pull request as draft October 20, 2020 06:38
@danjm danjm added this to the v8.1.next? milestone Nov 2, 2020
@danjm danjm requested review from Gudahtt and danjm November 3, 2020 17:13
@danjm danjm self-assigned this Nov 3, 2020
@rekmarks rekmarks force-pushed the approval-controller branch from 274765e to efb8c2e Compare November 9, 2020 23:53
@rekmarks rekmarks removed the request for review from whymarrh November 9, 2020 23:55
@rekmarks rekmarks force-pushed the approval-controller branch 3 times, most recently from 1b6d7b3 to 59ef71b Compare November 10, 2020 02:26
@rekmarks rekmarks marked this pull request as ready for review November 10, 2020 02:56
@rekmarks rekmarks changed the title Add approval controller Use approval controller for permissions requests Nov 10, 2020
@rekmarks rekmarks force-pushed the approval-controller branch from 5ab5ed7 to c50ff31 Compare November 10, 2020 17:24
@metamaskbot
Copy link
Collaborator

Builds ready [c50ff31]
Page Load Metrics (358 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318841168
domContentLoaded2645863569546
load2665873589546
domInteractive2645853569546

@rekmarks rekmarks force-pushed the approval-controller branch from c50ff31 to 19d492b Compare November 13, 2020 21:39
@metamaskbot
Copy link
Collaborator

Builds ready [19d492b]
Page Load Metrics (443 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint328949168
domContentLoaded28086744116177
load28486944316177
domInteractive28086744116177

@rekmarks rekmarks mentioned this pull request Nov 14, 2020
Merged
@metamaskbot
Copy link
Collaborator

Builds ready [35007dc]
Page Load Metrics (386 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3110044178
domContentLoaded28360738510048
load28460938610048
domInteractive28360738510048

@kumavis
Copy link
Member

kumavis commented Nov 19, 2020

ApprovalController major version bump MetaMask/core#313

@rekmarks rekmarks force-pushed the approval-controller branch from 35007dc to e78cefd Compare December 2, 2020 18:42
@metamaskbot
Copy link
Collaborator

Builds ready [e78cefd]
Page Load Metrics (520 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint2997612411
domContentLoaded3317375189947
load3327395209947
domInteractive3307375189947

Gudahtt
Gudahtt reviewed Dec 3, 2020
View reviewed changes
app/scripts/controllers/permissions/index.js Outdated Show resolved Hide resolved
app/scripts/metamask-controller.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [b176c8b]
Page Load Metrics (493 ± 32 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298543199
domContentLoaded3195574916732
load3205594936732
domInteractive3185574916732

brad-decker
brad-decker previously approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

awesome work, @rekmarks -- the separation of concerns in the permissions controller is super clean.

@brad-decker brad-decker assigned brad-decker and unassigned Gudahtt and danjm Dec 10, 2020
@rekmarks
Add approval controller
b5b5282 
* Use approval controller for badge updates
@rekmarks rekmarks force-pushed the approval-controller branch from b176c8b to b5b5282 Compare December 13, 2020 23:04
@rekmarks rekmarks requested a review from brad-decker December 13, 2020 23:05
@metamaskbot
Copy link
Collaborator

Builds ready [b5b5282]
Page Load Metrics (529 ± 14 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30604784
domContentLoaded4696045283014
load4706055293014
domInteractive4696035283014

@rekmarks rekmarks merged commit 8f40d03 into develop Dec 14, 2020
@rekmarks rekmarks deleted the approval-controller branch December 14, 2020 16:04
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Gudahtt Gudahtt Gudahtt left review comments

@brad-decker brad-decker brad-decker approved these changes

@danjm danjm Awaiting requested review from danjm

Assignees

@brad-decker brad-decker

Labels
None yet
Projects
None yet
Milestone
v8.1.next?
Development

Successfully merging this pull request may close these issues.
6 participants
@rekmarks @metamaskbot @kumavis @Gudahtt @brad-decker @danjm