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.

Sign up for GitHub

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

cli: ParseCoin doesn't do normalize #7954

Merged
merged 2 commits into from
Nov 25, 2020
Merged

cli: ParseCoin doesn't do normalize #7954

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a7dfaf
cli: ParseCoin doesn't do normalize
yihuang Nov 17, 2020
a8eb857
Merge branch 'master' into denom-conv
mergify[bot] Nov 25, 2020
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 types/bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func BenchmarkParseCoin(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
for _, coinStr := range coinStrs {
coin, err := types.ParseCoin(coinStr)
coin, err := types.ParseCoinNormalized(coinStr)
if err != nil {
b.Fatal(err)
}
Expand Down
27 changes: 6 additions & 21 deletions types/coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,11 +599,9 @@ var (
// Denominations can be 3 ~ 128 characters long and support letters, followed by either
// a letter, a number or a separator ('/').
reDnmString = `[a-zA-Z][a-zA-Z0-9/]{2,127}`
reAmt = `[[:digit:]]+`
reDecAmt = `[[:digit:]]+(?:\.[[:digit:]]+)?|\.[[:digit:]]+`
reSpc = `[[:space:]]*`
reDnm *regexp.Regexp
reCoin *regexp.Regexp
reDecCoin *regexp.Regexp
)

Expand All @@ -625,7 +623,6 @@ func SetCoinDenomRegex(reFn func() string) {
coinDenomRegex = reFn

reDnm = regexp.MustCompile(fmt.Sprintf(`^%s$`, coinDenomRegex()))
reCoin = regexp.MustCompile(fmt.Sprintf(`^(%s)%s(%s)$`, reAmt, reSpc, coinDenomRegex()))
reDecCoin = regexp.MustCompile(fmt.Sprintf(`^(%s)%s(%s)$`, reDecAmt, reSpc, coinDenomRegex()))
}

Expand All @@ -643,29 +640,17 @@ func mustValidateDenom(denom string) {
}
}

// ParseCoin parses a cli input for one coin type, returning errors if invalid or on an empty string
// ParseCoinNormalized parses and normalize a cli input for one coin type, returning errors if invalid or on an empty string
// as well.
// Expected format: "{amount}{denomination}"
func ParseCoin(coinStr string) (coin Coin, err error) {
coinStr = strings.TrimSpace(coinStr)

matches := reCoin.FindStringSubmatch(coinStr)
if matches == nil {
return Coin{}, fmt.Errorf("invalid coin expression: %s", coinStr)
}

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

amount, ok := NewIntFromString(amountStr)
if !ok {
return Coin{}, fmt.Errorf("failed to parse coin amount: %s", amountStr)
}

if err := ValidateDenom(denomStr); err != nil {
func ParseCoinNormalized(coinStr string) (coin Coin, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using decimal types in this function?

Copy link
Collaborator Author

@yihuang yihuang Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We already support decimal coins in ParseCoinsNormalized, so it's just keep it consistent with that.
Support both int and decimal coins is useful so we can write 0.1atom in place of 10...0utom.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean why are we using decimal coins in a function that returns Coin? This seems like a codesmell to me. Can we not have separate functions for Coin and DecCoin?

Copy link
Collaborator Author

@yihuang yihuang Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's a higher level function which involves both DecCoin and Coin, the behavior is clear:

  • parse dec coin
  • do conversion
  • truncate and take the int part

mainly used to parse user inputs in cli, I guess it's just a matter of where to place this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should have distinct separate functions for these.

Copy link
Collaborator Author

@yihuang yihuang Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, just two separate methods :)

Err, sorry, what do you mean by that? currently what I want is a function to parse decimal coins and convert it to int coins, I think it's best for cli UX, do you mean we should keep the basic version of ParseCoin and ParseDecCoin, I think they are removed for dead code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Where in the code do we convert between the two often?

There should be method to parse/normalize Coin(s) and a separate method to parse/normalize DecCoin(s).

Copy link
Collaborator Author

@yihuang yihuang Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Where in the code do we convert between the two often?

I think it's only used in cli to process user input.
and we want an int coin in the end, but want to accept inputs like 0.1atom for convenience.

EDIT: the ParseDecCoin is used for parsing stuff like gas prices, I guess those can also be changed to use normalization.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. So all user input-based coins (e.g. tx send) will have the coin amount (e.g. 1.2atom, 4000uatom, etc...) will have to go through ParseCoinNormalized and only that function? If so, then I think this is OK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. So all user input-based coins (e.g. tx send) will have the coin amount (e.g. 1.2atom, 4000uatom, etc...) will have to go through ParseCoinNormalized and only that function? If so, then I think this is OK.

yes, ParseCoinNormalized and ParseCoinsNormalized.

decCoin, err := ParseDecCoin(coinStr)
if err != nil {
return Coin{}, err
}

return NewCoin(denomStr, amount), nil
coin, _ = NormalizeDecCoin(decCoin).TruncateDecimal()
return coin, nil
}

// ParseCoinsNormalized will parse out a list of coins separated by commas, and normalize them by converting to smallest
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/applications/transfer/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ to the counterparty channel. Any timeout set to 0 is disabled.`),
srcChannel := args[1]
receiver := args[2]

coin, err := sdk.ParseCoin(args[3])
coin, err := sdk.ParseCoinNormalized(args[3])
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion x/staking/client/cli/cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (s *IntegrationTestSuite) SetupSuite() {
_, err := s.network.WaitForHeight(1)
s.Require().NoError(err)

unbond, err := sdk.ParseCoin("10stake")
unbond, err := sdk.ParseCoinNormalized("10stake")
s.Require().NoError(err)

val := s.network.Validators[0]
Expand Down
10 changes: 5 additions & 5 deletions x/staking/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ $ %s tx staking delegate %s1l2rsakp388kuv9k8qzq6lrm9taddae7fpx59wm 1000stake --f
return err
}

amount, err := sdk.ParseCoin(args[1])
amount, err := sdk.ParseCoinNormalized(args[1])
if err != nil {
return err
}
Expand Down Expand Up @@ -231,7 +231,7 @@ $ %s tx staking redelegate %s1gghjut3ccd8ay0zduzj64hwre2fxs9ldmqhffj %s1l2rsakp3
return err
}

amount, err := sdk.ParseCoin(args[2])
amount, err := sdk.ParseCoinNormalized(args[2])
if err != nil {
return err
}
Expand Down Expand Up @@ -279,7 +279,7 @@ $ %s tx staking unbond %s1gghjut3ccd8ay0zduzj64hwre2fxs9ldmqhffj 100stake --from
return err
}

amount, err := sdk.ParseCoin(args[1])
amount, err := sdk.ParseCoinNormalized(args[1])
if err != nil {
return err
}
Expand All @@ -300,7 +300,7 @@ $ %s tx staking unbond %s1gghjut3ccd8ay0zduzj64hwre2fxs9ldmqhffj 100stake --from

func NewBuildCreateValidatorMsg(clientCtx client.Context, txf tx.Factory, fs *flag.FlagSet) (tx.Factory, sdk.Msg, error) {
fAmount, _ := fs.GetString(FlagAmount)
amount, err := sdk.ParseCoin(fAmount)
amount, err := sdk.ParseCoinNormalized(fAmount)
if err != nil {
return txf, nil, err
}
Expand Down Expand Up @@ -514,7 +514,7 @@ func PrepareConfigForTxCreateValidator(flagSet *flag.FlagSet, moniker, nodeID, c
// BuildCreateValidatorMsg makes a new MsgCreateValidator.
func BuildCreateValidatorMsg(clientCtx client.Context, config TxCreateValidatorConfig, txBldr tx.Factory, generateOnly bool) (tx.Factory, sdk.Msg, error) {
amounstStr := config.Amount
amount, err := sdk.ParseCoin(amounstStr)
amount, err := sdk.ParseCoinNormalized(amounstStr)

if err != nil {
return txBldr, nil, err
Expand Down
2 changes: 1 addition & 1 deletion x/staking/client/rest/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (s *IntegrationTestSuite) SetupSuite() {
_, err := s.network.WaitForHeight(1)
s.Require().NoError(err)

unbond, err := sdk.ParseCoin("10stake")
unbond, err := sdk.ParseCoinNormalized("10stake")
s.Require().NoError(err)

val := s.network.Validators[0]
Expand Down