Apply suggestions from code review

Co-authored-by: derekpierre <[email protected]> Co-authored-by: David Núñez <[email protected]> Co-authored-by: Manuel Montenegro <[email protected]>
nucypher · Feb 4, 2025 · 4aac5f7 · 4aac5f7
1 parent 3fce81e
commit 4aac5f7
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 26 deletions.
diff --git a/contracts/contracts/coordination/GlobalAllowList.sol b/contracts/contracts/coordination/GlobalAllowList.sol
@@ -11,7 +11,7 @@ import "./Coordinator.sol";
 
 /**
  * @title GlobalAllowList
- * @notice Manages a global allow list of addresses that are authorized to decrypt ciphertexts.
+ * @notice Manages a global allow list of addresses that are authorized to decrypt plaintexts.
  */
 contract GlobalAllowList is IEncryptionAuthorizer, Initializable {
     using MessageHashUtils for bytes32;

diff --git a/contracts/contracts/coordination/ManagedAllowList.sol b/contracts/contracts/coordination/ManagedAllowList.sol
@@ -7,7 +7,7 @@ import "./GlobalAllowList.sol";
 
 /**
  * @title ManagedAllowList
- * @notice Manages a list of addresses that are authorized to decrypt ciphertexts, with additional management features.
+ * @notice Manages a list of addresses that are authorized to encrypt plaintexts, with additional management features.
  * This contract extends the GlobalAllowList contract and introduces additional management features.
  */
 contract ManagedAllowList is GlobalAllowList, AccessControlUpgradeable {
@@ -21,43 +21,65 @@ contract ManagedAllowList is GlobalAllowList, AccessControlUpgradeable {
      * @dev The coordinator contract cannot be a zero address and must have a valid number of rituals
      * @param _coordinator The address of the coordinator contract
      */
-    constructor(Coordinator _coordinator) GlobalAllowList(_coordinator) {
-        _disableInitializers();
-    }
+    constructor(Coordinator _coordinator) GlobalAllowList(_coordinator) {}
 
-    function ritualRole(uint32 ritualId, bytes32 role) public view returns (bytes32) {
+    /**
+     * Prepares role ID for the specific ritual
+     * @param ritualId The ID of the ritual
+     * @param role Role ID
+     */
+    function ritualRole(uint32 ritualId, bytes32 role) public pure returns (bytes32) {
         return keccak256(abi.encodePacked(ritualId, role));
     }
 
+    /**
+     * Prepares cohort admin role ID for the specific ritual
+     * @param ritualId The ID of the ritual
+     */
+    function cohortAdminRole(uint32 ritualId) public pure returns (bytes32) {
+        return ritualRole(ritualId, COHORT_ADMIN_BASE);
+    }
+
+    /**
+     * Prepares auth admin role ID for the specific ritual
+     * @param ritualId The ID of the ritual
+     */
+    function authAdminRole(uint32 ritualId) public pure returns (bytes32) {
+        return ritualRole(ritualId, AUTH_ADMIN_BASE);
+    }
+
     /**
      * Acquire cohort admin role
      * @param ritualId The ID of the ritual
      */
     function initializeCohortAdminRole(uint32 ritualId) public {
-        bytes32 cohortAdminRole = ritualRole(ritualId, COHORT_ADMIN_BASE);
+        bytes32 cohortAdminRole = cohortAdminRole(ritualId);
         address authority = coordinator.getAuthority(ritualId);
         require(authority != address(0), "Ritual is not initiated");
         if (hasRole(cohortAdminRole, authority)) {
             return;
         }
-        _setRoleAdmin(ritualRole(ritualId, AUTH_ADMIN_BASE), cohortAdminRole);
+        _setRoleAdmin(authAdminRole(ritualId), cohortAdminRole);
         _grantRole(cohortAdminRole, authority);
     }
 
+    /**
+     * Grants Auth Admin role. Can be called only by Cohort Admin or Authority of the ritual
+     * @dev Automatically grants Cohort Admin role to the authority if was not granted before
+     * @param ritualId The ID of the ritual
+     * @param account Address for Auth Admin role
+     */
     function grantAuthAdminRole(uint32 ritualId, address account) external {
         initializeCohortAdminRole(ritualId);
-        grantRole(ritualRole(ritualId, AUTH_ADMIN_BASE), account);
+        grantRole(authAdminRole(ritualId), account);
     }
 
     /**
-     * @notice Checks if the sender is the authority of the ritual
+     * @notice Checks if the sender is an Authorization Admin of the ritual
      * @param ritualId The ID of the ritual
      */
     modifier canSetAuthorizations(uint32 ritualId) virtual override {
-        require(
-            hasRole(ritualRole(ritualId, AUTH_ADMIN_BASE), msg.sender),
-            "Only auth admin is permitted"
-        );
+        require(hasRole(authAdminRole(ritualId), msg.sender), "Only auth admin is permitted");
         _;
     }
 
@@ -90,7 +112,7 @@ contract ManagedAllowList is GlobalAllowList, AccessControlUpgradeable {
             bytes32 lookupKey = LookupKey.lookupKey(ritualId, addresses[i]);
             require(
                 authAdmins[msg.sender][lookupKey],
-                "Encryptor must be authorized by the sender first"
+                "Encryptor has not been previously authorized by the sender"
             );
         }
         setAuthorizations(ritualId, addresses, false);

diff --git a/tests/test_managed_allow_list.py b/tests/test_managed_allow_list.py
@@ -9,14 +9,12 @@
 @pytest.fixture(scope="module")
 def initiator(accounts):
     initiator_index = 1
-    assert len(accounts) >= initiator_index
     return accounts[initiator_index]
 
 
 @pytest.fixture(scope="module")
 def deployer(accounts):
     deployer_index = 2
-    assert len(accounts) >= deployer_index
     return accounts[deployer_index]
 
 
@@ -59,10 +57,8 @@ def test_authorize_using_global_allow_list(coordinator, deployer, initiator, man
     signature = signed_digest.signature
 
     ritual_id = 0
-    cohort_admin_role = managed_allow_list.ritualRole(
-        ritual_id, managed_allow_list.COHORT_ADMIN_BASE()
-    )
-    auth_admin_role = managed_allow_list.ritualRole(ritual_id, managed_allow_list.AUTH_ADMIN_BASE())
+    cohort_admin_role = managed_allow_list.cohortAdminRole(ritual_id)
+    auth_admin_role = managed_allow_list.authAdminRole(ritual_id)
 
     # Not authorized
     assert not managed_allow_list.isAuthorized(0, bytes(signature), bytes(digest))
@@ -84,14 +80,12 @@ def test_authorize_using_global_allow_list(coordinator, deployer, initiator, man
     managed_allow_list.authorize(ritual_id, [deployer.address], sender=deployer)
 
     managed_allow_list.grantRole(auth_admin_role, initiator, sender=initiator)
-    with ape.reverts("Encryptor must be authorized by the sender first"):
+    with ape.reverts("Encryptor has not been previously authorized by the sender"):
         managed_allow_list.deauthorize(ritual_id, [deployer.address], sender=initiator)
 
     ritual_id = 1
-    cohort_admin_role = managed_allow_list.ritualRole(
-        ritual_id, managed_allow_list.COHORT_ADMIN_BASE()
-    )
-    auth_admin_role = managed_allow_list.ritualRole(ritual_id, managed_allow_list.AUTH_ADMIN_BASE())
+    cohort_admin_role = managed_allow_list.cohortAdminRole(ritual_id)
+    auth_admin_role = managed_allow_list.authAdminRole(ritual_id)
     coordinator.initiateRitual(ritual_id, initiator, sender=initiator)
     with ape.reverts():
         managed_allow_list.grantAuthAdminRole(ritual_id, deployer, sender=deployer)