-
Notifications
You must be signed in to change notification settings - Fork 157
Set indexer rewards destination using rewards pool #451
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
Conversation
I'm posting new data based on new benchmarks as gas usage will depend if sending the token to an address with zero balance and also if the rewards pool is zero or not. SSTORE take 20k when empty and 5k when non-empty.
The difference is smaller between the two balance / no balance versions. Gas usage on one or the other implementation will depend very much on how the indexer behaves (the withdrawal flow and frequency they use) |
I think its worth it to ask the community what they plan to do. In this implementation, the indexer will have to call an additional function For now at least, I think the simpler implementaiton in #450 is sufficient, and then we can add the pool later if people are concerned over saving 4% in gas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a few small comments
* @dev Withdraw accrued rewards. | ||
* @param _beneficiary Address to send rewards | ||
*/ | ||
function withdrawRewards(address _beneficiary) external override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this setup - we make them withdraw all tokens in the pool at once.
I am thinking out loud - I think it is fine, tokens sitting in the rewards pool are doing nothing, they are not earning rewrds and they are in the staking contract. I don't see a reason why someone would want to withdraw partially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also to note, we let them withdraw to any address by providing a beneficiary address here. Maybe do we want to have require( _beneficiary != address(0))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very good suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be here "read" fucntion to see How many GRT is in the rewards pool available to withdraw? It would help Indexer to decide whether it is worthwhile to withdraw or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapping(address => uint256) public rewardsPool; |
public
// Check indexer balance remains the same | ||
expect(afterIndexer1Balance).eq(beforeIndexer1Balance) | ||
// Check indexing rewards are kept in the staking contract | ||
expect(toRound(afterStakingBalance)).eq( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could also check to see their rewards pool balance went up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add that test
const beforeIndexer1Balance = await grt.balanceOf(indexer1.address) | ||
const beforeStakingBalance = await grt.balanceOf(staking.address) | ||
|
||
const expectedIndexingRewards = toGRT('1471954234') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was calculated on a spread sheet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we calculated all rewards tests agains a spreedsheet with the formulas, based on issuance rate and blocks, we could make the spreadsheet public and link
That's a great Idea! |
Archive as an alternative research solution |
Related to #450
The main differences is that it accrues any rewards on the stake or rebate pool depending on how rewards destination is set.
This version is about 4% more gas efficient on every close allocation call and is slightly more efficient when calling claim() without restake as it behaves the same way as close allocation. The reason for that is that transferring a token means doing two SSTORE, one to change the balance of the sender and another one for the receiver, additionally a CALL/SLOAD to get the graph token address. Instead it just use one SLOAD and one SSTORE to update the rewards pool.
Discussed in: https://forum.thegraph.com/t/rewards-destination-for-indexers-indexing-rewards/1279/64