-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cheatcodes): improve fork cheatcodes messages #9141
Conversation
crates/cheatcodes/src/evm/fork.rs
Outdated
.ecx | ||
.db | ||
.create_select_fork(fork, &mut ccx.ecx.env, &mut ccx.ecx.journaled_state) | ||
.map_err(|err| fmt_err!("{err:?}"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are for eyre errors then i would revert these changes and update the implementation of From<eyre::Error>
foundry/crates/cheatcodes/src/error.rs
Line 296 in 8bdcbfa
eyre::Error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, this would be also fixing same issue for non vm errors like here #8908 (comment) if I get this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaniPopes pls check c0ea850 displaying as
I'm not a fan of multi-line errors because it skews formatting for the whole test report, it would be better if it was flattened into one line |
Agree, multi line looks good for cli reports but not for tests. |
You can get an iterator over the error messages with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better 👍
Motivation
Closes #8908
follow up on #8936 (review) not sure what's the best solution here but I hit this while doing some tests, the issue is with wrapped errors that are not displaying entirely when failed from cheatcodes and propagated to test result, #8936 could be a better solution as will handle all other cases if any...
with this approach:
with fix(
evm
): improve err messaging #8936 approachcurrent
@yash-atreya @mattsse @klkvr
Solution