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.

Sign up for GitHub

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

Jump to bottom

autoload: AutoloadResult.ContractResult #131

Merged
merged 4 commits into from
Oct 1, 2024
Merged

autoload: AutoloadResult.ContractResult #131

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
902ab40
autoload: AutoloadResult{ContractResult} and AutoloadConfig{loadContr…
shazow Sep 29, 2024
bb9f2c9
auto: Dont follow DiamondProxies unless followProxies is set
shazow Sep 30, 2024
3dc8f52
src/proxies.ts: Add typedoc example for DiamondProxyResolver.facets
shazow Oct 1, 2024
fe8fa90
src/auto.ts: Undo respecting followProxies for DiamondProxy
shazow Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/__tests__/auto.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,27 @@ online_test('autoload non-contract', async ({ provider, env }) => {
expect(abi).toStrictEqual([]);
});

online_test('autoload verified etherscan', async ({ provider, env }) => {
online_test('autoload verified multi', async ({ provider, env }) => {
const address = "0x8f8ef111b67c04eb1641f5ff19ee54cda062f163"; // Uniswap v3 pool, verified on Etherscan and Sourcify
const result = await autoload(address, {
provider: provider,
...whatsabi.loaders.defaultsWithEnv(env),
});
expect(result.abiLoadedFrom?.name).toBeTruthy()
expect(result.abiLoadedFrom?.name).toBeTruthy();
});

online_test('autoload loadContractResult verified etherscan', async ({ provider, env }) => {
const address = "0xc3d688b66703497daa19211eedff47f25384cdc3"; // Compound USDC proxy
const result = await autoload(address, {
provider: provider,
loadContractResult: true,
followProxies: false,
abiLoader: new whatsabi.loaders.EtherscanABILoader({ apiKey: env.ETHERSCAN_API_KEY }),
});
expect(result.abiLoadedFrom?.name).toBe("EtherscanABILoader");
expect(result.contractResult?.ok).toBeTruthy();
expect(result.contractResult?.name).toBe("TransparentUpgradeableProxy");
expect(result.contractResult?.compilerVersion).toBe("v0.8.15+commit.e14f2714");
expect(result.contractResult?.loaderResult?.Proxy).toBe("1");
expect(result.contractResult?.loaderResult?.Implementation).toBe("0x8a807d39f1d642dd8c12fe2e249fe97847f01ba0");
});
74 changes: 58 additions & 16 deletions src/auto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Fragment, FunctionFragment } from "ethers";
import type { AnyProvider } from "./providers.js";
import type { ABI, ABIFunction } from "./abi.js";
import { type ProxyResolver, DiamondProxyResolver } from "./proxies.js";
import type { ABILoader, SignatureLookup } from "./loaders.js";
import type { ABILoader, SignatureLookup, ContractResult } from "./loaders.js";
import * as errors from "./errors.js";

import { CompatibleProvider } from "./providers.js";
Expand All @@ -28,6 +28,9 @@ export type AutoloadResult = {
/** Whether the `abi` is loaded from a verified source */
abiLoadedFrom?: ABILoader;

/** Full contract metadata result, only included if {@link AutoloadConfig.loadContractResult} is true. */
contractResult?: ContractResult;

/** List of resolveable proxies detected in the contract */
proxies: ProxyResolver[],

Expand Down Expand Up @@ -82,6 +85,15 @@ export type AutoloadConfig = {
*/
followProxies?: boolean;


/**
* Load full contract metadata result, include it in {@link AutoloadResult.ContractResult} if successful.
*
* This changes the behaviour of autoload to use {@link ABILoader.getContract} instead of {@link ABILoader.loadABI},
* which returns a larger superset result including all of the available verified contract metadata.
*/
loadContractResult?: boolean;

/**
* Enable pulling additional metadata from WhatsABI's static analysis, still unreliable
*
Expand Down Expand Up @@ -149,7 +161,7 @@ export async function autoload(address: string, config: AutoloadConfig): Promise
try {
bytecode = await provider.getCode(address);
} catch (err) {
throw new errors.AutoloadError(`Failed to fetch contract code due to provider error: ${err instanceof Error ? err.message : String(err) }`,
throw new errors.AutoloadError(`Failed to fetch contract code due to provider error: ${err instanceof Error ? err.message : String(err)}`,
{
context: { address },
cause: err as Error,
Expand All @@ -170,13 +182,30 @@ export async function autoload(address: string, config: AutoloadConfig): Promise
};

if (result.proxies.length === 1 && result.proxies[0] instanceof DiamondProxyResolver) {
onProgress("loadDiamondFacets", { address });
const diamondProxy = result.proxies[0] as DiamondProxyResolver;
const f = await diamondProxy.facets(provider, address);
Object.assign(facets, f);
if (config.followProxies) {
onProgress("loadDiamondFacets", { address });
const f = await diamondProxy.facets(provider, address);
Object.assign(facets, f);
} else {
result.followProxies = async function(selector?: string): Promise<AutoloadResult> {
if (selector) {
// Follow a single selector for DiamondProxy
onProgress("followProxies", { resolver: diamondProxy, address });
const resolved = await diamondProxy.resolve(provider, address, selector);
if (resolved !== undefined) return await autoload(resolved, config);
}
// Resolve all facets, unfortunately this requires doing the whole thing again with followProxy
// FIXME: Can we improve this codeflow easily?
// We could override the privider with a cached one here, but might be too magical and cause surprising bugs?
return await autoload(address, Object.assign({}, config, { followProxies: true }));
Copy link
Owner Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

@yohamta yohamta Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@yohamta yohamta Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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. 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

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

};
}

} else if (result.proxies.length > 0) {
result.followProxies = async function(selector?: string): Promise<AutoloadResult> {
// This attempts to follow the first proxy that resolves successfully.
// FIXME: If there are multiple proxies, should we attempt to merge them somehow?
for (const resolver of result.proxies) {
onProgress("followProxies", { resolver: resolver, address });
const resolved = await resolver.resolve(provider, address, selector);
Expand All @@ -196,7 +225,7 @@ export async function autoload(address: string, config: AutoloadConfig): Promise
onProgress("abiLoader", { address, facets: Object.keys(facets) });
const loader = abiLoader;

let abiLoadedFrom;
let abiLoadedFrom = loader;
let originalOnLoad;
if (loader instanceof MultiABILoader) {
// This is a workaround for avoiding to change the loadABI signature, we can remove it if we use getContract instead.
Expand All @@ -213,16 +242,29 @@ export async function autoload(address: string, config: AutoloadConfig): Promise
}

try {
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;
if (config.loadContractResult) {
const contractResult = await loader.getContract(address);
if (contractResult) {
// We assume that a verified contract ABI contains all of the relevant resolved proxy functions
// so we don't need to mess with resolving facets and can return immediately.
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;
}
}
} catch (error: any) {
// TODO: Catch useful errors
Expand Down
Loading