Protobuf Any Interface Redesign

[aaronc]

Recently we have made substantial progress on a code generator (https://github.com/cosmos/cosmos-proto) for the new google.golang.org/protobuf API. We decided it was time for gogo proto to go and that the golang community is by and large migrating to the new API. So far we have implemented fast marshaling and fast reflection and we are planning to support custom scalar types (in particular sdk.Int and sdk.Dec).

We have also discussed what to do with protobuf Anys as the current UnpackInterfacesMessage approach is rather cumbersome but needed for amino compatibility. The initial idea was to simply generate structs that had interface (i.e. sdk.Msg, auth.AccountI) fields rather than Anys and to handle the marshaling behind the scenes. We worked on a few designs such as this one proposed by @fdymylja which generates wrapper types and uses protobuf services: https://github.com/cosmos/cosmos-proto/pull/26/files.

Problems with interfaces on proto types

In our discussions, however, we began to question the value of defining golang interfaces directly on protobuf types at all and identified the following weaknesses with this strategy:

• defining interfaces on protobuf types moves implementation into APIs - for achieving proper semantic versioning (Cosmos SDK semver-compliant versioning proposal #10162) we want to move generated proto files to separate go modules, but coupling generated types with implementation details in interfaces complicates this
• defining interface methods on generated types does not allow chain and module developers to override functionality. @JimLarson from Agoric ran into issues where they are needing to jump through hoops to override certain vesting account functionality: Agoric/agoric-sdk@db79c42. Also, for things like public keys - do we really want to tightly couple generated code with cryptographic algorithms and address generation?

Alternative approach

So our current thinking is that we will actually not support interfaces with the new code generator any more than we already do with the clunky UnpackInterfacesMessage. Instead, we are considering deprecating interface usage on generated types generally within the SDK in favor of:

• decoupled handlers like those used to route gov.Content and Evidence
• declarative metadata on protobuf types for things that are also relevant to clients like:
  • which fields are signers in a Msg, or
  • simple validation rules which are currently handled opaquely in ValidateBasic
• considering ValidateBasic as a special extension of proto.Message that can be used in various scenarios where objects are routed to handlers or even serialized with the ORM

Refactoring of Existing Interfaces

Here’s how this would look for the various interface types used in the SDK:

sdk.Msg

• GetSigners would move to metadata in .proto files so that it is also obvious to clients, ex:

message MsgSend {
  option (cosmos.msg.v1.Signer) = "from_address";
  string from_address = 1;
  ...
}

• some use cases of ValidateBasic could be handled declaratively in .proto files

message MsgSend {
  string from_address = 1 [(cosmos_proto.validation.required) = true];
}

• other more complex use cases of ValidateBasic would be handled with a handler (see PubKey below for more details on handlers)

handler := validateBasicResolver.ResolveHandler(msg)
err := handler.ValidateBasic()
...

gov.Content

This is all going away anyway in favor of sdk.Msg (#9810)

PubKey

I propose having a resolver which returns different PubKey interface implementations for each protobuf pub key type. The pub key implementations would be registered through constructor functions which take the protobuf type and return the interface type. Ex:

type PubKeyResolver interface {
  GetHandler(proto.Any) (PubKey, error)
}

// handler constructor for secp key
func NewSecpPubKey(protoKey secpproto.PubKey) PubKey { ... }

This will free different chains to use different crypto libraries or different address implementations as desired (although different concrete proto types may be a better alternative to changing the address format).

auth.AccountI

This is a weird case where the interface has a number of complected concerns. Address, PubKey, AccountNumber, and Sequence are shared by all implementations so they maybe don’t even really belong in the interface but rather should move up to some concrete type. Then we have module accounts which are getting redesigned in accordance with ADR-028 and ADR-033, and vesting accounts which also are long overdue for a revamp and shouldn’t really be connected to authentication at all. So there is some refactoring that is long overdue here and likely a short-term solution is a handler approach as described above for PubKey, but a more involved refactor may make sense here.

Evidence, authz.Authorization, feegrant.FeeAllowanceI

These would be handled similarly to sdk.Msg with the routing stuff coming from the proto message name, ValidateBasic being a separate method, and the things moving to handlers.

Roadmap

In the short-term this approach would allow us to make the code generator mostly a drop in replacement for gogo proto because we would support the same UnpackInterfacesMessage based code and basically the same Codec API. In the mid-term, doing the interface refactorings above will allow us to move generated proto code into isolated semantically-versioned go modules without needing to worry about tight coupling of implementation logic there and all the dependencies that would bring.

We’d appreciate any thoughts on this approach as getting alignment on this is crucial to moving the code generator project along and doing some necessary clean-up of the SDK.

[fdymylja]

I support this approach, interfaces are the only amino left-over at codec level and they're not protobuf first class citizens, we have have discussed and trialed different approaches but they all ended up in clunky or unsafe APIs which were unfitting for the ProtoReflect standard.

Considering the usage of interfaces in the sdk, and considering that most of use-cases could be solved through handlers (pub key being the only real interface), the effort required to create and maintain a custom API and code generator does not seem to be justified for the little benefits it brings. On top of this handlers allow more customisation through extension or overriding.

cc @alessio, @jgimeno, @sahith-narahari, @gsora, @spoo-bar

[tac0turtle]

Huge fan of this proposal. It should help a lot in ergonomics for client developers.

[jgimeno]

These look great, a nice step in the Cosmos SDK.

[robert-zaremba]

Few questions:

• Are you proposing to go back to the handler paradigm, and manually (or semi-automatically) register handlers for each message type? Would the current approach work (semi-automatic Msg -> Msg service mapping)?
• What if signers will have to be computed? Imagine we have different allowance mechanism, where spending from an account could be signed by one of many accounts (eg one of many signers, kind of a multi sig).
• In this proposal, we will have pre-defined ValidateBasic for various types (eg address), but we it will still need to be open for a developer, to assemble other validation if required, correct? So generated proto package will be open for modification by developers?

[aaronc]

Signers are never really computed currently but explicitly specified in Msgs. We wouldn’t change Msg registration but other interfaces would need to register handlers

[fdymylja]

What if signers will have to be computed? Imagine we have different allowance mechanism, where spending from an account could be signed by one of many accounts (eg one of many signers, kind of a multi sig).

in 99% of use cases the signer is one or multiple and they must all be present, no need for OR logic. If we need more complex logic then I think it would fit better into ValidateBasic. Also another important thing is that by marking signers fields with proto options you're helping clients, because interface implementation is not something that you can communicate explicitly cross language.

In this proposal, we will have pre-defined ValidateBasic for various types (eg address), but we it will still need to be open for a developer, to assemble other validation if required, correct? So generated proto package will be open for modification by developers?

ValidateBasic could fit into the concept of admission handlers, and they are something that would be checked not only at checkTx/deliverTx level but also during intermodule communication, also developers could define multiple admission handlers for the same sdk.Msg.

Ex:

func AdmissionHandlerValidateBasic(ctx context.Context, m *MsgSend) error {
   validate that coins is not negative...
}

func AdmissionHandlerValidateNonBlocked(ctx context.Context, m *MsgSend) error {
    blocked := bankClient.IsBlocked(ctx, m.Sender)
    if blocked { return ftm.Errorf("blocked %s", m.Sender) }
    ...
}

Also I would say no solution to interface handling right now is set in stone, what we're trying to say is not to make interfaces first class citizens in protobuf because they are not, but let's find a different solution specific to the use-cases we have in the sdk. Then some use-cases (Evidence, Authz, FeeGrant) might be solved by the same framework, others might have different solutions.

[robert-zaremba]

Signers are never really computed currently but explicitly specified in Msgs.

In Core SDK. But we don't know if someone else is computing it. Currently, the interface allows to compute it.

[robert-zaremba]

Alternative: embedded implementers.

During the last week architecture call I had the following idea:

type MsgA struct {
// proto attributes
  Att1 string

// "functional" attributes, won't be serialized:
  // we inject the functionality:
  msgImplementation func(*MsgA) sdk.Msg
  // or, we inject a wrapper:
  msgImplementer sdk.Msg
  // or through anonymous field:
  sdk.Msg  // eg MsgA.Msg = MsgAWrapper{*Msg}
}

I think the last approach may have the most potential.
Example implementation for bank.MsgSend:

// MsgSend represents a message to send coins from one account to another.
message MsgSend {
  string   from_address                    = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
  string   to_address                      = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"];
  repeated cosmos.base.v1beta1.Coin amount = 3
      [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];

  cosmos.Implementer Msg [(cosmos_proto.implementer = "sdk.Msg")]
}

this will generate:

type MsgSend struct {
	FromAddress string
	ToAddress   string
	Amount      github_com_cosmos_cosmos_sdk_types.Coins

	sdk.Msg
}

And then in the module wiring we will need to have a function (or a provider for DI):

func (m BankModule) WireImplementers(r wiring.ImplementerRouter) {
   r.Add(proto.MessageName((nil)(&proto.MsgSend), (nil)(&sdk.Msg), newMsgSendMsg)
}

// creates a wrapper for MsgSend to implementing sdk.Msg
func newMsgSendMsg(m *proto.MsgSend) sdk.Msg {
   return msgMsgSend{m}
}

type msgMsgSend struct {
   m *proto.MsgSend
}

func (m msgMsgSend) GetSigners() []sdk.AccAddress {
	fromAddress, _ := sdk.AccAddressFromBech32(m.m.FromAddress)
	return []sdk.AccAddress{fromAddress}
}

Pros & Cons

Related to the current implementation
Cons:

• Need to write wrapper types for every Msg type.

[aaronc]

But this leaks the implementor struct to clients in the proto message itself

[fdymylja]

the thing that I don't like about both approaches is that they're implicit in what they do, like when you're dealing with types you need to inject functionality every time, from where is this functionality injected ? what happens when a user instantiates a message in the following way:

pubKey := &crypto.PubKey{}

[robert-zaremba]

The functionality would be injected by interceptor, or some middleware. So user wouldn't be able to efficiently hack it.

[robert-zaremba]

Alternative: embedded functional implementers .

Similar to the above, but instead having an interface type in the proto, we will have a functional type. I will use same example as above:

message MsgSend {
  string   from_address                    = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
  string   to_address                      = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"];
  repeated cosmos.base.v1beta1.Coin amount = 3
      [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];

  cosmos.Implementer ValidateBasic [(cosmos_proto.implementer = "wiring.ValidateBasic")]
  // we can do same with getSigners, but it could have a default implementation as outlined in the Aaron original idea.
  // cosmos.Implementer GetSingers [(cosmos_proto.implementer = "wiring.GetSingers")]
}

this will generate:

import goproto "...google/proto"

type MsgSend struct {
	FromAddress string
	ToAddress   string
	Amount      github_com_cosmos_cosmos_sdk_types.Coins

	ValidateBasic  wiring.ValidateBasic  // type: func(goproto.Message) error
}

We will need a wiring router as above....
And finally the implementation:

func MsgSendValidateBasic(m goproto.Message) error {
    m, ok := m.(*proto.MsgSend)
    if !ok { return errors.WrongType(...) }
    // validate m
}

[aaronc]

Ditto for my above comment. Why add more fields to the proto message?

[robert-zaremba]

I'm trying to brainstorm other approaches, which doesn't involve adding one more router.

[aaronc]

Here is one proposal for how interface handlers could be wired up generically using a "plugin host" mechanism: https://github.com/cosmos/cosmos-sdk/pull/10452/files. Basically this plugin host would allow for:

• registering constructors which take a proto type and return an interface implementation, and in turn
• resolving the interface implementations

There are other ways this could be done for sure but this is one fairly generic, reusable approach.

[aaronc]

I had previously said that ValidateBasic could still be defined on generated proto code with this refactoring, but that no longer seems like a good idea. ValidateBasic would need a handler approach similar to PubKey.

Due to the way that the api module can be updated separately from state machine logic, we want to avoid any state machine breaking changes in the api module that are not represented in the .proto files. Anything that has changed in .proto files can be dealt with using pinned file descriptors as described in #10582, but interface methods would cause state machine breakage if they changed.

[tac0turtle]

How can we move forward with the removal of interface encoding here? Could someone open an issue/epic on the discussion with potential work phases. Even if we need to do an adr

[pyramation]

I suppose I had good timing with the post in slack.

FWIW, I have been relying on the implements_interface and accepts_interface annotations to start building out client-side decoders (for Protobuf) as well as amino encoders/decoders. So for codegen, currently we need these annotations to decode google.protobuf.Any for BOTH proto + amino

Would this redesign remove usage of Any? It seems that we may in short to mid term still have these (for MsgSubmitProposal and some authz messages, for example)

[pyramation]

on a high-level, the aim for telescope here will be to remove any need to manage Any typings, since the encoders/decoders will be completely automated. As long as a Go dev adds the annotations, we can swap out Any for a multiplexer that will cast to the appropriate type by reading the type url.

[aaronc]

This redesign doesn't imply any change to our usage of Any within .proto files - it only affects golang. Apologies if there was any confusion.

I'm glad that implements_interface and accepts_interface are proving useful on the client side. That's the intention and hopefully our usage of these annotations can become clearer and more consistent.

[pyramation]

Regarding Interfaces I found some in the osmosis codebase that are using RegisterInterface with fully scoped names:

	registry.RegisterInterface(
		"osmosis.swaprouter.v1beta1.PoolI",
		(*swaproutertypes.PoolI)(nil),
		&Pool{},
	)
	registry.RegisterInterface(
		"osmosis.gamm.v1beta1.PoolI", // N.B.: the old proto-path is preserved for backwards-compatibility.
		(*types.CFMMPoolI)(nil),
		&Pool{},
	)

but for implements/accepts annotations, it's not scoped:

(cosmos_proto.implements_interface) = "PoolI"

or

(cosmos_proto.accepts_interface) = "PoolI"

Is the osmosis.swaprouter.v1beta1.PoolI intended to actually be a different interface? Seems to be that PoolI is intended to be a global interface for any Pool.

I ask because we’re potentially going to create conventions in the SDK-JS group that could impact this, so I’d like to better understand how it’s used

cc @ValarDragon @sunnya97

[pyramation]

My mental model of interfaces was that there is one interface PoolI (maybe arguably could be osmosis.gamm.v1beta1.PoolI) and then BalancerPool, SwaprouterPool and other future pool types could adhere to the interface of this single interface.

[pyramation]

Reading the osmosis Go code, my attempted understanding is that, osmosis.swaprouter.v1beta1.PoolI is the implementation for the PoolI interface, and in this case, this would make PoolI an arbitrary name that is global in the namespace of interfaces throughout.

If this assumption is true, then we should not be scoping {implements|accepts}_interface, and just using global names. For example, we'd need to make a few changes like this around cosmos sdk and ibc repos:

- option (cosmos_proto.implements_interface) = "cosmos.gov.v1beta1.Content";
+ option (cosmos_proto.implements_interface) = "Content";

[pyramation]

On a 2nd glance, I did find some interfaces in Go:

• type PoolI interface here
• type Content interface here

In the PoolI example, it's not clear how this would be used to map to types in other modules for polymorphism because it's defined only in the swaprouter

for the Content case, it does happen to be defined as a type in cosmos.gov.v1beta1 technically, but there is nothing in the protobuffers, so codegen tooling is still left a bit in the dark here.

[pyramation]

Also, even if we are using scopes (which does seem to be in the ADR for interfaces), I've found we're not adhering to it based on what scope lookups I can calculate if I understand the Go protobuf scope lookup correctly. I've filed an issue here with protos that I've found that have bad annotations #14353

Additionally, protobufs are missing any notion of interface definitions (e.g. cosmos.authz.Authorization is defined in a Go file, but not in a proto file anywhere), so while we could fix these protos and make sure they're mapping properly to existing interfaces, the protobufs (and therefore any codegen tool) would have no idea if an interface exists, and therefore would be technically impossible to match what interface is being looked up, if different scopes were provided.

Therefore I propose there are two possible solutions:

1. we use full scopes as @AmauryM had suggested
2. we export the interface declarations somehow to the protobuffers, (which I am guessing is more difficult)

(There could be a third, which is as long as the same string is used in any implements/accepts and is considered global, then that can work for codegen as well, but is theoretically less strict and a bit loose)