Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
PaulRBerg opened this issue Mar 3, 2022 · 9 comments
Labels
area:error-inferrer blocked-reason:needs-edr status:blocked Blocked by other issues or external reasons type:bug Something isn't working

Comments

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Mar 3, 2022

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
Copy link
Contributor Author

PaulRBerg commented Mar 3, 2022

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.

PaulRBerg added a commit to hifi-finance/hifi that referenced this issue Mar 3, 2022
…adeable" v4.3.2 as dep

build(protocol): upgrade to "@openzeppelin/contracts-upgradeable" v4.5.2
test(flash-swap): use latest automatic error message thrown by hardhat

See NomicFoundation/hardhat#2451
@fvictorio
Copy link
Member

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

@PaulRBerg
Copy link
Contributor Author

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

@fvictorio fvictorio added the type:bug Something isn't working label Mar 4, 2022
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Apr 30, 2022
@fvictorio
Copy link
Member

Still relevant

@github-actions github-actions bot removed the Stale label May 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2022

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.

@github-actions github-actions bot added the Stale label Jun 1, 2022
@fvictorio fvictorio added not-stale and removed Stale labels Jun 2, 2022
@anajuliabit
Copy link
Contributor

anajuliabit commented Sep 20, 2022

@fvictorio Any update on this? Same issue here

@fvictorio
Copy link
Member

@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 fvictorio removed their assignment Dec 27, 2022
@fvictorio
Copy link
Member

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.

@fvictorio fvictorio added priority:medium status:ready This issue is ready to be worked on area:error-inferrer and removed priority:medium labels Dec 27, 2022
@fvictorio fvictorio changed the title ErrorInferrer: misleading "unexpected amount of data" message when calling a non-contract account "function call to a non-contract account" heuristic doesn't work for view functions that return something Jan 20, 2023
@fvictorio fvictorio added status:blocked Blocked by other issues or external reasons and removed status:ready This issue is ready to be worked on labels Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:error-inferrer blocked-reason:needs-edr status:blocked Blocked by other issues or external reasons type:bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

3 participants