refactor(group): move ValidateBasic logic to msgServer

tests/e2e/group/tx.go

@@ -318,7 +318,7 @@ func (s *E2ETestSuite) TestTxCreateGroup() {
 				s.commonFlags...,
 			),
 			true,
-			"expected a positive decimal, got 0: invalid decimal string",
+			"weight must be positive",
 			nil,
 			0,
 		},
@@ -862,7 +862,7 @@ func (s *E2ETestSuite) TestTxCreateGroupWithPolicy() {
 				s.commonFlags...,
 			),
 			true,
-			"expected a positive decimal, got 0: invalid decimal string",
+			"weight must be positive",
 			nil,
 			0,
 		},
@@ -2645,15 +2645,20 @@ func (s *E2ETestSuite) createGroupThresholdPolicyWithBalance(adminAddress, group
 	return groupPolicyAddress
 }
 
-func (s *E2ETestSuite) newValidMembers(weights, membersAddress []string) group.MemberRequests {
+func (s *E2ETestSuite) newValidMembers(weights, membersAddress []string) struct{ Members []group.MemberRequest } {
 	s.Require().Equal(len(weights), len(membersAddress))
-	membersValid := group.MemberRequests{}
+	membersValid := []group.MemberRequest{}
 	for i, address := range membersAddress {
-		membersValid.Members = append(membersValid.Members, group.MemberRequest{
+		membersValid = append(membersValid, group.MemberRequest{
 			Address:  address,
 			Weight:   weights[i],
 			Metadata: validMetadata,
 		})
 	}
-	return membersValid
+
+	return struct {
+		Members []group.MemberRequest
+	}{
+		Members: membersValid,
+	}
 }

x/authz/keeper/msg_server.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"strings"
 
+	errorsmod "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/authz"
@@ -120,14 +121,14 @@ func (k Keeper) Exec(goCtx context.Context, msg *authz.MsgExec) (*authz.MsgExecR
 }
 
 func validateMsgs(msgs []sdk.Msg) error {
-	for _, msg := range msgs {
+	for i, msg := range msgs {
 		m, ok := msg.(sdk.HasValidateBasic)
 		if !ok {
 			continue
 		}
 
 		if err := m.ValidateBasic(); err != nil {
-			return err
+			return errorsmod.Wrapf(err, "msg %d", i)
 		}
 	}
 

x/group/client/cli/tx.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
+	"strings"
 
 	"cosmossdk.io/core/address"
 	"github.com/spf13/cobra"
@@ -13,6 +14,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/client/tx"
 	"github.com/cosmos/cosmos-sdk/version"
 	"github.com/cosmos/cosmos-sdk/x/group"
+	"github.com/cosmos/cosmos-sdk/x/group/internal/math"
 )
 
 const (
@@ -95,14 +97,17 @@ Where members.json contains:
 				return err
 			}
 
+			for _, member := range members {
+				if _, err := math.NewPositiveDecFromString(member.Weight); err != nil {
+					return fmt.Errorf("invalid weight %s for %s: weight must be positive", member.Weight, member.Address)
+				}
+			}
+
 			msg := &group.MsgCreateGroup{
 				Admin:    clientCtx.GetFromAddress().String(),
 				Members:  members,
 				Metadata: args[1],
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -162,14 +167,15 @@ Set a member's weight to "0" to delete it.
 				return err
 			}
 
+			if groupID == 0 {
+				return errors.New("group id cannot be 0")
+			}
+
 			msg := &group.MsgUpdateGroupMembers{
 				Admin:         clientCtx.GetFromAddress().String(),
 				MemberUpdates: members,
 				GroupId:       groupID,
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -187,8 +193,7 @@ func MsgUpdateGroupAdminCmd() *cobra.Command {
 		Short: "Update a group's admin",
 		Args:  cobra.ExactArgs(3),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := cmd.Flags().Set(flags.FlagFrom, args[0])
-			if err != nil {
+			if err := cmd.Flags().Set(flags.FlagFrom, args[0]); err != nil {
 				return err
 			}
 
@@ -202,14 +207,19 @@ func MsgUpdateGroupAdminCmd() *cobra.Command {
 				return err
 			}
 
+			if groupID == 0 {
+				return errors.New("group id cannot be 0")
+			}
+
+			if strings.EqualFold(args[0], args[2]) {
+				return errors.New("new admin cannot be the same as the current admin")
+			}
+
 			msg := &group.MsgUpdateGroupAdmin{
 				Admin:    clientCtx.GetFromAddress().String(),
 				NewAdmin: args[2],
 				GroupId:  groupID,
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -242,14 +252,15 @@ func MsgUpdateGroupMetadataCmd() *cobra.Command {
 				return err
 			}
 
+			if groupID == 0 {
+				return errors.New("group id cannot be 0")
+			}
+
 			msg := &group.MsgUpdateGroupMetadata{
 				Admin:    clientCtx.GetFromAddress().String(),
 				Metadata: args[2],
 				GroupId:  groupID,
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -322,11 +333,21 @@ and policy.json contains:
 				return err
 			}
 
+			for _, member := range members {
+				if _, err := math.NewPositiveDecFromString(member.Weight); err != nil {
+					return fmt.Errorf("invalid weight %s for %s: weight must be positive", member.Weight, member.Address)
+				}
+			}
+
 			policy, err := parseDecisionPolicy(clientCtx.Codec, args[4])
 			if err != nil {
 				return err
 			}
 
+			if err := policy.ValidateBasic(); err != nil {
+				return err
+			}
+
 			msg, err := group.NewMsgCreateGroupWithPolicy(
 				clientCtx.GetFromAddress().String(),
 				members,
@@ -339,10 +360,6 @@ and policy.json contains:
 				return err
 			}
 
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
@@ -398,11 +415,19 @@ Here, we can use percentage decision policy when needed, where 0 < percentage <=
 				return err
 			}
 
+			if groupID == 0 {
+				return errors.New("group id cannot be 0")
+			}
+
 			policy, err := parseDecisionPolicy(clientCtx.Codec, args[3])
 			if err != nil {
 				return err
 			}
 
+			if err := policy.ValidateBasic(); err != nil {
+				return err
+			}
+
 			msg, err := group.NewMsgCreateGroupPolicy(
 				clientCtx.GetFromAddress(),
 				groupID,
@@ -412,9 +437,6 @@ Here, we can use percentage decision policy when needed, where 0 < percentage <=
 			if err != nil {
 				return err
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -432,11 +454,14 @@ func MsgUpdateGroupPolicyAdminCmd() *cobra.Command {
 		Short: "Update a group policy admin",
 		Args:  cobra.ExactArgs(3),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := cmd.Flags().Set(flags.FlagFrom, args[0])
-			if err != nil {
+			if err := cmd.Flags().Set(flags.FlagFrom, args[0]); err != nil {
 				return err
 			}
 
+			if strings.EqualFold(args[0], args[2]) {
+				return errors.New("new admin cannot be the same as the current admin")
+			}
+
 			clientCtx, err := client.GetClientTxContext(cmd)
 			if err != nil {
 				return err
@@ -447,9 +472,6 @@ func MsgUpdateGroupPolicyAdminCmd() *cobra.Command {
 				GroupPolicyAddress: args[1],
 				NewAdmin:           args[2],
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -496,10 +518,6 @@ func MsgUpdateGroupPolicyDecisionPolicyCmd(ac address.Codec) *cobra.Command {
 				return err
 			}
 
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
@@ -516,8 +534,7 @@ func MsgUpdateGroupPolicyMetadataCmd() *cobra.Command {
 		Short: "Update a group policy metadata",
 		Args:  cobra.ExactArgs(3),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := cmd.Flags().Set(flags.FlagFrom, args[0])
-			if err != nil {
+			if err := cmd.Flags().Set(flags.FlagFrom, args[0]); err != nil {
 				return err
 			}
 
@@ -531,9 +548,6 @@ func MsgUpdateGroupPolicyMetadataCmd() *cobra.Command {
 				GroupPolicyAddress: args[1],
 				Metadata:           args[2],
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -611,7 +625,6 @@ metadata example:
 			}
 
 			execStr, _ := cmd.Flags().GetString(FlagExec)
-
 			msg, err := group.NewMsgSubmitProposal(
 				prop.GroupPolicyAddress,
 				prop.Proposers,
@@ -625,10 +638,6 @@ metadata example:
 				return err
 			}
 
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
@@ -653,8 +662,7 @@ Parameters:
 `,
 		Args: cobra.ExactArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := cmd.Flags().Set(flags.FlagFrom, args[1])
-			if err != nil {
+			if err := cmd.Flags().Set(flags.FlagFrom, args[1]); err != nil {
 				return err
 			}
 
@@ -668,19 +676,15 @@ Parameters:
 				return err
 			}
 
+			if proposalID == 0 {
+				return fmt.Errorf("invalid proposal id: %d", proposalID)
+			}
+
 			msg := &group.MsgWithdrawProposal{
 				ProposalId: proposalID,
 				Address:    clientCtx.GetFromAddress().String(),
 			}
 
-			if err != nil {
-				return err
-			}
-
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
@@ -739,13 +743,6 @@ Parameters:
 				Metadata:   args[3],
 				Exec:       execFromString(execStr),
 			}
-			if err != nil {
-				return err
-			}
-
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -778,13 +775,6 @@ func MsgExecCmd() *cobra.Command {
 				ProposalId: proposalID,
 				Executor:   clientCtx.GetFromAddress().String(),
 			}
-			if err != nil {
-				return err
-			}
-
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -820,13 +810,14 @@ Parameters:
 				return err
 			}
 
+			if groupID == 0 {
+				return errors.New("group id cannot be 0")
+			}
+
 			msg := &group.MsgLeaveGroup{
 				Address: clientCtx.GetFromAddress().String(),
 				GroupId: groupID,
 			}
-			if err = msg.ValidateBasic(); err != nil {
-				return fmt.Errorf("message validation failed: %w", err)
-			}
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},