anza-xyz / wallet-adapter

Modular TypeScript wallet adapters and components for Solana applications.
https://anza-xyz.github.io/wallet-adapter/
Apache License 2.0
1.59k stars 960 forks source link

[rfc] Work with wallet provider ecosystem to improve reliability and performance of wallet detection #215

Closed steveluscher closed 2 years ago

steveluscher commented 2 years ago

Sooner or later, the ability to determine who the user is and what they own finds itself on the critical path for any dApp to be able to deliver its core experience. For that reason, the performance and reliability of wallet detection and autoconnection is super important. In this RFC I would like to propose a different wallet readiness API than the one we have now, with the goal of being able to make wallet-bound decisions in dApps with 100% reliability and a minimum of delay.

Background

Today, most wallets announce themselves by adding variables to the global scope. For instance, when the Phantom extension boots, it adds the object window.phantom.solana to the global scope.

The situation today

The addition of an object to the global scope is not something that you can subscribe to. If dApps want to know whether Phantom is installed, they have to poll the global object looking for it.

Today, we do something like this in the Solana wallet adapter.

async function phantomIsInstalled(): Promise<boolean> {
  // Is this even a browser?
  if (typeof window === 'undefined' || typeof document === 'undefined') return false;

  function check() {
    return !!window.phantom?.solana;
  }

  // Is the page already loaded? Check now.
  if (document.readyState === 'complete') return check();

  // Wait until the page is loaded, then check.
  return new Promise(resolve => {
    function listener() {
      window.removeEventListener('load', listener);
      resolve(check());
    }
    window.addEventListener('load', listener);
  });
}

Problem

Reliability vulnerabilities

Performance liabilities

First, take a look at this diagram by Kudzanayi (link):

image

Antiproposal

One way to narrow the gap between a wallet becoming available and a dApp being able to make use of it is to poll more aggressively (eg. by listening to DOMContentLoaded or by firing a check using requestIdleCallback. Aggressive polling like this will add competition for the JS work queue, which can regress startup performance overall. We also have to continue polling forever, or risk missing wallets that choose to install themselves after the window's load event.

Proposal

The proposal is to work with wallet makers to settle on a Google Analytics style readiness API. The idea is this.

Create an array-compatible API that you can use to register a readiness callback.

type WalletReadinessCallback<Wallet> = (wallet: Wallet) => void;
interface PhantomAPI {
  push: (readinessCallback: WalletReadinessCallback<PhantomWallet>) => void;
  /* ... */
}

Consumers of this API must take care to create an empty array if it doesn't already exist in place of the expected wallet API. After that, they can call push() on it.

window.phantom: PhantomAPI = (window.phantom || []).push(
  (wallet) => {
    // Code to run the moment the wallet becomes available.
  },
);

This API makes it impossible to miss the installation of a wallet and reduces the delay between a wallet becoming ready and your app being able to use it to near-zero.

Example implementation

Using this API, the Solana wallet adapter's readiness callback (and wallet detection system) for Phantom would simply become this:

async function isPhantomInstalled(): Promise<boolean> {
  return new Promise((resolve) => {
    (window.phantom || []).push(() => resolve(true));
  });
}

This implementation would be unbreakable and would reduce the time between Phantom startup and wallet detection to near-zero.

Way forward

Thanks for reading! In order to achieve all of this, we'd need to reach out to wallet makers one by one, and then come up with a deprecation plan for the old polling-based implementation. I don't think there's a better time than the present to do this than now, since there are fewer Solana wallets at this moment than there will ever be :)

Please do poke at this RFC and let me know your thoughts.

jordaaash commented 2 years ago
  • If a wallet doesn't install its hooks by the time the window's load event has fired, we will miss it. Despite this being extremely difficult to reproduce, I have observed this happen in the wild, with Phantom.

Can you clarify "in the wild"? Until quite recently (https://github.com/solana-labs/wallet-adapter/pull/195), adapters were generally doing this by polling for 3 seconds which isn't great. Basically I'm wondering if this is an issue with the current code running in production or the old adapter code, which the overwhelming majority of apps are using.

jordaaash commented 2 years ago
  • The load event of a window fires after all image resources have loaded. In practice, this can delay our wallet readiness check by seconds

This definitely isn't great.

we'd need to reach out to wallet makers one by one, and then come up with a deprecation plan for the old polling-based implementation

This is doable for the most popular wallets that are actively maintained (Phantom, Slope) or open source (Sollet) even though it's not maintained. This may not be doable at all for some (Solong) that aren't maintained at all, but we could cover most users. We can also make it a requirement for new wallets to have their adapters added to this repo.

We also have to continue polling forever, or risk missing wallets that choose to install themselves after the window's load event.

How likely is this and what might cause it?

Side question: if a browser extension is installed, does the window need to be reloaded for it to be discovered?

One way to narrow the gap between a wallet becoming available and a dApp being able to make use of it is to poll more aggressively (eg. by listening to DOMContentLoaded or by firing a check using setIdleTimeout. Aggressive polling like this will add competition for the JS work queue, which can regress startup performance overall.

Offhand the performance of this doesn't seem too bad but I could see it getting out of hand with a bunch of adapters loaded. The design we're working toward with #206 / #214 is asking apps to just use all the adapters and not think about it. It makes sense for us to consider how many wallets this scales to.

steveluscher commented 2 years ago

Can you clarify "in the wild"?

Yeah, for sure. I ran this a bunch of times, and once I saw it hit the ‘Phantom was not detected after all’ console log. I've never been able to reproduce that since.

olistic commented 2 years ago

In general, I like this proposal. Some thoughts:

olistic commented 2 years ago

@steveluscher I believe there's a couple of bugs in the examples provided:

window.phantom: PhantomAPI = (window.phantom || []).push(/* ... */)

Should be:

window.phantom: PhantomAPI = window.phantom || []
window.phantom.push(/* ... */)

As Array.push() will return the new length of the array, and window.phantom would no longer reference the array or the PhantomAPI.

For the same reason, I think the example implementation should be:

async function isPhantomInstalled(): Promise<boolean> {
  return new Promise((resolve) => {
    window.phantom = window.phantom || []
    window.phantom.push(() => resolve(true))
  });
}
jordaaash commented 2 years ago

Closing here since this has been implemented in https://github.com/wallet-standard/wallet-standard