fix(eth): present revert "data" as plain bytes

decode the cbor return value for reverts and present that, as is expected by
Ethereum tooling

Author: rvagg

api/api_errors.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/filecoin-project/go-jsonrpc"
 	"github.com/filecoin-project/go-state-types/abi"
+	"github.com/filecoin-project/go-state-types/exitcode"
 )
 
 var invalidExecutionRevertedMsg = xerrors.New("invalid execution reverted error")
@@ -160,10 +161,10 @@ func (e *ErrExecutionReverted) ToJSONRPCError() (jsonrpc.JSONRPCError, error) {
 }
 
 // NewErrExecutionReverted creates a new ErrExecutionReverted with the given reason.
-func NewErrExecutionReverted(reason string) *ErrExecutionReverted {
+func NewErrExecutionReverted(exitCode exitcode.ExitCode, error, reason string, data []byte) *ErrExecutionReverted {
 	return &ErrExecutionReverted{
-		Message: "execution reverted",
-		Data:    reason,
+		Message: fmt.Sprintf("message execution failed (exit=[%s], revert reason=[%s], vm error=[%s])", exitCode, reason, error),
+		Data:    fmt.Sprintf("0x%x", data),
 	}
 }
 

itests/fevm_test.go

@@ -1031,10 +1031,10 @@ func TestFEVMErrorParsing(t *testing.T) {
 	require.NoError(t, err)
 	customError := ethtypes.EthBytes(kit.CalcFuncSignature("CustomError()")).String()
 	for sig, expected := range map[string]string{
-		"failRevertEmpty()":  "none",
-		"failRevertReason()": "Error(my reason)",
-		"failAssert()":       "Assert()",
-		"failDivZero()":      "DivideByZero()",
+		"failRevertEmpty()":  "0x",
+		"failRevertReason()": fmt.Sprintf("%x", []byte("my reason")),
+		"failAssert()":       "0x4e487b710000000000000000000000000000000000000000000000000000000000000001", // Assert()
+		"failDivZero()":      "0x4e487b710000000000000000000000000000000000000000000000000000000000000012", // DivideByZero()
 		"failCustom()":       customError,
 	} {
 		sig := sig
@@ -1602,10 +1602,10 @@ func TestEthCall(t *testing.T) {
 
 		var dataErr *api.ErrExecutionReverted
 		require.ErrorAs(t, err, &dataErr, "Expected error to be ErrExecutionReverted")
-		require.Contains(t, dataErr.Message, "execution reverted", "Expected 'execution reverted' message")
+		require.Regexp(t, `message execution failed [\s\S]+\[DivideByZero\(\)\]`, dataErr.Message)
 
 		// Get the error data
-		require.Equal(t, dataErr.Data, "DivideByZero()", "Expected error data to contain 'DivideByZero()'")
+		require.Equal(t, dataErr.Data, "0x4e487b710000000000000000000000000000000000000000000000000000000000000012", "Expected error data to contain 'DivideByZero()'")
 	})
 }
 
@@ -1627,12 +1627,12 @@ func TestEthEstimateGas(t *testing.T) {
 		name           string
 		function       string
 		expectedError  string
-		expectedErrMsg string
+		expectedErrMsg interface{}
 	}{
-		{"DivideByZero", "failDivZero()", "DivideByZero()", "execution reverted"},
-		{"Assert", "failAssert()", "Assert()", "execution reverted"},
-		{"RevertWithReason", "failRevertReason()", "Error(my reason)", "execution reverted"},
-		{"RevertEmpty", "failRevertEmpty()", "", "execution reverted"},
+		{"DivideByZero", "failDivZero()", "0x4e487b710000000000000000000000000000000000000000000000000000000000000012", `message execution failed [\s\S]+\[DivideByZero\(\)\]`},
+		{"Assert", "failAssert()", "0x4e487b710000000000000000000000000000000000000000000000000000000000000001", `message execution failed [\s\S]+\[Assert\(\)\]`},
+		{"RevertWithReason", "failRevertReason()", fmt.Sprintf("%x", []byte("my reason")), `message execution failed [\s\S]+\[Error\(my reason\)\]`},
+		{"RevertEmpty", "failRevertEmpty()", "0x", `message execution failed [\s\S]+\[none\]`},
 	}
 
 	for _, tc := range testCases {
@@ -1654,8 +1654,8 @@ func TestEthEstimateGas(t *testing.T) {
 				require.Error(t, err)
 				var dataErr *api.ErrExecutionReverted
 				require.ErrorAs(t, err, &dataErr, "Expected error to be ErrExecutionReverted")
-				require.Equal(t, tc.expectedErrMsg, dataErr.Message)
-				require.Contains(t, tc.expectedError, dataErr.Data)
+				require.Regexp(t, tc.expectedErrMsg, dataErr.Message)
+				require.Contains(t, dataErr.Data, tc.expectedError)
 			}
 		})
 	}

