internal/ethapi: return revert reason for eth_call (#21083)

* internal/ethapi: return revert reason for eth_call

* internal/ethapi: moved revert reason logic to doCall

* accounts/abi/bind/backends: added revert reason logic to simulated backend

* internal/ethapi: fixed linting error

* internal/ethapi: check if require reason can be unpacked

* internal/ethapi: better error logic

* internal/ethapi: simplify logic

* internal/ethapi: return vmError()

* internal/ethapi: move handling of revert out of docall

* graphql: removed revert logic until spec change

* rpc: internal/ethapi: added custom error types

* graphql: use returndata instead of return

Return() checks if there is an error. If an error is found, we return nil.
For most use cases it can be beneficial to return the output even if there
was an error. This code should be changed anyway once the spec supports
error reasons in graphql responses

* accounts/abi/bind/backends: added tests for revert reason

* internal/ethapi: add errorCode to revert error

* internal/ethapi: add errorCode of 3 to revertError

* internal/ethapi: unified estimateGasErrors, simplified logic

* internal/ethapi: unified handling of errors in DoEstimateGas

* rpc: print error data field

* accounts/abi/bind/backends: unify simulatedBackend and RPC

* internal/ethapi: added binary data to revertError data

* internal/ethapi: refactored unpacking logic into newRevertError

* accounts/abi/bind/backends: fix EstimateGas

* accounts, console, internal, rpc: minor error interface cleanups

* Revert "accounts, console, internal, rpc: minor error interface cleanups"

This reverts commit 2d3ef53c5304e429a04983210a417c1f4e0dafb7.

* re-apply the good parts of 2d3ef53c53

* rpc: add test for returning server error data from client
Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>

accounts/abi/bind/backends/simulated.go

@@ -28,6 +28,7 @@ import (
 	"github.com/ethereum/go-ethereum/accounts/abi"
 	"github.com/ethereum/go-ethereum/accounts/abi/bind"
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/common/math"
 	"github.com/ethereum/go-ethereum/consensus/ethash"
 	"github.com/ethereum/go-ethereum/core"
@@ -344,6 +345,36 @@ func (b *SimulatedBackend) PendingCodeAt(ctx context.Context, contract common.Ad
 	return b.pendingState.GetCode(contract), nil
 }
 
+func newRevertError(result *core.ExecutionResult) *revertError {
+	reason, errUnpack := abi.UnpackRevert(result.Revert())
+	err := errors.New("execution reverted")
+	if errUnpack == nil {
+		err = fmt.Errorf("execution reverted: %v", reason)
+	}
+	return &revertError{
+		error:  err,
+		reason: hexutil.Encode(result.Revert()),
+	}
+}
+
+// revertError is an API error that encompassas an EVM revertal with JSON error
+// code and a binary data blob.
+type revertError struct {
+	error
+	reason string // revert reason hex encoded
+}
+
+// ErrorCode returns the JSON error code for a revertal.
+// See: https://github.com/ethereum/wiki/wiki/JSON-RPC-Error-Codes-Improvement-Proposal
+func (e *revertError) ErrorCode() int {
+	return 3
+}
+
+// ErrorData returns the hex encoded revert reason.
+func (e *revertError) ErrorData() interface{} {
+	return e.reason
+}
+
 // CallContract executes a contract call.
 func (b *SimulatedBackend) CallContract(ctx context.Context, call ethereum.CallMsg, blockNumber *big.Int) ([]byte, error) {
 	b.mu.Lock()
@@ -360,7 +391,11 @@ func (b *SimulatedBackend) CallContract(ctx context.Context, call ethereum.CallM
 	if err != nil {
 		return nil, err
 	}
-	return res.Return(), nil
+	// If the result contains a revert reason, try to unpack and return it.
+	if len(res.Revert()) > 0 {
+		return nil, newRevertError(res)
+	}
+	return res.Return(), res.Err
 }
 
 // PendingCallContract executes a contract call on the pending state.
@@ -373,7 +408,11 @@ func (b *SimulatedBackend) PendingCallContract(ctx context.Context, call ethereu
 	if err != nil {
 		return nil, err
 	}
-	return res.Return(), nil
+	// If the result contains a revert reason, try to unpack and return it.
+	if len(res.Revert()) > 0 {
+		return nil, newRevertError(res)
+	}
+	return res.Return(), res.Err
 }
 
 // PendingNonceAt implements PendingStateReader.PendingNonceAt, retrieving
@@ -472,16 +511,10 @@ func (b *SimulatedBackend) EstimateGas(ctx context.Context, call ethereum.CallMs
 		}
 		if failed {
 			if result != nil && result.Err != vm.ErrOutOfGas {
-				errMsg := fmt.Sprintf("always failing transaction (%v)", result.Err)
 				if len(result.Revert()) > 0 {
-					ret, err := abi.UnpackRevert(result.Revert())
-					if err != nil {
-						errMsg += fmt.Sprintf(" (%#x)", result.Revert())
-					} else {
-						errMsg += fmt.Sprintf(" (%s)", ret)
-					}
+					return 0, newRevertError(result)
 				}
-				return 0, errors.New(errMsg)
+				return 0, result.Err
 			}
 			// Otherwise, the specified gas cap is too low
 			return 0, fmt.Errorf("gas required exceeds allowance (%d)", cap)

console/bridge.go

@@ -413,9 +413,7 @@ func (b *bridge) Send(call jsre.Call) (goja.Value, error) {
 		resp.Set("id", req.ID)
 
 		var result json.RawMessage
-		err = b.client.Call(&result, req.Method, req.Params...)
-		switch err := err.(type) {
-		case nil:
+		if err = b.client.Call(&result, req.Method, req.Params...); err == nil {
 			if result == nil {
 				// Special case null because it is decoded as an empty
 				// raw message for some reason.
@@ -428,19 +426,24 @@ func (b *bridge) Send(call jsre.Call) (goja.Value, error) {
 				}
 				resultVal, err := parse(goja.Null(), call.VM.ToValue(string(result)))
 				if err != nil {
-					setError(resp, -32603, err.Error())
+					setError(resp, -32603, err.Error(), nil)
 				} else {
 					resp.Set("result", resultVal)
 				}
 			}
-		case rpc.Error:
-			setError(resp, err.ErrorCode(), err.Error())
-		default:
-			setError(resp, -32603, err.Error())
+		} else {
+			code := -32603
+			var data interface{}
+			if err, ok := err.(rpc.Error); ok {
+				code = err.ErrorCode()
+			}
+			if err, ok := err.(rpc.DataError); ok {
+				data = err.ErrorData()
+			}
+			setError(resp, code, err.Error(), data)
 		}
 		resps = append(resps, resp)
 	}
-
 	// Return the responses either to the callback (if supplied)
 	// or directly as the return value.
 	var result goja.Value
@@ -456,8 +459,14 @@ func (b *bridge) Send(call jsre.Call) (goja.Value, error) {
 	return result, nil
 }
 
-func setError(resp *goja.Object, code int, msg string) {
-	resp.Set("error", map[string]interface{}{"code": code, "message": msg})
+func setError(resp *goja.Object, code int, msg string, data interface{}) {
+	err := make(map[string]interface{})
+	err["code"] = code
+	err["message"] = msg
+	if data != nil {
+		err["data"] = data
+	}
+	resp.Set("error", err)
 }
 
 // isNumber returns true if input value is a JS number.

graphql/graphql.go

@@ -811,8 +811,9 @@ func (b *Block) Call(ctx context.Context, args struct {
 	if result.Failed() {
 		status = 0
 	}
+
 	return &CallResult{
-		data:    result.Return(),
+		data:    result.ReturnData,
 		gasUsed: hexutil.Uint64(result.UsedGas),
 		status:  status,
 	}, nil
@@ -880,8 +881,9 @@ func (p *Pending) Call(ctx context.Context, args struct {
 	if result.Failed() {
 		status = 0
 	}
+
 	return &CallResult{
-		data:    result.Return(),
+		data:    result.ReturnData,
 		gasUsed: hexutil.Uint64(result.UsedGas),
 		status:  status,
 	}, nil

internal/ethapi/api.go

@@ -864,6 +864,36 @@ func DoCall(ctx context.Context, b Backend, args CallArgs, blockNrOrHash rpc.Blo
 	return result, err
 }
 
+func newRevertError(result *core.ExecutionResult) *revertError {
+	reason, errUnpack := abi.UnpackRevert(result.Revert())
+	err := errors.New("execution reverted")
+	if errUnpack == nil {
+		err = fmt.Errorf("execution reverted: %v", reason)
+	}
+	return &revertError{
+		error:  err,
+		reason: hexutil.Encode(result.Revert()),
+	}
+}
+
+// revertError is an API error that encompassas an EVM revertal with JSON error
+// code and a binary data blob.
+type revertError struct {
+	error
+	reason string // revert reason hex encoded
+}
+
+// ErrorCode returns the JSON error code for a revertal.
+// See: https://github.com/ethereum/wiki/wiki/JSON-RPC-Error-Codes-Improvement-Proposal
+func (e *revertError) ErrorCode() int {
+	return 3
+}
+
+// ErrorData returns the hex encoded revert reason.
+func (e *revertError) ErrorData() interface{} {
+	return e.reason
+}
+
 // Call executes the given transaction on the state for the given block number.
 //
 // Additionally, the caller can specify a batch of contract for fields overriding.
@@ -879,24 +909,11 @@ func (s *PublicBlockChainAPI) Call(ctx context.Context, args CallArgs, blockNrOr
 	if err != nil {
 		return nil, err
 	}
-	return result.Return(), nil
-}
-
-type estimateGasError struct {
-	error  string // Concrete error type if it's failed to estimate gas usage
-	vmerr  error  // Additional field, it's non-nil if the given transaction is invalid
-	revert string // Additional field, it's non-empty if the transaction is reverted and reason is provided
-}
-
-func (e estimateGasError) Error() string {
-	errMsg := e.error
-	if e.vmerr != nil {
-		errMsg += fmt.Sprintf(" (%v)", e.vmerr)
-	}
-	if e.revert != "" {
-		errMsg += fmt.Sprintf(" (%s)", e.revert)
+	// If the result contains a revert reason, try to unpack and return it.
+	if len(result.Revert()) > 0 {
+		return nil, newRevertError(result)
 	}
-	return errMsg
+	return result.Return(), result.Err
 }
 
 func DoEstimateGas(ctx context.Context, b Backend, args CallArgs, blockNrOrHash rpc.BlockNumberOrHash, gasCap *big.Int) (hexutil.Uint64, error) {
@@ -991,23 +1008,13 @@ func DoEstimateGas(ctx context.Context, b Backend, args CallArgs, blockNrOrHash
 		}
 		if failed {
 			if result != nil && result.Err != vm.ErrOutOfGas {
-				var revert string
 				if len(result.Revert()) > 0 {
-					ret, err := abi.UnpackRevert(result.Revert())
-					if err != nil {
-						revert = hexutil.Encode(result.Revert())
-					} else {
-						revert = ret
-					}
-				}
-				return 0, estimateGasError{
-					error:  "always failing transaction",
-					vmerr:  result.Err,
-					revert: revert,
+					return 0, newRevertError(result)
 				}
+				return 0, result.Err
 			}
 			// Otherwise, the specified gas cap is too low
-			return 0, estimateGasError{error: fmt.Sprintf("gas required exceeds allowance (%d)", cap)}
+			return 0, fmt.Errorf("gas required exceeds allowance (%d)", cap)
 		}
 	}
 	return hexutil.Uint64(hi), nil

rpc/client_test.go

@@ -66,6 +66,33 @@ func TestClientResponseType(t *testing.T) {
 	}
 }
 
+// This test checks that server-returned errors with code and data come out of Client.Call.
+func TestClientErrorData(t *testing.T) {
+	server := newTestServer()
+	defer server.Stop()
+	client := DialInProc(server)
+	defer client.Close()
+
+	var resp interface{}
+	err := client.Call(&resp, "test_returnError")
+	if err == nil {
+		t.Fatal("expected error")
+	}
+
+	// Check code.
+	if e, ok := err.(Error); !ok {
+		t.Fatalf("client did not return rpc.Error, got %#v", e)
+	} else if e.ErrorCode() != (testError{}.ErrorCode()) {
+		t.Fatalf("wrong error code %d, want %d", e.ErrorCode(), testError{}.ErrorCode())
+	}
+	// Check data.
+	if e, ok := err.(DataError); !ok {
+		t.Fatalf("client did not return rpc.DataError, got %#v", e)
+	} else if e.ErrorData() != (testError{}.ErrorData()) {
+		t.Fatalf("wrong error data %#v, want %#v", e.ErrorData(), testError{}.ErrorData())
+	}
+}
+
 func TestClientBatchRequest(t *testing.T) {
 	server := newTestServer()
 	defer server.Stop()

rpc/errors.go

@@ -18,6 +18,15 @@ package rpc
 
 import "fmt"
 
+var (
+	_ Error = new(methodNotFoundError)
+	_ Error = new(subscriptionNotFoundError)
+	_ Error = new(parseError)
+	_ Error = new(invalidRequestError)
+	_ Error = new(invalidMessageError)
+	_ Error = new(invalidParamsError)
+)
+
 const defaultErrorCode = -32000
 
 type methodNotFoundError struct{ method string }

rpc/handler.go

@@ -296,10 +296,16 @@ func (h *handler) handleCallMsg(ctx *callProc, msg *jsonrpcMessage) *jsonrpcMess
 		return nil
 	case msg.isCall():
 		resp := h.handleCall(ctx, msg)
+		var ctx []interface{}
+		ctx = append(ctx, "reqid", idForLog{msg.ID}, "t", time.Since(start))
 		if resp.Error != nil {
-			h.log.Warn("Served "+msg.Method, "reqid", idForLog{msg.ID}, "t", time.Since(start), "err", resp.Error.Message)
+			ctx = append(ctx, "err", resp.Error.Message)
+			if resp.Error.Data != nil {
+				ctx = append(ctx, "errdata", resp.Error.Data)
+			}
+			h.log.Warn("Served "+msg.Method, ctx...)
 		} else {
-			h.log.Debug("Served "+msg.Method, "reqid", idForLog{msg.ID}, "t", time.Since(start))
+			h.log.Debug("Served "+msg.Method, ctx...)
 		}
 		return resp
 	case msg.hasValidID():

rpc/json.go

@@ -115,6 +115,10 @@ func errorMessage(err error) *jsonrpcMessage {
 	if ok {
 		msg.Error.Code = ec.ErrorCode()
 	}
+	de, ok := err.(DataError)
+	if ok {
+		msg.Error.Data = de.ErrorData()
+	}
 	return msg
 }
 
@@ -135,6 +139,10 @@ func (err *jsonError) ErrorCode() int {
 	return err.Code
 }
 
+func (err *jsonError) ErrorData() interface{} {
+	return err.Data
+}
+
 // Conn is a subset of the methods of net.Conn which are sufficient for ServerCodec.
 type Conn interface {
 	io.ReadWriteCloser

rpc/server_test.go

@@ -45,7 +45,7 @@ func TestServerRegisterName(t *testing.T) {
 		t.Fatalf("Expected service calc to be registered")
 	}
 
-	wantCallbacks := 8
+	wantCallbacks := 9
 	if len(svc.callbacks) != wantCallbacks {
 		t.Errorf("Expected %d callbacks for service 'service', got %d", wantCallbacks, len(svc.callbacks))
 	}

rpc/testservice_test.go

@@ -63,6 +63,12 @@ type echoResult struct {
 	Args   *echoArgs
 }
 
+type testError struct{}
+
+func (testError) Error() string          { return "testError" }
+func (testError) ErrorCode() int         { return 444 }
+func (testError) ErrorData() interface{} { return "testError data" }
+
 func (s *testService) NoArgsRets() {}
 
 func (s *testService) Echo(str string, i int, args *echoArgs) echoResult {
@@ -99,6 +105,10 @@ func (s *testService) InvalidRets3() (string, string, error) {
 	return "", "", nil
 }
 
+func (s *testService) ReturnError() error {
+	return testError{}
+}
+
 func (s *testService) CallMeBack(ctx context.Context, method string, args []interface{}) (interface{}, error) {
 	c, ok := ClientFromContext(ctx)
 	if !ok {

rpc/types.go

@@ -41,6 +41,12 @@ type Error interface {
 	ErrorCode() int // returns the code
 }
 
+// A DataError contains some data in addition to the error message.
+type DataError interface {
+	Error() string          // returns the message
+	ErrorData() interface{} // returns the error data
+}
+
 // ServerCodec implements reading, parsing and writing RPC messages for the server side of
 // a RPC session. Implementations must be go-routine safe since the codec can be called in
 // multiple go-routines concurrently.