added tests on the hooks

x/ccv/provider/keeper/distribution.go

@@ -74,33 +74,33 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 		return
 	}
 
-	// Iterate over all registered consumer chains
-	for _, consumerChainID := range k.GetAllRegisteredConsumerIds(ctx) {
+	// Iterate over all launched consumer chains
+	for _, consumerId := range k.GetAllRegisteredConsumerIds(ctx) {
 
 		// note that it's possible that no rewards are collected even though the
 		// reward pool isn't empty. This can happen if the reward pool holds some tokens
 		// of non-whitelisted denominations.
-		alloc := k.GetConsumerRewardsAllocation(ctx, consumerChainID)
+		alloc := k.GetConsumerRewardsAllocation(ctx, consumerId)
 		if alloc.Rewards.IsZero() {
 			continue
 		}
 
 		// temporary workaround to keep CanWithdrawInvariant happy
 		// general discussions here: https://github.com/cosmos/cosmos-sdk/issues/2906#issuecomment-441867634
-		if k.ComputeConsumerTotalVotingPower(ctx, consumerChainID) == 0 {
+		if k.ComputeConsumerTotalVotingPower(ctx, consumerId) == 0 {
 			rewardsToSend, rewardsChange := alloc.Rewards.TruncateDecimal()
 			err := k.distributionKeeper.FundCommunityPool(context.Context(ctx), rewardsToSend, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress())
 			if err != nil {
 				k.Logger(ctx).Error(
 					"fail to allocate rewards from consumer chain %s to community pool: %s",
-					consumerChainID,
+					consumerId,
 					err,
 				)
 			}
 
 			// set the consumer allocation to the remaining reward decimals
 			alloc.Rewards = rewardsChange
-			k.SetConsumerRewardsAllocation(ctx, consumerChainID, alloc)
+			k.SetConsumerRewardsAllocation(ctx, consumerId, alloc)
 
 			return
 		}
@@ -112,7 +112,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 		if err != nil {
 			k.Logger(ctx).Error(
 				"cannot get community tax while allocating rewards from consumer chain %s: %s",
-				consumerChainID,
+				consumerId,
 				err,
 			)
 			continue
@@ -132,7 +132,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 		if err != nil {
 			k.Logger(ctx).Error(
 				"cannot send rewards to distribution module account %s: %s",
-				consumerChainID,
+				consumerId,
 				err,
 			)
 			continue
@@ -141,7 +141,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 		// allocate tokens to consumer validators
 		k.AllocateTokensToConsumerValidators(
 			ctx,
-			consumerChainID,
+			consumerId,
 			sdk.NewDecCoinsFromCoins(validatorsRewardsTrunc...),
 		)
 
@@ -151,15 +151,15 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 		if err != nil {
 			k.Logger(ctx).Error(
 				"fail to allocate rewards from consumer chain %s to community pool: %s",
-				consumerChainID,
+				consumerId,
 				err,
 			)
 			continue
 		}
 
 		// set consumer allocations to the remaining rewards decimals
 		alloc.Rewards = validatorsRewardsChange.Add(remainingChanges...)
-		k.SetConsumerRewardsAllocation(ctx, consumerChainID, alloc)
+		k.SetConsumerRewardsAllocation(ctx, consumerId, alloc)
 	}
 }
 

x/ccv/provider/keeper/hooks.go

@@ -115,44 +115,19 @@ func (h Hooks) AfterProposalSubmission(goCtx context.Context, proposalId uint64)
 		return fmt.Errorf("cannot retrieve proposal with id: %d", proposalId)
 	}
 
-	hasUpdateConsumerMsg := false
-	for _, msg := range p.GetMessages() {
-		sdkMsg, isMsgUpdateConsumer := msg.GetCachedValue().(*providertypes.MsgUpdateConsumer)
-		if isMsgUpdateConsumer {
-			// A `MsgUpdateConsumer` can only succeed if the owner of the consumer chain is the gov module.
-			// If that's not the case, we immediately fail the proposal.
-			// Note that someone could potentially change the owner of a chain to be that of the gov module
-			// while a proposal is active and before the proposal is executed. Even then, we still do not allow
-			// `MsgUpdateConsumer` proposals if the owner of the chain is not the gov module to avoid someone forgetting
-			// to change the owner address while the proposal is active.
-			ownerAddress, err := h.k.GetConsumerOwnerAddress(ctx, sdkMsg.ConsumerId)
-			if err != nil {
-				return fmt.Errorf("cannot find owner address for consumer with consumer id (%s): %s", sdkMsg.ConsumerId, err.Error())
-			} else if ownerAddress != h.k.GetAuthority() {
-				return fmt.Errorf("owner address (%s) is not the gov module (%s)", ownerAddress, h.k.GetAuthority())
-			}
-
-			if hasUpdateConsumerMsg {
-				return fmt.Errorf("proposal can contain at most one `MsgUpdateConsumer` message")
-			}
-			hasUpdateConsumerMsg = true
-			h.k.SetProposalIdToConsumerId(ctx, proposalId, sdkMsg.ConsumerId)
-		}
+	err = DoesNotHaveDeprecatedMessage(&p)
+	if err != nil {
+		return err
+	}
 
-		// if the proposal contains a deprecated message, cancel the proposal
-		_, isMsgConsumerAddition := msg.GetCachedValue().(*providertypes.MsgConsumerAddition)
-		if isMsgConsumerAddition {
-			return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerAddition`; use `MsgCreateConsumer` instead")
-		}
+	msgUpdateConsumer, err := h.k.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &p)
+	if err != nil {
+		return err
+	}
 
-		_, isMsgConsumerModification := msg.GetCachedValue().(*providertypes.MsgConsumerModification)
-		if isMsgConsumerModification {
-			return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerModification`; use `MsgUpdateConsumer` instead")
-		}
-		_, isMsgConsumerRemoval := msg.GetCachedValue().(*providertypes.MsgConsumerRemoval)
-		if isMsgConsumerRemoval {
-			return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerRemoval`; use `MsgRemoveConsumer` instead")
-		}
+	if msgUpdateConsumer != nil {
+		// a correctly set `MsgUpdateConsumer` was found
+		h.k.SetProposalIdToConsumerId(ctx, proposalId, msgUpdateConsumer.ConsumerId)
 	}
 
 	return nil

x/ccv/provider/keeper/permissionless.go

@@ -6,6 +6,7 @@ import (
 	"encoding/binary"
 	"fmt"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
 	"github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
 	"strconv"
 	"time"
@@ -692,3 +693,54 @@ func (k Keeper) CanLaunch(ctx sdk.Context, consumerId string) (time.Time, bool)
 	return initializationParameters.SpawnTime, spawnTimeInTheFuture && genesisHashSet && binaryHashSet && consumerRedistributionFractionSet &&
 		blocksPerDistributionTransmissionSet && historicalEntriesSet
 }
+
+// HasAtMostOnceCorrectMsgUpdateConsumer checks that the proposal has at most one `MsgUpdateConsumer` that is
+// correctly set (i.e., the owner address of the to-be-updated consumer corresponds to the gov module). Returns
+// the single `MsgUpdateConsumer` message if only one correctly set exists.
+func (k Keeper) HasAtMostOnceCorrectMsgUpdateConsumer(ctx sdk.Context, proposal *govv1.Proposal) (*types.MsgUpdateConsumer, error) {
+	var msgUpdateConsumer *types.MsgUpdateConsumer
+	for _, msg := range proposal.GetMessages() {
+		sdkMsg, isMsgUpdateConsumer := msg.GetCachedValue().(*types.MsgUpdateConsumer)
+		if isMsgUpdateConsumer {
+			// A `MsgUpdateConsumer` can only succeed if the owner of the consumer chain is the gov module.
+			// If that's not the case, we immediately fail the proposal.
+			// Note that someone could potentially change the owner of a chain to be that of the gov module
+			// while a proposal is active and before the proposal is executed. Even then, we still do not allow
+			// `MsgUpdateConsumer` proposals if the owner of the chain is not the gov module to avoid someone forgetting
+			// to change the owner address while the proposal is active.
+			ownerAddress, err := k.GetConsumerOwnerAddress(ctx, sdkMsg.ConsumerId)
+			if err != nil {
+				return nil, fmt.Errorf("cannot find owner address for consumer with consumer id (%s): %s", sdkMsg.ConsumerId, err.Error())
+			} else if ownerAddress != k.GetAuthority() {
+				return nil, fmt.Errorf("owner address (%s) is not the gov module (%s)", ownerAddress, k.GetAuthority())
+			}
+
+			if msgUpdateConsumer != nil {
+				return nil, fmt.Errorf("proposal can contain at most one `MsgUpdateConsumer` message")
+			}
+			msgUpdateConsumer = sdkMsg
+		}
+	}
+	return msgUpdateConsumer, nil
+}
+
+// DoesNotHaveDeprecatedMessage checks that the provided proposal does not contain any deprecated messages and returns
+// an error if this is the case
+func DoesNotHaveDeprecatedMessage(proposal *govv1.Proposal) error {
+	for _, msg := range proposal.GetMessages() {
+		// if the proposal contains a deprecated message, cancel the proposal
+		_, isMsgConsumerAddition := msg.GetCachedValue().(*types.MsgConsumerAddition)
+		if isMsgConsumerAddition {
+			return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerAddition`; use `MsgCreateConsumer` instead")
+		}
+		_, isMsgConsumerModification := msg.GetCachedValue().(*types.MsgConsumerModification)
+		if isMsgConsumerModification {
+			return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerModification`; use `MsgUpdateConsumer` instead")
+		}
+		_, isMsgConsumerRemoval := msg.GetCachedValue().(*types.MsgConsumerRemoval)
+		if isMsgConsumerRemoval {
+			return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerRemoval`; use `MsgRemoveConsumer` instead")
+		}
+	}
+	return nil
+}

x/ccv/provider/keeper/permissionless_test.go

@@ -2,6 +2,7 @@ package keeper_test
 
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 	"github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	testkeeper "github.com/cosmos/interchain-security/v5/testutil/keeper"
@@ -720,3 +721,60 @@ func TestCanLaunch(t *testing.T) {
 	require.NoError(t, err)
 	require.False(t, canLaunch)
 }
+
+func TestHasAtMostOnceCorrectMsgUpdateConsumer(t *testing.T) {
+	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+
+	expectedMsgUpdateConsumer := providertypes.MsgUpdateConsumer{Signer: "signer", ConsumerId: "consumerId", NewOwnerAddress: "new owner address"}
+
+	proposal, err := govv1.NewProposal([]sdk.Msg{&expectedMsgUpdateConsumer}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false)
+	require.NoError(t, err)
+
+	_, err = providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &proposal)
+	require.ErrorContains(t, err, "cannot find owner address")
+
+	// set owner address that is not the gov module
+	providerKeeper.SetConsumerOwnerAddress(ctx, "consumerId", "owner address")
+	_, err = providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &proposal)
+	require.ErrorContains(t, err, "is not the gov module")
+
+	// set owner address that is the gov module
+	providerKeeper.SetConsumerOwnerAddress(ctx, "consumerId", "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn")
+	actualMsgUpdateConsumer, err := providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &proposal)
+	require.NoError(t, err)
+	require.Equal(t, expectedMsgUpdateConsumer, *actualMsgUpdateConsumer)
+
+	// a proposal with 2 `MsgUpdateConsumer` messages
+	invalidProposal, err := govv1.NewProposal([]sdk.Msg{&expectedMsgUpdateConsumer, &expectedMsgUpdateConsumer}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false)
+	actualMsgUpdateConsumer, err = providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &invalidProposal)
+	require.ErrorContains(t, err, "proposal can contain at most one")
+	require.Nil(t, actualMsgUpdateConsumer)
+}
+
+func TestDoesNotHaveDeprecatedMessage(t *testing.T) {
+	msgConsumerAddition := providertypes.MsgConsumerAddition{}
+	proposal, err := govv1.NewProposal([]sdk.Msg{&msgConsumerAddition}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false)
+	require.NoError(t, err)
+	err = keeper.DoesNotHaveDeprecatedMessage(&proposal)
+	require.ErrorContains(t, err, "cannot contain deprecated `MsgConsumerAddition`")
+
+	msgConsumerModification := providertypes.MsgConsumerModification{}
+	proposal, err = govv1.NewProposal([]sdk.Msg{&msgConsumerModification}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false)
+	require.NoError(t, err)
+	err = keeper.DoesNotHaveDeprecatedMessage(&proposal)
+	require.ErrorContains(t, err, "cannot contain deprecated `MsgConsumerModification`")
+
+	msgConsumerRemoval := providertypes.MsgConsumerRemoval{}
+	proposal, err = govv1.NewProposal([]sdk.Msg{&msgConsumerRemoval}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false)
+	require.NoError(t, err)
+	err = keeper.DoesNotHaveDeprecatedMessage(&proposal)
+	require.ErrorContains(t, err, "cannot contain deprecated `MsgConsumerRemoval`")
+
+	// a proposal with no deprecated messages
+	msgUpdateConsumer := providertypes.MsgUpdateConsumer{Signer: "signer", ConsumerId: "consumerId", NewOwnerAddress: "new owner address"}
+	proposal, err = govv1.NewProposal([]sdk.Msg{&msgUpdateConsumer}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false)
+	require.NoError(t, err)
+	err = keeper.DoesNotHaveDeprecatedMessage(&proposal)
+	require.Nil(t, err)
+}

x/ccv/provider/keeper/proposal.go

@@ -488,13 +488,13 @@ func (k Keeper) BeginBlockCCR(ctx sdk.Context) {
 				"err", err.Error())
 			continue
 		}
-		// The cached context is created with a new EventManager so we merge the event
-		// into the original context
-		// TODO (PERMISSIONLESS): verify this here and in the initialized chains to launch
-		ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events())
 
 		k.SetConsumerPhase(cachedCtx, consumerId, Stopped)
 		k.RemoveConsumerToBeStoppedFromStopTime(ctx, consumerId, stopTime)
+
+		// The cached context is created with a new EventManager so we merge the event into the original context
+		ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events())
+
 		writeFn()
 
 		k.Logger(ctx).Info("executed consumer removal",