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

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

Open
steveluscher opened this issue Sep 22, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@steveluscher
Copy link
Collaborator

steveluscher commented Sep 22, 2023

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.

if (this.fetchTimeout === undefined) {
this.fetchTimeout = setTimeout(() => {
this.fetchTimeout = undefined;
if (this.pubkeys !== undefined) {
const pubkeys = Array.from(this.pubkeys).map(p => new PublicKey(p));
this.pubkeys.clear();
const { dispatch, cluster, url, dataMode } = this;
fetchMultipleAccounts({ cluster, dataMode, dispatch, pubkeys, url });
}
}, 100);

Suspicious function name in the stack trace: findAllByMintList.

Suggested implementation

#298 (comment)

@steveluscher steveluscher added the bug Something isn't working label Sep 22, 2023
@steveluscher steveluscher assigned ngundotra and unassigned ngundotra Sep 22, 2023
@mcintyre94
Copy link
Collaborator

See comment here for suggested approach to override window.fetch and replace MultipleAccountFetcher: #298 (comment)

@steveluscher steveluscher changed the title [Bug] getMultipleAccounts coalescer, isn't Replace MultipleAccountsFetcher with a fetch shim that coalesces account info requests coming from any code Sep 26, 2023
@jstarry
Copy link
Member

jstarry commented Oct 20, 2023

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
Copy link
Collaborator Author

steveluscher commented Oct 20, 2023

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.

const connection = new Connection(url);
const metadata = await programs.metadata.Metadata.load(connection, pda);

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
Copy link
Member

jstarry commented Oct 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants