graphprotocol / contracts

Contracts repository for The Graph protocol
https://thegraph.com
GNU General Public License v2.0
327 stars 143 forks source link

Emit `restake` in rebateClaimed #206

Closed davekaj closed 4 years ago

davekaj commented 4 years ago

https://github.com/graphprotocol/contracts/blob/67dc99f19bb8ffdb22bbb241d4a9becb4073cda4/contracts/Staking.sol#L483

can we add if restake is true or false to the event? Otherwise, we can't decrement graphNetwork.totalStakedGRT

@abarmat

abarmat commented 4 years ago

Dave, I think it might not be necessary. You should always decrement from RebateClaimed. When _restake is true, it then calls _settle() which should emit the emit StakeDeposited(_indexer, _tokens); so the effect will be neutral.

davekaj commented 4 years ago

You are right, but there is more to the story that I see after your comment.

Let me lay out how it works, so you can see my conclusion:

I thought we would need to decrement. But at this point, no because these tokens were never included in total stake, we don't need to. And If they do get restaked, like you said, they will be caught in StakeDeposited.

So.... all is good. BUT. I am thinking now, maybe we want the following values on GraphNetwork:

totalGRTStaked
totalGRTLocked
totalGRTAllocated
totalGRTSettling

And maybe even add to Indexer a field called tokensSettling

Wdyt?

abarmat commented 4 years ago

Yes, good observation. I'm not tracking that explicitly in the contract as an accumulated variable, but definitely there is a concept of totalGRTSettled that is waiting in the rebate pool for the particular epochs. Basically it's the sum of all outstanding Pool.fees

    struct Pool {
        uint256 fees;
        uint256 allocation;
        uint256 settlementsCount;
        // Settlements in this pool : indexer => subgraphID => Settlement
        mapping(address => mapping(bytes32 => Settlement)) settlements;
    }

To have a total account of the tokens in each state, I agree, we can add totalGRTSettled that increments on settle() and decrements on claim().

Maybe the name is not the best as someone could think is the total settled ever instead of the outstanding amount to be used for rebate rewards.

davekaj commented 4 years ago

maybe tokensRedeemable ??

abarmat commented 4 years ago

Or tokensClaimable? Just to match it to the claim() action. Choose the one you find best!

davekaj commented 4 years ago

Addressed in this commit https://github.com/graphprotocol/graph-network-subgraph/pull/40/commits/5ef11f027347085e2d421a50d054d94089fb4aae . We should close this issue when this pr gets merged https://github.com/graphprotocol/graph-network-subgraph/pull/40

davekaj commented 4 years ago

Has been merged, closing