refactor(x/consensus): audit QA v0.52 (#21416)

cosmos · Aug 27, 2024 · 8e0efbd · 8e0efbd
1 parent 355f748
commit 8e0efbd
Show file tree

Hide file tree

Showing 6 changed files with 25 additions and 59 deletions.
diff --git a/x/consensus/README.md b/x/consensus/README.md
@@ -31,7 +31,7 @@ it can be updated with governance or the address with authority.
 * Params: `0x05 | ProtocolBuffer(cometbft.ConsensusParams)`
 
 ```protobuf reference
-https://github.com/cosmos/cosmos-sdk/blob/381de6452693a9338371223c232fba0c42773a4b/proto/cosmos/consensus/v1/consensus.proto#L11-L18
+https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/consensus/proto/cosmos/consensus/v1/consensus.proto#L9-L15
 ```
 
 ## Keepers
@@ -45,22 +45,14 @@ The consensus module provides methods to Set and Get consensus params. It is rec
 Update consensus params.
 
 ```protobuf reference
-https://github.com/cosmos/cosmos-sdk/blob/381de6452693a9338371223c232fba0c42773a4b/proto/cosmos/consensus/v1/tx.proto#L12-L47
+https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/consensus/proto/cosmos/consensus/v1/tx.proto#L23-L44
 ```
 
 The message will fail under the following conditions:
 
 * The signer is not the set authority 
 * Not all values are set
 
-## Consensus Messages
-
-The consensus module has a consensus message that is used to set the consensus params when the chain initializes. It is similar to the `UpdateParams` message but it is only used once at the start of the chain.
-
-```protobuf reference
-https://github.com/cosmos/cosmos-sdk/blob/381de6452693a9338371223c232fba0c42773a4b/proto/cosmos/consensus/v1/consensus.proto#L9-L24
-```
-
 ## Events
 
 The consensus module emits the following events:

diff --git a/x/consensus/exported/exported.go b/x/consensus/exported/exported.go
@@ -6,13 +6,10 @@ import (
 	cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
 )
 
-type (
-
-	// ConsensusParamSetter defines the interface fulfilled by BaseApp's
-	// ParamStore which allows setting its appVersion field.
-	ConsensusParamSetter interface {
-		Get(ctx context.Context) (cmtproto.ConsensusParams, error)
-		Has(ctx context.Context) (bool, error)
-		Set(ctx context.Context, cp cmtproto.ConsensusParams) error
-	}
-)
+// ConsensusParamSetter defines the interface fulfilled by BaseApp's
+// ParamStore which allows setting its appVersion field.
+type ConsensusParamSetter interface {
+	Get(ctx context.Context) (cmtproto.ConsensusParams, error)
+	Has(ctx context.Context) (bool, error)
+	Set(ctx context.Context, cp cmtproto.ConsensusParams) error
+}
diff --git a/x/consensus/keeper/keeper.go b/x/consensus/keeper/keeper.go
@@ -29,6 +29,7 @@ type Keeper struct {
 
 var _ exported.ConsensusParamSetter = Keeper{}.ParamsStore
 
+// NewKeeper creates a new Keeper instance.
 func NewKeeper(cdc codec.BinaryCodec, env appmodule.Environment, authority string) Keeper {
 	sb := collections.NewSchemaBuilder(env.KVStoreService)
 	return Keeper{
@@ -38,21 +39,19 @@ func NewKeeper(cdc codec.BinaryCodec, env appmodule.Environment, authority strin
 	}
 }
 
+// GetAuthority returns the authority address for the consensus module.
+// This address has the permission to update consensus parameters.
 func (k *Keeper) GetAuthority() string {
 	return k.authority
 }
 
 // InitGenesis initializes the initial state of the module
 func (k *Keeper) InitGenesis(ctx context.Context) error {
 	value, ok := ctx.Value(corecontext.InitInfoKey).(*types.MsgUpdateParams)
-	if !ok {
+	if !ok || value == nil {
 		// no error for appv1 and appv2
 		return nil
 	}
-	if value == nil {
-		// no error for appv1
-		return nil
-	}
 
 	consensusParams, err := value.ToProtoConsensusParams()
 	if err != nil {
@@ -85,6 +84,7 @@ func (k Keeper) Params(ctx context.Context, _ *types.QueryParamsRequest) (*types
 
 var _ types.MsgServer = Keeper{}
 
+// UpdateParams updates the consensus parameters.
 func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
 	if k.GetAuthority() != msg.Authority {
 		return nil, fmt.Errorf("invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority)
@@ -116,17 +116,15 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (*
 
 // paramCheck validates the consensus params
 func (k Keeper) paramCheck(ctx context.Context, consensusParams cmtproto.ConsensusParams) (*cmttypes.ConsensusParams, error) {
-	paramsProto, err := k.ParamsStore.Get(ctx)
-
 	var params cmttypes.ConsensusParams
-	if err != nil {
-		if errors.Is(err, collections.ErrNotFound) {
-			params = cmttypes.ConsensusParams{}
-		} else {
-			return nil, err
-		}
-	} else {
+
+	paramsProto, err := k.ParamsStore.Get(ctx)
+	if err == nil {
 		params = cmttypes.ConsensusParamsFromProto(paramsProto)
+	} else if errors.Is(err, collections.ErrNotFound) {
+		params = cmttypes.ConsensusParams{}
+	} else {
+		return nil, err
 	}
 
 	nextParams := params.Update(&consensusParams)

diff --git a/x/consensus/keeper/keeper_test.go b/x/consensus/keeper/keeper_test.go
@@ -152,8 +152,6 @@ func (s *KeeperTestSuite) TestGRPCQueryConsensusParams() {
 	}
 
 	for _, tc := range testCases {
-		tc := tc
-
 		s.Run(tc.msg, func() {
 			s.SetupTest(false) // reset
 
@@ -189,8 +187,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 				Validator: defaultConsensusParams.Validator,
 				Evidence:  defaultConsensusParams.Evidence,
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "invalid  params",
@@ -258,8 +254,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					VoteExtensionsEnableHeight: &gogotypes.Int64Value{Value: 300},
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid Feature update - pbts",
@@ -272,8 +266,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					PbtsEnableHeight: &gogotypes.Int64Value{Value: 150},
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid Feature update - vote extensions + pbts",
@@ -287,8 +279,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					PbtsEnableHeight:           &gogotypes.Int64Value{Value: 110},
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name:            "valid noop Feature update - vote extensions + pbts (enabled feature)",
@@ -303,8 +293,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					PbtsEnableHeight:           &gogotypes.Int64Value{Value: 5},
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid (deprecated) ABCI update",
@@ -317,8 +305,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					VoteExtensionsEnableHeight: 90,
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "invalid Feature + (deprecated) ABCI vote extensions update",
@@ -462,8 +448,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					Precision: getDuration(3 * time.Second),
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid Synchrony update - delay",
@@ -476,8 +460,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					MessageDelay: getDuration(10 * time.Second),
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid Synchrony update - precision + delay",
@@ -491,8 +473,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					MessageDelay: getDuration(11 * time.Second),
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid Synchrony update - 0 precision",
@@ -505,8 +485,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					Precision: getDuration(0),
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "valid Synchrony update - 0 delay",
@@ -519,8 +497,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 					MessageDelay: getDuration(0),
 				},
 			},
-			expErr:    false,
-			expErrMsg: "",
 		},
 		{
 			name: "invalid Synchrony update - 0 precision with PBTS set",
@@ -559,7 +535,6 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 	}
 
 	for _, tc := range testCases {
-		tc := tc
 		s.Run(tc.name, func() {
 			s.SetupTest(tc.enabledFeatures)
 			_, err := s.consensusParamsKeeper.UpdateParams(s.ctx, tc.input)

diff --git a/x/consensus/types/codec.go b/x/consensus/types/codec.go
@@ -9,6 +9,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/msgservice"
 )
 
+// RegisterInterfaces registers the interfaces types with the interface registry.
 func RegisterInterfaces(registrar registry.InterfaceRegistrar) {
 	registrar.RegisterImplementations(
 		(*coretransaction.Msg)(nil),

diff --git a/x/consensus/types/msgs.go b/x/consensus/types/msgs.go
@@ -8,6 +8,9 @@ import (
 	"github.com/cosmos/gogoproto/types"
 )
 
+// ToProtoConsensusParams converts MsgUpdateParams to cmtproto.ConsensusParams.
+// It returns an error if any required parameters are missing or if there's a conflict
+// between ABCI and Feature parameters.
 func (msg MsgUpdateParams) ToProtoConsensusParams() (cmtproto.ConsensusParams, error) {
 	if msg.Evidence == nil || msg.Block == nil || msg.Validator == nil {
 		return cmtproto.ConsensusParams{}, errors.New("all parameters must be present")