Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Ownable to Roles #1146

Closed
nventuro opened this issue Aug 2, 2018 · 10 comments

Comments

@nventuro
Copy link
Member

commented Aug 2, 2018

With the 2.0 release coming soon, we want to stabilize as much of our API as possible, and in the process reduce the attack surface of our contracts. It's been pointed out that RBACOwnable is very similar to Ownable and could easily replace it, providing future extensibility, at the cost of added complexity.

We need to decide whether to keep both contracts or remove Ownable altogether from the codebase (which would impact some contracts like Heritable). I personally think this duplication is a mistake, which has caused the owner concept to extend to places where it doesn't belong (e.g. Mintable), and that we'd benefit greatly from the role semantics (minter, burner, etc.). This would also mean dropping the RBAC prefix from multiple contracts, since that's our standard role solution (e.g. RBACOwnable and RBACMintable would simply be Ownable and Mintable).

This is a place to discuss whether or not we want to do this: if consensus is reached and we decide to move forward, we'll also need to settle on a design for the affected contracts (like Inheritable).

Resolves #366.

@elopio

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

I am in favor of replacing Ownable with RBACOwnable. For people who feel strongly about using the simple Ownable, they can always install a pre-2.0 version of openzeppelin-solidity. And I think this is a momento to start thinking about extensibility and more interesting use cases for ethereum contracts. This is what an API designed on top of RBAC give us.

@shrugs

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

I'm in favor of switching to RBAC as described.

Only concern is that Ownable as a "single account that has admin permissions with minimal code" is a useful concept. Perhaps we could change it to WithAdmin and include it in a folder like //contracts/access/minimal/WithAdmin.sol?

@frangio

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

I'm in favor of replacing Ownable with RBAC and more granular roles. I'm not quite convinced that we should make everything RBACOwnable instead, because it doesn't make permissions more granular. (By the way, RBACOwnable is not merged yet. It's in #1020.)

To explain a bit further, the idea here is to change all the contracts that currently provide onlyOwner functionality, to instead define a specific role and use something like an only(role) modifier. So, for example, MintableToken would have a minter role, and PausableToken would have a pauser (?) role. These roles can then be assigned via RBAC to different sets of accounts.

There are a couple of outstanding problems that we need to resolve.

  1. #1090
  2. Provide a way to nicely declare and/or manage who is assigned to what roles.

Regarding the second point. Currently RBAC only provides internal functions and the user must design and include via inheritance their own role management scheme. This is not a very good experience. We should provide something out of the box. The complexity of having different roles shows here in that it's not trivial to design a universal API for it.

A first alternative is to have a role admin role who can dynamically add and remove roles to accounts. This feels like going back to Ownable but the admin can give up its role once everything is set up.

Another one is to have something like a "role descriptor" RBAC constructor argument that encodes the addresses assigned to each role, and potentially an admin that can change that during the contract lifetime.

These are just some ideas.

@nventuro

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2018

The current proposed changeset is as follows:

  • Solve #1090
  • Provide some sort of role-initialization in RBAC constructor (array of roles with addresses for each?)
  • Provide some sort of RBACWithAdmin mixin, a role that is able to add other roles, and give it up
  • Migrate all role-related uses of Ownable to RBAC (e.g. MintableToken, Pausable, etc)
  • Introduce a new concept (we don't have a name for it yet, we internally call it 'true ownership') with a behavior similar to Ownable's (minus transferOwnership), and use it to model the relationships between 'main' contracts and their 'auxiliary' contracts (e.g. PullPayment and its Escrow)
  • Keep Ownable in OpenZeppelin (though no contracts will use it), to both ease the transition and provide a contract that is useful for quick and dirty prototyping, probably with a comment describing the new alternate systems (roles and true ownership)
  • Remove Heritable and other Ownable-related contracts, possibly providing role-based alternatives (when applicable)
@shrugs

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@frangio

Regarding the second point. Currently RBAC only provides internal functions and the user must design and include via inheritance their own role management scheme. This is not a very good experience. We should provide something out of the box.

I'm not sure there's a default behavior that is both universally useful and not a security concern. Something like RBACWithAdmin is useful out of the box, but defining the management API of your access control layer is very much an application-specific problem. It feels like the best middle ground i stuff like RBACOwnable, RBACMintable, RBACHeritable, etc etc where we pre-code the access control layer CRUD operations for specific applications of RBAC.

@nventuro

Provide some sort of role-initialization in RBAC constructor (array of roles with addresses for each?)

probably the best option before abiencoderv2 ships

Introduce a new concept (we don't have a name for it yet, we internally call it 'true ownership') with a behavior similar to Ownable's (minus transferOwnership), and use it to model the relationships between 'main' contracts and their 'auxiliary' contracts (e.g. PullPayment and its Escrow)

what about "pinned"? i.e. "escrow is pinned to pull payment."
or maybe "pegged"?
or maybe "tracked"?

@frangio

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

I'm not sure there's a default behavior that is both universally useful and not a security concern.

I definitely agree. The thing that's missing that would be universally useful and secure is a way to set up an initial assignment of roles on deployment. It's one of the points that @nventuro wrote down though.

@frangio

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

Regarding names for "true ownership"... what about Private?

@shrugs

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

"private owner" could work..

We could take a term from the military and use "commander": escrow's commander is pull payment.

Or from computing and use "master/slave": escrow is slave to pull payment.

@nventuro

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Not a fan of 'commander', the other contract isn't really taking commands IMO. Also, the industry has moved a bit away from the 'master/slave/ terminology, since it doesn't evoke the happiest feelings.

@shrugs

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

Yup yup. The perspective on that master/slave terminology is a little odd, but those arguments have been well documented.

How about we keep Owner for the master/slave scenario and change Owner to Manager for the other case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.