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

EIP 838: ABI specification for REVERT reason string #838

Closed
federicobond opened this issue Jan 20, 2018 · 33 comments
Closed

EIP 838: ABI specification for REVERT reason string #838

federicobond opened this issue Jan 20, 2018 · 33 comments
Labels

Comments

@federicobond
Copy link

federicobond commented Jan 20, 2018

EIP: 838
Title: ABI specification for REVERT reason string
Author: Federico Bond
Status: Draft
Created: 2017-01-20

Simple Summary

A proposal to extend the ABI specification to include typed errors in the REVERT reason string.

Abstract

This proposal specifies how to encode potential error conditions in the JSON ABI of a smart contract. A high-level language could then provide a syntax for declaring and throwing these errors. The compiler will encode these errors in the reason parameter of the REVERT opcode in a way that can be easily reconstructed by libraries such as web3.

Motivation

It's important to provide clear feedback to users (and developers) about what went wrong with their Ethereum transactions. The REVERT opcode is a step in the right direction, as it allows smart contract developers to encode a message describing the failure in the reason parameter. There is an implementation under review in Solidity that accepts a string, thus providing a low-level interface to this parameter. However, standardizing a method for passing errors from this parameter back to clients will bring many benefits to both users and developers.

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.

Specification

To conform to this specification, compilers producing JSON ABIs SHOULD include error declarations alongside functions and events. Each error object MUST contain the keys name (string) and arguments (same types as the function’s inputs list). The value of type MUST be "error".

Example:

{ "type": "error", "name": "InsufficientBalance", "arguments": [ { "name": "amount", "type": "uint256" } ] }

A selector for this error can be computed from its signature (InsufficientBalance() for the example above) in the same way that it's currently done for public functions and events. This selector MUST be included in the reason string so that clients can perform a lookup. Any arguments for the error are RLP encoded in the same way as return values from functions. The exact format in which both the selector and the arguments are encoded is to be defined. The Solidity implementation mentioned above leaves room for expansion by prefixing the free-form string with uint256(0).

A high-level language like Solidity can then implement a syntax like this:

contract MyToken {
  error InsufficientFunds(uint256 amount);

  function transfer(address _to, uint256 _amount) {
    if (balances[msg.sender] <= _amount)
       throw InsufficientFunds(_amount);
    ...
  }
  ...
}

Possible extensions

  1. A NatSpec comment above the error declaration can be used to provide a default error message. Arguments to the error can be interpolated in the message string with familiar NatSpec syntax.
/// @notice You don't have enough funds to transfer `amount`.
error InsufficientFunds(uint256 amount);
  1. A function may declare to its callers which errors it can throw. A list of these errors must be included in the JSON ABI item for that function, under the errors key. Example:
function transfer(address _to, uint256 _amount) throws(InsufficientFunds);

Special consideration should be given to error overloading if we want to support a similar syntax in the future, as errors with same name but different arguments will produce a different selector.

Backwards Compatibility

Apps and tools that have not implemented this spec can ignore the encoded reason string when it's not prefixed by zero.

Revisions

  • 2018/01/23: Improved writing and clarified summary
  • 2018/01/10: Initial draft

Copyright

Copyright and related rights waived via CC0.

@federicobond federicobond changed the title EIP 835: ABI specification for REVERT reason string EIP 838: ABI specification for REVERT reason string Jan 20, 2018
@sinzin91
Copy link

Great idea!

@pirapira
Copy link
Member

Pinging @axic: he had some ideas how to use REVERT when he proposed REVERT.

@VoR0220
Copy link
Member

VoR0220 commented Mar 14, 2018

This is an excellent idea and is the perfect workaround to the problem with reverting on the chain while containing the exact errors. It makes it interpretive like event logs. I like it.

@Arachnid
Copy link
Contributor

Good idea. The term "reason string" is misleading, though - it's not a string, just return data.

@federicobond
Copy link
Author

You are right. I took string from the type of the parameter in the Solidity implementation.

@axic
Copy link
Member

axic commented Apr 6, 2018

We have discussed how this could be implemented and I think we are going on a path to have this in the future.

The current revert-with-reason proposal had a payload of <version><ABI encoded string>, which resulted in 00..00 00..20 <len> <data>.

Now we could instead just assume that there is a generic error with a signature of Error(string), which hashes to 08c379a0 followed by an ABI encoded string, therefore revert("oh noes") would result in the data 08c379a0 00...20 00..07 6f68206e6f6573..00.

