Conversation
rekmarks
force-pushed
the
approval-controller
branch
3 times, most recently
from
a86a95d to
a881e0d
Compare
rekmarks
force-pushed
the
approval-controller
branch
from
2c35ad2 to
35c49d5
Compare
|
Just so I make sure I understand. The grand scope of this Would it be used for:
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? |
|
@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
left a comment
brad-decker
left a comment
There was a problem hiding this comment.
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
rekmarks
force-pushed
the
approval-controller
branch
from
6998b65 to
97384c2
Compare
|
LGTM! |
* Add approval controller * Add approval controller tests * Add JavaScript tests for approval controller, update CI config
* Add approval controller * Add approval controller tests * Add JavaScript tests for approval controller, update CI config
This commit aligns this repo more closely with our module template. - Add `packageManager` to `package.json` - Add `allow-scripts` Yarn plugin - Add Yarn constraints (but modified to suit this package, whose build pipeline is not fully standardized with the module template) - Remove `setup` script (since `allow-scripts` gets called automatically now) - Replace `prepublishOnly` script with `prepack` - Update GitHub workflow to install Yarn via Corepack - Update Yarn-specific section of `.gitignore` - Update installation instructions in README - Upgrade `eslint-plugin-jsdoc` as the version we were using was previously not compatible with the version of Node we are using, but Yarn v1 did not pick this up. Fix lint violations to match.
--------- Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
2 participants
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.addAndShowApprovalRequestadds a pending request to the controller's state, and then callsshowApprovalRequest. 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, viaApprovalController.add.The approval controller was originally implemented in JavaScript in this extension PR, which will be updated to import the approval controller from
@metamask/controllersonce 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.