Add optional extension interface to manually specify GetSigners

[aaronc]

To handle EVM use cases (ex https://github.com/evmos/evmos/blob/95c83a1ed48063f1643ef853ca77d5d42a8d7639/x/evm/types/msg.go#L204) we need an alternative to cosmos.msg.v1.signer as implemented in #11275. I propose this interface:

type HasCustomSigners interface {
  GetCustomSigners() [][]byte
}

This way the interface doesn't overlap with the existing and deprecated GetSigners() []sdk.AccAddress

[aaronc]

This is a bit painful because we are now extracting signers by decoding the messages using the google.golang.org/protobuf API. This means that if a type is built with gogo proto we will extract its signers by unmarshaling to dynamicpb. So even if we define a custom signers interface on the gogo proto message, that won't get picked up by the GetSigners code unless we're very careful and maybe do some hacky things. Also we have validation in signing.Context to make sure all messages define cosmos.msg.v1.signer.

To address the validation issue, I propose a new option cosmos.msg.v1.custom_signer which indicates that validation should be skipped and some custom method will be used.

Two approaches I can think of to handle this custom method are:

1. registering a custom GetSigners function with signing.Context for these types, and/or
2. some hacky thing where we first check the gogo msg for HasCustomSigners

Maybe best to try both of them and then this gives people options.

Also, I wonder how this would be dealt with by dynamic clients like autocli or with cross lang modules. Thinking of the cross lang perspective of services all the way down, we'd probably want some sort of SignersService that modules implement which takes an Any and returns the list of signers. That would work elegantly cross lang and also work for dynamic clients (assuming this SignerService is stateless which it really should be).

[kocubinski]

Just so I understand the use case, why can cosmos.msg.v1.signer not be added to the proto IDL?
https://github.com/evmos/evmos/blob/95c83a1ed48063f1643ef853ca77d5d42a8d7639/proto/ethermint/evm/v1/tx.proto#L25
Is it in order to support custom logic, in this case err if sender is nil?
https://github.com/evmos/evmos/blob/95c83a1ed48063f1643ef853ca77d5d42a8d7639/x/evm/types/msg.go#LL210C1-L213C3

[robert-zaremba]

I am still thinking about a use-case of having a multisig msg wrapper:

type ContractMultisig struct {
  DAOid uint64
  PreDeterminedSigners []AccAddress
  Msg Any
}

[tac0turtle]

Just so I understand the use case, why can cosmos.msg.v1.signer not be added to the proto IDL? https://github.com/evmos/evmos/blob/95c83a1ed48063f1643ef853ca77d5d42a8d7639/proto/ethermint/evm/v1/tx.proto#L25 Is it in order to support custom logic, in this case err if sender is nil? https://github.com/evmos/evmos/blob/95c83a1ed48063f1643ef853ca77d5d42a8d7639/x/evm/types/msg.go#LL210C1-L213C3

if they want to provide the same ux as ethereuem they do signers differently, we should aim to provide a way for users to do this sort of thing. In ethereum you dont specify the signer, you recover the signer from the signature.

[tac0turtle]

To address the validation issue, I propose a new option cosmos.msg.v1.custom_signer which indicates that validation should be skipped and some custom method will be used.

can we just say if it doesnt have cosmos.v1.signer we assume the user has a getsigners method on the message? If the user forgets both then it would break the chain. I would have loved to have a migration path for current users as well so they can migrate over a release since this will be needed by everyone

Which ever path is taken, this is a requirement for the merge of the pr and inclusion in a release

[itsdevbear]

SignerService makes sense to me

I think the biggest ick I have about this change is the fact that you're effectively just replacing a deprecated function with another functionally identical function.

I'm a huge fan of address recover from the signature's recovery bit, and my bias would be to push that.

(I know not possible for all curves but still my bias)

[itsdevbear]

@aaronc with the way Polaris handles eth txs this all is a moot point. We simply unwrap to Geth type recover and don't worry about anything else

https://github.com/berachain/polaris/blob/main/cosmos/proto/polaris/evm/v1alpha1/tx.proto

[aaronc]

To address the validation issue, I propose a new option cosmos.msg.v1.custom_signer which indicates that validation should be skipped and some custom method will be used.

can we just say if it doesnt have cosmos.v1.signer we assume the user has a getsigners method on the message? If the user forgets both then it would break the chain.

I don't think we can just assume it has a custom GetSigners method without doing validation. cosmos.msg.v1.signer should cover 99% of the use cases so if it isn't included, it generally means the user forgot it.

[itsdevbear]

@aaronc we are trying to upgrade polaris to 0.38 comet and this GetSigners broke us. What is the status of #16184 we are totally blocked at the moment.

[aaronc]

I'll be working on #16184 this week and hoping to get it merged ASAP.

[itsdevbear]

🔥

[kocubinski]

Likewise, collaborating in #16340.

[itsdevbear]

wouldn't the real solution be to make the proto tag optional?

[kocubinski]

wouldn't the real solution be to make the proto tag optional?

You're proposing cosmos.msg.v1.signer optional, and if absent require a custom signer be registered?

[itsdevbear]

Yeah instead of defining this second proto tag.

and then reuse the GetSigners() function to prevent the breaking change.

if the msg.signer tag is missing, the GetSigners isn't generated by the proto gen and thus will need to be manually added. way cleaner

[kocubinski]

Not adding another proto tag makes sense to me. If omitted and a message is to be signed, this design has DefineCustomGetSigners being called somewhere in initialization code for the message needing custom signer fetching logic.

[kocubinski]

@itsdevbear We're getting close to merging #16340. You'd register a custom get signer func like this test shows:

cosmos-sdk/tests/integration/tx/context_test.go

Lines 18 to 28 in 410935f

	func ProvideCustomGetSigners() signing.CustomGetSigner {
	return signing.CustomGetSigner{
	MsgType: proto.MessageName(&testpb.TestRepeatedFields{}),
	Fn: func(msg proto.Message) ([][]byte, error) {
	testMsg := msg.(*testpb.TestRepeatedFields)
	// arbitrary logic
	signer := testMsg.NullableDontOmitempty[1].Value
	return [][]byte{[]byte(signer)}, nil
	},
	}
	}

We're using "pulsar" types (the latest official google protobuf spec/lib) for registration instead of the gogo types generated from cosmos-sdk/gogoproto since all of x/tx is built on ProtoReflect capable messages. My preference is not to support gogo at all in x/tx or this API. Even if your chain uses gogo they will be packed into pulsar types anyway in signing code.

Does this solution work for your team?

[aaronc]

Note we don't need to require pulsar codegen. Just the official codegen or pulsar