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.

Sign up for GitHub

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

Refactor createGauge to accomodate for CL gauges #5383

Closed
wants to merge 8 commits into from
Closed

Refactor createGauge to accomodate for CL gauges #5383

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dcc3fe4
update LockQueryType enum, default value is ByTime
hieuvubk Jun 1, 2023
9190730
use lockuptypes.QueryCondition{}, add check denom empty
hieuvubk Jun 1, 2023
c544e5c
update test
hieuvubk Jun 1, 2023
3e02f5f
expand condition for CL gauges creation
hieuvubk Jun 1, 2023
2d3e6e0
resolve conflict
hieuvubk Jun 5, 2023
aacbbd4
update lock.pb.go
hieuvubk Jun 5, 2023
6862cd9
add new LockQueryType for CL gauge
hieuvubk Jun 7, 2023
0458c57
update MsgCreateGauge ValidateBasic condition
hieuvubk Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions proto/osmosis/lockup/lock.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,16 @@ message PeriodLock {
enum LockQueryType {
option (gogoproto.goproto_enum_prefix) = false;

ByDuration = 0;
ByTime = 1;
ByTime = 0;
ByDuration = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this changed again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if we pass lockuptypes.QueryCondition{} to the function, it will still get the default value of distrTo.LockQueryType which is the first value of LockQueryType (In this case ByDuration => thus causing an error in the loop. )
So I just change the index to bypass.

Copy link
Contributor

Choose a reason for hiding this comment

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

for CL the we are not distributing via locktypes so, what i originally thought was that we could skip distrTo check below;

	if distrTo.LockQueryType == lockuptypes.ByDuration {
		durationOk := false
		for _, duration := range durations {
			if duration == distrTo.Duration {
				durationOk = true
				break
			}
		}
		if !durationOk {
			return 0, fmt.Errorf("invalid duration: %d", distrTo.Duration)
		}
	}

	// check if denom this gauge pays out to exists on-chain
	if distrTo.Denom != "" && !k.bk.HasSupply(ctx, distrTo.Denom) && !strings.Contains(distrTo.Denom, "osmovaloper") {
		return 0, fmt.Errorf("denom does not exist: %s", distrTo.Denom)
	}

by passing empty queryCondition. lmk if that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I understand ur idea. But the problem I face is even calling CreateGauge with empty queryCondition. It is still assigned a value in the function.
Thinking about adding new value to LockQueryType enum to bypass both ByDuration & ByTime. Lmk what u think about that

Copy link
Contributor

@stackman27 stackman27 Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i see what about changing the enumType to something like pretty much what you suggested;

enum LockQueryType {
  option (gogoproto.goproto_enum_prefix) = false;

  UNSPECIFIED = 0;
  ByTime = 0;
  ByDuration = 1;
}

if this works, hopefully we donot have to make lots of core logic changes and it works with existing queryCondition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we r same page 🙌

}

// QueryCondition is a struct used for querying locks upon different conditions.
// Duration field and timestamp fields could be optional, depending on the
// LockQueryType.
message QueryCondition {
// LockQueryType is a type of lock query, ByLockDuration | ByLockTime
LockQueryType lock_query_type = 1;
LockQueryType lock_query_type = 1 [ (gogoproto.nullable) = true ];
// Denom represents the token denomination we are looking to lock up
string denom = 2;
// Duration is used to query locks with longer duration than the specified
Expand Down
2 changes: 1 addition & 1 deletion x/incentives/keeper/gauge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr
}

// check if denom this gauge pays out to exists on-chain
if !k.bk.HasSupply(ctx, distrTo.Denom) && !strings.Contains(distrTo.Denom, "osmovaloper") {
if distrTo.Denom != "" && !k.bk.HasSupply(ctx, distrTo.Denom) && !strings.Contains(distrTo.Denom, "osmovaloper") {
return 0, fmt.Errorf("denom does not exist: %s", distrTo.Denom)
}

Expand Down
4 changes: 0 additions & 4 deletions x/incentives/types/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,6 @@ func (m MsgCreateGauge) ValidateBasic() error {
return errors.New("distribution period should be 1 epoch for perpetual gauge")
}

if lockuptypes.LockQueryType_name[int32(m.DistributeTo.LockQueryType)] != "ByDuration" {
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CreateGauge for CL pool we don't want to limit just using ByDuration right? Or lmk if im wrong

Copy link
Contributor

@stackman27 stackman27 Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can we add this back and add a clause that if lockTypes are used it has to be "byDuration"?

return errors.New("only duration query condition is allowed. Start time distr conditions is an obsolete codepath slated for deletion")
}

return nil
}

Expand Down
97 changes: 49 additions & 48 deletions x/lockup/types/lock.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions x/pool-incentives/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"

appParams "github.com/osmosis-labs/osmosis/v15/app/params"
poolmanagertypes "github.com/osmosis-labs/osmosis/v15/x/poolmanager/types"
)

Expand Down Expand Up @@ -108,10 +107,7 @@ func (k Keeper) CreateConcentratedLiquidityPoolGauge(ctx sdk.Context, poolId uin
// dummy variable so that the existing logic does not break
// CreateGauge checks if LockQueryType is `ByDuration` or not, we bypass this check by passing
// lockQueryType as byTime. Although we do not need this check, we still cannot pass empty struct.
lockuptypes.QueryCondition{
LockQueryType: lockuptypes.ByTime,
Denom: appParams.BaseCoinUnit,
},
lockuptypes.QueryCondition{},
ctx.BlockTime(),
1,
)
Expand Down
3 changes: 1 addition & 2 deletions x/pool-incentives/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/stretchr/testify/suite"

"github.com/osmosis-labs/osmosis/v15/app/apptesting"
appParams "github.com/osmosis-labs/osmosis/v15/app/params"
gammtypes "github.com/osmosis-labs/osmosis/v15/x/gamm/types"
incentivestypes "github.com/osmosis-labs/osmosis/v15/x/incentives/types"
"github.com/osmosis-labs/osmosis/v15/x/pool-incentives/types"
Expand Down Expand Up @@ -220,7 +219,7 @@ func (s *KeeperTestSuite) TestCreateConcentratedLiquidityPoolGauge() {
s.Require().True(gaugeInfo.IsPerpetual)
s.Require().Empty(gaugeInfo.Coins)
s.Require().Equal(s.Ctx.BlockTime(), gaugeInfo.StartTime)
s.Require().Equal(appParams.BaseCoinUnit, gaugeInfo.DistributeTo.Denom)
// s.Require().Equal(appParams.BaseCoinUnit, gaugeInfo.DistributeTo.Denom)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// s.Require().Equal(appParams.BaseCoinUnit, gaugeInfo.DistributeTo.Denom)
s.Require().Equal(appParams.BaseCoinUnit, gaugeInfo.DistributeTo.Denom)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CreateGauge we pass empty queryCondition so I dont think we should check gaugeInfo.DistributeTo.Denom cause its alway be "" as I think. If im wrong pls fix me

s.Require().Equal(uint64(1), gaugeInfo.NumEpochsPaidOver)
}
})
Expand Down