Skip to content

feat(forge): error hints #3135

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 4 commits into from
Closed

feat(forge): error hints #3135

wants to merge 4 commits into from

Conversation

onbjerg
Copy link
Collaborator

@onbjerg onbjerg commented Sep 8, 2022

Motivation

Some cheatcodes lack context when they fail, but our current setup does not allow us to add very much context without the output of forge test becoming super cluttered

Solution

Allow reverts from our backend and our inspectors to include optional hints

Effectively closes #928 as some of the more annoying cheatcode reverts are documented in other issues

@onbjerg onbjerg added the T-feature Type: feature label Sep 8, 2022
@onbjerg onbjerg force-pushed the onbjerg/error-hints branch from 643b25a to 6269656 Compare September 8, 2022 16:21
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do u see encode_error_with_hints being integrated?

@onbjerg
Copy link
Collaborator Author

onbjerg commented Sep 8, 2022

@gakonst adding an example rn, but for example u could use it for expectEmit - its a long standing issue that the error msg sucks a lot here since it doesn't tell u what part of the log didn't match, so u could add hints that explain what parts match and what parts dont

@onbjerg
Copy link
Collaborator Author

onbjerg commented Sep 8, 2022

Not the best example (expectEmit will take more work) but instead of writing the really big number inside of the short test result it could be a hint:

image

Similarly, sometimes the forking backend might fail for whatever reason - it might be that the account is not accessible on your node, so a hint could be added that it's too far back if we think it is and suggest an archive node or moving the block you're forking from

@gakonst
Copy link
Member

gakonst commented Sep 20, 2022

OK! This makes sense. Supportive.

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.

this looks pretty good, nice improvement

Comment on lines +92 to +103
Ok(match code {
0x00 => SolidityPanic::Generic,
0x01 => SolidityPanic::Assert,
0x11 => SolidityPanic::OverUnderFlow,
0x12 => SolidityPanic::DivideByZero,
0x21 => SolidityPanic::InvalidEnumCast,
0x22 => SolidityPanic::InvalidStorageByteArray,
0x31 => SolidityPanic::PopOnEmptyArray,
0x32 => SolidityPanic::IndexOutOfBounds,
0x41 => SolidityPanic::Alloc,
0x51 => SolidityPanic::InvalidPointer,
code => SolidityPanic::Unknown(code),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could move this to a separate From impl

@gakonst
Copy link
Member

gakonst commented Jan 17, 2023

Closing as stale.

@gakonst gakonst closed this Jan 17, 2023
@gakonst gakonst deleted the onbjerg/error-hints branch January 17, 2023 19:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(cheatcodes): show detailed reverts regardless of verbosity level defined
3 participants