revokeAll function

[juslar]

Fixes #333

Implements revokeAll(address _app, bytes32 _role)

[coveralls]

Coverage increased (+0.8%) to 99.543% when pulling 9294f59 on espresso-org:revoke-all into 96d9940 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]

We might as well use SafeMath here :).

[bingen]

Sure, although it seems really unlikely that this function gets called 2^256 times for the same app and role, I guess you mean instead of the next line, right?

[juslar]

I added SafeMath and will remove the require on the next line :)

[sohkai]

Let's add braces around the if.

[sohkai]

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

See #459 (comment); most of the time that check should be covered by SafeMath.

[juslar]

Thank you @sohkai. I made the changes :)

[bingen]

Sure, although it seems really unlikely that this function gets called 2^256 times for the same app and role, I guess you mean instead of the next line, right?

[bingen]

I wonder if using a pointer to roleEras[roleHash(_app, _role)] could save some gas here.

[juslar]

Do you mean using a storage pointer? I don't think I can on a uint256 value. Maybe I'm missing something though.

[bingen]

Oh, yes, you are right, sorry for the confusion. We could still save 2 calls to roleHash function, but as it's going to be one less if you remove the require, I don't know if that's a big gain.

[sohkai]

Probably still a decent net gain, since there also wouldn't be another SLOAD as well.

[juslar]

Good :) Pushed the optimization.

[sohkai]

Just realized, we should definitely emit an event here as otherwise it'd be very difficult to know this happened in a frontend.

[juslar]

Sorry for the delay! I added a RevokeAllPermissions event :)

[sohkai]

LGTM! Going to merge this after @aragon/[email protected] is released and stage it for the next release :).

[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 :).