f: cleanup modifiers

Author: matiasedgeandnode

packages/horizon/contracts/data-service/libraries/ProvisionManagerLib.sol

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity 0.8.27;
+
+import { IHorizonStaking } from "../../interfaces/IHorizonStaking.sol";
+import { ProvisionManager } from "../utilities/ProvisionManager.sol";
+
+library ProvisionManagerLib {
+    function requireAuthorizedForProvision(
+        IHorizonStaking graphStaking,
+        address serviceProvider,
+        address dataService,
+        address operator
+    ) external view {
+        require(
+            graphStaking.isAuthorized(serviceProvider, dataService, operator),
+            ProvisionManager.ProvisionManagerNotAuthorized(serviceProvider, operator)
+        );
+    }
+}

packages/horizon/contracts/data-service/utilities/ProvisionManager.sol

@@ -9,6 +9,7 @@ import { PPMMath } from "../../libraries/PPMMath.sol";
 import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import { GraphDirectory } from "../../utilities/GraphDirectory.sol";
 import { ProvisionManagerV1Storage } from "./ProvisionManagerStorage.sol";
+import { ProvisionManagerLib } from "../libraries/ProvisionManagerLib.sol";
 
 /**
  * @title ProvisionManager contract
@@ -111,18 +112,7 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      * @param serviceProvider The address of the service provider.
      */
     modifier onlyAuthorizedForProvision(address serviceProvider) {
-        require(
-            _graphStaking().isAuthorized(serviceProvider, address(this), msg.sender),
-            ProvisionManagerNotAuthorized(serviceProvider, msg.sender)
-        );
-        _;
-    }
-
-    modifier onlyAuthorizedForProvisionHack(address caller, address serviceProvider) {
-        require(
-            _graphStaking().isAuthorized(serviceProvider, address(this), caller),
-            ProvisionManagerNotAuthorized(serviceProvider, caller)
-        );
+        ProvisionManagerLib.requireAuthorizedForProvision(_graphStaking(), serviceProvider, address(this), msg.sender);
         _;
     }
 
@@ -132,10 +122,14 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      * @param serviceProvider The address of the service provider.
      */
     modifier onlyValidProvision(address serviceProvider) virtual {
+        this.requireValidProvision(serviceProvider);
+        _;
+    }
+
+    function requireValidProvision(address serviceProvider) external view {
         IHorizonStaking.Provision memory provision = _getProvision(serviceProvider);
         _checkProvisionTokens(provision);
         _checkProvisionParameters(provision, false);
-        _;
     }
 
     /**

packages/subgraph-service/contracts/SubgraphService.sol

@@ -56,10 +56,14 @@ contract SubgraphService is
      * @param indexer The address of the indexer
      */
     modifier onlyRegisteredIndexer(address indexer) {
-        require(indexers[indexer].registeredAt != 0, SubgraphServiceIndexerNotRegistered(indexer));
+        this.requireRegisteredIndexer(indexer);
         _;
     }
 
+    function requireRegisteredIndexer(address indexer) external view {
+        require(indexers[indexer].registeredAt != 0, SubgraphServiceIndexerNotRegistered(indexer));
+    }
+
     /**
      * @notice Constructor for the SubgraphService contract
      * @dev DataService and Directory constructors set a bunch of immutable variables
@@ -706,17 +710,9 @@ contract SubgraphService is
         }
     }
 
-    function modifiersHack(
-        address caller,
-        address indexer
-    )
-        external
-        view
-        whenNotPaused
-        onlyAuthorizedForProvisionHack(caller, indexer)
-        onlyValidProvision(indexer)
-        onlyRegisteredIndexer(indexer)
-    {}
+    function getGraphStaking() external view returns (address) {
+        return address(_graphStaking());
+    }
 
     /**
      * @notice Delegates the call to the SubgraphServiceExtension implementation.

packages/subgraph-service/contracts/SubgraphServiceExtension.sol

@@ -3,22 +3,31 @@ pragma solidity 0.8.27;
 
 import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol";
 import { IRecurringCollector } from "@graphprotocol/horizon/contracts/interfaces/IRecurringCollector.sol";
+import { IHorizonStaking } from "@graphprotocol/horizon/contracts/interfaces/IHorizonStaking.sol";
+import { ProvisionManagerLib } from "@graphprotocol/horizon/contracts/data-service/libraries/ProvisionManagerLib.sol";
 
 import { IndexingAgreement } from "./libraries/IndexingAgreement.sol";
 import { SubgraphService } from "./SubgraphService.sol";
 
 contract SubgraphServiceExtension is PausableUpgradeable {
     using IndexingAgreement for IndexingAgreement.Manager;
 
-    modifier modifiersHack(address indexer) {
-        SubgraphService(address(this)).modifiersHack(msg.sender, indexer);
+    modifier onlyValid(address indexer) {
+        ProvisionManagerLib.requireAuthorizedForProvision(
+            IHorizonStaking(_getBase().getGraphStaking()),
+            indexer,
+            address(this),
+            msg.sender
+        );
+        _getBase().requireValidProvision(indexer);
+        _getBase().requireRegisteredIndexer(indexer);
         _;
     }
 
     function updateIndexingAgreement(
         address indexer,
         IRecurringCollector.SignedRCAU calldata signedRCAU
-    ) external modifiersHack(indexer) {
+    ) external whenNotPaused onlyValid(indexer) {
         IndexingAgreement._getManager().update(indexer, signedRCAU);
     }
 
@@ -38,7 +47,7 @@ contract SubgraphServiceExtension is PausableUpgradeable {
      *
      * @param agreementId The id of the agreement
      */
-    function cancelIndexingAgreement(address indexer, bytes16 agreementId) external modifiersHack(indexer) {
+    function cancelIndexingAgreement(address indexer, bytes16 agreementId) external whenNotPaused onlyValid(indexer) {
         IndexingAgreement._getManager().cancel(indexer, agreementId);
     }
 
@@ -67,4 +76,8 @@ contract SubgraphServiceExtension is PausableUpgradeable {
     function _cancelAllocationIndexingAgreement(address _allocationId) internal {
         IndexingAgreement._getManager().cancelForAllocation(_allocationId);
     }
+
+    function _getBase() internal view returns (SubgraphService) {
+        return SubgraphService(address(this));
+    }
 }