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

Add support for Metamask Snap #1327

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

Nick-1979
Copy link
Contributor

@Nick-1979 Nick-1979 commented Mar 14, 2024

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!

Screenshot 2024-07-04 at 14 38 47 Screenshot 2024-07-04 at 14 36 43

@Tbaut
Copy link
Contributor

Tbaut commented Mar 14, 2024

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.

@Nick-1979
Copy link
Contributor Author

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.

@TarikGul
Copy link
Member

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.

@Tbaut
Copy link
Contributor

Tbaut commented Apr 6, 2024

I'd love to get @rossbulat feedback on this. I wrote you on several other platforms BTW without answers. My main questions would be:

  • can you share (here or in DM) some metrics regarding how much usage does MM snaps have on the staking dashboard
  • have you had any feedback from users?

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.

Copy link

@Dioliode Dioliode left a comment

Choose a reason for hiding this comment

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

Tyu

Copy link

@Dioliode Dioliode left a comment

Choose a reason for hiding this comment

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

DioX

@rossbulat
Copy link

I'd love to get @rossbulat feedback on this. I wrote you on several other platforms BTW without answers. My main questions would be:

  • can you share (here or in DM) some metrics regarding how much usage does MM snaps have on the staking dashboard
  • have you had any feedback from users?

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.

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.

Copy link

@Dioliode Dioliode left a comment

Choose a reason for hiding this comment

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

DioX

@Tbaut
Copy link
Contributor

Tbaut commented Apr 7, 2024

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:

  • see their own balance
  • send/receive tokens
  • do other things such as see their NFT, stake, etc.

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.

@Nick-1979
Copy link
Contributor Author

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.

Copy link
Contributor

@Tbaut Tbaut left a 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.

packages/extension-dapp/src/bundle.ts Outdated Show resolved Hide resolved
packages/extension-dapp/src/bundle.ts Outdated Show resolved Hide resolved
packages/extension-dapp/src/snap/defaults.ts Outdated Show resolved Hide resolved
@Nick-1979 Nick-1979 closed this Jun 27, 2024
@Nick-1979 Nick-1979 reopened this Jun 27, 2024
@Nick-1979 Nick-1979 requested a review from Tbaut July 1, 2024 11:33
Copy link
Contributor

@Tbaut Tbaut left a 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?

@Nick-1979 Nick-1979 closed this Jul 4, 2024
@Nick-1979 Nick-1979 deleted the resolve-minor-confilicts branch July 4, 2024 10:24
@Nick-1979 Nick-1979 reopened this Jul 4, 2024
@Nick-1979
Copy link
Contributor Author

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!

@Nick-1979 Nick-1979 reopened this Jul 31, 2024
@Nick-1979 Nick-1979 requested a review from Tbaut July 31, 2024 11:44
@Nick-1979
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants