yearn / veYFI

Voting YFI
75 stars 38 forks source link

veYFIRewards reward calculation benefits regular claims. #135

Closed chudnov closed 2 years ago

chudnov commented 2 years ago

Since the penalty of early withdrawal is distributed to the existing yfi stakers, shouldn't the distribution also depends on a yfi lockers veYFI weights at the time which a separate individual did early withdraw (and therefore lost some yfi to veyfi rewards)? The current implementation makes those who are periodically calling deposit/withdraw earn more rewards from penalties than those who don't. This is because, by the time someone claims penalties YFI, their veYFI balance could already be 0 as it decays.

Line 67

pandadefi commented 2 years ago

You are correct, this is one of the drawbacks of the system

If we were to distribute rewards when they are queued, we would need a snapshot of all user's balances to distribute accordingly which isn't possible.

If you claim on a regular basis and relock your overall power increases and that's the best strategy to get the best yield.

A second way to do that would be to keep a balance per user and a total balance based on the sum of the balances snapshotted but doing so will make it possible for someone to claim after the expiration of his lock. A solution to that is incentivising others to claim to trigger the ve balance update with some penalty mechanism (ex: 0.25% of the rewards are distributed to the one claiming for every week after a lock expired).

On this poc the balance is updated on claim, deposits, withdraw and new lock.

A third way to do this could be to take an average of the user power and total over a period of time but I am not sure that could work.

If you have ideas on how to solve or mitigate the issue, please share.

chudnov commented 2 years ago

How about using FeeDistributor from Curve? The logic is for distributing 50% of protocol revenue in 3CRV to veCRV holders (with correct balance calculations). How about using same thing but use penaltied YFI as reward in which burn is called instead of donate?

pandadefi commented 2 years ago

This is great and simple to use. Maybe a claimable view function could be added. It's pretty complex to get a precise amount to claim so that's why I guess it's not part of the contract and that, in the curve doc, FeeDistributor.claim:

Returns the amount of 3CRV received in the claim. For off-chain integrators, this function can be called as though it were a view method in order to check the claimable amount.

@chuddster do you have an idea of what can_checkpoint_token was for?

I just saw on your repo the PenaltyDistributor that seems inspired by the FeeDistributor you have an approximation of the claimable amount.

chudnov commented 2 years ago

yeah I modified FeeDistributor to add a claimable method on-chain

can_checkpoint_token should be turned on with toggle_allow_checkpoint_token on for burn() call to auto-checkpoint everything when its called. otherwise checkpoint() has to be called manually by admin.

pandadefi commented 2 years ago

can_checkpoint_token should be turned on with toggle_allow_checkpoint_token on for burn() call to auto-checkpoint everything when its called. otherwise checkpoint() has to be called manually by admin.

Sorry, what I meant is why is can_checkpoint_token here in the first place, or could it be removed?

The contract was deployed at https://etherscan.io/tx/0x703f983edc308dcb7aa6e4d2da2e68c523ec7a774597f4c57586848745948003 but the start time is two months prior to the deployment date. Then the admin changed the can_checkpoint_token about 2 months after: https://etherscan.io/tx/0x8c99552dc3b934c0b1b2367bf4f526e374b636bcca7e92ed85caede877023f08

chudnov commented 2 years ago

I think it can be removed. Only reason for it to be off is if you don't want burn() to checkpoint everything. the tx you are pointing to is a checkpoint_total_supply call btw.

ve_supply and tokens_per_week all default values at initialization. checkpoint calls update it accordingly

pandadefi commented 2 years ago

I think I got why the start point is 2 months prior to the deployment, it's done so that it will distribute rewards for the two months prior to the deployment. using _checkpoint_token.

chudnov commented 2 years ago

yep, for retroactive rewards. however I struggle to see the reason to do _checkpoint_token manually rather than through the initial burn?