modified the way we verify the new owner address, as well as nit refa…

…ctoring on the ConsumerIds
cosmos · Aug 26, 2024 · 35b4140 · 35b4140
1 parent e182d49
commit 35b4140
Show file tree

Hide file tree

Showing 10 changed files with 346 additions and 329 deletions.
diff --git a/proto/interchain_security/ccv/provider/v1/provider.proto b/proto/interchain_security/ccv/provider/v1/provider.proto
@@ -463,4 +463,4 @@ message PowerShapingParameters {
 
 // ConsumerIds contains consumer ids of chains
 // Used so we can easily (de)serialize slices of strings
-message ConsumerIds { repeated string consumer_ids = 1; }
+message ConsumerIds { repeated string ids = 1; }
diff --git a/proto/interchain_security/ccv/provider/v1/tx.proto b/proto/interchain_security/ccv/provider/v1/tx.proto
@@ -393,7 +393,7 @@ message MsgUpdateConsumer {
   string consumer_id = 2;
 
   // the new owner of the consumer when updated
-  string new_owner_address = 3;
+  string new_owner_address = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
 
   // the metadata of the consumer when updated
   ConsumerMetadata metadata = 4;

diff --git a/testutil/keeper/mocks.go b/testutil/keeper/mocks.go
diff --git a/x/ccv/provider/keeper/msg_server.go b/x/ccv/provider/keeper/msg_server.go
@@ -359,6 +359,14 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon
 			"cannot update consumer chain that is in the stopped phase: %s", consumerId)
 	}
 
+	// The new owner address can be empty, in which case the consumer chain does not change its owner.
+	// However, if the new owner address is not empty, we verify that it's a valid account address.
+	if strings.TrimSpace(msg.NewOwnerAddress) != "" {
+		if _, err := k.accountKeeper.AddressCodec().StringToBytes(msg.NewOwnerAddress); err != nil {
+			return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidNewOwnerAddress, "invalid new owner address %s", msg.NewOwnerAddress)
+		}
+	}
+
 	ownerAddress, err := k.Keeper.GetConsumerOwnerAddress(ctx, consumerId)
 	if err != nil {
 		return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrNoOwnerAddress, "cannot retrieve owner address %s", ownerAddress)

