This repository has been archived by the owner on Feb 26, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Bug fix: Provide reason for calls #2340
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
const { assert } = require("chai"); | ||
const util = require("./util"); | ||
|
||
describe("revert reasons", function() { | ||
let RevertingContract; | ||
const providerOptions = { vmErrorsOnRPCResponse: true }; // <--- TRUE | ||
|
||
before(async function() { | ||
this.timeout(10000); | ||
|
||
RevertingContract = await util.createRevertingContract(); | ||
|
||
await util.setUpProvider(RevertingContract, providerOptions); | ||
}); | ||
|
||
it("provides a reason when a function reverts", async () => { | ||
try { | ||
let instance = await RevertingContract.new(); | ||
await instance.revertingFunction(); | ||
assert.fail(); | ||
} catch (error) { | ||
assert(error.reason, "Too reverty of a function"); | ||
} | ||
}); | ||
|
||
it("provides a reason when a view function (call) reverts", async () => { | ||
try { | ||
let instance = await RevertingContract.new(); | ||
await instance.revertingView(); | ||
assert.fail(); | ||
} catch (error) { | ||
assert(error.reason, "Too reverty of a view"); | ||
} | ||
}); | ||
}); |
17 changes: 17 additions & 0 deletions
17
packages/truffle-contract/test/sources/RevertingContract.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
pragma solidity ^0.5.0; | ||
|
||
contract RevertingContract { | ||
uint256 public value; | ||
|
||
constructor() public { | ||
value = 0; | ||
} | ||
|
||
function revertingView() public view { | ||
require(value > 0, "Too reverty of a view"); | ||
} | ||
|
||
function revertingFunction() public { | ||
require(value > 0, "Too reverty of a function"); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5775,7 +5775,7 @@ [email protected], ethjs-util@^0.1.3, ethjs-util@^0.1.6: | |
is-hex-prefixed "1.0.0" | ||
strip-hex-prefix "1.0.0" | ||
|
||
ethpm-registry@^0.1.0-next.3: | ||
[email protected]: | ||
version "0.1.0-next.3" | ||
resolved "https://registry.yarnpkg.com/ethpm-registry/-/ethpm-registry-0.1.0-next.3.tgz#e335e7f57cda3c1cead388db6833903121773e9c" | ||
integrity sha512-PZZ1wo7lf2J3xWfvdEHzG6gJzh2wJiw8jzBcLgqOicLQoxVzgzCCcF6GZKakIeDZfMilrP/0fm7uu5PviZzECg== | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The view test fails when I set it to
false
. I think it istrue
by default as it passes when I don't give any options toutil.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!