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.
+/// Contract component that 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 zero 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,11 @@ pub mod OwnableComponent {
         OwnershipTransferStarted: OwnershipTransferStarted
     }
 
+    /// Emitted when `new_owner` is set as owner of the contract.
+    /// `previous_owner` is the address of the previous owner. It will be the zero address if
+    /// the ownership is transferred from the constructor.
+    /// `new_owner` is the address of the new owner. It will be the zero address if the ownership
+    /// is renounced.
     #[derive(Drop, PartialEq, starknet::Event)]
     pub struct OwnershipTransferred {
         #[key]
@@ -43,6 +48,10 @@ 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 new owner. It will be the zero address if the ownership
+    /// is being renounced.
     #[derive(Drop, PartialEq, starknet::Event)]
     pub struct OwnershipTransferStarted {
         #[key]
@@ -83,8 +92,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:
         ///
@@ -150,7 +159,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());
         }
     }
 
@@ -280,7 +290,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 +306,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 +317,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 });
         }
     }
 }