Add testing for Ownable events (#675)

* feat: add tests for ownable events * feat: improve tests * Update src/tests/utils.cairo Co-authored-by: Martín Triay <[email protected]> --------- Co-authored-by: Martín Triay <[email protected]>
OpenZeppelin · Aug 11, 2023 · f9c30da · f9c30da
1 parent c28c758
commit f9c30da
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 58 deletions.
diff --git a/src/tests/access/test_accesscontrol.cairo b/src/tests/access/test_accesscontrol.cairo
@@ -4,33 +4,13 @@ use openzeppelin::access::accesscontrol::AccessControl::InternalImpl;
 use openzeppelin::access::accesscontrol::AccessControl;
 use openzeppelin::access::accesscontrol::DEFAULT_ADMIN_ROLE;
 use openzeppelin::access::accesscontrol::interface::IACCESSCONTROL_ID;
+use openzeppelin::tests::utils::constants::{
+    ADMIN, AUTHORIZED, OTHER, OTHER_ADMIN, ROLE, OTHER_ROLE, ZERO
+};
 use starknet::ContractAddress;
 use starknet::contract_address_const;
 use starknet::testing;
 
-const ROLE: felt252 = 41;
-const OTHER_ROLE: felt252 = 42;
-
-fn ZERO() -> ContractAddress {
-    contract_address_const::<0>()
-}
-
-fn ADMIN() -> ContractAddress {
-    contract_address_const::<1>()
-}
-
-fn AUTHORIZED() -> ContractAddress {
-    contract_address_const::<2>()
-}
-
-fn OTHER() -> ContractAddress {
-    contract_address_const::<3>()
-}
-
-fn OTHER_ADMIN() -> ContractAddress {
-    contract_address_const::<4>()
-}
-
 //
 // Setup
 //

diff --git a/src/tests/access/test_dual_accesscontrol.cairo b/src/tests/access/test_dual_accesscontrol.cairo
@@ -1,37 +1,24 @@
 use array::ArrayTrait;
 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::IAccessControlCamelDispatcher;
 use openzeppelin::access::accesscontrol::interface::IAccessControlDispatcherTrait;
-use openzeppelin::tests::mocks::accesscontrol_panic_mock::CamelAccessControlPanicMock;
-use openzeppelin::tests::mocks::accesscontrol_panic_mock::SnakeAccessControlPanicMock;
+use openzeppelin::access::accesscontrol::interface::IAccessControlCamelDispatcherTrait;
+use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControlTrait;
+use openzeppelin::access::accesscontrol::dual_accesscontrol::DualCaseAccessControl;
+use openzeppelin::tests::mocks::snake_accesscontrol_mock::SnakeAccessControlMock;
 use openzeppelin::tests::mocks::camel_accesscontrol_mock::CamelAccessControlMock;
+use openzeppelin::tests::mocks::accesscontrol_panic_mock::SnakeAccessControlPanicMock;
+use openzeppelin::tests::mocks::accesscontrol_panic_mock::CamelAccessControlPanicMock;
 use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock;
-use openzeppelin::tests::mocks::snake_accesscontrol_mock::SnakeAccessControlMock;
+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;
 
-//
-// Constants
-//
-
-const ROLE: felt252 = 41;
-
-fn ADMIN() -> ContractAddress {
-    contract_address_const::<10>()
-}
-
-fn AUTHORIZED() -> ContractAddress {
-    contract_address_const::<20>()
-}
-
 //
 // Setup
 //

diff --git a/src/tests/access/test_ownable.cairo b/src/tests/access/test_ownable.cairo
@@ -1,25 +1,17 @@
+use openzeppelin::access::ownable::Ownable;
 use openzeppelin::access::ownable::Ownable::InternalImpl;
 use openzeppelin::access::ownable::Ownable::OwnableCamelOnlyImpl;
 use openzeppelin::access::ownable::Ownable::OwnableImpl;
+use openzeppelin::access::ownable::Ownable::OwnershipTransferred;
 use openzeppelin::access::ownable::Ownable::_owner::InternalContractStateTrait;
-use openzeppelin::access::ownable::Ownable;
+use openzeppelin::tests::utils;
+use openzeppelin::tests::utils::constants::{ZERO, OTHER, OWNER};
+use option::OptionTrait;
 use starknet::ContractAddress;
 use starknet::contract_address_const;
 use starknet::testing;
 use zeroable::Zeroable;
 
-fn ZERO() -> ContractAddress {
-    contract_address_const::<0>()
-}
-
-fn OWNER() -> ContractAddress {
-    contract_address_const::<10>()
-}
-
-fn OTHER() -> ContractAddress {
-    contract_address_const::<20>()
-}
-
 //
 // Setup
 //
@@ -31,6 +23,7 @@ fn STATE() -> Ownable::ContractState {
 fn setup() -> Ownable::ContractState {
     let mut state = STATE();
     InternalImpl::initializer(ref state, OWNER());
+    testing::pop_log_raw(ZERO());
     state
 }
 
@@ -44,6 +37,9 @@ fn test_initializer() {
     let mut state = STATE();
     assert(state._owner.read().is_zero(), 'Should be zero');
     InternalImpl::initializer(ref state, OWNER());
+
+    assert_event_ownership_transferred(ZERO(), OWNER());
+
     assert(state._owner.read() == OWNER(), 'Owner should be set');
 }
 
@@ -85,6 +81,9 @@ fn test_assert_only_owner_when_caller_zero() {
 fn test__transfer_ownership() {
     let mut state = setup();
     InternalImpl::_transfer_ownership(ref state, OTHER());
+
+    assert_event_ownership_transferred(OWNER(), OTHER());
+
     assert(state._owner.read() == OTHER(), 'Owner should be OTHER');
 }
 
@@ -98,6 +97,9 @@ fn test_transfer_ownership() {
     let mut state = setup();
     testing::set_caller_address(OWNER());
     OwnableImpl::transfer_ownership(ref state, OTHER());
+
+    assert_event_ownership_transferred(OWNER(), OTHER());
+
     assert(OwnableImpl::owner(@state) == OTHER(), 'Should transfer ownership');
 }
 
@@ -133,6 +135,9 @@ fn test_transferOwnership() {
     let mut state = setup();
     testing::set_caller_address(OWNER());
     OwnableCamelOnlyImpl::transferOwnership(ref state, OTHER());
+
+    assert_event_ownership_transferred(OWNER(), OTHER());
+
     assert(OwnableImpl::owner(@state) == OTHER(), 'Should transfer ownership');
 }
 
@@ -172,6 +177,9 @@ fn test_renounce_ownership() {
     let mut state = setup();
     testing::set_caller_address(OWNER());
     OwnableImpl::renounce_ownership(ref state);
+
+    assert_event_ownership_transferred(OWNER(), ZERO());
+
     assert(OwnableImpl::owner(@state) == ZERO(), 'Should renounce ownership');
 }
 
@@ -198,6 +206,9 @@ fn test_renounceOwnership() {
     let mut state = setup();
     testing::set_caller_address(OWNER());
     OwnableCamelOnlyImpl::renounceOwnership(ref state);
+
+    assert_event_ownership_transferred(OWNER(), ZERO());
+
     assert(OwnableImpl::owner(@state) == ZERO(), 'Should renounce ownership');
 }
 
@@ -217,3 +228,14 @@ fn test_renounceOwnership_from_nonowner() {
     testing::set_caller_address(OTHER());
     OwnableCamelOnlyImpl::renounceOwnership(ref state);
 }
+
+//
+// Helpers
+//
+
+fn assert_event_ownership_transferred(previous_owner: ContractAddress, new_owner: ContractAddress) {
+    let event = utils::pop_log::<OwnershipTransferred>(ZERO()).unwrap();
+    assert(event.previous_owner == previous_owner, 'Invalid previous_owner');
+    assert(event.new_owner == new_owner, 'Invalid new_owner');
+    utils::assert_no_events_left(ZERO());
+}
diff --git a/src/tests/utils.cairo b/src/tests/utils.cairo
@@ -1,8 +1,12 @@
+mod constants;
+
 use array::ArrayTrait;
+use array::SpanTrait;
 use core::result::ResultTrait;
 use option::OptionTrait;
-use starknet::ContractAddress;
 use starknet::class_hash::Felt252TryIntoClassHash;
+use starknet::ContractAddress;
+use starknet::testing;
 use traits::TryInto;
 
 fn deploy(contract_class_hash: felt252, calldata: Array<felt252>) -> ContractAddress {
@@ -12,3 +16,19 @@ fn deploy(contract_class_hash: felt252, calldata: Array<felt252>) -> ContractAdd
         .unwrap();
     address
 }
+
+/// Pop the earliest unpopped logged event for the contract as the requested type
+/// and checks there's no more data left on the event, preventing missing extra params.
+/// Indexed event members are currently not supported, so they are ignored.
+fn pop_log<T, impl TDrop: Drop<T>, impl TEvent: starknet::Event<T>>(
+    address: ContractAddress
+) -> Option<T> {
+    let (mut keys, mut data) = testing::pop_log_raw(address)?;
+    let ret = starknet::Event::deserialize(ref keys, ref data);
+    assert(data.is_empty(), 'Event has extra data');
+    ret
+}
+
+fn assert_no_events_left(address: ContractAddress) {
+    assert(testing::pop_log_raw(address).is_none(), 'Events remaining on queue');
+}
diff --git a/src/tests/utils/constants.cairo b/src/tests/utils/constants.cairo
@@ -0,0 +1,33 @@
+use starknet::ContractAddress;
+use starknet::contract_address_const;
+
+const ROLE: felt252 = 'ROLE';
+const OTHER_ROLE: felt252 = 'OTHER_ROLE';
+
+fn ADMIN() -> ContractAddress {
+    contract_address_const::<'ADMIN'>()
+}
+
+fn AUTHORIZED() -> ContractAddress {
+    contract_address_const::<'AUTHORIZED'>()
+}
+
+fn ZERO() -> ContractAddress {
+    contract_address_const::<0>()
+}
+
+fn OWNER() -> ContractAddress {
+    contract_address_const::<'OWNER'>()
+}
+
+fn NEW_OWNER() -> ContractAddress {
+    contract_address_const::<'NEW_OWNER'>()
+}
+
+fn OTHER() -> ContractAddress {
+    contract_address_const::<'OTHER'>()
+}
+
+fn OTHER_ADMIN() -> ContractAddress {
+    contract_address_const::<'OTHER_ADMIN'>()
+}