refactor: big ole proposal metadata, tests, rewards, and ranking refactor (#254)

* enhancement: add metadatas and refactor proposals

* feat: add target metadata to rewards

* bugfix: set _lowestRanking when only one prop

* test: add Rewards tests

* chore: remove solhint

* refactor: move GovernorMerkleVotes

* refactor: move sorting logic into Governor from Rewards

* chore: update bytecode

* chore: add in SUB_AND_VOTING_MERKLE_ROOT constant

* bugfix: ts-ignore ABI too many signatures issues

* chore: add TooManyMetadatas error, docs, rest of TODOs

Author: Sean McCaffery

package.json

@@ -18,11 +18,11 @@
     "react-app-revamp:test": "yarn workspace @scaffold-eth/react-app-revamp test",
     "dev": "yarn workspace @scaffold-eth/react-app-revamp dev",
     "format": "yarn workspace @scaffold-eth/forge forgeformat",
-    "lint": "yarn workspace @scaffold-eth/forge forgelint",
     "test": "yarn workspace @scaffold-eth/forge forgetest",
-    "deploy": "yarn workspace @scaffold-eth/forge forgebuild",
+    "forgebuild": "yarn workspace @scaffold-eth/forge forgebuild",
+    "deploy": "yarn workspace @scaffold-eth/forge forgebuildandcopy",
     "coverage": "yarn workspace @scaffold-eth/forge forgecoverage",
-    "smartcheck": "yarn format && yarn lint && yarn test && yarn deploy",
+    "smartcheck": "yarn format && yarn test && yarn deploy",
     "postinstall": "husky install"
   },
   "workspaces": [

packages/forge/.solhint.json

@@ -7,14 +7,13 @@
     "copyfiles": "^2.4.1"
   },
   "dependencies": {
-    "copyfiles": "^2.4.1",
-    "solhint": "^3.4.1"
+    "copyfiles": "^2.4.1"
   },
   "scripts": {
     "forgeformat": "forge fmt",
-    "forgelint": "yarn solhint 'src/**/*.sol'",
     "forgetest": "forge test",
     "forgecoverage": "forge coverage",
-    "forgebuild": "forge build && copyfiles -u 1 out/Contest.sol/*.json ../react-app-revamp/contracts/bytecodeAndAbi && copyfiles -u 1 out/RewardsModule.sol/*.json ../react-app-revamp/contracts/bytecodeAndAbi/modules/"
+    "forgebuild": "forge build",
+    "forgebuildandcopy": "forge build && copyfiles -u 1 out/Contest.sol/*.json ../react-app-revamp/contracts/bytecodeAndAbi && copyfiles -u 1 out/RewardsModule.sol/*.json ../react-app-revamp/contracts/bytecodeAndAbi/modules/"
   }
 }

packages/forge/package.json

@@ -6,7 +6,7 @@ import "./governance/extensions/GovernorSettings.sol";
 import "./governance/extensions/GovernorCountingSimple.sol";
 import "./governance/extensions/GovernorModuleRegistry.sol";
 
