Skip to content
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

Turnpike/f1 fee distribution #543

Closed

Conversation

xedonman
Copy link

Closes: #XXX

Description

Following changes are made in this PR:

  1. F1 Distribution implementation, basic key features:
  • Period
    • time duration in which bonded stakes remain constant
  • CurrentReward
    • represents current rewards and current period for a gauge
  • HistoricalReward
    • represents historical rewards for a gauge. Accumulated reward ratio is the sum from the zeroth period until current period of (rewards/stakes)
  1. give reward for unlocking locks more precisely
  • Fix bug with people during unbonding periods still get rewards
  1. Allow Partial Unlockings per Lock
  • if requested coins for unlock is less than that of previous unlock
    new period lock with requested coin amount is generated and begin unlock

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

xedonman and others added 30 commits October 20, 2021 18:09
No need to distribute using stake ratio.
there is always one current_reward to one gauge
fix wrong range for unlocking lock candidate check
accumulate store : accumulated sum of total locks
unlocking accumulate store : acculated sum of unlocking locks

total stake for distribution = accumulated value of locks - accumulated value of unqualfied unlocking
current reward
historical reward
period-lock-reward
@mattverse mattverse self-requested a review October 29, 2021 13:13
@codecov-commenter
Copy link

Codecov Report

Merging #543 (0155e64) into main (652a01b) will decrease coverage by 1.50%.
The diff coverage is 14.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #543      +/-   ##
==========================================
- Coverage   20.44%   18.93%   -1.51%     
==========================================
  Files         163      164       +1     
  Lines       23186    26806    +3620     
==========================================
+ Hits         4741     5077     +336     
- Misses      17668    20885    +3217     
- Partials      777      844      +67     
Impacted Files Coverage Δ
x/gamm/handler.go 3.03% <0.00%> (ø)
x/gamm/keeper/msg_server.go 1.42% <0.00%> (ø)
x/gamm/types/genesis.pb.go 0.90% <ø> (ø)
x/gamm/types/marshal.go 80.00% <ø> (ø)
x/gamm/types/query.pb.go 0.83% <0.00%> (-0.01%) ⬇️
x/incentives/client/cli/query.go 0.00% <0.00%> (ø)
x/incentives/client/cli/tx.go 0.00% <0.00%> (ø)
x/incentives/handler.go 0.00% <0.00%> (ø)
x/incentives/keeper/msg_server.go 0.00% <0.00%> (ø)
x/incentives/keeper/utils.go 100.00% <ø> (ø)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6f23dc...0155e64. Read the comment docs.

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main logic of the implementation of the F1 distribution looks pretty solid

proto/osmosis/incentives/query.proto Show resolved Hide resolved
proto/osmosis/incentives/query.proto Show resolved Hide resolved
x/incentives/types/keys.go Outdated Show resolved Hide resolved
KeyHistoricalReward = []byte{0x11} // TODO : Confirm this key name/value

// KeyHistoricalReward defines key for storing historical reward
KeyHistoricalRewardRef = []byte{0x12} // TODO : Confirm this key name/value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that I think I need explanation on why this key has Ref behind, the other Key names look ligit to me!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is explanation enough? or do you prefer to rename the key?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the key name should be fine!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that I'm still confused how KeyHistoricalReward and KeyHistoricalRewardsRef are different / how they are related, connected to each other

Copy link
Author

@xedonman xedonman Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HistoricalReward can be found with denom, duration, and "period" which is a range of time that total stakes have not changed
But when a lock is "unlocking" or gauge becomes "finished" state, period can not be referred because

  • "unlocking lock" should not be rewarded (or excluded from recent periods) if remaining locked duration is smaller than gauge's duration
  • f1 distribute() will not be called if no gauge for denom&duration exist.

KeyHistoricalRewardsRef is used to get exact "period" by epoch number

x/incentives/keeper/gauge.go Show resolved Hide resolved
x/incentives/keeper/gauge.go Outdated Show resolved Hide resolved
xedonman and others added 2 commits November 1, 2021 12:13
- change lockable_durations->lockable_duration
- remove for loop which only access to first element
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second round of reviews

x/incentives/types/keys.go Outdated Show resolved Hide resolved
x/incentives/types/keys.go Outdated Show resolved Hide resolved
KeyHistoricalReward = []byte{0x11} // TODO : Confirm this key name/value

// KeyHistoricalReward defines key for storing historical reward
KeyHistoricalRewardRef = []byte{0x12} // TODO : Confirm this key name/value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the key name should be fine!

KeyHistoricalReward = []byte{0x11} // TODO : Confirm this key name/value

// KeyHistoricalReward defines key for storing historical reward
KeyHistoricalRewardRef = []byte{0x12} // TODO : Confirm this key name/value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that I'm still confused how KeyHistoricalReward and KeyHistoricalRewardsRef are different / how they are related, connected to each other

x/incentives/keeper/gauge.go Show resolved Hide resolved
this comit also contains
- unify key combination for current_reward and historical reward
- remove unused key
@daniel-farina daniel-farina added this to the 2021 - December Milestone milestone Dec 2, 2021
@daniel-farina daniel-farina removed this from the 2021-12 Milestone milestone Dec 24, 2021
@ValarDragon ValarDragon assigned mattverse and unassigned ValarDragon Jan 6, 2022
@ValarDragon
Copy link
Member

Closing in favor of #750

@mattverse mattverse mentioned this pull request Feb 17, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants