Propagate custom Solidity errors for eth_estimateGas JSON RPC funct…

…ion (#1922) * Return result.ReturnValue on estimate gas * Fixed TestEth_EstimateGas_Reverts * CR changes, the test case for the custom error message
0xPolygon · Sep 27, 2023 · 4b2770e · 4b2770e
1 parent 32047b4
commit 4b2770e
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 41 deletions.
diff --git a/jsonrpc/eth_endpoint.go b/jsonrpc/eth_endpoint.go
@@ -652,23 +652,29 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error
 	}
 
 	// Run the transaction with the specified gas value.
-	// Returns a status indicating if the transaction failed and the accompanying error
-	testTransaction := func(gas uint64, shouldOmitErr bool) (bool, error) {
+	// Returns a status indicating if the transaction failed, return value (data), and the accompanying error
+	testTransaction := func(gas uint64, shouldOmitErr bool) (bool, interface{}, error) {
+		var data interface{}
+
 		transaction.Gas = gas
 
 		result, applyErr := e.store.ApplyTxn(header, transaction, nil)
 
+		if result != nil {
+			data = []byte(hex.EncodeToString(result.ReturnValue))
+		}
+
 		if applyErr != nil {
 			// Check the application error.
 			// Gas apply errors are valid, and should be ignored
 			if isGasApplyError(applyErr) && shouldOmitErr {
 				// Specifying the transaction failed, but not providing an error
 				// is an indication that a valid error occurred due to low gas,
 				// which will increase the lower bound for the search
-				return true, nil
+				return true, data, nil
 			}
 
-			return true, applyErr
+			return true, data, applyErr
 		}
 
 		// Check if an out of gas error happened during EVM execution
@@ -677,30 +683,30 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error
 				// Specifying the transaction failed, but not providing an error
 				// is an indication that a valid error occurred due to low gas,
 				// which will increase the lower bound for the search
-				return true, nil
+				return true, data, nil
 			}
 
 			if isEVMRevertError(result.Err) {
 				// The EVM reverted during execution, attempt to extract the
 				// error message and return it
-				return true, constructErrorFromRevert(result)
+				return true, data, constructErrorFromRevert(result)
 			}
 
-			return true, result.Err
+			return true, data, result.Err
 		}
 
-		return false, nil
+		return false, nil, nil
 	}
 
 	// Start the binary search for the lowest possible gas price
 	for lowEnd < highEnd {
 		mid := lowEnd + ((highEnd - lowEnd) >> 1) // (lowEnd + highEnd) / 2 can overflow
 
-		failed, testErr := testTransaction(mid, true)
+		failed, retVal, testErr := testTransaction(mid, true)
 		if testErr != nil && !isEVMRevertError(testErr) {
 			// Reverts are ignored in the binary search, but are checked later on
 			// during the execution for the optimal gas limit found
-			return 0, testErr
+			return retVal, testErr
 		}
 
 		if failed {
@@ -713,10 +719,10 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error
 	}
 
 	// Check if the highEnd is a good value to make the transaction pass
-	failed, err := testTransaction(highEnd, false)
+	failed, retVal, err := testTransaction(highEnd, false)
 	if failed {
 		// The transaction shouldn't fail, for whatever reason, at highEnd
-		return 0, fmt.Errorf(
+		return retVal, fmt.Errorf(
 			"unable to apply transaction even for the highest gas limit %d: %w",
 			highEnd,
 			err,

diff --git a/jsonrpc/eth_state_test.go b/jsonrpc/eth_state_test.go
@@ -692,7 +692,7 @@ func TestEth_EstimateGas_GasLimit(t *testing.T) {
 				assert.ErrorIs(t, estimateErr, testCase.expectedError)
 
 				// Make sure the estimate is nullified
-				assert.Equal(t, 0, estimate)
+				assert.Equal(t, []byte{}, estimate)
 			} else {
 				// Make sure no errors occurred
 				assert.NoError(t, estimateErr)
@@ -705,42 +705,71 @@ func TestEth_EstimateGas_GasLimit(t *testing.T) {
 }
 
 func TestEth_EstimateGas_Reverts(t *testing.T) {
-	// Example revert data that has the string "revert reason" as the revert reason
-	exampleReturnData := "08c379a000000000000000000000000000000000000000000000000000000000000000" +
-		"20000000000000000000000000000000000000000000000000000000000000000d72657665727420726561736f6e" +
-		"00000000000000000000000000000000000000"
-	rawReturnData, err := hex.DecodeHex(exampleReturnData)
-	assert.NoError(t, err)
-
-	revertReason := errors.New("revert reason")
+	testTable := []struct {
+		name              string
+		exampleReturnData string
+		revertReason      error
+		err               error
+	}{
+		{
+			"revert with string message 'revert reason'",
+			// Example revert data that has the string "revert reason" as the revert reason
+			"08c379a000000000000000000000000000000000000000000000000000000000000000" +
+				"20000000000000000000000000000000000000000000000000000000000000000d72657665727420726561736f6e" +
+				"00000000000000000000000000000000000000",
+			errors.New("revert reason"),
+			runtime.ErrExecutionReverted,
+		},
+		{
+			"revert with custom solidity error",
+			// First 4 bytes of ABI encoded custom solidity error PermissionedContractAccessDenied()
+			"8cd01c38",
+			errors.New(""), // revert reason in this case doesn't exist, reason is decoded in response data
+			runtime.ErrExecutionReverted,
+		},
+		{
+			"revert with empty response",
+			"",
+			errors.New(""),
+			runtime.ErrExecutionReverted,
+		},
+	}
 
 	store := getExampleStore()
 	ethEndpoint := newTestEthEndpoint(store)
 
-	// We want to simulate an EVM revert here
-	store.applyTxnHook = func(
-		header *types.Header,
-		txn *types.Transaction,
-	) (*runtime.ExecutionResult, error) {
-		return &runtime.ExecutionResult{
-			ReturnValue: rawReturnData,
-			Err:         runtime.ErrExecutionReverted,
-		}, nil
-	}
+	for _, test := range testTable {
+		rawReturnData, err := hex.DecodeHex(test.exampleReturnData)
+		assert.NoError(t, err)
+
+		// We want to simulate an EVM revert here
+		store.applyTxnHook = func(
+			header *types.Header,
+			txn *types.Transaction,
+		) (*runtime.ExecutionResult, error) {
+			return &runtime.ExecutionResult{
+				ReturnValue: rawReturnData,
+				Err:         runtime.ErrExecutionReverted,
+			}, nil
+		}
+
+		// Run the estimation
+		estimate, estimateErr := ethEndpoint.EstimateGas(
+			constructMockTx(nil, nil),
+			nil,
+		)
 
-	// Run the estimation
-	estimate, estimateErr := ethEndpoint.EstimateGas(
-		constructMockTx(nil, nil),
-		nil,
-	)
+		responseData, ok := estimate.([]byte)
+		assert.True(t, ok)
 
-	assert.Equal(t, 0, estimate)
+		assert.Equal(t, test.exampleReturnData, string(responseData))
 
-	// Make sure the EVM revert message is contained
-	assert.ErrorIs(t, estimateErr, runtime.ErrExecutionReverted)
+		// Make sure the EVM revert message is contained
+		assert.ErrorIs(t, estimateErr, runtime.ErrExecutionReverted)
 
-	// Make sure the EVM revert reason is contained
-	assert.ErrorAs(t, estimateErr, &revertReason)
+		// Make sure the EVM revert reason is contained
+		assert.ErrorAs(t, estimateErr, &test.revertReason)
+	}
 }
 
 func TestEth_EstimateGas_Errors(t *testing.T) {