"function call to a non-contract account" heuristic doesn't work for view functions that return something

PaulRBerg commented 2 years ago

Edit by @fvictorio: reproduction steps here and here.

Description

I've recently upgraded Hardhat to the latest version and one of my tests started to fail. Did you, by chance, change the Hardhat Network to return a different revert reason when calling an external function on an EOA?

My test used to check for this error:

function call to a non-contract account

But now the contract reverts with this:

function returned an unexpected amount of data

It seems to me that if the answer is "no", then this could be a recently introduced bug (potentially caused by some changes in the Solidity compiler itself). The former error message is much more specific, and it's clearer to the developer what went wrong. The latter is vague and doesn't reflect the fact that there has been an attempt to call a non-contract account.

Hardhat is now returning a RETURNDATA_SIZE_ERROR error instead of NONCONTRACT_ACCOUNT_CALLED_ERROR.

The latter is what I'd expected Hardhat to return because the test is set up to perform a call to a non-contract account.

More Details

This is the line that triggers the revert in my contract:

https://github.com/hifi-finance/hifi/blob/f3cb5431edd53945d15557d26d3349858515156e/packages/flash-swap/contracts/uniswap-v2/FlashUniswapV2.sol#L190

Environment

hardhat v2.8.4
solidity v0.8.12

PaulRBerg commented 2 years ago

It looks like Hardhat is now returning a RETURNDATA_SIZE_ERROR error instead of NONCONTRACT_ACCOUNT_CALLED_ERROR.

The latter is what I'd expected Hardhat to return because the test is set up to perform a call to a non-contract account.

fvictorio commented 2 years ago

@paulrberg did you also change the version of solidity you are using?

PaulRBerg commented 2 years ago

@fvictorio yes, from v0.8.9 to v0.8.12.

github-actions[bot] commented 2 years ago

This issue was marked as stale because it didn't have any activity in the last 30 days. If you think it's still relevant, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

fvictorio commented 2 years ago

Still relevant

github-actions[bot] commented 2 years ago

This issue was marked as stale because it didn't have any activity in the last 30 days. If you think it's still relevant, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

anajuliabit commented 2 years ago

@fvictorio Any update on this? Same issue here

fvictorio commented 2 years ago

@anajuliabit Not yet, but I'll try to bump this issue's priority. That doesn't mean we'll be able to get it fixed super soon, but it should move it a bit forward in our (huge) backlog.

I should mention that, while this is definitely a bug and something we want to fix, I don't think it's a good idea to assert this kind of error messages in the tests. The inferred errors are mainly about helping you understand why something failed. Plus, we don't consider them part of the stable API, meaning that we could change what they say at any time and break your tests (although we try not to).

fvictorio commented 1 year ago

Minimal reproduction steps for our future selves:

contract Bar {
  function g() public returns (uint) {
    return 2;
  }
}

contract Foo {
  function f(Bar bar) public {
    bar.g();
  }
}

Deploy Foo and call f with an EOA as the argument.

The problem starts happening with solc 0.8.10.

Also: this only happens if the called function returns something. If you remove the returns from Bar.g, the heuristic function correctly.

NomicFoundation / hardhat