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

Use UTL API directly for address token info #298

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

mcintyre94
Copy link
Collaborator

@mcintyre94 mcintyre94 commented Sep 25, 2023

The problem here was not with our request coalescer, the getMultipleAccounts requests are coming from the UTL SDK. Specifically on the transaction page where we use <Address pubkey={pubkey} ... fetchTokenLabelInfo. We do this on addresses that may be, but probably are not, token mints, ie every address in a transaction.

Previously this check came from the baked in legacy token list. When the token list was removed it was moved to the UTL SDK. The UTL SDK first calls the UTL API to see if the address has known token info. If it does not then it makes an on-chain call using getMultipleAccounts to fetch the token info from the Metaplex metadata program if it exists.

I don't think this fallback is appropriate for our use case of fetchTokenLabelInfo on addresses. Most addresses displayed are not tokens, and we display many addresses simultaneously. This is leading to many unnecessary getMultipleAccounts calls, and rate limiting.

Instead for this use case, we should only use the UTL API. This may mean that brand new tokens can be displayed in eg the token details page, but won't be correctly labelled in addresses. But this is a better tradeoff than making an on-chain request for every address that we render that may be a token.

Note that we already use the same public API for search (https://github.com/solana-labs/explorer/blob/master/app/utils/token-search.ts), where the SDK also can't provide an on-chain fallback.

Most addresses displayed are not tokens, so the UTL SDK fallback to making an on-chain request is inappropriate
@vercel
Copy link

vercel bot commented Sep 25, 2023

@mcintyre94 is attempting to deploy a commit to the Solana Labs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Sep 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2023 3:12pm

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Cool! This is fine, but here are some other approaches to kill this for good.

  1. We could build a Vercel API endpoint to fetch this data that caches the hell out of it. Comes with cost and all the problems you get with caching.
  2. We could do what Next.js does, and shim fetch(). You'd wrap fetch() in your own method, inspect RequestInfo.body to detect JSON RPC calls to getMultipleAccounts or getAccountInfo in the same JS runloop, batch them together into one or more calls to getMultipleAccounts, delegate to the inner fetch() at the end of the runloop, and then fan the responses out to the callers.

Option 2 would also let us just delete the MultipleAccountsFetcher context, and is where I think we should be taking this anyway.

EDIT: Doing option 2 would also make the current PR unnecessary. It would Just Work™.

@steveluscher
Copy link
Collaborator

Something like this:

const oldFetch = globalThis.fetch.bind(globalThis);
const accountsToFetchAtEndOfRunloop = new Set();
const resultPromises = new Map();
globalThis.fetch = (...args) => {
    const [resource, options] = args;
    const body = JSON.parse(options.body);
    if (
        body.jsonrpc === '2.0' &&
        (body.method === 'getMultipleAccounts' || body.method === 'getAccountInfo')
    ) {
        let resolveResult;
        const resultPromise = new Promise(resolve => {
            resolveResult = resolve;
        });
        const address = body.params[0]; // Different for gMA of course.
        resultPromises.set(
            address,
            [
                ...(resultPromises.get(address) ?? []),
                resultPromise,
            ],
        );
        if (accountsToFetchAtEndOfRunloop.size === 0) {
            queueMicrotask(() => {
                const accounts = [...accountsToFetchAtEndOfRunloop];
                accountsToFetchAtEndOfRunloop.clear();
                const results = await oldFetch(resource, {
                    ...options,
                    body: JSON.stringify({
                      jsonrpc: '2.0',
                      method: 'getMultipleAccounts',
                      params: [accounts],
                    }),
                });
                const promises = {...resultPromises};
                resultPromises.clear();
                accounts.forEach((address, ii) => {
                    promises[address].forEach(resolve => {
                        resolve(results[ii]);
                    });
                });
            })
        }
        accountsToFetchAtEndOfRunloop.add(address);
        return await resultPromise;
    } else {
        return await oldFetch(...args);
    }
};

Obviously way more to the code than that (multiple caches based on the params passed to the call, data validation, error handling, maximum batch size before splitting into a separate gMA call, etc.) but something like that.

@mcintyre94
Copy link
Collaborator Author

Cool that looks like a way better approach! I'm going to land this as-is for now, but leave the multiple accounts issue open. We should definitely replace our multiple accounts fetcher with that custom fetch solution.

@mcintyre94 mcintyre94 merged commit 3e5b2a7 into solana-labs:master Sep 26, 2023
4 checks passed
@mcintyre94 mcintyre94 deleted the gma-fix branch September 26, 2023 17:31
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.

2 participants