solana-labs / explorer

Explorer for Solana clusters
https://explorer.solana.com
MIT License
231 stars 291 forks source link

Replace `MultipleAccountsFetcher` with a `fetch` shim that coalesces account info requests coming from any code #296

Open steveluscher opened 1 year ago

steveluscher commented 1 year ago

Describe the bug The code that's supposed to coalesce multiple account fetches behind a single getMultipleAccounts is not doing that. They're all getting coalesced into batches of 1.

To Reproduce

  1. Visit https://explorer.solana.com/tx/2tmgfqVeqmqPdi2N18KFRvJwMj41ukiCKK7vJqQGGjvM1uRTY5LWjtoFAjWJ7Q4XZxcvfvvfQWkT5BhQGGhFpqyg

Screenshots

image

Additional context

This code, here.

https://github.com/solana-labs/explorer/blob/15a5268a1729168230de077c6758e1ad62f9ec52/app/providers/accounts/index.tsx#L135-L145

Suspicious function name in the stack trace: findAllByMintList.

Suggested implementation

https://github.com/solana-labs/explorer/pull/298#issuecomment-1734377246

mcintyre94 commented 1 year ago

See comment here for suggested approach to override window.fetch and replace MultipleAccountFetcher: https://github.com/solana-labs/explorer/pull/298#issuecomment-1734377246

jstarry commented 1 year ago

I took a look at this and I think we should take a different approach rather than using a fetch shim. Using a shim is supposed to provide a convenient catch-all for batching account requests and is most useful when it's difficult to construct a code path that batches requests but I don't think it's too common. In the event that we have a bunch of different places requesting accounts at the same rendering step, chances are that some of the params will be different and therefore the requests can't be batched anyways. I'm also just not in favor of magic shims that increase complexity and error surface area.

steveluscher commented 1 year ago

Thanks for taking a look!

Magic is sometimes bad, yes. In the case we're dealing with here the calls are going through third-party code (@metaplex/js) where fetch is the only common denominator.

https://github.com/solana-labs/explorer/blob/e7ee73dcf7aa781256129360375fa31216f43471/app/components/common/Address.tsx#L105-L106

Things we could do:

  1. Wrap globalThis.fetch and coalesce similar requests
  2. Instead of new Connection() in useTokenMetadata we could pass the Metaplex code a Connection-conforming object that actually uses the MultipleAccountFetcher under the hood when Metaplex calls getAccountInfo() on it.
  3. Eliminate @metaplex/js and fetch the data ourselves

As the person who suggested it, I obviously prefer the general solution of shimming globalThis.fetch. It sounds completely nuts, but is actually what next.js does under the hood to implement client-side route caching (link)!

jstarry commented 1 year ago

I think caching makes sense to do in an override because it can be generically implemented. This coalescing is inherently not possible to do generically and is brittle in the case that the response structure of getMultipleAccounts and getAccountInfo change independently