autoload: AutoloadResult.ContractResult

[shazow]

Fixes #130

[shazow]

@yohamta What do you think?

[yohamta]

@shazow Thank you for implementing this so quickly! It looks incredibly useful.

I have one question. In cases where no contract details are retrieved from any ABILoader, will the behavior fall back to the previous logic as expected? I’m not entirely sure, but I was wondering if the logic might need to be adjusted like this:

            if (config.loadContractResult) {
                const contractResult = await loader.getContract(address);
                if (contractResult) {
                    result.contractResult = contractResult;
                    result.abi = contractResult.abi;
                    result.abiLoadedFrom = contractResult.loader;
                    return result;
                }
+            }
-            } else {
                // Load ABIs of all available facets and merge
                const addresses = Object.keys(facets);
                const promises = addresses.map(addr => loader.loadABI(addr));
                const results = await Promise.all(promises);
                const abis = Object.fromEntries(results.map((abi, i) => {
                    return [addresses[i], abi];
                }));
                result.abi = pruneFacets(facets, abis);
                if (result.abi.length > 0) {
                    result.abiLoadedFrom = abiLoadedFrom;
                    return result;
                }
-            }

[shazow]

I actually had the logic you suggested originally, but my understanding is that the same conditions are required for the address to be present in either endpoint (loadABI and getContract). That is, address X must be verified on the respective service in both cases. If it's not present on one endpoint, I think it's safe to assume it won't be on the other too?

Or are there scenarios when that is not true?

[shazow]

Oh, I think I see what you mean: the block below fetches the proxy resolved addresses, whereas the getContract gets the unresolved address.

Hmm, what is the correct behaviour here? Should getContract work with resolved addresses too?

Or maybe it should fall back to the loadABI block iff address doesn't match facets?

[yohamta]

Thank you, and sorry for the confusion! It’s a bit tricky, but I was considering whether an implementation like the one below might be necessary.

In the case of a DiamondProxy, if it’s unlikely that the ABI for the proxy would be unavailable while the ABI for the implementation address is accessible, then I agree that the original implementation should be fine.

            let abi: any[] = [];
            try {
                if (config.loadContractResult) {
                    const contractResult = await loader.getContract(address);
                    result.contractResult = contractResult;
                    result.abi = contractResult.abi;
                    result.abiLoadedFrom = contractResult.loader;
                    abi = contractResult.abi;
                } else {
                    abi = await loader.loadABI(address);
                }
            } catch (error: any) {
                onError("getABI", error);
            }

            // Load ABIs of all available facets and merge
            const addresses = Object.keys(facets).filter((addr) => addr !== address);
            const promises = addresses.map((addr) => loader.loadABI(addr));
            const results = await Promise.all(promises);
            const abis = Object.fromEntries(
                results.map((abi, i) => {
                    return [addresses[i], abi];
                })
            );
            abis[address] = abi;

            result.abi = pruneFacets(facets, abis);
            if (result.abi.length > 0) {
                result.abiLoadedFrom = abiLoadedFrom;
                return result;
            }

After writing it out, I started to feel that adding this complexity might not be worth it or even I was wrong 😅 The original implementation might be the better option. What do you think?

[shazow]

After sleeping on it, I feel like this is the best option for now:

Or maybe it should fall back to the loadABI block iff address doesn't match facets?

This means if whatsabi detected proxies, it will still run its additional internal proxy resolving logic. If there are no proxies, then we can just accept whatever the ABILoader gave us.

Keep in mind, Sourcify does not yet return proxy information in getContract the way Etherscan does. And there's a chance Sourcify will just use WhatsABI to resolve proxies when it does, so it will be equivalent in that case. :)

[shazow]

After trying it, maybe a variant of your version is better! Let me hammer at it a bit more.

[shazow]

@yohamta What do you think of this if there is a single DiamondProxy but followProxies is false?

I'm not too happy with it because it'll rerun getCode again, but not sure if this is an edge case that is important?

[yohamta]

Thank you very much! It looks good to me :) As you mentioned, the redundant execution of getCode doesn’t seem too critical in this case.

One thing I've been thinking about is that, for DiamondProxy, the resolve() method requires a selector argument. This makes it seem a bit challenging to retrieve the implementation addresses for the facets externally. What are your thoughts on this? When followProxy: false is set and facets aren't being tracked, it would be helpful if users could easily get a list of facet addresses. Another potential solution could be to automatically resolve facets for DiamondProxy, even when followProxy: false is used. That said, I’m not sure which approach would be suitable right now.

[shazow]

One thing I've been thinking about is that, for DiamondProxy, the resolve() method requires a selector argument.

Correct, with this fix if you want to get all possible selectors, you'd need to do something like:

const result = whatsabi.autoload(address, { provider, followProxies: false });
const p = result.proxies[0] as DiamondProxyResolver;
const facets = await diamondProxy.facets(provider, address); // All possible address -> selector[] mappings

But just doing followProxies() should automagically do that for you and look them up and return the abi as it used to.

Another potential solution could be to automatically resolve facets for DiamondProxy, even when followProxy: false is used.

This was the original behaviour we're trying to fix, right?

[yohamta]

Thank you for the code snippet, it's very helpful!

But just doing followProxies() should automagically do that for you and look them up and return the abi as it used to.

Yes, that's true. On the other hand, some user might want to retrieve ContractResult for each facet individually. In that case, they can follow the method you've described above, which I think works well.

This was the original behaviour we're trying to fix, right?

Yes, exactly. I’m still unsure of the best way to handle DiamondProxy in this context, which is why I brought it up.

I’m also a bit curious if there could be cases where a deeper hierarchy of proxies exists under the DiamondProxy facets. However, I believe this would be a rare case and shouldn’t prevent this PR from being merged.

[shazow]

Thank you for the code snippet, it's very helpful!

I'll add it to the docs! Thanks for the prompt. :)

On the other hand, some user might want to retrieve ContractResult for each facet individually. In that case, they can follow the method you've described above, which I think works well.

Hmm are individual facet addresses typically verified? We should look at some examples of DiamondProxies in the wild to see how they're treated.

Yes, exactly. I’m still unsure of the best way to handle DiamondProxy in this context, which is why I brought it up.

That's fair. I'd like to do a release today or tomorrow, so I might just merge the ContractResult work and revert the changes to the followProxies for now, and we can think about it more after the release. :)

I’m also a bit curious if there could be cases where a deeper hierarchy of proxies exists under the DiamondProxy facets.

That's a good question! I'm skeptical this is used in practice, but it's certainly possible to do!

Someone should deploy a weird fractal multi-proxy monstrosity just so we have something to test against. 😅

[yohamta]

Sounds great, and thank you very much! Looking forward to the next release.