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

refactor(client/v2)!: remove client.Context #22493

Merged
merged 62 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
962ed4b
add: printer
JulianToledano Nov 5, 2024
528af7c
add: config
JulianToledano Nov 5, 2024
73d8d85
update: keyring
JulianToledano Nov 5, 2024
3ad45d6
Merge branch 'main' into julian/client-v2-context
JulianToledano Nov 5, 2024
004d071
add: ConfigOptions to AppOptions
JulianToledano Nov 6, 2024
a404604
add: default keyring
JulianToledano Nov 6, 2024
30f57d3
merge main
JulianToledano Nov 7, 2024
9509d1d
del: default keyring
JulianToledano Nov 7, 2024
06ffcbe
add: grpc query conn
JulianToledano Nov 7, 2024
7e0583f
add: default grpc qConn
JulianToledano Nov 8, 2024
76556fc
update: address flag + tests
JulianToledano Nov 12, 2024
e2af16d
merge main
JulianToledano Nov 12, 2024
3b2c8d5
del: client.Context from offchain
JulianToledano Nov 13, 2024
f67063c
update: offchain
JulianToledano Nov 13, 2024
7482bef
add: generateOrBroadcastTxWithV2
JulianToledano Nov 13, 2024
f3320d6
godoc
JulianToledano Nov 13, 2024
5198686
happy lint
JulianToledano Nov 13, 2024
1a8f7b5
undo config
JulianToledano Nov 13, 2024
68b5fed
scripts/go-mod-tidy-all.sh
JulianToledano Nov 13, 2024
fe28cec
format config
JulianToledano Nov 13, 2024
4d47bc4
fix: keyring just if needed
JulianToledano Nov 13, 2024
879db32
lint
JulianToledano Nov 13, 2024
ed9d006
fix: tests
JulianToledano Nov 14, 2024
045e717
Merge branch 'main' into julian/client-v2-context
JulianToledano Nov 14, 2024
c6a699c
udpate: use context
JulianToledano Nov 18, 2024
488c781
updates
JulianToledano Nov 18, 2024
4401ad8
fix lint
JulianToledano Nov 18, 2024
f81c59c
update: toml config
JulianToledano Nov 18, 2024
4727ae4
rabbit suggestions
JulianToledano Nov 18, 2024
958c106
update: offchain
JulianToledano Nov 18, 2024
54254d4
lint
JulianToledano Nov 19, 2024
79adc32
tidy-all
JulianToledano Nov 19, 2024
8c25235
rabbit suggestions
JulianToledano Nov 19, 2024
ae69fec
mint_test system test tag
JulianToledano Nov 19, 2024
dc85d30
Merge branch 'main' into julian/client-v2-context
JulianToledano Nov 19, 2024
2f4029a
remove ir from comet
JulianToledano Nov 20, 2024
6722779
updates
JulianToledano Nov 20, 2024
f9b7320
lint
JulianToledano Nov 20, 2024
6e4a04b
rabbit suggestions
JulianToledano Nov 20, 2024
2be9f51
merge main
JulianToledano Nov 20, 2024
d0938d3
go-mod-tidy-all
JulianToledano Nov 20, 2024
e0abf39
fix
JulianToledano Nov 20, 2024
dab815d
fix
JulianToledano Nov 20, 2024
2ff6fab
del: printer from context
JulianToledano Nov 22, 2024
7421b85
merge main
JulianToledano Nov 22, 2024
2949a28
rabbit
JulianToledano Nov 22, 2024
11ebebf
update: flags, remove confirmation
JulianToledano Nov 22, 2024
9c7eb26
update: generate broadcast
JulianToledano Nov 25, 2024
e63aa2b
Merge branch 'main' into julian/client-v2-context
JulianToledano Nov 25, 2024
ff44a71
update: move context
JulianToledano Nov 26, 2024
e289671
update: generateAndBroadcast
JulianToledano Nov 26, 2024
6cafaa8
update: flag import
JulianToledano Nov 26, 2024
6e1f112
fix: don't use v2 still
JulianToledano Nov 26, 2024
a509f03
update: use cmd as input output
JulianToledano Nov 27, 2024
d8909ed
fix: populate query height
JulianToledano Nov 27, 2024
ae00cea
lint
JulianToledano Nov 27, 2024
d4b36f9
lint :)
JulianToledano Nov 27, 2024
d63f468
typo
JulianToledano Nov 29, 2024
84891ce
merge main
JulianToledano Nov 29, 2024
f67151d
changelog
JulianToledano Nov 29, 2024
e6e1759
Merge branch 'main' into julian/client-v2-context
JulianToledano Dec 3, 2024
e2e2a03
go mod tidy
JulianToledano Dec 3, 2024
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
35 changes: 22 additions & 13 deletions client/v2/autocli/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,21 @@ package autocli
import (
"github.com/cosmos/gogoproto/proto"
"github.com/spf13/cobra"
"google.golang.org/grpc"
"google.golang.org/protobuf/reflect/protoregistry"

autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
apitxsigning "cosmossdk.io/api/cosmos/tx/signing/v1beta1"
"cosmossdk.io/client/v2/autocli/flag"
"cosmossdk.io/core/address"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/depinject"
"cosmossdk.io/log"
"cosmossdk.io/x/tx/signing"

"github.com/cosmos/cosmos-sdk/client"
sdkflags "github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/types"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
)

