Always cancel failed L2ForkArbitrator requests (#206)

RealityETH · Feb 1, 2024 · b3cb78a · b3cb78a
1 parent 1077059
commit b3cb78a
Show file tree

Hide file tree

Showing 4 changed files with 320 additions and 41 deletions.
diff --git a/contracts/L2ForkArbitrator.sol b/contracts/L2ForkArbitrator.sol
@@ -99,12 +99,16 @@ contract L2ForkArbitrator is IL2ForkArbitrator {
             revert ArbitrationAlreadyRequested();
         }
 
+        uint256 timeOfFirstRequest = arbitrationRequests[questionId]
+            .timeOfRequest == 0
+            ? block.timestamp
+            : arbitrationRequests[questionId].timeOfRequest;
         arbitrationRequests[questionId] = ArbitrationRequest(
             RequestStatus.QUEUED,
             payable(msg.sender),
             msg.value,
             bytes32(0),
-            block.timestamp
+            timeOfFirstRequest
         );
 
         realitio.notifyOfArbitrationRequest(
@@ -113,7 +117,6 @@ contract L2ForkArbitrator is IL2ForkArbitrator {
             maxPrevious
         );
         emit LogRequestArbitration(questionId, msg.value, msg.sender, 0);
-
         if (
             !isForkInProgress &&
             arbitrationData[questionId].delay == 0 &&
@@ -255,6 +258,13 @@ contract L2ForkArbitrator is IL2ForkArbitrator {
         arbitrationRequests[question_id].status = RequestStatus
             .FORK_REQUEST_FAILED;
 
+        realitio.cancelArbitration(question_id);
+        address payable payer = arbitrationRequests[question_id].payer;
+
+        refundsDue[payer] =
+            refundsDue[payer] +
+            arbitrationRequests[question_id].paid;
+        deleteArbitrationRequestsData(question_id);
         // We don't check the funds are back here, just assume L1GlobalForkRequester send them and they can be recovered.
     }
 
@@ -296,29 +306,13 @@ contract L2ForkArbitrator is IL2ForkArbitrator {
         delete (arbitrationRequests[question_id]);
     }
 
-    /// @inheritdoc IL2ForkArbitrator
-    function cancelArbitration(bytes32 question_id) external {
-        // For simplicity we won't let you cancel until forking is sorted, as you might retry and keep failing for the same reason
-        if (isForkInProgress) {
-            revert ForkInProgress();
-        }
-
-        if (msg.sender != arbitrationRequests[question_id].payer) {
-            revert WrongSender();
-        }
-
-        RequestStatus status = arbitrationRequests[question_id].status;
-        if (status != RequestStatus.FORK_REQUEST_FAILED) {
-            revert StatusNotForkRequestFailed();
-        }
-
-        address payable payer = arbitrationRequests[question_id].payer;
-        realitio.cancelArbitration(question_id);
-
-        refundsDue[payer] =
-            refundsDue[payer] +
-            arbitrationRequests[question_id].paid;
-        delete (arbitrationRequests[question_id]);
+    function deleteArbitrationRequestsData(bytes32 question_id) internal {
+        arbitrationRequests[question_id].status = RequestStatus.NONE;
+        // the following data does not need to be deleted, and with the new removal of restore opcode, we could leave them as they are.
+        arbitrationRequests[question_id].payer = payable(address(0));
+        arbitrationRequests[question_id].paid = 0;
+        arbitrationRequests[question_id].result = bytes32(0);
+        // we don't delete the timeOfRequest on purpose because it will be relevant for the next request
     }
 
     function claimRefund() external {

diff --git a/contracts/interfaces/IL2ForkArbitrator.sol b/contracts/interfaces/IL2ForkArbitrator.sol
@@ -106,13 +106,6 @@ interface IL2ForkArbitrator is IBridgeMessageReceiver {
         address last_answerer
     ) external;
 
-    /// @notice Cancel a previous arbitration request
-    /// @dev This is intended for situations where the stuff is happening non-atomically and the fee changes or someone else forks before us
-    /// @dev Another way to handle this might be to go back into QUEUED state and let people keep retrying
-    /// @dev NB This may revert if the contract has returned funds in the bridge but claimAsset hasn't been called yet
-    /// @param question_id The question in question
-    function cancelArbitration(bytes32 question_id) external;
-
     /// @notice Claim the refund for a question that was forked
     function claimRefund() external;
 }
diff --git a/test/AdjudicationFramework/AdjudicationFrameworkRequests.t.sol b/test/AdjudicationFramework/AdjudicationFrameworkRequests.t.sol
@@ -17,7 +17,6 @@ import {L2ChainInfo} from "../../contracts/L2ChainInfo.sol";
 import {MockPolygonZkEVMBridge} from "../testcontract/MockPolygonZkEVMBridge.sol";
 import {MinimalAdjudicationFramework} from "../../contracts/AdjudicationFramework/MinimalAdjudicationFramework.sol";
 import {AdjudicationFrameworkRequests} from "../../contracts/AdjudicationFramework/Pull/AdjudicationFrameworkRequests.sol";
-import {IL2ForkArbitrator} from "../../contracts/interfaces/IL2ForkArbitrator.sol";
 
 contract AdjudicationIntegrationTest is Test {
     Arbitrator public govArb;
@@ -702,14 +701,6 @@ contract AdjudicationIntegrationTest is Test {
             "Not in forking state"
         );
 
-        vm.expectRevert(IL2ForkArbitrator.WrongSender.selector);
-        l2ForkArbitrator.requestActivateFork(removalQuestionId);
-
-        vm.expectRevert(IL2ForkArbitrator.WrongSender.selector);
-        l2ForkArbitrator.cancelArbitration(removalQuestionId);
-
-        vm.prank(user2);
-        l2ForkArbitrator.cancelArbitration(removalQuestionId);
         assertEq(forkFee, l2ForkArbitrator.refundsDue(user2));
 
         uint256 user2Bal = user2.balance;