Skip to content

fix: change pending rewards due to resize to token amounts instead of per token (OZ H-01) #993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/contracts/contracts/rewards/IRewardsIssuer.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,20 @@ interface IRewardsIssuer {
* @return subgraphDeploymentId Subgraph deployment id for the allocation
* @return tokens Amount of allocated tokens
* @return accRewardsPerAllocatedToken Rewards snapshot
* @return accRewardsPending Tokens pending to be claimed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the description here isn't super clear. Maybe something like "Snapshot of accumulated rewards from previous allocation resizing, pending to be claimed"?

*/
function getAllocationData(
address allocationId
)
external
view
returns (address indexer, bytes32 subgraphDeploymentId, uint256 tokens, uint256 accRewardsPerAllocatedToken);
returns (
address indexer,
bytes32 subgraphDeploymentId,
uint256 tokens,
uint256 accRewardsPerAllocatedToken,
uint256 accRewardsPending
);

/**
* @notice Return the total amount of tokens allocated to subgraph.
Expand Down
2 changes: 2 additions & 0 deletions packages/contracts/contracts/rewards/IRewardsManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ interface IRewardsManager {

function getRewards(address _allocationID) external view returns (uint256);

function calcRewards(uint256 _tokens, uint256 _accRewardsPerAllocatedToken) external pure returns (uint256);

// -- Updates --

function updateAccRewardsPerSignal() external returns (uint256);
Expand Down
33 changes: 27 additions & 6 deletions packages/contracts/contracts/rewards/RewardsManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -357,12 +357,30 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
return 0;
}

(, bytes32 subgraphDeploymentId, uint256 tokens, uint256 alloAccRewardsPerAllocatedToken) = IRewardsIssuer(
rewardsIssuer
).getAllocationData(_allocationID);
(
,
bytes32 subgraphDeploymentId,
uint256 tokens,
uint256 alloAccRewardsPerAllocatedToken,
uint256 accRewardsPending
) = IRewardsIssuer(rewardsIssuer).getAllocationData(_allocationID);

(uint256 accRewardsPerAllocatedToken, ) = getAccRewardsPerAllocatedToken(subgraphDeploymentId);
return _calcRewards(tokens, alloAccRewardsPerAllocatedToken, accRewardsPerAllocatedToken);
return
accRewardsPending.add(_calcRewards(tokens, alloAccRewardsPerAllocatedToken, accRewardsPerAllocatedToken));
}

/**
* @dev Calculate rewards for a given accumulated rewards per allocated token.
* @param _tokens Tokens allocated
* @param _accRewardsPerAllocatedToken Allocation accumulated rewards per token
* @return Rewards amount
*/
function calcRewards(
uint256 _tokens,
uint256 _accRewardsPerAllocatedToken
) external pure override returns (uint256) {
return _accRewardsPerAllocatedToken.mul(_tokens).div(FIXED_POINT_SCALING_FACTOR);
}

/**
Expand Down Expand Up @@ -400,7 +418,8 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
address indexer,
bytes32 subgraphDeploymentID,
uint256 tokens,
uint256 accRewardsPerAllocatedToken
uint256 accRewardsPerAllocatedToken,
uint256 accRewardsPending
) = IRewardsIssuer(rewardsIssuer).getAllocationData(_allocationID);

uint256 updatedAccRewardsPerAllocatedToken = onSubgraphAllocationUpdate(subgraphDeploymentID);
Expand All @@ -412,7 +431,9 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
}

// Calculate rewards accrued by this allocation
uint256 rewards = _calcRewards(tokens, accRewardsPerAllocatedToken, updatedAccRewardsPerAllocatedToken);
uint256 rewards = accRewardsPending.add(
_calcRewards(tokens, accRewardsPerAllocatedToken, updatedAccRewardsPerAllocatedToken)
);
if (rewards > 0) {
// Mint directly to rewards issuer for the reward amount
// The rewards issuer contract will do bookkeeping of the reward and
Expand Down
4 changes: 3 additions & 1 deletion packages/contracts/contracts/staking/IStakingBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,9 @@ interface IStakingBase is IStakingData {
* @dev Note that this is only to make tests pass, as the staking contract with
* this changes will never get deployed. HorizonStaking is taking it's place.
*/
function getAllocationData(address _allocationID) external view returns (address, bytes32, uint256, uint256);
function getAllocationData(
address _allocationID
) external view returns (address, bytes32, uint256, uint256, uint256);