// AppOptions are input options for an autocli enabled app. These options can be built via depinject based on an app config.
Expand All @@ -38,8 +40,15 @@ type AppOptions struct {
// module or need to be improved.
ModuleOptions map[string]*autocliv1.ModuleOptions `optional:"true"`

// ClientCtx contains the necessary information needed to execute the commands.
ClientCtx client.Context
AddressCodec address.Codec // AddressCodec is used to encode/decode account addresses.
ValidatorAddressCodec address.ValidatorAddressCodec // ValidatorAddressCodec is used to encode/decode validator addresses.
ConsensusAddressCodec address.ConsensusAddressCodec // ConsensusAddressCodec is used to encode/decode consensus addresses.

// Cdc is the codec used for binary encoding/decoding of messages.
Cdc codec.Codec

// TxConfigOpts contains options for configuring transaction handling.
TxConfigOpts authtx.ConfigOptions

skipValidation bool
}
Expand All @@ -63,19 +72,19 @@ func (appOptions AppOptions) EnhanceRootCommand(rootCmd *cobra.Command) error {
builder := &Builder{
Builder: flag.Builder{
TypeResolver: protoregistry.GlobalTypes,
FileResolver: appOptions.ClientCtx.InterfaceRegistry,
AddressCodec: appOptions.ClientCtx.AddressCodec,
ValidatorAddressCodec: appOptions.ClientCtx.ValidatorAddressCodec,
ConsensusAddressCodec: appOptions.ClientCtx.ConsensusAddressCodec,
},
GetClientConn: func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
return client.GetClientQueryContext(cmd)
FileResolver: appOptions.Cdc.InterfaceRegistry(),
AddressCodec: appOptions.AddressCodec,
ValidatorAddressCodec: appOptions.ValidatorAddressCodec,
ConsensusAddressCodec: appOptions.ConsensusAddressCodec,
},
GetClientConn: getQueryClientConn(appOptions.Cdc, appOptions.Cdc.InterfaceRegistry()),
AddQueryConnFlags: func(c *cobra.Command) {
sdkflags.AddQueryFlagsToCmd(c)
sdkflags.AddKeyringFlags(c.Flags())
},
AddTxConnFlags: sdkflags.AddTxFlagsToCmd,
AddTxConnFlags: sdkflags.AddTxFlagsToCmd,
Cdc: appOptions.Cdc,
EnabledSignModes: []apitxsigning.SignMode{apitxsigning.SignMode_SIGN_MODE_DIRECT},
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
}

return appOptions.EnhanceRootCommandWithBuilder(rootCmd, builder)
Expand Down Expand Up @@ -170,9 +179,9 @@ func NewAppOptionsFromConfig(

return AppOptions{
Modules: cfg.Modules,
ClientCtx: client.Context{InterfaceRegistry: interfaceRegistry},
ModuleOptions: moduleOptions,
skipValidation: true,
Cdc: codec.NewProtoCodec(interfaceRegistry),
}, nil
}

