-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add global failure #30
Conversation
generally supportive, this seems like a nice fix to some legit issues. I would slightly prefer the cc @MrChico and @rainbreak, any thoughts and opinions? |
ACK on the fix - indifferent on the low level calls / storage slot name. Whatever @d-xo @MrChico @rainbreak prefer |
got an out of band lgtm from @MrChico. @brockelmore if you :
then I will merge 👍 |
Update for @MrChico: tested on all major versions (0.5.0, 0.6.0, 0.7.0, 0.8.0) |
Thanks! |
Since dapphub/ds-test#30 calls to `failed()` can contain reverting branches due to the call into abi.decode(). This meant that using prove tests with the latest ds-test version would always fail with a hard error. This change updates the symbolic test stepper to correctly report `bailed` as true for branches that reverted during the call to `failed()`.
Since dapphub/ds-test#30 calls to `failed()` can contain reverting branches due to the call into abi.decode(). This meant that using prove tests with the latest ds-test version would always fail with a hard error. This change updates the symbolic test stepper to correctly report `bailed` as true for branches that reverted during the call to `failed()`.
Open to feedback on this, but it adds a global failure check if the cheatcode address has any code in it (dapptools and forge both do). This works by
store
ing a bool in slot 0 of theHEVM_ADDRESS
& changing thefailed
to a private var with a custom view function that checks the slot. I was going to have it be a custom address, butforge
uses a state changeset that only produces state changes if a contract has bytecode & withoutforge
'setch
cheatcode we can't do that here. I doubt anyone uses the HEVM address for storage like this anyway so I dont think it should be a problem. I could change the slot tobytes32("failed")
if that would be better.Some notes on implementation:
HEVM_ADDRESS
as an actual address inextcodesize
call to satisfy 0.4.23 solidity semantics (constants not allowed in assembly blocks in that old version, but lifted in later versions)hevm
andforge
. I am willing to switch this to a low level call if the current implementation is annoyingWould allow for easy fix for dapphub/dapptools#768