Add account events (#687)

* feat: add tests for ownable events * define account events * add event to set_public_key * add event test * change param name * fix event emit param * fix formatting * add tests for events * add default checks in tests * feat: improve tests * Apply suggestions from code review Co-authored-by: Eric Nordelo <[email protected]> * reorder imports, remove Account prefix * fix events * fix formatting * Update src/tests/utils.cairo Co-authored-by: Martín Triay <[email protected]> * add event checks, use ZERO from constants * fix formatting --------- Co-authored-by: Eric Nordelo <[email protected]> Co-authored-by: Martín Triay <[email protected]>
OpenZeppelin · Aug 11, 2023 · 457b98b · 457b98b
1 parent f9c30da
commit 457b98b
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 19 deletions.
diff --git a/src/account/account.cairo b/src/account/account.cairo
@@ -49,6 +49,23 @@ mod Account {
         public_key: felt252
     }
 
+    #[event]
+    #[derive(Drop, starknet::Event)]
+    enum Event {
+        OwnerAdded: OwnerAdded,
+        OwnerRemoved: OwnerRemoved,
+    }
+
+    #[derive(Drop, starknet::Event)]
+    struct OwnerAdded {
+        new_owner_guid: felt252
+    }
+
+    #[derive(Drop, starknet::Event)]
+    struct OwnerRemoved {
+        removed_owner_guid: felt252
+    }
+
     #[constructor]
     fn constructor(ref self: ContractState, _public_key: felt252) {
         self.initializer(_public_key);
@@ -131,7 +148,8 @@ mod Account {
 
         fn set_public_key(ref self: ContractState, new_public_key: felt252) {
             assert_only_self();
-            self.public_key.write(new_public_key);
+            self.emit(OwnerRemoved { removed_owner_guid: self.public_key.read() });
+            self._set_public_key(new_public_key);
         }
     }
 
@@ -142,8 +160,7 @@ mod Account {
         }
 
         fn setPublicKey(ref self: ContractState, newPublicKey: felt252) {
-            assert_only_self();
-            self.public_key.write(newPublicKey);
+            PublicKeyImpl::set_public_key(ref self, newPublicKey);
         }
     }
 
@@ -166,7 +183,7 @@ mod Account {
         fn initializer(ref self: ContractState, _public_key: felt252) {
             let mut unsafe_state = SRC5::unsafe_new_contract_state();
             SRC5::InternalImpl::register_interface(ref unsafe_state, interface::ISRC6_ID);
-            self.public_key.write(_public_key);
+            self._set_public_key(_public_key);
         }
 
         fn validate_transaction(self: @ContractState) -> felt252 {
@@ -177,6 +194,11 @@ mod Account {
             starknet::VALIDATED
         }
 
+        fn _set_public_key(ref self: ContractState, new_public_key: felt252) {
+            self.public_key.write(new_public_key);
+            self.emit(OwnerAdded { new_owner_guid: new_public_key });
+        }
+
         fn _is_valid_signature(
             self: @ContractState, hash: felt252, signature: Span<felt252>
         ) -> bool {

diff --git a/src/tests/account/test_account.cairo b/src/tests/account/test_account.cairo
@@ -1,5 +1,9 @@
 use array::ArrayTrait;
 use core::traits::Into;
+use openzeppelin::account::Account::OwnerAdded;
+use openzeppelin::account::Account::OwnerRemoved;
+use openzeppelin::account::Account::PublicKeyCamelImpl;
+use openzeppelin::account::Account::PublicKeyImpl;
 use openzeppelin::account::Account;
 use openzeppelin::account::AccountABIDispatcher;
 use openzeppelin::account::AccountABIDispatcherTrait;
@@ -8,6 +12,7 @@ use openzeppelin::account::TRANSACTION_VERSION;
 use openzeppelin::account::interface::ISRC6_ID;
 use openzeppelin::introspection::interface::ISRC5_ID;
 use openzeppelin::tests::utils;
+use openzeppelin::tests::utils::constants::ZERO;
 use openzeppelin::token::erc20::ERC20;
 use openzeppelin::token::erc20::interface::IERC20Dispatcher;
 use openzeppelin::token::erc20::interface::IERC20DispatcherTrait;
@@ -100,9 +105,12 @@ fn deploy_erc20(recipient: ContractAddress, initial_supply: u256) -> IERC20Dispa
 fn test_constructor() {
     let mut state = STATE();
     Account::constructor(ref state, PUBLIC_KEY);
-    assert(
-        Account::PublicKeyImpl::get_public_key(@state) == PUBLIC_KEY, 'Should return public key'
-    );
+
+    let event = testing::pop_log::<OwnerAdded>(ZERO()).unwrap();
+    assert(event.new_owner_guid == PUBLIC_KEY, 'Invalid owner key');
+    utils::assert_no_events_left(ZERO());
+
+    assert(PublicKeyImpl::get_public_key(@state) == PUBLIC_KEY, 'Should return public key');
 }
 
 //
@@ -149,7 +157,7 @@ fn test_is_valid_signature() {
     let mut good_signature = array![data.r, data.s];
     let mut bad_signature = array![0x987, 0x564];
 
-    Account::PublicKeyImpl::set_public_key(ref state, data.public_key);
+    PublicKeyImpl::set_public_key(ref state, data.public_key);
 
     let is_valid = Account::SRC6Impl::is_valid_signature(@state, hash, good_signature);
     assert(is_valid == starknet::VALIDATED, 'Should accept valid signature');
@@ -168,7 +176,7 @@ fn test_isValidSignature() {
     let mut good_signature = array![data.r, data.s];
     let mut bad_signature = array![0x987, 0x564];
 
-    Account::PublicKeyImpl::set_public_key(ref state, data.public_key);
+    PublicKeyImpl::set_public_key(ref state, data.public_key);
 
     let is_valid = Account::SRC6CamelOnlyImpl::isValidSignature(@state, hash, good_signature);
     assert(is_valid == starknet::VALIDATED, 'Should accept valid signature');
@@ -437,9 +445,21 @@ fn test_public_key_setter_and_getter() {
     testing::set_contract_address(ACCOUNT_ADDRESS());
     testing::set_caller_address(ACCOUNT_ADDRESS());
 
-    Account::PublicKeyImpl::set_public_key(ref state, NEW_PUBKEY);
+    // Check default
+    let public_key = PublicKeyImpl::get_public_key(@state);
+    assert(public_key == 0, 'Should be zero');
+
+    // Set key
+    PublicKeyImpl::set_public_key(ref state, NEW_PUBKEY);
+
+    let event = testing::pop_log::<OwnerRemoved>(ACCOUNT_ADDRESS()).unwrap();
+    assert(event.removed_owner_guid == 0, 'Invalid old owner key');
 
-    let public_key = Account::PublicKeyImpl::get_public_key(@state);
+    let event = testing::pop_log::<OwnerAdded>(ACCOUNT_ADDRESS()).unwrap();
+    assert(event.new_owner_guid == NEW_PUBKEY, 'Invalid new owner key');
+    utils::assert_no_events_left(ACCOUNT_ADDRESS());
+
+    let public_key = PublicKeyImpl::get_public_key(@state);
     assert(public_key == NEW_PUBKEY, 'Should update key');
 }
 
@@ -452,7 +472,7 @@ fn test_public_key_setter_different_account() {
     testing::set_contract_address(ACCOUNT_ADDRESS());
     testing::set_caller_address(caller);
 
-    Account::PublicKeyImpl::set_public_key(ref state, NEW_PUBKEY);
+    PublicKeyImpl::set_public_key(ref state, NEW_PUBKEY);
 }
 
 //
@@ -466,9 +486,21 @@ fn test_public_key_setter_and_getter_camel() {
     testing::set_contract_address(ACCOUNT_ADDRESS());
     testing::set_caller_address(ACCOUNT_ADDRESS());
 
-    Account::PublicKeyCamelImpl::setPublicKey(ref state, NEW_PUBKEY);
+    // Check default
+    let public_key = PublicKeyCamelImpl::getPublicKey(@state);
+    assert(public_key == 0, 'Should be zero');
+
+    // Set key
+    PublicKeyCamelImpl::setPublicKey(ref state, NEW_PUBKEY);
+
+    let event = testing::pop_log::<OwnerRemoved>(ACCOUNT_ADDRESS()).unwrap();
+    assert(event.removed_owner_guid == 0, 'Invalid old owner key');
 
-    let public_key = Account::PublicKeyCamelImpl::getPublicKey(@state);
+    let event = testing::pop_log::<OwnerAdded>(ACCOUNT_ADDRESS()).unwrap();
+    assert(event.new_owner_guid == NEW_PUBKEY, 'Invalid new owner key');
+    utils::assert_no_events_left(ACCOUNT_ADDRESS());
+
+    let public_key = PublicKeyCamelImpl::getPublicKey(@state);
     assert(public_key == NEW_PUBKEY, 'Should update key');
 }
 
@@ -481,7 +513,7 @@ fn test_public_key_setter_different_account_camel() {
     testing::set_contract_address(ACCOUNT_ADDRESS());
     testing::set_caller_address(caller);
 
-    Account::PublicKeyCamelImpl::setPublicKey(ref state, NEW_PUBKEY);
+    PublicKeyCamelImpl::setPublicKey(ref state, NEW_PUBKEY);
 }
 
 //
@@ -493,9 +525,12 @@ fn test_public_key_setter_different_account_camel() {
 fn test_initializer() {
     let mut state = STATE();
     Account::InternalImpl::initializer(ref state, PUBLIC_KEY);
-    assert(
-        Account::PublicKeyImpl::get_public_key(@state) == PUBLIC_KEY, 'Should return public key'
-    );
+
+    let event = testing::pop_log::<OwnerAdded>(ZERO()).unwrap();
+    assert(event.new_owner_guid == PUBLIC_KEY, 'Invalid owner key');
+    utils::assert_no_events_left(ZERO());
+
+    assert(PublicKeyImpl::get_public_key(@state) == PUBLIC_KEY, 'Should return public key');
 }
 
 #[test]
@@ -527,7 +562,7 @@ fn test__is_valid_signature() {
     let mut bad_signature = array![0x987, 0x564];
     let mut invalid_length_signature = array![0x987];
 
-    Account::PublicKeyImpl::set_public_key(ref state, data.public_key);
+    PublicKeyImpl::set_public_key(ref state, data.public_key);
 
     let is_valid = Account::InternalImpl::_is_valid_signature(@state, hash, good_signature.span());
     assert(is_valid, 'Should accept valid signature');
@@ -540,3 +575,17 @@ fn test__is_valid_signature() {
     );
     assert(!is_valid, 'Should reject invalid length');
 }
+
+#[test]
+#[available_gas(2000000)]
+fn test__set_public_key() {
+    let mut state = STATE();
+    Account::InternalImpl::_set_public_key(ref state, PUBLIC_KEY);
+
+    let event = testing::pop_log::<OwnerAdded>(ZERO()).unwrap();
+    assert(event.new_owner_guid == PUBLIC_KEY, 'Invalid owner key');
+    utils::assert_no_events_left(ZERO());
+
+    let public_key = PublicKeyImpl::get_public_key(@state);
+    assert(public_key == PUBLIC_KEY, 'Should update key');
+}