feat(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events #7359

byteshijinn · 2024-03-10T14:52:00Z

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.

mattsse

supportive, though we can solve this a bit differently

lmk if anything's unclear

mattsse · 2024-03-12T18:22:11Z

crates/cast/bin/cmd/run.rs

+
+    #[arg(long, short, alias = "gs")]
+    pub generate_local_signatures: bool,


This needs docs+explainer

mattsse · 2024-03-12T18:30:41Z

crates/cast/bin/cmd/run.rs

+                let _ = fs::write_json_file(&path, &cached_signatures);
+            }
+        }
+
        handle_traces(result, &config, chain, self.label, self.debug).await?;


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,

)?)

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.

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.

mattsse · 2024-03-12T18:30:49Z

crates/cast/bin/cmd/run.rs

+use std::collections::BTreeMap;
+use serde::{Deserialize, Serialize};
+
+#[derive(Debug, Default, Serialize, Deserialize)]
+struct CachedSignatures {
+    events: BTreeMap<String, String>,
+    functions: BTreeMap<String, String>,
+}


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()

};

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.

byteshijinn · 2024-03-17T14:47:54Z

I updated my code. please check again @mattsse

mattsse

sorry this one slipped through,
some nits.

generally supportive, wdyt @DaniPopes @klkvr

mattsse · 2024-07-01T12:31:23Z

crates/cli/src/utils/cmd.rs

@@ -414,3 +414,25 @@ pub async fn print_traces(result: &mut TraceResult, decoder: &CallTraceDecoder)
    println!("Gas used: {}", result.gas_used);
    Ok(())
 }
+
+pub fn generate_local_signatures(output: &ProjectCompileOutput, cache_path: PathBuf) -> Result<()> {


this needs docs and should be renamed to cache_local_signatures

ok i'll fix it later

mattsse · 2024-07-01T12:32:38Z

crates/cast/bin/cmd/run.rs

@@ -220,6 +227,19 @@ impl RunArgs {
            }
        };

+        if self.generate_local_signatures {
+            let project = config.project()?;


this should print that we're compiling to get the signatures

mattsse · 2024-07-01T12:33:45Z

crates/cast/bin/cmd/run.rs

@@ -220,6 +227,19 @@ impl RunArgs {
            }
        };

+        if self.generate_local_signatures {
+            let project = config.project()?;
+            let compiler = ProjectCompiler::new().quiet(true);


we don't need to actually compile here, right?
@klkvr is there an easier way to just get the artifacts?

we need to compile as it might be run on a clean project. though I think we can only request abi here, should speed things up

if there are cached artifacts then compilation would just use them, so that's fine I believe

Usually users are more likely to trace their own project files and compile the whole project before deploying. But some one may dont want to run two command to generate signature cache with new file updates (may be an upgradable contract should try different events). If file not updated, compile will be complete fast.

klkvr · 2024-07-01T13:12:25Z

crates/cli/src/utils/cmd.rs

@@ -414,3 +414,25 @@ pub async fn print_traces(result: &mut TraceResult, decoder: &CallTraceDecoder)
    println!("Gas used: {}", result.gas_used);
    Ok(())
 }
