F1 Distribution #750
F1 Distribution #750
Conversation
| ]; | ||
| } | ||
|
|
||
| message PeriodLockReward { |
There was a problem hiding this comment.
I don't understand just via reading the proto files what this struct is for.
Is it the return value when getting rewards for a lock?
There was a problem hiding this comment.
Correct, we store how much rewards each individual locks are eligible for.
There was a problem hiding this comment.
if its per lock, why is it repeated LastEligibleEpochByDurationAndDenom last_eligible_epochs = 3; instead of just a single entry for last_claimed_epoch
(A lock can only have one lockup duration right?)
There was a problem hiding this comment.
Same answer as in the comment below
| } | ||
|
|
||
| message GenesisReward { |
There was a problem hiding this comment.
Oh interesting, this exports to genesis undistributed rewards. The cosmos SDK's staking auto distributes the rewards at export genesis.
What do you think makes the overall code simpler?
There was a problem hiding this comment.
Do you mean adding distribution logic at export genesis?
There was a problem hiding this comment.
Yeah. (I'm also fine not doing it, just a question of overall simplicity)
There was a problem hiding this comment.
So my reading of this is that we're copying over the pattern from staking/distribution, but with epochs instead of blocks. Is this intended to be the final result? or is this an intermediate solution, and we intend to eventually do block level updates of CurrentReward for each gauge?
I suppose a middle ground is always possible where we keep it epoch based, but progressively shorten the epoch length?
Also I'm assuming that the reason to make this initially (or always) epoch based is performance, as epoch=block would be highly costly, even if we aren't iterating all locks and are only iterating the gauges.
Codecov Report
@@ Coverage Diff @@
## main #750 +/- ##
==========================================
- Coverage 19.96% 18.58% -1.38%
==========================================
Files 189 190 +1
Lines 24542 27871 +3329
==========================================
+ Hits 4899 5180 +281
- Misses 18788 21779 +2991
- Partials 855 912 +57
Continue to review full report at Codecov.
|
|
So there has been a major change, I've deleted the whole |
| // LastEligibleEpochByDurationAndDenom is a struct to store the latest epoch for each denom + lock duration combination | ||
| message LastEligibleEpochByDurationAndDenom { |
There was a problem hiding this comment.
I don't apriori understand the purpose of this struct. (Why isn't the latest epoch for every denom, lock duration combo the latest)
There was a problem hiding this comment.
or is this instead for when a user is claiming rewards, what the 'start' epoch is?
If so, is this indexed by user-address, rather than lock ID?
There was a problem hiding this comment.
Ah, this can be rather confusing. So even though a period lock would have a single lock duration that could be locked, why would there be arrays of denom + lock duration?
Lock duration here does not mean the duration of the lock itself, we're keeping an array of lock duration for keeping track of at which epoch for each lock duration was this lock eligible for reward.
| @@ -718,6 +718,8 @@ func (k Keeper) unlock(ctx sdk.Context, lock types.PeriodLock) error { | |||
| return err | |||
| } | |||
|
|
|||
| k.hooks.OnTokenUnlocked(ctx, owner, lock.ID, lock.Coins, lock.Duration, lock.EndTime) | |||
There was a problem hiding this comment.
Is there a reason behind moving the hook? (I don't really have an opinion, just curious)
Also perhaps worth us coming back and renaming the hook to be AfterTokenUnlocked or Before. On has weird semantics
There was a problem hiding this comment.
So there's logic added to hooks that uses lock reference to update periodLockReward before we delete the lock!
There was a problem hiding this comment.
For renaming the hooks, do you think it's a better decision to do it in a separate PR or straight up ahead on this one?
|
Hi team! What is the status of this? |
|
@p0mvn We were delaying this since Jan mainly because of timelines, we should further discuss abt when we have the capacity to review this / push foward |
|
Going to close for now due to how stale this is. Lets revisit as we do more F1 work. But realistically, I think what we should be doing is making a generalized accumulator system, and then switch out parts of the stack to use that as a back-end. |
Description
This PR implements F1 distribtuion mainly in the
incentivesmodule and thelockupmodule. The implementation of f1 distribution also fixes the current bug in distribution logic where unbonding locks would still receive rewards.This PR mainly introduces three new state entries in
gauge.protoin the process of implementing f1 distribution.CurrentReward, reset CurrentReward and move what was in currentReward to HistoricalReward.CumulativeRewardRatio. This is used to calculate how much user is eligible to get distributed for a specific gauge reward(ex. rewardRatio * amtofTokens = amtToBeDistributed). Each unique denom + lock duration + period has its own key, each being stored with a different key.Main implementation of the f1 distribution is in
distribute.goandrewards.go(where getters and setters are implemented for the new state entries).Additional points to be reviewed:
For contributor use:
docs/) or specification (x/<module>/spec/)Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorer