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

Revert with reason #3364

Merged
merged 19 commits into from Apr 12, 2018
Merged

Revert with reason #3364

merged 19 commits into from Apr 12, 2018

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Dec 30, 2017

Part of issue #1686

Depends on #3373

I went with encoding the reason string as a function call to Erorr(string), including the function selector.

Still to do:

  • the same for require()
  • documentation for require()
  • forwarding of the reason string by the caller together with bubbling up the error
  • tests for forwarding reason string for weird cases like create and transfer.

The reason we might not want to forward the reason string is that it allows a called attacker to consume more gas in the caller (because the caller needs to copy the return data), but I think this is an acceptable "risk" given the benefit that the error string shows up in the transaction receipt.

@SilentCicero
Copy link

@SilentCicero SilentCicero commented Dec 31, 2017

Looking forward to this ;)

@GriffGreen
Copy link

@GriffGreen GriffGreen commented Jan 3, 2018

Me toooooooo!!!! :-D

@chriseth
Copy link
Contributor Author

@chriseth chriseth commented Jan 3, 2018

This is ready for review.

@chriseth chriseth requested a review from axic Jan 3, 2018
@chriseth chriseth force-pushed the revertWithReason branch 2 times, most recently from fd4f6b7 to ac20dfb Compare Jan 5, 2018
@onbjerg
Copy link

@onbjerg onbjerg commented Jan 7, 2018

❤️ ❤️ ❤️ ❤️ This is the best!

@izqui
Copy link

@izqui izqui commented Jan 7, 2018

Is the string returned regularly, right? In terms of forwarding the return data, would this work https://github.com/aragon/aragon-core/blob/dev/contracts/common/DelegateProxy.sol#L9

@chriseth
Copy link
Contributor Author

@chriseth chriseth commented Jan 10, 2018

Rebased.

@AnthonyAkentiev
Copy link

@AnthonyAkentiev AnthonyAkentiev commented Jan 11, 2018

Great improvement!

@tcoulter
Copy link

@tcoulter tcoulter commented Jan 11, 2018

Looking forward to implementing this in Truffle. Nice job @chriseth!

@chriseth chriseth added this to Planned for 0.4.21 in 0.4.21 Feb 16, 2018
@chriseth chriseth moved this from Planned for 0.4.21 to In Progress in 0.4.21 Feb 16, 2018
@chriseth chriseth moved this from In Progress to Under Review in 0.4.21 Feb 19, 2018
@chriseth chriseth force-pushed the revertWithReason branch 6 times, most recently from bb5bcbe to 2373264 Compare Mar 6, 2018
@chriseth chriseth added this to To do in 0.4.22 via automation Mar 7, 2018
@chriseth chriseth removed this from Under Review in 0.4.21 Mar 7, 2018
@naddison36
Copy link

@naddison36 naddison36 commented Mar 7, 2018

Sad to see this bumped to 0.4.22. I was getting ready to change my contracts to take advantage of this new feature with the 0.4.21 release

@chriseth
Copy link
Contributor Author

@chriseth chriseth commented Apr 12, 2018

Failure in appveyor is just bytecode upload and thus can be ignored.

@chriseth
Copy link
Contributor Author

@chriseth chriseth commented Apr 12, 2018

I think prefixing a specific 4-byte sequence allows us to extend the encoding later, while at the same time it does not reduce usability since it is fixed for now. Also, it is better than just prefixing a certain number of zeros, since it already suggests away how to extend it to encode different types.

I would opt to merge it :)

ekpyron
ekpyron previously approved these changes Apr 12, 2018
in a call to ``revert``. The ``throw`` keyword can also be used as an alternative to ``revert()``.
revert the current call. It is possible to provide a string message containing details about the error
that will be passed back to the caller.
The ``throw`` keyword can also be used as an alternative to ``revert()``, but is deprecated.
Copy link
Member

@axic axic Apr 12, 2018

Choose a reason for hiding this comment

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

... and it doesn't support an error string.

Copy link
Contributor Author

@chriseth chriseth Apr 12, 2018

Choose a reason for hiding this comment

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

Yes, alternative to rever(), not to revert("string") :)
Do you want me to clarify that or not?

Copy link
Member

@axic axic Apr 12, 2018

Choose a reason for hiding this comment

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

I think it may be cleaner that way. I didn't spot it before you mentioned, of course it is clear after you did.

@@ -160,11 +160,21 @@ BOOST_AUTO_TEST_CASE(location_test)
shared_ptr<string const> n = make_shared<string>("");
Copy link
Member

@axic axic Apr 12, 2018

Choose a reason for hiding this comment

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

This is still dead code.

Copy link
Member

@axic axic left a comment

Needs test cases for built in revert bubbling:

  • transfer
  • contract creation
  • regular foreign contract calls

i.e.

contract C {
  function f() {
    revert("failed here :(");
  }
}
contract D {
  D d = new D();
  function f() {
    return d.f();
  }
}

@chriseth
Copy link
Contributor Author

@chriseth chriseth commented Apr 12, 2018

The test case bubble_up_error_messages does it for a regular external function call, bubble_up_error_messages_through_transfer for transfer and bubble_up_error_messages_through_create for create....

axic
axic approved these changes Apr 12, 2018
@axic
Copy link
Member

@axic axic commented Apr 12, 2018

That's weird, I have seen revert_with_cause and require_with_message, but not the bubble up ones. Anyway, they look correct.

@chriseth chriseth merged commit 7054def into develop Apr 12, 2018
7 of 8 checks passed
0.4.22 automation moved this from Under Review to Done Apr 12, 2018
@axic axic deleted the revertWithReason branch Apr 12, 2018
@leanthebean
Copy link

@leanthebean leanthebean commented Apr 15, 2018

Oh wow, this is so great. Thank you!

@vsdigitall
Copy link

@vsdigitall vsdigitall commented Apr 30, 2018

contract Example {
  function test (uint i) {
    require(i == 1, "ERROR_TEXT");
  }
}

How to read require/revert error string from failed transaction log? (ERROR_TEXT in example above)

@andreafspeziale
Copy link

@andreafspeziale andreafspeziale commented Jun 17, 2018

I know that maybe is not the right place but I've not found any references or examples so +1 to @vsdigitall. I think that the reason string is in the output as hex in the failed transaction. But I've been not able to fully test this thing.

@gluk64
Copy link

@gluk64 gluk64 commented May 4, 2019

Here is a bash script to fetch revert reason:

https://ethereum.stackexchange.com/a/70391/3558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.4.22
  
Done
Development

Successfully merging this pull request may close these issues.

None yet