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

Solflare: Initialize MetaMask standard adapter #824

Merged
merged 18 commits into from
Sep 8, 2023

Conversation

vsakos
Copy link
Contributor

@vsakos vsakos commented Sep 1, 2023

Initialize the Solflare MetaMask standard adapter in the constructor of the Solflare adapter.

@jordaaash
Copy link
Collaborator

Sorry, I can't accept this PR as is. It introduces a new dependency on a third party SDK, which in turn includes other third party dependencies. Third party SDKs must be loaded asynchronously in the connect function of adapters after the user has selected the wallet.

However, I understand that approach won't produce your desired outcome. You want to be able to detect Metamask, maybe further detect Snap support asynchronously, and inject a Wallet Standard wrapper if found, before a user would ever select the wallet to connect. This implies some refactoring of the two dependencies, solflare-metamask-sdk and metamask-adapter-wallet-standard.

For me to include code to do what you want in the adapter here, you'll need to change the approach to only perform detection and injection of a very minimal Standard Wallet to interface with Metamask. It may not include any new build-time dependencies that aren't already part of Wallet Adapter / Standard.

Almost all the implementation of this interface, such as this and this, would need to be loaded asynchronously by the Standard Wallet. This should be possible to do by stubbing the required feature methods (including standard:connect), and having them asynchronously load an SDK when called.

I'll leave this PR open. Please let me know when it's ready for review or if clarification on the proposed approach is needed.

@jordaaash
Copy link
Collaborator

After some discussion with other maintainers, we've decided to stick with requiring one adapter to correspond with one wallet and accordingly not make an exception here to combine Metamask detection with the Solflare adapter.

We can happily include a separate adapter, or a function to detect and register support using a Standard Wallet since you already have built that, in the @solana/wallet-adapter-wallets package.

This shouldn't require a significant change to the latest approach in this PR. It will require that dapps

  • Update to the latest @solana/wallet-adapter-wallets version
  • Import a new function (e.g. registerSolflareMetamaskSnapWallet), or an adapter (e.g. SolflareMetamaskSnapAdapter, from the package
  • Respectively, call the function, or instantiate the adapter and pass it to their wallets array

I think the most straightforward and future-proof way to do it would be to register a Standard Wallet rather than an adapter. If you're okay with me making a change to this PR, I can add a commit to suggest the shape of that implementation.

@jordaaash
Copy link
Collaborator

jordaaash commented Sep 7, 2023

I've proposed some refactoring @ vsakos#2 that will land here when merged.

A few things we need to confirm/fix:

  • The chain identifiers supported by the wallet.
  • The SVG used for the icon contains a large bitmap.

As for the release and time window we'll allow for this change, we will give until December 1, 2023. This is aggressive but we want to only accept this exception to how adapters are injected for a limited time, and to be able to remove this before the holidays are in full swing and people take holiday breaks. This gives over a month and a half (I'm bad at math) almost 3 months for apps to update to use Solflare's own package. This is a one time exception, and after that, we'll update the adapter to revert these changes, and apps will be encouraged to update to whatever the latest versions are at that time.

@jordaaash jordaaash force-pushed the solflare-metamask-standard-adapter branch from 18e4f90 to fd8cacc Compare September 8, 2023 04:22
Comment on lines 147 to 149
if (intervalAttempts <= 0) {
clearInterval(interval);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The impetus behind this poller was to handle this use case (#327):

  1. You arrive on site A that tells you to install a wallet.
  2. You install it by whatever means.
  3. Without refreshing site A, the newly installed wallet becomes available.

This change would cause a regression.

Hmm. It may be time to actually complete #334, but to go even further.

  1. Schedule detection as an idle callback so as to be a good citizen.
  2. Pause detection when the window blurs/backgrounds, and resume it when the window focuses/foregrounds. Do this with a combination of blur/focus and visibilitychange events.

Copy link
Collaborator

Choose a reason for hiding this comment

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

U rite

Copy link
Collaborator

Choose a reason for hiding this comment

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

Reopened #334 and #337

@jordaaash jordaaash merged commit 02f20c2 into anza-xyz:master Sep 8, 2023
3 checks passed
@jordaaash jordaaash deleted the solflare-metamask-standard-adapter branch September 8, 2023 21:30
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.

3 participants