Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional delay to AccessControl role grants #1317

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/access/src/accesscontrol.cairo
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pub mod accesscontrol;
pub mod account_role_info;
pub mod interface;

pub use accesscontrol::AccessControlComponent;

pub const DEFAULT_ADMIN_ROLE: felt252 = 0;
pub use accesscontrol::AccessControlComponent::DEFAULT_ADMIN_ROLE;
132 changes: 118 additions & 14 deletions packages/access/src/accesscontrol/accesscontrol.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,27 @@
/// Extra precautions should be taken to secure accounts with this role.
#[starknet::component]
pub mod AccessControlComponent {
use crate::accesscontrol::account_role_info::AccountRoleInfo;
use crate::accesscontrol::interface;
use openzeppelin_introspection::src5::SRC5Component;
use openzeppelin_introspection::src5::SRC5Component::InternalImpl as SRC5InternalImpl;
use openzeppelin_introspection::src5::SRC5Component::SRC5Impl;
use starknet::ContractAddress;
use starknet::get_caller_address;
use starknet::storage::{Map, StorageMapReadAccess, StorageMapWriteAccess};

pub const DEFAULT_ADMIN_ROLE: felt252 = 0;

#[storage]
pub struct Storage {
pub AccessControl_role_admin: Map<felt252, felt252>,
pub AccessControl_role_member: Map<(felt252, ContractAddress), bool>,
pub AccessControl_role_member: Map<(felt252, ContractAddress), AccountRoleInfo>,
}

#[event]
#[derive(Drop, PartialEq, starknet::Event)]
pub enum Event {
RoleGranted: RoleGranted,
RoleGrantedWithDelay: RoleGrantedWithDelay,
RoleRevoked: RoleRevoked,
RoleAdminChanged: RoleAdminChanged,
}
Expand All @@ -51,6 +54,18 @@ pub mod AccessControlComponent {
pub sender: ContractAddress,
}

/// Emitted when `account` is granted `role` with a delay.
///
/// `sender` is the account that originated the contract call, an account with the admin role
/// or the deployer address if `grant_role` is called from the constructor.
#[derive(Drop, PartialEq, starknet::Event)]
pub struct RoleGrantedWithDelay {
pub role: felt252,
pub account: ContractAddress,
pub sender: ContractAddress,
pub delay: u64,
}

/// Emitted when `role` is revoked for `account`.
///
/// `sender` is the account that originated the contract call:
Expand All @@ -77,6 +92,7 @@ pub mod AccessControlComponent {
pub mod Errors {
pub const INVALID_CALLER: felt252 = 'Can only renounce role for self';
pub const MISSING_ROLE: felt252 = 'Caller is missing role';
pub const INVALID_DELAY: felt252 = 'Delay must be greater than 0';
}

#[embeddable_as(AccessControlImpl)]
Expand All @@ -86,11 +102,11 @@ pub mod AccessControlComponent {
+SRC5Component::HasComponent<TContractState>,
+Drop<TContractState>,
> of interface::IAccessControl<ComponentState<TContractState>> {
/// Returns whether `account` has been granted `role`.
/// Returns whether `account` can act as `role`.
fn has_role(
self: @ComponentState<TContractState>, role: felt252, account: ContractAddress,
) -> bool {
self.AccessControl_role_member.read((role, account))
self.is_role_effective(role, account)
}

/// Returns the admin role that controls `role`.
Expand Down Expand Up @@ -143,7 +159,7 @@ pub mod AccessControlComponent {
fn renounce_role(
ref self: ComponentState<TContractState>, role: felt252, account: ContractAddress,
) {
let caller = get_caller_address();
let caller = starknet::get_caller_address();
assert(caller == account, Errors::INVALID_CALLER);
self._revoke_role(role, account);
}
Expand Down Expand Up @@ -186,6 +202,33 @@ pub mod AccessControlComponent {
}
}

#[embeddable_as(AccessControlWithDelayImpl)]
impl AccessControlWithDelay<
TContractState,
+HasComponent<TContractState>,
+SRC5Component::HasComponent<TContractState>,
+Drop<TContractState>,
> of interface::IAccessControlWithDelay<ComponentState<TContractState>> {
/// Grants `role` to `account` with a delay.
///
/// If `account` has not been already granted `role`, emits a `RoleGranted` event.
///
/// Requirements:
///
/// - The caller must have `role`'s admin role.
/// - delay must be greater than 0.
fn grant_role_with_delay(
ref self: ComponentState<TContractState>,
role: felt252,
account: ContractAddress,
delay: u64,
) {
let admin = AccessControl::get_role_admin(@self, role);
self.assert_only_role(admin);
self._grant_role_with_delay(role, account, delay);
}
}

#[generate_trait]
pub impl InternalImpl<
TContractState,
Expand All @@ -199,13 +242,51 @@ pub mod AccessControlComponent {
src5_component.register_interface(interface::IACCESSCONTROL_ID);
}

/// Validates that the caller has the given role. Otherwise it panics.
/// Validates that the caller can act as the given role. Otherwise it panics.
fn assert_only_role(self: @ComponentState<TContractState>, role: felt252) {
let caller: ContractAddress = get_caller_address();
let authorized = AccessControl::has_role(self, role, caller);
let caller = starknet::get_caller_address();
let authorized = self.is_role_effective(role, caller);
assert(authorized, Errors::MISSING_ROLE);
}

/// Returns whether the account can act as the given role.
///
/// The account can act as the role if it is active and the effective from time is before
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The account can act as the role if it is active and the effective from time is before
/// The account can act as the role if it is active and the AccountRoleInfo::effective_from time is before

I think this reads a little easier. It wasn't obvious at first what "effective from time" meant

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even something like effective_from

/// the current time.
///
/// NOTE: If the effective from timepoint is 0, the role is effective immediately.
/// This is backwards compatible with implementations that didn't use delays but
/// a single boolean flag.
fn is_role_effective(
self: @ComponentState<TContractState>, role: felt252, account: ContractAddress,
) -> bool {
let AccountRoleInfo {
is_active, effective_from,
} = self.AccessControl_role_member.read((role, account));

if is_active {
if effective_from == 0 {
true
} else {
let now = starknet::get_block_timestamp();
effective_from <= now
}
} else {
false
}
}

/// Returns whether the account has the given role granted.
///
/// NOTE: The account may not be able to act as the role yet, if a delay was set and has not
/// passed yet. Use `is_role_effective` to check if the account can act as the role.
fn is_role_granted(
self: @ComponentState<TContractState>, role: felt252, account: ContractAddress,
) -> bool {
let account_role_info = self.AccessControl_role_member.read((role, account));
account_role_info.is_active
}

/// Sets `admin_role` as `role`'s admin role.
///
/// Internal function without access restriction.
Expand All @@ -227,13 +308,35 @@ pub mod AccessControlComponent {
fn _grant_role(
ref self: ComponentState<TContractState>, role: felt252, account: ContractAddress,
) {
if !AccessControl::has_role(@self, role, account) {
let caller: ContractAddress = get_caller_address();
self.AccessControl_role_member.write((role, account), true);
if !self.is_role_granted(role, account) {
let caller = starknet::get_caller_address();
let role_info = AccountRoleInfo { is_active: true, effective_from: 0 };
self.AccessControl_role_member.write((role, account), role_info);
self.emit(RoleGranted { role, account, sender: caller });
}
}

/// Attempts to grant `role` to `account` that will become effective when `delay` passes.
///
/// Internal function without access restriction.
///
/// May emit a `RoleGrantedWithDelay` event.
fn _grant_role_with_delay(
ref self: ComponentState<TContractState>,
role: felt252,
account: ContractAddress,
delay: u64,
) {
assert(delay > 0, Errors::INVALID_DELAY);
if !self.is_role_granted(role, account) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now users may want to use the grant_role_function to update a delay, or the grant_role function to remove the delay, these functions silently passing may confuse them. What do you think about adding an error on the else condition stating that the user already has the role?

let caller = starknet::get_caller_address();
let effective_from = starknet::get_block_timestamp() + delay;
let role_info = AccountRoleInfo { is_active: true, effective_from };
self.AccessControl_role_member.write((role, account), role_info);
self.emit(RoleGrantedWithDelay { role, account, sender: caller, delay });
}
}

/// Attempts to revoke `role` from `account`.
///
/// Internal function without access restriction.
Expand All @@ -242,9 +345,10 @@ pub mod AccessControlComponent {
fn _revoke_role(
ref self: ComponentState<TContractState>, role: felt252, account: ContractAddress,
) {
if AccessControl::has_role(@self, role, account) {
let caller: ContractAddress = get_caller_address();
self.AccessControl_role_member.write((role, account), false);
if self.is_role_granted(role, account) {
let caller = starknet::get_caller_address();
let account_role_info = AccountRoleInfo { is_active: false, effective_from: 0 };
self.AccessControl_role_member.write((role, account), account_role_info);
self.emit(RoleRevoked { role, account, sender: caller });
}
}
Expand Down
64 changes: 64 additions & 0 deletions packages/access/src/accesscontrol/account_role_info.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts for Cairo v0.20.0 (access/accesscontrol/account_role_info.cairo)

use starknet::storage_access::StorePacking;

/// Information about whether a role is active for an account or not.
#[derive(Copy, Drop, Serde, PartialEq, Debug)]
pub struct AccountRoleInfo {
pub effective_from: u64,
pub is_active: bool,
}

/// Packs an AccountRoleInfo into a single felt252.
///
/// The packing is done as follows:
///
/// 1. `effective_from` is stored at range [187,250] (0-indexed starting from the most
/// significant bits).
/// 2. `is_active` is stored at range [251, 251], following `effective_from`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

impl AccountRoleInfoStorePacking of StorePacking<AccountRoleInfo, felt252> {
fn pack(value: AccountRoleInfo) -> felt252 {
let AccountRoleInfo { effective_from, is_active } = value;

// shift-left to reach the corresponding positions
let effective_from_val = effective_from.into() * 2;
let is_active_val = if is_active {
1
} else {
0
};

effective_from_val + is_active_val
}

fn unpack(value: felt252) -> AccountRoleInfo {
let value: u256 = value.into();
let effective_from = value / 2;
let is_active = value % 2 == 1;

AccountRoleInfo { effective_from: effective_from.try_into().unwrap(), is_active }
}
}

#[cfg(test)]
mod tests {
use core::num::traits::Bounded;
use super::{AccountRoleInfo, AccountRoleInfoStorePacking};

#[test]
fn test_pack_and_unpack() {
let account_role_info = AccountRoleInfo { effective_from: 100, is_active: true };
let packed = AccountRoleInfoStorePacking::pack(account_role_info);
let unpacked = AccountRoleInfoStorePacking::unpack(packed);
assert_eq!(account_role_info, unpacked);
}

#[test]
fn test_pack_and_unpack_big_values() {
let account_role_info = AccountRoleInfo { effective_from: Bounded::MAX, is_active: true };
let packed = AccountRoleInfoStorePacking::pack(account_role_info);
let unpacked = AccountRoleInfoStorePacking::unpack(packed);
assert_eq!(account_role_info, unpacked);
}
}
5 changes: 5 additions & 0 deletions packages/access/src/accesscontrol/interface.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ pub trait IAccessControl<TState> {
fn renounce_role(ref self: TState, role: felt252, account: ContractAddress);
}

#[starknet::interface]
pub trait IAccessControlWithDelay<TState> {
fn grant_role_with_delay(ref self: TState, role: felt252, account: ContractAddress, delay: u64);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add this to the ABI, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I wonder if there's some value in having a new src5 id for this. I really don't think it's worth it, but I'm asking out loud just in case


#[starknet::interface]
pub trait IAccessControlCamel<TState> {
fn hasRole(self: @TState, role: felt252, account: ContractAddress) -> bool;
Expand Down
4 changes: 4 additions & 0 deletions packages/test_common/src/mocks/access.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ pub mod DualCaseAccessControlMock {
#[abi(embed_v0)]
impl AccessControlMixinImpl =
AccessControlComponent::AccessControlMixinImpl<ContractState>;
// AccessControlWithDelay
#[abi(embed_v0)]
impl AccessControlWithDelayImpl =
AccessControlComponent::AccessControlWithDelayImpl<ContractState>;
impl AccessControlInternalImpl = AccessControlComponent::InternalImpl<ContractState>;

#[storage]
Expand Down