cli: refactor flag reading (#6884)

* cli: refactor ReadPersistentCommandFlags

* updates

* fix tetts

* cover all cases

* fix tests

* fix tests

* fix tests

* fix tests

* godoc++

Co-authored-by: Aaron Craelius <aaron@regen.network>

client/cmd.go

@@ -77,20 +77,27 @@ func ValidateCmd(cmd *cobra.Command, args []string) error {
 }
 
 // ReadPersistentCommandFlags returns a Context with fields set for "persistent"
-// flags that do not necessarily change with context. These must be checked if
+// or common flags that do not necessarily change with context.
-// the caller explicitly changed the values.
+//
+// Note, the provided clientCtx may have field pre-populated. The following order
+// of precedence occurs:
+//
+// - client.Context field not pre-populated & flag not set: uses default flag value
+// - client.Context field not pre-populated & flag set: uses set flag value
+// - client.Context field pre-populated & flag not set: uses pre-populated value
+// - client.Context field pre-populated & flag set: uses set flag value
 func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, error) {
-	if flagSet.Changed(cli.OutputFlag) {
+	if clientCtx.OutputFormat == "" || flagSet.Changed(cli.OutputFlag) {
 		output, _ := flagSet.GetString(cli.OutputFlag)
 		clientCtx = clientCtx.WithOutputFormat(output)
 	}
 
-	if flagSet.Changed(flags.FlagHome) {
+	if clientCtx.HomeDir == "" || flagSet.Changed(flags.FlagHome) {
 		homeDir, _ := flagSet.GetString(flags.FlagHome)
 		clientCtx = clientCtx.WithHomeDir(homeDir)
 	}
 
-	if flagSet.Changed(flags.FlagChainID) {
+	if clientCtx.ChainID == "" || flagSet.Changed(flags.FlagChainID) {
 		chainID, _ := flagSet.GetString(flags.FlagChainID)
 		clientCtx = clientCtx.WithChainID(chainID)
 	}
@@ -119,60 +126,84 @@ func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Cont
 }
 
 // ReadQueryCommandFlags returns an updated Context with fields set based on flags
-// defined in GetCommands. An error is returned if any flag query fails.
+// defined in AddQueryFlagsToCmd. An error is returned if any flag query fails.
 //
-// Certain flags are naturally command and context dependent, so for these flags
+// Note, the provided clientCtx may have field pre-populated. The following order
-// we do not check if they've been explicitly set by the caller. Other flags can
+// of precedence occurs:
-// be considered "persistent" (e.g. KeyBase or Client) and these should be checked
+//
-// if the caller explicitly set those.
+// - client.Context field not pre-populated & flag not set: uses default flag value
+// - client.Context field not pre-populated & flag set: uses set flag value
+// - client.Context field pre-populated & flag not set: uses pre-populated value
+// - client.Context field pre-populated & flag set: uses set flag value
 func ReadQueryCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, error) {
-	height, _ := flagSet.GetInt64(flags.FlagHeight)
+	if clientCtx.Height == 0 || flagSet.Changed(flags.FlagHeight) {
-	clientCtx = clientCtx.WithHeight(height)
+		height, _ := flagSet.GetInt64(flags.FlagHeight)
+		clientCtx = clientCtx.WithHeight(height)
+	}
 
-	useLedger, _ := flagSet.GetBool(flags.FlagUseLedger)
+	if !clientCtx.UseLedger || flagSet.Changed(flags.FlagUseLedger) {
-	clientCtx = clientCtx.WithUseLedger(useLedger)
+		useLedger, _ := flagSet.GetBool(flags.FlagUseLedger)
+		clientCtx = clientCtx.WithUseLedger(useLedger)
+	}
 
 	return ReadPersistentCommandFlags(clientCtx, flagSet)
 }
 
 // ReadTxCommandFlags returns an updated Context with fields set based on flags
-// defined in PostCommands. An error is returned if any flag query fails.
+// defined in AddTxFlagsToCmd. An error is returned if any flag query fails.
 //
-// Certain flags are naturally command and context dependent, so for these flags
+// Note, the provided clientCtx may have field pre-populated. The following order
-// we do not check if they've been explicitly set by the caller. Other flags can
+// of precedence occurs:
-// be considered "persistent" (e.g. KeyBase or Client) and these should be checked
+//
-// if the caller explicitly set those.
+// - client.Context field not pre-populated & flag not set: uses default flag value
+// - client.Context field not pre-populated & flag set: uses set flag value
+// - client.Context field pre-populated & flag not set: uses pre-populated value
+// - client.Context field pre-populated & flag set: uses set flag value
 func ReadTxCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, error) {
 	clientCtx, err := ReadPersistentCommandFlags(clientCtx, flagSet)
 	if err != nil {
 		return clientCtx, err
 	}
 
-	genOnly, _ := flagSet.GetBool(flags.FlagGenerateOnly)
+	if !clientCtx.GenerateOnly || flagSet.Changed(flags.FlagGenerateOnly) {
-	clientCtx = clientCtx.WithGenerateOnly(genOnly)
+		genOnly, _ := flagSet.GetBool(flags.FlagGenerateOnly)
-
+		clientCtx = clientCtx.WithGenerateOnly(genOnly)
-	dryRun, _ := flagSet.GetBool(flags.FlagDryRun)
-	clientCtx = clientCtx.WithSimulation(dryRun)
-
-	offline, _ := flagSet.GetBool(flags.FlagOffline)
-	clientCtx = clientCtx.WithOffline(offline)
-
-	useLedger, _ := flagSet.GetBool(flags.FlagUseLedger)
-	clientCtx = clientCtx.WithUseLedger(useLedger)
-
-	bMode, _ := flagSet.GetString(flags.FlagBroadcastMode)
-	clientCtx = clientCtx.WithBroadcastMode(bMode)
-
-	skipConfirm, _ := flagSet.GetBool(flags.FlagSkipConfirmation)
-	clientCtx = clientCtx.WithSkipConfirmation(skipConfirm)
-
-	from, _ := flagSet.GetString(flags.FlagFrom)
-	fromAddr, fromName, err := GetFromFields(clientCtx.Keyring, from, clientCtx.GenerateOnly)
-	if err != nil {
-		return clientCtx, err
 	}
 
-	clientCtx = clientCtx.WithFrom(from).WithFromAddress(fromAddr).WithFromName(fromName)
+	if !clientCtx.Simulate || flagSet.Changed(flags.FlagDryRun) {
+		dryRun, _ := flagSet.GetBool(flags.FlagDryRun)
+		clientCtx = clientCtx.WithSimulation(dryRun)
+	}
+
+	if !clientCtx.Offline || flagSet.Changed(flags.FlagOffline) {
+		offline, _ := flagSet.GetBool(flags.FlagOffline)
+		clientCtx = clientCtx.WithOffline(offline)
+	}
+
+	if !clientCtx.UseLedger || flagSet.Changed(flags.FlagUseLedger) {
+		useLedger, _ := flagSet.GetBool(flags.FlagUseLedger)
+		clientCtx = clientCtx.WithUseLedger(useLedger)
+	}
+
+	if clientCtx.BroadcastMode == "" || flagSet.Changed(flags.FlagBroadcastMode) {
+		bMode, _ := flagSet.GetString(flags.FlagBroadcastMode)
+		clientCtx = clientCtx.WithBroadcastMode(bMode)
+	}
+
+	if !clientCtx.SkipConfirm || flagSet.Changed(flags.FlagSkipConfirmation) {
+		skipConfirm, _ := flagSet.GetBool(flags.FlagSkipConfirmation)
+		clientCtx = clientCtx.WithSkipConfirmation(skipConfirm)
+	}
+
+	if clientCtx.From == "" || flagSet.Changed(flags.FlagFrom) {
+		from, _ := flagSet.GetString(flags.FlagFrom)
+		fromAddr, fromName, err := GetFromFields(clientCtx.Keyring, from, clientCtx.GenerateOnly)
+		if err != nil {
+			return clientCtx, err
+		}
+
+		clientCtx = clientCtx.WithFrom(from).WithFromAddress(fromAddr).WithFromName(fromName)
+	}
 
 	return clientCtx, nil
 }

x/auth/client/cli/cli_test.go

@@ -211,7 +211,7 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	s.Require().Equal(len(txBuilder.GetTx().GetMsgs()), 1)
 	s.Require().Equal(0, len(txBuilder.GetTx().GetSignatures()))
 
-	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, val1.Address)
+	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), val1.Address)
 	s.Require().NoError(err)
 
 	var coins sdk.Coins