diff --git a/x/ccv/provider/keeper/permissionless.go b/x/ccv/provider/keeper/permissionless.go
@@ -276,10 +276,10 @@ func (k Keeper) AppendSpawnTimeForConsumerToBeLaunched(ctx sdk.Context, consumer
 	if err != nil {
 		return err
 	}
-	consumerIds := append(consumerIdsSlice.ConsumerIds, consumerId)
+	consumerIds := append(consumerIdsSlice.Ids, consumerId)
 
 	appendedConsumerIdsStr := types.ConsumerIds{
-		ConsumerIds: consumerIds,
+		Ids: consumerIds,
 	}
 
 	bz, err := appendedConsumerIdsStr.Marshal()
@@ -300,31 +300,31 @@ func (k Keeper) RemoveConsumerFromToBeLaunchedConsumers(ctx sdk.Context, consume
 		return err
 	}
 
-	if len(consumerIds.ConsumerIds) == 0 {
+	if len(consumerIds.Ids) == 0 {
 		return fmt.Errorf("no consumer ids for spawn time: %s", spawnTime.String())
 	}
 
 	// find the index of the consumer we want to remove
 	index := 0
-	for i := 0; i < len(consumerIds.ConsumerIds); i = i + 1 {
-		if consumerIds.ConsumerIds[i] == consumerId {
+	for i := 0; i < len(consumerIds.Ids); i = i + 1 {
+		if consumerIds.Ids[i] == consumerId {
 			index = i
 			break
 		}
 	}
-	if consumerIds.ConsumerIds[index] != consumerId {
+	if consumerIds.Ids[index] != consumerId {
 		return fmt.Errorf("failed to find consumer id (%s) in the chains to be launched", consumerId)
 	}
 
-	if len(consumerIds.ConsumerIds) == 1 {
+	if len(consumerIds.Ids) == 1 {
 		store.Delete(types.SpawnTimeToConsumerIdsKey(spawnTime))
 		return nil
 	}
 
-	updatedConsumerIds := append(consumerIds.ConsumerIds[:index], consumerIds.ConsumerIds[index+1:]...)
+	updatedConsumerIds := append(consumerIds.Ids[:index], consumerIds.Ids[index+1:]...)
 
 	updatedConsumerIdsStr := types.ConsumerIds{
-		ConsumerIds: updatedConsumerIds,
+		Ids: updatedConsumerIds,
 	}
 
 	bz, err := updatedConsumerIdsStr.Marshal()
@@ -362,10 +362,10 @@ func (k Keeper) AppendStopTimeForConsumerToBeStopped(ctx sdk.Context, consumerId
 	if err != nil {
 		return err
 	}
-	consumerIds := append(consumerIdsStr.ConsumerIds, consumerId)
+	consumerIds := append(consumerIdsStr.Ids, consumerId)
 
 	consumerIdsNewStr := types.ConsumerIds{
-		ConsumerIds: consumerIds,
+		Ids: consumerIds,
 	}
 
 	bz, err := consumerIdsNewStr.Marshal()
@@ -386,30 +386,30 @@ func (k Keeper) RemoveConsumerFromToBeStoppedConsumers(ctx sdk.Context, consumer
 		return err
 	}
 
-	if len(consumerIds.ConsumerIds) == 0 {
+	if len(consumerIds.Ids) == 0 {
 		return fmt.Errorf("no consumer ids for stop time: %s", stopTime.String())
 	}
 
 	// find the index of the consumer we want to remove
 	index := 0
-	for i := 0; i < len(consumerIds.ConsumerIds); i = i + 1 {
-		if consumerIds.ConsumerIds[i] == consumerId {
+	for i := 0; i < len(consumerIds.Ids); i = i + 1 {
+		if consumerIds.Ids[i] == consumerId {
 			index = i
 			break
 		}
 	}
-	if consumerIds.ConsumerIds[index] != consumerId {
+	if consumerIds.Ids[index] != consumerId {
 		return fmt.Errorf("failed to find consumer id (%s) in the chains to be stopped", consumerId)
 	}
 
-	if len(consumerIds.ConsumerIds) == 1 {
+	if len(consumerIds.Ids) == 1 {
 		store.Delete(types.StopTimeToConsumerIdsKey(stopTime))
 		return nil
 	}
 
-	updatedConsumerIds := append(consumerIds.ConsumerIds[:index], consumerIds.ConsumerIds[index+1:]...)
+	updatedConsumerIds := append(consumerIds.Ids[:index], consumerIds.Ids[index+1:]...)
 	updatedConsumerIdsStr := types.ConsumerIds{
-		ConsumerIds: updatedConsumerIds,
+		Ids: updatedConsumerIds,
 	}
 	bz, err := updatedConsumerIdsStr.Marshal()
 	if err != nil {
@@ -549,14 +549,14 @@ func (k Keeper) GetInitializedConsumersReadyToLaunch(ctx sdk.Context, limit uint
 				"error", err)
 			continue
 		}
-		if len(result)+len(consumerIds.ConsumerIds) >= int(limit) {
-			remainingConsumerIds := len(result) + len(consumerIds.ConsumerIds) - int(limit)
-			if len(consumerIds.ConsumerIds[:len(consumerIds.ConsumerIds)-remainingConsumerIds]) == 0 {
+		if len(result)+len(consumerIds.Ids) >= int(limit) {
+			remainingConsumerIds := len(result) + len(consumerIds.Ids) - int(limit)
+			if len(consumerIds.Ids[:len(consumerIds.Ids)-remainingConsumerIds]) == 0 {
 				return result
 			}
-			return append(result, consumerIds.ConsumerIds[:len(consumerIds.ConsumerIds)-remainingConsumerIds]...)
+			return append(result, consumerIds.Ids[:len(consumerIds.Ids)-remainingConsumerIds]...)
 		} else {
-			result = append(result, consumerIds.ConsumerIds...)
+			result = append(result, consumerIds.Ids...)
 		}
 	}
 
@@ -665,14 +665,14 @@ func (k Keeper) GetLaunchedConsumersReadyToStop(ctx sdk.Context, limit uint32) [
 				"error", err)
 			continue
 		}
-		if len(result)+len(consumerIds.ConsumerIds) >= int(limit) {
-			remainingConsumerIds := len(result) + len(consumerIds.ConsumerIds) - int(limit)
-			if len(consumerIds.ConsumerIds[:len(consumerIds.ConsumerIds)-remainingConsumerIds]) == 0 {
+		if len(result)+len(consumerIds.Ids) >= int(limit) {
+			remainingConsumerIds := len(result) + len(consumerIds.Ids) - int(limit)
+			if len(consumerIds.Ids[:len(consumerIds.Ids)-remainingConsumerIds]) == 0 {
 				return result
 			}
-			return append(result, consumerIds.ConsumerIds[:len(consumerIds.ConsumerIds)-remainingConsumerIds]...)
+			return append(result, consumerIds.Ids[:len(consumerIds.Ids)-remainingConsumerIds]...)
 		} else {
-			result = append(result, consumerIds.ConsumerIds...)
+			result = append(result, consumerIds.Ids...)
 		}
 	}
 
@@ -727,19 +727,21 @@ func (k Keeper) CanLaunch(ctx sdk.Context, consumerId string) (time.Time, bool)
 		return time.Time{}, false
 	}
 
-	if initializationParameters, err := k.GetConsumerInitializationParameters(ctx, consumerId); err == nil {
-		// a chain can only launch if the spawn time is in the future
-		spawnTimeInTheFuture := initializationParameters.SpawnTime.After(ctx.BlockTime())
+	initializationParameters, err := k.GetConsumerInitializationParameters(ctx, consumerId)
+	if err != nil {
+		return time.Time{}, false
+	}
 
-		genesisHashSet := initializationParameters.GenesisHash != nil
-		binaryHashSet := initializationParameters.BinaryHash != nil
+	// a chain can only launch if the spawn time is in the future
+	spawnTimeInTheFuture := initializationParameters.SpawnTime.After(ctx.BlockTime())
 
-		consumerRedistributionFractionSet := initializationParameters.ConsumerRedistributionFraction != ""
-		blocksPerDistributionTransmissionSet := initializationParameters.BlocksPerDistributionTransmission > 0
-		historicalEntriesSet := initializationParameters.HistoricalEntries > 0
+	genesisHashSet := initializationParameters.GenesisHash != nil
+	binaryHashSet := initializationParameters.BinaryHash != nil
 
-		return initializationParameters.SpawnTime, spawnTimeInTheFuture && genesisHashSet && binaryHashSet && consumerRedistributionFractionSet &&
-			blocksPerDistributionTransmissionSet && historicalEntriesSet
-	}
-	return time.Time{}, false
+	consumerRedistributionFractionSet := initializationParameters.ConsumerRedistributionFraction != ""
+	blocksPerDistributionTransmissionSet := initializationParameters.BlocksPerDistributionTransmission > 0
+	historicalEntriesSet := initializationParameters.HistoricalEntries > 0
+
+	return initializationParameters.SpawnTime, spawnTimeInTheFuture && genesisHashSet && binaryHashSet && consumerRedistributionFractionSet &&
+		blocksPerDistributionTransmissionSet && historicalEntriesSet
 }
diff --git a/x/ccv/provider/types/errors.go b/x/ccv/provider/types/errors.go
@@ -43,7 +43,8 @@ var (
 	ErrNoConsumerId                            = errorsmod.Register(ModuleName, 35, "missing consumer id")
 	ErrAlreadyOptedIn                          = errorsmod.Register(ModuleName, 36, "already opted in to a chain with the same chain id")
 	ErrNoOwnerAddress                          = errorsmod.Register(ModuleName, 37, "missing owner address")
-	ErrInvalidTransformToTopN                  = errorsmod.Register(ModuleName, 38, "invalid transform to Top N chain")
-	ErrInvalidTransformToOptIn                 = errorsmod.Register(ModuleName, 39, "invalid transform to Opt In chain")
-	ErrCannotCreateTopNChain                   = errorsmod.Register(ModuleName, 40, "cannot create Top N chain outside permissionlessly")
+	ErrInvalidNewOwnerAddress                  = errorsmod.Register(ModuleName, 38, "invalid new owner address")
+	ErrInvalidTransformToTopN                  = errorsmod.Register(ModuleName, 39, "invalid transform to Top N chain")
+	ErrInvalidTransformToOptIn                 = errorsmod.Register(ModuleName, 40, "invalid transform to Opt In chain")
+	ErrCannotCreateTopNChain                   = errorsmod.Register(ModuleName, 41, "cannot create Top N chain outside permissionlessly")
 )
diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go
@@ -327,15 +327,6 @@ func (msg MsgUpdateConsumer) ValidateBasic() error {
 		return err
 	}
 
-	// The new owner address can be empty, in which case the consumer chain does not change its owner.
-	// However, if the new owner address is not empty, we verify that it's a valid account address.
-	if strings.TrimSpace(msg.NewOwnerAddress) != "" {
-		_, err := sdk.AccAddressFromBech32(msg.NewOwnerAddress)
-		if err != nil {
-			return err
-		}
-	}
-
 	if msg.Metadata != nil {
 		if err := ValidateConsumerMetadata(*msg.Metadata); err != nil {
 			return err