ethereum / execution-apis

Collection of APIs provided by Ethereum execution layer clients
Creative Commons Zero v1.0 Universal
952 stars 378 forks source link

`eth_call` underspecified return error message #463

Open emlautarom1 opened 1 year ago

emlautarom1 commented 1 year ago

The eth_call method can result in a execution error when, for example, a contract uses a revert operation. The current spec does not define how should the return error look like, making it hard for users to parse the execution failure cause.

The following screenshot perfectly describes the current situation for users:

E5PEFfrXoAAjaGU

Source: https://twitter.com/smpalladino/status/1410686918296313869/photo/1.

It would be great if all clients could agree in a single, standardized format for error messages.

MicahZoltu commented 12 months ago

Historically, JSON-RPC errors were reserved for protocol errors and not applied to EVM execution errors. If we want to maintain this pattern, then the JSON-RPC response would be a success with something in the result field. I generally think we should continue following this pattern as upcoming API feature additions like eth_multicall may have a subset of calls that revert and some that are a success so from the protocol level, a success should be returned.

As to what to put in the result field of the JSON-RPC response, I don't think the Ethereum client (JSON-RPC server) should try to do any sort of revert byte decoding. The JSON-RPC server doesn't have enough context to do this correctly so at best it would just be making educated guesses. The client (e.g., web app) has far more context and is in a much better position to properly decode the bytes.

It does seem like there needs to be some way to indicate that the result was erroneous, and prepending some ASCII text to a hex encoded byte array feels like just about the worst way to achieve this. An ideal solution, if we were building something from the ground up, would be to have the JSON-RPC result be an object like { success: false, reason: 'Execution Reverted', data: '0x...' }.

This would allow clients to discriminate on the success and if we want to add other failure modes in the future we can include additional strings to a union of possible values for the reason field which can also be discriminated on.

emlautarom1 commented 12 months ago

It does seem like there needs to be some way to indicate that the result was erroneous, and prepending some ASCII text to a hex encoded byte array feels like just about the worst way to achieve this.

100% agree since it forces clients to perform prefix/regex checks.

I don't think the Ethereum client (JSON-RPC server) should try to do any sort of revert byte decoding. The JSON-RPC server doesn't have enough context to do this correctly so at best it would just be making educated guesses.

Servers can decode information from these error messages using well defined formats (see: https://docs.soliditylang.org/en/v0.8.22/control-structures.html#revert). Still, these formats are subject to change, so I think it should be considered an optional feature, maybe by including a message field in case of failure.

For a failure case, the response would look something like:

{
    "success": false,
    "reason": "Execution Reverted",
    "message": "Contract check failed", // optional on a per-client basis (not enforced by spec)
    "data": "0x..."
}

while on success, it would look like:

{
    "success": true,
    "gas": "0x123456"
}
mfw78 commented 12 months ago

This is a great initiative to standardise this part of the JSON-RPC spec, and has been the source of many headaches.


while on success, it would look like:

```js
{
    "success": true,
    "gas": "0x123456"
}

Just confirming on the success case that there would also be data, such that:

{
    "success": true,
    "gas": "0x123456",
    "data": "0x..."
}
s1na commented 11 months ago

I agree it'd be good to standardize this between clients, and not only this. The way I'd approach this is to add an error code for "execution reverted" and let clients flexible on the actual wording. But agree with adding a separate field for the decoded revert message.

We can take this further by defining an error code for all failures that can happen as part of EVM execution, e.g. stack over/underflow. All clients necessarily will share the same failures because this is consensus-critical code. Only their error messages could be different. Adding an error code would allow for different messages (e.g. some clients might wish to add context like opcode information).

For context, this is roughly the list of all these faults:

const (
    VMErrorCodeOutOfGas = 1 + iota
    VMErrorCodeCodeStoreOutOfGas
    VMErrorCodeDepth
    VMErrorCodeInsufficientBalance
    VMErrorCodeContractAddressCollision
    VMErrorCodeExecutionReverted
    VMErrorCodeMaxInitCodeSizeExceeded
    VMErrorCodeMaxCodeSizeExceeded
    VMErrorCodeInvalidJump
    VMErrorCodeWriteProtection
    VMErrorCodeReturnDataOutOfBounds
    VMErrorCodeGasUintOverflow
    VMErrorCodeInvalidCode
    VMErrorCodeNonceUintOverflow
    VMErrorCodeStackUnderflow
    VMErrorCodeStackOverflow
    VMErrorCodeInvalidOpCode
 )
MicahZoltu commented 11 months ago

We potentially have the opportunity to get this "right" with eth_multicall (https://github.com/ethereum/execution-apis/pull/383), if we decide we want to delay release of that feature until we settle on how EVM errors are handled. In the case of eth_multicall, it would not be a JSON-RPC error because a subset of the calls may fail while others succeed and this would be a successful JSON-RPC request/response just as a valid block can have some reverting transactions.

If we decide to implement this in eth_multicall, then I think the right way to do it would be to have the call result (for each call in the multicall) be an object like:

{
    success: true,
    data: `0x${string}`,
} | {
    success: false,
    error_code: Omit<Errors, RevertedError>,
    message: string,
} | {
    success: false,
    error_code: RevertedError,
    message: string,
    data?: `0x${string}`,
}

In the success case, we would just include whatever the call returndata was.

In the failure case, we would have an error_code that contains the specific VM error that occurred (from the list provided by Sina) and if the error was a revert then we would put the revertdata in the data field. The message field would be a freeform string where the client can put any additional information they like such as stack traces, etc.

My preference would be to have the Errors type be a string union, so that it is easy for a developer to read, rather than having numeric codes that then have to be looked up in a table on the client side of the connection.


Note: The list provided by @s1na above doesn't include all possible per-transaction errors, like invalid signature and contract-caller. For eth_multicall, I believe we are ignoring both of these so it doesn't matter, but we may want to make sure there are no other ways an individual transaction can fail other than the list provided.

ryanschneider commented 10 months ago

I'm 100% on board with this, but would ask that we "objectize" success and revert rather than keying off the boolean success value to decide what fields are valid.

So a successful call would have a success object value:

{
  // standard JSONRPC id etc elided
  "result": {
     "success":  {
        "data": "0x..",
        "gas": "0x.."
     }
   }
}

While a failed call would contain a revert like:

{ 
  // standard JSONRPC id etc elided
  "result" { 
    "revert": {
        "code": ...
        "reason": ...
    }
}

(I'm not advocating for any specific keys in the revert above since I'm not a domain expert on what data people need when a call reverts, just the overall format).

This allows much cleaner schemas (both in the OpenRPC spec and in client and user code) since success and revert objects can be modeled separately instead of their fields intermingling.

edit: and in the case of multicall result would be an array of [{ "success": ... }, { "revert": ... }] objects.