graphprotocol / support

Community support for the Hosted Service of The Graph
https://thegraph.com/
10 stars 3 forks source link

`try_` calls don't handle ABI decoding errors / mismatches #33

Closed Jannis closed 4 years ago

Jannis commented 5 years ago

Contract calls made with try handle assertion failures in contracts but they don't currently handle ABI decoding errors / ABI mismatches.

For example, different contracts could implement a name function with different return types. A call like

let result = contract.try_name()
if (!result.reverted) {
  let s: string = result.value
}

currently works with contracts that have a name function that returns a string, but will fail with contracts that have a name function that returns e.g. a bytes32.

try exists not just to detect reverts (although that was its original purpose), but could also allow to test whether a method can be called on a contract at all. Cases that would be good to cover:

  1. The function doesn't exist on the contract.
  2. The function ABI doesn't match (i.e. the result fails to decode).

One idea could be to add an resultDecodingFailed field similar to reverted so you could do

let result = contract.try_name()
if (!reverted && !result.resultDecodingFailed) {
  let s: string = result.value
}
leoyvens commented 5 years ago

This seems reasonable, after this we should be more careful when making changes to the decoding logic, for example if we fix a bug to successfully decode something that was failing, we need to version because it's a breaking change.

jjgonecrypto commented 5 years ago

This would be a boon for us at Synthetix. We emit events on a proxy contract and the underlying ABIs have shifted over time.

leoyvens commented 5 years ago

To clarify, only a very specific ABI change should be affected by this, which is when you call a function with the same name and parameters as the one in the subgraph's ABI, but the return type has changed.

If the name or input types change, we should already catch it and reverted should be true.

jjgonecrypto commented 5 years ago

Ah so in that case we shouldn't be impacted by it. It seemed we were running into this with different argument parameters but perhaps not; if I can repro it I will post here.

wighawag commented 5 years ago

This seems to be a general issue, graph-node crash the whole subgraph when decoding fails.

See : https://github.com/graphprotocol/graph-ts/issues/85