Merge pull request #766 from MercuricChloride/dev

feat: changed dispute manager to not delete bad actors disputes on re…

Author: pcarranzav

contracts/disputes/DisputeManager.sol

@@ -154,6 +154,15 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
         _;
     }
 
+    modifier onlyPendingDispute(bytes32 _disputeID) {
+        require(isDisputeCreated(_disputeID), "Dispute does not exist");
+        require(
+            disputes[_disputeID].status == IDisputeManager.DisputeStatus.Pending,
+            "Dispute must be pending"
+        );
+        _;
+    }
+
     // -- Functions --
 
     /**
@@ -290,7 +299,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
      * @param _disputeID True if dispute already exists
      */
     function isDisputeCreated(bytes32 _disputeID) public view override returns (bool) {
-        return disputes[_disputeID].fisherman != address(0);
+        return disputes[_disputeID].status != DisputeStatus.Null;
     }
 
     /**
@@ -479,7 +488,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
             _fisherman,
             _deposit,
             0, // no related dispute,
-            DisputeType.QueryDispute
+            DisputeType.QueryDispute,
+            IDisputeManager.DisputeStatus.Pending
         );
 
         emit QueryDisputeCreated(
@@ -546,7 +556,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
             _fisherman,
             _deposit,
             0,
-            DisputeType.IndexingDispute
+            DisputeType.IndexingDispute,
+            IDisputeManager.DisputeStatus.Pending
         );
 
         emit IndexingDisputeCreated(disputeID, alloc.indexer, _fisherman, _deposit, _allocationID);
@@ -562,8 +573,16 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
      * @notice Accept a dispute with ID `_disputeID`
      * @param _disputeID ID of the dispute to be accepted
      */