+
+pub fn generate_local_signatures(output: &ProjectCompileOutput, cache_path: PathBuf) -> Result<()> {
+    let mut cached_signatures = CachedSignatures::default();


shouldn't we firstly read the contents of the file here? looks like this always overwrites existing cache

I believe that generating function signatures for a single project.sol file during most project debugging is sufficient, so I choose to overwrite the existing file. If you want to merge the original signature cache, you also need to consider the handling logic for 4-byte signature conflicts. Such as same 4-byte signature with different arguments

I don't think it's correct to override existing cache anyway. That way running cast run ... --generate-local-signatures twice would result in a bunch of duplicated requests to fetch signatures for unknown contracts

Also wondering if we should instead inject local abis into CallTraceDecoder directly? This would allow us to also decode output values. SignatureIdentifier is mostly suited for decoding only selectors of external contracts

@klkvr Thank you for your guidance. While implementing this feature, I was focused on getting it ready for trace call as soon as possible and didn't look closely at the SignatureIdentifier part. I think I need some time to finish modifying the code, test it with examples of 4-byte signature conflicts, and then resubmit.

I kept the cached local signatures and merged the new signatures with them. However, I need to point out the issue you mentioned earlier with the SignatureIdentifier. If a function's 4-byte signature matches one in the local signatures, the SignatureIdentifier will not automatically attempt to fetch a matching function from external sources, even if the parameters do not match.

signature-test.zip
pic1:

pic2:

with test sol files, deploy and call run, then cast run trace will output like the pic1.

vm.startBroadcast(pkey); Foobar foo1 = new Foobar(); Foobar2 foo2 = new Foobar2(); Foobar3 foo3 = new Foobar3(foo1, foo2); foo3.run(); vm.stopBroadcast();

and then I remove "0x6a627842":"watch_tg_invmru_89cb189(uint256,address)" in the cache file, then cast run trace will output like the pic2. anyway that is an another question.

mattsse · 2024-07-09T14:48:52Z

how to proceed here @klkvr ?

klkvr · 2024-07-09T15:27:51Z

crates/cli/src/utils/cmd.rs

+                | NamedChain::ArbitrumTestnet
+                | NamedChain::ArbitrumGoerli
+                | NamedChain::ArbitrumSepolia
+                | NamedChain::Moonbeam


please remove all the formatting changes

sorry for my mistakes, I will fix it

klkvr · 2024-07-09T15:33:15Z

crates/cli/src/utils/cmd.rs

+        let cache_contents = std::fs::read_to_string(path.clone()).unwrap();
+        match serde_json::from_str::<CachedSignatures>(&cache_contents) {
+            Ok(existed_signatures) => cached_signatures = existed_signatures,
+            Err(e) => {
+                println!("parse cached local signatures file error: {}", e);
+            }
+        }
+        dbg!(&cached_signatures);


this is now duplicated with SignatureIdentifier::new, can we move this logic to CachedSignatures::load(path)

klkvr · 2024-07-09T15:44:29Z

So this can be broken into 2 separate usecases

Have a way to flush selectors from local project contracts into global signature cache (forge build --cache_local_signatures). Current impl lgtm, however, wondering if it makes more sense to be forge selectors cache instead?
Use local ABIs when decoding call trace from cast run. For this usecase I'd prefer a different implementation which would obtain Project, compile it, construct known_contracts and include them in both CallTraceDecoder and TraceIdentifiers similarly to how it's done in forge test:

foundry/crates/forge/bin/cmd/test/mod.rs

Line 402 in b1b8151

let mut identifier = TraceIdentifiers::new().with_local(&known_contracts);

The motivation is that if we have an entire Project, then we can not only use artifacts for identification of selectors, but also to resolve return values of functions, and identify contracts by their bytecodes. ref tracing: Improve decoding of functions output #6531 which had similar motivation

…racts functions and events

merge project signatures with exists cached local signatures instead of just override them

byteshijinn · 2024-07-12T14:30:58Z

So this can be broken into 2 separate usecases

Have a way to flush selectors from local project contracts into global signature cache (forge build --cache_local_signatures). Current impl lgtm, however, wondering if it makes more sense to be forge selectors cache instead?

Use local ABIs when decoding call trace from cast run. For this usecase I'd prefer a different implementation which would obtain Project, compile it, construct known_contracts and include them in both CallTraceDecoder and TraceIdentifiers similarly to how it's done in forge test:

foundry/crates/forge/bin/cmd/test/mod.rs

Line 402 in b1b8151

let mut identifier = TraceIdentifiers::new().with_local(&known_contracts);

The motivation is that if we have an entire Project, then we can not only use artifacts for identification of selectors, but also to resolve return values of functions, and identify contracts by their bytecodes. ref tracing: Improve decoding of functions output #6531 which had similar motivation

For case 1, I believe having helper options in forge build and cast run would make this feature more noticeable. I think adding a command to clear the cache would be useful because SignatureIdentifier merges matched signatures into the cache. My previous tests showed that if a local signature is matched, even if the parameters do not match, it still displays the matched function name.

For case 2, I have considered the solution you mentioned. However, the changes involved are quite extensive and a bit difficult for me, which is why I chose the current method. It allows me to get it working quickly.

byteshijinn requested review from DaniPopes and mattsse as code owners March 10, 2024 14:52

mattsse requested changes Mar 12, 2024

View reviewed changes

byteshijinn force-pushed the master branch 2 times, most recently from 2a08d1c to 4f0ee53 Compare March 17, 2024 14:18

byteshijinn requested a review from mattsse March 23, 2024 03:35

byteshijinn changed the title ~~add RunArgs generate_local_signatures to enable trace with local contracts functions and events~~ feat(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events Apr 20, 2024

byteshijinn requested a review from Evalir as a code owner June 30, 2024 12:00

byteshijinn force-pushed the master branch from 17a6f74 to 759a009 Compare June 30, 2024 13:19

mattsse requested changes Jul 1, 2024

View reviewed changes

klkvr reviewed Jul 1, 2024

View reviewed changes

byteshijinn force-pushed the master branch from 759a009 to 273a62e Compare July 6, 2024 08:57

mattsse requested a review from klkvr July 9, 2024 14:43

klkvr reviewed Jul 9, 2024

View reviewed changes

byteshijinn force-pushed the master branch 2 times, most recently from 825579e to c575b2d Compare July 10, 2024 16:59

add RunArgs generate_local_signatures to enable trace with local cont…

19f0b13

…racts functions and events

byteshijinn force-pushed the master branch from 584af8d to c575b2d Compare July 12, 2024 14:10

byteshijinn added 3 commits July 12, 2024 22:12

make generate_local_signatures as a helper function

093ea83

rename generate_local_signatures to cache_local_signatures

82e7c67

merge project signatures with exists cached local signatures instead of just override them

extract duplicate method for CachedSignatures

fa0d4de

byteshijinn force-pushed the master branch from c575b2d to fa0d4de Compare July 12, 2024 14:13

byteshijinn added 3 commits July 12, 2024 23:37

Merge branch 'master' into master

e00d358

Merge branch 'master' into master

81f7c5f

Merge branch 'master' into master

d34f2a3

byteshijinn added 2 commits July 15, 2024 21:45

Merge branch 'master' into master

a2db0f8

Merge branch 'master' into master

92e298c

zerosnacks added this to the v1.0.0 milestone Jul 31, 2024

zerosnacks added C-cast Command: cast C-forge Command: forge A-tracing Area: tracing Cmd-forge-build Command: forge build labels Jul 31, 2024

Merge branch 'master' into local_signature

fe6bace

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events #7359

feat(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events #7359

byteshijinn commented Mar 10, 2024

mattsse left a comment

mattsse Mar 12, 2024

mattsse Mar 12, 2024

byteshijinn Mar 13, 2024

byteshijinn Mar 13, 2024

mattsse Mar 12, 2024

byteshijinn Mar 13, 2024

byteshijinn commented Mar 17, 2024

mattsse left a comment

mattsse Jul 1, 2024

byteshijinn Jul 3, 2024

mattsse Jul 1, 2024

mattsse Jul 1, 2024

klkvr Jul 1, 2024 •

edited

Loading

byteshijinn Jul 3, 2024 •

edited

Loading

klkvr Jul 1, 2024

byteshijinn Jul 3, 2024 •

edited

Loading

klkvr Jul 3, 2024 •

edited

Loading

byteshijinn Jul 3, 2024

byteshijinn Jul 6, 2024

byteshijinn Jul 6, 2024

mattsse commented Jul 9, 2024

klkvr Jul 9, 2024 •

edited

Loading

byteshijinn Jul 10, 2024

klkvr Jul 9, 2024

klkvr commented Jul 9, 2024 •

edited

Loading

byteshijinn commented Jul 12, 2024


		#[arg(long, short, alias = "gs")]
		pub generate_local_signatures: bool,

	.with_signature_identifier(SignaturesIdentifier::new(
	Config::foundry_cache_dir(),
	config.offline,
	)?)

	#[derive(Debug, Default, Serialize, Deserialize)]
	struct CachedSignatures {
	events: BTreeMap<String, String>,
	functions: BTreeMap<String, String>,
	}

	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()
	};

feat(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events #7359

Are you sure you want to change the base?

feat(forge, cast): add RunArgs generate_local_signatures to enable trace with local contracts functions and events #7359

Conversation

byteshijinn commented Mar 10, 2024

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byteshijinn commented Mar 17, 2024

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

byteshijinn Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byteshijinn Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

klkvr Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse commented Jul 9, 2024

klkvr Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr commented Jul 9, 2024 • edited Loading

byteshijinn commented Jul 12, 2024

klkvr Jul 1, 2024 •

edited

Loading

byteshijinn Jul 3, 2024 •

edited

Loading

byteshijinn Jul 3, 2024 •

edited

Loading

klkvr Jul 3, 2024 •

edited

Loading

klkvr Jul 9, 2024 •

edited

Loading

klkvr commented Jul 9, 2024 •

edited

Loading