Add support for parsing custom errors & fixing revert error propagation #42

jimthematrix · 2022-11-22T22:14:10Z

Depends on:

Add Errors spec support to contract deploy request firefly-transaction-manager#44 (build fails until this is merged)
Use RPCError as return from CallRPC() to catch error details firefly-signer#30 (build fails until this is merged)

This allows the transaction payload to include an optional errors section that specifies the FFI for custom error definitions:

{
  "ffcapi": {
    "version": "v1.0.0",
    "id": "904F177C-C790-4B01-BDF4-F2B4E52E607E",
    "type": "exec_query"
  },
  "from": "0xb480F96c0a3d6E9e9a263e4665a39bFa6c4d01E8",
  "to": "0xe1a078b9e2b145d0a7387f09277c6ae1d9470771",
  "nonce": "222",
  "method": {
    "inputs": [
      {
        "internalType": " uint256",
        "name": "x",
        "type": "uint256"
      }
    ],
    "name": "set",
    "outputs": [
      {
        "internalType": "uint256",
        "name": "",
        "type": "uint256"
      },
      {
        "type": "string"
      }
    ],
    "stateMutability": "nonpayable",
    "type": "function"
  },
  "errors": [
    {
      "inputs": [
        {
          "internalType": "uint256",
          "name": "x",
          "type": "uint256"
        },
        {
          "internalType": "uint256",
          "name": "y",
          "type": "uint256"
        }
      ],
      "name": "CustomError1",
      "type": "error"
    },
    {
      "inputs": [
        {
          "internalType": "string",
          "name": "x",
          "type": "string"
        }
      ],
      "name": "CustomError2",
      "type": "error"
    }
  ],
  "params": [3]
}

for a test solidity contract that declares custom errors like the following :

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

error CustomError1(uint256 x, uint256 y);
error CustomError2(string x);

contract SimpleStorage {
    uint256 storedData;

    constructor(uint256 x) {
        if (x == 0) {
            revert CustomError1({x: x, y: x});
        }
        if (x == 1) {
            revert CustomError2({x: "This is a custom error"});
        }
        require(x > 2, "Can not equal to 2");
    }

    function set(uint256 x) public {
        if (x == 0) {
            revert CustomError1({x: x, y: x});
        }
        if (x == 1) {
            revert CustomError2({x: "This is a custom error"});
        }
        require(x > 2, "Can not equal to 2");
        storedData = x;
    }
}

fixes #41

Signed-off-by: Jim Zhang <[email protected]>

peterbroadhurst

One request for clarification on the estimate-gas flow and testing @jimthematrix

peterbroadhurst · 2022-11-30T14:07:17Z

internal/ethereum/exec_query.go

