feat(client/v2): get keyring from context (backport #19646) (#20727)

Co-authored-by: Julien Robert <[email protected]>

Author: mergify[bot]

.github/workflows/test.yml

@@ -434,14 +434,6 @@ jobs:
         run: |
           cd simapp
           go test -mod=readonly -timeout 30m -tags='app_v1 norace ledger test_ledger_mock rocksdb_build' ./...
-      - name: sonarcloud
-        if: ${{ env.GIT_DIFF && !github.event.pull_request.draft && env.SONAR_TOKEN != null }}
-        uses: SonarSource/sonarcloud-github-action@master
-        env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
-        with:
-          projectBaseDir: simapp/
 
   test-collections:
     runs-on: ubuntu-latest

client/v2/CHANGELOG.md

@@ -36,16 +36,21 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-## [v2.0.0-beta.2] - 2024-XX-XX
+## [v2.0.0-beta.2] - 2024-06-19
+
+### Features
+
+* [#19039](https://github.com/cosmos/cosmos-sdk/pull/19039) Add support for pubkey in autocli.
 
 ### Improvements
 
+* [#19646](https://github.com/cosmos/cosmos-sdk/pull/19646) Use keyring from command context.
 * (deps) [#19810](https://github.com/cosmos/cosmos-sdk/pull/19810) Upgrade SDK version due to prometheus breaking change.
 * (deps) [#19810](https://github.com/cosmos/cosmos-sdk/pull/19810) Bump `cosmossdk.io/store` to v1.1.0.
 * [#20083](https://github.com/cosmos/cosmos-sdk/pull/20083) Integrate latest version of cosmos-proto and improve version filtering.
 * [#19618](https://github.com/cosmos/cosmos-sdk/pull/19618) Marshal enum as string in queries.
 * [#19060](https://github.com/cosmos/cosmos-sdk/pull/19060) Use client context from root (or enhanced) command in autocli commands.
-    * Note, the given command must have a `client.Context` in its context.
+  * Note, the given command must have a `client.Context` in its context.
 * [#19216](https://github.com/cosmos/cosmos-sdk/pull/19216) Do not overwrite TxConfig, use directly the one provided in context. TxConfig should always be set in the `client.Context` in `root.go` of an app.
 * [#20266](https://github.com/cosmos/cosmos-sdk/pull/20266) Add ability to override the short description in AutoCLI-generated top-level commands.
 
@@ -56,6 +61,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#19060](https://github.com/cosmos/cosmos-sdk/pull/19060) Simplify key flag parsing logic in flag handler.
 * [#20033](https://github.com/cosmos/cosmos-sdk/pull/20033) Respect output format from client ctx.
 
+### API Breaking Changes
+
+* [#19646](https://github.com/cosmos/cosmos-sdk/pull/19646) Remove keyring from `autocli.AppOptions` and `flag.Builder` options.
+
 ## [v2.0.0-beta.1] - 2023-11-07
 
 This is the first tagged version of client/v2.

client/v2/README.md

@@ -75,10 +75,10 @@ if err := rootCmd.Execute(); err != nil {
 
 ### Keyring
 
-`autocli` uses a keyring for key name resolving and signing transactions. Providing a keyring is optional, but if you want to use the `autocli` generated commands to sign transactions, you must provide a keyring.
+`autocli` uses a keyring for key name resolving names and signing transactions.
 
 :::tip
-This provides a better UX as it allows to resolve key names directly from the keyring in all transactions and commands.
+AutoCLI provides a better UX than normal CLI as it allows to resolve key names directly from the keyring in all transactions and commands.
 
 ```sh
 <appd> q bank balances alice
@@ -87,8 +87,9 @@ This provides a better UX as it allows to resolve key names directly from the ke
 
 :::
 
-The keyring to be provided to `client/v2` must match the `client/v2` keyring interface.
-The keyring should be provided in the `appOptions` struct as follows, and can be gotten from the client context:
+The keyring used for resolving names and signing transactions is provided via the `client.Context`.
+The keyring is then converted to the `client/v2/autocli/keyring` interface.
+If no keyring is provided, the `autocli` generated command will not be able to sign transactions, but will still be able to query the chain.
 
 :::tip
 The Cosmos SDK keyring and Hubl keyring both implement the `client/v2/autocli/keyring` interface, thanks to the following wrapper:
@@ -99,18 +100,6 @@ keyring.NewAutoCLIKeyring(kb)
 
 :::
 
-:::warning
-When using AutoCLI the keyring will only be created once and before any command flag parsing.
-:::
-
-```go
-// Set the keyring in the appOptions
-appOptions.Keyring = keyring
-
-err := autoCliOpts.EnhanceRootCommand(rootCmd)
-...
-```
-
 ## Signing
 
 `autocli` supports signing transactions with the keyring.

client/v2/autocli/app.go

@@ -7,7 +7,6 @@ import (
 
 	autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
 	"cosmossdk.io/client/v2/autocli/flag"
-	"cosmossdk.io/client/v2/autocli/keyring"
 	"cosmossdk.io/core/address"
 	"cosmossdk.io/core/appmodule"
 	"cosmossdk.io/depinject"
@@ -42,9 +41,6 @@ type AppOptions struct {
 	ValidatorAddressCodec runtime.ValidatorAddressCodec
 	ConsensusAddressCodec runtime.ConsensusAddressCodec
 
-	// Keyring is the keyring to use for client/v2.
-	Keyring keyring.Keyring `optional:"true"`
-
 	// ClientCtx contains the necessary information needed to execute the commands.
 	ClientCtx client.Context
 }
@@ -72,7 +68,6 @@ func (appOptions AppOptions) EnhanceRootCommand(rootCmd *cobra.Command) error {
 			AddressCodec:          appOptions.AddressCodec,
 			ValidatorAddressCodec: appOptions.ValidatorAddressCodec,
 			ConsensusAddressCodec: appOptions.ConsensusAddressCodec,
-			Keyring:               appOptions.Keyring,
 		},
 		GetClientConn: func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
 			return client.GetClientQueryContext(cmd)

client/v2/autocli/common.go

@@ -2,6 +2,7 @@ package autocli
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/spf13/cobra"
 	"google.golang.org/protobuf/reflect/protoreflect"
@@ -52,38 +53,35 @@ func (b *Builder) buildMethodCommandCommon(descriptor protoreflect.MethodDescrip
 		Version:      options.Version,
 	}
 
-	binder, err := b.AddMessageFlags(cmd.Context(), cmd.Flags(), inputType, options)
+	// we need to use a pointer to the context as the correct context is set in the RunE function
+	// however we need to set the flags before the RunE function is called
+	ctx := cmd.Context()
+	binder, err := b.AddMessageFlags(&ctx, cmd.Flags(), inputType, options)
 	if err != nil {
 		return nil, err
 	}
-
 	cmd.Args = binder.CobraArgs
 
 	cmd.RunE = func(cmd *cobra.Command, args []string) error {
+		ctx = cmd.Context()
+
 		input, err := binder.BuildMessage(args)
 		if err != nil {
 			return err
 		}
 
 		// signer related logic, triggers only when there is a signer defined
 		if binder.SignerInfo.FieldName != "" {
-			// mark the signer flag as required if defined
-			// TODO(@julienrbrt): UX improvement by only marking the flag as required when there is more than one key in the keyring;
-			// when there is only one key, use that key by default.
 			if binder.SignerInfo.IsFlag {
-				if err := cmd.MarkFlagRequired(binder.SignerInfo.FieldName); err != nil {
-					return err
-				}
-
 				// the client context uses the from flag to determine the signer.
 				// this sets the signer flags to the from flag value if a custom signer flag is set.
-				if binder.SignerInfo.FieldName != flags.FlagFrom {
-					signer, err := cmd.Flags().GetString(binder.SignerInfo.FieldName)
-					if err != nil {
-						return fmt.Errorf("failed to get signer flag: %w", err)
+				// marks the custom flag as required.
+				if binder.SignerInfo.FlagName != flags.FlagFrom {
+					if err := cmd.MarkFlagRequired(binder.SignerInfo.FlagName); err != nil {
+						return err
 					}
 
-					if err := cmd.Flags().Set(flags.FlagFrom, signer); err != nil {
+					if err := cmd.Flags().Set(flags.FlagFrom, cmd.Flag(binder.SignerInfo.FlagName).Value.String()); err != nil {
 						return err
 					}
 				}
@@ -251,6 +249,6 @@ func (b *Builder) outOrStdoutFormat(cmd *cobra.Command, out []byte) error {
 		}
 	}
 
-	_, err = fmt.Fprintln(cmd.OutOrStdout(), string(out))
-	return err
+	cmd.Println(strings.TrimSpace(string(out)))
+	return nil
 }

client/v2/autocli/common_test.go

@@ -55,9 +55,6 @@ func initFixture(t *testing.T) *fixture {
 	kr, err := sdkkeyring.New(sdk.KeyringServiceName(), sdkkeyring.BackendMemory, home, nil, encodingConfig.Codec)
 	assert.NilError(t, err)
 
-	akr, err := sdkkeyring.NewAutoCLIKeyring(kr)
-	assert.NilError(t, err)
-
 	interfaceRegistry := encodingConfig.Codec.InterfaceRegistry()
 	banktypes.RegisterInterfaces(interfaceRegistry)
 
@@ -76,7 +73,6 @@ func initFixture(t *testing.T) *fixture {
 		Builder: flag.Builder{
 			TypeResolver:          protoregistry.GlobalTypes,
 			FileResolver:          protoregistry.GlobalFiles,
-			Keyring:               akr,
 			AddressCodec:          addresscodec.NewBech32Codec("cosmos"),
 			ValidatorAddressCodec: addresscodec.NewBech32Codec("cosmosvaloper"),
 			ConsensusAddressCodec: addresscodec.NewBech32Codec("cosmosvalcons"),

client/v2/autocli/flag/address.go

@@ -9,16 +9,18 @@ import (
 	"cosmossdk.io/client/v2/autocli/keyring"
 	"cosmossdk.io/core/address"
 
+	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
+	sdkkeyring "github.com/cosmos/cosmos-sdk/crypto/keyring"
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 )
 
 type addressStringType struct{}
 
-func (a addressStringType) NewValue(_ context.Context, b *Builder) Value {
-	return &addressValue{addressCodec: b.AddressCodec, keyring: b.Keyring}
+func (a addressStringType) NewValue(ctx *context.Context, b *Builder) Value {
+	return &addressValue{addressCodec: b.AddressCodec, ctx: ctx}
 }
 
 func (a addressStringType) DefaultValue() string {
@@ -27,18 +29,19 @@ func (a addressStringType) DefaultValue() string {
 
 type validatorAddressStringType struct{}
 
-func (a validatorAddressStringType) NewValue(_ context.Context, b *Builder) Value {
-	return &addressValue{addressCodec: b.ValidatorAddressCodec, keyring: b.Keyring}
+func (a validatorAddressStringType) NewValue(ctx *context.Context, b *Builder) Value {
+	return &addressValue{addressCodec: b.ValidatorAddressCodec, ctx: ctx}
 }
 
 func (a validatorAddressStringType) DefaultValue() string {
 	return ""
 }
 
 type addressValue struct {
-	value        string
+	ctx          *context.Context
 	addressCodec address.Codec
-	keyring      keyring.Keyring
+
+	value string
 }
 
 func (a addressValue) Get(protoreflect.Value) (protoreflect.Value, error) {
@@ -51,7 +54,9 @@ func (a addressValue) String() string {
 
 // Set implements the flag.Value interface for addressValue.
 func (a *addressValue) Set(s string) error {
-	addr, err := a.keyring.LookupAddressByKeyName(s)
+	// we get the keyring on set, as in NewValue the context is the parent context (before RunE)
+	keyring := getKeyringFromCtx(a.ctx)
+	addr, err := keyring.LookupAddressByKeyName(s)
 	if err == nil {
 		addrStr, err := a.addressCodec.BytesToString(addr)
 		if err != nil {
@@ -62,9 +67,11 @@ func (a *addressValue) Set(s string) error {
 		return nil
 	}
 
-	// failed all validation, just accept the input.
-	// TODO(@julienrbrt), for final client/v2 2.0.0 revert the logic and
-	// do a better keyring instantiation.
+	_, err = a.addressCodec.StringToBytes(s)
+	if err != nil {
+		return fmt.Errorf("invalid account address or key name: %w", err)
+	}
+
 	a.value = s
 
 	return nil
@@ -76,11 +83,11 @@ func (a addressValue) Type() string {
 
 type consensusAddressStringType struct{}
 
-func (a consensusAddressStringType) NewValue(ctx context.Context, b *Builder) Value {
+func (a consensusAddressStringType) NewValue(ctx *context.Context, b *Builder) Value {
 	return &consensusAddressValue{
 		addressValue: addressValue{
 			addressCodec: b.ConsensusAddressCodec,
-			keyring:      b.Keyring,
+			ctx:          ctx,
 		},
 	}
 }
@@ -102,7 +109,9 @@ func (a consensusAddressValue) String() string {
 }
 
 func (a *consensusAddressValue) Set(s string) error {
-	addr, err := a.keyring.LookupAddressByKeyName(s)
+	// we get the keyring on set, as in NewValue the context is the parent context (before RunE)
+	keyring := getKeyringFromCtx(a.ctx)
+	addr, err := keyring.LookupAddressByKeyName(s)
 	if err == nil {
 		addrStr, err := a.addressCodec.BytesToString(addr)
 		if err != nil {
@@ -127,11 +136,7 @@ func (a *consensusAddressValue) Set(s string) error {
 	var pk cryptotypes.PubKey
 	err2 := cdc.UnmarshalInterfaceJSON([]byte(s), &pk)
 	if err2 != nil {
-		// failed all validation, just accept the input.
-		// TODO(@julienrbrt), for final client/v2 2.0.0 revert the logic and
-		// do a better keyring instantiation.
-		a.value = s
-		return nil
+		return fmt.Errorf("input isn't a pubkey (%w) or is an invalid account address (%w)", err, err2)
 	}
 
 	a.value, err = a.addressCodec.BytesToString(pk.Address())
@@ -141,3 +146,21 @@ func (a *consensusAddressValue) Set(s string) error {
 
 	return nil
 }
+
+func getKeyringFromCtx(ctx *context.Context) keyring.Keyring {
+	dctx := *ctx
+	if dctx != nil {
+		if clientCtx := dctx.Value(client.ClientContextKey); clientCtx != nil {
+			k, err := sdkkeyring.NewAutoCLIKeyring(clientCtx.(*client.Context).Keyring)
+			if err != nil {
+				panic(fmt.Errorf("failed to create keyring: %w", err))
+			}
+
+			return k
+		} else if k := dctx.Value(keyring.KeyringContextKey); k != nil {
+			return k.(*keyring.KeyringImpl)
+		}
+	}
+
+	return keyring.NoKeyring{}
+}

client/v2/autocli/flag/binary.go

@@ -14,7 +14,7 @@ type binaryType struct{}
 
 var _ Value = (*fileBinaryValue)(nil)
 
-func (f binaryType) NewValue(context.Context, *Builder) Value {
+func (f binaryType) NewValue(*context.Context, *Builder) Value {
 	return &fileBinaryValue{}
 }