explorer icon indicating copy to clipboard operation
explorer copied to clipboard

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

Open steveluscher opened this issue 1 year ago • 4 comments

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

steveluscher avatar Sep 22 '23 15:09 steveluscher

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

mcintyre94 avatar Sep 26 '23 15:09 mcintyre94

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.

jstarry avatar Oct 20 '23 11:10 jstarry

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)!

steveluscher avatar Oct 20 '23 20:10 steveluscher

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

jstarry avatar Oct 21 '23 04:10 jstarry