-
Notifications
You must be signed in to change notification settings - Fork 415
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
Add support for Metamask Snap #1327
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR but why didn't you discuss this in an issue first! Hardcoding such snap and version is probably something we don't want to do. The polkadot package hasn't been released in many years, and still worked until now for the main reason that it has remained very generic. I am very much on the fence regarding this. From my experience, Metamask snaps is a very sub-optimal way to manage accounts for Polkadot. We have many other great wallets that are multi-chain, and do a much better job. The UX of snaps is clunky, you see your balance in some weird UI. Regarding the supported networks, do you have to add them manually? This is for sure opening a can of worms, with default snap etc. You don't want to favor and hardcode a snap rather than another. |
Hi @Tbaut, Thank you for your prompt feedback. Firstly, I'd like to clarify that this project was initiated several months ago with the support of W3F. Our project operates independently and currently offers a package available on npm named @polkagate/extension-dapp. By integrating it into the official @ polkadot/extension-dapp package, the entire ecosystem can benefit seamlessly without necessitating a package switch. Regarding versioning, the specified version serves as a foundation, allowing support for all subsequent versions without altering the extension-dapp package itself. While Polkagate Snap is designated as the default option for now, the provided code serves as placeholders for potential future snaps. They are not hardcoded but rather intended to accommodate future snap additions. We acknowledge the existence of other extensions and have indeed developed Polkagate extensions with extensive features. However, the intention behind Metamask snaps is to facilitate the onboarding of Metamask users, particularly those without any native Polkadot extensions installed, into the Polkadot ecosystem. Contrary to the perception of remaining generic and not being updated for years, the @polkadot/extension-dapp is regularly updated, with the latest version being updated on npm just a few hours ago. Furthermore, it's essential to note that connection to extensions can be managed via dapps, granting dapp developers the flexibility to choose which extensions or snaps to utilize. In essence, this PR merely upgrades the @polkadot/extension-dapp package, enabling dapps utilizing this package to integrate Metamask snaps, including Polkagate Snap, as a new option. As for account management, users can handle accounts via the Polkadot apps, and switching chains can be seamlessly achieved by switching chains within the apps, which the snap will recognize. By accepting this PR and publishing the new @polkadot/extension-dapp, and then updating the new version of this package on Polkadot JS apps, Metamask snaps will be another option for communicating with the apps, without the need for additional changes in the Polkadot JS apps code. The snap code is publicly available here, and we're more than willing to address any additional concerns or queries that may arise. Thank you for your attention and feedback. |
Generally speaking, there is a lot to unpack in this PR. There is no guarantee it will go in soon, but I will promise to do the research and look into this more deeply, as it requires a deep review. |
I'd love to get @rossbulat feedback on this. I wrote you on several other platforms BTW without answers. My main questions would be:
Also we absolutely need to make it fair for other snaps providers, having a hard-coded default doesn't work, there are already several snaps out there. |
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.
Tyu
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.
DioX
No strong opinions. They are good to have as options in the spirit of collaboration & to provide feedback loops to allow them time to evolve & improve. No feedback has been received regarding usage as of yet. Usage statistics are not being tracked. |
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.
DioX
The only feedback I've ever received from a user using a snap (that was on Solana) was a positive one in conjunction with a wallet app (this is Solarflare) that allows to do things such as:
Now the points 1 and 2 are crucial, if users with no polkadot compatible extension land on an app such as the staking dashboard for the first time, and install a snap from there, they are guaranteed to be totally lost. What's my account, how can I fund it, what's my balance... IMHO the snap would only be good to use with an all in one dashboard such as the Talisman and Subwallet's ones, or Nova Spektr. I purposefully don't mention pjs/apps because it shouldn't be a Dapp for first time Polkadot users. For this reason, I don't think it MM snaps should be pushed by default to all Dapps, or even worse, be forced to users as it's done right now in the staking dashboard, with the MM popup at launch without any user action. It should be available for devs who want to enable it though, without hardcoding anything in this lib. |
Polkagate Snap's initial release, already whitelisted and available on the MetaMask directory, focuses on displaying users' balances across different chains via the Snap homepage. I'm excited to share that our upcoming release, already implemented and undergoing testing, will encompass the additional features you mentioned, all within the standalone Snap. Regarding Snap usage, it's designed to benefit users unfamiliar with full-featured extensions like Polkagate extension or Talisman, who may only have MetaMask and are transitioning from the Ethereum ecosystem. By leveraging their familiar extension, they can seamlessly onboard to the Polkadot ecosystem. As for the dashboard issue, we've submitted a fix, which was merged yesterday, and we're actively working on further improvements to be released soon. At Polkagate, we strongly believe that our approach is a win-win solution for bringing users from other ecosystems into the Polkadot ecosystem. We hope that you will see the value in this approach. Thank you for considering our contributions. |
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.
I'm totally convinced (and I changed my mind) that Snaps have place, and that they may be useful to some users, in some specific conditions. My comments regarding the current implementation remain though. As this is the lib that is used by all dapps, hardcoding a default is not acceptable.
9998b76
to
d26efaf
Compare
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.
Can you please resolve conflicts before requesting a review again?
Update index.spec.ts
Update defaults.ts Update defaults.ts Delete note
Update bundle.ts
… inject snaps like what w3ux in Staking Dashboard needs
use snapList as the list of supported snaps
update types
847237b
to
526a273
Compare
Hi @Tbaut , Sure, thank you for your prompt response. The conflicts have been resolved. However, given the project's high activity recently, new PRs might introduce further conflicts. We are prepared to resolve any new conflicts as they arise. We are highly hopeful that with your agreement and @TarikGul ’s acceptance, this PR can be merged successfully. This will be a significant step forward, enabling the vast Metamask user base to easily onboard into the Polkadot ecosystem. Thank you for your support! |
fa4aedf
to
a62db1f
Compare
…te/polkadot-js-extension into resolve-minor-confilicts
Hi @TarikGul , I hope you're doing well. Given your busy schedule, I appreciate you taking the time to consider this. We believe merging PR #1327 to add support for the Metamask Snap will be a significant win-win for the community. This integration has undergone thorough security audits and is supported by W3F. By allowing users to leverage their familiar Metamask extension, it will facilitate seamless onboarding into the Polkadot ecosystem, especially for those coming from the Ethereum ecosystem. We have addressed the concerns raised regarding hardcoding and ensured the implementation is flexible for future updates. Moreover, the latest updates have resolved all conflicts, making it ready for a smooth merge. Thank you very much for your attention and support in advancing this integration. |
We're thrilled to announce the addition of a new 'snap' folder within the extension-dapp directory. This integration empowers websites like polkadotjs, leveraging @polkadot/extension-dapp, to seamlessly connect with PolkaGate's Metamask snap functionality.
Noteworthy Features:
Experience the Power of Polkagate Snap:
Smooth Integration:
Following this merge, all websites utilizing @polkadot/extension-dapp will automatically support Polkagate Metamask snap without requiring any additional updates. Just upgrading to the latest version of @polkadot/extension-dapp will do the trick!