Skip to content

[feat] cast call --trace (& code reuse in cast run)#5477

Merged
Evalir merged 44 commits intofoundry-rs:masterfrom
nhtyy:master
Aug 10, 2023
Merged

[feat] cast call --trace (& code reuse in cast run)#5477
Evalir merged 44 commits intofoundry-rs:masterfrom
nhtyy:master

Conversation

@nhtyy
Copy link
Contributor

@nhtyy nhtyy commented Jul 26, 2023

#5461

Forks remote rpc, executes a call and prints a trace

edit: this PR also touches on cast run just to dedup some code we copy

Motivation

I come up needing something like this quite often, using a forge test on the fly rn with calldata

@nhtyy nhtyy marked this pull request as ready for review July 26, 2023 18:44
@nhtyy
Copy link
Contributor Author

nhtyy commented Jul 26, 2023

hi, so this PR is fully implemented as a extension to cast call, although, im actually not sure if should instead become its own command like cast trace.

i only say this because now, cast call has options like --debug or --verbose that can only be used in trace mode, but they still appear at the top level --help call...

Making --trace a subcommand would alter the current semantics of how create calls are handled so im thinking this maybe should have its own command.

lots of this code is shared with cast run also, if someone can give some guidance on where to store this code so we can dedup all of that

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.

great feature,

I think having it as flag in cast call is the best approach

I wonder if we can unify a lot of stuff with cast run

but perhaps not really possible since cast run behaves slightly differently

wdyt @Evalir

only glanced over the changes, looks promising

I'd like to move the TracingExecutor and the standalone functions to a separate file though

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! some comments & agree with mattsse over moving the tracingexecutor & related funcs

Comment on lines +120 to +171
struct TracingExecutor {
executor: Executor,
}

impl TracingExecutor {
pub async fn new(
config: &foundry_config::Config,
version: Option<EvmVersion>,
rpc_opts: RpcOpts,
debug: bool,
) -> eyre::Result<(Self, EvmOpts)> {
// todo:n find out what this is
let figment =
Config::figment_with_root(find_project_root_path(None).unwrap()).merge(rpc_opts);

let mut evm_opts = figment.extract::<EvmOpts>()?;

evm_opts.fork_url = Some(config.get_rpc_url_or_localhost_http()?.into_owned());
evm_opts.fork_block_number = config.fork_block_number;

// Set up the execution environment
let env = evm_opts.evm_env().await;

let db = Backend::spawn(evm_opts.get_fork(&config, env.clone())).await;

// configures a bare version of the evm executor: no cheatcode inspector is enabled,
// tracing will be enabled only for the targeted transaction
let builder = ExecutorBuilder::default()
.with_config(env)
.with_spec(evm_spec(&version.unwrap_or(config.evm_version)));

let mut executor = builder.build(db);

executor.set_tracing(true).set_debugger(debug);

Ok((Self { executor }, evm_opts))
}
}

impl std::ops::Deref for TracingExecutor {
type Target = Executor;

fn deref(&self) -> &Self::Target {
&self.executor
}
}

impl DerefMut for TracingExecutor {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.executor
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like this. wdyt about moving it to the evm crate? that way we have executors all in one place—we can do the config stuff before building the executor and therefore not depend on the config

Ok(DeployResult { gas_used, traces, debug: run_debug, .. }) => {
TraceResult {
success: true,
traces: vec![(TraceKind::Execution, traces.unwrap_or_default())],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, as traces are enabled by default, is there any situation where we do not want to error if we can't unwrap the traces? thinking it might be better to just bail here if not

}

builder.set_data(data);
// fill it last because we dont need anything from the built one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this comment a tad clearer? can't easily figure out what you mean without context 😄

}
_ => {
builder.value(tx.value);
// fill it first becasue builder parses args / addr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fill it first becasue builder parses args / addr
// fill it first because builder parses args / addr

@nhtyy
Copy link
Contributor Author

nhtyy commented Jul 28, 2023

hi @Evalir i addressed your requests in the more recent commits, as well as moved the TracingExecutor to evm::trace, and moved handle_trace, print_trace, run_debugger to cli::cmd::utils. Just want to confirm those are good spots..

As well all items mentioned can by re used in cast run! Should i do that here or should i make another pr?

@Evalir Evalir requested review from Evalir and mattsse July 28, 2023 02:57
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm bar few doc comments! can't push to your branch as it's protected (master)

i think we can do the other stuff in a followup pr if you're up for it 🙏

@nhtyy
Copy link
Contributor Author

nhtyy commented Jul 31, 2023

@Evalir

just finished this, currently its a pr on my repo, wasnt sure where to point it

nhtyy#1

@Evalir
Copy link
Member

Evalir commented Aug 1, 2023

@nhtyy nice! should we fix the failing tests on this pr and open that one here?

@nhtyy
Copy link
Contributor Author

nhtyy commented Aug 1, 2023

@Evalir hmm sorry im not sure how to open both of them at the same time on this repo

@nhtyy
Copy link
Contributor Author

nhtyy commented Aug 1, 2023

maybe we should just merge them all here? diff shouldnt be to bad, sent you an invite to my repo as well

@Evalir
Copy link
Member

Evalir commented Aug 2, 2023

sure! let's merge them all here then, and update the pr description accordingly

@nhtyy nhtyy changed the title [feat] cast call --trace [feat] cast call --trace (& code reuse in cast run) Aug 2, 2023
@nhtyy
Copy link
Contributor Author

nhtyy commented Aug 7, 2023

re-requesting review since there was some merge conflicts blocking

@nhtyy nhtyy requested a review from Evalir August 7, 2023 20:22
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thank you! just needs fmt, but can't push to your branch since this is on master. let's fix ci then we can send it

@meetmangukiya
Copy link
Contributor

was missing this feature a lot! great stuff 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants