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

Support ACL permissions with parameters #807

Merged
merged 17 commits into from
Oct 9, 2019
Merged

Support ACL permissions with parameters #807

merged 17 commits into from
Oct 9, 2019

Conversation

macor161
Copy link
Contributor

@macor161 macor161 commented Oct 4, 2019

🦅 Pull Request

Add support for ACL parameters:

dao acl grant <dao-addr> <app-proxy-addr> <role> <entity> [params...]

Each parameter must have the following syntax: "<id>,<op>,<value>". Multiple parameters must be separated by spaces.

See the ACL documentation for the list of available operators.

Closes #407

🚨 Test instructions

dao acl grant <dao> <app> <role> <entity> "0,GT,2"
dao acl grant <dao> <app> <role> <entity> "2,EQ,0xC7f8dDbc7B3BFd432dEAc0CA270110467EcE01c3"
dao acl grant <dao> <app> <role> <entity> "LOGIC_OP_PARAM_ID,OR,(1,2)"  "0,LT,4"  "1,EQ,42"
dao acl grant <dao> <app> <role> <entity> "LOGIC_OP_PARAM_ID,IF_ELSE,(1,2,3)" 
 "BLOCK_NUMBER_PARAM_ID,GT,1000"  "0,EQ,42"  "0,EQ,0"

✔️ PR Todo

  • Include links to related issues/PRs
  • Update unit tests for this change
  • Update the relevant documentation
  • Clear dependencies on other modules that have to be released before merging this

@macor161 macor161 marked this pull request as ready for review October 4, 2019 16:21
Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @macor161 this new feature is amazing, will enable much more use cases and experimentation 👨‍🔬

I tested it with the help of @fabriziovigevani using the Token Oracle 1Hive is using for the Time Lock app and is working great 💪

@0xGabi 0xGabi merged commit ce97259 into aragon:master Oct 9, 2019
@macor161 macor161 deleted the acl-params branch October 10, 2019 17:02
@@ -0,0 +1,156 @@
const { isString } = require('lodash')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone get the fire trucks, cause this is hot 🔥:fire: :fire:

Haven't checked the correctness (we should certainly add more tests), but:

  • We should scope this out into its own package
  • We may be able to define a light DSL language for this and use something like nearley or peg.js to parse it; we would most likely want to use this in a browser environment so we'll have to be conscious of the parser's size (and obviously handrolling it like now would be the most optimal)
  • We should add a decoder to go from the param bytes, for use in UI

Copy link
Contributor Author

@macor161 macor161 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha thanks! 😄
Definitely good ideas. I added them to my to-do list. I'm not super familiar with DSLs however, I will have to read the subject a little bit more. But point 1 and 3 should be fairly straightforward to do.

@0xGabi 0xGabi mentioned this pull request Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support permissions with parameters
3 participants