From 4ccb6b81b3846c823852ee9b4df7f5fea7c5f353 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Wed, 15 Feb 2023 12:28:54 -0800 Subject: [PATCH 1/7] wip --- tests/e2e/normal_operations.go | 54 +++++++++++++++++++++++++++++++++ testutil/e2e/debug_test.go | 8 +++++ x/ccv/consumer/keeper/keeper.go | 23 ++++++++++++++ x/ccv/provider/keeper/keeper.go | 22 ++++++++++++++ 4 files changed, 107 insertions(+) diff --git a/tests/e2e/normal_operations.go b/tests/e2e/normal_operations.go index 5fc6689467..3074a6e00b 100644 --- a/tests/e2e/normal_operations.go +++ b/tests/e2e/normal_operations.go @@ -1,11 +1,65 @@ package e2e import ( + "reflect" + "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" ) +// TestProviderExternalKeepers tests that the provider keeper is initialized with non-zero and +// non-nil values for all its fields. This is a direct test of the provider app initer given to the test suite. +// +// TODO: test against bad gaia version +func (s CCVTestSuite) TestProviderExternalKeepers() { + + providerKeeper := s.providerApp.GetProviderKeeper() + + cdc, storeKey, paramSpace, scopedKeeper, channelKeeper, portKeeper, connectionKeeper, + accountKeeper, clientKeeper, stakingKeeper, slashingKeeper, evidenceKeeper, + feeColl := providerKeeper.ExposeAllFields() + + s.Require().NotZero(reflect.ValueOf(cdc)) + s.Require().NotZero(reflect.ValueOf(storeKey)) + s.Require().NotZero(reflect.ValueOf(paramSpace)) + s.Require().NotZero(reflect.ValueOf(scopedKeeper)) + s.Require().NotZero(reflect.ValueOf(channelKeeper)) + s.Require().NotZero(reflect.ValueOf(portKeeper)) + s.Require().NotZero(reflect.ValueOf(connectionKeeper)) + s.Require().NotZero(reflect.ValueOf(accountKeeper)) + s.Require().NotZero(reflect.ValueOf(clientKeeper)) + s.Require().NotZero(reflect.ValueOf(stakingKeeper)) + s.Require().NotZero(reflect.ValueOf(slashingKeeper)) + s.Require().NotZero(reflect.ValueOf(evidenceKeeper)) + s.Require().NotZero(reflect.ValueOf(feeColl)) +} + +// TestConsumerExternalKeepers tests that the consumer keeper is initialized with non-zero and +// non-nil values for all its fields. This is a direct test of the consumer app initer given to the test suite. +func (s CCVTestSuite) TestConsumerExternalKeepers() { + consumerKeeper := s.consumerApp.GetConsumerKeeper() + + storeKey, cdc, paramSpace, scopedKeeper, channelKeeper, portKeeper, connectionKeeper, + clientKeeper, slashingKeeper, bankKeeper, accountKeeper, ibcTransferKeeper, + ibcCoreKeeper, feeColl := consumerKeeper.ExposeAllFields() + + s.Require().NotZero(reflect.ValueOf(storeKey)) + s.Require().NotZero(reflect.ValueOf(cdc)) + s.Require().NotZero(reflect.ValueOf(paramSpace)) + s.Require().NotZero(reflect.ValueOf(scopedKeeper)) + s.Require().NotZero(reflect.ValueOf(channelKeeper)) + s.Require().NotZero(reflect.ValueOf(portKeeper)) + s.Require().NotZero(reflect.ValueOf(connectionKeeper)) + s.Require().NotZero(reflect.ValueOf(clientKeeper)) + s.Require().NotZero(reflect.ValueOf(slashingKeeper)) + s.Require().NotZero(reflect.ValueOf(bankKeeper)) + s.Require().NotZero(reflect.ValueOf(accountKeeper)) + s.Require().NotZero(reflect.ValueOf(ibcTransferKeeper)) + s.Require().NotZero(reflect.ValueOf(ibcCoreKeeper)) + s.Require().NotZero(reflect.ValueOf(feeColl)) +} + // Tests the tracking of historical info in the context of new blocks being committed func (k CCVTestSuite) TestHistoricalInfo() { consumerKeeper := k.consumerApp.GetConsumerKeeper() diff --git a/testutil/e2e/debug_test.go b/testutil/e2e/debug_test.go index 75cf6b420f..0f3c180705 100644 --- a/testutil/e2e/debug_test.go +++ b/testutil/e2e/debug_test.go @@ -100,6 +100,14 @@ func TestConsumerPacketSendExpiredClient(t *testing.T) { // Normal operations tests // +func TestProviderExternalKeepers(t *testing.T) { + runCCVTestByName(t, "TestProviderExternalKeepers") +} + +func TestConsumerExternalKeepers(t *testing.T) { + runCCVTestByName(t, "TestConsumerExternalKeepers") +} + func TestHistoricalInfo(t *testing.T) { runCCVTestByName(t, "TestHistoricalInfo") } diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go index 95f72689e0..ab971b0a9c 100644 --- a/x/ccv/consumer/keeper/keeper.go +++ b/x/ccv/consumer/keeper/keeper.go @@ -76,6 +76,29 @@ func NewKeeper( } } +// ExposeAllFields exposes (only by value) all the un-exported fields of the ccv consumer keeper, +// purely for testing purposes. DO NOT Use this method outside of testing, as it breaks encapsulation. +func (k Keeper) ExposeAllFields() ( + sdk.StoreKey, + codec.BinaryCodec, + paramtypes.Subspace, + ccv.ScopedKeeper, + ccv.ChannelKeeper, + ccv.PortKeeper, + ccv.ConnectionKeeper, + ccv.ClientKeeper, + ccv.SlashingKeeper, + ccv.BankKeeper, + ccv.AccountKeeper, + ccv.IBCTransferKeeper, + ccv.IBCCoreKeeper, + string) { + + return k.storeKey, k.cdc, k.paramStore, k.scopedKeeper, k.channelKeeper, k.portKeeper, + k.connectionKeeper, k.clientKeeper, k.slashingKeeper, k.bankKeeper, k.authKeeper, + k.ibcTransferKeeper, k.ibcCoreKeeper, k.feeCollectorName +} + // Logger returns a module-specific logger. func (k Keeper) Logger(ctx sdk.Context) log.Logger { return ctx.Logger().With("module", "x/"+host.ModuleName+"-"+types.ModuleName) diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index dc612b110a..c9f38286e3 100644 --- a/x/ccv/provider/keeper/keeper.go +++ b/x/ccv/provider/keeper/keeper.go @@ -73,6 +73,28 @@ func NewKeeper( } } +// ExposeAllFields exposes (only by value) all the un-exported fields of the ccv provider keeper, +// purely for testing purposes. DO NOT Use this method outside of testing, as it breaks encapsulation. +func (k Keeper) ExposeAllFields() ( + codec.BinaryCodec, + sdk.StoreKey, + paramtypes.Subspace, + ccv.ScopedKeeper, + ccv.ChannelKeeper, + ccv.PortKeeper, + ccv.ConnectionKeeper, + ccv.AccountKeeper, + ccv.ClientKeeper, + ccv.StakingKeeper, + ccv.SlashingKeeper, + ccv.EvidenceKeeper, + string) { + + return k.cdc, k.storeKey, k.paramSpace, k.scopedKeeper, k.channelKeeper, + k.portKeeper, k.connectionKeeper, k.accountKeeper, k.clientKeeper, k.stakingKeeper, + k.slashingKeeper, k.evidenceKeeper, k.feeCollectorName +} + // Logger returns a module-specific logger. func (k Keeper) Logger(ctx sdk.Context) log.Logger { return ctx.Logger().With("module", "x/"+host.ModuleName+"-"+types.ModuleName) From 5f726df9012baf75d3d19099066013f49c037017 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Wed, 15 Feb 2023 12:40:08 -0800 Subject: [PATCH 2/7] add len check --- tests/e2e/normal_operations.go | 71 ++++++++++++++++++--------------- testutil/e2e/debug_test.go | 8 ++-- x/ccv/consumer/keeper/keeper.go | 3 +- 3 files changed, 45 insertions(+), 37 deletions(-) diff --git a/tests/e2e/normal_operations.go b/tests/e2e/normal_operations.go index 3074a6e00b..694b9c0c7d 100644 --- a/tests/e2e/normal_operations.go +++ b/tests/e2e/normal_operations.go @@ -8,11 +8,11 @@ import ( tmproto "github.com/tendermint/tendermint/proto/tendermint/types" ) -// TestProviderExternalKeepers tests that the provider keeper is initialized with non-zero and +// TestProviderKeeperFields tests that the provider keeper is initialized with non-zero and // non-nil values for all its fields. This is a direct test of the provider app initer given to the test suite. // // TODO: test against bad gaia version -func (s CCVTestSuite) TestProviderExternalKeepers() { +func (s CCVTestSuite) TestProviderKeeperFields() { providerKeeper := s.providerApp.GetProviderKeeper() @@ -20,44 +20,51 @@ func (s CCVTestSuite) TestProviderExternalKeepers() { accountKeeper, clientKeeper, stakingKeeper, slashingKeeper, evidenceKeeper, feeColl := providerKeeper.ExposeAllFields() - s.Require().NotZero(reflect.ValueOf(cdc)) - s.Require().NotZero(reflect.ValueOf(storeKey)) - s.Require().NotZero(reflect.ValueOf(paramSpace)) - s.Require().NotZero(reflect.ValueOf(scopedKeeper)) - s.Require().NotZero(reflect.ValueOf(channelKeeper)) - s.Require().NotZero(reflect.ValueOf(portKeeper)) - s.Require().NotZero(reflect.ValueOf(connectionKeeper)) - s.Require().NotZero(reflect.ValueOf(accountKeeper)) - s.Require().NotZero(reflect.ValueOf(clientKeeper)) - s.Require().NotZero(reflect.ValueOf(stakingKeeper)) - s.Require().NotZero(reflect.ValueOf(slashingKeeper)) - s.Require().NotZero(reflect.ValueOf(evidenceKeeper)) - s.Require().NotZero(reflect.ValueOf(feeColl)) + s.Require().NotZero(reflect.ValueOf(cdc)) // 1 + s.Require().NotZero(reflect.ValueOf(storeKey)) // 2 + s.Require().NotZero(reflect.ValueOf(paramSpace)) // 3 + s.Require().NotZero(reflect.ValueOf(scopedKeeper)) // 4 + s.Require().NotZero(reflect.ValueOf(channelKeeper)) // 5 + s.Require().NotZero(reflect.ValueOf(portKeeper)) // 6 + s.Require().NotZero(reflect.ValueOf(connectionKeeper)) // 7 + s.Require().NotZero(reflect.ValueOf(accountKeeper)) // 8 + s.Require().NotZero(reflect.ValueOf(clientKeeper)) // 9 + s.Require().NotZero(reflect.ValueOf(stakingKeeper)) // 10 + s.Require().NotZero(reflect.ValueOf(slashingKeeper)) // 11 + s.Require().NotZero(reflect.ValueOf(evidenceKeeper)) // 12 + s.Require().NotZero(reflect.ValueOf(feeColl)) // 13 + + // Ensures we didn't miss any fields + s.Require().Equal(13, reflect.ValueOf(providerKeeper).NumField()) } -// TestConsumerExternalKeepers tests that the consumer keeper is initialized with non-zero and +// TestConsumerKeeperFields tests that the consumer keeper is initialized with non-zero and // non-nil values for all its fields. This is a direct test of the consumer app initer given to the test suite. -func (s CCVTestSuite) TestConsumerExternalKeepers() { +func (s CCVTestSuite) TestConsumerKeeperFields() { consumerKeeper := s.consumerApp.GetConsumerKeeper() storeKey, cdc, paramSpace, scopedKeeper, channelKeeper, portKeeper, connectionKeeper, - clientKeeper, slashingKeeper, bankKeeper, accountKeeper, ibcTransferKeeper, + clientKeeper, slashingKeeper, hooks, bankKeeper, accountKeeper, ibcTransferKeeper, ibcCoreKeeper, feeColl := consumerKeeper.ExposeAllFields() - s.Require().NotZero(reflect.ValueOf(storeKey)) - s.Require().NotZero(reflect.ValueOf(cdc)) - s.Require().NotZero(reflect.ValueOf(paramSpace)) - s.Require().NotZero(reflect.ValueOf(scopedKeeper)) - s.Require().NotZero(reflect.ValueOf(channelKeeper)) - s.Require().NotZero(reflect.ValueOf(portKeeper)) - s.Require().NotZero(reflect.ValueOf(connectionKeeper)) - s.Require().NotZero(reflect.ValueOf(clientKeeper)) - s.Require().NotZero(reflect.ValueOf(slashingKeeper)) - s.Require().NotZero(reflect.ValueOf(bankKeeper)) - s.Require().NotZero(reflect.ValueOf(accountKeeper)) - s.Require().NotZero(reflect.ValueOf(ibcTransferKeeper)) - s.Require().NotZero(reflect.ValueOf(ibcCoreKeeper)) - s.Require().NotZero(reflect.ValueOf(feeColl)) + s.Require().NotZero(reflect.ValueOf(storeKey)) // 1 + s.Require().NotZero(reflect.ValueOf(cdc)) // 2 + s.Require().NotZero(reflect.ValueOf(paramSpace)) // 3 + s.Require().NotZero(reflect.ValueOf(scopedKeeper)) // 4 + s.Require().NotZero(reflect.ValueOf(channelKeeper)) // 5 + s.Require().NotZero(reflect.ValueOf(portKeeper)) // 6 + s.Require().NotZero(reflect.ValueOf(connectionKeeper)) // 7 + s.Require().NotZero(reflect.ValueOf(clientKeeper)) // 8 + s.Require().NotZero(reflect.ValueOf(slashingKeeper)) // 9 + s.Require().NotZero(reflect.ValueOf(hooks)) // 10 + s.Require().NotZero(reflect.ValueOf(bankKeeper)) // 11 + s.Require().NotZero(reflect.ValueOf(accountKeeper)) // 12 + s.Require().NotZero(reflect.ValueOf(ibcTransferKeeper)) // 13 + s.Require().NotZero(reflect.ValueOf(ibcCoreKeeper)) // 14 + s.Require().NotZero(reflect.ValueOf(feeColl)) // 15 + + // Ensures we didn't miss any fields + s.Require().Equal(15, reflect.ValueOf(consumerKeeper).NumField()) } // Tests the tracking of historical info in the context of new blocks being committed diff --git a/testutil/e2e/debug_test.go b/testutil/e2e/debug_test.go index 0f3c180705..2d4d879484 100644 --- a/testutil/e2e/debug_test.go +++ b/testutil/e2e/debug_test.go @@ -100,12 +100,12 @@ func TestConsumerPacketSendExpiredClient(t *testing.T) { // Normal operations tests // -func TestProviderExternalKeepers(t *testing.T) { - runCCVTestByName(t, "TestProviderExternalKeepers") +func TestProviderKeeperFields(t *testing.T) { + runCCVTestByName(t, "TestProviderKeeperFields") } -func TestConsumerExternalKeepers(t *testing.T) { - runCCVTestByName(t, "TestConsumerExternalKeepers") +func TestConsumerKeeperFields(t *testing.T) { + runCCVTestByName(t, "TestConsumerKeeperFields") } func TestHistoricalInfo(t *testing.T) { diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go index ab971b0a9c..e0439d3649 100644 --- a/x/ccv/consumer/keeper/keeper.go +++ b/x/ccv/consumer/keeper/keeper.go @@ -88,6 +88,7 @@ func (k Keeper) ExposeAllFields() ( ccv.ConnectionKeeper, ccv.ClientKeeper, ccv.SlashingKeeper, + ccv.ConsumerHooks, ccv.BankKeeper, ccv.AccountKeeper, ccv.IBCTransferKeeper, @@ -95,7 +96,7 @@ func (k Keeper) ExposeAllFields() ( string) { return k.storeKey, k.cdc, k.paramStore, k.scopedKeeper, k.channelKeeper, k.portKeeper, - k.connectionKeeper, k.clientKeeper, k.slashingKeeper, k.bankKeeper, k.authKeeper, + k.connectionKeeper, k.clientKeeper, k.slashingKeeper, k.hooks, k.bankKeeper, k.authKeeper, k.ibcTransferKeeper, k.ibcCoreKeeper, k.feeCollectorName } From 0ff1afe48584ec9c1bdcd138f5f9cdccb305a327 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Wed, 15 Feb 2023 13:44:33 -0800 Subject: [PATCH 3/7] use reflect's NotZero() --- tests/e2e/normal_operations.go | 58 ++++++++++++++++------------------ 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/tests/e2e/normal_operations.go b/tests/e2e/normal_operations.go index 694b9c0c7d..9ece58eeb7 100644 --- a/tests/e2e/normal_operations.go +++ b/tests/e2e/normal_operations.go @@ -10,8 +10,6 @@ import ( // TestProviderKeeperFields tests that the provider keeper is initialized with non-zero and // non-nil values for all its fields. This is a direct test of the provider app initer given to the test suite. -// -// TODO: test against bad gaia version func (s CCVTestSuite) TestProviderKeeperFields() { providerKeeper := s.providerApp.GetProviderKeeper() @@ -20,19 +18,19 @@ func (s CCVTestSuite) TestProviderKeeperFields() { accountKeeper, clientKeeper, stakingKeeper, slashingKeeper, evidenceKeeper, feeColl := providerKeeper.ExposeAllFields() - s.Require().NotZero(reflect.ValueOf(cdc)) // 1 - s.Require().NotZero(reflect.ValueOf(storeKey)) // 2 - s.Require().NotZero(reflect.ValueOf(paramSpace)) // 3 - s.Require().NotZero(reflect.ValueOf(scopedKeeper)) // 4 - s.Require().NotZero(reflect.ValueOf(channelKeeper)) // 5 - s.Require().NotZero(reflect.ValueOf(portKeeper)) // 6 - s.Require().NotZero(reflect.ValueOf(connectionKeeper)) // 7 - s.Require().NotZero(reflect.ValueOf(accountKeeper)) // 8 - s.Require().NotZero(reflect.ValueOf(clientKeeper)) // 9 - s.Require().NotZero(reflect.ValueOf(stakingKeeper)) // 10 - s.Require().NotZero(reflect.ValueOf(slashingKeeper)) // 11 - s.Require().NotZero(reflect.ValueOf(evidenceKeeper)) // 12 - s.Require().NotZero(reflect.ValueOf(feeColl)) // 13 + s.Require().False(reflect.ValueOf(cdc).IsZero()) // 1 + s.Require().False(reflect.ValueOf(storeKey).IsZero()) // 2 + s.Require().False(reflect.ValueOf(paramSpace).IsZero()) // 3 + s.Require().False(reflect.ValueOf(scopedKeeper).IsZero()) // 4 + s.Require().False(reflect.ValueOf(channelKeeper).IsZero()) // 5 + s.Require().False(reflect.ValueOf(portKeeper).IsZero()) // 6 + s.Require().False(reflect.ValueOf(connectionKeeper).IsZero()) // 7 + s.Require().False(reflect.ValueOf(accountKeeper).IsZero()) // 8 + s.Require().False(reflect.ValueOf(clientKeeper).IsZero()) // 9 + s.Require().False(reflect.ValueOf(stakingKeeper).IsZero()) // 10 + s.Require().False(reflect.ValueOf(slashingKeeper).IsZero()) // 11 + s.Require().False(reflect.ValueOf(evidenceKeeper).IsZero()) // 12 + s.Require().False(reflect.ValueOf(feeColl).IsZero()) // 13 // Ensures we didn't miss any fields s.Require().Equal(13, reflect.ValueOf(providerKeeper).NumField()) @@ -47,21 +45,21 @@ func (s CCVTestSuite) TestConsumerKeeperFields() { clientKeeper, slashingKeeper, hooks, bankKeeper, accountKeeper, ibcTransferKeeper, ibcCoreKeeper, feeColl := consumerKeeper.ExposeAllFields() - s.Require().NotZero(reflect.ValueOf(storeKey)) // 1 - s.Require().NotZero(reflect.ValueOf(cdc)) // 2 - s.Require().NotZero(reflect.ValueOf(paramSpace)) // 3 - s.Require().NotZero(reflect.ValueOf(scopedKeeper)) // 4 - s.Require().NotZero(reflect.ValueOf(channelKeeper)) // 5 - s.Require().NotZero(reflect.ValueOf(portKeeper)) // 6 - s.Require().NotZero(reflect.ValueOf(connectionKeeper)) // 7 - s.Require().NotZero(reflect.ValueOf(clientKeeper)) // 8 - s.Require().NotZero(reflect.ValueOf(slashingKeeper)) // 9 - s.Require().NotZero(reflect.ValueOf(hooks)) // 10 - s.Require().NotZero(reflect.ValueOf(bankKeeper)) // 11 - s.Require().NotZero(reflect.ValueOf(accountKeeper)) // 12 - s.Require().NotZero(reflect.ValueOf(ibcTransferKeeper)) // 13 - s.Require().NotZero(reflect.ValueOf(ibcCoreKeeper)) // 14 - s.Require().NotZero(reflect.ValueOf(feeColl)) // 15 + s.Require().False(reflect.ValueOf(storeKey).IsZero()) // 1 + s.Require().False(reflect.ValueOf(cdc).IsZero()) // 2 + s.Require().False(reflect.ValueOf(paramSpace).IsZero()) // 3 + s.Require().False(reflect.ValueOf(scopedKeeper).IsZero()) // 4 + s.Require().False(reflect.ValueOf(channelKeeper).IsZero()) // 5 + s.Require().False(reflect.ValueOf(portKeeper).IsZero()) // 6 + s.Require().False(reflect.ValueOf(connectionKeeper).IsZero()) // 7 + s.Require().False(reflect.ValueOf(clientKeeper).IsZero()) // 8 + s.Require().False(reflect.ValueOf(slashingKeeper).IsZero()) // 9 + s.Require().False(reflect.ValueOf(hooks).IsZero()) // 10 + s.Require().False(reflect.ValueOf(bankKeeper).IsZero()) // 11 + s.Require().False(reflect.ValueOf(accountKeeper).IsZero()) // 12 + s.Require().False(reflect.ValueOf(ibcTransferKeeper).IsZero()) // 13 + s.Require().False(reflect.ValueOf(ibcCoreKeeper).IsZero()) // 14 + s.Require().False(reflect.ValueOf(feeColl).IsZero()) // 15 // Ensures we didn't miss any fields s.Require().Equal(15, reflect.ValueOf(consumerKeeper).NumField()) From 5f489259f77e5ad463a5941206a8f97c254e9348 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 16 Feb 2023 10:03:52 -0800 Subject: [PATCH 4/7] panics, not tests --- tests/e2e/normal_operations.go | 59 ------------------------ testutil/e2e/debug_test.go | 8 ---- x/ccv/consumer/keeper/keeper.go | 81 +++++++++++++++++++++++---------- x/ccv/provider/keeper/keeper.go | 56 ++++++++++++++++++++++- 4 files changed, 113 insertions(+), 91 deletions(-) diff --git a/tests/e2e/normal_operations.go b/tests/e2e/normal_operations.go index 9ece58eeb7..5fc6689467 100644 --- a/tests/e2e/normal_operations.go +++ b/tests/e2e/normal_operations.go @@ -1,70 +1,11 @@ package e2e import ( - "reflect" - "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" ) -// TestProviderKeeperFields tests that the provider keeper is initialized with non-zero and -// non-nil values for all its fields. This is a direct test of the provider app initer given to the test suite. -func (s CCVTestSuite) TestProviderKeeperFields() { - - providerKeeper := s.providerApp.GetProviderKeeper() - - cdc, storeKey, paramSpace, scopedKeeper, channelKeeper, portKeeper, connectionKeeper, - accountKeeper, clientKeeper, stakingKeeper, slashingKeeper, evidenceKeeper, - feeColl := providerKeeper.ExposeAllFields() - - s.Require().False(reflect.ValueOf(cdc).IsZero()) // 1 - s.Require().False(reflect.ValueOf(storeKey).IsZero()) // 2 - s.Require().False(reflect.ValueOf(paramSpace).IsZero()) // 3 - s.Require().False(reflect.ValueOf(scopedKeeper).IsZero()) // 4 - s.Require().False(reflect.ValueOf(channelKeeper).IsZero()) // 5 - s.Require().False(reflect.ValueOf(portKeeper).IsZero()) // 6 - s.Require().False(reflect.ValueOf(connectionKeeper).IsZero()) // 7 - s.Require().False(reflect.ValueOf(accountKeeper).IsZero()) // 8 - s.Require().False(reflect.ValueOf(clientKeeper).IsZero()) // 9 - s.Require().False(reflect.ValueOf(stakingKeeper).IsZero()) // 10 - s.Require().False(reflect.ValueOf(slashingKeeper).IsZero()) // 11 - s.Require().False(reflect.ValueOf(evidenceKeeper).IsZero()) // 12 - s.Require().False(reflect.ValueOf(feeColl).IsZero()) // 13 - - // Ensures we didn't miss any fields - s.Require().Equal(13, reflect.ValueOf(providerKeeper).NumField()) -} - -// TestConsumerKeeperFields tests that the consumer keeper is initialized with non-zero and -// non-nil values for all its fields. This is a direct test of the consumer app initer given to the test suite. -func (s CCVTestSuite) TestConsumerKeeperFields() { - consumerKeeper := s.consumerApp.GetConsumerKeeper() - - storeKey, cdc, paramSpace, scopedKeeper, channelKeeper, portKeeper, connectionKeeper, - clientKeeper, slashingKeeper, hooks, bankKeeper, accountKeeper, ibcTransferKeeper, - ibcCoreKeeper, feeColl := consumerKeeper.ExposeAllFields() - - s.Require().False(reflect.ValueOf(storeKey).IsZero()) // 1 - s.Require().False(reflect.ValueOf(cdc).IsZero()) // 2 - s.Require().False(reflect.ValueOf(paramSpace).IsZero()) // 3 - s.Require().False(reflect.ValueOf(scopedKeeper).IsZero()) // 4 - s.Require().False(reflect.ValueOf(channelKeeper).IsZero()) // 5 - s.Require().False(reflect.ValueOf(portKeeper).IsZero()) // 6 - s.Require().False(reflect.ValueOf(connectionKeeper).IsZero()) // 7 - s.Require().False(reflect.ValueOf(clientKeeper).IsZero()) // 8 - s.Require().False(reflect.ValueOf(slashingKeeper).IsZero()) // 9 - s.Require().False(reflect.ValueOf(hooks).IsZero()) // 10 - s.Require().False(reflect.ValueOf(bankKeeper).IsZero()) // 11 - s.Require().False(reflect.ValueOf(accountKeeper).IsZero()) // 12 - s.Require().False(reflect.ValueOf(ibcTransferKeeper).IsZero()) // 13 - s.Require().False(reflect.ValueOf(ibcCoreKeeper).IsZero()) // 14 - s.Require().False(reflect.ValueOf(feeColl).IsZero()) // 15 - - // Ensures we didn't miss any fields - s.Require().Equal(15, reflect.ValueOf(consumerKeeper).NumField()) -} - // Tests the tracking of historical info in the context of new blocks being committed func (k CCVTestSuite) TestHistoricalInfo() { consumerKeeper := k.consumerApp.GetConsumerKeeper() diff --git a/testutil/e2e/debug_test.go b/testutil/e2e/debug_test.go index 2d4d879484..75cf6b420f 100644 --- a/testutil/e2e/debug_test.go +++ b/testutil/e2e/debug_test.go @@ -100,14 +100,6 @@ func TestConsumerPacketSendExpiredClient(t *testing.T) { // Normal operations tests // -func TestProviderKeeperFields(t *testing.T) { - runCCVTestByName(t, "TestProviderKeeperFields") -} - -func TestConsumerKeeperFields(t *testing.T) { - runCCVTestByName(t, "TestConsumerKeeperFields") -} - func TestHistoricalInfo(t *testing.T) { runCCVTestByName(t, "TestHistoricalInfo") } diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go index e0439d3649..5fb9b60e9f 100644 --- a/x/ccv/consumer/keeper/keeper.go +++ b/x/ccv/consumer/keeper/keeper.go @@ -3,6 +3,7 @@ package keeper import ( "encoding/binary" "fmt" + "reflect" "time" "github.com/cosmos/cosmos-sdk/codec" @@ -58,7 +59,7 @@ func NewKeeper( paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable()) } - return Keeper{ + k := Keeper{ storeKey: key, cdc: cdc, paramStore: paramSpace, @@ -74,30 +75,64 @@ func NewKeeper( ibcCoreKeeper: ibcCoreKeeper, feeCollectorName: feeCollectorName, } + + k.mustValidateFields() + return k } -// ExposeAllFields exposes (only by value) all the un-exported fields of the ccv consumer keeper, -// purely for testing purposes. DO NOT Use this method outside of testing, as it breaks encapsulation. -func (k Keeper) ExposeAllFields() ( - sdk.StoreKey, - codec.BinaryCodec, - paramtypes.Subspace, - ccv.ScopedKeeper, - ccv.ChannelKeeper, - ccv.PortKeeper, - ccv.ConnectionKeeper, - ccv.ClientKeeper, - ccv.SlashingKeeper, - ccv.ConsumerHooks, - ccv.BankKeeper, - ccv.AccountKeeper, - ccv.IBCTransferKeeper, - ccv.IBCCoreKeeper, - string) { - - return k.storeKey, k.cdc, k.paramStore, k.scopedKeeper, k.channelKeeper, k.portKeeper, - k.connectionKeeper, k.clientKeeper, k.slashingKeeper, k.hooks, k.bankKeeper, k.authKeeper, - k.ibcTransferKeeper, k.ibcCoreKeeper, k.feeCollectorName +// Validates that the provider keeper is initialized with non-zero and +// non-nil values for all its fields. Otherwise this method will panic. +func (k Keeper) mustValidateFields() { + + // Ensures no fields are missed in this validation + if reflect.ValueOf(k).NumField() != 15 { + panic("number of fields in provider keeper is not 15") + } + + // Note 14 fields will be validated, hooks are explicitly set after the constructor + + if reflect.ValueOf(k.storeKey).IsZero() { // 1 + panic("storeKey is zero-valued or nil") + } + if reflect.ValueOf(k.cdc).IsZero() { // 2 + panic("cdc is zero-valued or nil") + } + if reflect.ValueOf(k.paramStore).IsZero() { // 3 + panic("paramStore is zero-valued or nil") + } + if reflect.ValueOf(k.scopedKeeper).IsZero() { // 4 + panic("scopedKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.channelKeeper).IsZero() { // 5 + panic("channelKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.portKeeper).IsZero() { // 6 + panic("portKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.connectionKeeper).IsZero() { // 7 + panic("connectionKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.clientKeeper).IsZero() { // 8 + panic("clientKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.slashingKeeper).IsZero() { // 9 + panic("slashingKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.bankKeeper).IsZero() { // 10 + panic("bankKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.authKeeper).IsZero() { // 11 + panic("authKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.ibcTransferKeeper).IsZero() { // 12 + panic("ibcTransferKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.ibcCoreKeeper).IsZero() { // 13 + panic("ibcCoreKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.feeCollectorName).IsZero() { // 14 + panic("feeCollectorName is zero-valued or nil") + } } // Logger returns a module-specific logger. diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index c9f38286e3..510421f86a 100644 --- a/x/ccv/provider/keeper/keeper.go +++ b/x/ccv/provider/keeper/keeper.go @@ -3,6 +3,7 @@ package keeper import ( "encoding/binary" "fmt" + "reflect" "time" "github.com/cosmos/cosmos-sdk/codec" @@ -56,7 +57,7 @@ func NewKeeper( paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable()) } - return Keeper{ + k := Keeper{ cdc: cdc, storeKey: key, paramSpace: paramSpace, @@ -71,6 +72,59 @@ func NewKeeper( evidenceKeeper: evidenceKeeper, feeCollectorName: feeCollectorName, } + + k.mustValidateFields() + return k +} + +// Validates that the provider keeper is initialized with non-zero and +// non-nil values for all its fields. Otherwise this method will panic. +func (k Keeper) mustValidateFields() { + + // Ensures no fields are missed in this validation + if reflect.ValueOf(k).NumField() != 13 { + panic("number of fields in provider keeper is not 13") + } + + if reflect.ValueOf(k.cdc).IsZero() { // 1 + panic("cdc is zero-valued or nil") + } + if reflect.ValueOf(k.storeKey).IsZero() { // 2 + panic("storeKey is zero-valued or nil") + } + if reflect.ValueOf(k.paramSpace).IsZero() { // 3 + panic("paramSpace is zero-valued or nil") + } + if reflect.ValueOf(k.scopedKeeper).IsZero() { // 4 + panic("scopedKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.channelKeeper).IsZero() { // 5 + panic("channelKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.portKeeper).IsZero() { // 6 + panic("portKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.connectionKeeper).IsZero() { // 7 + panic("connectionKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.accountKeeper).IsZero() { // 8 + panic("accountKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.clientKeeper).IsZero() { // 9 + panic("clientKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.stakingKeeper).IsZero() { // 10 + panic("stakingKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.slashingKeeper).IsZero() { // 11 + panic("slashingKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.evidenceKeeper).IsZero() { // 12 + panic("evidenceKeeper is zero-valued or nil") + } + if reflect.ValueOf(k.feeCollectorName).IsZero() { // 13 + panic("feeCollectorName is zero-valued or nil") + } } // ExposeAllFields exposes (only by value) all the un-exported fields of the ccv provider keeper, From 467f89a6874062cbb8c0492e8ab81bd756e79911 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 16 Feb 2023 10:05:53 -0800 Subject: [PATCH 5/7] rm old --- x/ccv/provider/keeper/keeper.go | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index 510421f86a..9f60f8c4e0 100644 --- a/x/ccv/provider/keeper/keeper.go +++ b/x/ccv/provider/keeper/keeper.go @@ -127,28 +127,6 @@ func (k Keeper) mustValidateFields() { } } -// ExposeAllFields exposes (only by value) all the un-exported fields of the ccv provider keeper, -// purely for testing purposes. DO NOT Use this method outside of testing, as it breaks encapsulation. -func (k Keeper) ExposeAllFields() ( - codec.BinaryCodec, - sdk.StoreKey, - paramtypes.Subspace, - ccv.ScopedKeeper, - ccv.ChannelKeeper, - ccv.PortKeeper, - ccv.ConnectionKeeper, - ccv.AccountKeeper, - ccv.ClientKeeper, - ccv.StakingKeeper, - ccv.SlashingKeeper, - ccv.EvidenceKeeper, - string) { - - return k.cdc, k.storeKey, k.paramSpace, k.scopedKeeper, k.channelKeeper, - k.portKeeper, k.connectionKeeper, k.accountKeeper, k.clientKeeper, k.stakingKeeper, - k.slashingKeeper, k.evidenceKeeper, k.feeCollectorName -} - // Logger returns a module-specific logger. func (k Keeper) Logger(ctx sdk.Context) log.Logger { return ctx.Logger().With("module", "x/"+host.ModuleName+"-"+types.ModuleName) From b89c4d37c007d175e9916b9475a2a867f2992553 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 16 Feb 2023 10:17:46 -0800 Subject: [PATCH 6/7] fix tests --- testutil/keeper/unit_test_helpers.go | 5 +++-- x/ccv/consumer/keeper/distribution_test.go | 2 +- x/ccv/provider/ibc_module_test.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/testutil/keeper/unit_test_helpers.go b/testutil/keeper/unit_test_helpers.go index 3dd5d98299..ab0eeef2ea 100644 --- a/testutil/keeper/unit_test_helpers.go +++ b/testutil/keeper/unit_test_helpers.go @@ -6,6 +6,7 @@ import ( "testing" "time" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" abci "github.com/tendermint/tendermint/abci/types" "github.com/cosmos/cosmos-sdk/codec" @@ -122,7 +123,7 @@ func NewInMemProviderKeeper(params InMemKeeperParams, mocks MockedKeepers) provi mocks.MockSlashingKeeper, mocks.MockAccountKeeper, mocks.MockEvidenceKeeper, - "", + authtypes.FeeCollectorName, ) } @@ -142,7 +143,7 @@ func NewInMemConsumerKeeper(params InMemKeeperParams, mocks MockedKeepers) consu mocks.MockAccountKeeper, mocks.MockIBCTransferKeeper, mocks.MockIBCCoreKeeper, - "", + authtypes.FeeCollectorName, ) } diff --git a/x/ccv/consumer/keeper/distribution_test.go b/x/ccv/consumer/keeper/distribution_test.go index 842e190877..39d7e22e5e 100644 --- a/x/ccv/consumer/keeper/distribution_test.go +++ b/x/ccv/consumer/keeper/distribution_test.go @@ -41,7 +41,7 @@ func TestGetEstimatedNextFeeDistribution(t *testing.T) { // Setup mock calls gomock.InOrder( - mockAccountKeeper.EXPECT().GetModuleAccount(ctx, ""). + mockAccountKeeper.EXPECT().GetModuleAccount(ctx, authTypes.FeeCollectorName). Return(mAcc). Times(1), mockBankKeeper.EXPECT().GetAllBalances(ctx, mAcc.GetAddress()). diff --git a/x/ccv/provider/ibc_module_test.go b/x/ccv/provider/ibc_module_test.go index ab41a7b530..681cb62482 100644 --- a/x/ccv/provider/ibc_module_test.go +++ b/x/ccv/provider/ibc_module_test.go @@ -147,7 +147,7 @@ func TestOnChanOpenTry(t *testing.T) { mocks.MockClientKeeper.EXPECT().GetClientState(ctx, "clientIDToConsumer").Return( &ibctmtypes.ClientState{ChainId: "consumerChainID"}, true, ).AnyTimes(), - mocks.MockAccountKeeper.EXPECT().GetModuleAccount(ctx, "").Return(&moduleAcct).AnyTimes(), + mocks.MockAccountKeeper.EXPECT().GetModuleAccount(ctx, authtypes.FeeCollectorName).Return(&moduleAcct).AnyTimes(), ) tc.mutateParams(¶ms, &providerKeeper) From dad07c6a108b8eb15e655042adee7a760e2a3529 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 16 Feb 2023 10:19:42 -0800 Subject: [PATCH 7/7] Update keeper.go --- x/ccv/consumer/keeper/keeper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go index 5fb9b60e9f..7c4e4e578b 100644 --- a/x/ccv/consumer/keeper/keeper.go +++ b/x/ccv/consumer/keeper/keeper.go @@ -80,7 +80,7 @@ func NewKeeper( return k } -// Validates that the provider keeper is initialized with non-zero and +// Validates that the consumer keeper is initialized with non-zero and // non-nil values for all its fields. Otherwise this method will panic. func (k Keeper) mustValidateFields() {