-
Notifications
You must be signed in to change notification settings - Fork 115
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 metamask-snap support #208
feat: add metamask-snap support #208
Conversation
@naorye2 please use the new release "1.0.0-dev-fc04076-202312205118" for your development |
- Show metamask if there is metamask extension with snap support - Load consensys metamask package if the user selects metamask from the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for your work, i leave some comment
- Add @module-federation/runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, A small change request on createMetaMaskProviderWrapper
name: "MetaMaskStarknetSnapWallet", | ||
alias: "MetaMaskStarknetSnapWallet", | ||
entry: | ||
"https://s3.eu-central-1.amazonaws.com/dev.snaps.consensys.io/get-starknet/remoteEntry.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we move this URL in discovery.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stanleyyconsensys
I am not sure this url need to be part of the discovery as this is relevant only for metamask.
In version 4 there is a definition of Virtual Wallet the includes this url.
packages/core/src/main.ts
Outdated
@@ -75,6 +76,8 @@ export function getStarknet( | |||
} | |||
const lastConnectedStore = storageFactoryImplementation("gsw-last") | |||
|
|||
injectMetamaskBridge() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass ssrSafeWindow
to avoid assuming window
availability later inside injectMetamaskBridge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
async function hasSnapSupport(provider: MetaMaskProvider) { | ||
try { | ||
await provider.request({ method: "wallet_getSnaps" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use non-api call to check for starknet support (not just and snaps?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @avimak the life cycle of a snap is working like that way
- you detect where you have a install metamask
- you detect where you have an correct version of metamask -> which using similar method as above
- then you ask metamask to install the starknet snap -> that should be trigger by enable() / or any similar method
so you can only cehck if the metamask support starknet by first checking if it has supported snap
In addtional, metamask does not inject any extra method to the provider (window.ethereum) to verify if it is support snap, it means, you must using request method to determined
does it help to clarify your questions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avimak
After consulting with @stanleyyconsensys we decided to remove that check.
Pending for metamask-starknet packge implementation