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): fix variable arguments (backport #17313) #17884

Merged
merged 1 commit into from
Sep 26, 2023
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
15 changes: 7 additions & 8 deletions client/v2/autocli/flag/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ func (b *Builder) addMessageFlags(ctx context.Context, flagSet *pflag.FlagSet, m
}

isPositional := map[string]bool{}
hasVarargs := false
hasOptional := false
n := len(commandOptions.PositionalArgs)
// positional args are also parsed using a FlagSet so that we can reuse all the same parsers
handler.positionalFlagSet = pflag.NewFlagSet("positional", pflag.ContinueOnError)
Expand All @@ -114,15 +112,15 @@ func (b *Builder) addMessageFlags(ctx context.Context, flagSet *pflag.FlagSet, m
return nil, fmt.Errorf("varargs positional argument %s must be the last argument", arg.ProtoField)
}

hasVarargs = true
handler.hasVarargs = true
}

if arg.Optional {
if i != n-1 {
return nil, fmt.Errorf("optional positional argument %s must be the last argument", arg.ProtoField)
}

hasOptional = true
handler.hasOptional = true
}

_, hasValue, err := b.addFieldFlag(
Expand All @@ -142,14 +140,15 @@ func (b *Builder) addMessageFlags(ctx context.Context, flagSet *pflag.FlagSet, m
})
}

if hasVarargs {
if handler.hasVarargs {
handler.CobraArgs = cobra.MinimumNArgs(n - 1)
handler.hasVarargs = true
} else if hasOptional {
handler.MandatoryArgUntil = n - 1
} else if handler.hasOptional {
handler.CobraArgs = cobra.RangeArgs(n-1, n)
handler.hasOptional = true
handler.MandatoryArgUntil = n - 1
} else {
handler.CobraArgs = cobra.ExactArgs(n)
handler.MandatoryArgUntil = n
}

// validate flag options
Expand Down
6 changes: 6 additions & 0 deletions client/v2/autocli/flag/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package flag

import (
"context"
"fmt"
"strings"

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

Expand Down Expand Up @@ -36,6 +38,10 @@ func (c *coinValue) String() string {
}

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

coin, err := coins.ParseCoin(stringValue)
if err != nil {
return err
Expand Down
8 changes: 4 additions & 4 deletions client/v2/autocli/flag/messager_binder.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import (

// MessageBinder binds multiple flags in a flag set to a protobuf message.
type MessageBinder struct {
CobraArgs cobra.PositionalArgs
MandatoryArgUntil int
CobraArgs cobra.PositionalArgs

positionalFlagSet *pflag.FlagSet
positionalArgs []fieldBinding
Expand Down Expand Up @@ -38,10 +39,9 @@ func (m MessageBinder) Bind(msg protoreflect.Message, positionalArgs []string) e
}

name := fmt.Sprintf("%d", i)
if i == n-1 && m.hasVarargs {
if i == m.MandatoryArgUntil && m.hasVarargs {
for _, v := range positionalArgs[i:] {
err := m.positionalFlagSet.Set(name, v)
if err != nil {
if err := m.positionalFlagSet.Set(name, v); err != nil {
return err
}
}
Expand Down
39 changes: 37 additions & 2 deletions client/v2/autocli/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,16 +191,51 @@ func TestCoin(t *testing.T) {
fixture := initFixture(t)

_, err := runCmd(fixture.conn, fixture.b, buildModuleQueryCommand,
"echo",
"1",
"abc",
"1234foo,4321bar",
"100uatom",
"--a-coin", "100000foo",
)
assert.ErrorContains(t, err, "coin flag must be a single coin, specific multiple coins with multiple flags or spaces")

_, err = runCmd(fixture.conn, fixture.b, buildModuleQueryCommand,
"echo",
"1",
"abc",
"1234foo",
"4321bar",
"100uatom",
"--a-coin", "100000foo",
"--duration", "4h3s",
"--coins", "100000bar",
"--coins", "100uatom",
)
assert.NilError(t, err)
assert.DeepEqual(t, fixture.conn.lastRequest, fixture.conn.lastResponse.(*testpb.EchoResponse).Request, protocmp.Transform())
expectedResp := &testpb.EchoResponse{
Request: &testpb.EchoRequest{
Positional1: 1,
Positional2: "abc",
Positional3Varargs: []*basev1beta1.Coin{
{Amount: "1234", Denom: "foo"},
{Amount: "4321", Denom: "bar"},
{Amount: "100", Denom: "uatom"},
},
ACoin: &basev1beta1.Coin{
Amount: "100000",
Denom: "foo",
},
Coins: []*basev1beta1.Coin{
{Amount: "100000", Denom: "bar"},
{Amount: "100", Denom: "uatom"},
},
Page: &queryv1beta1.PageRequest{},
I32: 3,
U64: 5,
},
}
assert.DeepEqual(t, fixture.conn.lastResponse.(*testpb.EchoResponse), expectedResp, protocmp.Transform())
}

func TestOptional(t *testing.T) {
Expand Down Expand Up @@ -354,7 +389,7 @@ func TestEverything(t *testing.T) {
Positional2: "abc",
Positional3Varargs: []*basev1beta1.Coin{
{Amount: "123.123123124", Denom: "foo"},
// {Amount: "4321", Denom: "bar"}, // TODO fix repeated fields
{Amount: "4321", Denom: "bar"},
},
ABool: true,
AnEnum: testpb.Enum_ENUM_ONE,
Expand Down
1 change: 1 addition & 0 deletions client/v2/autocli/testdata/help-deprecated.golden
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Flags:
--an-enum Enum (unspecified | one | two | five | neg-three) (default unspecified)
--bools bools (default [])
--bz binary
--coins cosmos.base.v1beta1.Coin (repeated)
--deprecated-field string
--duration duration
--durations duration (repeated)
Expand Down
1 change: 1 addition & 0 deletions client/v2/autocli/testdata/help-echo.golden
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Flags:
--an-enum Enum (unspecified | one | two | five | neg-three) (default unspecified)
--bools bools (default [])
--bz binary some bytes
--coins cosmos.base.v1beta1.Coin (repeated)
--deprecated-field string (DEPRECATED: don't use this)
--duration duration some random duration
--durations duration (repeated)
Expand Down
12 changes: 2 additions & 10 deletions client/v2/internal/testpb/msg_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions client/v2/internal/testpb/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ message EchoRequest {
map<string, cosmos.base.v1beta1.Coin> map_string_coin = 35;
string a_validator_address = 36 [(cosmos_proto.scalar) = "cosmos.ValidatorAddressString"];
string a_consensus_address = 37 [(cosmos_proto.scalar) = "cosmos.ConsensusAddressString"];

repeated cosmos.base.v1beta1.Coin coins = 38;
}

enum Enum {
Expand Down
Loading
Loading