Remove governor duplicates in modules (#67)

* Use modifier * Remove duplicate governor * Remove governor duplicates
voteagora · Nov 25, 2024 · db92e5d · db92e5d
1 parent 1ac1177
commit db92e5d
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 27 deletions.
diff --git a/src/modules/ApprovalVotingModule.sol b/src/modules/ApprovalVotingModule.sol
@@ -42,7 +42,6 @@ struct ProposalOption {
 }
 
 struct Proposal {
-    address governor;
     uint256 initBalance;
     uint128[] optionVotes;
     ProposalOption[] options;
@@ -92,13 +91,16 @@ contract ApprovalVotingModule is VotingModule {
      * @param proposalId The id of the proposal.
      * @param proposalData The proposal data encoded as `PROPOSAL_DATA_ENCODING`.
      */
-    function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash) external override {
-        _onlyGovernor();
-        if (proposalId != uint256(keccak256(abi.encode(msg.sender, address(this), proposalData, descriptionHash)))) {
+    function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash)
+        external
+        override
+        onlyGovernor
+    {
+        if (proposalId != uint256(keccak256(abi.encode(governor, address(this), proposalData, descriptionHash)))) {
             revert WrongProposalId();
         }
 
-        if (proposals[proposalId].governor != address(0)) {
+        if (proposals[proposalId].optionVotes.length != 0) {
             revert ExistingProposal();
         }
 
@@ -128,7 +130,6 @@ contract ApprovalVotingModule is VotingModule {
             }
         }
 
-        proposals[proposalId].governor = msg.sender;
         proposals[proposalId].settings = proposalSettings;
         proposals[proposalId].optionVotes = new uint128[](optionsLength);
     }
@@ -146,8 +147,8 @@ contract ApprovalVotingModule is VotingModule {
         external
         virtual
         override
+        onlyGovernor
     {
-        _onlyGovernor();
         Proposal memory proposal = proposals[proposalId];
 
         if (support == uint8(VoteType.For)) {
@@ -175,19 +176,18 @@ contract ApprovalVotingModule is VotingModule {
     function _formatExecuteParams(uint256 proposalId, bytes memory proposalData)
         public
         override
+        onlyGovernor
         returns (address[] memory targets, uint256[] memory values, bytes[] memory calldatas)
     {
-        _onlyGovernor();
         (ProposalOption[] memory options, ProposalSettings memory settings) =
             abi.decode(proposalData, (ProposalOption[], ProposalSettings));
 
         {
-            IAgoraGovernor governor = IAgoraGovernor(proposals[proposalId].governor);
-
             // If budgetToken is not ETH
             if (settings.budgetToken != address(0)) {
                 // Save initBalance to be used as comparison in `_afterExecute`
-                proposals[proposalId].initBalance = IERC20(settings.budgetToken).balanceOf(governor.timelock());
+                proposals[proposalId].initBalance =
+                    IERC20(settings.budgetToken).balanceOf(IAgoraGovernor(governor).timelock());
             }
         }
 
@@ -286,10 +286,8 @@ contract ApprovalVotingModule is VotingModule {
         (, ProposalSettings memory settings) = abi.decode(proposalData, (ProposalOption[], ProposalSettings));
 
         if (settings.budgetToken != address(0) && settings.budgetAmount > 0) {
-            IAgoraGovernor governor = IAgoraGovernor(proposals[proposalId].governor);
-
             uint256 initBalance = proposals[proposalId].initBalance;
-            uint256 finalBalance = IERC20(settings.budgetToken).balanceOf(governor.timelock());
+            uint256 finalBalance = IERC20(settings.budgetToken).balanceOf(IAgoraGovernor(governor).timelock());
 
             // If `finalBalance` is higher than `initBalance`, ignore the budget check
             if (finalBalance < initBalance) {

diff --git a/src/modules/OptimisticModule.sol b/src/modules/OptimisticModule.sol
@@ -19,7 +19,6 @@ struct ProposalSettings {
 }
 
 struct Proposal {
-    address governor;
     ProposalSettings settings;
 }
 
@@ -60,21 +59,24 @@ contract OptimisticModule is VotingModule {
      * @param proposalId The id of the proposal.
      * @param proposalData The proposal data encoded as `PROPOSAL_DATA_ENCODING`.
      */
-    function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash) external override {
-        _onlyGovernor();
-        if (proposalId != uint256(keccak256(abi.encode(msg.sender, address(this), proposalData, descriptionHash)))) {
+    function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash)
+        external
+        override
+        onlyGovernor
+    {
+        if (proposalId != uint256(keccak256(abi.encode(governor, address(this), proposalData, descriptionHash)))) {
             revert WrongProposalId();
         }
 
-        if (proposals[proposalId].governor != address(0)) {
+        if (proposals[proposalId].settings.againstThreshold != 0) {
             revert ExistingProposal();
         }
 
         ProposalSettings memory proposalSettings = abi.decode(proposalData, (ProposalSettings));
 
-        uint8 proposalTypeId = IAgoraGovernor(msg.sender).getProposalType(proposalId);
+        uint8 proposalTypeId = IAgoraGovernor(governor).getProposalType(proposalId);
         IProposalTypesConfigurator proposalConfigurator =
-            IProposalTypesConfigurator(IAgoraGovernor(msg.sender).PROPOSAL_TYPES_CONFIGURATOR());
+            IProposalTypesConfigurator(IAgoraGovernor(governor).PROPOSAL_TYPES_CONFIGURATOR());
         IProposalTypesConfigurator.ProposalType memory proposalType = proposalConfigurator.proposalTypes(proposalTypeId);
 
         if (proposalType.quorum != 0 || proposalType.approvalThreshold != 0) {
@@ -87,7 +89,6 @@ contract OptimisticModule is VotingModule {
             revert InvalidParams();
         }
 
-        proposals[proposalId].governor = msg.sender;
         proposals[proposalId].settings = proposalSettings;
     }
 
@@ -127,12 +128,12 @@ contract OptimisticModule is VotingModule {
      */
     function _voteSucceeded(uint256 proposalId) external view override returns (bool) {
         Proposal memory proposal = proposals[proposalId];
-        (uint256 againstVotes,,) = IAgoraGovernor(proposal.governor).proposalVotes(proposalId);
+        (uint256 againstVotes,,) = IAgoraGovernor(governor).proposalVotes(proposalId);
 
         uint256 againstThreshold = proposal.settings.againstThreshold;
         if (proposal.settings.isRelativeToVotableSupply) {
-            uint256 snapshotBlock = IGovernorUpgradeable(proposal.governor).proposalSnapshot(proposalId);
-            IVotesUpgradeable token = IAgoraGovernor(proposal.governor).token();
+            uint256 snapshotBlock = IGovernorUpgradeable(governor).proposalSnapshot(proposalId);
+            IVotesUpgradeable token = IAgoraGovernor(governor).token();
             againstThreshold = (token.getPastTotalSupply(snapshotBlock) * againstThreshold) / PERCENT_DIVISOR;
         }
 

diff --git a/src/modules/VotingModule.sol b/src/modules/VotingModule.sol
@@ -21,8 +21,9 @@ abstract contract VotingModule {
                                MODIFIERS
     //////////////////////////////////////////////////////////////*/
 
-    function _onlyGovernor() internal view {
+    modifier onlyGovernor() {
         if (msg.sender != governor) revert NotGovernor();
+        _;
     }
 
     /*//////////////////////////////////////////////////////////////

diff --git a/test/ApprovalVotingModule.t.sol b/test/ApprovalVotingModule.t.sol
@@ -65,7 +65,6 @@ contract ApprovalVotingModuleTest is Test {
 
         Proposal memory proposal = module._proposals(proposalId);
 
-        assertEq(proposal.governor, governor);
         assertEq(proposal.optionVotes[0], 0);
         assertEq(proposal.optionVotes[1], 0);
         assertEq(proposal.optionVotes[2], 0);