We could also have Error(cstring), where cstring is encoded as bytes32 in the ABI, but is assumed to be a C-string, where a zero denotes the end of the string. In that case for short messages we could save same space: 60ed0e86 6f68206e6f6573..00

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

Why not just allow functions to specify an error ABI just as they currently specify a return ABI?

@axic
Copy link
Member

axic commented Apr 6, 2018

Why not just allow functions to specify an error ABI just as they currently specify a return ABI?

Hopefully that will be possible in the future, but right now we only have a feature which supports passing an error string via require() and revert(). This way people can get started with a generic error reporting feature and if in the future we extend this to support something similar to what this EIP is proposing then it will be properly part of the ABI.

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

In that case, why not just return an ABI encoded string, and say that all functions implicitly have an error ABI of string? The function selector seems unnecessary here; we don't have 'return selectors'.

@axic
Copy link
Member

axic commented Apr 6, 2018

Because if we have that generic selector for Error(string) then we can introduce the above EIP frictionlessly. The tools do not need to have a special case handling the non-selector based legacy messages.

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

I'm suggesting that having an error selector in general is needless.

@axic
Copy link
Member

axic commented Apr 6, 2018

Why do you think it is needless?

I imagine that most errors can be encoded as a selector only, if it is exposed to the user, without any other ABI payload.

For example:

contract Wallet {
  error UnauthorisedUser();
  error InsufficientBalance();

  function authorise(bytes32 hash) errors(UnathorisedUser, InsufficientBalance) {
    require(authorisedUsers[msg.sender], UnauthorisedUser); // encoded as a selector only
    require(msg.balance >= pending[hash].amount, InsufficientBalance); // encoded as a selector only
    revert("Unimplemented feature"); // encoded as Error(string) + string
  }
}

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

I guess the question is whether we're implementing an exception mechanism, where uncaught exceptions propagate automatically to the caller, or whether it's a C- or Go-style error mechanism. If the latter, it seems to me that errors and return values should be encoded the same way, with an ABI; if you want to distinguish error types, you can return an enum.

@chriseth
Copy link
Contributor

chriseth commented Apr 6, 2018

@Arachnid my impression of this is the former, this is also the default implementation of function calls, errors are always propagated upwards. Otherwise you have to implement a handler for every single error type and this includes all EVM errors like stack overflow.

So since errors are propagated, we cannot use a fixed enum and thus use the function selector to distinguish the actual error types.

I thought about using a function selector and basically forcing the name to be empty, but dropped it in the end because this would allow you to decode the data in more cases (you can guess most type combinations), but that still does not help you: Now you know the structure of the data, but you cannot handle it unless you also know the meaning. The meaning is provided by having an error type or name. This is of course still no silver bullet, but I think this is as far as we can go when we want to support both propagation of arbitrary errors and handling of some known errors.

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

Okay; if the idea is that errors can be nonlocal and propagate to arbitrary callers, I can see how sending an identifier along with everything else makes sense.

@jacqueswww
Copy link

Hey there!
Just wanted to get clarity on what was decided in terms of this EIP, so I know what to do for vyper when I want to implement the same. Will the clients support decoding an error message from the ABI definitions? Which clients currently support this?
Basically following <sig: 4 bytes><abi_arguments:x bytes> format?

@axic
Copy link
Member

axic commented May 17, 2018

@jacqueswww for the current behaviour see: #838 (comment)

I'm writing up a proper description of this.

@ricmoo
Copy link
Contributor

ricmoo commented Jun 23, 2018

I have added this to the experimental TypeScript branch for ethers.js and currently I do not see any version byte.

I might suggest the following, if the length (in decoded bytes) of the result is congruent to 4 mod 32, then it is the above . 0 mod 32 indicates nothing special and that the abi should be parsed as usual. Version and additions in the future will change the remainder, but libraries can start inferring from the non-word sized-ness that the result is special, and fail with a generic error if they don’t understand how to interpret the specialness and if they do understand it, do the library specific thing.

@fulldecent
Copy link
Contributor

Please explain the caveat "when it's not prefixed by zero" in the backwards compatibility section.

I am concerned what is the cost in practice of this EIP against deployed contract size. The contract size limit is at https://github.com/ethereum/EIPs/blob/master/EIPS/eip-170.md

Here is a contract I am familiar with https://github.com/0xcert/ethereum-erc721/blob/master/contracts/tokens/NFToken.sol and there are 19 require statements and 6 or so require statements. What is the additional cost, in contract size, if each require did also include a descriptive statement with RLP-encoded reason arguments when appropriate?

