Skip to content

Fix restricted Snap method implementations - #150

Merged
rekmarks merged 3 commits into
mainfrom
fix-snap-restricted-methods
Dec 8, 2021
Merged

rekmarks merged 3 commits into
mainfrom
fix-snap-restricted-methods

Conversation

@rekmarks

@rekmarks rekmarks commented Dec 8, 2021

Copy link
Copy Markdown
Contributor

Following #132, restricted methods can no longer be bound to the requesting origin. Instead, they receive a single object parameter with an object property context, which in turn has a string property origin that identifies the the subject that made the request.

This PR ensures that all restricted method hooks use context.origin where necessary instead of assuming that they are bound to the requesting origin.

@rekmarks
rekmarks requested a review from ritave December 8, 2021 19:27
export type ConfirmMethodHooks = {
/**
*
* @param snapId - The ID of the Snap that created the confirmation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is a "snapId" the same thing as an "origin"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. We need some terminology cleanup to happen after launch. In RPC-method-land, origin and snapId are used interchangeably. Probably we'll move to subjectIdentifier or something like that?

@Gudahtt Gudahtt Dec 8, 2021 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What will the snap ID be? Could it be expressed as an origin? We could just use origin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could perhaps use an origin, but in #140 I decided to encode the publishing location of the Snap in the ID itself to make it easy to do stuff like this: https://github.com/MetaMask/snaps-skunkworks/blob/snap-publishing-v0.1/packages/controllers/src/snaps/SnapController.ts/#L932-L939

@Gudahtt Gudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@rekmarks
rekmarks merged commit 0e50620 into main Dec 8, 2021
@rekmarks
rekmarks deleted the fix-snap-restricted-methods branch December 8, 2021 22:25
rekmarks added a commit that referenced this pull request Dec 10, 2021
Following #132, restricted methods can no longer be bound to the requesting origin. Instead, they receive a single object parameter with an object property `context`, which in turn has a string property `origin` that identifies the the subject that made the request.

This PR ensures that all restricted method hooks use `context.origin` where necessary instead of assuming that they are bound to the requesting origin.
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.

2 participants