@@ -281,7 +281,7 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	s.Require().True(strings.Contains(res.String(), "[OK]"))
 
 	// Ensure foo has right amount of funds
-	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx, val1.Address)
+	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), val1.Address)
 	s.Require().NoError(err)
 
 	err = val1.ClientCtx.JSONMarshaler.UnmarshalJSON(resp.Bytes(), &coins)
@@ -304,7 +304,7 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	s.Require().NoError(s.network.WaitForNextBlock())
 
 	// Ensure destiny account state
-	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx, account.GetAddress())
+	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), account.GetAddress())
 	s.Require().NoError(err)
 
 	err = val1.ClientCtx.JSONMarshaler.UnmarshalJSON(resp.Bytes(), &coins)
@@ -312,7 +312,7 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	s.Require().Equal(sendTokens.Amount, coins.AmountOf(s.cfg.BondDenom))
 
 	// Ensure origin account state
-	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx, val1.Address)
+	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), val1.Address)
 	s.Require().NoError(err)
 
 	err = val1.ClientCtx.JSONMarshaler.UnmarshalJSON(resp.Bytes(), &coins)
@@ -448,7 +448,7 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() {
 	multisigInfo, err := val1.ClientCtx.Keyring.Key("multi")
 	s.Require().NoError(err)
 
-	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, multisigInfo.GetAddress())
+	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), multisigInfo.GetAddress())
 	s.Require().NoError(err)
 
 	var coins sdk.Coins
