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

feat: Add abstraction for Snaps permissions #25175

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

david0xd
Copy link
Contributor

@david0xd david0xd commented Jun 10, 2024

Description

Add abstraction for Snaps permissions. Permissions under certain weight threshold are hidden, following the rules described in the related task.

Change summary

  • Changes are made to Snap Instal and Snap Update components, as well in the hook useScrollRequired in order to handle the new scrolling behavior requested by @eriknson. The new requirement is to never show scroll arrow (requirement) again, if the user has scrolled to the bottom once.
  • New component SnapPermissionAdapter is added to avoid further code complication and duplication. This component will handle displaying final set of permissions.
  • Snap settings view is supposed to show all granted permissions at once. So, the new flag is added to the permission list component so the permission abstraction can be disabled in Snap settings.
  • Configurations for permission weight thresholds used for abstraction, are added to shared/constants/permissions.ts.
  • Permission weights are updated according to the notion document.

Notes

(For reviewers) Some small fragments of UI might be different from the design sources linked. This is outcome of the several discussions between me and @eriknson. For more information contact me or @eriknson.

Open in GitHub Codespaces

Related issues

Fixes: MetaMask/snaps#2235

Manual testing steps

  1. Try installing or updating Snaps with different numbers of permissions with different weights. The best option is to try many of the test Snaps.
  2. Verify that UI is rendered according to logic described in the related task.

Screenshots/Recordings

Before

Before there was no "See all permissions" and similar functionalities related to the scrolling. I think that's the only thing that could compare. There is probably no need to screenshot everything from before, etc. QA to verify.

After

Screenshot 2024-06-13 at 12 19 45
Screenshot 2024-06-13 at 12 20 01
Screenshot 2024-06-13 at 12 20 23
Screenshot 2024-06-13 at 12 20 33
Screenshot 2024-06-13 at 12 22 07
Screenshot 2024-06-13 at 12 21 56
Screenshot 2024-06-13 at 12 21 34
Screenshot 2024-06-13 at 12 21 20

Pre-merge author checklist

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.

@david0xd david0xd added the team-snaps-platform Snaps Platform team label Jun 10, 2024
@david0xd david0xd self-assigned this Jun 10, 2024
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.

@david0xd david0xd changed the title Add abstraction for Snaps permissions feat: Add abstraction for Snaps permissions Jun 10, 2024
@david0xd david0xd force-pushed the dd/abstract-system-level-permissions branch 4 times, most recently from 06cac8e to 82ef486 Compare June 13, 2024 10:04
@metamaskbot
Copy link
Collaborator

Builds ready [82ef486]
Page Load Metrics (46 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint64897773
domContentLoaded9131011
load40724684
domInteractive9131011
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.35 KiB (0.08%)
  • common: 273 Bytes (0.00%)

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.06%. Comparing base (717376e) to head (1861d6e).
Report is 13 commits behind head on develop.

Files Patch % Lines
ui/helpers/utils/util.js 81.82% 2 Missing ⚠️
ui/hooks/useScrollRequired.js 71.43% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25175      +/-   ##
===========================================
- Coverage    70.13%   70.06%   -0.07%     
===========================================
  Files         1435     1422      -13     
  Lines        50309    49946     -363     
  Branches     13897    13860      -37     
===========================================
- Hits         35283    34991     -292     
+ Misses       15026    14955      -71     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@david0xd david0xd marked this pull request as ready for review June 13, 2024 11:04
@david0xd david0xd requested review from a team as code owners June 13, 2024 11:04
@david0xd david0xd force-pushed the dd/abstract-system-level-permissions branch from 0984e69 to 2525138 Compare June 23, 2024 16:22
@metamaskbot
Copy link
Collaborator

Builds ready [af6896d]
Page Load Metrics (52 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65937974
domContentLoaded9121110
load41695284
domInteractive9121110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.81 KiB (0.08%)
  • common: 881 Bytes (0.01%)

@david0xd david0xd force-pushed the dd/abstract-system-level-permissions branch from 4611681 to d352000 Compare June 25, 2024 11:18
@metamaskbot
Copy link
Collaborator

Builds ready [d352000]
Page Load Metrics (53 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6510181105
domContentLoaded9261242
load39705394
domInteractive9261242
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.71 KiB (0.08%)
  • common: 881 Bytes (0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [9a40b7e]
Page Load Metrics (63 ± 6 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5612093157
domContentLoaded9532594
load458463136
domInteractive9532594
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.15 KiB (0.07%)
  • common: 1.12 KiB (0.02%)

@david0xd david0xd force-pushed the dd/abstract-system-level-permissions branch from 9a40b7e to 3084981 Compare July 15, 2024 09:54
@david0xd david0xd force-pushed the dd/abstract-system-level-permissions branch from 3084981 to d53b6db Compare August 12, 2024 12:58
Refactor and handle edge cases

Turn off abstraction for Snap settings

Fix broken unit test and refactor some stuff

Fix Snap View unit test

Fix lint in mock-state.json

Try fixing scrolling

fix hasScrolledToBottom

Fix scrolling on snap update

Apply some non-functional refactoring

Update and refactor Permission Weights

Remove unused function for permission icon

Fix another edge case with required scrolling

Update permission weights again

Fix permission fade-overlay on Snap update

Try fixing failing e2e tests

Add additional e2e test fix

Minor refactoring

Do some requested refactoring

Fix edge case when showing only less important permissions

Add some comments

Fix permission weight spec

Add different implementation for rendering permission list

Update permission weight for accounts

Revert e2e test changes for accounts

Fix unit test for permission list

Revert some other e2e test changes as well

Refactor initial show all logic

Remove useMemo on Snap update

Refactor usage of permission adapter

Update show all initial state logic for update
@david0xd david0xd force-pushed the dd/abstract-system-level-permissions branch from 5796676 to 1ea21d9 Compare August 13, 2024 15:45
@metamaskbot
Copy link
Collaborator

Builds ready [1ea21d9]
Page Load Metrics (274 ± 301 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint58149972311
domContentLoaded95929178
load382278274627301
domInteractive95929178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.28 KiB (0.07%)
  • common: 1.34 KiB (0.02%)

ui/helpers/utils/util.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [4af4851]
Page Load Metrics (460 ± 354 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint713091265024
domContentLoaded980342210
load481991460738354
domInteractive980342210
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.25 KiB (0.07%)
  • common: 1.28 KiB (0.02%)

Copy link

@metamaskbot
Copy link
Collaborator

Builds ready [1861d6e]
Page Load Metrics (146 ± 182 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6615089199
domContentLoaded9451794
load411799146380182
domInteractive9451794
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 5.25 KiB (0.07%)
  • common: 1.29 KiB (0.02%)

@david0xd david0xd merged commit de89667 into develop Aug 15, 2024
78 checks passed
@david0xd david0xd deleted the dd/abstract-system-level-permissions branch August 15, 2024 13:31
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 15, 2024
@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstract system-level permissions
7 participants