Append ABI-decoded revert reason to message field in error responses (h…

…yperledger#5706) Signed-off-by: Matthew Whitehead <[email protected]> Signed-off-by: Simon Dudley <[email protected]> Signed-off-by: garyschulte <[email protected]>
garyschulte · Aug 28, 2023 · 08e0eb2 · 08e0eb2
1 parent 4539d41
commit 08e0eb2
Show file tree

Hide file tree

Showing 7 changed files with 272 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,8 @@
 
 ### Breaking Changes
 
+- Add ABI-decoded revert reason to `eth_call` and `eth_estimateGas` responses [#5705](https://github.com/hyperledger/besu/issues/5705)
+
 ### Additions and Improvements
 
 ### Bug Fixes

diff --git a/...s/src/test/java/org/hyperledger/besu/tests/acceptance/privacy/PrivCallAcceptanceTest.java b/...s/src/test/java/org/hyperledger/besu/tests/acceptance/privacy/PrivCallAcceptanceTest.java
@@ -139,7 +139,7 @@ public void mustRevertWithRevertReason() throws Exception {
         privCall(privacyGroupId, revertReasonContract, false, false, true);
 
     EthCall resp = priv_call.send();
-    assertThat(resp.getRevertReason()).isEqualTo("Execution reverted");
+    assertThat(resp.getRevertReason()).isEqualTo("Execution reverted: RevertReason");
 
     byte[] bytes = Hex.decode(resp.getError().getData().substring(3, 203));
     String revertMessage =

diff --git a/ethereum/api/build.gradle b/ethereum/api/build.gradle
@@ -72,6 +72,12 @@ dependencies {
   implementation 'org.bouncycastle:bcprov-jdk18on'
   implementation 'org.springframework.security:spring-security-crypto'
   implementation 'org.xerial.snappy:snappy-java'
+  implementation 'com.google.guava:guava'
+  implementation 'io.vertx:vertx-core'
+  implementation 'com.fasterxml.jackson.core:jackson-databind'
+  implementation 'io.tmio:tuweni-bytes'
+  implementation 'io.tmio:tuweni-units'
+  implementation 'org.web3j:abi'
 
   annotationProcessor "org.immutables:value"
   implementation "org.immutables:value-annotations"

diff --git a/...c/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcError.java b/...c/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcError.java
@@ -21,13 +21,15 @@
 import com.fasterxml.jackson.annotation.JsonGetter;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.tuweni.bytes.Bytes;
 
 @JsonInclude(value = JsonInclude.Include.NON_NULL)
 @JsonFormat(shape = JsonFormat.Shape.OBJECT)
 public class JsonRpcError {
   private final int code;
   private final String message;
   private final String data;
+  private String reason;
 
   @JsonCreator
   public JsonRpcError(
@@ -41,6 +43,15 @@ public JsonRpcError(
 
   public JsonRpcError(final RpcErrorType errorType, final String data) {
     this(errorType.getCode(), errorType.getMessage(), data);
+
+    // For execution reverted errors decode the data (if present)
+    if (errorType == RpcErrorType.REVERT_ERROR && data != null) {
+      JsonRpcErrorResponse.decodeRevertReason(Bytes.fromHexString(data))
+          .ifPresent(
+              (decodedReason) -> {
+                this.reason = decodedReason;
+              });
+    }
   }
 
   public JsonRpcError(final RpcErrorType errorType) {
@@ -54,7 +65,7 @@ public int getCode() {
 
   @JsonGetter("message")
   public String getMessage() {
-    return message;
+    return (reason == null ? message : message + ": " + reason);
   }
 
   @JsonGetter("data")
@@ -72,12 +83,12 @@ public boolean equals(final Object o) {
     }
     final JsonRpcError that = (JsonRpcError) o;
     return code == that.code
-        && Objects.equals(message, that.message)
+        && Objects.equals(message.split(":", -1)[0], that.message.split(":", -1)[0])
         && Objects.equals(data, that.data);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(code, message, data);
+    return Objects.hash(code, message.split(":", -1)[0], data);
   }
 }
diff --git a/...ava/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcErrorResponse.java b/...ava/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcErrorResponse.java
@@ -15,12 +15,21 @@
 package org.hyperledger.besu.ethereum.api.jsonrpc.internal.response;
 
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 
 import com.fasterxml.jackson.annotation.JsonGetter;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonPropertyOrder;
 import com.google.common.base.MoreObjects;
+import org.apache.tuweni.bytes.Bytes;
+import org.web3j.abi.FunctionReturnDecoder;
+import org.web3j.abi.TypeReference;
+import org.web3j.abi.datatypes.AbiTypes;
+import org.web3j.abi.datatypes.Type;
+import org.web3j.abi.datatypes.Utf8String;
 
 @JsonPropertyOrder({"jsonrpc", "id", "error"})
 public class JsonRpcErrorResponse implements JsonRpcResponse {
@@ -29,6 +38,9 @@ public class JsonRpcErrorResponse implements JsonRpcResponse {
   private final JsonRpcError error;
   @JsonIgnore private final RpcErrorType errorType;
 
+  // Encoding of "Error(string)" to check for at the start of the revert reason
+  static final String errorMethodABI = "0x08c379a0";
+
   public JsonRpcErrorResponse(final Object id, final JsonRpcError error) {
     this.id = id;
     this.error = error;
@@ -84,8 +96,33 @@ public RpcErrorType getErrorType() {
 
   private RpcErrorType findErrorType(final int code, final String message) {
     return Arrays.stream(RpcErrorType.values())
-        .filter(e -> e.getCode() == code && e.getMessage().equals(message))
+        .filter(e -> e.getCode() == code && message.startsWith(e.getMessage()))
         .findFirst()
         .get();
   }
+
+  @SuppressWarnings({"unchecked", "rawtypes"})
+  public static Optional<String> decodeRevertReason(final Bytes revertReason) {
+    if (revertReason.toHexString().startsWith(errorMethodABI)) {
+      // Remove the "Error(string)" prefix
+      final String encodedReasonText =
+          revertReason.toHexString().substring(errorMethodABI.length());
+
+      try {
+        List<TypeReference<Type>> revertReasonTypes =
+            Collections.singletonList(
+                TypeReference.create((Class<Type>) AbiTypes.getType("string")));
+        List<Type> decoded = FunctionReturnDecoder.decode(encodedReasonText, revertReasonTypes);
+
+        // Expect a single decoded string
+        if (decoded.size() == 1 && (decoded.get(0) instanceof Utf8String)) {
+          Utf8String decodedRevertReason = (Utf8String) decoded.get(0);
+          return Optional.of(decodedRevertReason.getValue());
+        }
+      } catch (StringIndexOutOfBoundsException exception) {
+        return Optional.of("ABI decode error");
+      }
+    }
+    return Optional.empty();
+  }
 }
diff --git a/...src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCallTest.java b/...src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCallTest.java
@@ -17,6 +17,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.BLOCK_NOT_FOUND;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.INTERNAL_ERROR;
+import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.REVERT_ERROR;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
@@ -32,6 +33,7 @@
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.JsonCallParameter;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
@@ -44,6 +46,7 @@
 import org.hyperledger.besu.ethereum.mainnet.ImmutableTransactionValidationParams;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidationParams;
 import org.hyperledger.besu.ethereum.mainnet.ValidationResult;
+import org.hyperledger.besu.ethereum.processing.TransactionProcessingResult;
 import org.hyperledger.besu.ethereum.transaction.CallParameter;
 import org.hyperledger.besu.ethereum.transaction.PreCloseStateHandler;
 import org.hyperledger.besu.ethereum.transaction.TransactionSimulator;
@@ -164,6 +167,128 @@ public void shouldReturnExecutionResultWhenExecutionIsSuccessful() {
     verifyNoMoreInteractions(blockchainQueries);
   }
 
+  @Test
+  public void shouldReturnBasicExecutionRevertErrorWithoutReason() {
+    final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");
+
+    // Expect a revert error with no decoded reason (error doesn't begin "Error(string)" so ignored)
+    final String abiHexString = "0x1234";
+    final JsonRpcError expectedError = new JsonRpcError(REVERT_ERROR, abiHexString);
+    final JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, expectedError);
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted");
+
+    mockTransactionProcessorSuccessResult(expectedResponse);
+    when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHead()).thenReturn(chainHead);
+
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ZERO));
+    when(chainHead.getBlockHeader()).thenReturn(blockHeader);
+
+    final JsonRpcResponse response = method.response(request);
+
+    final TransactionProcessingResult processingResult =
+        new TransactionProcessingResult(
+            null, null, 0, 0, null, null, Optional.of(Bytes.fromHexString(abiHexString)));
+
+    final TransactionSimulatorResult result = mock(TransactionSimulatorResult.class);
+    when(result.isSuccessful()).thenReturn(false);
+    when(result.getValidationResult()).thenReturn(ValidationResult.valid());
+    when(result.getResult()).thenReturn(processingResult);
+    verify(transactionSimulator).process(any(), any(), any(), mapperCaptor.capture(), any());
+    assertThat(mapperCaptor.getValue().apply(mock(MutableWorldState.class), Optional.of(result)))
+        .isEqualTo(Optional.of(expectedResponse));
+
+    assertThat(response).usingRecursiveComparison().isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) response).getError().getMessage())
+        .isEqualTo("Execution reverted");
+  }
+
+  @Test
+  public void shouldReturnExecutionRevertErrorWithABIParseError() {
+    final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");
+
+    // Expect a revert error with no decoded reason (error begins with "Error(string)" but trailing
+    // bytes are invalid ABI)
+    final String abiHexString = "0x08c379a002d36d";
+    final JsonRpcError expectedError = new JsonRpcError(REVERT_ERROR, abiHexString);
+    final JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, expectedError);
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ABI decode error");
+
+    mockTransactionProcessorSuccessResult(expectedResponse);
+    when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHead()).thenReturn(chainHead);
+
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ZERO));
+    when(chainHead.getBlockHeader()).thenReturn(blockHeader);
+
+    final JsonRpcResponse response = method.response(request);
+    final TransactionProcessingResult processingResult =
+        new TransactionProcessingResult(
+            null, null, 0, 0, null, null, Optional.of(Bytes.fromHexString(abiHexString)));
+
+    final TransactionSimulatorResult result = mock(TransactionSimulatorResult.class);
+    when(result.isSuccessful()).thenReturn(false);
+    when(result.getValidationResult()).thenReturn(ValidationResult.valid());
+    when(result.getResult()).thenReturn(processingResult);
+    verify(transactionSimulator).process(any(), any(), any(), mapperCaptor.capture(), any());
+    assertThat(mapperCaptor.getValue().apply(mock(MutableWorldState.class), Optional.of(result)))
+        .isEqualTo(Optional.of(expectedResponse));
+
+    assertThat(response).usingRecursiveComparison().isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) response).getError().getMessage())
+        .isEqualTo("Execution reverted: ABI decode error");
+  }
+
+  @Test
+  public void shouldReturnExecutionRevertErrorWithParsedABI() {
+    final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");
+
+    // Expect a revert error with decoded reason (error begins with "Error(string)", trailing bytes
+    // = "ERC20: transfer from the zero address")
+    final String abiHexString =
+        "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002545524332303a207472616e736665722066726f6d20746865207a65726f2061646472657373000000000000000000000000000000000000000000000000000000";
+    final JsonRpcError expectedError = new JsonRpcError(REVERT_ERROR, abiHexString);
+    final JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, expectedError);
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ERC20: transfer from the zero address");
+
+    mockTransactionProcessorSuccessResult(expectedResponse);
+    when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHead()).thenReturn(chainHead);
+
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ZERO));
+    when(chainHead.getBlockHeader()).thenReturn(blockHeader);
+
+    final JsonRpcResponse response = method.response(request);
+
+    final TransactionProcessingResult processingResult =
+        new TransactionProcessingResult(
+            null, null, 0, 0, null, null, Optional.of(Bytes.fromHexString(abiHexString)));
+
+    final TransactionSimulatorResult result = mock(TransactionSimulatorResult.class);
+    when(result.isSuccessful()).thenReturn(false);
+    when(result.getValidationResult()).thenReturn(ValidationResult.valid());
+    when(result.getResult()).thenReturn(processingResult);
+
+    verify(transactionSimulator).process(any(), any(), any(), mapperCaptor.capture(), any());
+    System.out.println(result);
+    System.out.println(expectedResponse);
+    assertThat(mapperCaptor.getValue().apply(mock(MutableWorldState.class), Optional.of(result)))
+        .isEqualTo(Optional.of(expectedResponse));
+
+    assertThat(response).usingRecursiveComparison().isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) response).getError().getMessage())
+        .isEqualTo("Execution reverted: ERC20: transfer from the zero address");
+  }
+
   @Test
   public void shouldUseCorrectBlockNumberWhenLatest() {
     final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");