fix: overallocated condition no longer allows force closing (TRST-H05)

Signed-off-by: Tomás Migone <[email protected]>
graphprotocol · Nov 29, 2024 · 7efedcc · 7efedcc
1 parent 0c0d090
commit 7efedcc
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 70 deletions.
diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol
@@ -312,11 +312,9 @@ contract SubgraphService is
     /**
      * @notice See {ISubgraphService.closeStaleAllocation}
      */
-    function forceCloseAllocation(address allocationId) external override {
+    function closeStaleAllocation(address allocationId) external override {
         Allocation.State memory allocation = allocations.get(allocationId);
-        bool isStale = allocation.isStale(maxPOIStaleness);
-        bool isOverAllocated_ = _isOverAllocated(allocation.indexer, delegationRatio);
-        require(isStale || isOverAllocated_, SubgraphServiceCannotForceCloseAllocation(allocationId));
+        require(allocation.isStale(maxPOIStaleness), SubgraphServiceCannotForceCloseAllocation(allocationId));
         require(!allocation.isAltruistic(), SubgraphServiceAllocationIsAltruistic(allocationId));
         _closeAllocation(allocationId);
     }

diff --git a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol
@@ -127,22 +127,20 @@ interface ISubgraphService is IDataServiceFees {
     error SubgraphServiceInvalidZeroStakeToFeesRatio();
 
     /**
-     * @notice Force close an allocation
-     * @dev This function can be permissionlessly called when the allocation is stale or
-     * if the indexer is over-allocated. This ensures that rewards for other allocations are
-     * not diluted by an inactive allocation, and that over-allocated indexers stop accumulating
-     * rewards with tokens they no longer have allocated.
+     * @notice Force close a stale allocation
+     * @dev This function can be permissionlessly called when the allocation is stale. This
+     * ensures that rewards for other allocations are not diluted by an inactive allocation.
      *
      * Requirements:
      * - Allocation must exist and be open
-     * - Allocation must be stale or indexer must be over-allocated
+     * - Allocation must be stale
      * - Allocation cannot be altruistic
      *
      * Emits a {AllocationClosed} event.
      *
      * @param allocationId The id of the allocation
      */
-    function forceCloseAllocation(address allocationId) external;
+    function closeStaleAllocation(address allocationId) external;
 
     /**
      * @notice Change the amount of tokens in an allocation

diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol
@@ -151,7 +151,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
         assertEq(afterSubgraphAllocatedTokens, _tokens);
     }
 
-    function _forceCloseAllocation(address _allocationId) internal {
+    function _closeStaleAllocation(address _allocationId) internal {
         assertTrue(subgraphService.isActiveAllocation(_allocationId));
 
         Allocation.State memory allocation = subgraphService.getAllocation(_allocationId);
@@ -168,7 +168,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
         );
 
         // close stale allocation
-        subgraphService.forceCloseAllocation(_allocationId);
+        subgraphService.closeStaleAllocation(_allocationId);
 
         // update allocation
         allocation = subgraphService.getAllocation(_allocationId);

diff --git a/packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol b/packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol
@@ -3,38 +3,30 @@ pragma solidity 0.8.27;
 
 import "forge-std/Test.sol";
 
-import { IDataService } from "@graphprotocol/horizon/contracts/data-service/interfaces/IDataService.sol";
 import { IGraphPayments } from "@graphprotocol/horizon/contracts/interfaces/IGraphPayments.sol";
-import { ProvisionManager } from "@graphprotocol/horizon/contracts/data-service/utilities/ProvisionManager.sol";
-import { ProvisionTracker } from "@graphprotocol/horizon/contracts/data-service/libraries/ProvisionTracker.sol";
 
 import { Allocation } from "../../../contracts/libraries/Allocation.sol";
-import { AllocationManager } from "../../../contracts/utilities/AllocationManager.sol";
 import { ISubgraphService } from "../../../contracts/interfaces/ISubgraphService.sol";
-import { LegacyAllocation } from "../../../contracts/libraries/LegacyAllocation.sol";
 import { SubgraphServiceTest } from "../SubgraphService.t.sol";
 
 contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
-
     address private permissionlessBob = makeAddr("permissionlessBob");
 
     /*
      * TESTS
      */
 
-    function test_SubgraphService_Allocation_ForceClose_Stale(
-        uint256 tokens
-    ) public useIndexer useAllocation(tokens) {
+    function test_SubgraphService_Allocation_ForceClose_Stale(uint256 tokens) public useIndexer useAllocation(tokens) {
         // Skip forward
         skip(maxPOIStaleness + 1);
 
         resetPrank(permissionlessBob);
-        _forceCloseAllocation(allocationID);
+        _closeStaleAllocation(allocationID);
     }
 
     function test_SubgraphService_Allocation_ForceClose_Stale_AfterCollecting(
         uint256 tokens
-    ) public useIndexer useAllocation(tokens) {      
+    ) public useIndexer useAllocation(tokens) {
         // Simulate POIs being submitted
         uint8 numberOfPOIs = 5;
         uint256 timeBetweenPOIs = 5 days;
@@ -52,43 +44,10 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
 
         // Close the stale allocation
         resetPrank(permissionlessBob);
-        _forceCloseAllocation(allocationID);
-    }
-
-    function test_SubgraphService_Allocation_ForceClose_OverAllocated(
-        uint256 tokens
-    ) public useIndexer useAllocation(tokens) { 
-        // thaw some tokens to become over allocated
-        staking.thaw(users.indexer, address(subgraphService), tokens / 2);
-
-        resetPrank(permissionlessBob);
-        _forceCloseAllocation(allocationID);
-    }
-
-    function test_SubgraphService_Allocation_ForceClose_OverAllocated_AfterCollecting(
-        uint256 tokens
-    ) public useIndexer useAllocation(tokens) {      
-        // Simulate POIs being submitted
-        uint8 numberOfPOIs = 5;
-        uint256 timeBetweenPOIs = 5 days;
-
-        for (uint8 i = 0; i < numberOfPOIs; i++) {
-            // Skip forward
-            skip(timeBetweenPOIs);
-
-            bytes memory data = abi.encode(allocationID, bytes32("POI1"));
-            _collect(users.indexer, IGraphPayments.PaymentTypes.IndexingRewards, data);
-        }
-
-        // thaw some tokens to become over allocated
-        staking.thaw(users.indexer, address(subgraphService), tokens / 2);
-
-        // Close the over allocated allocation
-        resetPrank(permissionlessBob);
-        _forceCloseAllocation(allocationID);
+        _closeStaleAllocation(allocationID);
     }
 
-    function test_SubgraphService_Allocation_ForceClose_RevertIf_NotStaleOrOverAllocated(
+    function test_SubgraphService_Allocation_ForceClose_RevertIf_NotStale(
         uint256 tokens
     ) public useIndexer useAllocation(tokens) {
         // Simulate POIs being submitted
@@ -98,26 +57,24 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
         for (uint8 i = 0; i < numberOfPOIs; i++) {
             // Skip forward
             skip(timeBetweenPOIs);
-            
+
             resetPrank(users.indexer);
 
             bytes memory data = abi.encode(allocationID, bytes32("POI1"));
             _collect(users.indexer, IGraphPayments.PaymentTypes.IndexingRewards, data);
-            
+
             resetPrank(permissionlessBob);
             vm.expectRevert(
                 abi.encodeWithSelector(
                     ISubgraphService.SubgraphServiceCannotForceCloseAllocation.selector,
                     allocationID
                 )
             );
-            subgraphService.forceCloseAllocation(allocationID);
+            subgraphService.closeStaleAllocation(allocationID);
         }
     }
 
-    function test_SubgraphService_Allocation_ForceClose_RevertIf_Altruistic(
-        uint256 tokens
-    ) public useIndexer {
+    function test_SubgraphService_Allocation_ForceClose_RevertIf_Altruistic(uint256 tokens) public useIndexer {
         tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
 
         _createProvision(users.indexer, tokens, maxSlashingPercentage, disputePeriod);
@@ -130,11 +87,8 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
 
         resetPrank(permissionlessBob);
         vm.expectRevert(
-            abi.encodeWithSelector(
-                ISubgraphService.SubgraphServiceAllocationIsAltruistic.selector,
-                allocationID
-            )
+            abi.encodeWithSelector(ISubgraphService.SubgraphServiceAllocationIsAltruistic.selector, allocationID)
         );
-        subgraphService.forceCloseAllocation(allocationID);
+        subgraphService.closeStaleAllocation(allocationID);
     }
 }