Add Dynamic Permissions #2088
Open
Add Dynamic Permissions #2088
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
packages/snaps-cli/src/commands/manifest/implementation.test.ts
Outdated
Show resolved
Hide resolved
david0xd
force-pushed
the
dd/dynamic-permissions
branch
2 times, most recently
from
fdb83ad to
9711d60
Compare
david0xd
force-pushed
the
dd/dynamic-permissions
branch
2 times, most recently
from
a479c44 to
bc8e2f7
Compare
david0xd
force-pushed
the
dd/dynamic-permissions
branch
from
2559e6f to
e28c9c3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2088 +/- ##
==========================================
+ Coverage 96.60% 96.73% +0.12%
==========================================
Files 337 340 +3
Lines 7610 7693 +83
Branches 1180 1198 +18
==========================================
+ Hits 7352 7442 +90
+ Misses 258 251 -7 ☔ View full report in Codecov by Sentry. |
david0xd
force-pushed
the
dd/dynamic-permissions
branch
2 times, most recently
from
ab403e8 to
9882a1e
Compare
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
david0xd
commented
Jan 24, 2024
david0xd
force-pushed
the
dd/dynamic-permissions
branch
3 times, most recently
from
f2347c9 to
c035ac2
Compare
david0xd
force-pushed
the
dd/dynamic-permissions
branch
2 times, most recently
from
d1f7e63 to
01c8ce2
Compare
There was a problem hiding this comment.
Once this is rebased we should make sure to support https://github.com/MetaMask/snaps/blob/main/packages/snaps-controllers/src/snaps/SnapController.ts#L1216-L1218
david0xd
force-pushed
the
dd/dynamic-permissions
branch
6 times, most recently
from
841f74b to
3a1a5f3
Compare
13 tasks
packages/snaps-rpc-methods/src/permitted/requestPermissions.test.ts
Outdated
Show resolved
Hide resolved
david0xd
force-pushed
the
dd/dynamic-permissions
branch
from
64c3f03 to
09f13a7
Compare
david0xd
force-pushed
the
dd/dynamic-permissions
branch
from
659014e to
b472197
Compare
FrederikBolding
requested changes
Feb 29, 2024
david0xd
force-pushed
the
dd/dynamic-permissions
branch
from
b472197 to
9741a63
Compare
Refactoring (1) Fix unit test coverage in utils Register new method and refactor types Add RPC method implementation for request permissions Add more tests for the permission request RPC method Revert controller changes Add getPermissions RPC method and do some refactoring Add initial snap_revokePermissions RPC Method implementation Fix after rebase Do some refactoring Fix Lavamoat policy Fix import in SDK methods Add revokePermissions method validation and tests Fix after rebase Revert old chromedriver Review request refactoring Fix after rebase Fix types in test Refactor added RPC methods and tests Update hook params Add permission related types from PermissionController Fix RPC methods coverage after rebase Fix coverage for utils after rebase Fix after rebasing Add processing of the permissions before sending the request Fix after rebase Fix snaps-utils coverage after rebase Modify logic for allow-listing requirement for dynamic permissions Addressing review requests (refactoring) Addressing review requests (return type change) Update dynamic permission allow-list requirement logic Remove comment
david0xd
force-pushed
the
dd/dynamic-permissions
branch
from
b1becee to
3debb3e
Compare
13 tasks
david0xd
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Apr 24, 2024
## **Description** This PR introduces some major code refactoring and UI updates to **_Connect_** and **_Permission Approval_** screens. #### Motivation There are several blockers encountered while trying to introduce (integrate) new features provided by Snaps Core Platform such as Dynamic Permissions. The major reason for a blocker is the state of the current implementation of the _Permission Approval_ component. More specifically, the large difference and inconsistencies between the UI components and overall UI structure used for the native and Snaps flows related to the permission approval features are making it close to impossible to maintain and extend with the new functionalities. Building even more fragmented conditional UI in order to introduce new features, as part of the same flows and features, would dramatically increase code complexity and maintenance demands, while resulting in poor UX/UI in the final product. Hence, after multiple discussions inside the teams and between the different teams, it is determined that the best approach would be complete UI refactoring and consolidation of the user interface between native and Snaps flows. This PR is the first major step made in order to achieve the proposed results and unblock the new features waiting for integration. #### What is done in this PR - Updated UI for Account Connection and Permission Approval screens. - Refactoring of the Connection and Permission Approval components. - Refactoring of the other components related to Connect and Permission pages. - Replaced old way of UI styling with the new one aligned with Design System approach. This includes replacement of all deprecated Design System components with the current. - Removed redundant SCSS code. UI styling is now mostly handled by Design System components. - Updated some parts of the Storybook for the related components. [](https://codespaces.new/MetaMask/metamask-extension/pull/23557?quickstart=1) ## **Related issues** Fixes: #23316 Design: https://www.figma.com/file/jr7XsFvcPCnf4HOMvNWFfn/Permission-System?node-id=1%3A2&mode=dev (see facelift section). Blocks: MetaMask/snaps#1821 Blocks: MetaMask/snaps#2088 Blocks: MetaMask/snaps#2198 ## **Manual testing steps** 1. Go to [E2E Test Dapp](https://metamask.github.io/test-dapp/) or [Test Snaps](https://metamask.github.io/snaps/test-snaps/latest/) website. 2. Try connecting the accounts to the Test Dapp. Try approving permission requested. Check the UI. 3. Try installing a Snap and check all the related UI starting from a connection to the website. 4. Try installing Ethereum Provider Snap and verify the UI for the complete flow including triggering option for getting Ethereum accounts. 5. Try using Multi Install Snap feature provided by the Test Snaps web application. 6. Check any other website connection or permission approval related flows and features and make sure that there are no regressions. 7. Verify that there are no regressions in all the UX/UI related to the all features mentioned above. Also, verify the functionality of the related features. ## **Screenshots/Recordings** ### **Before**  <img width="1018" alt="Screenshot 2024-03-29 at 15 35 51" src="https://github.com/MetaMask/metamask-extension/assets/13301024/b06fa630-3912-445a-80e8-00f85a0ce301"> <img width="1462" alt="Screenshot 2024-03-29 at 15 37 26" src="https://github.com/MetaMask/metamask-extension/assets/13301024/20b4cea6-9258-482a-9eb4-38de2234f15a"> <img width="927" alt="Screenshot 2024-03-29 at 15 39 52" src="https://github.com/MetaMask/metamask-extension/assets/13301024/c7643215-78cc-43b8-8c89-5ae504e05bb2"> ### **After**       ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
](https://codespaces.new/MetaMask/metamask-extension/pull/23557?quickstart=1){kind=link}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds new RPC methods used to support dynamic permissions.
Fixes: #1821
Dynamic Permissions feature is specified in SIP-14.
Related
metamask-extensionPR: MetaMask/metamask-extension#22878Summary of changes in this PR
RPC Methods
Three new RPC methods are added to enable Dynamic Permissions feature functionality:
snap_requestPermissionssnap_getPermissionssnap_revokePermissionsAll RPC methods added are following naming convention proposed in
SIP-14.Manifest
dynamicPermissionsfield is added to the manifest.New structure
DynamicPermissionsStructis added to the manifest validation process and refined to follow the validation requirements proposed in the SIP-14.Snap Controller