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.

Sign up for GitHub

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

Jump to bottom

Add a function to convert an ACL Param to uint256 #427

Open
leftab opened this issue Sep 20, 2018 · 8 comments · May be fixed by #456
Open

Add a function to convert an ACL Param to uint256 #427

leftab opened this issue Sep 20, 2018 · 8 comments · May be fixed by #456
Labels
enhancement
Milestone
aragonOS 5.0

Comments

@leftab
Copy link

leftab commented Sep 20, 2018

Currently, ACL parameters are created with the Param struct:

struct Param {
    uint8 id;
    uint8 op;
    uint240 value;
}

but the ACL grantPermissionP function takes a uint256[] for its parameters:

function grantPermissionP(address _entity, address _app, bytes32 _role, uint256[] _params)

It could be interesting to have a helper function to convert a Param struct to uint256.
Something like this:

function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) {
    return uint256(id) << 248 | uint256(op) << 240 | value;
}
@leftab
Copy link
Author

leftab commented Sep 20, 2018

I will submit a pull request if you think it's a good idea

@bingen
Copy link
Contributor

bingen commented Sep 22, 2018

Hey @leftab , yes I think it's a good idea. Besides I think it would good also to:

  • expose struct Param somewhere where can be used from outside (maybe ACLHelpers and then inherit it when needed?)
  • expose also Op enum and the *_ID constants
  • add a version of that new function that takes Param as argument, instead of those integers
  • add a version of that function that takes an array of Param instead of only one. Maybe we could try to use some assembly conversion to make it more efficient

Anyway I would wait to hear @izqui or @sohkai opinion about it.

@sohkai
Copy link
Contributor

sohkai commented Sep 25, 2018

On top of @bingen's list, I think there are a number of ways we could restructure the ACL so that the "scripting language" we're defining and evaluating with the params is logically decoupled from the ACL itself.

Users should be able to include a library (there's a few technical problems with this) or inherit from a contract to get access to the definitions needed to construct and massage params, and the evaluator could be its own contract so any subclass could potentially have the ability to evaluate these params.

@Quazia
Copy link
Contributor

Quazia commented Oct 22, 2018

I think restructuring to address the decoupling and adding the more customizable approach as detailed by @sohkai can be addressed at a later date but the additional helper function and changes detailed by @bingen seem safe enough to try. @leftab are you still interested in making these changes?

@leftab
Copy link
Author

leftab commented Oct 30, 2018

Sorry for the delay we were a little bit busy completing our first milestone hehe. I agree with @Quazia that maybe it would be a good idea to split this issue in two. I can certainly work on a pull request for @bingen's changes. It should be ready next week :)

@leftab
Copy link
Author

leftab commented Nov 6, 2018 •
edited
Loading

@sohkai @bingen I was about to add some tests for the new functions but I just noticed that the ACLSyntaxSugar.sol is in the skip list of .solcover.js

Do you know the reason why?

@sohkai
Copy link
Contributor

sohkai commented Nov 7, 2018

@leftab Mostly because it's a sugar library whose tests would be fairly academic, but there would be value in adding tests for it, especially the ACLHelpers :).

@leftab
Copy link
Author

leftab commented Nov 7, 2018

Got it! Thanks for the explaination @sohkai ! ;)

@leftab leftab linked a pull request Nov 9, 2018 that will close this issue
Open
1 task
@sohkai sohkai added this to the A1 Sprint: 4.1 milestone Feb 4, 2019
@sohkai sohkai modified the milestones: A1 Sprint: 4.1, aragonOS 5.0 May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
aragonOS 5.0
Development

Successfully merging a pull request may close this issue.

encodeParam and encodeParams functions leftab/aragonOS
5 participants
@bingen @luisivan @sohkai @Quazia @leftab