Remove Claimable, DelayedClaimable, Heritable

[frangio]

With the move away from Ownable (#1146), these extensions don't make as much sense as they used to. Note that we will be keeping Ownable itself, but because it's a quick and dirty way to implement permissions, and it will be discouraged for more complex scenarioss. The extensions do not fit in that criteria.

An additional thesis we have is that these extensions can be implemented on the side of the account that holds ownerhip or a role. (Sort of related: #1104)

[come-maiz]

contracts/examples/SimpleSavingsWallet.sol is Heritable, it needs to be updated.

[frangio]

Thanks for the review @ElOpio. I really should at least compile before pushing. 😅 It should be fixed now.

fixed

[come-maiz]

+1

[jtakalai]

So Claimable (which was what Ownable really should be: ensuring during a transfer that target address isn't foobar) was removed, and instead v2.0.0 has four essentially copy-pasted(!) versions of the same contract, employing Roles. Who the heck is responsible for overall architecting of RBAC? https://openzeppelin.org/api/docs/ownership_rbac_RBAC.html says 2.0.0 in the header but the link refers to 1.11.0 code.

Those four copy-pasted things may make sense from the Bouncer dev's point of view, or whatever. For sure it's not sane library management. Deleting useful contracts and plastering that over with "these extensions can be implemented on the side of the account that holds ownerhip or a role", but not actually implementing them... I don't even. So suppose someone upgrades to a new version of OpenZeppelin and discovers Claimable doesn't exist anymore. What's the upgrade path? Here's what it should be: a contract with the same name, hopefully in same path (or different path, maybe access instead of ownership, no big deal).

This is the kind of library management that drives people to roll their own. I'm going to right now take an old version of Claimable (and maybe improve it to look decent, at least inline the inheritance from Ownable) and add it to my own project. I might contribute it back to OpenZeppelin, but it might not make sense as a generic thing without modifications.

[jtakalai]

On a positive note, I think RBAC is an improvement, and things like https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC20/ERC20Mintable.sol look better with the MinterRole. Of course, as code copy-paste often suggests, maybe generalizing the re-use into one more abstract idea might make sense...

[come-maiz]

@jtakalai: implementing your own Claimable contract is a good solution, if what we provide on OpenZeppelin doesn't fit your use case.

It's on purpose that we don't offer an upgrade path. 2.0 is a breaking release and to upgrade you will probably have to adjust your code. Of course, you are welcome to contribute back the things you add or modify, and if they make sense for this milestone of OpenZeppelin we will include them.

pura vida.

[nventuro]

@jtakalai

Of course, as code copy-paste often suggests, maybe generalizing the re-use into one more abstract idea might make sense...

Indeed, an abstraction on top of Roles is what we used to have. However, there are major issues with this approach, so we decided to go for the simpler solution (although more limiting) of having near duplicates, until we come up with a better solution (possible after some Solidity features are implemented).