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

Automatically generate Role contracts #1290

Closed
nventuro opened this issue Sep 6, 2018 · 11 comments
Closed

Automatically generate Role contracts #1290

nventuro opened this issue Sep 6, 2018 · 11 comments
Labels
automation Tests and coverage running. Docsite publishing. contracts Smart contract code. feature New contracts, functions, or helpers.

Comments

@nventuro
Copy link
Contributor

nventuro commented Sep 6, 2018

The 2.0 release will introduce MinterRole, SignerRole, etc.: these are all manually generated, as are their tests, mocks, etc. We should have some sort of automatic code generation step in our build process to both make it easier to add new roles, and reduce the potential for human error.

@nventuro nventuro added feature New contracts, functions, or helpers. automation Tests and coverage running. Docsite publishing. contracts Smart contract code. labels Sep 6, 2018
@celeduc
Copy link

celeduc commented Oct 5, 2018

Instead of a code generator to avoid human error I humbly request that we instead change the architecture to not require that everyone include the same duplicate code. At best, this will be expensive (with gas), error prone (with typos) and hard to upgrade and maintain.

@nventuro
Copy link
Contributor Author

nventuro commented Oct 5, 2018

Hey @celeduc! Is there any such architecture you have in mind? We've tried a couple things (e.g. the old RBAC), but they each had their own issues (#1090). Maintainability, extensibility and usability are definitely things we strive for, and this was the most minimal example we were happy with.

@celeduc
Copy link

celeduc commented Oct 5, 2018

Wow, I didn't know about #1090. Yuck.

Implementing a role with this scheme costs a substantial amount of coding (43 lines of Solidity contact and 11 lines of JavaScript test), not to mention the gas to deploy it. Implementing a new role with RBAC was a one-liner, and all of the access methods were orthogonal (making meta-coding and Web3 coding a lot easier).

With C++ I'd use a template class to implement RBAC with whatever datatype is passed in, so if you wanted integer-based, cool, if you want strings, cool. But of course Solidity doesn't have generics yet. But there is enum which at least gives a modicum of type safety.

In any case, experience tells me that whenever I see the words "code generator" or "code wizard" I think "architecture problem" and "maintenance nightmare". Windows. Rails scaffolds. Etc.

@nventuro
Copy link
Contributor Author

nventuro commented Oct 5, 2018

The problem with enum is that we have no way of accessing each of the values from the outside, e.g. if we did

enum Roles {
  Minter,
  Pauser, 
  Signer
}

we'd have to hardcode the values in JavaScript, since they are not included in the artifact (the Role.json file).

That said, I fully agree with your concerns regarding code generation, and would much prefer not using it, should we find a better alternative.

@celeduc
Copy link

celeduc commented Oct 5, 2018

I've finally read all the way through 1090 and understand why it is the way it is. Although it may be right within the scope of this project I'm afraid it won't be maintainable in my dependent project, plus it'll make the code super clumsy, so I'll just fork out RBAC and stay with that until something better comes along. Thanks! (and please don't write a code generator ;)

@nventuro
Copy link
Contributor Author

nventuro commented Oct 5, 2018

If you don't mind, could you expand a bit on why the current Roles solution is not fit for your project? Do you require a dynamic number of rules? We try to cater to as many use cases as possible, and would much prefer it if we could prevent you from doing that extra work yourself :)

@celeduc
Copy link

celeduc commented Oct 5, 2018

I have n different related permissions to manage, Record101, Record102, Record103 in which one account may have Record rights for 101 and 102 but not 103, etc. With RBAC I can set up n permissions. I thought I was going to append the number to the string, but that turns out to be expensive (string support is still terrible). So instead I might just use integers and bitmasking. Those are fast, and the const variables are accessible through Web3. Will append working code when working :/

@nventuro
Copy link
Contributor Author

nventuro commented Oct 5, 2018

Please do! I'm very interested in looking at it :)

@celeduc
Copy link

celeduc commented Oct 10, 2018

I've come to the conclusion that I should just be using Roles directly. It's more straightforward, and that way I can index by the appropriate type. Less code, too. As I only have one permission type I can just use the following:

mapping (uint => Roles.role) private recordRoles;
...
recordRoles[index].add(address);
recordRoles[index].remove(address);
recordRoles[index].has(address);

if and when I need more permission types, I'll just map the mapping to a list of permission types:
mapping (uint => mapping (uint => Roles.role)) private ourRoles;

Actual code here.

Thanks for the help!

@nventuro
Copy link
Contributor Author

Awesome! Indeed, if that index isn't a problem in your application, using Roles sounds like a good idea :) MinterRole and friends exist to make it easier for users to get started using roles without much fanfare, but they might not be suited to all use cases.

@nventuro
Copy link
Contributor Author

With the release of AccessControl in v3.0 this no longer applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Tests and coverage running. Docsite publishing. contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

No branches or pull requests

2 participants