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

fix(client/v2/autocli): add CoinDec flag #22817

Merged
merged 7 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions client/v2/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

* [#21853](https://github.com/cosmos/cosmos-sdk/pull/21853) Fix `*big.Int` unmarshalling in txs.
* [#22576](https://github.com/cosmos/cosmos-sdk/pull/22576) Fix duplicate command addition in `autocli` when custom enhanced command has a different name than module name
* [#22817](https://github.com/cosmos/cosmos-sdk/pull/22817) Add DecCoin support in autocli flag builder.
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Move changelog entry to Features section

The addition of DecCoin support in autocli flag builder is a new feature rather than a bug fix. Consider moving this entry to the "Features" section:

 ### Features
+* [#22817](https://github.com/cosmos/cosmos-sdk/pull/22817) Add DecCoin support in autocli flag builder.

 ### Bug Fixes
-* [#22817](https://github.com/cosmos/cosmos-sdk/pull/22817) Add DecCoin support in autocli flag builder.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [#22817](https://github.com/cosmos/cosmos-sdk/pull/22817) Add DecCoin support in autocli flag builder.
### Features
* [#22817](https://github.com/cosmos/cosmos-sdk/pull/22817) Add DecCoin support in autocli flag builder.
### Bug Fixes


## [v2.0.0-beta.5] - 2024-09-18

Expand Down
1 change: 1 addition & 0 deletions client/v2/autocli/flag/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func (b *Builder) init() {
b.messageFlagTypes["google.protobuf.Timestamp"] = timestampType{}
b.messageFlagTypes["google.protobuf.Duration"] = durationType{}
b.messageFlagTypes["cosmos.base.v1beta1.Coin"] = coinType{}
b.messageFlagTypes["cosmos.base.v1beta1.DecCoin"] = decCoinType{}
}

if b.scalarFlagTypes == nil {
Expand Down
54 changes: 54 additions & 0 deletions client/v2/autocli/flag/dec_coin.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package flag

import (
"context"
"errors"
"strings"

"google.golang.org/protobuf/reflect/protoreflect"

basev1beta1 "cosmossdk.io/api/cosmos/base/v1beta1"
"cosmossdk.io/client/v2/internal/coins"
)

type decCoinType struct{}

type decCoinValue struct {
value *basev1beta1.DecCoin
}

func (c decCoinType) NewValue(*context.Context, *Builder) Value {
return &decCoinValue{}
}

func (c decCoinType) DefaultValue() string {
return "zero"
}

func (c *decCoinValue) Get(protoreflect.Value) (protoreflect.Value, error) {
if c.value == nil {
return protoreflect.Value{}, nil
}
return protoreflect.ValueOfMessage(c.value.ProtoReflect()), nil
}
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved

func (c *decCoinValue) String() string {
return c.value.String()
}
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 10, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add nil check in String method

The current implementation could panic if value is nil. Consider adding a nil check:

 func (c *decCoinValue) String() string {
+    if c.value == nil {
+        return ""
+    }
     return c.value.String()
 }
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (c *decCoinValue) String() string {
return c.value.String()
}
func (c *decCoinValue) String() string {
if c.value == nil {
return ""
}
return c.value.String()
}

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

@julienrbrt, thank you for addressing the issue!


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


func (c *decCoinValue) Set(stringValue string) error {
if strings.Contains(stringValue, ",") {
return errors.New("coin flag must be a single coin, specific multiple coins with multiple flags or spaces")
}

coin, err := coins.ParseDecCoin(stringValue)
if err != nil {
return err
}
c.value = coin
return nil
}

func (c *decCoinValue) Type() string {
return "cosmos.base.v1beta1.DecCoin"
}
39 changes: 33 additions & 6 deletions client/v2/internal/coins/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,47 @@ var coinRegex = regexp.MustCompile(`^(\d+(\.\d+)?)([a-zA-Z][a-zA-Z0-9\/\:\._\-]{
// ParseCoin parses a coin from a string. The string must be in the format
// <amount><denom>, where <amount> is a number and <denom> is a valid denom.
func ParseCoin(input string) (*basev1beta1.Coin, error) {
amount, denom, err := parseCoin(input)
if err != nil {
return nil, err
}

return &basev1beta1.Coin{
Amount: amount,
Denom: denom,
}, nil
}

// ParseDecCoin parses a decCoin from a string. The string must be in the format
// <amount><denom>, where <amount> is a number and <denom> is a valid denom.
func ParseDecCoin(input string) (*basev1beta1.DecCoin, error) {
amount, denom, err := parseCoin(input)
if err != nil {
return nil, err
}

return &basev1beta1.DecCoin{
Amount: amount,
Denom: denom,
}, nil
}

// parseCoin parses a coin string into its amount and denom components.
// The input string must be in the format <amount><denom>.
// It returns the amount string, denom string, and any error encountered.
// Returns an error if the input is empty or doesn't match the expected format.
func parseCoin(input string) (amount, denom string, err error) {
input = strings.TrimSpace(input)

if input == "" {
return nil, errors.New("empty input when parsing coin")
return "", "", errors.New("empty input when parsing coin")
}

matches := coinRegex.FindStringSubmatch(input)

if len(matches) == 0 {
return nil, errors.New("invalid input format")
return "", "", errors.New("invalid input format")
}

return &basev1beta1.Coin{
Amount: matches[1],
Denom: matches[3],
}, nil
return matches[1], matches[3], nil
}
66 changes: 61 additions & 5 deletions client/v2/internal/coins/format_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,72 @@
package coins_test
package coins

import (
"testing"

"github.com/stretchr/testify/require"

"cosmossdk.io/client/v2/internal/coins"
)

func TestDecodeCoin(t *testing.T) {
func Test_parseCoin(t *testing.T) {
tests := []struct {
name string
input string
amount string
denom string
err string
}{
{
name: "ok",
input: "1000stake",
amount: "1000",
denom: "stake",
},
{
name: "empty",
input: "",
err: "empty input when parsing coin",
},
{
name: "empty denom",
input: "1000",
err: "invalid input format",
},
{
name: "empty amount",
input: "stake",
err: "invalid input format",
},
{
name: "<denom><amount> format",
input: "stake1000",
err: "invalid input format",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
amount, denom, err := parseCoin(tt.input)
if tt.err != "" {
require.Error(t, err)
require.Contains(t, err.Error(), tt.err)
} else {
require.NoError(t, err)
require.Equal(t, tt.amount, amount)
require.Equal(t, tt.denom, denom)
}
})
}
}

func TestParseCoin(t *testing.T) {
encodedCoin := "1000000000foo"
coin, err := ParseCoin(encodedCoin)
require.NoError(t, err)
require.Equal(t, "1000000000", coin.Amount)
require.Equal(t, "foo", coin.Denom)
}

func TestParseDecCoin(t *testing.T) {
encodedCoin := "1000000000foo"
coin, err := coins.ParseCoin(encodedCoin)
coin, err := ParseDecCoin(encodedCoin)
require.NoError(t, err)
require.Equal(t, "1000000000", coin.Amount)
require.Equal(t, "foo", coin.Denom)
Expand Down
Loading