-
Notifications
You must be signed in to change notification settings - Fork 578
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
(v18: feat) Volume-Split, setup gauges to split evenly #6085
Conversation
57609c0
to
95a914b
Compare
// - we only distribute external incentive in epoch 1. | ||
// - Check that incentive record has been correctly created and gauge has been correctly updated. | ||
// - all perpetual gauges must finish distributing records | ||
// - ClPool1 will recieve full 1Musdc, 1Meth in this epoch. | ||
// - ClPool2 will recieve 500kusdc, 500keth in this epoch. | ||
// - ClPool3 will recieve full 1Musdc, 1Meth in this epoch whereas | ||
// | ||
// 6. Remove distribution records for internal incentives using HandleReplacePoolIncentivesProposal | ||
// 7. let epoch 2 pass | ||
// - We distribute internal incentive in epoch 2. | ||
// - check only external non-perpetual gauges with 2 epochs distributed | ||
// - check gauge has been correctly updated | ||
// - ClPool1 will already have 1Musdc, 1Meth (from epoch1) as external incentive. Will recieve 750Kstake as internal incentive. | ||
// - ClPool2 will already have 500kusdc, 500keth (from epoch1) as external incentive. Will recieve 500kusdc, 500keth (from epoch 2) as external incentive and 750Kstake as internal incentive. | ||
// - ClPool3 will already have 1M, 1M (from epoch1) as external incentive. This pool will not recieve any internal incentive. | ||
// - We distribute internal incentive in epoch 2. | ||
// - check only external non-perpetual gauges with 2 epochs distributed | ||
// - check gauge has been correctly updated | ||
// - ClPool1 will already have 1Musdc, 1Meth (from epoch1) as external incentive. Will recieve 750Kstake as internal incentive. | ||
// - ClPool2 will already have 500kusdc, 500keth (from epoch1) as external incentive. Will recieve 500kusdc, 500keth (from epoch 2) as external incentive and 750Kstake as internal incentive. | ||
// - ClPool3 will already have 1M, 1M (from epoch1) as external incentive. This pool will not recieve any internal incentive. | ||
// | ||
// 8. let epoch 3 pass | ||
// - nothing distributes as non-perpetual gauges with 2 epochs have ended and perpetual gauges have not been reloaded | ||
// - nothing should change in terms of incentive records | ||
// - nothing distributes as non-perpetual gauges with 2 epochs have ended and perpetual gauges have not been reloaded | ||
// - nothing should change in terms of incentive records |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter change, can ignore
95a914b
to
6812b45
Compare
@devbot-wizard help |
Hi! I'm DevBot, a bot that helps with common tasks in the development process. Commands:
|
devbot add changelog (v18: feat) Volume-Split, setup gauges to split evenly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core logic looks generally good, but I had a couple of concerns around checks & testing that will require a little bit more work before this is merge-ready
// internalGauges. | ||
message GroupGauge { | ||
uint64 group_gauge_id = 1; | ||
repeated uint64 internal_ids = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include a splitting policy field here as an enum, even if it currently just evenly splits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x/incentives/keeper/distribute.go
Outdated
@@ -441,7 +480,10 @@ func (k Keeper) Distribute(ctx sdk.Context, gauges []types.Gauge) (sdk.Coins, er | |||
ctx.Logger().Debug("distributeSyntheticInternal, gauge id %d, %d", "module", types.ModuleName, "gaugeId", gauge.Id, "height", ctx.BlockHeight()) | |||
gaugeDistributedCoins, err = k.distributeSyntheticInternal(ctx, gauge, filteredLocks, &distrInfo) | |||
} else { | |||
gaugeDistributedCoins, err = k.distributeInternal(ctx, gauge, filteredLocks, &distrInfo) | |||
// Donot distribue if LockQueryType = Group, because if we distribute here we will be double distributing. | |||
if gauge.DistributeTo.LockQueryType != lockuptypes.ByGroup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe skipping in this way might lead to totalDistributedCoins
just double counting the previous loop's gaugeDistributedCoins
(if this is the case, let's make sure that tests catch this in the future).
The more appropriate logic seems to be if [same condition], continue
and then leave the distributeInternal
unchanged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var ( | ||
epochInfo = s.App.IncentivesKeeper.GetEpochInfo(s.Ctx) | ||
|
||
expectedCoinsDistributed = sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(11_111_111))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, having new logic rely entirely on a single hard coded test vector is probably not a sufficient testing practice. Happy to discuss what might be a more robust approach offline if you'd like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to make it more robust and readable: https://github.com/osmosis-labs/osmosis/pull/6085/files#diff-d5967ddef40ae47ef4140b9c47026cac85b85d9d04173b271c6c23dd182abdbcR1200
// create 3 internal Gauge | ||
var internalGauges []uint64 | ||
for i := 0; i <= 2; i++ { | ||
internalGauge := s.CreateNoLockExternalGauges(clPool.GetId(), sdk.NewCoins(), s.TestAccs[1], uint64(1)) // gauge id = 2,3,4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of more readable & robust tests, it would probably be better to abstract this setup into a test helper and add numberOfExistingGauges
as a test parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created helper for creating internalGauges for GroupGauges
c99cb1d#diff-1000dbecb1333fe09226de57d089fe167e289bc9edae3aac2f040029bfa91b1eR51
proto/osmosis/incentives/gauge.proto
Outdated
Liquidity = 1; | ||
Evenly = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Liquidity = 1; | |
Evenly = 2; | |
Evenly = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're likely to simply remove liquidity based splitting for the reasons described in #6093
internalGaugeIds: []uint64{2, 3, 4, 5}, | ||
} | ||
|
||
tests := []struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo the map[string]struct
structure is much cleaner (e.g. see how it's done here – no need for name as a field and you can pick up the test case name from the first for loop field on line 1289 below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, i personally like writing name field, it just gives me more clarity while writing tests 😅
happy to change tho if we want to stay consistent with rest of the test
x/incentives/keeper/distribute.go
Outdated
} | ||
|
||
// CalcSplitPolicyCoins calculates tokens to split given a policy and groupGauge. | ||
func (k Keeper) CalcSplitPolicyCoins(ctx sdk.Context, policy types.SplittingPolicy, groupGauge *types.Gauge, groupGaugeObj types.GroupGauge) (sdk.Coins, sdk.Coins, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should probably be a private function
} | ||
|
||
for _, groupGauge := range groupGauges { | ||
gauge, err := k.GetGaugeByID(ctx, groupGauge.GroupGaugeId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err what are we actually getting here? Don't we already have the group gauge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAllGroupGauges
returns the ids for ex: {GroupGaugeId, InternalIds and Splitting policy}, we still have to fetch the actual gauge to get its fields
x/incentives/keeper/distribute.go
Outdated
} | ||
|
||
// only allow distribution if the GroupGauge is Active | ||
if !currTime.Before(gauge.StartTime) && (gauge.IsPerpetual || gauge.FilledEpochs < gauge.NumEpochsPaidOver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for general readability, I think doing something like "if currTime.After(startTime) && (!gaugeIsActive bool), then continue
" would be cleaner instead of having half the function in an if branch.
Ofc feel free to ignore this if you think the current approach is more readable (hence the nit) – my thinking is just that you need to keep track of fewer things mentally when you're not in a branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup i think we can just replace this with
gauge.IsActiveGauge(currTime)
as well. Looks cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work
k.SetGroupGauge(ctx, newGroupGauge) | ||
k.SetLastGaugeID(ctx, gauge.Id) | ||
|
||
// TODO: check if this is necessary, will investigate this in following PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you flag this in an issue?
* (v18: feat) Volume-Split, setup gauges to split evenly * update changelog * halfway done refactoring test * refactored test * alp comments and refactored tests * alp comments * nits --------- Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com>
Part of : #6057
What is the purpose of the change
Testing and Verifying
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)