Support cosmos.msg.v1.signer as alternative to Msg.GetSigners

[aaronc]

Summary

sdk.Msg implementation should be able to omit the GetSigners() (which depends on global bech32 prefixes) and instead use the cosmos.msg.v1.signer protobuf annotation.

Problem Definition

Currently all sdk.Msg types must implement a method GetSigners() []sdk.AccAddress which both depends on a type definition (sdk.AccAddress) which depends on a global sdk.Config variable. This is problematic both because of many issues with the global variable (see #13140 and other linked issues) and because we want to make it so that a module can be written with a dependency just on cosmossdk.io/core and not the full SDK.

Proposal

All messages must define the cosmos.msg.v1.signer protobuf annotation which declaratively defines which fields are required to sign messages. We can utilize this in the AnteHandler and Tx implementation instead of GetSigners.

Previously we refactored GetSigners() to return []string in #9239 but that was unfortunately reverted in #9885 because it was considered too big of a breaking change.

An alternative would be to allow Msgs to either implement the current interface or to just implement the method ValidateBasic() error. We would primarily need to make changes to Tx.GetSigners and BaseApp.createEvents to accomodate this change. If GetSigners() is not defined then the protoreflect API would be used to read the cosmos.msg.v1.signer annotation and extract the signers from the message. To support gogo proto messages which don't support protoreflect, we can use the file descriptors extracted from gogo proto with dynamicpb to use protoreflect for this.

In this way, modules can be written which just depend on cosmossdk.io/core and don't use either sdk.AccAddress or the bech32 global.

[tac0turtle]

this can be closed right?

[aaronc]

It's still relevant. The issue should just reference AnteHandler's and not Middleware.

[aaronc]

Planning to pick this one up this week.

[Olshansk]

@aaronc tl;dr Is it a fair assumption that the SDK verifies the message is signed by cosmos.msg.v1.signer before it is passed to the `msgServer``?

---

For example, I was looking in x/protocolpool/keeper/msg_server.go which uses this protobuf

message MsgClaimBudget {
  option (cosmos.msg.v1.signer) = "recipient_address";
  string recipient_address      = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
}

Whose implementation looks like this:

func (k MsgServer) ClaimBudget(ctx context.Context, msg *types.MsgClaimBudget) (*types.MsgClaimBudgetResponse, error) {
	recipient, err := k.Keeper.authKeeper.AddressCodec().StringToBytes(msg.RecipientAddress)
	if err != nil {
		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid recipient address: %s", err)
	}

	// ...
}

The validity of the address is verified but not the underlying signature.

However, when it comes to the governance check (which is a bit different), looking in x/bank/keeper/msg_server.go:

func (k msgServer) SetSendEnabled(ctx context.Context, msg *types.MsgSetSendEnabled) (*types.MsgSetSendEnabledResponse, error) {
	if k.GetAuthority() != msg.Authority {
		return nil, errorsmod.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority)
	}

We find the extra k.GetAuthority() != msg.Authority comparison, but still no signature validation.

Here is the proto for reference:

message MsgSetSendEnabled {
  option (cosmos.msg.v1.signer) = "authority";
  option (amino.name)           = "cosmos-sdk/MsgSetSendEnabled";

  // authority is the address that controls the module.
  string authority = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];

cc @tac0turtle for visibility as well.

[tac0turtle]

signature verification happens at a higher level. It is safe to assume that if modules receive a message either verification has happened or the message is coming from internally.

[Olshansk]

Thanks @tac0turtle!