From ac48e57e82a5f607a45f98f8e481f12ad561fd2a Mon Sep 17 00:00:00 2001 From: Alex Metelli Date: Mon, 25 Sep 2023 15:26:55 -0700 Subject: [PATCH] Feat: Completed Implementation of `role_store` contract. (#443) completed role_store + tests Co-authored-by: zarboq <37303126+zarboq@users.noreply.github.com> --- src/role/role_store.cairo | 249 +++++++++++++++++++-------- src/tests/role/test_role_store.cairo | 217 ++++++++++++----------- 2 files changed, 285 insertions(+), 181 deletions(-) diff --git a/src/role/role_store.cairo b/src/role/role_store.cairo index 7ee4f6a7..eb78928a 100644 --- a/src/role/role_store.cairo +++ b/src/role/role_store.cairo @@ -43,32 +43,30 @@ trait IRoleStore { /// Returns the number of roles stored in the contract. /// # Return /// The number of roles. - fn get_role_count(self: @TContractState) -> u128; -/// # [TO FIX] -/// Returns the keys of the roles stored in the contract. -/// # Arguments -/// `start` - The starting index of the range of roles to return. -/// `end` - The ending index of the range of roles to return. -/// # Return -/// The keys of the roles. -/// fn get_roles(self: @TContractState, start: u32, end: u32) -> Array; - -/// # [TO DO] -/// Returns the number of members of the specified role. -/// # Arguments -/// `role_key` - The key of the role. -/// # Return -/// The number of members of the role. -/// fn get_role_member_count(self: @TContractState, role_key: felt252) -> u128; - -/// Returns the members of the specified role. -/// # Arguments -/// `role_key` - The key of the role. -/// `start` - The start index, the value for this index will be included. -/// `end` - The end index, the value for this index will not be included. -/// # Return -/// The members of the role. -/// fn get_role_members(self: @TContractState, role_key: felt252, start: u128, end: u128) -> Array; + fn get_role_count(self: @TContractState) -> u32; + /// Returns the keys of the roles stored in the contract. + /// # Arguments + /// `start` - The starting index of the range of roles to return. + /// `end` - The ending index of the range of roles to return. + /// # Return + /// The keys of the roles. + fn get_roles(self: @TContractState, start: u32, end: u32) -> Array; + /// Returns the number of members of the specified role. + /// # Arguments + /// `role_key` - The key of the role. + /// # Return + /// The number of members of the role. + fn get_role_member_count(self: @TContractState, role_key: felt252) -> u32; + /// Returns the members of the specified role. + /// # Arguments + /// `role_key` - The key of the role. + /// `start` - The start index, the value for this index will be included. + /// `end` - The end index, the value for this index will not be included. + /// # Return + /// The members of the role. + fn get_role_members( + self: @TContractState, role_key: felt252, start: u32, end: u32 + ) -> Array; } #[starknet::contract] @@ -78,8 +76,8 @@ mod RoleStore { // ************************************************************************* // Core lib imports. - use starknet::{ContractAddress, get_caller_address}; - //use array::ArrayTrait; + use core::zeroable::Zeroable; + use starknet::{ContractAddress, get_caller_address, contract_address_const}; // Local imports. use satoru::role::{role, error::RoleError}; @@ -90,13 +88,17 @@ mod RoleStore { #[storage] struct Storage { /// Maps accounts to their roles. - role_members: LegacyMap::<(felt252, ContractAddress), bool>, + has_role: LegacyMap::<(felt252, ContractAddress), bool>, + /// Stores the number of the indexes used to a specific role. + role_members_count: LegacyMap::, + /// Stores all the account that have a specific role. + role_members: LegacyMap::<(felt252, u32), ContractAddress>, /// Stores unique role names. role_names: LegacyMap::, - /// Store the number of unique roles. - role_count: u128, - /// List of all role keys. - ///role_keys: Array, + /// Store the number of indexes of the roles. + roles_count: u32, + /// List of all role keys. + roles: LegacyMap::, } // ************************************************************************* @@ -140,8 +142,7 @@ mod RoleStore { let caller = get_caller_address(); // Grant the caller admin role. self._grant_role(caller, role::ROLE_ADMIN); - // Initialize the role_count to 1 due to the line just above. - self.role_count.write(1); + // Initialize the role_count to 1 due to the line just above. } // ************************************************************************* @@ -154,17 +155,15 @@ mod RoleStore { } fn grant_role(ref self: ContractState, account: ContractAddress, role_key: felt252) { - let caller = get_caller_address(); // Check that the caller has the admin role. - self._assert_only_role(caller, role::ROLE_ADMIN); + self._assert_only_role(get_caller_address(), role::ROLE_ADMIN); // Grant the role. self._grant_role(account, role_key); } fn revoke_role(ref self: ContractState, account: ContractAddress, role_key: felt252) { - let caller = get_caller_address(); // Check that the caller has the admin role. - self._assert_only_role(caller, role::ROLE_ADMIN); + self._assert_only_role(get_caller_address(), role::ROLE_ADMIN); // Revoke the role. self._revoke_role(account, role_key); } @@ -173,29 +172,78 @@ mod RoleStore { self._assert_only_role(account, role_key); } - fn get_role_count(self: @ContractState) -> u128 { - return self.role_count.read(); + fn get_role_count(self: @ContractState) -> u32 { + let mut count = 0; + let mut i = 1; + loop { + if i > self.roles_count.read() { + break; + } + if !self.roles.read(i).is_zero() { + count += 1; + } + i += 1; + }; + count + } + + fn get_roles(self: @ContractState, start: u32, mut end: u32) -> Array { + let mut arr = array![]; + let roles_count = self.roles_count.read(); + if end > roles_count { + end = roles_count; + } + let mut i = start; + loop { + if i > end { + break; + } + let role = self.roles.read(i); + if !role.is_zero() { + arr.append(role); + } + i += 1; + }; + arr + } + + fn get_role_member_count(self: @ContractState, role_key: felt252) -> u32 { + let mut count = 0; + let mut i = 1; + loop { + if i > self.role_members_count.read(role_key) { + break; + } + if !(self.role_members.read((role_key, i)) == contract_address_const::<0>()) { + count += 1; + } + i += 1; + }; + count + } + + fn get_role_members( + self: @ContractState, role_key: felt252, start: u32, mut end: u32 + ) -> Array { + let mut arr: Array = array![]; + let mut i = start; + loop { + if i > end || i > self.role_members_count.read(role_key) { + break; + } + let role_member = self.role_members.read((role_key, i)); + // Since some role members will have indexes with zero address if a zero address + // is found end increase by 1 to mock array behaviour. + if role_member.is_zero() { + end += 1; + } + if !(role_member == contract_address_const::<0>()) { + arr.append(role_member); + } + i += 1; + }; + arr } - //fn get_roles(self: @ContractState, start: u32, end: u32) -> Array { - // Create a new array to store the result. - //let mut result = ArrayTrait::::new(); - //let role_keys_length = self.role_keys.read().len(); - // Ensure the range is valid. - //assert(start < end, "InvalidRange"); - //assert(end <= role_keys_length, "EndOutOfBounds"); - //let mut current_index = start; - //loop { - // Check if we've reached the end of the specified range. - //if current_index >= end { - //break; - //} - //let key = *self.role_keys.read().at(current_index); - //result.append(key); - // Increment the index. - //current_index += 1; - //}; - //return result; - //} } // ************************************************************************* @@ -205,7 +253,7 @@ mod RoleStore { impl InternalFunctions of InternalFunctionsTrait { #[inline(always)] fn _has_role(self: @ContractState, account: ContractAddress, role_key: felt252) -> bool { - self.role_members.read((role_key, account)) + self.has_role.read((role_key, account)) } #[inline(always)] @@ -216,31 +264,80 @@ mod RoleStore { fn _grant_role(ref self: ContractState, account: ContractAddress, role_key: felt252) { // Only grant the role if the account doesn't already have it. if !self._has_role(account, role_key) { - let caller: ContractAddress = get_caller_address(); - self.role_members.write((role_key, account), true); + self.has_role.write((role_key, account), true); + // Iterates through indexes for role members, if an index has zero ContractAddress + // it writes the account to that index for the role. + let roles_members_count = self.role_members_count.read(role_key); + let current_roles_count = self.roles_count.read(); + let mut i = 1; + loop { + let stored_role_member = self.role_members.read((role_key, i)); + if stored_role_member.is_zero() { + self.role_members.write((role_key, i), account); + self.role_members_count.write(role_key, roles_members_count + 1); + break; + } + i += 1; + }; + // Store the role name if it's not already stored. if self.role_names.read(role_key) == false { self.role_names.write(role_key, true); - let mut current_count: u128 = self.role_count.read(); - self.role_count.write(current_count + 1); - // Read the current state of role_keys into a local variable. - // let mut local_role_keys = self.role_keys.read(); - // Modify the local variable. - // local_role_keys.append(role_key); - // Write back the modified local variable to the contract state. - // self.role_keys.write(local_role_keys); + self.roles_count.write(current_roles_count + 1); } - self.emit(RoleGranted { role_key, account, sender: caller }); + self.emit(RoleGranted { role_key, account, sender: get_caller_address() }); } + // Iterates through indexes in stored_roles and if a value for the index is zero + // it writes the role_key to that index. + let mut i = 1; + loop { + let stored_role = self.roles.read(i); + if stored_role.is_zero() { + self.roles.write(i, role_key); + break; + } + i += 1; + }; } fn _revoke_role(ref self: ContractState, account: ContractAddress, role_key: felt252) { + let current_roles_count = self.roles_count.read(); // Only revoke the role if the account has it. if self._has_role(account, role_key) { - let caller: ContractAddress = get_caller_address(); - self.role_members.write((role_key, account), false); - self.emit(RoleRevoked { role_key, account, sender: caller }); + self.has_role.write((role_key, account), false); + self.emit(RoleRevoked { role_key, account, sender: get_caller_address() }); + let current_role_members_count = self.role_members_count.read(role_key); + let mut i = 1; + loop { + let stored_role_member = self.role_members.read((role_key, i)); + if stored_role_member == account { + self.role_members.write((role_key, i), contract_address_const::<0>()); + break; + } + i += 1; + }; + // If the role has no members remove the role from roles. + if self.get_role_member_count(role_key).is_zero() { + let role_index = self._find_role_index(role_key); + self.roles.write(role_index, Zeroable::zero()); + } } } + + fn _find_role_index(ref self: ContractState, role_key: felt252) -> u32 { + let mut index = 0; + let mut i = 1; + loop { + if i > self.roles_count.read() { + break; + } + if self.roles.read(i) == role_key { + index = i; + break; + } + i += 1; + }; + index + } } } diff --git a/src/tests/role/test_role_store.cairo b/src/tests/role/test_role_store.cairo index 0fe91617..1e23d0b5 100644 --- a/src/tests/role/test_role_store.cairo +++ b/src/tests/role/test_role_store.cairo @@ -2,7 +2,7 @@ use result::ResultTrait; use traits::TryInto; use starknet::{ContractAddress, contract_address_const}; use starknet::Felt252TryIntoContractAddress; -use snforge_std::{declare, start_prank, ContractClassTrait}; +use snforge_std::{declare, start_prank, ContractClassTrait, PrintTrait}; //use array::ArrayTrait; use satoru::role::role::ROLE_ADMIN; @@ -12,158 +12,165 @@ use satoru::role::role_store::IRoleStoreDispatcher; use satoru::role::role_store::IRoleStoreDispatcherTrait; #[test] -fn given_normal_conditions_when_grant_role_then_works() { - // ********************************************************************************************* - // * SETUP * - // ********************************************************************************************* +fn given_normal_conditions_when_has_role_after_grant_then_works() { let role_store = setup(); - // ********************************************************************************************* - // * TEST LOGIC * - // ********************************************************************************************* // Use the address that has been used to deploy role_store. - let caller_address: ContractAddress = 0x101.try_into().unwrap(); - start_prank(role_store.contract_address, caller_address); - - let account_address: ContractAddress = contract_address_const::<1>(); + start_prank(role_store.contract_address, admin()); // Check that the account address does not have the admin role. - assert(!role_store.has_role(account_address, ROLE_ADMIN), 'Invalid role'); + assert(!role_store.has_role(account_1(), ROLE_ADMIN), 'Invalid role'); // Grant admin role to account address. - role_store.grant_role(account_address, ROLE_ADMIN); + role_store.grant_role(account_1(), ROLE_ADMIN); // Check that the account address has the admin role. - assert(role_store.has_role(account_address, ROLE_ADMIN), 'Invalid role'); - - // ********************************************************************************************* - // * TEARDOWN * - // ********************************************************************************************* - teardown(); + assert(role_store.has_role(account_1(), ROLE_ADMIN), 'Invalid role'); } #[test] -fn given_normal_conditions_when_revoke_role_then_works() { - // ********************************************************************************************* - // * SETUP * - // ********************************************************************************************* +fn given_normal_conditions_when_has_role_after_revoke_then_works() { let role_store = setup(); - // ********************************************************************************************* - // * TEST LOGIC * - // ********************************************************************************************* - // Use the address that has been used to deploy role_store. - let caller_address: ContractAddress = 0x101.try_into().unwrap(); - start_prank(role_store.contract_address, caller_address); - - let account_address: ContractAddress = contract_address_const::<1>(); + start_prank(role_store.contract_address, admin()); // Grant admin role to account address. - role_store.grant_role(account_address, ROLE_ADMIN); + role_store.grant_role(account_1(), ROLE_ADMIN); // Check that the account address has the admin role. - assert(role_store.has_role(account_address, ROLE_ADMIN), 'Invalid role'); + assert(role_store.has_role(account_1(), ROLE_ADMIN), 'Invalid role'); // Revoke admin role from account address. - role_store.revoke_role(account_address, ROLE_ADMIN); + role_store.revoke_role(account_1(), ROLE_ADMIN); // Check that the account address does not have the admin role. - assert(!role_store.has_role(account_address, ROLE_ADMIN), 'Invalid role'); - - // ********************************************************************************************* - // * TEARDOWN * - // ********************************************************************************************* - teardown(); + assert(!role_store.has_role(account_1(), ROLE_ADMIN), 'Invalid role'); } #[test] -fn test_get_role_count() { - // ********************************************************************************************* - // * SETUP * - // ********************************************************************************************* +fn given_normal_conditions_when_get_role_count_then_works() { let role_store = setup(); - // ********************************************************************************************* - // * TEST LOGIC * - // ********************************************************************************************* - // Use the address that has been used to deploy role_store. - let caller_address: ContractAddress = 0x101.try_into().unwrap(); - start_prank(role_store.contract_address, caller_address); - - let account_address: ContractAddress = contract_address_const::<1>(); + start_prank(role_store.contract_address, admin()); // Here, we will test the role count. Initially, it should be 1. assert(role_store.get_role_count() == 1, 'Initial role count should be 1'); // Grant CONTROLLER role to account address. - role_store.grant_role(account_address, CONTROLLER); + role_store.grant_role(account_1(), CONTROLLER); // After granting the role CONTROLLER, the count should be 2. assert(role_store.get_role_count() == 2, 'Role count should be 2'); // Grant MARKET_KEEPER role to account address. - role_store.grant_role(account_address, MARKET_KEEPER); + role_store.grant_role(account_1(), MARKET_KEEPER); // After granting the role MARKET_KEEPER, the count should be 3. assert(role_store.get_role_count() == 3, 'Role count should be 3'); // The ROLE_ADMIN role is already assigned, let's try to reassign it to see if duplicates are managed. // Grant ROLE_ADMIN role to account address. - role_store.grant_role(account_address, ROLE_ADMIN); + role_store.grant_role(account_1(), ROLE_ADMIN); // Duplicates, the count should be 3. assert(role_store.get_role_count() == 3, 'Role count should be 3'); - // ********************************************************************************************* - // * TEARDOWN * - // ********************************************************************************************* - teardown(); + // Revoke a MARKET_KEEPER role, since the role has now no members the roles count + // is decreased. + role_store.revoke_role(account_1(), MARKET_KEEPER); + assert(role_store.get_role_count() == 2, 'Role count should be 2'); +} + +#[test] +fn given_normal_conditions_when_get_roles_then_works() { + let role_store = setup(); + + // Use the address that has been used to deploy role_store. + start_prank(role_store.contract_address, admin()); + + // Grant CONTROLLER role to account address. + role_store.grant_role(account_1(), CONTROLLER); + + // Grant MARKET_KEEPER role to account address. + role_store.grant_role(account_1(), MARKET_KEEPER); + + // Get roles from index 1 to 2 (should return ROLE_ADMIN and CONTROLLER). + // Note: Starknet's storage starts at 1, for this reason storage index 0 will + // always be empty. + let roles_0_to_2 = role_store.get_roles(1, 2); + let first_role = roles_0_to_2.at(0); + let second_role = roles_0_to_2.at(1); + assert(*first_role == ROLE_ADMIN, '1 should be ROLE_ADMIN'); + assert(*second_role == CONTROLLER, '2 should be CONTROLLER'); + + // Get roles from index 2 to 3 (should return CONTROLLER and MARKET_KEEPER). + let roles_1_to_3 = role_store.get_roles(2, 3); + let first_role = roles_1_to_3.at(0); + let second_role = roles_1_to_3.at(1); + assert(*first_role == CONTROLLER, '3 should be CONTROLLER'); + assert(*second_role == MARKET_KEEPER, '4 should be MARKET_KEEPER'); +} + +#[test] +fn given_normal_conditions_when_get_role_member_count_then_works() { + let role_store = setup(); + + // Use the address that has been used to deploy role_store. + start_prank(role_store.contract_address, admin()); + // Grant CONTROLLER role to account address. + role_store.grant_role(account_1(), CONTROLLER); + role_store.grant_role(account_2(), CONTROLLER); + role_store.grant_role(account_3(), CONTROLLER); + + assert(role_store.get_role_member_count(CONTROLLER) == 3, 'members count != 3'); + + role_store.revoke_role(account_3(), CONTROLLER); + assert(role_store.get_role_member_count(CONTROLLER) == 2, 'members count != 2'); + + role_store.revoke_role(account_2(), CONTROLLER); + assert(role_store.get_role_member_count(CONTROLLER) == 1, 'members count != 1'); } -//#[test] -//fn test_get_roles() { -// ********************************************************************************************* -// * SETUP * -// ********************************************************************************************* -//let role_store = setup(); - -// ********************************************************************************************* -// * TEST LOGIC * -// ********************************************************************************************* - -// Use the address that has been used to deploy role_store. -//let caller_address: ContractAddress = 0x101.try_into().unwrap(); -//start_prank(role_store.contract_address, caller_address); - -//let account_address: ContractAddress = contract_address_const::<1>(); - -// Grant CONTROLLER role to account address. -//role_store.grant_role(account_address, CONTROLLER); - -// Grant MARKET_KEEPER role to account address. -//role_store.grant_role(account_address, MARKET_KEEPER); - -// Get roles from index 0 to 2 (should return ROLE_ADMIN and CONTROLLER). -//let roles_0_to_2 = role_store.get_roles(0, 2); -//let first_role = roles_0_to_2.at(0); -//let second_role = roles_0_to_2.at(1); -//assert(*first_role == ROLE_ADMIN, '1 should be ROLE_ADMIN'); -//assert(*second_role == CONTROLLER, '2 should be CONTROLLER'); - -// Get roles from index 1 to 3 (should return CONTROLLER and MARKET_KEEPER). -//let roles_1_to_3 = role_store.get_roles(1, 3); -//let first_role = roles_1_to_3.at(0); -//let second_role = roles_1_to_3.at(1); -//assert(*first_role == CONTROLLER, '3 should be CONTROLLER'); -//assert(*second_role == MARKET_KEEPER, '4 should be MARKET_KEEPER'); - -// ********************************************************************************************* -// * TEARDOWN * -// ********************************************************************************************* -//teardown(); -//} +#[test] +fn given_normal_conditions_when_get_role_members_then_works() { + let role_store = setup(); + + // Use the address that has been used to deploy role_store. + start_prank(role_store.contract_address, admin()); + // Grant CONTROLLER role to accounts. + role_store.grant_role(account_1(), CONTROLLER); + role_store.grant_role(account_2(), CONTROLLER); + role_store.grant_role(account_3(), CONTROLLER); + + let members = role_store.get_role_members(CONTROLLER, 1, 3); + assert(*members.at(0) == account_1(), 'should be acc_1'); + assert(*members.at(1) == account_2(), 'should be acc_2'); + assert(*members.at(2) == account_3(), 'should be acc_3'); + + role_store.revoke_role(account_2(), CONTROLLER); + let members = role_store.get_role_members(CONTROLLER, 1, 2); + assert(*members.at(0) == account_1(), 'should be acc_1'); + assert(*members.at(1) == account_3(), 'should be acc_3'); + + role_store.revoke_role(account_1(), CONTROLLER); + let members = role_store.get_role_members(CONTROLLER, 1, 2); + assert(*members.at(0) == account_3(), 'should be acc_3'); +} + +fn admin() -> ContractAddress { + contract_address_const::<0x101>() +} + +fn account_1() -> ContractAddress { + contract_address_const::<1>() +} + +fn account_2() -> ContractAddress { + contract_address_const::<2>() +} + +fn account_3() -> ContractAddress { + contract_address_const::<3>() +} /// Utility function to setup the test environment. fn setup() -> IRoleStoreDispatcher { IRoleStoreDispatcher { contract_address: deploy_role_store() } } -/// Utility function to teardown the test environment. -fn teardown() {} - // Utility function to deploy a role store contract and return its address. fn deploy_role_store() -> ContractAddress { let contract = declare('RoleStore');