Clean up and small refactors to the access module (#1221)

* refactor access module * improve comment for initializable * remove tests that check caller is not the zero address * Update packages/access/src/accesscontrol/accesscontrol.cairo Co-authored-by: Eric Nordelo <[email protected]> * Update packages/access/src/ownable/ownable.cairo Co-authored-by: Eric Nordelo <[email protected]> * check owner is not zero on initializer and improve comments * Update packages/access/src/accesscontrol/accesscontrol.cairo Co-authored-by: Eric Nordelo <[email protected]> * Update packages/access/src/ownable/ownable.cairo Co-authored-by: Eric Nordelo <[email protected]> * remove caller zero address tests from presets * update docs * update changelog * remove tuple syntax from test * Update docs/modules/ROOT/pages/api/access.adoc Co-authored-by: Andrew Fleming <[email protected]> * update changelog --------- Co-authored-by: Eric Nordelo <[email protected]> Co-authored-by: Andrew Fleming <[email protected]>
OpenZeppelin · Nov 20, 2024 · f502aeb · f502aeb
1 parent 6962bbe
commit f502aeb
Show file tree

Hide file tree

Showing 15 changed files with 85 additions and 249 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed (Breaking)
 
+- The initializer in `OwnableComponent` now checks that `owner` is not the zero address (#1221)
 - Add `verifying_contract` member to the `Delegation` struct used in Votes `delegate_by_sig` (#1214)
 use crate::votes::VotesComponent::VotingUnitsTrait;
 - VotingUnitsTrait moved from `openzeppelin_governance::votes::votes` to `openzeppelin_governance::votes::VotesComponent` (#1214)

diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc
@@ -156,7 +156,8 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event.
 [[OwnableComponent-two-step-transfer_ownership]]
 ==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external#
 
-Starts the two step ownership transfer process, by setting the pending owner.
+Starts the two step ownership transfer process, by setting the pending owner. Setting `new_owner` to the zero address is allowed, this can be used to cancel an initiated ownership transfer.
+
 Can only be called by the current owner.
 
 Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event.
@@ -211,6 +212,10 @@ See xref:OwnableComponent-two-step-renounce_ownership[renounce_ownership].
 
 Initializes the contract and sets `owner` as the initial owner.
 
+Requirements:
+
+- `owner` cannot be the zero address.
+
 Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event.
 
 [.contract-item]

diff --git a/packages/access/src/accesscontrol/accesscontrol.cairo b/packages/access/src/accesscontrol/accesscontrol.cairo
@@ -3,8 +3,19 @@
 
 /// # AccessControl Component
 ///
-/// The AccessControl component allows contracts to implement role-based access control mechanisms.
-/// Roles are referred to by their `felt252` identifier.
+/// The AccessControl component enables role-based access control mechanisms. This is a lightweight
+/// implementation that doesn't support on-chain enumeration of role members, though role membership
+/// can be tracked off-chain through contract events.
+///
+/// Roles can be granted and revoked dynamically via `grant_role` and `revoke_role`. Each role
+/// has an associated admin role that controls who can grant and revoke it. By default, all roles
+/// use `DEFAULT_ADMIN_ROLE` as their admin role.
+/// Accounts can also renounce roles they have been granted by using `renounce_role`.
+///
+/// More complex role hierarchies can be created using `set_role_admin`.
+///
+/// WARNING: The `DEFAULT_ADMIN_ROLE` is its own admin, meaning it can grant and revoke itself.
+/// Extra precautions should be taken to secure accounts with this role.
 #[starknet::component]
 pub mod AccessControlComponent {
     use crate::accesscontrol::interface;
@@ -31,16 +42,16 @@ pub mod AccessControlComponent {
 
     /// Emitted when `account` is granted `role`.
     ///
-    /// `sender` is the account that originated the contract call, an admin role
-    /// bearer (except if `_grant_role` is called during initialization from the constructor).
+    /// `sender` is the account that originated the contract call, an account with the admin role
+    /// or the deployer address if `grant_role` is called from the constructor.
     #[derive(Drop, PartialEq, starknet::Event)]
     pub struct RoleGranted {
         pub role: felt252,
         pub account: ContractAddress,
         pub sender: ContractAddress
     }
 
-    /// Emitted when `account` is revoked `role`.
+    /// Emitted when `role` is revoked for `account`.
     ///
     /// `sender` is the account that originated the contract call:
     ///   - If using `revoke_role`, it is the admin role bearer.
@@ -55,7 +66,7 @@ pub mod AccessControlComponent {
     /// Emitted when `new_admin_role` is set as `role`'s admin role, replacing `previous_admin_role`
     ///
     /// `DEFAULT_ADMIN_ROLE` is the starting admin for all roles, despite
-    /// {RoleAdminChanged} not being emitted signaling this.
+    /// `RoleAdminChanged` not being emitted signaling this.
     #[derive(Drop, PartialEq, starknet::Event)]
     pub struct RoleAdminChanged {
         pub role: felt252,
@@ -89,7 +100,7 @@ pub mod AccessControlComponent {
 
         /// Grants `role` to `account`.
         ///
-        /// If `account` had not been already granted `role`, emits a `RoleGranted` event.
+        /// If `account` has not been already granted `role`, emits a `RoleGranted` event.
         ///
         /// Requirements:
         ///
@@ -104,7 +115,7 @@ pub mod AccessControlComponent {
 
         /// Revokes `role` from `account`.
         ///
-        /// If `account` had been granted `role`, emits a `RoleRevoked` event.
+        /// If `account` has been granted `role`, emits a `RoleRevoked` event.
         ///
         /// Requirements:
         ///
@@ -132,7 +143,7 @@ pub mod AccessControlComponent {
         fn renounce_role(
             ref self: ComponentState<TContractState>, role: felt252, account: ContractAddress
         ) {
-            let caller: ContractAddress = get_caller_address();
+            let caller = get_caller_address();
             assert(caller == account, Errors::INVALID_CALLER);
             self._revoke_role(role, account);
         }

diff --git a/packages/access/src/ownable/ownable.cairo b/packages/access/src/ownable/ownable.cairo
@@ -35,6 +35,8 @@ pub mod OwnableComponent {
         OwnershipTransferStarted: OwnershipTransferStarted
     }
 
+    /// Emitted when `new_owner` is set as owner of the contract.
+    /// `new_owner` can be set to zero only if the ownership is renounced.
     #[derive(Drop, PartialEq, starknet::Event)]
     pub struct OwnershipTransferred {
         #[key]
@@ -43,6 +45,9 @@ pub mod OwnableComponent {
         pub new_owner: ContractAddress,
     }
 
+    /// Emitted when `transfer_ownership` is called on a contract that implements `IOwnableTwoStep`.
+    /// `previous_owner` is the address of the current owner.
+    /// `new_owner` is the address of the pending owner.
     #[derive(Drop, PartialEq, starknet::Event)]
     pub struct OwnershipTransferStarted {
         #[key]
@@ -54,7 +59,6 @@ pub mod OwnableComponent {
     pub mod Errors {
         pub const NOT_OWNER: felt252 = 'Caller is not the owner';
         pub const NOT_PENDING_OWNER: felt252 = 'Caller is not the pending owner';
-        pub const ZERO_ADDRESS_CALLER: felt252 = 'Caller is the zero address';
         pub const ZERO_ADDRESS_OWNER: felt252 = 'New owner is the zero address';
     }
 
@@ -83,8 +87,8 @@ pub mod OwnableComponent {
             self._transfer_ownership(new_owner);
         }
 
-        /// Leaves the contract without owner. It will not be possible to call `assert_only_owner`
-        /// functions anymore. Can only be called by the current owner.
+        /// Leaves the contract without an owner. It will not be possible to call
+        /// `assert_only_owner` functions anymore. Can only be called by the current owner.
         ///
         /// Requirements:
         ///
@@ -129,6 +133,8 @@ pub mod OwnableComponent {
 
         /// Starts the two-step ownership transfer process by setting the pending owner.
         ///
+        /// Setting `new_owner` to the zero address is allowed; this can be used to cancel an
+        /// initiated ownership transfer.
         /// Requirements:
         ///
         /// - The caller is the contract owner.
@@ -150,7 +156,8 @@ pub mod OwnableComponent {
         ///
         /// Emits an `OwnershipTransferred` event.
         fn renounce_ownership(ref self: ComponentState<TContractState>) {
-            Ownable::renounce_ownership(ref self);
+            self.assert_only_owner();
+            self._transfer_ownership(Zero::zero());
         }
     }
 
@@ -270,8 +277,13 @@ pub mod OwnableComponent {
     > of InternalTrait<TContractState> {
         /// Sets the contract's initial owner.
         ///
+        /// Requirements:
+        ///
+        /// - `owner` is not the zero address.
+        ///
         /// This function should be called at construction time.
         fn initializer(ref self: ComponentState<TContractState>, owner: ContractAddress) {
+            assert(!owner.is_zero(), Errors::ZERO_ADDRESS_OWNER);
             self._transfer_ownership(owner);
         }
 
@@ -280,7 +292,6 @@ pub mod OwnableComponent {
         fn assert_only_owner(self: @ComponentState<TContractState>) {
             let owner = self.Ownable_owner.read();
             let caller = get_caller_address();
-            assert(!caller.is_zero(), Errors::ZERO_ADDRESS_CALLER);
             assert(caller == owner, Errors::NOT_OWNER);
         }
 
@@ -297,10 +308,7 @@ pub mod OwnableComponent {
 
             let previous_owner: ContractAddress = self.Ownable_owner.read();
             self.Ownable_owner.write(new_owner);
-            self
-                .emit(
-                    OwnershipTransferred { previous_owner: previous_owner, new_owner: new_owner }
-                );
+            self.emit(OwnershipTransferred { previous_owner, new_owner });
         }
 
         /// Sets a new pending owner.
@@ -311,12 +319,7 @@ pub mod OwnableComponent {
         fn _propose_owner(ref self: ComponentState<TContractState>, new_owner: ContractAddress) {
             let previous_owner = self.Ownable_owner.read();
             self.Ownable_pending_owner.write(new_owner);
-            self
-                .emit(
-                    OwnershipTransferStarted {
-                        previous_owner: previous_owner, new_owner: new_owner
-                    }
-                );
+            self.emit(OwnershipTransferStarted { previous_owner, new_owner });
         }
     }
 }
diff --git a/packages/access/src/tests/test_accesscontrol.cairo b/packages/access/src/tests/test_accesscontrol.cairo
@@ -81,15 +81,15 @@ fn test_assert_only_role() {
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_assert_only_role_unauthorized() {
     let state = setup();
     start_cheat_caller_address(test_address(), OTHER());
     state.assert_only_role(ROLE);
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_assert_only_role_unauthorized_when_authorized_for_another_role() {
     let mut state = setup();
     state.grant_role(ROLE, AUTHORIZED());
@@ -151,15 +151,15 @@ fn test_grantRole_multiple_times_for_granted_role() {
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_grant_role_unauthorized() {
     let mut state = setup();
     start_cheat_caller_address(test_address(), AUTHORIZED());
     state.grant_role(ROLE, AUTHORIZED());
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_grantRole_unauthorized() {
     let mut state = setup();
     start_cheat_caller_address(test_address(), AUTHORIZED());
@@ -245,15 +245,15 @@ fn test_revokeRole_multiple_times_for_granted_role() {
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_revoke_role_unauthorized() {
     let mut state = setup();
     start_cheat_caller_address(test_address(), OTHER());
     state.revoke_role(ROLE, AUTHORIZED());
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_revokeRole_unauthorized() {
     let mut state = setup();
     start_cheat_caller_address(test_address(), OTHER());
@@ -345,7 +345,7 @@ fn test_renounceRole_multiple_times_for_granted_role() {
 }
 
 #[test]
-#[should_panic(expected: ('Can only renounce role for self',))]
+#[should_panic(expected: 'Can only renounce role for self')]
 fn test_renounce_role_unauthorized() {
     let mut state = setup();
     let contract_address = test_address();
@@ -357,7 +357,7 @@ fn test_renounce_role_unauthorized() {
 }
 
 #[test]
-#[should_panic(expected: ('Can only renounce role for self',))]
+#[should_panic(expected: 'Can only renounce role for self')]
 fn test_renounceRole_unauthorized() {
     let mut state = setup();
     start_cheat_caller_address(test_address(), ADMIN());
@@ -423,7 +423,7 @@ fn test_new_admin_can_revoke_roles() {
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_previous_admin_cannot_grant_roles() {
     let mut state = setup();
     state.set_role_admin(ROLE, OTHER_ROLE);
@@ -432,7 +432,7 @@ fn test_previous_admin_cannot_grant_roles() {
 }
 
 #[test]
-#[should_panic(expected: ('Caller is missing role',))]
+#[should_panic(expected: 'Caller is missing role')]
 fn test_previous_admin_cannot_revoke_roles() {
     let mut state = setup();
     state.set_role_admin(ROLE, OTHER_ROLE);