question about GetModuleToDistributeCoins

[catShaark]

Background

GetModuleToDistributeCoins get all the gauge coins from :

• active gauges
• upcoming gauges that has start time after current block time

What about upcoming gauges that has start time before or equal to current block time ? Are we supposed to ignore those gauges ?

[stackman27]

How can an "upcoming gauge" have start_time <= current_block_time?

I thought the upcoming gauge are, gauges whose start_time has not yet occurred

[catShaark]

How can an "upcoming gauge" have start_time <= current_block_time?

I thought the upcoming gauge are, gauges whose start_time has not yet occurred

Because gauges are initially set as upcoming gauges, upcoming gauges will become active gauges at epoch block if they have start time before that epoch block time.

[stackman27]

aah i see so you're talking about this case;

Epoch time: 1, Start time: 3 
When Epoch = 4 then, Start time < Epoch time
Hence, activate gauge

What is the difference between epoch time and current block time here? If they are the same then i think this property holds

upcoming_gauge_time > epoch_time 
active_gauge_time < epoch_time 

[catShaark]

aah i see so you're talking about this case;

Epoch time: 1, Start time: 3 
When Epoch = 4 then, Start time < Epoch time
Hence, activate gauge

What is the difference between epoch time and current block time here? If they are the same then i think this property holds

upcoming_gauge_time > epoch_time 
active_gauge_time < epoch_time 

So if you set a gauge (that gauge has start time before current block time) at a block which is not epoch block. That gauge will become an upcoming gauge which has start time before that current block or any block after. This means GetModuleToDistributeCoins will ignore that gauge until epoch when that gauge becomes an active gauge.

[mattverse]

@catShaark yes, your understanding is correct

[catShaark]

So we gotta fix the function right, or is this intended ?

[mattverse]

Its def intended, not a bug

[p0mvn]

This is definitely a weird case that was spotted and thanks for bringing it up! I agree with the conclusion

From my understanding, the intent was to make sure that we only distribute to gauges on or after the block they become active.

[catShaark]

I have a weird case that goes like this: if I set a gauge with start time = 5, and the current block time = 4, then GetModuleToDistributeCoins will count that gauge, but at the next block when block time = 5, GetModuleToDistributeCoins won't count that gauge. GetModuleToDistributeCoins will continue to not count that gauge until epoch.

I think that this doesn't make sense.

[p0mvn]

I have a weird case that goes like this: if I set a gauge with start time = 5, and the current block time = 4, then GetModuleToDistributeCoins will count that gauge, but at the next block when block time = 5, GetModuleToDistributeCoins won't count that gauge. GetModuleToDistributeCoins will continue to not count that gauge until epoch.

I think that this doesn't make sense.

Hmm, this seems wrong. Do you have a branch where I could see and run that test case?

[catShaark]

@p0mvn, plz checkout this test I wrote.

[mattverse]

I do think that the problems might come from gnarly test settings in the test case your dealing with, can you try testing and setting up epoch env using begin blocker with WithBlockTime instead of using local machine times to test this?

[catShaark]

u mean like this

[p0mvn]

u mean like this

I think that's what Matt meant. Have you gotten a chance to try it? Also, @catShaark could you please give me the branch where you wrote the weird case test, and I can try looking into it more

[catShaark]

https://github.com/osmosis-labs/osmosis/compare/weird-case-getmoduletodistributecoin, here is the branch. Also this is the test's name TestWeirdCaseGetModuleToDistributeCoins

[p0mvn]

@catShaark Thank you for your patience and for giving me the branch.

So I saw the following log when I ran your test:

gauge start time: 123456799

block 1 time 123456789
GetModuleToDistributeCoins at block 1: 99999999uosmo
beginning block  2
block 2 time 123456794
GetModuleToDistributeCoins at block 2: 99999999uosmo
beginning block  3
block 3 time 123456799
GetModuleToDistributeCoins at block 3: 

• note that we stopped seeing anything at block with time = gauge start time

Apparently, until block 3 we were returning the gauge as an upcoming gauage. However, at block 3, it stopped happening.

I think the problem is the following: https://github.com/osmosis-labs/osmosis/blob/293198d5401b7d8c5db18f3a6128df6e9dbdb03c/x/incentives/keeper/distribute.go#L386-L391

• note k.UpcomingGaugesIteratorAfterTime(ctx, ctx.BlockTime())

I don't understand why we would be only considering gauges after ctx block time.

I think it might make sense to change this to: k.UpcomingGaugesIterator(ctx) since if a gauge is upcoming, we are yet to distribute to it which is in-lin with the spec: https://github.com/osmosis-labs/osmosis/blob/293198d5401b7d8c5db18f3a6128df6e9dbdb03c/x/incentives/keeper/distribute.go#L386

Once I made the change, the gauge would be printed every time in your test case.

Please let me know what you think

[p0mvn]

That being said, I don't understand who are the consumers of this query and what value exactly it provides. Do we even want to have it?

[catShaark]

@catShaark Thank you for your patience and for giving me the branch.

So I saw the following log when I ran your test:

gauge start time: 123456799

block 1 time 123456789
GetModuleToDistributeCoins at block 1: 99999999uosmo
beginning block  2
block 2 time 123456794
GetModuleToDistributeCoins at block 2: 99999999uosmo
beginning block  3
block 3 time 123456799
GetModuleToDistributeCoins at block 3: 

• note that we stopped seeing anything at block with time = gauge start time

Apparently, until block 3 we were returning the gauge as an upcoming gauage. However, at block 3, it stopped happening.

I think the problem is the following:

https://github.com/osmosis-labs/osmosis/blob/293198d5401b7d8c5db18f3a6128df6e9dbdb03c/x/incentives/keeper/distribute.go#L386-L391

• note k.UpcomingGaugesIteratorAfterTime(ctx, ctx.BlockTime())

I don't understand why we would be only considering gauges after ctx block time.

I think it might make sense to change this to: k.UpcomingGaugesIterator(ctx) since if a gauge is upcoming, we are yet to distribute to it which is in-lin with the spec:

https://github.com/osmosis-labs/osmosis/blob/293198d5401b7d8c5db18f3a6128df6e9dbdb03c/x/incentives/keeper/distribute.go#L386

Once I made the change, the gauge would be printed every time in your test case.

Please let me know what you think

You said exactly what I'm thinking

[catShaark]

That being said, I don't understand who are the consumers of this query and what value exactly it provides. Do we even want to have it?

It's only used by the user, there's no logic that depends on it

[p0mvn]

That being said, I don't understand who are the consumers of this query and what value exactly it provides. Do we even want to have it?

It's only used by the user, there's no logic that depends on it

Let's keep it for now but create a new issue to investigate the removal. For now, we can try applying k.UpcomingGaugesIterator(ctx) like we earlier discussed.

Do you agree with the approach?

[catShaark]

yess, I totally agree with you, should I create a PR for applying k.UpcomingGaugesIterator(ctx)

[p0mvn]

That would be great! Please do when you get a chance. That way, we can also get feedback from more people on this as well!

[ValarDragon]

Closing this issue, as question appears answered!