Skip to content

Commit 2fbe0e1

Browse files
tmigonetmigone
authored
Merge pull request #1074 from graphprotocol/tmigone/trust-fixes-subgraph-service
2 parents 8e1144c + ac8a071 commit 2fbe0e1

File tree

9 files changed

+160
-94
lines changed

9 files changed

+160
-94
lines changed

‎packages/subgraph-service/contracts/SubgraphService.sol‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,11 +312,9 @@ contract SubgraphService is
312312
/**
313313
* @notice See {ISubgraphService.closeStaleAllocation}
314314
*/
315-
function forceCloseAllocation(address allocationId) external override {
315+
function closeStaleAllocation(address allocationId) external override {
316316
Allocation.State memory allocation = allocations.get(allocationId);
317-
bool isStale = allocation.isStale(maxPOIStaleness);
318-
bool isOverAllocated_ = _isOverAllocated(allocation.indexer, delegationRatio);
319-
require(isStale || isOverAllocated_, SubgraphServiceCannotForceCloseAllocation(allocationId));
317+
require(allocation.isStale(maxPOIStaleness), SubgraphServiceCannotForceCloseAllocation(allocationId));
320318
require(!allocation.isAltruistic(), SubgraphServiceAllocationIsAltruistic(allocationId));
321319
_closeAllocation(allocationId);
322320
}
@@ -424,6 +422,7 @@ contract SubgraphService is
424422
* @return subgraphDeploymentId Subgraph deployment id for the allocation
425423
* @return tokens Amount of allocated tokens
426424
* @return accRewardsPerAllocatedToken Rewards snapshot
425+
* @return accRewardsPending Rewards pending to be minted due to allocation resize
427426
*/
428427
function getAllocationData(
429428
address allocationId

‎packages/subgraph-service/contracts/interfaces/ISubgraphService.sol‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -125,22 +125,20 @@ interface ISubgraphService is IDataServiceFees {
125125
error SubgraphServiceInvalidZeroStakeToFeesRatio();
126126

127127
/**
128-
* @notice Force close an allocation
129-
* @dev This function can be permissionlessly called when the allocation is stale or
130-
* if the indexer is over-allocated. This ensures that rewards for other allocations are
131-
* not diluted by an inactive allocation, and that over-allocated indexers stop accumulating
132-
* rewards with tokens they no longer have allocated.
128+
* @notice Force close a stale allocation
129+
* @dev This function can be permissionlessly called when the allocation is stale. This
130+
* ensures that rewards for other allocations are not diluted by an inactive allocation.
133131
*
134132
* Requirements:
135133
* - Allocation must exist and be open
136-
* - Allocation must be stale or indexer must be over-allocated
134+
* - Allocation must be stale
137135
* - Allocation cannot be altruistic
138136
*
139137
* Emits a {AllocationClosed} event.
140138
*
141139
* @param allocationId The id of the allocation
142140
*/
143-
function forceCloseAllocation(address allocationId) external;
141+
function closeStaleAllocation(address allocationId) external;
144142

145143
/**
146144
* @notice Change the amount of tokens in an allocation

‎packages/subgraph-service/contracts/libraries/LegacyAllocation.sol‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// SPDX-License-Identifier: GPL-3.0-or-later
22
pragma solidity 0.8.27;
33

4+
import { IHorizonStaking } from "@graphprotocol/horizon/contracts/interfaces/IHorizonStaking.sol";
5+
46
/**
57
* @title LegacyAllocation library
68
* @notice A library to handle legacy Allocations.
@@ -22,20 +24,14 @@ library LegacyAllocation {
2224
* @notice Thrown when attempting to migrate an allocation with an existing id
2325
* @param allocationId The allocation id
2426
*/
25-
error LegacyAllocationExists(address allocationId);
27+
error LegacyAllocationAlreadyExists(address allocationId);
2628

2729
/**
2830
* @notice Thrown when trying to get a non-existent allocation
2931
* @param allocationId The allocation id
3032
*/
3133
error LegacyAllocationDoesNotExist(address allocationId);
3234

33-
/**
34-
* @notice Thrown when trying to migrate an allocation that has already been migrated
35-
* @param allocationId The allocation id
36-
*/
37-
error LegacyAllocationAlreadyMigrated(address allocationId);
38-
3935
/**
4036
* @notice Migrate a legacy allocation
4137
* @dev Requirements:
@@ -52,7 +48,7 @@ library LegacyAllocation {
5248
address allocationId,
5349
bytes32 subgraphDeploymentId
5450
) internal {
55-
require(!self[allocationId].exists(), LegacyAllocationAlreadyMigrated(allocationId));
51+
require(!self[allocationId].exists(), LegacyAllocationAlreadyExists(allocationId));
5652

5753
State memory allocation = State({ indexer: indexer, subgraphDeploymentId: subgraphDeploymentId });
5854
self[allocationId] = allocation;
@@ -69,11 +65,19 @@ library LegacyAllocation {
6965

7066
/**
7167
* @notice Revert if a legacy allocation exists
68+
* @dev We first check the migrated mapping then the old staking contract.
69+
* @dev TODO: after the transition period when all the allocations are migrated we can
70+
* remove the call to the staking contract.
7271
* @param self The legacy allocation list mapping
7372
* @param allocationId The allocation id
7473
*/
75-
function revertIfExists(mapping(address => State) storage self, address allocationId) internal view {
76-
require(!self[allocationId].exists(), LegacyAllocationExists(allocationId));
74+
function revertIfExists(
75+
mapping(address => State) storage self,
76+
IHorizonStaking graphStaking,
77+
address allocationId
78+
) internal view {
79+
require(!self[allocationId].exists(), LegacyAllocationAlreadyExists(allocationId));
80+
require(!graphStaking.isAllocation(allocationId), LegacyAllocationAlreadyExists(allocationId));
7781
}
7882

7983
/**

‎packages/subgraph-service/contracts/utilities/AllocationManager.sol‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
212212

213213
// Ensure allocation id is not reused
214214
// need to check both subgraph service (on allocations.create()) and legacy allocations
215-
legacyAllocations.revertIfExists(_allocationId);
215+
legacyAllocations.revertIfExists(_graphStaking(), _allocationId);
216216
Allocation.State memory allocation = allocations.create(
217217
_indexer,
218218
_allocationId,

‎packages/subgraph-service/test/shared/HorizonStakingShared.t.sol‎

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@ import "forge-std/Test.sol";
55

66
import { IGraphPayments } from "@graphprotocol/horizon/contracts/interfaces/IGraphPayments.sol";
77
import { IHorizonStakingTypes } from "@graphprotocol/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol";
8+
import { IHorizonStakingExtension } from "@graphprotocol/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol";
89

910
import { SubgraphBaseTest } from "../SubgraphBaseTest.t.sol";
1011

1112
abstract contract HorizonStakingSharedTest is SubgraphBaseTest {
12-
1313
/*
1414
* HELPERS
1515
*/
@@ -45,11 +45,11 @@ abstract contract HorizonStakingSharedTest is SubgraphBaseTest {
4545
function _thawDeprovisionAndUnstake(address _indexer, address _verifier, uint256 _tokens) internal {
4646
// Initiate thaw request
4747
staking.thaw(_indexer, _verifier, _tokens);
48-
48+
4949
// Skip thawing period
5050
IHorizonStakingTypes.Provision memory provision = staking.getProvision(_indexer, _verifier);
5151
skip(provision.thawingPeriod + 1);
52-
52+
5353
// Deprovision and unstake
5454
staking.deprovision(_indexer, _verifier, 0);
5555
staking.unstake(_tokens);
@@ -64,6 +64,67 @@ abstract contract HorizonStakingSharedTest is SubgraphBaseTest {
6464
staking.setProvisionParameters(_indexer, _verifier, _maxVerifierCut, _thawingPeriod);
6565
}
6666

67+
function _setStorage_allocation_hardcoded(address indexer, address allocationId, uint256 tokens) internal {
68+
IHorizonStakingExtension.Allocation memory allocation = IHorizonStakingExtension.Allocation({
69+
indexer: indexer,
70+
subgraphDeploymentID: bytes32("0x12344321"),
71+
tokens: tokens,
72+
createdAtEpoch: 1234,
73+
closedAtEpoch: 1235,
74+
collectedFees: 1234,
75+
__DEPRECATED_effectiveAllocation: 1222234,
76+
accRewardsPerAllocatedToken: 1233334,
77+
distributedRebates: 1244434
78+
});
79+
80+
// __DEPRECATED_allocations
81+
uint256 allocationsSlot = 15;
82+
bytes32 allocationBaseSlot = keccak256(abi.encode(allocationId, allocationsSlot));
83+
vm.store(address(staking), allocationBaseSlot, bytes32(uint256(uint160(allocation.indexer))));
84+
vm.store(address(staking), bytes32(uint256(allocationBaseSlot) + 1), allocation.subgraphDeploymentID);
85+
vm.store(address(staking), bytes32(uint256(allocationBaseSlot) + 2), bytes32(tokens));
86+
vm.store(address(staking), bytes32(uint256(allocationBaseSlot) + 3), bytes32(allocation.createdAtEpoch));
87+
vm.store(address(staking), bytes32(uint256(allocationBaseSlot) + 4), bytes32(allocation.closedAtEpoch));
88+
vm.store(address(staking), bytes32(uint256(allocationBaseSlot) + 5), bytes32(allocation.collectedFees));
89+
vm.store(
90+
address(staking),
91+
bytes32(uint256(allocationBaseSlot) + 6),
92+
bytes32(allocation.__DEPRECATED_effectiveAllocation)
93+
);
94+
vm.store(
95+
address(staking),
96+
bytes32(uint256(allocationBaseSlot) + 7),
97+
bytes32(allocation.accRewardsPerAllocatedToken)
98+
);
99+
vm.store(address(staking), bytes32(uint256(allocationBaseSlot) + 8), bytes32(allocation.distributedRebates));
100+
101+
// _serviceProviders
102+
uint256 serviceProviderSlot = 14;
103+
bytes32 serviceProviderBaseSlot = keccak256(abi.encode(allocation.indexer, serviceProviderSlot));
104+
uint256 currentTokensStaked = uint256(vm.load(address(staking), serviceProviderBaseSlot));
105+
uint256 currentTokensProvisioned = uint256(
106+
vm.load(address(staking), bytes32(uint256(serviceProviderBaseSlot) + 1))
107+
);
108+
vm.store(
109+
address(staking),
110+
bytes32(uint256(serviceProviderBaseSlot) + 0),
111+
bytes32(currentTokensStaked + tokens)
112+
);
113+
vm.store(
114+
address(staking),
115+
bytes32(uint256(serviceProviderBaseSlot) + 1),
116+
bytes32(currentTokensProvisioned + tokens)
117+
);
118+
119+
// __DEPRECATED_subgraphAllocations
120+
uint256 subgraphsAllocationsSlot = 16;
121+
bytes32 subgraphAllocationsBaseSlot = keccak256(
122+
abi.encode(allocation.subgraphDeploymentID, subgraphsAllocationsSlot)
123+
);
124+
uint256 currentAllocatedTokens = uint256(vm.load(address(staking), subgraphAllocationsBaseSlot));
125+
vm.store(address(staking), subgraphAllocationsBaseSlot, bytes32(currentAllocatedTokens + tokens));
126+
}
127+
67128
/*
68129
* PRIVATE
69130
*/

‎packages/subgraph-service/test/subgraphService/SubgraphService.t.sol‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
151151
assertEq(afterSubgraphAllocatedTokens, _tokens);
152152
}
153153

154-
function _forceCloseAllocation(address _allocationId) internal {
154+
function _closeStaleAllocation(address _allocationId) internal {
155155
assertTrue(subgraphService.isActiveAllocation(_allocationId));
156156

157157
Allocation.State memory allocation = subgraphService.getAllocation(_allocationId);
@@ -168,7 +168,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
168168
);
169169

170170
// close stale allocation
171-
subgraphService.forceCloseAllocation(_allocationId);
171+
subgraphService.closeStaleAllocation(_allocationId);
172172

173173
// update allocation
174174
allocation = subgraphService.getAllocation(_allocationId);
@@ -380,6 +380,17 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
380380
}
381381
}
382382

383+
function _migrateLegacyAllocation(address _indexer, address _allocationId, bytes32 _subgraphDeploymentID) internal {
384+
vm.expectEmit(address(subgraphService));
385+
emit AllocationManager.LegacyAllocationMigrated(_indexer, _allocationId, _subgraphDeploymentID);
386+
387+
subgraphService.migrateLegacyAllocation(_indexer, _allocationId, _subgraphDeploymentID);
388+
389+
(address afterIndexer, bytes32 afterSubgraphDeploymentId) = subgraphService.legacyAllocations(_allocationId);
390+
assertEq(afterIndexer, _indexer);
391+
assertEq(afterSubgraphDeploymentId, _subgraphDeploymentID);
392+
}
393+
383394
/*
384395
* HELPERS
385396
*/

‎packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol‎

Lines changed: 11 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -3,38 +3,30 @@ pragma solidity 0.8.27;
33

44
import "forge-std/Test.sol";
55

6-
import { IDataService } from "@graphprotocol/horizon/contracts/data-service/interfaces/IDataService.sol";
76
import { IGraphPayments } from "@graphprotocol/horizon/contracts/interfaces/IGraphPayments.sol";
8-
import { ProvisionManager } from "@graphprotocol/horizon/contracts/data-service/utilities/ProvisionManager.sol";
9-
import { ProvisionTracker } from "@graphprotocol/horizon/contracts/data-service/libraries/ProvisionTracker.sol";
107

118
import { Allocation } from "../../../contracts/libraries/Allocation.sol";
12-
import { AllocationManager } from "../../../contracts/utilities/AllocationManager.sol";
139
import { ISubgraphService } from "../../../contracts/interfaces/ISubgraphService.sol";
14-
import { LegacyAllocation } from "../../../contracts/libraries/LegacyAllocation.sol";
1510
import { SubgraphServiceTest } from "../SubgraphService.t.sol";
1611

1712
contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
18-
1913
address private permissionlessBob = makeAddr("permissionlessBob");
2014

2115
/*
2216
* TESTS
2317
*/
2418

25-
function test_SubgraphService_Allocation_ForceClose_Stale(
26-
uint256 tokens
27-
) public useIndexer useAllocation(tokens) {
19+
function test_SubgraphService_Allocation_ForceClose_Stale(uint256 tokens) public useIndexer useAllocation(tokens) {
2820
// Skip forward
2921
skip(maxPOIStaleness + 1);
3022

3123
resetPrank(permissionlessBob);
32-
_forceCloseAllocation(allocationID);
24+
_closeStaleAllocation(allocationID);
3325
}
3426

3527
function test_SubgraphService_Allocation_ForceClose_Stale_AfterCollecting(
3628
uint256 tokens
37-
) public useIndexer useAllocation(tokens) {
29+
) public useIndexer useAllocation(tokens) {
3830
// Simulate POIs being submitted
3931
uint8 numberOfPOIs = 5;
4032
uint256 timeBetweenPOIs = 5 days;
@@ -52,43 +44,10 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
5244

5345
// Close the stale allocation
5446
resetPrank(permissionlessBob);
55-
_forceCloseAllocation(allocationID);
56-
}
57-
58-
function test_SubgraphService_Allocation_ForceClose_OverAllocated(
59-
uint256 tokens
60-
) public useIndexer useAllocation(tokens) {
61-
// thaw some tokens to become over allocated
62-
staking.thaw(users.indexer, address(subgraphService), tokens / 2);
63-
64-
resetPrank(permissionlessBob);
65-
_forceCloseAllocation(allocationID);
66-
}
67-
68-
function test_SubgraphService_Allocation_ForceClose_OverAllocated_AfterCollecting(
69-
uint256 tokens
70-
) public useIndexer useAllocation(tokens) {
71-
// Simulate POIs being submitted
72-
uint8 numberOfPOIs = 5;
73-
uint256 timeBetweenPOIs = 5 days;
74-
75-
for (uint8 i = 0; i < numberOfPOIs; i++) {
76-
// Skip forward
77-
skip(timeBetweenPOIs);
78-
79-
bytes memory data = abi.encode(allocationID, bytes32("POI1"));
80-
_collect(users.indexer, IGraphPayments.PaymentTypes.IndexingRewards, data);
81-
}
82-
83-
// thaw some tokens to become over allocated
84-
staking.thaw(users.indexer, address(subgraphService), tokens / 2);
85-
86-
// Close the over allocated allocation
87-
resetPrank(permissionlessBob);
88-
_forceCloseAllocation(allocationID);
47+
_closeStaleAllocation(allocationID);
8948
}
9049

91-
function test_SubgraphService_Allocation_ForceClose_RevertIf_NotStaleOrOverAllocated(
50+
function test_SubgraphService_Allocation_ForceClose_RevertIf_NotStale(
9251
uint256 tokens
9352
) public useIndexer useAllocation(tokens) {
9453
// Simulate POIs being submitted
@@ -98,26 +57,24 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
9857
for (uint8 i = 0; i < numberOfPOIs; i++) {
9958
// Skip forward
10059
skip(timeBetweenPOIs);
101-
60+
10261
resetPrank(users.indexer);
10362

10463
bytes memory data = abi.encode(allocationID, bytes32("POI1"));
10564
_collect(users.indexer, IGraphPayments.PaymentTypes.IndexingRewards, data);
106-
65+
10766
resetPrank(permissionlessBob);
10867
vm.expectRevert(
10968
abi.encodeWithSelector(
11069
ISubgraphService.SubgraphServiceCannotForceCloseAllocation.selector,
11170
allocationID
11271
)
11372
);
114-
subgraphService.forceCloseAllocation(allocationID);
73+
subgraphService.closeStaleAllocation(allocationID);
11574
}
11675
}
11776

118-
function test_SubgraphService_Allocation_ForceClose_RevertIf_Altruistic(
119-
uint256 tokens
120-
) public useIndexer {
77+
function test_SubgraphService_Allocation_ForceClose_RevertIf_Altruistic(uint256 tokens) public useIndexer {
12178
tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
12279

12380
_createProvision(users.indexer, tokens, maxSlashingPercentage, disputePeriod);
@@ -130,11 +87,8 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
13087

13188
resetPrank(permissionlessBob);
13289
vm.expectRevert(
133-
abi.encodeWithSelector(
134-
ISubgraphService.SubgraphServiceAllocationIsAltruistic.selector,
135-
allocationID
136-
)
90+
abi.encodeWithSelector(ISubgraphService.SubgraphServiceAllocationIsAltruistic.selector, allocationID)
13791
);
138-
subgraphService.forceCloseAllocation(allocationID);
92+
subgraphService.closeStaleAllocation(allocationID);
13993
}
14094
}

0 commit comments

Comments
 (0)