From 2fa5fc730a2e6fd8e0414bfb7ef440b9852d87e2 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 19:18:28 -0500 Subject: [PATCH 01/12] incentives cli updates --- x/incentives/client/cli/cli_test.go | 171 +++++++++++++++++++--------- x/incentives/client/cli/flags.go | 5 +- x/incentives/client/cli/query.go | 97 +++++++++++++--- x/incentives/client/cli/tx.go | 6 +- 4 files changed, 200 insertions(+), 79 deletions(-) diff --git a/x/incentives/client/cli/cli_test.go b/x/incentives/client/cli/cli_test.go index 4ee07b0a737..fce100e1225 100644 --- a/x/incentives/client/cli/cli_test.go +++ b/x/incentives/client/cli/cli_test.go @@ -2,13 +2,22 @@ package cli_test import ( "fmt" + "testing" "github.com/gogo/protobuf/proto" "github.com/stretchr/testify/suite" + tmcli "github.com/tendermint/tendermint/libs/cli" + + gammtypes "github.com/osmosis-labs/osmosis/v7/x/gamm/types" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/osmosis-labs/osmosis/v7/app" + gammtestutil "github.com/osmosis-labs/osmosis/v7/x/gamm/client/testutil" "github.com/osmosis-labs/osmosis/v7/x/incentives/client/cli" "github.com/osmosis-labs/osmosis/v7/x/incentives/types" + lockuptestutil "github.com/osmosis-labs/osmosis/v7/x/lockup/client/testutil" clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli" "github.com/cosmos/cosmos-sdk/testutil/network" @@ -26,10 +35,50 @@ func (s *IntegrationTestSuite) SetupSuite() { s.cfg = app.DefaultConfig() + // modification to pay pool creation fee with test bond denom "stake" + // marshal result into genesis json + genesisState := app.ModuleBasics.DefaultGenesis(s.cfg.Codec) + gammGen := gammtypes.DefaultGenesis() + gammGen.Params.PoolCreationFee = sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, 1000000)} + gammGenJson := s.cfg.Codec.MustMarshalJSON(gammGen) + genesisState[gammtypes.ModuleName] = gammGenJson + s.cfg.GenesisState = genesisState + + // create a network with a validator s.network = network.New(s.T(), s.cfg) + val := s.network.Validators[0] + + // create a pool to receive gamm tokens + _, err := gammtestutil.MsgCreatePool(s.T(), val.ClientCtx, val.Address, "5stake,5node0token", "100stake,100node0token", "0.01", "0.01", "") + s.Require().NoError(err) + + _, err = s.network.WaitForHeight(1) + s.Require().NoError(err) + + lockAmt, err := sdk.ParseCoinNormalized(fmt.Sprint("100000gamm/pool/1")) + s.Require().NoError(err) + + _, err = s.network.WaitForHeight(1) + s.Require().NoError(err) + + // lock gamm pool tokens to create lock ID 1 + lockuptestutil.MsgLockTokens(val.ClientCtx, val.Address, lockAmt, "24h") + + _, err = s.network.WaitForHeight(1) + s.Require().NoError(err) + + secondLockAmt, err := sdk.ParseCoinNormalized(fmt.Sprint("100000gamm/pool/1")) + s.Require().NoError(err) - _, err := s.network.WaitForHeight(1) + _, err = s.network.WaitForHeight(1) s.Require().NoError(err) + + // lock gamm pool tokens to create lock ID 2 + lockuptestutil.MsgLockTokens(val.ClientCtx, val.Address, secondLockAmt, "168h") + + _, err = s.network.WaitForHeight(1) + s.Require().NoError(err) + } func (s *IntegrationTestSuite) TearDownSuite() { @@ -43,11 +92,14 @@ func (s *IntegrationTestSuite) TestGetCmdGauges() { testCases := []struct { name string expectErr bool + args []string respType proto.Message }{ { "query gauges", - false, &types.GaugesResponse{}, + false, + []string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + &types.GaugesResponse{}, }, } @@ -58,9 +110,7 @@ func (s *IntegrationTestSuite) TestGetCmdGauges() { cmd := cli.GetCmdGauges() clientCtx := val.ClientCtx - args := []string{} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -76,12 +126,15 @@ func (s *IntegrationTestSuite) TestGetCmdToDistributeCoins() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query to distribute coins", - false, &types.ModuleToDistributeCoinsResponse{}, + []string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.ModuleToDistributeCoinsResponse{}, }, } @@ -92,9 +145,7 @@ func (s *IntegrationTestSuite) TestGetCmdToDistributeCoins() { cmd := cli.GetCmdToDistributeCoins() clientCtx := val.ClientCtx - args := []string{} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -110,12 +161,15 @@ func (s *IntegrationTestSuite) TestGetCmdDistributedCoins() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query to distribute coins", - false, &types.ModuleDistributedCoinsResponse{}, + []string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.ModuleDistributedCoinsResponse{}, }, } @@ -126,9 +180,7 @@ func (s *IntegrationTestSuite) TestGetCmdDistributedCoins() { cmd := cli.GetCmdDistributedCoins() clientCtx := val.ClientCtx - args := []string{} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -144,12 +196,15 @@ func (s *IntegrationTestSuite) TestGetCmdGaugeByID() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query gauge by id", - false, &types.GaugeByIDResponse{}, + []string{"1", fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.GaugeByIDResponse{}, }, } @@ -160,9 +215,7 @@ func (s *IntegrationTestSuite) TestGetCmdGaugeByID() { cmd := cli.GetCmdGaugeByID() clientCtx := val.ClientCtx - args := []string{"1"} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -178,12 +231,15 @@ func (s *IntegrationTestSuite) TestGetCmdActiveGauges() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query active gauges", - false, &types.ActiveGaugesResponse{}, + []string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.ActiveGaugesResponse{}, }, } @@ -194,9 +250,7 @@ func (s *IntegrationTestSuite) TestGetCmdActiveGauges() { cmd := cli.GetCmdActiveGauges() clientCtx := val.ClientCtx - args := []string{} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -212,12 +266,15 @@ func (s *IntegrationTestSuite) TestGetCmdActiveGaugesPerDenom() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query active gauges per denom", - false, &types.ActiveGaugesPerDenomResponse{}, + []string{s.cfg.BondDenom, fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.ActiveGaugesPerDenomResponse{}, }, } @@ -228,9 +285,7 @@ func (s *IntegrationTestSuite) TestGetCmdActiveGaugesPerDenom() { cmd := cli.GetCmdActiveGaugesPerDenom() clientCtx := val.ClientCtx - args := []string{s.cfg.BondDenom} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -246,12 +301,15 @@ func (s *IntegrationTestSuite) TestGetCmdUpcomingGauges() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query upcoming gauges", - false, &types.UpcomingGaugesResponse{}, + []string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.UpcomingGaugesResponse{}, }, } @@ -262,9 +320,7 @@ func (s *IntegrationTestSuite) TestGetCmdUpcomingGauges() { cmd := cli.GetCmdUpcomingGauges() clientCtx := val.ClientCtx - args := []string{} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -280,12 +336,15 @@ func (s *IntegrationTestSuite) TestGetCmdUpcomingGaugesPerDenom() { testCases := []struct { name string + args []string expectErr bool respType proto.Message }{ { "query upcoming gauges per denom", - false, &types.UpcomingGaugesPerDenomResponse{}, + []string{s.cfg.BondDenom, fmt.Sprintf("--%s=json", tmcli.OutputFlag)}, + false, + &types.UpcomingGaugesPerDenomResponse{}, }, } @@ -296,9 +355,7 @@ func (s *IntegrationTestSuite) TestGetCmdUpcomingGaugesPerDenom() { cmd := cli.GetCmdUpcomingGaugesPerDenom() clientCtx := val.ClientCtx - args := []string{s.cfg.BondDenom} - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -314,32 +371,38 @@ func (s *IntegrationTestSuite) TestGetCmdRewardsEst() { testCases := []struct { name string - owner string - lockIds string - endEpoch int64 + args []string expectErr bool respType proto.Message }{ { "query rewards estimation by owner", - val.Address.String(), - "", - 100, - false, &types.RewardsEstResponse{}, + []string{ + fmt.Sprintf("--%s=%s", cli.FlagOwner, val.Address.String()), + fmt.Sprintf("--%s=100", cli.FlagEndEpoch), + fmt.Sprintf("--%s=json", tmcli.OutputFlag), + }, + false, + &types.RewardsEstResponse{}, }, { "query rewards estimation by lock id", - "", - "1,2", - 100, - false, &types.RewardsEstResponse{}, + []string{ + fmt.Sprintf("--%s=1,2", cli.FlagLockIds), + fmt.Sprintf("--%s=100", cli.FlagEndEpoch), + fmt.Sprintf("--%s=json", tmcli.OutputFlag), + }, + false, + &types.RewardsEstResponse{}, }, { "query rewards estimation with empty end epoch", - "", - "1,2", - 0, - false, &types.RewardsEstResponse{}, + []string{ + fmt.Sprintf("--%s=1,2", cli.FlagLockIds), + fmt.Sprintf("--%s=json", tmcli.OutputFlag), + }, + false, + &types.RewardsEstResponse{}, }, } @@ -350,13 +413,7 @@ func (s *IntegrationTestSuite) TestGetCmdRewardsEst() { cmd := cli.GetCmdRewardsEst() clientCtx := val.ClientCtx - args := []string{ - fmt.Sprintf("--%s=%s", cli.FlagOwner, tc.owner), - fmt.Sprintf("--%s=%s", cli.FlagLockIds, tc.lockIds), - fmt.Sprintf("--%s=%d", cli.FlagEndEpoch, tc.endEpoch), - } - - out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) if tc.expectErr { s.Require().Error(err) } else { @@ -366,3 +423,7 @@ func (s *IntegrationTestSuite) TestGetCmdRewardsEst() { }) } } + +func TestIntegrationTestSuite(t *testing.T) { + suite.Run(t, new(IntegrationTestSuite)) +} diff --git a/x/incentives/client/cli/flags.go b/x/incentives/client/cli/flags.go index 11801813f28..4f8ad1585e1 100644 --- a/x/incentives/client/cli/flags.go +++ b/x/incentives/client/cli/flags.go @@ -6,20 +6,19 @@ import ( flag "github.com/spf13/pflag" ) -// flags for lockup module tx commands. +// Flags for incentives module tx commands. const ( FlagDuration = "duration" FlagStartTime = "start-time" FlagEpochs = "epochs" FlagPerpetual = "perpetual" - FlagTimestamp = "timestamp" FlagOwner = "owner" FlagLockIds = "lock-ids" FlagEndEpoch = "end-epoch" ) -// FlagSetCreateGauge returns flags for creating gauge. +// FlagSetCreateGauge returns flags for creating gauges. func FlagSetCreateGauge() *flag.FlagSet { fs := flag.NewFlagSet("", flag.ContinueOnError) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index 4257668dbab..d2fa6a7b041 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -4,19 +4,21 @@ import ( "fmt" "strconv" "strings" + "time" "github.com/spf13/cobra" "github.com/osmosis-labs/osmosis/v7/x/incentives/types" + lockuptypes "github.com/osmosis-labs/osmosis/v7/x/lockup/types" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/version" ) -// GetQueryCmd returns the cli query commands for this module. +// GetQueryCmd returns the query commands for this module. func GetQueryCmd() *cobra.Command { - // Group incentives queries under a subcommand + // group incentives queries under a subcommand cmd := &cobra.Command{ Use: types.ModuleName, Short: fmt.Sprintf("Querying commands for the %s module", types.ModuleName), @@ -40,7 +42,7 @@ func GetQueryCmd() *cobra.Command { return cmd } -// GetCmdGauges returns full available gauges. +// GetCmdGauges returns all available gauges. func GetCmdGauges() *cobra.Command { cmd := &cobra.Command{ Use: "gauges", @@ -84,7 +86,7 @@ $ %s query incentives gauges return cmd } -// GetCmdToDistributeCoins returns coins that is going to be distributed. +// GetCmdToDistributeCoins returns coins that are going to be distributed. func GetCmdToDistributeCoins() *cobra.Command { cmd := &cobra.Command{ Use: "to-distribute-coins", @@ -120,7 +122,7 @@ $ %s query incentives to-distribute-coins return cmd } -// GetCmdDistributedCoins returns coins that are distributed so far. +// GetCmdDistributedCoins returns coins that have been distributed so far. func GetCmdDistributedCoins() *cobra.Command { cmd := &cobra.Command{ Use: "distributed-coins", @@ -156,7 +158,7 @@ $ %s query incentives distributed-coins return cmd } -// GetCmdGaugeByID returns Gauge by id. +// GetCmdGaugeByID returns a gauge by ID. func GetCmdGaugeByID() *cobra.Command { cmd := &cobra.Command{ Use: "gauge-by-id [id]", @@ -238,7 +240,7 @@ $ %s query incentives active-gauges return cmd } -// GetCmdActiveGaugesPerDenom returns active gauges for specified denom. +// GetCmdActiveGaugesPerDenom returns active gauges for a specified denom. func GetCmdActiveGaugesPerDenom() *cobra.Command { cmd := &cobra.Command{ Use: "active-gauges-per-denom [denom]", @@ -320,7 +322,7 @@ $ %s query incentives upcoming-gauges return cmd } -// GetCmdActiveGaugesPerDenom returns active gauges for specified denom. +// GetCmdUpcomingGaugesPerDenom returns active gauges for a specified denom. func GetCmdUpcomingGaugesPerDenom() *cobra.Command { cmd := &cobra.Command{ Use: "upcoming-gauges-per-denom [denom]", @@ -361,7 +363,7 @@ func GetCmdRewardsEst() *cobra.Command { fmt.Sprintf(`Query rewards estimation. Example: -$ %s query incentives rewards-estimation +$ %s query incentives rewards-estimation `, version.AppName, ), @@ -384,14 +386,57 @@ $ %s query incentives rewards-estimation return err } - lockIdStrs := strings.Split(lockIdsCombined, ",") - lockIds := []uint64{} - for _, lockIdStr := range lockIdStrs { - lockId, err := strconv.ParseUint(lockIdStr, 10, 64) + var res *lockuptypes.AccountLockedLongerDurationResponse + if owner != "" { + queryClientLockup := lockuptypes.NewQueryClient(clientCtx) + + res, err = queryClientLockup.AccountLockedLongerDuration(cmd.Context(), &lockuptypes.AccountLockedLongerDurationRequest{Owner: owner, Duration: time.Millisecond}) if err != nil { return err } - lockIds = append(lockIds, lockId) + } else { + owner = "" + } + + ownerLocks := []uint64{} + + if res != nil { + for _, lockId := range res.Locks { + ownerLocks = append(ownerLocks, lockId.ID) + } + } + + lockIdStrs := strings.Split(lockIdsCombined, ",") + lockIds := []uint64{} + // if user doesn't provide any lockIDs or a lock owner, we don't have enough information to proceed + if lockIdsCombined == "" && owner == "" { + err = fmt.Errorf("if owner flag is not set, lock IDs must be provided") + return err + + // if user provides lockIDs, use these lockIDs in our rewards estimation + } else if lockIdsCombined != "" { + for _, lockIdStr := range lockIdStrs { + lockId, err := strconv.ParseUint(lockIdStr, 10, 64) + if err != nil { + return err + } + lockIds = append(lockIds, lockId) + } + + // if no lockIDs are provided but an owner is provided, we query the rewards for all of the locks the owner has + } else if lockIdsCombined == "" && owner != "" { + lockIds = append(lockIds, ownerLocks...) + } + + // if lockIDs are provided and an owner is provided, only query the lockIDs that are provided + // if a lockID was provided and it doesn't belong to the owner, return an error + if len(lockIds) != 0 && owner != "" { + for _, lockId := range lockIds { + validInputLockId := contains(ownerLocks, lockId) + if !validInputLockId { + return fmt.Errorf("lock-id %v does not belong to %v", lockId, owner) + } + } } endEpoch, err := cmd.Flags().GetInt64(FlagEndEpoch) @@ -399,16 +444,22 @@ $ %s query incentives rewards-estimation return err } - res, err := queryClient.RewardsEst(cmd.Context(), &types.RewardsEstRequest{ + // TODO: Figure out why some lock ids are good and some causes "Error: rpc error: code = Unknown desc = panic message redacted to hide potentially sensitive system info: panic" + // since owner checks have already been completed above, we switch the owner address to a random module account address since a blank owner panics. + // we should find a better way to circumvent this address validity check + if owner == "" { + owner = "osmo14kjcwdwcqsujkdt8n5qwpd8x8ty2rys5rjrdjj" + } + res1, err1 := queryClient.RewardsEst(cmd.Context(), &types.RewardsEstRequest{ Owner: owner, // owner is used only when lockIds are empty LockIds: lockIds, EndEpoch: endEpoch, }) - if err != nil { - return err + if err1 != nil { + return err1 } - return clientCtx.PrintProto(res) + return clientCtx.PrintProto(res1) }, } @@ -419,3 +470,13 @@ $ %s query incentives rewards-estimation return cmd } + +func contains(s []uint64, value uint64) bool { + for _, v := range s { + if v == value { + return true + } + } + + return false +} diff --git a/x/incentives/client/cli/tx.go b/x/incentives/client/cli/tx.go index c2cfb9e4084..b91a43eec3a 100644 --- a/x/incentives/client/cli/tx.go +++ b/x/incentives/client/cli/tx.go @@ -35,7 +35,7 @@ func GetTxCmd() *cobra.Command { return cmd } -// NewCreateGaugeCmd broadcast MsgCreateGauge. +// NewCreateGaugeCmd broadcasts a CreateGauge message. func NewCreateGaugeCmd() *cobra.Command { cmd := &cobra.Command{ Use: "create-gauge [lockup_denom] [reward] [flags]", @@ -67,7 +67,7 @@ func NewCreateGaugeCmd() *cobra.Command { } else if timeRFC, err := time.Parse(time.RFC3339, timeStr); err == nil { // RFC time startTime = timeRFC } else { // invalid input - return errors.New("invalid start time format") + return errors.New("Invalid start time format") } epochs, err := cmd.Flags().GetUint64(FlagEpochs) @@ -114,7 +114,7 @@ func NewCreateGaugeCmd() *cobra.Command { return cmd } -// NewAddToGaugeCmd broadcast MsgAddToGauge. +// NewAddToGaugeCmd broadcasts a AddToGauge message. func NewAddToGaugeCmd() *cobra.Command { cmd := &cobra.Command{ Use: "add-to-gauge [gauge_id] [rewards] [flags]", From 58170fd64cb8f1954f425afd6dd1291df963bc9f Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 19:23:52 -0500 Subject: [PATCH 02/12] linter fix --- x/incentives/client/cli/tx.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/incentives/client/cli/tx.go b/x/incentives/client/cli/tx.go index b91a43eec3a..9d8b52a2094 100644 --- a/x/incentives/client/cli/tx.go +++ b/x/incentives/client/cli/tx.go @@ -67,7 +67,7 @@ func NewCreateGaugeCmd() *cobra.Command { } else if timeRFC, err := time.Parse(time.RFC3339, timeStr); err == nil { // RFC time startTime = timeRFC } else { // invalid input - return errors.New("Invalid start time format") + return errors.New("invalid start time format") } epochs, err := cmd.Flags().GetUint64(FlagEpochs) From 01deaefa13764e74046f5e68e234583c5bef1efd Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 22:04:24 -0500 Subject: [PATCH 03/12] Update x/incentives/client/cli/query.go Co-authored-by: Roman --- x/incentives/client/cli/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index d2fa6a7b041..da926c06e2f 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -408,7 +408,7 @@ $ %s query incentives rewards-estimation lockIdStrs := strings.Split(lockIdsCombined, ",") lockIds := []uint64{} - // if user doesn't provide any lockIDs or a lock owner, we don't have enough information to proceed + // if user doesn't provide at least one of the lock ids or owner, we don't have enough information to proceed. if lockIdsCombined == "" && owner == "" { err = fmt.Errorf("if owner flag is not set, lock IDs must be provided") return err From 1f85336ada590beec5fe0b2770d321f0e9732b47 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 22:04:59 -0500 Subject: [PATCH 04/12] Update x/incentives/client/cli/query.go Co-authored-by: Roman --- x/incentives/client/cli/query.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index da926c06e2f..df864ae17eb 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -410,8 +410,7 @@ $ %s query incentives rewards-estimation lockIds := []uint64{} // if user doesn't provide at least one of the lock ids or owner, we don't have enough information to proceed. if lockIdsCombined == "" && owner == "" { - err = fmt.Errorf("if owner flag is not set, lock IDs must be provided") - return err + return fmt.Errorf("if owner flag is not set, lock IDs must be provided") // if user provides lockIDs, use these lockIDs in our rewards estimation } else if lockIdsCombined != "" { From 67e09d478c1e8f6eea80fae2d3a0209379ddb2bc Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 22:47:34 -0500 Subject: [PATCH 05/12] make Roman suggestions --- x/incentives/client/cli/query.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index df864ae17eb..99f74d73016 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -400,10 +400,8 @@ $ %s query incentives rewards-estimation ownerLocks := []uint64{} - if res != nil { - for _, lockId := range res.Locks { - ownerLocks = append(ownerLocks, lockId.ID) - } + for _, lockId := range res.Locks { + ownerLocks = append(ownerLocks, lockId.ID) } lockIdStrs := strings.Split(lockIdsCombined, ",") @@ -413,7 +411,7 @@ $ %s query incentives rewards-estimation return fmt.Errorf("if owner flag is not set, lock IDs must be provided") // if user provides lockIDs, use these lockIDs in our rewards estimation - } else if lockIdsCombined != "" { + } else if owner == "" { for _, lockIdStr := range lockIdStrs { lockId, err := strconv.ParseUint(lockIdStr, 10, 64) if err != nil { @@ -423,7 +421,7 @@ $ %s query incentives rewards-estimation } // if no lockIDs are provided but an owner is provided, we query the rewards for all of the locks the owner has - } else if lockIdsCombined == "" && owner != "" { + } else if lockIdsCombined == "" { lockIds = append(lockIds, ownerLocks...) } From 3a1951f744ad268c4fa666474cccbf57edb71a10 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 22:59:16 -0500 Subject: [PATCH 06/12] make review suggestions --- x/incentives/client/cli/query.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index 99f74d73016..a9e3e79c043 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -394,8 +394,6 @@ $ %s query incentives rewards-estimation if err != nil { return err } - } else { - owner = "" } ownerLocks := []uint64{} @@ -447,16 +445,16 @@ $ %s query incentives rewards-estimation if owner == "" { owner = "osmo14kjcwdwcqsujkdt8n5qwpd8x8ty2rys5rjrdjj" } - res1, err1 := queryClient.RewardsEst(cmd.Context(), &types.RewardsEstRequest{ + rewardsEstimateResult, err := queryClient.RewardsEst(cmd.Context(), &types.RewardsEstRequest{ Owner: owner, // owner is used only when lockIds are empty LockIds: lockIds, EndEpoch: endEpoch, }) - if err1 != nil { - return err1 + if err != nil { + return err } - return clientCtx.PrintProto(res1) + return clientCtx.PrintProto(rewardsEstimateResult) }, } From 6030c5c3d016c7eafb48440d6f02533d10de6793 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 23:10:51 -0500 Subject: [PATCH 07/12] revert suggestion --- x/incentives/client/cli/query.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index a9e3e79c043..5a8aedf09fa 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -398,8 +398,10 @@ $ %s query incentives rewards-estimation ownerLocks := []uint64{} - for _, lockId := range res.Locks { - ownerLocks = append(ownerLocks, lockId.ID) + if res != nil { + for _, lockId := range res.Locks { + ownerLocks = append(ownerLocks, lockId.ID) + } } lockIdStrs := strings.Split(lockIdsCombined, ",") From 17f08cd9b2ee9999c96aa641ae01154aff7267c3 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 23:40:41 -0500 Subject: [PATCH 08/12] address roman comment error verbiage --- x/incentives/client/cli/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index 5a8aedf09fa..5eb16d82384 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -408,7 +408,7 @@ $ %s query incentives rewards-estimation lockIds := []uint64{} // if user doesn't provide at least one of the lock ids or owner, we don't have enough information to proceed. if lockIdsCombined == "" && owner == "" { - return fmt.Errorf("if owner flag is not set, lock IDs must be provided") + err = fmt.Errorf("either one of owner flag or lock IDs must be provided") // if user provides lockIDs, use these lockIDs in our rewards estimation } else if owner == "" { From 042e456c5c6ea015297f693f5032b9dde3a7e347 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 6 Jul 2022 23:44:21 -0500 Subject: [PATCH 09/12] linter --- x/incentives/client/cli/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index 5eb16d82384..66794918fef 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -408,7 +408,7 @@ $ %s query incentives rewards-estimation lockIds := []uint64{} // if user doesn't provide at least one of the lock ids or owner, we don't have enough information to proceed. if lockIdsCombined == "" && owner == "" { - err = fmt.Errorf("either one of owner flag or lock IDs must be provided") + return fmt.Errorf("either one of owner flag or lock IDs must be provided") // if user provides lockIDs, use these lockIDs in our rewards estimation } else if owner == "" { From fbb7152a40c95377141ee7fd1e1b783970c7c43f Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Thu, 7 Jul 2022 13:37:55 -0500 Subject: [PATCH 10/12] address review comments --- x/incentives/client/cli/query.go | 45 +++++++++++++++----------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index 66794918fef..53a0ca4f758 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -370,11 +370,9 @@ $ %s query incentives rewards-estimation ), Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, args []string) error { - clientCtx, err := client.GetClientQueryContext(cmd) - if err != nil { - return err - } - queryClient := types.NewQueryClient(clientCtx) + var res *lockuptypes.AccountLockedLongerDurationResponse + ownerLocks := []uint64{} + lockIds := []uint64{} owner, err := cmd.Flags().GetString(FlagOwner) if err != nil { @@ -385,27 +383,13 @@ $ %s query incentives rewards-estimation if err != nil { return err } + lockIdStrs := strings.Split(lockIdsCombined, ",") - var res *lockuptypes.AccountLockedLongerDurationResponse - if owner != "" { - queryClientLockup := lockuptypes.NewQueryClient(clientCtx) - - res, err = queryClientLockup.AccountLockedLongerDuration(cmd.Context(), &lockuptypes.AccountLockedLongerDurationRequest{Owner: owner, Duration: time.Millisecond}) - if err != nil { - return err - } - } - - ownerLocks := []uint64{} - - if res != nil { - for _, lockId := range res.Locks { - ownerLocks = append(ownerLocks, lockId.ID) - } + endEpoch, err := cmd.Flags().GetInt64(FlagEndEpoch) + if err != nil { + return err } - lockIdStrs := strings.Split(lockIdsCombined, ",") - lockIds := []uint64{} // if user doesn't provide at least one of the lock ids or owner, we don't have enough information to proceed. if lockIdsCombined == "" && owner == "" { return fmt.Errorf("either one of owner flag or lock IDs must be provided") @@ -436,10 +420,23 @@ $ %s query incentives rewards-estimation } } - endEpoch, err := cmd.Flags().GetInt64(FlagEndEpoch) + clientCtx, err := client.GetClientQueryContext(cmd) if err != nil { return err } + queryClient := types.NewQueryClient(clientCtx) + + if owner != "" { + queryClientLockup := lockuptypes.NewQueryClient(clientCtx) + + res, err = queryClientLockup.AccountLockedLongerDuration(cmd.Context(), &lockuptypes.AccountLockedLongerDurationRequest{Owner: owner, Duration: time.Millisecond}) + if err != nil { + return err + } + for _, lockId := range res.Locks { + ownerLocks = append(ownerLocks, lockId.ID) + } + } // TODO: Figure out why some lock ids are good and some causes "Error: rpc error: code = Unknown desc = panic message redacted to hide potentially sensitive system info: panic" // since owner checks have already been completed above, we switch the owner address to a random module account address since a blank owner panics. From eb5b5166950c02179c261d1abbcb68d398be623f Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Sat, 9 Jul 2022 12:33:51 -0500 Subject: [PATCH 11/12] get est rewards fix --- x/incentives/client/cli/query.go | 9 ++------- x/incentives/keeper/distribute.go | 12 +++++++----- x/incentives/keeper/gauge.go | 6 +++++- x/incentives/keeper/grpc_query.go | 14 +++++++++----- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index 53a0ca4f758..fa38e9a0fe9 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -322,7 +322,7 @@ $ %s query incentives upcoming-gauges return cmd } -// GetCmdUpcomingGaugesPerDenom returns active gauges for a specified denom. +// GetCmdActiveGaugesPerDenom returns active gauges for specified denom. func GetCmdUpcomingGaugesPerDenom() *cobra.Command { cmd := &cobra.Command{ Use: "upcoming-gauges-per-denom [denom]", @@ -438,12 +438,7 @@ $ %s query incentives rewards-estimation } } - // TODO: Figure out why some lock ids are good and some causes "Error: rpc error: code = Unknown desc = panic message redacted to hide potentially sensitive system info: panic" - // since owner checks have already been completed above, we switch the owner address to a random module account address since a blank owner panics. - // we should find a better way to circumvent this address validity check - if owner == "" { - owner = "osmo14kjcwdwcqsujkdt8n5qwpd8x8ty2rys5rjrdjj" - } + // TODO: Fix accumulation store bug. For now, we return a graceful error when attempting to query bugged gauges rewardsEstimateResult, err := queryClient.RewardsEst(cmd.Context(), &types.RewardsEstRequest{ Owner: owner, // owner is used only when lockIds are empty LockIds: lockIds, diff --git a/x/incentives/keeper/distribute.go b/x/incentives/keeper/distribute.go index 06c3c01ac52..2442bfc4687 100644 --- a/x/incentives/keeper/distribute.go +++ b/x/incentives/keeper/distribute.go @@ -105,10 +105,13 @@ func (k Keeper) getLocksToDistributionWithMaxDuration(ctx sdk.Context, distrTo l // filteredLocks are all locks that are valid for gauge // It also applies an update for the gauge, handling the sending of the rewards. // (Note this update is in-memory, it does not change state.) -func (k Keeper) FilteredLocksDistributionEst(ctx sdk.Context, gauge types.Gauge, filteredLocks []lockuptypes.PeriodLock) (types.Gauge, sdk.Coins, error) { +func (k Keeper) FilteredLocksDistributionEst(ctx sdk.Context, gauge types.Gauge, filteredLocks []lockuptypes.PeriodLock) (types.Gauge, sdk.Coins, bool, error) { TotalAmtLocked := k.lk.GetPeriodLocksAccumulation(ctx, gauge.DistributeTo) if TotalAmtLocked.IsZero() { - return types.Gauge{}, nil, nil + return types.Gauge{}, nil, false, nil + } + if TotalAmtLocked.IsNegative() { + return types.Gauge{}, nil, true, nil } remainCoins := gauge.Coins.Sub(gauge.DistributedCoins) @@ -119,9 +122,8 @@ func (k Keeper) FilteredLocksDistributionEst(ctx sdk.Context, gauge types.Gauge, if !gauge.IsPerpetual { remainEpochs = gauge.NumEpochsPaidOver - gauge.FilledEpochs } - // TODO: Should this return err if remainEpochs == 0 { - return gauge, sdk.Coins{}, nil + return gauge, sdk.Coins{}, false, nil } remainCoinsPerEpoch := sdk.Coins{} @@ -154,7 +156,7 @@ func (k Keeper) FilteredLocksDistributionEst(ctx sdk.Context, gauge types.Gauge, gauge.FilledEpochs += 1 gauge.DistributedCoins = gauge.DistributedCoins.Add(remainCoinsPerEpoch...) - return gauge, filteredDistrCoins, nil + return gauge, filteredDistrCoins, false, nil } // distributionInfo stores all of the information for pent up sends for rewards distributions. diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index cda28bfb133..59bc7436dc2 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -3,6 +3,7 @@ package keeper import ( "encoding/json" "fmt" + "strconv" "strings" "time" @@ -261,10 +262,13 @@ func (k Keeper) GetRewardsEst(ctx sdk.Context, addr sdk.AccAddress, locks []lock } for epoch := distrBeginEpoch; epoch <= endEpoch; epoch++ { - newGauge, distrCoins, err := k.FilteredLocksDistributionEst(cacheCtx, gauge, locks) + newGauge, distrCoins, isBuggedGauge, err := k.FilteredLocksDistributionEst(cacheCtx, gauge, locks) if err != nil { continue } + if isBuggedGauge { + ctx.Logger().Error("Reward estimation does not include gauge " + strconv.Itoa(int(gauge.Id)) + " due to accumulation store bug") + } estimatedRewards = estimatedRewards.Add(distrCoins...) gauge = newGauge } diff --git a/x/incentives/keeper/grpc_query.go b/x/incentives/keeper/grpc_query.go index 5fe8e7b45fc..6395910873e 100644 --- a/x/incentives/keeper/grpc_query.go +++ b/x/incentives/keeper/grpc_query.go @@ -138,10 +138,11 @@ func (q Querier) UpcomingGaugesPerDenom(goCtx context.Context, req *types.Upcomi // RewardsEst returns rewards estimation at a future specific time. func (q Querier) RewardsEst(goCtx context.Context, req *types.RewardsEstRequest) (*types.RewardsEstResponse, error) { + var ownerAddress sdk.AccAddress if req == nil { return nil, status.Error(codes.InvalidArgument, "empty request") } - if len(req.Owner) == 0 { + if len(req.Owner) == 0 && len(req.LockIds) == 0 { return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "empty owner") } @@ -151,9 +152,12 @@ func (q Querier) RewardsEst(goCtx context.Context, req *types.RewardsEstRequest) return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "end epoch out of ranges") } - owner, err := sdk.AccAddressFromBech32(req.Owner) - if err != nil { - return nil, err + if len(req.Owner) != 0 { + owner, err := sdk.AccAddressFromBech32(req.Owner) + if err != nil { + return nil, err + } + ownerAddress = owner } locks := make([]lockuptypes.PeriodLock, 0, len(req.LockIds)) @@ -165,7 +169,7 @@ func (q Querier) RewardsEst(goCtx context.Context, req *types.RewardsEstRequest) locks = append(locks, *lock) } - return &types.RewardsEstResponse{Coins: q.Keeper.GetRewardsEst(ctx, owner, locks, req.EndEpoch)}, nil + return &types.RewardsEstResponse{Coins: q.Keeper.GetRewardsEst(ctx, ownerAddress, locks, req.EndEpoch)}, nil } func (q Querier) LockableDurations(ctx context.Context, _ *types.QueryLockableDurationsRequest) (*types.QueryLockableDurationsResponse, error) { From 1bb91ad8b1b70cb25b8e902dc45c1966eca53bba Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Sat, 9 Jul 2022 12:52:58 -0500 Subject: [PATCH 12/12] fix comment --- x/incentives/client/cli/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/incentives/client/cli/query.go b/x/incentives/client/cli/query.go index fa38e9a0fe9..27a0afa89c8 100644 --- a/x/incentives/client/cli/query.go +++ b/x/incentives/client/cli/query.go @@ -322,7 +322,7 @@ $ %s query incentives upcoming-gauges return cmd } -// GetCmdActiveGaugesPerDenom returns active gauges for specified denom. +// GetCmdUpcomingGaugesPerDenom returns active gauges for specified denom. func GetCmdUpcomingGaugesPerDenom() *cobra.Command { cmd := &cobra.Command{ Use: "upcoming-gauges-per-denom [denom]",