+					idBytes := e.FunctionSelectorBytes()
+					if bytes.Equal(signature, idBytes) {
+						errorInfo, err := e.DecodeCallDataCtx(ctx, outputData)
+						if err == nil {


I think we've got too deep nesting here - could we split out this section for formatting the error for a given match into a separate function?

peterbroadhurst · 2022-11-30T14:17:43Z

internal/ethereum/exec_query.go

 	// Build the base transaction object
 	tx, err := c.buildTx(ctx, txTypeQuery, req.From, req.To, req.Nonce, req.Gas, req.Value, callData)
 	if err != nil {
 		return nil, ffcapi.ErrorReasonInvalidInputs, err
 	}

 	// Do the call, with processing of revert reasons
-	outputs, reason, err := c.callTransaction(ctx, tx, method)
+	outputs, reason, err := c.callTransaction(ctx, tx, method, errors)


Think it's really important to make sure this gets called for the estimate gas path - wasn't immediately obvious to me that it is.

We should test this for both the send-transaction, and the deploy-contract, flows to make sure the error comes out successfully against Geth 1.10 and Besu if that's ok.

Signed-off-by: Jim Zhang <[email protected]>

jimthematrix · 2022-12-04T22:00:00Z

Test with Geth 1.10

Geth 1.10 returns the error details in the eth_estimateGas response.

Deploy contract

{
  "headers": {
    "id": "904F177C-C790-4B01-BDF4-F2B4E52E6082",
    "type": "DeployContract"
  },
  "from": "0x3eee66f28ede9bd661e9bab117f9a8f11a47bbfd",
  "contract": "0x608060405234801561001057600080fd5b5060405161057f38038061057f83398181016040528101906100329190610140565b600081036100795780816040517fd286870d00000000000000000000000000000000000000000000000000000000815260040161007092919061017c565b60405180910390fd5b600181036100bc576040517f60b7d47e0000000000000000000000000000000000000000000000000000000081526004016100b390610202565b60405180910390fd5b600281116100ff576040517f08c379a00000000000000000000000000000000000000000000000000000000081526004016100f69061026e565b60405180910390fd5b5061028e565b600080fd5b6000819050919050565b61011d8161010a565b811461012857600080fd5b50565b60008151905061013a81610114565b92915050565b60006020828403121561015657610155610105565b5b60006101648482850161012b565b91505092915050565b6101768161010a565b82525050565b6000604082019050610191600083018561016d565b61019e602083018461016d565b9392505050565b600082825260208201905092915050565b7f54686973206973206120637573746f6d206572726f7200000000000000000000600082015250565b60006101ec6016836101a5565b91506101f7826101b6565b602082019050919050565b6000602082019050818103600083015261021b816101df565b9050919050565b7f43616e206e6f7420657175616c20746f20320000000000000000000000000000600082015250565b60006102586012836101a5565b915061026382610222565b602082019050919050565b600060208201905081810360008301526102878161024b565b9050919050565b6102e28061029d6000396000f3fe608060405234801561001057600080fd5b506004361061002b5760003560e01c806360fe47b114610030575b600080fd5b61004a6004803603810190610045919061015e565b61004c565b005b600081036100935780816040517fd286870d00000000000000000000000000000000000000000000000000000000815260040161008a92919061019a565b60405180910390fd5b600181036100d6576040517f60b7d47e0000000000000000000000000000000000000000000000000000000081526004016100cd90610220565b60405180910390fd5b60028111610119576040517f08c379a00000000000000000000000000000000000000000000000000000000081526004016101109061028c565b60405180910390fd5b8060008190555050565b600080fd5b6000819050919050565b61013b81610128565b811461014657600080fd5b50565b60008135905061015881610132565b92915050565b60006020828403121561017457610173610123565b5b600061018284828501610149565b91505092915050565b61019481610128565b82525050565b60006040820190506101af600083018561018b565b6101bc602083018461018b565b9392505050565b600082825260208201905092915050565b7f54686973206973206120637573746f6d206572726f7200000000000000000000600082015250565b600061020a6016836101c3565b9150610215826101d4565b602082019050919050565b60006020820190508181036000830152610239816101fd565b9050919050565b7f43616e206e6f7420657175616c20746f20320000000000000000000000000000600082015250565b60006102766012836101c3565b915061028182610240565b602082019050919050565b600060208201905081810360008301526102a581610269565b905091905056fea2646970667358221220ef8b5ef70b52d832f31f43977da72bd6547d618e6b0e52e0d432e82b4f55f41164736f6c634300080d0033",
  "nonce": "9",
  "definition": [{
      "inputs": [
        {
          "internalType": "uint256",
          "name": "x",
          "type": "uint256"
        }
      ],
      "stateMutability": "nonpayable",
      "type": "constructor"
    }],
  "errors": [
    {
      "inputs": [
        {
          "internalType": "uint256",
          "name": "x",
          "type": "uint256"
        },
        {
          "internalType": "uint256",
          "name": "y",
          "type": "uint256"
        }
      ],
      "name": "CustomError1",
      "type": "error"
    },
    {
      "inputs": [
        {
          "internalType": "string",
          "name": "x",
          "type": "string"
        }
      ],
      "name": "CustomError2",
      "type": "error"
    }
  ],
  "params":  [
    0
  ]
}

Result

for x: 0:

{
  "error": "FF23021: EVM reverted: CustomError1(\"0\", \"0\")"
}

for x:1:

{
  "error": "FF23021: EVM reverted: CustomError2(\"This is a custom error\")"
}

for x:2 (non-custom error):

{
  "error": "FF23021: EVM reverted: Can not equal to 2"
}

Send transaction

{
  "headers": {
    "id": "904F177C-C790-4B01-BDF4-F2B4E52E6081",
    "type": "SendTransaction"
  },
  "from": "0x3eee66f28ede9bd661e9bab117f9a8f11a47bbfd",
  "to": "0xd56b117200bff05ff5333a6958d34b463a2ab133",
  "nonce": "9",
  "method": {
      "inputs": [
        {
          "internalType": "uint256",
          "name": "x",
          "type": "uint256"
        }
      ],
      "name": "set",
      "outputs": [],
      "stateMutability": "nonpayable",
      "type": "function"
    },
  "errors": [
    {
      "inputs": [
        {
          "internalType": "uint256",
          "name": "x",
          "type": "uint256"
        },
        {
          "internalType": "uint256",
          "name": "y",
          "type": "uint256"
        }
      ],
      "name": "CustomError1",
      "type": "error"
    },
    {
      "inputs": [
        {
          "internalType": "string",
          "name": "x",
          "type": "string"
        }
      ],
      "name": "CustomError2",
      "type": "error"
    }
  ],
  "params":  [
    0
  ]
}

Result:

for x:0:

{
  "error": "FF23021: EVM reverted: CustomError1(\"0\", \"0\")"
}

for x:1:

{
  "error": "FF23021: EVM reverted: CustomError2(\"This is a custom error\")"
}

for x:2:

{
  "error": "FF23021: EVM reverted: Can not equal to 2"
}

jimthematrix · 2022-12-05T02:23:59Z

Test with Besu v22.7.5

besu returns the error details in the eth_estimateGas response.

Deploy contract

for x:0:

{
  "error": "FF23021: EVM reverted: CustomError1(\"0\", \"0\")"
}

for x:2:

{
  "error": "FF23021: EVM reverted: Can not equal to 2"
}

Send transaction

for x:1:

{
  "error": "FF23021: EVM reverted: CustomError2(\"This is a custom error\")"
}

for x:2:

{
  "error": "FF23021: EVM reverted: Can not equal to 2"
}

using transaction payload without the errors spec, and x:0:

{
  "error": "FF23022: EVM reverted: 0xd286870d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
}

Signed-off-by: Jim Zhang <[email protected]>

peterbroadhurst · 2022-12-06T22:51:31Z

https://github.com/hyperledger/firefly-signer/releases/tag/v1.1.4 is ready to pull in @jimthematrix

peterbroadhurst · 2022-12-06T22:51:53Z

go.mod

-	github.com/hyperledger/firefly-transaction-manager v1.1.2
-	github.com/sirupsen/logrus v1.8.1
+	github.com/hyperledger/firefly-common v1.1.4
+	github.com/hyperledger/firefly-signer v1.1.3


Need to pull in https://github.com/hyperledger/firefly-signer/releases/tag/v1.1.4

internal/ethereum/exec_query.go

peterbroadhurst · 2022-12-06T22:57:34Z

internal/ethereum/exec_query.go

+				}
+			}
+		}
+		return outputData.String(), fmt.Errorf("raw data returned")