@@ -472,7 +472,7 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() {
 
 	s.Require().NoError(s.network.WaitForNextBlock())
 
-	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx, multisigInfo.GetAddress())
+	resp, err = bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), multisigInfo.GetAddress())
 	s.Require().NoError(err)
 
 	err = val1.ClientCtx.JSONMarshaler.UnmarshalJSON(resp.Bytes(), &coins)
@@ -564,7 +564,7 @@ func (s *IntegrationTestSuite) TestCLIMultisign() {
 
 	s.Require().NoError(s.network.WaitForNextBlock())
 
-	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, multisigInfo.GetAddress())
+	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx.WithOutputFormat("json"), multisigInfo.GetAddress())
 	s.Require().NoError(err)
 
 	var coins sdk.Coins
@@ -648,6 +648,7 @@ func TestGetBroadcastCommand_WithoutOfflineFlag(t *testing.T) {
 	ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx)
 
 	cmd := authcli.GetBroadcastCommand()
+	_, out := testutil.ApplyMockIO(cmd)
 
 	testDir, cleanFunc := testutil.NewTestCaseDir(t)
 	t.Cleanup(cleanFunc)
@@ -667,10 +668,8 @@ func TestGetBroadcastCommand_WithoutOfflineFlag(t *testing.T) {
 	require.NoError(t, err)
 
 	cmd.SetArgs([]string{txFileName})
-	err = cmd.ExecuteContext(ctx)
+	require.Error(t, cmd.ExecuteContext(ctx))
-
+	require.Contains(t, out.String(), "unsupported return type ; supported types: sync, async, block")
-	// We test it tries to broadcast but we set unsupported tx to get the error.
-	require.EqualError(t, err, "unsupported return type ; supported types: sync, async, block")
 }
 
 func TestIntegrationTestSuite(t *testing.T) {

x/bank/client/cli/cli_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/suite"
+	tmcli "github.com/tendermint/tendermint/libs/cli"
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
@@ -57,6 +58,7 @@ func (s *IntegrationTestSuite) TestGetBalancesCmd() {
 			"total account balance",
 			[]string{
 				val.Address.String(),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 				fmt.Sprintf("--%s=1", flags.FlagHeight),
 			},
 			false,
@@ -70,6 +72,7 @@ func (s *IntegrationTestSuite) TestGetBalancesCmd() {
 			"total account balance of a specific denom",
 			[]string{
 				val.Address.String(),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 				fmt.Sprintf("--%s=%s", cli.FlagDenom, s.cfg.BondDenom),
 				fmt.Sprintf("--%s=1", flags.FlagHeight),
 			},
@@ -79,7 +82,7 @@ func (s *IntegrationTestSuite) TestGetBalancesCmd() {
 		},
 		{
 			"total account balance of a bogus denom",
-			[]string{val.Address.String(), fmt.Sprintf("--%s=foobar", cli.FlagDenom)},
+			[]string{val.Address.String(), fmt.Sprintf("--%s=foobar", cli.FlagDenom), fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			false,
 			&sdk.Coin{},
 			sdk.NewCoin("foobar", sdk.ZeroInt()),
@@ -125,7 +128,10 @@ func (s *IntegrationTestSuite) TestGetCmdQueryTotalSupply() {
 	}{
 		{
 			"total supply",
-			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight)},
+			[]string{
+				fmt.Sprintf("--%s=1", flags.FlagHeight),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
+			},
 			false,
 			&sdk.Coins{},
 			sdk.NewCoins(
@@ -138,6 +144,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryTotalSupply() {
 			[]string{
 				fmt.Sprintf("--%s=1", flags.FlagHeight),
 				fmt.Sprintf("--%s=%s", cli.FlagDenom, s.cfg.BondDenom),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			&sdk.Coin{},
@@ -148,6 +155,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryTotalSupply() {
 			[]string{
 				fmt.Sprintf("--%s=1", flags.FlagHeight),
 				fmt.Sprintf("--%s=foobar", cli.FlagDenom),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			&sdk.Coin{},

x/distribution/client/cli/cli_test.go

@@ -75,8 +75,8 @@ func (s *IntegrationTestSuite) TestGetCmdQueryParams() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
-			[]string{},
+			[]string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			`{"community_tax":"0.020000000000000000","base_proposer_reward":"0.010000000000000000","bonus_proposer_reward":"0.040000000000000000","withdraw_addr_enabled":true}`,
 		},
 		{
@@ -132,10 +132,11 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorOutstandingRewards() {
 			"",
 		},
 		{
-			"default output",
+			"json output",
 			[]string{
 				fmt.Sprintf("--%s=3", flags.FlagHeight),
 				sdk.ValAddress(val.Address).String(),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			`{"rewards":[{"denom":"stake","amount":"232.260000000000000000"}]}`,
@@ -202,10 +203,11 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorCommission() {
 			"",
 		},
 		{
-			"default output",
+			"json output",
 			[]string{
 				fmt.Sprintf("--%s=3", flags.FlagHeight),
 				sdk.ValAddress(val.Address).String(),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			`{"commission":[{"denom":"stake","amount":"116.130000000000000000"}]}`,
@@ -290,10 +292,11 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorSlashes() {
 			"",
 		},
 		{
-			"default output",
+			"json output",
 			[]string{
 				fmt.Sprintf("--%s=3", flags.FlagHeight),
 				sdk.ValAddress(val.Address).String(), "1", "3",
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			"null",
@@ -369,19 +372,21 @@ func (s *IntegrationTestSuite) TestGetCmdQueryDelegatorRewards() {
 			"",
 		},
 		{
-			"default output",
+			"json output",
 			[]string{
 				fmt.Sprintf("--%s=10", flags.FlagHeight),
 				addr.String(),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			fmt.Sprintf(`{"rewards":[{"validator_address":"%s","reward":[{"denom":"stake","amount":"387.100000000000000000"}]}],"total":[{"denom":"stake","amount":"387.100000000000000000"}]}`, valAddr.String()),
 		},
 		{
-			"default output (specific validator)",
+			"json output (specific validator)",
 			[]string{
 				fmt.Sprintf("--%s=10", flags.FlagHeight),
 				addr.String(), valAddr.String(),
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			false,
 			`[{"denom":"stake","amount":"387.100000000000000000"}]`,
@@ -454,8 +459,8 @@ func (s *IntegrationTestSuite) TestGetCmdQueryCommunityPool() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
-			[]string{fmt.Sprintf("--%s=3", flags.FlagHeight)},
+			[]string{fmt.Sprintf("--%s=3", flags.FlagHeight), fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			`[{"denom":"stake","amount":"4.740000000000000000"}]`,
 		},
 		{

x/mint/client/cli/cli_test.go

@@ -66,8 +66,8 @@ func (s *IntegrationTestSuite) TestGetCmdQueryParams() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
-			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight)},
+			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight), fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			`{"mint_denom":"stake","inflation_rate_change":"0.130000000000000000","inflation_max":"1.000000000000000000","inflation_min":"1.000000000000000000","goal_bonded":"0.670000000000000000","blocks_per_year":"6311520"}`,
 		},
 		{
@@ -112,8 +112,8 @@ func (s *IntegrationTestSuite) TestGetCmdQueryInflation() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
-			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight)},
+			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight), fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			`"1.000000000000000000"`,
 		},
 		{
@@ -153,8 +153,8 @@ func (s *IntegrationTestSuite) TestGetCmdQueryAnnualProvisions() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
-			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight)},
+			[]string{fmt.Sprintf("--%s=1", flags.FlagHeight), fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			`"500000000.000000000000000000"`,
 		},
 		{

x/params/client/cli/cli_test.go

@@ -49,9 +49,10 @@ func (s *IntegrationTestSuite) TestNewQuerySubspaceParamsCmd() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
 			[]string{
 				"staking", "MaxValidators",
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
 			`{"subspace":"staking","key":"MaxValidators","value":"100"}`,
 		},

x/slashing/client/cli/cli_test.go

@@ -60,9 +60,10 @@ func (s *IntegrationTestSuite) TestGetCmdQuerySigningInfo() {
 	}{
 		{"invalid address", []string{"foo"}, true, ``},
 		{
-			"valid address (default output)",
+			"valid address (json output)",
 			[]string{
 				valConsPubKey,
+				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 				fmt.Sprintf("--%s=1", flags.FlagHeight),
 			},
 			false,
@@ -116,8 +117,8 @@ func (s *IntegrationTestSuite) TestGetCmdQueryParams() {
 		expectedOutput string
 	}{
 		{
-			"default output",
+			"json output",
-			[]string{},
+			[]string{fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			`{"signed_blocks_window":"100","min_signed_per_window":"0.500000000000000000","downtime_jail_duration":"600000000000","slash_fraction_double_sign":"0.050000000000000000","slash_fraction_downtime":"0.010000000000000000"}`,
 		},
 		{