Fix test by veryfing provider's chain state is clean.

testutil/keeper/unit_test_helpers.go

@@ -207,15 +207,25 @@ func GetNewVSCMaturedPacketData() types.VSCMaturedPacketData {
 }
 
 // SetupForStoppingConsumerChain registers expected mock calls and corresponding state setup
-// which asserts that a consumer chain was properly stopped from StopConsumerChain().
+// which asserts that a consumer chain was properly setup to be later stopped from `StopConsumerChain`.
+// Note: This function only tests that we correctly setup a consumer chain that we could later stop when calling
+// `StopConsumerChain` -- this does NOT necessarily mean that the consumer chain is stopped if this function assertions
+// succeed. Use this function together with `TestProviderStateIsCleanedAfterConsumerChainIsStopped` to verify that the
+// provider's chain state is cleaned after a consumer chain is stopped. Furthermore, note that if `closeChan` is `true`,
+// this function additionally tests that the underlying channel is closed in `StopConsumerChain`.
 func SetupForStoppingConsumerChain(t *testing.T, ctx sdk.Context,
-	providerKeeper *providerkeeper.Keeper, mocks MockedKeepers,
+	providerKeeper *providerkeeper.Keeper, mocks MockedKeepers, closeChan bool,
 ) {
 	t.Helper()
 	expectations := GetMocksForCreateConsumerClient(ctx, &mocks,
 		"chainID", clienttypes.NewHeight(4, 5))
 	expectations = append(expectations, GetMocksForSetConsumerChain(ctx, &mocks, "chainID")...)
-	expectations = append(expectations, GetMocksForStopConsumerChain(ctx, &mocks)...)
+
+	// StopConsumerChain receives a `closeChan` parameter. If the `closeChan` argument is true, then
+	// we register additional expected mock calls that capture the fact that the underlying channel is closed.
+	if closeChan {
+		expectations = append(expectations, GetMocksForStopConsumerChain(ctx, &mocks)...)
+	}
 
 	gomock.InOrder(expectations...)
 
@@ -226,6 +236,43 @@ func SetupForStoppingConsumerChain(t *testing.T, ctx sdk.Context,
 	require.NoError(t, err)
 }
 
+// TestProviderStateIsCleanedAfterConsumerChainIsStopped executes test assertions for the provider's state being cleaned
+// after a stopped consumer chain.
+func TestProviderStateIsCleanedAfterConsumerChainIsStopped(t *testing.T, ctx sdk.Context, providerKeeper providerkeeper.Keeper,
+	expectedChainID, expectedChannelID string,
+) {
+	t.Helper()
+	_, found := providerKeeper.GetConsumerClientId(ctx, expectedChainID)
+	require.False(t, found)
+	_, found = providerKeeper.GetChainToChannel(ctx, expectedChainID)
+	require.False(t, found)
+	_, found = providerKeeper.GetChannelToChain(ctx, expectedChannelID)
+	require.False(t, found)
+	_, found = providerKeeper.GetInitChainHeight(ctx, expectedChainID)
+	require.False(t, found)
+	acks := providerKeeper.GetSlashAcks(ctx, expectedChainID)
+	require.Empty(t, acks)
+	_, found = providerKeeper.GetInitTimeoutTimestamp(ctx, expectedChainID)
+	require.False(t, found)
+
+	require.Empty(t, providerKeeper.GetAllVscSendTimestamps(ctx, expectedChainID))
+
+	// test key assignment state is cleaned
+	require.Empty(t, providerKeeper.GetAllValidatorConsumerPubKeys(ctx, &expectedChainID))
+	require.Empty(t, providerKeeper.GetAllValidatorsByConsumerAddr(ctx, &expectedChainID))
+	require.Empty(t, providerKeeper.GetAllKeyAssignmentReplacements(ctx, expectedChainID))
+	require.Empty(t, providerKeeper.GetAllConsumerAddrsToPrune(ctx, expectedChainID))
+
+	allGlobalEntries := providerKeeper.GetAllGlobalSlashEntries(ctx)
+	for _, entry := range allGlobalEntries {
+		require.NotEqual(t, expectedChainID, entry.ConsumerChainID)
+	}
+
+	slashPacketData, vscMaturedPacketData, _, _ := providerKeeper.GetAllThrottledPacketData(ctx, expectedChainID)
+	require.Empty(t, slashPacketData)
+	require.Empty(t, vscMaturedPacketData)
+}
+
 func GetTestConsumerAdditionProp() *providertypes.ConsumerAdditionProposal {
 	prop := providertypes.NewConsumerAdditionProposal(
 		"chainID",

x/ccv/provider/keeper/proposal_test.go

@@ -472,7 +472,7 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
 		// Note: when expAppendProp is false, no mocks are setup,
 		// meaning no external keeper methods are allowed to be called.
 		if tc.expAppendProp {
-			testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks)
+			testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks, true)
 		}
 
 		tc.setupMocks(ctx, providerKeeper, tc.prop.ChainId)
@@ -526,7 +526,7 @@ func TestStopConsumerChain(t *testing.T) {
 		{
 			description: "valid stop of consumer chain, throttle related queues are cleaned",
 			setup: func(ctx sdk.Context, providerKeeper *providerkeeper.Keeper, mocks testkeeper.MockedKeepers) {
-				testkeeper.SetupForStoppingConsumerChain(t, ctx, providerKeeper, mocks)
+				testkeeper.SetupForStoppingConsumerChain(t, ctx, providerKeeper, mocks, true)
 
 				providerKeeper.QueueGlobalSlashEntry(ctx, providertypes.NewGlobalSlashEntry(
 					ctx.BlockTime(), "chainID", 1, cryptoutil.NewCryptoIdentityFromIntSeed(90).ProviderConsAddress()))
@@ -545,7 +545,7 @@ func TestStopConsumerChain(t *testing.T) {
 		{
 			description: "valid stop of consumer chain, all mock calls hit",
 			setup: func(ctx sdk.Context, providerKeeper *providerkeeper.Keeper, mocks testkeeper.MockedKeepers) {
-				testkeeper.SetupForStoppingConsumerChain(t, ctx, providerKeeper, mocks)
+				testkeeper.SetupForStoppingConsumerChain(t, ctx, providerKeeper, mocks, true)
 			},
 			expErr: false,
 		},
@@ -569,48 +569,12 @@ func TestStopConsumerChain(t *testing.T) {
 			require.NoError(t, err)
 		}
 
-		testProviderStateIsCleaned(t, ctx, providerKeeper, "chainID", "channelID")
+		testkeeper.TestProviderStateIsCleanedAfterConsumerChainIsStopped(t, ctx, providerKeeper, "chainID", "channelID")
 
 		ctrl.Finish()
 	}
 }
 
-// testProviderStateIsCleaned executes test assertions for the proposer's state being cleaned after a stopped consumer chain.
-func testProviderStateIsCleaned(t *testing.T, ctx sdk.Context, providerKeeper providerkeeper.Keeper,
-	expectedChainID, expectedChannelID string,
-) {
-	t.Helper()
-	_, found := providerKeeper.GetConsumerClientId(ctx, expectedChainID)
-	require.False(t, found)
-	_, found = providerKeeper.GetChainToChannel(ctx, expectedChainID)
-	require.False(t, found)
-	_, found = providerKeeper.GetChannelToChain(ctx, expectedChannelID)
-	require.False(t, found)
-	_, found = providerKeeper.GetInitChainHeight(ctx, expectedChainID)
-	require.False(t, found)
-	acks := providerKeeper.GetSlashAcks(ctx, expectedChainID)
-	require.Empty(t, acks)
-	_, found = providerKeeper.GetInitTimeoutTimestamp(ctx, expectedChainID)
-	require.False(t, found)
-
-	require.Empty(t, providerKeeper.GetAllVscSendTimestamps(ctx, expectedChainID))
-
-	// test key assignment state is cleaned
-	require.Empty(t, providerKeeper.GetAllValidatorConsumerPubKeys(ctx, &expectedChainID))
-	require.Empty(t, providerKeeper.GetAllValidatorsByConsumerAddr(ctx, &expectedChainID))
-	require.Empty(t, providerKeeper.GetAllKeyAssignmentReplacements(ctx, expectedChainID))
-	require.Empty(t, providerKeeper.GetAllConsumerAddrsToPrune(ctx, expectedChainID))
-
-	allGlobalEntries := providerKeeper.GetAllGlobalSlashEntries(ctx)
-	for _, entry := range allGlobalEntries {
-		require.NotEqual(t, expectedChainID, entry.ConsumerChainID)
-	}
-
-	slashPacketData, vscMaturedPacketData, _, _ := providerKeeper.GetAllThrottledPacketData(ctx, expectedChainID)
-	require.Empty(t, slashPacketData)
-	require.Empty(t, vscMaturedPacketData)
-}
-
 // TestPendingConsumerRemovalPropDeletion tests the getting/setting
 // and deletion methods for pending consumer removal props
 func TestPendingConsumerRemovalPropDeletion(t *testing.T) {

x/ccv/provider/keeper/relay_test.go

@@ -1,6 +1,7 @@
 package keeper_test
 
 import (
+	"strings"
 	"testing"
 	"time"
 
@@ -715,3 +716,72 @@ func TestSendVSCPacketsToChainFailure(t *testing.T) {
 	// Pending VSC packets should be deleted in StopConsumerChain
 	require.Empty(t, providerKeeper.GetPendingVSCPackets(ctx, "consumerChainID"))
 }
+
+// TestOnTimeoutPacketWithNoChainFound tests the `OnTimeoutPacket` method fails when no chain is found
+func TestOnTimeoutPacketWithNoChainFound(t *testing.T) {
+	// Keeper setup
+	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+
+	// We do not `SetChannelToChain` for "channelID" and therefore `OnTimeoutPacket` fails
+	packet := channeltypes.Packet{
+		SourceChannel: "channelID",
+	}
+	err := providerKeeper.OnTimeoutPacket(ctx, packet)
+	require.Error(t, err)
+	require.True(t, strings.Contains(err.Error(), channeltypes.ErrInvalidChannel.Error()))
+}
+
+// TestOnTimeoutPacketStopsChain tests that the chain is stopped in case of a timeout
+func TestOnTimeoutPacketStopsChain(t *testing.T) {
+	// Keeper setup
+	providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+	providerKeeper.SetParams(ctx, providertypes.DefaultParams())
+
+	testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks, false)
+
+	packet := channeltypes.Packet{
+		SourceChannel: "channelID",
+	}
+	err := providerKeeper.OnTimeoutPacket(ctx, packet)
+
+	testkeeper.TestProviderStateIsCleanedAfterConsumerChainIsStopped(t, ctx, providerKeeper, "chainID", "channelID")
+	require.NoError(t, err)
+}
+
+// TestOnAcknowledgementPacketWithNoAckError tests `OnAcknowledgementPacket` when the underlying ack contains no error
+func TestOnAcknowledgementPacketWithNoAckError(t *testing.T) {
+	// Keeper setup
+	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+
+	ack := channeltypes.Acknowledgement{Response: &channeltypes.Acknowledgement_Result{Result: []byte{}}}
+	err := providerKeeper.OnAcknowledgementPacket(ctx, channeltypes.Packet{}, ack)
+	require.NoError(t, err)
+}
+
+// TestOnAcknowledgementPacketWithAckError tests `OnAcknowledgementPacket` when the underlying ack contains an error
+func TestOnAcknowledgementPacketWithAckError(t *testing.T) {
+	// Keeper setup
+	providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+	providerKeeper.SetParams(ctx, providertypes.DefaultParams())
+
+	// test that `OnAcknowledgementPacket` returns an error if the ack contains an error and the channel is unknown
+	ackError := channeltypes.Acknowledgement{Response: &channeltypes.Acknowledgement_Error{Error: "some error"}}
+	err := providerKeeper.OnAcknowledgementPacket(ctx, channeltypes.Packet{}, ackError)
+	require.Error(t, err)
+	require.True(t, strings.Contains(err.Error(), providertypes.ErrUnknownConsumerChannelId.Error()))
+
+	// test that we stop the consumer chain when `OnAcknowledgementPacket` returns an error and the chain is found
+	testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks, false)
+	packet := channeltypes.Packet{
+		SourceChannel: "channelID",
+	}
+
+	err = providerKeeper.OnAcknowledgementPacket(ctx, packet, ackError)
+
+	testkeeper.TestProviderStateIsCleanedAfterConsumerChainIsStopped(t, ctx, providerKeeper, "chainID", "channelID")
+	require.NoError(t, err)
+}