Wrap ProtoCodec in interface #7637
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7637 +/- ##
=======================================
Coverage 54.15% 54.15%
=======================================
Files 611 611
Lines 38566 38566
=======================================
Hits 20884 20884
Misses 15550 15550
Partials 2132 2132 |
| // binary and JSON encoding. | ||
| type ProtoCodecMarshaler interface { | ||
| Marshaler | ||
| InterfaceRegistry() types.InterfaceRegistry |
There was a problem hiding this comment.
I don't think we want that - this is used for Amino.
There was a problem hiding this comment.
see #7638 for reference. I don't think this change is necessary
| @@ -11,13 +11,21 @@ import ( | |||
| "github.com/gogo/protobuf/proto" | |||
| ) | |||
|
|
|||
| // ProtoCodecMarshaler defines and interface for codecs that utilize Protobuf for both | |||
| // binary and JSON encoding. | |||
| type ProtoCodecMarshaler interface { | |||
There was a problem hiding this comment.
| type ProtoCodecMarshaler interface { | |
| type ProtoCodecI interface { |
There was a problem hiding this comment.
The ProtoMarshaler type is already defined in that package.
There was a problem hiding this comment.
Maybe ProtoMarshalerI to align with other interface names in the SDK like AccountI ?
There was a problem hiding this comment.
This is a terrible name but I understand the reasoning. We should rename the other ProtoMarshaler separately
There was a problem hiding this comment.
Actually how about ProtoCodecI. Maybe that's best?
|
also per @colin-axner's comment, this doesn't touch the getsignbytes thing. This touches the tx encoding for proto and is needed. |
|
Can we merge this and include in rc2? edit: looks like that is the case with the backports tag |
I think the rc1 is the next one. |
|
We did rc1 last week |
* WIP encoding change * Add test that describes issue * WIP debugging * remove extra code * Update codec/proto_codec.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Ok cleaned up this PR. Changes are minimal