Feature/Access Control - Roles by account #2139

[julianmrodri]

2139

Fixes #2139

This PR introduces the required changes to be able to enumerate all Roles for a specific Account.

It is developed following the same approach currently implemented for getting all role members. It implements two functions that need to be used together. One to get the count of roles for a specific account and then another one to get each individual Role. (Requires to iterate from outside the contract).

function getAccountRoleCount(address account) public view returns (uint256)
function getAccountRole(address account, uint256 index) public view returns (bytes32)

In addition to this, I had to create a new implementation for EnumerableSet named Bytes32Set.

• All tests implemented. All lint checks passed. Also including analysis for Gas costs.

[julianmrodri]

Deployment Cost Analysis

I did a comparison as requested for the Deployment costs of this new version of AccessControl.sol. For that I created this simple contract that implements it. I ran it with the previous implementation and new implementation. I used EthGasReporter for this analysis. The new version increments the deployment cost by 22.7% (139651 gas).

pragma solidity ^0.6.0;

import "./AccessControl.sol";

contract AccountRoles is AccessControl {
    constructor() public  {}
}

Previous implementation: 615116
New implementation: 754767 (increments by 139651 - 22.7% )

I also did a check with the same code on Remix and got a similar result.

Remix Previous implementation: Transaction cost 740684 (Execution cost: 522556 )
Remix New implementation: Transaction cost 913055 (Execution cost: 652083). - Increment of 23.27%

Screenshots provided below

• Previous - EthGasReporter*
  

• New - EthGasReporter*
  

• Remix Previous Implementation

• Remix New implementation
  

[julianmrodri]

ByteCode Analysis

By checking the build/contracts/*.json files using the length of bytecode string divided by 2 (two hex characters make a byte).

Previous version: build/contracts/AccountRolesPrev.json: 2644.5. bytes

New version: build/contracts/AccountRoles.json: 3291.5. bytes. (increment by 24%)

[julianmrodri]

Gas Execution Analysis

I used Remix to check the transaction costs for Grant and Revoke roles functions which were modified in this change.
The is a very significant increase (as expected) on gas costs for granting a Role, and also an increase when revoking a role.

Previous version - Gas Costs
-Grant Role Transaction cost: 77265 gas. (Execution cost 52409)
-Revoke Role Transaction cost: 25803 gas (Execution cost 26750)

New version - Gas Costs
-Grant Role Transaction cost: 140329 gas. (Execution cost 115473) - Increase by 81%
-Revoke Role Transaction cost: 36973 gas (Execution cost 49090) - Increase by 38%

Previous version - Remix
-Grant

-Revoke

New Version
-Grant

• Revoke
  

[julianmrodri]

@nventuro @frangio Here I completed the changes for #2139 and also the analysis of the additional Gas costs for deployment and execution if we implement this.

Please feel free to review and let me know if there is something else we should look upon or modify. Thanks!

[nventuro]

This looks great @julianmrodri, short and to the point!

Your grant gas costs seem a bit high, and revoke a bit low. Did you perform the measurements on a brand new contract? If so, then they likely include both the fee when storage is first set (15k gas) and the refund when a slot is cleared, which disrupt the comparison a bit.

Could you repeat the measurements in a scenario where there an account already has a few roles, which themselves have been granted to some other accounts, and then add/revoke a new one? This data would be helpful in making a comparison.

Thanks a lot!

[julianmrodri]

Thanks @nventuro for the suggestion. Good catch. It adds the 15K the first time I setup a Role for a new account.

Everytime I grant the first role for an account I get the value reported above (140329). But the second time I grant another role to that same account I get 15K less (Transaction Cost: 125329 gas Execution cost: 100473 gas). Something similar happens with revoke I was probably getting a refund in certain situations.

These are the values for executing in the new scenario:

Grant Role

• New version - Transaction Cost: 125329 gas Execution cost: 100473 gas.
• Previous version - Transaction Cost: 77265 gas Execution cost: 52409 gas.

Revoke Role

• New version - Transaction Cost 45373 gas Execution cost: 65890 gas.
• Prev version - Transaction Cost: 30028 gas Execution cost: 35172 gas.

[nventuro]

Thanks! Those numbers make more sense, the 50k increase in grant comes from:

• 20k: store the new role in the account's roles
• 5k: update the length of the role array
• 20k: store the new role's index in the array, required for enumeration

[julianmrodri]

Awesome. Thanks to you for the explanation! Let me know if you want to do some more analysis I can prepare a more formal script if required with some more cases if you need. Just wanted to provide some initial numbers for you to better understand if its worth proceeding. Thanks!

[stale]

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

[nventuro]

@frangio what's your take on this?

[stale]

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

[stale]

Hi folks!
This Pull Request is being closed as there was no response to the previous prompt. However, please leave a comment whenever you're ready to resume, so it can be reopened.
Thanks again!

[frangio]

@julianmrodri Sorry for the long delay in replying. I guess it's tough for me to decide on this because as far as I can tell we haven't received too many requests for this feature, and in this scenario I think I favor keeping the contract slimmer and the costs down. It's in our plans to do some general research about the state of gas costs across OpenZeppelin Contracts, and with that research we will be able to come back to this feature and evaluate with more information.

Thanks for all the work in this regardless!

[julianmrodri]

@frangio No problem at all! I agree with you, changes like this you have a compelling case. Thanks to you for the support.