feat: add more tests

packages/governance/Scarb.toml

@@ -47,6 +47,7 @@ name = "openzeppelin_governance_unittest"
 build-external-contracts = [
     "openzeppelin_test_common::mocks::account::SnakeAccountMock",
     "openzeppelin_test_common::mocks::governor::GovernorMock",
+    "openzeppelin_test_common::mocks::governor::GovernorTimelockedMock",
     "openzeppelin_test_common::mocks::timelock::TimelockControllerMock",
     "openzeppelin_test_common::mocks::timelock::MockContract",
     "openzeppelin_test_common::mocks::timelock::TimelockAttackerMock",

packages/governance/src/governor/extensions/governor_settings.cairo

@@ -184,9 +184,9 @@ pub mod GovernorSettingsComponent {
             new_voting_period: u64,
             new_proposal_threshold: u256
         ) {
-            self.set_voting_delay(new_voting_delay);
-            self.set_voting_period(new_voting_period);
-            self.set_proposal_threshold(new_proposal_threshold);
+            self._set_voting_delay(new_voting_delay);
+            self._set_voting_period(new_voting_period);
+            self._set_proposal_threshold(new_proposal_threshold);
         }
 
         /// Wrapper for `Governor::assert_only_governance`.

packages/governance/src/governor/extensions/governor_timelock_execution.cairo

@@ -66,6 +66,9 @@ pub mod GovernorTimelockExecutionComponent {
     // Extensions
     //
 
+    /// NOTE: Some of these function can reenter through the external calls to the timelock, but we
+    /// assume the timelock is trusted and well behaved (according to TimelockController) and this
+    /// will not happen.
     pub impl GovernorExecution<
         TContractState,
         +GovernorComponent::HasComponent<TContractState>,
@@ -174,10 +177,6 @@ pub mod GovernorTimelockExecutionComponent {
         /// See `GovernorComponent::GovernorExecutionTrait::cancel_operations`.
         ///
         /// Cancels the timelocked proposal if it has already been queued.
-        ///
-        /// NOTE: This function can reenter through the external call to the timelock, but we assume
-        /// the timelock is trusted and well behaved (according to TimelockController) and this will
-        /// not happen.
         fn cancel_operations(
             ref self: GovernorComponentState<TContractState>,
             proposal_id: felt252,
@@ -218,6 +217,13 @@ pub mod GovernorTimelockExecutionComponent {
             self.Governor_timelock_controller.read()
         }
 
+        /// Returns the timelock proposal id for a given proposal id.
+        fn get_timelock_id(
+            self: @ComponentState<TContractState>, proposal_id: felt252
+        ) -> TimelockProposalId {
+            self.Governor_timelock_ids.read(proposal_id)
+        }
+
         /// Updates the associated timelock.
         ///
         /// Requirements:

packages/governance/src/governor/extensions/interface.cairo

@@ -29,6 +29,9 @@ pub trait ITimelocked<TState> {
     /// Returns address of the associated timelock.
     fn timelock(self: @TState) -> ContractAddress;
 
+    /// Returns the timelock proposal id for a given proposal id.
+    fn get_timelock_id(self: @TState, proposal_id: felt252) -> felt252;
+
     /// Updates the associated timelock.
     fn update_timelock(ref self: TState, new_timelock: ContractAddress);
 }

packages/governance/src/tests/governor.cairo

@@ -3,3 +3,4 @@ mod test_governor;
 mod test_governor_core_execution;
 mod test_governor_counting_simple;
 mod test_governor_settings;
+mod test_governor_timelock_execution;

packages/governance/src/tests/governor/common.cairo

@@ -96,6 +96,12 @@ pub fn get_mock_state(
     }
 }
 
+pub fn set_executor(
+    ref mock_state: GovernorTimelockedMock::ContractState, executor: ContractAddress
+) {
+    mock_state.governor_timelock_execution.Governor_timelock_controller.write(executor);
+}
+
 //
 // Setup proposals
 //

packages/governance/src/tests/governor/test_governor.cairo

@@ -19,7 +19,7 @@ use openzeppelin_test_common::mocks::timelock::{
     IMockContractDispatcher, IMockContractDispatcherTrait
 };
 use openzeppelin_testing as utils;
-use openzeppelin_testing::constants::{ADMIN, OTHER, ZERO};
+use openzeppelin_testing::constants::{ADMIN, OTHER, ZERO, VOTES_TOKEN};
 use openzeppelin_testing::events::EventSpyExt;
 use openzeppelin_utils::bytearray::ByteArrayExtTrait;
 use openzeppelin_utils::cryptography::snip12::OffchainMessageHash;
@@ -33,14 +33,6 @@ use starknet::account::Call;
 use starknet::storage::{StoragePathEntry, StoragePointerWriteAccess, StorageMapWriteAccess};
 use starknet::{ContractAddress, contract_address_const};
 
-//
-// Constants
-//
-
-fn VOTES_TOKEN() -> ContractAddress {
-    contract_address_const::<'VOTES_TOKEN'>()
-}
-
 //
 // Dispatchers
 //
@@ -2071,6 +2063,22 @@ pub(crate) impl GovernorSpyHelpersImpl of GovernorSpyHelpers {
         self.assert_no_events_left_from(contract);
     }
 
+    fn assert_event_proposal_queued(
+        ref self: EventSpy, contract: ContractAddress, proposal_id: felt252, eta_seconds: u64
+    ) {
+        let expected = GovernorComponent::Event::ProposalQueued(
+            GovernorComponent::ProposalQueued { proposal_id, eta_seconds }
+        );
+        self.assert_emitted_single(contract, expected);
+    }
+
+    fn assert_only_event_proposal_queued(
+        ref self: EventSpy, contract: ContractAddress, proposal_id: felt252, eta_seconds: u64
+    ) {
+        self.assert_event_proposal_queued(contract, proposal_id, eta_seconds);
+        self.assert_no_events_left_from(contract);
+    }
+
     fn assert_event_proposal_executed(
         ref self: EventSpy, contract: ContractAddress, proposal_id: felt252
     ) {

packages/governance/src/tests/governor/test_governor_settings.cairo

@@ -5,11 +5,11 @@ use crate::governor::extensions::GovernorSettingsComponent::{
     GovernorSettings, GovernorSettingsAdminImpl
 };
 use crate::governor::extensions::GovernorSettingsComponent;
+use crate::tests::governor::common::set_executor;
 use crate::tests::governor::common::{
     COMPONENT_STATE_TIMELOCKED as COMPONENT_STATE, CONTRACT_STATE_TIMELOCKED as CONTRACT_STATE
 };
 use openzeppelin_test_common::mocks::governor::GovernorTimelockedMock::SNIP12MetadataImpl;
-use openzeppelin_test_common::mocks::governor::GovernorTimelockedMock;
 use openzeppelin_testing::constants::OTHER;
 use openzeppelin_testing::events::EventSpyExt;
 use snforge_std::start_cheat_caller_address;
@@ -388,14 +388,6 @@ fn test__set_proposal_threshold_no_change() {
     spy.assert_no_events_left();
 }
 
-//
-// Helpers
-//
-
-fn set_executor(ref mock_state: GovernorTimelockedMock::ContractState, executor: ContractAddress) {
-    mock_state.governor_timelock_execution.Governor_timelock_controller.write(executor);
-}
-
 //
 // Event helpers
 //