From 6ed8c74e133b20b20309ccb0f518f84acd03a798 Mon Sep 17 00:00:00 2001 From: Gustavo Gonzalez Date: Mon, 18 Nov 2024 19:02:49 +0700 Subject: [PATCH] refactor access module --- .../src/accesscontrol/accesscontrol.cairo | 27 ++++++++++++----- packages/access/src/ownable/ownable.cairo | 29 ++++++++++--------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/packages/access/src/accesscontrol/accesscontrol.cairo b/packages/access/src/accesscontrol/accesscontrol.cairo index 239306afe..7ae4c292a 100644 --- 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. +/// 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,8 +42,8 @@ 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, @@ -40,7 +51,7 @@ pub mod AccessControlComponent { 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, 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 index b001e8dbd..ad59d601e 100644 --- a/packages/access/src/ownable/ownable.cairo +++ b/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) { - 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) { 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, 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 }); } } }