From d9561c2d086ae061e5b8b3e50962c79734612ff0 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Sun, 24 Mar 2024 19:56:38 -0600 Subject: [PATCH] Revert "Backport: Removal of regex usage on denom validation (#570)" This reverts commit ab4bc056639199fce60f3275cf5b24acebc22f2d. --- CHANGELOG.md | 2 - tests/e2e/authz/tx.go | 2 +- .../bank/keeper/deterministic_test.go | 2 +- .../staking/keeper/determinstic_test.go | 3 +- types/coin.go | 67 ++++++------------- types/coin_test.go | 31 +-------- types/dec_coin.go | 66 ++---------------- x/authz/client/cli/tx_test.go | 2 +- x/gov/client/cli/util_test.go | 8 +-- 9 files changed, 37 insertions(+), 146 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a5da4f64b62..5913ceaa4b17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,8 +37,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] -## [State Breaking] - * (store) [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV speedups * (slashing) [#548](https://github.com/osmosis-labs/cosmos-sdk/pull/548) Implement v0.50 slashing bitmap logic * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block diff --git a/tests/e2e/authz/tx.go b/tests/e2e/authz/tx.go index aad6f4cf6749..03d39c79441a 100644 --- a/tests/e2e/authz/tx.go +++ b/tests/e2e/authz/tx.go @@ -339,7 +339,7 @@ func (s *E2ETestSuite) TestCLITxGrantAuthorization() { }, 0, true, - "invalid character in denomination", + "invalid decimal coin expression", }, { "valid tx delegate authorization allowed validators", diff --git a/tests/integration/bank/keeper/deterministic_test.go b/tests/integration/bank/keeper/deterministic_test.go index 1bc3d4de8cb5..112d11489260 100644 --- a/tests/integration/bank/keeper/deterministic_test.go +++ b/tests/integration/bank/keeper/deterministic_test.go @@ -36,7 +36,7 @@ type DeterministicTestSuite struct { } var ( - denomRegex = `[a-zA-Z][a-zA-Z0-9/:._-]{2,127}` + denomRegex = sdk.DefaultCoinDenomRegex() addr1 = sdk.MustAccAddressFromBech32("cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5") coin1 = sdk.NewCoin("denom", sdk.NewInt(10)) metadataAtom = banktypes.Metadata{ diff --git a/tests/integration/staking/keeper/determinstic_test.go b/tests/integration/staking/keeper/determinstic_test.go index 08f17fcd8ae8..eefb8c7ec819 100644 --- a/tests/integration/staking/keeper/determinstic_test.go +++ b/tests/integration/staking/keeper/determinstic_test.go @@ -687,10 +687,9 @@ func (suite *DeterministicTestSuite) TestGRPCRedelegations() { } func (suite *DeterministicTestSuite) TestGRPCParams() { - coinDenomRegex := `[a-zA-Z][a-zA-Z0-9/:._-]{2,127}` rapid.Check(suite.T(), func(t *rapid.T) { params := stakingtypes.Params{ - BondDenom: rapid.StringMatching(coinDenomRegex).Draw(t, "bond-denom"), + BondDenom: rapid.StringMatching(sdk.DefaultCoinDenomRegex()).Draw(t, "bond-denom"), UnbondingTime: durationGenerator().Draw(t, "duration"), MaxValidators: rapid.Uint32Min(1).Draw(t, "max-validators"), MaxEntries: rapid.Uint32Min(1).Draw(t, "max-entries"), diff --git a/types/coin.go b/types/coin.go index dbf7dc50cc31..41a8b24f52cf 100644 --- a/types/coin.go +++ b/types/coin.go @@ -6,7 +6,6 @@ import ( "regexp" "sort" "strings" - "unicode" ) //----------------------------------------------------------------------------- @@ -836,15 +835,30 @@ func (coins Coins) Sort() Coins { return coins } +//----------------------------------------------------------------------------- +// Parsing + var ( - reDecAmt = `[[:digit:]]+(?:\.[[:digit:]]+)?|\.[[:digit:]]+` - reSpc = `[[:space:]]*` + // Denominations can be 3 ~ 128 characters long and support letters, followed by either + // a letter, a number or a separator ('/', ':', '.', '_' or '-'). + reDnmString = `[a-zA-Z][a-zA-Z0-9/:._-]{2,127}` + reDecAmt = `[[:digit:]]+(?:\.[[:digit:]]+)?|\.[[:digit:]]+` + reSpc = `[[:space:]]*` + reDnm *regexp.Regexp + reDecCoin *regexp.Regexp +) - coinDenomRegex func() string +func init() { + SetCoinDenomRegex(DefaultCoinDenomRegex) +} - reDnm *regexp.Regexp - reDecCoin *regexp.Regexp -) +// DefaultCoinDenomRegex returns the default regex string +func DefaultCoinDenomRegex() string { + return reDnmString +} + +// coinDenomRegex returns the current regex string and can be overwritten for custom validation +var coinDenomRegex = DefaultCoinDenomRegex // SetCoinDenomRegex allows for coin's custom validation by overriding the regular // expression string used for denom validation. @@ -857,49 +871,12 @@ func SetCoinDenomRegex(reFn func() string) { // ValidateDenom is the default validation function for Coin.Denom. func ValidateDenom(denom string) error { - if reDnm == nil || reDecCoin == nil { //nolint:gocritic - // Convert the string to a byte slice as required by the Ragel-generated function. - - // Call the Ragel-generated function. - if !MatchDenom(denom) { //nolint:gocritic - return fmt.Errorf("invalid denom: %s", denom) - } - } else if !reDnm.MatchString(denom) { // If reDnm has been initialized, use it for matching. + if !reDnm.MatchString(denom) { return fmt.Errorf("invalid denom: %s", denom) } - return nil } -// isValidRune checks if a given rune is a valid character for a rune. -// It returns true if the rune is a letter, digit, '/', ':', '.', '_', or '-'. -func isValidRune(r rune) bool { - return unicode.IsLetter(r) || unicode.IsDigit(r) || r == '/' || r == ':' || r == '.' || r == '_' || r == '-' -} - -// MatchDenom checks if the given string is a valid denomination. -// A valid denomination must have a length between 3 and 128 characters, -// start with a letter, and only contain valid runes. -func MatchDenom(s string) bool { - length := len(s) - if length < 3 || length > 128 { - return false - } - - firstRune := rune(s[0]) - if !unicode.IsLetter(firstRune) { - return false - } - - for _, r := range s[1:] { - if !isValidRune(r) { - return false - } - } - - return true -} - func mustValidateDenom(denom string) { if err := ValidateDenom(denom); err != nil { panic(err) diff --git a/types/coin_test.go b/types/coin_test.go index 1d8b663017b7..f2337c1586f3 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -112,8 +112,6 @@ func (s *coinTestSuite) TestCoinIsValid() { func (s *coinTestSuite) TestCustomValidation() { newDnmRegex := `[\x{1F600}-\x{1F6FF}]` - reDnmString := `[a-zA-Z][a-zA-Z0-9/:._-]{2,127}` - sdk.SetCoinDenomRegex(func() string { return newDnmRegex }) @@ -132,7 +130,7 @@ func (s *coinTestSuite) TestCustomValidation() { for i, tc := range cases { s.Require().Equal(tc.expectPass, tc.coin.IsValid(), "unexpected result for IsValid, tc #%d", i) } - sdk.SetCoinDenomRegex(func() string { return reDnmString }) + sdk.SetCoinDenomRegex(sdk.DefaultCoinDenomRegex) } func (s *coinTestSuite) TestCoinsDenoms() { @@ -1000,33 +998,6 @@ func (s *coinTestSuite) TestParseCoins() { } } -func (s *coinTestSuite) TestValidateDenom() { - cases := []struct { - input string - valid bool - }{ - {"", false}, - {"stake", true}, - {"stake,", false}, - {"me coin", false}, - {"me coin much", false}, - {"not a coin", false}, - {"foo:bar", true}, // special characters '/' | ':' | '.' | '_' | '-' are allowed - {"atom10", true}, // number in denom is allowed - {"transfer/channelToA/uatom", true}, - {"ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", true}, - } - - for _, tc := range cases { - err := sdk.ValidateDenom(tc.input) - if !tc.valid { - s.Require().Error(err) - } else { - s.Require().NoError(err) - } - } -} - func (s *coinTestSuite) TestSortCoins() { good := sdk.Coins{ sdk.NewInt64Coin("gas", 1), diff --git a/types/dec_coin.go b/types/dec_coin.go index a279471db307..42ff885d58a6 100644 --- a/types/dec_coin.go +++ b/types/dec_coin.go @@ -4,7 +4,6 @@ import ( "fmt" "sort" "strings" - "unicode" "github.com/pkg/errors" ) @@ -622,24 +621,15 @@ func (coins DecCoins) Sort() DecCoins { // ParseDecCoin parses a decimal coin from a string, returning an error if // invalid. An empty string is considered invalid. func ParseDecCoin(coinStr string) (coin DecCoin, err error) { - var amountStr, denomStr string - // if custom parsing has not been set, use default coin regex - if reDecCoin == nil { //nolint:gocritic - amountStr, denomStr, err = ParseDecAmount(coinStr) - if err != nil { - return DecCoin{}, err - } - } else { - coinStr = strings.TrimSpace(coinStr) - - matches := reDecCoin.FindStringSubmatch(coinStr) - if matches == nil { - return DecCoin{}, fmt.Errorf("invalid decimal coin expression: %s", coinStr) - } + coinStr = strings.TrimSpace(coinStr) - amountStr, denomStr = matches[1], matches[2] + matches := reDecCoin.FindStringSubmatch(coinStr) + if matches == nil { + return DecCoin{}, fmt.Errorf("invalid decimal coin expression: %s", coinStr) } + amountStr, denomStr := matches[1], matches[2] + amount, err := NewDecFromStr(amountStr) if err != nil { return DecCoin{}, errors.Wrap(err, fmt.Sprintf("failed to parse decimal coin amount: %s", amountStr)) @@ -652,50 +642,6 @@ func ParseDecCoin(coinStr string) (coin DecCoin, err error) { return NewDecCoinFromDec(denomStr, amount), nil } -// ParseDecAmount parses the given string into amount, denomination. -func ParseDecAmount(coinStr string) (string, string, error) { - var amountRune, denomRune []rune - - // Indicates the start of denom parsing - seenLetter := false - // Indicates we're currently parsing the amount - parsingAmount := true - - for _, r := range strings.TrimSpace(coinStr) { - if parsingAmount { //nolint:gocritic - if unicode.IsDigit(r) || r == '.' { //nolint:gocritic - amountRune = append(amountRune, r) - } else if unicode.IsSpace(r) { // if space is seen, indicates that we have finished parsing amount - parsingAmount = false - } else if unicode.IsLetter(r) { // if letter is seen, indicates that it is the start of denom - parsingAmount = false - seenLetter = true - denomRune = append(denomRune, r) - } else { // Invalid character encountered in amount part - return "", "", fmt.Errorf("invalid character in coin string: %s", string(r)) - } - } else if !seenLetter { // This logic flow is for skipping spaces between amount and denomination - if unicode.IsLetter(r) { - seenLetter = true - denomRune = append(denomRune, r) - } else if !unicode.IsSpace(r) { - // Invalid character before denomination starts - return "", "", fmt.Errorf("invalid start of denomination: %s", string(r)) - } - } else { - // Parsing the denomination - if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '/' || r == ':' || r == '.' || r == '_' || r == '-' { - denomRune = append(denomRune, r) - } else { - // Invalid character encountered in denomination part - return "", "", fmt.Errorf("invalid character in denomination: %s", string(r)) - } - } - } - - return string(amountRune), string(denomRune), nil -} - // ParseDecCoins will parse out a list of decimal coins separated by commas. If the parsing is successuful, // the provided coins will be sanitized by removing zero coins and sorting the coin set. Lastly // a validation of the coin set is executed. If the check passes, ParseDecCoins will return the sanitized coins. diff --git a/x/authz/client/cli/tx_test.go b/x/authz/client/cli/tx_test.go index b5e7ee74eddb..7355c3591601 100644 --- a/x/authz/client/cli/tx_test.go +++ b/x/authz/client/cli/tx_test.go @@ -326,7 +326,7 @@ func (s *CLITestSuite) TestCLITxGrantAuthorization() { fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(10))).String()), }, true, - "nvalid character in denomination: ", + "invalid decimal coin expression", }, { "Valid tx send authorization", diff --git a/x/gov/client/cli/util_test.go b/x/gov/client/cli/util_test.go index 05922f8c205d..53b40293e376 100644 --- a/x/gov/client/cli/util_test.go +++ b/x/gov/client/cli/util_test.go @@ -347,7 +347,7 @@ func TestReadGovPropFlags(t *testing.T) { name: "only deposit invalid coins", fromAddr: nil, args: []string{argDeposit, "not really coins"}, - expErr: []string{"invalid deposit", "invalid character in denomination"}, + expErr: []string{"invalid deposit", "invalid decimal coin expression", "not really coins"}, }, { name: "only deposit two coins", @@ -377,19 +377,19 @@ func TestReadGovPropFlags(t *testing.T) { name: "only deposit coin 1 of 3 bad", fromAddr: nil, args: []string{argDeposit, "1bad^coin,2bcoin,3ccoin"}, - expErr: []string{"invalid deposit", "invalid character in denomination"}, + expErr: []string{"invalid deposit", "invalid decimal coin expression", "1bad^coin"}, }, { name: "only deposit coin 2 of 3 bad", fromAddr: nil, args: []string{argDeposit, "1acoin,2bad^coin,3ccoin"}, - expErr: []string{"invalid deposit", "invalid character in denomination"}, + expErr: []string{"invalid deposit", "invalid decimal coin expression", "2bad^coin"}, }, { name: "only deposit coin 3 of 3 bad", fromAddr: nil, args: []string{argDeposit, "1acoin,2bcoin,3bad^coin"}, - expErr: []string{"invalid deposit", "invalid character in denomination"}, + expErr: []string{"invalid deposit", "invalid decimal coin expression", "3bad^coin"}, }, // As far as I can tell, there's no way to make flagSet.GetString return an error for a defined string flag. // So I don't have a test for the "could not read deposit" error case.