Signing options (e.g. CustomGetSigners) should be passed down to NewTxConfig

[SpicyLemon]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

The top of NewSimApp looks like this (in v0.50.10):

cosmos-sdk/simapp/app.go

Lines 197 to 214 in 6dc6e8b

	interfaceRegistry, _ := types.NewInterfaceRegistryWithOptions(types.InterfaceRegistryOptions{
	ProtoFiles: proto.HybridResolver,
	SigningOptions: signing.Options{
	AddressCodec: address.Bech32Codec{
	Bech32Prefix: sdk.GetConfig().GetBech32AccountAddrPrefix(),
	},
	ValidatorAddressCodec: address.Bech32Codec{
	Bech32Prefix: sdk.GetConfig().GetBech32ValidatorAddrPrefix(),
	},
	},
	})
	appCodec := codec.NewProtoCodec(interfaceRegistry)
	legacyAmino := codec.NewLegacyAmino()
	txConfig := tx.NewTxConfig(appCodec, tx.DefaultSignModes)
	if err := txConfig.SigningContext().Validate(); err != nil {
	panic(err)
	}

If CustomGetSigners are added to the SigningOptions (that are provided to NewInterfaceRegistryWithOptions), they don't end up being conveyed to the result of NewTxConfig. That causes txConfig.SigningContext().Validate() to return an error for the protos that use a custom GetSigners method (and don't have a signer option): no cosmos.msg.v1.signer option found for message <msg name>; use DefineCustomGetSigners to specify a custom getter.

I would not expect to get an error there, though, since the interfaceRegistry and appCodec both handle the custom-get-signer stuff just fine. I.e. Neither interfaceRegistry.SigningContext().Validate() nor appCodec.InterfaceRegistry().SigningContext().Validate() returns an error.

A workaround is to use NewTxConfigWithOptions instead of NewTxConfig, and extract those signing options into its own variable so that they can be provided when creating both the interface registry and the tx-config. But I feel that NewTxConfig should get that info from the interface registry in the provided appCodec, and NewTxConfigWithOptions should too when the authtx.ConfigOptions.SigningOptions is nil (or maybe there's a more specific field to look at).

Cosmos SDK Version

v0.50.10

How to reproduce?

You'll need a proto with an endpoint that has a request message without a cosmos.msg.v1.signer option and a custom GetSigners function for it.

Add that info to the SigningOptions.CustomGetSigners map (e.g. using DefineCustomGetSigners) in the InterfaceRegistryOptions.SigningOptions that is provided to NewInterfaceRegistryWithOptions

Example code that panics:

sigOpts := signing.Options{
	AddressCodec: address.Bech32Codec{Bech32Prefix: sdk.GetConfig().GetBech32AccountAddrPrefix()},
	ValidatorAddressCodec: address.Bech32Codec{Bech32Prefix: sdk.GetConfig().GetBech32ValidatorAddrPrefix()},
}
sigOpts.DefineCustomGetSigners("", nil) // TODO: Fill in appropriately
interfaceRegistry, _ := types.NewInterfaceRegistryWithOptions(types.InterfaceRegistryOptions{
	ProtoFiles: proto.HybridResolver,
	SigningOptions: sigOpts,
})
appCodec := codec.NewProtoCodec(interfaceRegistry)
legacyAmino := codec.NewLegacyAmino()
txConfig := tx.NewTxConfig(appCodec, tx.DefaultSignModes)

if err := txConfig.SigningContext().Validate(); err != nil {
	panic(err)
}

When that runs, you'll get a panic because txConfig.SigningContext().Validate() will return an error: no cosmos.msg.v1.signer option found for message <msg name>; use DefineCustomGetSigners to specify a custom getter.

The workaround looks something like this:

sigOpts := signing.Options{
	AddressCodec: address.Bech32Codec{Bech32Prefix: sdk.GetConfig().GetBech32AccountAddrPrefix()},
	ValidatorAddressCodec: address.Bech32Codec{Bech32Prefix: sdk.GetConfig().GetBech32ValidatorAddrPrefix()},
}
sigOpts.DefineCustomGetSigners("", nil) // TODO: Fill in appropriately
interfaceRegistry, _ := types.NewInterfaceRegistryWithOptions(types.InterfaceRegistryOptions{
	ProtoFiles: proto.HybridResolver,
	SigningOptions: sigOpts,
})
appCodec := codec.NewProtoCodec(interfaceRegistry)
legacyAmino := codec.NewLegacyAmino()
txConfigOpts := authtx.ConfigOptions{
	EnabledSignModes: authtx.DefaultSignModes,
	SigningOptions:   &signingOptions,
}
txConfig, err := authtx.NewTxConfigWithOptions(appCodec, txConfigOpts)
if err != nil {
	panic(err)
}
if err := txConfig.SigningContext().Validate(); err != nil {
	panic(err)
}

Then, later, after the bank keeper is made, to enable sign mode textual, you could just update txConfigOpts and give it to NewTxConfigWithOptions again.

txConfigOpts.EnabledSignModes = append(txConfigOpts.EnabledSignModes, sigtypes.SignMode_SIGN_MODE_TEXTUAL)
txConfigOpts.TextualCoinMetadataQueryFn = txmodule.NewBankKeeperCoinMetadataQueryFn(app.BankKeeper)
txConfig, err = authtx.NewTxConfigWithOptions(appCodec, txConfigOpts)
if err != nil {
	panic(err)
}

But again, I feel like the custom GetSigners stuff should automatically get propagated from the interface registry in the appCodec to the txConfig without needing to also provide the signing options to NewTxConfigWithOptions.

[julienrbrt]

I think it is a valid point indeed. It was anyway a typo, as it should have been interfaceRegistry.SigningContext().Validate() instead of using txConfig, like we do in runtime, in the simapp app.go.

cosmos-sdk/runtime/module.go

Line 181 in b57ce4d

if err := interfaceRegistry.SigningContext().Validate(); err != nil {

I would note that you don't need to do your workaround, as you can pass it directly in NewTxConfig, but your point remains valid, passing it twice, is not best:

cosmos-sdk/x/auth/tx/config.go

Lines 76 to 78 in 6dc6e8b

	func NewTxConfig(protoCodec codec.Codec, enabledSignModes []signingtypes.SignMode,
	customSignModes ...txsigning.SignModeHandler,
	) client.TxConfig {

.