nits for params migrations (#3853)

* nits for consistency * add logging message in migrations * address review comment * gofumpt --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
cosmos · Jun 15, 2023 · 7850184 · 7850184
1 parent 8ccc873
commit 7850184
Show file tree

Hide file tree

Showing 13 changed files with 124 additions and 30 deletions.
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go
@@ -58,6 +58,7 @@ func (m Migrator) MigrateParams(ctx sdk.Context) error {
 		m.keeper.legacySubspace.GetParamSet(ctx, &params)
 
 		m.keeper.SetParams(ctx, params)
+		m.keeper.Logger(ctx).Info("successfully migrated ica/controller submodule to self-manage params")
 	}
 	return nil
 }
diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs_test.go b/modules/apps/27-interchain-accounts/controller/types/msgs_test.go
@@ -195,8 +195,8 @@ func TestMsgSendTxGetSigners(t *testing.T) {
 	require.Equal(t, []sdk.AccAddress{expSigner}, msg.GetSigners())
 }
 
-// TestMsgUpdateParamsValidation tests ValidateBasic for MsgUpdateParams
-func TestMsgUpdateParamsValidation(t *testing.T) {
+// TestMsgUpdateParamsValidateBasic tests ValidateBasic for MsgUpdateParams
+func TestMsgUpdateParamsValidateBasic(t *testing.T) {
 	testCases := []struct {
 		name    string
 		msg     *types.MsgUpdateParams

diff --git a/modules/apps/27-interchain-accounts/host/keeper/migrations.go b/modules/apps/27-interchain-accounts/host/keeper/migrations.go
@@ -27,6 +27,7 @@ func (m Migrator) MigrateParams(ctx sdk.Context) error {
 			return err
 		}
 		m.keeper.SetParams(ctx, params)
+		m.keeper.Logger(ctx).Info("successfully migrated ica/host submodule to self-manage params")
 	}
 	return nil
 }
diff --git a/modules/apps/27-interchain-accounts/host/types/msgs_test.go b/modules/apps/27-interchain-accounts/host/types/msgs_test.go
@@ -4,8 +4,10 @@ import (
 	"testing"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
 	"github.com/stretchr/testify/require"
+
+	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
+	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 )
 
 func TestMsgUpdateParamsValidateBasic(t *testing.T) {
@@ -20,7 +22,7 @@ func TestMsgUpdateParamsValidateBasic(t *testing.T) {
 			"success: valid authority address",
 			func() {
 				msg = &types.MsgUpdateParams{
-					Authority: sdk.AccAddress("authority").String(),
+					Authority: ibctesting.TestAccAddress,
 					Params:    types.DefaultParams(),
 				}
 			},
@@ -39,7 +41,7 @@ func TestMsgUpdateParamsValidateBasic(t *testing.T) {
 			"failure: invalid allowed message",
 			func() {
 				msg = &types.MsgUpdateParams{
-					Authority: sdk.AccAddress("authority").String(),
+					Authority: ibctesting.TestAccAddress,
 					Params: types.Params{
 						AllowMessages: []string{""},
 					},
@@ -64,10 +66,26 @@ func TestMsgUpdateParamsValidateBasic(t *testing.T) {
 }
 
 func TestMsgUpdateParamsGetSigners(t *testing.T) {
-	authority := sdk.AccAddress("authority")
-	msg := types.MsgUpdateParams{
-		Authority: authority.String(),
-		Params:    types.DefaultParams(),
+	testCases := []struct {
+		name    string
+		address sdk.AccAddress
+		expPass bool
+	}{
+		{"success: valid address", sdk.AccAddress(ibctesting.TestAccAddress), true},
+		{"failure: nil address", nil, false},
+	}
+
+	for _, tc := range testCases {
+		msg := types.MsgUpdateParams{
+			Authority: tc.address.String(),
+			Params:    types.DefaultParams(),
+		}
+		if tc.expPass {
+			require.Equal(t, []sdk.AccAddress{tc.address}, msg.GetSigners())
+		} else {
+			require.Panics(t, func() {
+				msg.GetSigners()
+			})
+		}
 	}
-	require.Equal(t, []sdk.AccAddress{authority}, msg.GetSigners())
 }
diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go
@@ -146,7 +146,7 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
 
 	controllerMigrator := controllerkeeper.NewMigrator(am.controllerKeeper)
 	if err := cfg.RegisterMigration(types.ModuleName, 1, controllerMigrator.AssertChannelCapabilityMigrations); err != nil {
-		panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 1 to 2: %v", err))
+		panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 1 to 2 (channel capabilities owned by controller submodule check): %v", err))
 	}
 
 	hostMigrator := hostkeeper.NewMigrator(am.hostKeeper)
@@ -156,7 +156,7 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
 		}
 		return controllerMigrator.MigrateParams(ctx)
 	}); err != nil {
-		panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 2 to 3: %v", err))
+		panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 2 to 3 (self-managed params migration): %v", err))
 	}
 }
 

diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go
@@ -113,8 +113,8 @@ func (k Keeper) SetPort(ctx sdk.Context, portID string) {
 func (k Keeper) GetParams(ctx sdk.Context) types.Params {
 	store := ctx.KVStore(k.storeKey)
 	bz := store.Get([]byte(types.ParamsKey))
-	if bz == nil { // only panics on unset params and not on empty params
-		panic("ibc transfer params are not set in store")
+	if bz == nil { // only panic on unset params and not on empty params
+		panic("transfer params are not set in store")
 	}
 
 	var params types.Params

diff --git a/modules/apps/transfer/keeper/migrations.go b/modules/apps/transfer/keeper/migrations.go
@@ -26,6 +26,7 @@ func (m Migrator) MigrateParams(ctx sdk.Context) error {
 	m.keeper.legacySubspace.GetParamSet(ctx, &params)
 
 	m.keeper.SetParams(ctx, params)
+	m.keeper.Logger(ctx).Info("successfully migrated transfer app self-manage params")
 	return nil
 }
 
@@ -81,8 +82,7 @@ func (m Migrator) MigrateTotalEscrowForDenom(ctx sdk.Context) error {
 		m.keeper.SetTotalEscrowForDenom(ctx, totalEscrow)
 	}
 
-	logger := m.keeper.Logger(ctx)
-	logger.Info("successfully set total escrow for %d denominations", totalEscrowed.Len())
+	m.keeper.Logger(ctx).Info("successfully set total escrow for %d denominations", totalEscrowed.Len())
 	return nil
 }
 

diff --git a/modules/apps/transfer/module.go b/modules/apps/transfer/module.go
@@ -105,15 +105,15 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
 
 	m := keeper.NewMigrator(am.keeper)
 	if err := cfg.RegisterMigration(types.ModuleName, 1, m.MigrateTraces); err != nil {
-		panic(fmt.Sprintf("failed to migrate transfer app from version 1 to 2: %v", err))
+		panic(fmt.Sprintf("failed to migrate transfer app from version 1 to 2 (denom trace format migration): %v", err))
 	}
 
 	if err := cfg.RegisterMigration(types.ModuleName, 2, m.MigrateTotalEscrowForDenom); err != nil {
-		panic(fmt.Sprintf("failed to migrate transfer app from version 2 to 3: %v", err))
+		panic(fmt.Sprintf("failed to migrate transfer app from version 2 to 3 (total escrow entry migration): %v", err))
 	}
 
 	if err := cfg.RegisterMigration(types.ModuleName, 3, m.MigrateParams); err != nil {
-		panic(fmt.Sprintf("failed to migrate params from version 3 to 4: %v", err))
+		panic(fmt.Sprintf("failed to migrate transfer app version 3 to 4 (self-managed params migration): %v", err))
 	}
 }
 

diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go
@@ -102,8 +102,8 @@ func TestMsgTransferGetSigners(t *testing.T) {
 	require.Equal(t, []sdk.AccAddress{addr}, res)
 }
 
-// TestMsgUpdateParamsValidation tests ValidateBasic for MsgUpdateParams
-func TestMsgUpdateParamsValidation(t *testing.T) {
+// TestMsgUpdateParamsValidateBasic tests ValidateBasic for MsgUpdateParams
+func TestMsgUpdateParamsValidateBasic(t *testing.T) {
 	testCases := []struct {
 		name    string
 		msg     *types.MsgUpdateParams
@@ -126,10 +126,26 @@ func TestMsgUpdateParamsValidation(t *testing.T) {
 
 // TestMsgUpdateParamsGetSigners tests GetSigners for MsgUpdateParams
 func TestMsgUpdateParamsGetSigners(t *testing.T) {
-	authority := sdk.AccAddress("authority")
-	msg := types.MsgUpdateParams{
-		Authority: authority.String(),
-		Params:    types.DefaultParams(),
+	testCases := []struct {
+		name    string
+		address sdk.AccAddress
+		expPass bool
+	}{
+		{"success: valid address", sdk.AccAddress(ibctesting.TestAccAddress), true},
+		{"failure: nil address", nil, false},
+	}
+
+	for _, tc := range testCases {
+		msg := types.MsgUpdateParams{
+			Authority: tc.address.String(),
+			Params:    types.DefaultParams(),
+		}
+		if tc.expPass {
+			require.Equal(t, []sdk.AccAddress{tc.address}, msg.GetSigners())
+		} else {
+			require.Panics(t, func() {
+				msg.GetSigners()
+			})
+		}
 	}
-	require.Equal(t, []sdk.AccAddress{authority}, msg.GetSigners())
 }
diff --git a/modules/core/02-client/keeper/migrations.go b/modules/core/02-client/keeper/migrations.go
@@ -44,5 +44,6 @@ func (m Migrator) MigrateParams(ctx sdk.Context) error {
 		return err
 	}
 	m.keeper.SetParams(ctx, params)
+	m.keeper.Logger(ctx).Info("successfully migrated client to self-manage params")
 	return nil
 }
diff --git a/modules/core/02-client/types/msgs_test.go b/modules/core/02-client/types/msgs_test.go
@@ -4,7 +4,9 @@ import (
 	"testing"
 	"time"
 
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/golang/protobuf/proto" //nolint:staticcheck
+	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 
 	"github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
@@ -610,8 +612,8 @@ func (suite *TypesTestSuite) TestMsgSubmitMisbehaviour_ValidateBasic() {
 	}
 }
 
-// TestMsgUpdateParams_ValidateBasic tests ValidateBasic for MsgUpdateParams
-func (suite *TypesTestSuite) TestMsgUpdateParams_ValidateBasic() {
+// TestMsgUpdateParamsValidateBasic tests ValidateBasic for MsgUpdateParams
+func (suite *TypesTestSuite) TestMsgUpdateParamsValidateBasic() {
 	authority := suite.chainA.App.GetIBCKeeper().GetAuthority()
 	testCases := []struct {
 		name    string
@@ -649,3 +651,29 @@ func (suite *TypesTestSuite) TestMsgUpdateParams_ValidateBasic() {
 		}
 	}
 }
+
+// TestMsgUpdateParamsGetSigners tests GetSigners for MsgUpdateParams
+func TestMsgUpdateParamsGetSigners(t *testing.T) {
+	testCases := []struct {
+		name    string
+		address sdk.AccAddress
+		expPass bool
+	}{
+		{"success: valid address", sdk.AccAddress(ibctesting.TestAccAddress), true},
+		{"failure: nil address", nil, false},
+	}
+
+	for _, tc := range testCases {
+		msg := types.MsgUpdateParams{
+			Authority: tc.address.String(),
+			Params:    types.DefaultParams(),
+		}
+		if tc.expPass {
+			require.Equal(t, []sdk.AccAddress{tc.address}, msg.GetSigners())
+		} else {
+			require.Panics(t, func() {
+				msg.GetSigners()
+			})
+		}
+	}
+}
diff --git a/modules/core/03-connection/keeper/migrations.go b/modules/core/03-connection/keeper/migrations.go
@@ -35,5 +35,6 @@ func (m Migrator) MigrateParams(ctx sdk.Context) error {
 		return err
 	}
 	m.keeper.SetParams(ctx, params)
+	m.keeper.Logger(ctx).Info("successfully migrated connection to self-manage params")
 	return nil
 }
diff --git a/modules/core/03-connection/types/msgs_test.go b/modules/core/03-connection/types/msgs_test.go
@@ -11,6 +11,8 @@ import (
 	"github.com/cosmos/cosmos-sdk/store/iavl"
 	"github.com/cosmos/cosmos-sdk/store/rootmulti"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 
 	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
@@ -232,8 +234,8 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenConfirm() {
 	}
 }
 
-// TestMsgUpdateParams_ValidateBasic tests ValidateBasic for MsgUpdateParams
-func (suite *MsgTestSuite) TestMsgUpdateParams_ValidateBasic() {
+// TestMsgUpdateParamsValidateBasic tests ValidateBasic for MsgUpdateParams
+func (suite *MsgTestSuite) TestMsgUpdateParamsValidateBasic() {
 	authority := suite.chainA.App.GetIBCKeeper().GetAuthority()
 	testCases := []struct {
 		name    string
@@ -266,3 +268,29 @@ func (suite *MsgTestSuite) TestMsgUpdateParams_ValidateBasic() {
 		}
 	}
 }
+
+// TestMsgUpdateParamsGetSigners tests GetSigners for MsgUpdateParams
+func TestMsgUpdateParamsGetSigners(t *testing.T) {
+	testCases := []struct {
+		name    string
+		address sdk.AccAddress
+		expPass bool
+	}{
+		{"success: valid address", sdk.AccAddress(ibctesting.TestAccAddress), true},
+		{"failure: nil address", nil, false},
+	}
+
+	for _, tc := range testCases {
+		msg := types.MsgUpdateParams{
+			Authority: tc.address.String(),
+			Params:    types.DefaultParams(),
+		}
+		if tc.expPass {
+			require.Equal(t, []sdk.AccAddress{tc.address}, msg.GetSigners())
+		} else {
+			require.Panics(t, func() {
+				msg.GetSigners()
+			})
+		}
+	}
+}