From f8a6526ec97e78c84ee7ac1978ee380059d5cd89 Mon Sep 17 00:00:00 2001 From: Oba Date: Thu, 24 Oct 2024 10:27:33 +0200 Subject: [PATCH 1/3] feat: self management functions protocolHandler Adds also cairo-coverage for snfoundry and assert_macros --- cairo/protocol_handler/.gitignore | 3 + cairo/protocol_handler/Scarb.toml | 7 + .../src/protocol_handler.cairo | 147 +++++++- .../tests/test_protocol_handler.cairo | 322 +++++++++++++++++- 4 files changed, 461 insertions(+), 18 deletions(-) diff --git a/cairo/protocol_handler/.gitignore b/cairo/protocol_handler/.gitignore index eb5a316cb..5e3810b4a 100644 --- a/cairo/protocol_handler/.gitignore +++ b/cairo/protocol_handler/.gitignore @@ -1 +1,4 @@ target +.snfoundry_versioned_programs/ +coverage/ +snfoundry_trace/ diff --git a/cairo/protocol_handler/Scarb.toml b/cairo/protocol_handler/Scarb.toml index a1142ed13..0bb86aa55 100644 --- a/cairo/protocol_handler/Scarb.toml +++ b/cairo/protocol_handler/Scarb.toml @@ -12,6 +12,7 @@ openzeppelin = { git = "https://github.com/OpenZeppelin/cairo-contracts.git", ta [dev-dependencies] snforge_std = { git = "https://github.com/foundry-rs/starknet-foundry.git", tag = "v0.31.0" } snforge_utils = { path = "../snforge_utils" } +assert_macros = "2.8.2" [[target.starknet-contract]] casm = true @@ -19,3 +20,9 @@ casm-add-pythonic-hints = true [scripts] test = "snforge test" +test-coverage = "snforge test --coverage" + +[profile.dev.cairo] +unstable-add-statements-code-locations-debug-info = true +unstable-add-statements-functions-debug-info = true +inlining-strategy = "avoid" diff --git a/cairo/protocol_handler/src/protocol_handler.cairo b/cairo/protocol_handler/src/protocol_handler.cairo index aea87296f..3a6a86074 100644 --- a/cairo/protocol_handler/src/protocol_handler.cairo +++ b/cairo/protocol_handler/src/protocol_handler.cairo @@ -70,6 +70,53 @@ pub trait IProtocolHandler { /// * `UNAUTHORIZED_SELECTOR` in case the selector is not authorized /// * `ONLY_KAKAROT_CAN_BE_CALLED` in case the call is not to the Kakarot contract fn execute_call(ref self: TContractState, call: Call); + + /// Change the operator of the Kakarot contract. + /// Only the security council can call this function. + /// # Arguments + /// * `new_operator_address` - The new operator address + /// + /// # Panics + /// * `Caller is missing role` in case the caller is not the security council + fn change_operator(ref self: TContractState, new_operator_address: ContractAddress); + + /// Change the security council of the Kakarot contract. + /// Only the security council can call this function. + /// # Arguments + /// * `new_security_council_address` - The new security council address + /// + /// # Panics + /// * `Caller is missing role` in case the caller is not the security council + fn change_security_council( + ref self: TContractState, new_security_council_address: ContractAddress + ); + + /// Change the gas price admin of the Kakarot contract. + /// Only the security council can call this function. + /// # Arguments + /// * `new_gas_price_admin` - The new gas price admin address + /// + /// # Panics + /// * `Caller is missing role` in case the caller is not the security council + fn change_gas_price_admin(ref self: TContractState, new_gas_price_admin: ContractAddress); + + /// Change the guardians of the Kakarot contract. + /// Only the security council can call this function. + /// # Arguments + /// * `new_guardians_address` - The new guardians address + /// + /// # Panics + /// * `Caller is missing role` in case the caller is not the security council + fn add_guardian(ref self: TContractState, new_guardian_address: ContractAddress); + + /// Remove a guardian from the Kakarot contract. + /// Only the security council can call this function. + /// # Arguments + /// * `guardian_address` - The guardian address to be removed + /// + /// # Panics + /// * `Caller is missing role` in case the caller is not the security council + fn remove_guardian(ref self: TContractState, guardian_address: ContractAddress); } #[starknet::contract] @@ -104,10 +151,10 @@ pub mod ProtocolHandler { //* ------------------------------------------------------------------------ *// // Access controls roles - const SECURITY_COUNCIL_ROLE: felt252 = selector!("SECURITY_COUNCIL_ROLE"); - const OPERATOR_ROLE: felt252 = selector!("OPERATOR_ROLE"); - const GUARDIAN_ROLE: felt252 = selector!("GUARDIAN_ROLE"); - const GAS_PRICE_ADMIN_ROLE: felt252 = selector!("GAS_PRICE_ADMIN_ROLE"); + pub const SECURITY_COUNCIL_ROLE: felt252 = selector!("SECURITY_COUNCIL_ROLE"); + pub const OPERATOR_ROLE: felt252 = selector!("OPERATOR_ROLE"); + pub const GUARDIAN_ROLE: felt252 = selector!("GUARDIAN_ROLE"); + pub const GAS_PRICE_ADMIN_ROLE: felt252 = selector!("GAS_PRICE_ADMIN_ROLE"); // Pause delay pub const SOFT_PAUSE_DELAY: u64 = 12 * 60 * 60; // 12 hours pub const HARD_PAUSE_DELAY: u64 = 7 * 24 * 60 * 60; // 7 days @@ -130,6 +177,7 @@ pub mod ProtocolHandler { #[storage] pub struct Storage { pub kakarot: IKakarotDispatcher, + pub security_council: ContractAddress, pub operator: ContractAddress, pub guardians: Map, pub gas_price_admin: ContractAddress, @@ -213,19 +261,11 @@ pub mod ProtocolHandler { gas_price_admin: ContractAddress, mut guardians: Span ) { - // Store the Kakarot address + // Store the Kakarot, security council, operator and gas price admin addresses self.kakarot.write(IKakarotDispatcher { contract_address: kakarot }); - - // AccessControl-related initialization - self.accesscontrol.initializer(); - - // Grant roles - self.accesscontrol._grant_role(SECURITY_COUNCIL_ROLE, security_council); - self.accesscontrol._grant_role(OPERATOR_ROLE, operator); - for guardian in guardians { - self.accesscontrol._grant_role(GUARDIAN_ROLE, *guardian); - }; - self.accesscontrol._grant_role(GAS_PRICE_ADMIN_ROLE, gas_price_admin); + self.security_council.write(security_council); + self.operator.write(operator); + self.gas_price_admin.write(gas_price_admin); // Store the authorized selectors for the operator self.authorized_operator_selector.write(selector!("set_native_token"), true); @@ -245,6 +285,17 @@ pub mod ProtocolHandler { self .authorized_operator_selector .write(selector!("set_l1_messaging_contract_address"), true); + + // AccessControl-related initialization + self.accesscontrol.initializer(); + + // Grant roles + self.accesscontrol._grant_role(SECURITY_COUNCIL_ROLE, security_council); + self.accesscontrol._grant_role(OPERATOR_ROLE, operator); + for guardian in guardians { + self.accesscontrol._grant_role(GUARDIAN_ROLE, *guardian); + }; + self.accesscontrol._grant_role(GAS_PRICE_ADMIN_ROLE, gas_price_admin); } #[abi(embed_v0)] @@ -386,5 +437,69 @@ pub mod ProtocolHandler { // Emit Event Execution event self.emit(Execution { call }); } + + //* ------------------------------------------------------------------------ *// + //* SELF MANAGEMENT *// + //* ------------------------------------------------------------------------ *// + + fn change_operator(ref self: ContractState, new_operator_address: ContractAddress) { + // Check only security council can call + self.accesscontrol.assert_only_role(SECURITY_COUNCIL_ROLE); + + // Revoke the OPERATOR_ROLE from the current operator + self.accesscontrol._revoke_role(OPERATOR_ROLE, self.operator.read()); + + // Grant role to the new operator + self.accesscontrol._grant_role(OPERATOR_ROLE, new_operator_address); + + // Update the operator + self.operator.write(new_operator_address); + } + + fn change_security_council( + ref self: ContractState, new_security_council_address: ContractAddress + ) { + // Check only security council can call + self.accesscontrol.assert_only_role(SECURITY_COUNCIL_ROLE); + + // Revoke the SECURITY_COUNCIL_ROLE from the current security council + self.accesscontrol._revoke_role(SECURITY_COUNCIL_ROLE, self.security_council.read()); + + // Grant role to the new security council + self.accesscontrol._grant_role(SECURITY_COUNCIL_ROLE, new_security_council_address); + + // Update the security council + self.security_council.write(new_security_council_address); + } + + fn change_gas_price_admin(ref self: ContractState, new_gas_price_admin: ContractAddress) { + // Check only security council can call + self.accesscontrol.assert_only_role(SECURITY_COUNCIL_ROLE); + + // Revoke the GAS_PRICE_ADMIN_ROLE from the current gas price admin + self.accesscontrol._revoke_role(GAS_PRICE_ADMIN_ROLE, self.gas_price_admin.read()); + + // Grant role to the new gas price admin + self.accesscontrol._grant_role(GAS_PRICE_ADMIN_ROLE, new_gas_price_admin); + + // Update the gas price admin + self.gas_price_admin.write(new_gas_price_admin); + } + + fn add_guardian(ref self: ContractState, new_guardian_address: ContractAddress) { + // Check only security council can call + self.accesscontrol.assert_only_role(SECURITY_COUNCIL_ROLE); + + // Grant the GUARDIAN_ROLE to the new guardian + self.accesscontrol._grant_role(GUARDIAN_ROLE, new_guardian_address); + } + + fn remove_guardian(ref self: ContractState, guardian_address: ContractAddress) { + // Check only security council can call + self.accesscontrol.assert_only_role(SECURITY_COUNCIL_ROLE); + + // Revoke the GUARDIAN_ROLE from the guardian + self.accesscontrol._revoke_role(GUARDIAN_ROLE, guardian_address); + } } } diff --git a/cairo/protocol_handler/tests/test_protocol_handler.cairo b/cairo/protocol_handler/tests/test_protocol_handler.cairo index 412e5f3f7..1bb6e97b9 100644 --- a/cairo/protocol_handler/tests/test_protocol_handler.cairo +++ b/cairo/protocol_handler/tests/test_protocol_handler.cairo @@ -1,6 +1,7 @@ use snforge_std::{ ContractClassTrait, ContractClass, declare, DeclareResultTrait, EventSpyTrait, - start_cheat_block_timestamp_global, start_cheat_caller_address, mock_call, spy_events, store + start_cheat_block_timestamp_global, start_cheat_caller_address, mock_call, spy_events, store, + load }; use starknet::{ContractAddress, contract_address_const, get_block_timestamp}; use starknet::account::Call; @@ -13,6 +14,13 @@ use snforge_utils::snforge_utils::{ EventsFilterBuilderTrait, ContractEventsTrait, assert_called_with }; +use openzeppelin_access::accesscontrol::AccessControlComponent; +use openzeppelin_access::accesscontrol::AccessControlComponent::{ + InternalImpl, RoleGranted, RoleRevoked +}; +use openzeppelin_access::accesscontrol::interface::IAccessControlDispatcher; +use openzeppelin_access::accesscontrol::interface::IAccessControlDispatcherTrait; + fn kakarot_mock() -> ContractAddress { contract_address_const::<'security_council_mock'>() } @@ -141,6 +149,7 @@ fn test_protocol_upgrade_fail_wrong_caller() { protocol_handler.upgrade(contract.class_hash); } +#[test] fn test_protocol_upgrade_should_pass() { let (protocol_handler, contract) = setup_contracts_for_testing(); @@ -565,7 +574,6 @@ fn test_protocol_handler_execute_call_wrong_selector_should_fail() { } } - #[test] fn test_protocol_handler_execute_call_should_pass() { let (protocol_handler, _) = setup_contracts_for_testing(); @@ -615,3 +623,313 @@ fn test_protocol_handler_execute_call_should_pass() { contract_events.assert_emitted(@expected); } } + +#[test] +#[should_panic(expected: 'Caller is missing role')] +fn test_protocol_handler_change_operator_should_fail_wrong_caller() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to random caller address + let random_caller = contract_address_const::<'random_caller'>(); + start_cheat_caller_address(protocol_handler.contract_address, random_caller); + + // Call the protocol handler change_operator, should fail as caller is not security council + let new_operator = contract_address_const::<'new_operator'>(); + protocol_handler.change_operator(new_operator); +} + +#[test] +fn test_protocol_handler_change_operator_should_pass() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to security council + start_cheat_caller_address(protocol_handler.contract_address, security_council_mock()); + + // Spy on the events + let mut spy = spy_events(); + + // Call the protocol handler change_operator + let new_operator = contract_address_const::<'new_operator'>(); + protocol_handler.change_operator(new_operator); + + // Check the old operator is revoked and the new operator is granted + let access_control_dispatcher = IAccessControlDispatcher { + contract_address: protocol_handler.contract_address + }; + assert( + !access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, operator_mock()), + 'Old operator not revoked' + ); + assert( + access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, new_operator), + 'New operator not granted' + ); + + // Check the Access control related events are emitted + let expected_revoked = AccessControlComponent::Event::RoleRevoked( + RoleRevoked { + role: ProtocolHandler::OPERATOR_ROLE, + account: operator_mock(), + sender: security_council_mock() + } + ); + let expected_granted = AccessControlComponent::Event::RoleGranted( + RoleGranted { + role: ProtocolHandler::OPERATOR_ROLE, + account: new_operator, + sender: security_council_mock() + } + ); + let contract_events = EventsFilterBuilderTrait::from_events(@spy.get_events()) + .with_contract_address(protocol_handler.contract_address) + .build(); + contract_events.assert_emitted(@expected_revoked); + contract_events.assert_emitted(@expected_granted); + + // Check the new operator is set in the contract state + let loaded = load(protocol_handler.contract_address, selector!("operator"), 1); + assert_eq!(*loaded[0], new_operator.into(), "New operator not set"); +} + + +#[test] +#[should_panic(expected: 'Caller is missing role')] +fn test_protocol_handler_change_security_council_should_fail_wrong_caller() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to random caller address + let random_caller = contract_address_const::<'random_caller'>(); + start_cheat_caller_address(protocol_handler.contract_address, random_caller); + + // Call the protocol handler change_security_council, should fail as caller is not security + // council + let new_security_council = contract_address_const::<'new_security_council'>(); + protocol_handler.change_security_council(new_security_council); +} + +#[test] +fn test_protocol_handler_change_security_council_should_pass() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to security council + start_cheat_caller_address(protocol_handler.contract_address, security_council_mock()); + + // Spy on the events + let mut spy = spy_events(); + + // Call the protocol handler change_security_council + let new_security_council = contract_address_const::<'new_security_council'>(); + protocol_handler.change_security_council(new_security_council); + + // Check the old security council is revoked and the new security council is granted + let access_control_dispatcher = IAccessControlDispatcher { + contract_address: protocol_handler.contract_address + }; + assert( + !access_control_dispatcher + .has_role(ProtocolHandler::SECURITY_COUNCIL_ROLE, security_council_mock()), + 'Old SC not revoked' + ); + assert( + access_control_dispatcher + .has_role(ProtocolHandler::SECURITY_COUNCIL_ROLE, new_security_council), + 'New SC not granted' + ); + + // Check the Access control related events are emitted + let expected_revoked = AccessControlComponent::Event::RoleRevoked( + RoleRevoked { + role: ProtocolHandler::SECURITY_COUNCIL_ROLE, + account: security_council_mock(), + sender: security_council_mock() + } + ); + let expected_granted = AccessControlComponent::Event::RoleGranted( + RoleGranted { + role: ProtocolHandler::SECURITY_COUNCIL_ROLE, + account: new_security_council, + sender: security_council_mock() + } + ); + let contract_events = EventsFilterBuilderTrait::from_events(@spy.get_events()) + .with_contract_address(protocol_handler.contract_address) + .build(); + contract_events.assert_emitted(@expected_revoked); + contract_events.assert_emitted(@expected_granted); + + // Check the new security council is set in the contract state + let loaded = load(protocol_handler.contract_address, selector!("security_council"), 1); + assert_eq!(*loaded[0], new_security_council.into(), "New SC not set"); +} + +#[test] +#[should_panic(expected: 'Caller is missing role')] +fn test_protocol_handler_change_gas_price_admin_should_fail_wrong_caller() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to random caller address + let random_caller = contract_address_const::<'random_caller'>(); + start_cheat_caller_address(protocol_handler.contract_address, random_caller); + + // Call the protocol handler change_gas_price_admin, should fail as caller is not security + // council + let new_gas_price_admin = contract_address_const::<'new_gas_price_admin'>(); + protocol_handler.change_gas_price_admin(new_gas_price_admin); +} + +#[test] +fn test_protocol_handler_change_gas_price_admin_should_pass() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to security council + start_cheat_caller_address(protocol_handler.contract_address, security_council_mock()); + + // Spy on the events + let mut spy = spy_events(); + + // Call the protocol handler change_gas_price_admin + let new_gas_price_admin = contract_address_const::<'new_gas_price_admin'>(); + protocol_handler.change_gas_price_admin(new_gas_price_admin); + + // Check the old gas price admin is revoked and the new gas price admin is granted + let access_control_dispatcher = IAccessControlDispatcher { + contract_address: protocol_handler.contract_address + }; + assert( + !access_control_dispatcher + .has_role(ProtocolHandler::GAS_PRICE_ADMIN_ROLE, gas_price_admin_mock()), + 'Old GPA not revoked' + ); + assert( + access_control_dispatcher + .has_role(ProtocolHandler::GAS_PRICE_ADMIN_ROLE, new_gas_price_admin), + 'New GPA not granted' + ); + + // Check the Access control related events are emitted + let expected_revoked = AccessControlComponent::Event::RoleRevoked( + RoleRevoked { + role: ProtocolHandler::GAS_PRICE_ADMIN_ROLE, + account: gas_price_admin_mock(), + sender: security_council_mock() + } + ); + let expected_granted = AccessControlComponent::Event::RoleGranted( + RoleGranted { + role: ProtocolHandler::GAS_PRICE_ADMIN_ROLE, + account: new_gas_price_admin, + sender: security_council_mock() + } + ); + let contract_events = EventsFilterBuilderTrait::from_events(@spy.get_events()) + .with_contract_address(protocol_handler.contract_address) + .build(); + contract_events.assert_emitted(@expected_revoked); + contract_events.assert_emitted(@expected_granted); + + // Check the new gas price admin is set in the contract state + let loaded = load(protocol_handler.contract_address, selector!("gas_price_admin"), 1); + assert_eq!(*loaded[0], new_gas_price_admin.into(), "New GPA not set"); +} + +#[test] +#[should_panic(expected: 'Caller is missing role')] +fn test_protocol_handler_add_guardian_should_fail_wrong_caller() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to random caller address + let random_caller = contract_address_const::<'random_caller'>(); + start_cheat_caller_address(protocol_handler.contract_address, random_caller); + + // Call the protocol handler add_guardian, should fail as caller is not security council + let new_guardian = contract_address_const::<'new_guardian'>(); + protocol_handler.add_guardian(new_guardian); +} + +#[test] +fn test_protocol_handler_add_guardian_should_pass() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to security council + start_cheat_caller_address(protocol_handler.contract_address, security_council_mock()); + + // Spy on the events + let mut spy = spy_events(); + + // Call the protocol handler add_guardian + let new_guardian = contract_address_const::<'new_guardian'>(); + protocol_handler.add_guardian(new_guardian); + + // Check the new guardian is granted + let access_control_dispatcher = IAccessControlDispatcher { + contract_address: protocol_handler.contract_address + }; + assert( + access_control_dispatcher.has_role(ProtocolHandler::GUARDIAN_ROLE, new_guardian), + 'New guardian not granted' + ); + + // Check the Access control related events are emitted + let expected_granted = AccessControlComponent::Event::RoleGranted( + RoleGranted { + role: ProtocolHandler::GUARDIAN_ROLE, + account: new_guardian, + sender: security_council_mock() + } + ); + let contract_events = EventsFilterBuilderTrait::from_events(@spy.get_events()) + .with_contract_address(protocol_handler.contract_address) + .build(); + contract_events.assert_emitted(@expected_granted); +} + +#[test] +#[should_panic(expected: 'Caller is missing role')] +fn test_protocol_handler_remove_guardian_should_fail_wrong_caller() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to random caller address + let random_caller = contract_address_const::<'random_caller'>(); + start_cheat_caller_address(protocol_handler.contract_address, random_caller); + + // Call the protocol handler remove_guardian, should fail as caller is not security council + let guardian = guardians_mock()[0]; + protocol_handler.remove_guardian(*guardian); +} + +#[test] +fn test_protocol_handler_remove_guardian_should_pass() { + let (protocol_handler, _) = setup_contracts_for_testing(); + + // Change caller to security council + start_cheat_caller_address(protocol_handler.contract_address, security_council_mock()); + + // Spy on the events + let mut spy = spy_events(); + + // Call the protocol handler remove_guardian + let guardian = guardians_mock()[0]; + protocol_handler.remove_guardian(*guardian); + + // Check the guardian is revoked + let access_control_dispatcher = IAccessControlDispatcher { + contract_address: protocol_handler.contract_address + }; + assert( + !access_control_dispatcher.has_role(ProtocolHandler::GUARDIAN_ROLE, *guardian), + 'Guardian not revoked' + ); + + // Check the Access control related events are emitted + let expected_revoked = AccessControlComponent::Event::RoleRevoked( + RoleRevoked { + role: ProtocolHandler::GUARDIAN_ROLE, + account: *guardian, + sender: security_council_mock() + } + ); + let contract_events = EventsFilterBuilderTrait::from_events(@spy.get_events()) + .with_contract_address(protocol_handler.contract_address) + .build(); + contract_events.assert_emitted(@expected_revoked); +} From fa028ebc01ca22f08505ef272b21abf6e6bf85c4 Mon Sep 17 00:00:00 2001 From: Oba Date: Mon, 25 Nov 2024 15:02:49 +0100 Subject: [PATCH 2/3] test: use assert macro --- cairo/protocol_handler/tests/test_protocol_handler.cairo | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cairo/protocol_handler/tests/test_protocol_handler.cairo b/cairo/protocol_handler/tests/test_protocol_handler.cairo index 1bb6e97b9..f63abf8a4 100644 --- a/cairo/protocol_handler/tests/test_protocol_handler.cairo +++ b/cairo/protocol_handler/tests/test_protocol_handler.cairo @@ -656,13 +656,11 @@ fn test_protocol_handler_change_operator_should_pass() { let access_control_dispatcher = IAccessControlDispatcher { contract_address: protocol_handler.contract_address }; - assert( + assert!( !access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, operator_mock()), - 'Old operator not revoked' ); - assert( + assert!( access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, new_operator), - 'New operator not granted' ); // Check the Access control related events are emitted @@ -688,7 +686,7 @@ fn test_protocol_handler_change_operator_should_pass() { // Check the new operator is set in the contract state let loaded = load(protocol_handler.contract_address, selector!("operator"), 1); - assert_eq!(*loaded[0], new_operator.into(), "New operator not set"); + assert_eq!(*loaded[0], new_operator.into()); } From 6604148260b09bcd8d734eed09cdb84f160c901e Mon Sep 17 00:00:00 2001 From: Oba Date: Mon, 25 Nov 2024 17:00:34 +0100 Subject: [PATCH 3/3] Add guardians Vec --- .../src/protocol_handler.cairo | 27 +++++++++-- .../tests/test_protocol_handler.cairo | 47 ++++++++++++++----- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/cairo/protocol_handler/src/protocol_handler.cairo b/cairo/protocol_handler/src/protocol_handler.cairo index 3a6a86074..d05e69086 100644 --- a/cairo/protocol_handler/src/protocol_handler.cairo +++ b/cairo/protocol_handler/src/protocol_handler.cairo @@ -123,10 +123,12 @@ pub trait IProtocolHandler { pub mod ProtocolHandler { use starknet::event::EventEmitter; use starknet::account::Call; - use starknet::{ContractAddress, ClassHash, get_block_timestamp, SyscallResultTrait}; + use starknet::{ + ContractAddress, contract_address_const, ClassHash, get_block_timestamp, SyscallResultTrait + }; use starknet::storage::{ Map, StoragePointerReadAccess, StoragePointerWriteAccess, StorageMapReadAccess, - StorageMapWriteAccess + StorageMapWriteAccess, Vec, MutableVecTrait }; use openzeppelin_access::accesscontrol::AccessControlComponent; use openzeppelin_introspection::src5::SRC5Component; @@ -179,7 +181,7 @@ pub mod ProtocolHandler { pub kakarot: IKakarotDispatcher, pub security_council: ContractAddress, pub operator: ContractAddress, - pub guardians: Map, + pub guardians: Vec, pub gas_price_admin: ContractAddress, pub protocol_frozen_until: u64, pub authorized_operator_selector: Map, @@ -261,11 +263,14 @@ pub mod ProtocolHandler { gas_price_admin: ContractAddress, mut guardians: Span ) { - // Store the Kakarot, security council, operator and gas price admin addresses + // Store the Kakarot, security council, operator, gas price admin and guardians addresses self.kakarot.write(IKakarotDispatcher { contract_address: kakarot }); self.security_council.write(security_council); self.operator.write(operator); self.gas_price_admin.write(gas_price_admin); + for guardian in guardians { + self.guardians.append().write(*guardian); + }; // Store the authorized selectors for the operator self.authorized_operator_selector.write(selector!("set_native_token"), true); @@ -492,6 +497,9 @@ pub mod ProtocolHandler { // Grant the GUARDIAN_ROLE to the new guardian self.accesscontrol._grant_role(GUARDIAN_ROLE, new_guardian_address); + + // Add the guardian to the guardians list + self.guardians.append().write(new_guardian_address) } fn remove_guardian(ref self: ContractState, guardian_address: ContractAddress) { @@ -500,6 +508,17 @@ pub mod ProtocolHandler { // Revoke the GUARDIAN_ROLE from the guardian self.accesscontrol._revoke_role(GUARDIAN_ROLE, guardian_address); + + // Remove the guardian from the guardians list + for i in 0 + ..self + .guardians + .len() { + let guardian = self.guardians.at(i).read(); + if guardian == guardian_address { + self.guardians.at(i).write(contract_address_const::<0>()); + } + }; } } } diff --git a/cairo/protocol_handler/tests/test_protocol_handler.cairo b/cairo/protocol_handler/tests/test_protocol_handler.cairo index f63abf8a4..304052b15 100644 --- a/cairo/protocol_handler/tests/test_protocol_handler.cairo +++ b/cairo/protocol_handler/tests/test_protocol_handler.cairo @@ -1,3 +1,5 @@ +use core::hash::{HashStateExTrait, HashStateTrait}; +use core::pedersen::PedersenTrait; use snforge_std::{ ContractClassTrait, ContractClass, declare, DeclareResultTrait, EventSpyTrait, start_cheat_block_timestamp_global, start_cheat_caller_address, mock_call, spy_events, store, @@ -656,12 +658,8 @@ fn test_protocol_handler_change_operator_should_pass() { let access_control_dispatcher = IAccessControlDispatcher { contract_address: protocol_handler.contract_address }; - assert!( - !access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, operator_mock()), - ); - assert!( - access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, new_operator), - ); + assert!(!access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, operator_mock()),); + assert!(access_control_dispatcher.has_role(ProtocolHandler::OPERATOR_ROLE, new_operator),); // Check the Access control related events are emitted let expected_revoked = AccessControlComponent::Event::RoleRevoked( @@ -879,6 +877,20 @@ fn test_protocol_handler_add_guardian_should_pass() { .with_contract_address(protocol_handler.contract_address) .build(); contract_events.assert_emitted(@expected_granted); + + // Check the guardian is added to the list + let length = load( + protocol_handler.contract_address, + selector!("guardians"), + 1 + ); + assert_eq!(*length[0], (guardians_mock().len() + 1).into(), "Guardian not added"); + // get storage slot for guardian + let storage_slot = PedersenTrait::new(selector!("guardians")).update_with(2).finalize(); + let guardian3 = load( + protocol_handler.contract_address, storage_slot, 1 + ); + assert_eq!(*guardian3[0], new_guardian.into(), "Guardian not added"); } #[test] @@ -905,16 +917,23 @@ fn test_protocol_handler_remove_guardian_should_pass() { // Spy on the events let mut spy = spy_events(); - // Call the protocol handler remove_guardian - let guardian = guardians_mock()[0]; - protocol_handler.remove_guardian(*guardian); + // Ensure the guardian is in the list + let guardian_removed = *(guardians_mock()[0]); + let storage_slot = PedersenTrait::new(selector!("guardians")).update_with(0).finalize(); + let guardian_loaded = load( + protocol_handler.contract_address, storage_slot, 1 + ); + assert_eq!(*guardian_loaded[0], guardian_removed.into(), "Guardian not in Vec"); + + // Remove the guardian + protocol_handler.remove_guardian(guardian_removed); // Check the guardian is revoked let access_control_dispatcher = IAccessControlDispatcher { contract_address: protocol_handler.contract_address }; assert( - !access_control_dispatcher.has_role(ProtocolHandler::GUARDIAN_ROLE, *guardian), + !access_control_dispatcher.has_role(ProtocolHandler::GUARDIAN_ROLE, guardian_removed), 'Guardian not revoked' ); @@ -922,7 +941,7 @@ fn test_protocol_handler_remove_guardian_should_pass() { let expected_revoked = AccessControlComponent::Event::RoleRevoked( RoleRevoked { role: ProtocolHandler::GUARDIAN_ROLE, - account: *guardian, + account: guardian_removed, sender: security_council_mock() } ); @@ -930,4 +949,10 @@ fn test_protocol_handler_remove_guardian_should_pass() { .with_contract_address(protocol_handler.contract_address) .build(); contract_events.assert_emitted(@expected_revoked); + + // Check the guardian is set to 0 in the guardian list + let guardian_loaded = load( + protocol_handler.contract_address, storage_slot, 1 + ); + assert_eq!(*guardian_loaded[0], 0, "Guardian not in Vec"); }