RBAC and Ownable migration towards Roles

[nventuro]

Notable changes:

• Ownable is no longer used (except in drafts)
• RBAC has been removed
• Secondary was introduced to replace Ownable in simple cases (e.g. Escrow)
• The Roles library is now much more widely used, though the role contracts themselves have to be manually generated (see Automatically generate Role contracts #1290)

Closes #366
Closes #1090
Closes #1146

[frangio]

On hold for #1276.

[shrugs]

👍

[come-maiz]

Ok, in here I just have $opinions that we can discuss later, maybe with beers when we meet :)
My only complaints are about things that are back to public, I will prepare a PR for this so you review it tomorrow.

[come-maiz]

I'm looking at you Venturo! 🙄

[nventuro]

Not sure what you mean D:

[come-maiz]

I find it weird that addCapper is not consistent with removeCapper. A good design principle is that everything you can do you, you will be able to undo. I guess you had a good reason to go this way, so again, just thinking out loud. We can do any changes later.

[nventuro]

We've discussed this extensively with @frangio. Our reasoning goes as follows:

An account with a role should always be able to transfer it. If the account is a contract, then the conditions under which a transfer will be executed will be clear from the code. If it's an EOA, then the user already has the liberty to do whatever she wants with it, from complying to transaction requests to giving up the private keys to someone else, effectively transferring control.

So, transferring can always be done. This includes transferring the role to a contract that will forward all calls from a whitelisted set of addresses, which effectively is an add to multiple addresses. We decided to formalize this mechanism, and simply remove transfer and make add public by default. Note that remove plays no part here: at no point does having a role give you the power to take it away from someone else, but it does let you act at someone else's behest.

TL;DR: EOAs shouldn't be trusted, since people can do anything they want, if you really need control over this kind of stuff, the role-havers should also be contracts.

[frangio]

We might want a notion of a public remove function but we will wait and see what use-cases come up to add later. :)

[come-maiz]

is it ok that a capper can remove other cappers? Shouldn't there be instead an admin that adds and removes cappers?

[shrugs]

that's a good point; a lot of the api surface for these runs into the same issues with the problem with RBAC and it's that you can't define a public api surface by default because then changing it is super hard. and you don't know what sort of process a project might want to be able to use to be able to change the cap, for example.

[frangio]

Our rationale was that someone who has a role is already able to perform actions using that role on behalf of other people. In fact, with ownership, it was trivial to transfer ownership to a forwarder contract that would let any set of accounts use the ownership. But this could also be done informally, as an off-chain favor.

Since this was already possible, we decided to make it part of the API. Otherwise it just feels like obscuring what is really possible.

[come-maiz]

I like that we assign nice names to the functions. I don't like so much that the code for Minter is the same as the code for Capper. 🤔

[nventuro]

I probably should've gone into some more detail in the PR description: the role contracts (everything under access/roles/, and associated tests) will be auto-generated in the future (see #1290), since they are all identical (we call it the 'role boilerplate'). This is sort of a limitation of how libraries work in Solidty, but I'm not yet convinced what'd be the best way to tackle this moving forward. For now, code repetition is what we have.

[come-maiz]

hey, what happened here? I suppose this was a failed merge.

[shrugs]

I think this needs to be merged into the new master again? not sure

[frangio]

We must've merged this file wrong.

[come-maiz]

public? Nooo

[shrugs]

good point; if we update this, also update the ERC20Mintable that this was copy/pasted from

[frangio]

[come-maiz]

[nventuro]

Thanks for the thorough review @ElOpio @shrugs @frangio 🎉

[mjdietzx]

Just to be clear, the recommended way to implement Whitelist functionality after this change is to do something like:

• Create a Whitelist contract using Roles similar to https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/access/roles/CapperRole.sol
• Implement something like addBatch and removeBatch
• Check against the whitelist as normal (ie in MyCrowdSale)

Just so I can understand, why did you decide to remove the whitelist and whitelisted crowd-sale? Was it just not adding core functionality / unnecessary? Or is there some deeper reason?

[nventuro]

Hey @mjdietzx! There's a short explanation here: the gist of it is that, if implemented using the current role system, the whitelistees would also become whitelisters themselves. This may or may not be an issue: we need to figure out what our purpose is with those contracts, and them implement them. Since 2.0 now features a stable API, I didn't want to include a contract we weren't 100% sure about.

[mjdietzx]

Hey @nventuro, thanks for the response! Just some feedback, I think Whitelist.sol and whitelisted crowd-sales in general are a very common use-case (kyc & aml keep getting stricter 😕) And a whitelisted address being able to add another address to the whitelist wouldn't be acceptable for this use-case. So I would think it's important to be able to implement this well in openzeppelin.

Either way, we'll be updating to v2.0 (looking forward to it) and will find a good solution for our whitelist with whatever you decide to go with. Looking forward to the release, thanks!

[frangio]

@mjdietzx Please check out my comment in #1292 and let us know what you think over there!