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

feat: identify addresses from etherscan when forking #1190

Merged
merged 15 commits into from
Apr 7, 2022

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented Apr 4, 2022

Blocked by: gakonst/ethers-rs#1108

To do

  • Ensure the cache path exists
  • Add some tracing in the code for better visibiltiy when debugging
  • Pull chain ID from provider and Etherscan key from config
  • Implement rate limiting
  • Filter out addresses we have already identified to save on requests

Closes #1015

@onbjerg onbjerg added the T-feature Type: feature label Apr 4, 2022
@onbjerg
Copy link
Member Author

onbjerg commented Apr 4, 2022

Some notes:

  • Some contracts may show up with the same name, especially Vyper contracts. Etherscan does not expose an API to get public labels
  • The rate limit algo is pretty naive but it should work as long as there are not a lot of unidentified addresses
  • It seems that some local contracts are no longer identified.. I think it's because of sparse mode, but I'm not sure yet. Waiting for feat(config): make sparse mode opt in #1191 to be merged to check

@mds1
Copy link
Collaborator

mds1 commented Apr 4, 2022

Some contracts may show up with the same name, especially Vyper contracts. Etherscan does not expose an API to get public labels

Could we append an index to help differentiate contracts in cases like these? For example if the contract name is Curve and there's two of them, the trace would label them Curve-1 and Curve-2. Or some other differentiator like Curve{1} also works of course.

@onbjerg onbjerg force-pushed the onbjerg/etherscan-identifier branch from 03cf500 to 3e6685a Compare April 4, 2022 20:18
cli/src/cmd/forge/test.rs Outdated Show resolved Hide resolved
evm/src/trace/identifier/etherscan.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg marked this pull request as ready for review April 4, 2022 20:21
@onbjerg onbjerg force-pushed the onbjerg/etherscan-identifier branch from f3c38d3 to 4d7f9a7 Compare April 6, 2022 06:22
@onbjerg
Copy link
Member Author

onbjerg commented Apr 6, 2022

Should be good to merge after gakonst/ethers-rs#1114 and ethers bump!

@onbjerg
Copy link
Member Author

onbjerg commented Apr 6, 2022

Not entirely sure why the integration tests are failing.. Is the filter directive something with the tracing crate?

@onbjerg onbjerg force-pushed the onbjerg/etherscan-identifier branch from 6410e1c to 4e44b3c Compare April 7, 2022 06:21
config/src/lib.rs Show resolved Hide resolved
evm/src/trace/identifier/etherscan.rs Outdated Show resolved Hide resolved
evm/src/trace/mod.rs Outdated Show resolved Hide resolved
evm/src/trace/identifier/etherscan.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg force-pushed the onbjerg/etherscan-identifier branch from c7eff81 to ae82442 Compare April 7, 2022 12:57
@onbjerg onbjerg requested a review from mattsse April 7, 2022 12:57
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.

smol nits

evm/src/trace/identifier/etherscan.rs Show resolved Hide resolved
evm/src/trace/identifier/etherscan.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg merged commit 8494ef4 into master Apr 7, 2022
@onbjerg onbjerg deleted the onbjerg/etherscan-identifier branch April 7, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Etherscan trace decoder
4 participants