revokeAll function

[juslar]

Fixes #333

Implements revokeAll(address _app, bytes32 _role)

[coveralls]

Coverage increased (+0.8%) to 99.543% when pulling 9294f594f2899b5c03adef94527ca4186ebd0f4b on espresso-org:revoke-all into 96d9940b0eee64412b7c1f30c97c03eb27ae6e97 on aragon:dev.

[juslar]

I thought about a potential breaking change with the implementation I submitted. If a DAO currently in production upgrades to this version of the ACL, all the permissions would be invalid. Not too sure how to address this. An easy fix would be to add something like:

function permissionHash(address _who, address _where, bytes32 _what) internal view returns (bytes32) {
	uint256 roleEra = roleEras[roleHash(_where, _what)];
	
	if (roleEra == 0)
		return keccak256(abi.encodePacked("PERMISSION", _who, _where, _what));
	
	return keccak256(abi.encodePacked("PERMISSION", roleEra, _who, _where, _what));
}

But that would consume a bit more gas. What do you think?

[sohkai]

@juslar Nice! I think the backwards-compatible version isn't a terrible cost to swallow, and we should make sure to comment it.

An interesting thing we might want to set up in the future is upgradability tests between compatible versions.

[juslar]

Thanks! I added the backwards-compatible line.

Regarding the coverage decreasing, it's due to this line:

require(newRoleEra >= roleEras[roleHash(_app, _role)], ERROR_ROLE_ERA_INCREMENT);

Do you have an idea how I could cover this? Is it ok to leave it this way?

[sohkai]

Do you have an idea how I could cover this? Is it ok to leave it this way?

See https://github.com/aragon/aragonOS/pull/459#discussion_r247033374; most of the time that check should be covered by SafeMath.

[juslar]

Thank you @sohkai. I made the changes :)

[sohkai]

@juslar Because this wasn't pulled into some of the minor aragonOS 4.x releases (and because we didn't want to risk re-deploying the ACL for 0.7), we'll roll this into aragonOS 5 :).