Skip to content

Add v2 verify routes #73

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

Merged
merged 14 commits into from
Mar 15, 2025
Merged

Conversation

iainnash
Copy link
Contributor

@iainnash iainnash commented Jan 7, 2025

  • Fix existing verify integration tests
  • Add standard-json-input integration tests
  • Fix current single file test
  • Add v2 integration tests
  • Allow v2 setting on client with etherscan client

This is a prerequisite PR for foundry-rs/foundry#9196

Notes:

  • Fantomscan test has no env var passed in and can be removed safely.
  • Gas estimates now do not fail with even 1 passed in, so skipping the failure test because I couldn't find a failure test.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty, smol nits and suggestions

@iainnash
Copy link
Contributor Author

@mattsse updated with suggestions

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry for the delay, last nits

Comment on lines +120 to +125
let api_key = std::env::var("ETHERSCAN_API_KEY")?;
Client::builder()
.with_api_version(EtherscanApiVersion::V2)
.with_api_key(api_key)
.chain(chain)?
.build()
Copy link
Member

Choose a reason for hiding this comment

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

then this can just call new ad with_version(v2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little confused as to where this would be called -- within the builder or skip the builder entirely and use with_version() rather than new_v2_from_env?

@sakulstra
Copy link

sakulstra commented Feb 27, 2025

Hey, not sure if it#s the right place to comment, but i think there's currently a little bug in the etherscan verification that could be considered when migrating to v2.

If there's a similar match, it seems like foundry currently skips verification, but it should not.
I benchmarked etherscan a bit, and seems like the etherscan api is highly irregular (at least on v1).
Some networks(e.g. mainnet & base) will report SimilarMatch as part of the response, but also provide SourceCode.
Some networks(e.g. sonic) will throw an error.
Some networks(e.g. zksync) will just return SourceCode and not indicate anything.

Perhaps foundry should always try to verify and not even bother to check if it's verified or not.

@iainnash
Copy link
Contributor Author

iainnash commented Feb 27, 2025 via email

@0xV4L3NT1N3
Copy link

Tracking this as well, happy to take any questions from Etherscan's end!

@sakulstra noted on the Similar Match case, this new field is being added to all chains and will be standardised

@zerosnacks zerosnacks added the enhancement New feature or request label Mar 7, 2025
@zerosnacks zerosnacks requested a review from mattsse March 7, 2025 15:41
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!

terribly sorry for the delay here...

this lgtm! especially since this is opt-in, we can try experiment with this in foundry and see what we're missing

@mattsse mattsse merged commit ac68cb6 into foundry-rs:main Mar 15, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants