fix(codec): MarshalInterface should err when UnmarshalInterface will fail

CHANGELOG.md

@@ -109,6 +109,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (appModule) Remove `Route`, `QuerierRoute` and `LegacyQuerierHandler` from AppModule Interface. 
 * (x/modules) Remove all LegacyQueries and related code from modules
 * (store) [#11825](https://github.com/cosmos/cosmos-sdk/pull/11825) Make extension snapshotter interface safer to use, renamed the util function `WriteExtensionItem` to `WriteExtensionPayload`.
+* (codec) [#12964](https://github.com/cosmos/cosmos-sdk/pull/12964) `ProtoCodec.MarshalInterface` now returns an error when serializing unregistered types and a subsequent `ProtoCodec.UnmarshalInterface` would fail.
 
 ### CLI Breaking Changes
 

client/grpc_query.go

@@ -176,3 +176,7 @@ func (f failingInterfaceRegistry) ListAllInterfaces() []string {
 func (f failingInterfaceRegistry) ListImplementations(ifaceTypeURL string) []string {
 	panic("cannot be called")
 }
+
+func (f failingInterfaceRegistry) EnsureRegistered(iface interface{}) error {
+	panic("cannot be called")
+}

client/internal_client_test.go

@@ -0,0 +1,31 @@
+package client
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestFailingInterfaceRegistry(t *testing.T) {
+	reg := failingInterfaceRegistry{}
+
+	require.Error(t, reg.UnpackAny(nil, nil))
+	_, err := reg.Resolve("")
+	require.Error(t, err)
+
+	require.Panics(t, func() {
+		reg.RegisterInterface("", nil)
+	})
+	require.Panics(t, func() {
+		reg.RegisterImplementations(nil, nil)
+	})
+	require.Panics(t, func() {
+		reg.ListAllInterfaces()
+	})
+	require.Panics(t, func() {
+		reg.ListImplementations("")
+	})
+	require.Panics(t, func() {
+		reg.EnsureRegistered(nil)
+	})
+}

codec/any_test.go

@@ -26,35 +26,47 @@ func NewTestInterfaceRegistry() types.InterfaceRegistry {
 }
 
 func TestMarshalAny(t *testing.T) {
+	catRegistry := types.NewInterfaceRegistry()
+	catRegistry.RegisterImplementations((*testdata.Animal)(nil), &testdata.Cat{})
+
 	registry := types.NewInterfaceRegistry()
 
 	cdc := codec.NewProtoCodec(registry)
 
 	kitty := &testdata.Cat{Moniker: "Kitty"}
-	bz, err := cdc.MarshalInterface(kitty)
+	emptyBz, err := cdc.MarshalInterface(kitty)
+	require.ErrorContains(t, err, "does not have a registered interface")
+
+	catBz, err := codec.NewProtoCodec(catRegistry).MarshalInterface(kitty)
 	require.NoError(t, err)
+	require.NotEmpty(t, catBz)
 
 	var animal testdata.Animal
 
-	// empty registry should fail
-	err = cdc.UnmarshalInterface(bz, &animal)
-	require.Error(t, err)
+	// deserializing cat bytes should error in an empty registry
+	err = cdc.UnmarshalInterface(catBz, &animal)
+	require.ErrorContains(t, err, "no registered implementations of type testdata.Animal")
+
+	// deserializing an empty byte array will return nil, but no error
+	err = cdc.UnmarshalInterface(emptyBz, &animal)
+	require.Nil(t, animal)
+	require.NoError(t, err)
 
 	// wrong type registration should fail
 	registry.RegisterImplementations((*testdata.Animal)(nil), &testdata.Dog{})
-	err = cdc.UnmarshalInterface(bz, &animal)
+	err = cdc.UnmarshalInterface(catBz, &animal)
 	require.Error(t, err)
 
 	// should pass
 	registry = NewTestInterfaceRegistry()
 	cdc = codec.NewProtoCodec(registry)
-	err = cdc.UnmarshalInterface(bz, &animal)
+	err = cdc.UnmarshalInterface(catBz, &animal)
 	require.NoError(t, err)
 	require.Equal(t, kitty, animal)
 
 	// nil should fail
 	registry = NewTestInterfaceRegistry()
-	err = cdc.UnmarshalInterface(bz, nil)
+	err = cdc.UnmarshalInterface(catBz, nil)
 	require.Error(t, err)
 }
 

codec/proto_codec.go

@@ -194,6 +194,10 @@ func (pc *ProtoCodec) MarshalInterface(i gogoproto.Message) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
+	err = pc.interfaceRegistry.EnsureRegistered(i)
+	if err != nil {
+		return nil, err
+	}
 
 	return pc.Marshal(any)
 }

codec/proto_codec_test.go

@@ -3,6 +3,7 @@ package codec_test
 import (
 	"errors"
 	"fmt"
+	"reflect"
 	"testing"
 
 	"github.com/gogo/protobuf/proto"
@@ -46,6 +47,70 @@ func (lpm *lyingProtoMarshaler) Size() int {
 	return lpm.falseSize
 }
 
+func TestEnsureRegistered(t *testing.T) {
+	interfaceRegistry := types.NewInterfaceRegistry()
+	cat := &testdata.Cat{Moniker: "Garfield"}
+
+	err := interfaceRegistry.EnsureRegistered(*cat)
+	require.ErrorContains(t, err, "testdata.Cat is not a pointer")
+
+	err = interfaceRegistry.EnsureRegistered(cat)
+	require.ErrorContains(t, err, "testdata.Cat does not have a registered interface")
+
+	interfaceRegistry.RegisterInterface("testdata.Animal",
+		(*testdata.Animal)(nil),
+		&testdata.Cat{},
+	)
+
+	require.NoError(t, interfaceRegistry.EnsureRegistered(cat))
+}
+
+func TestProtoCodecMarshal(t *testing.T) {
+	interfaceRegistry := types.NewInterfaceRegistry()
+	interfaceRegistry.RegisterInterface("testdata.Animal",
+		(*testdata.Animal)(nil),
+		&testdata.Cat{},
+	)
+	cdc := codec.NewProtoCodec(interfaceRegistry)
+
+	cartonRegistry := types.NewInterfaceRegistry()
+	cartonRegistry.RegisterInterface("testdata.Cartoon",
+		(*testdata.Cartoon)(nil),
+		&testdata.Bird{},
+	)
+	cartoonCdc := codec.NewProtoCodec(cartonRegistry)
+
+	cat := &testdata.Cat{Moniker: "Garfield", Lives: 6}
+	bird := &testdata.Bird{Species: "Passerina ciris"}
+	require.NoError(t, interfaceRegistry.EnsureRegistered(cat))
+
+	var (
+		animal  testdata.Animal
+		cartoon testdata.Cartoon
+	)
+
+	// sanity check
+	require.True(t, reflect.TypeOf(cat).Implements(reflect.TypeOf((*testdata.Animal)(nil)).Elem()))
+
+	bz, err := cdc.MarshalInterface(cat)
+	require.NoError(t, err)
+
+	err = cdc.UnmarshalInterface(bz, &animal)
+	require.NoError(t, err)
+
+	bz, err = cdc.MarshalInterface(bird)
+	require.ErrorContains(t, err, "does not have a registered interface")
+
+	bz, err = cartoonCdc.MarshalInterface(bird)
+	require.NoError(t, err)
+
+	err = cdc.UnmarshalInterface(bz, &cartoon)
+	require.ErrorContains(t, err, "no registered implementations")
+
+	err = cartoonCdc.UnmarshalInterface(bz, &cartoon)
+	require.NoError(t, err)
+}
+
 func TestProtoCodecUnmarshalLengthPrefixedChecks(t *testing.T) {
 	cdc := codec.NewProtoCodec(createTestInterfaceRegistry())
 

codec/types/interface_registry.go

@@ -52,6 +52,9 @@ type InterfaceRegistry interface {
 	// ListImplementations lists the valid type URLs for the given interface name that can be used
 	// for the provided interface type URL.
 	ListImplementations(ifaceTypeURL string) []string
+
+	// EnsureRegistered ensures there is a registered interface for the given concrete type.
+	EnsureRegistered(iface interface{}) error
 }
 
 // UnpackInterfacesMessage is meant to extend protobuf types (which implement
@@ -81,6 +84,7 @@ type UnpackInterfacesMessage interface {
 type interfaceRegistry struct {
 	interfaceNames map[string]reflect.Type
 	interfaceImpls map[reflect.Type]interfaceMap
+	implInterfaces map[reflect.Type]reflect.Type
 	typeURLMap     map[string]reflect.Type
 }
 
@@ -91,6 +95,7 @@ func NewInterfaceRegistry() InterfaceRegistry {
 	return &interfaceRegistry{
 		interfaceNames: map[string]reflect.Type{},
 		interfaceImpls: map[reflect.Type]interfaceMap{},
+		implInterfaces: map[reflect.Type]reflect.Type{},
 		typeURLMap:     map[string]reflect.Type{},
 	}
 }
@@ -100,10 +105,26 @@ func (registry *interfaceRegistry) RegisterInterface(protoName string, iface int
 	if typ.Elem().Kind() != reflect.Interface {
 		panic(fmt.Errorf("%T is not an interface type", iface))
 	}
+
 	registry.interfaceNames[protoName] = typ
 	registry.RegisterImplementations(iface, impls...)
 }
 
+// EnsureRegistered ensures there is a registered interface for the given concrete type.
+//
+// Returns an error if not, and nil if so.
+func (registry *interfaceRegistry) EnsureRegistered(impl interface{}) error {
+	if reflect.ValueOf(impl).Kind() != reflect.Ptr {
+		return fmt.Errorf("%T is not a pointer", impl)
+	}
+
+	if _, found := registry.implInterfaces[reflect.TypeOf(impl)]; !found {
+		return fmt.Errorf("%T does not have a registered interface", impl)
+	}
+
+	return nil
+}
+
 // RegisterImplementations registers a concrete proto Message which implements
 // the given interface.
 //
@@ -162,7 +183,7 @@ func (registry *interfaceRegistry) registerImpl(iface interface{}, typeURL strin
 
 	imap[typeURL] = implType
 	registry.typeURLMap[typeURL] = implType
-
+	registry.implInterfaces[implType] = ityp
 	registry.interfaceImpls[ityp] = imap
 }
 

crypto/keys/secp256r1/pubkey_internal_test.go

@@ -79,11 +79,15 @@ func (suite *PKSuite) TestMarshalProto() {
 	/**** test structure marshalling with codec ****/
 
 	pk = PubKey{}
+	emptyRegistry := types.NewInterfaceRegistry()
+	emptyCodec := codec.NewProtoCodec(emptyRegistry)
 	registry := types.NewInterfaceRegistry()
-	cdc := codec.NewProtoCodec(registry)
-	bz, err = cdc.Marshal(suite.pk)
+	RegisterInterfaces(registry)
+	pubkeyCodec := codec.NewProtoCodec(registry)
+
+	bz, err = emptyCodec.Marshal(suite.pk)
 	require.NoError(err)
-	require.NoError(cdc.Unmarshal(bz, &pk))
+	require.NoError(emptyCodec.Unmarshal(bz, &pk))
 	require.True(pk.Equals(suite.pk))
 
 	const bufSize = 100
@@ -101,17 +105,17 @@ func (suite *PKSuite) TestMarshalProto() {
 	require.Equal(bz, bz2[(bufSize-pk.Size()):])
 
 	/**** test interface marshalling ****/
-	bz, err = cdc.MarshalInterface(suite.pk)
+	bz, err = pubkeyCodec.MarshalInterface(suite.pk)
 	require.NoError(err)
 	var pkI cryptotypes.PubKey
-	err = cdc.UnmarshalInterface(bz, &pkI)
+	err = emptyCodec.UnmarshalInterface(bz, &pkI)
 	require.EqualError(err, "no registered implementations of type types.PubKey")
 
-	RegisterInterfaces(registry)
-	require.NoError(cdc.UnmarshalInterface(bz, &pkI))
+	RegisterInterfaces(emptyRegistry)
+	require.NoError(emptyCodec.UnmarshalInterface(bz, &pkI))
 	require.True(pkI.Equals(suite.pk))
 
-	require.Error(cdc.UnmarshalInterface(bz, nil), "nil should fail")
+	require.Error(emptyCodec.UnmarshalInterface(bz, nil), "nil should fail")
 }
 
 func (suite *PKSuite) TestSize() {

go.work.sum

@@ -9,12 +9,13 @@ github.com/coinbase/kryptology v1.8.0/go.mod h1:RYXOAPdzOGUe3qlSFkMGn58i3xUA8hmx
 github.com/consensys/bavard v0.1.8-0.20210915155054-088da2f7f54a/go.mod h1:9ItSMtA/dXMAiL7BG6bqW2m3NdSEObYWoH223nGHukI=
 github.com/consensys/gnark-crypto v0.5.3/go.mod h1:hOdPlWQV1gDLp7faZVeg8Y0iEPFaOUnCc4XeCCk96p0=
 github.com/cosmos/cosmos-sdk/db v1.0.0-beta.1/go.mod h1:JUMM2MxF9wuwzRWZJjb8BjXsn1BmPmdBd3a75pIct4I=
-github.com/cosmos/keyring v1.2.0/go.mod h1:fc+wB5KTk9wQ9sDx0kFXB3A0MaeGHM9AwRStKOQ5vOA=
 github.com/dgraph-io/ristretto v0.0.3/go.mod h1:KPxhHT9ZxKefz+PCeOGsrHpl1qZ7i70dGTu2u+Ahh6E=
 github.com/ethereum/go-ethereum v1.10.19/go.mod h1:IJBNMtzKcNHPtllYihy6BL2IgK1u+32JriaTbdt4v+w=
 github.com/facebookgo/ensure v0.0.0-20200202191622-63f1cf65ac4c/go.mod h1:Yg+htXGokKKdzcwhuNDwVvN+uBxDGXJ7G/VN1d8fa64=
 github.com/facebookgo/subset v0.0.0-20200203212716-c811ad88dec4/go.mod h1:5tD+neXqOorC30/tWg0LCSkrqj/AR6gu8yY8/fpw1q0=
+github.com/fogleman/gg v1.2.1-0.20190220221249-0403632d5b90 h1:WXb3TSNmHp2vHoCroCIB1foO/yQ36swABL8aOVeDpgg=
 github.com/go-kit/log v0.2.1 h1:MRVx0/zhvdseW+Gza6N9rVzU/IVzaeE1SFI4raAhmBU=
 github.com/go-logfmt/logfmt v0.5.1 h1:otpy5pqBCBZ1ng9RQ0dPu4PN7ba75Y/aA+UpowDyNVA=
 github.com/go-playground/assert/v2 v2.0.1 h1:MsBgLAaY856+nPRTKrp3/OZK38U/wa0CcBYNjji3q3A=
 github.com/golang/freetype v0.0.0-20170609003504-e2365dfdc4a0 h1:DACJavvAHhabrF08vX0COfcOBJRhZ8lUbR+ZWIs0Y5g=
+golang.org/x/image v0.0.0-20190802002840-cff245a6509b h1:+qEpEAPhDZ1o0x3tHzZTQDArnOixOzGD9HUJfcg0mb4=

testutil/testdata/animal.go

@@ -17,10 +17,20 @@ type Animal interface {
 	Greet() string
 }
 
-func (c Cat) Greet() string {
+type Cartoon interface {
+	proto.Message
+
+	Identify() string
+}
+
+func (c *Cat) Greet() string {
 	return fmt.Sprintf("Meow, my name is %s", c.Moniker)
 }
 
+func (c *Bird) Identify() string {
+	return "This is Tweety."
+}
+
 func (d Dog) Greet() string {
 	return fmt.Sprintf("Roof, my name is %s", d.Name)
 }