Graph Horizon: tests and internal review fixes

packages/horizon/.solhintignore

@@ -0,0 +1 @@
+contracts/mocks/*

packages/horizon/contracts/data-service/extensions/DataServiceFees.sol

@@ -39,6 +39,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
      * @param _unlockTimestamp The timestamp when the tokens can be released
      */
     function _lockStake(address _serviceProvider, uint256 _tokens, uint256 _unlockTimestamp) internal {
+        require(_tokens != 0, DataServiceFeesZeroTokens());
         feesProvisionTracker.lock(_graphStaking(), _serviceProvider, _tokens, delegationRatio);
 
         LinkedList.List storage claimsList = claimsLists[_serviceProvider];

packages/horizon/contracts/data-service/extensions/DataServicePausable.sol

@@ -50,7 +50,7 @@ abstract contract DataServicePausable is Pausable, DataService, IDataServicePaus
      * @param _pauseGuardian The address of the pause guardian
      * @param _allowed The allowed status of the pause guardian
      */
-    function _setPauseGuardian(address _pauseGuardian, bool _allowed) internal whenNotPaused {
+    function _setPauseGuardian(address _pauseGuardian, bool _allowed) internal {
         pauseGuardians[_pauseGuardian] = _allowed;
         emit PauseGuardianSet(_pauseGuardian, _allowed);
     }

packages/horizon/contracts/data-service/interfaces/IDataServiceFees.sol

@@ -78,6 +78,11 @@ interface IDataServiceFees is IDataService {
      */
     error DataServiceFeesClaimNotFound(bytes32 claimId);
 
+    /**
+     * @notice Emitted when trying to lock zero tokens in a stake claim
+     */
+    error DataServiceFeesZeroTokens();
+
     /**
      * @notice Releases expired stake claims for the caller.
      * @dev This function is only meant to be called if the service provider has enough

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

@@ -58,6 +58,13 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      */
     error ProvisionManagerInvalidValue(bytes message, uint256 value, uint256 min, uint256 max);
 
+    /**
+     * @notice Thrown when attempting to set a range where min is greater than max.
+     * @param min The minimum value.
+     * @param max The maximum value.
+     */
+    error ProvisionManagerInvalidRange(uint256 min, uint256 max);
+
     /**
      * @notice Thrown when the caller is not authorized to manage the provision of a service provider.
      * @param caller The address of the caller.
@@ -156,7 +163,7 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      *
      * @param _serviceProvider The address of the service provider.
      */
-    function _acceptProvisionParameters(address _serviceProvider) internal virtual {
+    function _acceptProvisionParameters(address _serviceProvider) internal {
         _checkProvisionParameters(_serviceProvider, true);
         _graphStaking().acceptProvisionParameters(_serviceProvider);
     }
@@ -177,6 +184,7 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      * @param _max The maximum allowed value for the provision tokens.
      */
     function _setProvisionTokensRange(uint256 _min, uint256 _max) internal {
+        require(_min <= _max, ProvisionManagerInvalidRange(_min, _max));
         minimumProvisionTokens = _min;
         maximumProvisionTokens = _max;
         emit ProvisionTokensRangeSet(_min, _max);
@@ -188,6 +196,7 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      * @param _max The maximum allowed value for the max verifier cut.
      */
     function _setVerifierCutRange(uint32 _min, uint32 _max) internal {
+        require(_min <= _max, ProvisionManagerInvalidRange(_min, _max));
         minimumVerifierCut = _min;
         maximumVerifierCut = _max;
         emit VerifierCutRangeSet(_min, _max);
@@ -199,6 +208,7 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      * @param _max The maximum allowed value for the thawing period.
      */
     function _setThawingPeriodRange(uint64 _min, uint64 _max) internal {
+        require(_min <= _max, ProvisionManagerInvalidRange(_min, _max));
         minimumThawingPeriod = _min;
         maximumThawingPeriod = _max;
         emit ThawingPeriodRangeSet(_min, _max);
@@ -261,7 +271,7 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
      * @notice Gets the delegation ratio.
      * @return The delegation ratio
      */
-    function _getDelegationRatio() internal view virtual returns (uint32) {
+    function _getDelegationRatio() internal view returns (uint32) {
         return delegationRatio;
     }
 

packages/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol

@@ -164,5 +164,6 @@ interface IHorizonStakingExtension is IRewardsIssuer {
      * @notice Retrun the time in blocks to unstake
      * @return Thawing period in blocks
      */
+    // solhint-disable-next-line func-name-mixedcase
     function __DEPRECATED_getThawingPeriod() external view returns (uint64);
 }

packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol

@@ -612,7 +612,7 @@ interface IHorizonStakingMain {
      * @dev This two step update process prevents the service provider from changing the parameters
      * without the verifier's consent.
      *
-     * Emits a {ProvisionParametersStaged} event.
+     * Emits a {ProvisionParametersStaged} event if at least one of the parameters changed.
      *
      * @param serviceProvider The service provider address
      * @param verifier The verifier address

packages/horizon/contracts/mocks/CurationMock.sol

@@ -5,7 +5,6 @@ pragma solidity 0.8.26;
 import { MockGRTToken } from "./MockGRTToken.sol";
 
 contract CurationMock {
-
     mapping(bytes32 => uint256) public curation;
 
     function signal(bytes32 _subgraphDeploymentID, uint256 _tokens) public {

packages/horizon/contracts/mocks/EpochManagerMock.sol

@@ -5,14 +5,13 @@ pragma solidity 0.8.26;
 import { IEpochManager } from "@graphprotocol/contracts/contracts/epochs/IEpochManager.sol";
 
 contract EpochManagerMock is IEpochManager {
-
     // -- Variables --
-    
+
     uint256 public epochLength;
     uint256 public lastRunEpoch;
     uint256 public lastLengthUpdateEpoch;
     uint256 public lastLengthUpdateBlock;
-    
+
     // -- Configuration --
 
     function setEpochLength(uint256 _epochLength) public {

packages/horizon/contracts/mocks/RewardsManagerMock.sol

@@ -5,7 +5,6 @@ pragma solidity 0.8.26;
 import { MockGRTToken } from "./MockGRTToken.sol";
 
 contract RewardsManagerMock {
-
     // -- Variables --
     MockGRTToken public token;
     uint256 private rewards;

packages/horizon/contracts/staking/HorizonStaking.sol

@@ -230,9 +230,13 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         uint64 thawingPeriod
     ) external override notPaused onlyAuthorized(serviceProvider, verifier) {
         Provision storage prov = _provisions[serviceProvider][verifier];
-        prov.maxVerifierCutPending = maxVerifierCut;
-        prov.thawingPeriodPending = thawingPeriod;
-        emit ProvisionParametersStaged(serviceProvider, verifier, maxVerifierCut, thawingPeriod);
+        require(prov.createdAt != 0, HorizonStakingInvalidProvision(serviceProvider, verifier));
+
+        if ((prov.maxVerifierCutPending != maxVerifierCut) || (prov.thawingPeriodPending != thawingPeriod)) {
+            prov.maxVerifierCutPending = maxVerifierCut;
+            prov.thawingPeriodPending = thawingPeriod;
+            emit ProvisionParametersStaged(serviceProvider, verifier, maxVerifierCut, thawingPeriod);
+        }
     }
 
     /**
@@ -241,9 +245,11 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
     function acceptProvisionParameters(address serviceProvider) external override notPaused {
         address verifier = msg.sender;
         Provision storage prov = _provisions[serviceProvider][verifier];
-        prov.maxVerifierCut = prov.maxVerifierCutPending;
-        prov.thawingPeriod = prov.thawingPeriodPending;
-        emit ProvisionParametersSet(serviceProvider, verifier, prov.maxVerifierCut, prov.thawingPeriod);
+        if ((prov.maxVerifierCutPending != prov.maxVerifierCut) || (prov.thawingPeriodPending != prov.thawingPeriod)) {
+            prov.maxVerifierCut = prov.maxVerifierCutPending;
+            prov.thawingPeriod = prov.thawingPeriodPending;
+            emit ProvisionParametersSet(serviceProvider, verifier, prov.maxVerifierCut, prov.thawingPeriod);
+        }
     }
 
     /*

packages/horizon/contracts/staking/HorizonStakingExtension.sol

@@ -294,6 +294,16 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon
         return _serviceProviders[indexer].tokensStaked > 0;
     }
 
+    /**
+     * @notice Retrun the time in blocks to unstake
+     * Deprecated, now enforced by each data service (verifier)
+     * @return Thawing period in blocks
+     */
+    // solhint-disable-next-line func-name-mixedcase
+    function __DEPRECATED_getThawingPeriod() external view returns (uint64) {
+        return __DEPRECATED_thawingPeriod;
+    }
+
     /**
      * @notice (Legacy) Return true if operator is allowed for the service provider on the subgraph data service.
      * @dev TODO: Delete after the transition period
@@ -305,15 +315,6 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon
         return _legacyOperatorAuth[serviceProvider][operator];
     }
 
-    /**
-     * @notice Retrun the time in blocks to unstake
-     * Deprecated, now enforced by each data service (verifier)
-     * @return Thawing period in blocks
-     */
-    function __DEPRECATED_getThawingPeriod() external view returns (uint64) {
-        return __DEPRECATED_thawingPeriod;
-    }
-
     /**
      * @dev Receive an Indexer's stake from L1.
      * The specified amount is added to the indexer's stake; the indexer's
@@ -450,7 +451,8 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon
         // Anyone is allowed to close ONLY under two concurrent conditions
         // - After maxAllocationEpochs passed
         // - When the allocation is for non-zero amount of tokens
-        bool isIndexerOrOperator = msg.sender == alloc.indexer || isOperator(alloc.indexer, SUBGRAPH_DATA_SERVICE_ADDRESS);
+        bool isIndexerOrOperator = msg.sender == alloc.indexer ||
+            isOperator(alloc.indexer, SUBGRAPH_DATA_SERVICE_ADDRESS);
         if (epochs <= __DEPRECATED_maxAllocationEpochs || alloc.tokens == 0) {
             require(isIndexerOrOperator, "!auth");
         }

packages/horizon/test/GraphBase.t.sol

@@ -84,6 +84,9 @@ abstract contract GraphBaseTest is Utils, Constants {
         vm.label({ account: address(escrow), newLabel: "PaymentsEscrow" });
         vm.label({ account: address(staking), newLabel: "HorizonStaking" });
         vm.label({ account: address(stakingExtension), newLabel: "HorizonStakingExtension" });
+
+        // Ensure caller is back to the original msg.sender
+        vm.stopPrank();
     }
 
     function deployProtocolContracts() private {