ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.27k stars 5.76k forks source link

Add option to automatically add revert reason strings when omitted from require. #9086

Closed MicahZoltu closed 1 year ago

MicahZoltu commented 4 years ago

Abstract

When a user includes a require(variable < 5) without providing a reason string, the compiler will automatically add a reason string that gives a hint to anyone debugging where the error may be. One option would be to just use the conditional text ("variable < 5" in the example above) and another would be to include file/contract/line number.

This was originally proposed in https://github.com/ethereum/solidity/issues/7260#issuecomment-531168669 but the original issue got fixed and it appears this request was lost when that issue was closed.

Motivation

Debugging Solidity contracts is incredibly difficult at the moment compared to other modern languages due to the lack of a good integrated debugger. Even when an integrated debugger is available, this provides no help with runtime failures which currently do not include a stack trace (parity_trace kinda helps but it doesn't show you where in a function you were when the failure occurred).

If information was available at runtime as to where the failure occurred then debugging why the failure occurred would be much easier because it would greatly narrow the search space for the bug. While the user can go through and manually add reason strings to all require statements, it is often just the author manually duplicating the contents of the condition or noting a file/contract/function/counter. This manual labor could be done by the compiler instead.

Specification

// fruit.sol
contract Apple {
    function banana() public pure {
        require(condition);
    }
}

The generated reason string for the above would be fruit.sol:Apple:banana:1

I'm interested in hearing opinions from others on what the automated reason should include. Anything that assists in locating the failing require would work. Copying the condition has the problem of potentially being ambiguous and potentially being very long, which is why I have gone with file/contract/function/line above. Needs spec on how to handle requires in modifiers, presumably something like file/contract/modifier/line?

Backwards Compatibility

No issues.

MicahZoltu commented 4 years ago

Using the proposed syntax, how would function overloads be differentiated?

chriseth commented 4 years ago

Thank you for re-suggesting this, I did not mean to exclude it from the scope of the original issue!

About the reason string: Note that file names (including path) can be rather long. Also - shouldn't it include the failing expression prominently: fruit.sol:1 - "condition"

MicahZoltu commented 4 years ago

I'm not opposed to including the failing expression, I only left it out of the specification because previously there arguments about the reason string becoming unbearably long. For example if you have a require statement like:

require(apple != 0x123456789123456789123456789123456789 && banana != 0x123456789123456789123456789123456789 || cherry != 0xbaadf00dbaadf00dbaadf00dbaadf00dbaadf00dbaadf00dbaadf00dbaadf00d)

You are right though, global names (file names) can become quite long as well since they sometimes will be full paths or relative paths. I'm not personally opposed to long reason strings, it just increases deployment gas costs which I don't really care about, and you can always disable the auto-reason strings or you can add reason strings in manually (like you probably should be doing anyway).

The big win for this change for me personally is when I'm using a library written by someone else who decided not to use reason strings, I don't have to fork their code just to make my code debuggable.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

github-actions[bot] commented 1 year ago

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.