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

Add Approval Controller #289

Merged
merged 7 commits into from
Nov 3, 2020
Merged

Add Approval Controller #289

merged 7 commits into from
Nov 3, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Oct 20, 2020

Summary

This PR adds a generic "approval" controller for managing user confirmations and displaying them in the MetaMask UI. The controller is designed to be used in both the extension and mobile.

The approval controller accepts a single config argument, showApprovalRequest, which is a function that should trigger the display of a user confirmation in the MetaMask UI. ApprovalController.addAndShowApprovalRequest adds a pending request to the controller's state, and then calls showApprovalRequest. This allows the UI to select the pending request via the approval controller's state.

Optionally, a pending request can be added without calling showApprovalRequest, via ApprovalController.add.

The approval controller was originally implemented in JavaScript in this extension PR, which will be updated to import the approval controller from @metamask/controllers once this PR has been merged.

CI Changes

For the original approval controller implementation, I added input validation for all public methods (at least somewhere along their code paths), and unit tests to test the input validation. Since, it's impossible to pass invalid inputs in TypeScript, I had to write additional tests in JavaScript, and test against the built approval controller files. This necessitated making the build step in CI a precondition for formatting and unit tests.

Miscellaneous

This PR includes the lint/formatting changes also introduced in #291, which should be merged first.

@brad-decker
Copy link
Contributor

Just so I make sure I understand. The grand scope of this ApprovalController is managing any request for user approval/sign-off in MetaMask?

Would it be used for:

  1. Permissions request
  2. Transaction Approval
  3. Encryption/Decryption?

Transaction approval is a broad scope that covers every kind of tx flow, but is that the intention to normalize and update the way we handle these types of approvals in state?

@rekmarks
Copy link
Member Author

rekmarks commented Oct 26, 2020

@brad-decker, I think it'd be too difficult to replace the existing transaction confirmation logic, at least in the extension. But, broadly speaking, your suggestion is accurate. This could, and IMO should, replace the confirmation logic for the majority of requests in both the extension and mobile.

Aside from adding custom networks and the permissions system, this can be used to eliminate the messages managers, which includes encryption and decryption. In order to do that, there's another piece to the puzzle, which is the generic method middleware in the extension.

For the full vision, see this draft PR on the extension (which includes the original JavaScript approval controller): MetaMask/metamask-extension#9724

brad-decker
brad-decker previously approved these changes Oct 28, 2020
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.

This LGTM, just some questions. I referenced the PR you mentioned while reviewing and I'm excited about how much more concise the approval logic can be. Great work, @rekmarks

src/approval/ApprovalController.ts Outdated Show resolved Hide resolved
src/approval/ApprovalController.ts Outdated Show resolved Hide resolved
src/approval/ApprovalController.ts Outdated Show resolved Hide resolved
src/approval/ApprovalController.ts Show resolved Hide resolved
@brad-decker
Copy link
Contributor

LGTM!

@rekmarks rekmarks merged commit 1450178 into develop Nov 3, 2020
@rekmarks rekmarks deleted the approval-controller branch November 3, 2020 18:00
@rekmarks rekmarks changed the title Add approval controller Add Approval Controller Nov 3, 2020
@rekmarks rekmarks mentioned this pull request Nov 3, 2020
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* Add approval controller

* Add approval controller tests

* Add JavaScript tests for approval controller, update CI config
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* Add approval controller

* Add approval controller tests

* Add JavaScript tests for approval controller, update CI config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants