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.

Sign up for GitHub

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

Jump to bottom

BREAKING: Remove wallet global in favor of snaps and ethereum #939

Merged
merged 13 commits into from Nov 15, 2022
Merged

BREAKING: Remove wallet global in favor of snaps and ethereum #939

merged 13 commits into from Nov 15, 2022

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Nov 9, 2022 •
edited

Removes the wallet object. Replacing it with snaps and ethereum. snaps is always exposed to a snap while ethereum requires a new permission endowment:ethereum-provider.

Fixes #908
Fixes #891

@FrederikBolding FrederikBolding changed the title Refactor global APIs BREAKING: Refactor global APIs Nov 10, 2022
@FrederikBolding FrederikBolding marked this pull request as ready for review November 11, 2022 10:06
@FrederikBolding FrederikBolding requested a review from a team as a code owner November 11, 2022 10:06
@FrederikBolding FrederikBolding changed the title BREAKING: Refactor global APIs BREAKING: Remove wallet global in favor of snaps and ethereum Nov 11, 2022
@FrederikBolding FrederikBolding changed the title BREAKING: Remove wallet global in favor of snaps and ethereum BREAKING: Remove wallet global in favor of snap and ethereum Nov 11, 2022
Mrtenz
Mrtenz requested changes Nov 11, 2022
View reviewed changes
packages/snaps-controllers/src/services/iframe/IframeExecutionService.test.ts Outdated Show resolved Hide resolved
packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts Outdated Show resolved Hide resolved
packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts Outdated Show resolved Hide resolved
packages/snaps-controllers/src/snaps/SnapController.test.ts Outdated Show resolved Hide resolved
packages/snaps-controllers/src/snaps/endowments/eip1193.ts Outdated Show resolved Hide resolved
packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts Outdated Show resolved Hide resolved
packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts Outdated Show resolved Hide resolved
packages/snaps-execution-environments/src/common/endowments/index.ts Outdated Show resolved Hide resolved
packages/snaps-execution-environments/src/common/endowments/index.ts Outdated Show resolved Hide resolved
packages/snaps-types/src/types.d.ts Outdated
@@ -22,7 +22,9 @@ export type OnCronjobHandler = (args: {
request: JsonRpcRequest<unknown[] | { [key: string]: unknown }>;
}) => Promise<unknown>;

export type SnapProvider = MetaMaskInpageProvider;
export type SnapAPI = { request: StreamProvider['request'] };
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "SnapsProvider"?

Copy link
Member Author

Choose a reason for hiding this comment

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

afaik we specifically wanted to move away from calling it a provider

Copy link
Member

Choose a reason for hiding this comment

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

"SnapsAPI"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@rekmarks

Copy link
Member

Choose a reason for hiding this comment

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

SnapsGlobalObject, perhaps? I'm inspired by the TC39 spec, where e.g. the JSON global is just called "The JSON object". In natural language we should refer to the SnapsGlobalObject as "the snaps object" or "the global snaps object".

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, this seems to imply that the object should be called snaps. Is that intended?

Copy link
Member

Choose a reason for hiding this comment

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

SnAPI

Mrtenz
Mrtenz previously approved these changes Nov 15, 2022
View reviewed changes
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.

LGTM. Just confirm with @rekmarks if it should be named SnapsObject or SnapObject before merging.

@FrederikBolding FrederikBolding changed the title BREAKING: Remove wallet global in favor of snap and ethereum BREAKING: Remove wallet global in favor of snaps and ethereum Nov 15, 2022
@FrederikBolding FrederikBolding merged commit 841ae5b into main Nov 15, 2022
@FrederikBolding FrederikBolding deleted the fb/global-apis branch November 15, 2022 13:03
@firnprotocol
Copy link

firnprotocol commented Jun 1, 2023 •
edited

hi, can i please ask why adding the endowment:ethereum-provider endowment results in a big scary orange warning?

it's hard to imagine a snap so simple that it can get away without invoking ethereum. for example, it couldn't even know which chainId it's on (or am i missing something?).

@Montoya
Copy link
Collaborator

Montoya commented Jun 1, 2023

hi, can i please ask why adding the endowment:ethereum-provider endowment results in a big scary orange warning?

it's hard to imagine a snap so simple that it can get away without invoking ethereum. for example, it couldn't even know which chainId it's on (or am i missing something?).

This was an oversight in the current version of Flask. We plan to make it a regular, not scary, permission before release to MetaMask extension.

@FrederikBolding
Copy link
Member Author

@firnprotocol This should be fixed in the latest version of Flask released to both the Chrome and FF stores!

@firnprotocol
Copy link

@FrederikBolding cool, thank you; i can confirm that it's been fixed.

unfortunately I can't proceed with development whatsoever until this discussion is resolved. this also means I will miss the deadline for auditing and getting "allowlisted" for snaps v1, which is a disappointment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritave ritave ritave left review comments

@rekmarks rekmarks rekmarks left review comments

@Mrtenz Mrtenz Mrtenz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add EIP-1193 endowment permission Finalize wallet provider API
6 participants
@FrederikBolding @firnprotocol @Montoya @ritave @Mrtenz @rekmarks