Skip to content

Commit

Permalink
Revert "Backport: Removal of regex usage on denom validation (#570)"
Browse files Browse the repository at this point in the history
This reverts commit ab4bc05.
  • Loading branch information
czarcas7ic committed Mar 25, 2024
1 parent c4af9fa commit a07f5de
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 144 deletions.
2 changes: 1 addition & 1 deletion tests/e2e/authz/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ func (s *E2ETestSuite) TestCLITxGrantAuthorization() {
},
0,
true,
"invalid character in denomination",
"invalid decimal coin expression",
},
{
"valid tx delegate authorization allowed validators",
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/bank/keeper/deterministic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
3 changes: 1 addition & 2 deletions tests/integration/staking/keeper/determinstic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
67 changes: 22 additions & 45 deletions types/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"regexp"
"sort"
"strings"
"unicode"
)

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -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.
Expand All @@ -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)
Expand Down
31 changes: 1 addition & 30 deletions types/coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand All @@ -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() {
Expand Down Expand Up @@ -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),
Expand Down
66 changes: 6 additions & 60 deletions types/dec_coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"sort"
"strings"
"unicode"

"github.com/pkg/errors"
)
Expand Down Expand Up @@ -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))
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion x/authz/client/cli/tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
8 changes: 4 additions & 4 deletions x/gov/client/cli/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit a07f5de

Please sign in to comment.