From fe9d605a0e14dbb4fc519f3ab1c7a8674b7868f6 Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Wed, 2 Oct 2024 06:46:17 +1000 Subject: [PATCH] chore(SubgraphService): allow force closing over allocated allocations (#1044) * chore(SubgraphService): allow force closing over allocated allocations * fix: merge close stale and over allocated functions * fix: removed unused error and fixed natspec for new error --- .../contracts/SubgraphService.sol | 23 +++++-- .../contracts/interfaces/ISubgraphService.sol | 31 +++++++--- .../contracts/utilities/AllocationManager.sol | 12 +++- .../subgraphService/SubgraphService.t.sol | 4 +- .../{closeStale.t.sol => forceClose.t.sol} | 60 ++++++++++++++----- .../collect/indexing/indexing.t.sol | 2 +- 6 files changed, 101 insertions(+), 31 deletions(-) rename packages/subgraph-service/test/subgraphService/allocation/{closeStale.t.sol => forceClose.t.sol} (63%) diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index 39b12a4c9..58c730f4d 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/packages/subgraph-service/contracts/SubgraphService.sol @@ -312,12 +312,11 @@ contract SubgraphService is /** * @notice See {ISubgraphService.closeStaleAllocation} */ - function closeStaleAllocation(address allocationId) external override { + function forceCloseAllocation(address allocationId) external override { Allocation.State memory allocation = allocations.get(allocationId); - require( - allocation.isStale(maxPOIStaleness), - SubgraphServiceAllocationNotStale(allocationId, allocation.lastPOIPresentedAt) - ); + bool isStale = allocation.isStale(maxPOIStaleness); + bool isOverAllocated_ = _isOverAllocated(allocation.indexer, delegationRatio); + require(isStale || isOverAllocated_, SubgraphServiceCannotForceCloseAllocation(allocationId)); require(!allocation.isAltruistic(), SubgraphServiceAllocationIsAltruistic(allocationId)); _closeAllocation(allocationId); } @@ -476,6 +475,20 @@ contract SubgraphService is return _encodeAllocationProof(indexer, allocationId); } + /** + * @notice See {ISubgraphService.isStaleAllocation} + */ + function isStaleAllocation(address allocationId) external view override returns (bool) { + return allocations.get(allocationId).isStale(maxPOIStaleness); + } + + /** + * @notice See {ISubgraphService.isOverAllocated} + */ + function isOverAllocated(address indexer) external view override returns (bool) { + return _isOverAllocated(indexer, delegationRatio); + } + // -- Data service parameter getters -- /** * @notice Getter for the accepted thawing period range for provisions diff --git a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol index b83d672f7..32ea9e8fb 100644 --- a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol +++ b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol @@ -110,11 +110,10 @@ interface ISubgraphService is IDataServiceFees { error SubgraphServiceInvalidRAV(address ravIndexer, address allocationIndexer); /** - * @notice Thrown when trying to force close an allocation that is not stale + * @notice Thrown when trying to force close an allocation that is not stale and the indexer is not over-allocated * @param allocationId The id of the allocation - * @param lastPOIPresentedAt The timestamp when the last POI was presented */ - error SubgraphServiceAllocationNotStale(address allocationId, uint256 lastPOIPresentedAt); + error SubgraphServiceCannotForceCloseAllocation(address allocationId); /** * @notice Thrown when trying to force close an altruistic allocation @@ -128,20 +127,22 @@ interface ISubgraphService is IDataServiceFees { error SubgraphServiceInvalidZeroStakeToFeesRatio(); /** - * @notice Close a stale allocation - * @dev This function can be permissionlessly called when the allocation is stale. - * This ensures rewards for other allocations are not diluted by an inactive allocation + * @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. * * Requirements: * - Allocation must exist and be open - * - Allocation must be stale + * - Allocation must be stale or indexer must be over-allocated * - Allocation cannot be altruistic * * Emits a {AllocationClosed} event. * * @param allocationId The id of the allocation */ - function closeStaleAllocation(address allocationId) external; + function forceCloseAllocation(address allocationId) external; /** * @notice Change the amount of tokens in an allocation @@ -237,4 +238,18 @@ interface ISubgraphService is IDataServiceFees { * @param allocationId The id of the allocation */ function encodeAllocationProof(address indexer, address allocationId) external view returns (bytes32); + + /** + * @notice Checks if an allocation is stale + * @param allocationId The id of the allocation + * @return True if the allocation is stale, false otherwise + */ + function isStaleAllocation(address allocationId) external view returns (bool); + + /** + * @notice Checks if an indexer is over-allocated + * @param allocationId The id of the allocation + * @return True if the indexer is over-allocated, false otherwise + */ + function isOverAllocated(address allocationId) external view returns (bool); } diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index 97c3adf99..3d66224c3 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -322,7 +322,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca ); // Check if the indexer is over-allocated and close the allocation if necessary - if (!allocationProvisionTracker.check(_graphStaking(), allocation.indexer, _delegationRatio)) { + if (_isOverAllocated(allocation.indexer, _delegationRatio)) { _closeAllocation(_allocationId); } @@ -479,4 +479,14 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca function _encodeAllocationProof(address _indexer, address _allocationId) internal view returns (bytes32) { return _hashTypedDataV4(keccak256(abi.encode(EIP712_ALLOCATION_PROOF_TYPEHASH, _indexer, _allocationId))); } + + /** + * @notice Checks if an allocation is over-allocated + * @param _indexer The address of the indexer + * @param _delegationRatio The delegation ratio to consider when locking tokens + * @return True if the allocation is over-allocated, false otherwise + */ + function _isOverAllocated(address _indexer, uint32 _delegationRatio) internal view returns (bool) { + return !allocationProvisionTracker.check(_graphStaking(), _indexer, _delegationRatio); + } } diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index e62ce8211..f48235d48 100644 --- 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 _closeStaleAllocation(address _allocationId) internal { + function _forceCloseAllocation(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.closeStaleAllocation(_allocationId); + subgraphService.forceCloseAllocation(_allocationId); // update allocation allocation = subgraphService.getAllocation(_allocationId); diff --git a/packages/subgraph-service/test/subgraphService/allocation/closeStale.t.sol b/packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol similarity index 63% rename from packages/subgraph-service/test/subgraphService/allocation/closeStale.t.sol rename to packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol index 68d736261..8817355f6 100644 --- a/packages/subgraph-service/test/subgraphService/allocation/closeStale.t.sol +++ b/packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol @@ -14,7 +14,7 @@ import { ISubgraphService } from "../../../contracts/interfaces/ISubgraphService import { LegacyAllocation } from "../../../contracts/libraries/LegacyAllocation.sol"; import { SubgraphServiceTest } from "../SubgraphService.t.sol"; -contract SubgraphServiceAllocationCloseStaleTest is SubgraphServiceTest { +contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest { address private permissionlessBob = makeAddr("permissionlessBob"); @@ -22,17 +22,17 @@ contract SubgraphServiceAllocationCloseStaleTest is SubgraphServiceTest { * TESTS */ - function test_SubgraphService_Allocation_CloseStale( + function test_SubgraphService_Allocation_ForceClose_Stale( uint256 tokens - ) public useIndexer useAllocation(tokens) { - // Skip forward + ) public useIndexer useAllocation(tokens) { + // Skip forward skip(maxPOIStaleness + 1); resetPrank(permissionlessBob); - _closeStaleAllocation(allocationID); + _forceCloseAllocation(allocationID); } - function test_SubgraphService_Allocation_CloseStale_AfterCollecting( + function test_SubgraphService_Allocation_ForceClose_Stale_AfterCollecting( uint256 tokens ) public useIndexer useAllocation(tokens) { // Simulate POIs being submitted @@ -52,10 +52,43 @@ contract SubgraphServiceAllocationCloseStaleTest is SubgraphServiceTest { // Close the stale allocation resetPrank(permissionlessBob); - _closeStaleAllocation(allocationID); + _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); } - function test_SubgraphService_Allocation_CloseStale_RevertIf_NotStale( + function test_SubgraphService_Allocation_ForceClose_RevertIf_NotStaleOrOverAllocated( uint256 tokens ) public useIndexer useAllocation(tokens) { // Simulate POIs being submitted @@ -74,16 +107,15 @@ contract SubgraphServiceAllocationCloseStaleTest is SubgraphServiceTest { resetPrank(permissionlessBob); vm.expectRevert( abi.encodeWithSelector( - ISubgraphService.SubgraphServiceAllocationNotStale.selector, - allocationID, - block.timestamp + ISubgraphService.SubgraphServiceCannotForceCloseAllocation.selector, + allocationID ) ); - subgraphService.closeStaleAllocation(allocationID); + subgraphService.forceCloseAllocation(allocationID); } } - function test_SubgraphService_Allocation_CloseStale_RevertIf_Altruistic( + function test_SubgraphService_Allocation_ForceClose_RevertIf_Altruistic( uint256 tokens ) public useIndexer { tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS); @@ -103,6 +135,6 @@ contract SubgraphServiceAllocationCloseStaleTest is SubgraphServiceTest { allocationID ) ); - subgraphService.closeStaleAllocation(allocationID); + subgraphService.forceCloseAllocation(allocationID); } } diff --git a/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol b/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol index 38e9e7865..465f5c8cd 100644 --- a/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol +++ b/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol @@ -109,7 +109,7 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { } } - function test_SubgraphService_Collect_Indexing_RevertWhen_OverAllocated(uint256 tokens) public useIndexer { + function test_SubgraphService_Collect_Indexing_OverAllocated(uint256 tokens) public useIndexer { tokens = bound(tokens, minimumProvisionTokens * 2, 10_000_000_000 ether); // setup allocation