We shouldn't have any fmt.Errorf() - instead adding to the translated error file.

this is a "transient" error as it simply signals to the caller that the string was going to be the raw data, then it gets thrown away. as such it doesn't need to be translated. but if the coding style requires translation, I'd be happy to add it. please let me know.

Details are in the comment of the method processRevertReason():

// processRevertReason returns under 3 different circumstances: // 1. non-empty string with nil error: valid reason has been successfully parsed // 2. non-empty string with non-nil error: error detail was present but failed to parse, string was raw data // 3. empty string with nil error: outputData is NOT an error detail data

A sentinel error would be a better pattern than a string constant, if you wouldn't mind.

Or simply a boolean ok return would be clearer in the case there's not an error for the caller to process, simply an indication that the data couldn't be extracted?

I would like us to have a linter that checks for Errorf anywhere in the code at some point.

Also reading the code caught me out because above err != nil didn't mean there was an error to process, but instead it meant that the function didn't complete its task.

thanks for suggesting that. went with the boolean return value pattern.

peterbroadhurst

This is great, thanks for working through the 1.10 change too.
Couple of very simple requests (along with pulling the new dependency releases in) before merg.

Signed-off-by: Jim Zhang <[email protected]>

peterbroadhurst

Thanks @jimthematrix 🙇

nedgar · 2022-12-12T20:07:33Z

Thanks @jimthematrix. With this, callers will be able to distinguish custom errors from regular reverts, but they'll have to parse the error string for details. It would be nice if the error signature, error name, and param values could be provided as separate fields in the result so that the client can dispatch according to which error was thrown without having to parse.

Add support for parsing custom errors

0f471b1

