Skip to content
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 exception handling #1409

Merged

Conversation

danhper
Copy link
Contributor

@danhper danhper commented Jan 26, 2022

What I did

Exception handling seems to be failing in a few cases with Ganache 7, which is now the latest stable version.
This seems a bit hacky overall but does seem to do the job for our fairly large test suite, so hopefully, it should be better than nothing. It should also not introduce any regression since most of the additions only occur if the logic already in place failed to handle the exception.

How I did it

  1. With the latest ganache, for some reason, web3py does not raise a ValueError when a contracts revert so we manually create a VirtualMachineError in Brownie when the receipt status is 0 otherwise, we have an attribute error in Brownie because the exception is None. This works for almost everything but we lose the PC and a few other nice-to-have features. I think that the longer-term fix would be either in Ganache (keep a backward-compatible format) or in web3py (support new Ganache response format)
  2. The error format seems to be slightly different in the latest version of Ganache (i.e. the txid is not in the data), which causes the VirtualMachineError to throw a ValueError, breaking brownie.reverts. When this is the case, we try to see if we can still find the program counter and revert_type before throwing a ValueError

How to verify it

A simple script running on ganache v7 should do the trick.

// Foo.sol
contract Foo {
  function foo() external {
    revert("done");
  }
}
# foo.py
foo = accounts[0].deploy(Foo)
with brownie.reverts("done"):
    foo.foo({"from": accounts[0]})

This fails with the current version of Brownie but works with this patch.

Checklist

  • I have confirmed that my PR passes all linting checks
  • I have included test cases
  • I have updated the documentation
  • I have added an entry to the changelog

@danhper danhper force-pushed the fix-exception-handling branch 2 times, most recently from 3740c5e to 6884a06 Compare January 27, 2022 18:39
@danhper danhper force-pushed the fix-exception-handling branch from 6884a06 to 3173816 Compare January 27, 2022 18:47
@iamdefinitelyahuman iamdefinitelyahuman merged commit af0be99 into eth-brownie:master Jan 29, 2022
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.

2 participants