Expand Down
6 changes: 6 additions & 0 deletions client/v2/autocli/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import (
"github.com/spf13/cobra"
"google.golang.org/grpc"

apisigning "cosmossdk.io/api/cosmos/tx/signing/v1beta1"
"cosmossdk.io/client/v2/autocli/flag"

"github.com/cosmos/cosmos-sdk/codec"
)

// Builder manages options for building CLI commands.
Expand All @@ -19,6 +22,9 @@ type Builder struct {
// AddQueryConnFlags and AddTxConnFlags are functions that add flags to query and transaction commands
AddQueryConnFlags func(*cobra.Command)
AddTxConnFlags func(*cobra.Command)

Cdc codec.Codec
EnabledSignModes []apisigning.SignMode
}

// ValidateAndComplete the builder fields.
Expand Down
165 changes: 144 additions & 21 deletions client/v2/autocli/common.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,28 @@
package autocli

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

"github.com/spf13/cobra"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
grpcinsecure "google.golang.org/grpc/credentials/insecure"
"google.golang.org/protobuf/reflect/protoreflect"
"sigs.k8s.io/yaml"

autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
"cosmossdk.io/client/v2/autocli/config"
clientcontext "cosmossdk.io/client/v2/autocli/context"
"cosmossdk.io/client/v2/autocli/keyring"
"cosmossdk.io/client/v2/autocli/print"
"cosmossdk.io/client/v2/broadcast/comet"
"cosmossdk.io/client/v2/internal/flags"
"cosmossdk.io/client/v2/internal/util"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/types"
)

type cmdType int
Expand Down Expand Up @@ -62,8 +72,13 @@ func (b *Builder) buildMethodCommandCommon(descriptor protoreflect.MethodDescrip
}
cmd.Args = binder.CobraArgs

cmd.PreRunE = b.preRunE()

cmd.RunE = func(cmd *cobra.Command, args []string) error {
ctx = cmd.Context()
ctx, err = b.getContext(cmd)
if err != nil {
return err
}
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved

input, err := binder.BuildMessage(args)
if err != nil {
Expand Down Expand Up @@ -228,27 +243,135 @@ func enhanceCustomCmd(builder *Builder, cmd *cobra.Command, cmdType cmdType, mod

// outOrStdoutFormat formats the output based on the output flag and writes it to the command's output stream.
func (b *Builder) outOrStdoutFormat(cmd *cobra.Command, out []byte) error {
clientCtx := client.Context{}
if v := cmd.Context().Value(client.ClientContextKey); v != nil {
clientCtx = *(v.(*client.Context))
}
flagSet := cmd.Flags()
if clientCtx.OutputFormat == "" || flagSet.Changed(flags.FlagOutput) {
output, _ := flagSet.GetString(flags.FlagOutput)
clientCtx = clientCtx.WithOutputFormat(output)
}

var err error
outputType := clientCtx.OutputFormat
// if the output type is text, convert the json to yaml
// if output type is json or nil, default to json
if outputType == flags.OutputFormatText {
out, err = yaml.JSONToYAML(out)
p, err := print.NewPrinter(cmd)
if err != nil {
return err
}
return p.PrintBytes(out)
}

// getContext creates and returns a new context.Context with a clientcontext.Context value.
// It initializes a printer and, if necessary, a keyring based on command flags.
func (b *Builder) getContext(cmd *cobra.Command) (context.Context, error) {
printer, err := print.NewPrinter(cmd)
if err != nil {
return nil, err
}

// if the command uses the keyring this must be set
var k keyring.Keyring
if cmd.Flags().Lookup("keyring-dir") != nil && cmd.Flags().Lookup("keyring-backend") != nil {
k, err = keyring.NewKeyringFromFlags(cmd.Flags(), b.AddressCodec, cmd.InOrStdin(), b.Cdc)
if err != nil {
return nil, err
}
}

clientCtx := clientcontext.Context{
Flags: cmd.Flags(),
AddressCodec: b.AddressCodec,
ValidatorAddressCodec: b.ValidatorAddressCodec,
ConsensusAddressCodec: b.ConsensusAddressCodec,
Cdc: b.Cdc,
Printer: printer,
Keyring: k,
EnabledSignmodes: b.EnabledSignModes,
}

return context.WithValue(cmd.Context(), clientcontext.ContextKey, clientCtx), nil
}

// preRunE returns a function that sets flags from the configuration before running a command.
// It is used as a PreRunE hook for cobra commands to ensure flags are properly initialized
// from the configuration before command execution.
func (b *Builder) preRunE() func(cmd *cobra.Command, args []string) error {
return func(cmd *cobra.Command, args []string) error {
err := b.setFlagsFromConfig(cmd)
if err != nil {
return err
}

return nil
}
}

func (b *Builder) setFlagsFromConfig(cmd *cobra.Command) error {
conf, err := config.CreateClientConfigFromFlags(cmd.Flags())
if err != nil {
return err
}

if cmd.Flags().Lookup("chain-id") != nil && !cmd.Flags().Changed("chain-id") {
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
_ = cmd.Flags().Set("chain-id", conf.ChainID)
}

if cmd.Flags().Lookup("keyring-backend") != nil && !cmd.Flags().Changed("keyring-backend") {
_ = cmd.Flags().Set("keyring-backend", conf.KeyringBackend)
}

if cmd.Flags().Lookup("from") != nil && !cmd.Flags().Changed("from") {
_ = cmd.Flags().Set("from", conf.KeyringDefaultKeyName)
}

if cmd.Flags().Lookup("output") != nil && !cmd.Flags().Changed("output") {
_ = cmd.Flags().Set("output", conf.Output)
}

if cmd.Flags().Lookup("node") != nil && !cmd.Flags().Changed("node") {
_ = cmd.Flags().Set("node", conf.Node)
}

if cmd.Flags().Lookup("broadcast-mode") != nil && !cmd.Flags().Changed("broadcast-mode") {
_ = cmd.Flags().Set("broadcast-mode", conf.BroadcastMode)
}

if cmd.Flags().Lookup("grpc-addr") != nil && !cmd.Flags().Changed("grpc-addr") {
_ = cmd.Flags().Set("grpc-addr", conf.GRPC.Address)
}

if cmd.Flags().Lookup("grpc-insecure") != nil && !cmd.Flags().Changed("grpc-insecure") {
_ = cmd.Flags().Set("grpc-insecure", strconv.FormatBool(conf.GRPC.Insecure))
}

cmd.Println(strings.TrimSpace(string(out)))
return nil
}

JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
// getQueryClientConn returns a function that creates a gRPC client connection based on command flags.
// It handles the creation of secure or insecure connections and falls back to a CometBFT broadcaster
// if no gRPC address is specified.
func getQueryClientConn(cdc codec.Codec, ir types.InterfaceRegistry) func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
var err error
creds := grpcinsecure.NewCredentials()

insecure := true
if cmd.Flags().Lookup("grpc-insecure") != nil {
insecure, err = cmd.Flags().GetBool("grpc-insecure")
if err != nil {
return nil, err
}
}
if !insecure {
creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})
}
Comment on lines +353 to +354
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

⚠️ Potential issue

Set explicit server name in TLS configuration

When creating TLS credentials with credentials.NewTLS, the ServerName field in tls.Config is not set, which might cause hostname verification to be skipped. This can reduce the security of the TLS connection.

Modify the TLS configuration to include the server name derived from the address:

 if !insecure {
-    creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})
+    tlsConfig := &tls.Config{
+        MinVersion: tls.VersionTLS12,
+        ServerName: extractHostname(addr),
+    }
+    creds = credentials.NewTLS(tlsConfig)
 }

Implement a helper function extractHostname to parse and return the hostname from the addr variable.

Committable suggestion skipped: line range outside the PR's diff.


var addr string
if cmd.Flags().Lookup("grpc-addr") != nil {
addr, err = cmd.Flags().GetString("grpc-addr")
if err != nil {
return nil, err
}
}
if addr == "" {
// if grpc-addr has not been set, use the default clientConn
// TODO: default is comet
node, err := cmd.Flags().GetString("node")
if err != nil {
return nil, err
}
return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc, ir)
}

return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...)
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
}
facundomedica marked this conversation as resolved.
Show resolved Hide resolved
}
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

⚠️ Potential issue

Defaulting to insecure gRPC connections may pose security risks

In the getQueryClientConn function, the insecure variable is initialized to true, which could default to establishing insecure gRPC connections if the grpc-insecure flag is not explicitly set by the user. This may inadvertently expose the connection to man-in-the-middle attacks or other vulnerabilities.

Recommend changing the default value of insecure to false to prioritize secure connections by default. Apply this diff to implement the change:

 func getQueryClientConn(cdc codec.Codec, ir types.InterfaceRegistry) func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
     return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
         var err error
-        creds := grpcinsecure.NewCredentials()

-        insecure := true
+        insecure := false
+        creds := credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})

         if cmd.Flags().Lookup("grpc-insecure") != nil {
             insecure, err = cmd.Flags().GetBool("grpc-insecure")
             if err != nil {
                 return nil, err
             }
         }
         if insecure {
             creds = grpcinsecure.NewCredentials()
         }

This ensures that the application defaults to secure gRPC connections, enhancing security. Users can still opt-in to insecure connections by explicitly setting the --grpc-insecure flag.

📝 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
// getQueryClientConn returns a function that creates a gRPC client connection based on command flags.
// It handles the creation of secure or insecure connections and falls back to a CometBFT broadcaster
// if no gRPC address is specified.
func getQueryClientConn(cdc codec.Codec, ir types.InterfaceRegistry) func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
var err error
creds := grpcinsecure.NewCredentials()
insecure := true
if cmd.Flags().Lookup("grpc-insecure") != nil {
insecure, err = cmd.Flags().GetBool("grpc-insecure")
if err != nil {
return nil, err
}
}
if !insecure {
creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})
}
var addr string
if cmd.Flags().Lookup("grpc-addr") != nil {
addr, err = cmd.Flags().GetString("grpc-addr")
if err != nil {
return nil, err
}
}
if addr == "" {
// if grpc-addr has not been set, use the default clientConn
// TODO: default is comet
node, err := cmd.Flags().GetString("node")
if err != nil {
return nil, err
}
return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc, ir)
}
return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...)
}
}
// getQueryClientConn returns a function that creates a gRPC client connection based on command flags.
// It handles the creation of secure or insecure connections and falls back to a CometBFT broadcaster
// if no gRPC address is specified.
func getQueryClientConn(cdc codec.Codec, ir types.InterfaceRegistry) func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
var err error
insecure := false
creds := credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})
if cmd.Flags().Lookup("grpc-insecure") != nil {
insecure, err = cmd.Flags().GetBool("grpc-insecure")
if err != nil {
return nil, err
}
}
if insecure {
creds = grpcinsecure.NewCredentials()
}
var addr string
if cmd.Flags().Lookup("grpc-addr") != nil {
addr, err = cmd.Flags().GetString("grpc-addr")
if err != nil {
return nil, err
}
}
if addr == "" {
// if grpc-addr has not been set, use the default clientConn
// TODO: default is comet
node, err := cmd.Flags().GetString("node")
if err != nil {
return nil, err
}
return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc, ir)
}
return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...)
}
}

17 changes: 16 additions & 1 deletion client/v2/autocli/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ type fixture struct {
conn *testClientConn
b *Builder
clientCtx client.Context

home string
chainID string
kBackend string
}

func initFixture(t *testing.T) *fixture {
Expand Down Expand Up @@ -85,17 +89,28 @@ func initFixture(t *testing.T) *fixture {
return conn, nil
},
AddQueryConnFlags: flags.AddQueryFlagsToCmd,
AddTxConnFlags: flags.AddTxFlagsToCmd,
AddTxConnFlags: addTxAndGlobalFlagsToCmd,
Cdc: encodingConfig.Codec,
}
assert.NilError(t, b.ValidateAndComplete())

return &fixture{
conn: conn,
b: b,
clientCtx: clientCtx,

home: home,
chainID: "autocli-test",
kBackend: sdkkeyring.BackendMemory,
}
}

func addTxAndGlobalFlagsToCmd(cmd *cobra.Command) {
f := cmd.Flags()
f.String("home", "", "home directory")
flags.AddTxFlagsToCmd(cmd)
}

func runCmd(fixture *fixture, command func(moduleName string, f *fixture) (*cobra.Command, error), args ...string) (*bytes.Buffer, error) {
out := &bytes.Buffer{}
cmd, err := command("test", fixture)
Expand Down
Loading
Loading