Handling function signatures with varying return values

[davekaj]

Somewhat related to #892 . Once we solved that issue, we ran into a new issue. The above issue solves calling a function that does not exist.

But now we ran into the following issue, which I will exaplain below. Right now I have the follow abi used in the dynamic data sources for Uniswap:

  {
    "constant": true,
    "inputs": [],
    "name": "symbol",
    "outputs": [
      {
        "name": "",
        "type": "string"
      }
    ],
    "payable": false,
    "stateMutability": "view",
    "type": "function"
  },

But, unfortunately some of the ERC20 tokens on uniswap decided to have symbol return bytes32 instead of string. Some also return bytes32 instead of uint for decimals. So we end up calling the right function, but the graph node expects a string , and then it crashes because it doesn't have that. Something to note as well, solidity function signatures are not based on the return values, , so these functions will have the same function signatures.

The overarching problem here is making sure graph node can handle ambiguous return values for contract signatures. But this can only really happen if we have the wrong ABI, and are using dynamic data sources. In the case of Uniswap, it is a rare one, where over 1000 dynamic data sources exist, and it is ERC20, so I can't think of another case where this would happen. I think right now it would only happen with ERC20 and ERC721 dynamic subgraphs. (Note, this MAY effect the ERC20 subgraph that protofire is working on

Would be interested in hearing what @leoyvens and @Jannis and @fordN have to say, since me and Ford talked about this yesterday night, and Leo and Jannis were involved in the last issue.

We could provide multiple return value json function signatures for symbol, and get this back:

  symbol(): string {
    let result = super.call("symbol", []);

    return result[0].toString();
  }

  try_symbol(): CallResult<string> {
    let result = super.tryCall("symbol", []);
    if (result.reverted) {
      return new CallResult();
    }
    let value = result.value;
    return CallResult.fromValue(value[0].toString());
  }

  symbol1(): Bytes {
    let result = super.call("symbol", []);

    return result[0].toBytes();
  }

  try_symbol1(): CallResult<Bytes> {
    let result = super.tryCall("symbol", []);
    if (result.reverted) {
      return new CallResult();
    }
    let value = result.value;
    return CallResult.fromValue(value[0].toBytes());
  }

And then, hard code in specific contracts that need to use bytes32 instead of string. But that is hacky, and could be broken in the future by someone deploying a contract to uniswap on purpose to break it.

Is there a way we can graciously handle the value returned to us, and then decide to convert it to a string, if it is returned as a string or bytes32?

Also note, this is an important issue for Uniswap, because we have to hardcode in all exchanges because dynamic data sources do now work, and this is causing us to have to redeploy uniswap every few weeks, because every few weeks a few more exchanges have been added, and we need to hardcode those in.

[davekaj]

Ford wrote yesterday:

Would having another form of ethereum.call() that allows you to supply an ABI string for function signature, (input, output, name) be enough to make this work?
Ford 9:23 PM
So in the mappings you’d have access to an additional method on each contract class.  Maybe something like:
 dynamic_call(name: string, inputs: string | null, outputs: string | null): EthereumValue { }

I think having another form of ethereum.call() which allows you to supply the return values could be of use here

[ianlapham]

Great overview of the issue. This owuld be huge to fix for Uniswap so new exhcnages automatically get tarcked.

[davekaj]

So, this open PR will actually fix this! #1375 (review)

From now on, decode failures will be considered as reverts. So you will be able to catch that and then try calling with a different ABI.

The only thing is, we will have to wait until it is on the hosted service. Which could be about a week or so

[ianlapham]

Amazing - thats fine we can wait until thats deployed. Thanks for pushing this through.

[leoyvens]

Because that PR has been merged, I'll go ahead and close this.