Improve modifer coverage report

[nventuro]

As of 0.5, modifers report being hit on their definition, but not at the call site. This is an issue because modifiers are mostly used to check preconditions e.g. onlyOwner, so not knowing if both branches of the require statements inside the modifiers were taken prevents developers from knowing if their test suite is thorough. For example, OpenZeppelin's Pausable is Ownable, but its tests fail to account for this (all transactions come from the owner account), and yet coverage is reported to be 100%.

Something we'd need to solve in order to implement this is deciding which line we should assign the coverage report to. If we can't indicate a range (e.g. line 4 columns 20 to 26), I think just pointing to the function declaration would be fine for now (though some extra logic would be needed for contracts with multiple modifiers).

Regarding instrumentation, I'm not completely sure. The main difficulty lies in that modifiers can be overridden and inherited, so we can't simply inline their body at the call site. Transforming them into internal functions would work, but that wouldn't solve the problem of not knowing if both require branches were taken (since all we'd see is the function call, not what happened inside of it). Ideas are welcome!

[cgewecke]

Leaving some implementation notes for a possible approach...

Things to resolve:

• representation in instanbul / coveralls format
• instrumentation / modifier shadowing

Representation
We could just have a second table that shows this info and get CI to pass/fail modifier coverage changes using the codechecks service that MakerDao engineer Kris Kaczor has built. There's an example of what that might look like in practice at eth-gas-reporter, here.

Contract	Method	Modifier	Success	Revert
PausableCrowdsale	halt	onlyOwner	✓	✓
PausableCrowdsale	halt	isRunning	✓	x
SimpleToken	withdraw	onlyOwner	✓	x

The codechecks diff should highlight whether or not a modifier has been removed, e.g if I remove onlyOwner from Pausable in a PR, the report should indicate it, maybe in a summary.

Instrumentation
We would instrument in two passes - the first sweep would create a map of modifiers to the contract methods that consume them. The second pass would instrument the modifiers:

// pseudo-code instrumentation

modifier onlyOwner {
  // Remember to exclude single branch modifier hits if there's no matching fn coverage
  modifierAssertPre("contractA:methodA"); 
  modifierAssertPre("contractB:methodC"); 
  assertPre("ownable:onlyOwner"); 
  require(condition);
  assertPost("ownable:onlyOwner"); 
  _;
}

function methodA() onlyOwner {
  modifierAssertPost("contractA:methodA:onlyOwner");
  ...
}

function methodC() onlyOwner {
  modifierAssertPost("contractB:methodC:onlyOwner");
}  

Resources
consensys/surya has a c3-linearization inheritance tool that might be helpful for figuring out the overridden modifiers problem.

[cgewecke]

Question: is anyone using modifiers for 'non-branch' purposes - e.g as gates which do common preparatory work or to normalize params?

[yxliang01]

I wouldn't be surprised if anyone using modifiers for increasing certain counters?

[nventuro]

I thought a simple way we can implement this that plays very nicely with onlyOwner-type modifiers, that is, modifiers that perform some sort of validation before a function is executed.

We can look for all instances where a modifier is applied, and then create a new modifier for each of these functions, identical to the original one.

// original code
contract Foo {
  function foo() onlyOwner { .. }
  function bar() onlyOwner { .. }
}

// instrumented
contract Foo {
  modifier onlyOwner_foo { .. }
  modifier onlyOwner_bar { .. }

  function foo() onlyOwner_foo { .. }
  function bar() onlyOwner_bar { .. }
}

The same logic that is used to instrument modifiers today is then applied: the only new element to add is something to map coverage of each new modifier to their callsite. We could also combine the coverage reports for all modifiers to compute the original modifier's coverage, but that might not be critical.

A glaring issue of this solution is that it doesn't address modifiers being overidden. I have however never seen that feature be used, and think we might get away with only instrumenting non-virtual modifiers.

[nventuro]

It could be argued that the approach I just described can also be extended to regular internal functions, and that expecting 100% coverage of all function code paths at every callsite might not be achievable.

I haven't thought about this in great detail, but think starting with just modifiers is a good idea. They are often used only for input or state validation (as opposed to functions), and providing feedback on whether a test suite exercises these checks is very useful.

We'd need to figure out the issues with overrides if we intend to extend this to functions as well, however.

[cgewecke]

Cleaning up the issues ... this is all in v0.8.x