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

Bump snaps packages and support dynamic interfaces #22828

Merged
merged 25 commits into from Feb 22, 2024

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Feb 6, 2024

Description

Bump snaps packages to latest and deals with breaking changes.

Summary of changes in the snaps deps:

  • BREAKING: Update permission format for name lookup Snaps
  • Add dynamic interfaces
    • Can be created with snap_createInterface and updated with snap_updateInterface
    • All existing UI is backwards compatible
  • Add support for Snap defined timeouts
    • A Snap can now define maxRequestTime to extend or shorten its execution timeout
  • Loosen allowlist requirements
    • Snaps with certain permissions can now be installed without being allowlisted
  • Reintroduced DecompressionStream for improved installation performance
  • Endowment permission specifications were moved requiring imports to be updated
  • Bump tar-stream letting us remove a patch

This PR also contains #22795 merged in, which contains the integration for dynamic interfaces.

Related issues

Closes https://github.com/MetaMask/MetaMask-planning/issues/1954

Manual testing steps

  1. Fire up the interactive UI test snap https://metamask.github.io/snaps/test-snaps/latest/
  2. Make sure it works as intended

@FrederikBolding FrederikBolding added the team-snaps DEPRECATED: Use "team-snaps-platform" or "team-snaps-ecosystem" instead label Feb 6, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 6, 2024
@FrederikBolding FrederikBolding changed the title Fb/snaps bump v31 Bump snaps packages Feb 6, 2024
Copy link

socket-security bot commented Feb 6, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@metamask/post-message-stream@8.0.0, npm/@metamask/snaps-controllers@5.0.1, npm/@metamask/snaps-rpc-methods@6.0.0

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@FrederikBolding
Copy link
Member Author

@SocketSecurity ignore npm/@metamask/post-message-stream@8.0.0
@SocketSecurity ignore npm/@metamask/snaps-utils@6.0.0
@SocketSecurity ignore npm/@metamask/snaps-controllers@5.0.0
@SocketSecurity ignore npm/@metamask/snaps-sdk@2.0.0
@SocketSecurity ignore npm/@metamask/snaps-rpc-methods@6.0.0

Trusted authors and these were just published.

@FrederikBolding FrederikBolding changed the title Bump snaps packages Bump snaps packages and support dynamic interfaces Feb 14, 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

With the blessing of @brad-decker, I've deleted this story as it is broken due to circular dependencies.

@FrederikBolding FrederikBolding marked this pull request as ready for review February 19, 2024 09:24
@FrederikBolding FrederikBolding requested review from a team as code owners February 19, 2024 09:24
@FrederikBolding
Copy link
Member Author

@SocketSecurity ignore npm/@metamask/snaps-controllers@5.0.1
just published.

@metamaskbot
Copy link
Collaborator

Builds ready [2405d1f]
Page Load Metrics (1062 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1262701913919
domContentLoaded9124413818
load9051365106213464
domInteractive9124413818
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 405.31 KiB (11.94%)
  • ui: 19.67 KiB (0.28%)
  • common: -398.57 KiB (-7.92%)

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (3687b18) 68.50% compared to head (751bc5b) 68.46%.

❗ Current head 751bc5b differs from pull request most recent head 0767d65. Consider uploading reports for the commit 0767d65 to get more accurate results

Files Patch % Lines
app/scripts/metamask-controller.js 47.06% 9 Missing ⚠️
ui/store/actions.ts 0.00% 8 Missing ⚠️
ui/selectors/selectors.js 57.14% 6 Missing ⚠️
ui/ducks/domains.js 0.00% 5 Missing ⚠️
...i/pages/confirmations/confirmation/confirmation.js 50.00% 5 Missing ⚠️
ui/hooks/useTransactionInsights.js 66.67% 2 Missing ⚠️
...ages/confirmations/confirmation/templates/index.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #22828      +/-   ##
===========================================
- Coverage    68.50%   68.46%   -0.04%     
===========================================
  Files         1089     1089              
  Lines        43076    43107      +31     
  Branches     11480    11487       +7     
===========================================
+ Hits         29507    29511       +4     
- Misses       13569    13596      +27     

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

package.json Show resolved Hide resolved
FrederikBolding and others added 17 commits February 22, 2024 10:50
Co-authored-by: Hassan Malik <41640681+hmalik88@users.noreply.github.com>
## **Description**

This PR integrates the necessary changes for interactive Interfaces.

- Setup and update the various controllers.
- Update the `SnapUIRenderer` to handle an `interfaceId` rather than
direct content.
- Add the logic for interface state and event handling in a new
`InterfaceContext` react context.
- Update the different APIs to handle the newly returned values.
- Move the UI components mapping out of the `SnapUIRenderer` file
- Rework confirmations to take the `SnapUIRenderer`.

## **Related issues**

## **Manual testing steps**

1. Fire up the interactive UI test snap [coming soon]
2. Play with the snap

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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.
- [ ] 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.

---------

Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
@FrederikBolding FrederikBolding merged commit ae291c1 into develop Feb 22, 2024
64 of 66 checks passed
@FrederikBolding FrederikBolding deleted the fb/snaps-bump-v31 branch February 22, 2024 10:07
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
@metamaskbot metamaskbot added the release-11.12.0 Issue or pull request that will be included in release 11.12.0 label Feb 22, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [0767d65]
Page Load Metrics (1995 ± 109 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint943641877234
domContentLoaded108528199
load155724041995227109
domInteractive108528199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 405.31 KiB (11.93%)
  • ui: 19.68 KiB (0.28%)
  • common: -398.57 KiB (-8.01%)

@ziedbrini ziedbrini added team-snaps-platform Snaps Platform team and removed team-snaps DEPRECATED: Use "team-snaps-platform" or "team-snaps-ecosystem" instead labels Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-11.12.0 Issue or pull request that will be included in release 11.12.0 team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants