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

feat: add --grpc client option #14051

Merged
merged 6 commits into from
Dec 7, 2022
Merged

feat: add --grpc client option #14051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8ba54e
feat: add --grpc client option
aaronc Nov 28, 2022
3c1db4d
updates
aaronc Nov 28, 2022
2263135
default to TLS credentials
aaronc Nov 29, 2022
42f5ca2
add min version because of gosec lint
aaronc Nov 29, 2022
41d9385
Merge branch 'main' into aaronc/grpc-client-flag
tac0turtle Dec 7, 2022
98c8002
Merge branch 'main' into aaronc/grpc-client-flag
tac0turtle Dec 7, 2022
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
26 changes: 26 additions & 0 deletions client/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
package client

import (
"crypto/tls"
"fmt"
"strings"

"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/tendermint/tendermint/libs/cli"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"

"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
Expand Down Expand Up @@ -147,6 +151,28 @@ func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Cont
}
}

if clientCtx.GRPCClient == nil || flagSet.Changed(flags.FlagGRPC) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please invert this condition and return early if .GRPCClient != nil or if !flagSet.Changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

But we don't actually want an early return. This isn't an error condition. It follows the same pattern as the rest of the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Early returns ease readability and reduce nesting. This code incurs 3 levels of nesting that could be trivially cut out just by early returns.

Copy link
Member Author

@aaronc aaronc Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But look at the rest of the function. This is not an early return condition. I know this is the last clause in the function, but adding an early return here would just make maintenance of this function harder because it doesn't fit the general pattern in the rest of the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Left to your discretion :-)

grpcURI, _ := flagSet.GetString(flags.FlagGRPC)
if grpcURI != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also do an early return here if grpcURI == "" { return clientCtx }

var dialOpts []grpc.DialOption

useInsecure, _ := flagSet.GetBool(flags.FlagGRPCInsecure)
aaronc marked this conversation as resolved.
Show resolved Hide resolved
if useInsecure {
dialOpts = append(dialOpts, grpc.WithTransportCredentials(insecure.NewCredentials()))
} else {
dialOpts = append(dialOpts, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{
MinVersion: tls.VersionTLS12,
})))
}

grpcClient, err := grpc.Dial(grpcURI, dialOpts...)
if err != nil {
return Context{}, err
}
clientCtx = clientCtx.WithGRPCClient(grpcClient)
}
}

return clientCtx, nil
}

Expand Down
4 changes: 4 additions & 0 deletions client/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const (
FlagUseLedger = "ledger"
FlagChainID = "chain-id"
FlagNode = "node"
FlagGRPC = "grpc"
FlagGRPCInsecure = "grpc-insecure"
FlagHeight = "height"
FlagGasAdjustment = "gas-adjustment"
FlagFrom = "from"
Expand Down Expand Up @@ -92,6 +94,8 @@ var LineBreak = &cobra.Command{Run: func(*cobra.Command, []string) {}}
// AddQueryFlagsToCmd adds common flags to a module query command.
func AddQueryFlagsToCmd(cmd *cobra.Command) {
cmd.Flags().String(FlagNode, "tcp://localhost:26657", "<host>:<port> to Tendermint RPC interface for this chain")
cmd.Flags().String(FlagGRPC, "", "the gRPC endpoint to use for this chain")
cmd.Flags().Bool(FlagGRPCInsecure, false, "allow gRPC over insecure channels, if not TLS the server must use TLS")
cmd.Flags().Int64(FlagHeight, 0, "Use a specific height to query state at (this can error if the node is pruning state)")
cmd.Flags().StringP(FlagOutput, "o", "text", "Output format (text|json)")

Expand Down