ACL: Multiple enhancements by facuspagnuolo · Pull Request #584 · aragon/aragonOS

facuspagnuolo · 2020-06-10T21:28:14Z

Fixes #582

This PR includes:

Optimization for internal calls: external + internal over public functions
Allow knowing the original address being evaluated for permissions set to ANY_ENTITY

coveralls · 2020-06-11T12:41:06Z

Coverage decreased (-0.3%) to 98.862% when pulling 17c8ee9 on acl_enhancements into d068879 on next.

bingen · 2020-06-12T14:22:43Z

contracts/acl/ACL.sol

    */
-    function getPermissionManager(address _app, bytes32 _role) public view returns (address) {
-        return permissionManager[roleHash(_app, _role)];
+    function getPermissionManager(address _app, bytes32 _role) external view returns (address) {


What’s the reason for this change?

I'm proposing to avoid having public functions and provide both external and internal functions instead so we reduce gas costs when using these

How do you reduce gas costs? Unless the compiler with optimizations enabled decides to inline the internal method, which not only is kind of uncertain but also would increase the bytecode size, I don’t understand the gas benefit of doing it this way.

I think in this case we don't get any gas savings; IIRC you only get them with dynamically sized calldata parameters on external interfaces.

But I don't mind forcing anything we don't use internally to be external anyway—the idea of making something easy to inherit has come and gone, I think.

I agree if we don’t use it internally, but this function we do use it, so I don’t see the point. I don’t think there is gas saving here.

bingen · 2020-06-12T14:39:57Z

contracts/acl/IACL.sol

-    // TODO: this should be external
-    // See https://github.com/ethereum/solidity/issues/4832
-    function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool);
+    function hasPermission(address who, address where, bytes32 what, bytes how) external view returns (bool);


I’m not aware of any app inheriting from here (other than ACL here), so I guess it’s fine, but this may break them.

I think this will be released as a major version so I think we can introduce breaking changes

contracts/acl/ACL.sol

bingen · 2020-06-12T15:41:24Z

contracts/test/helpers/ACLHelper.sol


 contract AcceptOracle is IACLOracle {
-    function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
+    function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {


I wonder, does it make sense to assign an ACL Oracle with anything else than ANY_ENTITY? Because otherwise we could keep only one param and send the real sender instead of ANY_ENTITY in _hasPermission.

does it make sense to assign an ACL Oracle with anything else than ANY_ENTITY?

Probably it doesn't, but I wouldn't assume that.

Contrived, but you could assign a specific entity (e.g. an Agent) from not performing something via the ACL Oracle.

This could, for example, be used to disallow the AN DAO from changing court values too steeply.

I don’t get it: so would you assign the permission to the Agent instead of ANY_ENTITY? Then only the agent would be able to call the function protected by the ACL Oracle, right?

Yes, exactly.

But if you assign the role to a particular entity (Agent for instance), what do you want the new address param for?

Right, attending to the initial question:

Because otherwise we could keep only one param and send the real sender

It is contrived, but I consider the old behaviour as "not a bug" specifically because your ACL Oracle could behave differently if it was defined to be completely open for ANY_ADDRESS vs. specific to an address.

Hm, that makes ACL Oracles even more complex and harder to understand.

Agreed—in this case, it was a couple of different concerns:

This information about the ANY_ENTITY grantee was available in the old version

We needed a different function signature so any combination of ACLv1, ACLv2, ACLOracleV1, and ACLOracleV2 could work together (assuming the app implemented both)

sohkai

Have a couple of small notes, but looks good to me otherwise 🏎!

sohkai · 2020-07-01T11:59:39Z

contracts/acl/ACL.sol

    */
-    function getPermissionManager(address _app, bytes32 _role) public view returns (address) {
-        return permissionManager[roleHash(_app, _role)];
+    function getPermissionManager(address _app, bytes32 _role) external view returns (address) {


I think in this case we don't get any gas savings; IIRC you only get them with dynamically sized calldata parameters on external interfaces.

But I don't mind forcing anything we don't use internally to be external anyway—the idea of making something easy to inherit has come and gone, I think.

contracts/acl/ACL.sol

contracts/test/tests/TestACLInterpreter.sol

test/contracts/acl/acl_params.js

contracts/acl/IACLOracle.sol

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>

facuspagnuolo · 2020-07-01T21:40:07Z

@sohkai addressed your comments! Let me know your thoughts!

sohkai

LGTM, I just added a commit to separate the ACLOracle tests into separate ones with a bit more comments :).

This reverts commit a70d806.

* Revert "ACL: update user/who parameters to who/grantee (#589)" This reverts commit e6ea55f. * Revert "ACL: Multiple enhancements (#584)" This reverts commit a70d806. * Disputable: Rollback ACL v2 support for Agreement mock

facuspagnuolo requested review from bingen and sohkai June 10, 2020 21:28

facuspagnuolo self-assigned this Jun 10, 2020

facuspagnuolo force-pushed the acl_enhancements branch from 4aede0d to 438c85b Compare June 11, 2020 12:25

acl: optimize internal calls

64b93f3

facuspagnuolo force-pushed the acl_enhancements branch from 438c85b to 4141b29 Compare June 11, 2020 14:39

acl: allow knowing original user when set to any

b140996

facuspagnuolo force-pushed the acl_enhancements branch from 4141b29 to b140996 Compare June 12, 2020 14:23

bingen approved these changes Jun 12, 2020

View reviewed changes

acl: move grant permission modifier to internal method

3998f45

sohkai approved these changes Jul 1, 2020

View reviewed changes

facuspagnuolo and others added 7 commits July 1, 2020 12:29

ACL: Apply suggestions from @sohkai

9388fa9

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>

acl: move grant permission modifier to external methods

99a9657

acl: clarify ACL oracle interface

186ae2e

acl: improve internal logic eval functions naming

1641a72

acl: inline internal oracle helper function

9263b6a

acl: add more test cases for oracles

caf5a12

acl: add previous acl oracle interface

712324d

ACL: update complex ACLOracle tests to be separate tests

5462e80

sohkai approved these changes Jul 1, 2020

View reviewed changes

Merge branch 'next' into acl_enhancements

17c8ee9

facuspagnuolo merged commit a70d806 into next Jul 1, 2020

facuspagnuolo deleted the acl_enhancements branch July 1, 2020 23:15

facuspagnuolo added a commit that referenced this pull request Aug 3, 2020

Revert "ACL: Multiple enhancements (#584)"

163fcb7

This reverts commit a70d806.

Conversation

facuspagnuolo commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohkai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facuspagnuolo commented Jul 1, 2020

Uh oh!

sohkai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

facuspagnuolo commented Jun 10, 2020 •

edited

Loading

coveralls commented Jun 11, 2020 •

edited

Loading