Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nguyer opened this issue Feb 14, 2024 · 10 comments
Open

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

nguyer opened this issue Feb 14, 2024 · 10 comments

Comments

@nguyer
Copy link
Contributor

nguyer commented Feb 14, 2024

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
Copy link
Contributor

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
Copy link
Contributor

@peterbroadhurst
Copy link
Contributor

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

@peterbroadhurst
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

nguyer commented Feb 20, 2024

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
Copy link
Contributor

@matthew1001 @peterbroadhurst I think this one is done

@matthew1001
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants