Improve eth-abi decodeParameters error message

[cgewecke]

Addresses #3134

Also adding a couple small tests / e2e fixtures for 'method.call', because those are missing.

There's discussion in #3134 about whether current error makes sense at all because it mentions OOG in a context where that's unlikely to be a cause. Am very reluctant to change error messages in 1.x because people may be grepping them to handle certain error conditions, so have just appended some more info that might be helpful.

[barakman]

Just to clarify, I have initially opened #3134 because I was grepping error.message.startsWith("Couldn't decode") while using web3 version 1.0.0-beta.34 (a very stable and reliable version BTW).

After upgrading to version 1.2.1, I stumble across this issue, but as a runtime error which occurred long after I had upgraded web3. As a result, I had to conduct a rather long investigation before I was able to figure out the cause.

So I definitely agree that this would be an API-breaking change for those already using version 1.2.1, and that it should therefore not be changed at any future version 1.x.

Side note for readers stumbling across the same problem:
I have ultimately resorted to relying only on a single error-message, namely:

error.message.startsWith("Invalid JSON RPC response")

So it could be a solution (or a workaround if you will) for some of you.

[nivida]

@cgewecke We can keep the new error msg in the ABICoder as it is but we have to back-port the fix from the CallContractMethod. I was following several discussions in Geth issues about the correct handling of this case and the overall opinion and also mine was to return null.

[cgewecke]

the correct handling of this case and the overall opinion and also mine was to return null.

@nivida Ok, interesting. Is this a breaking change though?

Do you remember where this was discussed at geth so it can be cross-documented here when the change is made?

[nivida]

@cgewecke

Ok, interesting. Is this a breaking change though?

Returning null instead of throwing an error if the requested value isn't available because of a block number defined in the future or if the node isn't in sync is in my view of point the more accurate behavior instead of throwing the current confusing error on for example a contract method call. I think this is such a small breaking change and does fix a "bug" (clarifies the situation) that we actually could do this on the 1.2.3 release.

Do you remember where this was discussed at geth so it can be cross-documented here when the change is made?

I couldn't find it but I'm pretty sure the last research I've done for the fix in 2.x brought this conclusion up.

[cgewecke]

@nivida I agree the error message is not good but changing behavior from throwing an error to not throwing one could be a big difference to someone - it's very hard to anticipate how people rely on this....

To me it seems the safest improvement is to list the known conditions which trigger this. Will add the two other cases you've mentioned.

[coveralls]

Coverage remained the same at 84.347% when pulling e9f6cfa on cgewecke:issue/3134 into a9c3420 on ethereum:1.x.

[nivida]

@cgewecke I will keep issue #3134 open for later (1.3.0) implementing an improved solution for this bug. We could, for example, do further node-state-related JSON-RPC requests to determine more details and to return a more defined error msg.