From af2a9a61dd6e921415507aec04fc085b751111a5 Mon Sep 17 00:00:00 2001 From: andrew Date: Wed, 3 Jul 2024 14:12:20 -0500 Subject: [PATCH 1/7] migrate pausable and initializable tests --- src/tests.cairo | 4 +- src/tests/security.cairo | 3 +- src/tests/security/common.cairo | 40 +++++++++++++++++++ src/tests/security/test_pausable.cairo | 37 ++++++----------- src/tests/security/test_reentrancyguard.cairo | 15 +++++-- 5 files changed, 68 insertions(+), 31 deletions(-) create mode 100644 src/tests/security/common.cairo diff --git a/src/tests.cairo b/src/tests.cairo index 6b45d6e2c..eceac55c6 100644 --- a/src/tests.cairo +++ b/src/tests.cairo @@ -9,8 +9,8 @@ mod mocks; // #[cfg(test)] // mod presets; -// #[cfg(test)] -// mod security; +#[cfg(test)] +mod security; #[cfg(test)] mod token; // #[cfg(test)] diff --git a/src/tests/security.cairo b/src/tests/security.cairo index 35c30a0e8..87a1e05f2 100644 --- a/src/tests/security.cairo +++ b/src/tests/security.cairo @@ -1,3 +1,4 @@ +mod common; mod test_initializable; mod test_pausable; -mod test_reentrancyguard; +//mod test_reentrancyguard; diff --git a/src/tests/security/common.cairo b/src/tests/security/common.cairo new file mode 100644 index 000000000..ab7ca5048 --- /dev/null +++ b/src/tests/security/common.cairo @@ -0,0 +1,40 @@ +use openzeppelin::security::PausableComponent::{Paused, Unpaused}; +use openzeppelin::security::PausableComponent; +use snforge_std::{EventSpy, EventAssertions}; +use starknet::ContractAddress; + +pub(crate) fn assert_event_paused( + ref spy: EventSpy, + contract: ContractAddress, + account: ContractAddress, +) { + let expected = PausableComponent::Event::Paused(Paused { account }); + spy.assert_emitted(@array![(contract, expected)]); +} + +pub(crate) fn assert_only_event_paused( + ref spy: EventSpy, + contract: ContractAddress, + account: ContractAddress, +) { + assert_event_paused(ref spy, contract, account); + assert(spy.events.len() == 0, 'Events remaining on queue'); +} + +pub(crate) fn assert_event_unpaused( + ref spy: EventSpy, + contract: ContractAddress, + account: ContractAddress, +) { + let expected = PausableComponent::Event::Unpaused(Unpaused { account }); + spy.assert_emitted(@array![(contract, expected)]); +} + +pub(crate) fn assert_only_event_unpaused( + ref spy: EventSpy, + contract: ContractAddress, + account: ContractAddress, +) { + assert_event_unpaused(ref spy, contract, account); + assert(spy.events.len() == 0, 'Events remaining on queue'); +} diff --git a/src/tests/security/test_pausable.cairo b/src/tests/security/test_pausable.cairo index 8fd170df0..7fd153447 100644 --- a/src/tests/security/test_pausable.cairo +++ b/src/tests/security/test_pausable.cairo @@ -4,9 +4,13 @@ use openzeppelin::security::PausableComponent; use openzeppelin::tests::mocks::pausable_mocks::PausableMock; use openzeppelin::tests::utils::constants::{CALLER, ZERO}; use openzeppelin::tests::utils; +use snforge_std::{EventSpy, EventAssertions}; +use snforge_std::{test_address, start_cheat_caller_address}; use starknet::ContractAddress; use starknet::contract_address_const; use starknet::testing; +use super::common::{assert_event_paused, assert_only_event_paused}; +use super::common::{assert_event_unpaused, assert_only_event_unpaused}; type ComponentState = PausableComponent::ComponentState; @@ -73,11 +77,13 @@ fn test_assert_not_paused_when_not_paused() { #[test] fn test_pause_when_unpaused() { let mut state = COMPONENT_STATE(); - testing::set_caller_address(CALLER()); + let contract_address = test_address(); + let mut spy = utils::spy_on(contract_address); + start_cheat_caller_address(contract_address, CALLER()); state.pause(); - assert_event_paused(CALLER()); + assert_only_event_paused(ref spy, contract_address, CALLER()); assert!(state.is_paused()); } @@ -96,14 +102,15 @@ fn test_pause_when_paused() { #[test] fn test_unpause_when_paused() { let mut state = COMPONENT_STATE(); - testing::set_caller_address(CALLER()); + let contract_address = test_address(); + let mut spy = utils::spy_on(contract_address); + start_cheat_caller_address(test_address(), CALLER()); state.pause(); - utils::drop_event(ZERO()); - state.unpause(); - assert_event_unpaused(CALLER()); + assert_event_paused(ref spy, contract_address, CALLER()); + assert_only_event_unpaused(ref spy, contract_address, CALLER()); assert!(!state.is_paused()); } @@ -114,21 +121,3 @@ fn test_unpause_when_unpaused() { assert!(!state.is_paused()); state.unpause(); } - -// -// Helpers -// - -fn assert_event_paused(account: ContractAddress) { - let event = utils::pop_log::(ZERO()).unwrap(); - let expected = PausableComponent::Event::Paused(Paused { account }); - assert!(event == expected); - utils::assert_no_events_left(ZERO()); -} - -fn assert_event_unpaused(account: ContractAddress) { - let event = utils::pop_log::(ZERO()).unwrap(); - let expected = PausableComponent::Event::Unpaused(Unpaused { account }); - assert!(event == expected); - utils::assert_no_events_left(ZERO()); -} diff --git a/src/tests/security/test_reentrancyguard.cairo b/src/tests/security/test_reentrancyguard.cairo index 327f72fa4..348de115e 100644 --- a/src/tests/security/test_reentrancyguard.cairo +++ b/src/tests/security/test_reentrancyguard.cairo @@ -5,6 +5,7 @@ use openzeppelin::tests::mocks::reentrancy_mocks::{ }; use openzeppelin::tests::utils; use starknet::storage::StorageMemberAccessTrait; +use snforge_std::{test_address, start_cheat_caller_address}; type ComponentState = ReentrancyGuardComponent::ComponentState; @@ -12,10 +13,15 @@ fn COMPONENT_STATE() -> ComponentState { ReentrancyGuardComponent::component_state_for_testing() } -fn deploy_mock() -> IReentrancyMockDispatcher { +//fn deploy_mock() -> IReentrancyMockDispatcher { +// let calldata = array![]; +// let address = utils::deploy(ReentrancyMock::TEST_CLASS_HASH, calldata); +// IReentrancyMockDispatcher { contract_address: address } +//} + +fn deploy_mock() -> ContractAddress { let calldata = array![]; - let address = utils::deploy(ReentrancyMock::TEST_CLASS_HASH, calldata); - IReentrancyMockDispatcher { contract_address: address } + utils::declare_and_deploy("ReentrancyMock", calldata) } // @@ -77,7 +83,8 @@ fn test_remote_callback() { // Deploy attacker let calldata = ArrayTrait::new(); - let attacker_addr = utils::deploy(Attacker::TEST_CLASS_HASH, calldata); + //let attacker_addr = utils::deploy(Attacker::TEST_CLASS_HASH, calldata); + let attacker_addr = utils::declare_and_deploy("ReentrancyMock", calldata) contract.count_and_call(attacker_addr); } From bbb46d3f7e067c16fdc020178493c144d45fd077 Mon Sep 17 00:00:00 2001 From: andrew Date: Wed, 3 Jul 2024 14:38:53 -0500 Subject: [PATCH 2/7] migrate reentrancy guard tests --- src/tests/security.cairo | 2 +- src/tests/security/test_reentrancyguard.cairo | 34 +++++-------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/src/tests/security.cairo b/src/tests/security.cairo index 87a1e05f2..473ac1005 100644 --- a/src/tests/security.cairo +++ b/src/tests/security.cairo @@ -1,4 +1,4 @@ mod common; mod test_initializable; mod test_pausable; -//mod test_reentrancyguard; +mod test_reentrancyguard; diff --git a/src/tests/security/test_reentrancyguard.cairo b/src/tests/security/test_reentrancyguard.cairo index 348de115e..4a12bc918 100644 --- a/src/tests/security/test_reentrancyguard.cairo +++ b/src/tests/security/test_reentrancyguard.cairo @@ -1,11 +1,10 @@ use openzeppelin::security::ReentrancyGuardComponent::InternalImpl; use openzeppelin::security::ReentrancyGuardComponent; use openzeppelin::tests::mocks::reentrancy_mocks::{ - Attacker, ReentrancyMock, IReentrancyMockDispatcher, IReentrancyMockDispatcherTrait + ReentrancyMock, IReentrancyMockDispatcher, IReentrancyMockDispatcherTrait }; use openzeppelin::tests::utils; use starknet::storage::StorageMemberAccessTrait; -use snforge_std::{test_address, start_cheat_caller_address}; type ComponentState = ReentrancyGuardComponent::ComponentState; @@ -13,15 +12,10 @@ fn COMPONENT_STATE() -> ComponentState { ReentrancyGuardComponent::component_state_for_testing() } -//fn deploy_mock() -> IReentrancyMockDispatcher { -// let calldata = array![]; -// let address = utils::deploy(ReentrancyMock::TEST_CLASS_HASH, calldata); -// IReentrancyMockDispatcher { contract_address: address } -//} - -fn deploy_mock() -> ContractAddress { +fn deploy_mock() -> IReentrancyMockDispatcher { let calldata = array![]; - utils::declare_and_deploy("ReentrancyMock", calldata) + let address = utils::declare_and_deploy("ReentrancyMock", calldata); + IReentrancyMockDispatcher { contract_address: address } } // @@ -70,36 +64,26 @@ fn test_reentrancy_guard_end() { // #[test] -#[should_panic( - expected: ( - 'ReentrancyGuard: reentrant call', - 'ENTRYPOINT_FAILED', - 'ENTRYPOINT_FAILED', - 'ENTRYPOINT_FAILED' - ), -)] +#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] fn test_remote_callback() { let contract = deploy_mock(); // Deploy attacker - let calldata = ArrayTrait::new(); - //let attacker_addr = utils::deploy(Attacker::TEST_CLASS_HASH, calldata); - let attacker_addr = utils::declare_and_deploy("ReentrancyMock", calldata) + let calldata = array![]; + let attacker_addr = utils::declare_and_deploy("Attacker", calldata); contract.count_and_call(attacker_addr); } #[test] -#[should_panic(expected: ('ReentrancyGuard: reentrant call', 'ENTRYPOINT_FAILED'))] +#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] fn test_local_recursion() { let contract = deploy_mock(); contract.count_local_recursive(10); } #[test] -#[should_panic( - expected: ('ReentrancyGuard: reentrant call', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED') -)] +#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] fn test_external_recursion() { let contract = deploy_mock(); contract.count_external_recursive(10); From c60f6c0d385fe9bf00545489f406786a829548b5 Mon Sep 17 00:00:00 2001 From: andrew Date: Wed, 3 Jul 2024 14:46:28 -0500 Subject: [PATCH 3/7] clean up tests --- src/tests/security.cairo | 1 - src/tests/security/common.cairo | 40 -------------------------- src/tests/security/test_pausable.cairo | 39 +++++++++++++++++++++---- src/tests/token.cairo | 2 +- 4 files changed, 35 insertions(+), 47 deletions(-) delete mode 100644 src/tests/security/common.cairo diff --git a/src/tests/security.cairo b/src/tests/security.cairo index 473ac1005..35c30a0e8 100644 --- a/src/tests/security.cairo +++ b/src/tests/security.cairo @@ -1,4 +1,3 @@ -mod common; mod test_initializable; mod test_pausable; mod test_reentrancyguard; diff --git a/src/tests/security/common.cairo b/src/tests/security/common.cairo deleted file mode 100644 index ab7ca5048..000000000 --- a/src/tests/security/common.cairo +++ /dev/null @@ -1,40 +0,0 @@ -use openzeppelin::security::PausableComponent::{Paused, Unpaused}; -use openzeppelin::security::PausableComponent; -use snforge_std::{EventSpy, EventAssertions}; -use starknet::ContractAddress; - -pub(crate) fn assert_event_paused( - ref spy: EventSpy, - contract: ContractAddress, - account: ContractAddress, -) { - let expected = PausableComponent::Event::Paused(Paused { account }); - spy.assert_emitted(@array![(contract, expected)]); -} - -pub(crate) fn assert_only_event_paused( - ref spy: EventSpy, - contract: ContractAddress, - account: ContractAddress, -) { - assert_event_paused(ref spy, contract, account); - assert(spy.events.len() == 0, 'Events remaining on queue'); -} - -pub(crate) fn assert_event_unpaused( - ref spy: EventSpy, - contract: ContractAddress, - account: ContractAddress, -) { - let expected = PausableComponent::Event::Unpaused(Unpaused { account }); - spy.assert_emitted(@array![(contract, expected)]); -} - -pub(crate) fn assert_only_event_unpaused( - ref spy: EventSpy, - contract: ContractAddress, - account: ContractAddress, -) { - assert_event_unpaused(ref spy, contract, account); - assert(spy.events.len() == 0, 'Events remaining on queue'); -} diff --git a/src/tests/security/test_pausable.cairo b/src/tests/security/test_pausable.cairo index 7fd153447..b7e88315c 100644 --- a/src/tests/security/test_pausable.cairo +++ b/src/tests/security/test_pausable.cairo @@ -2,15 +2,11 @@ use openzeppelin::security::PausableComponent::{InternalImpl, PausableImpl}; use openzeppelin::security::PausableComponent::{Paused, Unpaused}; use openzeppelin::security::PausableComponent; use openzeppelin::tests::mocks::pausable_mocks::PausableMock; -use openzeppelin::tests::utils::constants::{CALLER, ZERO}; +use openzeppelin::tests::utils::constants::CALLER; use openzeppelin::tests::utils; use snforge_std::{EventSpy, EventAssertions}; use snforge_std::{test_address, start_cheat_caller_address}; use starknet::ContractAddress; -use starknet::contract_address_const; -use starknet::testing; -use super::common::{assert_event_paused, assert_only_event_paused}; -use super::common::{assert_event_unpaused, assert_only_event_unpaused}; type ComponentState = PausableComponent::ComponentState; @@ -121,3 +117,36 @@ fn test_unpause_when_unpaused() { assert!(!state.is_paused()); state.unpause(); } + +// +// Helpers +// + +fn assert_event_paused( + ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, +) { + let expected = PausableComponent::Event::Paused(Paused { account }); + spy.assert_emitted(@array![(contract, expected)]); +} + +fn assert_only_event_paused( + ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, +) { + assert_event_paused(ref spy, contract, account); + assert(spy.events.len() == 0, 'Events remaining on queue'); +} + +fn assert_event_unpaused( + ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, +) { + let expected = PausableComponent::Event::Unpaused(Unpaused { account }); + spy.assert_emitted(@array![(contract, expected)]); +} + +fn assert_only_event_unpaused( + ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, +) { + assert_event_unpaused(ref spy, contract, account); + assert(spy.events.len() == 0, 'Events remaining on queue'); +} + diff --git a/src/tests/token.cairo b/src/tests/token.cairo index 8833c90c3..25979f784 100644 --- a/src/tests/token.cairo +++ b/src/tests/token.cairo @@ -1,3 +1,3 @@ // pub(crate) mod erc1155; -pub(crate) mod erc20; // pub(crate) mod erc721; +pub(crate) mod erc20; From dea2bd38e9221abe7a2bd683b60f3a4ed3213d36 Mon Sep 17 00:00:00 2001 From: andrew Date: Wed, 3 Jul 2024 14:46:48 -0500 Subject: [PATCH 4/7] fix fmt --- src/tests/security/test_pausable.cairo | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/tests/security/test_pausable.cairo b/src/tests/security/test_pausable.cairo index b7e88315c..418fd192b 100644 --- a/src/tests/security/test_pausable.cairo +++ b/src/tests/security/test_pausable.cairo @@ -122,9 +122,7 @@ fn test_unpause_when_unpaused() { // Helpers // -fn assert_event_paused( - ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, -) { +fn assert_event_paused(ref spy: EventSpy, contract: ContractAddress, account: ContractAddress,) { let expected = PausableComponent::Event::Paused(Paused { account }); spy.assert_emitted(@array![(contract, expected)]); } @@ -136,9 +134,7 @@ fn assert_only_event_paused( assert(spy.events.len() == 0, 'Events remaining on queue'); } -fn assert_event_unpaused( - ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, -) { +fn assert_event_unpaused(ref spy: EventSpy, contract: ContractAddress, account: ContractAddress,) { let expected = PausableComponent::Event::Unpaused(Unpaused { account }); spy.assert_emitted(@array![(contract, expected)]); } From d74da6da8ef058d6dd4fa912d40619355c3c7a84 Mon Sep 17 00:00:00 2001 From: andrew Date: Thu, 4 Jul 2024 09:48:43 -0500 Subject: [PATCH 5/7] add event trait to pausable, fix tests --- src/tests/security/test_pausable.cairo | 68 ++++++++++++++------------ 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/src/tests/security/test_pausable.cairo b/src/tests/security/test_pausable.cairo index 418fd192b..f1970e6b4 100644 --- a/src/tests/security/test_pausable.cairo +++ b/src/tests/security/test_pausable.cairo @@ -3,9 +3,9 @@ use openzeppelin::security::PausableComponent::{Paused, Unpaused}; use openzeppelin::security::PausableComponent; use openzeppelin::tests::mocks::pausable_mocks::PausableMock; use openzeppelin::tests::utils::constants::CALLER; -use openzeppelin::tests::utils; -use snforge_std::{EventSpy, EventAssertions}; -use snforge_std::{test_address, start_cheat_caller_address}; +use openzeppelin::tests::utils::events::EventSpyExt; +use snforge_std::EventSpy; +use snforge_std::{spy_events, test_address, start_cheat_caller_address}; use starknet::ContractAddress; type ComponentState = PausableComponent::ComponentState; @@ -75,11 +75,11 @@ fn test_pause_when_unpaused() { let mut state = COMPONENT_STATE(); let contract_address = test_address(); - let mut spy = utils::spy_on(contract_address); + let mut spy = spy_events(); start_cheat_caller_address(contract_address, CALLER()); state.pause(); - assert_only_event_paused(ref spy, contract_address, CALLER()); + spy.assert_only_event_paused(contract_address, CALLER()); assert!(state.is_paused()); } @@ -100,13 +100,13 @@ fn test_unpause_when_paused() { let mut state = COMPONENT_STATE(); let contract_address = test_address(); - let mut spy = utils::spy_on(contract_address); + let mut spy = spy_events(); start_cheat_caller_address(test_address(), CALLER()); state.pause(); state.unpause(); - assert_event_paused(ref spy, contract_address, CALLER()); - assert_only_event_unpaused(ref spy, contract_address, CALLER()); + spy.assert_event_paused(contract_address, CALLER()); + spy.assert_only_event_unpaused(contract_address, CALLER()); assert!(!state.is_paused()); } @@ -122,27 +122,33 @@ fn test_unpause_when_unpaused() { // Helpers // -fn assert_event_paused(ref spy: EventSpy, contract: ContractAddress, account: ContractAddress,) { - let expected = PausableComponent::Event::Paused(Paused { account }); - spy.assert_emitted(@array![(contract, expected)]); +#[generate_trait] +pub(crate) impl PausableSpyHelpersImpl of PausableSpyHelpers { + fn assert_event_paused( + ref self: EventSpy, contract: ContractAddress, account: ContractAddress + ) { + let expected = PausableComponent::Event::Paused(Paused { account }); + self.assert_emitted_single(contract, expected); + } + + fn assert_only_event_paused( + ref self: EventSpy, contract: ContractAddress, account: ContractAddress, + ) { + self.assert_event_paused(contract, account); + self.assert_no_events_left_from(contract); + } + + fn assert_event_unpaused( + ref self: EventSpy, contract: ContractAddress, account: ContractAddress + ) { + let expected = PausableComponent::Event::Unpaused(Unpaused { account }); + self.assert_emitted_single(contract, expected); + } + + fn assert_only_event_unpaused( + ref self: EventSpy, contract: ContractAddress, account: ContractAddress, + ) { + self.assert_event_unpaused(contract, account); + self.assert_no_events_left_from(contract); + } } - -fn assert_only_event_paused( - ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, -) { - assert_event_paused(ref spy, contract, account); - assert(spy.events.len() == 0, 'Events remaining on queue'); -} - -fn assert_event_unpaused(ref spy: EventSpy, contract: ContractAddress, account: ContractAddress,) { - let expected = PausableComponent::Event::Unpaused(Unpaused { account }); - spy.assert_emitted(@array![(contract, expected)]); -} - -fn assert_only_event_unpaused( - ref spy: EventSpy, contract: ContractAddress, account: ContractAddress, -) { - assert_event_unpaused(ref spy, contract, account); - assert(spy.events.len() == 0, 'Events remaining on queue'); -} - From d100a016a9d892109472b63de234bcd42dee4bf1 Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Fri, 5 Jul 2024 10:17:34 -0500 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Eric Nordelo --- src/tests/security/test_pausable.cairo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/security/test_pausable.cairo b/src/tests/security/test_pausable.cairo index f1970e6b4..8a2080004 100644 --- a/src/tests/security/test_pausable.cairo +++ b/src/tests/security/test_pausable.cairo @@ -123,7 +123,7 @@ fn test_unpause_when_unpaused() { // #[generate_trait] -pub(crate) impl PausableSpyHelpersImpl of PausableSpyHelpers { + impl PausableSpyHelpersImpl of PausableSpyHelpers { fn assert_event_paused( ref self: EventSpy, contract: ContractAddress, account: ContractAddress ) { From 441b3d6f9310e2f67dd4c54c2c658d94862ddfdb Mon Sep 17 00:00:00 2001 From: andrew Date: Fri, 5 Jul 2024 10:19:21 -0500 Subject: [PATCH 7/7] fix fmt --- src/tests/security/test_pausable.cairo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/security/test_pausable.cairo b/src/tests/security/test_pausable.cairo index 8a2080004..ea4ff3878 100644 --- a/src/tests/security/test_pausable.cairo +++ b/src/tests/security/test_pausable.cairo @@ -123,7 +123,7 @@ fn test_unpause_when_unpaused() { // #[generate_trait] - impl PausableSpyHelpersImpl of PausableSpyHelpers { +impl PausableSpyHelpersImpl of PausableSpyHelpers { fn assert_event_paused( ref self: EventSpy, contract: ContractAddress, account: ContractAddress ) {