-    function acceptDispute(bytes32 _disputeID) external override onlyArbitrator {
-        Dispute memory dispute = _resolveDispute(_disputeID);
+    function acceptDispute(bytes32 _disputeID)
+        external
+        override
+        onlyArbitrator
+        onlyPendingDispute(_disputeID)
+    {
+        Dispute storage dispute = disputes[_disputeID];
+
+        // store the dispute status
+        dispute.status = IDisputeManager.DisputeStatus.Accepted;
 
         // Slash
         (, uint256 tokensToReward) = _slashIndexer(
@@ -575,8 +594,9 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
         // Give the fisherman their deposit back
         TokenUtils.pushTokens(graphToken(), dispute.fisherman, dispute.deposit);
 
-        // Resolve the conflicting dispute if any
-        _resolveDisputeInConflict(dispute);
+        if (_isDisputeInConflict(dispute)) {
+            rejectDispute(dispute.relatedDisputeID);
+        }
 
         emit DisputeAccepted(
             _disputeID,
@@ -591,8 +611,16 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
      * @notice Reject a dispute with ID `_disputeID`
      * @param _disputeID ID of the dispute to be rejected
      */
-    function rejectDispute(bytes32 _disputeID) external override onlyArbitrator {
-        Dispute memory dispute = _resolveDispute(_disputeID);
+    function rejectDispute(bytes32 _disputeID)
+        public
+        override
+        onlyArbitrator
+        onlyPendingDispute(_disputeID)
+    {
+        Dispute storage dispute = disputes[_disputeID];
+
+        // store dispute status
+        dispute.status = IDisputeManager.DisputeStatus.Rejected;
 
         // Handle conflicting dispute if any
         require(
@@ -611,52 +639,48 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa
      * @notice Ignore a dispute with ID `_disputeID`
      * @param _disputeID ID of the dispute to be disregarded
      */
-    function drawDispute(bytes32 _disputeID) external override onlyArbitrator {
-        Dispute memory dispute = _resolveDispute(_disputeID);
+    function drawDispute(bytes32 _disputeID)
+        public
+        override
+        onlyArbitrator
+        onlyPendingDispute(_disputeID)
+    {
+        Dispute storage dispute = disputes[_disputeID];
 
         // Return deposit to the fisherman
         TokenUtils.pushTokens(graphToken(), dispute.fisherman, dispute.deposit);
 
-        // Resolve the conflicting dispute if any
-        _resolveDisputeInConflict(dispute);
-
-        emit DisputeDrawn(_disputeID, dispute.indexer, dispute.fisherman, dispute.deposit);
-    }
-
-    /**
-     * @dev Resolve a dispute by removing it from storage and returning a memory copy.
-     * @param _disputeID ID of the dispute to resolve
-     * @return Dispute
-     */
-    function _resolveDispute(bytes32 _disputeID) private returns (Dispute memory) {
-        require(isDisputeCreated(_disputeID), "Dispute does not exist");
+        // resolve related dispute if any
+        _drawDisputeInConflict(dispute);
 
-        Dispute memory dispute = disputes[_disputeID];
+        // store dispute status
+        dispute.status = IDisputeManager.DisputeStatus.Drawn;
 
-        // Resolve dispute
-        delete disputes[_disputeID]; // Re-entrancy
-
-        return dispute;
+        emit DisputeDrawn(_disputeID, dispute.indexer, dispute.fisherman, dispute.deposit);
     }
 
     /**
      * @dev Returns whether the dispute is for a conflicting attestation or not.
      * @param _dispute Dispute
      * @return True conflicting attestation dispute
      */
-    function _isDisputeInConflict(Dispute memory _dispute) private pure returns (bool) {
-        return _dispute.relatedDisputeID != 0;
+    function _isDisputeInConflict(Dispute memory _dispute) private view returns (bool) {
+        bytes32 relatedID = _dispute.relatedDisputeID;
+        // this is so the check returns false when rejecting the related dispute.
+        return
+            relatedID != 0 && disputes[relatedID].status == IDisputeManager.DisputeStatus.Pending;
     }
 
     /**
      * @dev Resolve the conflicting dispute if there is any for the one passed to this function.
      * @param _dispute Dispute
      * @return True if resolved
      */
-    function _resolveDisputeInConflict(Dispute memory _dispute) private returns (bool) {
+    function _drawDisputeInConflict(Dispute memory _dispute) private returns (bool) {
         if (_isDisputeInConflict(_dispute)) {
             bytes32 relatedDisputeID = _dispute.relatedDisputeID;
-            delete disputes[relatedDisputeID];
+            Dispute storage relatedDispute = disputes[relatedDisputeID];
+            relatedDispute.status = IDisputeManager.DisputeStatus.Drawn;
             return true;
         }
         return false;

contracts/disputes/IDisputeManager.sol

@@ -12,13 +12,22 @@ interface IDisputeManager {
         QueryDispute
     }
 
+    enum DisputeStatus {
+        Null,
+        Accepted,
+        Rejected,
+        Drawn,
+        Pending
+    }
+
     // Disputes contain info necessary for the Arbitrator to verify and resolve
     struct Dispute {
         address indexer;
         address fisherman;
         uint256 deposit;
         bytes32 relatedDisputeID;
         DisputeType disputeType;
+        DisputeStatus status;
     }
 
     // -- Attestation --

test/disputes/query.test.ts

@@ -21,6 +21,7 @@ import {
 } from '../lib/testHelpers'
 
 import { Dispute, createQueryDisputeID, encodeAttestation, MAX_PPM } from './common'
+import { isExportDeclaration } from 'typescript'
 
 const { AddressZero, HashZero } = constants
 
@@ -528,8 +529,10 @@ describe('DisputeManager:Query', async () => {
       // Do
       await disputeManager.connect(arbitrator.signer).acceptDispute(dID1)
       // Check
+      const mainDispute = await disputeManager.disputes(dID1)
+      expect(mainDispute.status).to.eq(1) // 1 = DisputeStatus.Accepted
       const relatedDispute = await disputeManager.disputes(dID2)
-      expect(relatedDispute.indexer).eq(AddressZero)
+      expect(relatedDispute.status).to.eq(2) // 2 = DisputeStatus.Rejected
     })
 
     it('should not allow to reject, user need to accept the related dispute ID to reject it', async function () {
@@ -549,7 +552,7 @@ describe('DisputeManager:Query', async () => {
       await disputeManager.connect(arbitrator.signer).drawDispute(dID1)
       // Check
       const relatedDispute = await disputeManager.disputes(dID2)
-      expect(relatedDispute.indexer).eq(AddressZero)
+      expect(relatedDispute.status).not.eq(4) // 4 = DisputeStatus.Pending
     })
   })
 })