node/impl/full/eth.go

@@ -1496,9 +1496,16 @@ func (a *EthModule) applyMessage(ctx context.Context, msg *types.Message, tsk ty
 	}
 
 	if res.MsgRct.ExitCode.IsError() {
-		return nil, api.NewErrExecutionReverted(
-			parseEthRevert(res.MsgRct.Return),
-		)
+		reason := "none"
+		var cbytes abi.CborBytes
+		if err := cbytes.UnmarshalCBOR(bytes.NewReader(res.MsgRct.Return)); err != nil {
+			log.Warnw("failed to unmarshal cbor bytes from message receipt return", "error", err)
+			reason = "ERROR: revert reason is not cbor encoded bytes"
+		} // else leave as empty bytes
+		if len(cbytes) > 0 {
+			reason = parseEthRevert(cbytes)
+		}
+		return nil, api.NewErrExecutionReverted(res.MsgRct.ExitCode, reason, res.Error, cbytes)
 	}
 
 	return res, nil

node/impl/full/eth_utils.go

@@ -331,62 +331,52 @@ var panicErrorCodes = map[uint64]string{
 //
 // See https://docs.soliditylang.org/en/latest/control-structures.html#panic-via-assert-and-error-via-require
 func parseEthRevert(ret []byte) string {
-	if len(ret) == 0 {
-		return "none"
-	}
-	var cbytes abi.CborBytes
-	if err := cbytes.UnmarshalCBOR(bytes.NewReader(ret)); err != nil {
-		return "ERROR: revert reason is not cbor encoded bytes"
-	}
-	if len(cbytes) == 0 {
-		return "none"
-	}
 	// If it's not long enough to contain an ABI encoded response, return immediately.
-	if len(cbytes) < 4+32 {
-		return ethtypes.EthBytes(cbytes).String()
+	if len(ret) < 4+32 {
+		return ethtypes.EthBytes(ret).String()
 	}
-	switch string(cbytes[:4]) {
+	switch string(ret[:4]) {
 	case panicFunctionSelector:
-		cbytes := cbytes[4 : 4+32]
+		ret := ret[4 : 4+32]
 		// Read the and check the code.
-		code, err := ethtypes.EthUint64FromBytes(cbytes)
+		code, err := ethtypes.EthUint64FromBytes(ret)
 		if err != nil {
 			// If it's too big, just return the raw value.
-			codeInt := big.PositiveFromUnsignedBytes(cbytes)
+			codeInt := big.PositiveFromUnsignedBytes(ret)
 			return fmt.Sprintf("Panic(%s)", ethtypes.EthBigInt(codeInt).String())
 		}
 		if s, ok := panicErrorCodes[uint64(code)]; ok {
 			return s
 		}
 		return fmt.Sprintf("Panic(0x%x)", code)
 	case errorFunctionSelector:
-		cbytes := cbytes[4:]
-		cbytesLen := ethtypes.EthUint64(len(cbytes))
+		ret := ret[4:]
+		retLen := ethtypes.EthUint64(len(ret))
 		// Read the and check the offset.
-		offset, err := ethtypes.EthUint64FromBytes(cbytes[:32])
+		offset, err := ethtypes.EthUint64FromBytes(ret[:32])
 		if err != nil {
 			break
 		}
-		if cbytesLen < offset {
+		if retLen < offset {
 			break
 		}
 
 		// Read and check the length.
-		if cbytesLen-offset < 32 {
+		if retLen-offset < 32 {
 			break
 		}
 		start := offset + 32
-		length, err := ethtypes.EthUint64FromBytes(cbytes[offset : offset+32])
+		length, err := ethtypes.EthUint64FromBytes(ret[offset : offset+32])
 		if err != nil {
 			break
 		}
-		if cbytesLen-start < length {
+		if retLen-start < length {
 			break
 		}
 		// Slice the error message.
-		return fmt.Sprintf("Error(%s)", cbytes[start:start+length])
+		return fmt.Sprintf("Error(%s)", ret[start:start+length])
 	}
-	return ethtypes.EthBytes(cbytes).String()
+	return ethtypes.EthBytes(ret).String()
 }
 
 // lookupEthAddress makes its best effort at finding the Ethereum address for a