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

Proper error handling in EtherRouter #129

Merged
merged 14 commits into from
Dec 13, 2017

Conversation

elenadimitrova
Copy link
Contributor

@elenadimitrova elenadimitrova commented Dec 11, 2017

Since delegatecalls return a value indicates success or failure of the call, we use an assert in EtherRouter to check that. Since the introduction or the different validators: assert, require and revert however calls that fail the gas-efficient revert in contracts still consume all gas because of the EtherRouter assert check.
Here we've switched that assert to a require which eliminates this problem returning the unspent gas in failed requires and consuming all the gas still in failed asserts down the call chain from EtherRouter.

When using EtherRouter and further down the chain an assert fails, not all sent gas is spent in the overall transaction, but ~73000 gas less than the total.

Further we improve and simplify test checks for errors in two: checkErrorAssert and checkErrorRevert where both check

  • the transaction receipt return value is 0 (for failure)
  • if client is testrpc we ensure the return error message is a revert (from EtherRouter)
  • also we validate gas usage for checkErrorAssert to differentiate from checkErrorRevert

Note that an upgrade of the parity client was required due to a fix there for openethereum/parity-ethereum#6920
This takes us to Beta version 1.8.3

This PR also enables the test suite to be run under parity by adding a check in testHelperer.forwardTime function that skips any tests that calls it, since this is not yet implemented in parity

@elenadimitrova elenadimitrova self-assigned this Dec 11, 2017
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch 2 times, most recently from c1e2882 to 9684056 Compare December 11, 2017 19:57
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch 2 times, most recently from d897c87 to 415ef53 Compare December 11, 2017 22:22
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch from 415ef53 to 26b1bfb Compare December 12, 2017 09:24
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch 13 times, most recently from cca59ad to dd171cf Compare December 12, 2017 16:06
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch 3 times, most recently from f7749e1 to ccab8a8 Compare December 12, 2017 16:50
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch from ccab8a8 to 3cdd8d4 Compare December 12, 2017 17:02
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch from 2caf0b0 to 7cd9eb4 Compare December 13, 2017 15:12
@elenadimitrova elenadimitrova force-pushed the maintenance/proper-exception-handling branch from 7cd9eb4 to dbae55b Compare December 13, 2017 15:33
@area area merged commit 91105ef into develop Dec 13, 2017
@elenadimitrova elenadimitrova deleted the maintenance/proper-exception-handling branch December 13, 2017 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants