Open valentinfernandez1 opened 1 year ago
real props to @arturgontijo for helping me find this needle in a haystack 😛
Update: When recreating the same setup on hardhat the issue seems to disappear and the error for a reverted transaction is returned properly. So this seems to be a truffle specific problem.
Executed test on hardhat
it("should revert transfer to zero address", async () => {
let zero_address = "0x0000000000000000000000000000000000000000";
let expected_revert_reason = 'ERC20: transfer to the zero address';
const {deployedToken, accounts} = await deployToken();
console.log("---log the actual result of executing the transaction---")
try {
await deployedToken.transfer(zero_address, 10) //will revert
} catch (tx_err) {
console.log(tx_err)
}
})
Result
ProviderError: VM Exception while processing transaction: revert ERC20: transfer to the zero address
Interesting Issue posted on ethers.js.
Update: The result that can be observed in polkadot.js is different when executing the same test on truffle and hardhat is completely different.
If we run the transaction on hardhat we can se that the revert message is properly returned but in polkadot.js the emitted event is:
If we execute the same transaction sent from truffle, we face the issue with the revert reason on truffle but the event emitted on polkadot.js is:
Clearly the transactions from truffle and hardhat are going trough different paths of the frontier code since the resulting event emitted is different. I suppose the next stage would be try to catch the transaction being sent from both frameworks to detect what is the difference in the transactions to see if that gives us a hint as to why they are resulting in different responses.
So I have some interesting findings that I made in the past few days
Firstly, there is a way to mitigate this issue on the Truffle side, when writing tests. When calling a method from the contract, it can be done in the traditional way which is:
await deployedToken.transfer(zero_address, 10)
As I described previously, this reverts, but it doesn't provide the revert reason. Another way to handle this is by using a Solidity call. We can achieve this by appending .call(params)
to the function we want to execute, like so:
await deployedToken.transfer.call(zero_address, 10)
This returns the proper result, as can be seen in the execution logs:
---log the actual result of executing the transaction---
VM Exception while processing transaction: revert ERC20: transfer to the zero address
And the event emitted is different from the one I showed previously:
However, it's important to note that this is not a long-term solution, as it requires Solidity developers to manually add this change to any existing tests they have. Until this is fixed on Frontier's side, it can serve as a temporary workaround.
Have you tried to change anything on frontier
code?
It looks like Truffle is not able to parse the revert reason as hardhat is.
I'd start changing revert
to reverted
here:
https://github.com/paritytech/frontier/blob/master/client/rpc/src/eth/execute.rs#L978
If that doesn't work, it would be good to have the responses for that transfer(zero_address, 10)
call when sending it to frontier
, hardhat node
and truffle develop
via curl
.
While exploring the previous workaround, I encountered another issue related to testing revert conditions in a contract's constructor. As mentioned earlier, we can mitigate the revert issue by using .call(params)
. However, this approach cannot be applied to a contract's constructor. Let me explain the problem with an example.
Suppose we have a contract, and its constructor contains a require clause like this:
constructor(uint256 initialSupply) ERC20("MyToken", "MYTOK") {
require(initialSupply > 0, "init supply 0 not valid");
}
Testing the deployment of this contract without triggering the require statement works without any issues. Now, when we attempt to test whether it returns the correct revert reason using Truffle, we write a test case like this:
it.only("revert when init supply 0", async () => {
let token = await Token.new(0, { from: accounts[0] });
console.log(token);
});
The result we get is:
Error: The contract code couldn't be stored, please check your gas limit.
This error indicates that the contract couldn't be stored due to a lack of gas. This is puzzling because when the require
statement didn't trigger a revert, there were no issues with gas. However, now that the contract reverts due to the require, we encounter a gas-related problem. Strangely, when running this test with Hardhat, there is no issue, and we receive the proper revert message. Additionally, the event emitted by this call is as follows:
My guess is that similar to the previous issue when calling contract functions, this case also doesn't return the revert reason, leading to a generic message being returned, in this case the lack of gas.
We just need to change this line https://github.com/paritytech/frontier/blob/e89f33688e0e438d0d26ca4f5eaba16a17201a22/client/rpc/src/eth/execute.rs#L978C3-L978C3 to contain "execution reverted" and that should fix the problem.
We just need to change this line https://github.com/paritytech/frontier/blob/e89f33688e0e438d0d26ca4f5eaba16a17201a22/client/rpc/src/eth/execute.rs#L978C3-L978C3 to contain "execution reverted" and that should fix the problem.
I tried changing that to VM Exception while processing transaction: execution reverted
but it didn't work.
Am I right that your Truffle's repo is even breaking when running it agains the Truffle's develop chain?
##### Using expect from openzeppelin/test-helpers #####
1) should revert transfer to zero address
Events emitted during test:
---------------------------
[object Object].Transfer(
from: <indexed> 0x0000000000000000000000000000000000000000 of unknown class (type: address),
to: <indexed> 0x3dd4B8B9046c5643323E1E2b76173F60da12f74e of unknown class (type: address),
value: 8000000000000000000000000 (type: uint256)
)
---------------------------
3 passing (353ms)
1 failing
1) Contract: MyToken
should revert transfer to zero address:
Wrong kind of exception received
+ expected - actual
-Transaction: 0x4c385fd070b87ef4f840eb580319135e0f45b83b1c33ee330f3b2a4762c67a8b exited with an error (status 0).
- Please check that the transaction:
- - satisfies all conditions set by Solidity `require` statements.
- - does not trigger a Solidity `revert` statement.
+ERC20: transfer to the zero address
at expectException (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:20:30)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at expectRevert (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:74:3)
at Context.<anonymous> (test/ERC20_revert.js:58:9)
More findings that can give us more light here:
When sending eth_estimateGas
for that transfer(ZeroAddress, 1)
, here are the responses that I've got from multiple clients:
Hardhat Node:
{
"jsonrpc": "2.0",
"id": 1,
"error": {
"code": -32603,
"message": "Error: VM Exception while processing transaction: reverted with reason string 'ERC20: transfer to the zero address'",
"data": {
"message": "Error: VM Exception while processing transaction: reverted with reason string 'ERC20: transfer to the zero address'",
"data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002345524332303a207472616e7366657220746f20746865207a65726f20616464726573730000000000000000000000000000000000000000000000000000000000"
}
}
}
Truffle develop
{
"id": 1,
"jsonrpc": "2.0",
"error": {
"message": "VM Exception while processing transaction: revert ERC20: transfer to the zero address",
"stack": "RuntimeError: VM Exception while processing transaction: revert ERC20: transfer to the zero address\n at exactimate (/Users/arturgontijo/workspace/parity/mine/frontier-revert-issue/node_modules/ganache/dist/node/webpack:/Ganache/chains/ethereum/ethereum/lib/src/helpers/gas-estimator.js:257:23)",
"code": -32000,
"name": "RuntimeError",
"data": {
"hash": null,
"programCounter": 788,
"result": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002345524332303a207472616e7366657220746f20746865207a65726f20616464726573730000000000000000000000000000000000000000000000000000000000",
"reason": "ERC20: transfer to the zero address",
"message": "revert"
}
}
}
geth:
{
"jsonrpc": "2.0",
"id": 1,
"error": {
"code": 3,
"message": "execution reverted: ERC20: transfer to the zero address",
"data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002345524332303a207472616e7366657220746f20746865207a65726f20616464726573730000000000000000000000000000000000000000000000000000000000"
}
}
Frontier:
{
"jsonrpc": "2.0",
"error": {
"code": -32603,
"message": "VM Exception while processing transaction: revert ERC20: transfer to the zero address",
"data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002345524332303a207472616e7366657220746f20746865207a65726f20616464726573730000000000000000000000000000000000000000000000000000000000"
},
"id": 1
}
Maybe truffle parses the reason from error.data.reason
instead of obtaining it from the message like hardhat does. I guess is a possibility worth exploring.
Just revisiting this one...testing the OP's repo agains Frontier it shows the following warning:
@openzeppelin/test-helpers WARN expectRevert: Assertions may yield false negatives!
Revert reason checks are only known to work on Ganache >=2.2.0 and Hardhat, and the current node is frontier-template/v1.1/fc-rpc-2.0.0-dev.
If your node does support revert reasons, please let us know: https://github.com/OpenZeppelin/openzeppelin-test-helpers/issues/new
Against geth
:
...
Revert reason checks are only known to work on Ganache >=2.2.0 and Hardhat, and the current node is Geth/v1.10.25-stable-69568c55/darwin-amd64/go1.18.5.
...
And parity-ethereum
:
....
Revert reason checks are only known to work on Ganache >=2.2.0 and Hardhat, and the current node is Parity-Ethereum//v2.7.2-stable-d961010f63-20200205/x86_64-apple-darwin/rustc1.41.0.
...
So it looks like expectRevert
should only be used with those test nodes.
I know that this issue also found other issue around the Frontier revert reason, so it'd would be better to close this one and create a new one to avoid mixing things, is that ok for you @valentinfernandez1 ?
EDIT: Just pushed a Hardhat "version" of your repo that is passing all tests: https://github.com/arturgontijo/hardhat-fast-test
Context
When writing Solidity contracts to be deployed on an EVM-compatible network, it is common to include functions with conditions that, when false, revert the execution of the function with a specific message. For example:
When testing these contracts using Truffle, it is common to write tests that verify if a function reverts properly and returns the expected message. For instance:
Problem
While attempting to test previously developed tests for different Solidity smart contracts using Frontier and Truffle, I discovered that the tests were failing because the
revert_reason
was undefined. The issue may be caused by the Frontier RPC, which does not correctly return this value.Recreating the Scenario
To show this issue I have created a Repository with a simple Truffle setup, an ERC20 token and a few tests to recreate the described problem. Follow the instructions in the
README.md
to execute the test scenario.Props to @bernardoaraujor for detecting this.