-contract Contest is Governor, GovernorSettings, GovernorCountingSimple, GovernorModuleRegistry {
+contract Contest is Governor, GovernorSettings, GovernorSorting, GovernorModuleRegistry {
     constructor(
         string memory _name,
         string memory _prompt,

packages/forge/src/Contest.sol

@@ -10,7 +10,7 @@ import "@openzeppelin/utils/math/SafeCast.sol";
 import "@openzeppelin/utils/Address.sol";
 import "@openzeppelin/utils/Context.sol";
 import "./IGovernor.sol";
-import "./extensions/GovernorMerkleVotes.sol";
+import "./GovernorMerkleVotes.sol";
 
 /**
  * @dev Core of the governance system, designed to be extended though various modules.
@@ -29,14 +29,8 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
     bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)");
     uint64 public constant AMOUNT_FOR_SUMBITTER_PROOF = 10000000000000000000;
     mapping(address => uint256) public addressTotalVotes;
-    mapping(address => bool) private _addressTotalVotesVerified;
-    mapping(address => bool) private _addressSubmitterVerified;
-
-    struct ProposalCore {
-        address author;
-        string description;
-        bool exists;
-    }
+    mapping(address => bool) public addressTotalVotesVerified;
+    mapping(address => bool) public addressSubmitterVerified;
 
     uint256[] private _proposalIds;
     mapping(uint256 => uint256) private _deletedProposalIds;
@@ -46,6 +40,9 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
     mapping(uint256 => ProposalCore) private _proposals;
     mapping(address => uint256) private _numSubmissions;
 
+    /// @notice Thrown if there is metadata included in a proposal that isn't covered in data validation
+    error TooManyMetadatas();
+
     /**
      * @dev Restrict access of functions to the governance executor, which may be the Governor itself or a timelock
      * contract, as specified by {_executor}. This generally means that function with this modifier must be voted on and
@@ -116,8 +113,8 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
      * accross multiple networks. This also means that in order to execute the same operation twice (on the same
      * governor) the proposer will have to change the description in order to avoid proposal id conflicts.
      */
-    function hashProposal(string memory proposalDescription) public pure virtual override returns (uint256) {
-        return uint256(keccak256(abi.encode(proposalDescription)));
+    function hashProposal(ProposalCore memory proposal) public pure virtual override returns (uint256) {
+        return uint256(keccak256(abi.encode(proposal)));
     }
 
     /**
@@ -225,56 +222,78 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
      * @dev See {IGovernor-verifySubmitter}.
      */
     function verifySubmitter(address account, bytes32[] calldata proof) public override returns (bool verified) {
-        if (!_addressSubmitterVerified[account]) {
+        if (!addressSubmitterVerified[account]) {
             checkProof(account, AMOUNT_FOR_SUMBITTER_PROOF, proof, false); // will revert with NotInMerkle if not valid
-            _addressSubmitterVerified[account] = true;
+            addressSubmitterVerified[account] = true;
         }
         return true;
     }
 
     /**
-     * @dev See {IGovernor-verifyTotalVotes}.
+     * @dev See {IGovernor-validateProposalData}.
      */
-    function verifyTotalVotes(address account, uint256 totalVotes, bytes32[] calldata proof)
-        public
-        override
-        returns (bool verified)
-    {
-        if (!_addressTotalVotesVerified[account]) {
-            checkProof(account, totalVotes, proof, false); // will revert with NotInMerkle if not valid
-            addressTotalVotes[account] = totalVotes;
-            _addressTotalVotesVerified[account] = true;
+    function validateProposalData(ProposalCore memory proposal) public virtual override returns (bool dataValidated) {
+        for (uint256 index = 0; index < METADATAS_COUNT; index++) {
+            Metadatas currentMetadata = Metadatas(index);
+            if (currentMetadata == Metadatas.Target) {
+                continue; // Nothing to check here since strictly typed to address
+            } else if (currentMetadata == Metadatas.Safe) {
+                require(
+                    proposal.safeMetadata.signers.length != 0,
+                    "GovernorMetadataValidation: there cannot be zero signers in safeMetadata"
+                );
+                require(
+                    proposal.safeMetadata.threshold != 0,
+                    "GovernorMetadataValidation: threshold cannot be zero in safeMetadata"
+                );
+                require(proposal.safeMetadata.signers.length != 0);
+            } else {
+                revert TooManyMetadatas();
+            }
         }
+        require(bytes(proposal.description).length != 0, "Governor: empty proposal");
         return true;
     }
 
     /**
      * @dev See {IGovernor-propose}.
      */
-    function propose(string memory proposalDescription, bytes32[] calldata proof)
+    function propose(ProposalCore memory proposal, bytes32[] calldata proof)
         public
         virtual
         override
         returns (uint256)
     {
+        require(verifySubmitter(msg.sender, proof), "Governor: address is not permissioned to submit");
+        validateProposalData(proposal);
+        return _castProposal(proposal);
+    }
+
+    /**
+     * @dev See {IGovernor-proposeWithoutProof}.
+     */
+    function proposeWithoutProof(ProposalCore memory proposal) public virtual override returns (uint256) {
+        require(addressSubmitterVerified[msg.sender], "Governor: address is not permissioned to submit");
+        validateProposalData(proposal);
+        return _castProposal(proposal);
+    }
+
+    function _castProposal(ProposalCore memory proposal) internal virtual returns (uint256) {
         require(state() == ContestState.Queued, "Governor: contest must be queued for proposals to be submitted");
         require(
             _numSubmissions[msg.sender] < numAllowedProposalSubmissions(),
             "Governor: the same cannot submit more than the numAllowedProposalSubmissions for this contest"
         );
         require(_proposalIds.length < maxProposalCount(), "Governor: the max number of proposals have been submitted");
-        require(verifySubmitter(msg.sender, proof), "Governor: address is not permissioned to submit");
-
-        require(bytes(proposalDescription).length != 0, "Governor: empty proposal");
 
-        uint256 proposalId = hashProposal(proposalDescription);
+        uint256 proposalId = hashProposal(proposal);
         require(!_proposals[proposalId].exists, "Governor: duplicate proposals not allowed");
 
         _proposalIds.push(proposalId);
-        _proposals[proposalId] = ProposalCore({author: msg.sender, description: proposalDescription, exists: true});
+        _proposals[proposalId] = proposal;
         _numSubmissions[msg.sender] += 1;
 
-        emit ProposalCreated(proposalId, proposalDescription, _msgSender());
+        emit ProposalCreated(proposalId, _msgSender());
 
         return proposalId;
     }
@@ -318,6 +337,22 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
         emit ContestCanceled();
     }
 
+    /**
+     * @dev See {IGovernor-verifyTotalVotes}.
+     */
+    function verifyTotalVotes(address account, uint256 totalVotes, bytes32[] calldata proof)
+        public
+        override
+        returns (bool verified)
+    {
+        if (!addressTotalVotesVerified[account]) {
+            checkProof(account, totalVotes, proof, false); // will revert with NotInMerkle if not valid
+            addressTotalVotes[account] = totalVotes;
+            addressTotalVotesVerified[account] = true;
+        }
+        return true;
+    }
+
     /**
      * @dev See {IGovernor-castVote}.
      */
@@ -343,7 +378,7 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
     {
         address voter = _msgSender();
         require(
-            _addressTotalVotesVerified[voter],
+            addressTotalVotesVerified[voter],
             "Governor: you need to cast a vote with the proof at least once and you haven't yet"
         );
         return _castVote(proposalId, voter, support, numVotes, "");
@@ -364,7 +399,7 @@ abstract contract Governor is Context, ERC165, EIP712, GovernorMerkleVotes, IGov
         require(numVotes > 0, "Governor: cannot vote with 0 or fewer votes");
 
         require(
-            _addressTotalVotesVerified[account],
+            addressTotalVotesVerified[account],
             "Governor: you need to verify your number of votes against the merkle root first"
         );
         _countVote(proposalId, account, support, numVotes, addressTotalVotes[account]);

packages/forge/src/governance/Governor.sol

@@ -19,10 +19,34 @@ abstract contract IGovernor is IERC165 {
         Completed
     }
 
+    uint8 public constant METADATAS_COUNT = 2;
+
+    enum Metadatas {
+        Target,
+        Safe
+    }
+
+    struct TargetMetadata {
+        address targetAddress;
+    }
+
+    struct SafeMetadata {
+        address[] signers;
+        uint256 threshold;
+    }
+
+    struct ProposalCore {
+        address author;
+        string description;
+        bool exists;
+        TargetMetadata targetMetadata;
+        SafeMetadata safeMetadata;
+    }
+
     /**
      * @dev Emitted when a proposal is created.
      */
-    event ProposalCreated(uint256 proposalId, string description, address proposer);
+    event ProposalCreated(uint256 proposalId, address proposer);
 
     /**
      * @dev Emitted when proposals are deleted.
@@ -83,7 +107,7 @@ abstract contract IGovernor is IERC165 {
      * @notice module:core
      * @dev Hashing function used to (re)build the proposal id from the proposal details..
      */
-    function hashProposal(string memory proposalDescription) public pure virtual returns (uint256);
+    function hashProposal(ProposalCore memory proposal) public pure virtual returns (uint256);
 
     /**
      * @notice module:core
@@ -140,23 +164,36 @@ abstract contract IGovernor is IERC165 {
     function verifySubmitter(address account, bytes32[] calldata proof) public virtual returns (bool);
 
     /**
-     * @dev Verifies that `account` is permissioned to vote with `totalVotes` via merkle proof.
+     * @dev Verifies that all of the metadata in the proposal is valid.
      */
-    function verifyTotalVotes(address account, uint256 totalVotes, bytes32[] calldata proof)
+    function validateProposalData(ProposalCore memory proposal) public virtual returns (bool);
+
+    /**
+     * @dev Create a new proposal. Vote start {IGovernor-votingDelay} blocks after the proposal is created and ends
+     * {IGovernor-votingPeriod} blocks after the voting starts.
+     *
+     * Emits a {ProposalCreated} event.
+     */
+    function propose(ProposalCore memory proposal, bytes32[] calldata proof)
         public
         virtual
-        returns (bool);
+        returns (uint256 proposalId);
 
     /**
      * @dev Create a new proposal. Vote start {IGovernor-votingDelay} blocks after the proposal is created and ends
      * {IGovernor-votingPeriod} blocks after the voting starts.
      *
      * Emits a {ProposalCreated} event.
      */
-    function propose(string memory proposalDescription, bytes32[] calldata proof)
+    function proposeWithoutProof(ProposalCore memory proposal) public virtual returns (uint256 proposalId);
+
+    /**
+     * @dev Verifies that `account` is permissioned to vote with `totalVotes` via merkle proof.
+     */
+    function verifyTotalVotes(address account, uint256 totalVotes, bytes32[] calldata proof)
         public
         virtual
-        returns (uint256 proposalId);
+        returns (bool);
 
     /**
      * @dev Cast a vote with a merkle proof.