Skip to content

modifiers should generate branches checks on all place they are used #618

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

Closed
wighawag opened this issue Feb 25, 2021 · 2 comments
Closed

Comments

@wighawag
Copy link

Currently modifiers are considered individually and not as part of where they are called
this cause coverage to wrongly assume the modifier code has been called on all branches simply because if it was executed in another context

solidity-coverage should copy-paste the modifier code and generate branching check on that resulting code.

For now, the only workaround is to not use modifier and instead copy the modifier code in place.

for example:

function transferOwnership(address newOwner) external onlyOwner {
  emit OwnershipTransferred(owner, newOwner);
  owner = newOwner;
}

function setFee(uint256 fee) external onlyOwner {
  _fee = fee;
}

modifier onlyOwner() {
  require(msg.sender == owner, "NOT_AUTHORIZED");
  _;
}

will generate 100% coverage of branches even if only the following test are executed

  • transferOwnership from owner
  • setFee from owner
  • setFee from non-owner

the missing test for "transferOwnership from non-owner" will not be seen by the coverage.

On the contrary the following code will correctly trigger solidity-coverage to see less than 100% coverage as the require(... branching will be detected

function transferOwnership(address newOwner) external {
  require(msg.sender == owner, "NOT_AUTHORIZED");
  emit OwnershipTransferred(owner, newOwner);
  owner = newOwner;
}
function setFee(uint256 fee) external  {
  require(msg.sender == owner, "NOT_AUTHORIZED");
  _fee = fee;
}
@cgewecke
Copy link
Member

@wighawag Yes!

There is an 0.8.x beta which addresses this issue. See this release note.

It was published at the beginning of January so there are some more recent bug fixes missing - I think it's equivalent to 0.7.13.

Do you have any interest in trying it out? Would love some real-world feedback about whether or not things work.

@wighawag
Copy link
Author

Cool, I tried to find existing issue, but could not find (I think I was search for "modifiers" with a "s" :)
This is a duplicate of : #286
I ll close and I ll happily try the beta when I find some time. I ll report back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants