-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(evm): compile etherscan sources and use them on the debugger #1413
Conversation
I think there was talk of source maps being added to the Etherscan API (it's already present in the UI) - is this something we can get confirmed somewhere, and if so, are we willing to wait for that considering that this is sort of a workaround for that? |
it's not yet included in the |
Yeah, I meant that there was talks that it would be included soon-ish. I just don't know when, but if we can get a timeline and its not too far out we should maybe hold off and wait for that? |
It will still take some time for etherscan to add it from my understanding. And when it does arrive, some of these changes will need to happen anyway.(eg. passing sourcemaps per external contracts to the debugger UI). But even then, I feel like it could be added because:
|
Alright, I was just saying that if it would be added soon there probably wouldn't be much of a point adding something we are going to delete, but if you feel strongly that it will take a long time/be incomplete then you're free to go ahead :) |
decoder: CallTraceDecoder, | ||
known_contracts: BTreeMap<ArtifactId, ContractBytecodeSome>, |
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 should probably merge this at some earlier point instead of passing both of these to the function since they serve the same purpose
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.
Wanted to clarify this real quick, did you mean merge this contract
map into one object or merge it with the sources
map?
result: RunResult, | ||
decoder: CallTraceDecoder, | ||
known_contracts: BTreeMap<ArtifactId, ContractBytecodeSome>, | ||
sources: BTreeMap<ArtifactId, 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.
Same here
&self, | ||
) -> eyre::Result<(BTreeMap<ArtifactId, String>, BTreeMap<ArtifactId, ContractBytecodeSome>)> | ||
{ | ||
if self.compiles { |
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.
Any reason you opted for a bool here instead of just ommitting the call to get_compiled_contracts_with_sources
when we don't want to compile?
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.
older code, part of the planned clean-up
for (addr, _) in addresses { | ||
fetcher.push(*addr); | ||
} |
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 can probably filter out addresses here if it is already present in self.contracts
@@ -1011,6 +1018,67 @@ pub fn init_tracing_subscriber() { | |||
.ok(); | |||
} | |||
|
|||
pub async fn compile_contract_source( |
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.
Maybe this should be in the CLI crate instead since we already have a compile module there? 🤔
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.
@onbjerg thinking about this a bit. Problem with having this in the cli
crate is that we introduce a cyclical dependency between the evm
crate and the cli
crate (evm references itself)
One level higher, maybe the cli crate isn't the right place for the compile module at all, it could be in common
instead?
wyt?
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 OK to pull it all in common
self.contracts | ||
.insert(label.clone(), (source_code, optimization, runs, version)); |
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 we should do this keyed by address instead - if we have multiple addresses with the same label (which IIRC is just the contract name for this identifier) then we might overwrite info for two contracts with the same name, but with different implementations. E.g. if you interact with two tokens and both of them show up as "Token", then only one of them would be decoded... I think
e4cdc0a
to
8112635
Compare
@joshieDo are we interested in picking this up again? |
IMO yes, this would be a really great feature. Having a module for etherscan compilation would also be useful for things like #2494 (comment) |
i'd love to have this feature, but with all the refactors/conflicts (also the incoming TUI one), it's probably better to start from scratch. Maybe use this one as a potential reference. |
Agree that this should be started from scratch. Will see if I can find someone to take it over the finish line using this as a reference. |
Sorry for the delay on this team, I'll have a new draft PR building on the work that JoshieDo has done in the next couple days. |
Thanks boss |
superseded by #3006 |
Motivation
WIP
Debugging with sources coming from etherscan is nice.
Solution
Missing:
It's working now with some limitations:
--via-ir
isn't provided by etherscan (i think...).Working example:
If there are many contracts, be ready to wait a while...