script refactoring

[klkvr]

Motivation

The idea is to refactor script to be more like a state-machine, so its easier to navigate/update and we are able to debug each state transition separately.

Solution

I've started with build module, it now returns and operates on BuildData and LinkedBuildData structs which contain all build artifacts and solc-related data and allow us to use them through script execution instead of passing Linker everywhere.

Closes #7244
Closes #4476

[mattsse]

I like this approach a lot.

structuring this in compile+link steps makes a lot of sense

[klkvr]

Refactoring goals were mostly increasing readability and safety of the code.

Readability right now should be better because the logic overall became more flat. If earlier we had handle_broadcastable_transactions going up to 4-5 functions deep, it's now split across several states which are invoked sequentially and don't go deeper than 2 small functions.

Safety isn't great because we have a lot of Options which are assumed to be Some after some point. It is not really easy to know where this point is and it does not help both reading existing code and extending it. I've tried to reduce such practicies as well. Example of those is ScriptConfig:

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

Lines 619 to 625 in c24933d

	pub target_contract: Option<ArtifactId>,
	/// Function called by the script
	pub called_function: Option<Function>,
	/// Unique list of rpc urls present
	pub total_rpcs: HashSet<RpcUrl>,
	/// If true, one of the transactions did not have a rpc
	pub missing_rpc: bool,

All of those parameters are initialized with default values and populated later, accessing those in the wrong place might result in panic or unintended behavior.

I've also simplified logic where it was possible, however, we still have several pretty long functions which would be nice in the shorter form, but this can be fixed a bit later, esp considering that all of the state transaction functions are normally readable

[klkvr]

And we also no more need the --multi flag, because we are executing script anyway and have the full list of RPCs before resuming, so we can just check if it has more than 1 RPC: https://github.com/klkvr/foundry/blob/klkvr/refactor-script/crates/forge/bin/cmd/script/resume.rs#L31

[klkvr]

We should also be able to remove onchain simulation step once we have isolation mode enabled by default

[mattsse]

this looks great, this is a lot easier to navigate

I'd like to see a few more docs on functions,types,fields

[mattsse]

this is great!

I'd like a few more function docs that cover the workflow

[mattsse]

still would like a brief explainer for run_script, what the individual steps are

[mattsse]

the individual states are very easy to follow now.

@DaniPopes would appreciate a closer look

[mattsse]

still would like a brief explainer for run_script, what the individual steps are

[gakonst]

@joshieDo cc FYI in case interested!

[DaniPopes]

Can we extract this into a separate crate?

[DaniPopes]

fn sequences{_mut}(&self) -> &[ScriptSequence] rather than iter IMO

[DaniPopes]

no spaces in between imports

[mattsse]

Can we extract this into a separate crate?

oh yeah, totally forgot about this, I'd like that, yes

[klkvr]

@DaniPopes @mattsse I fthink it makes sense to avoid having clap dependency in the separate crate. A reasonable way to do this might be to move some of ScriptArgs methods to ScriptConfig and populate them in ScriptArgs::preprocess. But we'd duplicate half of ScriptArgs fields that way.

Do you see any cleaner solution here?

[klkvr]

ah, we'd still need clap for VerifyArgs, so we can either line up verification extraction and get rid of clap where it will be possible then, or divide ScriptArgs and ScriptConfig now anyway

[klkvr]

Extracted scripting code to separate crate and also removed ScriptArgs dependency on BuildArgs. It now uses CoreBuildArgs + separate --skip parameter. Removed params are --names, --sizes, --watch* and --format-json. All of those had not effect. This change should close #4476

[mattsse]

amazing.

I see that a few impl blocks are not colocated with thir struct def, this is probably because the previous ScriptArgs impl was scattered across multiple files, but this makes it a lot harder to navigate.

impl blocks should go under their respective struct def

[mattsse]

lgtm

pending @DaniPopes