Clean up and small refactors to the access module

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.
@@ -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);
         }

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 `newOwner` 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 });
         }
     }
 }

packages/access/src/tests/test_ownable.cairo

@@ -44,6 +44,13 @@ fn test_initializer_owner() {
     assert_eq!(new_owner, OWNER());
 }
 
+#[test]
+#[should_panic(expected: ('New owner is the zero address',))]
+fn test_initializer_zero_owner() {
+    let mut state = COMPONENT_STATE();
+    state.initializer(ZERO());
+}
+
 //
 // assert_only_owner
 //
@@ -63,13 +70,6 @@ fn test_assert_only_owner_when_not_owner() {
     state.assert_only_owner();
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_assert_only_owner_when_caller_zero() {
-    let state = setup();
-    state.assert_only_owner();
-}
-
 //
 // _transfer_ownership
 //
@@ -124,13 +124,6 @@ fn test_transfer_ownership_to_zero() {
     state.transfer_ownership(ZERO());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_transfer_ownership_from_zero() {
-    let mut state = setup();
-    state.transfer_ownership(OTHER());
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_transfer_ownership_from_nonowner() {
@@ -159,13 +152,6 @@ fn test_transferOwnership_to_zero() {
     state.transferOwnership(ZERO());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_transferOwnership_from_zero() {
-    let mut state = setup();
-    state.transferOwnership(OTHER());
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_transferOwnership_from_nonowner() {
@@ -190,13 +176,6 @@ fn test_renounce_ownership() {
     assert!(state.owner().is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_renounce_ownership_from_zero_address() {
-    let mut state = setup();
-    state.renounce_ownership();
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_renounce_ownership_from_nonowner() {
@@ -217,13 +196,6 @@ fn test_renounceOwnership() {
     assert!(state.owner().is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_renounceOwnership_from_zero_address() {
-    let mut state = setup();
-    state.renounceOwnership();
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_renounceOwnership_from_nonowner() {

packages/access/src/tests/test_ownable_twostep.cairo

@@ -96,13 +96,6 @@ fn test_transfer_ownership_to_zero() {
     assert_eq!(state.pending_owner(), ZERO());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_transfer_ownership_from_zero() {
-    let mut state = setup();
-    state.transfer_ownership(OTHER());
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_transfer_ownership_from_nonowner() {
@@ -144,13 +137,6 @@ fn test_transferOwnership_to_zero() {
     assert!(state.pendingOwner().is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_transferOwnership_from_zero() {
-    let mut state = setup();
-    state.transferOwnership(OTHER());
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_transferOwnership_from_nonowner() {
@@ -244,13 +230,6 @@ fn test_renounce_ownership_resets_pending_owner() {
     assert!(current_pending_owner.is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_renounce_ownership_from_zero_address() {
-    let mut state = setup();
-    state.renounce_ownership();
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_renounce_ownership_from_nonowner() {
@@ -272,13 +251,6 @@ fn test_renounceOwnership() {
     assert!(state.owner().is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_renounceOwnership_from_zero_address() {
-    let mut state = setup();
-    state.renounceOwnership();
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_renounceOwnership_from_nonowner() {

packages/presets/src/tests/test_erc1155.cairo

@@ -656,14 +656,6 @@ fn test_transfer_ownership_to_zero() {
     dispatcher.transfer_ownership(ZERO());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_transfer_ownership_from_zero() {
-    let (_, dispatcher, _) = setup_dispatcher();
-    start_cheat_caller_address(dispatcher.contract_address, ZERO());
-    dispatcher.transfer_ownership(OTHER());
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_transfer_ownership_from_nonowner() {
@@ -690,14 +682,6 @@ fn test_transferOwnership_to_zero() {
     dispatcher.transferOwnership(ZERO());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_transferOwnership_from_zero() {
-    let (_, dispatcher, _) = setup_dispatcher();
-    start_cheat_caller_address(dispatcher.contract_address, ZERO());
-    dispatcher.transferOwnership(OTHER());
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_transferOwnership_from_nonowner() {
@@ -720,14 +704,6 @@ fn test_renounce_ownership() {
     assert!(dispatcher.owner().is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_renounce_ownership_from_zero_address() {
-    let (_, dispatcher, _) = setup_dispatcher();
-    start_cheat_caller_address(dispatcher.contract_address, ZERO());
-    dispatcher.renounce_ownership();
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_renounce_ownership_from_nonowner() {
@@ -746,14 +722,6 @@ fn test_renounceOwnership() {
     assert!(dispatcher.owner().is_zero());
 }
 
-#[test]
-#[should_panic(expected: ('Caller is the zero address',))]
-fn test_renounceOwnership_from_zero_address() {
-    let (_, dispatcher, _) = setup_dispatcher();
-    start_cheat_caller_address(dispatcher.contract_address, ZERO());
-    dispatcher.renounceOwnership();
-}
-
 #[test]
 #[should_panic(expected: ('Caller is not the owner',))]
 fn test_renounceOwnership_from_nonowner() {