Switch to custom errors (#76)

Current best practice is to throw custom error types rather than strings on revert cases, and to append the contract name to the front of custom error types with `__` in between. This PR updates all revert cases in the FlexibleVotingClient.sol to use this convention. Since you cannot throw custom error types in require statements except in the latest version of solidity, this PR also converts from require statements to conditionals.

.github/workflows/ci.yml

@@ -106,7 +106,7 @@ jobs:
         uses: zgosalvez/github-actions-report-lcov@v2
         with:
           coverage-files: ./lcov.info
-          minimum-coverage: 92 # Set coverage threshold.
+          minimum-coverage: 91 # Set coverage threshold.
 
   slither-analyze:
     runs-on: ubuntu-latest

src/FlexVotingClient.sol

@@ -88,6 +88,11 @@ abstract contract FlexVotingClient {
   /// given time.
   Checkpoints.Trace208 internal totalBalanceCheckpoints;
 
+  error FlexVotingClient__NoVotingWeight();
+  error FlexVotingClient__AlreadyVoted();
+  error FlexVotingClient__InvalidSupportValue();
+  error FlexVotingClient__NoVotesExpressed();
+
   /// @param _governor The address of the flex-voting-compatible governance contract.
   constructor(address _governor) {
     GOVERNOR = IFractionalGovernor(_governor);
@@ -116,9 +121,9 @@ abstract contract FlexVotingClient {
   /// @param support The depositor's vote preferences in accordance with the `VoteType` enum.
   function expressVote(uint256 proposalId, uint8 support) external {
     uint256 weight = getPastRawBalance(msg.sender, GOVERNOR.proposalSnapshot(proposalId));
-    require(weight > 0, "no weight");
+    if (weight == 0) revert FlexVotingClient__NoVotingWeight();
 
-    require(!proposalVotersHasVoted[proposalId][msg.sender], "already voted");
+    if (proposalVotersHasVoted[proposalId][msg.sender]) revert FlexVotingClient__AlreadyVoted();
     proposalVotersHasVoted[proposalId][msg.sender] = true;
 
     if (support == uint8(VoteType.Against)) {
@@ -128,7 +133,8 @@ abstract contract FlexVotingClient {
     } else if (support == uint8(VoteType.Abstain)) {
       proposalVotes[proposalId].abstainVotes += SafeCast.toUint128(weight);
     } else {
-      revert("invalid support value, must be included in VoteType enum");
+      // Support value must be included in VoteType enum.
+      revert FlexVotingClient__InvalidSupportValue();
     }
   }
 
@@ -139,10 +145,10 @@ abstract contract FlexVotingClient {
   /// @param proposalId The ID of the proposal which the Pool will now vote on.
   function castVote(uint256 proposalId) external {
     ProposalVote storage _proposalVote = proposalVotes[proposalId];
-    require(
-      _proposalVote.forVotes + _proposalVote.againstVotes + _proposalVote.abstainVotes > 0,
-      "no votes expressed"
-    );
+    if (_proposalVote.forVotes + _proposalVote.againstVotes + _proposalVote.abstainVotes == 0) {
+      revert FlexVotingClient__NoVotesExpressed();
+    }
+
     uint256 _proposalSnapshotBlockNumber = GOVERNOR.proposalSnapshot(proposalId);
 
     // We use the snapshot of total raw balances to determine the weight with

test/FlexVotingClient.t.sol

@@ -9,6 +9,7 @@ import {IGovernor} from "@openzeppelin/contracts/governance/Governor.sol";
 import {IVotingToken} from "src/interfaces/IVotingToken.sol";
 import {IFractionalGovernor} from "src/interfaces/IFractionalGovernor.sol";
 import {GovernorCountingFractional as GCF} from "src/GovernorCountingFractional.sol";
+import {FlexVotingClient as FVC} from "src/FlexVotingClient.sol";
 import {MockFlexVotingClient} from "test/MockFlexVotingClient.sol";
 import {GovToken} from "test/GovToken.sol";
 import {FractionalGovernor} from "test/FractionalGovernor.sol";
@@ -502,7 +503,7 @@ contract ExpressVote is FlexVotingClientTest {
 
     // Now try to express a voting preference on the proposal.
     assertEq(flexClient.deposits(_user), _voteWeight);
-    vm.expectRevert(bytes("no weight"));
+    vm.expectRevert(FVC.FlexVotingClient__NoVotingWeight.selector);
     vm.prank(_user);
     flexClient.expressVote(_proposalId, uint8(_voteType));
   }
@@ -524,7 +525,7 @@ contract ExpressVote is FlexVotingClientTest {
     uint256 _proposalId = _createAndSubmitProposal();
 
     // _user should NOT be able to express his/her vote on the proposal.
-    vm.expectRevert(bytes("no weight"));
+    vm.expectRevert(FVC.FlexVotingClient__NoVotingWeight.selector);
     vm.prank(_user);
     flexClient.expressVote(_proposalId, uint8(_voteType));
 
@@ -536,7 +537,7 @@ contract ExpressVote is FlexVotingClientTest {
     // _user should still NOT be able to express his/her vote on the proposal.
     // Despite having a deposit balance, he/she didn't have a balance at the
     // proposal snapshot.
-    vm.expectRevert(bytes("no weight"));
+    vm.expectRevert(FVC.FlexVotingClient__NoVotingWeight.selector);
     vm.prank(_user);
     flexClient.expressVote(_proposalId, uint8(_voteType));
   }
@@ -567,7 +568,7 @@ contract ExpressVote is FlexVotingClientTest {
     assertEq(_abstainVotesExpressedInit, _voteType == GCF.VoteType.Abstain ? _voteWeight : 0);
 
     // Vote early and often!
-    vm.expectRevert(bytes("already voted"));
+    vm.expectRevert(FVC.FlexVotingClient__AlreadyVoted.selector);
     vm.prank(_user);
     flexClient.expressVote(_proposalId, uint8(_voteType));
 
@@ -595,7 +596,7 @@ contract ExpressVote is FlexVotingClientTest {
     uint256 _proposalId = _createAndSubmitProposal();
 
     // Now try to express a voting preference with a bogus support type.
-    vm.expectRevert(bytes("invalid support value, must be included in VoteType enum"));
+    vm.expectRevert(FVC.FlexVotingClient__InvalidSupportValue.selector);
     vm.prank(_user);
     flexClient.expressVote(_proposalId, _supportType);
   }
@@ -626,7 +627,7 @@ contract ExpressVote is FlexVotingClientTest {
     assert(_proposalId != _id);
 
     // Now try to express a voting preference on the bogus proposal.
-    vm.expectRevert("no weight");
+    vm.expectRevert(FVC.FlexVotingClient__NoVotingWeight.selector);
     vm.prank(_user);
     flexClient.expressVote(_proposalId, _supportType);
   }
@@ -1077,7 +1078,7 @@ contract CastVote is FlexVotingClientTest {
     uint256 _proposalId = _createAndSubmitProposal();
 
     // No one has expressed, there are no votes to cast.
-    vm.expectRevert(bytes("no votes expressed"));
+    vm.expectRevert(FVC.FlexVotingClient__NoVotesExpressed.selector);
     flexClient.castVote(_proposalId);
 
     // _user expresses his/her vote on the proposal.
@@ -1088,7 +1089,7 @@ contract CastVote is FlexVotingClientTest {
     flexClient.castVote(_proposalId);
 
     // All votes have been cast, there's nothing new to send to the governor.
-    vm.expectRevert(bytes("no votes expressed"));
+    vm.expectRevert(FVC.FlexVotingClient__NoVotesExpressed.selector);
     flexClient.castVote(_proposalId);
   }