fix: do not allow empty plan name

x/farming/keeper/proposal_handler.go

@@ -48,7 +48,7 @@ func (k Keeper) AddPublicPlanProposal(ctx sdk.Context, proposals []*types.AddReq
 			return err
 		}
 
-		if p.EpochAmount.IsAllPositive() {
+		if p.IsForFixedAmountPlan() {
 			msg := types.NewMsgCreateFixedAmountPlan(
 				p.GetName(),
 				farmingPoolAddrAcc,
@@ -65,8 +65,7 @@ func (k Keeper) AddPublicPlanProposal(ctx sdk.Context, proposals []*types.AddReq
 
 			logger := k.Logger(ctx)
 			logger.Info("created public fixed amount plan", "fixed_amount_plan", plan)
-
-		} else if p.EpochRatio.IsPositive() {
+		} else {
 			msg := types.NewMsgCreateRatioPlan(
 				p.GetName(),
 				farmingPoolAddrAcc,
@@ -76,10 +75,6 @@ func (k Keeper) AddPublicPlanProposal(ctx sdk.Context, proposals []*types.AddReq
 				p.EpochRatio,
 			)
 
-			if err = msg.ValidateBasic(); err != nil {
-				return err
-			}
-
 			plan, err := k.CreateRatioPlan(ctx, msg, farmingPoolAddrAcc, terminationAcc, types.PlanTypePublic)
 			if err != nil {
 				return err
@@ -101,123 +96,72 @@ func (k Keeper) UpdatePublicPlanProposal(ctx sdk.Context, proposals []*types.Upd
 			return sdkerrors.Wrapf(sdkerrors.ErrNotFound, "plan %d is not found", p.GetPlanId())
 		}
 
-		if p.EpochAmount.IsAllPositive() {
-			if p.GetName() != "" {
-				if err := plan.SetName(p.GetName()); err != nil {
-					return err
-				}
+		if p.GetName() != "" {
+			if err := plan.SetName(p.GetName()); err != nil {
+				return err
 			}
+		}
 
-			if p.GetFarmingPoolAddress() != "" {
-				farmingPoolAddrAcc, err := sdk.AccAddressFromBech32(p.GetFarmingPoolAddress())
-				if err != nil {
-					return err
-				}
-				if err := plan.SetFarmingPoolAddress(farmingPoolAddrAcc); err != nil {
-					return err
-				}
+		if p.GetFarmingPoolAddress() != "" {
+			farmingPoolAddrAcc, err := sdk.AccAddressFromBech32(p.GetFarmingPoolAddress())
+			if err != nil {
+				return err
 			}
-
-			if p.GetTerminationAddress() != "" {
-				terminationAddrAcc, err := sdk.AccAddressFromBech32(p.GetTerminationAddress())
-				if err != nil {
-					return err
-				}
-				if err := plan.SetTerminationAddress(terminationAddrAcc); err != nil {
-					return err
-				}
+			if err := plan.SetFarmingPoolAddress(farmingPoolAddrAcc); err != nil {
+				return err
 			}
+		}
 
-			if p.GetStakingCoinWeights() != nil {
-				if err := plan.SetStakingCoinWeights(p.GetStakingCoinWeights()); err != nil {
-					return err
-				}
+		if p.GetTerminationAddress() != "" {
+			terminationAddrAcc, err := sdk.AccAddressFromBech32(p.GetTerminationAddress())
+			if err != nil {
+				return err
+			}
+			if err := plan.SetTerminationAddress(terminationAddrAcc); err != nil {
+				return err
 			}
+		}
 
-			if p.GetStartTime() != nil {
-				if err := plan.SetStartTime(*p.GetStartTime()); err != nil {
-					return err
-				}
+		if p.GetStakingCoinWeights() != nil {
+			if err := plan.SetStakingCoinWeights(p.GetStakingCoinWeights()); err != nil {
+				return err
 			}
+		}
 
-			if p.GetEndTime() != nil {
-				if err := plan.SetEndTime(*p.GetEndTime()); err != nil {
-					return err
-				}
+		if p.GetStartTime() != nil {
+			if err := plan.SetStartTime(*p.GetStartTime()); err != nil {
+				return err
 			}
+		}
 
-			// change the plan to fixed amount plan if an epoch amount exists
-			if p.GetEpochAmount().IsAllPositive() {
-				plan = types.NewFixedAmountPlan(plan.GetBasePlan(), p.GetEpochAmount())
+		if p.GetEndTime() != nil {
+			if err := plan.SetEndTime(*p.GetEndTime()); err != nil {
+				return err
 			}
+		}
 
-			k.SetPlan(ctx, plan)
+		if p.IsForFixedAmountPlan() {
+			// change the plan to fixed amount plan
+			plan = types.NewFixedAmountPlan(plan.GetBasePlan(), p.GetEpochAmount())
 
 			logger := k.Logger(ctx)
 			logger.Info("updated public fixed amount plan", "fixed_amount_plan", plan)
 
-		} else if p.EpochRatio.IsPositive() {
-			if p.GetName() != "" {
-				if err := plan.SetName(p.GetName()); err != nil {
-					return err
-				}
-			}
-
-			if p.GetFarmingPoolAddress() != "" {
-				farmingPoolAddrAcc, err := sdk.AccAddressFromBech32(p.GetFarmingPoolAddress())
-				if err != nil {
-					return err
-				}
-				if err := plan.SetFarmingPoolAddress(farmingPoolAddrAcc); err != nil {
-					return err
-				}
-			}
-
-			if p.GetTerminationAddress() != "" {
-				terminationAddrAcc, err := sdk.AccAddressFromBech32(p.GetTerminationAddress())
-				if err != nil {
-					return err
-				}
-				if err := plan.SetTerminationAddress(terminationAddrAcc); err != nil {
-					return err
-				}
-			}
-
-			if p.GetStakingCoinWeights() != nil {
-				if err := plan.SetStakingCoinWeights(p.GetStakingCoinWeights()); err != nil {
-					return err
-				}
-			}
-
-			if p.GetStartTime() != nil {
-				if err := plan.SetStartTime(*p.GetStartTime()); err != nil {
-					return err
-				}
-			}
-
-			if p.GetEndTime() != nil {
-				if err := plan.SetEndTime(*p.GetEndTime()); err != nil {
-					return err
-				}
-			}
-
-			// change the plan to ratio plan if an epoch ratio exists
-			if p.EpochRatio.IsPositive() {
-				plan = types.NewRatioPlan(plan.GetBasePlan(), p.EpochRatio)
-			}
-
-			k.SetPlan(ctx, plan)
+		} else if p.IsForRatioPlan() {
+			// change the plan to ratio plan
+			plan = types.NewRatioPlan(plan.GetBasePlan(), p.EpochRatio)
 
 			logger := k.Logger(ctx)
 			logger.Info("updated public ratio plan", "ratio_plan", plan)
-
 		}
+
+		k.SetPlan(ctx, plan)
 	}
 
 	return nil
 }
 
-// DeletePublicPlanProposal delets public plan proposal once the governance proposal is passed.
+// DeletePublicPlanProposal deletes public plan proposal once the governance proposal is passed.
 func (k Keeper) DeletePublicPlanProposal(ctx sdk.Context, proposals []*types.DeleteRequestProposal) error {
 	for _, p := range proposals {
 		plan, found := k.GetPlan(ctx, p.GetPlanId())

x/farming/keeper/proposal_handler_test.go

@@ -29,7 +29,7 @@ func (suite *KeeperTestSuite) TestValidateAddPublicPlanProposal() {
 				types.ParseTime("2021-08-01T00:00:00Z"),
 				types.ParseTime("2021-08-30T00:00:00Z"),
 				sdk.NewCoins(sdk.NewInt64Coin(denom3, 100_000_000)),
-				sdk.ZeroDec(),
+				sdk.Dec{},
 			)},
 			nil,
 		},
@@ -125,7 +125,7 @@ func (suite *KeeperTestSuite) TestValidateAddPublicPlanProposal() {
 				sdk.NewCoins(sdk.NewInt64Coin(denom3, 1)),
 				sdk.NewDec(1),
 			)},
-			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "only one of epoch amount or epoch ratio must be provided"),
+			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "exactly one of epoch amount or epoch ratio must be provided"),
 		},
 		{
 			"epoch amount & epoch ratio case #2",
@@ -142,7 +142,7 @@ func (suite *KeeperTestSuite) TestValidateAddPublicPlanProposal() {
 				sdk.NewCoins(),
 				sdk.ZeroDec(),
 			)},
-			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "only one of epoch amount or epoch ratio must be provided"),
+			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "exactly one of epoch amount or epoch ratio must be provided"),
 		},
 	} {
 		suite.Run(tc.name, func() {
@@ -155,7 +155,9 @@ func (suite *KeeperTestSuite) TestValidateAddPublicPlanProposal() {
 			}
 
 			err := proposal.ValidateBasic()
-			if err == nil {
+			if tc.expectedErr == nil {
+				suite.NoError(err)
+
 				err := keeper.HandlePublicPlanProposal(suite.ctx, suite.keeper, proposal)
 				suite.Require().NoError(err)
 
@@ -320,7 +322,7 @@ func (suite *KeeperTestSuite) TestValidateUpdatePublicPlanProposal() {
 					sdk.NewCoins(sdk.NewInt64Coin("stake", 100_000)),
 					plan.(*types.RatioPlan).EpochRatio,
 				)},
-			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "only one of epoch amount or epoch ratio must be provided"),
+			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "at most one of epoch amount or epoch ratio must be provided"),
 		},
 		{
 			"epoch amount & epoch ratio case #2",
@@ -333,10 +335,10 @@ func (suite *KeeperTestSuite) TestValidateUpdatePublicPlanProposal() {
 					plan.GetStakingCoinWeights(),
 					plan.GetStartTime(),
 					plan.GetEndTime(),
-					sdk.NewCoins(sdk.NewInt64Coin("stake", 0)),
+					sdk.Coins{sdk.NewInt64Coin("stake", 0)},
 					sdk.ZeroDec(),
 				)},
-			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "only one of epoch amount or epoch ratio must be provided"),
+			sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "invalid epoch amount: coin 0stake amount is not positive"),
 		},
 	} {
 		suite.Run(tc.name, func() {
@@ -349,7 +351,9 @@ func (suite *KeeperTestSuite) TestValidateUpdatePublicPlanProposal() {
 			}
 
 			err := proposal.ValidateBasic()
-			if err == nil {
+			if tc.expectedErr == nil {
+				suite.NoError(err)
+
 				err := keeper.HandlePublicPlanProposal(suite.ctx, suite.keeper, proposal)
 				suite.Require().NoError(err)
 

x/farming/simulation/proposals.go

@@ -15,7 +15,7 @@ import (
 /*
 [TODO]:
 	We need to come up with better ways to simulate public plan proposals.
-	Currently, the details are igored and only basic logics are written to simulate.
+	Currently, the details are ignored and only basic logics are written to simulate.
 
 	These are some of the following considerations that i think need to be discussed and addressed:
 	1. Randomize staking coin weights (single or multiple denoms)
@@ -78,7 +78,6 @@ func SimulateAddPublicPlanProposal(ak types.AccountKeeper, bk types.BankKeeper,
 			StartTime:          ctx.BlockTime(),
 			EndTime:            ctx.BlockTime().AddDate(0, 1, 0),
 			EpochAmount:        sdk.NewCoins(sdk.NewInt64Coin(poolCoins[r.Intn(3)].Denom, int64(simtypes.RandIntBetween(r, 10_000_000, 1_000_000_000)))),
-			EpochRatio:         sdk.ZeroDec(),
 		}
 		addRequests := []*types.AddRequestProposal{req}
 

x/farming/types/msgs.go

@@ -57,15 +57,9 @@ func (msg MsgCreateFixedAmountPlan) ValidateBasic() error {
 	if !msg.EndTime.After(msg.StartTime) {
 		return sdkerrors.Wrapf(ErrInvalidPlanEndTime, "end time %s must be greater than start time %s", msg.EndTime.Format(time.RFC3339), msg.StartTime.Format(time.RFC3339))
 	}
-	if msg.StakingCoinWeights.Empty() {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "staking coin weights must not be empty")
-	}
-	if err := msg.StakingCoinWeights.Validate(); err != nil {
+	if err := ValidateStakingCoinTotalWeights(msg.StakingCoinWeights); err != nil {
 		return err
 	}
-	if ok := ValidateStakingCoinTotalWeights(msg.StakingCoinWeights); !ok {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "total weight must be 1")
-	}
 	if msg.EpochAmount.Empty() {
 		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "epoch amount must not be empty")
 	}
@@ -125,17 +119,11 @@ func (msg MsgCreateRatioPlan) ValidateBasic() error {
 	if !msg.EndTime.After(msg.StartTime) {
 		return sdkerrors.Wrapf(ErrInvalidPlanEndTime, "end time %s must be greater than start time %s", msg.EndTime.Format(time.RFC3339), msg.StartTime.Format(time.RFC3339))
 	}
-	if msg.StakingCoinWeights.Empty() {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "staking coin weights must not be empty")
-	}
-	if err := msg.StakingCoinWeights.Validate(); err != nil {
+	if err := ValidateStakingCoinTotalWeights(msg.StakingCoinWeights); err != nil {
 		return err
 	}
-	if ok := ValidateStakingCoinTotalWeights(msg.StakingCoinWeights); !ok {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "total weight must be 1")
-	}
-	if !msg.EpochRatio.IsPositive() {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "invalid epoch ratio")
+	if err := ValidateEpochRatio(msg.EpochRatio); err != nil {
+		return err
 	}
 	if msg.EpochRatio.GT(sdk.NewDec(1)) {
 		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "invalid epoch ratio")

x/farming/types/msgs_test.go

@@ -119,7 +119,7 @@ func TestMsgCreateRatioPlan(t *testing.T) {
 			),
 		},
 		{
-			"invalid epoch ratio: invalid request",
+			"epoch ratio must be positive: -1.000000000000000000: invalid request",
 			types.NewMsgCreateRatioPlan(
 				name, creatorAddr, stakingCoinWeights,
 				startTime, endTime, sdk.NewDec(-1),

x/farming/types/plan.go

@@ -177,15 +177,8 @@ func (plan BasePlan) Validate() error {
 	if len(plan.Name) > MaxNameLength {
 		return sdkerrors.Wrapf(ErrInvalidPlanNameLength, "plan name cannot be longer than max length of %d", MaxNameLength)
 	}
-
-	if plan.StakingCoinWeights.Empty() {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "staking coin weights must not be empty")
-	}
-	if err := plan.StakingCoinWeights.Validate(); err != nil {
-		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid staking coin weights: %v", err)
-	}
-	if ok := ValidateStakingCoinTotalWeights(plan.StakingCoinWeights); !ok {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "total weight must be 1")
+	if err := ValidateStakingCoinTotalWeights(plan.StakingCoinWeights); err != nil {
+		return err
 	}
 	if !plan.EndTime.After(plan.StartTime) {
 		return sdkerrors.Wrapf(ErrInvalidPlanEndTime, "end time %s must be greater than start time %s", plan.EndTime, plan.StartTime)
@@ -301,6 +294,25 @@ func ValidateTotalEpochRatio(i interface{}) error {
 	return nil
 }
 
+// ValidateEpochRatio validate a epoch ratio that must be positive and less than 1.
+func ValidateEpochRatio(epochRatio sdk.Dec) error {
+	if !epochRatio.IsPositive() {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "epoch ratio must be positive: %s", epochRatio)
+	}
+	if epochRatio.GT(sdk.OneDec()) {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "epoch ratio must be less than 1: %s", epochRatio)
+	}
+	return nil
+}
+
+// ValidateEpochAmount validate a epoch amount that must be valid coins.
+func ValidateEpochAmount(epochAmount sdk.Coins) error {
+	if err := epochAmount.Validate(); err != nil {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid epoch amount: %v", err)
+	}
+	return nil
+}
+
 // PackPlan converts PlanI to Any
 func PackPlan(plan PlanI) (*codectypes.Any, error) {
 	any, err := codectypes.NewAnyWithValue(plan)
@@ -349,12 +361,21 @@ func UnpackPlans(plansAny []*codectypes.Any) ([]PlanI, error) {
 }
 
 // ValidateStakingCoinTotalWeights validates the total staking coin weights must be equal to 1.
-func ValidateStakingCoinTotalWeights(weights sdk.DecCoins) bool {
+func ValidateStakingCoinTotalWeights(weights sdk.DecCoins) error {
+	if weights.Empty() {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "staking coin weights must not be empty")
+	}
+	if err := weights.Validate(); err != nil {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid staking coin weights: %v", err)
+	}
 	totalWeight := sdk.ZeroDec()
 	for _, w := range weights {
 		totalWeight = totalWeight.Add(w.Amount)
 	}
-	return totalWeight.Equal(sdk.NewDec(1))
+	if !totalWeight.Equal(sdk.OneDec()) {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "total weight must be 1")
+	}
+	return nil
 }
 
 // IsPlanActiveAt returns if the plan is active at given time t.