Bug fix: Provide reason for calls #2340
Conversation
… transaction reverted
|
NOTE: I'm not sure I fully understand the role of the file |
|
Sounds like a great idea, comin' right up! |
…r calls and sends
|
@cgewecke if you're around, do you remember if there was a reason we didn't implement this in the first place? |
|
Calls don't return a receipt with a status and that's how we (or web3) detect whether or not a failure occurred. If you ran these changes against geth or ganache with Very unclear to me if WDYT? |
|
Are you basically saying that against a node in production that any call that reverts won't be caught by that |
|
|
||
| describe("revert reasons", function() { | ||
| let RevertingContract; | ||
| const providerOptions = { vmErrorsOnRPCResponse: true }; // <--- TRUE |
There was a problem hiding this comment.
@eggplantzzz If you set this option to false (e.g emulate production nodes) does the view test pass?
There was a problem hiding this comment.
The view test fails when I set it to false. I think it is true by default as it passes when I don't give any options to util.setUpProvider...weird. Actually this got lifted from the errors test in truffle-contract. Maybe it should be removed.
There was a problem hiding this comment.
But the point here I suppose is that I wasn't aware that this was production node behavior, in which case this PR probably shouldn't go through. Although I can see value in it for testing purposes I suppose. I dunno, what do you think @gnidan? You can make the call.
There was a problem hiding this comment.
Yeah, the tests are crazy because there are two clients (geth and ganache) and then ganache can be run in these two modes. And responses vary across those. IIRC responses varied depending on whether ganache was run as a provider or a server when these tests were written, so it's a mess. There was also a lot of discussion about what ganache's default run mode should be . . .
I think the risk here is that TC makes a design pattern seem like it will work in the wild and people don't do due diligence re: their tests passing vs. a reference client.
There was a problem hiding this comment.
Yeah, that is my worry as well. It seems like it could be useful but it also doesn't seem good to create expectations about what will happen in prod.
There was a problem hiding this comment.
Gosh I forgot that the JSON RPC completely excludes any kind of status information at all for eth_call.
I concur with @cgewecke about this being a dangerous idea. I think we should hold off until we do something like run the call though ethereumjs-vm ourselves to get this data.
(cc @haltman-at to try to put the idea in your head)
There was a problem hiding this comment.
Oh geez, I didn't realize this was an issue -- thanks for notifying me, because this actually potentially affects some stuff I've been thinking about.
But, I'm a little confused here. At least with Ganache, running normally, on a revert, the response -- whether to a call or a sendTransaction -- does not have the usual form specified by the RPC, but rather puts all its info in an error field, and if you dig into that you can find the revert message (both decoded by Ganache and in its raw binary form). In the case of a sendTransaction, this allows getting the return value (which normally isn't possible); and in the case of a call, this allows getting the status (which normally isn't possible).
Being largely unfamiliar with the other ethereum clients, I didn't realize that this behavior was not universal, or could be turned off in Ganache, forcing JSON-RPC-conforming (but less informative) responses even on a revert. Is that indeed the case? Am I understanding this correctly? @cgewecke, can you provide any clarity here? I would really like to know when I should expect a standard RPC response even on error, vs when I should expect this other error format. (Or are there other alternatives out there that I haven't even seen...?) Or at the least I would like to know for a certainty whether I do potentially have to handle both. (Not that I'm going to get to this stuff for quite a while, but...)
Anyway thank you! I'd been wondering about this question so I'm glad I was tagged here.
There was a problem hiding this comment.
@haltman-at The production clients do not attach an error field. That's a convenience ganache/testrpc added in order to be an effective chain simulator for tests in the days before transaction receipts included a status field.
Truffle has fixtures to run tests against an insta-mining geth client (@CruzMolina knows all about) and ganache has a flag you can enable to make it more geth-like as well. (See above)
There was a problem hiding this comment.
Wow, I totally didn't realize. So that's not a standard thing at all, huh. OK, thanks a bunch, I'll have to account for that!
@eggplantzzz yes, I think this is correct. The tests are using ganache in a "pre-byzantium" mode which errors the client response. This behavior was necessary before status fields were added to the receipts to do any sort of meaningful contract testing, but it's not how production clients respond. |
|
I am going to close this for now. |
Fixes #2321
Previously, reasons for reverts were provided only when a "send" was reverted. This PR extracts a method that parses out a revert reason and uses it when "call"s revert as well.