Address feedback

contracts/governance/Governor.sol

@@ -21,9 +21,9 @@ import {IGovernor, IERC6372} from "./IGovernor.sol";
  *
  * This contract is abstract and requires several functions to be implemented in various modules:
  *
- * - A counting module must implement {proposalVotes}, {_quorumReached}, {_voteSucceeded} and {_countVote}
- * - A voting module must implement {_getVotes} and {quorum}
- * - Additionally, {votingPeriod} must also be implemented
+ * - A counting module must implement {_quorumReached}, {_voteSucceeded} and {_countVote}
+ * - A voting module must implement {_getVotes}
+ * - Additionally, {votingPeriod}, {votingDelay}, and {quorum} must also be implemented
  */
 abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC721Receiver, IERC1155Receiver {
     using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
@@ -837,13 +837,6 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
      */
     function quorum(uint256 timepoint) public view virtual returns (uint256);
 
-    /**
-     * @inheritdoc IGovernor
-     */
-    function proposalVotes(
-        uint256 proposalId
-    ) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes);
-
     /**
      * @dev Reads a bytes32 from a bytes array without bounds checking.
      *

contracts/governance/IGovernor.sol

@@ -272,14 +272,6 @@ interface IGovernor is IERC165, IERC6372 {
      */
     function proposalNeedsQueuing(uint256 proposalId) external view returns (bool);
 
-    /**
-     * @notice module:core
-     * @dev Accessor to the internal vote counts.
-     */
-    function proposalVotes(
-        uint256 proposalId
-    ) external view returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes);
-
     /**
      * @notice module:user-config
      * @dev Delay, between the proposal is created and the vote starts. The unit this duration is expressed in depends
@@ -309,8 +301,7 @@ interface IGovernor is IERC165, IERC6372 {
 
     /**
      * @notice module:user-config
-     * @dev Minimum number of cast voted required for a proposal to be successful. Both FOR and ABSTAIN votes are counted
-     * towards the quorum.
+     * @dev Minimum number of cast voted required for a proposal to be successful.
      *
      * NOTE: The `timepoint` parameter corresponds to the snapshot used for counting vote. This allows to scale the
      * quorum depending on values such as the totalSupply of a token at this timepoint (see {ERC20Votes}).

contracts/governance/extensions/GovernorCountingFractional.sol

@@ -5,6 +5,7 @@ pragma solidity ^0.8.20;
 
 import {Governor} from "../Governor.sol";
 import {GovernorCountingSimple} from "./GovernorCountingSimple.sol";
+import {IGovernorCounting} from "./IGovernorCounting.sol";
 import {Math} from "../../utils/math/Math.sol";
 
 /**
@@ -31,7 +32,7 @@ import {Math} from "../../utils/math/Math.sol";
  *
  * _Available since v5.1._
  */
