fix(client/v2): fix variable arguments (backport #17313) (#17884)

Co-authored-by: Julien Robert <[email protected]>
cosmos · Sep 26, 2023 · 14d163a · 14d163a
1 parent ab6a1e6
commit 14d163a
Show file tree

Hide file tree

Showing 10 changed files with 983 additions and 803 deletions.
diff --git a/client/v2/autocli/flag/builder.go b/client/v2/autocli/flag/builder.go
@@ -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)
@@ -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(
@@ -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

diff --git a/client/v2/autocli/flag/coin.go b/client/v2/autocli/flag/coin.go
@@ -2,6 +2,8 @@ package flag
 
 import (
 	"context"
+	"fmt"
+	"strings"
 
 	"google.golang.org/protobuf/reflect/protoreflect"
 
@@ -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

diff --git a/client/v2/autocli/flag/messager_binder.go b/client/v2/autocli/flag/messager_binder.go
@@ -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
@@ -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
 				}
 			}

diff --git a/client/v2/autocli/query_test.go b/client/v2/autocli/query_test.go
@@ -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) {
@@ -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,

diff --git a/client/v2/autocli/testdata/help-deprecated.golden b/client/v2/autocli/testdata/help-deprecated.golden
@@ -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)                                        

diff --git a/client/v2/autocli/testdata/help-echo.golden b/client/v2/autocli/testdata/help-echo.golden
@@ -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)                                        

diff --git a/client/v2/internal/testpb/msg_grpc.pb.go b/client/v2/internal/testpb/msg_grpc.pb.go
diff --git a/client/v2/internal/testpb/query.proto b/client/v2/internal/testpb/query.proto
@@ -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 {