-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events #7359
base: master
Are you sure you want to change the base?
Conversation
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.
supportive, though we can solve this a bit differently
lmk if anything's unclear
crates/cast/bin/cmd/run.rs
Outdated
|
||
#[arg(long, short, alias = "gs")] | ||
pub generate_local_signatures: bool, |
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.
This needs docs+explainer
let _ = fs::write_json_file(&path, &cached_signatures); | ||
} | ||
} | ||
|
||
handle_traces(result, &config, chain, self.label, self.debug).await?; |
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.
perhaps a better solution than doing the preprocess step would be to provide the local signatures here:
so they can be merged with this here
foundry/crates/cli/src/utils/cmd.rs
Lines 393 to 396 in eef87de
.with_signature_identifier(SignaturesIdentifier::new( | |
Config::foundry_cache_dir(), | |
config.offline, | |
)?) |
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 implemented it in run.rs to make it optional, and handle_traces as a public method I wanted to change it as little as possible until I understood all others crates package if refered to it.
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.
In fact, it makes more sense for this function to be executed at forge compile time, and it apears in cast run as a shortcut entry to avoid typing the command twice.
crates/cast/bin/cmd/run.rs
Outdated
use std::collections::BTreeMap; | ||
use serde::{Deserialize, Serialize}; | ||
|
||
#[derive(Debug, Default, Serialize, Deserialize)] | ||
struct CachedSignatures { | ||
events: BTreeMap<String, String>, | ||
functions: BTreeMap<String, 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.
we already have this type here
foundry/crates/evm/traces/src/identifier/signatures.rs
Lines 14 to 18 in edb3a4b
#[derive(Debug, Default, Serialize, Deserialize)] | |
struct CachedSignatures { | |
events: BTreeMap<String, String>, | |
functions: BTreeMap<String, String>, | |
} |
we should prevent duplicating this and instead find a way to reuse the existing type
we can make the type pub instead and also make this a helper function:
foundry/crates/evm/traces/src/identifier/signatures.rs
Lines 44 to 56 in eef87de
let identifier = if let Some(cache_path) = cache_path { | |
let path = cache_path.join("signatures"); | |
trace!(target: "evm::traces", ?path, "reading signature cache"); | |
let cached = if path.is_file() { | |
fs::read_json_file(&path) | |
.map_err(|err| warn!(target: "evm::traces", ?path, ?err, "failed to read cache file")) | |
.unwrap_or_default() | |
} else { | |
if let Err(err) = std::fs::create_dir_all(cache_path) { | |
warn!(target: "evm::traces", "could not create signatures cache dir: {:?}", err); | |
} | |
CachedSignatures::default() | |
}; |
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 defined it again because I find it does not exported. For maintenance reasons, I think you're right. At first I was just using it for myself so I didn't think about it that much, I'll spend some time refactoring the code next.
I updated my code. please check again @mattsse |
Motivation
Supply a fast way to add all project contracts functions and events to signatures makes them decode correctly.
Solution
This PR introduces such a possibility using a cli argument --generate-local-signatures for cast run.