From df2501000f76c6515ac212990c9dca3c6f03733a Mon Sep 17 00:00:00 2001 From: Remo Date: Wed, 18 Mar 2026 14:41:19 +0200 Subject: [PATCH 1/3] feat: minimal roles component --- packages/utils/src/components.cairo | 1 + .../src/components/blocklist/blocklist.cairo | 14 +- .../components/blocklist/mock_contract.cairo | 7 +- .../utils/src/components/common_roles.cairo | 9 + .../common_roles/common_roles.cairo | 211 +++++++++ .../common_roles/mock_contract.cairo | 84 ++++ .../utils/src/components/common_roles/spec.md | 124 ++++++ .../src/components/common_roles/test.cairo | 376 ++++++++++++++++ .../src/components/pausable/pausable.cairo | 14 +- .../src/components/replaceability/mock.cairo | 14 +- .../replaceability/replaceability.cairo | 18 +- .../src/components/replaceability/test.cairo | 25 +- .../replaceability/test_utils.cairo | 8 +- .../utils/src/components/roles/errors.cairo | 4 +- .../src/components/roles/interface.cairo | 116 ++++- .../src/components/roles/mock_contract.cairo | 5 + .../utils/src/components/roles/roles.cairo | 412 +++++++++++++----- .../utils/src/components/roles/test.cairo | 187 +++++++- 18 files changed, 1475 insertions(+), 154 deletions(-) create mode 100644 packages/utils/src/components/common_roles.cairo create mode 100644 packages/utils/src/components/common_roles/common_roles.cairo create mode 100644 packages/utils/src/components/common_roles/mock_contract.cairo create mode 100644 packages/utils/src/components/common_roles/spec.md create mode 100644 packages/utils/src/components/common_roles/test.cairo diff --git a/packages/utils/src/components.cairo b/packages/utils/src/components.cairo index 6581ec90..5d68863e 100644 --- a/packages/utils/src/components.cairo +++ b/packages/utils/src/components.cairo @@ -1,4 +1,5 @@ pub mod blocklist; +pub mod common_roles; pub mod deposit; pub mod nonce; pub mod pausable; diff --git a/packages/utils/src/components/blocklist/blocklist.cairo b/packages/utils/src/components/blocklist/blocklist.cairo index bc56b75c..d7f4a097 100644 --- a/packages/utils/src/components/blocklist/blocklist.cairo +++ b/packages/utils/src/components/blocklist/blocklist.cairo @@ -1,13 +1,13 @@ #[starknet::component] pub mod blocklist { - use RolesComponent::InternalTrait as RolesInternalTrait; + use CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; use openzeppelin::access::accesscontrol::AccessControlComponent; use openzeppelin::introspection::src5::SRC5Component; use starknet::storage::{Map, StorageMapReadAccess, StorageMapWriteAccess}; use starknet::{ContractAddress, get_caller_address}; use starkware_utils::components::blocklist::events::{Blocklisted, Unblocklisted}; use starkware_utils::components::blocklist::interface::IBlocklist; - use starkware_utils::components::roles::RolesComponent; + use starkware_utils::components::common_roles::CommonRolesComponent; #[storage] pub struct Storage { @@ -30,7 +30,7 @@ pub mod blocklist { TContractState, +HasComponent, +Drop, - impl Roles: RolesComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, +AccessControlComponent::HasComponent, +SRC5Component::HasComponent, > of IBlocklist> { @@ -43,8 +43,8 @@ pub mod blocklist { /// /// Can be called only by security admin. fn add_to_blocklist(ref self: ComponentState, account: ContractAddress) { - let roles = get_dep_component!(@self, Roles); - roles.only_security_admin(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_security_admin(); self.blocklist.write(account, true); self.emit(Blocklisted { account, caller: get_caller_address() }); } @@ -55,8 +55,8 @@ pub mod blocklist { fn remove_from_blocklist( ref self: ComponentState, account: ContractAddress, ) { - let roles = get_dep_component!(@self, Roles); - roles.only_security_admin(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_security_admin(); self.blocklist.write(account, false); self.emit(Unblocklisted { account, caller: get_caller_address() }); } diff --git a/packages/utils/src/components/blocklist/mock_contract.cairo b/packages/utils/src/components/blocklist/mock_contract.cairo index 44fb591e..0ff77a4f 100644 --- a/packages/utils/src/components/blocklist/mock_contract.cairo +++ b/packages/utils/src/components/blocklist/mock_contract.cairo @@ -6,6 +6,7 @@ pub mod blocklist_mock_contract { use starknet::ContractAddress; use starkware_utils::components::blocklist::blocklist::blocklist as BlocklistComponent; use starkware_utils::components::blocklist::blocklist::blocklist::InternalTrait as BlocklistInternalTrait; + use starkware_utils::components::common_roles::CommonRolesComponent; use starkware_utils::components::roles::RolesComponent; use starkware_utils::components::roles::RolesComponent::InternalTrait as RolesInternalTrait; use starkware_utils::interfaces::mintable_token::IMintableToken; @@ -13,6 +14,7 @@ pub mod blocklist_mock_contract { component!(path: ERC20Component, storage: erc20, event: ERC20Event); component!(path: BlocklistComponent, storage: blocklist, event: BlocklistEvent); component!(path: RolesComponent, storage: roles, event: RolesEvent); + component!(path: CommonRolesComponent, storage: common_roles, event: CommonRolesEvent); component!(path: AccessControlComponent, storage: access_control, event: AccessControlEvent); component!(path: SRC5Component, storage: src5, event: SRC5Event); @@ -21,7 +23,6 @@ pub mod blocklist_mock_contract { #[abi(embed_v0)] impl ERC20MetadataImpl = ERC20Component::ERC20MetadataImpl; - #[abi(embed_v0)] impl BlocklistImpl = BlocklistComponent::BlocklistImpl; #[abi(embed_v0)] @@ -39,6 +40,8 @@ pub mod blocklist_mock_contract { #[substorage(v0)] roles: RolesComponent::Storage, #[substorage(v0)] + common_roles: CommonRolesComponent::Storage, + #[substorage(v0)] access_control: AccessControlComponent::Storage, #[substorage(v0)] src5: SRC5Component::Storage, @@ -54,6 +57,8 @@ pub mod blocklist_mock_contract { #[flat] RolesEvent: RolesComponent::Event, #[flat] + CommonRolesEvent: CommonRolesComponent::Event, + #[flat] AccessControlEvent: AccessControlComponent::Event, #[flat] SRC5Event: SRC5Component::Event, diff --git a/packages/utils/src/components/common_roles.cairo b/packages/utils/src/components/common_roles.cairo new file mode 100644 index 00000000..661efb15 --- /dev/null +++ b/packages/utils/src/components/common_roles.cairo @@ -0,0 +1,9 @@ +pub(crate) mod common_roles; + +pub use common_roles::CommonRolesComponent; + +#[cfg(test)] +pub(crate) mod mock_contract; + +#[cfg(test)] +mod test; diff --git a/packages/utils/src/components/common_roles/common_roles.cairo b/packages/utils/src/components/common_roles/common_roles.cairo new file mode 100644 index 00000000..258ac6ad --- /dev/null +++ b/packages/utils/src/components/common_roles/common_roles.cairo @@ -0,0 +1,211 @@ +#[starknet::component] +pub(crate) mod CommonRolesComponent { + use core::num::traits::Zero; + use openzeppelin::access::accesscontrol::AccessControlComponent; + use openzeppelin::access::accesscontrol::AccessControlComponent::{ + AccessControlImpl, InternalTrait as AccessInternalTrait, + }; + use openzeppelin::introspection::src5::SRC5Component; + use starknet::{ContractAddress, get_caller_address}; + use starkware_utils::components::roles::errors::AccessErrors; + use starkware_utils::components::roles::interface::{ + APP_GOVERNOR, APP_ROLE_ADMIN, GOVERNANCE_ADMIN, ICommonRoles, OPERATOR, Role, RoleId, + SECURITY_ADMIN, SECURITY_AGENT, SECURITY_GOVERNOR, TOKEN_ADMIN, UPGRADE_AGENT, + UPGRADE_GOVERNOR, is_renounceable, + }; + + pub const ROLE_ADMIN_PAIRS: [(RoleId, RoleId); 10] = [ + (APP_GOVERNOR, APP_ROLE_ADMIN), (APP_ROLE_ADMIN, GOVERNANCE_ADMIN), + (GOVERNANCE_ADMIN, GOVERNANCE_ADMIN), (OPERATOR, APP_ROLE_ADMIN), + (TOKEN_ADMIN, APP_ROLE_ADMIN), (UPGRADE_AGENT, APP_ROLE_ADMIN), + (UPGRADE_GOVERNOR, GOVERNANCE_ADMIN), (SECURITY_ADMIN, SECURITY_ADMIN), + (SECURITY_AGENT, SECURITY_ADMIN), (SECURITY_GOVERNOR, SECURITY_ADMIN), + ]; + + #[storage] + pub struct Storage {} + + #[event] + #[derive(Drop, starknet::Event)] + pub enum Event {} + + #[embeddable_as(CommonRolesImpl)] + pub impl CommonRoles< + TContractState, + +HasComponent, + +Drop, + impl Access: AccessControlComponent::HasComponent, + +SRC5Component::HasComponent, + > of ICommonRoles> { + fn grant_role( + ref self: ComponentState, role: Role, account: ContractAddress, + ) { + let role_id: RoleId = role.into(); + assert!(account.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS); + let mut access_comp = get_dep_component_mut!(ref self, Access); + access_comp.grant_role(role: role_id, :account); + } + + fn revoke_role( + ref self: ComponentState, role: Role, account: ContractAddress, + ) { + let role_id: RoleId = role.into(); + let mut access_comp = get_dep_component_mut!(ref self, Access); + access_comp.revoke_role(role: role_id, :account); + } + + fn has_role( + self: @ComponentState, role: Role, account: ContractAddress, + ) -> bool { + let role_id: RoleId = role.into(); + let access_comp = get_dep_component!(self, Access); + access_comp.has_role(role: role_id, :account) + } + + fn renounce(ref self: ComponentState, role: Role) { + assert!(is_renounceable(role), "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); + let role_id: RoleId = role.into(); + let mut access_comp = get_dep_component_mut!(ref self, Access); + access_comp.renounce_role(role: role_id, account: get_caller_address()); + } + } + + #[generate_trait] + pub impl InternalImpl< + TContractState, + +HasComponent, + +Drop, + impl Access: AccessControlComponent::HasComponent, + +SRC5Component::HasComponent, + > of InternalTrait { + // WARNING + // Unprotected — call only from constructor (or test setup). + fn initialize(ref self: ComponentState, governance_admin: ContractAddress) { + let mut access_comp = get_dep_component_mut!(ref self, Access); + let un_initialized = access_comp.get_role_admin(role: GOVERNANCE_ADMIN).is_zero(); + assert!(un_initialized, "{}", AccessErrors::ALREADY_INITIALIZED); + access_comp.initializer(); + assert!(governance_admin.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS_GOV_ADMIN); + access_comp._grant_role(role: GOVERNANCE_ADMIN, account: governance_admin); + access_comp._grant_role(role: SECURITY_ADMIN, account: governance_admin); + Self::_set_role_admins(ref access_comp); + } + + fn _set_role_admins( + ref access_comp: AccessControlComponent::ComponentState, + ) { + for (role, admin_role) in ROLE_ADMIN_PAIRS.span() { + if access_comp.get_role_admin(role: *role).is_zero() { + access_comp.set_role_admin(role: *role, admin_role: *admin_role); + } + } + } + + // WARNING + // Unprotected — intended for use by contracts performing an upgrade migration + // that adds new roles to an already-initialized deployment. Idempotent: only sets + // role-admin pairs that are currently zero, so safe to call on a fresh contract too. + fn ensure_role_admins(ref self: ComponentState) { + let mut access_comp = get_dep_component_mut!(ref self, Access); + Self::_set_role_admins(ref access_comp); + } + + fn _grant_role( + ref self: ComponentState, role: RoleId, account: ContractAddress, + ) { + let mut access_comp = get_dep_component_mut!(ref self, Access); + access_comp._grant_role(:role, :account); + } + + fn _revoke_role( + ref self: ComponentState, role: RoleId, account: ContractAddress, + ) { + let mut access_comp = get_dep_component_mut!(ref self, Access); + access_comp._revoke_role(:role, :account); + } + + fn only_role(self: @ComponentState, role: Role) { + let role_id: RoleId = role.into(); + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: role_id, account: get_caller_address()), + "{}", + AccessErrors::CALLER_MISSING_ROLE, + ); + } + + fn only_app_governor(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: APP_GOVERNOR, account: get_caller_address()), + "{}", + AccessErrors::ONLY_APP_GOVERNOR, + ); + } + + fn only_operator(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: OPERATOR, account: get_caller_address()), + "{}", + AccessErrors::ONLY_OPERATOR, + ); + } + + fn only_token_admin(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: TOKEN_ADMIN, account: get_caller_address()), + "{}", + AccessErrors::ONLY_TOKEN_ADMIN, + ); + } + + fn only_upgrade_governor(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: UPGRADE_GOVERNOR, account: get_caller_address()), + "{}", + AccessErrors::ONLY_UPGRADE_GOVERNOR, + ); + } + + fn only_upgrader(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + let caller = get_caller_address(); + assert!( + access_comp.has_role(role: UPGRADE_AGENT, account: caller) + || access_comp.has_role(role: UPGRADE_GOVERNOR, account: caller), + "{}", + AccessErrors::ONLY_UPGRADER, + ); + } + + fn only_security_admin(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: SECURITY_ADMIN, account: get_caller_address()), + "{}", + AccessErrors::ONLY_SECURITY_ADMIN, + ); + } + + fn only_security_agent(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: SECURITY_AGENT, account: get_caller_address()), + "{}", + AccessErrors::ONLY_SECURITY_AGENT, + ); + } + + fn only_security_governor(self: @ComponentState) { + let access_comp = get_dep_component!(self, Access); + assert!( + access_comp.has_role(role: SECURITY_GOVERNOR, account: get_caller_address()), + "{}", + AccessErrors::ONLY_SECURITY_GOVERNOR, + ); + } + } +} diff --git a/packages/utils/src/components/common_roles/mock_contract.cairo b/packages/utils/src/components/common_roles/mock_contract.cairo new file mode 100644 index 00000000..594357d6 --- /dev/null +++ b/packages/utils/src/components/common_roles/mock_contract.cairo @@ -0,0 +1,84 @@ +#[starknet::interface] +pub(crate) trait IGuardTest { + fn assert_only_role(self: @TState, role: starkware_utils::components::roles::interface::Role); + fn assert_only_app_governor(self: @TState); + fn assert_only_operator(self: @TState); + fn assert_only_token_admin(self: @TState); + fn assert_only_upgrade_governor(self: @TState); + fn assert_only_upgrader(self: @TState); + fn assert_only_security_admin(self: @TState); + fn assert_only_security_agent(self: @TState); + fn assert_only_security_governor(self: @TState); +} + +#[starknet::contract] +pub(crate) mod CommonRolesMock { + use openzeppelin::access::accesscontrol::AccessControlComponent; + use openzeppelin::introspection::src5::SRC5Component; + use starknet::ContractAddress; + use starkware_utils::components::common_roles::CommonRolesComponent; + use starkware_utils::components::common_roles::CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; + use starkware_utils::components::roles::interface::Role; + use super::IGuardTest; + + component!(path: CommonRolesComponent, storage: common_roles, event: CommonRolesEvent); + component!(path: AccessControlComponent, storage: accesscontrol, event: AccessControlEvent); + component!(path: SRC5Component, storage: src5, event: SRC5Event); + + #[storage] + struct Storage { + #[substorage(v0)] + common_roles: CommonRolesComponent::Storage, + #[substorage(v0)] + accesscontrol: AccessControlComponent::Storage, + #[substorage(v0)] + src5: SRC5Component::Storage, + } + + #[event] + #[derive(Drop, starknet::Event)] + pub(crate) enum Event { + CommonRolesEvent: CommonRolesComponent::Event, + AccessControlEvent: AccessControlComponent::Event, + SRC5Event: SRC5Component::Event, + } + + #[constructor] + fn constructor(ref self: ContractState, governance_admin: ContractAddress) { + self.common_roles.initialize(:governance_admin); + } + + #[abi(embed_v0)] + impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; + + #[abi(embed_v0)] + impl GuardTestImpl of IGuardTest { + fn assert_only_role(self: @ContractState, role: Role) { + self.common_roles.only_role(:role); + } + fn assert_only_app_governor(self: @ContractState) { + self.common_roles.only_app_governor(); + } + fn assert_only_operator(self: @ContractState) { + self.common_roles.only_operator(); + } + fn assert_only_token_admin(self: @ContractState) { + self.common_roles.only_token_admin(); + } + fn assert_only_upgrade_governor(self: @ContractState) { + self.common_roles.only_upgrade_governor(); + } + fn assert_only_upgrader(self: @ContractState) { + self.common_roles.only_upgrader(); + } + fn assert_only_security_admin(self: @ContractState) { + self.common_roles.only_security_admin(); + } + fn assert_only_security_agent(self: @ContractState) { + self.common_roles.only_security_agent(); + } + fn assert_only_security_governor(self: @ContractState) { + self.common_roles.only_security_governor(); + } + } +} diff --git a/packages/utils/src/components/common_roles/spec.md b/packages/utils/src/components/common_roles/spec.md new file mode 100644 index 00000000..712bc385 --- /dev/null +++ b/packages/utils/src/components/common_roles/spec.md @@ -0,0 +1,124 @@ +# CommonRolesComponent — Specification + +## Overview + +The role system is split into two layers: + +- **`CommonRolesComponent`** — lean infrastructure. No own storage, empty event enum. Owns the role constants, the `Role` enum, role admin configuration, role guards (`only_X`), and a generic 4-method ABI (`ICommonRoles`). +- **`RolesComponent`** — full-featured layer. Delegates all role state to `CommonRolesComponent` internally. Adds named role events (20 variants), category-scoped embeddable impls (`IGovernanceRoles`, `ISecurityRoles`, `IAppRoles`), the fat `IRoles` interface (~30 EPs), and legacy role reclaim support. + +The design lets contracts pick exactly the role surface they need — from zero ABI overhead up to the full named interface — without duplicating any role logic. + +--- + +## Role Hierarchy + +| Role | Admin Role | Category Interface | +|---|---|---| +| `GovernanceAdmin` | `GovernanceAdmin` (self) | `IGovernanceRoles` | +| `AppRoleAdmin` | `GovernanceAdmin` | `IGovernanceRoles` | +| `UpgradeGovernor` | `GovernanceAdmin` | `IGovernanceRoles` | +| `UpgradeAgent` | `AppRoleAdmin` | `IGovernanceRoles` | +| `SecurityAdmin` | `SecurityAdmin` (self) | `ISecurityRoles` | +| `SecurityAgent` | `SecurityAdmin` | `ISecurityRoles` | +| `SecurityGovernor` | `SecurityAdmin` | `ISecurityRoles` | +| `AppGovernor` | `AppRoleAdmin` | `IAppRoles` | +| `Operator` | `AppRoleAdmin` | `IAppRoles` | +| `TokenAdmin` | `AppRoleAdmin` | `IAppRoles` | + +`GovernanceAdmin` and `SecurityAdmin` are self-administered and cannot be renounced. All other roles are renounceable. + +--- + +## Category Sub-Interfaces + +Three `#[starknet::interface]` traits group related roles into a narrower ABI slice: + +- **`IGovernanceRoles`** — governance admin, app role admin, upgrade governor, upgrade agent. These are coupled: governance admin appoints upgrade governors; upgrade governors control contract upgrades; app role admin is the gateway to the app-level roles. +- **`ISecurityRoles`** — security admin, security agent, security governor. +- **`IAppRoles`** — app governor, operator, token admin. + +These interfaces are defined in `roles/interface.cairo` and **implemented only by `RolesComponent`** (`GovernanceRolesImpl`, `SecurityRolesImpl`, `AppRolesImpl`). They exist so contracts can expose a scoped ABI without pulling in all 30 `IRoles` entry points. + +--- + +## Usage Patterns + +### Tier A — Infrastructure only + +For components that only need role *checks*, with no named role management ABI of their own (e.g., replaceability, pausable, blocklist). + +**Wire:** `CommonRolesComponent` + `AccessControlComponent` + `SRC5Component`. No `RolesComponent`. + +**Embed:** `CommonRolesImpl` → 4 generic entry points: `grant_role`, `revoke_role`, `has_role`, `renounce`. + +**Role management:** callers use `ICommonRoles::grant_role(Role::UpgradeGovernor, account)`. + +**Constructor:** +```cairo +fn constructor(ref self: ContractState, governance_admin: ContractAddress) { + self.common_roles.initialize(:governance_admin); +} +``` + +**Overhead:** none. `CommonRolesComponent::Event` is empty; `CommonRolesComponent` has no storage. + +--- + +### Tier B — Selective named roles + +For contracts that need a subset of named role management EPs and their named events, but not the full `IRoles` bloat (e.g., an ERC20 with replaceability that only needs governance + upgrade management). + +**Wire:** `RolesComponent` + `CommonRolesComponent` + `AccessControlComponent` + `SRC5Component`. + +**Embed:** only the category impl(s) you need — do NOT embed `RolesImpl`. + +```cairo +#[abi(embed_v0)] +impl GovernanceRolesImpl = RolesComponent::GovernanceRolesImpl; +#[abi(embed_v0)] +impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; +// NO RolesImpl +``` + +**ABI result:** `IGovernanceRoles` (12 EPs) + `ICommonRoles` (4 EPs). Named events (`UpgradeGovernorAdded`, etc.) are emitted by the category impl methods. Unused event variants are dead-code eliminated from Sierra. + +**Constructor:** +```cairo +fn constructor(ref self: ContractState, governance_admin: ContractAddress) { + self.roles.initialize(:governance_admin); // NOT common_roles.initialize +} +``` + +> **Why `roles.initialize` and not `common_roles.initialize`?** +> +> `roles.initialize` does two things: calls `common_roles.initialize` (role setup) and then writes `legacy_role_reclaim_disabled = true`. If a future upgrade adds `RolesImpl` to the ABI, this flag blocks inadvertent legacy role reclaim. Calling `common_roles.initialize` directly skips the flag, leaving a latent upgrade footgun — harmless today, but dangerous if `RolesImpl` is ever added. + +**Overhead:** 2 legacy storage slots in `RolesComponent` (`role_members` map + `legacy_role_reclaim_disabled` bool). Neither is ever written in normal operation, so there is no runtime storage cost. + +--- + +### Tier C — Full roles + +For contracts that expose the complete named role management API (existing pattern). + +**Wire:** `RolesComponent` + `CommonRolesComponent` + `AccessControlComponent` + `SRC5Component`. + +**Embed:** `RolesImpl` → all ~30 entry points + all 20 named events. + +**Constructor:** +```cairo +fn constructor(ref self: ContractState, governance_admin: ContractAddress) { + self.roles.initialize(:governance_admin); +} +``` + +--- + +## Key Invariants + +- `CommonRolesComponent` carries no role state of its own — all state lives in `AccessControlComponent`. +- `RolesComponent` never duplicates role state — it reads and writes through `CommonRolesComponent`'s internal methods. +- Named events exist only in `RolesComponent::Event`. `CommonRolesComponent::Event` is always empty. +- Category impls exist only in `RolesComponent`. There is no duplication between layers. +- `IGovernanceRolesDispatcher` dispatches correctly against any contract that embeds `RolesImpl`, because `IRoles` exposes the same selectors as `IGovernanceRoles`. diff --git a/packages/utils/src/components/common_roles/test.cairo b/packages/utils/src/components/common_roles/test.cairo new file mode 100644 index 00000000..37e6a84a --- /dev/null +++ b/packages/utils/src/components/common_roles/test.cairo @@ -0,0 +1,376 @@ +use openzeppelin::access::accesscontrol::AccessControlComponent::Errors as OZAccessErrors; +use snforge_std::{ContractClassTrait, DeclareResultTrait, declare}; +use starknet::ContractAddress; +use starkware_utils::components::common_roles::mock_contract::{ + IGuardTestDispatcher, IGuardTestDispatcherTrait, IGuardTestSafeDispatcher, + IGuardTestSafeDispatcherTrait, +}; +use starkware_utils::components::roles::errors::AccessErrors; +use starkware_utils::components::roles::interface::{ + ICommonRolesDispatcher, ICommonRolesDispatcherTrait, ICommonRolesSafeDispatcher, + ICommonRolesSafeDispatcherTrait, Role, +}; +use starkware_utils::errors::Describable; +use starkware_utils_testing::constants; +use starkware_utils_testing::test_utils::{ + assert_panic_with_error, assert_panic_with_felt_error, cheat_caller_address_once, +}; + +fn deploy() -> (ContractAddress, ICommonRolesDispatcher) { + let contract = *declare("CommonRolesMock").unwrap().contract_class(); + let (address, _) = contract.deploy(@array![constants::INITIAL_ROOT_ADMIN.into()]).unwrap(); + (address, ICommonRolesDispatcher { contract_address: address }) +} + +fn deploy_safe() -> (ContractAddress, ICommonRolesSafeDispatcher) { + let (address, _) = deploy(); + (address, ICommonRolesSafeDispatcher { contract_address: address }) +} + +// ─── initialize +// ────────────────────────────────────────────────────────────── + +#[test] +fn test_initialize_grants_governance_admin() { + let (_, dispatcher) = deploy(); + assert!(dispatcher.has_role(Role::GovernanceAdmin, constants::INITIAL_ROOT_ADMIN)); +} + +#[test] +fn test_initialize_grants_security_admin() { + let (_, dispatcher) = deploy(); + assert!(dispatcher.has_role(Role::SecurityAdmin, constants::INITIAL_ROOT_ADMIN)); +} + +// ─── grant_role / has_role +// ──────────────────────────────────────────────────── + +#[test] +fn test_grant_role_authorized() { + let (address, dispatcher) = deploy(); + let account = constants::APP_ROLE_ADMIN; + let gov_admin = constants::INITIAL_ROOT_ADMIN; + + assert!(!dispatcher.has_role(Role::AppRoleAdmin, account)); + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.grant_role(Role::AppRoleAdmin, account); + assert!(dispatcher.has_role(Role::AppRoleAdmin, account)); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_grant_role_unauthorized() { + let (address, safe) = deploy_safe(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = safe.grant_role(Role::AppRoleAdmin, constants::APP_ROLE_ADMIN); + assert_panic_with_felt_error(:result, expected_error: OZAccessErrors::MISSING_ROLE); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_grant_role_zero_address() { + let (address, safe) = deploy_safe(); + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + let result = safe.grant_role(Role::GovernanceAdmin, 0.try_into().unwrap()); + assert_panic_with_error(:result, expected_error: AccessErrors::ZERO_ADDRESS.describe()); +} + +// ─── revoke_role +// ───────────────────────────────────────────────────────────── + +#[test] +fn test_revoke_role() { + let (address, dispatcher) = deploy(); + let account = constants::APP_ROLE_ADMIN; + let gov_admin = constants::INITIAL_ROOT_ADMIN; + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.grant_role(Role::AppRoleAdmin, account); + assert!(dispatcher.has_role(Role::AppRoleAdmin, account)); + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.revoke_role(Role::AppRoleAdmin, account); + assert!(!dispatcher.has_role(Role::AppRoleAdmin, account)); +} + +// ─── renounce +// ──────────────────────────────────────────────────────────────── + +#[test] +fn test_renounce_renounceable_role() { + let (address, dispatcher) = deploy(); + let account = constants::APP_ROLE_ADMIN; + let gov_admin = constants::INITIAL_ROOT_ADMIN; + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.grant_role(Role::AppRoleAdmin, account); + + cheat_caller_address_once(contract_address: address, caller_address: account); + dispatcher.renounce(Role::AppRoleAdmin); + assert!(!dispatcher.has_role(Role::AppRoleAdmin, account)); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_renounce_governance_admin_panics() { + let (address, safe) = deploy_safe(); + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + let result = safe.renounce(Role::GovernanceAdmin); + assert_panic_with_error( + :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), + ); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_renounce_security_admin_panics() { + let (address, safe) = deploy_safe(); + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + let result = safe.renounce(Role::SecurityAdmin); + assert_panic_with_error( + :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), + ); +} + +// ─── only_X guards +// ──────────────────────────────────────────────────────────── + +fn setup_guard_test() -> (ContractAddress, ICommonRolesDispatcher, IGuardTestSafeDispatcher) { + let (address, dispatcher) = deploy(); + let guard = IGuardTestSafeDispatcher { contract_address: address }; + (address, dispatcher, guard) +} + +fn grant( + address: ContractAddress, dispatcher: ICommonRolesDispatcher, role: Role, to: ContractAddress, +) { + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(:role, account: to); +} + +// ── only_role ── + +#[test] +fn test_only_role_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + grant(address, dispatcher, Role::AppRoleAdmin, constants::APP_ROLE_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::APP_ROLE_ADMIN); + guard.assert_only_role(Role::AppRoleAdmin); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_role_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_role(Role::AppRoleAdmin); + assert_panic_with_error(:result, expected_error: AccessErrors::CALLER_MISSING_ROLE.describe()); +} + +// ── only_app_governor ── + +#[test] +fn test_only_app_governor_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::AppRoleAdmin, constants::APP_ROLE_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::APP_ROLE_ADMIN); + dispatcher.grant_role(Role::AppGovernor, constants::APP_GOVERNOR); + cheat_caller_address_once(contract_address: address, caller_address: constants::APP_GOVERNOR); + guard.assert_only_app_governor(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_app_governor_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_app_governor(); + assert_panic_with_error(:result, expected_error: AccessErrors::ONLY_APP_GOVERNOR.describe()); +} + +// ── only_operator ── + +#[test] +fn test_only_operator_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::AppRoleAdmin, constants::APP_ROLE_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::APP_ROLE_ADMIN); + dispatcher.grant_role(Role::Operator, constants::OPERATOR); + cheat_caller_address_once(contract_address: address, caller_address: constants::OPERATOR); + guard.assert_only_operator(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_operator_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_operator(); + assert_panic_with_error(:result, expected_error: AccessErrors::ONLY_OPERATOR.describe()); +} + +// ── only_token_admin ── + +#[test] +fn test_only_token_admin_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::AppRoleAdmin, constants::APP_ROLE_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::APP_ROLE_ADMIN); + dispatcher.grant_role(Role::TokenAdmin, constants::TOKEN_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::TOKEN_ADMIN); + guard.assert_only_token_admin(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_token_admin_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_token_admin(); + assert_panic_with_error(:result, expected_error: AccessErrors::ONLY_TOKEN_ADMIN.describe()); +} + +// ── only_upgrade_governor ── + +#[test] +fn test_only_upgrade_governor_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + grant(address, dispatcher, Role::UpgradeGovernor, constants::UPGRADE_GOVERNOR); + cheat_caller_address_once( + contract_address: address, caller_address: constants::UPGRADE_GOVERNOR, + ); + guard.assert_only_upgrade_governor(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_upgrade_governor_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_upgrade_governor(); + assert_panic_with_error( + :result, expected_error: AccessErrors::ONLY_UPGRADE_GOVERNOR.describe(), + ); +} + +// ── only_upgrader (upgrade_governor OR upgrade_agent) ── + +#[test] +fn test_only_upgrader_authorized_as_governor() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + grant(address, dispatcher, Role::UpgradeGovernor, constants::UPGRADE_GOVERNOR); + cheat_caller_address_once( + contract_address: address, caller_address: constants::UPGRADE_GOVERNOR, + ); + guard.assert_only_upgrader(); +} + +#[test] +fn test_only_upgrader_authorized_as_agent() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::AppRoleAdmin, constants::APP_ROLE_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::APP_ROLE_ADMIN); + dispatcher.grant_role(Role::UpgradeAgent, constants::UPGRADE_AGENT); + cheat_caller_address_once(contract_address: address, caller_address: constants::UPGRADE_AGENT); + guard.assert_only_upgrader(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_upgrader_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_upgrader(); + assert_panic_with_error(:result, expected_error: AccessErrors::ONLY_UPGRADER.describe()); +} + +// ── only_security_admin ── + +#[test] +fn test_only_security_admin_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + grant(address, dispatcher, Role::SecurityAdmin, constants::SECURITY_ADMIN); + cheat_caller_address_once(contract_address: address, caller_address: constants::SECURITY_ADMIN); + guard.assert_only_security_admin(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_security_admin_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_security_admin(); + assert_panic_with_error(:result, expected_error: AccessErrors::ONLY_SECURITY_ADMIN.describe()); +} + +// ── only_security_agent ── + +#[test] +fn test_only_security_agent_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + grant(address, dispatcher, Role::SecurityAgent, constants::SECURITY_AGENT); + cheat_caller_address_once(contract_address: address, caller_address: constants::SECURITY_AGENT); + guard.assert_only_security_agent(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_security_agent_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_security_agent(); + assert_panic_with_error(:result, expected_error: AccessErrors::ONLY_SECURITY_AGENT.describe()); +} + +// ── only_security_governor ── + +#[test] +fn test_only_security_governor_authorized() { + let (address, dispatcher) = deploy(); + let guard = IGuardTestDispatcher { contract_address: address }; + grant(address, dispatcher, Role::SecurityGovernor, constants::SECURITY_GOVERNOR); + cheat_caller_address_once( + contract_address: address, caller_address: constants::SECURITY_GOVERNOR, + ); + guard.assert_only_security_governor(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_only_security_governor_unauthorized() { + let (address, _, guard) = setup_guard_test(); + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = guard.assert_only_security_governor(); + assert_panic_with_error( + :result, expected_error: AccessErrors::ONLY_SECURITY_GOVERNOR.describe(), + ); +} diff --git a/packages/utils/src/components/pausable/pausable.cairo b/packages/utils/src/components/pausable/pausable.cairo index a18c0862..570d64f8 100644 --- a/packages/utils/src/components/pausable/pausable.cairo +++ b/packages/utils/src/components/pausable/pausable.cairo @@ -1,12 +1,12 @@ #[starknet::component] pub(crate) mod PausableComponent { - use RolesComponent::InternalTrait as RolesInternalTrait; + use CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; use openzeppelin::access::accesscontrol::AccessControlComponent; use openzeppelin::introspection::src5::SRC5Component; use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess}; use starknet::{ContractAddress, get_caller_address}; + use starkware_utils::components::common_roles::CommonRolesComponent; use starkware_utils::components::pausable::interface::IPausable; - use starkware_utils::components::roles::RolesComponent; #[storage] pub struct Storage { @@ -42,7 +42,7 @@ pub(crate) mod PausableComponent { TContractState, +HasComponent, +Drop, - impl Roles: RolesComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, +AccessControlComponent::HasComponent, +SRC5Component::HasComponent, > of IPausable> { @@ -59,8 +59,8 @@ pub(crate) mod PausableComponent { /// /// Emits a `Paused` event. fn pause(ref self: ComponentState) { - let roles = get_dep_component!(@self, Roles); - roles.only_security_agent(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_security_agent(); self.assert_not_paused(); self.paused.write(true); self.emit(Paused { account: get_caller_address() }); @@ -74,8 +74,8 @@ pub(crate) mod PausableComponent { /// /// Emits an `Unpaused` event. fn unpause(ref self: ComponentState) { - let roles = get_dep_component!(@self, Roles); - roles.only_security_governor(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_security_governor(); self.assert_paused(); self.paused.write(false); self.emit(Unpaused { account: get_caller_address() }); diff --git a/packages/utils/src/components/replaceability/mock.cairo b/packages/utils/src/components/replaceability/mock.cairo index 4b6d879c..9cafcd4f 100644 --- a/packages/utils/src/components/replaceability/mock.cairo +++ b/packages/utils/src/components/replaceability/mock.cairo @@ -1,15 +1,15 @@ #[starknet::contract] pub(crate) mod ReplaceabilityMock { - use RolesComponent::InternalTrait as RolesInternalTrait; + use CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; use openzeppelin::access::accesscontrol::AccessControlComponent; use openzeppelin::introspection::src5::SRC5Component; use starknet::ContractAddress; + use starkware_utils::components::common_roles::CommonRolesComponent; use starkware_utils::components::replaceability::ReplaceabilityComponent; use starkware_utils::components::replaceability::ReplaceabilityComponent::InternalReplaceabilityTrait; - use starkware_utils::components::roles::RolesComponent; component!(path: ReplaceabilityComponent, storage: replaceability, event: ReplaceabilityEvent); - component!(path: RolesComponent, storage: roles, event: RolesEvent); + component!(path: CommonRolesComponent, storage: common_roles, event: CommonRolesEvent); component!(path: AccessControlComponent, storage: accesscontrol, event: AccessControlEvent); component!(path: SRC5Component, storage: src5, event: SRC5Event); @@ -18,7 +18,7 @@ pub(crate) mod ReplaceabilityMock { #[substorage(v0)] replaceability: ReplaceabilityComponent::Storage, #[substorage(v0)] - roles: RolesComponent::Storage, + common_roles: CommonRolesComponent::Storage, #[substorage(v0)] accesscontrol: AccessControlComponent::Storage, #[substorage(v0)] @@ -29,14 +29,14 @@ pub(crate) mod ReplaceabilityMock { #[derive(Drop, starknet::Event)] pub(crate) enum Event { ReplaceabilityEvent: ReplaceabilityComponent::Event, - RolesEvent: RolesComponent::Event, + CommonRolesEvent: CommonRolesComponent::Event, AccessControlEvent: AccessControlComponent::Event, SRC5Event: SRC5Component::Event, } #[constructor] fn constructor(ref self: ContractState, upgrade_delay: u64, governance_admin: ContractAddress) { - self.roles.initialize(:governance_admin); + self.common_roles.initialize(:governance_admin); self.replaceability.initialize(:upgrade_delay); } @@ -45,5 +45,5 @@ pub(crate) mod ReplaceabilityMock { ReplaceabilityComponent::ReplaceabilityImpl; #[abi(embed_v0)] - impl RolesImpl = RolesComponent::RolesImpl; + impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; } diff --git a/packages/utils/src/components/replaceability/replaceability.cairo b/packages/utils/src/components/replaceability/replaceability.cairo index 977fb4ad..93f10c76 100644 --- a/packages/utils/src/components/replaceability/replaceability.cairo +++ b/packages/utils/src/components/replaceability/replaceability.cairo @@ -10,13 +10,13 @@ pub(crate) mod ReplaceabilityComponent { StoragePointerWriteAccess, }; use starknet::syscalls::{library_call_syscall, replace_class_syscall}; + use starkware_utils::components::common_roles::CommonRolesComponent; + use starkware_utils::components::common_roles::CommonRolesComponent::InternalTrait; use starkware_utils::components::replaceability::errors::ReplaceErrors; use starkware_utils::components::replaceability::interface::{ EIC_INITIALIZE_SELECTOR, IMPLEMENTATION_EXPIRATION, IReplaceable, ImplementationAdded, ImplementationData, ImplementationFinalized, ImplementationRemoved, ImplementationReplaced, }; - use starkware_utils::components::roles::RolesComponent; - use starkware_utils::components::roles::RolesComponent::InternalTrait; #[storage] @@ -53,7 +53,7 @@ pub(crate) mod ReplaceabilityComponent { pub impl Replaceability< TContractState, +HasComponent, - impl Roles: RolesComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, +AccessControlComponent::HasComponent, +SRC5Component::HasComponent, +Drop, @@ -73,8 +73,8 @@ pub(crate) mod ReplaceabilityComponent { ref self: ComponentState, implementation_data: ImplementationData, ) { // The call is restricted to the upgrade governor. - let roles_comp = get_dep_component!(@self, Roles); - roles_comp.only_upgrade_governor(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_upgrade_governor(); let activation_time = get_block_timestamp() + self.get_upgrade_delay(); let expiration_time = activation_time + IMPLEMENTATION_EXPIRATION; @@ -89,8 +89,8 @@ pub(crate) mod ReplaceabilityComponent { ref self: ComponentState, implementation_data: ImplementationData, ) { // The call is restricted to the upgrade governor. - let roles_comp = get_dep_component!(@self, Roles); - roles_comp.only_upgrade_governor(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_upgrade_governor(); // Read implementation activation time. let impl_activation_time = self.get_impl_activation_time(:implementation_data); @@ -108,8 +108,8 @@ pub(crate) mod ReplaceabilityComponent { ref self: ComponentState, implementation_data: ImplementationData, ) { // The call is restricted to the upgrade agent or upgrade governor. - let roles_comp = get_dep_component!(@self, Roles); - roles_comp.only_upgrader(); + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_upgrader(); // Validate implementation is not finalized. assert!(!self.is_finalized(), "{}", ReplaceErrors::FINALIZED); diff --git a/packages/utils/src/components/replaceability/test.cairo b/packages/utils/src/components/replaceability/test.cairo index bf9571d4..c5f5018a 100644 --- a/packages/utils/src/components/replaceability/test.cairo +++ b/packages/utils/src/components/replaceability/test.cairo @@ -10,7 +10,7 @@ mod ReplaceabilityTests { use replaceability::mock::ReplaceabilityMock; use replaceability::test_utils::Constants::{ DEFAULT_UPGRADE_DELAY, DUMMY_FINAL_IMPLEMENTATION_DATA, DUMMY_NONFINAL_IMPLEMENTATION_DATA, - EIC_UPGRADE_DELAY_ADDITION, NOT_UPGRADE_GOVERNOR_ACCOUNT, + EIC_UPGRADE_DELAY_ADDITION, GOVERNANCE_ADMIN, NOT_UPGRADE_GOVERNOR_ACCOUNT, }; use replaceability::test_utils::{ assert_finalized_status, assert_implementation_finalized_event_emitted, @@ -24,6 +24,9 @@ mod ReplaceabilityTests { cheat_caller_address, get_class_hash, spy_events, }; use starkware_utils::components::replaceability; + use starkware_utils::components::roles::interface::{ + ICommonRolesDispatcher, ICommonRolesDispatcherTrait, Role, + }; use starkware_utils_testing::test_utils::cheat_caller_address_once; #[test] @@ -482,4 +485,24 @@ mod ReplaceabilityTests { // Should revert with FINALIZED as the implementation is already finalized. replaceable_dispatcher.replace_to(:implementation_data); } + + // ─── Replaceability role enforcement + // ────────────────────────────────────── + + #[test] + fn test_upgrade_governor_can_use_replaceability() { + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + let account = NOT_UPGRADE_GOVERNOR_ACCOUNT; + + // Grant upgrade governor via ICommonRoles. + cheat_caller_address_once(:contract_address, caller_address: GOVERNANCE_ADMIN); + ICommonRolesDispatcher { contract_address } + .grant_role(role: Role::UpgradeGovernor, :account); + + // Verify the granted account can call add_new_implementation. + cheat_caller_address_once(:contract_address, caller_address: account); + replaceable_dispatcher + .add_new_implementation(implementation_data: DUMMY_NONFINAL_IMPLEMENTATION_DATA()); + } } diff --git a/packages/utils/src/components/replaceability/test_utils.cairo b/packages/utils/src/components/replaceability/test_utils.cairo index 619bf922..bb244586 100644 --- a/packages/utils/src/components/replaceability/test_utils.cairo +++ b/packages/utils/src/components/replaceability/test_utils.cairo @@ -8,7 +8,9 @@ use starkware_utils::components::replaceability::interface::{ ImplementationReplaced, }; use starkware_utils::components::replaceability::mock::ReplaceabilityMock; -use starkware_utils::components::roles::interface::{IRolesDispatcher, IRolesDispatcherTrait}; +use starkware_utils::components::roles::interface::{ + ICommonRolesDispatcher, ICommonRolesDispatcherTrait, Role, +}; use starkware_utils_testing::event_test_utils::is_emitted; use starkware_utils_testing::test_utils::cheat_caller_address_once; @@ -68,9 +70,9 @@ pub(crate) fn get_upgrade_governor_account(contract_address: ContractAddress) -> } fn set_caller_as_upgrade_governor(contract_address: ContractAddress, caller: ContractAddress) { - let roles_dispatcher = IRolesDispatcher { contract_address: contract_address }; + let dispatcher = ICommonRolesDispatcher { contract_address }; cheat_caller_address_once(:contract_address, caller_address: Constants::GOVERNANCE_ADMIN); - roles_dispatcher.register_upgrade_governor(account: caller); + dispatcher.grant_role(Role::UpgradeGovernor, caller); } pub(crate) fn dummy_final_implementation_data_with_class_hash( diff --git a/packages/utils/src/components/roles/errors.cairo b/packages/utils/src/components/roles/errors.cairo index 4d0d1d71..8478316b 100644 --- a/packages/utils/src/components/roles/errors.cairo +++ b/packages/utils/src/components/roles/errors.cairo @@ -18,7 +18,7 @@ pub enum AccessErrors { ONLY_SECURITY_GOVERNOR, ONLY_MINTER, ONLY_SELF_CAN_RENOUNCE, - GOV_ADMIN_CANNOT_RENOUNCE, + ROLE_CANNOT_BE_RENOUNCED, MISSING_ROLE, LEGACY_ROLE_RECLAIM_DISABLED, } @@ -42,7 +42,7 @@ impl DescribableError of Describable { AccessErrors::ONLY_SECURITY_GOVERNOR => "ONLY_SECURITY_GOVERNOR", AccessErrors::ONLY_MINTER => "MINTER_ONLY", AccessErrors::ONLY_SELF_CAN_RENOUNCE => "ONLY_SELF_CAN_RENOUNCE", - AccessErrors::GOV_ADMIN_CANNOT_RENOUNCE => "GOV_ADMIN_CANNOT_SELF_REMOVE", + AccessErrors::ROLE_CANNOT_BE_RENOUNCED => "ROLE_CANNOT_BE_RENOUNCED", AccessErrors::MISSING_ROLE => "Caller is missing role", AccessErrors::LEGACY_ROLE_RECLAIM_DISABLED => "LEGACY_ROLE_RECLAIM_DISABLED", } diff --git a/packages/utils/src/components/roles/interface.cairo b/packages/utils/src/components/roles/interface.cairo index f48b4a1c..67d25003 100644 --- a/packages/utils/src/components/roles/interface.cairo +++ b/packages/utils/src/components/roles/interface.cairo @@ -88,20 +88,6 @@ pub trait IRoles { fn disable_legacy_role_reclaim(ref self: TContractState); } -#[starknet::interface] -pub trait IMinimalRoles { - fn is_governance_admin(self: @TContractState, account: ContractAddress) -> bool; - fn is_upgrade_governor(self: @TContractState, account: ContractAddress) -> bool; - fn is_upgrade_agent(self: @TContractState, account: ContractAddress) -> bool; - fn register_governance_admin(ref self: TContractState, account: ContractAddress); - fn remove_governance_admin(ref self: TContractState, account: ContractAddress); - fn register_upgrade_governor(ref self: TContractState, account: ContractAddress); - fn remove_upgrade_governor(ref self: TContractState, account: ContractAddress); - fn register_upgrade_agent(ref self: TContractState, account: ContractAddress); - fn remove_upgrade_agent(ref self: TContractState, account: ContractAddress); - fn renounce(ref self: TContractState, role: RoleId); -} - #[derive(Copy, Drop, PartialEq, starknet::Event)] pub(crate) struct AppGovernorAdded { pub added_account: ContractAddress, @@ -220,3 +206,105 @@ pub(crate) struct UpgradeAgentRemoved { pub removed_account: ContractAddress, pub removed_by: ContractAddress, } + +// ─── CommonRoles types +// ──────────────────────────────────────────────────────── + +#[derive(Copy, Drop, PartialEq, Serde)] +pub enum Role { + AppGovernor, + AppRoleAdmin, + GovernanceAdmin, + Operator, + TokenAdmin, + UpgradeAgent, + UpgradeGovernor, + SecurityAdmin, + SecurityAgent, + SecurityGovernor, +} + +pub impl RoleIntoRoleId of Into { + fn into(self: Role) -> RoleId { + match self { + Role::AppGovernor => APP_GOVERNOR, + Role::AppRoleAdmin => APP_ROLE_ADMIN, + Role::GovernanceAdmin => GOVERNANCE_ADMIN, + Role::Operator => OPERATOR, + Role::TokenAdmin => TOKEN_ADMIN, + Role::UpgradeAgent => UPGRADE_AGENT, + Role::UpgradeGovernor => UPGRADE_GOVERNOR, + Role::SecurityAdmin => SECURITY_ADMIN, + Role::SecurityAgent => SECURITY_AGENT, + Role::SecurityGovernor => SECURITY_GOVERNOR, + } + } +} + +pub fn is_renounceable(role: Role) -> bool { + match role { + Role::AppGovernor => true, + Role::AppRoleAdmin => true, + Role::GovernanceAdmin => false, + Role::Operator => true, + Role::TokenAdmin => true, + Role::UpgradeAgent => true, + Role::UpgradeGovernor => true, + Role::SecurityAdmin => false, + Role::SecurityAgent => true, + Role::SecurityGovernor => true, + } +} + +#[starknet::interface] +pub trait ICommonRoles { + fn grant_role(ref self: TState, role: Role, account: ContractAddress); + fn revoke_role(ref self: TState, role: Role, account: ContractAddress); + fn has_role(self: @TState, role: Role, account: ContractAddress) -> bool; + fn renounce(ref self: TState, role: Role); +} + +// ─── Category-scoped role interfaces +// ───────────────────────────────────────── + +#[starknet::interface] +pub trait ISecurityRoles { + fn is_security_admin(self: @TState, account: ContractAddress) -> bool; + fn register_security_admin(ref self: TState, account: ContractAddress); + fn remove_security_admin(ref self: TState, account: ContractAddress); + fn is_security_agent(self: @TState, account: ContractAddress) -> bool; + fn register_security_agent(ref self: TState, account: ContractAddress); + fn remove_security_agent(ref self: TState, account: ContractAddress); + fn is_security_governor(self: @TState, account: ContractAddress) -> bool; + fn register_security_governor(ref self: TState, account: ContractAddress); + fn remove_security_governor(ref self: TState, account: ContractAddress); +} + +#[starknet::interface] +pub trait IGovernanceRoles { + fn is_governance_admin(self: @TState, account: ContractAddress) -> bool; + fn register_governance_admin(ref self: TState, account: ContractAddress); + fn remove_governance_admin(ref self: TState, account: ContractAddress); + fn is_app_role_admin(self: @TState, account: ContractAddress) -> bool; + fn register_app_role_admin(ref self: TState, account: ContractAddress); + fn remove_app_role_admin(ref self: TState, account: ContractAddress); + fn is_upgrade_governor(self: @TState, account: ContractAddress) -> bool; + fn register_upgrade_governor(ref self: TState, account: ContractAddress); + fn remove_upgrade_governor(ref self: TState, account: ContractAddress); + fn is_upgrade_agent(self: @TState, account: ContractAddress) -> bool; + fn register_upgrade_agent(ref self: TState, account: ContractAddress); + fn remove_upgrade_agent(ref self: TState, account: ContractAddress); +} + +#[starknet::interface] +pub trait IAppRoles { + fn is_app_governor(self: @TState, account: ContractAddress) -> bool; + fn register_app_governor(ref self: TState, account: ContractAddress); + fn remove_app_governor(ref self: TState, account: ContractAddress); + fn is_operator(self: @TState, account: ContractAddress) -> bool; + fn register_operator(ref self: TState, account: ContractAddress); + fn remove_operator(ref self: TState, account: ContractAddress); + fn is_token_admin(self: @TState, account: ContractAddress) -> bool; + fn register_token_admin(ref self: TState, account: ContractAddress); + fn remove_token_admin(ref self: TState, account: ContractAddress); +} diff --git a/packages/utils/src/components/roles/mock_contract.cairo b/packages/utils/src/components/roles/mock_contract.cairo index ed7c9c6b..002d4467 100644 --- a/packages/utils/src/components/roles/mock_contract.cairo +++ b/packages/utils/src/components/roles/mock_contract.cairo @@ -3,10 +3,12 @@ pub mod MockContract { use openzeppelin::access::accesscontrol::AccessControlComponent; use openzeppelin::introspection::src5::SRC5Component; use starknet::ContractAddress; + use starkware_utils::components::common_roles::CommonRolesComponent; use starkware_utils::components::roles::RolesComponent; use starkware_utils::components::roles::RolesComponent::InternalTrait as RolesInternalTrait; component!(path: RolesComponent, storage: roles, event: RolesEvent); + component!(path: CommonRolesComponent, storage: common_roles, event: CommonRolesEvent); component!(path: AccessControlComponent, storage: access_control, event: AccessControlEvent); component!(path: SRC5Component, storage: src5, event: SRC5Event); @@ -15,6 +17,8 @@ pub mod MockContract { #[substorage(v0)] roles: RolesComponent::Storage, #[substorage(v0)] + common_roles: CommonRolesComponent::Storage, + #[substorage(v0)] access_control: AccessControlComponent::Storage, #[substorage(v0)] src5: SRC5Component::Storage, @@ -24,6 +28,7 @@ pub mod MockContract { #[derive(Drop, starknet::Event)] pub enum Event { RolesEvent: RolesComponent::Event, + CommonRolesEvent: CommonRolesComponent::Event, AccessControlEvent: AccessControlComponent::Event, SRC5Event: SRC5Component::Event, } diff --git a/packages/utils/src/components/roles/roles.cairo b/packages/utils/src/components/roles/roles.cairo index 87fe562e..a54b7bed 100644 --- a/packages/utils/src/components/roles/roles.cairo +++ b/packages/utils/src/components/roles/roles.cairo @@ -2,30 +2,30 @@ pub(crate) mod RolesComponent { use RolesInterface::{ APP_GOVERNOR, APP_ROLE_ADMIN, AppGovernorAdded, AppGovernorRemoved, AppRoleAdminAdded, - AppRoleAdminRemoved, GOVERNANCE_ADMIN, GovernanceAdminAdded, GovernanceAdminRemoved, IRoles, - OPERATOR, OperatorAdded, OperatorRemoved, RoleId, SECURITY_ADMIN, SECURITY_AGENT, - SECURITY_GOVERNOR, SecurityAdminAdded, SecurityAdminRemoved, SecurityAgentAdded, - SecurityAgentRemoved, SecurityGovernorAdded, SecurityGovernorRemoved, TOKEN_ADMIN, - TokenAdminAdded, TokenAdminRemoved, UPGRADE_AGENT, UPGRADE_GOVERNOR, UpgradeAgentAdded, - UpgradeAgentRemoved, UpgradeGovernorAdded, UpgradeGovernorRemoved, + AppRoleAdminRemoved, GOVERNANCE_ADMIN, GovernanceAdminAdded, GovernanceAdminRemoved, + IAppRoles, IGovernanceRoles, IRoles, ISecurityRoles, OPERATOR, OperatorAdded, + OperatorRemoved, RoleId, SECURITY_ADMIN, SECURITY_AGENT, SECURITY_GOVERNOR, + SecurityAdminAdded, SecurityAdminRemoved, SecurityAgentAdded, SecurityAgentRemoved, + SecurityGovernorAdded, SecurityGovernorRemoved, TOKEN_ADMIN, TokenAdminAdded, + TokenAdminRemoved, UPGRADE_AGENT, UPGRADE_GOVERNOR, UpgradeAgentAdded, UpgradeAgentRemoved, + UpgradeGovernorAdded, UpgradeGovernorRemoved, }; use core::num::traits::Zero; + use openzeppelin::access::accesscontrol::AccessControlComponent; + use openzeppelin::access::accesscontrol::AccessControlComponent::{ + AccessControlImpl, InternalTrait as AccessInternalTrait, + }; + use openzeppelin::introspection::src5::SRC5Component; use starknet::storage::{ StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, StoragePointerWriteAccess, }; use starknet::{ContractAddress, get_caller_address}; + use starkware_utils::components::common_roles::CommonRolesComponent; + use starkware_utils::components::common_roles::CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; use starkware_utils::components::roles::errors::AccessErrors; use starkware_utils::components::roles::interface as RolesInterface; - const ROLE_ADMIN_PAIRS: [(RoleId, RoleId); 10] = [ - (APP_GOVERNOR, APP_ROLE_ADMIN), (APP_ROLE_ADMIN, GOVERNANCE_ADMIN), - (GOVERNANCE_ADMIN, GOVERNANCE_ADMIN), (OPERATOR, APP_ROLE_ADMIN), - (TOKEN_ADMIN, APP_ROLE_ADMIN), (UPGRADE_AGENT, APP_ROLE_ADMIN), - (UPGRADE_GOVERNOR, GOVERNANCE_ADMIN), (SECURITY_ADMIN, SECURITY_ADMIN), - (SECURITY_AGENT, SECURITY_ADMIN), (SECURITY_GOVERNOR, SECURITY_ADMIN), - ]; - #[storage] pub struct Storage { // LEGACY: This is the old storage for role members. @@ -59,12 +59,6 @@ pub(crate) mod RolesComponent { UpgradeAgentAdded: UpgradeAgentAdded, UpgradeAgentRemoved: UpgradeAgentRemoved, } - use openzeppelin::access::accesscontrol::AccessControlComponent; - use openzeppelin::access::accesscontrol::AccessControlComponent::{ - AccessControlImpl, InternalTrait as AccessInternalTrait, - }; - use openzeppelin::interfaces::access::accesscontrol::IAccessControl; - use openzeppelin::introspection::src5::SRC5Component; #[embeddable_as(RolesImpl)] pub impl Roles< @@ -72,6 +66,7 @@ pub(crate) mod RolesComponent { +HasComponent, +Drop, impl Access: AccessControlComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, +SRC5Component::HasComponent, > of IRoles> { fn is_app_governor( @@ -83,60 +78,51 @@ pub(crate) mod RolesComponent { fn is_app_role_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: APP_ROLE_ADMIN, :account) + get_dep_component!(self, Access).has_role(role: APP_ROLE_ADMIN, :account) } fn is_governance_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: GOVERNANCE_ADMIN, :account) + get_dep_component!(self, Access).has_role(role: GOVERNANCE_ADMIN, :account) } fn is_operator(self: @ComponentState, account: ContractAddress) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: OPERATOR, :account) + get_dep_component!(self, Access).has_role(role: OPERATOR, :account) } fn is_security_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: SECURITY_ADMIN, :account) + get_dep_component!(self, Access).has_role(role: SECURITY_ADMIN, :account) } fn is_security_agent( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: SECURITY_AGENT, :account) + get_dep_component!(self, Access).has_role(role: SECURITY_AGENT, :account) } fn is_security_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: SECURITY_GOVERNOR, :account) + get_dep_component!(self, Access).has_role(role: SECURITY_GOVERNOR, :account) } fn is_token_admin(self: @ComponentState, account: ContractAddress) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: TOKEN_ADMIN, :account) + get_dep_component!(self, Access).has_role(role: TOKEN_ADMIN, :account) } fn is_upgrade_agent( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: UPGRADE_AGENT, :account) + get_dep_component!(self, Access).has_role(role: UPGRADE_AGENT, :account) } fn is_upgrade_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: UPGRADE_GOVERNOR, :account) + get_dep_component!(self, Access).has_role(role: UPGRADE_GOVERNOR, :account) } fn register_app_governor( @@ -242,9 +228,7 @@ pub(crate) mod RolesComponent { ref self: ComponentState, account: ContractAddress, ) { let caller_address = get_caller_address(); - - // Governance admin musn't remove itself, to avoid losing governance. - assert!(account != caller_address, "{}", AccessErrors::GOV_ADMIN_CANNOT_RENOUNCE); + assert!(account != caller_address, "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); let event = Event::GovernanceAdminRemoved( GovernanceAdminRemoved { removed_account: account, removed_by: caller_address }, ); @@ -320,7 +304,11 @@ pub(crate) mod RolesComponent { } fn renounce(ref self: ComponentState, role: RoleId) { - assert!(role != GOVERNANCE_ADMIN, "{}", AccessErrors::GOV_ADMIN_CANNOT_RENOUNCE); + assert!( + role != GOVERNANCE_ADMIN && role != SECURITY_ADMIN, + "{}", + AccessErrors::ROLE_CANNOT_BE_RENOUNCED, + ); let mut access_comp = get_dep_component_mut!(ref self, Access); access_comp.renounce_role(:role, account: get_caller_address()) } @@ -328,8 +316,8 @@ pub(crate) mod RolesComponent { fn reclaim_legacy_roles(ref self: ComponentState) { self.assert_role_reclaim_enabled(); self._reclaim_legacy_roles_for_account(get_caller_address()); - let mut access_comp = get_dep_component_mut!(ref self, Access); - RolesInternalImpl::_set_role_admins(ref access_comp); + let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); + common_roles.ensure_role_admins(); } fn reclaim_legacy_roles_for_accounts( @@ -348,6 +336,273 @@ pub(crate) mod RolesComponent { } } + // ─── Category-scoped role impls + // ─────────────────────────────────────────── + + #[embeddable_as(SecurityRolesImpl)] + pub impl SecurityRoles< + TContractState, + +HasComponent, + +Drop, + impl Access: AccessControlComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, + +SRC5Component::HasComponent, + > of ISecurityRoles> { + fn is_security_admin( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: SECURITY_ADMIN, :account) + } + + fn register_security_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::SecurityAdminAdded( + SecurityAdminAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: SECURITY_ADMIN, :account, :event); + } + + fn remove_security_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::SecurityAdminRemoved( + SecurityAdminRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: SECURITY_ADMIN, :account, :event); + } + + fn is_security_agent( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: SECURITY_AGENT, :account) + } + + fn register_security_agent( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::SecurityAgentAdded( + SecurityAgentAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: SECURITY_AGENT, :account, :event); + } + + fn remove_security_agent( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::SecurityAgentRemoved( + SecurityAgentRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: SECURITY_AGENT, :account, :event); + } + + fn is_security_governor( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: SECURITY_GOVERNOR, :account) + } + + fn register_security_governor( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::SecurityGovernorAdded( + SecurityGovernorAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: SECURITY_GOVERNOR, :account, :event); + } + + fn remove_security_governor( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::SecurityGovernorRemoved( + SecurityGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ); + self._revoke_role_and_emit(role: SECURITY_GOVERNOR, :account, :event); + } + } + + #[embeddable_as(GovernanceRolesImpl)] + pub impl GovernanceRoles< + TContractState, + +HasComponent, + +Drop, + impl Access: AccessControlComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, + +SRC5Component::HasComponent, + > of IGovernanceRoles> { + fn is_governance_admin( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: GOVERNANCE_ADMIN, :account) + } + + fn register_governance_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::GovernanceAdminAdded( + GovernanceAdminAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: GOVERNANCE_ADMIN, :account, :event); + } + + fn remove_governance_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let caller_address = get_caller_address(); + assert!(account != caller_address, "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); + let event = Event::GovernanceAdminRemoved( + GovernanceAdminRemoved { removed_account: account, removed_by: caller_address }, + ); + self._revoke_role_and_emit(role: GOVERNANCE_ADMIN, :account, :event); + } + + fn is_app_role_admin( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: APP_ROLE_ADMIN, :account) + } + + fn register_app_role_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::AppRoleAdminAdded( + AppRoleAdminAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: APP_ROLE_ADMIN, :account, :event); + } + + fn remove_app_role_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::AppRoleAdminRemoved( + AppRoleAdminRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: APP_ROLE_ADMIN, :account, :event); + } + + fn is_upgrade_governor( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: UPGRADE_GOVERNOR, :account) + } + + fn register_upgrade_governor( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::UpgradeGovernorAdded( + UpgradeGovernorAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: UPGRADE_GOVERNOR, :account, :event); + } + + fn remove_upgrade_governor( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::UpgradeGovernorRemoved( + UpgradeGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ); + self._revoke_role_and_emit(role: UPGRADE_GOVERNOR, :account, :event); + } + + fn is_upgrade_agent( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: UPGRADE_AGENT, :account) + } + + fn register_upgrade_agent( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::UpgradeAgentAdded( + UpgradeAgentAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: UPGRADE_AGENT, :account, :event); + } + + fn remove_upgrade_agent( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::UpgradeAgentRemoved( + UpgradeAgentRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: UPGRADE_AGENT, :account, :event); + } + } + + #[embeddable_as(AppRolesImpl)] + pub impl AppRoles< + TContractState, + +HasComponent, + +Drop, + impl Access: AccessControlComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, + +SRC5Component::HasComponent, + > of IAppRoles> { + fn is_app_governor( + self: @ComponentState, account: ContractAddress, + ) -> bool { + get_dep_component!(self, Access).has_role(role: APP_GOVERNOR, :account) + } + + fn register_app_governor( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::AppGovernorAdded( + AppGovernorAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: APP_GOVERNOR, :account, :event); + } + + fn remove_app_governor(ref self: ComponentState, account: ContractAddress) { + let event = Event::AppGovernorRemoved( + AppGovernorRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: APP_GOVERNOR, :account, :event); + } + + fn is_operator(self: @ComponentState, account: ContractAddress) -> bool { + get_dep_component!(self, Access).has_role(role: OPERATOR, :account) + } + + fn register_operator(ref self: ComponentState, account: ContractAddress) { + let event = Event::OperatorAdded( + OperatorAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: OPERATOR, :account, :event); + } + + fn remove_operator(ref self: ComponentState, account: ContractAddress) { + let event = Event::OperatorRemoved( + OperatorRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: OPERATOR, :account, :event); + } + + fn is_token_admin(self: @ComponentState, account: ContractAddress) -> bool { + get_dep_component!(self, Access).has_role(role: TOKEN_ADMIN, :account) + } + + fn register_token_admin( + ref self: ComponentState, account: ContractAddress, + ) { + let event = Event::TokenAdminAdded( + TokenAdminAdded { added_account: account, added_by: get_caller_address() }, + ); + self._grant_role_and_emit(role: TOKEN_ADMIN, :account, :event); + } + + fn remove_token_admin(ref self: ComponentState, account: ContractAddress) { + let event = Event::TokenAdminRemoved( + TokenAdminRemoved { removed_account: account, removed_by: get_caller_address() }, + ); + self._revoke_role_and_emit(role: TOKEN_ADMIN, :account, :event); + } + } + #[generate_trait] pub impl ClaimRoleImpl< TContractState, @@ -381,7 +636,7 @@ pub(crate) mod RolesComponent { fn _reclaim_legacy_roles_for_account( ref self: ComponentState, account: ContractAddress, ) { - for (role, _) in ROLE_ADMIN_PAIRS.span() { + for (role, _) in CommonRolesComponent::ROLE_ADMIN_PAIRS.span() { self._reclaim_role(role: *role, :account); } } @@ -393,9 +648,9 @@ pub(crate) mod RolesComponent { +HasComponent, +Drop, impl Access: AccessControlComponent::HasComponent, + impl CommonRoles: CommonRolesComponent::HasComponent, +SRC5Component::HasComponent, > of InternalTrait { - // TODO - change the fn name to _grant_role when we can have modularity. fn _grant_role_and_emit( ref self: ComponentState, role: RoleId, @@ -410,7 +665,6 @@ pub(crate) mod RolesComponent { } } - // TODO - change the fn name to _revoke_role when we can have modularity. fn _revoke_role_and_emit( ref self: ComponentState, role: RoleId, @@ -435,86 +689,42 @@ pub(crate) mod RolesComponent { // WARNING // The following internal method is unprotected and should only be used from the containing // contract's constructor (or, in context of tests, from the setup method). - // It should be called after the initialization of the access_control component. fn initialize(ref self: ComponentState, governance_admin: ContractAddress) { - let mut access_comp = get_dep_component_mut!(ref self, Access); - let un_initialized = access_comp.get_role_admin(role: GOVERNANCE_ADMIN).is_zero(); - assert!(un_initialized, "{}", AccessErrors::ALREADY_INITIALIZED); - access_comp.initializer(); - assert!(governance_admin.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS_GOV_ADMIN); - access_comp._grant_role(role: GOVERNANCE_ADMIN, account: governance_admin); - access_comp._grant_role(role: SECURITY_ADMIN, account: governance_admin); + let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); + common_roles.initialize(:governance_admin); self.legacy_role_reclaim_disabled.write(true); - Self::_set_role_admins(ref access_comp); - } - - // Recovers role_admin heirarcy. - // Set role_admin only if not set already. - fn _set_role_admins( - ref access_comp: AccessControlComponent::ComponentState, - ) { - for (role, admin_role) in ROLE_ADMIN_PAIRS.span() { - if access_comp.get_role_admin(role: *role).is_zero() { - access_comp.set_role_admin(role: *role, admin_role: *admin_role); - } - } } fn only_app_governor(self: @ComponentState) { - assert!( - self.is_app_governor(get_caller_address()), "{}", AccessErrors::ONLY_APP_GOVERNOR, - ); + get_dep_component!(self, CommonRoles).only_app_governor(); } fn only_operator(self: @ComponentState) { - assert!(self.is_operator(get_caller_address()), "{}", AccessErrors::ONLY_OPERATOR); + get_dep_component!(self, CommonRoles).only_operator(); } fn only_token_admin(self: @ComponentState) { - assert!( - self.is_token_admin(get_caller_address()), "{}", AccessErrors::ONLY_TOKEN_ADMIN, - ); + get_dep_component!(self, CommonRoles).only_token_admin(); } fn only_upgrade_governor(self: @ComponentState) { - assert!( - self.is_upgrade_governor(get_caller_address()), - "{}", - AccessErrors::ONLY_UPGRADE_GOVERNOR, - ); + get_dep_component!(self, CommonRoles).only_upgrade_governor(); } fn only_upgrader(self: @ComponentState) { - assert!( - self.is_upgrade_agent(get_caller_address()) - || self.is_upgrade_governor(get_caller_address()), - "{}", - AccessErrors::ONLY_UPGRADER, - ); + get_dep_component!(self, CommonRoles).only_upgrader(); } fn only_security_admin(self: @ComponentState) { - assert!( - self.is_security_admin(get_caller_address()), - "{}", - AccessErrors::ONLY_SECURITY_ADMIN, - ); + get_dep_component!(self, CommonRoles).only_security_admin(); } fn only_security_agent(self: @ComponentState) { - assert!( - self.is_security_agent(get_caller_address()), - "{}", - AccessErrors::ONLY_SECURITY_AGENT, - ); + get_dep_component!(self, CommonRoles).only_security_agent(); } fn only_security_governor(self: @ComponentState) { - assert!( - self.is_security_governor(get_caller_address()), - "{}", - AccessErrors::ONLY_SECURITY_GOVERNOR, - ); + get_dep_component!(self, CommonRoles).only_security_governor(); } } } diff --git a/packages/utils/src/components/roles/test.cairo b/packages/utils/src/components/roles/test.cairo index b2a1036f..e835e291 100644 --- a/packages/utils/src/components/roles/test.cairo +++ b/packages/utils/src/components/roles/test.cairo @@ -1,6 +1,8 @@ use core::num::traits::zero::Zero; use interface::{ - IRolesDispatcher, IRolesDispatcherTrait, IRolesSafeDispatcher, IRolesSafeDispatcherTrait, + IAppRolesDispatcher, IAppRolesDispatcherTrait, IGovernanceRolesDispatcher, + IGovernanceRolesDispatcherTrait, IRolesDispatcher, IRolesDispatcherTrait, IRolesSafeDispatcher, + IRolesSafeDispatcherTrait, ISecurityRolesDispatcher, ISecurityRolesDispatcherTrait, }; use openzeppelin::access::accesscontrol::AccessControlComponent::Errors as OZAccessErrors; use roles::{event_test_utils, interface}; @@ -923,7 +925,7 @@ fn test_renounce() { cheat_caller_address_once(:contract_address, caller_address: governance_admin); let result = roles_safe_dispatcher.renounce(role: interface::GOVERNANCE_ADMIN); assert_panic_with_error( - :result, expected_error: AccessErrors::GOV_ADMIN_CANNOT_RENOUNCE.describe(), + :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), ); // Renounce role without being registered. @@ -950,3 +952,184 @@ fn test_renounce() { actual: events.len(), expected: 1, message: "test_remove_security_admin second", ); } + +// ─── Category interface parity tests +// ───────────────────────────────────────── +// +// These tests verify that dispatching via a category interface (e.g. IUpgradeRoles) +// against a contract that embeds RolesComponent produces identical results to +// dispatching via IRoles — same state changes, same named events. + +// ── IUpgradeRoles ── + +#[test] +fn test_upgrade_roles_parity_register_via_category() { + let contract_address = test_utils::deploy_mock_contract(); + let roles = IRolesDispatcher { contract_address }; + let upgrade_roles = IGovernanceRolesDispatcher { contract_address }; + let governance_admin = constants::INITIAL_ROOT_ADMIN; + let account = constants::UPGRADE_GOVERNOR; + + assert!(!roles.is_upgrade_governor(:account)); + assert!(!upgrade_roles.is_upgrade_governor(:account)); + + let mut spy = snforge_std::spy_events(); + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + upgrade_roles.register_upgrade_governor(:account); + + assert!(roles.is_upgrade_governor(:account)); + assert!(upgrade_roles.is_upgrade_governor(:account)); + let events = spy.get_events().emitted_by(:contract_address).events; + assert_number_of_events( + actual: events.len(), expected: 2, message: "upgrade_roles parity register", + ); + event_test_utils::assert_upgrade_governor_added_event( + events[1], added_account: account, added_by: governance_admin, + ); +} + +#[test] +fn test_upgrade_roles_parity_remove_via_category() { + let contract_address = test_utils::deploy_mock_contract(); + let roles = IRolesDispatcher { contract_address }; + let upgrade_roles = IGovernanceRolesDispatcher { contract_address }; + let governance_admin = constants::INITIAL_ROOT_ADMIN; + let account = constants::UPGRADE_GOVERNOR; + + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + roles.register_upgrade_governor(:account); + + let mut spy = snforge_std::spy_events(); + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + upgrade_roles.remove_upgrade_governor(:account); + + assert!(!roles.is_upgrade_governor(:account)); + assert!(!upgrade_roles.is_upgrade_governor(:account)); + let events = spy.get_events().emitted_by(:contract_address).events; + assert_number_of_events( + actual: events.len(), expected: 2, message: "upgrade_roles parity remove", + ); + event_test_utils::assert_upgrade_governor_removed_event( + events[1], removed_account: account, removed_by: governance_admin, + ); +} + +#[test] +fn test_upgrade_agent_parity_register_via_category() { + let contract_address = test_utils::deploy_mock_contract(); + let governance_admin = constants::INITIAL_ROOT_ADMIN; + let app_role_admin = constants::APP_ROLE_ADMIN; + let account = constants::UPGRADE_AGENT; + + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + IRolesDispatcher { contract_address }.register_app_role_admin(account: app_role_admin); + + let roles = IRolesDispatcher { contract_address }; + let upgrade_roles = IGovernanceRolesDispatcher { contract_address }; + + assert!(!roles.is_upgrade_agent(:account)); + assert!(!upgrade_roles.is_upgrade_agent(:account)); + + let mut spy = snforge_std::spy_events(); + cheat_caller_address_once(:contract_address, caller_address: app_role_admin); + upgrade_roles.register_upgrade_agent(:account); + + assert!(roles.is_upgrade_agent(:account)); + assert!(upgrade_roles.is_upgrade_agent(:account)); + let events = spy.get_events().emitted_by(:contract_address).events; + assert_number_of_events( + actual: events.len(), expected: 2, message: "upgrade_agent parity register", + ); + event_test_utils::assert_upgrade_agent_added_event( + events[1], added_account: account, added_by: app_role_admin, + ); +} + +// ── ISecurityRoles ── + +#[test] +fn test_security_roles_parity_register_via_category() { + let contract_address = test_utils::deploy_mock_contract(); + let roles = IRolesDispatcher { contract_address }; + let security_roles = ISecurityRolesDispatcher { contract_address }; + let governance_admin = constants::INITIAL_ROOT_ADMIN; + let account = constants::SECURITY_AGENT; + + assert!(!roles.is_security_agent(:account)); + assert!(!security_roles.is_security_agent(:account)); + + let mut spy = snforge_std::spy_events(); + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + security_roles.register_security_agent(:account); + + assert!(roles.is_security_agent(:account)); + assert!(security_roles.is_security_agent(:account)); + let events = spy.get_events().emitted_by(:contract_address).events; + assert_number_of_events( + actual: events.len(), expected: 2, message: "security_roles parity register", + ); + event_test_utils::assert_security_agent_added_event( + events[1], added_account: account, added_by: governance_admin, + ); +} + +// ── IGovernanceRoles ── + +#[test] +fn test_governance_roles_parity_register_via_category() { + let contract_address = test_utils::deploy_mock_contract(); + let roles = IRolesDispatcher { contract_address }; + let gov_roles = IGovernanceRolesDispatcher { contract_address }; + let governance_admin = constants::INITIAL_ROOT_ADMIN; + let account = constants::APP_ROLE_ADMIN; + + assert!(!roles.is_app_role_admin(:account)); + assert!(!gov_roles.is_app_role_admin(:account)); + + let mut spy = snforge_std::spy_events(); + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + gov_roles.register_app_role_admin(:account); + + assert!(roles.is_app_role_admin(:account)); + assert!(gov_roles.is_app_role_admin(:account)); + let events = spy.get_events().emitted_by(:contract_address).events; + assert_number_of_events( + actual: events.len(), expected: 2, message: "governance_roles parity register", + ); + event_test_utils::assert_app_role_admin_added_event( + events[1], added_account: account, added_by: governance_admin, + ); +} + +// ── IAppRoles ── + +#[test] +fn test_app_roles_parity_register_via_category() { + let contract_address = test_utils::deploy_mock_contract(); + let governance_admin = constants::INITIAL_ROOT_ADMIN; + let app_role_admin = constants::APP_ROLE_ADMIN; + let account = constants::APP_GOVERNOR; + + cheat_caller_address_once(:contract_address, caller_address: governance_admin); + IRolesDispatcher { contract_address }.register_app_role_admin(account: app_role_admin); + + let roles = IRolesDispatcher { contract_address }; + let app_roles = IAppRolesDispatcher { contract_address }; + + assert!(!roles.is_app_governor(:account)); + assert!(!app_roles.is_app_governor(:account)); + + let mut spy = snforge_std::spy_events(); + cheat_caller_address_once(:contract_address, caller_address: app_role_admin); + app_roles.register_app_governor(:account); + + assert!(roles.is_app_governor(:account)); + assert!(app_roles.is_app_governor(:account)); + let events = spy.get_events().emitted_by(:contract_address).events; + assert_number_of_events( + actual: events.len(), expected: 2, message: "app_roles parity register", + ); + event_test_utils::assert_app_governor_added_event( + events[1], added_account: account, added_by: app_role_admin, + ); +} From 750c420518b811ce7d3bf57f97019b6864397540 Mon Sep 17 00:00:00 2001 From: Remo Date: Thu, 19 Mar 2026 02:56:57 +0200 Subject: [PATCH 2/3] review fixes and completions --- .../common_roles/common_roles.cairo | 115 ++- .../common_roles/mock_contract.cairo | 69 ++ .../utils/src/components/common_roles/spec.md | 20 +- .../src/components/common_roles/test.cairo | 230 +++++- .../src/components/roles/interface.cairo | 57 +- .../src/components/roles/mock_contract.cairo | 2 + .../utils/src/components/roles/roles.cairo | 741 ++++++++++-------- .../utils/src/components/roles/test.cairo | 35 +- 8 files changed, 907 insertions(+), 362 deletions(-) diff --git a/packages/utils/src/components/common_roles/common_roles.cairo b/packages/utils/src/components/common_roles/common_roles.cairo index 258ac6ad..6e03f584 100644 --- a/packages/utils/src/components/common_roles/common_roles.cairo +++ b/packages/utils/src/components/common_roles/common_roles.cairo @@ -6,6 +6,10 @@ pub(crate) mod CommonRolesComponent { AccessControlImpl, InternalTrait as AccessInternalTrait, }; use openzeppelin::introspection::src5::SRC5Component; + use starknet::storage::{ + StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, + StoragePointerWriteAccess, + }; use starknet::{ContractAddress, get_caller_address}; use starkware_utils::components::roles::errors::AccessErrors; use starkware_utils::components::roles::interface::{ @@ -23,7 +27,13 @@ pub(crate) mod CommonRolesComponent { ]; #[storage] - pub struct Storage {} + pub struct Storage { + // LEGACY: Old role-membership map from before OZ AccessControl integration. + // Kept to allow one-time reclaim migration. Read-only after initialize(). + role_members: starknet::storage::Map<(RoleId, ContractAddress), bool>, + // Once set, legacy reclaim is permanently disabled. + legacy_role_reclaim_disabled: bool, + } #[event] #[derive(Drop, starknet::Event)] @@ -40,26 +50,19 @@ pub(crate) mod CommonRolesComponent { fn grant_role( ref self: ComponentState, role: Role, account: ContractAddress, ) { - let role_id: RoleId = role.into(); - assert!(account.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS); - let mut access_comp = get_dep_component_mut!(ref self, Access); - access_comp.grant_role(role: role_id, :account); + InternalTrait::grant_role(ref self, :role, :account); } fn revoke_role( ref self: ComponentState, role: Role, account: ContractAddress, ) { - let role_id: RoleId = role.into(); - let mut access_comp = get_dep_component_mut!(ref self, Access); - access_comp.revoke_role(role: role_id, :account); + InternalTrait::revoke_role(ref self, :role, :account); } fn has_role( self: @ComponentState, role: Role, account: ContractAddress, ) -> bool { - let role_id: RoleId = role.into(); - let access_comp = get_dep_component!(self, Access); - access_comp.has_role(role: role_id, :account) + InternalTrait::has_role(self, role, account) } fn renounce(ref self: ComponentState, role: Role) { @@ -68,6 +71,27 @@ pub(crate) mod CommonRolesComponent { let mut access_comp = get_dep_component_mut!(ref self, Access); access_comp.renounce_role(role: role_id, account: get_caller_address()); } + + fn reclaim_legacy_roles(ref self: ComponentState) { + InternalTrait::assert_role_reclaim_enabled(@self); + InternalTrait::_reclaim_legacy_roles_for_account(ref self, get_caller_address()); + InternalTrait::ensure_role_admins(ref self); + } + + fn reclaim_legacy_roles_for_accounts( + ref self: ComponentState, accounts: Span, + ) { + InternalTrait::only_security_governor(@self); + InternalTrait::assert_role_reclaim_enabled(@self); + for account in accounts { + InternalTrait::_reclaim_legacy_roles_for_account(ref self, *account); + }; + } + + fn disable_legacy_role_reclaim(ref self: ComponentState) { + InternalTrait::only_upgrade_governor(@self); + self.legacy_role_reclaim_disabled.write(true); + } } #[generate_trait] @@ -82,13 +106,15 @@ pub(crate) mod CommonRolesComponent { // Unprotected — call only from constructor (or test setup). fn initialize(ref self: ComponentState, governance_admin: ContractAddress) { let mut access_comp = get_dep_component_mut!(ref self, Access); - let un_initialized = access_comp.get_role_admin(role: GOVERNANCE_ADMIN).is_zero(); - assert!(un_initialized, "{}", AccessErrors::ALREADY_INITIALIZED); + let uninitialized = access_comp.get_role_admin(role: GOVERNANCE_ADMIN).is_zero(); + assert!(uninitialized, "{}", AccessErrors::ALREADY_INITIALIZED); access_comp.initializer(); assert!(governance_admin.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS_GOV_ADMIN); access_comp._grant_role(role: GOVERNANCE_ADMIN, account: governance_admin); access_comp._grant_role(role: SECURITY_ADMIN, account: governance_admin); Self::_set_role_admins(ref access_comp); + // Fresh contracts have no legacy storage — disable reclaim immediately. + self.legacy_role_reclaim_disabled.write(true); } fn _set_role_admins( @@ -110,18 +136,37 @@ pub(crate) mod CommonRolesComponent { Self::_set_role_admins(ref access_comp); } - fn _grant_role( - ref self: ComponentState, role: RoleId, account: ContractAddress, + fn has_role( + self: @ComponentState, role: Role, account: ContractAddress, + ) -> bool { + let role_id: RoleId = role.into(); + let access_comp = get_dep_component!(self, Access); + access_comp.has_role(role: role_id, :account) + } + + /// Grants `role` to `account`. Enforces caller is the role's admin (via OZ grant_role). + fn grant_role( + ref self: ComponentState, role: Role, account: ContractAddress, ) { + assert!(account.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS); + let role: RoleId = role.into(); let mut access_comp = get_dep_component_mut!(ref self, Access); - access_comp._grant_role(:role, :account); + access_comp.grant_role(:role, :account); } - fn _revoke_role( - ref self: ComponentState, role: RoleId, account: ContractAddress, + /// Revokes `role` from `account`. Blocks self-revoke of non-renounceable roles. + /// Enforces caller is the role's admin (via OZ revoke_role). + fn revoke_role( + ref self: ComponentState, role: Role, account: ContractAddress, ) { + assert!( + get_caller_address() != account || is_renounceable(role), + "{}", + AccessErrors::ROLE_CANNOT_BE_RENOUNCED, + ); + let role: RoleId = role.into(); let mut access_comp = get_dep_component_mut!(ref self, Access); - access_comp._revoke_role(:role, :account); + access_comp.revoke_role(:role, :account); } fn only_role(self: @ComponentState, role: Role) { @@ -207,5 +252,37 @@ pub(crate) mod CommonRolesComponent { AccessErrors::ONLY_SECURITY_GOVERNOR, ); } + + fn assert_role_reclaim_enabled(self: @ComponentState) { + assert!( + !self.legacy_role_reclaim_disabled.read(), + "{}", + AccessErrors::LEGACY_ROLE_RECLAIM_DISABLED, + ); + } + + fn _has_legacy_role( + self: @ComponentState, account: ContractAddress, role: RoleId, + ) -> bool { + self.role_members.read((role, account)) + } + + fn _reclaim_role( + ref self: ComponentState, role: RoleId, account: ContractAddress, + ) { + if self._has_legacy_role(account, role) { + self.role_members.write((role, account), false); + let mut access_comp = get_dep_component_mut!(ref self, Access); + access_comp._grant_role(:role, :account); + } + } + + fn _reclaim_legacy_roles_for_account( + ref self: ComponentState, account: ContractAddress, + ) { + for (role, _) in ROLE_ADMIN_PAIRS.span() { + self._reclaim_role(role: *role, :account); + } + } } } diff --git a/packages/utils/src/components/common_roles/mock_contract.cairo b/packages/utils/src/components/common_roles/mock_contract.cairo index 594357d6..626a0936 100644 --- a/packages/utils/src/components/common_roles/mock_contract.cairo +++ b/packages/utils/src/components/common_roles/mock_contract.cairo @@ -1,3 +1,72 @@ +/// Backdoor for setting legacy role membership in tests. +#[starknet::interface] +pub(crate) trait ILegacySetup { + fn set_legacy_role(ref self: TState, role_id: felt252, account: starknet::ContractAddress); +} + +/// Simulates a contract that was deployed with the old role storage and then upgraded to +/// CommonRolesComponent without calling `initialize` (so `legacy_role_reclaim_disabled` +/// remains false, and legacy role data can be reclaimed). +#[starknet::contract] +pub(crate) mod LegacyCommonRolesMock { + use openzeppelin::access::accesscontrol::AccessControlComponent; + use openzeppelin::access::accesscontrol::AccessControlComponent::InternalTrait as AccessInternalTrait; + use openzeppelin::introspection::src5::SRC5Component; + use starknet::ContractAddress; + use starknet::storage::StorageMapWriteAccess; + use starkware_utils::components::common_roles::CommonRolesComponent; + use starkware_utils::components::common_roles::CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; + use starkware_utils::components::roles::interface::{GOVERNANCE_ADMIN, SECURITY_ADMIN}; + use super::ILegacySetup; + + component!(path: CommonRolesComponent, storage: common_roles, event: CommonRolesEvent); + component!(path: AccessControlComponent, storage: accesscontrol, event: AccessControlEvent); + component!(path: SRC5Component, storage: src5, event: SRC5Event); + + #[storage] + #[allow(starknet::colliding_storage_paths)] + struct Storage { + #[substorage(v0)] + common_roles: CommonRolesComponent::Storage, + #[substorage(v0)] + accesscontrol: AccessControlComponent::Storage, + #[substorage(v0)] + src5: SRC5Component::Storage, + // Collides with CommonRolesComponent::role_members via substorage(v0) flat layout. + // Used only for test setup — allows writing legacy role entries without pub visibility. + #[rename("role_members")] + legacy_role_members: starknet::storage::Map<(felt252, ContractAddress), bool>, + } + + #[event] + #[derive(Drop, starknet::Event)] + pub(crate) enum Event { + CommonRolesEvent: CommonRolesComponent::Event, + AccessControlEvent: AccessControlComponent::Event, + SRC5Event: SRC5Component::Event, + } + + #[constructor] + fn constructor(ref self: ContractState, governance_admin: ContractAddress) { + // Migrate path: wire roles without initialize(), leaving + // legacy_role_reclaim_disabled=false. + self.accesscontrol.initializer(); + self.accesscontrol._grant_role(role: GOVERNANCE_ADMIN, account: governance_admin); + self.accesscontrol._grant_role(role: SECURITY_ADMIN, account: governance_admin); + self.common_roles.ensure_role_admins(); + } + + #[abi(embed_v0)] + impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; + + #[abi(embed_v0)] + impl LegacySetupImpl of ILegacySetup { + fn set_legacy_role(ref self: ContractState, role_id: felt252, account: ContractAddress) { + self.legacy_role_members.write((role_id, account), true); + } + } +} + #[starknet::interface] pub(crate) trait IGuardTest { fn assert_only_role(self: @TState, role: starkware_utils::components::roles::interface::Role); diff --git a/packages/utils/src/components/common_roles/spec.md b/packages/utils/src/components/common_roles/spec.md index 712bc385..f49d81c7 100644 --- a/packages/utils/src/components/common_roles/spec.md +++ b/packages/utils/src/components/common_roles/spec.md @@ -4,8 +4,8 @@ The role system is split into two layers: -- **`CommonRolesComponent`** — lean infrastructure. No own storage, empty event enum. Owns the role constants, the `Role` enum, role admin configuration, role guards (`only_X`), and a generic 4-method ABI (`ICommonRoles`). -- **`RolesComponent`** — full-featured layer. Delegates all role state to `CommonRolesComponent` internally. Adds named role events (20 variants), category-scoped embeddable impls (`IGovernanceRoles`, `ISecurityRoles`, `IAppRoles`), the fat `IRoles` interface (~30 EPs), and legacy role reclaim support. +- **`CommonRolesComponent`** — lean infrastructure. Owns the role constants, the `Role` enum, role admin configuration, role guards (`only_X`), a 7-method ABI (`ICommonRoles`: `grant_role`, `revoke_role`, `has_role`, `renounce`, and the three legacy reclaim entry points), and the legacy reclaim storage (`role_members` map + `legacy_role_reclaim_disabled` flag). +- **`RolesComponent`** — full-featured layer. Delegates all role state to `CommonRolesComponent` internally. Adds named role events (20 variants) and category-scoped embeddable implementations (`IGovernanceRoles`, `ISecurityRoles`, `IAppRoles`), and the fat `IRoles` interface (~30 EPs). The design lets contracts pick exactly the role surface they need — from zero ABI overhead up to the full named interface — without duplicating any role logic. @@ -50,7 +50,7 @@ For components that only need role *checks*, with no named role management ABI o **Wire:** `CommonRolesComponent` + `AccessControlComponent` + `SRC5Component`. No `RolesComponent`. -**Embed:** `CommonRolesImpl` → 4 generic entry points: `grant_role`, `revoke_role`, `has_role`, `renounce`. +**Embed:** `CommonRolesImpl` → 7 entry points: `grant_role`, `revoke_role`, `has_role`, `renounce`, `reclaim_legacy_roles`, `reclaim_legacy_roles_for_accounts`, `disable_legacy_role_reclaim`. **Role management:** callers use `ICommonRoles::grant_role(Role::UpgradeGovernor, account)`. @@ -61,8 +61,6 @@ fn constructor(ref self: ContractState, governance_admin: ContractAddress) { } ``` -**Overhead:** none. `CommonRolesComponent::Event` is empty; `CommonRolesComponent` has no storage. - --- ### Tier B — Selective named roles @@ -81,21 +79,15 @@ impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; // NO RolesImpl ``` -**ABI result:** `IGovernanceRoles` (12 EPs) + `ICommonRoles` (4 EPs). Named events (`UpgradeGovernorAdded`, etc.) are emitted by the category impl methods. Unused event variants are dead-code eliminated from Sierra. +**ABI result:** `IGovernanceRoles` (12 EPs) + `ICommonRoles` (7 EPs). Named events (`UpgradeGovernorAdded`, etc.) are emitted by the category impl methods. Unused event variants are dead-code eliminated from Sierra. **Constructor:** ```cairo fn constructor(ref self: ContractState, governance_admin: ContractAddress) { - self.roles.initialize(:governance_admin); // NOT common_roles.initialize + self.roles.initialize(:governance_admin); } ``` -> **Why `roles.initialize` and not `common_roles.initialize`?** -> -> `roles.initialize` does two things: calls `common_roles.initialize` (role setup) and then writes `legacy_role_reclaim_disabled = true`. If a future upgrade adds `RolesImpl` to the ABI, this flag blocks inadvertent legacy role reclaim. Calling `common_roles.initialize` directly skips the flag, leaving a latent upgrade footgun — harmless today, but dangerous if `RolesImpl` is ever added. - -**Overhead:** 2 legacy storage slots in `RolesComponent` (`role_members` map + `legacy_role_reclaim_disabled` bool). Neither is ever written in normal operation, so there is no runtime storage cost. - --- ### Tier C — Full roles @@ -117,7 +109,7 @@ fn constructor(ref self: ContractState, governance_admin: ContractAddress) { ## Key Invariants -- `CommonRolesComponent` carries no role state of its own — all state lives in `AccessControlComponent`. +- `CommonRolesComponent` owns all role state — `AccessControlComponent` holds the active role memberships; `CommonRolesComponent` holds the legacy reclaim storage (`role_members` map + `legacy_role_reclaim_disabled` flag). - `RolesComponent` never duplicates role state — it reads and writes through `CommonRolesComponent`'s internal methods. - Named events exist only in `RolesComponent::Event`. `CommonRolesComponent::Event` is always empty. - Category impls exist only in `RolesComponent`. There is no duplication between layers. diff --git a/packages/utils/src/components/common_roles/test.cairo b/packages/utils/src/components/common_roles/test.cairo index 37e6a84a..46e0eb4b 100644 --- a/packages/utils/src/components/common_roles/test.cairo +++ b/packages/utils/src/components/common_roles/test.cairo @@ -3,7 +3,7 @@ use snforge_std::{ContractClassTrait, DeclareResultTrait, declare}; use starknet::ContractAddress; use starkware_utils::components::common_roles::mock_contract::{ IGuardTestDispatcher, IGuardTestDispatcherTrait, IGuardTestSafeDispatcher, - IGuardTestSafeDispatcherTrait, + IGuardTestSafeDispatcherTrait, ILegacySetupDispatcher, ILegacySetupDispatcherTrait, }; use starkware_utils::components::roles::errors::AccessErrors; use starkware_utils::components::roles::interface::{ @@ -28,7 +28,7 @@ fn deploy_safe() -> (ContractAddress, ICommonRolesSafeDispatcher) { } // ─── initialize -// ────────────────────────────────────────────────────────────── +// ─────────────────────────────────────────────── #[test] fn test_initialize_grants_governance_admin() { @@ -43,7 +43,7 @@ fn test_initialize_grants_security_admin() { } // ─── grant_role / has_role -// ──────────────────────────────────────────────────── +// ────────────────────────── #[test] fn test_grant_role_authorized() { @@ -78,7 +78,7 @@ fn test_grant_role_zero_address() { } // ─── revoke_role -// ───────────────────────────────────────────────────────────── +// ───────────────────────────────────────────── #[test] fn test_revoke_role() { @@ -95,8 +95,49 @@ fn test_revoke_role() { assert!(!dispatcher.has_role(Role::AppRoleAdmin, account)); } +#[test] +#[feature("safe_dispatcher")] +fn test_revoke_role_self_governance_admin_panics() { + let (address, safe) = deploy_safe(); + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + let result = safe.revoke_role(Role::GovernanceAdmin, constants::INITIAL_ROOT_ADMIN); + assert_panic_with_error( + :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), + ); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_revoke_role_self_security_admin_panics() { + let (address, safe) = deploy_safe(); + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + let result = safe.revoke_role(Role::SecurityAdmin, constants::INITIAL_ROOT_ADMIN); + assert_panic_with_error( + :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), + ); +} + +#[test] +fn test_revoke_role_other_account_governance_admin_succeeds() { + let (address, dispatcher) = deploy(); + let gov_admin = constants::INITIAL_ROOT_ADMIN; + let other_gov_admin = constants::GOVERNANCE_ADMIN; + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.grant_role(Role::GovernanceAdmin, other_gov_admin); + assert!(dispatcher.has_role(Role::GovernanceAdmin, other_gov_admin)); + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.revoke_role(Role::GovernanceAdmin, other_gov_admin); + assert!(!dispatcher.has_role(Role::GovernanceAdmin, other_gov_admin)); +} + // ─── renounce -// ──────────────────────────────────────────────────────────────── +// ─────────────────────────────────────────────────── #[test] fn test_renounce_renounceable_role() { @@ -139,7 +180,7 @@ fn test_renounce_security_admin_panics() { } // ─── only_X guards -// ──────────────────────────────────────────────────────────── +// ────────────────────────────────────────── fn setup_guard_test() -> (ContractAddress, ICommonRolesDispatcher, IGuardTestSafeDispatcher) { let (address, dispatcher) = deploy(); @@ -374,3 +415,180 @@ fn test_only_security_governor_unauthorized() { :result, expected_error: AccessErrors::ONLY_SECURITY_GOVERNOR.describe(), ); } + +// ─── legacy role reclaim +// ────────────────────────────── + +fn deploy_legacy() -> (ContractAddress, ICommonRolesDispatcher, ILegacySetupDispatcher) { + let contract = *declare("LegacyCommonRolesMock").unwrap().contract_class(); + let (address, _) = contract.deploy(@array![constants::INITIAL_ROOT_ADMIN.into()]).unwrap(); + ( + address, + ICommonRolesDispatcher { contract_address: address }, + ILegacySetupDispatcher { contract_address: address }, + ) +} + +fn deploy_legacy_safe() -> (ContractAddress, ICommonRolesSafeDispatcher, ILegacySetupDispatcher) { + let (address, _, setup) = deploy_legacy(); + (address, ICommonRolesSafeDispatcher { contract_address: address }, setup) +} + +#[test] +#[feature("safe_dispatcher")] +fn test_reclaim_legacy_roles_disabled_after_initialize() { + // Fresh contracts that called initialize() must NOT allow reclaim. + let (_, safe) = deploy_safe(); + let result = safe.reclaim_legacy_roles(); + assert_panic_with_error( + :result, expected_error: AccessErrors::LEGACY_ROLE_RECLAIM_DISABLED.describe(), + ); +} + +#[test] +fn test_reclaim_legacy_roles_restores_membership() { + use starkware_utils::components::roles::interface::OPERATOR; + let (address, dispatcher, setup) = deploy_legacy(); + let account = constants::OPERATOR; + + assert!(!dispatcher.has_role(Role::Operator, account)); + setup.set_legacy_role(OPERATOR, account); + cheat_caller_address_once(contract_address: address, caller_address: account); + dispatcher.reclaim_legacy_roles(); + assert!(dispatcher.has_role(Role::Operator, account)); +} + +#[test] +fn test_reclaim_legacy_roles_clears_legacy_entry() { + use starkware_utils::components::roles::interface::OPERATOR; + let (address, dispatcher, setup) = deploy_legacy(); + let account = constants::OPERATOR; + + setup.set_legacy_role(OPERATOR, account); + cheat_caller_address_once(contract_address: address, caller_address: account); + dispatcher.reclaim_legacy_roles(); + // Second reclaim should be a no-op (legacy entry cleared). + cheat_caller_address_once(contract_address: address, caller_address: account); + dispatcher.reclaim_legacy_roles(); + assert!(dispatcher.has_role(Role::Operator, account)); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_reclaim_legacy_roles_for_accounts_requires_security_governor() { + let (address, _, _) = deploy_legacy(); + let safe = ICommonRolesSafeDispatcher { contract_address: address }; + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = safe.reclaim_legacy_roles_for_accounts(array![constants::OPERATOR].span()); + assert_panic_with_error( + :result, expected_error: AccessErrors::ONLY_SECURITY_GOVERNOR.describe(), + ); +} + +#[test] +fn test_reclaim_legacy_roles_for_accounts_restores_membership() { + use starkware_utils::components::roles::interface::OPERATOR; + let (address, dispatcher, setup) = deploy_legacy(); + let account = constants::OPERATOR; + + assert!(!dispatcher.has_role(Role::Operator, account)); + setup.set_legacy_role(OPERATOR, account); + + // Grant security_governor role to INITIAL_ROOT_ADMIN so they can call the function. + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::SecurityGovernor, constants::INITIAL_ROOT_ADMIN); + + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.reclaim_legacy_roles_for_accounts(array![account].span()); + assert!(dispatcher.has_role(Role::Operator, account)); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_disable_legacy_role_reclaim_requires_upgrade_governor() { + let (address, _, _) = deploy_legacy(); + let safe = ICommonRolesSafeDispatcher { contract_address: address }; + cheat_caller_address_once(contract_address: address, caller_address: constants::WRONG_ADMIN); + let result = safe.disable_legacy_role_reclaim(); + assert_panic_with_error( + :result, expected_error: AccessErrors::ONLY_UPGRADE_GOVERNOR.describe(), + ); +} + +#[test] +fn test_reclaim_legacy_roles_noop_when_no_legacy_roles() { + let (address, dispatcher, _) = deploy_legacy(); + // No legacy roles set — reclaim should succeed without granting anything. + cheat_caller_address_once(contract_address: address, caller_address: constants::OPERATOR); + dispatcher.reclaim_legacy_roles(); + assert!(!dispatcher.has_role(Role::Operator, constants::OPERATOR)); +} + +#[test] +fn test_revoke_then_regrant_role() { + let (address, dispatcher) = deploy(); + let account = constants::APP_ROLE_ADMIN; + let gov_admin = constants::INITIAL_ROOT_ADMIN; + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.grant_role(Role::AppRoleAdmin, account); + assert!(dispatcher.has_role(Role::AppRoleAdmin, account)); + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.revoke_role(Role::AppRoleAdmin, account); + assert!(!dispatcher.has_role(Role::AppRoleAdmin, account)); + + cheat_caller_address_once(contract_address: address, caller_address: gov_admin); + dispatcher.grant_role(Role::AppRoleAdmin, account); + assert!(dispatcher.has_role(Role::AppRoleAdmin, account)); +} + +#[test] +fn test_disable_legacy_role_reclaim_idempotent() { + let (address, dispatcher, _) = deploy_legacy(); + + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::UpgradeGovernor, constants::INITIAL_ROOT_ADMIN); + + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.disable_legacy_role_reclaim(); + // Second call should succeed (idempotent write). + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.disable_legacy_role_reclaim(); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_disable_legacy_role_reclaim_prevents_reclaim() { + let (address, dispatcher, _) = deploy_legacy(); + let safe = ICommonRolesSafeDispatcher { contract_address: address }; + + // Grant upgrade_governor to INITIAL_ROOT_ADMIN. + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.grant_role(Role::UpgradeGovernor, constants::INITIAL_ROOT_ADMIN); + + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + dispatcher.disable_legacy_role_reclaim(); + + cheat_caller_address_once( + contract_address: address, caller_address: constants::INITIAL_ROOT_ADMIN, + ); + let result = safe.reclaim_legacy_roles(); + assert_panic_with_error( + :result, expected_error: AccessErrors::LEGACY_ROLE_RECLAIM_DISABLED.describe(), + ); +} diff --git a/packages/utils/src/components/roles/interface.cairo b/packages/utils/src/components/roles/interface.cairo index 67d25003..7d32dfff 100644 --- a/packages/utils/src/components/roles/interface.cairo +++ b/packages/utils/src/components/roles/interface.cairo @@ -76,16 +76,12 @@ pub trait IRoles { fn remove_upgrade_agent(ref self: TContractState, account: ContractAddress); fn register_upgrade_governor(ref self: TContractState, account: ContractAddress); fn remove_upgrade_governor(ref self: TContractState, account: ContractAddress); - fn renounce(ref self: TContractState, role: RoleId); fn register_security_admin(ref self: TContractState, account: ContractAddress); fn remove_security_admin(ref self: TContractState, account: ContractAddress); fn register_security_agent(ref self: TContractState, account: ContractAddress); fn remove_security_agent(ref self: TContractState, account: ContractAddress); fn register_security_governor(ref self: TContractState, account: ContractAddress); fn remove_security_governor(ref self: TContractState, account: ContractAddress); - fn reclaim_legacy_roles(ref self: TContractState); - fn reclaim_legacy_roles_for_accounts(ref self: TContractState, accounts: Span); - fn disable_legacy_role_reclaim(ref self: TContractState); } #[derive(Copy, Drop, PartialEq, starknet::Event)] @@ -208,9 +204,9 @@ pub(crate) struct UpgradeAgentRemoved { } // ─── CommonRoles types -// ──────────────────────────────────────────────────────── +// ────────────────────────────────── -#[derive(Copy, Drop, PartialEq, Serde)] +#[derive(Copy, Drop, PartialEq)] pub enum Role { AppGovernor, AppRoleAdmin, @@ -224,6 +220,48 @@ pub enum Role { SecurityGovernor, } +pub impl RoleSerde of Serde { + fn serialize(self: @Role, ref output: Array) { + let role_id: RoleId = (*self).into(); + role_id.serialize(ref output); + } + + fn deserialize(ref serialized: Span) -> Option { + let role_id: RoleId = Serde::deserialize(ref serialized)?; + if role_id == APP_GOVERNOR { + return Option::Some(Role::AppGovernor); + } + if role_id == APP_ROLE_ADMIN { + return Option::Some(Role::AppRoleAdmin); + } + if role_id == GOVERNANCE_ADMIN { + return Option::Some(Role::GovernanceAdmin); + } + if role_id == OPERATOR { + return Option::Some(Role::Operator); + } + if role_id == TOKEN_ADMIN { + return Option::Some(Role::TokenAdmin); + } + if role_id == UPGRADE_AGENT { + return Option::Some(Role::UpgradeAgent); + } + if role_id == UPGRADE_GOVERNOR { + return Option::Some(Role::UpgradeGovernor); + } + if role_id == SECURITY_ADMIN { + return Option::Some(Role::SecurityAdmin); + } + if role_id == SECURITY_AGENT { + return Option::Some(Role::SecurityAgent); + } + if role_id == SECURITY_GOVERNOR { + return Option::Some(Role::SecurityGovernor); + } + Option::None + } +} + pub impl RoleIntoRoleId of Into { fn into(self: Role) -> RoleId { match self { @@ -262,10 +300,13 @@ pub trait ICommonRoles { fn revoke_role(ref self: TState, role: Role, account: ContractAddress); fn has_role(self: @TState, role: Role, account: ContractAddress) -> bool; fn renounce(ref self: TState, role: Role); + // Always present — rescue path for contracts upgrading from legacy role storage. + fn reclaim_legacy_roles(ref self: TState); + fn reclaim_legacy_roles_for_accounts(ref self: TState, accounts: Span); + fn disable_legacy_role_reclaim(ref self: TState); } -// ─── Category-scoped role interfaces -// ───────────────────────────────────────── +// ─── Category-scoped role interfaces ───── #[starknet::interface] pub trait ISecurityRoles { diff --git a/packages/utils/src/components/roles/mock_contract.cairo b/packages/utils/src/components/roles/mock_contract.cairo index 002d4467..fbf55a22 100644 --- a/packages/utils/src/components/roles/mock_contract.cairo +++ b/packages/utils/src/components/roles/mock_contract.cairo @@ -35,6 +35,8 @@ pub mod MockContract { #[abi(embed_v0)] impl RolesImpl = RolesComponent::RolesImpl; + #[abi(embed_v0)] + impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; #[constructor] fn constructor(ref self: ContractState, governance_admin: ContractAddress) { diff --git a/packages/utils/src/components/roles/roles.cairo b/packages/utils/src/components/roles/roles.cairo index a54b7bed..b7108b0b 100644 --- a/packages/utils/src/components/roles/roles.cairo +++ b/packages/utils/src/components/roles/roles.cairo @@ -1,25 +1,16 @@ #[starknet::component] pub(crate) mod RolesComponent { use RolesInterface::{ - APP_GOVERNOR, APP_ROLE_ADMIN, AppGovernorAdded, AppGovernorRemoved, AppRoleAdminAdded, - AppRoleAdminRemoved, GOVERNANCE_ADMIN, GovernanceAdminAdded, GovernanceAdminRemoved, - IAppRoles, IGovernanceRoles, IRoles, ISecurityRoles, OPERATOR, OperatorAdded, - OperatorRemoved, RoleId, SECURITY_ADMIN, SECURITY_AGENT, SECURITY_GOVERNOR, - SecurityAdminAdded, SecurityAdminRemoved, SecurityAgentAdded, SecurityAgentRemoved, - SecurityGovernorAdded, SecurityGovernorRemoved, TOKEN_ADMIN, TokenAdminAdded, - TokenAdminRemoved, UPGRADE_AGENT, UPGRADE_GOVERNOR, UpgradeAgentAdded, UpgradeAgentRemoved, - UpgradeGovernorAdded, UpgradeGovernorRemoved, + AppGovernorAdded, AppGovernorRemoved, AppRoleAdminAdded, AppRoleAdminRemoved, + GovernanceAdminAdded, GovernanceAdminRemoved, IAppRoles, IGovernanceRoles, IRoles, + ISecurityRoles, OperatorAdded, OperatorRemoved, Role, SecurityAdminAdded, + SecurityAdminRemoved, SecurityAgentAdded, SecurityAgentRemoved, SecurityGovernorAdded, + SecurityGovernorRemoved, TokenAdminAdded, TokenAdminRemoved, UpgradeAgentAdded, + UpgradeAgentRemoved, UpgradeGovernorAdded, UpgradeGovernorRemoved, }; - use core::num::traits::Zero; use openzeppelin::access::accesscontrol::AccessControlComponent; - use openzeppelin::access::accesscontrol::AccessControlComponent::{ - AccessControlImpl, InternalTrait as AccessInternalTrait, - }; + use openzeppelin::access::accesscontrol::AccessControlComponent::AccessControlImpl; use openzeppelin::introspection::src5::SRC5Component; - use starknet::storage::{ - StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, - StoragePointerWriteAccess, - }; use starknet::{ContractAddress, get_caller_address}; use starkware_utils::components::common_roles::CommonRolesComponent; use starkware_utils::components::common_roles::CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; @@ -27,13 +18,7 @@ pub(crate) mod RolesComponent { use starkware_utils::components::roles::interface as RolesInterface; #[storage] - pub struct Storage { - // LEGACY: This is the old storage for role members. - // We need it to allow reclaim legacy roles. - role_members: starknet::storage::Map<(RoleId, ContractAddress), bool>, - // Flag to disable legacy role reclaim. Once set, legacy roles cannot be reclaimed. - legacy_role_reclaim_disabled: bool, - } + pub struct Storage {} #[event] #[derive(Copy, Drop, PartialEq, starknet::Event)] @@ -72,156 +57,220 @@ pub(crate) mod RolesComponent { fn is_app_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: APP_GOVERNOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::AppGovernor, :account) } fn is_app_role_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: APP_ROLE_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::AppRoleAdmin, :account) } fn is_governance_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: GOVERNANCE_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::GovernanceAdmin, :account) } fn is_operator(self: @ComponentState, account: ContractAddress) -> bool { - get_dep_component!(self, Access).has_role(role: OPERATOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::Operator, :account) } fn is_security_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: SECURITY_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::SecurityAdmin, :account) } fn is_security_agent( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: SECURITY_AGENT, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::SecurityAgent, :account) } fn is_security_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: SECURITY_GOVERNOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::SecurityGovernor, :account) } fn is_token_admin(self: @ComponentState, account: ContractAddress) -> bool { - get_dep_component!(self, Access).has_role(role: TOKEN_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::TokenAdmin, :account) } fn is_upgrade_agent( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: UPGRADE_AGENT, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::UpgradeAgent, :account) } fn is_upgrade_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: UPGRADE_GOVERNOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::UpgradeGovernor, :account) } fn register_app_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::AppGovernorAdded( - AppGovernorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: APP_GOVERNOR, :account, :event); + self + ._register_role( + Role::AppGovernor, + account, + Event::AppGovernorAdded( + AppGovernorAdded { added_account: account, added_by: get_caller_address() }, + ), + ); } fn remove_app_governor(ref self: ComponentState, account: ContractAddress) { - let event = Event::AppGovernorRemoved( - AppGovernorRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: APP_GOVERNOR, :account, :event); + self + ._remove_role( + Role::AppGovernor, + account, + Event::AppGovernorRemoved( + AppGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_app_role_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::AppRoleAdminAdded( - AppRoleAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: APP_ROLE_ADMIN, :account, :event); + self + ._register_role( + Role::AppRoleAdmin, + account, + Event::AppRoleAdminAdded( + AppRoleAdminAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_app_role_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::AppRoleAdminRemoved( - AppRoleAdminRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: APP_ROLE_ADMIN, :account, :event); + self + ._remove_role( + Role::AppRoleAdmin, + account, + Event::AppRoleAdminRemoved( + AppRoleAdminRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_security_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAdminAdded( - SecurityAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: SECURITY_ADMIN, :account, :event); + self + ._register_role( + Role::SecurityAdmin, + account, + Event::SecurityAdminAdded( + SecurityAdminAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_security_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAdminRemoved( - SecurityAdminRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: SECURITY_ADMIN, :account, :event); + let caller_address = get_caller_address(); + assert!(account != caller_address, "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); + self + ._remove_role( + Role::SecurityAdmin, + account, + Event::SecurityAdminRemoved( + SecurityAdminRemoved { + removed_account: account, removed_by: caller_address, + }, + ), + ); } fn register_security_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAgentAdded( - SecurityAgentAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: SECURITY_AGENT, :account, :event); + self + ._register_role( + Role::SecurityAgent, + account, + Event::SecurityAgentAdded( + SecurityAgentAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_security_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAgentRemoved( - SecurityAgentRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: SECURITY_AGENT, :account, :event); + self + ._remove_role( + Role::SecurityAgent, + account, + Event::SecurityAgentRemoved( + SecurityAgentRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_security_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityGovernorAdded( - SecurityGovernorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: SECURITY_GOVERNOR, :account, :event); + self + ._register_role( + Role::SecurityGovernor, + account, + Event::SecurityGovernorAdded( + SecurityGovernorAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_security_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityGovernorRemoved( - SecurityGovernorRemoved { - removed_account: account, removed_by: get_caller_address(), - }, - ); - self._revoke_role_and_emit(role: SECURITY_GOVERNOR, :account, :event); + self + ._remove_role( + Role::SecurityGovernor, + account, + Event::SecurityGovernorRemoved( + SecurityGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_governance_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::GovernanceAdminAdded( - GovernanceAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: GOVERNANCE_ADMIN, :account, :event); + self + ._register_role( + Role::GovernanceAdmin, + account, + Event::GovernanceAdminAdded( + GovernanceAdminAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_governance_admin( @@ -229,115 +278,130 @@ pub(crate) mod RolesComponent { ) { let caller_address = get_caller_address(); assert!(account != caller_address, "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); - let event = Event::GovernanceAdminRemoved( - GovernanceAdminRemoved { removed_account: account, removed_by: caller_address }, - ); - self._revoke_role_and_emit(role: GOVERNANCE_ADMIN, :account, :event); + self + ._remove_role( + Role::GovernanceAdmin, + account, + Event::GovernanceAdminRemoved( + GovernanceAdminRemoved { + removed_account: account, removed_by: caller_address, + }, + ), + ); } fn register_operator(ref self: ComponentState, account: ContractAddress) { - let event = Event::OperatorAdded( - OperatorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: OPERATOR, :account, :event); + self + ._register_role( + Role::Operator, + account, + Event::OperatorAdded( + OperatorAdded { added_account: account, added_by: get_caller_address() }, + ), + ); } fn remove_operator(ref self: ComponentState, account: ContractAddress) { - let event = Event::OperatorRemoved( - OperatorRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: OPERATOR, :account, :event); + self + ._remove_role( + Role::Operator, + account, + Event::OperatorRemoved( + OperatorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_token_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::TokenAdminAdded( - TokenAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: TOKEN_ADMIN, :account, :event); + self + ._register_role( + Role::TokenAdmin, + account, + Event::TokenAdminAdded( + TokenAdminAdded { added_account: account, added_by: get_caller_address() }, + ), + ); } fn remove_token_admin(ref self: ComponentState, account: ContractAddress) { - let event = Event::TokenAdminRemoved( - TokenAdminRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: TOKEN_ADMIN, :account, :event); + self + ._remove_role( + Role::TokenAdmin, + account, + Event::TokenAdminRemoved( + TokenAdminRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_upgrade_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeAgentAdded( - UpgradeAgentAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: UPGRADE_AGENT, :account, :event); + self + ._register_role( + Role::UpgradeAgent, + account, + Event::UpgradeAgentAdded( + UpgradeAgentAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_upgrade_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeAgentRemoved( - UpgradeAgentRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: UPGRADE_AGENT, :account, :event); + self + ._remove_role( + Role::UpgradeAgent, + account, + Event::UpgradeAgentRemoved( + UpgradeAgentRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn register_upgrade_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeGovernorAdded( - UpgradeGovernorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: UPGRADE_GOVERNOR, :account, :event); + self + ._register_role( + Role::UpgradeGovernor, + account, + Event::UpgradeGovernorAdded( + UpgradeGovernorAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_upgrade_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeGovernorRemoved( - UpgradeGovernorRemoved { - removed_account: account, removed_by: get_caller_address(), - }, - ); - self._revoke_role_and_emit(role: UPGRADE_GOVERNOR, :account, :event); - } - - fn renounce(ref self: ComponentState, role: RoleId) { - assert!( - role != GOVERNANCE_ADMIN && role != SECURITY_ADMIN, - "{}", - AccessErrors::ROLE_CANNOT_BE_RENOUNCED, - ); - let mut access_comp = get_dep_component_mut!(ref self, Access); - access_comp.renounce_role(:role, account: get_caller_address()) - } - - fn reclaim_legacy_roles(ref self: ComponentState) { - self.assert_role_reclaim_enabled(); - self._reclaim_legacy_roles_for_account(get_caller_address()); - let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); - common_roles.ensure_role_admins(); - } - - fn reclaim_legacy_roles_for_accounts( - ref self: ComponentState, accounts: Span, - ) { - self.only_security_governor(); - self.assert_role_reclaim_enabled(); - for account in accounts { - self._reclaim_legacy_roles_for_account(*account); - }; - } - - fn disable_legacy_role_reclaim(ref self: ComponentState) { - self.only_upgrade_governor(); - self.legacy_role_reclaim_disabled.write(true); + self + ._remove_role( + Role::UpgradeGovernor, + account, + Event::UpgradeGovernorRemoved( + UpgradeGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } } - // ─── Category-scoped role impls - // ─────────────────────────────────────────── + // ─── Category-scoped role impls ──────────── #[embeddable_as(SecurityRolesImpl)] pub impl SecurityRoles< @@ -351,75 +415,111 @@ pub(crate) mod RolesComponent { fn is_security_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: SECURITY_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::SecurityAdmin, :account) } fn register_security_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAdminAdded( - SecurityAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: SECURITY_ADMIN, :account, :event); + self + ._register_role( + Role::SecurityAdmin, + account, + Event::SecurityAdminAdded( + SecurityAdminAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_security_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAdminRemoved( - SecurityAdminRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: SECURITY_ADMIN, :account, :event); + let caller_address = get_caller_address(); + assert!(account != caller_address, "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); + self + ._remove_role( + Role::SecurityAdmin, + account, + Event::SecurityAdminRemoved( + SecurityAdminRemoved { + removed_account: account, removed_by: caller_address, + }, + ), + ); } fn is_security_agent( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: SECURITY_AGENT, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::SecurityAgent, :account) } fn register_security_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAgentAdded( - SecurityAgentAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: SECURITY_AGENT, :account, :event); + self + ._register_role( + Role::SecurityAgent, + account, + Event::SecurityAgentAdded( + SecurityAgentAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_security_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityAgentRemoved( - SecurityAgentRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: SECURITY_AGENT, :account, :event); + self + ._remove_role( + Role::SecurityAgent, + account, + Event::SecurityAgentRemoved( + SecurityAgentRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn is_security_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: SECURITY_GOVERNOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::SecurityGovernor, :account) } fn register_security_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityGovernorAdded( - SecurityGovernorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: SECURITY_GOVERNOR, :account, :event); + self + ._register_role( + Role::SecurityGovernor, + account, + Event::SecurityGovernorAdded( + SecurityGovernorAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_security_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::SecurityGovernorRemoved( - SecurityGovernorRemoved { - removed_account: account, removed_by: get_caller_address(), - }, - ); - self._revoke_role_and_emit(role: SECURITY_GOVERNOR, :account, :event); + self + ._remove_role( + Role::SecurityGovernor, + account, + Event::SecurityGovernorRemoved( + SecurityGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } } @@ -435,16 +535,22 @@ pub(crate) mod RolesComponent { fn is_governance_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: GOVERNANCE_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::GovernanceAdmin, :account) } fn register_governance_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::GovernanceAdminAdded( - GovernanceAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: GOVERNANCE_ADMIN, :account, :event); + self + ._register_role( + Role::GovernanceAdmin, + account, + Event::GovernanceAdminAdded( + GovernanceAdminAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_governance_admin( @@ -452,84 +558,124 @@ pub(crate) mod RolesComponent { ) { let caller_address = get_caller_address(); assert!(account != caller_address, "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); - let event = Event::GovernanceAdminRemoved( - GovernanceAdminRemoved { removed_account: account, removed_by: caller_address }, - ); - self._revoke_role_and_emit(role: GOVERNANCE_ADMIN, :account, :event); + self + ._remove_role( + Role::GovernanceAdmin, + account, + Event::GovernanceAdminRemoved( + GovernanceAdminRemoved { + removed_account: account, removed_by: caller_address, + }, + ), + ); } fn is_app_role_admin( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: APP_ROLE_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::AppRoleAdmin, :account) } fn register_app_role_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::AppRoleAdminAdded( - AppRoleAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: APP_ROLE_ADMIN, :account, :event); + self + ._register_role( + Role::AppRoleAdmin, + account, + Event::AppRoleAdminAdded( + AppRoleAdminAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_app_role_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::AppRoleAdminRemoved( - AppRoleAdminRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: APP_ROLE_ADMIN, :account, :event); + self + ._remove_role( + Role::AppRoleAdmin, + account, + Event::AppRoleAdminRemoved( + AppRoleAdminRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn is_upgrade_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: UPGRADE_GOVERNOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::UpgradeGovernor, :account) } fn register_upgrade_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeGovernorAdded( - UpgradeGovernorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: UPGRADE_GOVERNOR, :account, :event); + self + ._register_role( + Role::UpgradeGovernor, + account, + Event::UpgradeGovernorAdded( + UpgradeGovernorAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_upgrade_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeGovernorRemoved( - UpgradeGovernorRemoved { - removed_account: account, removed_by: get_caller_address(), - }, - ); - self._revoke_role_and_emit(role: UPGRADE_GOVERNOR, :account, :event); + self + ._remove_role( + Role::UpgradeGovernor, + account, + Event::UpgradeGovernorRemoved( + UpgradeGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn is_upgrade_agent( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: UPGRADE_AGENT, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::UpgradeAgent, :account) } fn register_upgrade_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeAgentAdded( - UpgradeAgentAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: UPGRADE_AGENT, :account, :event); + self + ._register_role( + Role::UpgradeAgent, + account, + Event::UpgradeAgentAdded( + UpgradeAgentAdded { + added_account: account, added_by: get_caller_address(), + }, + ), + ); } fn remove_upgrade_agent( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::UpgradeAgentRemoved( - UpgradeAgentRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: UPGRADE_AGENT, :account, :event); + self + ._remove_role( + Role::UpgradeAgent, + account, + Event::UpgradeAgentRemoved( + UpgradeAgentRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } } @@ -545,100 +691,91 @@ pub(crate) mod RolesComponent { fn is_app_governor( self: @ComponentState, account: ContractAddress, ) -> bool { - get_dep_component!(self, Access).has_role(role: APP_GOVERNOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::AppGovernor, :account) } fn register_app_governor( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::AppGovernorAdded( - AppGovernorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: APP_GOVERNOR, :account, :event); + self + ._register_role( + Role::AppGovernor, + account, + Event::AppGovernorAdded( + AppGovernorAdded { added_account: account, added_by: get_caller_address() }, + ), + ); } fn remove_app_governor(ref self: ComponentState, account: ContractAddress) { - let event = Event::AppGovernorRemoved( - AppGovernorRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: APP_GOVERNOR, :account, :event); + self + ._remove_role( + Role::AppGovernor, + account, + Event::AppGovernorRemoved( + AppGovernorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn is_operator(self: @ComponentState, account: ContractAddress) -> bool { - get_dep_component!(self, Access).has_role(role: OPERATOR, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::Operator, :account) } fn register_operator(ref self: ComponentState, account: ContractAddress) { - let event = Event::OperatorAdded( - OperatorAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: OPERATOR, :account, :event); + self + ._register_role( + Role::Operator, + account, + Event::OperatorAdded( + OperatorAdded { added_account: account, added_by: get_caller_address() }, + ), + ); } fn remove_operator(ref self: ComponentState, account: ContractAddress) { - let event = Event::OperatorRemoved( - OperatorRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: OPERATOR, :account, :event); + self + ._remove_role( + Role::Operator, + account, + Event::OperatorRemoved( + OperatorRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } fn is_token_admin(self: @ComponentState, account: ContractAddress) -> bool { - get_dep_component!(self, Access).has_role(role: TOKEN_ADMIN, :account) + get_dep_component!(self, CommonRoles).has_role(role: Role::TokenAdmin, :account) } fn register_token_admin( ref self: ComponentState, account: ContractAddress, ) { - let event = Event::TokenAdminAdded( - TokenAdminAdded { added_account: account, added_by: get_caller_address() }, - ); - self._grant_role_and_emit(role: TOKEN_ADMIN, :account, :event); + self + ._register_role( + Role::TokenAdmin, + account, + Event::TokenAdminAdded( + TokenAdminAdded { added_account: account, added_by: get_caller_address() }, + ), + ); } fn remove_token_admin(ref self: ComponentState, account: ContractAddress) { - let event = Event::TokenAdminRemoved( - TokenAdminRemoved { removed_account: account, removed_by: get_caller_address() }, - ); - self._revoke_role_and_emit(role: TOKEN_ADMIN, :account, :event); - } - } - - #[generate_trait] - pub impl ClaimRoleImpl< - TContractState, - +HasComponent, - +Drop, - impl Access: AccessControlComponent::HasComponent, - +SRC5Component::HasComponent, - > of ClaimRoleInternal { - // Reinstate role membership per legacy role membership: - // 1. If the account held the legacy role, it will be granted in the current realm. - // 2. Role membership in the current realm will not be cleared if no legacy member held. - // 3. Legacy membership is cleared after reading, so reclaim can be done effectively only - // once. - fn _reclaim_role( - ref self: ComponentState, role: RoleId, account: ContractAddress, - ) { - if self._has_legacy_role(account, role) { - // Clear legacy membership to prevent double claiming. - self.role_members.write((role, account), false); - let mut access_comp = get_dep_component_mut!(ref self, Access); - access_comp._grant_role(:role, :account); - } - } - - fn _has_legacy_role( - self: @ComponentState, account: ContractAddress, role: RoleId, - ) -> bool { - self.role_members.read((role, account)) - } - - fn _reclaim_legacy_roles_for_account( - ref self: ComponentState, account: ContractAddress, - ) { - for (role, _) in CommonRolesComponent::ROLE_ADMIN_PAIRS.span() { - self._reclaim_role(role: *role, :account); - } + self + ._remove_role( + Role::TokenAdmin, + account, + Event::TokenAdminRemoved( + TokenAdminRemoved { + removed_account: account, removed_by: get_caller_address(), + }, + ), + ); } } @@ -651,39 +788,32 @@ pub(crate) mod RolesComponent { impl CommonRoles: CommonRolesComponent::HasComponent, +SRC5Component::HasComponent, > of InternalTrait { - fn _grant_role_and_emit( + fn _register_role( ref self: ComponentState, - role: RoleId, + role: Role, account: ContractAddress, event: Event, ) { - let mut access_comp = get_dep_component_mut!(ref self, Access); - if !access_comp.has_role(:role, :account) { - assert!(account.is_non_zero(), "{}", AccessErrors::ZERO_ADDRESS); - access_comp.grant_role(:role, :account); - self.emit(event); + let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); + if common_roles.has_role(:role, :account) { + return; } + common_roles.grant_role(:role, :account); + self.emit(event); } - fn _revoke_role_and_emit( + fn _remove_role( ref self: ComponentState, - role: RoleId, + role: Role, account: ContractAddress, event: Event, ) { - let mut access_comp = get_dep_component_mut!(ref self, Access); - if access_comp.has_role(:role, :account) { - access_comp.revoke_role(:role, :account); - self.emit(event); + let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); + if !common_roles.has_role(:role, :account) { + return; } - } - - fn assert_role_reclaim_enabled(self: @ComponentState) { - assert!( - !self.legacy_role_reclaim_disabled.read(), - "{}", - AccessErrors::LEGACY_ROLE_RECLAIM_DISABLED, - ); + common_roles.revoke_role(:role, :account); + self.emit(event); } // WARNING @@ -692,7 +822,6 @@ pub(crate) mod RolesComponent { fn initialize(ref self: ComponentState, governance_admin: ContractAddress) { let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); common_roles.initialize(:governance_admin); - self.legacy_role_reclaim_disabled.write(true); } fn only_app_governor(self: @ComponentState) { diff --git a/packages/utils/src/components/roles/test.cairo b/packages/utils/src/components/roles/test.cairo index e835e291..3fce00ed 100644 --- a/packages/utils/src/components/roles/test.cairo +++ b/packages/utils/src/components/roles/test.cairo @@ -1,8 +1,10 @@ use core::num::traits::zero::Zero; use interface::{ - IAppRolesDispatcher, IAppRolesDispatcherTrait, IGovernanceRolesDispatcher, - IGovernanceRolesDispatcherTrait, IRolesDispatcher, IRolesDispatcherTrait, IRolesSafeDispatcher, - IRolesSafeDispatcherTrait, ISecurityRolesDispatcher, ISecurityRolesDispatcherTrait, + IAppRolesDispatcher, IAppRolesDispatcherTrait, ICommonRolesDispatcher, + ICommonRolesDispatcherTrait, ICommonRolesSafeDispatcher, ICommonRolesSafeDispatcherTrait, + IGovernanceRolesDispatcher, IGovernanceRolesDispatcherTrait, IRolesDispatcher, + IRolesDispatcherTrait, IRolesSafeDispatcher, IRolesSafeDispatcherTrait, + ISecurityRolesDispatcher, ISecurityRolesDispatcherTrait, Role, }; use openzeppelin::access::accesscontrol::AccessControlComponent::Errors as OZAccessErrors; use roles::{event_test_utils, interface}; @@ -909,6 +911,21 @@ fn test_remove_security_admin() { assert!(!roles_dispatcher.is_security_admin(account: security_admin)); } +#[feature("safe_dispatcher")] +#[test] +fn test_remove_security_admin_self_panics() { + // A security admin cannot remove themselves via remove_security_admin. + let contract_address = test_utils::deploy_mock_contract(); + let roles_safe_dispatcher = IRolesSafeDispatcher { contract_address }; + let security_admin = constants::INITIAL_ROOT_ADMIN; + + cheat_caller_address_once(:contract_address, caller_address: security_admin); + let result = roles_safe_dispatcher.remove_security_admin(account: security_admin); + assert_panic_with_error( + :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), + ); +} + #[feature("safe_dispatcher")] #[test] @@ -916,14 +933,15 @@ fn test_renounce() { // Deploy mock contract. let contract_address = test_utils::deploy_mock_contract(); let roles_dispatcher = IRolesDispatcher { contract_address }; - let roles_safe_dispatcher = IRolesSafeDispatcher { contract_address }; + let common_roles_dispatcher = ICommonRolesDispatcher { contract_address }; + let common_roles_safe_dispatcher = ICommonRolesSafeDispatcher { contract_address }; let governance_admin = constants::INITIAL_ROOT_ADMIN; let app_role_admin = constants::APP_ROLE_ADMIN; // Try to renounce governance admin. // Note: the caller doesn't have to be a governance admin for this error as it's checked first. cheat_caller_address_once(:contract_address, caller_address: governance_admin); - let result = roles_safe_dispatcher.renounce(role: interface::GOVERNANCE_ADMIN); + let result = common_roles_safe_dispatcher.renounce(role: Role::GovernanceAdmin); assert_panic_with_error( :result, expected_error: AccessErrors::ROLE_CANNOT_BE_RENOUNCED.describe(), ); @@ -931,7 +949,7 @@ fn test_renounce() { // Renounce role without being registered. let mut spy = snforge_std::spy_events(); cheat_caller_address_once(:contract_address, caller_address: app_role_admin); - roles_dispatcher.renounce(role: interface::APP_ROLE_ADMIN); + common_roles_dispatcher.renounce(role: Role::AppRoleAdmin); let events = spy.get_events().emitted_by(:contract_address).events; assert_number_of_events( actual: events.len(), expected: 0, message: "test_remove_security_admin second", @@ -944,7 +962,7 @@ fn test_renounce() { // Renounce app role admin. let mut spy = snforge_std::spy_events(); cheat_caller_address_once(:contract_address, caller_address: app_role_admin); - roles_dispatcher.renounce(role: interface::APP_ROLE_ADMIN); + common_roles_dispatcher.renounce(role: Role::AppRoleAdmin); let events = spy.get_events().emitted_by(:contract_address).events; // We don't assert any specific event, because the only event emitted is by accesss control. @@ -953,8 +971,7 @@ fn test_renounce() { ); } -// ─── Category interface parity tests -// ───────────────────────────────────────── +// ─── Category interface parity tests ───── // // These tests verify that dispatching via a category interface (e.g. IUpgradeRoles) // against a contract that embeds RolesComponent produces identical results to From 80181c35add383d64f98d9b58d751c2c8ef2bbfd Mon Sep 17 00:00:00 2001 From: Remo Date: Thu, 19 Mar 2026 03:15:38 +0200 Subject: [PATCH 3/3] In code comments etc. --- .../common_roles/common_roles.cairo | 55 +++++++++++++++++-- .../utils/src/components/common_roles/spec.md | 4 ++ .../utils/src/components/roles/roles.cairo | 30 +++++++++- 3 files changed, 82 insertions(+), 7 deletions(-) diff --git a/packages/utils/src/components/common_roles/common_roles.cairo b/packages/utils/src/components/common_roles/common_roles.cairo index 6e03f584..27e2df63 100644 --- a/packages/utils/src/components/common_roles/common_roles.cairo +++ b/packages/utils/src/components/common_roles/common_roles.cairo @@ -18,6 +18,10 @@ pub(crate) mod CommonRolesComponent { UPGRADE_GOVERNOR, is_renounceable, }; + // Maps each role to its admin role — used both at initialization (to configure OZ + // AccessControl) + // and during legacy role reclaim (to ensure admin pairs are set on upgraded contracts). + // `GOVERNANCE_ADMIN` and `SECURITY_ADMIN` are self-administered (admin == role). pub const ROLE_ADMIN_PAIRS: [(RoleId, RoleId); 10] = [ (APP_GOVERNOR, APP_ROLE_ADMIN), (APP_ROLE_ADMIN, GOVERNANCE_ADMIN), (GOVERNANCE_ADMIN, GOVERNANCE_ADMIN), (OPERATOR, APP_ROLE_ADMIN), @@ -28,10 +32,14 @@ pub(crate) mod CommonRolesComponent { #[storage] pub struct Storage { - // LEGACY: Old role-membership map from before OZ AccessControl integration. - // Kept to allow one-time reclaim migration. Read-only after initialize(). + // LEGACY: Role-membership map from before OZ AccessControl integration. + // Written only by the legacy reclaim path (_reclaim_role erases each entry as it migrates + // it to OZ storage). Private — test mocks access it via the #[rename] storage-collision + // trick rather than requiring pub visibility on production code. role_members: starknet::storage::Map<(RoleId, ContractAddress), bool>, - // Once set, legacy reclaim is permanently disabled. + // Guards the one-time reclaim window. Fresh contracts set this to `true` in `initialize`; + // legacy-upgraded contracts leave it `false` until the upgrade is complete and + // `disable_legacy_role_reclaim` is called by an upgrade governor. legacy_role_reclaim_disabled: bool, } @@ -39,6 +47,12 @@ pub(crate) mod CommonRolesComponent { #[derive(Drop, starknet::Event)] pub enum Event {} + // ─── Public ABI (ICommonRoles) + // ──────────────────────────────────────────── + // These 7 entry points are the minimal role-management surface. They are the + // only role-related entry points on Tier-A contracts (CommonRolesComponent + // only), and they complement the named-role entry points on Tier-B/C contracts + // (RolesComponent adds IRoles / category interfaces on top). #[embeddable_as(CommonRolesImpl)] pub impl CommonRoles< TContractState, @@ -66,18 +80,26 @@ pub(crate) mod CommonRolesComponent { } fn renounce(ref self: ComponentState, role: Role) { + // GOVERNANCE_ADMIN and SECURITY_ADMIN are non-renounceable — self-removal of those + // roles would leave the contract permanently ungovernable / un-paused. assert!(is_renounceable(role), "{}", AccessErrors::ROLE_CANNOT_BE_RENOUNCED); let role_id: RoleId = role.into(); let mut access_comp = get_dep_component_mut!(ref self, Access); access_comp.renounce_role(role: role_id, account: get_caller_address()); } + // Allows the caller to migrate their own roles from legacy storage to OZ AccessControl. + // Also calls ensure_role_admins in case this is the first account to reclaim on a + // contract that was upgraded without going through initialize(). fn reclaim_legacy_roles(ref self: ComponentState) { InternalTrait::assert_role_reclaim_enabled(@self); InternalTrait::_reclaim_legacy_roles_for_account(ref self, get_caller_address()); InternalTrait::ensure_role_admins(ref self); } + // Batch version gated behind SECURITY_GOVERNOR — intended for the upgrade governor to + // migrate multiple accounts in one tx. ensure_role_admins is not called here because the + // upgrade constructor always calls it before any account can reach this path. fn reclaim_legacy_roles_for_accounts( ref self: ComponentState, accounts: Span, ) { @@ -88,12 +110,16 @@ pub(crate) mod CommonRolesComponent { }; } + // Permanently closes the reclaim window. Idempotent. Called by an upgrade governor + // once all legacy roles have been migrated. fn disable_legacy_role_reclaim(ref self: ComponentState) { InternalTrait::only_upgrade_governor(@self); self.legacy_role_reclaim_disabled.write(true); } } + // ─── Internal helpers + // ───────────────────────────────────────────────────── #[generate_trait] pub impl InternalImpl< TContractState, @@ -103,9 +129,14 @@ pub(crate) mod CommonRolesComponent { +SRC5Component::HasComponent, > of InternalTrait { // WARNING - // Unprotected — call only from constructor (or test setup). + // Unprotected — call only from a constructor (or test setup). + // Sets up OZ AccessControl (initializer + role admin pairs), grants GOVERNANCE_ADMIN and + // SECURITY_ADMIN to `governance_admin`, then immediately disables legacy role reclaim + // (fresh contracts have no legacy storage to migrate). fn initialize(ref self: ComponentState, governance_admin: ContractAddress) { let mut access_comp = get_dep_component_mut!(ref self, Access); + // Detect double-initialization: if GOVERNANCE_ADMIN already has an admin it means + // initialize() was already called on this deployment. let uninitialized = access_comp.get_role_admin(role: GOVERNANCE_ADMIN).is_zero(); assert!(uninitialized, "{}", AccessErrors::ALREADY_INITIALIZED); access_comp.initializer(); @@ -117,6 +148,8 @@ pub(crate) mod CommonRolesComponent { self.legacy_role_reclaim_disabled.write(true); } + // Idempotent: only sets admin pairs whose current admin is zero. This lets it be called + // on both fresh deployments and upgrades without overwriting intentional customization. fn _set_role_admins( ref access_comp: AccessControlComponent::ComponentState, ) { @@ -145,6 +178,8 @@ pub(crate) mod CommonRolesComponent { } /// Grants `role` to `account`. Enforces caller is the role's admin (via OZ grant_role). + /// Rejects the zero address — granting a role to zero is always a mistake and can make + /// the role permanently inaccessible if it is self-administered (e.g., GOVERNANCE_ADMIN). fn grant_role( ref self: ComponentState, role: Role, account: ContractAddress, ) { @@ -169,6 +204,11 @@ pub(crate) mod CommonRolesComponent { access_comp.revoke_role(:role, :account); } + // ─── Role guards + // ───────────────────────────────────────────────────── + // These are the canonical authorization checks. Both `CommonRolesComponent` and + // `RolesComponent` expose them as internal methods so sibling components can call + // them without going through the public ABI. fn only_role(self: @ComponentState, role: Role) { let role_id: RoleId = role.into(); let access_comp = get_dep_component!(self, Access); @@ -261,12 +301,17 @@ pub(crate) mod CommonRolesComponent { ); } + // NOTE: parameter order is (account, role) but the storage key is (role, account) — + // intentional, but keep in mind when reading call sites. fn _has_legacy_role( self: @ComponentState, account: ContractAddress, role: RoleId, ) -> bool { self.role_members.read((role, account)) } + // Migrates a single (role, account) entry: clears the legacy map entry and grants + // the role in OZ AccessControl. No-op if the account doesn't hold the role in legacy + // storage — safe to call unconditionally for all roles. fn _reclaim_role( ref self: ComponentState, role: RoleId, account: ContractAddress, ) { @@ -277,6 +322,8 @@ pub(crate) mod CommonRolesComponent { } } + // Iterates all 10 known roles for one account. O(10) regardless of which roles the + // account actually holds — acceptable for a one-time migration path. fn _reclaim_legacy_roles_for_account( ref self: ComponentState, account: ContractAddress, ) { diff --git a/packages/utils/src/components/common_roles/spec.md b/packages/utils/src/components/common_roles/spec.md index f49d81c7..9e3c8b1d 100644 --- a/packages/utils/src/components/common_roles/spec.md +++ b/packages/utils/src/components/common_roles/spec.md @@ -7,6 +7,10 @@ The role system is split into two layers: - **`CommonRolesComponent`** — lean infrastructure. Owns the role constants, the `Role` enum, role admin configuration, role guards (`only_X`), a 7-method ABI (`ICommonRoles`: `grant_role`, `revoke_role`, `has_role`, `renounce`, and the three legacy reclaim entry points), and the legacy reclaim storage (`role_members` map + `legacy_role_reclaim_disabled` flag). - **`RolesComponent`** — full-featured layer. Delegates all role state to `CommonRolesComponent` internally. Adds named role events (20 variants) and category-scoped embeddable implementations (`IGovernanceRoles`, `ISecurityRoles`, `IAppRoles`), and the fat `IRoles` interface (~30 EPs). +### Role wire format + +`Role` serializes as its corresponding `RoleId` felt (the keccak selector constant, e.g. `GOVERNANCE_ADMIN = 0x371…`), **not** as a zero-based variant index. This makes calldata self-documenting, stable across enum reordering, and backward-compatible with the felt values already in use by operators and indexers. + The design lets contracts pick exactly the role surface they need — from zero ABI overhead up to the full named interface — without duplicating any role logic. --- diff --git a/packages/utils/src/components/roles/roles.cairo b/packages/utils/src/components/roles/roles.cairo index b7108b0b..a1c97117 100644 --- a/packages/utils/src/components/roles/roles.cairo +++ b/packages/utils/src/components/roles/roles.cairo @@ -45,6 +45,13 @@ pub(crate) mod RolesComponent { UpgradeAgentRemoved: UpgradeAgentRemoved, } + // ─── IRoles (full 30-EP interface) + // ──────────────────────────────────────── + // Each method either reads from or delegates writes to CommonRolesComponent. + // Reads go through `get_dep_component!(self, CommonRoles).has_role(...)`. + // Writes go through `_register_role` / `_remove_role` (see InternalTrait below), + // which call `CommonRolesComponent::InternalTrait::grant_role` / `revoke_role` + // and emit the corresponding named event only on an actual state change. #[embeddable_as(RolesImpl)] pub impl Roles< TContractState, @@ -401,7 +408,11 @@ pub(crate) mod RolesComponent { } } - // ─── Category-scoped role impls ──────────── + // ─── Category-scoped implementations + // ───────────────────────────────────── + // These three embeddable implementations cover ISecurityRoles / IGovernanceRoles / + // IAppRoles. A contract that does not need all 30 IRoles entry points can embed just + // the category implementation it needs instead of RolesImpl. #[embeddable_as(SecurityRolesImpl)] pub impl SecurityRoles< @@ -779,6 +790,8 @@ pub(crate) mod RolesComponent { } } + // ─── Internal helpers + // ───────────────────────────────────────────────────── #[generate_trait] pub impl RolesInternalImpl< TContractState, @@ -788,6 +801,9 @@ pub(crate) mod RolesComponent { impl CommonRoles: CommonRolesComponent::HasComponent, +SRC5Component::HasComponent, > of InternalTrait { + // Idempotent grant: no-op (and no event) if `account` already holds `role`. + // The caller constructs the event before calling, so the only branching needed here + // is the early-return guard — avoids closures or runtime event dispatch. fn _register_role( ref self: ComponentState, role: Role, @@ -802,6 +818,9 @@ pub(crate) mod RolesComponent { self.emit(event); } + // Idempotent revoke: no-op (and no event) if `account` does not hold `role`. + // Authorization (caller must be role admin) is enforced inside + // `CommonRolesComponent::InternalTrait::revoke_role` via OZ AccessControl. fn _remove_role( ref self: ComponentState, role: Role, @@ -817,13 +836,18 @@ pub(crate) mod RolesComponent { } // WARNING - // The following internal method is unprotected and should only be used from the containing - // contract's constructor (or, in context of tests, from the setup method). + // Unprotected — call only from a constructor (or test setup). + // Thin wrapper that forwards to CommonRolesComponent::initialize, which seeds + // GOVERNANCE_ADMIN and SECURITY_ADMIN and sets all role admin pairs. fn initialize(ref self: ComponentState, governance_admin: ContractAddress) { let mut common_roles = get_dep_component_mut!(ref self, CommonRoles); common_roles.initialize(:governance_admin); } + // ─── Role guards (delegates to CommonRolesComponent) + // ───────────────── + // Embeddable methods on a sibling component are not directly callable via + // `get_dep_component!`, so each guard is re-exposed here as a thin wrapper. fn only_app_governor(self: @ComponentState) { get_dep_component!(self, CommonRoles).only_app_governor(); }