From 8fd8de6a43ed34b5d9705792f56e0418c82b7dd5 Mon Sep 17 00:00:00 2001 From: Juraj Piar Date: Wed, 10 Apr 2024 12:54:09 +0100 Subject: [PATCH] refactor(rpc): clean-up error handling --- .../co/rsk/rpc/modules/eth/EthModule.java | 40 ++++++++++++---- .../eth/EthModuleTransactionInstant.java | 15 +++++- .../co/rsk/rpc/modules/eth/ProgramRevert.java | 47 ------------------- .../rpc/exception/RskErrorResolver.java | 5 +- .../exception/RskJsonRpcRequestException.java | 35 +++++++------- .../co/rsk/rpc/modules/eth/EthModuleTest.java | 13 ++++- .../rpc/netty/JsonRpcCustomServerTest.java | 17 ++++--- .../rpc/exception/RskErrorResolverTest.java | 40 +++++++--------- 8 files changed, 101 insertions(+), 111 deletions(-) delete mode 100644 rskj-core/src/main/java/co/rsk/rpc/modules/eth/ProgramRevert.java diff --git a/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModule.java b/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModule.java index 2ca79605ef6..acb5e1d2eb9 100644 --- a/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModule.java +++ b/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModule.java @@ -33,7 +33,14 @@ import co.rsk.trie.TrieStoreImpl; import co.rsk.util.HexUtils; import com.google.common.annotations.VisibleForTesting; -import org.ethereum.core.*; +import org.apache.commons.lang3.tuple.Pair; +import org.ethereum.core.Block; +import org.ethereum.core.Blockchain; +import org.ethereum.core.CallTransaction; +import org.ethereum.core.Repository; +import org.ethereum.core.Transaction; +import org.ethereum.core.TransactionExecutor; +import org.ethereum.core.TransactionPool; import org.ethereum.datasource.HashMapDB; import org.ethereum.db.MutableRepository; import org.ethereum.rpc.CallArguments; @@ -51,7 +58,9 @@ import javax.annotation.Nonnull; import java.io.IOException; -import java.util.*; +import java.util.Arrays; +import java.util.List; +import java.util.Map; import static java.util.Arrays.copyOfRange; import static org.ethereum.rpc.exception.RskJsonRpcRequestException.invalidParamError; @@ -138,13 +147,24 @@ public String call(CallArgumentsParam argsParam, BlockIdentifierParam bnOrId) { res = callConstant(args, block); } if (res.isRevert()) { + Pair programRevert = decodeProgramRevert(res); + String revertReason = programRevert.getLeft(); + byte[] revertData = programRevert.getRight(); + if (revertData == null) { + throw RskJsonRpcRequestException.transactionRevertedExecutionError(); + } + + if (revertReason == null) { + throw RskJsonRpcRequestException.transactionRevertedExecutionError(revertData); + } - throw RskJsonRpcRequestException.transactionRevertedExecutionError(decodeProgramRevert(res)); + throw RskJsonRpcRequestException.transactionRevertedExecutionError(revertReason, revertData); } hReturn = HexUtils.toUnformattedJsonHex(res.getHReturn()); return hReturn; - } finally { + } + finally { LOGGER.debug("eth_call(): {}", hReturn); } } @@ -296,26 +316,26 @@ public ProgramResult callConstant(CallArguments args, Block executionBlock) { * @param programResult contains the result of execution of a smart contract * @return revert reason and revert data. reason may be nullable or empty */ - public static ProgramRevert decodeProgramRevert(ProgramResult programResult) { + public static Pair decodeProgramRevert(ProgramResult programResult) { byte[] bytes = programResult.getHReturn(); - if (bytes.length < 4) { + if (bytes == null || bytes.length < 4) { - return new ProgramRevert(bytes); + return Pair.of(null, null); } final byte[] signature = copyOfRange(bytes, 0, 4); if (!Arrays.equals(signature, ERROR_ABI_FUNCTION_SIGNATURE)) { - return new ProgramRevert(bytes); + return Pair.of(null, bytes); } final Object[] decode = ERROR_ABI_FUNCTION.decode(bytes); if (decode == null || decode.length == 0) { - return new ProgramRevert(bytes); + return Pair.of(null, bytes); } - return new ProgramRevert((String) decode[0], bytes); + return Pair.of((String) decode[0], bytes); } private ProgramResult callConstantWithState(CallArguments args, Block executionBlock, Trie state) { diff --git a/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModuleTransactionInstant.java b/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModuleTransactionInstant.java index 3948cdad2e0..77d3d903f01 100644 --- a/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModuleTransactionInstant.java +++ b/rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModuleTransactionInstant.java @@ -25,10 +25,12 @@ import co.rsk.mine.MinerServer; import co.rsk.net.TransactionGateway; import co.rsk.util.HexUtils; +import org.apache.commons.lang3.tuple.Pair; import org.ethereum.config.Constants; import org.ethereum.core.Blockchain; import org.ethereum.core.TransactionPool; import org.ethereum.db.TransactionInfo; +import org.ethereum.rpc.exception.RskJsonRpcRequestException; import org.ethereum.rpc.parameters.CallArgumentsParam; import org.ethereum.rpc.parameters.HexDataParam; import org.ethereum.vm.program.ProgramResult; @@ -111,7 +113,18 @@ private String getReturnMessage(String txHash) { ProgramResult programResult = this.blockExecutor.getProgramResult(hash); if (programResult != null && programResult.isRevert()) { - throw transactionRevertedExecutionError(EthModule.decodeProgramRevert(programResult)); + Pair programRevert = EthModule.decodeProgramRevert(programResult); + String revertReason = programRevert.getLeft(); + byte[] revertData = programRevert.getRight(); + if (revertData == null) { + throw RskJsonRpcRequestException.transactionRevertedExecutionError(); + } + + if (revertReason == null) { + throw RskJsonRpcRequestException.transactionRevertedExecutionError(revertData); + } + + throw RskJsonRpcRequestException.transactionRevertedExecutionError(revertReason, revertData); } if (!transactionInfo.getReceipt().isSuccessful()) { diff --git a/rskj-core/src/main/java/co/rsk/rpc/modules/eth/ProgramRevert.java b/rskj-core/src/main/java/co/rsk/rpc/modules/eth/ProgramRevert.java deleted file mode 100644 index 45fb9679802..00000000000 --- a/rskj-core/src/main/java/co/rsk/rpc/modules/eth/ProgramRevert.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * This file is part of RskJ - * Copyright (C) 2018 RSK Labs Ltd. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Lesser General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program. If not, see . - */ -package co.rsk.rpc.modules.eth; - -import javax.annotation.Nonnull; - -public class ProgramRevert { - @Nonnull - private final String reason; - - @Nonnull - private final byte[] data; - - public ProgramRevert(@Nonnull byte[] data) { - this("", data); - } - - public ProgramRevert(@Nonnull String reason, @Nonnull byte[] data) { - this.reason = reason; - this.data = data; - } - - @Nonnull - public String getReason() { - return reason; - } - - @Nonnull - public byte[] getData() { - return data; - } -} \ No newline at end of file diff --git a/rskj-core/src/main/java/org/ethereum/rpc/exception/RskErrorResolver.java b/rskj-core/src/main/java/org/ethereum/rpc/exception/RskErrorResolver.java index b74bc17cb56..601ca5d4d90 100644 --- a/rskj-core/src/main/java/org/ethereum/rpc/exception/RskErrorResolver.java +++ b/rskj-core/src/main/java/org/ethereum/rpc/exception/RskErrorResolver.java @@ -2,6 +2,7 @@ import co.rsk.core.exception.InvalidRskAddressException; import co.rsk.jsonrpc.JsonRpcError; +import co.rsk.util.HexUtils; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.exc.InvalidFormatException; @@ -14,8 +15,6 @@ import java.util.Iterator; import java.util.List; -import static javax.xml.bind.DatatypeConverter.printHexBinary; - /** * Created by mario on 17/10/2016. */ @@ -35,7 +34,7 @@ public JsonError resolveError(Throwable t, Method method, List argumen } else if (t instanceof RskJsonRpcRequestException) { RskJsonRpcRequestException rskJsonRpcRequestException = (RskJsonRpcRequestException) t; byte[] revertData = rskJsonRpcRequestException.getRevertData(); - String errorDataHexString = "0x" + printHexBinary(revertData == null ? new byte[]{} : revertData).toLowerCase(); + String errorDataHexString = revertData == null ? null : HexUtils.toUnformattedJsonHex(revertData); error = new JsonError(rskJsonRpcRequestException.getCode(), t.getMessage(), errorDataHexString); } else if (t instanceof InvalidFormatException) { error = new JsonError(JsonRpcError.INTERNAL_ERROR, "Internal server error, probably due to invalid parameter type", null); diff --git a/rskj-core/src/main/java/org/ethereum/rpc/exception/RskJsonRpcRequestException.java b/rskj-core/src/main/java/org/ethereum/rpc/exception/RskJsonRpcRequestException.java index d941202ef2c..a5a7578f540 100644 --- a/rskj-core/src/main/java/org/ethereum/rpc/exception/RskJsonRpcRequestException.java +++ b/rskj-core/src/main/java/org/ethereum/rpc/exception/RskJsonRpcRequestException.java @@ -1,16 +1,15 @@ package org.ethereum.rpc.exception; -import co.rsk.rpc.modules.eth.ProgramRevert; - +import javax.annotation.Nonnull; import javax.annotation.Nullable; public class RskJsonRpcRequestException extends RuntimeException { - private final Integer code; + @Nullable private final byte[] revertData; - protected RskJsonRpcRequestException(Integer code, byte[] revertData, String message, Exception e) { + protected RskJsonRpcRequestException(Integer code, @Nullable byte[] revertData, String message, Exception e) { super(message, e); this.code = code; this.revertData = revertData; @@ -20,7 +19,7 @@ protected RskJsonRpcRequestException(Integer code, String message, Exception e) this(code, new byte[]{}, message, e); } - public RskJsonRpcRequestException(Integer code, byte[] revertData, String message) { + public RskJsonRpcRequestException(Integer code, @Nullable byte[] revertData, String message) { super(message); this.code = code; this.revertData = revertData; @@ -34,24 +33,28 @@ public Integer getCode() { return code; } - @Nullable public byte[] getRevertData() { return revertData; } - public static RskJsonRpcRequestException transactionRevertedExecutionError(ProgramRevert programRevert) { - byte[] revertData = programRevert.getData(); - String revertReason = programRevert.getReason(); - if (revertReason.isEmpty()) { - - return executionError("transaction reverted, no reason specified", revertData); - } + public static RskJsonRpcRequestException transactionRevertedExecutionError() { + return executionError("transaction reverted", null); + } - return executionError("revert " + revertReason, revertData); + public static RskJsonRpcRequestException transactionRevertedExecutionError(@Nonnull byte[] revertData) { + return executionError("transaction reverted", revertData); } - public static RskJsonRpcRequestException transactionRevertedExecutionError() { - return executionError("transaction reverted", null); + public static RskJsonRpcRequestException transactionRevertedExecutionError( + @Nonnull String revertReason, + @Nonnull byte[] revertData + ) { + return executionError( + revertReason.isEmpty() + ? "transaction reverted, no reason specified" + : "revert " + revertReason, + revertData + ); } public static RskJsonRpcRequestException unknownError(String message) { diff --git a/rskj-core/src/test/java/co/rsk/rpc/modules/eth/EthModuleTest.java b/rskj-core/src/test/java/co/rsk/rpc/modules/eth/EthModuleTest.java index 144a95d214d..b9300c268cc 100644 --- a/rskj-core/src/test/java/co/rsk/rpc/modules/eth/EthModuleTest.java +++ b/rskj-core/src/test/java/co/rsk/rpc/modules/eth/EthModuleTest.java @@ -189,10 +189,19 @@ void test_revertedTransaction() { config.getGasEstimationCap(), config.getCallGasCap()); - RskJsonRpcRequestException exception = assertThrows(RskJsonRpcRequestException.class, () -> eth.call(TransactionFactoryHelper.toCallArgumentsParam(args), new BlockIdentifierParam("latest"))); + BlockIdentifierParam blockIdentifierParam = new BlockIdentifierParam("latest"); + + CallArgumentsParam callArgumentsParam = TransactionFactoryHelper.toCallArgumentsParam(args); + RskJsonRpcRequestException exception = assertThrows( + RskJsonRpcRequestException.class, + () -> eth.call( + callArgumentsParam, + blockIdentifierParam + ) + ); assertThat(exception.getMessage(), Matchers.containsString("deposit too big")); assertNotNull(exception.getRevertData()); - assertEquals(hReturn, exception.getRevertData()); + assertArrayEquals(hReturn, exception.getRevertData()); } @Test diff --git a/rskj-core/src/test/java/co/rsk/rpc/netty/JsonRpcCustomServerTest.java b/rskj-core/src/test/java/co/rsk/rpc/netty/JsonRpcCustomServerTest.java index 8d4d20c9655..53584064197 100644 --- a/rskj-core/src/test/java/co/rsk/rpc/netty/JsonRpcCustomServerTest.java +++ b/rskj-core/src/test/java/co/rsk/rpc/netty/JsonRpcCustomServerTest.java @@ -22,7 +22,6 @@ import co.rsk.rpc.ModuleDescription; import co.rsk.rpc.exception.JsonRpcResponseLimitError; import co.rsk.rpc.exception.JsonRpcTimeoutError; -import co.rsk.rpc.modules.eth.ProgramRevert; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; @@ -34,7 +33,6 @@ import org.ethereum.rpc.parameters.CallArgumentsParam; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import java.util.ArrayList; @@ -46,7 +44,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) class JsonRpcCustomServerTest { @@ -93,7 +95,7 @@ void testHandleJsonNodeRequest_WithMethodModule() throws Exception { String response = "test_method_response"; when(handler.test_second("param", "param2")).thenAnswer(invocation -> { - waitFor(150); + waitFor(150); return response; }); @@ -246,11 +248,8 @@ void failedEthCall_givenRskErrorResolver_shouldThrowWithData() throws JsonProces JsonNode request = objectMapper.readTree(jsonRequest); FakeWeb3ForEthCall web3ForEthCall = mock(FakeWeb3ForEthCall.class); - ProgramRevert fakeProgramRevert = new ProgramRevert("deposit too big", revertBytes); - RskJsonRpcRequestException exception = RskJsonRpcRequestException.transactionRevertedExecutionError(fakeProgramRevert); - when(web3ForEthCall.eth_call(any(), any())).then(invocation -> { - throw exception; - }); + RskJsonRpcRequestException exception = RskJsonRpcRequestException.transactionRevertedExecutionError("deposit too big", revertBytes); + when(web3ForEthCall.eth_call(any(), any())).thenThrow(exception); jsonRpcCustomServer = new JsonRpcCustomServer(web3ForEthCall, FakeWeb3ForEthCall.class, modules, objectMapper); jsonRpcCustomServer.setErrorResolver(new RskErrorResolver()); diff --git a/rskj-core/src/test/java/org/ethereum/rpc/exception/RskErrorResolverTest.java b/rskj-core/src/test/java/org/ethereum/rpc/exception/RskErrorResolverTest.java index 7a76a2cf54c..53ca05d1604 100644 --- a/rskj-core/src/test/java/org/ethereum/rpc/exception/RskErrorResolverTest.java +++ b/rskj-core/src/test/java/org/ethereum/rpc/exception/RskErrorResolverTest.java @@ -7,10 +7,10 @@ import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException; import com.fasterxml.jackson.databind.node.JsonNodeFactory; import com.fasterxml.jackson.databind.ser.DefaultSerializerProvider; -import org.bouncycastle.util.encoders.Hex; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.spongycastle.util.encoders.Hex; import java.lang.reflect.Method; import java.util.ArrayList; @@ -34,26 +34,13 @@ void setup() { } @Test - void test_resolveError_givenRskJsonRpcRequestException_returnsJsonErrorWithErrorCode() throws NoSuchMethodException { + void test_resolveError_givenRskJsonRpcRequestExceptionWithoutData_returnsJsonErrorAsExpected() throws NoSuchMethodException { // Given - Integer expectedErrorCode = 1; - RskJsonRpcRequestException exception = new RskJsonRpcRequestException(expectedErrorCode, "message"); - Method methodMock = this.getClass().getMethod("mockMethod"); - List jsonNodeListMock = new ArrayList<>(); - - // When - JsonError result = rskErrorResolver.resolveError(exception, methodMock, jsonNodeListMock); - - // Then - Assertions.assertNotNull(result); - Assertions.assertEquals(expectedErrorCode, (Integer) result.code); - } + Integer code = 1; + String message = "message"; + String expectedData = "0x"; + RskJsonRpcRequestException exception = new RskJsonRpcRequestException(code, message); - @Test - void test_resolveError_givenRskJsonRpcRequestException_returnsJsonErrorWithErrorMessage() throws NoSuchMethodException { - // Given - String expectedErrorMessage = "message"; - RskJsonRpcRequestException exception = new RskJsonRpcRequestException(1, expectedErrorMessage); Method methodMock = this.getClass().getMethod("mockMethod"); List jsonNodeListMock = new ArrayList<>(); @@ -62,18 +49,24 @@ void test_resolveError_givenRskJsonRpcRequestException_returnsJsonErrorWithError // Then Assertions.assertNotNull(result); - Assertions.assertEquals(expectedErrorMessage, result.message); + Assertions.assertEquals(code, (Integer) result.code); + Assertions.assertEquals(message, result.message); + Assertions.assertEquals(expectedData, result.data); } @Test - void test_resolveError_givenRskJsonRpcRequestExceptionWithData_returnsJsonErrorWithDataAsHexString() throws NoSuchMethodException { + void test_resolveError_givenRskJsonRpcRequestExceptionWithData_returnsJsonErrorAsExpected() throws NoSuchMethodException { // Given + Integer code = 1; + String message = "message"; String dataString = "08c379a000000000000000000000000000000000000000000000000000000000" + "0000002000000000000000000000000000000000000000000000000000000000" + "0000000f6465706f73697420746f6f2062696700000000000000000000000000" + "00000000"; byte[] data = Hex.decode(dataString); - RskJsonRpcRequestException exception = new RskJsonRpcRequestException(1, data, "message"); + String expectedData = "0x" + dataString; + RskJsonRpcRequestException exception = new RskJsonRpcRequestException(code, data, message); + Method methodMock = this.getClass().getMethod("mockMethod"); List jsonNodeListMock = new ArrayList<>(); @@ -81,8 +74,9 @@ void test_resolveError_givenRskJsonRpcRequestExceptionWithData_returnsJsonErrorW JsonError result = rskErrorResolver.resolveError(exception, methodMock, jsonNodeListMock); // Then - String expectedData = "0x" + dataString; Assertions.assertNotNull(result); + Assertions.assertEquals(code, (Integer) result.code); + Assertions.assertEquals(message, result.message); Assertions.assertEquals(expectedData, result.data); }