livepeer / protocol

Livepeer protocol
MIT License
154 stars 45 forks source link

Move `EarningsClaimed` event outside conditional #360

Closed adamsoffer closed 4 years ago

adamsoffer commented 4 years ago

Based on this conditional, it appears it's possible for a delegator to call claimEarnings even if it no longer has a delegate. If this is the case, due to the fact that theEarningsClaimed event only ever gets emitted inside this conditional, there will be times where a delegator's lastClaimRound gets updated, but no event gets emitted. This would lead to stale data in indexers relying on this event.

Am I correct in this thinking? If so, can we move the EarningsClaimed event emitter outside the conditional?

yondonfu commented 4 years ago

Thanks for the mentioning this! Yeah at the moment, if a delegator does not have a delegate and calls claimEarnings(), an event will not be emitted. We can emit the event even if the delegator does not have a delegate.

kyriediculous commented 4 years ago

What about changing the conditional to if (del.delegateAddress == address(0)) { return; } And execute the current function body afterwards?

Since state is unaffected except for updating lastClaimRound when a delegator does not have a delegate it seems better to me to not log an event or update its lastClaimRound either.

yondonfu commented 4 years ago

it seems better to me to not log an event or update its lastClaimRound either.

We need to update the lastClaimRound even if the delegator does not have a delegate because we need to make sure that the delegator's lastClaimRound is updated properly before it delegates to a new address.

Suppose delegator A does not have a delegate and its lastClaimRound = X. Now, delegator A calls BondingManager.bond() in round Y in order to delegate to an address. Prior to the execution of the logic within bond(), the autoClaimEarnings() modifier will be executed. If we don't update the delegator's lastClaimRound since it does not have a delegate at this point, then, after the execution of the logic within bond(), the delegator's lastClaimRound would still be X.

Now, suppose delegator A calls claimEarnings(). The earnings calculation will start from lastClaimRound = X even though A was only delegated to its current delegator starting from round Y > X. As a result, A would claim from the earnings pool for its delegate for rounds that it was not actually delegated.

yondonfu commented 4 years ago

Closed by #361