Consider to encode certain reverts (e.g. abi decoding / input validation errors) as `Error(uint256)` with defined error codes

[cburgdorf]

Abstract

Consider to encode certain reverts as Error(uint256) with defined error codes

Motivation

Both Solidity and Fe currently use Panic(uint256) to encode certain reverts with different panic codes as defined in the documentation.

However, there are certain reverts around decoding of invalid abi encoded data for which Solidity reverts with zero data. Fe currently encodes these reverts as Panic(uint256) with panic code 0x99.

But as @ekpyron pointed out Panic(uint256)was meant for a different category of failures.

Citing @ekpyron

short summary is: panics, which have the panic codes, were introduced to replace "asserts", resp. "this should never happen" situations, while decoding invalid abi encoded data has to be expected to happen and rather falls into the category of "require" errors... which is why they weren't covered in the panic codes

The Fe team wants to avoid reverting with zero information to improve the developer experience. The Fe team is quite happy to re-align to a different encoding if the Solidity teams want to start encoding these reverts differently.

We briefly discussed using the signature Error(uint256) for this category of errors and the purpose of this issue is to find out if that would be a favorable way forward for this category of errors.

[chriseth]

The solidity compiler has a switch to enable error messages for ABI decoding failures which cause a revert of type Error(string). You could use that (even with an empty string) until we agree on the error codes.

[cburgdorf]

Well, we are not relying on the solidity compiler for that so I guess we will just continue using 0x99 with Panic(uint256) in Fe but happily make the switch to something else if we can find a common scheme. Or maybe we just spearhead the Error(uint256) idea if we feel adventurous. We are not yet aiming for language stability just yet so we feel free to make these changes more lightly.

[chriseth]

Even if you plan to change this in the future, I would ask you not to use Panic(uint256) for input validation. In Solidity, Panic(uint256) is exclusively used for internal errors with the idea in mind that bytecode-based dynamic analysis tools (formal verification, symbolic execution, fuzzing, ...) can check if such errors can be created at all by the code and flag an error if this is the case. If you use Panic(uint256) for input validation, these errors are always reachable.

Feel free to introduce a code scheme for Error(uint256) or even a different kind of error (DecodingError(uint256)?)

[cburgdorf]

@chriseth Thank you for your input, that makes sense. I filed an issue to change that: https://github.com/ethereum/fe/issues/492

[fvictorio]

In principle, there could be custom errors in the wild that already use the Error(uint256) signature, right?

I'm asking this because a tool like Hardhat could intercept those errors and show a message like "Invalid ABI decode" (or whatever) when it was actually a custom user error. Of course, the same thing happens with Panic(uint256), but someone defining a custom Error(uint256) seems more likely.

I don't think this is a big deal though, just something to keep in mind. Maybe solhint should have a rule forbidding custom errors with those signatures.

[ekpyron]

Error(uint256) (resp. all error signatures with error names Error and Panic) are already reserved and cannot be used for user-defined errors.

[cameel]

Well, even though they're reserved, anyone can just generate revert data containing any error signature. But at least you can assume it won't happen just by accident in normal use. Someone would have to do it on purpose.

[ekpyron]

Yeah, sure, but that we can never avoid.

[fvictorio]

Error(uint256) (resp. all error signatures with error names Error and Panic) are already reserved and cannot be used for user-defined errors.

Oh, nice, I didn't know that

[cameel]

Also, we have a (still undecided) feature request that might result in Error and/or Panic being usable by user-level code (and maybe no longer being reserved): #11792

[ekpyron]

Also, we have a (still undecided) feature request that might result in Error and/or Panic being usable by user-level code (and maybe no longer being reserved): #11792

Yeah, but only by importing it explicitly, not by redeclaring them, so in that sense still reserved even then - and if we allowed that, the understanding would be that it'd be for cases in which users explicitly want to mimic the builtin errors - so I wouldn't think that'd be much of an issue here.

[github-actions[bot]]

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]]

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.

[0xalpharush]

Can we reopen this?