Intercoin / CommunityContract

Smart contract for managing community membership and roles
https://intercoin.org
GNU Affero General Public License v3.0
1 stars 2 forks source link

Add a method to manage multiple roles #40

Closed EGreg closed 1 year ago

EGreg commented 1 year ago

Please add a method with the following signature, that just calls manageRole in a loop:

function manageRoles(
        uint8[] byRole,
        uint8[] ofRole,
        bool[] canGrantRole,
        bool[] canRevokeRole,
        uint8[] requireRole,
        uint256[] maxAddresses,
        uint64[] duration
    )

Rename getRoles() to be allRoles and return canGrantRoles and canRevokeRoles arrays in it:

returns (uint8[] memory, string[] memory, string[] memory, uint8[], uint8[])

Let's make _rolesByIndex public and rename it to roleByIndex.

By the way, why do we need allRoles if we can just make _rolesByIndex public? Is it because some libraries can read arrays of arrays easier than array of struct which contains EnumerableSetUpgradeable.UintSet ? How is a UintSet even stored, where is it defined? Please update this issue with that info.

struct Role {
        bytes32 name;
        string roleURI;
        //mapping(address => string) extraURI;
        //EnumerableSetUpgradeable.UintSet canManageRoles;
        EnumerableSetUpgradeable.UintSet canGrantRoles;
        EnumerableSetUpgradeable.UintSet canRevokeRoles;
        mapping(uint8 => GrantSettings) grantSettings;
        EnumerableSetUpgradeable.AddressSet members;
    }

Also, what is this method getRolesWhichAccountCanGrant and why isn't there getRolesWhichAccountCanRevoke? It takes takes string roles, but do any of our contracts actually use it? If not, please remove this function!

Make common variables instead of retyping code, such as line 1022 and 1018

Remove == true on on lines 1505, 1515, 1020 -- it is unnecessary

Actually, lines 1500-1509 seem to be totally useless

            iLen = 0;
            for (uint256 i = 0; i < _rolesByAddress[sender].length(); i++) {
                if (
                    _rolesByIndex[uint8(_rolesByAddress[sender].get(i))]
                        .canGrantRoles
                        .contains(targetRoleIndex) == true
                ) {
                    iLen++;
                }
            }

Improve documentation on all functions, including grantRolesExternal and revokeRolesExternal

artman325 commented 1 year ago
Actually, lines 1500-1509 seem to be totally useless

This happens because we need to return an array. However, dynamic arrays are only available in storage, not in memory. So, we need to calculate "how many items we will return," then declare an array with the appropriate length, and finally, fill it. That's it.

artman325 commented 1 year ago
returns (uint8[] memory, string[] memory, string[] memory, uint8[], uint8[])

Should be like this returns (uint8[] memory, string[] memory, string[] memory, uint8[][] memory, uint8[][] memory) Because canGrantRoles - it's already an array and belong to the single role

EGreg commented 1 year ago
Actually, lines 1500-1509 seem to be totally useless

This happens because we need to return an array. However, dynamic arrays are only available in storage, not in memory. So, we need to calculate "how many items we will return," then declare an array with the appropriate length, and finally, fill it. That's it.

alright, sure

EGreg commented 1 year ago
returns (uint8[] memory, string[] memory, string[] memory, uint8[], uint8[])

Should be like this returns (uint8[] memory, string[] memory, string[] memory, uint8[][] memory, uint8[][] memory) Because canGrantRoles - it's already an array and belong to the single role

makes sense, ok update ABI also