From fac8f0be9e2043b3156feaa9d63f8156509793a3 Mon Sep 17 00:00:00 2001 From: Augustus Chang Date: Mon, 23 Sep 2024 14:47:38 -0400 Subject: [PATCH 1/6] timelock + set done --- contracts/src/access_control.cairo | 1 + .../src/access_control/rbac_timelock.cairo | 495 ++++++++++++++++++ contracts/src/libraries.cairo | 1 + contracts/src/libraries/enumerable_set.cairo | 97 ++++ contracts/src/libraries/mocks.cairo | 1 + .../libraries/mocks/mock_enumerable_set.cairo | 54 ++ contracts/src/tests.cairo | 2 + contracts/src/tests/test_enumerable_set.cairo | 151 ++++++ contracts/src/tests/test_rbac_timelock.cairo | 32 ++ 9 files changed, 834 insertions(+) create mode 100644 contracts/src/access_control/rbac_timelock.cairo create mode 100644 contracts/src/libraries/enumerable_set.cairo create mode 100644 contracts/src/libraries/mocks/mock_enumerable_set.cairo create mode 100644 contracts/src/tests/test_enumerable_set.cairo create mode 100644 contracts/src/tests/test_rbac_timelock.cairo diff --git a/contracts/src/access_control.cairo b/contracts/src/access_control.cairo index f66d52735..ba69e0132 100644 --- a/contracts/src/access_control.cairo +++ b/contracts/src/access_control.cairo @@ -1 +1,2 @@ mod access_controller; +mod rbac_timelock; diff --git a/contracts/src/access_control/rbac_timelock.cairo b/contracts/src/access_control/rbac_timelock.cairo new file mode 100644 index 000000000..7ce555f68 --- /dev/null +++ b/contracts/src/access_control/rbac_timelock.cairo @@ -0,0 +1,495 @@ +use starknet::ContractAddress; +use alexandria_bytes::{Bytes, BytesTrait}; +use alexandria_encoding::sol_abi::sol_bytes::SolBytesTrait; +use alexandria_encoding::sol_abi::encode::SolAbiEncodeTrait; + +#[derive(Copy, Drop, Serde)] +struct Call { + target: ContractAddress, + selector: felt252, + data: Span, +} + +fn _hash_operation_batch(calls: Span, predecessor: u256, salt: u256) -> u256 { + let mut encoded: Bytes = BytesTrait::new_empty(); + + let mut i = 0; + while i < calls + .len() { + let call = *calls.at(i); + encoded = encoded.encode(call.target).encode(call.selector); + let mut j = 0; + while j < call.data.len() { + encoded = encoded.encode(*call.data.at(j)); + j += 1; + }; + i += 1; + }; + + encoded = encoded.encode(predecessor).encode(salt); + encoded.keccak() +} + +#[starknet::interface] +trait IRBACTimelock { + fn schedule_batch( + ref self: TContractState, calls: Span, predecessor: u256, salt: u256, delay: u256 + ); + fn cancel(ref self: TContractState, id: u256); + fn execute_batch(ref self: TContractState, calls: Span, predecessor: u256, salt: u256); + fn update_delay(ref self: TContractState, new_delay: u256); + fn bypasser_execute_batch(ref self: TContractState, calls: Span); + fn block_function_selector(ref self: TContractState, selector: felt252); + fn unblock_function_selector(ref self: TContractState, selector: felt252); + fn get_blocked_function_selector_count(self: @TContractState) -> u256; + fn get_blocked_function_selector_at(self: @TContractState, index: u256) -> felt252; + fn is_operation(self: @TContractState, id: u256) -> bool; + fn is_operation_pending(self: @TContractState, id: u256) -> bool; + fn is_operation_ready(self: @TContractState, id: u256) -> bool; + fn is_operation_done(self: @TContractState, id: u256) -> bool; + fn get_timestamp(self: @TContractState, id: u256) -> u256; + fn get_min_delay(self: @TContractState) -> u256; + fn hash_operation_batch( + self: @TContractState, calls: Span, predecessor: u256, salt: u256 + ) -> u256; +} + +// todo: add the erc receiver stuff + supports interface (register it for coin safe transfers) + +#[starknet::contract] +mod RBACTimelock { + use core::traits::TryInto; + use core::starknet::SyscallResultTrait; + use starknet::{ContractAddress, call_contract_syscall}; + use openzeppelin::{ + access::accesscontrol::AccessControlComponent, introspection::src5::SRC5Component, + token::erc1155::erc1155_receiver::ERC1155ReceiverComponent, + token::erc721::erc721_receiver::ERC721ReceiverComponent, + }; + use chainlink::libraries::enumerable_set::EnumerableSetComponent; + use super::{Call, _hash_operation_batch}; + use alexandria_bytes::{Bytes, BytesTrait}; + use alexandria_encoding::sol_abi::sol_bytes::SolBytesTrait; + use alexandria_encoding::sol_abi::encode::SolAbiEncodeTrait; + + component!(path: SRC5Component, storage: src5, event: SRC5Event); + component!(path: AccessControlComponent, storage: access_control, event: AccessControlEvent); + component!(path: EnumerableSetComponent, storage: set, event: EnumerableSetEvent); + component!( + path: ERC1155ReceiverComponent, storage: erc1155_receiver, event: ERC1155ReceiverEvent + ); + component!(path: ERC721ReceiverComponent, storage: erc721_receiver, event: ERC721ReceiverEvent); + + // SRC5 + #[abi(embed_v0)] + impl SRC5Impl = SRC5Component::SRC5Impl; + impl SRC5InternalImpl = SRC5Component::InternalImpl; + + // AccessControl + #[abi(embed_v0)] + impl AccessControlImpl = + AccessControlComponent::AccessControlImpl; + impl AccessControlInternalImpl = AccessControlComponent::InternalImpl; + + // ERC1155Receiver + #[abi(embed_v0)] + impl ERC1155ReceiverImpl = + ERC1155ReceiverComponent::ERC1155ReceiverImpl; + impl ERC1155ReceiverInternalImpl = ERC1155ReceiverComponent::InternalImpl; + + // ERC721Receiver + #[abi(embed_v0)] + impl ERC721ReceiverImpl = + ERC721ReceiverComponent::ERC721ReceiverImpl; + impl ERC721ReceiverInternalImpl = ERC721ReceiverComponent::InternalImpl; + + // EnumerableSet + impl EnumerableSetInternalImpl = EnumerableSetComponent::InternalImpl; + + // we use sn_keccak intead of keccak256 + const ADMIN_ROLE: felt252 = selector!("ADMIN_ROLE"); + const PROPOSER_ROLE: felt252 = selector!("PROPOSER_ROLE"); + const EXECUTOR_ROLE: felt252 = selector!("EXECUTOR_ROLE"); + const CANCELLER_ROLE: felt252 = selector!("CANCELLER_ROLE"); + const BYPASSER_ROLE: felt252 = selector!("BYPASSER_ROLE"); + const _DONE_TIMESTAMP: u256 = 0x1; + + const BLOCKED_FUNCTIONS: u256 = 'BLOCKED_FUNCTION_SELECTORS'; + + #[storage] + struct Storage { + #[substorage(v0)] + erc721_receiver: ERC721ReceiverComponent::Storage, + #[substorage(v0)] + erc1155_receiver: ERC1155ReceiverComponent::Storage, + #[substorage(v0)] + set: EnumerableSetComponent::Storage, + #[substorage(v0)] + src5: SRC5Component::Storage, + #[substorage(v0)] + access_control: AccessControlComponent::Storage, + // id -> timestamp + _timestamps: LegacyMap, // timestamp at which operation is ready to be executed + _min_delay: u256 + } + + #[derive(Drop, starknet::Event)] + struct MinDelayChange { + old_duration: u256, + new_duration: u256 + } + + #[derive(Drop, starknet::Event)] + struct CallScheduled { + #[key] + id: u256, + #[key] + index: u256, + predecessor: u256, + salt: u256, + delay: u256, + target: ContractAddress, + selector: felt252, + data: Span, + } + + #[derive(Drop, starknet::Event)] + struct Cancelled { + #[key] + id: u256 + } + + #[derive(Drop, starknet::Event)] + struct CallExecuted { + #[key] + id: u256, + #[key] + index: u256, + target: ContractAddress, + selector: felt252, + data: Span, + } + + #[derive(Drop, starknet::Event)] + struct BypasserCallExecuted { + #[key] + index: u256, + target: ContractAddress, + selector: felt252, + data: Span, + } + + #[derive(Drop, starknet::Event)] + struct FunctionSelectorBlocked { + #[key] + selector: felt252 + } + + #[derive(Drop, starknet::Event)] + struct FunctionSelectorUnblocked { + #[key] + selector: felt252 + } + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + #[flat] + ERC721ReceiverEvent: ERC721ReceiverComponent::Event, + #[flat] + ERC1155ReceiverEvent: ERC1155ReceiverComponent::Event, + #[flat] + SRC5Event: SRC5Component::Event, + #[flat] + AccessControlEvent: AccessControlComponent::Event, + #[flat] + EnumerableSetEvent: EnumerableSetComponent::Event, + MinDelayChange: MinDelayChange, + CallScheduled: CallScheduled, + Cancelled: Cancelled, + CallExecuted: CallExecuted, + BypasserCallExecuted: BypasserCallExecuted, + FunctionSelectorBlocked: FunctionSelectorBlocked, + FunctionSelectorUnblocked: FunctionSelectorUnblocked + } + + + #[constructor] + fn constructor( + ref self: ContractState, + min_delay: u256, + admin: ContractAddress, + proposers: Span, + executors: Span, + cancellers: Span, + bypassers: Span + ) { + self.access_control.initializer(); + self.erc1155_receiver.initializer(); + self.erc721_receiver.initializer(); + + self.access_control._set_role_admin(ADMIN_ROLE, ADMIN_ROLE); + self.access_control._set_role_admin(PROPOSER_ROLE, ADMIN_ROLE); + self.access_control._set_role_admin(EXECUTOR_ROLE, ADMIN_ROLE); + self.access_control._set_role_admin(CANCELLER_ROLE, ADMIN_ROLE); + self.access_control._set_role_admin(BYPASSER_ROLE, ADMIN_ROLE); + self.access_control._grant_role(ADMIN_ROLE, admin); + + let mut i = 0; + while i < proposers + .len() { + self.access_control._grant_role(PROPOSER_ROLE, *proposers.at(i)); + i += 1; + }; + + let mut i = 0; + while i < executors + .len() { + self.access_control._grant_role(EXECUTOR_ROLE, *executors.at(i)); + i += 1; + }; + + let mut i = 0; + while i < cancellers + .len() { + self.access_control._grant_role(CANCELLER_ROLE, *cancellers.at(i)); + i += 1 + }; + + let mut i = 0; + while i < bypassers + .len() { + self.access_control._grant_role(BYPASSER_ROLE, *bypassers.at(i)); + i += 1 + }; + + self._min_delay.write(min_delay); + + self + .emit( + Event::MinDelayChange(MinDelayChange { old_duration: 0, new_duration: min_delay, }) + ) + } + + #[abi(embed_v0)] + impl RBACTimelockImpl of super::IRBACTimelock { + fn schedule_batch( + ref self: ContractState, calls: Span, predecessor: u256, salt: u256, delay: u256 + ) { + self._assert_only_role_or_admin_role(PROPOSER_ROLE); + + let id = self.hash_operation_batch(calls, predecessor, salt); + self._schedule(id, delay); + + let mut i = 0; + while i < calls + .len() { + let call = *calls.at(i); + assert( + !self.set.contains(BLOCKED_FUNCTIONS, call.selector.into()), + 'selector is blocked' + ); + + self + .emit( + Event::CallScheduled( + CallScheduled { + id: id, + index: i.into(), + target: call.target, + selector: call.selector, + data: call.data, + predecessor: predecessor, + salt: salt, + delay: delay + } + ) + ); + + i += 1; + } + } + + fn cancel(ref self: ContractState, id: u256) { + self._assert_only_role_or_admin_role(CANCELLER_ROLE); + + assert(self.is_operation_pending(id), 'rbact: cant cancel operation'); + + self._timestamps.write(id, 0); + + self.emit(Event::Cancelled(Cancelled { id: id })); + } + + fn execute_batch( + ref self: ContractState, calls: Span, predecessor: u256, salt: u256 + ) { + self._assert_only_role_or_admin_role(EXECUTOR_ROLE); + + let id = self.hash_operation_batch(calls, predecessor, salt); + + self._before_call(id, predecessor); + + let mut i = 0; + while i < calls + .len() { + let call = *(calls.at(i)); + self._execute(call); + self + .emit( + Event::CallExecuted( + CallExecuted { + id: id, + index: i.into(), + target: call.target, + selector: call.selector, + data: call.data + } + ) + ); + i += 1; + }; + + self._after_call(id); + } + + fn update_delay(ref self: ContractState, new_delay: u256) { + self.access_control.assert_only_role(ADMIN_ROLE); + + self + .emit( + Event::MinDelayChange( + MinDelayChange { + old_duration: self._min_delay.read(), new_duration: new_delay, + } + ) + ); + self._min_delay.write(new_delay); + } + + fn bypasser_execute_batch(ref self: ContractState, calls: Span) { + self._assert_only_role_or_admin_role(BYPASSER_ROLE); + + let mut i = 0; + while i < calls + .len() { + let call = *calls.at(i); + self._execute(call); + self + .emit( + Event::BypasserCallExecuted( + BypasserCallExecuted { + index: i.into(), + target: call.target, + selector: call.selector, + data: call.data + } + ) + ); + + i += 1; + } + } + + fn block_function_selector(ref self: ContractState, selector: felt252) { + self.access_control.assert_only_role(ADMIN_ROLE); + + // cast to u256 because that's what set stores + if self.set.add(BLOCKED_FUNCTIONS, selector.into()) { + self + .emit( + Event::FunctionSelectorBlocked( + FunctionSelectorBlocked { selector: selector } + ) + ); + } + } + + fn unblock_function_selector(ref self: ContractState, selector: felt252) { + self.access_control.assert_only_role(ADMIN_ROLE); + + if self.set.remove(BLOCKED_FUNCTIONS, selector.into()) { + self + .emit( + Event::FunctionSelectorUnblocked( + FunctionSelectorUnblocked { selector: selector } + ) + ); + } + } + + fn get_blocked_function_selector_count(self: @ContractState) -> u256 { + self.set.length(BLOCKED_FUNCTIONS) + } + + fn get_blocked_function_selector_at(self: @ContractState, index: u256) -> felt252 { + // cast from u256 to felt252 should never error + self.set.at(BLOCKED_FUNCTIONS, index).try_into().unwrap() + } + + fn is_operation(self: @ContractState, id: u256) -> bool { + self.get_timestamp(id) > 0 + } + + fn is_operation_pending(self: @ContractState, id: u256) -> bool { + self.get_timestamp(id) > _DONE_TIMESTAMP + } + + fn is_operation_ready(self: @ContractState, id: u256) -> bool { + let timestamp = self.get_timestamp(id); + timestamp > _DONE_TIMESTAMP && timestamp <= starknet::info::get_block_timestamp().into() + } + + fn is_operation_done(self: @ContractState, id: u256) -> bool { + self.get_timestamp(id) == _DONE_TIMESTAMP + } + + fn get_timestamp(self: @ContractState, id: u256) -> u256 { + self._timestamps.read(id) + } + + fn get_min_delay(self: @ContractState) -> u256 { + self._min_delay.read() + } + + fn hash_operation_batch( + self: @ContractState, calls: Span, predecessor: u256, salt: u256 + ) -> u256 { + _hash_operation_batch(calls, predecessor, salt) + } + } + + + #[generate_trait] + impl InternalFunctions of InternalFunctionsTrait { + fn _assert_only_role_or_admin_role(ref self: ContractState, role: felt252) { + let caller = starknet::info::get_caller_address(); + if !self.access_control.has_role(ADMIN_ROLE, caller) { + self.access_control.assert_only_role(role); + } + } + + fn _schedule(ref self: ContractState, id: u256, delay: u256) { + assert(!self.is_operation(id), 'operation already scheduled'); + assert(delay >= self.get_min_delay(), 'insufficient delay'); + + self._timestamps.write(id, starknet::info::get_block_timestamp().into() + delay) + } + + fn _before_call(self: @ContractState, id: u256, predecessor: u256) { + assert(self.is_operation_ready(id), 'rbact: operation not ready'); + assert( + predecessor == 0 || self.is_operation_done(predecessor), 'rbact: missing dependency' + ); + } + + fn _after_call(ref self: ContractState, id: u256) { + assert(self.is_operation_ready(id), 'rbact: operation not ready'); + self._timestamps.write(id, _DONE_TIMESTAMP); + } + + fn _execute(ref self: ContractState, call: Call) { + let _response = call_contract_syscall(call.target, call.selector, call.data) + .unwrap_syscall(); + } + } +} diff --git a/contracts/src/libraries.cairo b/contracts/src/libraries.cairo index 02d62120e..fbf6911cd 100644 --- a/contracts/src/libraries.cairo +++ b/contracts/src/libraries.cairo @@ -3,3 +3,4 @@ mod token; mod upgradeable; mod mocks; mod type_and_version; +mod enumerable_set; diff --git a/contracts/src/libraries/enumerable_set.cairo b/contracts/src/libraries/enumerable_set.cairo new file mode 100644 index 000000000..54e0b024b --- /dev/null +++ b/contracts/src/libraries/enumerable_set.cairo @@ -0,0 +1,97 @@ +#[starknet::component] +mod EnumerableSetComponent { + use core::array::ArrayTrait; + + #[storage] + pub struct Storage { + // access index by value + // set_id -> item_value -> item_index + // note: item_index is +1 because 0 means item is not in set + pub _indexes: LegacyMap::<(u256, u256), u256>, + // access value by index + // set_id -> item_id -> item_value + // note: item_index is +1 because 0 means item is not in set + pub _values: LegacyMap::<(u256, u256), u256>, + // set_id -> size of set + pub _length: LegacyMap + } + + #[event] + #[derive(Drop, starknet::Event)] + enum Event {} + + + #[generate_trait] + pub impl InternalImpl< + TContractState, +HasComponent + > of InternalTrait { + fn add(ref self: ComponentState, set_id: u256, value: u256) -> bool { + if !self.contains(set_id, value) { + // The value is stored at _length-1, but we add 1 to all indexes + let index = self._length.read(set_id) + 1; + self._indexes.write((set_id, value), index); + self._values.write((set_id, index), value); + self._length.write(set_id, index); + true + } else { + false + } + } + + // swap target value with the last value in the set + fn remove( + ref self: ComponentState, set_id: u256, target_value: u256 + ) -> bool { + let target_index = self._indexes.read((set_id, target_value)); + if target_index == 0 { + false + } else { + let last_index = self._length.read(set_id); + let last_value = self._values.read((set_id, last_index)); + + // if we are NOT trying to remove the last element + // update the last element mappings + if last_index != target_index { + self._indexes.write((set_id, last_value), target_index); + self._values.write((set_id, target_index), last_value); + } + + // if we are removing the last element both target value and last_index + // refer to the same item. + self._indexes.write((set_id, target_value), 0); + self._values.write((set_id, last_index), 0); + + // decrement length of set by 1 + self._length.write(set_id, last_index - 1); + + true + } + } + + fn contains(self: @ComponentState, set_id: u256, value: u256) -> bool { + self._indexes.read((set_id, value)) != 0 + } + + fn length(self: @ComponentState, set_id: u256) -> u256 { + self._length.read(set_id) + } + + fn at(self: @ComponentState, set_id: u256, index: u256) -> u256 { + self._values.read((set_id, index)) + } + + fn values(self: @ComponentState, set_id: u256) -> Array { + let len = self.length(set_id); + + let mut result: Array = ArrayTrait::new(); + + let mut i = 1; + while i <= len { + result.append(self.at(set_id, i)); + i += 1; + }; + + result + } + } +} diff --git a/contracts/src/libraries/mocks.cairo b/contracts/src/libraries/mocks.cairo index a6e8a7395..5042203bc 100644 --- a/contracts/src/libraries/mocks.cairo +++ b/contracts/src/libraries/mocks.cairo @@ -1,3 +1,4 @@ mod mock_upgradeable; mod mock_non_upgradeable; mod mock_multisig_target; +mod mock_enumerable_set; diff --git a/contracts/src/libraries/mocks/mock_enumerable_set.cairo b/contracts/src/libraries/mocks/mock_enumerable_set.cairo new file mode 100644 index 000000000..e71c12cd6 --- /dev/null +++ b/contracts/src/libraries/mocks/mock_enumerable_set.cairo @@ -0,0 +1,54 @@ +#[starknet::interface] +trait IMockEnumerableSet { + fn add(ref self: TContractState, set_id: u256, value: u256) -> bool; + fn remove(ref self: TContractState, set_id: u256, target_value: u256) -> bool; + fn contains(self: @TContractState, set_id: u256, value: u256) -> bool; + fn length(self: @TContractState, set_id: u256) -> u256; + fn at(self: @TContractState, set_id: u256, index: u256) -> u256; + fn values(self: @TContractState, set_id: u256) -> Array; +} + +#[starknet::contract] +mod MockEnumerableSet { + use chainlink::libraries::enumerable_set::EnumerableSetComponent; + + component!(path: EnumerableSetComponent, storage: set, event: EnumerableSetEvent); + + // EnumerableSet + impl EnumerableSetInternalImpl = EnumerableSetComponent::InternalImpl; + + #[storage] + struct Storage { + #[substorage(v0)] + set: EnumerableSetComponent::Storage, + } + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + #[flat] + EnumerableSetEvent: EnumerableSetComponent::Event, + } + + #[abi(embed_v0)] + impl MockEnumerableSetImpl of super::IMockEnumerableSet { + fn add(ref self: ContractState, set_id: u256, value: u256) -> bool { + self.set.add(set_id, value) + } + fn remove(ref self: ContractState, set_id: u256, target_value: u256) -> bool { + self.set.remove(set_id, target_value) + } + fn contains(self: @ContractState, set_id: u256, value: u256) -> bool { + self.set.contains(set_id, value) + } + fn length(self: @ContractState, set_id: u256) -> u256 { + self.set.length(set_id) + } + fn at(self: @ContractState, set_id: u256, index: u256) -> u256 { + self.set.at(set_id, index) + } + fn values(self: @ContractState, set_id: u256) -> Array { + self.set.values(set_id) + } + } +} diff --git a/contracts/src/tests.cairo b/contracts/src/tests.cairo index 3ee396c65..fb561654c 100644 --- a/contracts/src/tests.cairo +++ b/contracts/src/tests.cairo @@ -11,3 +11,5 @@ mod test_access_controller; mod test_mock_aggregator; mod test_sequencer_uptime_feed; mod test_mcms; +mod test_enumerable_set; +mod test_rbac_timelock; diff --git a/contracts/src/tests/test_enumerable_set.cairo b/contracts/src/tests/test_enumerable_set.cairo new file mode 100644 index 000000000..b5d23ef8f --- /dev/null +++ b/contracts/src/tests/test_enumerable_set.cairo @@ -0,0 +1,151 @@ +use starknet::ContractAddress; +use chainlink::libraries::mocks::mock_enumerable_set::{ + MockEnumerableSet, IMockEnumerableSet, IMockEnumerableSetDispatcher, + IMockEnumerableSetDispatcherTrait, IMockEnumerableSetSafeDispatcher, + IMockEnumerableSetSafeDispatcherTrait +}; +use snforge_std::{declare, ContractClassTrait}; + +const MOCK_SET_ID: u256 = 'adfasdf'; +const OTHER_SET_ID: u256 = 'fakeasdf'; + +fn setup_mock() -> ( + ContractAddress, IMockEnumerableSetDispatcher, IMockEnumerableSetSafeDispatcher +) { + let calldata = array![]; + let (mock_address, _) = declare("MockEnumerableSet").unwrap().deploy(@calldata).unwrap(); + + ( + mock_address, + IMockEnumerableSetDispatcher { contract_address: mock_address }, + IMockEnumerableSetSafeDispatcher { contract_address: mock_address } + ) +} + +#[test] +fn test_add() { + let (_, mock, _) = setup_mock(); + + // ensure that adding to other sets do not interfere with current set + mock.add(OTHER_SET_ID, 6); + + let first_value = 12; + + assert(mock.add(MOCK_SET_ID, first_value), 'should add'); + + assert(mock.contains(MOCK_SET_ID, first_value), 'should contain'); + assert(mock.length(MOCK_SET_ID) == 1, 'should equal 1'); + assert(mock.at(MOCK_SET_ID, 1) == first_value, 'should return val'); + assert(mock.values(MOCK_SET_ID) == array![first_value], 'arrays should equal'); + + assert(!mock.add(MOCK_SET_ID, first_value), 'should not add'); + + let second_value = 100; + + assert(mock.add(MOCK_SET_ID, second_value), 'should add'); + assert( + mock.contains(MOCK_SET_ID, first_value) && mock.contains(MOCK_SET_ID, second_value), + 'should contain' + ); + assert(mock.length(MOCK_SET_ID) == 2, 'should equal 2'); + assert( + mock.at(MOCK_SET_ID, 1) == first_value && mock.at(MOCK_SET_ID, 2) == second_value, + 'should return val' + ); + assert(mock.values(MOCK_SET_ID) == array![first_value, second_value], 'arrays should equal'); +} + +#[test] +fn test_remove() { + let (_, mock, _) = setup_mock(); + let first_value = 12; + + // ensure that removing other sets do not interfere with current set + mock.add(OTHER_SET_ID, 6); + mock.add(OTHER_SET_ID, 7); + mock.remove(OTHER_SET_ID, 7); + + assert(!mock.remove(MOCK_SET_ID, first_value), 'should not remove'); + + // [12] + mock.add(MOCK_SET_ID, first_value); + + // [] + assert(mock.remove(MOCK_SET_ID, first_value), 'should remove'); + + assert(!mock.contains(MOCK_SET_ID, first_value), 'should not contain'); + assert(mock.length(MOCK_SET_ID) == 0, 'len should == 0'); + assert(mock.values(MOCK_SET_ID) == array![], 'should be empty array'); + + // [100, 200, 300] + mock.add(MOCK_SET_ID, 100); + mock.add(MOCK_SET_ID, 200); + mock.add(MOCK_SET_ID, 300); + + // [100, 200] + assert(mock.remove(MOCK_SET_ID, 300), 'remove 300 from end'); + assert(mock.length(MOCK_SET_ID) == 2, 'length should equal 2'); + assert(!mock.contains(MOCK_SET_ID, 300), 'does not contain 300'); + assert( + mock.contains(MOCK_SET_ID, 100) && mock.contains(MOCK_SET_ID, 200), 'contains 100 & 200' + ); + assert(mock.at(MOCK_SET_ID, 1) == 100 && mock.at(MOCK_SET_ID, 2) == 200, 'indexes match'); + assert(mock.at(MOCK_SET_ID, 3) == 0, 'no entry at 3rd index'); + assert(mock.values(MOCK_SET_ID) == array![100, 200], 'values should match'); + + // [100, 200, 300] + mock.add(MOCK_SET_ID, 300); + + // [300, 200] + assert(mock.remove(MOCK_SET_ID, 100), 'remove 100'); + assert(mock.length(MOCK_SET_ID) == 2, 'length should equal 2'); + assert(!mock.contains(MOCK_SET_ID, 100), 'does not contain 100'); + assert( + mock.contains(MOCK_SET_ID, 300) && mock.contains(MOCK_SET_ID, 200), 'contains 300 & 200' + ); + assert(mock.at(MOCK_SET_ID, 1) == 300 && mock.at(MOCK_SET_ID, 2) == 200, 'indexes match'); + assert(mock.at(MOCK_SET_ID, 3) == 0, 'no entry at 3rd index'); + assert(mock.values(MOCK_SET_ID) == array![300, 200], 'values should match'); + + // [200] + assert(mock.remove(MOCK_SET_ID, 300), 'remove 300'); + assert(mock.length(MOCK_SET_ID) == 1, 'length should equal 1'); + assert(!mock.contains(MOCK_SET_ID, 300), 'does not contain 300'); + assert(mock.contains(MOCK_SET_ID, 200), 'contains 200'); + assert(mock.at(MOCK_SET_ID, 1) == 200, 'indexes match'); + assert(mock.at(MOCK_SET_ID, 2) == 0, 'no entry at 2nd index'); + assert(mock.values(MOCK_SET_ID) == array![200], 'values should match'); + + // [] + assert(mock.remove(MOCK_SET_ID, 200), 'remove 200'); + + assert(mock.length(MOCK_SET_ID) == 0, 'empty list'); + assert(mock.values(MOCK_SET_ID) == array![], 'empty list'); +} + +#[test] +fn test_contains() { + let (_, mock, _) = setup_mock(); + + assert(!mock.contains(MOCK_SET_ID, 6), 'should not contain'); + + mock.add(MOCK_SET_ID, 7); + + assert(mock.contains(MOCK_SET_ID, 7), 'should contain'); + + assert(!mock.contains(OTHER_SET_ID, 7), 'should not contain'); +} + +#[test] +fn test_length() { + let (_, mock, _) = setup_mock(); + + assert(mock.length(MOCK_SET_ID) == 0, 'should be 0'); + + mock.add(MOCK_SET_ID, 7); + + assert(mock.length(MOCK_SET_ID) == 1, 'should be 1'); + + assert(mock.length(OTHER_SET_ID) == 0, 'should be 0'); +} + diff --git a/contracts/src/tests/test_rbac_timelock.cairo b/contracts/src/tests/test_rbac_timelock.cairo new file mode 100644 index 000000000..d12615ef9 --- /dev/null +++ b/contracts/src/tests/test_rbac_timelock.cairo @@ -0,0 +1,32 @@ +use starknet::ContractAddress; +use chainlink::access_control::rbac_timelock::{ + RBACTimelock, IRBACTimelock, IRBACTimelockDispatcher, IRBACTimelockDispatcherTrait, + IRBACTimelockSafeDispatcher, IRBACTimelockSafeDispatcherTrait +}; +// 1. test supports access controller, erc1155 receiver, and erc721 receiver +// 2. test has_roles after constructor is called + min delay + event emitted +// 3. test schedule_batch, cancel, execute_batch, update_delay, bypasser_execute_batch , block, unblock +// 4. test schedule with too small of a delay +// 5. test schedule with blocked selector +// 6. test schedule_batch + get operation is true +// 7. test can't cancel operation that isn't scheduled +// 8. test can't cancel operation that was already executed +// 9. test execute call that isn't ready yet +// 10. test execute call which predecessor is not executed +// 11. test execute call successful + assert it's done +// 12. test update delay is true. +// 13. test scheduled batch and then updated delay changes the return of is_operation_ready +// 14. test bypasser execute batch works +// 15. test bypasser execute batch fails +// 16. test block function selector once and then twice +// 17. test unblock fx selector unsuccessful and then successful +// 18. test get block fx selector count +// 19. test get blocked fx selector index throughout a bunch of add and removals +// 20. test is_operation, pending, ready, done throughout life cycle of id +// 21. test block and unblock the max felt size + +// fn setup_timelock() -> (ContractAddress, IRBACTimelockDispatcher, IRBACTimelockSafeDispatcher) { + +// } + + From ee5167190daa07e8d8c8d3111f9010e46d411659 Mon Sep 17 00:00:00 2001 From: Augustus Chang Date: Mon, 23 Sep 2024 16:37:14 -0400 Subject: [PATCH 2/6] few functions --- .../src/access_control/rbac_timelock.cairo | 47 +++--- contracts/src/tests/test_rbac_timelock.cairo | 147 +++++++++++++++++- 2 files changed, 169 insertions(+), 25 deletions(-) diff --git a/contracts/src/access_control/rbac_timelock.cairo b/contracts/src/access_control/rbac_timelock.cairo index 7ce555f68..04f756025 100644 --- a/contracts/src/access_control/rbac_timelock.cairo +++ b/contracts/src/access_control/rbac_timelock.cairo @@ -37,8 +37,8 @@ trait IRBACTimelock { ); fn cancel(ref self: TContractState, id: u256); fn execute_batch(ref self: TContractState, calls: Span, predecessor: u256, salt: u256); - fn update_delay(ref self: TContractState, new_delay: u256); fn bypasser_execute_batch(ref self: TContractState, calls: Span); + fn update_delay(ref self: TContractState, new_delay: u256); fn block_function_selector(ref self: TContractState, selector: felt252); fn unblock_function_selector(ref self: TContractState, selector: felt252); fn get_blocked_function_selector_count(self: @TContractState) -> u256; @@ -219,15 +219,14 @@ mod RBACTimelock { ref self: ContractState, min_delay: u256, admin: ContractAddress, - proposers: Span, - executors: Span, - cancellers: Span, - bypassers: Span + proposers: Array, + executors: Array, + cancellers: Array, + bypassers: Array ) { self.access_control.initializer(); self.erc1155_receiver.initializer(); self.erc721_receiver.initializer(); - self.access_control._set_role_admin(ADMIN_ROLE, ADMIN_ROLE); self.access_control._set_role_admin(PROPOSER_ROLE, ADMIN_ROLE); self.access_control._set_role_admin(EXECUTOR_ROLE, ADMIN_ROLE); @@ -352,20 +351,6 @@ mod RBACTimelock { self._after_call(id); } - fn update_delay(ref self: ContractState, new_delay: u256) { - self.access_control.assert_only_role(ADMIN_ROLE); - - self - .emit( - Event::MinDelayChange( - MinDelayChange { - old_duration: self._min_delay.read(), new_duration: new_delay, - } - ) - ); - self._min_delay.write(new_delay); - } - fn bypasser_execute_batch(ref self: ContractState, calls: Span) { self._assert_only_role_or_admin_role(BYPASSER_ROLE); @@ -390,6 +375,24 @@ mod RBACTimelock { } } + fn update_delay(ref self: ContractState, new_delay: u256) { + self.access_control.assert_only_role(ADMIN_ROLE); + + self + .emit( + Event::MinDelayChange( + MinDelayChange { + old_duration: self._min_delay.read(), new_duration: new_delay, + } + ) + ); + self._min_delay.write(new_delay); + } + + // + // ONLY ADMIN + // + fn block_function_selector(ref self: ContractState, selector: felt252) { self.access_control.assert_only_role(ADMIN_ROLE); @@ -417,6 +420,10 @@ mod RBACTimelock { } } + // + // VIEW ONLY + // + fn get_blocked_function_selector_count(self: @ContractState) -> u256 { self.set.length(BLOCKED_FUNCTIONS) } diff --git a/contracts/src/tests/test_rbac_timelock.cairo b/contracts/src/tests/test_rbac_timelock.cairo index d12615ef9..e485595ba 100644 --- a/contracts/src/tests/test_rbac_timelock.cairo +++ b/contracts/src/tests/test_rbac_timelock.cairo @@ -1,11 +1,27 @@ -use starknet::ContractAddress; +use starknet::{ContractAddress, contract_address_const}; use chainlink::access_control::rbac_timelock::{ RBACTimelock, IRBACTimelock, IRBACTimelockDispatcher, IRBACTimelockDispatcherTrait, - IRBACTimelockSafeDispatcher, IRBACTimelockSafeDispatcherTrait + IRBACTimelockSafeDispatcher, IRBACTimelockSafeDispatcherTrait, + RBACTimelock::{ADMIN_ROLE, PROPOSER_ROLE, EXECUTOR_ROLE, CANCELLER_ROLE, BYPASSER_ROLE} +}; +use openzeppelin::{ + introspection::interface::{ISRC5, ISRC5Dispatcher, ISRC5DispatcherTrait, ISRC5_ID}, + access::accesscontrol::{ + interface::{ + IACCESSCONTROL_ID, IAccessControl, IAccessControlDispatcher, + IAccessControlDispatcherTrait + }, + accesscontrol::AccessControlComponent::Errors + }, + token::{erc1155::interface::{IERC1155_RECEIVER_ID}, erc721::interface::{IERC721_RECEIVER_ID}} +}; +use snforge_std::{ + declare, ContractClassTrait, spy_events, EventSpyAssertionsTrait, + start_cheat_caller_address_global }; // 1. test supports access controller, erc1155 receiver, and erc721 receiver // 2. test has_roles after constructor is called + min delay + event emitted -// 3. test schedule_batch, cancel, execute_batch, update_delay, bypasser_execute_batch , block, unblock +// 3. test schedule_batch, cancel, execute_batch, update_delay, bypasser_execute_batch , block, unblock with wrong roles // 4. test schedule with too small of a delay // 5. test schedule with blocked selector // 6. test schedule_batch + get operation is true @@ -25,8 +41,129 @@ use chainlink::access_control::rbac_timelock::{ // 20. test is_operation, pending, ready, done throughout life cycle of id // 21. test block and unblock the max felt size -// fn setup_timelock() -> (ContractAddress, IRBACTimelockDispatcher, IRBACTimelockSafeDispatcher) { +fn deploy_args() -> ( + u256, ContractAddress, ContractAddress, ContractAddress, ContractAddress, ContractAddress +) { + let min_delay: u256 = 0x9; + let admin = contract_address_const::<1>(); + let proposer = contract_address_const::<2>(); + let executor = contract_address_const::<3>(); + let canceller = contract_address_const::<4>(); + let bypasser = contract_address_const::<5>(); + (min_delay, admin, proposer, executor, canceller, bypasser) +} + +fn setup_timelock() -> (ContractAddress, IRBACTimelockDispatcher, IRBACTimelockSafeDispatcher) { + let (min_delay, admin, proposer, executor, canceller, bypasser) = deploy_args(); + let proposers = array![proposer]; + let executors = array![executor]; + let cancellers = array![canceller]; + let bypassers = array![bypasser]; + + let mut calldata = ArrayTrait::new(); + Serde::serialize(@min_delay, ref calldata); + Serde::serialize(@admin, ref calldata); + Serde::serialize(@proposers, ref calldata); + Serde::serialize(@executors, ref calldata); + Serde::serialize(@cancellers, ref calldata); + Serde::serialize(@bypassers, ref calldata); + + let (timelock_address, _) = declare("RBACTimelock").unwrap().deploy(@calldata).unwrap(); + + ( + timelock_address, + IRBACTimelockDispatcher { contract_address: timelock_address }, + IRBACTimelockSafeDispatcher { contract_address: timelock_address } + ) +} + +#[test] +fn test_supports_interfaces() { + let (timelock_address, _, _) = setup_timelock(); + + let timelock = ISRC5Dispatcher { contract_address: timelock_address }; + + assert(timelock.supports_interface(ISRC5_ID), 'supports ISRC5'); + + assert(timelock.supports_interface(IACCESSCONTROL_ID), 'supports IACCESSCONTROL_ID'); + + assert(timelock.supports_interface(IERC1155_RECEIVER_ID), 'supports IERC1155_RECEIVER_ID'); + + assert(timelock.supports_interface(IERC721_RECEIVER_ID), 'supports IERC721_RECEIVER_ID'); + + assert(!timelock.supports_interface(0x0123123123), 'does not support random one'); +} + +#[test] +fn test_roles() { + let (_, admin, proposer, executor, canceller, bypasser) = deploy_args(); + let (timelock_address, _, _) = setup_timelock(); + + let timelock = IAccessControlDispatcher { contract_address: timelock_address }; + + // admin role controls rest of roles + assert( + timelock.get_role_admin(ADMIN_ROLE) == ADMIN_ROLE + && timelock.get_role_admin(PROPOSER_ROLE) == ADMIN_ROLE + && timelock.get_role_admin(EXECUTOR_ROLE) == ADMIN_ROLE + && timelock.get_role_admin(CANCELLER_ROLE) == ADMIN_ROLE + && timelock.get_role_admin(BYPASSER_ROLE) == ADMIN_ROLE, + 'admin role controls all roles' + ); + + // admin address + assert(timelock.has_role(ADMIN_ROLE, admin), 'is admin'); + assert(timelock.has_role(PROPOSER_ROLE, proposer), 'is proposer'); + assert(timelock.has_role(EXECUTOR_ROLE, executor), 'is executor'); + assert(timelock.has_role(CANCELLER_ROLE, canceller), 'is canceller'); + assert(timelock.has_role(BYPASSER_ROLE, bypasser), 'is bypasser'); +} + +#[test] +fn test_deploy() { + let mut spy = spy_events(); + + let (min_delay, _, _, _, _, _) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + assert(timelock.get_min_delay() == min_delay, 'min delay correct'); + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::MinDelayChange( + RBACTimelock::MinDelayChange { old_duration: 0, new_duration: min_delay } + ) + ) + ] + ); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_funcs_fail_wrong_role() { + let (min_delay, admin, proposer, executor, canceller, bypasser) = deploy_args(); + let (timelock_address, timelock, safe_timelock) = setup_timelock(); + + start_cheat_caller_address_global(contract_address_const::<123123>()); + + expect_missing_role(safe_timelock.schedule_batch(array![].span(), 0, 0, 0)); + expect_missing_role(safe_timelock.cancel(0)); + expect_missing_role(safe_timelock.execute_batch(array![].span(), 0, 0)); + expect_missing_role(safe_timelock.update_delay(0)); + expect_missing_role(safe_timelock.bypasser_execute_batch(array![].span())); + expect_missing_role(safe_timelock.block_function_selector(0x0)); + expect_missing_role(safe_timelock.unblock_function_selector(0x0)); +} -// } +fn expect_missing_role(result: Result<(), Array>) { + match result { + Result::Ok(_) => panic!("expect 'Caller is missing role'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == Errors::MISSING_ROLE, *panic_data.at(0)); + } + } +} From 95244b927cf97601ac9f1cefe6f88bac5e24cb25 Mon Sep 17 00:00:00 2001 From: Augustus Chang Date: Wed, 25 Sep 2024 08:03:42 -0400 Subject: [PATCH 3/6] contract + most tests done --- contracts/src/tests/test_rbac_timelock.cairo | 509 ++++++++++++++++++- 1 file changed, 502 insertions(+), 7 deletions(-) diff --git a/contracts/src/tests/test_rbac_timelock.cairo b/contracts/src/tests/test_rbac_timelock.cairo index e485595ba..cf258f0a2 100644 --- a/contracts/src/tests/test_rbac_timelock.cairo +++ b/contracts/src/tests/test_rbac_timelock.cairo @@ -1,9 +1,16 @@ use starknet::{ContractAddress, contract_address_const}; -use chainlink::access_control::rbac_timelock::{ - RBACTimelock, IRBACTimelock, IRBACTimelockDispatcher, IRBACTimelockDispatcherTrait, - IRBACTimelockSafeDispatcher, IRBACTimelockSafeDispatcherTrait, - RBACTimelock::{ADMIN_ROLE, PROPOSER_ROLE, EXECUTOR_ROLE, CANCELLER_ROLE, BYPASSER_ROLE} +use chainlink::{ + access_control::rbac_timelock::{ + RBACTimelock, IRBACTimelock, IRBACTimelockDispatcher, IRBACTimelockDispatcherTrait, + IRBACTimelockSafeDispatcher, IRBACTimelockSafeDispatcherTrait, + RBACTimelock::{ADMIN_ROLE, PROPOSER_ROLE, EXECUTOR_ROLE, CANCELLER_ROLE, BYPASSER_ROLE}, + Call + }, + libraries::mocks::mock_multisig_target::{ + IMockMultisigTarget, IMockMultisigTargetDispatcherTrait, IMockMultisigTargetDispatcher + } }; + use openzeppelin::{ introspection::interface::{ISRC5, ISRC5Dispatcher, ISRC5DispatcherTrait, ISRC5_ID}, access::accesscontrol::{ @@ -17,7 +24,7 @@ use openzeppelin::{ }; use snforge_std::{ declare, ContractClassTrait, spy_events, EventSpyAssertionsTrait, - start_cheat_caller_address_global + start_cheat_caller_address_global, start_cheat_block_timestamp_global }; // 1. test supports access controller, erc1155 receiver, and erc721 receiver // 2. test has_roles after constructor is called + min delay + event emitted @@ -25,11 +32,15 @@ use snforge_std::{ // 4. test schedule with too small of a delay // 5. test schedule with blocked selector // 6. test schedule_batch + get operation is true + // 7. test can't cancel operation that isn't scheduled // 8. test can't cancel operation that was already executed +// test cancel is successful + // 9. test execute call that isn't ready yet // 10. test execute call which predecessor is not executed // 11. test execute call successful + assert it's done + // 12. test update delay is true. // 13. test scheduled batch and then updated delay changes the return of is_operation_ready // 14. test bypasser execute batch works @@ -53,6 +64,13 @@ fn deploy_args() -> ( (min_delay, admin, proposer, executor, canceller, bypasser) } +fn setup_mock_target() -> (ContractAddress, IMockMultisigTargetDispatcher) { + let calldata = ArrayTrait::new(); + let mock_target_contract = declare("MockMultisigTarget").unwrap(); + let (target_address, _) = mock_target_contract.deploy(@calldata).unwrap(); + (target_address, IMockMultisigTargetDispatcher { contract_address: target_address }) +} + fn setup_timelock() -> (ContractAddress, IRBACTimelockDispatcher, IRBACTimelockSafeDispatcher) { let (min_delay, admin, proposer, executor, canceller, bypasser) = deploy_args(); let proposers = array![proposer]; @@ -143,8 +161,8 @@ fn test_deploy() { #[test] #[feature("safe_dispatcher")] fn test_funcs_fail_wrong_role() { - let (min_delay, admin, proposer, executor, canceller, bypasser) = deploy_args(); - let (timelock_address, timelock, safe_timelock) = setup_timelock(); + let (min_delay, _, _, _, _, _) = deploy_args(); + let (_, _, safe_timelock) = setup_timelock(); start_cheat_caller_address_global(contract_address_const::<123123>()); @@ -167,3 +185,480 @@ fn expect_missing_role(result: Result<(), Array>) { } } +// schedule + +// 4. test schedule with too small of a delay [x] +// 5. test schedule with blocked selector [x] +// 6. test schedule_batch + get operation is true [x] +// 7 . test schedule fails when id scheduled already [x] + +#[test] +#[feature("safe_dispatcher")] +fn test_schedule_delay_too_small() { + let (min_delay, _, proposer, _, _, _) = deploy_args(); + let (_, _, safe_timelock) = setup_timelock(); + + start_cheat_caller_address_global(proposer); + + let result = safe_timelock.schedule_batch(array![].span(), 0, 0, min_delay - 1); + match result { + Result::Ok(_) => panic!("expect 'insufficient delay'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'insufficient delay', *panic_data.at(0)); + } + } +} + +#[test] +fn test_schedule_success() { + let (min_delay, _, proposer, _, _, _) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + + start_cheat_caller_address_global(proposer); + start_cheat_block_timestamp_global(mock_time); + + let call = Call { + target: contract_address_const::<100>(), + selector: selector!("doesnt_exist"), + data: array![0x123].span() + }; + let calls = array![call].span(); + let predecessor = 0; + let salt = 1; + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + + assert(!timelock.is_operation(id), 'should not exist'); + + let mut spy = spy_events(); + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::CallScheduled( + RBACTimelock::CallScheduled { + id: id, + index: 0, + target: call.target, + selector: call.selector, + data: call.data, + predecessor: predecessor, + salt: salt, + delay: min_delay + } + ) + ) + ] + ); + + assert(timelock.is_operation(id), 'should exist'); + assert(timelock.is_operation_pending(id), 'should be pending'); + assert(!timelock.is_operation_ready(id), 'should not be ready'); + + start_cheat_block_timestamp_global(mock_ready_time); + + assert(timelock.is_operation_ready(id), 'should be ready'); + + assert(timelock.get_timestamp(id) == mock_ready_time.into(), 'timestamps match'); +} + +#[test] +#[feature("safe_dispatcher")] +fn test_schedule_twice() { + let (min_delay, _, proposer, _, _, _) = deploy_args(); + let (_, timelock, safe_timelock) = setup_timelock(); + + start_cheat_caller_address_global(proposer); + + let calls = array![ + Call { + target: contract_address_const::<100>(), + selector: selector!("doesnt_exist"), + data: array![0x123].span() + } + ] + .span(); + let predecessor = 0; + let salt = 1; + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + let result = safe_timelock.schedule_batch(calls, predecessor, salt, min_delay); + match result { + Result::Ok(_) => panic!("expect 'operation already scheduled'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'operation already scheduled', *panic_data.at(0)); + } + } +} + +#[test] +#[feature("safe_dispatcher")] +fn test_schedule_blocked() { + let (min_delay, admin, proposer, _, _, _) = deploy_args(); + let (_, timelock, safe_timelock) = setup_timelock(); + + let selector = selector!("doesnt_exist"); + + // first, block the fx + start_cheat_caller_address_global(admin); + + timelock.block_function_selector(selector); + + start_cheat_caller_address_global(proposer); + + let calls = array![ + Call { + target: contract_address_const::<100>(), selector: selector, data: array![0x123].span() + } + ] + .span(); + let predecessor = 0; + let salt = 1; + + let result = safe_timelock.schedule_batch(calls, predecessor, salt, min_delay); + match result { + Result::Ok(_) => panic!("expect 'selector is blocked'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'selector is blocked', *panic_data.at(0)); + } + } +} + +// 7. test can't cancel operation that isn't scheduled [x] +// 8. test can't cancel operation that was already executed +// test cancel is successful +#[test] +#[feature("safe_dispatcher")] +fn test_cancel_id_not_pending() { + let (_, _, _, _, canceller, _) = deploy_args(); + let (_, _, safe_timelock) = setup_timelock(); + + start_cheat_caller_address_global(canceller); + + // unscheduled id + let result = safe_timelock.cancel(123123123123); + + match result { + Result::Ok(_) => panic!("expect 'rbact: cant cancel operation'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'rbact: cant cancel operation', *panic_data.at(0)); + } + } +// executed id + +// todo: add case where it was already executed here +} + +#[test] +fn test_cancel_success() { + let (min_delay, _, proposer, _, canceller, _) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + let mock_time = 3; + + start_cheat_caller_address_global(proposer); + start_cheat_block_timestamp_global(mock_time); + + let call = Call { + target: contract_address_const::<100>(), + selector: selector!("doesnt_exist"), + data: array![0x123].span() + }; + let calls = array![call].span(); + let predecessor = 0; + let salt = 1; + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + let mut spy = spy_events(); + + start_cheat_caller_address_global(canceller); + + timelock.cancel(id); + + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::Cancelled(RBACTimelock::Cancelled { id: id }) + ) + ] + ); + + assert(!timelock.is_operation(id), 'not operation'); + assert(timelock.get_timestamp(id) == 0, 'matches 0'); +} + +// 9. test execute call that isn't ready yet [x] +// 10. test execute call which predecessor is not executed [x] +// 11. test execute call successful + assert it's done +// 12. test the invocation fails for some reason [x] + +#[test] +#[feature("safe_dispatcher")] +fn test_execute_op_not_ready() { + let (min_delay, _, proposer, executor, _, _) = deploy_args(); + let (timelock_address, timelock, safe_timelock) = setup_timelock(); + + let mock_time = 3; + start_cheat_block_timestamp_global(mock_time); + + let call = Call { + target: contract_address_const::<100>(), + selector: selector!("doesnt_exist"), + data: array![0x123].span() + }; + let calls = array![call].span(); + let predecessor = 0; + let salt = 1; + + start_cheat_caller_address_global(executor); + + // test not scheduled + let result = safe_timelock.execute_batch(calls, predecessor, salt); + match result { + Result::Ok(_) => panic!("expect 'rbact: operation not ready'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'rbact: operation not ready', *panic_data.at(0)); + } + } + + // test not ready + + // first, schedule it + start_cheat_caller_address_global(proposer); + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + // then, execute + start_cheat_block_timestamp_global( + mock_time + min_delay.try_into().unwrap() - 1 + ); // still not ready + start_cheat_caller_address_global(executor); + let result = safe_timelock.execute_batch(calls, predecessor, salt); + match result { + Result::Ok(_) => panic!("expect 'rbact: operation not ready'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'rbact: operation not ready', *panic_data.at(0)); + } + } +// todo: +// test executed +} + +#[test] +#[feature("safe_dispatcher")] +fn test_execute_predecessor_invalid() { + let (min_delay, _, proposer, executor, _, _) = deploy_args(); + let (timelock_address, timelock, safe_timelock) = setup_timelock(); + + let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + start_cheat_block_timestamp_global(mock_time); + + let call = Call { + target: contract_address_const::<100>(), + selector: selector!("doesnt_exist"), + data: array![0x123].span() + }; + let calls = array![call].span(); + let predecessor = 4; + let salt = 1; + + start_cheat_caller_address_global(proposer); + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + start_cheat_caller_address_global(executor); + start_cheat_block_timestamp_global(mock_ready_time); + let result = safe_timelock.execute_batch(calls, predecessor, salt); + match result { + Result::Ok(_) => panic!("expect 'rbact: missing dependency'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'rbact: missing dependency', *panic_data.at(0)); + } + } +} +// snforge does not treat contract invocation failures as a panic (yet) +// #[test] +// #[feature("safe_dispatcher")] +// fn test_execute_invalid_call() { +// let (min_delay, _, proposer, executor, _, _) = deploy_args(); +// let (timelock_address, timelock, safe_timelock) = setup_timelock(); + +// let mock_time = 3; +// let mock_ready_time = mock_time + min_delay.try_into().unwrap(); +// start_cheat_block_timestamp_global(mock_time); + +// let call = Call { +// target: contract_address_const::<100>(), +// selector: selector!("doesnt_exist"), +// data: array![0x123].span() +// }; +// let calls = array![call].span(); +// let predecessor = 0; +// let salt = 1; + +// start_cheat_caller_address_global(proposer); +// timelock.schedule_batch(calls, predecessor, salt, min_delay); + +// // will fail because target contract does not exist +// start_cheat_block_timestamp_global(mock_ready_time); +// start_cheat_caller_address_global(executor); +// let result = safe_timelock.execute_batch(calls, predecessor, salt); +// } + +#[test] +fn test_execute_successful() { + let (target_address, target) = setup_mock_target(); + let (min_delay, _, proposer, executor, _, _) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + + let call_1 = Call { + target: target_address, selector: selector!("set_value"), data: array![0x56162].span() + }; + + let call_2 = Call { + target: target_address, selector: selector!("flip_toggle"), data: array![].span() + }; + + let calls = array![call_1, call_2].span(); + let predecessor = 0; + let salt = 1; + + start_cheat_caller_address_global(proposer); + start_cheat_block_timestamp_global(mock_time); + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + + start_cheat_caller_address_global(executor); + start_cheat_block_timestamp_global(mock_ready_time); + + let mut spy = spy_events(); + + timelock.execute_batch(calls, predecessor, salt); + + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::CallExecuted( + RBACTimelock::CallExecuted { + id: id, + index: 0, + target: call_1.target, + selector: call_1.selector, + data: call_1.data + } + ) + ), + ( + timelock_address, + RBACTimelock::Event::CallExecuted( + RBACTimelock::CallExecuted { + id: id, + index: 1, + target: call_2.target, + selector: call_2.selector, + data: call_2.data + } + ) + ) + ] + ); + + let (actual_value, actual_toggle) = target.read(); + assert(actual_value == 0x56162, 'value equal'); + assert(actual_toggle, 'toggle true'); +} + + +// 12. test update delay is true. +// 13. test scheduled batch and then updated delay changes the return of is_operation_ready *not + +#[test] +fn test_update_delay_success() { + let (min_delay, admin, _, _, _, _) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + start_cheat_caller_address_global(admin); + + let mut spy = spy_events(); + + timelock.update_delay(0x92289); + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::MinDelayChange( + RBACTimelock::MinDelayChange { + old_duration: min_delay, new_duration: 0x92289 + } + ) + ) + ] + ); + + assert(timelock.get_min_delay() == 0x92289, 'new min delay'); +} + +#[test] +fn test_update_delay_no_affect_op_readiness() { + let (min_delay, admin, proposer, _, _, _) = deploy_args(); + let (_, timelock, _) = setup_timelock(); + + let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + start_cheat_block_timestamp_global(mock_time); + + let call = Call { + target: contract_address_const::<100>(), + selector: selector!("doesnt_exist"), + data: array![0x123].span() + }; + let calls = array![call].span(); + let predecessor = 0; + let salt = 1; + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + + start_cheat_caller_address_global(proposer); + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + start_cheat_block_timestamp_global(mock_ready_time); + + assert(timelock.is_operation_ready(id), 'confirm op ready'); + + start_cheat_caller_address_global(admin); + timelock.update_delay(0x92289); + + assert(timelock.is_operation_ready(id), 'op still ready'); +} + +// 14. test bypasser execute batch works +// 15. test bypasser execute batch fails +// 16. test block function selector once and then twice +// 17. test unblock fx selector unsuccessful and then successful +// 18. test get block fx selector count +// 19. test get blocked fx selector index throughout a bunch of add and removals +// 20. test is_operation, pending, ready, done throughout life cycle of id +// 21. test block and unblock the max felt size + +fn test_bypasser_execute_success() {} + From 91ca3f044c456cca49362cf0c1c776afd7f1ed68 Mon Sep 17 00:00:00 2001 From: Augustus Chang Date: Thu, 26 Sep 2024 14:19:56 -0400 Subject: [PATCH 4/6] finish tests --- contracts/src/tests/test_rbac_timelock.cairo | 369 ++++++++++++++----- 1 file changed, 285 insertions(+), 84 deletions(-) diff --git a/contracts/src/tests/test_rbac_timelock.cairo b/contracts/src/tests/test_rbac_timelock.cairo index cf258f0a2..ad3d4a950 100644 --- a/contracts/src/tests/test_rbac_timelock.cairo +++ b/contracts/src/tests/test_rbac_timelock.cairo @@ -26,31 +26,6 @@ use snforge_std::{ declare, ContractClassTrait, spy_events, EventSpyAssertionsTrait, start_cheat_caller_address_global, start_cheat_block_timestamp_global }; -// 1. test supports access controller, erc1155 receiver, and erc721 receiver -// 2. test has_roles after constructor is called + min delay + event emitted -// 3. test schedule_batch, cancel, execute_batch, update_delay, bypasser_execute_batch , block, unblock with wrong roles -// 4. test schedule with too small of a delay -// 5. test schedule with blocked selector -// 6. test schedule_batch + get operation is true - -// 7. test can't cancel operation that isn't scheduled -// 8. test can't cancel operation that was already executed -// test cancel is successful - -// 9. test execute call that isn't ready yet -// 10. test execute call which predecessor is not executed -// 11. test execute call successful + assert it's done - -// 12. test update delay is true. -// 13. test scheduled batch and then updated delay changes the return of is_operation_ready -// 14. test bypasser execute batch works -// 15. test bypasser execute batch fails -// 16. test block function selector once and then twice -// 17. test unblock fx selector unsuccessful and then successful -// 18. test get block fx selector count -// 19. test get blocked fx selector index throughout a bunch of add and removals -// 20. test is_operation, pending, ready, done throughout life cycle of id -// 21. test block and unblock the max felt size fn deploy_args() -> ( u256, ContractAddress, ContractAddress, ContractAddress, ContractAddress, ContractAddress @@ -161,7 +136,7 @@ fn test_deploy() { #[test] #[feature("safe_dispatcher")] fn test_funcs_fail_wrong_role() { - let (min_delay, _, _, _, _, _) = deploy_args(); + let (_, _, _, _, _, _) = deploy_args(); let (_, _, safe_timelock) = setup_timelock(); start_cheat_caller_address_global(contract_address_const::<123123>()); @@ -185,12 +160,14 @@ fn expect_missing_role(result: Result<(), Array>) { } } -// schedule - -// 4. test schedule with too small of a delay [x] -// 5. test schedule with blocked selector [x] -// 6. test schedule_batch + get operation is true [x] -// 7 . test schedule fails when id scheduled already [x] +fn expect_operation_not_ready(result: Result<(), Array>) { + match result { + Result::Ok(_) => panic!("expect 'rbact: operation not ready'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'rbact: operation not ready', *panic_data.at(0)); + } + } +} #[test] #[feature("safe_dispatcher")] @@ -332,14 +309,27 @@ fn test_schedule_blocked() { } } -// 7. test can't cancel operation that isn't scheduled [x] -// 8. test can't cancel operation that was already executed -// test cancel is successful #[test] #[feature("safe_dispatcher")] fn test_cancel_id_not_pending() { - let (_, _, _, _, canceller, _) = deploy_args(); - let (_, _, safe_timelock) = setup_timelock(); + let (target_address, _) = setup_mock_target(); + let (min_delay, _, proposer, executor, canceller, _) = deploy_args(); + let (_, timelock, safe_timelock) = setup_timelock(); + + let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + + let call_1 = Call { + target: target_address, selector: selector!("set_value"), data: array![0x56162].span() + }; + + let call_2 = Call { + target: target_address, selector: selector!("flip_toggle"), data: array![].span() + }; + + let calls = array![call_1, call_2].span(); + let predecessor = 0; + let salt = 1; start_cheat_caller_address_global(canceller); @@ -352,9 +342,32 @@ fn test_cancel_id_not_pending() { assert(*panic_data.at(0) == 'rbact: cant cancel operation', *panic_data.at(0)); } } -// executed id -// todo: add case where it was already executed here + // test that after a batch has been executed, you can't cancel it + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + + start_cheat_caller_address_global(proposer); + start_cheat_block_timestamp_global(mock_time); + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + assert(timelock.is_operation_pending(id), 'id is now pending'); + + start_cheat_caller_address_global(executor); + start_cheat_block_timestamp_global(mock_ready_time); + + timelock.execute_batch(calls, predecessor, salt); + + start_cheat_caller_address_global(canceller); + + let result = safe_timelock.cancel(id); + match result { + Result::Ok(_) => panic!("expect 'rbact: cant cancel operation'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'rbact: cant cancel operation', *panic_data.at(0)); + } + } } #[test] @@ -399,39 +412,33 @@ fn test_cancel_success() { assert(timelock.get_timestamp(id) == 0, 'matches 0'); } -// 9. test execute call that isn't ready yet [x] -// 10. test execute call which predecessor is not executed [x] -// 11. test execute call successful + assert it's done -// 12. test the invocation fails for some reason [x] - #[test] #[feature("safe_dispatcher")] fn test_execute_op_not_ready() { + let (target_address, _) = setup_mock_target(); let (min_delay, _, proposer, executor, _, _) = deploy_args(); - let (timelock_address, timelock, safe_timelock) = setup_timelock(); + let (_, timelock, safe_timelock) = setup_timelock(); let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); start_cheat_block_timestamp_global(mock_time); - let call = Call { - target: contract_address_const::<100>(), - selector: selector!("doesnt_exist"), - data: array![0x123].span() + let call_1 = Call { + target: target_address, selector: selector!("set_value"), data: array![0x56162].span() }; - let calls = array![call].span(); + + let call_2 = Call { + target: target_address, selector: selector!("flip_toggle"), data: array![].span() + }; + + let calls = array![call_1, call_2].span(); let predecessor = 0; let salt = 1; start_cheat_caller_address_global(executor); // test not scheduled - let result = safe_timelock.execute_batch(calls, predecessor, salt); - match result { - Result::Ok(_) => panic!("expect 'rbact: operation not ready'"), - Result::Err(panic_data) => { - assert(*panic_data.at(0) == 'rbact: operation not ready', *panic_data.at(0)); - } - } + expect_operation_not_ready(safe_timelock.execute_batch(calls, predecessor, salt)); // test not ready @@ -439,27 +446,23 @@ fn test_execute_op_not_ready() { start_cheat_caller_address_global(proposer); timelock.schedule_batch(calls, predecessor, salt, min_delay); - // then, execute - start_cheat_block_timestamp_global( - mock_time + min_delay.try_into().unwrap() - 1 - ); // still not ready + // then, execute at a block that is NOT ready + start_cheat_block_timestamp_global(mock_ready_time - 1); start_cheat_caller_address_global(executor); - let result = safe_timelock.execute_batch(calls, predecessor, salt); - match result { - Result::Ok(_) => panic!("expect 'rbact: operation not ready'"), - Result::Err(panic_data) => { - assert(*panic_data.at(0) == 'rbact: operation not ready', *panic_data.at(0)); - } - } -// todo: -// test executed + expect_operation_not_ready(safe_timelock.execute_batch(calls, predecessor, salt)); + + // execute, then test it can't be executed after it's done already + start_cheat_block_timestamp_global(mock_ready_time); + timelock.execute_batch(calls, predecessor, salt); + + expect_operation_not_ready(safe_timelock.execute_batch(calls, predecessor, salt)); } #[test] #[feature("safe_dispatcher")] fn test_execute_predecessor_invalid() { let (min_delay, _, proposer, executor, _, _) = deploy_args(); - let (timelock_address, timelock, safe_timelock) = setup_timelock(); + let (_, timelock, safe_timelock) = setup_timelock(); let mock_time = 3; let mock_ready_time = mock_time + min_delay.try_into().unwrap(); @@ -487,6 +490,7 @@ fn test_execute_predecessor_invalid() { } } } + // snforge does not treat contract invocation failures as a panic (yet) // #[test] // #[feature("safe_dispatcher")] @@ -584,11 +588,9 @@ fn test_execute_successful() { let (actual_value, actual_toggle) = target.read(); assert(actual_value == 0x56162, 'value equal'); assert(actual_toggle, 'toggle true'); -} - -// 12. test update delay is true. -// 13. test scheduled batch and then updated delay changes the return of is_operation_ready *not + assert(timelock.is_operation_done(id), 'operation is done'); +} #[test] fn test_update_delay_success() { @@ -651,14 +653,213 @@ fn test_update_delay_no_affect_op_readiness() { assert(timelock.is_operation_ready(id), 'op still ready'); } -// 14. test bypasser execute batch works -// 15. test bypasser execute batch fails -// 16. test block function selector once and then twice -// 17. test unblock fx selector unsuccessful and then successful -// 18. test get block fx selector count -// 19. test get blocked fx selector index throughout a bunch of add and removals -// 20. test is_operation, pending, ready, done throughout life cycle of id -// 21. test block and unblock the max felt size +fn test_bypasser_execute_success() { + let (target_address, target) = setup_mock_target(); + let (_, _, _, _, _, bypasser) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + let mock_time = 3; + + let call_1 = Call { + target: target_address, selector: selector!("set_value"), data: array![0x56162].span() + }; + + let call_2 = Call { + target: target_address, selector: selector!("flip_toggle"), data: array![].span() + }; + + let calls = array![call_1, call_2].span(); -fn test_bypasser_execute_success() {} + start_cheat_caller_address_global(bypasser); + start_cheat_block_timestamp_global(mock_time); + + let mut spy = spy_events(); + + timelock.bypasser_execute_batch(calls); + + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::BypasserCallExecuted( + RBACTimelock::BypasserCallExecuted { + index: 0, + target: call_1.target, + selector: call_1.selector, + data: call_1.data + } + ) + ), + ( + timelock_address, + RBACTimelock::Event::BypasserCallExecuted( + RBACTimelock::BypasserCallExecuted { + index: 1, + target: call_2.target, + selector: call_2.selector, + data: call_2.data + } + ) + ) + ] + ); + + let (actual_value, actual_toggle) = target.read(); + assert(actual_value == 0x56162, 'value equal'); + assert(actual_toggle, 'toggle true'); +} + +#[test] +fn test_unblock_selector() { + let (_, admin, _, _, _, _) = deploy_args(); + let (timelock_address, timelock, _) = setup_timelock(); + + start_cheat_caller_address_global(admin); + + let mut spy = spy_events(); + + let selector = 'test'; + + // unblock should do nothing + timelock.unblock_function_selector(selector); + + spy + .assert_not_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::FunctionSelectorUnblocked( + RBACTimelock::FunctionSelectorUnblocked { selector: selector, } + ) + ) + ] + ); + + assert(timelock.get_blocked_function_selector_count() == 0, 'count is 0'); + + timelock.block_function_selector(selector); + + let mut spy = spy_events(); + + // unblock should succeed + timelock.unblock_function_selector(selector); + + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::FunctionSelectorUnblocked( + RBACTimelock::FunctionSelectorUnblocked { selector: selector, } + ) + ) + ] + ); + + assert(timelock.get_blocked_function_selector_count() == 0, 'count is 0'); +} + +#[test] +fn test_blocked_selector_indexes() { + let (_, admin, _, _, _, _) = deploy_args(); + let (_, timelock, _) = setup_timelock(); + + start_cheat_caller_address_global(admin); + + let selector1: felt252 = 'test'; + let selector2: felt252 = 'brick'; + let selector3: felt252 = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff; + + timelock.block_function_selector(selector1); + timelock.block_function_selector(selector2); + timelock.block_function_selector(selector3); + + // [selector1, selector2, selector3] + assert(timelock.get_blocked_function_selector_count() == 3, 'count is 3'); + assert(timelock.get_blocked_function_selector_at(1) == selector1, 'selector 1'); + assert(timelock.get_blocked_function_selector_at(2) == selector2, 'selector 2'); + assert(timelock.get_blocked_function_selector_at(3) == selector3, 'selector 3'); + assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); + + timelock.unblock_function_selector(selector1); + + // [selector3, selector2] + assert(timelock.get_blocked_function_selector_count() == 2, 'count is 2'); + assert(timelock.get_blocked_function_selector_at(1) == selector3, 'selector 3'); + assert(timelock.get_blocked_function_selector_at(2) == selector2, 'selector 2'); + assert(timelock.get_blocked_function_selector_at(3) == 0, 'selector 3'); + assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); + + timelock.unblock_function_selector(selector2); + + // [selector3] + assert(timelock.get_blocked_function_selector_count() == 1, 'count is 1'); + assert(timelock.get_blocked_function_selector_at(1) == selector3, 'selector 3'); + assert(timelock.get_blocked_function_selector_at(2) == 0, 'no selector'); + assert(timelock.get_blocked_function_selector_at(3) == 0, 'no selector'); + assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); + + timelock.unblock_function_selector(selector3); + + assert(timelock.get_blocked_function_selector_count() == 0, 'count is 0'); + assert(timelock.get_blocked_function_selector_at(1) == 0, 'no selector'); + assert(timelock.get_blocked_function_selector_at(2) == 0, 'no selector'); + assert(timelock.get_blocked_function_selector_at(3) == 0, 'no selector'); + assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); +} + +#[test] +fn test_lifecycle_of_id() { + let (target_address, _) = setup_mock_target(); + let (min_delay, _, proposer, executor, _, _) = deploy_args(); + let (_, timelock, _) = setup_timelock(); + + let mock_time = 3; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + + let call_1 = Call { + target: target_address, selector: selector!("set_value"), data: array![0x56162].span() + }; + + let call_2 = Call { + target: target_address, selector: selector!("flip_toggle"), data: array![].span() + }; + + let calls = array![call_1, call_2].span(); + let predecessor = 0; + let salt = 1; + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + + assert(!timelock.is_operation(id), 'does not exist yet'); + assert(!timelock.is_operation_pending(id), 'does not exist yet'); + assert(!timelock.is_operation_ready(id), 'does not exist yet'); + assert(!timelock.is_operation_done(id), 'does not exist yet'); + + start_cheat_caller_address_global(proposer); + start_cheat_block_timestamp_global(mock_time); + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + assert(timelock.is_operation(id), 'is operation'); + assert(timelock.is_operation_pending(id), 'is pending'); + assert(!timelock.is_operation_ready(id), 'not ready'); + assert(!timelock.is_operation_done(id), 'not done'); + + start_cheat_caller_address_global(executor); + start_cheat_block_timestamp_global(mock_ready_time); + + assert(timelock.is_operation(id), 'is operation'); + assert(timelock.is_operation_pending(id), 'is pending'); + assert(timelock.is_operation_ready(id), 'is ready'); + assert(!timelock.is_operation_done(id), 'not done'); + + timelock.execute_batch(calls, predecessor, salt); + + assert(timelock.is_operation(id), 'is operation'); + assert(!timelock.is_operation_pending(id), 'is not pending'); + assert(!timelock.is_operation_ready(id), 'is not ready'); + assert(timelock.is_operation_done(id), 'is done'); +} From b0ba9ecc5ceb648f7b22b4af6ce0c7d76aee5964 Mon Sep 17 00:00:00 2001 From: Augustus Chang Date: Thu, 26 Sep 2024 14:24:04 -0400 Subject: [PATCH 5/6] remove comment --- contracts/src/access_control/rbac_timelock.cairo | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/src/access_control/rbac_timelock.cairo b/contracts/src/access_control/rbac_timelock.cairo index 04f756025..c709003da 100644 --- a/contracts/src/access_control/rbac_timelock.cairo +++ b/contracts/src/access_control/rbac_timelock.cairo @@ -54,8 +54,6 @@ trait IRBACTimelock { ) -> u256; } -// todo: add the erc receiver stuff + supports interface (register it for coin safe transfers) - #[starknet::contract] mod RBACTimelock { use core::traits::TryInto; From 4922344fe60cf72cdc7f55b59bdc52fefb025201 Mon Sep 17 00:00:00 2001 From: Augustus Chang Date: Mon, 30 Sep 2024 17:06:43 -0400 Subject: [PATCH 6/6] address comments --- .../src/access_control/rbac_timelock.cairo | 11 ++- contracts/src/libraries/enumerable_set.cairo | 4 + contracts/src/tests/test_enumerable_set.cairo | 71 +++++++++++++++++- contracts/src/tests/test_rbac_timelock.cairo | 73 ++++++++++++++++--- 4 files changed, 137 insertions(+), 22 deletions(-) diff --git a/contracts/src/access_control/rbac_timelock.cairo b/contracts/src/access_control/rbac_timelock.cairo index c709003da..5fe078996 100644 --- a/contracts/src/access_control/rbac_timelock.cairo +++ b/contracts/src/access_control/rbac_timelock.cairo @@ -373,6 +373,10 @@ mod RBACTimelock { } } + // + // ONLY ADMIN + // + fn update_delay(ref self: ContractState, new_delay: u256) { self.access_control.assert_only_role(ADMIN_ROLE); @@ -387,10 +391,6 @@ mod RBACTimelock { self._min_delay.write(new_delay); } - // - // ONLY ADMIN - // - fn block_function_selector(ref self: ContractState, selector: felt252) { self.access_control.assert_only_role(ADMIN_ROLE); @@ -493,8 +493,7 @@ mod RBACTimelock { } fn _execute(ref self: ContractState, call: Call) { - let _response = call_contract_syscall(call.target, call.selector, call.data) - .unwrap_syscall(); + call_contract_syscall(call.target, call.selector, call.data).unwrap_syscall(); } } } diff --git a/contracts/src/libraries/enumerable_set.cairo b/contracts/src/libraries/enumerable_set.cairo index 54e0b024b..791b35606 100644 --- a/contracts/src/libraries/enumerable_set.cairo +++ b/contracts/src/libraries/enumerable_set.cairo @@ -2,6 +2,7 @@ mod EnumerableSetComponent { use core::array::ArrayTrait; + // set is 1-indexed, not 0-indexed #[storage] pub struct Storage { // access index by value @@ -11,6 +12,7 @@ mod EnumerableSetComponent { // access value by index // set_id -> item_id -> item_value // note: item_index is +1 because 0 means item is not in set + // note: _values.read(set_id, item_id) == 0, is only valid iff item_id <= _length.read(set_id) pub _values: LegacyMap::<(u256, u256), u256>, // set_id -> size of set pub _length: LegacyMap @@ -77,6 +79,8 @@ mod EnumerableSetComponent { } fn at(self: @ComponentState, set_id: u256, index: u256) -> u256 { + assert(index != 0, 'set is 1-indexed'); + assert(index <= self._length.read(set_id), 'index out of bounds'); self._values.read((set_id, index)) } diff --git a/contracts/src/tests/test_enumerable_set.cairo b/contracts/src/tests/test_enumerable_set.cairo index b5d23ef8f..1f4e008b8 100644 --- a/contracts/src/tests/test_enumerable_set.cairo +++ b/contracts/src/tests/test_enumerable_set.cairo @@ -9,6 +9,24 @@ use snforge_std::{declare, ContractClassTrait}; const MOCK_SET_ID: u256 = 'adfasdf'; const OTHER_SET_ID: u256 = 'fakeasdf'; +fn expect_out_of_bounds>(result: Result>) { + match result { + Result::Ok(_) => panic!("expect 'index out of bounds'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'index out of bounds', *panic_data.at(0)); + } + } +} + +fn expect_set_is_1_indexed>(result: Result>) { + match result { + Result::Ok(_) => panic!("expect 'set is 1-indexed'"), + Result::Err(panic_data) => { + assert(*panic_data.at(0) == 'set is 1-indexed', *panic_data.at(0)); + } + } +} + fn setup_mock() -> ( ContractAddress, IMockEnumerableSetDispatcher, IMockEnumerableSetSafeDispatcher ) { @@ -56,8 +74,9 @@ fn test_add() { } #[test] +#[feature("safe_dispatcher")] fn test_remove() { - let (_, mock, _) = setup_mock(); + let (_, mock, safe_mock) = setup_mock(); let first_value = 12; // ensure that removing other sets do not interfere with current set @@ -90,7 +109,7 @@ fn test_remove() { mock.contains(MOCK_SET_ID, 100) && mock.contains(MOCK_SET_ID, 200), 'contains 100 & 200' ); assert(mock.at(MOCK_SET_ID, 1) == 100 && mock.at(MOCK_SET_ID, 2) == 200, 'indexes match'); - assert(mock.at(MOCK_SET_ID, 3) == 0, 'no entry at 3rd index'); + expect_out_of_bounds(safe_mock.at(MOCK_SET_ID, 3)); assert(mock.values(MOCK_SET_ID) == array![100, 200], 'values should match'); // [100, 200, 300] @@ -104,7 +123,7 @@ fn test_remove() { mock.contains(MOCK_SET_ID, 300) && mock.contains(MOCK_SET_ID, 200), 'contains 300 & 200' ); assert(mock.at(MOCK_SET_ID, 1) == 300 && mock.at(MOCK_SET_ID, 2) == 200, 'indexes match'); - assert(mock.at(MOCK_SET_ID, 3) == 0, 'no entry at 3rd index'); + expect_out_of_bounds(safe_mock.at(MOCK_SET_ID, 3)); assert(mock.values(MOCK_SET_ID) == array![300, 200], 'values should match'); // [200] @@ -113,7 +132,7 @@ fn test_remove() { assert(!mock.contains(MOCK_SET_ID, 300), 'does not contain 300'); assert(mock.contains(MOCK_SET_ID, 200), 'contains 200'); assert(mock.at(MOCK_SET_ID, 1) == 200, 'indexes match'); - assert(mock.at(MOCK_SET_ID, 2) == 0, 'no entry at 2nd index'); + expect_out_of_bounds(safe_mock.at(MOCK_SET_ID, 2)); assert(mock.values(MOCK_SET_ID) == array![200], 'values should match'); // [] @@ -149,3 +168,47 @@ fn test_length() { assert(mock.length(OTHER_SET_ID) == 0, 'should be 0'); } +#[test] +#[feature("safe_dispatcher")] +fn test_zero() { + let (_, mock, safe_mock) = setup_mock(); + + expect_set_is_1_indexed(safe_mock.at(MOCK_SET_ID, 0)); + + // [0] + assert(mock.add(MOCK_SET_ID, 0), 'should add 0'); + assert(mock.contains(MOCK_SET_ID, 0), 'contains 0'); + + assert(mock.length(MOCK_SET_ID) == 1, 'should be 1'); + + // [0, 1] + assert(mock.add(MOCK_SET_ID, 1), 'should add 1'); + assert(!mock.add(MOCK_SET_ID, 1), 'shouldnt add 1'); + + assert(mock.length(MOCK_SET_ID) == 2, 'should be 2'); + + assert(mock.at(MOCK_SET_ID, 1) == 0, 'set[1] = 0'); + assert(mock.at(MOCK_SET_ID, 2) == 1, 'set[2] = 0'); + + // [1] + assert(mock.remove(MOCK_SET_ID, 0), 'should remove 0'); + assert(!mock.remove(MOCK_SET_ID, 0), 'shouldnt remove 0'); + + assert(mock.at(MOCK_SET_ID, 1) == 1, 'set[1] = 1'); + assert(!mock.contains(MOCK_SET_ID, 0), '0 is gone'); + assert(mock.length(MOCK_SET_ID) == 1, 'length 1'); + + // [] + assert(mock.remove(MOCK_SET_ID, 1), '1 removed'); + + // [0] + mock.add(MOCK_SET_ID, 0); + + assert(mock.at(MOCK_SET_ID, 1) == 0, 'set[1] = 0'); + + // [] + mock.remove(MOCK_SET_ID, 0); + + expect_out_of_bounds(safe_mock.at(MOCK_SET_ID, 1)); +} + diff --git a/contracts/src/tests/test_rbac_timelock.cairo b/contracts/src/tests/test_rbac_timelock.cairo index ad3d4a950..dd29ccc62 100644 --- a/contracts/src/tests/test_rbac_timelock.cairo +++ b/contracts/src/tests/test_rbac_timelock.cairo @@ -10,7 +10,6 @@ use chainlink::{ IMockMultisigTarget, IMockMultisigTargetDispatcherTrait, IMockMultisigTargetDispatcher } }; - use openzeppelin::{ introspection::interface::{ISRC5, ISRC5Dispatcher, ISRC5DispatcherTrait, ISRC5_ID}, access::accesscontrol::{ @@ -22,6 +21,7 @@ use openzeppelin::{ }, token::{erc1155::interface::{IERC1155_RECEIVER_ID}, erc721::interface::{IERC721_RECEIVER_ID}} }; +use chainlink::tests::test_enumerable_set::{expect_out_of_bounds, expect_set_is_1_indexed}; use snforge_std::{ declare, ContractClassTrait, spy_events, EventSpyAssertionsTrait, start_cheat_caller_address_global, start_cheat_block_timestamp_global @@ -590,6 +590,55 @@ fn test_execute_successful() { assert(actual_toggle, 'toggle true'); assert(timelock.is_operation_done(id), 'operation is done'); + + // let's try to schedule another batch of operations using the predecessor + + let mock_time = 3000; + let mock_ready_time = mock_time + min_delay.try_into().unwrap(); + + let call_3 = Call { + target: target_address, selector: selector!("flip_toggle"), data: array![].span() + }; + let calls = array![call_3].span(); + let predecessor = id; + let salt = 2; + + start_cheat_caller_address_global(proposer); + start_cheat_block_timestamp_global(mock_time); + + timelock.schedule_batch(calls, predecessor, salt, min_delay); + + let id = timelock.hash_operation_batch(calls, predecessor, salt); + + start_cheat_caller_address_global(executor); + start_cheat_block_timestamp_global(mock_ready_time); + + let mut spy = spy_events(); + + timelock.execute_batch(calls, predecessor, salt); + + spy + .assert_emitted( + @array![ + ( + timelock_address, + RBACTimelock::Event::CallExecuted( + RBACTimelock::CallExecuted { + id: id, + index: 0, + target: call_3.target, + selector: call_3.selector, + data: call_3.data + } + ) + ) + ] + ); + + let (_, actual_toggle) = target.read(); + assert(!actual_toggle, 'toggle went to false again'); + + assert(timelock.is_operation_done(id), 'operation is done'); } #[test] @@ -761,9 +810,10 @@ fn test_unblock_selector() { } #[test] +#[feature("safe_dispatcher")] fn test_blocked_selector_indexes() { let (_, admin, _, _, _, _) = deploy_args(); - let (_, timelock, _) = setup_timelock(); + let (_, timelock, safe_timelock) = setup_timelock(); start_cheat_caller_address_global(admin); @@ -775,12 +825,14 @@ fn test_blocked_selector_indexes() { timelock.block_function_selector(selector2); timelock.block_function_selector(selector3); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(5)); + expect_set_is_1_indexed(safe_timelock.get_blocked_function_selector_at(0)); + // [selector1, selector2, selector3] assert(timelock.get_blocked_function_selector_count() == 3, 'count is 3'); assert(timelock.get_blocked_function_selector_at(1) == selector1, 'selector 1'); assert(timelock.get_blocked_function_selector_at(2) == selector2, 'selector 2'); assert(timelock.get_blocked_function_selector_at(3) == selector3, 'selector 3'); - assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); timelock.unblock_function_selector(selector1); @@ -788,25 +840,22 @@ fn test_blocked_selector_indexes() { assert(timelock.get_blocked_function_selector_count() == 2, 'count is 2'); assert(timelock.get_blocked_function_selector_at(1) == selector3, 'selector 3'); assert(timelock.get_blocked_function_selector_at(2) == selector2, 'selector 2'); - assert(timelock.get_blocked_function_selector_at(3) == 0, 'selector 3'); - assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(3)); timelock.unblock_function_selector(selector2); // [selector3] assert(timelock.get_blocked_function_selector_count() == 1, 'count is 1'); assert(timelock.get_blocked_function_selector_at(1) == selector3, 'selector 3'); - assert(timelock.get_blocked_function_selector_at(2) == 0, 'no selector'); - assert(timelock.get_blocked_function_selector_at(3) == 0, 'no selector'); - assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(2)); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(3)); timelock.unblock_function_selector(selector3); assert(timelock.get_blocked_function_selector_count() == 0, 'count is 0'); - assert(timelock.get_blocked_function_selector_at(1) == 0, 'no selector'); - assert(timelock.get_blocked_function_selector_at(2) == 0, 'no selector'); - assert(timelock.get_blocked_function_selector_at(3) == 0, 'no selector'); - assert(timelock.get_blocked_function_selector_at(0) == 0, 'no selector'); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(1)); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(2)); + expect_out_of_bounds(safe_timelock.get_blocked_function_selector_at(3)); } #[test]