From 97daa925a9443f890a1243d25468a0138332ac76 Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Tue, 10 Sep 2024 14:04:31 +0200 Subject: [PATCH] fix: address comments from permissionless review (#2237) * AttributeConsumerSpawnTime only on InitializeConsumer * fix error message for ValidateByteSlice * remove unnecessary errors * fix comments * add comment for consumer genesis creation * adding comment on why not removing all consumer state * improve comments --- x/ccv/provider/keeper/consumer_lifecycle.go | 13 +++++++++---- x/ccv/provider/keeper/msg_server.go | 19 ++++++++----------- x/ccv/provider/keeper/params.go | 2 +- x/ccv/provider/types/errors.go | 5 ----- x/ccv/provider/types/msg.go | 2 +- 5 files changed, 19 insertions(+), 22 deletions(-) diff --git a/x/ccv/provider/keeper/consumer_lifecycle.go b/x/ccv/provider/keeper/consumer_lifecycle.go index bf09f7210f..609cca108e 100644 --- a/x/ccv/provider/keeper/consumer_lifecycle.go +++ b/x/ccv/provider/keeper/consumer_lifecycle.go @@ -60,7 +60,7 @@ func (k Keeper) InitializeConsumer(ctx sdk.Context, consumerId string) (time.Tim return initializationParameters.SpawnTime, true } -// BeginBlockLaunchConsumers launches initialized consumers that are ready to launch +// BeginBlockLaunchConsumers launches initialized consumers chains for which the spawn time has passed func (k Keeper) BeginBlockLaunchConsumers(ctx sdk.Context) error { consumerIds, err := k.ConsumeIdsFromTimeQueue( ctx, @@ -204,10 +204,10 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, consumerId string) error { // Set minimum height for equivocation evidence from this consumer chain k.SetEquivocationEvidenceMinHeight(ctx, consumerId, initializationRecord.InitialHeight.RevisionHeight) - // Consumers start out with the unbonding period from the consumer addition prop + // Consumers start out with the unbonding period from the initialization parameters consumerUnbondingPeriod := initializationRecord.UnbondingPeriod - // Create client state by getting template client from parameters and filling in zeroed fields from proposal. + // Create client state by getting template client from initialization parameters clientState := k.GetTemplateClient(ctx) clientState.ChainId = chainId clientState.LatestHeight = initializationRecord.InitialHeight @@ -351,6 +351,8 @@ func (k Keeper) MakeConsumerGenesis( } hash := tmtypes.NewValidatorSet(updatesAsValSet).Hash() + // note that providerFeePoolAddrStr is sent to the consumer during the IBC Channel handshake; + // see HandshakeMetadata in OnChanOpenTry on the provider-side, and OnChanOpenAck on the consumer-side consumerGenesisParams := ccv.NewParams( true, initializationRecord.BlocksPerDistributionTransmission, @@ -399,7 +401,7 @@ func (k Keeper) StopAndPrepareForConsumerRemoval(ctx sdk.Context, consumerId str return nil } -// BeginBlockRemoveConsumers iterates over the pending consumer proposals and stop/removes the chain if the removal time has passed +// BeginBlockRemoveConsumers removes stopped consumer chain for which the removal time has passed func (k Keeper) BeginBlockRemoveConsumers(ctx sdk.Context) error { consumerIds, err := k.ConsumeIdsFromTimeQueue( ctx, @@ -483,8 +485,11 @@ func (k Keeper) DeleteConsumerChain(ctx sdk.Context, consumerId string) (err err k.DeleteConsumerRemovalTime(ctx, consumerId) // TODO (PERMISSIONLESS) add newly-added state to be deleted + // Note that we do not delete ConsumerIdToChainIdKey and ConsumerIdToPhase, as well // as consumer metadata, initialization and power-shaping parameters. + // This is to enable block explorers and front ends to show information of + // consumer chains that were removed without needing an archive node. k.SetConsumerPhase(ctx, consumerId, types.CONSUMER_PHASE_DELETED) k.Logger(ctx).Info("consumer chain deleted from provider", "consumerId", consumerId) diff --git a/x/ccv/provider/keeper/msg_server.go b/x/ccv/provider/keeper/msg_server.go index 89a4de03a6..45fad0f7d7 100644 --- a/x/ccv/provider/keeper/msg_server.go +++ b/x/ccv/provider/keeper/msg_server.go @@ -386,11 +386,6 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon return &resp, errorsmod.Wrapf(types.ErrInvalidConsumerInitializationParameters, "cannot set consumer initialization parameters: %s", err.Error()) } - if !initializationParameters.SpawnTime.IsZero() { - // add SpawnTime event attribute - eventAttributes = append(eventAttributes, - sdk.NewAttribute(types.AttributeConsumerSpawnTime, initializationParameters.SpawnTime.String())) - } // power-shaping parameters are optional and hence could be nil; // in that case, set the default @@ -413,6 +408,10 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon return &resp, errorsmod.Wrapf(ccvtypes.ErrInvalidConsumerState, "cannot prepare chain with consumer id (%s) for launch", consumerId) } + + // add SpawnTime event attribute + eventAttributes = append(eventAttributes, + sdk.NewAttribute(types.AttributeConsumerSpawnTime, initializationParameters.SpawnTime.String())) } // add Phase event attribute @@ -508,12 +507,6 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon return &resp, errorsmod.Wrapf(types.ErrInvalidConsumerInitializationParameters, "cannot set consumer initialization parameters: %s", err.Error()) } - - if !msg.InitializationParameters.SpawnTime.IsZero() { - // add SpawnTime event attribute - eventAttributes = append(eventAttributes, - sdk.NewAttribute(types.AttributeConsumerSpawnTime, msg.InitializationParameters.SpawnTime.String())) - } } if msg.PowerShapingParameters != nil { @@ -569,6 +562,10 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon return &resp, errorsmod.Wrapf(ccvtypes.ErrInvalidConsumerState, "cannot prepare chain with consumer id (%s) for launch", consumerId) } + + // add SpawnTime event attribute + eventAttributes = append(eventAttributes, + sdk.NewAttribute(types.AttributeConsumerSpawnTime, msg.InitializationParameters.SpawnTime.String())) } // add Owner event attribute diff --git a/x/ccv/provider/keeper/params.go b/x/ccv/provider/keeper/params.go index 3bdb419a0a..aa3a306301 100644 --- a/x/ccv/provider/keeper/params.go +++ b/x/ccv/provider/keeper/params.go @@ -11,7 +11,7 @@ import ( "github.com/cosmos/interchain-security/v6/x/ccv/provider/types" ) -// GetTemplateClient returns the template client for provider proposals +// GetTemplateClient returns the template consumer client func (k Keeper) GetTemplateClient(ctx sdk.Context) *ibctmtypes.ClientState { params := k.GetParams(ctx) return params.TemplateClient diff --git a/x/ccv/provider/types/errors.go b/x/ccv/provider/types/errors.go index 2d87ee1345..dd2a6ea4c6 100644 --- a/x/ccv/provider/types/errors.go +++ b/x/ccv/provider/types/errors.go @@ -6,8 +6,6 @@ import ( // Provider sentinel errors var ( - ErrInvalidConsumerAdditionProposal = errorsmod.Register(ModuleName, 1, "invalid consumer addition proposal") - ErrInvalidConsumerRemovalProp = errorsmod.Register(ModuleName, 2, "invalid consumer removal proposal") ErrUnknownConsumerId = errorsmod.Register(ModuleName, 3, "no consumer chain with this consumer id") ErrUnknownConsumerChannelId = errorsmod.Register(ModuleName, 4, "no consumer chain with this channel id") ErrInvalidConsumerId = errorsmod.Register(ModuleName, 6, "invalid consumer id") @@ -15,12 +13,9 @@ var ( ErrCannotAssignDefaultKeyAssignment = errorsmod.Register(ModuleName, 11, "cannot re-assign default key assignment") ErrInvalidConsumerRewardDenom = errorsmod.Register(ModuleName, 14, "invalid consumer reward denom") ErrInvalidConsumerClient = errorsmod.Register(ModuleName, 16, "ccv channel is not built on correct client") - ErrConsumerChainNotFound = errorsmod.Register(ModuleName, 18, "consumer chain not found") ErrCannotOptOutFromTopN = errorsmod.Register(ModuleName, 20, "cannot opt out from a Top N chain") - ErrInvalidConsumerModificationProposal = errorsmod.Register(ModuleName, 22, "invalid consumer modification proposal") ErrNoUnbondingTime = errorsmod.Register(ModuleName, 23, "provider unbonding time not found") ErrUnauthorized = errorsmod.Register(ModuleName, 25, "unauthorized") - ErrBlankConsumerChainID = errorsmod.Register(ModuleName, 26, "consumer chain id must not be blank") ErrInvalidPhase = errorsmod.Register(ModuleName, 27, "cannot perform action in the current phase of consumer chain") ErrInvalidConsumerMetadata = errorsmod.Register(ModuleName, 28, "invalid consumer metadata") ErrInvalidPowerShapingParameters = errorsmod.Register(ModuleName, 29, "invalid power shaping parameters") diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index 0d0ea89c75..63a868ad8a 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -577,7 +577,7 @@ func ValidateInitializationParameters(initializationParameters ConsumerInitializ func ValidateByteSlice(hash []byte, maxLength int) error { if len(hash) > maxLength { - return fmt.Errorf("hash is too long; got: %d, max: %d", len(hash), MaxHashLength) + return fmt.Errorf("hash is too long; got: %d, max: %d", len(hash), maxLength) } return nil }