Skip to content

Conversation

@iFrostizz
Copy link
Contributor

@iFrostizz iFrostizz commented Aug 21, 2023

First part of #5547 breaking into smaller chunks.
This PR refactors the fuzz function in order to have access to a more granular, single-step, function called single_fuzz.
It will let us in the context of the debugger to be able to run again the fuzzer on the last relevant (either a counter-example, or the first case, see the current imeplemtnation of the debugger launching https://github.com/foundry-rs/foundry/blob/master/crates/forge/bin/cmd/test/mod.rs#L209-L213 with this time debug traces (they are expensive, especially when fuzzed).

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.

Nice! Loving how small this is. Some comments on docs and other things

wondering if it's possible to get a PR description on what exactly this is deduping? It's pretty clear if you read the code, but just for future reference

Also wondering, are these split PRs safe to be merged in any order or should we hold off?

@iFrostizz
Copy link
Contributor Author

Thanks @Evalir ! Adding a desc soon!
I'll streamline the PRs as they go to build https://github.com/foundry-rs/foundry/pull/5547/files with a reasonable amount of changes, would be great to merge them in order to avoid duplicate work.

@iFrostizz iFrostizz requested a review from Evalir August 22, 2023 19:33
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@iFrostizz
Copy link
Contributor Author

@Evalir @mattsse What's up with this one ?

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.

ty for breaking this up into smaller chunks!

some nits, otherwise lgtm

@iFrostizz iFrostizz requested a review from mattsse August 25, 2023 08:31
@Evalir
Copy link
Member

Evalir commented Aug 25, 2023

@iFrostizz sorry for the long review time—we've been setting up for travel and traveling over the past week so it's taken a tad more time to review things—let's get this in ASAP

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.

sweet, looks good. just a smol nit on docs

will defer to @mattsse for final approve / merge

@iFrostizz
Copy link
Contributor Author

@Evalir all good, and hope that you're all having fun there!
Thanks for the review :)

@iFrostizz
Copy link
Contributor Author

Could we get rid of this use of RefCell ? https://github.com/foundry-rs/foundry/blob/master/crates/evm/src/fuzz/mod.rs#L67-L80
Also, in a further PR we could get rid of the FuzzCase struct

@iFrostizz iFrostizz requested a review from Evalir August 27, 2023 16:02
@Evalir
Copy link
Member

Evalir commented Aug 29, 2023

@iFrostizz if it's not needed anymore, then sure!

@iFrostizz
Copy link
Contributor Author

@Evalir could that be preferably done after the debugger refactor ?

@Evalir
Copy link
Member

Evalir commented Aug 29, 2023

yep sounds good!

@mattsse friendly ping

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.

aight sweet, sorry for taking so long with this one—let's send it and keep iterating

@Evalir Evalir merged commit c976619 into foundry-rs:master Aug 30, 2023
@iFrostizz
Copy link
Contributor Author

iFrostizz commented Aug 30, 2023

thanks @Evalir !
next one coming soon, prob gonna be the last one, sorry I couldn't cut less. But noticed that some code moves were unnecessary for that task, so it should be reduced to ~500 lines i hope!

mikelodder7 pushed a commit to LIT-Protocol/foundry that referenced this pull request Sep 12, 2023
* 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 docki docs

* fmt

---------

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
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