Expose contract call reverts to mappings

[leoyvens]

This is the plan for fixing the issue first reported in graphprotocol/support#21.

Rationale / Use Cases

Several subgraphs have had the need to handle reverts in a contract call, currently the behaviour of reverts is not consistent, depending on the ethereum node's response we either fail the subgraph or return the revert data as the return value, neither of which are desirable. We should change this so that the semantics are:

• Useful, allowing a mapping to handle the revert.
• As consistent as possible, considering that the representation of a revert in the JSON-RPC interface is not standardized.

Requirements

• Mappings should be able to handle a revert.
• The node should recognize reverts on the most common ethereum clients.

Proposed User Experience

A first question is if we should attempt to expose the revert reason. For a first implementation I'd say no, it's hard enough to determinstically determine if the call was reverted or not, and I don't believe the current use cases have a necessity of inspecting the revert reason.

The AssemblyScript return value of a contract call will need to change so to account for the possibility of a revert. Since try/catch mechanisms are not an option in AS, the mapping will need to explicitly handle the result value. To maintain convenience for calls that are assumed to not revert and to avoid breaking changes, we'll continue to generate call functions as we do today, and additionally generate a version prefixed with try_ which returns a result value, as described by @Jannis in this comment.

Proposed Implementation

In the node, the ethereum-adapter will detect reverts based on how they're presented by the latest stable of Geth and Parity, and by the current responses of Infura and Alchemy. The ethereum.call host export will return null in case of a revert.

In graph-ts, the following type will added:

class CallResult<T> {
    private _value: T,
    isOk: bool
    
    get value(): T {
    	if (isOk) {
    		return _value;
    	} else {
    		abort(....)
    	}
    }
}

In graph-cli, for each contract call two functions will be generated, one that aborts on revert and one that exposes the revert. The first will be the same as the functions we already generate, with the change that they will check for null and abort with a good error message in that case. The second will prefixed with try_ and return a CallResult<T> where T is the return type of the call. If the call returned null, then value will be something meaningless and isOk will be false.

Proposed Documentation Updates

The section 'AssemblyScript API -> Access to Smart Contract State' should be updated to describe when and how to use the try_ functions.

Proposed Tests / Acceptance Criteria

• Unit tests for codegen.

• Manual end-to-end testing of a subgraph calling try_ functions against the latest stable of Geth and Parity, and also Infura and Alchemy.

Tasks

• (0.5d) Find or make good reproduction case.
• (1d) graph-node: Detect and test Parity-style reverts.
• (1d) graph-node: Detect and test Geth-style reverts.
• (0.5d) graph-node: Make sure Infura and Alchemy match either the Parity or Geth response.
• (0.2d) graph-node: Return null from the ethereum.call host export in case of a revert.
• (0.1d) graph-ts: Add class CallResult<T>
• (0.5d) graph-cli: Check for null in the generated call methods.
• (1d) graph-cli: Generate try_ variant of call methods.
• (0.2d) docs: Update documentation.
• (1d) End-to-end testing.

[yanivtal]

This looks like a solid plan. My only feedback is that I think we've had at least one request for accessing the revert reason. I think Justin from Synthetix asked for it so it may be worth trying to include depending on the cost.

[Jannis]

I agree, if that's possible that would be great. I'm not sure it is though.

That aside, the plan looks great!

[leoyvens]

@yanivtal @Jannis thanks for the review! You are referring to #1055, which is about having handlers for reverted transactions so not directly related to this.

[Jannis]

@leoyvens By "revert reason" we mean the reason why the Ethereum call failed. That's different from transactions failing.

Let's say you call a view function function numberOfKitties() public view returns (uint) from a mapping and this function has an assertion like this in it:

require(someVariable % 2 == 0, "Even value required.");

then it would be great if we could get the assertion error back and pass it back to the mapping.

[leoyvens]

@Jannis I understand, that's further client/language specific behaviour so we should wait for an EIP like ethereum/EIPs#838 before exposing that.

I've been investigating in what scenarios we can detect a revert. I've tested against Infura and Alchemy, I believe Infura follows the Geth response format and Alchemy follows the Parity response format. I've tested reverts with and without a reason string.

The conclusion so far is that we can detect a revert in Alchemy/Parity in all tested situations, but for Infura/Geth we'll only be able to detect reverts in Solidity contracts that call revert or require with a reason string. Ganache is not yet tested.

[jjgonecrypto]

Hey guys - this is looking really promising. Just a few questions:

1. In the case where, within Graph processing, I want to invoke a secondary smart contract and I want to try/catch a function that may not yet exist at that block I can expect call to return null and tryCall to return a CallResult, correct?
2. From what I ascertain, getting the revert reason is out of scope due to the reasons mentioned above.
3. Finally, is there a way I could actually get a handler to be invoked when a failure resulted from a function invocation? E.g. if a transaction invoked transfer on an ERC20 contract which ended up Out of gas or Reverted, could a mapping be made to handle and track that? (So we could get accurate numbers on the success rate our users have).

[leoyvens]

@justinjmoses hey, thanks for dropping by! Answers:

1. Yes, this should also catch calling a non-existing function on a contract.
2. When able do extract it, we do log the revert reason, but exposing it to the mapping is something we don't do. Because of the concerns about this not being standardized and also because there wasn't a motivating use case where the mapping needs the revert reason, do you have one?
3. That is Add support for failed transactions #1055, which we didn't forget about but also didn't get to investigate more yet.

[jjgonecrypto]

Thanks @leoyvens.

2. I only really want the revert reason while processing failed transactions (my 3. above) - not during invocations in mapping code - so I think we're aligned here.