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

doc: add a sequence diagram of the snap invocation #1356

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

naugtur
Copy link

@naugtur naugtur commented Apr 18, 2023

This is a sequence diagram based on a security-focused walkthrough we had on 2023-02-20
The level of detail is arbitrary, but adding more would seem like rewriting the code into a diagram.

Descriptions and notes might need clarification.

@FrederikBolding I left you a todo at the bottom because I didn't want to make things up ;)

Preview:
https://github.com/MetaMask/snaps-monorepo/blob/naugtur/diagram/diagram.md

@naugtur naugtur requested a review from a team as a code owner April 18, 2023 11:16
@naugtur naugtur marked this pull request as draft April 18, 2023 11:20
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #1356 (e129192) into main (c6a524b) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1356      +/-   ##
==========================================
+ Coverage   96.20%   96.34%   +0.14%     
==========================================
  Files         149      151       +2     
  Lines        4555     4598      +43     
  Branches      743      747       +4     
==========================================
+ Hits         4382     4430      +48     
+ Misses        173      168       -5     

see 31 files with indirect coverage changes

@FrederikBolding
Copy link
Member

I fixed a few things and added some detail as to how responses are returned. I'm not sure all the notes are still relevant to include though 🤔

Copy link
Member

@Mrtenz Mrtenz left a comment

Choose a reason for hiding this comment

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

Lots of nits. Sorry.

diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated Show resolved Hide resolved
diagram.md Outdated

`subjectType` is being checked before a middleware gets to handle an RPC request. The snap is going through the same permission mechanism in the provider as a dApp would.

PermissionsController is fed the snapId as origin, but the snapId is coming from
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is incomplete.

Copy link
Author

Choose a reason for hiding this comment

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

oh no, a lost stream of consciousness. gonna be hard to finish now :D

diagram.md Outdated Show resolved Hide resolved
FrederikBolding and others added 2 commits May 4, 2023 14:14
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
@naugtur naugtur requested a review from Mrtenz May 19, 2023 07:31
@Mrtenz Mrtenz removed their request for review March 1, 2024 09:01
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.

None yet

3 participants