Minor governance module improvements plus adding veryfing_contract to Delegation

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed (Breaking)
 
+- Add `verifying_contract` member to the `Delegation` struct used in Votes `delegate_by_sig` (#1214)
 - VestingComponent `release` function won't emit an event or attempt to transfer when the amount is zero (#1209)
 - Bump snforge_std to v0.33.0 (#1203)
 

docs/modules/ROOT/pages/api/governance.adoc

@@ -131,10 +131,10 @@ Schedule an operation containing a single transaction.
 
 Requirements:
 
-- the caller must have the `PROPOSER_ROLE` role.
+- The caller must have the `PROPOSER_ROLE` role.
 
 Emits {CallScheduled} event.
-If `salt` is not zero, emits {CallSalt} event.
+Emits {CallSalt} event if `salt` is not zero.
 
 [.contract-item]
 [[ITimelock-schedule_batch]]
@@ -147,7 +147,7 @@ Requirements:
 - The caller must have the `PROPOSER_ROLE` role.
 
 Emits one {CallScheduled} event for each transaction in the batch.
-If `salt` is not zero, emits {CallSalt} event.
+Emits {CallSalt} event if `salt` is not zero.
 
 [.contract-item]
 [[ITimelock-cancel]]
@@ -406,10 +406,12 @@ Schedule an operation containing a single transaction.
 
 Requirements:
 
-- the caller must have the `PROPOSER_ROLE` role.
+- The caller must have the `PROPOSER_ROLE` role.
+- The proposal must not already exist.
+- `delay` must not be greater than or equal to the min delay.
 
 Emits {CallScheduled} event.
-If `salt` is not zero, emits {CallSalt} event.
+Emits {CallSalt} event if `salt` is not zero.
 
 [.contract-item]
 [[TimelockControllerComponent-schedule_batch]]
@@ -420,9 +422,11 @@ Schedule an operation containing a batch of transactions.
 Requirements:
 
 - The caller must have the `PROPOSER_ROLE` role.
+- The proposal must not already exist.
+- `delay` must not be greater than or equal to the min delay.
 
 Emits one {CallScheduled} event for each transaction in the batch.
-If `salt` is not zero, emits {CallSalt} event.
+Emits {CallSalt} event if `salt` is not zero.
 
 [.contract-item]
 [[TimelockControllerComponent-cancel]]

packages/governance/src/multisig/multisig.cairo

@@ -130,6 +130,10 @@ pub mod MultisigComponent {
         pub const QUORUM_TOO_HIGH: felt252 = 'Multisig: quorum > signers';
     }
 
+    //
+    // External
+    //
+
     #[embeddable_as(MultisigImpl)]
     impl Multisig<
         TContractState, +HasComponent<TContractState>, +Drop<TContractState>
@@ -140,7 +144,7 @@ pub mod MultisigComponent {
             self.Multisig_signers_info.read().quorum
         }
 
-        /// Returns whether the given `signer` is a registered signer.
+        /// Returns whether the given `signer` is registered.
         /// Only registered signers can submit, confirm, or execute transactions.
         fn is_signer(self: @ComponentState<TContractState>, signer: ContractAddress) -> bool {
             self.Multisig_is_signer.read(signer)
@@ -173,7 +177,8 @@ pub mod MultisigComponent {
         /// NOTE: Even if a `signer` is removed after confirming a transaction, this function will
         /// still return true. However, their confirmation does not count toward the quorum when it
         /// is checked. Therefore, this function should not be relied upon to verify legitimate
-        /// confirmations toward meeting the quorum.
+        /// confirmations toward meeting the quorum. For that, use `get_transaction_confirmations`
+        /// instead.
         fn is_confirmed_by(
             self: @ComponentState<TContractState>, id: TransactionID, signer: ContractAddress
         ) -> bool {
@@ -227,6 +232,7 @@ pub mod MultisigComponent {
         /// - `new_quorum` must be less than or equal to the total number of signers after addition.
         ///
         /// Emits a `SignerAdded` event for each signer added.
+        /// Emits a `QuorumUpdated` event if the quorum changes.
         fn add_signers(
             ref self: ComponentState<TContractState>,
             new_quorum: u32,
@@ -244,6 +250,7 @@ pub mod MultisigComponent {
         /// - `new_quorum` must be less than or equal to the total number of signers after removal.
         ///
         /// Emits a `SignerRemoved` event for each signer removed.
+        /// Emits a `QuorumUpdated` event if the quorum changes.
         fn remove_signers(
             ref self: ComponentState<TContractState>,
             new_quorum: u32,
@@ -260,6 +267,7 @@ pub mod MultisigComponent {
         /// - The caller must be the contract itself.
         /// - `signer_to_remove` must be an existing signer.
         /// - `signer_to_add` must not be an existing signer.
+        /// - `signer_to_add` must be a non-zero address.
         ///
         /// Emits a `SignerRemoved` event for the removed signer.
         /// Emits a `SignerAdded` event for the new signer.
@@ -291,9 +299,10 @@ pub mod MultisigComponent {
         /// Requirements:
         ///
         /// - The caller must be a registered signer.
+        /// - The transaction must not have been submitted before.
         ///
         /// Emits a `TransactionSubmitted` event.
-        /// If `salt` is not zero, emits a `CallSalt` event.
+        /// Emits a `CallSalt` event if `salt` is not zero.
         fn submit_transaction(
             ref self: ComponentState<TContractState>,
             to: ContractAddress,
@@ -310,9 +319,10 @@ pub mod MultisigComponent {
         /// Requirements:
         ///
         /// - The caller must be a registered signer.
+        /// - The transaction must not have been submitted before.
         ///
         /// Emits a `TransactionSubmitted` event.
-        /// If `salt` is not zero, emits a `CallSalt` event.
+        /// Emits a `CallSalt` event if `salt` is not zero.
         fn submit_transaction_batch(
             ref self: ComponentState<TContractState>, calls: Span<Call>, salt: felt252
         ) -> TransactionID {
@@ -441,6 +451,10 @@ pub mod MultisigComponent {
         }
     }
 
+    //
+    // Internal
+    //
+
     #[generate_trait]
     pub impl InternalImpl<
         TContractState, +HasComponent<TContractState>, +Drop<TContractState>

packages/governance/src/tests/test_votes.cairo

@@ -1,9 +1,7 @@
-use crate::votes::delegation::Delegation;
-use crate::votes::votes::VotesComponent::{
-    DelegateChanged, DelegateVotesChanged, VotesImpl, InternalImpl,
-};
-use crate::votes::votes::VotesComponent;
-use crate::votes::votes::VotingUnitsTrait;
+use crate::votes::Delegation;
+use crate::votes::VotesComponent::VotingUnitsTrait;
+use crate::votes::VotesComponent::{DelegateChanged, DelegateVotesChanged, VotesImpl, InternalImpl,};
+use crate::votes::VotesComponent;
 use openzeppelin_test_common::mocks::votes::ERC721VotesMock::SNIP12MetadataImpl;
 use openzeppelin_test_common::mocks::votes::{ERC721VotesMock, ERC20VotesMock};
 use openzeppelin_testing as utils;
@@ -267,11 +265,12 @@ fn test_delegate_by_sig() {
     // Set up delegation parameters
     let nonce = 0;
     let expiry = 'ts2';
+    let verifying_contract = contract_address;
     let delegator = account;
     let delegatee = DELEGATEE();
 
     // Create and sign the delegation message
-    let delegation = Delegation { delegatee, nonce, expiry };
+    let delegation = Delegation { verifying_contract, delegatee, nonce, expiry };
     let msg_hash = delegation.get_message_hash(delegator);
     let (r, s) = key_pair.sign(msg_hash).unwrap();
 
@@ -292,8 +291,9 @@ fn test_delegate_by_sig_hash_generation() {
     let delegator = contract_address_const::<
         0x70b0526a4bfbc9ca717c96aeb5a8afac85181f4585662273668928585a0d628
     >();
+    let verifying_contract = contract_address_const::<'VERIFIER'>();
     let delegatee = RECIPIENT();
-    let delegation = Delegation { delegatee, nonce, expiry };
+    let delegation = Delegation { verifying_contract, delegatee, nonce, expiry };
 
     let hash = delegation.get_message_hash(delegator);
 
@@ -302,11 +302,12 @@ fn test_delegate_by_sig_hash_generation() {
     // - version: 'DAPP_VERSION'
     // - chainId: 'SN_TEST'
     // - account: 0x70b0526a4bfbc9ca717c96aeb5a8afac85181f4585662273668928585a0d628
+    // - verifying_contract: 'VERIFIER'
     // - delegatee: 'RECIPIENT'
     // - nonce: 0
     // - expiry: 'ts2'
     // - revision: '1'
-    let expected_hash = 0x314bd38b22b62d576691d8dafd9f8ea0601329ebe686bc64ca28e4d8821d5a0;
+    let expected_hash = 0x1fa1af6d3d0ede7d09790ef20a894668af9445b6bc93714e87a758be40efdc7;
     assert_eq!(hash, expected_hash);
 }
 
@@ -342,7 +343,8 @@ fn test_delegate_by_sig_invalid_signature() {
     let expiry = 'ts2';
     let delegator = account;
     let delegatee = DELEGATEE();
-    let delegation = Delegation { delegatee, nonce, expiry };
+    let verifying_contract = test_address();
+    let delegation = Delegation { verifying_contract, delegatee, nonce, expiry };
     let msg_hash = delegation.get_message_hash(delegator);
     let (r, s) = key_pair.sign(msg_hash).unwrap();
 
@@ -362,8 +364,9 @@ fn test_delegate_by_sig_bad_delegatee() {
     let expiry = 'ts2';
     let delegator = account;
     let delegatee = DELEGATEE();
+    let verifying_contract = test_address();
     let bad_delegatee = contract_address_const::<0x1234>();
-    let delegation = Delegation { delegatee, nonce, expiry };
+    let delegation = Delegation { verifying_contract, delegatee, nonce, expiry };
     let msg_hash = delegation.get_message_hash(delegator);
     let (r, s) = key_pair.sign(msg_hash).unwrap();
 
@@ -383,7 +386,8 @@ fn test_delegate_by_sig_reused_signature() {
     let expiry = 'ts2';
     let delegator = account;
     let delegatee = DELEGATEE();
-    let delegation = Delegation { delegatee, nonce, expiry };
+    let verifying_contract = test_address();
+    let delegation = Delegation { verifying_contract, delegatee, nonce, expiry };
     let msg_hash = delegation.get_message_hash(delegator);
     let (r, s) = key_pair.sign(msg_hash).unwrap();
 

packages/governance/src/timelock.cairo

@@ -1,7 +1,7 @@
 pub mod interface;
 pub mod timelock_controller;
 
-pub use timelock_controller::OperationState;
+pub use interface::OperationState;
 pub use timelock_controller::TimelockControllerComponent::{
     PROPOSER_ROLE, CANCELLER_ROLE, EXECUTOR_ROLE
 };

packages/governance/src/timelock/interface.cairo

@@ -1,10 +1,17 @@
 // SPDX-License-Identifier: MIT
 // OpenZeppelin Contracts for Cairo v0.19.0 (governance/timelock/interface.cairo)
 
-use crate::timelock::OperationState;
 use starknet::ContractAddress;
 use starknet::account::Call;
 
+#[derive(Drop, Serde, PartialEq, Debug)]
+pub enum OperationState {
+    Unset,
+    Waiting,
+    Ready,
+    Done
+}
+
 #[starknet::interface]
 pub trait ITimelock<TState> {
     fn is_operation(self: @TState, id: felt252) -> bool;

packages/governance/src/timelock/timelock_controller.cairo

@@ -17,7 +17,7 @@ pub mod TimelockControllerComponent {
     use core::hash::{HashStateTrait, HashStateExTrait};
     use core::num::traits::Zero;
     use core::pedersen::PedersenTrait;
-    use crate::timelock::interface::{ITimelock, TimelockABI};
+    use crate::timelock::interface::{OperationState, ITimelock, TimelockABI};
     use crate::utils::call_impls::{HashCallImpl, HashCallsImpl, CallPartialEq};
     use openzeppelin_access::accesscontrol::AccessControlComponent::InternalTrait as AccessControlInternalTrait;
     use openzeppelin_access::accesscontrol::AccessControlComponent::{
@@ -30,11 +30,8 @@ pub mod TimelockControllerComponent {
     use starknet::ContractAddress;
     use starknet::SyscallResultTrait;
     use starknet::account::Call;
-    use starknet::storage::{
-        Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess,
-        StoragePointerWriteAccess
-    };
-    use super::OperationState;
+    use starknet::storage::{Map, StorageMapReadAccess, StorageMapWriteAccess};
+    use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess};
 
     // Constants
     pub const PROPOSER_ROLE: felt252 = selector!("PROPOSER_ROLE");
@@ -112,6 +109,10 @@ pub mod TimelockControllerComponent {
         pub const UNAUTHORIZED_CALLER: felt252 = 'Timelock: unauthorized caller';
     }
 
+    //
+    // External
+    //
+
     #[embeddable_as(TimelockImpl)]
     impl Timelock<
         TContractState,
@@ -198,14 +199,16 @@ pub mod TimelockControllerComponent {
                 .finalize()
         }
 
-        /// Schedule an operation containing a single transaction.
+        /// Schedules an operation containing a single transaction.
         ///
         /// Requirements:
         ///
-        /// - the caller must have the `PROPOSER_ROLE` role.
+        /// - The caller must have the `PROPOSER_ROLE` role.
+        /// - The proposal must not already exist.
+        /// - `delay` must not be greater than or equal to the min delay.
         ///
         /// Emits `CallScheduled` event.
-        /// If `salt` is not zero, emits `CallSalt` event.
+        /// Emits `CallSalt` event if `salt` is not zero.
         fn schedule(
             ref self: ComponentState<TContractState>,
             call: Call,
@@ -224,14 +227,16 @@ pub mod TimelockControllerComponent {
             }
         }
 
-        /// Schedule an operation containing a batch of transactions.
+        /// Schedules an operation containing a batch of transactions.
         ///
         /// Requirements:
         ///
-        /// - the caller must have the `PROPOSER_ROLE` role.
+        /// - The caller must have the `PROPOSER_ROLE` role.
+        /// - The proposal must not already exist.
+        /// - `delay` must not be greater than or equal to the min delay.
         ///
         /// Emits one `CallScheduled` event for each transaction in the batch.
-        /// If `salt` is not zero, emits `CallSalt` event.
+        /// Emits `CallSalt` event if `salt` is not zero.
         fn schedule_batch(
             ref self: ComponentState<TContractState>,
             calls: Span<Call>,
@@ -255,12 +260,12 @@ pub mod TimelockControllerComponent {
             }
         }
 
-        /// Cancel an operation.
+        /// Cancels an operation.
         ///
         /// Requirements:
         ///
         /// - The caller must have the `CANCELLER_ROLE` role.
-        /// - `id` must be an operation.
+        /// - `id` must be a pending operation.
         ///
         /// Emits a `CallCancelled` event.
         fn cancel(ref self: ComponentState<TContractState>, id: felt252) {
@@ -271,7 +276,7 @@ pub mod TimelockControllerComponent {
             self.emit(CallCancelled { id });
         }
 
-        /// Execute a (Ready) operation containing a single Call.
+        /// Executes a (Ready) operation containing a single Call.
         ///
         /// Requirements:
         ///
@@ -299,7 +304,7 @@ pub mod TimelockControllerComponent {
             self._after_call(id);
         }
 
-        /// Execute a (Ready) operation containing a batch of Calls.
+        /// Executes a (Ready) operation containing a batch of Calls.
         ///
         /// Requirements:
         ///
@@ -522,6 +527,10 @@ pub mod TimelockControllerComponent {
         }
     }
 
+    //
+    // Internal
+    //
+
     #[generate_trait]
     pub impl InternalImpl<
         TContractState,
@@ -651,11 +660,3 @@ pub mod TimelockControllerComponent {
         }
     }
 }
-
-#[derive(Drop, Serde, PartialEq, Debug)]
-pub enum OperationState {
-    Unset,
-    Waiting,
-    Ready,
-    Done
-}

packages/governance/src/votes.cairo

@@ -1,5 +1,6 @@
 pub mod delegation;
 pub mod interface;
 pub mod votes;
+pub use delegation::Delegation;
 
 pub use votes::VotesComponent;