Roles can now be transfered.

[nventuro]

[note that this is going into the rbac-migration branch, which will be merged into master once #1146 is considered solved by it]

Roles can now be transfered to any account. Contracts that use the Roles library still need to explicitly provide access to this method, though.

[frangio]

This enables transfering to address(0) which is disabled in Ownable. The thing is if we add this restriction we may need an extra method to renounce the role, which will add to the boilerplate...

Another thing I find odd is the ability to transfer the role to someone who already has it. How would you feel about rejecting that?

[nventuro]

I think that rejection could be useful, yes. We'd need to add a check for address(0) though, since it may acquire the role after someone renounces it. Having role.add(0) be a no-op is probably the simplest way of handling that.

[frangio]

I'm not sure I followed. I was proposing to make transfer(•, 0) revert.

[nventuro]

My bad for not being clear.

I don't have strong opinions regarding transfer(role, 0) vs renounce(role), but would be ok with adding an extra method, considering the boilerplate is already quite large, what's another stripe on a tiger?

My previous response was to your suggestion of rejecting the ability to transfer the role to someone who already has it, assuming we allowed transferring it to address(0).

[shrugs]

my controversial opinion is that this code shouldn't care about giving roles to 0x0 or users that already have the role; those are application-level concerns. Likewise for two-stage transfers, which could be supported but aren't be default. Perhaps we add a SafeRoles lib that checks for all of these cases?

[frangio]

Two stage transfer is indeed another thing I was going to bring up. 😅

I think it makes sense to check for 0x0 in some cases because 0x0 is the default value of address variables, so it may catch some errors.

It's interesting to say that some of these concerns are application-level, and I'd be interested in exploring going towards that, but since we're a general-purpose library it's not clear what the application will be. If we're aiming towards as much security as we can provide, I think it makes sense to have some of these redundant checks. But this may change, I don't know.

[shrugs]

I mean, I'll support either way :D but just like in the safemath assert/require sitaution and the ERC721 two-stage transferFrom debates, I think the base library should be as agnostic and minimal as possible, especially since we literally pay for complexity increases when building on Ethereum. People who care about safety or strict requirements should inherit and alter the implementation to include their more complex features.

That said, I realize that's sometimes unreasonable, like in the case of safemath, where nobody was doing their application checks anyway. Perhaps the same situation applies here.

That said, I'd definitely prefer aBaseRoles library that doesn't include those checks, if that's something we want to bake into the normal Roles lib.

[nventuro]

I agree with @frangio on this one,

but since we're a general-purpose library it's not clear what the application will be

The thing about Roles is that our users will likely not use them directly, but interact with them through the different MinterRole, PauserRole, etc interfaces we'll define. They won't be able, then, to simply replace that Roles with SafeRoles, without modifying OZ source code.

We could, of course, provide both and have them choose, but how should someone go about doing that? 'Option A is cheap and risky, B is more expensive and safer'? I think forcing users to answer this question increases friction needlessly: I'd rather we made those sorts of decisions for them, following the principles of the rest of the lib (i.e. security first).

[shrugs]

good point on modifying the code. I agree with the "disallow address(0) but also provide renounceRole" scenario we talked about in gitter

[shrugs]

LGTM

dismissing until discussion is over

[shrugs]

here's the linting error:

function renounce(Role storage _role)
  internal
{
}