From a58f807db20d1b7e82c0502a1995cd676d168fb2 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Wed, 18 Sep 2024 14:23:44 +0100 Subject: [PATCH 01/13] fix: change EIP712_ALLOCATION_PROOF_TYPEHASH from variable to constant. (OZ N-06) --- .../subgraph-service/contracts/utilities/AllocationManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index 2e40f28d3..a6b719df4 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -30,7 +30,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca using TokenUtils for IGraphToken; ///@dev EIP712 typehash for allocation proof - bytes32 private immutable EIP712_ALLOCATION_PROOF_TYPEHASH = + bytes32 private constant EIP712_ALLOCATION_PROOF_TYPEHASH = keccak256("AllocationIdProof(address indexer,address allocationId)"); /** From 516a4e3bf2841d5ed764183e48e25df0a5dfc799 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Wed, 18 Sep 2024 17:24:44 +0100 Subject: [PATCH 02/13] fix: add override keyword in functions --- .../contracts/data-service/extensions/DataServicePausable.sol | 4 ++-- .../extensions/DataServicePausableUpgradeable.sol | 4 ++-- packages/subgraph-service/contracts/SubgraphService.sol | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/horizon/contracts/data-service/extensions/DataServicePausable.sol b/packages/horizon/contracts/data-service/extensions/DataServicePausable.sol index 4d88cb72e..3c83fbb0e 100644 --- a/packages/horizon/contracts/data-service/extensions/DataServicePausable.sol +++ b/packages/horizon/contracts/data-service/extensions/DataServicePausable.sol @@ -30,14 +30,14 @@ abstract contract DataServicePausable is Pausable, DataService, IDataServicePaus /** * @notice See {IDataServicePausable-pause} */ - function pause() external onlyPauseGuardian whenNotPaused { + function pause() external override onlyPauseGuardian whenNotPaused { _pause(); } /** * @notice See {IDataServicePausable-pause} */ - function unpause() external onlyPauseGuardian whenPaused { + function unpause() external override onlyPauseGuardian whenPaused { _unpause(); } diff --git a/packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol b/packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol index 52f27d9c4..82d7cc63b 100644 --- a/packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol +++ b/packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol @@ -26,14 +26,14 @@ abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataSer /** * @notice See {IDataServicePausable-pause} */ - function pause() external onlyPauseGuardian whenNotPaused { + function pause() external override onlyPauseGuardian whenNotPaused { _pause(); } /** * @notice See {IDataServicePausable-pause} */ - function unpause() external onlyPauseGuardian whenPaused { + function unpause() external override onlyPauseGuardian whenPaused { _unpause(); } diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index 5023e6160..ce64d9ec0 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/packages/subgraph-service/contracts/SubgraphService.sol @@ -351,7 +351,7 @@ contract SubgraphService is /** * @notice See {ISubgraphService.setRewardsDestination} */ - function setRewardsDestination(address rewardsDestination) external { + function setRewardsDestination(address rewardsDestination) external override { _setRewardsDestination(msg.sender, rewardsDestination); } From 9dfb8b0f1e7e3e454a8a249bcf86ce200bb38d84 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Wed, 18 Sep 2024 17:44:23 +0100 Subject: [PATCH 03/13] fix: remove unused return value. (OZ N-10) --- .../subgraph-service/contracts/utilities/AllocationManager.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index 2e40f28d3..978335b58 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -396,7 +396,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca * * @param _allocationId The id of the allocation to be closed */ - function _closeAllocation(address _allocationId) internal returns (Allocation.State memory) { + function _closeAllocation(address _allocationId) internal { Allocation.State memory allocation = allocations.get(_allocationId); // Take rewards snapshot to prevent other allos from counting tokens from this allo @@ -414,7 +414,6 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca allocation.tokens; emit AllocationClosed(allocation.indexer, _allocationId, allocation.subgraphDeploymentId, allocation.tokens); - return allocations[_allocationId]; } /** From bd62e959dadf8b011b7d3cf766d2cf1103b50120 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Wed, 18 Sep 2024 17:58:49 +0100 Subject: [PATCH 04/13] fix: remove unused return values from the Allocation library. (OZ N-10) --- .../contracts/libraries/Allocation.sol | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/packages/subgraph-service/contracts/libraries/Allocation.sol b/packages/subgraph-service/contracts/libraries/Allocation.sol index a55034f17..320547f92 100644 --- a/packages/subgraph-service/contracts/libraries/Allocation.sol +++ b/packages/subgraph-service/contracts/libraries/Allocation.sol @@ -96,12 +96,10 @@ library Allocation { * @param self The allocation list mapping * @param allocationId The allocation id */ - function presentPOI(mapping(address => State) storage self, address allocationId) internal returns (State memory) { + function presentPOI(mapping(address => State) storage self, address allocationId) internal { State storage allocation = _get(self, allocationId); require(allocation.isOpen(), AllocationClosed(allocationId, allocation.closedAt)); allocation.lastPOIPresentedAt = block.timestamp; - - return allocation; } /** @@ -116,12 +114,10 @@ library Allocation { mapping(address => State) storage self, address allocationId, uint256 accRewardsPerAllocatedToken - ) internal returns (State memory) { + ) internal { State storage allocation = _get(self, allocationId); require(allocation.isOpen(), AllocationClosed(allocationId, allocation.closedAt)); allocation.accRewardsPerAllocatedToken = accRewardsPerAllocatedToken; - - return allocation; } /** @@ -131,15 +127,10 @@ library Allocation { * @param self The allocation list mapping * @param allocationId The allocation id */ - function clearPendingRewards( - mapping(address => State) storage self, - address allocationId - ) internal returns (State memory) { + function clearPendingRewards(mapping(address => State) storage self, address allocationId) internal { State storage allocation = _get(self, allocationId); require(allocation.isOpen(), AllocationClosed(allocationId, allocation.closedAt)); allocation.accRewardsPending = 0; - - return allocation; } /** @@ -149,12 +140,10 @@ library Allocation { * @param self The allocation list mapping * @param allocationId The allocation id */ - function close(mapping(address => State) storage self, address allocationId) internal returns (State memory) { + function close(mapping(address => State) storage self, address allocationId) internal { State storage allocation = _get(self, allocationId); require(allocation.isOpen(), AllocationClosed(allocationId, allocation.closedAt)); allocation.closedAt = block.timestamp; - - return allocation; } /** From 374f57d015dd6f6f0b44507e217b46193c3d1e7b Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 19 Sep 2024 15:08:08 +0100 Subject: [PATCH 05/13] fix: typographical errors throughout codebase. (OZ N-07) --- packages/contracts/contracts/curation/Curation.sol | 4 ++-- packages/contracts/contracts/curation/ICuration.sol | 4 ++-- .../contracts/contracts/l2/curation/L2Curation.sol | 6 +++--- .../contracts/contracts/rewards/IRewardsIssuer.sol | 4 ++-- packages/contracts/contracts/staking/Staking.sol | 2 +- packages/contracts/contracts/staking/libs/Stakes.sol | 2 +- .../horizon/contracts/interfaces/ITAPCollector.sol | 2 +- .../interfaces/internal/IHorizonStakingExtension.sol | 2 +- .../interfaces/internal/IHorizonStakingMain.sol | 12 ++++++------ .../interfaces/internal/IHorizonStakingTypes.sol | 4 ++-- .../horizon/contracts/payments/GraphPayments.sol | 2 +- .../horizon/contracts/staking/HorizonStaking.sol | 4 ++-- .../horizon/contracts/staking/HorizonStakingBase.sol | 2 +- .../contracts/staking/HorizonStakingExtension.sol | 6 +++--- .../subgraph-service/contracts/DisputeManager.sol | 2 +- .../contracts/utilities/AllocationManager.sol | 4 ++-- 16 files changed, 31 insertions(+), 31 deletions(-) diff --git a/packages/contracts/contracts/curation/Curation.sol b/packages/contracts/contracts/curation/Curation.sol index bd3032046..e289d048c 100644 --- a/packages/contracts/contracts/curation/Curation.sol +++ b/packages/contracts/contracts/curation/Curation.sol @@ -257,7 +257,7 @@ contract Curation is CurationV2Storage, GraphUpgradeable { /** * @notice Get the amount of token reserves in a curation pool. - * @param _subgraphDeploymentID Subgraph deployment curation poool + * @param _subgraphDeploymentID Subgraph deployment curation pool * @return Amount of token reserves in the curation pool */ function getCurationPoolTokens(bytes32 _subgraphDeploymentID) external view override returns (uint256) { @@ -286,7 +286,7 @@ contract Curation is CurationV2Storage, GraphUpgradeable { /** * @notice Get the amount of signal in a curation pool. - * @param _subgraphDeploymentID Subgraph deployment curation poool + * @param _subgraphDeploymentID Subgraph deployment curation pool * @return Amount of signal minted for the subgraph deployment */ function getCurationPoolSignal(bytes32 _subgraphDeploymentID) public view override returns (uint256) { diff --git a/packages/contracts/contracts/curation/ICuration.sol b/packages/contracts/contracts/curation/ICuration.sol index fe2f0e929..4f2c2bac5 100644 --- a/packages/contracts/contracts/curation/ICuration.sol +++ b/packages/contracts/contracts/curation/ICuration.sol @@ -84,14 +84,14 @@ interface ICuration { /** * @notice Get the amount of signal in a curation pool. - * @param _subgraphDeploymentID Subgraph deployment curation poool + * @param _subgraphDeploymentID Subgraph deployment curation pool * @return Amount of signal minted for the subgraph deployment */ function getCurationPoolSignal(bytes32 _subgraphDeploymentID) external view returns (uint256); /** * @notice Get the amount of token reserves in a curation pool. - * @param _subgraphDeploymentID Subgraph deployment curation poool + * @param _subgraphDeploymentID Subgraph deployment curation pool * @return Amount of token reserves in the curation pool */ function getCurationPoolTokens(bytes32 _subgraphDeploymentID) external view returns (uint256); diff --git a/packages/contracts/contracts/l2/curation/L2Curation.sol b/packages/contracts/contracts/l2/curation/L2Curation.sol index 3f8728d0a..f6d64209b 100644 --- a/packages/contracts/contracts/l2/curation/L2Curation.sol +++ b/packages/contracts/contracts/l2/curation/L2Curation.sol @@ -147,7 +147,7 @@ contract L2Curation is CurationV3Storage, GraphUpgradeable, IL2Curation { /** * @notice Assign Graph Tokens collected as curation fees to the curation pool reserve. - * @dev This function can only be called by the Staking contract and will do the bookeeping of + * @dev This function can only be called by the Staking contract and will do the Bookkeeping of * transferred tokens into this contract. * @param _subgraphDeploymentID SubgraphDeployment where funds should be allocated as reserves * @param _tokens Amount of Graph Tokens to add to reserves @@ -326,7 +326,7 @@ contract L2Curation is CurationV3Storage, GraphUpgradeable, IL2Curation { /** * @notice Get the amount of token reserves in a curation pool. - * @param _subgraphDeploymentID Subgraph deployment curation poool + * @param _subgraphDeploymentID Subgraph deployment curation pool * @return Amount of token reserves in the curation pool */ function getCurationPoolTokens(bytes32 _subgraphDeploymentID) external view override returns (uint256) { @@ -355,7 +355,7 @@ contract L2Curation is CurationV3Storage, GraphUpgradeable, IL2Curation { /** * @notice Get the amount of signal in a curation pool. - * @param _subgraphDeploymentID Subgraph deployment curation poool + * @param _subgraphDeploymentID Subgraph deployment curation pool * @return Amount of signal minted for the subgraph deployment */ function getCurationPoolSignal(bytes32 _subgraphDeploymentID) public view override returns (uint256) { diff --git a/packages/contracts/contracts/rewards/IRewardsIssuer.sol b/packages/contracts/contracts/rewards/IRewardsIssuer.sol index 705ce8db8..fe6963fa7 100644 --- a/packages/contracts/contracts/rewards/IRewardsIssuer.sol +++ b/packages/contracts/contracts/rewards/IRewardsIssuer.sol @@ -33,9 +33,9 @@ interface IRewardsIssuer { function getSubgraphAllocatedTokens(bytes32 _subgraphDeploymentId) external view returns (uint256); /** - * @notice Wether or not an allocation is active (i.e open) + * @notice Whether or not an allocation is active (i.e open) * @param _allocationId Allocation Id - * @return Wether or not the allocation is active + * @return Whether or not the allocation is active */ function isActiveAllocation(address _allocationId) external view returns (bool); } diff --git a/packages/contracts/contracts/staking/Staking.sol b/packages/contracts/contracts/staking/Staking.sol index d44d4767e..6aef50efc 100644 --- a/packages/contracts/contracts/staking/Staking.sol +++ b/packages/contracts/contracts/staking/Staking.sol @@ -912,7 +912,7 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M if (curationFees > 0) { // Transfer and call collect() // This function transfer tokens to a trusted protocol contracts - // Then we call collect() to do the transfer bookeeping + // Then we call collect() to do the transfer Bookkeeping rewardsManager().onSubgraphSignalUpdate(_subgraphDeploymentID); TokenUtils.pushTokens(_graphToken, address(curation), curationFees); curation.collect(_subgraphDeploymentID, curationFees); diff --git a/packages/contracts/contracts/staking/libs/Stakes.sol b/packages/contracts/contracts/staking/libs/Stakes.sol index b0524b14c..e856cdec1 100644 --- a/packages/contracts/contracts/staking/libs/Stakes.sol +++ b/packages/contracts/contracts/staking/libs/Stakes.sol @@ -85,7 +85,7 @@ library Stakes { /** * @dev Unlock tokens. * @param stake Stake data - * @param _tokens Amount of tokens to unkock + * @param _tokens Amount of tokens to unlock */ function unlockTokens(Stakes.Indexer storage stake, uint256 _tokens) internal { stake.tokensLocked = stake.tokensLocked.sub(_tokens); diff --git a/packages/horizon/contracts/interfaces/ITAPCollector.sol b/packages/horizon/contracts/interfaces/ITAPCollector.sol index e7b5bc4fd..6cfc16485 100644 --- a/packages/horizon/contracts/interfaces/ITAPCollector.sol +++ b/packages/horizon/contracts/interfaces/ITAPCollector.sol @@ -35,7 +35,7 @@ interface ITAPCollector is IPaymentsCollector { } /** - * Trown when the caller is not the data service the RAV was issued to + * Thrown when the caller is not the data service the RAV was issued to * @param caller The address of the caller * @param dataService The address of the data service */ diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol index 6e29cb5c9..fcf70cc32 100644 --- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol +++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol @@ -162,7 +162,7 @@ interface IHorizonStakingExtension is IRewardsIssuer, IL2StakingBase { function isAllocation(address allocationID) external view returns (bool); /** - * @notice Retrun the time in blocks to unstake + * @notice Return the time in blocks to unstake * @return Thawing period in blocks */ // solhint-disable-next-line func-name-mixedcase diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol index 418459d9f..ed7e90804 100644 --- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol +++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol @@ -253,7 +253,7 @@ interface IHorizonStakingMain { * @param thawRequestId The ID of the thaw request * @param tokens The amount of tokens being released * @param shares The amount of shares being released - * @param thawingUntil The timestamp until the stake has thawn + * @param thawingUntil The timestamp until the stake has thawed */ event ThawRequestFulfilled(bytes32 indexed thawRequestId, uint256 tokens, uint256 shares, uint64 thawingUntil); @@ -417,7 +417,7 @@ interface IHorizonStakingMain { error HorizonStakingStillThawing(uint256 until); /** - * @notice Thrown when a service provider attempts to operate on verifieres that are not allowed. + * @notice Thrown when a service provider attempts to operate on verifiers that are not allowed. * @dev Only applies to stake from locked wallets. */ error HorizonStakingVerifierNotAllowed(address verifier); @@ -578,7 +578,7 @@ interface IHorizonStakingMain { /** * @notice Remove tokens from a provision and move them back to the service provider's idle stake. * @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw - * requests in the event that fulfulling all of them results in a gas limit error. + * requests in the event that fulfilling all of them results in a gas limit error. * * Requirements: * - Must have previously initiated a thaw request using {thaw}. @@ -704,7 +704,7 @@ interface IHorizonStakingMain { * @notice Withdraw undelegated tokens from a provision after thawing. * Tokens can be automatically re-delegated to another provision by setting `newServiceProvider`. * @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw - * requests in the event that fulfulling all of them results in a gas limit error. + * requests in the event that fulfilling all of them results in a gas limit error. * * Requirements: * - Must have previously initiated a thaw request using {undelegate}. @@ -744,7 +744,7 @@ interface IHorizonStakingMain { /** * @notice Delegate tokens to the subgraph data service provision. * This function is for backwards compatibility with the legacy staking contract. - * It only allows delegting to the subgraph data service and DOES NOT have slippage protection. + * It only allows delegating to the subgraph data service and DOES NOT have slippage protection. * @dev See {delegate}. * @param serviceProvider The service provider address * @param tokens The amount of tokens to delegate @@ -754,7 +754,7 @@ interface IHorizonStakingMain { /** * @notice Undelegate tokens from the subgraph data service provision and start thawing them. * This function is for backwards compatibility with the legacy staking contract. - * It only allows undelegting from the subgraph data service. + * It only allows undelegating from the subgraph data service. * @dev See {undelegate}. * @param serviceProvider The service provider address * @param shares The amount of shares to undelegate diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol index eef8098e6..42b5588ef 100644 --- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol +++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol @@ -39,7 +39,7 @@ interface IHorizonStakingTypes { * @dev See {ServiceProviderInternal} for the actual storage representation */ struct ServiceProvider { - // Total amount of tokens on the provider stake (only staked by the provider, inludes all provisions) + // Total amount of tokens on the provider stake (only staked by the provider, includes all provisions) uint256 tokensStaked; // Total amount of tokens locked in provisions (only staked by the provider) uint256 tokensProvisioned; @@ -50,7 +50,7 @@ interface IHorizonStakingTypes { * @dev It contains deprecated fields from the `Indexer` struct to maintain storage compatibility. */ struct ServiceProviderInternal { - // Total amount of tokens on the provider stake (only staked by the provider, inludes all provisions) + // Total amount of tokens on the provider stake (only staked by the provider, includes all provisions) uint256 tokensStaked; // (Deprecated) Tokens used in allocations uint256 __DEPRECATED_tokensAllocated; // solhint-disable-line graph/leading-underscore diff --git a/packages/horizon/contracts/payments/GraphPayments.sol b/packages/horizon/contracts/payments/GraphPayments.sol index b7cb34db7..05fbcb066 100644 --- a/packages/horizon/contracts/payments/GraphPayments.sol +++ b/packages/horizon/contracts/payments/GraphPayments.sol @@ -24,7 +24,7 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I /** * @notice Constructor for the {GraphPayments} contract - * @dev This contract is upgradeable however we stil use the constructor to set + * @dev This contract is upgradeable however we still use the constructor to set * a few immutable variables. * @param controller The address of the Graph controller * @param protocolPaymentCut The protocol tax in PPM diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol index 74b2d8d09..e36d33a73 100644 --- a/packages/horizon/contracts/staking/HorizonStaking.sol +++ b/packages/horizon/contracts/staking/HorizonStaking.sol @@ -62,7 +62,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { } /** - * @dev The staking contract is upgradeable however we stil use the constructor to set + * @dev The staking contract is upgradeable however we still use the constructor to set * a few immutable variables. * @param controller The address of the Graph controller contract. * @param stakingExtensionAddress The address of the staking extension contract. @@ -903,7 +903,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { * * @param _thawRequestId The ID of the current thaw request * @param _acc The accumulator data for the thaw requests being fulfilled - * @return Wether the thaw request is still thawing, indicating that the traversal should continue or stop. + * @return Whether the thaw request is still thawing, indicating that the traversal should continue or stop. * @return The updated accumulator data */ function _fulfillThawRequest(bytes32 _thawRequestId, bytes memory _acc) private returns (bool, bytes memory) { diff --git a/packages/horizon/contracts/staking/HorizonStakingBase.sol b/packages/horizon/contracts/staking/HorizonStakingBase.sol index b3e94c7ea..b55b39fd8 100644 --- a/packages/horizon/contracts/staking/HorizonStakingBase.sol +++ b/packages/horizon/contracts/staking/HorizonStakingBase.sol @@ -39,7 +39,7 @@ abstract contract HorizonStakingBase is address internal immutable SUBGRAPH_DATA_SERVICE_ADDRESS; /** - * @dev The staking contract is upgradeable however we stil use the constructor to set + * @dev The staking contract is upgradeable however we still use the constructor to set * a few immutable variables. * @param controller The address of the Graph controller contract. * @param subgraphDataServiceAddress The address of the subgraph data service. diff --git a/packages/horizon/contracts/staking/HorizonStakingExtension.sol b/packages/horizon/contracts/staking/HorizonStakingExtension.sol index ba7eb3a5b..66efe5521 100644 --- a/packages/horizon/contracts/staking/HorizonStakingExtension.sol +++ b/packages/horizon/contracts/staking/HorizonStakingExtension.sol @@ -41,7 +41,7 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon } /** - * @dev The staking contract is upgradeable however we stil use the constructor to set + * @dev The staking contract is upgradeable however we still use the constructor to set * a few immutable variables. * @param controller The address of the Graph controller contract. * @param subgraphDataServiceAddress The address of the subgraph data service. @@ -296,7 +296,7 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon } /** - * @notice Retrun the time in blocks to unstake + * @notice Return the time in blocks to unstake * Deprecated, now enforced by each data service (verifier) * @return Thawing period in blocks */ @@ -569,7 +569,7 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon if (curationFees > 0) { // Transfer and call collect() // This function transfer tokens to a trusted protocol contracts - // Then we call collect() to do the transfer bookeeping + // Then we call collect() to do the transfer Bookkeeping _graphRewardsManager().onSubgraphSignalUpdate(_subgraphDeploymentID); _graphToken().pushTokens(address(curation), curationFees); curation.collect(_subgraphDeploymentID, curationFees); diff --git a/packages/subgraph-service/contracts/DisputeManager.sol b/packages/subgraph-service/contracts/DisputeManager.sol index f48c20463..8bf0f363b 100644 --- a/packages/subgraph-service/contracts/DisputeManager.sol +++ b/packages/subgraph-service/contracts/DisputeManager.sol @@ -272,7 +272,7 @@ contract DisputeManager is } /** - * @notice Once the dispute period ends, if the disput status remains Pending, + * @notice Once the dispute period ends, if the dispute status remains Pending, * the fisherman can cancel the dispute and get back their initial deposit. * @dev Cancel a dispute with Id `disputeId` * @param disputeId Id of the dispute to be cancelled diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index 2e40f28d3..e0d3ea036 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -142,7 +142,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca error AllocationManagerZeroTokensAllocation(address allocationId); /** - * @notice Thrown when attempting to collect indexing rewards on a closed allocationl + * @notice Thrown when attempting to collect indexing rewards on a closed allocation * @param allocationId The id of the allocation */ error AllocationManagerAllocationClosed(address allocationId); @@ -454,7 +454,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca } /** - * @notice Verifies ownsership of an allocation id by verifying an EIP712 allocation proof + * @notice Verifies ownership of an allocation id by verifying an EIP712 allocation proof * @dev Requirements: * - Signer must be the allocation id address * @param _indexer The address of the indexer From 79f7828bb96c76524c3e07e8fd87a713695ea916 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 19 Sep 2024 18:28:37 +0100 Subject: [PATCH 06/13] fix: reorder parameter use in mulPPM function call in mulPPMRoundUp. (OZ N-12) --- packages/horizon/contracts/libraries/PPMMath.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/horizon/contracts/libraries/PPMMath.sol b/packages/horizon/contracts/libraries/PPMMath.sol index 5bd636add..38b653954 100644 --- a/packages/horizon/contracts/libraries/PPMMath.sol +++ b/packages/horizon/contracts/libraries/PPMMath.sol @@ -42,7 +42,7 @@ library PPMMath { */ function mulPPMRoundUp(uint256 a, uint256 b) internal pure returns (uint256) { require(isValidPPM(b), PPMMathInvalidPPM(b)); - return a - mulPPM(MAX_PPM - b, a); + return a - mulPPM(a, MAX_PPM - b); } /** From 4354737396d8503b35b3e0bdfceddd1cd3230c1d Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Fri, 20 Sep 2024 05:08:03 +1000 Subject: [PATCH 07/13] fix: only add to delegation pool if shares is greater than 0 (#1031) --- .../contracts/utilities/AllocationManager.sol | 8 ++++- .../subgraphService/SubgraphService.t.sol | 6 ++-- .../collect/indexing/indexing.t.sol | 31 +++++++++++++++++-- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index c277e2e9e..9604f92bb 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.27; import { IGraphPayments } from "@graphprotocol/horizon/contracts/interfaces/IGraphPayments.sol"; import { IGraphToken } from "@graphprotocol/contracts/contracts/token/IGraphToken.sol"; +import { IHorizonStakingTypes } from "@graphprotocol/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol"; import { GraphDirectory } from "@graphprotocol/horizon/contracts/utilities/GraphDirectory.sol"; import { AllocationManagerV1Storage } from "./AllocationManagerStorage.sol"; @@ -289,7 +290,12 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca address(this), IGraphPayments.PaymentTypes.IndexingFee ); - tokensDelegationRewards = tokensRewards.mulPPM(delegatorCut); + IHorizonStakingTypes.DelegationPool memory delegationPool = _graphStaking().getDelegationPool( + allocation.indexer, + address(this) + ); + // If delegation pool has no shares then we don't need to distribute rewards to delegators + tokensDelegationRewards = delegationPool.shares > 0 ? tokensRewards.mulPPM(delegatorCut) : 0; if (tokensDelegationRewards > 0) { _graphToken().approve(address(_graphStaking()), tokensDelegationRewards); _graphStaking().addToDelegationPool(allocation.indexer, address(this), tokensDelegationRewards); diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index 05c038680..845a7b7fb 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -11,6 +11,7 @@ import { ITAPCollector } from "@graphprotocol/horizon/contracts/interfaces/ITAPC import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import { LinkedList } from "@graphprotocol/horizon/contracts/libraries/LinkedList.sol"; import { IDataServiceFees } from "@graphprotocol/horizon/contracts/data-service/interfaces/IDataServiceFees.sol"; +import { IHorizonStakingTypes } from "@graphprotocol/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol"; import { Allocation } from "../../contracts/libraries/Allocation.sol"; import { AllocationManager } from "../../contracts/utilities/AllocationManager.sol"; @@ -255,7 +256,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead IGraphPayments.PaymentTypes.IndexingFee ); - indexingRewardsData.tokensDelegationRewards = paymentCollected.mulPPM(delegatorCut); + IHorizonStakingTypes.DelegationPool memory delegationPool = staking.getDelegationPool(allocation.indexer, address(subgraphService)); + indexingRewardsData.tokensDelegationRewards = delegationPool.shares > 0 ? paymentCollected.mulPPM(delegatorCut) : 0; indexingRewardsData.tokensIndexerRewards = paymentCollected - indexingRewardsData.tokensDelegationRewards; vm.expectEmit(address(subgraphService)); @@ -322,7 +324,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { assertEq(stakeClaim.createdAt, block.timestamp); assertEq(stakeClaim.releaseAt, block.timestamp + disputePeriod); assertEq(stakeClaim.nextClaim, bytes32(0)); - } else { + } else if (_paymentType == IGraphPayments.PaymentTypes.IndexingRewards) { // Update allocation after collecting rewards allocation = subgraphService.getAllocation(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 8b29ec830..ae2a085ed 100644 --- a/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol +++ b/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol @@ -27,14 +27,38 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { uint256 delegationTokens, uint256 delegationFeeCut ) public useIndexer useAllocation(tokens) useDelegation(delegationTokens) { + // Collect reverts if delegationFeeCut is 100% + delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM - 1); + _setDelegationFeeCut( + users.indexer, + address(subgraphService), + // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead + IGraphPayments.PaymentTypes.IndexingFee, + delegationFeeCut + ); + IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards; + bytes memory data = abi.encode(allocationID, bytes32("POI")); + _collect(users.indexer, paymentType, data); + } + + function test_SubgraphService_Collect_Indexing_AfterUndelegate( + uint256 tokens, + uint256 delegationTokens, + uint256 delegationFeeCut + ) public useIndexer useAllocation(tokens) useDelegation(delegationTokens) { + // Collect reverts if delegationFeeCut is 100% delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM); _setDelegationFeeCut( users.indexer, address(subgraphService), // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead IGraphPayments.PaymentTypes.IndexingFee, - 100_000 + delegationFeeCut ); + // Undelegate + resetPrank(users.delegator); + staking.undelegate(users.indexer, address(subgraphService), delegationTokens); + resetPrank(users.indexer); IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards; bytes memory data = abi.encode(allocationID, bytes32("POI")); _collect(users.indexer, paymentType, data); @@ -68,13 +92,14 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { uint256 delegationTokens, uint256 delegationFeeCut ) public useIndexer useAllocation(tokens) useDelegation(delegationTokens) { - delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM); + // Collect reverts if delegationFeeCut is 100% + delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM - 1); _setDelegationFeeCut( users.indexer, address(subgraphService), // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead IGraphPayments.PaymentTypes.IndexingFee, - 100_000 + delegationFeeCut ); uint8 numberOfPOIs = 20; From fe380e46d9262f87b8b98c5caa88c53e2113abe4 Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Fri, 20 Sep 2024 05:09:13 +1000 Subject: [PATCH 08/13] fix(SubgraphService): use correct payment type for collect indexing rewards (#1032) --- .../contracts/utilities/AllocationManager.sol | 2 +- .../test/subgraphService/SubgraphService.t.sol | 3 +-- .../test/subgraphService/collect/indexing/indexing.t.sol | 9 +++------ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index 9604f92bb..04a6e09dc 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -288,7 +288,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca uint256 delegatorCut = _graphStaking().getDelegationFeeCut( allocation.indexer, address(this), - IGraphPayments.PaymentTypes.IndexingFee + IGraphPayments.PaymentTypes.IndexingRewards ); IHorizonStakingTypes.DelegationPool memory delegationPool = _graphStaking().getDelegationPool( allocation.indexer, diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index 845a7b7fb..b5b7f744e 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -253,8 +253,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { uint256 delegatorCut = staking.getDelegationFeeCut( allocation.indexer, address(subgraphService), - // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead - IGraphPayments.PaymentTypes.IndexingFee + IGraphPayments.PaymentTypes.IndexingRewards ); IHorizonStakingTypes.DelegationPool memory delegationPool = staking.getDelegationPool(allocation.indexer, address(subgraphService)); indexingRewardsData.tokensDelegationRewards = delegationPool.shares > 0 ? paymentCollected.mulPPM(delegatorCut) : 0; 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 ae2a085ed..bff35f574 100644 --- a/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol +++ b/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol @@ -32,8 +32,7 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { _setDelegationFeeCut( users.indexer, address(subgraphService), - // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead - IGraphPayments.PaymentTypes.IndexingFee, + IGraphPayments.PaymentTypes.IndexingRewards, delegationFeeCut ); IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards; @@ -51,8 +50,7 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { _setDelegationFeeCut( users.indexer, address(subgraphService), - // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead - IGraphPayments.PaymentTypes.IndexingFee, + IGraphPayments.PaymentTypes.IndexingRewards, delegationFeeCut ); // Undelegate @@ -97,8 +95,7 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { _setDelegationFeeCut( users.indexer, address(subgraphService), - // TODO: this should be fixed in AllocationManager, it should be IndexingRewards instead - IGraphPayments.PaymentTypes.IndexingFee, + IGraphPayments.PaymentTypes.IndexingRewards, delegationFeeCut ); From 95b3f83b1abe85449a63ab4922e84896f989ac57 Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Fri, 20 Sep 2024 05:10:11 +1000 Subject: [PATCH 09/13] fix(Horizon): only allow addToDelegationPool if provision exists and pool has shares (#1042) --- .../internal/IHorizonStakingMain.sol | 7 + .../contracts/staking/HorizonStaking.sol | 8 + packages/horizon/test/escrow/collect.t.sol | 147 +++++++++++++++-- .../horizon/test/payments/GraphPayments.t.sol | 149 ++++++++++++++++-- .../test/staking/delegation/addToPool.t.sol | 63 +++++--- 5 files changed, 324 insertions(+), 50 deletions(-) diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol index ed7e90804..eafce457a 100644 --- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol +++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol @@ -402,6 +402,13 @@ interface IHorizonStakingMain { */ error HorizonStakingInvalidDelegationPoolState(address serviceProvider, address verifier); + /** + * @notice Thrown when attempting to operate with a delegation pool that does not exist. + * @param serviceProvider The service provider address + * @param verifier The verifier address + */ + error HorizonStakingInvalidDelegationPool(address serviceProvider, address verifier); + // -- Errors: thaw requests -- error HorizonStakingNothingThawing(); diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol index e36d33a73..c8709e639 100644 --- a/packages/horizon/contracts/staking/HorizonStaking.sol +++ b/packages/horizon/contracts/staking/HorizonStaking.sol @@ -279,7 +279,15 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { uint256 tokens ) external override notPaused { require(tokens != 0, HorizonStakingInvalidZeroTokens()); + + // Provision must exist before adding to delegation pool + Provision memory prov = _provisions[serviceProvider][verifier]; + require(prov.createdAt != 0, HorizonStakingInvalidProvision(serviceProvider, verifier)); + + // Delegation pool must exist before adding tokens DelegationPoolInternal storage pool = _getDelegationPool(serviceProvider, verifier); + require(pool.shares > 0, HorizonStakingInvalidDelegationPool(serviceProvider, verifier)); + pool.tokens = pool.tokens + tokens; _graphToken().pullTokens(msg.sender, tokens); emit TokensToDelegationPoolAdded(serviceProvider, verifier, tokens); diff --git a/packages/horizon/test/escrow/collect.t.sol b/packages/horizon/test/escrow/collect.t.sol index 67efcc6e7..cbf945515 100644 --- a/packages/horizon/test/escrow/collect.t.sol +++ b/packages/horizon/test/escrow/collect.t.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.27; import "forge-std/Test.sol"; +import { IHorizonStakingMain } from "../../contracts/interfaces/internal/IHorizonStakingMain.sol"; import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { IPaymentsEscrow } from "../../contracts/interfaces/IPaymentsEscrow.sol"; @@ -10,30 +11,107 @@ import { GraphEscrowTest } from "./GraphEscrow.t.sol"; contract GraphEscrowCollectTest is GraphEscrowTest { + struct CollectPaymentData { + uint256 escrowBalance; + uint256 paymentsBalance; + uint256 receiverBalance; + uint256 delegationPoolBalance; + uint256 dataServiceBalance; + } + + function _collect( + IGraphPayments.PaymentTypes _paymentType, + address _payer, + address _receiver, + uint256 _tokens, + address _dataService, + uint256 _tokensDataService + ) private { + // Previous balances + (uint256 previousPayerEscrowBalance,,) = escrow.escrowAccounts(_payer, _receiver); + CollectPaymentData memory previousBalances = CollectPaymentData({ + escrowBalance: token.balanceOf(address(escrow)), + paymentsBalance: token.balanceOf(address(payments)), + receiverBalance: token.balanceOf(_receiver), + delegationPoolBalance: staking.getDelegatedTokensAvailable( + _receiver, + _dataService + ), + dataServiceBalance: token.balanceOf(_dataService) + }); + + vm.expectEmit(address(escrow)); + emit IPaymentsEscrow.EscrowCollected(_payer, _receiver, _tokens); + escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _tokensDataService); + + // Calculate cuts + uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); + uint256 delegatorCut = staking.getDelegationFeeCut( + _receiver, + _dataService, + _paymentType + ); + uint256 tokensProtocol = _tokens * protocolPaymentCut / MAX_PPM; + uint256 tokensDelegation = _tokens * delegatorCut / MAX_PPM; + + // After balances + (uint256 afterPayerEscrowBalance,,) = escrow.escrowAccounts(_payer, _receiver); + CollectPaymentData memory afterBalances = CollectPaymentData({ + escrowBalance: token.balanceOf(address(escrow)), + paymentsBalance: token.balanceOf(address(payments)), + receiverBalance: token.balanceOf(_receiver), + delegationPoolBalance: staking.getDelegatedTokensAvailable( + _receiver, + _dataService + ), + dataServiceBalance: token.balanceOf(_dataService) + }); + + // Check receiver balance after payment + uint256 receiverExpectedPayment = _tokens - _tokensDataService - tokensProtocol - tokensDelegation; + assertEq(afterBalances.receiverBalance - previousBalances.receiverBalance, receiverExpectedPayment); + assertEq(token.balanceOf(address(payments)), 0); + + // Check delegation pool balance after payment + assertEq(afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, tokensDelegation); + + // Check that the escrow account has been updated + assertEq(previousBalances.escrowBalance, afterBalances.escrowBalance + _tokens); + + // Check that payments balance didn't change + assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); + + // Check data service balance after payment + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + + // Check payers escrow balance after payment + assertEq(previousPayerEscrowBalance - _tokens, afterPayerEscrowBalance); + } + /* * TESTS */ function testCollect_Tokens( - uint256 amount, + uint256 tokens, + uint256 delegationTokens, uint256 tokensDataService - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - uint256 tokensProtocol = amount * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegatoion = amount * delegationFeeCut / MAX_PPM; - vm.assume(tokensDataService < amount - tokensProtocol - tokensDelegatoion); - - vm.startPrank(users.gateway); - escrow.approveCollector(users.verifier, amount); - _depositTokens(amount); + ) public useIndexer useProvision(tokens, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + uint256 tokensProtocol = tokens * protocolPaymentCut / MAX_PPM; + uint256 tokensDelegatoion = tokens * delegationFeeCut / MAX_PPM; + vm.assume(tokensDataService < tokens - tokensProtocol - tokensDelegatoion); - uint256 indexerPreviousBalance = token.balanceOf(users.indexer); - vm.startPrank(users.verifier); - escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); + vm.assume(delegationTokens > MIN_DELEGATION); + vm.assume(delegationTokens <= MAX_STAKING_TOKENS); + resetPrank(users.delegator); + _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0); - uint256 indexerBalance = token.balanceOf(users.indexer); - uint256 indexerExpectedPayment = amount - tokensDataService - tokensProtocol - tokensDelegatoion; - assertEq(indexerBalance - indexerPreviousBalance, indexerExpectedPayment); - assertEq(token.balanceOf(address(payments)), 0); + resetPrank(users.gateway); + escrow.approveCollector(users.verifier, tokens); + _depositTokens(tokens); + + resetPrank(users.verifier); + _collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, tokens, subgraphDataServiceAddress, tokensDataService); } function testCollect_RevertWhen_CollectorNotAuthorized(uint256 amount) public { @@ -78,4 +156,41 @@ contract GraphEscrowCollectTest is GraphEscrowTest { escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 0); vm.stopPrank(); } + + function testCollect_RevertWhen_InvalidPool( + uint256 amount + ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + vm.assume(amount > 1 ether); + + resetPrank(users.gateway); + escrow.approveCollector(users.verifier, amount); + _depositTokens(amount); + + resetPrank(users.verifier); + vm.expectRevert(abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + users.indexer, + subgraphDataServiceAddress + )); + escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 1); + } + + function testCollect_RevertWhen_InvalidProvision( + uint256 amount + ) public useIndexer useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + vm.assume(amount > 1 ether); + vm.assume(amount <= MAX_STAKING_TOKENS); + + resetPrank(users.gateway); + escrow.approveCollector(users.verifier, amount); + _depositTokens(amount); + + resetPrank(users.verifier); + vm.expectRevert(abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + users.indexer, + subgraphDataServiceAddress + )); + escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 1); + } } \ No newline at end of file diff --git a/packages/horizon/test/payments/GraphPayments.t.sol b/packages/horizon/test/payments/GraphPayments.t.sol index 62e582c26..8b76678b4 100644 --- a/packages/horizon/test/payments/GraphPayments.t.sol +++ b/packages/horizon/test/payments/GraphPayments.t.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.27; import "forge-std/Test.sol"; +import { IHorizonStakingMain } from "../../contracts/interfaces/internal/IHorizonStakingMain.sol"; import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { GraphPayments } from "../../contracts/payments/GraphPayments.sol"; @@ -10,6 +11,87 @@ import { HorizonStakingSharedTest } from "../shared/horizon-staking/HorizonStaki contract GraphPaymentsTest is HorizonStakingSharedTest { + struct CollectPaymentData { + uint256 escrowBalance; + uint256 paymentsBalance; + uint256 receiverBalance; + uint256 delegationPoolBalance; + uint256 dataServiceBalance; + } + + function _collect( + IGraphPayments.PaymentTypes _paymentType, + address _receiver, + uint256 _tokens, + address _dataService, + uint256 _tokensDataService + ) private { + // Previous balances + CollectPaymentData memory previousBalances = CollectPaymentData({ + escrowBalance: token.balanceOf(address(escrow)), + paymentsBalance: token.balanceOf(address(payments)), + receiverBalance: token.balanceOf(_receiver), + delegationPoolBalance: staking.getDelegatedTokensAvailable( + _receiver, + _dataService + ), + dataServiceBalance: token.balanceOf(_dataService) + }); + + // Calculate cuts + uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); + uint256 delegatorCut = staking.getDelegationFeeCut( + _receiver, + _dataService, + _paymentType + ); + uint256 tokensProtocol = _tokens * protocolPaymentCut / MAX_PPM; + uint256 tokensDelegation = _tokens * delegatorCut / MAX_PPM; + + uint256 receiverExpectedPayment = _tokens - _tokensDataService - tokensProtocol - tokensDelegation; + + (,address msgSender, ) = vm.readCallers(); + vm.expectEmit(address(payments)); + emit IGraphPayments.PaymentCollected( + msgSender, + _receiver, + _dataService, + receiverExpectedPayment, + tokensDelegation, + _tokensDataService, + tokensProtocol + ); + payments.collect(_paymentType, _receiver, _tokens, _dataService, _tokensDataService); + + // After balances + CollectPaymentData memory afterBalances = CollectPaymentData({ + escrowBalance: token.balanceOf(address(escrow)), + paymentsBalance: token.balanceOf(address(payments)), + receiverBalance: token.balanceOf(_receiver), + delegationPoolBalance: staking.getDelegatedTokensAvailable( + _receiver, + _dataService + ), + dataServiceBalance: token.balanceOf(_dataService) + }); + + // Check receiver balance after payment + assertEq(afterBalances.receiverBalance - previousBalances.receiverBalance, receiverExpectedPayment); + assertEq(token.balanceOf(address(payments)), 0); + + // Check delegation pool balance after payment + assertEq(afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, tokensDelegation); + + // Check that the escrow account has been updated + assertEq(previousBalances.escrowBalance, afterBalances.escrowBalance + _tokens); + + // Check that payments balance didn't change + assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); + + // Check data service balance after payment + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + } + /* * TESTS */ @@ -28,32 +110,28 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { function testCollect( uint256 amount, - uint256 tokensDataService + uint256 tokensDataService, + uint256 tokensDelegate ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { uint256 tokensProtocol = amount * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegatoion = amount * delegationFeeCut / MAX_PPM; - vm.assume(tokensDataService < amount - tokensProtocol - tokensDelegatoion); + uint256 tokensDelegation = amount * delegationFeeCut / MAX_PPM; + vm.assume(tokensDataService < amount - tokensProtocol - tokensDelegation); address escrowAddress = address(escrow); + // Delegate tokens + vm.assume(tokensDelegate > MIN_DELEGATION); + vm.assume(tokensDelegate <= MAX_STAKING_TOKENS); + vm.startPrank(users.delegator); + _delegate(users.indexer, subgraphDataServiceAddress, tokensDelegate, 0); + // Add tokens in escrow mint(escrowAddress, amount); vm.startPrank(escrowAddress); approve(address(payments), amount); // Collect payments through GraphPayments - uint256 indexerPreviousBalance = token.balanceOf(users.indexer); - payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); + _collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); vm.stopPrank(); - - uint256 indexerBalance = token.balanceOf(users.indexer); - uint256 expectedPayment = amount - tokensDataService - tokensProtocol - tokensDelegatoion; - assertEq(indexerBalance - indexerPreviousBalance, expectedPayment); - - uint256 dataServiceBalance = token.balanceOf(subgraphDataServiceAddress); - assertEq(dataServiceBalance, tokensDataService); - - uint256 delegatorBalance = staking.getDelegatedTokensAvailable(users.indexer, subgraphDataServiceAddress); - assertEq(delegatorBalance, tokensDelegatoion); } function testCollect_RevertWhen_InsufficientAmount( @@ -77,4 +155,45 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { vm.expectRevert(expectedError); payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); } + + function testCollect_RevertWhen_InvalidPool( + uint256 amount + ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + vm.assume(amount > 1 ether); + address escrowAddress = address(escrow); + + // Add tokens in escrow + mint(escrowAddress, amount); + vm.startPrank(escrowAddress); + approve(address(payments), amount); + + // Collect payments through GraphPayments + vm.expectRevert(abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + users.indexer, + subgraphDataServiceAddress + )); + payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, 1); + } + + function testCollect_RevertWhen_InvalidProvision( + uint256 amount + ) public useIndexer useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + vm.assume(amount > 1 ether); + vm.assume(amount <= MAX_STAKING_TOKENS); + address escrowAddress = address(escrow); + + // Add tokens in escrow + mint(escrowAddress, amount); + vm.startPrank(escrowAddress); + approve(address(payments), amount); + + // Collect payments through GraphPayments + vm.expectRevert(abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + users.indexer, + subgraphDataServiceAddress + )); + payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, 1); + } } diff --git a/packages/horizon/test/staking/delegation/addToPool.t.sol b/packages/horizon/test/staking/delegation/addToPool.t.sol index e88becbbf..b9a583ee8 100644 --- a/packages/horizon/test/staking/delegation/addToPool.t.sol +++ b/packages/horizon/test/staking/delegation/addToPool.t.sol @@ -9,6 +9,12 @@ import { HorizonStakingTest } from "../HorizonStaking.t.sol"; contract HorizonStakingDelegationAddToPoolTest is HorizonStakingTest { modifier useValidDelegationAmount(uint256 tokens) { + vm.assume(tokens > MIN_DELEGATION); + vm.assume(tokens <= MAX_STAKING_TOKENS); + _; + } + + modifier useValidAddToPoolAmount(uint256 tokens) { vm.assume(tokens > 0); vm.assume(tokens <= MAX_STAKING_TOKENS); _; @@ -20,36 +26,31 @@ contract HorizonStakingDelegationAddToPoolTest is HorizonStakingTest { function test_Delegation_AddToPool_Verifier( uint256 amount, - uint256 delegationAmount - ) public useIndexer useProvision(amount, 0, 0) useValidDelegationAmount(delegationAmount) { - uint256 stakingPreviousBalance = token.balanceOf(address(staking)); - + uint256 delegationAmount, + uint256 addToPoolAmount + ) public useIndexer useProvision(amount, 0, 0) useValidDelegationAmount(delegationAmount) useValidAddToPoolAmount(addToPoolAmount) { + // Initialize delegation pool + resetPrank(users.delegator); + _delegate(users.indexer, subgraphDataServiceAddress, delegationAmount, 0); + resetPrank(subgraphDataServiceAddress); - mint(subgraphDataServiceAddress, delegationAmount); - token.approve(address(staking), delegationAmount); - _addToDelegationPool(users.indexer, subgraphDataServiceAddress, delegationAmount); - - uint256 delegatedTokens = staking.getDelegatedTokensAvailable(users.indexer, subgraphDataServiceAddress); - assertEq(delegatedTokens, delegationAmount); - assertEq(token.balanceOf(subgraphDataServiceAddress), 0); - assertEq(token.balanceOf(address(staking)), stakingPreviousBalance + delegationAmount); + mint(subgraphDataServiceAddress, addToPoolAmount); + token.approve(address(staking), addToPoolAmount); + _addToDelegationPool(users.indexer, subgraphDataServiceAddress, addToPoolAmount); } function test_Delegation_AddToPool_Payments( uint256 amount, uint256 delegationAmount - ) public useIndexer useProvision(amount, 0, 0) useValidDelegationAmount(delegationAmount) { - uint256 stakingPreviousBalance = token.balanceOf(address(staking)); + ) public useIndexer useProvision(amount, 0, 0) useValidDelegationAmount(delegationAmount) useValidAddToPoolAmount(delegationAmount) { + // Initialize delegation pool + resetPrank(users.delegator); + _delegate(users.indexer, subgraphDataServiceAddress, delegationAmount, 0); resetPrank(address(payments)); mint(address(payments), delegationAmount); token.approve(address(staking), delegationAmount); _addToDelegationPool(users.indexer, subgraphDataServiceAddress, delegationAmount); - - uint256 delegatedTokens = staking.getDelegatedTokensAvailable(users.indexer, subgraphDataServiceAddress); - assertEq(delegatedTokens, delegationAmount); - assertEq(token.balanceOf(subgraphDataServiceAddress), 0); - assertEq(token.balanceOf(address(staking)), stakingPreviousBalance + delegationAmount); } function test_Delegation_AddToPool_RevertWhen_ZeroTokens( @@ -61,5 +62,29 @@ contract HorizonStakingDelegationAddToPoolTest is HorizonStakingTest { staking.addToDelegationPool(users.indexer, subgraphDataServiceAddress, 0); } + function test_Delegation_AddToPool_RevertWhen_PoolHasNoShares( + uint256 amount + ) public useIndexer useProvision(amount, 0, 0) { + vm.startPrank(subgraphDataServiceAddress); + bytes memory expectedError = abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + users.indexer, + subgraphDataServiceAddress + ); + vm.expectRevert(expectedError); + staking.addToDelegationPool(users.indexer, subgraphDataServiceAddress, 1); + } + + function test_Deletaion_AddToPool_RevertWhen_NoProvision() public { + vm.startPrank(subgraphDataServiceAddress); + bytes memory expectedError = abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + users.indexer, + subgraphDataServiceAddress + ); + vm.expectRevert(expectedError); + staking.addToDelegationPool(users.indexer, subgraphDataServiceAddress, 1); + } + // TODO: test recovering an invalid delegation pool } \ No newline at end of file From 3bdaec6ffb23810e8b739f314d695e96659780cc Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Sat, 21 Sep 2024 00:38:15 +1000 Subject: [PATCH 10/13] chore: add stakeToFeesRatio to SubgraphService initialize (#1027) * chore: add stakeToFeesRatio to SubgraphService initialize * fix: revert parameter name --- .../contracts/SubgraphService.sol | 16 +++++-- .../contracts/interfaces/ISubgraphService.sol | 5 +++ .../test/SubgraphBaseTest.t.sol | 6 +-- .../subgraphService/SubgraphService.t.sol | 6 +++ .../governance/stakeToFeesRatio.t.sol | 43 +++++++++++++++++++ 5 files changed, 70 insertions(+), 6 deletions(-) create mode 100644 packages/subgraph-service/test/subgraphService/governance/stakeToFeesRatio.t.sol diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index ce64d9ec0..0b77b277f 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/packages/subgraph-service/contracts/SubgraphService.sol @@ -71,7 +71,11 @@ contract SubgraphService is * @param minimumProvisionTokens The minimum amount of provisioned tokens required to create an allocation * @param maximumDelegationRatio The maximum delegation ratio allowed for an allocation */ - function initialize(uint256 minimumProvisionTokens, uint32 maximumDelegationRatio) external initializer { + function initialize( + uint256 minimumProvisionTokens, + uint32 maximumDelegationRatio, + uint256 stakeToFeesRatio + ) external initializer { __Ownable_init(msg.sender); __DataService_init(); __DataServicePausable_init(); @@ -79,6 +83,7 @@ contract SubgraphService is _setProvisionTokensRange(minimumProvisionTokens, type(uint256).max); _setDelegationRatio(maximumDelegationRatio); + _setStakeToFeesRatio(stakeToFeesRatio); } /** @@ -373,8 +378,7 @@ contract SubgraphService is * @notice See {ISubgraphService.setStakeToFeesRatio} */ function setStakeToFeesRatio(uint256 stakeToFeesRatio_) external override onlyOwner { - stakeToFeesRatio = stakeToFeesRatio_; - emit StakeToFeesRatioSet(stakeToFeesRatio_); + _setStakeToFeesRatio(stakeToFeesRatio_); } /** @@ -558,4 +562,10 @@ contract SubgraphService is emit QueryFeesCollected(indexer, tokensCollected, tokensCurators); return tokensCollected; } + + function _setStakeToFeesRatio(uint256 _stakeToFeesRatio) private { + require(_stakeToFeesRatio != 0, SubgraphServiceInvalidZeroStakeToFeesRatio()); + stakeToFeesRatio = _stakeToFeesRatio; + emit StakeToFeesRatioSet(_stakeToFeesRatio); + } } diff --git a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol index e12554303..b83d672f7 100644 --- a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol +++ b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol @@ -122,6 +122,11 @@ interface ISubgraphService is IDataServiceFees { */ error SubgraphServiceAllocationIsAltruistic(address allocationId); + /** + * @notice Thrown when trying to set stake to fees ratio to zero + */ + error SubgraphServiceInvalidZeroStakeToFeesRatio(); + /** * @notice Close a stale allocation * @dev This function can be permissionlessly called when the allocation is stale. diff --git a/packages/subgraph-service/test/SubgraphBaseTest.t.sol b/packages/subgraph-service/test/SubgraphBaseTest.t.sol index d42b66e29..f1aa9b96c 100644 --- a/packages/subgraph-service/test/SubgraphBaseTest.t.sol +++ b/packages/subgraph-service/test/SubgraphBaseTest.t.sol @@ -151,7 +151,7 @@ abstract contract SubgraphBaseTest is Utils, Constants { address subgraphServiceProxy = UnsafeUpgrades.deployTransparentProxy( subgraphServiceImplementation, users.governor, - abi.encodeCall(SubgraphService.initialize, (minimumProvisionTokens, delegationRatio)) + abi.encodeCall(SubgraphService.initialize, (minimumProvisionTokens, delegationRatio, stakeToFeesRatio)) ); subgraphService = SubgraphService(subgraphServiceProxy); @@ -183,10 +183,10 @@ abstract contract SubgraphBaseTest is Utils, Constants { } function setupProtocol() private { + resetPrank(users.deployer); + subgraphService.transferOwnership(users.governor); resetPrank(users.governor); staking.setMaxThawingPeriod(MAX_THAWING_PERIOD); - resetPrank(users.deployer); - subgraphService.setStakeToFeesRatio(stakeToFeesRatio); subgraphService.setMaxPOIStaleness(maxPOIStaleness); subgraphService.setCurationCut(curationCut); } diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index b5b7f744e..91c486a7b 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -31,6 +31,12 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { * MODIFIERS */ + modifier useGovernor() { + vm.startPrank(users.governor); + _; + vm.stopPrank(); + } + modifier useOperator() { resetPrank(users.indexer); staking.setOperator(users.operator, address(subgraphService), true); diff --git a/packages/subgraph-service/test/subgraphService/governance/stakeToFeesRatio.t.sol b/packages/subgraph-service/test/subgraphService/governance/stakeToFeesRatio.t.sol new file mode 100644 index 000000000..bfd09073e --- /dev/null +++ b/packages/subgraph-service/test/subgraphService/governance/stakeToFeesRatio.t.sol @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.27; + +import "forge-std/Test.sol"; + +import { ISubgraphService } from "../../../contracts/interfaces/ISubgraphService.sol"; +import { SubgraphServiceTest } from "../SubgraphService.t.sol"; +import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; + +contract DisputeManagerGovernanceArbitratorTest is SubgraphServiceTest { + + /** + * ACTIONS + */ + + function _setStakeToFeesRatio(uint256 _stakeToFeesRatio) internal { + vm.expectEmit(address(subgraphService)); + emit ISubgraphService.StakeToFeesRatioSet(_stakeToFeesRatio); + subgraphService.setStakeToFeesRatio(_stakeToFeesRatio); + assertEq(subgraphService.stakeToFeesRatio(), _stakeToFeesRatio); + } + + /* + * TESTS + */ + + function test_Governance_SetStakeToFeesRatio(uint256 stakeToFeesRatio) public useGovernor { + vm.assume(stakeToFeesRatio > 0); + _setStakeToFeesRatio(stakeToFeesRatio); + } + + function test_Governance_RevertWhen_ZeroValue() public useGovernor { + uint256 stakeToFeesRatio = 0; + vm.expectRevert(abi.encodeWithSelector(ISubgraphService.SubgraphServiceInvalidZeroStakeToFeesRatio.selector)); + subgraphService.setStakeToFeesRatio(stakeToFeesRatio); + } + + function test_Governance_RevertWhen_NotGovernor() public useIndexer { + uint256 stakeToFeesRatio = 2; + vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, users.indexer)); + subgraphService.setStakeToFeesRatio(stakeToFeesRatio); + } +} From 51dd7a8f22b359fa8aaf3820916ca7c40708bd5b Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Sat, 21 Sep 2024 00:38:51 +1000 Subject: [PATCH 11/13] chore(SubgraphService): add Multicall to SubgraphService (#1030) * chore: add Multicall to SubgraphService * fix: add multicall init --- packages/subgraph-service/contracts/SubgraphService.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index 0b77b277f..e50762453 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/packages/subgraph-service/contracts/SubgraphService.sol @@ -8,6 +8,7 @@ import { IRewardsIssuer } from "@graphprotocol/contracts/contracts/rewards/IRewa import { ISubgraphService } from "./interfaces/ISubgraphService.sol"; import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import { MulticallUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/MulticallUpgradeable.sol"; import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import { DataServicePausableUpgradeable } from "@graphprotocol/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol"; import { DataService } from "@graphprotocol/horizon/contracts/data-service/DataService.sol"; @@ -24,6 +25,7 @@ import { LegacyAllocation } from "./libraries/LegacyAllocation.sol"; contract SubgraphService is Initializable, OwnableUpgradeable, + MulticallUpgradeable, DataService, DataServicePausableUpgradeable, DataServiceFees, @@ -77,6 +79,7 @@ contract SubgraphService is uint256 stakeToFeesRatio ) external initializer { __Ownable_init(msg.sender); + __Multicall_init(); __DataService_init(); __DataServicePausable_init(); __AllocationManager_init("SubgraphService", "1.0"); From 08539afc31fd3198d8ec7eea716cc935042e5369 Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Sat, 21 Sep 2024 00:39:05 +1000 Subject: [PATCH 12/13] fix(SubgraphService): allow collecting rewards when delegation fee cut is 100% (#1038) --- .../contracts/utilities/AllocationManager.sol | 14 ++++++++------ .../collect/indexing/indexing.t.sol | 7 ++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index c555f271c..5bb22cbe2 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -303,12 +303,14 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca // Distribute rewards to indexer tokensIndexerRewards = tokensRewards - tokensDelegationRewards; - address rewardsDestination = rewardsDestination[allocation.indexer]; - if (rewardsDestination == address(0)) { - _graphToken().approve(address(_graphStaking()), tokensIndexerRewards); - _graphStaking().stakeToProvision(allocation.indexer, address(this), tokensIndexerRewards); - } else { - _graphToken().pushTokens(rewardsDestination, tokensIndexerRewards); + if (tokensIndexerRewards > 0) { + address rewardsDestination = rewardsDestination[allocation.indexer]; + if (rewardsDestination == address(0)) { + _graphToken().approve(address(_graphStaking()), tokensIndexerRewards); + _graphStaking().stakeToProvision(allocation.indexer, address(this), tokensIndexerRewards); + } else { + _graphToken().pushTokens(rewardsDestination, tokensIndexerRewards); + } } } 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 bff35f574..38e9e7865 100644 --- a/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol +++ b/packages/subgraph-service/test/subgraphService/collect/indexing/indexing.t.sol @@ -27,8 +27,7 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { uint256 delegationTokens, uint256 delegationFeeCut ) public useIndexer useAllocation(tokens) useDelegation(delegationTokens) { - // Collect reverts if delegationFeeCut is 100% - delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM - 1); + delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM); _setDelegationFeeCut( users.indexer, address(subgraphService), @@ -45,7 +44,6 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { uint256 delegationTokens, uint256 delegationFeeCut ) public useIndexer useAllocation(tokens) useDelegation(delegationTokens) { - // Collect reverts if delegationFeeCut is 100% delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM); _setDelegationFeeCut( users.indexer, @@ -90,8 +88,7 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { uint256 delegationTokens, uint256 delegationFeeCut ) public useIndexer useAllocation(tokens) useDelegation(delegationTokens) { - // Collect reverts if delegationFeeCut is 100% - delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM - 1); + delegationFeeCut = bound(delegationFeeCut, 0, MAX_PPM); _setDelegationFeeCut( users.indexer, address(subgraphService), From 3e57ba31739a14ed594cd9ca0b86b1a931605adc Mon Sep 17 00:00:00 2001 From: Maikol <86025070+Maikol@users.noreply.github.com> Date: Sat, 21 Sep 2024 02:22:05 +1000 Subject: [PATCH 13/13] =?UTF-8?q?chore(SubgraphService):=20add=20current?= =?UTF-8?q?=20epoch=20to=20IndexingRewardsCollected=E2=80=A6=20(#1036)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(SubgraphService): add current epoch to IndexingRewardsCollected event * fix: add missing parameter to natspec --- .../contracts/utilities/AllocationManager.sol | 7 ++- .../test/SubgraphBaseTest.t.sol | 6 +- .../test/mocks/MockEpochManager.sol | 63 +++++++++++++++++++ .../subgraphService/SubgraphService.t.sol | 3 +- .../subgraph-service/test/utils/Constants.sol | 3 +- 5 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 packages/subgraph-service/test/mocks/MockEpochManager.sol diff --git a/packages/subgraph-service/contracts/utilities/AllocationManager.sol b/packages/subgraph-service/contracts/utilities/AllocationManager.sol index 5bb22cbe2..4ab5fd418 100644 --- a/packages/subgraph-service/contracts/utilities/AllocationManager.sol +++ b/packages/subgraph-service/contracts/utilities/AllocationManager.sol @@ -57,6 +57,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca * @param tokensIndexerRewards The amount of tokens collected for the indexer * @param tokensDelegationRewards The amount of tokens collected for delegators * @param poi The POI presented + * @param currentEpoch The current epoch */ event IndexingRewardsCollected( address indexed indexer, @@ -65,7 +66,8 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca uint256 tokensRewards, uint256 tokensIndexerRewards, uint256 tokensDelegationRewards, - bytes32 poi + bytes32 poi, + uint256 currentEpoch ); /** @@ -321,7 +323,8 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca tokensRewards, tokensIndexerRewards, tokensDelegationRewards, - _poi + _poi, + _graphEpochManager().currentEpoch() ); // Check if the indexer is over-allocated and close the allocation if necessary diff --git a/packages/subgraph-service/test/SubgraphBaseTest.t.sol b/packages/subgraph-service/test/SubgraphBaseTest.t.sol index f1aa9b96c..6ab0c6d24 100644 --- a/packages/subgraph-service/test/SubgraphBaseTest.t.sol +++ b/packages/subgraph-service/test/SubgraphBaseTest.t.sol @@ -26,6 +26,7 @@ import { Utils } from "./utils/Utils.sol"; import { MockCuration } from "./mocks/MockCuration.sol"; import { MockGRTToken } from "./mocks/MockGRTToken.sol"; import { MockRewardsManager } from "./mocks/MockRewardsManager.sol"; +import { MockEpochManager } from "./mocks/MockEpochManager.sol"; abstract contract SubgraphBaseTest is Utils, Constants { @@ -50,6 +51,7 @@ abstract contract SubgraphBaseTest is Utils, Constants { MockCuration curation; MockGRTToken token; MockRewardsManager rewardsManager; + MockEpochManager epochManager; /* Users */ @@ -91,6 +93,7 @@ abstract contract SubgraphBaseTest is Utils, Constants { GraphProxy stakingProxy = new GraphProxy(address(0), address(proxyAdmin)); rewardsManager = new MockRewardsManager(token, rewardsPerSignal, rewardsPerSubgraphAllocationUpdate); curation = new MockCuration(); + epochManager = new MockEpochManager(); // GraphPayments predict address bytes32 saltGraphPayments = keccak256("GraphPaymentsSalt"); @@ -126,7 +129,7 @@ abstract contract SubgraphBaseTest is Utils, Constants { controller.setContractProxy(keccak256("RewardsManager"), address(rewardsManager)); controller.setContractProxy(keccak256("GraphPayments"), predictedGraphPaymentsAddress); controller.setContractProxy(keccak256("PaymentsEscrow"), predictedEscrowAddress); - controller.setContractProxy(keccak256("EpochManager"), makeAddr("EpochManager")); + controller.setContractProxy(keccak256("EpochManager"), address(epochManager)); controller.setContractProxy(keccak256("GraphTokenGateway"), makeAddr("GraphTokenGateway")); controller.setContractProxy(keccak256("GraphProxyAdmin"), makeAddr("GraphProxyAdmin")); controller.setContractProxy(keccak256("Curation"), address(curation)); @@ -187,6 +190,7 @@ abstract contract SubgraphBaseTest is Utils, Constants { subgraphService.transferOwnership(users.governor); resetPrank(users.governor); staking.setMaxThawingPeriod(MAX_THAWING_PERIOD); + epochManager.setEpochLength(EPOCH_LENGTH); subgraphService.setMaxPOIStaleness(maxPOIStaleness); subgraphService.setCurationCut(curationCut); } diff --git a/packages/subgraph-service/test/mocks/MockEpochManager.sol b/packages/subgraph-service/test/mocks/MockEpochManager.sol new file mode 100644 index 000000000..060a92e21 --- /dev/null +++ b/packages/subgraph-service/test/mocks/MockEpochManager.sol @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +pragma solidity 0.8.27; + +import { IEpochManager } from "@graphprotocol/contracts/contracts/epochs/IEpochManager.sol"; + +contract MockEpochManager is IEpochManager { + // -- Variables -- + + uint256 public epochLength; + uint256 public lastRunEpoch; + uint256 public lastLengthUpdateEpoch; + uint256 public lastLengthUpdateBlock; + + // -- Configuration -- + + function setEpochLength(uint256 _epochLength) public { + lastLengthUpdateEpoch = 1; + lastLengthUpdateBlock = blockNum(); + epochLength = _epochLength; + } + + // -- Epochs + + function runEpoch() public { + lastRunEpoch = currentEpoch(); + } + + // -- Getters -- + + function isCurrentEpochRun() public view returns (bool) { + return lastRunEpoch == currentEpoch(); + } + + function blockNum() public view returns (uint256) { + return block.number; + } + + function blockHash(uint256 _block) public view returns (bytes32) { + return blockhash(_block); + } + + function currentEpoch() public view returns (uint256) { + return lastLengthUpdateEpoch + epochsSinceUpdate(); + } + + function currentEpochBlock() public view returns (uint256) { + return lastLengthUpdateBlock + (epochsSinceUpdate() * epochLength); + } + + function currentEpochBlockSinceStart() public view returns (uint256) { + return blockNum() - currentEpochBlock(); + } + + function epochsSince(uint256 _epoch) public view returns (uint256) { + uint256 epoch = currentEpoch(); + return _epoch < epoch ? (epoch - _epoch) : 0; + } + + function epochsSinceUpdate() public view returns (uint256) { + return (blockNum() - lastLengthUpdateBlock) / epochLength; + } +} diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index 91c486a7b..e62ce8211 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -273,7 +273,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { paymentCollected, indexingRewardsData.tokensIndexerRewards, indexingRewardsData.tokensDelegationRewards, - indexingRewardsData.poi + indexingRewardsData.poi, + epochManager.currentEpoch() ); } diff --git a/packages/subgraph-service/test/utils/Constants.sol b/packages/subgraph-service/test/utils/Constants.sol index 1dbfd082e..e70ca8b41 100644 --- a/packages/subgraph-service/test/utils/Constants.sol +++ b/packages/subgraph-service/test/utils/Constants.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.27; abstract contract Constants { uint256 internal constant MAX_TOKENS = 10_000_000_000 ether; uint256 internal constant MAX_PPM = 1_000_000; + uint256 internal constant EPOCH_LENGTH = 1; // Dispute Manager uint64 internal constant disputePeriod = 300; // 5 minutes uint256 internal constant disputeDeposit = 100 ether; // 100 GRT @@ -26,4 +27,4 @@ abstract contract Constants { // RewardsMananger parameters uint256 public constant rewardsPerSignal = 10000; uint256 public constant rewardsPerSubgraphAllocationUpdate = 1000; -} \ No newline at end of file +}