/**
* @dev New function to get the allocation active status for the rewards manager
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts/contracts/staking/Staking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -479,9 +479,9 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
*/
function getAllocationData(
address _allocationID
) external view override returns (address, bytes32, uint256, uint256) {
) external view override returns (address, bytes32, uint256, uint256, uint256) {
Allocation memory alloc = __allocations[_allocationID];
return (alloc.indexer, alloc.subgraphDeploymentID, alloc.tokens, alloc.accRewardsPerAllocatedToken);
return (alloc.indexer, alloc.subgraphDeploymentID, alloc.tokens, alloc.accRewardsPerAllocatedToken, 0);
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/contracts/eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ module.exports = [
'@typescript-eslint/no-unsafe-call': 'off',
'@typescript-eslint/no-unsafe-member-access': 'off',
'@typescript-eslint/no-unsafe-argument': 'off',
'@typescript-eslint/no-unsafe-return': 'off',
'@typescript-eslint/no-redundant-type-constituents': 'off',
},
},
{
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
},
"devDependencies": {
"@arbitrum/sdk": "~3.1.13",
"@defi-wonderland/smock": "^2.0.7",
"@defi-wonderland/smock": "~2.3.0",
"@ethersproject/experimental": "^5.6.0",
"@graphprotocol/common-ts": "^1.8.3",
"@nomiclabs/hardhat-ethers": "^2.2.3",
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts/test/unit/lib/graphTokenTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export function grtTests(isL2: boolean): void {
return permit
}

async function createPermitTransaction(permit: Permit, signer: string, salt: string) {
function createPermitTransaction(permit: Permit, signer: string, salt: string) {
const signature: Signature = signPermit(signer, graph.chainId, grt.address, permit, salt)
const wallet = new ethers.Wallet(signer, graph.provider)
return grt
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,14 +238,15 @@ contract HorizonStakingExtension is HorizonStakingBase, IL2StakingBase, IHorizon
* @notice Return allocation data by ID.
* @dev To be called by the Rewards Manager to calculate rewards issuance.
* @dev TODO: Remove after Horizon transition period
* @dev Note that the accRewardsPending field is not used and will always be zero.
* @param allocationID Address used as allocation identifier
* @return Allocation data
*/
function getAllocationData(
address allocationID
) external view override returns (address, bytes32, uint256, uint256) {
) external view override returns (address, bytes32, uint256, uint256, uint256) {
Allocation memory allo = __DEPRECATED_allocations[allocationID];
return (allo.indexer, allo.subgraphDeploymentID, allo.tokens, allo.accRewardsPerAllocatedToken);
return (allo.indexer, allo.subgraphDeploymentID, allo.tokens, allo.accRewardsPerAllocatedToken, 0);
}

/**
Expand Down
16 changes: 9 additions & 7 deletions packages/horizon/test/data-service/DataService.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -254,13 +254,15 @@ contract DataServiceTest is HorizonStakingSharedTest {
staking.setProvisionParameters(users.indexer, address(dataService), maxVerifierCut, thawingPeriod);

// accept provision parameters
vm.expectEmit();
emit IHorizonStakingMain.ProvisionParametersSet(
users.indexer,
address(dataService),
maxVerifierCut,
thawingPeriod
);
if (maxVerifierCut != dataService.VERIFIER_CUT_MIN() || thawingPeriod != dataService.THAWING_PERIOD_MIN()) {
vm.expectEmit();
emit IHorizonStakingMain.ProvisionParametersSet(
users.indexer,
address(dataService),
maxVerifierCut,
thawingPeriod
);
}
dataService.acceptProvisionParameters(users.indexer);
}

Expand Down
3 changes: 2 additions & 1 deletion packages/horizon/test/staking/allocation/allocation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ contract HorizonStakingAllocationTest is HorizonStakingExtensionTest {
}

function testAllocation_GetAllocationData() public useAllocation {
(address indexer, bytes32 subgraphDeploymentID, uint256 tokens, uint256 accRewardsPerAllocatedToken) =
(address indexer, bytes32 subgraphDeploymentID, uint256 tokens, uint256 accRewardsPerAllocatedToken, uint256 accRewardsPending) =
staking.getAllocationData(_allocationId);
assertEq(indexer, _allocation.indexer);
assertEq(subgraphDeploymentID, _allocation.subgraphDeploymentID);
assertEq(tokens, _allocation.tokens);
assertEq(accRewardsPerAllocatedToken, _allocation.accRewardsPerAllocatedToken);
assertEq(accRewardsPending, 0);
}

function testAllocation_GetAllocationState_Active() public useAllocation {
Expand Down
19 changes: 15 additions & 4 deletions packages/subgraph-service/contracts/SubgraphService.sol
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,10 @@ contract SubgraphService is
bytes calldata data
) external override onlyProvisionAuthorized(indexer) onlyRegisteredIndexer(indexer) whenNotPaused {
address allocationId = abi.decode(data, (address));
require(allocations[allocationId].indexer == indexer, SubgraphServiceAllocationNotAuthorized(indexer, allocationId));
require(
allocations[allocationId].indexer == indexer,
SubgraphServiceAllocationNotAuthorized(indexer, allocationId)
);
_closeAllocation(allocationId);
emit ServiceStopped(indexer, data);
}
Expand Down Expand Up @@ -251,7 +254,14 @@ contract SubgraphService is
address indexer,
IGraphPayments.PaymentTypes paymentType,
bytes calldata data
) external override onlyProvisionAuthorized(indexer) onlyValidProvision(indexer) onlyRegisteredIndexer(indexer) whenNotPaused {
)
external
override
onlyProvisionAuthorized(indexer)
onlyValidProvision(indexer)
onlyRegisteredIndexer(indexer)
whenNotPaused
{
uint256 paymentCollected = 0;

if (paymentType == IGraphPayments.PaymentTypes.QueryFee) {
Expand Down Expand Up @@ -391,13 +401,14 @@ contract SubgraphService is
*/
function getAllocationData(
address allocationId
) external view override returns (address, bytes32, uint256, uint256) {
) external view override returns (address, bytes32, uint256, uint256, uint256) {
Allocation.State memory allo = allocations[allocationId];
return (
allo.indexer,
allo.subgraphDeploymentId,
allo.tokens,
allo.accRewardsPerAllocatedToken + allo.accRewardsPending
allo.accRewardsPerAllocatedToken,
allo.accRewardsPending
);
}

Expand Down
3 changes: 1 addition & 2 deletions packages/subgraph-service/contracts/libraries/Allocation.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ library Allocation {
uint256 lastPOIPresentedAt;
// Accumulated rewards per allocated token
uint256 accRewardsPerAllocatedToken;
// Accumulated rewards per allocated token that are pending to be claimed
// due to an allocation resize
// Accumulated rewards that are pending to be claimed due allocation resize
uint256 accRewardsPending;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +374,17 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
uint256 accRewardsPerAllocatedToken = _graphRewardsManager().onSubgraphAllocationUpdate(
allocation.subgraphDeploymentId
);
uint256 accRewardsPending = !allocation.isAltruistic()
uint256 accRewardsPerAllocatedTokenPending = !allocation.isAltruistic()
? accRewardsPerAllocatedToken - allocation.accRewardsPerAllocatedToken
: 0;

// Update the allocation
allocations[_allocationId].tokens = _tokens;
allocations[_allocationId].accRewardsPerAllocatedToken = accRewardsPerAllocatedToken;
allocations[_allocationId].accRewardsPending += accRewardsPending;
allocations[_allocationId].accRewardsPending += _graphRewardsManager().calcRewards(
oldTokens,
accRewardsPerAllocatedTokenPending
);

// Update total allocated tokens for the subgraph deployment
if (_tokens > oldTokens) {
Expand Down
2 changes: 2 additions & 0 deletions packages/subgraph-service/test/mocks/MockRewardsManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ contract MockRewardsManager is IRewardsManager {

function getRewards(address) external view returns (uint256) {}

function calcRewards(uint256, uint256) external pure returns (uint256) {}

// -- Updates --

function updateAccRewardsPerSignal() external returns (uint256) {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,46 @@ contract SubgraphServiceAllocateResizeTest is SubgraphServiceTest {
*/

function _setupResize(address _indexer, uint256 _tokens) private {

token.approve(address(staking), _tokens);
staking.stakeTo(_indexer, _tokens);
staking.addToProvision(_indexer, address(subgraphService), _tokens);
}

function _resizeAllocation(address _indexer, address _allocationID, bytes32 _subgraphDeployment, uint256 _tokens) private {
uint256 oldAllocatedTokens = subgraphService.getSubgraphAllocatedTokens(_subgraphDeployment);
// before
uint256 beforeSubgraphAllocatedTokens = subgraphService.getSubgraphAllocatedTokens(_subgraphDeployment);
uint256 beforeAllocatedTokens = subgraphService.allocationProvisionTracker(_indexer);
Allocation.State memory beforeAllocation = subgraphService.getAllocation(_allocationID);

uint256 allocatedTokensDelta;
if (_tokens > beforeAllocation.tokens) {
allocatedTokensDelta = _tokens - beforeAllocation.tokens;
} else {
allocatedTokensDelta = beforeAllocation.tokens - _tokens;
}

// resize
vm.expectEmit(address(subgraphService));
emit AllocationManager.AllocationResized(_indexer, _allocationID, _subgraphDeployment, _tokens, oldAllocatedTokens);
emit AllocationManager.AllocationResized(_indexer, _allocationID, _subgraphDeployment, _tokens, beforeSubgraphAllocatedTokens);
subgraphService.resizeAllocation(_indexer, _allocationID, _tokens);

Allocation.State memory allocation = subgraphService.getAllocation(_allocationID);
assertEq(allocation.tokens, _tokens);
assertEq(allocation.accRewardsPerAllocatedToken, rewardsPerSubgraphAllocationUpdate);

uint256 subgraphAllocatedTokens = subgraphService.getSubgraphAllocatedTokens(_subgraphDeployment);
assertEq(subgraphAllocatedTokens, _tokens);
// after
uint256 afterSubgraphAllocatedTokens = subgraphService.getSubgraphAllocatedTokens(_subgraphDeployment);
uint256 afterAllocatedTokens = subgraphService.allocationProvisionTracker(_indexer);
Allocation.State memory afterAllocation = subgraphService.getAllocation(_allocationID);
uint256 accRewardsPerAllocatedTokenDelta = afterAllocation.accRewardsPerAllocatedToken - beforeAllocation.accRewardsPerAllocatedToken;
uint256 afterAccRewardsPending = rewardsManager.calcRewards(beforeAllocation.tokens, accRewardsPerAllocatedTokenDelta);

//assert
if (_tokens > beforeAllocation.tokens) {
assertEq(afterAllocatedTokens, beforeAllocatedTokens + allocatedTokensDelta);
} else {
assertEq(afterAllocatedTokens, beforeAllocatedTokens - allocatedTokensDelta);
}
assertEq(afterAllocation.tokens, _tokens);
assertEq(afterAllocation.accRewardsPerAllocatedToken, rewardsPerSubgraphAllocationUpdate);
assertEq(afterAllocation.accRewardsPending, afterAccRewardsPending);
assertEq(afterSubgraphAllocatedTokens, _tokens);
}

/*
Expand Down
Loading
Loading