script refactoring#7247
Conversation
mattsse
left a comment
There was a problem hiding this comment.
I like this approach a lot.
structuring this in compile+link steps makes a lot of sense
be683bf to
7ba569c
Compare
|
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 Safety isn't great because we have a lot of foundry/crates/forge/bin/cmd/script/mod.rs Lines 619 to 625 in c24933d 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 |
|
And we also no more need the |
|
We should also be able to remove onchain simulation step once we have isolation mode enabled by default |
mattsse
left a comment
There was a problem hiding this comment.
this looks great, this is a lot easier to navigate
I'd like to see a few more docs on functions,types,fields
| let pre_simulation = self | ||
| .preprocess() | ||
| .await? | ||
| .compile()? | ||
| .link()? | ||
| .prepare_execution() | ||
| .await? | ||
| .execute() | ||
| .await? | ||
| .prepare_simulation() | ||
| .await?; |
There was a problem hiding this comment.
this is great!
I'd like a few more function docs that cover the workflow
There was a problem hiding this comment.
still would like a brief explainer for run_script, what the individual steps are
mattsse
left a comment
There was a problem hiding this comment.
the individual states are very easy to follow now.
@DaniPopes would appreciate a closer look
| let pre_simulation = self | ||
| .preprocess() | ||
| .await? | ||
| .compile()? | ||
| .link()? | ||
| .prepare_execution() | ||
| .await? | ||
| .execute() | ||
| .await? | ||
| .prepare_simulation() | ||
| .await?; |
There was a problem hiding this comment.
still would like a brief explainer for run_script, what the individual steps are
|
@joshieDo cc FYI in case interested! |
DaniPopes
left a comment
There was a problem hiding this comment.
Can we extract this into a separate crate?
| } | ||
| } | ||
|
|
||
| pub fn iter_sequences(&self) -> impl Iterator<Item = &ScriptSequence> { |
There was a problem hiding this comment.
fn sequences{_mut}(&self) -> &[ScriptSequence] rather than iter IMO
| use alloy_json_abi::{Function, InternalType, JsonAbi}; | ||
| use alloy_primitives::{Address, Bytes, Log, U256, U64}; | ||
| use alloy_rpc_types::request::TransactionRequest; | ||
|
|
oh yeah, totally forgot about this, I'd like that, yes |
|
@DaniPopes @mattsse I fthink it makes sense to avoid having Do you see any cleaner solution here? |
|
ah, we'd still need clap for |
ca3e21d to
79213c3
Compare
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
79213c3 to
300a181
Compare
|
Extracted scripting code to separate crate and also removed |
mattsse
left a comment
There was a problem hiding this comment.
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
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
buildmodule, it now returns and operates onBuildDataandLinkedBuildDatastructs which contain all build artifacts and solc-related data and allow us to use them through script execution instead of passingLinkereverywhere.Closes #7244
Closes #4476