-
Notifications
You must be signed in to change notification settings - Fork 2k
Adding support to etherscan-v2 #10298
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
base: master
Are you sure you want to change the base?
Conversation
@iainnash there's a new 0.13.1 version of block-explorers released, with fix for foundry-rs/block-explorers#82 Thank you! |
[profile.default] | ||
|
||
[etherscan] | ||
mumbai = { key = "dummykey", chain = 80001, url = "https://api-testnet.polygonscan.com/" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think would be nice to have an optional version here in EtherscanOpts
too, defaults to v1. @zerosnacks wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I can add that.
Testing this it’s going to be nice to use one API key against all chains.
I made a tool called chains at zora which manages api keys and rpc urls globally. May be less useful now https://github.com/ourzora/chains-cli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll get in touch with Etherscan to see whether switching to V2 by default and deprecating V1 support altogether makes sense. From the documentation it appears that V2 is available on all chains I would expect them to be and all the relevant RPC methods are available. If not I would agree that using an optional version field here defaulting to V1 would be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the transition should be gradual afaik the API key they accept is only the mainnet key and it will fail with other keys which need to hit the specific instances instead.
I can reach out and confirm with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd recommend teams to switch to V2 entirely, all other API methods are available as well for existing code to check if a contract is verified and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for using v2, but just to note sth that we noticed on migration from v1 to v2 on our internal services:
Etherscan has the same api limits on v2 vs v1. For us this caused some breakage as on v1 the limit is per chain and on v2 (given it's only one key) it is for all combined. Not sure if this can cause problems on multichain scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good concern @sakulstra, it is something we're thinking about and monitoring as users adopt V2
for now we observe most users fare well with a few chains and retry/backoff mechanisms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iainnash given the v1 will be eol at end of May, we should go for v2. Would you be able to wrap up this PR, becomes quite a priority now. thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, updated the underlying package and fixed parsing to support the toml config.
There seems to be an avalanche test failing after changing the default so looking into that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! going to commit a change to point to required block explorers rev so CI to show exactly status of failed tests and continue from there
Hi yes!
I’m working on parsing the configs. Will push EOD once I finish some
seralization.
…On Tue, Apr 29, 2025 at 08:33 grandizzy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In crates/verify/src/etherscan/mod.rs
<#10298 (comment)>:
> @@ -539,6 +550,86 @@ mod tests {
assert!(format!("{client:?}").contains("dummykey"));
}
+ #[test]
+ fn can_extract_etherscan_v2_verify_config() {
+ let temp = tempdir().unwrap();
+ let root = temp.path();
+
+ let config = r#"
+ [profile.default]
+
+ [etherscan]
+ mumbai = { key = "dummykey", chain = 80001, url = "https://api-testnet.polygonscan.com/" }
@iainnash <https://github.com/iainnash> given the v1 will be eol at end
of May, we should go for v2. Would you be able to wrap up this PR, becomes
quite a priority now. thank you!
—
Reply to this email directly, view it on GitHub
<#10298 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGMCOHJKLVILY55TQIUFML235WN7AVCNFSM6AAAAAB3B3VGD6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMBTGE2DGOBVHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This needs another update to block-explorers :/ The update setting V2 to the default is somewhat large as it requires more functions available on the enum for parsing along with changing the default api version. |
6f81bb9
to
395712a
Compare
@@ -324,6 +324,7 @@ jiff = "0.2" | |||
idna_adapter = "=1.1.0" | |||
|
|||
[patch.crates-io] | |||
foundry-block-explorers = { git = "https://github.com/foundry-rs/block-explorers", rev = "8c03122" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this package is released, the tag should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tysm for this,
some nits and qs
/// Etherscan API Version. | ||
#[serde(default, skip_serializing_if = "Option::is_none")] | ||
pub api_version: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also be just etherscanapiversion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn’t too sure what the pattern was for the __Resolved struct if it was recommended to be a string since the other fields were and it’d be parsed later. If this is the case I can switch it :). Went back and forth.
@iainnash wonder if we really need the extra
For custom verifiers, use
wdyt, thanks! |
Motivation
Fixes #9196
For tests to pass and to merge this pr, an update to
block-explorers
is first required: foundry-rs/block-explorers#82.Solution
Add case statements and proper flags for
ev2
andetherscan-v2
for verifying with etherscan v2 API which sets the /mode for the backend contracts to use different URL structures and logic for interacting with the etherscan API.Added one test, need to add more or update existing tests to handle new logic.
PR Checklist