Signed-off-by: Jim Zhang <[email protected]>

jimthematrix requested review from peterbroadhurst and nguyer as code owners November 22, 2022 22:14

jimthematrix mentioned this pull request Nov 22, 2022

Add Errors to transaction input to support custom errors hyperledger/firefly-transaction-manager#42

Merged

Formatting update

9f5123d

Signed-off-by: Jim Zhang <[email protected]>

peterbroadhurst reviewed Nov 30, 2022

View reviewed changes

jimthematrix added 2 commits November 30, 2022 15:10

Refactor error formatting function; added unit test; dependency updates

9472ec6

Signed-off-by: Jim Zhang <[email protected]>

Always look inside response object for errors

58fc122

Signed-off-by: Jim Zhang <[email protected]>

Use RPCError as return from CallRPC() to catch error details

6e27599

Signed-off-by: Jim Zhang <[email protected]>

jimthematrix changed the title ~~Add support for parsing custom errors~~ Add support for parsing custom errors & fixing revert error propagation Dec 5, 2022

peterbroadhurst reviewed Dec 6, 2022

View reviewed changes

internal/ethereum/exec_query.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed Dec 6, 2022

View reviewed changes

internal/ethereum/exec_query.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed Dec 6, 2022

View reviewed changes

peterbroadhurst requested changes Dec 6, 2022

View reviewed changes

jimthematrix added 2 commits December 6, 2022 20:33

Update to latest versions of FFTM and signer

2e6032c

Signed-off-by: Jim Zhang <[email protected]>

Addressing review comments

aaeb621

Signed-off-by: Jim Zhang <[email protected]>

peterbroadhurst mentioned this pull request Dec 8, 2022

Update FFTM to v1.1.4 #43

Closed

Minor adjustment to error situations when parsing revert reasons

c730c1e

Signed-off-by: Jim Zhang <[email protected]>

peterbroadhurst approved these changes Dec 12, 2022

View reviewed changes

peterbroadhurst merged commit 21f56bd into hyperledger:main Dec 12, 2022

peterbroadhurst deleted the custom-errors branch December 12, 2022 16:08

This was referenced Jan 1, 2023

Add custom error support for FFI hyperledger/firefly-common#42

Merged

Custom error support in Ethereum ABI hyperledger/firefly-signer#31

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parsing custom errors & fixing revert error propagation #42

Add support for parsing custom errors & fixing revert error propagation #42

jimthematrix commented Nov 22, 2022 •

edited

Loading

peterbroadhurst left a comment

peterbroadhurst Nov 30, 2022

peterbroadhurst Nov 30, 2022

jimthematrix commented Dec 4, 2022

jimthematrix commented Dec 5, 2022 •

edited

Loading

peterbroadhurst commented Dec 6, 2022

peterbroadhurst Dec 6, 2022

peterbroadhurst Dec 6, 2022

jimthematrix Dec 7, 2022 •

edited

Loading

peterbroadhurst Dec 8, 2022

jimthematrix Dec 12, 2022

peterbroadhurst left a comment

peterbroadhurst left a comment

nedgar commented Dec 12, 2022

Add support for parsing custom errors & fixing revert error propagation #42

Add support for parsing custom errors & fixing revert error propagation #42

Conversation

jimthematrix commented Nov 22, 2022 • edited Loading

peterbroadhurst left a comment

Choose a reason for hiding this comment

peterbroadhurst Nov 30, 2022

Choose a reason for hiding this comment

peterbroadhurst Nov 30, 2022

Choose a reason for hiding this comment

jimthematrix commented Dec 4, 2022

Test with Geth 1.10

Deploy contract

Result

Send transaction

Result:

jimthematrix commented Dec 5, 2022 • edited Loading

Test with Besu v22.7.5

Deploy contract

Send transaction

peterbroadhurst commented Dec 6, 2022

peterbroadhurst Dec 6, 2022

Choose a reason for hiding this comment

peterbroadhurst Dec 6, 2022

Choose a reason for hiding this comment

jimthematrix Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

peterbroadhurst Dec 8, 2022

Choose a reason for hiding this comment

jimthematrix Dec 12, 2022

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

nedgar commented Dec 12, 2022

jimthematrix commented Nov 22, 2022 •

edited

Loading

jimthematrix commented Dec 5, 2022 •

edited

Loading

jimthematrix Dec 7, 2022 •

edited

Loading