Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add back removal regex #591

Merged
merged 2 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

## v24

## [v0.47.5-v24-osmo-2](https://github.com/osmosis-labs/cosmos-sdk/releases/tag/v0.47.5-v24-osmo-2)
## [v0.47.5-v24-osmo-3](https://github.com/osmosis-labs/cosmos-sdk/releases/tag/v0.47.5-v24-osmo-3)

* (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
Expand Down
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 decimal coin expression",
"invalid character in denomination",
},
{
"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 = sdk.DefaultCoinDenomRegex()
denomRegex = `[a-zA-Z][a-zA-Z0-9/:._-]{2,127}`
addr1 = sdk.MustAccAddressFromBech32("cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5")
coin1 = sdk.NewCoin("denom", sdk.NewInt(10))
metadataAtom = banktypes.Metadata{
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/staking/keeper/determinstic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -687,9 +687,10 @@ 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(sdk.DefaultCoinDenomRegex()).Draw(t, "bond-denom"),
BondDenom: rapid.StringMatching(coinDenomRegex).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: 45 additions & 22 deletions types/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"regexp"
"sort"
"strings"
"unicode"
)

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -835,30 +836,15 @@ func (coins Coins) Sort() Coins {
return coins
}

//-----------------------------------------------------------------------------
// Parsing

var (
// 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
)
reDecAmt = `[[:digit:]]+(?:\.[[:digit:]]+)?|\.[[:digit:]]+`
reSpc = `[[:space:]]*`

func init() {
SetCoinDenomRegex(DefaultCoinDenomRegex)
}
coinDenomRegex func() string

// 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
reDnm *regexp.Regexp
reDecCoin *regexp.Regexp
)

// SetCoinDenomRegex allows for coin's custom validation by overriding the regular
// expression string used for denom validation.
Expand All @@ -871,12 +857,49 @@ func SetCoinDenomRegex(reFn func() string) {

// ValidateDenom is the default validation function for Coin.Denom.
func ValidateDenom(denom string) error {
if !reDnm.MatchString(denom) {
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.
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
}
Comment on lines +874 to +901
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The introduction of isValidRune and MatchDenom functions enhances the validation logic by providing clear, focused checks for denomination validity. This modular approach improves the maintainability and readability of the code. It's recommended to add unit tests for these functions to ensure their correctness and handle edge cases effectively.

Would you like me to help with adding unit tests for these new validation functions?


func mustValidateDenom(denom string) {
if err := ValidateDenom(denom); err != nil {
panic(err)
Expand Down
31 changes: 30 additions & 1 deletion types/coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ 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 @@ -130,7 +132,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(sdk.DefaultCoinDenomRegex)
sdk.SetCoinDenomRegex(func() string { return reDnmString })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After modifying the coin denomination regex with sdk.SetCoinDenomRegex, it's a good practice to reset it to its original state at the end of the test to prevent unintended side effects on subsequent tests.

+ defer sdk.SetCoinDenomRegex(sdk.DefaultCoinDenomRegex)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
sdk.SetCoinDenomRegex(func() string { return reDnmString })
sdk.SetCoinDenomRegex(func() string { return reDnmString })
defer sdk.SetCoinDenomRegex(sdk.DefaultCoinDenomRegex)

}

func (s *coinTestSuite) TestCoinsDenoms() {
Expand Down Expand Up @@ -998,6 +1000,33 @@ 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: 60 additions & 6 deletions types/dec_coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"sort"
"strings"
"unicode"

"github.com/pkg/errors"
)
Expand Down Expand Up @@ -621,14 +622,23 @@ 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) {
coinStr = strings.TrimSpace(coinStr)
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)
}
matches := reDecCoin.FindStringSubmatch(coinStr)
if matches == nil {
return DecCoin{}, fmt.Errorf("invalid decimal coin expression: %s", coinStr)
}

amountStr, denomStr := matches[1], matches[2]
amountStr, denomStr = matches[1], matches[2]
}

amount, err := NewDecFromStr(amountStr)
if err != nil {
Expand All @@ -642,6 +652,50 @@ 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,
"invalid decimal coin expression",
"nvalid character in denomination: ",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message has been modified to be more specific, which is beneficial. However, there appears to be a typographical error in the message. It should likely start with "Invalid" instead of "nvalid".

- "nvalid character in denomination: ",
+ "Invalid character in denomination: ",

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
"nvalid character in denomination: ",
"Invalid character in denomination: ",

},
{
"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 decimal coin expression", "not really coins"},
expErr: []string{"invalid deposit", "invalid character in denomination"},
},
{
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 decimal coin expression", "1bad^coin"},
expErr: []string{"invalid deposit", "invalid character in denomination"},
},
{
name: "only deposit coin 2 of 3 bad",
fromAddr: nil,
args: []string{argDeposit, "1acoin,2bad^coin,3ccoin"},
expErr: []string{"invalid deposit", "invalid decimal coin expression", "2bad^coin"},
expErr: []string{"invalid deposit", "invalid character in denomination"},
},
{
name: "only deposit coin 3 of 3 bad",
fromAddr: nil,
args: []string{argDeposit, "1acoin,2bcoin,3bad^coin"},
expErr: []string{"invalid deposit", "invalid decimal coin expression", "3bad^coin"},
expErr: []string{"invalid deposit", "invalid character in denomination"},
},
// 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
Loading