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

Fix proxy detection for EIP1967 when storage slot is only referenced in creation code #1957

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

manuelwedler
Copy link
Contributor

Fixes #1928

  • Updates whatsabi to 0.20.0 which is the version that supports this kind of proxy
  • Uses creation bytecode for detection if available in v2 lookup

…in creation code

- Updates whatsabi to 0.20.0 which is the version that supports this kind of proxy
- Uses creation bytecode for detection if available in v2 lookup
Copy link
Member

@kuzdogan kuzdogan left a comment

Choose a reason for hiding this comment

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

Looks ok but couple comments and questions about the code in general:

  1. I notice this comment, do we have an issue for this?
  // TODO:
  // The proxy detection will be integrated into the verification process,
  // and the proxy detection result will be stored in the database.
  // When we have this, we will only need to resolve the implementation address here,
  // and don't need to query the onchain runtime bytecode.

and this

// TODO:
// onchain bytecodes are a temporary solution for running the proxy detection inside the getContractEndpoint handler.
// Remove onchainRuntimeBytecode and onchainCreationBytecode when proxy detection result is stored in database.
  1. Are we making sure the current StorageService is a DB and not the legacy filesystems? We use
  const contract = await services.storage.performServiceOperation(
    "getContract",
    [req.params.chainId, req.params.address, fields, omit],
  );

What happens if it runs a RepoService.getContract function? Ie. is there a way we make sure APIv2 endpoints are only used by the DB storages services?

@@ -110,7 +110,8 @@ export async function getContractEndpoint(
const sourcifyChain =
chainRepository.supportedChainMap[req.params.chainId];
const proxyDetectionResult = await detectAndResolveProxy(
contract.proxyResolution!.onchainRuntimeBytecode!,
contract.proxyResolution!.onchainCreationBytecode ??
Copy link
Member

Choose a reason for hiding this comment

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

Can the onChainBytecode be empty string ""? If so it's better to use || instead of ??. ?? checks only for null and undefined, || checks all falsey values.

@@ -110,7 +110,8 @@ export async function getContractEndpoint(
const sourcifyChain =
chainRepository.supportedChainMap[req.params.chainId];
const proxyDetectionResult = await detectAndResolveProxy(
Copy link
Member

Choose a reason for hiding this comment

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

Looking back at this again, I don't understand why contract.proxyResolution has onchainBytecode fields. Why do we have them in that object? Why don't we use contract.onchainRuntimeBytecode etc. directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Sprint - Needs Review
Development

Successfully merging this pull request may close these issues.

Failing proxy resolution for ERC1967Proxy
2 participants