@federicobond
Copy link
Author

Note that we are already moving towards having rich error reporting in contracts. This EIPs is intended to avoid the wastefulness of storing full strings like “The amount needs to be positive” instead of short 4-byte error selectors with optional arguments.

I haven’t made any calculations on the increased storage requirements, I believe those belong in the discussion of REVERT itself.

@fulldecent
Copy link
Contributor

Actually, even the 4-byte selectors are unnecessary. I could explain how that works but it might be a premature optimization. That's why I'm asking for some metrics.

@ricmoo
Copy link
Contributor

ricmoo commented Jun 28, 2018

Keep in mind one advantage of the 4 byte selector is determining error conditions vs correct operation. All results from a contract are currently congruent to 0 mod 32 bytes. The 4 bytes extra provides a nice safe way to detect something isn’t normal. Just something to keep in mind. :)

@fulldecent
Copy link
Contributor

I hear the pain. It makes sense. Maybe the simple, and extensible way to do this is:

A normal asset transfer. Already backwards compatible with EIPs like ERC-721.

function transfer(address _to, uint256 _amount) throws(byte statusCode);

This is how Compound Finance and others already work.

function getOraclePrice(address asset, uint256 time) returns(uint256 price) throws(byte statusCode);

Here is how your batch transfer would work.

function batchTransfer(address asset[], uint256 quantities[], address recipient) 
    returns(uint256 timestamp) 
    throws(byte statusCode, uint256 failedTransferIndex);

And all of these support the obvious try-catch syntax:

contract BankAgent {
    function settleFunds() {
        uint timestamp;
        try {
            timestamp = batchTransfer(...);
        } catch (uint256 failedTransferIndex) {
            pauseContract(); // block further actions until settlement is possible
        }
    }
}

Here the try-catch only supports a single statement. The verbosity is justified by having an explicit code path for when a function call reverts. It is already much cleaner than using assembly.

Then the scope of this EIP is limited a programming convention. To assign semantics to these status codes we need something else like #1066.


If semantics (i.e. what exact message does MetaMask and other client software show end users) is important then we need to address the namespacing issue. And we should have buy-in from MetaMask. In contrast, the proposal in this comment is actionable without relying on anybody else.

Shadowfiend added a commit to keep-network/keep-core that referenced this issue May 10, 2019
The code was doing its own interpretive dance, but it now uses the
proper subbits of ABI decoding to interpret a returned error in a good
general way. The structure used is the one described at
ethereum/EIPs#838 (comment) .
Shadowfiend added a commit to keep-network/keep-core that referenced this issue May 10, 2019
The code was doing its own interpretive dance, but it now uses the
proper subbits of ABI decoding to interpret a returned error in a good
general way. The structure used is the one described at
ethereum/EIPs#838 (comment) .
@chriseth
Copy link
Contributor

This has been implemented in Solidity 0.8.4.

@ricmoo
Copy link
Contributor

ricmoo commented Apr 22, 2021

Can we get this added to the official EIPs branch so it is included in eips.ethereum.org? I'm not sure what it means if the initial draft is final. :)

@chriseth
Copy link
Contributor

Oh by the way: Solidity reserved the following selectors for future use:

  • 0x00000000
  • 0xffffffff

And those two currently cannot be defined in user code (which is maybe less relevant for the EIP):

  • Error(string)
  • Panic(uint256)

So maybe at least the first two could be added to the EIP.

@chriseth
Copy link
Contributor

Note that the way Solidity implemented EIP838 could be interpreted to deviate from the standard specified here. Specifically:

Any arguments for the error are RLP encoded in the same way as return values from functions. The exact format in which both the selector and the arguments are encoded is to be defined. The Solidity implementation mentioned above leaves room for expansion by prefixing the free-form string with uint256(0).

EIP 838 does not specify how data is encoded but suggests RLP. At the same time, it says "in the same way as return values from functions" - which is the case for the Solidity implementation: It uses ABI encoding. More details can be found in the Solidity documentation

@github-actions
Copy link

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Dec 19, 2021
@github-actions
Copy link

github-actions bot commented Jan 2, 2022

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@RenanSouza2
Copy link
Contributor

Hello @federicobond and everyone, Is there any PR with this EIP to officialize it? I think it is import since solidity adopted it.

I also posted this EIP in https://ethereum-magicians.org/t/eip-838-what-is-the-current-status/14671

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

No branches or pull requests