Revert Reason #1603

pinges · 2019-06-25T05:56:20Z

PR description

This will include the revert reason associated with revert evm operation as an additional field in the transaction receipt when retrieved via the eth_getTransactionReceipt rpc call.

Fixed Issue(s)

If available, add the revert reason thrown inside a contract to the Transaction Receipt returned via a rpc calls.

CLAassistant · 2019-06-25T05:56:27Z

All committers have signed the CLA.

jframe

Looks good apart from the missing tests

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetProtocolSpecs.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java

rain-on · 2019-06-26T01:19:48Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java

@@ -115,7 +144,9 @@ public void writeTo(final RLPOutput out) {
    out.writeLongScalar(cumulativeGasUsed);
    out.writeBytesValue(bloomFilter.getBytes());
    out.writeList(logs, Log::writeTo);
-
+    if (withRevertReason && revertReason.isPresent()) {
+      out.writeBytesValue(BytesValue.wrap(revertReason.get().getBytes(StandardCharsets.UTF_8)));


If someone has asked for the reason, but it doesn't exist, should we include an empty string?
I.e. Client requested the RevertReason explicitly, probably should give it to them (even if it doesn't exist).

Don't think an empty string makes sense as an empty string is a valid revert reason. And returning a null for the revert reason as it does now makes more sense as there is no revert reason value to return.

👍 null return value for a null revert reason.

In this context null and an empty string would have different meanings i.e. no revert message vs an empty revert message.

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java

rain-on · 2019-06-26T01:30:14Z

.../src/test-support/java/tech/pegasys/pantheon/ethereum/core/BlockDataGeneratorWithReason.java

+import java.util.Arrays;
+import java.util.Optional;
+
+public class BlockDataGeneratorWithReason extends BlockDataGenerator {


Could this just be done with Mocking?

Could be but theres a BlockDataGenerator that already exists. And the code is nicer using the data generator rather than adding mock. I've also added a receipt(revertReason) method to the existing BlockDataGenerator remove need for this extra BlockDataGenerator class.

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/core/TransactionReceiptTest.java

...ts/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/eth/EthConditions.java

...ptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/PantheonNode.java

…'t need start another server and avoid passing through host and ports in tests

...eon/tests/acceptance/dsl/transaction/eth/EthGetTransactionReceiptRawResponseTransaction.java

acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/web3j/RevertReason.sol

...ptance-tests/src/test/java/tech/pegasys/pantheon/tests/web3j/RevertReasonAcceptanceTest.java

…don't need to expose web3j

CjHare

I'm not keen on the EthGetTransactionReceiptRaw naming, I think it's disingenuous, it is just a EthGetTransactionReceipt

...n/tests/acceptance/dsl/condition/eth/ExpectSuccessfulEthGetTransactionReceiptWithReason.java

...ests/acceptance/dsl/condition/eth/ExpectSuccessfulEthGetTransactionReceiptWithoutReason.java

CjHare · 2019-07-02T06:47:38Z

...h/pegasys/pantheon/tests/acceptance/dsl/transaction/CallSmartContractFunctionWithRevert.java

+import org.web3j.protocol.core.methods.response.EthSendTransaction;
+import org.web3j.tx.RawTransactionManager;
+
+public class CallSmartContractFunctionWithRevert implements Transaction<EthSendTransaction> {


What in this class links to WithRevert? ...as it looks like a generic smart contract function invocation

renamed to CallSmartContractFunction

CjHare · 2019-07-02T06:51:35Z

...st/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/eth/EthRawRequestFactory.java

+import org.web3j.protocol.core.Response;
+import org.web3j.protocol.core.methods.response.TransactionReceipt;
+
+public class EthRawRequestFactory {


This class is bunk.

The one method in the class is calling eth_getTransactionReceipt, we have simply extended that function to include an additional field. Why not put this new method in Eth?

Moved this rpc request method into the custom rpc factory

pantheon/src/test/resources/everything_config.toml

CjHare

Only Javadoc issues in this pass

CjHare · 2019-07-03T03:07:49Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java


  /**
   * Creates an instance of a state root-encoded transaction receipt.
   *
   * @param stateRoot the state root for the world state after the transaction has been processed
   * @param cumulativeGasUsed the total amount of gas consumed in the block after this transaction
   * @param logs the logs generated within the transaction
+   * @param revertReason whether storing the revert reason is for failed transactions


Grammer.

@param revertReason whether to store the revert reason for failed transactions

This is actually the revert reason not a flag whether or not to store the revert reason. So have updated this and the below constructor to reflect this.

CjHare · 2019-07-03T03:08:14Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java

@@ -69,32 +85,40 @@ private TransactionReceipt(
   * @param status the status code for the transaction (1 for success and 0 for failure)
   * @param cumulativeGasUsed the total amount of gas consumed in the block after this transaction
   * @param logs the logs generated within the transaction
+   * @param revertReason the revert reason


@param revertReason whether to store the revert reason for failed transactions

As above, the java doc was misleading have updated to the same as the java doc for the other constructor

CjHare · 2019-07-03T03:09:51Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java

@@ -115,7 +144,9 @@ public void writeTo(final RLPOutput out) {
    out.writeLongScalar(cumulativeGasUsed);
    out.writeBytesValue(bloomFilter.getBytes());
    out.writeList(logs, Log::writeTo);
-
+    if (withRevertReason && revertReason.isPresent()) {
+      out.writeBytesValue(BytesValue.wrap(revertReason.get().getBytes(StandardCharsets.UTF_8)));


👍 null return value for a null revert reason.

In this context null and an empty string would have different meanings i.e. no revert message vs an empty revert message.

CjHare · 2019-07-03T03:11:26Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/TransactionProcessor.java

@@ -95,6 +97,13 @@ default boolean isSuccessful() {
     * @return the validation result, with the reason for failure (if applicable.)
     */
    ValidationResult<TransactionInvalidReason> getValidationResult();
+
+    /**
+     * Returns the reason a transaction was reverted (if applicable).


Missing word.

Returns the reason why a transaction was reverted (if applicable).

pinges added 2 commits June 25, 2019 14:38

for review

e4b9bf3

merge

46b7fe1

pinges requested review from rain-on, jframe and CjHare June 25, 2019 05:57

jframe reviewed Jun 25, 2019

View reviewed changes

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java Outdated Show resolved Hide resolved

ajsutton reviewed Jun 25, 2019

View reviewed changes

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetProtocolSpecs.java Outdated Show resolved Hide resolved

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionReceipt.java Show resolved Hide resolved

rain-on reviewed Jun 26, 2019

View reviewed changes

revert reason can be disabled now

4cac6f9

CjHare reviewed Jun 30, 2019

View reviewed changes

jframe added 3 commits July 1, 2019 13:08

Merge branch 'master' into reason

8b0c9d9

spotless

e7fc105

change revert reason transaction to use web3j instead of vertx so don…

48f860e

…'t need start another server and avoid passing through host and ports in tests

CjHare reviewed Jul 1, 2019

View reviewed changes

jframe added 9 commits July 1, 2019 17:01

create transaction to call a smart contract function that reverts so …

7b4cc75

…don't need to expose web3j

add final

45cb0da

revert reason AT configuration

5e00f0c

Merge branch 'master' into reason

5d2db35

rename transaction class

fdef2bc

formatting

96697bc

add receipt with revert reason method to block data generator

c133263

simplify revert reason solidity contract

91496e8

ignore raw types for generated code

267700b

CjHare reviewed Jul 2, 2019

View reviewed changes

pantheon/src/test/resources/everything_config.toml Outdated Show resolved Hide resolved

jframe added 4 commits July 3, 2019 08:39

PR changes

206cc00

Merge branch 'master' into reason

24c86bb

PR changes

b0fb509

Merge branch 'reason' of github.com:pinges/pantheon into reason

7128979

PR changes

26af22b

jframe marked this pull request as ready for review July 3, 2019 00:47

CjHare reviewed Jul 3, 2019

View reviewed changes

jframe added 3 commits July 3, 2019 13:31

PR changes

1a27344

PR changes

471dc95

Merge branch 'master' into reason

1f08fd5

CjHare approved these changes Jul 3, 2019

View reviewed changes

Merge branch 'master' into reason

b7a17cd

jframe merged commit 6ce23f3 into PegaSysEng:master Jul 4, 2019

iikirilov mentioned this pull request Sep 12, 2019

Add revert reason to transaction receipt hyperledger-web3j/web3j#1024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert Reason #1603

Revert Reason #1603

pinges commented Jun 25, 2019 •

edited by jframe

Loading

CLAassistant commented Jun 25, 2019 •

edited

Loading

jframe left a comment

rain-on Jun 26, 2019

jframe Jul 2, 2019

CjHare Jul 3, 2019

rain-on Jun 26, 2019

jframe Jul 2, 2019

CjHare left a comment

CjHare Jul 2, 2019

jframe Jul 3, 2019

CjHare Jul 2, 2019

jframe Jul 3, 2019

CjHare left a comment

CjHare Jul 3, 2019

jframe Jul 3, 2019

CjHare Jul 3, 2019

jframe Jul 3, 2019

CjHare Jul 3, 2019

CjHare Jul 3, 2019

jframe Jul 3, 2019

Revert Reason #1603

Revert Reason #1603

Conversation

pinges commented Jun 25, 2019 • edited by jframe Loading

PR description

Fixed Issue(s)

CLAassistant commented Jun 25, 2019 • edited Loading

jframe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CjHare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CjHare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges commented Jun 25, 2019 •

edited by jframe

Loading

CLAassistant commented Jun 25, 2019 •

edited

Loading