Debugger Refactor #2: `DebuggerArgs` by iFrostizz · Pull Request #5753 · foundry-rs/foundry

iFrostizz · 2023-08-30T21:37:20Z

Second and last part of #5547 (sorry, cannot do smoller)
Adding the new DebuggerArgs struct that all part of the code should comply with,
Removing the run_debugger function that is ran from forge script
Simpler way of mapping contract sources, see ui::debugger::ContractSources type

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

Evalir

nice! let's fix conflicts before doing a deeper review

iFrostizz · 2023-08-30T21:50:24Z

yep doing that now sorry sir

iFrostizz · 2023-08-30T22:21:41Z

did a self review, looks good to me @Evalir
there are less changes than it seems in the diff, git is not smart enough to show the correct diffs for lib.rs and make it look like there are many.

Evalir

need to look at this more closely, but some preliminary comments

crates/common/src/compile.rs

crates/common/src/contracts.rs

crates/evm/src/fuzz/mod.rs

crates/evm/src/trace/identifier/etherscan.rs

Evalir · 2023-09-01T00:05:14Z

crates/evm/src/trace/identifier/etherscan.rs

+    )> {
        let mut compiled_contracts = BTreeMap::new();
-        let mut sources = BTreeMap::new();
+        let mut sources = HashMap::new();


This shuffles ordering—is ordering an issue here?

We don't need ordering, so there should be better performances with a HashMap
Can leave as-is if not appriopriate for this PR

crates/forge/bin/cmd/script/build.rs

Evalir · 2023-09-01T00:06:25Z

crates/forge/bin/cmd/script/build.rs

    pub libraries: Libraries,
    pub predeploy_libraries: Vec<ethers::types::Bytes>,
-    pub sources: BTreeMap<u32, String>,
+    pub sources: HashMap<String, HashMap<u32, (String, ContractBytecodeSome)>>,


Almost sure this might cause linking problems, ordering might be important here

Could you please elaborate ? I'm not sure why ordering would matter because we map the sources from the artifact Id https://github.com/foundry-rs/foundry/pull/5753/files/466d24a4be210ff3be7b7eedb8e62c76e4630d6e#diff-fdebd782f78bd93139131fe4f614d8ab47564bceef84dd9a009483b0c5ac7b41R42

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

crates/forge/src/runner.rs

mattsse

Since we're refactoring this to make it better, I'm being extra nitpicky here

mattsse · 2023-09-03T14:35:09Z

crates/common/src/compile.rs

+/// Id to map from the bytecode to the source file
+pub type FileId = u32;


please no public type aliases for primtive values. either make it private or add a new wrapper type.

and move to top of file

I think that it's reasonable to just drop this type

crates/common/src/compile.rs

crates/forge/bin/cmd/script/build.rs

iFrostizz · 2023-09-03T23:39:03Z

Since we're refactoring this to make it better, I'm being extra nitpicky here

This is good

Evalir

a few more comments, need to do another pass on the linking section—sorry we're being nitpicky, we just really wanna improve this! great work so far

crates/common/src/compile.rs

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

crates/forge/src/multi_runner.rs

Evalir · 2023-09-04T14:21:48Z

crates/forge/src/runner.rs

+            let calldata = if let Some(counterexample) = result.counterexample.as_ref() {
+                match counterexample {
+                    CounterExample::Single(ce) => ce.calldata.clone(),
+                    _ => unimplemented!(),


will we resolve this?

I don't know, I'm not sure how to handle this because everything has been shaped for one calldata and here it messes things up to have a sequence of calls

Could we do that in #3 ? Probably modify the single_fuzz to take in a Vec<calldata, value> to support sequence calls. Just noticed that the value is defaulted to 0 right now as well, might make it a bug

I'm just not familiar how we can force the fuzzer to construct a sequence of calls

mattsse

nice, defer to @Evalir to get this over the line

crates/common/src/compile.rs

iFrostizz · 2023-09-08T09:15:03Z

Friendly ping @Evalir

mattsse

lgtm pending conflicts, sorry about that

Evalir

yep agreed with mattsse_—sorry for the rebasing work needed here

iFrostizz · 2023-09-08T17:48:43Z

No it's all G, thanks for the thorough review !

Evalir

gg, sweet, happy that this is over the line! (let's merge on monday just in case)

* fuzz single refactor * add struct docs * Update crates/evm/src/fuzz/mod.rs Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com> * add docs and move types to types.rs * fmt * add new debugger args type * add minimal debugger-refactor changes * finish him! * fmt * remove TODO * minimal diff * apply review suggestions * add TODO * looks better * make ContractSources wrapper * add more docki docs * write file_id docs! --------- Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

iFrostizz and others added 9 commits August 21, 2023 23:15

fuzz single refactor

07468d2

add struct docs

f470de9

Update crates/evm/src/fuzz/mod.rs

e016eaa

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

add docs and move types to types.rs

78dca6a

fmt

15e4709

add new debugger args type

4b40bc3

add minimal debugger-refactor changes

e9f8942

finish him!

29aa8ab

fmt

f41cf33

Evalir requested changes Aug 30, 2023

View reviewed changes

iFrostizz added 3 commits August 31, 2023 01:00

Merge branch 'master' into franfran/debugger-args

7818767

remove TODO

0058201

minimal diff

466d24a

iFrostizz requested a review from Evalir August 30, 2023 22:20

iFrostizz changed the title ~~Franfran/debugger args~~ Debugger Refactor #1: DebbugerArgs Aug 30, 2023

Evalir requested changes Sep 1, 2023

View reviewed changes

iFrostizz changed the title ~~Debugger Refactor #1: DebbugerArgs~~ Debugger Refactor #2: DebbugerArgs Sep 2, 2023

iFrostizz added 2 commits September 2, 2023 12:58

apply review suggestions

7b71916

add TODO

d336bfe

iFrostizz requested a review from Evalir September 2, 2023 10:00

looks better

c6a66fb

mattsse requested changes Sep 3, 2023

View reviewed changes

make ContractSources wrapper

17ddabc

iFrostizz requested a review from mattsse September 3, 2023 23:36

Evalir requested changes Sep 4, 2023

View reviewed changes

add more docki docs

0aec987

mattsse requested changes Sep 4, 2023

View reviewed changes

crates/common/src/compile.rs Show resolved Hide resolved

write file_id docs!

b513de3

iFrostizz requested a review from Evalir September 4, 2023 18:34

mattsse requested changes Sep 8, 2023

View reviewed changes

Evalir requested changes Sep 8, 2023

View reviewed changes

Merge branch 'master' into franfran/debugger-args

9663c2b

iFrostizz requested review from Evalir and mattsse September 8, 2023 17:48

Evalir changed the title ~~Debugger Refactor #2: DebbugerArgs~~ Debugger Refactor #2: DebuggerArgs Sep 8, 2023

Evalir approved these changes Sep 8, 2023

View reviewed changes

Evalir merged commit 523354e into foundry-rs:master Sep 11, 2023

iFrostizz mentioned this pull request Sep 12, 2023

fix(forge) hotfix - filter tests #5822

Closed

trevorgjohnson mentioned this pull request Sep 22, 2023

chore(test): ensure decoder building occurs only when --debug is present #5879

Merged

DaniPopes mentioned this pull request Dec 8, 2023

fix(debugger): don't panic when arena is empty #6547

Merged

iFrostizz mentioned this pull request Jun 28, 2024

feat(cheatcodes): add vm.chisel() to load chisel session from test function #4171

Open

		/// Id to map from the bytecode to the source file
		pub type FileId = u32;

Conversation

iFrostizz commented Aug 30, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

iFrostizz commented Aug 30, 2023

Uh oh!

iFrostizz commented Aug 30, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iFrostizz commented Sep 3, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iFrostizz commented Sep 8, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

iFrostizz commented Sep 8, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants