perf: avoid redundant validations in functions

defi-wonderland · Jul 19, 2024 · 5f7ab67 · 5f7ab67
1 parent d542d41
commit 5f7ab67
Show file tree

Hide file tree

Showing 8 changed files with 24 additions and 87 deletions.
diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol
@@ -33,27 +33,24 @@ contract BondEscalationModule is Module, IBondEscalationModule {
     IOracle.Response calldata _response,
     IOracle.Dispute calldata _dispute
   ) external onlyOracle {
-    bytes32 _requestId = _getId(_request);
-    if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId();
-
     RequestParameters memory _params = decodeRequestData(_request.disputeModuleData);
 
     if (block.number > ORACLE.createdAt(_dispute.responseId) + _params.disputeWindow) {
       revert BondEscalationModule_DisputeWindowOver();
     }
 
-    BondEscalation storage _escalation = _escalations[_requestId];
+    BondEscalation storage _escalation = _escalations[_dispute.requestId];
     bytes32 _disputeId = _getId(_dispute);
 
     _params.accountingExtension.bond({
       _bonder: _dispute.disputer,
-      _requestId: _requestId,
+      _requestId: _dispute.requestId,
       _token: _params.bondToken,
       _amount: _params.bondSize
     });
 
     emit ResponseDisputed({
-      _requestId: _requestId,
+      _requestId: _dispute.requestId,
       _responseId: _dispute.responseId,
       _disputeId: _disputeId,
       _dispute: _dispute,
@@ -66,7 +63,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
       if (block.timestamp > _params.bondEscalationDeadline) revert BondEscalationModule_BondEscalationOver();
       _escalation.status = BondEscalationStatus.Active;
       _escalation.disputeId = _disputeId;
-      emit BondEscalationStatusUpdated(_requestId, _disputeId, BondEscalationStatus.Active);
+      emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, BondEscalationStatus.Active);
     } else if (_disputeId != _escalation.disputeId) {
       ORACLE.escalateDispute(_request, _response, _dispute);
     }
@@ -79,11 +76,8 @@ contract BondEscalationModule is Module, IBondEscalationModule {
     IOracle.Response calldata,
     IOracle.Dispute calldata _dispute
   ) external onlyOracle {
-    bytes32 _requestId = _getId(_request);
-    if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId();
-
     RequestParameters memory _params = decodeRequestData(_request.disputeModuleData);
-    BondEscalation storage _escalation = _escalations[_requestId];
+    BondEscalation storage _escalation = _escalations[_dispute.requestId];
     IOracle.DisputeStatus _disputeStatus = ORACLE.disputeStatus(_disputeId);
     BondEscalationStatus _newStatus;
 
@@ -109,7 +103,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
 
         if (_escalation.amountOfPledgesForDispute + _escalation.amountOfPledgesAgainstDispute > 0) {
           _params.accountingExtension.onSettleBondEscalation({
-            _requestId: _requestId,
+            _requestId: _dispute.requestId,
             _disputeId: _disputeId,
             _token: _params.bondToken,
             _amountPerPledger: _params.bondSize,
@@ -118,7 +112,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
         }
 
         _params.accountingExtension.release({
-          _requestId: _requestId,
+          _requestId: _dispute.requestId,
           _bonder: _dispute.disputer,
           _token: _params.bondToken,
           _amount: _params.bondSize
@@ -140,7 +134,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
               + FixedPointMathLib.mulDivDown(_pledgesForDispute, _params.bondSize, _pledgesAgainstDispute);
 
           _params.accountingExtension.onSettleBondEscalation({
-            _requestId: _requestId,
+            _requestId: _dispute.requestId,
             _disputeId: _escalation.disputeId,
             _token: _params.bondToken,
             _amountPerPledger: _amountToPay,
@@ -149,7 +143,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
         }
 
         _params.accountingExtension.pay({
-          _requestId: _requestId,
+          _requestId: _dispute.requestId,
           _payer: _won ? _dispute.proposer : _dispute.disputer,
           _receiver: _won ? _dispute.disputer : _dispute.proposer,
           _token: _params.bondToken,
@@ -158,7 +152,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
 
         if (_won) {
           _params.accountingExtension.release({
-            _requestId: _requestId,
+            _requestId: _dispute.requestId,
             _bonder: _dispute.disputer,
             _token: _params.bondToken,
             _amount: _params.bondSize
@@ -176,7 +170,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
         // Refund the disputer, the bond escalation status stays Escalated
         _newStatus = BondEscalationStatus.Escalated;
         _params.accountingExtension.release({
-          _requestId: _requestId,
+          _requestId: _dispute.requestId,
           _bonder: _dispute.disputer,
           _token: _params.bondToken,
           _amount: _params.bondSize
@@ -188,7 +182,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
         _newStatus = _won ? BondEscalationStatus.DisputerWon : BondEscalationStatus.DisputerLost;
 
         _params.accountingExtension.pay({
-          _requestId: _requestId,
+          _requestId: _dispute.requestId,
           _payer: _won ? _dispute.proposer : _dispute.disputer,
           _receiver: _won ? _dispute.disputer : _dispute.proposer,
           _token: _params.bondToken,
@@ -197,7 +191,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
 
         if (_won) {
           _params.accountingExtension.release({
-            _requestId: _requestId,
+            _requestId: _dispute.requestId,
             _bonder: _dispute.disputer,
             _token: _params.bondToken,
             _amount: _params.bondSize
@@ -207,7 +201,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
     }
 
     _escalation.status = _newStatus;
-    emit BondEscalationStatusUpdated(_requestId, _disputeId, _newStatus);
+    emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, _newStatus);
     emit DisputeStatusChanged({_disputeId: _disputeId, _dispute: _dispute, _status: _disputeStatus});
   }
 

diff --git a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol
@@ -83,9 +83,6 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu
     IOracle.Response calldata _response,
     IOracle.Dispute calldata _dispute
   ) external onlyOracle {
-    bytes32 _requestId = _getId(_request);
-    if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId();
-
     Escalation storage _escalation = escalations[_disputeId];
 
     if (_escalation.resolution != Resolution.Unresolved) revert BondEscalationResolutionModule_AlreadyResolved();
@@ -120,7 +117,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu
     }
 
     ORACLE.updateDisputeStatus(_request, _response, _dispute, _disputeStatus);
-    emit DisputeResolved(_requestId, _disputeId, _disputeStatus);
+    emit DisputeResolved(_dispute.requestId, _disputeId, _disputeStatus);
   }
 
   /// @inheritdoc IBondEscalationResolutionModule

diff --git a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol
@@ -86,9 +86,6 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule {
     IOracle.Response calldata _response,
     IOracle.Dispute calldata _dispute
   ) external onlyOracle {
-    bytes32 _requestId = _getId(_request);
-    if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId();
-
     // Check disputeId actually exists and that it isn't resolved already
     if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) {
       revert ERC20ResolutionModule_AlreadyResolved();
@@ -108,10 +105,10 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule {
     // Update status
     if (_quorumReached == 1) {
       ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won);
-      emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Won);
+      emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won);
     } else {
       ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Lost);
-      emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Lost);
+      emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost);
     }
   }
 

diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol
@@ -119,9 +119,6 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
     IOracle.Response calldata _response,
     IOracle.Dispute calldata _dispute
   ) external onlyOracle {
-    bytes32 _requestId = _getId(_request);
-    if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId();
-
     if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute();
     if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) {
       revert PrivateERC20ResolutionModule_AlreadyResolved();
@@ -143,10 +140,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
 
     if (_quorumReached == 1) {
       ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won);
-      emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Won);
+      emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won);
     } else {
       ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Lost);
-      emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Lost);
+      emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost);
     }
 
     address _voter;

diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol
@@ -409,16 +409,6 @@ contract BondEscalationModule_Unit_DisputeResponse is BaseTest {
     bondEscalationModule.disputeResponse(_request, mockResponse, mockDispute);
   }
 
-  /**
-   * @notice Tests that disputeResponse reverts if request ids do not match.
-   */
-  function test_revertIfInvalidRequestId() public {
-    // Check: does it revert if request ids do not match?
-    vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector);
-    vm.prank(address(oracle));
-    bondEscalationModule.disputeResponse(mockRequest, mockResponse, mockDispute);
-  }
-
   /**
    * @notice Tests that disputeResponse reverts if the challenge period for the response is over.
    */
@@ -613,16 +603,6 @@ contract BondEscalationModule_Unit_OnDisputeStatusChange is BaseTest {
     bondEscalationModule.onDisputeStatusChange(_disputeId, _request, mockResponse, mockDispute);
   }
 
-  /**
-   * @notice Tests that onDisputeStatusChange reverts if request ids do not match
-   */
-  function test_revertIfInvalidRequestId(bytes32 _disputeId) public {
-    // Check: does it revert if request ids do not match?
-    vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector);
-    vm.prank(address(oracle));
-    bondEscalationModule.onDisputeStatusChange(_disputeId, mockRequest, mockResponse, mockDispute);
-  }
-
   /**
    * @notice Tests that onDisputeStatusChange pays the proposer if the disputer lost
    */

diff --git a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol
@@ -820,20 +820,12 @@ contract BondEscalationResolutionModule_Unit_ResolveDispute is BaseTest {
        the disputer.
   */
 
-  function test_reverts(
-    bytes32 _disputeId,
-    IBondEscalationResolutionModule.RequestParameters memory _params
-  ) public assumeFuzzable(address(_params.accountingExtension)) {
-    // Check: does it revert if request ids do not match?
-    vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector);
-
-    vm.prank(address(oracle));
-    module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute);
-
+  function test_reverts(IBondEscalationResolutionModule.RequestParameters memory _params)
+    public
+    assumeFuzzable(address(_params.accountingExtension))
+  {
     // 1. BondEscalationResolutionModule_AlreadyResolved
-    bytes32 _requestId;
-    bytes32 _responseId;
-    (_requestId, _responseId, _disputeId) = _setResolutionModuleData(_params);
+    (bytes32 _requestId, bytes32 _responseId, bytes32 _disputeId) = _setResolutionModuleData(_params);
 
     // Set mock escalation with resolution == DisputerWon
     module.forTest_setEscalation(_disputeId, IBondEscalationResolutionModule.Resolution.DisputerWon, 0, 0, 0);

diff --git a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol
@@ -351,16 +351,6 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest {
     module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute);
   }
 
-  /**
-   * @notice Test that `resolveDispute` reverts if request ids do not match.
-   */
-  function test_revertIfInvalidRequestId(bytes32 _disputeId) public {
-    // Check: does it revert if request ids do not match?
-    vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector);
-    vm.prank(address(oracle));
-    module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute);
-  }
-
   /**
    * @notice Test that `resolveDispute` reverts if called during voting phase.
    */

diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol
@@ -652,16 +652,6 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest {
     module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute);
   }
 
-  /**
-   * @notice Test that `resolveDispute` reverts if request ids do not match.
-   */
-  function test_revertIfInvalidRequestId(bytes32 _disputeId) public {
-    // Check: does it revert if request ids do not match?
-    vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector);
-    vm.prank(address(oracle));
-    module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute);
-  }
-
   /**
    * @notice Test that `resolveDispute` reverts if called during committing or revealing time window.
    */