From 82b284abeda6c263b3f95fcd2a3f43e3b75a4b0c Mon Sep 17 00:00:00 2001 From: Alpin Yukseloglu Date: Tue, 8 Feb 2022 21:20:46 -0800 Subject: [PATCH 1/4] ensure gauges can only be created for onchain denoms + comments --- proto/osmosis/lockup/lock.proto | 6 ++++-- x/incentives/keeper/gauge.go | 27 ++++++++++++++++++++++++++ x/incentives/types/expected_keepers.go | 2 ++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/proto/osmosis/lockup/lock.proto b/proto/osmosis/lockup/lock.proto index 146eff68270..8ce1370df14 100644 --- a/proto/osmosis/lockup/lock.proto +++ b/proto/osmosis/lockup/lock.proto @@ -39,8 +39,10 @@ enum LockQueryType { } message QueryCondition { - LockQueryType lock_query_type = 1; // type of lock query, ByLockDuration | ByLockTime - string denom = 2; // What token denomination are we looking for lockups of + // type of lock query, ByLockDuration | ByLockTime + LockQueryType lock_query_type = 1; + // What token denomination are we looking for lockups of + string denom = 2; // valid when query condition is ByDuration google.protobuf.Duration duration = 3 [ (gogoproto.stdduration) = true, diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 38e45fcf1c4..6cca5f80e2c 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -100,6 +100,8 @@ func (k Keeper) SetGaugeWithRefKey(ctx sdk.Context, gauge *types.Gauge) error { // CreateGauge create a gauge and send coins to the gauge func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddress, coins sdk.Coins, distrTo lockuptypes.QueryCondition, startTime time.Time, numEpochsPaidOver uint64) (uint64, error) { + + // Ensure that this gauge's duration is one of the allowed durations on chain durations := k.GetLockableDurations(ctx) if distrTo.LockQueryType == lockuptypes.ByDuration { durationOk := false @@ -114,6 +116,31 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr } } + /* Questions: + 1. Are the function inputs correct? + + 2. How do we account for what happens if k.bk.HasSupply() fails or throws an error? - go stuff + general in go: https://earthly.dev/blog/golang-errors/ + + 3. Is using "break" here a clean approach, or would it be better to bake this into the logic + of the whole function (i.e. wrap in an if statement)? + + 4. How can I run existing tests against this and/or create my own for it? + setup state you want - we want table driven tests. automate most of the setup/post-condition testing bits. + + go test ./... + + example: https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/pool-models/balancer/balancer_pool_test.go#L151-L272 + + 5. Why is the distrTo parameter type "lockuptypes.QueryCondition"? What does the "QueryCondition" + part mean and where does it come from? + */ + + // Ensure that the denom this gauge pays out to exists on-chain + if k.bk.HasSupply(ctx, distrTo.Denom) { + return 0, fmt.Errorf("denom does not exist: %d", distrTo.Denom) + } + gauge := types.Gauge{ Id: k.getLastGaugeID(ctx) + 1, IsPerpetual: isPerpetual, diff --git a/x/incentives/types/expected_keepers.go b/x/incentives/types/expected_keepers.go index 0b74ed748c3..fd7e0b73fb2 100644 --- a/x/incentives/types/expected_keepers.go +++ b/x/incentives/types/expected_keepers.go @@ -12,6 +12,8 @@ import ( type BankKeeper interface { GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins + HasSupply(ctx sdk.Context, denom string) bool + SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error SendCoinsFromModuleToManyAccounts( ctx sdk.Context, senderModule string, recipientAddrs []sdk.AccAddress, amts []sdk.Coins, From e69034c211e10954faaa6d33cacc8883c604208d Mon Sep 17 00:00:00 2001 From: Alpin Yukseloglu Date: Fri, 11 Feb 2022 16:52:52 -0800 Subject: [PATCH 2/4] added and fixed tests + cleaned up comments --- x/incentives/keeper/gauge.go | 26 +++----------------------- x/incentives/keeper/gauge_test.go | 17 +++++++++++++++++ x/incentives/keeper/suite_test.go | 6 ++++++ 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 6cca5f80e2c..8f7097602c2 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -100,7 +100,7 @@ func (k Keeper) SetGaugeWithRefKey(ctx sdk.Context, gauge *types.Gauge) error { // CreateGauge create a gauge and send coins to the gauge func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddress, coins sdk.Coins, distrTo lockuptypes.QueryCondition, startTime time.Time, numEpochsPaidOver uint64) (uint64, error) { - + // Ensure that this gauge's duration is one of the allowed durations on chain durations := k.GetLockableDurations(ctx) if distrTo.LockQueryType == lockuptypes.ByDuration { @@ -116,29 +116,9 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr } } - /* Questions: - 1. Are the function inputs correct? - - 2. How do we account for what happens if k.bk.HasSupply() fails or throws an error? - go stuff - general in go: https://earthly.dev/blog/golang-errors/ - - 3. Is using "break" here a clean approach, or would it be better to bake this into the logic - of the whole function (i.e. wrap in an if statement)? - - 4. How can I run existing tests against this and/or create my own for it? - setup state you want - we want table driven tests. automate most of the setup/post-condition testing bits. - - go test ./... - - example: https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/pool-models/balancer/balancer_pool_test.go#L151-L272 - - 5. Why is the distrTo parameter type "lockuptypes.QueryCondition"? What does the "QueryCondition" - part mean and where does it come from? - */ - // Ensure that the denom this gauge pays out to exists on-chain - if k.bk.HasSupply(ctx, distrTo.Denom) { - return 0, fmt.Errorf("denom does not exist: %d", distrTo.Denom) + if !k.bk.HasSupply(ctx, distrTo.Denom) { + return 0, fmt.Errorf("denom does not exist: %s", distrTo.Denom) } gauge := types.Gauge{ diff --git a/x/incentives/keeper/gauge_test.go b/x/incentives/keeper/gauge_test.go index 96c2a1be24e..56bdf0e9c9f 100644 --- a/x/incentives/keeper/gauge_test.go +++ b/x/incentives/keeper/gauge_test.go @@ -25,6 +25,23 @@ func (suite *KeeperTestSuite) TestInvalidDurationGaugeCreationValidation() { suite.Require().NoError(err) } +func (suite *KeeperTestSuite) TestNonExistentDenomGaugeCreation() { + suite.SetupTest() + + addrNoSupply := sdk.AccAddress([]byte("Gauge_Creation_Addr_")) + addrs := suite.SetupManyLocks(1, defaultLiquidTokens, defaultLPTokens, defaultLockDuration) + distrTo := lockuptypes.QueryCondition{ + LockQueryType: lockuptypes.ByDuration, + Denom: defaultLPDenom, + Duration: defaultLockDuration, + } + _, err := suite.app.IncentivesKeeper.CreateGauge(suite.ctx, false, addrNoSupply, defaultLiquidTokens, distrTo, time.Time{}, 1) + suite.Require().Error(err) + + _, err = suite.app.IncentivesKeeper.CreateGauge(suite.ctx, false, addrs[0], defaultLiquidTokens, distrTo, time.Time{}, 1) + suite.Require().NoError(err) +} + // TODO: Make this test table driven // OR if it needs to be script based, // remove lots of boilerplate so this can actually be followed diff --git a/x/incentives/keeper/suite_test.go b/x/incentives/keeper/suite_test.go index 07c77ac188b..445a4da6efa 100644 --- a/x/incentives/keeper/suite_test.go +++ b/x/incentives/keeper/suite_test.go @@ -114,6 +114,12 @@ func (suite *KeeperTestSuite) setupNewGaugeWithDuration(isPerpetual bool, coins Denom: "lptoken", Duration: duration, } + + // mints coins so supply exists on chain + mintCoins := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} + err := simapp.FundAccount(suite.app.BankKeeper, suite.ctx, addr, mintCoins) + suite.Require().NoError(err) + numEpochsPaidOver := uint64(2) if isPerpetual { numEpochsPaidOver = uint64(1) From 0e88b741c45c93ee63214ad5898de3dea17aba13 Mon Sep 17 00:00:00 2001 From: Alpin Yukseloglu Date: Fri, 11 Feb 2022 18:15:15 -0800 Subject: [PATCH 3/4] added support for superfluid synthetic denoms --- x/incentives/keeper/gauge.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 8f7097602c2..2599d6f4ea4 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -3,6 +3,7 @@ package keeper import ( "encoding/json" "fmt" + "strings" "time" sdk "github.com/cosmos/cosmos-sdk/types" @@ -117,7 +118,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr } // Ensure that the denom this gauge pays out to exists on-chain - if !k.bk.HasSupply(ctx, distrTo.Denom) { + if !k.bk.HasSupply(ctx, distrTo.Denom) && !strings.Contains(distrTo.Denom, "osmovaloper") { return 0, fmt.Errorf("denom does not exist: %s", distrTo.Denom) } From f292078a21a18ca530247e0833076661476875e1 Mon Sep 17 00:00:00 2001 From: Alpin Yukseloglu Date: Sat, 12 Feb 2022 16:27:27 -0800 Subject: [PATCH 4/4] fixed tests to accommodate new checks --- x/incentives/abci_test.go | 12 ++++++++++++ x/incentives/genesis_test.go | 6 ++++++ x/mint/keeper/hooks_test.go | 6 ++++++ x/mint/keeper/keeper_test.go | 6 ++++++ 4 files changed, 30 insertions(+) diff --git a/x/incentives/abci_test.go b/x/incentives/abci_test.go index c08dfa45200..dfeef2ccdf3 100644 --- a/x/incentives/abci_test.go +++ b/x/incentives/abci_test.go @@ -30,6 +30,12 @@ func TestPerpetualGaugeNotExpireAfterDistribution(t *testing.T) { Denom: "lptoken", Duration: time.Second, } + + // mints coins so supply exists on chain + mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} + err = simapp.FundAccount(app.BankKeeper, ctx, addr, mintLPtokens) + require.NoError(t, err) + _, err = app.IncentivesKeeper.CreateGauge(ctx, true, addr, coins, distrTo, time.Now(), 1) require.NoError(t, err) @@ -62,6 +68,12 @@ func TestNonPerpetualGaugeExpireAfterDistribution(t *testing.T) { Denom: "lptoken", Duration: time.Second, } + + // mints coins so supply exists on chain + mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} + err = simapp.FundAccount(app.BankKeeper, ctx, addr, mintLPtokens) + require.NoError(t, err) + _, err = app.IncentivesKeeper.CreateGauge(ctx, false, addr, coins, distrTo, time.Now(), 1) require.NoError(t, err) diff --git a/x/incentives/genesis_test.go b/x/incentives/genesis_test.go index 368a81cc14e..294ad58f001 100644 --- a/x/incentives/genesis_test.go +++ b/x/incentives/genesis_test.go @@ -32,6 +32,12 @@ func TestIncentivesExportGenesis(t *testing.T) { startTime := time.Now() err := simapp.FundAccount(app.BankKeeper, ctx, addr, coins) require.NoError(t, err) + + // mints coins so supply exists on chain + mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} + err = simapp.FundAccount(app.BankKeeper, ctx, addr, mintLPtokens) + require.NoError(t, err) + gaugeID, err := app.IncentivesKeeper.CreateGauge(ctx, true, addr, coins, distrTo, startTime, 1) require.NoError(t, err) diff --git a/x/mint/keeper/hooks_test.go b/x/mint/keeper/hooks_test.go index a24b515b245..d883f5dd37c 100644 --- a/x/mint/keeper/hooks_test.go +++ b/x/mint/keeper/hooks_test.go @@ -216,6 +216,12 @@ func setupGaugeForLPIncentives(t *testing.T, app *osmoapp.OsmosisApp, ctx sdk.Co Denom: "lptoken", Duration: time.Second, } + + // mints coins so supply exists on chain + mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} + err = simapp.FundAccount(app.BankKeeper, ctx, addr, mintLPtokens) + require.NoError(t, err) + _, err = app.IncentivesKeeper.CreateGauge(ctx, true, addr, coins, distrTo, time.Now(), 1) require.NoError(t, err) } diff --git a/x/mint/keeper/keeper_test.go b/x/mint/keeper/keeper_test.go index a3013f030d3..818c94a0461 100644 --- a/x/mint/keeper/keeper_test.go +++ b/x/mint/keeper/keeper_test.go @@ -82,6 +82,12 @@ func (suite *KeeperTestSuite) TestDistrAssetToDeveloperRewardsAddrWhenNotEmpty() Denom: "lptoken", Duration: time.Second, } + + // mints coins so supply exists on chain + mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} + err = simapp.FundAccount(suite.app.BankKeeper, suite.ctx, gaugeCreator, mintLPtokens) + suite.Require().NoError(err) + gaugeId, err := suite.app.IncentivesKeeper.CreateGauge(suite.ctx, true, gaugeCreator, coins, distrTo, time.Now(), 1) suite.NoError(err) err = suite.app.PoolIncentivesKeeper.UpdateDistrRecords(suite.ctx, poolincentivestypes.DistrRecord{