From 5791be609dd2d9e15494074a0bee020113cd47de Mon Sep 17 00:00:00 2001 From: mpoke Date: Fri, 30 Aug 2024 08:23:04 +0200 Subject: [PATCH 1/5] add testing for ValidateStringField --- x/ccv/provider/types/msg.go | 16 +++++++------ x/ccv/provider/types/msg_test.go | 40 +++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index 08a4b516c9..fe7380f0d3 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -257,7 +257,7 @@ func (msg MsgCreateConsumer) Route() string { return RouterKey } // ValidateBasic implements the sdk.Msg interface. func (msg MsgCreateConsumer) ValidateBasic() error { - if err := ValidateField("chain id", msg.ChainId, cmttypes.MaxChainIDLen); err != nil { + if err := ValidateStringField("chain id", msg.ChainId, cmttypes.MaxChainIDLen); err != nil { return err } @@ -736,8 +736,10 @@ func ValidateConsumerId(consumerId string) error { return nil } -// ValidateField validates that `field` is not empty and has at most `maxLength` characters -func ValidateField(nameOfTheField string, field string, maxLength int) error { +// ValidateStringField validates that a string `field` satisfies the following properties: +// - is not empty +// - has at most `maxLength` characters +func ValidateStringField(nameOfTheField string, field string, maxLength int) error { if strings.TrimSpace(field) == "" { return fmt.Errorf("%s cannot be empty", nameOfTheField) } else if len(field) > maxLength { @@ -752,15 +754,15 @@ func ValidateConsumerMetadata(metadata ConsumerMetadata) error { const maxDescriptionLength = 50000 const maxMetadataLength = 1000 - if err := ValidateField("name", metadata.Name, maxNameLength); err != nil { + if err := ValidateStringField("name", metadata.Name, maxNameLength); err != nil { return err } - if err := ValidateField("description", metadata.Description, maxDescriptionLength); err != nil { + if err := ValidateStringField("description", metadata.Description, maxDescriptionLength); err != nil { return err } - if err := ValidateField("metadata", metadata.Metadata, maxMetadataLength); err != nil { + if err := ValidateStringField("metadata", metadata.Metadata, maxMetadataLength); err != nil { return err } @@ -822,7 +824,7 @@ func ValidateInitializationParameters(initializationParameters ConsumerInitializ if err := ccvtypes.ValidateStringFraction(initializationParameters.ConsumerRedistributionFraction); err != nil { return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "consumer redistribution fraction is invalid: %s", err.Error()) - } else if err := ValidateField("consumer redistribution fraction", initializationParameters.ConsumerRedistributionFraction, maxConsumerRedistributionFractionLength); err != nil { + } else if err := ValidateStringField("consumer redistribution fraction", initializationParameters.ConsumerRedistributionFraction, maxConsumerRedistributionFractionLength); err != nil { return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "consumer redistribution fraction is invalid: %s", err.Error()) } diff --git a/x/ccv/provider/types/msg_test.go b/x/ccv/provider/types/msg_test.go index da6d24d793..380adced0d 100644 --- a/x/ccv/provider/types/msg_test.go +++ b/x/ccv/provider/types/msg_test.go @@ -1,9 +1,10 @@ package types_test import ( + "testing" + "github.com/cosmos/interchain-security/v5/x/ccv/provider/types" "github.com/stretchr/testify/require" - "testing" ) func TestValidateConsumerId(t *testing.T) { @@ -19,3 +20,40 @@ func TestValidateConsumerId(t *testing.T) { require.NoError(t, types.ValidateConsumerId("0")) require.NoError(t, types.ValidateConsumerId("18446744073709551615")) // 2^64 - 1 } + +func TestValidateStringField(t *testing.T) { + testCases := []struct { + name string + field string + maxLength int + valid bool + }{ + { + name: "invalid: empty", + field: "", + maxLength: 5, + valid: false, + }, + { + name: "invalid: too long", + field: "this field is too long", + maxLength: 5, + valid: false, + }, + { + name: "valid", + field: "valid", + maxLength: 5, + valid: true, + }, + } + + for _, tc := range testCases { + err := types.ValidateStringField("field", tc.field, tc.maxLength) + if tc.valid { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } + } +} From ae6e8308e18a66f94d0b566e8e58c864cb97deed Mon Sep 17 00:00:00 2001 From: mpoke Date: Fri, 30 Aug 2024 09:42:14 +0200 Subject: [PATCH 2/5] ValidateInitializationParameters --- x/ccv/provider/types/msg.go | 91 ++++++++----- x/ccv/provider/types/msg_test.go | 217 ++++++++++++++++++++++++++++++- 2 files changed, 272 insertions(+), 36 deletions(-) diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index fe7380f0d3..65f38fc9cb 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -33,6 +33,16 @@ const ( TypeMsgOptIn = "opt_in" TypeMsgOptOut = "opt_out" TypeMsgSetConsumerCommissionRate = "set_consumer_commission_rate" + + // MaxNameLength defines the maximum consumer name length + MaxNameLength = 50 + // MaxDescriptionLength + MaxDescriptionLength = 10000 + // MaxMetadataLength defines the maximum consumer metadata length + MaxMetadataLength = 255 + + // MaxHashLength defines the maximum length of a hash + MaxHashLength = 64 ) var ( @@ -257,7 +267,7 @@ func (msg MsgCreateConsumer) Route() string { return RouterKey } // ValidateBasic implements the sdk.Msg interface. func (msg MsgCreateConsumer) ValidateBasic() error { - if err := ValidateStringField("chain id", msg.ChainId, cmttypes.MaxChainIDLen); err != nil { + if err := ValidateStringField("ChainId", msg.ChainId, cmttypes.MaxChainIDLen); err != nil { return err } @@ -748,21 +758,35 @@ func ValidateStringField(nameOfTheField string, field string, maxLength int) err return nil } +// TruncateString truncates a string to maximum length characters +func TruncateString(str string, length int) string { + if length <= 0 { + return "" + } + + truncated := "" + count := 0 + for _, char := range str { + truncated += string(char) + count++ + if count >= length { + break + } + } + return truncated +} + // ValidateConsumerMetadata validates that all the provided metadata are in the expected range func ValidateConsumerMetadata(metadata ConsumerMetadata) error { - const maxNameLength = 100 - const maxDescriptionLength = 50000 - const maxMetadataLength = 1000 - - if err := ValidateStringField("name", metadata.Name, maxNameLength); err != nil { + if err := ValidateStringField("name", metadata.Name, MaxNameLength); err != nil { return err } - if err := ValidateStringField("description", metadata.Description, maxDescriptionLength); err != nil { + if err := ValidateStringField("description", metadata.Description, MaxDescriptionLength); err != nil { return err } - if err := ValidateStringField("metadata", metadata.Metadata, maxMetadataLength); err != nil { + if err := ValidateStringField("metadata", metadata.Metadata, MaxMetadataLength); err != nil { return err } @@ -797,63 +821,60 @@ func ValidatePowerShapingParameters(powerShapingParameters PowerShapingParameter // ValidateInitializationParameters validates that all the provided parameters are in the expected range func ValidateInitializationParameters(initializationParameters ConsumerInitializationParameters) error { - const maxGenesisHashLength = 1000 - const maxBinaryHashLength = 1000 - const maxConsumerRedistributionFractionLength = 50 - const maxDistributionTransmissionChannelLength = 1000 - if initializationParameters.InitialHeight.IsZero() { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "initial height cannot be zero") + return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "InitialHeight cannot be zero") } - if len(initializationParameters.GenesisHash) == 0 { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "genesis hash cannot be empty") - } else if len(initializationParameters.GenesisHash) > maxGenesisHashLength { - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "genesis hash cannot exceed %d bytes", maxGenesisHashLength) + if err := validateHash(initializationParameters.GenesisHash); err != nil { + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "GenesisHash: %s", err.Error()) } - if len(initializationParameters.BinaryHash) == 0 { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "binary hash cannot be empty") - } else if len(initializationParameters.BinaryHash) > maxBinaryHashLength { - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "binary hash cannot exceed %d bytes", maxBinaryHashLength) + if err := validateHash(initializationParameters.BinaryHash); err != nil { + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "BinaryHash: %s", err.Error()) } if initializationParameters.SpawnTime.IsZero() { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "spawn time cannot be zero") + return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "SpawnTime cannot be zero") } if err := ccvtypes.ValidateStringFraction(initializationParameters.ConsumerRedistributionFraction); err != nil { - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "consumer redistribution fraction is invalid: %s", err.Error()) - } else if err := ValidateStringField("consumer redistribution fraction", initializationParameters.ConsumerRedistributionFraction, maxConsumerRedistributionFractionLength); err != nil { - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "consumer redistribution fraction is invalid: %s", err.Error()) + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "ConsumerRedistributionFraction: %s", err.Error()) } if err := ccvtypes.ValidatePositiveInt64(initializationParameters.BlocksPerDistributionTransmission); err != nil { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "blocks per distribution transmission has to be positive") + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "BlocksPerDistributionTransmission: %s", err.Error()) } if err := ccvtypes.ValidateDistributionTransmissionChannel(initializationParameters.DistributionTransmissionChannel); err != nil { - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "distribution transmission channel is invalid: %s", err.Error()) - } else if len(initializationParameters.DistributionTransmissionChannel) > maxDistributionTransmissionChannelLength { - // note that the distribution transmission channel can be the empty string (i.e., "") and hence we only check its max length here - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "distribution transmission channel exceeds %d length", maxDistributionTransmissionChannelLength) + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "DistributionTransmissionChannel: %s", err.Error()) } if err := ccvtypes.ValidatePositiveInt64(initializationParameters.HistoricalEntries); err != nil { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "historical entries has to be positive") + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "HistoricalEntries: %s", err.Error()) } if err := ccvtypes.ValidateDuration(initializationParameters.CcvTimeoutPeriod); err != nil { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "ccv timeout period cannot be zero") + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "CcvTimeoutPeriod: %s", err.Error()) } if err := ccvtypes.ValidateDuration(initializationParameters.TransferTimeoutPeriod); err != nil { - return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "transfer timeout period cannot be zero") + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "TransferTimeoutPeriod: %s", err.Error()) } if err := ccvtypes.ValidateDuration(initializationParameters.UnbondingPeriod); err != nil { - return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "invalid unbonding period: %s", err.Error()) + return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "UnbondingPeriod: %s", err.Error()) } return nil } + +// validateHash validates a hash +func validateHash(hash []byte) error { + if len(hash) == 0 { + return fmt.Errorf("hash cannot be empty") + } + if len(hash) > MaxHashLength { + return fmt.Errorf("hash is too long; got: %d, max: %d", len(hash), MaxHashLength) + } + return nil +} diff --git a/x/ccv/provider/types/msg_test.go b/x/ccv/provider/types/msg_test.go index 380adced0d..7b54d8c8f6 100644 --- a/x/ccv/provider/types/msg_test.go +++ b/x/ccv/provider/types/msg_test.go @@ -2,7 +2,9 @@ package types_test import ( "testing" + "time" + clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" "github.com/cosmos/interchain-security/v5/x/ccv/provider/types" "github.com/stretchr/testify/require" ) @@ -49,7 +51,220 @@ func TestValidateStringField(t *testing.T) { } for _, tc := range testCases { - err := types.ValidateStringField("field", tc.field, tc.maxLength) + err := types.ValidateStringField(tc.name, tc.field, tc.maxLength) + if tc.valid { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } + } +} + +func TestTruncateString(t *testing.T) { + testCases := []struct { + str string + maxLength int + expStr string + }{ + {"drink", 3, "dri"}, + {"drink", 6, "drink"}, + {"drink", 0, ""}, + {"drink", -1, ""}, + {"drink", 100, "drink"}, + {"pub", 100, "pub"}, + {"こんにちは", 3, "こんに"}, + } + + for _, tc := range testCases { + truncated := types.TruncateString(tc.str, tc.maxLength) + require.Equal(t, tc.expStr, truncated) + } +} + +func TestValidateInitializationParameters(t *testing.T) { + now := time.Now().UTC() + coolStr := "Cosmos Hub is the best place to launch a chain. Interchain Security is awesome." + tooLongHash := []byte(coolStr) + + testCases := []struct { + name string + params types.ConsumerInitializationParameters + valid bool + }{ + { + name: "valid", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: true, + }, + { + name: "invalid - zero height", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.ZeroHeight(), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid - hash too long", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: tooLongHash, + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid - zero spawn time", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: time.Time{}, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid - zero duration", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: 0, + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid -- ConsumerRedistributionFraction > 1", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "1.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid -- ConsumerRedistributionFraction wrong format", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: coolStr, + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid - BlocksPerDistributionTransmission zero", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 0, + HistoricalEntries: 10000, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid - HistoricalEntries zero", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 0, + DistributionTransmissionChannel: "", + }, + valid: false, + }, + { + name: "invalid - DistributionTransmissionChannel too long", + params: types.ConsumerInitializationParameters{ + InitialHeight: clienttypes.NewHeight(3, 4), + GenesisHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, // cannot be empty + SpawnTime: now, + UnbondingPeriod: time.Duration(100000000000), + CcvTimeoutPeriod: time.Duration(100000000000), + TransferTimeoutPeriod: time.Duration(100000000000), + ConsumerRedistributionFraction: "0.75", + BlocksPerDistributionTransmission: 10, + HistoricalEntries: 10000, + DistributionTransmissionChannel: coolStr, + }, + valid: false, + }, + } + + for _, tc := range testCases { + err := types.ValidateInitializationParameters(tc.params) if tc.valid { require.NoError(t, err, tc.name) } else { From 9dbdb0a86b936fa866f568968ff8d04fd6f37f62 Mon Sep 17 00:00:00 2001 From: mpoke Date: Fri, 30 Aug 2024 10:12:40 +0200 Subject: [PATCH 3/5] ValidatePowerShapingParameters --- x/ccv/provider/types/msg.go | 35 +++++++++++++-------- x/ccv/provider/types/msg_test.go | 53 ++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index 65f38fc9cb..f1b0fdd210 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -40,9 +40,10 @@ const ( MaxDescriptionLength = 10000 // MaxMetadataLength defines the maximum consumer metadata length MaxMetadataLength = 255 - // MaxHashLength defines the maximum length of a hash MaxHashLength = 64 + // MaxValidatorCount defines the maximum number of validators + MaxValidatorCount = 1000 ) var ( @@ -793,27 +794,37 @@ func ValidateConsumerMetadata(metadata ConsumerMetadata) error { return nil } +// ValidateConsAddressList validates a list of consensus addresses +func ValidateConsAddressList(list []string, maxLength int) error { + if len(list) > maxLength { + return fmt.Errorf("consensus address list too long; got: %d, max: %d", len(list), maxLength) + } + for _, address := range list { + _, err := sdk.ConsAddressFromBech32(address) + if err != nil { + return fmt.Errorf("invalid address %s: %s", address, err.Error()) + } + } + return nil +} + // ValidatePowerShapingParameters validates that all the provided power-shaping parameters are in the expected range func ValidatePowerShapingParameters(powerShapingParameters PowerShapingParameters) error { - const maxAllowlistLength = 500 - const maxDenylistLength = 500 - // Top N corresponds to the top N% of validators that have to validate the consumer chain and can only be 0 (for an // Opt In chain) or in the range [50, 100] (for a Top N chain). if powerShapingParameters.Top_N != 0 && (powerShapingParameters.Top_N < 50 || powerShapingParameters.Top_N > 100) { - return fmt.Errorf("parameter Top N can either be 0 or in the range [50, 100]") + return errorsmod.Wrap(ErrInvalidPowerShapingParameters, "Top N can either be 0 or in the range [50, 100]") } - if powerShapingParameters.ValidatorsPowerCap != 0 && powerShapingParameters.ValidatorsPowerCap > 100 { - return fmt.Errorf("validators' power cap has to be in the range [1, 100]") + if powerShapingParameters.ValidatorsPowerCap > 100 { + return errorsmod.Wrap(ErrInvalidPowerShapingParameters, "ValidatorsPowerCap has to be in the range [0, 100]") } - if len(powerShapingParameters.Allowlist) > maxAllowlistLength { - return fmt.Errorf("allowlist cannot exceed length: %d", maxAllowlistLength) + if err := ValidateConsAddressList(powerShapingParameters.Allowlist, MaxValidatorCount); err != nil { + return errorsmod.Wrapf(ErrInvalidPowerShapingParameters, "Allowlist: %s", err.Error()) } - - if len(powerShapingParameters.Denylist) > maxDenylistLength { - return fmt.Errorf("denylist cannot exceed length: %d", maxDenylistLength) + if err := ValidateConsAddressList(powerShapingParameters.Denylist, MaxValidatorCount); err != nil { + return errorsmod.Wrapf(ErrInvalidPowerShapingParameters, "Denylist: %s", err.Error()) } return nil diff --git a/x/ccv/provider/types/msg_test.go b/x/ccv/provider/types/msg_test.go index 7b54d8c8f6..284f2a5f21 100644 --- a/x/ccv/provider/types/msg_test.go +++ b/x/ccv/provider/types/msg_test.go @@ -272,3 +272,56 @@ func TestValidateInitializationParameters(t *testing.T) { } } } + +func TestValidateConsAddressList(t *testing.T) { + consAddr1 := "cosmosvalcons1qmq08eruchr5sf5s3rwz7djpr5a25f7xw4mceq" + consAddr2 := "cosmosvalcons1nx7n5uh0ztxsynn4sje6eyq2ud6rc6klc96w39" + invalidConsAddr := "cosmosvalcons1nx7n5uh0ztxsynn4sje6ey" + + testCases := []struct { + name string + list []string + maxLength int + valid bool + }{ + { + name: "valid - empty list", + list: []string{}, + maxLength: 10, + valid: true, + }, + { + name: "valid - non-empty list", + list: []string{consAddr1, consAddr2}, + maxLength: 10, + valid: true, + }, + { + name: "invalid - address with wrong format", + list: []string{invalidConsAddr}, + maxLength: 10, + valid: false, + }, + { + name: "invalid - empty address", + list: []string{""}, + maxLength: 10, + valid: false, + }, + { + name: "invalid - list length", + list: []string{consAddr1, consAddr2}, + maxLength: 1, + valid: false, + }, + } + + for _, tc := range testCases { + err := types.ValidateConsAddressList(tc.list, tc.maxLength) + if tc.valid { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } + } +} From 0a73edcd96acca045c4b9fb50489ae1834d49b42 Mon Sep 17 00:00:00 2001 From: mpoke Date: Fri, 30 Aug 2024 10:28:34 +0200 Subject: [PATCH 4/5] validate NewOwnerAddress --- x/ccv/provider/types/errors.go | 2 ++ x/ccv/provider/types/msg.go | 26 +++++++++++++++----------- x/ccv/types/shared_params.go | 2 +- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/x/ccv/provider/types/errors.go b/x/ccv/provider/types/errors.go index 57ab5c2a99..90bb46ccaf 100644 --- a/x/ccv/provider/types/errors.go +++ b/x/ccv/provider/types/errors.go @@ -49,4 +49,6 @@ var ( ErrCannotCreateTopNChain = errorsmod.Register(ModuleName, 41, "cannot create Top N chain outside permissionlessly") ErrCannotPrepareForLaunch = errorsmod.Register(ModuleName, 42, "cannot prepare chain for launch") ErrInvalidStopTime = errorsmod.Register(ModuleName, 43, "invalid stop time") + ErrInvalidMsgCreateConsumer = errorsmod.Register(ModuleName, 44, "invalid create consumer message") + ErrInvalidMsgUpdateConsumer = errorsmod.Register(ModuleName, 45, "invalid update consumer message") ) diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index f1b0fdd210..406886b325 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -269,16 +269,16 @@ func (msg MsgCreateConsumer) Route() string { return RouterKey } // ValidateBasic implements the sdk.Msg interface. func (msg MsgCreateConsumer) ValidateBasic() error { if err := ValidateStringField("ChainId", msg.ChainId, cmttypes.MaxChainIDLen); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgCreateConsumer, "ChainId: %s", err.Error()) } if err := ValidateConsumerMetadata(msg.Metadata); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgCreateConsumer, "Metadata: %s", err.Error()) } if msg.InitializationParameters != nil { if err := ValidateInitializationParameters(*msg.InitializationParameters); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgCreateConsumer, "InitializationParameters: %s", err.Error()) } } @@ -288,7 +288,7 @@ func (msg MsgCreateConsumer) ValidateBasic() error { "first create the chain and then use `MsgUpdateConsumer` to make the chain Top N") } if err := ValidatePowerShapingParameters(*msg.PowerShapingParameters); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgCreateConsumer, "PowerShapingParameters: %s", err.Error()) } } @@ -336,24 +336,28 @@ func (msg MsgUpdateConsumer) Route() string { return RouterKey } // ValidateBasic implements the sdk.Msg interface. func (msg MsgUpdateConsumer) ValidateBasic() error { if err := ValidateConsumerId(msg.ConsumerId); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "ConsumerId: %s", err.Error()) + } + + if err := ccvtypes.ValidateAccAddress(msg.NewOwnerAddress); err != nil { + return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "NewOwnerAddress: %s", err.Error()) } if msg.Metadata != nil { if err := ValidateConsumerMetadata(*msg.Metadata); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "Metadata: %s", err.Error()) } } if msg.InitializationParameters != nil { if err := ValidateInitializationParameters(*msg.InitializationParameters); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "InitializationParameters: %s", err.Error()) } } if msg.PowerShapingParameters != nil { if err := ValidatePowerShapingParameters(*msg.PowerShapingParameters); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "PowerShapingParameters: %s", err.Error()) } } @@ -780,15 +784,15 @@ func TruncateString(str string, length int) string { // ValidateConsumerMetadata validates that all the provided metadata are in the expected range func ValidateConsumerMetadata(metadata ConsumerMetadata) error { if err := ValidateStringField("name", metadata.Name, MaxNameLength); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidConsumerMetadata, "Name: %s", err.Error()) } if err := ValidateStringField("description", metadata.Description, MaxDescriptionLength); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidConsumerMetadata, "Description: %s", err.Error()) } if err := ValidateStringField("metadata", metadata.Metadata, MaxMetadataLength); err != nil { - return err + return errorsmod.Wrapf(ErrInvalidConsumerMetadata, "Metadata: %s", err.Error()) } return nil diff --git a/x/ccv/types/shared_params.go b/x/ccv/types/shared_params.go index fa2e28c81c..d1a82df576 100644 --- a/x/ccv/types/shared_params.go +++ b/x/ccv/types/shared_params.go @@ -77,7 +77,7 @@ func ValidateChannelIdentifier(i interface{}) error { return ibchost.ChannelIdentifierValidator(value) } -func ValidateBech32(i interface{}) error { +func ValidateAccAddress(i interface{}) error { value, ok := i.(string) if !ok { return fmt.Errorf("invalid parameter type: %T", i) From 27c616771472fad3cefc2ec76dac51729942a5ef Mon Sep 17 00:00:00 2001 From: mpoke Date: Fri, 30 Aug 2024 11:56:56 +0200 Subject: [PATCH 5/5] apply review suggestions --- x/ccv/provider/types/msg.go | 24 ++++------ x/ccv/provider/types/msg_test.go | 75 ++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 34 deletions(-) diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index 406886b325..9bca3f996f 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -36,7 +36,7 @@ const ( // MaxNameLength defines the maximum consumer name length MaxNameLength = 50 - // MaxDescriptionLength + // MaxDescriptionLength defines the maximum consumer description length MaxDescriptionLength = 10000 // MaxMetadataLength defines the maximum consumer metadata length MaxMetadataLength = 255 @@ -339,9 +339,7 @@ func (msg MsgUpdateConsumer) ValidateBasic() error { return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "ConsumerId: %s", err.Error()) } - if err := ccvtypes.ValidateAccAddress(msg.NewOwnerAddress); err != nil { - return errorsmod.Wrapf(ErrInvalidMsgUpdateConsumer, "NewOwnerAddress: %s", err.Error()) - } + // Note that NewOwnerAddress is validated when handling the message in UpdateConsumer if msg.Metadata != nil { if err := ValidateConsumerMetadata(*msg.Metadata); err != nil { @@ -764,8 +762,8 @@ func ValidateStringField(nameOfTheField string, field string, maxLength int) err } // TruncateString truncates a string to maximum length characters -func TruncateString(str string, length int) string { - if length <= 0 { +func TruncateString(str string, maxLength int) string { + if maxLength <= 0 { return "" } @@ -774,7 +772,7 @@ func TruncateString(str string, length int) string { for _, char := range str { truncated += string(char) count++ - if count >= length { + if count >= maxLength { break } } @@ -840,11 +838,11 @@ func ValidateInitializationParameters(initializationParameters ConsumerInitializ return errorsmod.Wrap(ErrInvalidConsumerInitializationParameters, "InitialHeight cannot be zero") } - if err := validateHash(initializationParameters.GenesisHash); err != nil { + if err := ValidateByteSlice(initializationParameters.GenesisHash, MaxHashLength); err != nil { return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "GenesisHash: %s", err.Error()) } - if err := validateHash(initializationParameters.BinaryHash); err != nil { + if err := ValidateByteSlice(initializationParameters.BinaryHash, MaxHashLength); err != nil { return errorsmod.Wrapf(ErrInvalidConsumerInitializationParameters, "BinaryHash: %s", err.Error()) } @@ -883,12 +881,8 @@ func ValidateInitializationParameters(initializationParameters ConsumerInitializ return nil } -// validateHash validates a hash -func validateHash(hash []byte) error { - if len(hash) == 0 { - return fmt.Errorf("hash cannot be empty") - } - if len(hash) > MaxHashLength { +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 nil diff --git a/x/ccv/provider/types/msg_test.go b/x/ccv/provider/types/msg_test.go index 284f2a5f21..60f7d31675 100644 --- a/x/ccv/provider/types/msg_test.go +++ b/x/ccv/provider/types/msg_test.go @@ -95,8 +95,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "valid", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -112,8 +112,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid - zero height", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.ZeroHeight(), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -130,7 +130,7 @@ func TestValidateInitializationParameters(t *testing.T) { params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), GenesisHash: tooLongHash, - BinaryHash: []byte{0x01}, // cannot be empty + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -146,8 +146,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid - zero spawn time", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: time.Time{}, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -163,8 +163,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid - zero duration", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: 0, CcvTimeoutPeriod: time.Duration(100000000000), @@ -180,8 +180,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid -- ConsumerRedistributionFraction > 1", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -197,8 +197,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid -- ConsumerRedistributionFraction wrong format", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -214,8 +214,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid - BlocksPerDistributionTransmission zero", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -231,8 +231,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid - HistoricalEntries zero", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -248,8 +248,8 @@ func TestValidateInitializationParameters(t *testing.T) { name: "invalid - DistributionTransmissionChannel too long", params: types.ConsumerInitializationParameters{ InitialHeight: clienttypes.NewHeight(3, 4), - GenesisHash: []byte{0x01}, // cannot be empty - BinaryHash: []byte{0x01}, // cannot be empty + GenesisHash: []byte{0x01}, + BinaryHash: []byte{0x01}, SpawnTime: now, UnbondingPeriod: time.Duration(100000000000), CcvTimeoutPeriod: time.Duration(100000000000), @@ -325,3 +325,40 @@ func TestValidateConsAddressList(t *testing.T) { } } } + +func TestValidateByteSlice(t *testing.T) { + testCases := []struct { + name string + slice []byte + maxLength int + valid bool + }{ + { + name: "valid: empty", + slice: []byte{}, + maxLength: 5, + valid: true, + }, + { + name: "invalid: too long", + slice: []byte{0x01, 0x02}, + maxLength: 1, + valid: false, + }, + { + name: "valid", + slice: []byte{0x01, 0x02}, + maxLength: 5, + valid: true, + }, + } + + for _, tc := range testCases { + err := types.ValidateByteSlice(tc.slice, tc.maxLength) + if tc.valid { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } + } +}