-
Notifications
You must be signed in to change notification settings - Fork 158
staking: channel allocation and settlement #235
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
8d2a291
to
f087523
Compare
contracts/Staking.sol
Outdated
|
||
// TODO: find a rebate pool for the epoch and accumulate stuff there | ||
// TODO: review if we can replace Settlement with Allocation | ||
// TODO: can we unallocate two Allocations on the same epoch - rebate? |
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.
If we do support multiple allocations per Indexer + subgraph, then I think it's okay to have multiple settlements on the same epoch.
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.
Looks pretty good, some small comments and changes
|
||
// When all settlements processed then prune rebate pool | ||
if (pool.settlementsCount == 0) { | ||
delete rebates[_epoch]; | ||
delete rebates[alloc.settledAtEpoch]; |
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 like the new way we record and store allocation amounts. it seems much less complex.
I am thinking maybe we should implement some asserts()
somewhere, in a different PR.
Just so we can avoid sceanarios where , maybe there is a bug, and someone can withdraw more than they should be able to. and we know the token allocations or fees we are recording should never go below 0, or it shouldnt hit 0 unless settlement counts is also 0.
I am thinking we could make a linear task, and do this in the future
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.
Yes, let's do that.
alloc.collectedFees = 0; | ||
alloc.effectiveAllocation = 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.
I wonder if @Zerim wants to keep either of these, possibly for time-weighted calculations , for indexer reputation?
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.
are we setting these to 0 to save space? Does this get a gas refund?
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'm setting those to zero to unallocate that storage, as a good Ethereum citizen. I was thinking to selectively leave information we need later, maybe when we review the indexing rewards. For all the rest we have the subgraph :)
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.
lets leave it as is for now, and we can add it back in if needed
Changes summary: https://www.notion.so/thegraph/25JUN2020-Changes-to-how-allocation-and-settlement-work-e02cf3bc164b4864854a681acd7abaab