From 22aafa42c4de1f0778492f94cec5ce21e66d1f66 Mon Sep 17 00:00:00 2001 From: immrsd Date: Tue, 9 Jul 2024 15:08:04 +0200 Subject: [PATCH 1/8] Migrate AccessControl tests --- src/tests.cairo | 4 +- src/tests/access.cairo | 9 +- src/tests/access/test_accesscontrol.cairo | 184 +++++++++++++--------- 3 files changed, 114 insertions(+), 83 deletions(-) diff --git a/src/tests.cairo b/src/tests.cairo index eceac55c6..c7cc28f22 100644 --- a/src/tests.cairo +++ b/src/tests.cairo @@ -1,5 +1,5 @@ -// #[cfg(test)] -// mod access; +#[cfg(test)] +mod access; // #[cfg(test)] // mod account; // #[cfg(test)] diff --git a/src/tests/access.cairo b/src/tests/access.cairo index cfa981899..f479ae445 100644 --- a/src/tests/access.cairo +++ b/src/tests/access.cairo @@ -1,7 +1,8 @@ -pub(crate) mod common; +// pub(crate) mod common; mod test_accesscontrol; mod test_dual_accesscontrol; -mod test_dual_ownable; -mod test_ownable; -mod test_ownable_twostep; +// mod test_dual_ownable; +// mod test_ownable; +// mod test_ownable_twostep; + diff --git a/src/tests/access/test_accesscontrol.cairo b/src/tests/access/test_accesscontrol.cairo index 35a76c136..dcb0ea5e8 100644 --- a/src/tests/access/test_accesscontrol.cairo +++ b/src/tests/access/test_accesscontrol.cairo @@ -11,9 +11,10 @@ use openzeppelin::tests::mocks::accesscontrol_mocks::DualCaseAccessControlMock; use openzeppelin::tests::utils::constants::{ ADMIN, AUTHORIZED, OTHER, OTHER_ADMIN, ROLE, OTHER_ROLE, ZERO }; +use openzeppelin::tests::utils::events::EventSpyExt; use openzeppelin::tests::utils; +use snforge_std::{EventSpy, spy_events, start_cheat_caller_address, test_address}; use starknet::ContractAddress; -use starknet::testing; // // Setup @@ -33,7 +34,6 @@ fn COMPONENT_STATE() -> ComponentState { fn setup() -> ComponentState { let mut state = COMPONENT_STATE(); state._grant_role(DEFAULT_ADMIN_ROLE, ADMIN()); - utils::drop_event(ZERO()); state } @@ -76,10 +76,11 @@ fn test_hasRole() { #[test] fn test_assert_only_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(contract_address, AUTHORIZED()); state.assert_only_role(ROLE); } @@ -87,7 +88,7 @@ fn test_assert_only_role() { #[should_panic(expected: ('Caller is missing role',))] fn test_assert_only_role_unauthorized() { let state = setup(); - testing::set_caller_address(OTHER()); + start_cheat_caller_address(test_address(), OTHER()); state.assert_only_role(ROLE); } @@ -97,7 +98,7 @@ fn test_assert_only_role_unauthorized_when_authorized_for_another_role() { let mut state = setup(); state.grant_role(ROLE, AUTHORIZED()); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(test_address(), AUTHORIZED()); state.assert_only_role(OTHER_ROLE); } @@ -108,10 +109,12 @@ fn test_assert_only_role_unauthorized_when_authorized_for_another_role() { #[test] fn test_grant_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let mut spy = spy_events(); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - assert_event_role_granted(ROLE, AUTHORIZED(), ADMIN()); + spy.assert_event_role_granted(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_role = state.has_role(ROLE, AUTHORIZED()); assert!(has_role); @@ -120,10 +123,12 @@ fn test_grant_role() { #[test] fn test_grantRole() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let mut spy = spy_events(); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grantRole(ROLE, AUTHORIZED()); - assert_event_role_granted(ROLE, AUTHORIZED(), ADMIN()); + spy.assert_event_role_granted(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_role = state.hasRole(ROLE, AUTHORIZED()); assert!(has_role); @@ -132,7 +137,7 @@ fn test_grantRole() { #[test] fn test_grant_role_multiple_times_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.grant_role(ROLE, AUTHORIZED()); state.grant_role(ROLE, AUTHORIZED()); @@ -142,7 +147,7 @@ fn test_grant_role_multiple_times_for_granted_role() { #[test] fn test_grantRole_multiple_times_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.grantRole(ROLE, AUTHORIZED()); state.grantRole(ROLE, AUTHORIZED()); @@ -153,7 +158,7 @@ fn test_grantRole_multiple_times_for_granted_role() { #[should_panic(expected: ('Caller is missing role',))] fn test_grant_role_unauthorized() { let mut state = setup(); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(test_address(), AUTHORIZED()); state.grant_role(ROLE, AUTHORIZED()); } @@ -161,7 +166,7 @@ fn test_grant_role_unauthorized() { #[should_panic(expected: ('Caller is missing role',))] fn test_grantRole_unauthorized() { let mut state = setup(); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(test_address(), AUTHORIZED()); state.grantRole(ROLE, AUTHORIZED()); } @@ -172,28 +177,29 @@ fn test_grantRole_unauthorized() { #[test] fn test_revoke_role_for_role_not_granted() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.revoke_role(ROLE, AUTHORIZED()); } #[test] fn test_revokeRole_for_role_not_granted() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.revokeRole(ROLE, AUTHORIZED()); } #[test] fn test_revoke_role_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - utils::drop_event(ZERO()); + let mut spy = spy_events(); state.revoke_role(ROLE, AUTHORIZED()); - assert_event_role_revoked(ROLE, AUTHORIZED(), ADMIN()); + spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_not_role = !state.has_role(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -202,14 +208,15 @@ fn test_revoke_role_for_granted_role() { #[test] fn test_revokeRole_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grantRole(ROLE, AUTHORIZED()); - utils::drop_event(ZERO()); + let mut spy = spy_events(); state.revokeRole(ROLE, AUTHORIZED()); - assert_event_role_revoked(ROLE, AUTHORIZED(), ADMIN()); + spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_not_role = !state.hasRole(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -218,7 +225,7 @@ fn test_revokeRole_for_granted_role() { #[test] fn test_revoke_role_multiple_times_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.grant_role(ROLE, AUTHORIZED()); state.revoke_role(ROLE, AUTHORIZED()); @@ -231,7 +238,7 @@ fn test_revoke_role_multiple_times_for_granted_role() { #[test] fn test_revokeRole_multiple_times_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.grantRole(ROLE, AUTHORIZED()); state.revokeRole(ROLE, AUTHORIZED()); @@ -245,7 +252,7 @@ fn test_revokeRole_multiple_times_for_granted_role() { #[should_panic(expected: ('Caller is missing role',))] fn test_revoke_role_unauthorized() { let mut state = setup(); - testing::set_caller_address(OTHER()); + start_cheat_caller_address(test_address(), OTHER()); state.revoke_role(ROLE, AUTHORIZED()); } @@ -253,7 +260,7 @@ fn test_revoke_role_unauthorized() { #[should_panic(expected: ('Caller is missing role',))] fn test_revokeRole_unauthorized() { let mut state = setup(); - testing::set_caller_address(OTHER()); + start_cheat_caller_address(test_address(), OTHER()); state.revokeRole(ROLE, AUTHORIZED()); } @@ -264,29 +271,30 @@ fn test_revokeRole_unauthorized() { #[test] fn test_renounce_role_for_role_not_granted() { let mut state = setup(); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(test_address(), AUTHORIZED()); state.renounce_role(ROLE, AUTHORIZED()); } #[test] fn test_renounceRole_for_role_not_granted() { let mut state = setup(); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(test_address(), AUTHORIZED()); state.renounceRole(ROLE, AUTHORIZED()); } #[test] fn test_renounce_role_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - utils::drop_event(ZERO()); - testing::set_caller_address(AUTHORIZED()); + let mut spy = spy_events(); + start_cheat_caller_address(contract_address, AUTHORIZED()); state.renounce_role(ROLE, AUTHORIZED()); - assert_event_role_revoked(ROLE, AUTHORIZED(), AUTHORIZED()); + spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), AUTHORIZED()); let has_not_role = !state.has_role(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -295,15 +303,16 @@ fn test_renounce_role_for_granted_role() { #[test] fn test_renounceRole_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grantRole(ROLE, AUTHORIZED()); - utils::drop_event(ZERO()); - testing::set_caller_address(AUTHORIZED()); + let mut spy = spy_events(); + start_cheat_caller_address(contract_address, AUTHORIZED()); state.renounceRole(ROLE, AUTHORIZED()); - assert_event_role_revoked(ROLE, AUTHORIZED(), AUTHORIZED()); + spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), AUTHORIZED()); let has_not_role = !state.hasRole(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -312,10 +321,11 @@ fn test_renounceRole_for_granted_role() { #[test] fn test_renounce_role_multiple_times_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(contract_address, AUTHORIZED()); state.renounce_role(ROLE, AUTHORIZED()); state.renounce_role(ROLE, AUTHORIZED()); @@ -326,10 +336,11 @@ fn test_renounce_role_multiple_times_for_granted_role() { #[test] fn test_renounceRole_multiple_times_for_granted_role() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grantRole(ROLE, AUTHORIZED()); - testing::set_caller_address(AUTHORIZED()); + start_cheat_caller_address(contract_address, AUTHORIZED()); state.renounceRole(ROLE, AUTHORIZED()); state.renounceRole(ROLE, AUTHORIZED()); @@ -341,10 +352,11 @@ fn test_renounceRole_multiple_times_for_granted_role() { #[should_panic(expected: ('Can only renounce role for self',))] fn test_renounce_role_unauthorized() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + let contract_address = test_address(); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - testing::set_caller_address(ZERO()); + start_cheat_caller_address(contract_address, ZERO()); state.renounce_role(ROLE, AUTHORIZED()); } @@ -352,7 +364,7 @@ fn test_renounce_role_unauthorized() { #[should_panic(expected: ('Can only renounce role for self',))] fn test_renounceRole_unauthorized() { let mut state = setup(); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.grantRole(ROLE, AUTHORIZED()); // Admin is unauthorized caller @@ -366,10 +378,13 @@ fn test_renounceRole_unauthorized() { #[test] fn test_set_role_admin() { let mut state = setup(); + let contract_address = test_address(); + let mut spy = spy_events(); + assert_eq!(state.get_role_admin(ROLE), DEFAULT_ADMIN_ROLE); state.set_role_admin(ROLE, OTHER_ROLE); - assert_event_role_admin_changed(ROLE, DEFAULT_ADMIN_ROLE, OTHER_ROLE); + spy.assert_event_role_admin_changed(contract_address, ROLE, DEFAULT_ADMIN_ROLE, OTHER_ROLE); let current_admin_role = state.get_role_admin(ROLE); assert_eq!(current_admin_role, OTHER_ROLE); @@ -378,12 +393,13 @@ fn test_set_role_admin() { #[test] fn test_new_admin_can_grant_roles() { let mut state = setup(); + let contract_address = test_address(); state.set_role_admin(ROLE, OTHER_ROLE); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(OTHER_ROLE, OTHER_ADMIN()); - testing::set_caller_address(OTHER_ADMIN()); + start_cheat_caller_address(contract_address, OTHER_ADMIN()); state.grant_role(ROLE, AUTHORIZED()); let has_role = state.has_role(ROLE, AUTHORIZED()); @@ -393,12 +409,13 @@ fn test_new_admin_can_grant_roles() { #[test] fn test_new_admin_can_revoke_roles() { let mut state = setup(); + let contract_address = test_address(); state.set_role_admin(ROLE, OTHER_ROLE); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(OTHER_ROLE, OTHER_ADMIN()); - testing::set_caller_address(OTHER_ADMIN()); + start_cheat_caller_address(contract_address, OTHER_ADMIN()); state.grant_role(ROLE, AUTHORIZED()); state.revoke_role(ROLE, AUTHORIZED()); @@ -411,7 +428,7 @@ fn test_new_admin_can_revoke_roles() { fn test_previous_admin_cannot_grant_roles() { let mut state = setup(); state.set_role_admin(ROLE, OTHER_ROLE); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.grant_role(ROLE, AUTHORIZED()); } @@ -420,7 +437,7 @@ fn test_previous_admin_cannot_grant_roles() { fn test_previous_admin_cannot_revoke_roles() { let mut state = setup(); state.set_role_admin(ROLE, OTHER_ROLE); - testing::set_caller_address(ADMIN()); + start_cheat_caller_address(test_address(), ADMIN()); state.revoke_role(ROLE, AUTHORIZED()); } @@ -447,31 +464,44 @@ fn test_default_admin_role_is_its_own_admin() { // Helpers // -fn assert_event_role_revoked(role: felt252, account: ContractAddress, sender: ContractAddress) { - let event = utils::pop_log::(ZERO()).unwrap(); - let expected = AccessControlComponent::Event::RoleRevoked( - RoleRevoked { role, account, sender } - ); - assert!(event == expected); - utils::assert_no_events_left(ZERO()); -} - -fn assert_event_role_granted(role: felt252, account: ContractAddress, sender: ContractAddress) { - let event = utils::pop_log::(ZERO()).unwrap(); - let expected = AccessControlComponent::Event::RoleGranted( - RoleGranted { role, account, sender } - ); - assert!(event == expected); - utils::assert_no_events_left(ZERO()); -} - -fn assert_event_role_admin_changed( - role: felt252, previous_admin_role: felt252, new_admin_role: felt252 -) { - let event = utils::pop_log::(ZERO()).unwrap(); - let expected = AccessControlComponent::Event::RoleAdminChanged( - RoleAdminChanged { role, previous_admin_role, new_admin_role } - ); - assert!(event == expected); - utils::assert_no_events_left(ZERO()); +#[generate_trait] +impl AccessControlSpyHelpersImpl of AccessControlSpyHelpers { + fn assert_event_role_revoked( + ref self: EventSpy, + from_address: ContractAddress, + role: felt252, + account: ContractAddress, + sender: ContractAddress + ) { + let expected = AccessControlComponent::Event::RoleRevoked( + RoleRevoked { role, account, sender } + ); + self.assert_only_event(from_address, expected); + } + + fn assert_event_role_granted( + ref self: EventSpy, + from_address: ContractAddress, + role: felt252, + account: ContractAddress, + sender: ContractAddress + ) { + let expected = AccessControlComponent::Event::RoleGranted( + RoleGranted { role, account, sender } + ); + self.assert_only_event(from_address, expected); + } + + fn assert_event_role_admin_changed( + ref self: EventSpy, + from_address: ContractAddress, + role: felt252, + previous_admin_role: felt252, + new_admin_role: felt252 + ) { + let expected = AccessControlComponent::Event::RoleAdminChanged( + RoleAdminChanged { role, previous_admin_role, new_admin_role } + ); + self.assert_only_event(from_address, expected); + } } From ee09fbf531c3b0559fc696afa8e902c153bc2960 Mon Sep 17 00:00:00 2001 From: immrsd Date: Tue, 9 Jul 2024 15:08:32 +0200 Subject: [PATCH 2/8] Migrate AccessControl Dual Dispatcher tests --- .../access/test_dual_accesscontrol.cairo | 121 +++++++++--------- 1 file changed, 63 insertions(+), 58 deletions(-) diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo index e172384a7..d9c66d5f6 100644 --- a/src/tests/access/test_dual_accesscontrol.cairo +++ b/src/tests/access/test_dual_accesscontrol.cairo @@ -1,22 +1,13 @@ use openzeppelin::access::accesscontrol::DEFAULT_ADMIN_ROLE; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControl; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControlTrait; -use openzeppelin::access::accesscontrol::interface::IACCESSCONTROL_ID; -use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcher; -use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcherTrait; -use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcher; -use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcherTrait; -use openzeppelin::tests::mocks::accesscontrol_mocks::{ - CamelAccessControlMock, SnakeAccessControlMock, CamelAccessControlPanicMock, - SnakeAccessControlPanicMock -}; +use openzeppelin::access::accesscontrol::interface::{IACCESSCONTROL_ID, IAccessControlDispatcher, IAccessControlDispatcherTrait, IAccessControlCamelDispatcher, IAccessControlCamelDispatcherTrait}; use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; use openzeppelin::tests::utils::constants::{ADMIN, AUTHORIZED, ROLE}; -use openzeppelin::tests::utils; use openzeppelin::utils::serde::SerializedAppend; use starknet::ContractAddress; -use starknet::contract_address_const; -use starknet::testing::set_contract_address; +use openzeppelin::tests::utils; +use snforge_std::{start_cheat_caller_address, test_address}; // // Setup @@ -25,7 +16,7 @@ use starknet::testing::set_contract_address; fn setup_snake() -> (DualCaseAccessControl, IAccessControlDispatcher) { let mut calldata = array![]; calldata.append_serde(ADMIN()); - let target = utils::deploy(SnakeAccessControlMock::TEST_CLASS_HASH, calldata); + let target = utils::declare_and_deploy("SnakeAccessControlMock", calldata); ( DualCaseAccessControl { contract_address: target }, IAccessControlDispatcher { contract_address: target } @@ -35,7 +26,7 @@ fn setup_snake() -> (DualCaseAccessControl, IAccessControlDispatcher) { fn setup_camel() -> (DualCaseAccessControl, IAccessControlCamelDispatcher) { let mut calldata = array![]; calldata.append_serde(ADMIN()); - let target = utils::deploy(CamelAccessControlMock::TEST_CLASS_HASH, calldata); + let target = utils::declare_and_deploy("CamelAccessControlMock", calldata); ( DualCaseAccessControl { contract_address: target }, IAccessControlCamelDispatcher { contract_address: target } @@ -43,13 +34,13 @@ fn setup_camel() -> (DualCaseAccessControl, IAccessControlCamelDispatcher) { } fn setup_non_accesscontrol() -> DualCaseAccessControl { - let target = utils::deploy(NonImplementingMock::TEST_CLASS_HASH, array![]); + let target = utils::declare_and_deploy("NonImplementingMock", array![]); DualCaseAccessControl { contract_address: target } } fn setup_accesscontrol_panic() -> (DualCaseAccessControl, DualCaseAccessControl) { - let snake_target = utils::deploy(SnakeAccessControlPanicMock::TEST_CLASS_HASH, array![]); - let camel_target = utils::deploy(CamelAccessControlPanicMock::TEST_CLASS_HASH, array![]); + let snake_target = utils::declare_and_deploy("SnakeAccessControlPanicMock", array![]); + let camel_target = utils::declare_and_deploy("CamelAccessControlPanicMock", array![]); ( DualCaseAccessControl { contract_address: snake_target }, DualCaseAccessControl { contract_address: camel_target } @@ -68,6 +59,7 @@ fn test_dual_supports_interface() { } #[test] +#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_supports_interface() { let dispatcher = setup_non_accesscontrol(); @@ -75,20 +67,21 @@ fn test_dual_no_supports_interface() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[should_panic(expected: ("Some error",))] fn test_dual_supports_interface_exists_and_panics() { - let (dispatcher, _) = setup_accesscontrol_panic(); - dispatcher.supports_interface(IACCESSCONTROL_ID); + let (snake_dispatcher, _) = setup_accesscontrol_panic(); + snake_dispatcher.supports_interface(IACCESSCONTROL_ID); } #[test] fn test_dual_has_role() { - let (dispatcher, _) = setup_snake(); - let has_role = dispatcher.has_role(DEFAULT_ADMIN_ROLE, ADMIN()); + let (snake_dispatcher, _) = setup_snake(); + let has_role = snake_dispatcher.has_role(DEFAULT_ADMIN_ROLE, ADMIN()); assert!(has_role); } #[test] +#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_has_role() { let dispatcher = setup_non_accesscontrol(); @@ -96,7 +89,7 @@ fn test_dual_no_has_role() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[should_panic(expected: ("Some error",))] fn test_dual_has_role_exists_and_panics() { let (dispatcher, _) = setup_accesscontrol_panic(); dispatcher.has_role(DEFAULT_ADMIN_ROLE, ADMIN()); @@ -105,12 +98,12 @@ fn test_dual_has_role_exists_and_panics() { #[test] fn test_dual_get_role_admin() { let (dispatcher, _) = setup_snake(); - let current_admin_role = dispatcher.get_role_admin(ROLE); assert_eq!(current_admin_role, DEFAULT_ADMIN_ROLE); } #[test] +#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_get_role_admin() { let dispatcher = setup_non_accesscontrol(); @@ -118,16 +111,16 @@ fn test_dual_no_get_role_admin() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[should_panic(expected: ("Some error",))] fn test_dual_get_role_admin_exists_and_panics() { - let (dispatcher, _) = setup_accesscontrol_panic(); - dispatcher.get_role_admin(ROLE); + let (snake_dispatcher, _) = setup_accesscontrol_panic(); + snake_dispatcher.get_role_admin(ROLE); } #[test] fn test_dual_grant_role() { let (dispatcher, target) = setup_snake(); - set_contract_address(ADMIN()); + start_cheat_caller_address(target.contract_address, ADMIN()); dispatcher.grant_role(ROLE, AUTHORIZED()); let has_role = target.has_role(ROLE, AUTHORIZED()); @@ -135,6 +128,7 @@ fn test_dual_grant_role() { } #[test] +#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_grant_role() { let dispatcher = setup_non_accesscontrol(); @@ -142,16 +136,16 @@ fn test_dual_no_grant_role() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[should_panic(expected: ("Some error",))] fn test_dual_grant_role_exists_and_panics() { - let (dispatcher, _) = setup_accesscontrol_panic(); - dispatcher.grant_role(ROLE, AUTHORIZED()); + let (snake_dispatcher, _) = setup_accesscontrol_panic(); + snake_dispatcher.grant_role(ROLE, AUTHORIZED()); } #[test] fn test_dual_revoke_role() { let (dispatcher, target) = setup_snake(); - set_contract_address(ADMIN()); + start_cheat_caller_address(target.contract_address, ADMIN()); dispatcher.revoke_role(ROLE, AUTHORIZED()); let has_not_role = !target.has_role(ROLE, AUTHORIZED()); @@ -159,6 +153,7 @@ fn test_dual_revoke_role() { } #[test] +#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_revoke_role() { let dispatcher = setup_non_accesscontrol(); @@ -166,16 +161,16 @@ fn test_dual_no_revoke_role() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[should_panic(expected: ("Some error",))] fn test_dual_revoke_role_exists_and_panics() { - let (dispatcher, _) = setup_accesscontrol_panic(); - dispatcher.revoke_role(ROLE, AUTHORIZED()); + let (snake_dispatcher, _) = setup_accesscontrol_panic(); + snake_dispatcher.revoke_role(ROLE, AUTHORIZED()); } #[test] fn test_dual_renounce_role() { let (dispatcher, target) = setup_snake(); - set_contract_address(ADMIN()); + start_cheat_caller_address(target.contract_address, ADMIN()); dispatcher.renounce_role(DEFAULT_ADMIN_ROLE, ADMIN()); let has_not_role = !target.has_role(DEFAULT_ADMIN_ROLE, ADMIN()); @@ -183,6 +178,7 @@ fn test_dual_renounce_role() { } #[test] +#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_renounce_role() { let dispatcher = setup_non_accesscontrol(); @@ -190,10 +186,10 @@ fn test_dual_no_renounce_role() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[should_panic(expected: ("Some error",))] fn test_dual_renounce_role_exists_and_panics() { - let (dispatcher, _) = setup_accesscontrol_panic(); - dispatcher.renounce_role(DEFAULT_ADMIN_ROLE, ADMIN()); + let (snake_dispatcher, _) = setup_accesscontrol_panic(); + snake_dispatcher.renounce_role(DEFAULT_ADMIN_ROLE, ADMIN()); } // @@ -201,6 +197,7 @@ fn test_dual_renounce_role_exists_and_panics() { // #[test] +#[ignore] // TODO: Enable when try/catch is supported fn test_dual_hasRole() { let (dispatcher, _) = setup_camel(); @@ -209,13 +206,15 @@ fn test_dual_hasRole() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[ignore] // TODO: Enable when try/catch is supported +#[should_panic(expected: ("Some error",))] fn test_dual_hasRole_exists_and_panics() { - let (_, dispatcher) = setup_accesscontrol_panic(); - dispatcher.has_role(DEFAULT_ADMIN_ROLE, ADMIN()); + let (_, camel_dispatcher) = setup_accesscontrol_panic(); + camel_dispatcher.has_role(DEFAULT_ADMIN_ROLE, ADMIN()); } #[test] +#[ignore] // TODO: Enable when try/catch is supported fn test_dual_getRoleAdmin() { let (dispatcher, _) = setup_camel(); @@ -224,16 +223,18 @@ fn test_dual_getRoleAdmin() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[ignore] // TODO: Enable when try/catch is supported +#[should_panic(expected: ("Some error",))] fn test_dual_getRoleAdmin_exists_and_panics() { - let (_, dispatcher) = setup_accesscontrol_panic(); - dispatcher.get_role_admin(ROLE); + let (_, camel_dispatcher) = setup_accesscontrol_panic(); + camel_dispatcher.get_role_admin(ROLE); } #[test] +#[ignore] // TODO: Enable when try/catch is supported fn test_dual_grantRole() { let (dispatcher, target) = setup_camel(); - set_contract_address(ADMIN()); + start_cheat_caller_address(target.contract_address, ADMIN()); dispatcher.grant_role(ROLE, AUTHORIZED()); let has_role = target.hasRole(ROLE, AUTHORIZED()); @@ -241,16 +242,18 @@ fn test_dual_grantRole() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[ignore] // TODO: Enable when try/catch is supported +#[should_panic(expected: ("Some error",))] fn test_dual_grantRole_exists_and_panics() { - let (_, dispatcher) = setup_accesscontrol_panic(); - dispatcher.grant_role(ROLE, AUTHORIZED()); + let (_, camel_dispatcher) = setup_accesscontrol_panic(); + camel_dispatcher.grant_role(ROLE, AUTHORIZED()); } #[test] +#[ignore] // TODO: Enable when try/catch is supported fn test_dual_revokeRole() { let (dispatcher, target) = setup_camel(); - set_contract_address(ADMIN()); + start_cheat_caller_address(target.contract_address, ADMIN()); dispatcher.grant_role(ROLE, AUTHORIZED()); dispatcher.revoke_role(ROLE, AUTHORIZED()); @@ -259,16 +262,18 @@ fn test_dual_revokeRole() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[ignore] // TODO: Enable when try/catch is supported +#[should_panic(expected: ("Some error",))] fn test_dual_revokeRole_exists_and_panics() { - let (_, dispatcher) = setup_accesscontrol_panic(); - dispatcher.revoke_role(ROLE, AUTHORIZED()); + let (_, camel_dispatcher) = setup_accesscontrol_panic(); + camel_dispatcher.revoke_role(ROLE, AUTHORIZED()); } #[test] +#[ignore] // TODO: Enable when try/catch is supported fn test_dual_renounceRole() { let (dispatcher, target) = setup_camel(); - set_contract_address(ADMIN()); + start_cheat_caller_address(target.contract_address, ADMIN()); dispatcher.renounce_role(DEFAULT_ADMIN_ROLE, ADMIN()); let has_not_role = !target.hasRole(DEFAULT_ADMIN_ROLE, ADMIN()); @@ -276,9 +281,9 @@ fn test_dual_renounceRole() { } #[test] -#[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] +#[ignore] // TODO: Enable when try/catch is supported +#[should_panic(expected: ("Some error",))] fn test_dual_renounceRole_exists_and_panics() { - let (_, dispatcher) = setup_accesscontrol_panic(); - dispatcher.renounce_role(DEFAULT_ADMIN_ROLE, ADMIN()); + let (_, camel_dispatcher) = setup_accesscontrol_panic(); + camel_dispatcher.renounce_role(DEFAULT_ADMIN_ROLE, ADMIN()); } - From af290c7b3e9532b810492ca20355e4953c79b51f Mon Sep 17 00:00:00 2001 From: immrsd Date: Tue, 9 Jul 2024 15:16:32 +0200 Subject: [PATCH 3/8] Fix imports --- src/tests/access.cairo | 5 ++--- src/tests/access/test_dual_accesscontrol.cairo | 9 ++++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/tests/access.cairo b/src/tests/access.cairo index f479ae445..ffb1c2c51 100644 --- a/src/tests/access.cairo +++ b/src/tests/access.cairo @@ -1,8 +1,7 @@ // pub(crate) mod common; -mod test_accesscontrol; -mod test_dual_accesscontrol; // mod test_dual_ownable; // mod test_ownable; // mod test_ownable_twostep; - +mod test_accesscontrol; +mod test_dual_accesscontrol; diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo index d9c66d5f6..7f35337a7 100644 --- a/src/tests/access/test_dual_accesscontrol.cairo +++ b/src/tests/access/test_dual_accesscontrol.cairo @@ -1,13 +1,16 @@ use openzeppelin::access::accesscontrol::DEFAULT_ADMIN_ROLE; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControl; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControlTrait; -use openzeppelin::access::accesscontrol::interface::{IACCESSCONTROL_ID, IAccessControlDispatcher, IAccessControlDispatcherTrait, IAccessControlCamelDispatcher, IAccessControlCamelDispatcherTrait}; +use openzeppelin::access::accesscontrol::interface::{ + IACCESSCONTROL_ID, IAccessControlDispatcher, IAccessControlDispatcherTrait, + IAccessControlCamelDispatcher, IAccessControlCamelDispatcherTrait +}; use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; use openzeppelin::tests::utils::constants::{ADMIN, AUTHORIZED, ROLE}; -use openzeppelin::utils::serde::SerializedAppend; -use starknet::ContractAddress; use openzeppelin::tests::utils; +use openzeppelin::utils::serde::SerializedAppend; use snforge_std::{start_cheat_caller_address, test_address}; +use starknet::ContractAddress; // // Setup From 01f01ab19e9c195a6950f1cc0956465cd26aba0f Mon Sep 17 00:00:00 2001 From: immrsd Date: Fri, 12 Jul 2024 20:11:40 +0200 Subject: [PATCH 4/8] Address review comments --- src/tests/access/test_accesscontrol.cairo | 32 +++++++------- .../access/test_dual_accesscontrol.cairo | 43 ++++++++++--------- 2 files changed, 39 insertions(+), 36 deletions(-) diff --git a/src/tests/access/test_accesscontrol.cairo b/src/tests/access/test_accesscontrol.cairo index dcb0ea5e8..36b006c74 100644 --- a/src/tests/access/test_accesscontrol.cairo +++ b/src/tests/access/test_accesscontrol.cairo @@ -12,7 +12,6 @@ use openzeppelin::tests::utils::constants::{ ADMIN, AUTHORIZED, OTHER, OTHER_ADMIN, ROLE, OTHER_ROLE, ZERO }; use openzeppelin::tests::utils::events::EventSpyExt; -use openzeppelin::tests::utils; use snforge_std::{EventSpy, spy_events, start_cheat_caller_address, test_address}; use starknet::ContractAddress; @@ -114,7 +113,7 @@ fn test_grant_role() { start_cheat_caller_address(contract_address, ADMIN()); state.grant_role(ROLE, AUTHORIZED()); - spy.assert_event_role_granted(contract_address, ROLE, AUTHORIZED(), ADMIN()); + spy.assert_only_event_role_granted(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_role = state.has_role(ROLE, AUTHORIZED()); assert!(has_role); @@ -128,7 +127,7 @@ fn test_grantRole() { start_cheat_caller_address(contract_address, ADMIN()); state.grantRole(ROLE, AUTHORIZED()); - spy.assert_event_role_granted(contract_address, ROLE, AUTHORIZED(), ADMIN()); + spy.assert_only_event_role_granted(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_role = state.hasRole(ROLE, AUTHORIZED()); assert!(has_role); @@ -199,7 +198,7 @@ fn test_revoke_role_for_granted_role() { let mut spy = spy_events(); state.revoke_role(ROLE, AUTHORIZED()); - spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), ADMIN()); + spy.assert_only_event_role_revoked(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_not_role = !state.has_role(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -216,7 +215,7 @@ fn test_revokeRole_for_granted_role() { let mut spy = spy_events(); state.revokeRole(ROLE, AUTHORIZED()); - spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), ADMIN()); + spy.assert_only_event_role_revoked(contract_address, ROLE, AUTHORIZED(), ADMIN()); let has_not_role = !state.hasRole(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -294,7 +293,7 @@ fn test_renounce_role_for_granted_role() { start_cheat_caller_address(contract_address, AUTHORIZED()); state.renounce_role(ROLE, AUTHORIZED()); - spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), AUTHORIZED()); + spy.assert_only_event_role_revoked(contract_address, ROLE, AUTHORIZED(), AUTHORIZED()); let has_not_role = !state.has_role(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -312,7 +311,7 @@ fn test_renounceRole_for_granted_role() { start_cheat_caller_address(contract_address, AUTHORIZED()); state.renounceRole(ROLE, AUTHORIZED()); - spy.assert_event_role_revoked(contract_address, ROLE, AUTHORIZED(), AUTHORIZED()); + spy.assert_only_event_role_revoked(contract_address, ROLE, AUTHORIZED(), AUTHORIZED()); let has_not_role = !state.hasRole(ROLE, AUTHORIZED()); assert!(has_not_role); @@ -384,7 +383,10 @@ fn test_set_role_admin() { assert_eq!(state.get_role_admin(ROLE), DEFAULT_ADMIN_ROLE); state.set_role_admin(ROLE, OTHER_ROLE); - spy.assert_event_role_admin_changed(contract_address, ROLE, DEFAULT_ADMIN_ROLE, OTHER_ROLE); + spy + .assert_only_event_role_admin_changed( + contract_address, ROLE, DEFAULT_ADMIN_ROLE, OTHER_ROLE + ); let current_admin_role = state.get_role_admin(ROLE); assert_eq!(current_admin_role, OTHER_ROLE); @@ -466,9 +468,9 @@ fn test_default_admin_role_is_its_own_admin() { #[generate_trait] impl AccessControlSpyHelpersImpl of AccessControlSpyHelpers { - fn assert_event_role_revoked( + fn assert_only_event_role_revoked( ref self: EventSpy, - from_address: ContractAddress, + contract: ContractAddress, role: felt252, account: ContractAddress, sender: ContractAddress @@ -476,12 +478,12 @@ impl AccessControlSpyHelpersImpl of AccessControlSpyHelpers { let expected = AccessControlComponent::Event::RoleRevoked( RoleRevoked { role, account, sender } ); - self.assert_only_event(from_address, expected); + self.assert_only_event(contract, expected); } - fn assert_event_role_granted( + fn assert_only_event_role_granted( ref self: EventSpy, - from_address: ContractAddress, + contract: ContractAddress, role: felt252, account: ContractAddress, sender: ContractAddress @@ -489,10 +491,10 @@ impl AccessControlSpyHelpersImpl of AccessControlSpyHelpers { let expected = AccessControlComponent::Event::RoleGranted( RoleGranted { role, account, sender } ); - self.assert_only_event(from_address, expected); + self.assert_only_event(contract, expected); } - fn assert_event_role_admin_changed( + fn assert_only_event_role_admin_changed( ref self: EventSpy, from_address: ContractAddress, role: felt252, diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo index 7f35337a7..a2eee436e 100644 --- a/src/tests/access/test_dual_accesscontrol.cairo +++ b/src/tests/access/test_dual_accesscontrol.cairo @@ -1,15 +1,16 @@ use openzeppelin::access::accesscontrol::DEFAULT_ADMIN_ROLE; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControl; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControlTrait; -use openzeppelin::access::accesscontrol::interface::{ - IACCESSCONTROL_ID, IAccessControlDispatcher, IAccessControlDispatcherTrait, - IAccessControlCamelDispatcher, IAccessControlCamelDispatcherTrait -}; +use openzeppelin::access::accesscontrol::interface::IACCESSCONTROL_ID; +use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcher; +use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcherTrait; +use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcher; +use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcherTrait; use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; use openzeppelin::tests::utils::constants::{ADMIN, AUTHORIZED, ROLE}; use openzeppelin::tests::utils; use openzeppelin::utils::serde::SerializedAppend; -use snforge_std::{start_cheat_caller_address, test_address}; +use snforge_std::start_cheat_caller_address; use starknet::ContractAddress; // @@ -62,7 +63,7 @@ fn test_dual_supports_interface() { } #[test] -#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved +#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_supports_interface() { let dispatcher = setup_non_accesscontrol(); @@ -84,7 +85,7 @@ fn test_dual_has_role() { } #[test] -#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved +#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_has_role() { let dispatcher = setup_non_accesscontrol(); @@ -106,7 +107,7 @@ fn test_dual_get_role_admin() { } #[test] -#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved +#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_get_role_admin() { let dispatcher = setup_non_accesscontrol(); @@ -131,7 +132,7 @@ fn test_dual_grant_role() { } #[test] -#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved +#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_grant_role() { let dispatcher = setup_non_accesscontrol(); @@ -156,7 +157,7 @@ fn test_dual_revoke_role() { } #[test] -#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved +#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_revoke_role() { let dispatcher = setup_non_accesscontrol(); @@ -181,7 +182,7 @@ fn test_dual_renounce_role() { } #[test] -#[ignore] // TODO: Enable when ENTRYPOINT_NOT_FOUND issue is solved +#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_renounce_role() { let dispatcher = setup_non_accesscontrol(); @@ -200,7 +201,7 @@ fn test_dual_renounce_role_exists_and_panics() { // #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail fn test_dual_hasRole() { let (dispatcher, _) = setup_camel(); @@ -209,7 +210,7 @@ fn test_dual_hasRole() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail #[should_panic(expected: ("Some error",))] fn test_dual_hasRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -217,7 +218,7 @@ fn test_dual_hasRole_exists_and_panics() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail fn test_dual_getRoleAdmin() { let (dispatcher, _) = setup_camel(); @@ -226,7 +227,7 @@ fn test_dual_getRoleAdmin() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail #[should_panic(expected: ("Some error",))] fn test_dual_getRoleAdmin_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -234,7 +235,7 @@ fn test_dual_getRoleAdmin_exists_and_panics() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail fn test_dual_grantRole() { let (dispatcher, target) = setup_camel(); start_cheat_caller_address(target.contract_address, ADMIN()); @@ -245,7 +246,7 @@ fn test_dual_grantRole() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail #[should_panic(expected: ("Some error",))] fn test_dual_grantRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -253,7 +254,7 @@ fn test_dual_grantRole_exists_and_panics() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail fn test_dual_revokeRole() { let (dispatcher, target) = setup_camel(); start_cheat_caller_address(target.contract_address, ADMIN()); @@ -265,7 +266,7 @@ fn test_dual_revokeRole() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail #[should_panic(expected: ("Some error",))] fn test_dual_revokeRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -273,7 +274,7 @@ fn test_dual_revokeRole_exists_and_panics() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail fn test_dual_renounceRole() { let (dispatcher, target) = setup_camel(); start_cheat_caller_address(target.contract_address, ADMIN()); @@ -284,7 +285,7 @@ fn test_dual_renounceRole() { } #[test] -#[ignore] // TODO: Enable when try/catch is supported +#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail #[should_panic(expected: ("Some error",))] fn test_dual_renounceRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); From 2953c200a1e6deadf13f357d155c9ebb30bdadcb Mon Sep 17 00:00:00 2001 From: immrsd Date: Tue, 23 Jul 2024 15:09:23 +0200 Subject: [PATCH 5/8] Update error messages --- src/tests/access.cairo | 3 +- .../access/test_dual_accesscontrol.cairo | 32 +++++++++---------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/tests/access.cairo b/src/tests/access.cairo index 64603ee89..0eab73305 100644 --- a/src/tests/access.cairo +++ b/src/tests/access.cairo @@ -1,5 +1,4 @@ -// pub(crate) mod common; - +pub(crate) mod common; mod test_accesscontrol; mod test_dual_accesscontrol; mod test_dual_ownable; diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo index a2eee436e..bfb24d441 100644 --- a/src/tests/access/test_dual_accesscontrol.cairo +++ b/src/tests/access/test_dual_accesscontrol.cairo @@ -63,7 +63,7 @@ fn test_dual_supports_interface() { } #[test] -#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_supports_interface() { let dispatcher = setup_non_accesscontrol(); @@ -85,7 +85,7 @@ fn test_dual_has_role() { } #[test] -#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_has_role() { let dispatcher = setup_non_accesscontrol(); @@ -107,7 +107,7 @@ fn test_dual_get_role_admin() { } #[test] -#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_get_role_admin() { let dispatcher = setup_non_accesscontrol(); @@ -132,7 +132,7 @@ fn test_dual_grant_role() { } #[test] -#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_grant_role() { let dispatcher = setup_non_accesscontrol(); @@ -157,7 +157,7 @@ fn test_dual_revoke_role() { } #[test] -#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_revoke_role() { let dispatcher = setup_non_accesscontrol(); @@ -182,7 +182,7 @@ fn test_dual_renounce_role() { } #[test] -#[ignore] // REASON: inconsistent ENTRYPOINT_NOT_FOUND panic message +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_renounce_role() { let dispatcher = setup_non_accesscontrol(); @@ -201,7 +201,7 @@ fn test_dual_renounce_role_exists_and_panics() { // #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_hasRole() { let (dispatcher, _) = setup_camel(); @@ -210,7 +210,7 @@ fn test_dual_hasRole() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_hasRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -218,7 +218,7 @@ fn test_dual_hasRole_exists_and_panics() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_getRoleAdmin() { let (dispatcher, _) = setup_camel(); @@ -227,7 +227,7 @@ fn test_dual_getRoleAdmin() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_getRoleAdmin_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -235,7 +235,7 @@ fn test_dual_getRoleAdmin_exists_and_panics() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_grantRole() { let (dispatcher, target) = setup_camel(); start_cheat_caller_address(target.contract_address, ADMIN()); @@ -246,7 +246,7 @@ fn test_dual_grantRole() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_grantRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -254,7 +254,7 @@ fn test_dual_grantRole_exists_and_panics() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_revokeRole() { let (dispatcher, target) = setup_camel(); start_cheat_caller_address(target.contract_address, ADMIN()); @@ -266,7 +266,7 @@ fn test_dual_revokeRole() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_revokeRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); @@ -274,7 +274,7 @@ fn test_dual_revokeRole_exists_and_panics() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_renounceRole() { let (dispatcher, target) = setup_camel(); start_cheat_caller_address(target.contract_address, ADMIN()); @@ -285,7 +285,7 @@ fn test_dual_renounceRole() { } #[test] -#[ignore] // REASON: lack of error handling causes try_selector_with_fallback to fail +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_renounceRole_exists_and_panics() { let (_, camel_dispatcher) = setup_accesscontrol_panic(); From 058193f467d53f571acb8ce98d1b8bc4511aa40f Mon Sep 17 00:00:00 2001 From: immrsd Date: Tue, 23 Jul 2024 15:11:11 +0200 Subject: [PATCH 6/8] Bring back separator line --- src/tests/access.cairo | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/access.cairo b/src/tests/access.cairo index 0eab73305..cfa981899 100644 --- a/src/tests/access.cairo +++ b/src/tests/access.cairo @@ -1,4 +1,5 @@ pub(crate) mod common; + mod test_accesscontrol; mod test_dual_accesscontrol; mod test_dual_ownable; From 56a83ed569cd9418cc691f8bf35853a5b2fe8013 Mon Sep 17 00:00:00 2001 From: immrsd Date: Tue, 23 Jul 2024 15:26:54 +0200 Subject: [PATCH 7/8] Fix ignore reasons in access module --- src/tests/access/test_dual_accesscontrol.cairo | 12 ++++++------ src/tests/access/test_dual_ownable.cairo | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo index bfb24d441..b82d28962 100644 --- a/src/tests/access/test_dual_accesscontrol.cairo +++ b/src/tests/access/test_dual_accesscontrol.cairo @@ -63,7 +63,7 @@ fn test_dual_supports_interface() { } #[test] -#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. +#[ignore] // REASON: should_panic attribute not fit for complex panic messages. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_supports_interface() { let dispatcher = setup_non_accesscontrol(); @@ -85,7 +85,7 @@ fn test_dual_has_role() { } #[test] -#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. +#[ignore] // REASON: should_panic attribute not fit for complex panic messages. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_has_role() { let dispatcher = setup_non_accesscontrol(); @@ -107,7 +107,7 @@ fn test_dual_get_role_admin() { } #[test] -#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. +#[ignore] // REASON: should_panic attribute not fit for complex panic messages. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_get_role_admin() { let dispatcher = setup_non_accesscontrol(); @@ -132,7 +132,7 @@ fn test_dual_grant_role() { } #[test] -#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. +#[ignore] // REASON: should_panic attribute not fit for complex panic messages. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_grant_role() { let dispatcher = setup_non_accesscontrol(); @@ -157,7 +157,7 @@ fn test_dual_revoke_role() { } #[test] -#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. +#[ignore] // REASON: should_panic attribute not fit for complex panic messages. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_revoke_role() { let dispatcher = setup_non_accesscontrol(); @@ -182,7 +182,7 @@ fn test_dual_renounce_role() { } #[test] -#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. +#[ignore] // REASON: should_panic attribute not fit for complex panic messages. #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_renounce_role() { let dispatcher = setup_non_accesscontrol(); diff --git a/src/tests/access/test_dual_ownable.cairo b/src/tests/access/test_dual_ownable.cairo index 16c22514f..e1cef0031 100644 --- a/src/tests/access/test_dual_ownable.cairo +++ b/src/tests/access/test_dual_ownable.cairo @@ -138,7 +138,7 @@ fn test_dual_renounce_ownership_exists_and_panics() { // #[test] -#[ignore] +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_transferOwnership() { let (dispatcher, _) = setup_camel(); start_cheat_caller_address(dispatcher.contract_address, OWNER()); @@ -149,7 +149,7 @@ fn test_dual_transferOwnership() { } #[test] -#[ignore] +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_transferOwnership_exists_and_panics() { let (_, camel_dispatcher) = setup_ownable_panic(); @@ -157,7 +157,7 @@ fn test_dual_transferOwnership_exists_and_panics() { } #[test] -#[ignore] +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. fn test_dual_renounceOwnership() { let (dispatcher, _) = setup_camel(); start_cheat_caller_address(dispatcher.contract_address, OWNER()); @@ -168,7 +168,7 @@ fn test_dual_renounceOwnership() { } #[test] -#[ignore] +#[ignore] // REASON: foundry entrypoint_not_found error message inconsistent with mainnet. #[should_panic(expected: ("Some error",))] fn test_dual_renounceOwnership_exists_and_panics() { let (_, camel_dispatcher) = setup_ownable_panic(); From 2c4ad1d4adeb9fb31753567ede06723a590da62c Mon Sep 17 00:00:00 2001 From: immrsd Date: Thu, 25 Jul 2024 09:40:59 +0200 Subject: [PATCH 8/8] Fix review issues --- src/tests/access/test_accesscontrol.cairo | 15 +++++++-------- src/tests/access/test_dual_accesscontrol.cairo | 13 ++++++------- src/tests/access/test_dual_ownable.cairo | 4 ---- 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/tests/access/test_accesscontrol.cairo b/src/tests/access/test_accesscontrol.cairo index 36b006c74..809669e69 100644 --- a/src/tests/access/test_accesscontrol.cairo +++ b/src/tests/access/test_accesscontrol.cairo @@ -1,11 +1,10 @@ -use openzeppelin::access::accesscontrol::AccessControlComponent::InternalImpl; -use openzeppelin::access::accesscontrol::AccessControlComponent::RoleAdminChanged; -use openzeppelin::access::accesscontrol::AccessControlComponent::RoleGranted; -use openzeppelin::access::accesscontrol::AccessControlComponent::RoleRevoked; -use openzeppelin::access::accesscontrol::AccessControlComponent; -use openzeppelin::access::accesscontrol::DEFAULT_ADMIN_ROLE; -use openzeppelin::access::accesscontrol::interface::IACCESSCONTROL_ID; -use openzeppelin::access::accesscontrol::interface::{IAccessControl, IAccessControlCamel}; +use openzeppelin::access::accesscontrol::AccessControlComponent::{ + InternalImpl, RoleAdminChanged, RoleGranted, RoleRevoked +}; +use openzeppelin::access::accesscontrol::interface::{ + IAccessControl, IAccessControlCamel, IACCESSCONTROL_ID +}; +use openzeppelin::access::accesscontrol::{AccessControlComponent, DEFAULT_ADMIN_ROLE}; use openzeppelin::introspection::interface::ISRC5; use openzeppelin::tests::mocks::accesscontrol_mocks::DualCaseAccessControlMock; use openzeppelin::tests::utils::constants::{ diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo index b82d28962..bd8f17781 100644 --- a/src/tests/access/test_dual_accesscontrol.cairo +++ b/src/tests/access/test_dual_accesscontrol.cairo @@ -1,17 +1,16 @@ use openzeppelin::access::accesscontrol::DEFAULT_ADMIN_ROLE; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControl; use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControlTrait; -use openzeppelin::access::accesscontrol::interface::IACCESSCONTROL_ID; -use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcher; -use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcherTrait; -use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcher; -use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcherTrait; -use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; +use openzeppelin::access::accesscontrol::interface::{ + IACCESSCONTROL_ID, IAccessControlCamelDispatcher, IAccessControlCamelDispatcherTrait +}; +use openzeppelin::access::accesscontrol::interface::{ + IAccessControlDispatcher, IAccessControlDispatcherTrait +}; use openzeppelin::tests::utils::constants::{ADMIN, AUTHORIZED, ROLE}; use openzeppelin::tests::utils; use openzeppelin::utils::serde::SerializedAppend; use snforge_std::start_cheat_caller_address; -use starknet::ContractAddress; // // Setup diff --git a/src/tests/access/test_dual_ownable.cairo b/src/tests/access/test_dual_ownable.cairo index e1cef0031..08a863498 100644 --- a/src/tests/access/test_dual_ownable.cairo +++ b/src/tests/access/test_dual_ownable.cairo @@ -3,10 +3,6 @@ use openzeppelin::access::ownable::dual_ownable::{DualCaseOwnable, DualCaseOwnab use openzeppelin::access::ownable::interface::{ IOwnableDispatcher, IOwnableCamelOnlyDispatcher, IOwnableDispatcherTrait }; -use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; -use openzeppelin::tests::mocks::ownable_mocks::{ - CamelOwnableMock, CamelOwnablePanicMock, SnakeOwnableMock, SnakeOwnablePanicMock -}; use openzeppelin::tests::utils::constants::{OWNER, NEW_OWNER}; use openzeppelin::tests::utils; use openzeppelin::utils::serde::SerializedAppend;