hyperledger / firefly

Hyperledger FireFly is the first open source Supernode: a complete stack for enterprises to build and scale secure Web3 applications. The FireFly API for digital assets, data flows, and blockchain transactions makes it radically faster to build production-ready apps on popular chains and protocols.
https://hyperledger.github.io/firefly
Apache License 2.0
508 stars 209 forks source link

Transaction failure reason not visible when using fixed gas amount #1466

Open nguyer opened 9 months ago

nguyer commented 9 months ago

FireFly normally reports custom error messages all the way back to the API response which is really helpful for developers. Typically, if a transaction will fail, it will also fail the gas estimation step which happens synchronously with submitting the transaction, so the custom error message is reported immediately.

If I submit a transaction, by invoking a custom contract API and adding a fixed gas amount, the behavior is different.

{
  "input": {},
  "options": {
    "gas": "20000"
  }
}

The request is accepted with a 202 and internally gas estimation is skipped (I think). This results in a failed transaction, but my custom error message does not show up in the Operation Output or Details so it is really hard to know why the transaction failed, especially if I have complex logic that could reject the transaction for one of several reasons.

peterbroadhurst commented 9 months ago

Example of how this can be acheived:

{
    "jsonrpc": "2.0",
    "id": 1,
    "method": "debug_traceTransaction",
    "params": ["0x5126bbcb9b77a26fecd2ece4b8c96e2794aaccd997fa6beffb06a0405dc270b2", {"enableReturnData": true}]
}

    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "gas": 21476,
        "failed": true,
        "returnValue": "08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003706f700000000000000000000000000000000000000000000000000000000000",
        "structLogs": [...]
peterbroadhurst commented 9 months ago

There does seem to be a well worked function for this in fact: https://github.com/hyperledger/firefly-evmconnect/blob/35424990511fcc7677840ee8f335a4bd24684050/internal/ethereum/get_receipt.go#L145-L176

peterbroadhurst commented 9 months ago

I do see the error going into receipt.extraInfo.errorMessage for EVM:

image
peterbroadhurst commented 9 months ago

Above was a simple revert, but here I see with a structured error it can't be decoded - so the problem might be the ABI definition of the errors aren't being passed down to the code:

image
peterbroadhurst commented 9 months ago

For ref, the simple contract I'm using is:

pragma solidity ^0.8.0;

contract reverter {
  uint public storedData;

  error Pop(string message, uint256 value);

  constructor() {}

  function pop1() pure public {
    revert("pop");
  }

  function pop2() pure public {
    revert Pop("bang", 12345);
  }
}
peterbroadhurst commented 9 months ago

Yes - that's the problem. We need to work on FFTM to pass through the error structure from the original TX down into the TransactionReceiptRequest to be able to decode the errors.

I suggest at the same time, we promote errorMessage to be a top-level field in the receipt response.

peterbroadhurst commented 9 months ago

Ok - FFTM does not actually store the error ABI details, they are held in FireFly core and only passed on prepare.

They are a big glob of JSON that is associated with the interface, not the transaction, so storing them in FFTM is a big performance overhead.

So a more practical approach is to store the result byes from the receipt in FFTM (as for a non-archive node they won't be available indefinitely), and provide an API to decode them that is passed the error ABI.

FireFly core could call this API automatically.

nguyer commented 9 months ago

I'm seeing a slightly different behavior with a require() in my smart contract. Here is the test contract that I am using:

// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.10;

// Declares a new contract
contract SimpleStorage {
    // Storage. Persists in between transactions
    uint256 _x;

    // Allows the unsigned integer stored to be changed
    function set(uint256 x) public {
        _x = x;
        emit Changed(msg.sender, _x);
    }

        // Allows the unsigned integer stored to be changed
    function setIf(uint256 x, bool actuallyDoIt) public {
        require(actuallyDoIt, "Not going to do it");
        _x = x;
        emit Changed(msg.sender, _x);
    }

    // Returns the currently stored unsigned integer
    function get() public view returns (uint256 x) {
        return (_x);
    }

    event Changed(address indexed from, uint256 x);
}

If I set actuallyDoIt to false here is what I see in my receipt in FireFly:

Screenshot 2024-02-20 at 11 55 54 AM

Notice how "errorMessage": null is not even in my receipt here

So this is a little different than what @peterbroadhurst was seeing with revert() or a custom error. We want to make sure this case is covered by the fix for this as well.

EnriqueL8 commented 3 months ago

@matthew1001 @peterbroadhurst I think this one is done

matthew1001 commented 3 months ago

I believe so, with the exception of custom error ABI parsing as described in some of the comments.