-abstract contract GovernorCountingFractional is Governor {
+abstract contract GovernorCountingFractional is Governor, IGovernorCounting {
     using Math for *;
 
     uint8 internal constant VOTE_TYPE_FRACTIONAL = 255;

contracts/governance/extensions/GovernorCountingOverridable.sol

@@ -7,12 +7,13 @@ import {SignatureChecker} from "../../utils/cryptography/SignatureChecker.sol";
 import {SafeCast} from "../../utils/math/SafeCast.sol";
 import {VotesExtended} from "../utils/VotesExtended.sol";
 import {GovernorVotes} from "./GovernorVotes.sol";
+import {IGovernorCounting} from "./IGovernorCounting.sol";
 
 /**
  * @dev Extension of {Governor} which enables delegators to override the vote of their delegates. This module requires a
  * token that inherits {VotesExtended}.
  */
-abstract contract GovernorCountingOverridable is GovernorVotes {
+abstract contract GovernorCountingOverridable is GovernorVotes, IGovernorCounting {
     bytes32 public constant OVERRIDE_BALLOT_TYPEHASH =
         keccak256("OverrideBallot(uint256 proposalId,uint8 support,address voter,uint256 nonce,string reason)");
 

contracts/governance/extensions/GovernorCountingSimple.sol

@@ -4,11 +4,12 @@
 pragma solidity ^0.8.20;
 
 import {Governor} from "../Governor.sol";
+import {IGovernorCounting} from "./IGovernorCounting.sol";
 
 /**
  * @dev Extension of {Governor} for simple, 3 options, vote counting.
  */
-abstract contract GovernorCountingSimple is Governor {
+abstract contract GovernorCountingSimple is Governor, IGovernorCounting {
     /**
      * @dev Supported vote types. Matches Governor Bravo ordering.
      */

contracts/governance/extensions/GovernorSuperQuorum.sol

@@ -2,12 +2,14 @@
 pragma solidity ^0.8.20;
 
 import {Governor} from "../Governor.sol";
+import {IGovernorCounting} from "./IGovernorCounting.sol";
 import {SafeCast} from "../../utils/math/SafeCast.sol";
 import {Checkpoints} from "../../utils/structs/Checkpoints.sol";
 
 /**
  * @dev Extension of {Governor} with a super quorum. Proposals that meet the super quorum can be executed
- * earlier than the proposal deadline.
+ * earlier than the proposal deadline. Counting modules that want to use this extension must implement
+ * {IGovernorCounting}.
  *
  * NOTE: It's up to developers to implement `superQuorum` and validate it against `quorum`.
  */
@@ -24,19 +26,20 @@ abstract contract GovernorSuperQuorum is Governor {
 
     /**
      * @dev Overridden version of the {Governor-state} function that checks if the proposal has reached the super quorum.
+     *
+     * NOTE: If the proposal reaches super quorum but {_voteSucceeded} returns false, eg, assuming the super quorum has been set
+     * low enough that both FOR and AGAINST votes have exceeded it and AGAINST votes exceed FOR votes, the proposal continues to
+     * be active until {_voteSucceeded} returns true or the proposal deadline is reached. This means that with a low super quorum
+     * it is also possible that a vote can succeed prematurely before enough AGAINST voters have a chance to vote. Hence, it is
+     * recommended to set a high enough super quorum to avoid these types of scenarios.
      */
     function state(uint256 proposalId) public view virtual override returns (ProposalState) {
         ProposalState currentState = super.state(proposalId);
         if (currentState != ProposalState.Active) return currentState;
 
-        (, uint256 forVotes, ) = proposalVotes(proposalId);
-        bool superQuorumReached = forVotes >= superQuorum(proposalSnapshot(proposalId));
-        if (!superQuorumReached) {
-            return currentState;
-        }
-
-        if (!_voteSucceeded(proposalId)) {
-            return ProposalState.Defeated;
+        (, uint256 forVotes, ) = IGovernorCounting(address(this)).proposalVotes(proposalId);
+        if (forVotes < superQuorum(proposalSnapshot(proposalId)) || !_voteSucceeded(proposalId)) {
+            return ProposalState.Active;
         } else if (proposalEta(proposalId) == 0) {
             return ProposalState.Succeeded;
         } else {

contracts/governance/extensions/IGovernorCounting.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.20;
+
+/**
+ * @dev Interface to count votes for the following counting modules:
+ * - GovernorCountingFractional
+ * - GovernorCountingSimple
+ * - GovernorCountingOverridable
+ *
+ * This interface is used by GovernorSuperQuorum to get counted votes from the above counting modules.
+ * Other counting modules that need to work with GovernorSuperQuorum MUST implement this interface.
+ */
+interface IGovernorCounting {
+    /**
+     * @dev Accessor to the internal vote counts.
+     */
+    function proposalVotes(
+        uint256 proposalId
+    ) external view returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes);
+}

test/governance/Governor.t.sol

@@ -56,6 +56,4 @@ contract GovernorInternalTest is Test, Governor {
     function _getVotes(address, uint256, bytes memory) internal pure virtual override returns (uint256) {}
 
     function _countVote(uint256, address, uint8, uint256, bytes memory) internal virtual override returns (uint256) {}
-
-    function proposalVotes(uint256) public view virtual override returns (uint256, uint256, uint256) {}
 }

test/governance/extensions/GovernorSuperQuorum.test.js

@@ -113,7 +113,7 @@ describe('GovernorSuperQuorum', function () {
         expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Succeeded);
       });
 
-      it('proposal can still fail with super quorum if against votes are higher', async function () {
+      it('proposal remains active if super quorum is reached but vote fails', async function () {
         await this.helper.connect(this.proposer).propose();
         await this.helper.waitForSnapshot();
 
@@ -124,7 +124,13 @@ describe('GovernorSuperQuorum', function () {
         // Vote for with voter1 (40) (reaching super quorum)
         await this.helper.connect(this.voter1).vote({ support: VoteType.For });
 
-        // Should be defeated since against votes are higher
+        // should be active since super quorum is reached but vote fails
+        expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Active);
+
+        // wait for deadline
+        await this.helper.waitForDeadline(1n);
+
+        // should be defeated since against votes are higher
         expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Defeated);
       });
 

test/utils/introspection/SupportsInterface.behavior.js

@@ -15,7 +15,6 @@ const GOVERNOR_INTERFACE = [
   'proposalProposer(uint256)',
   'proposalEta(uint256)',
   'proposalNeedsQueuing(uint256)',
-  'proposalVotes(uint256)',
   'votingDelay()',
   'votingPeriod()',
   'quorum(uint256)',