-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sdk.Msg implement proto.Message #6327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6327 +/- ##
==========================================
- Coverage 55.69% 55.66% -0.04%
==========================================
Files 448 448
Lines 26941 26956 +15
==========================================
Hits 15005 15005
- Misses 10862 10877 +15
Partials 1074 1074 |
It isn't clear why Msgs need to become pointer receivers everywhere. Msgs should stay immutable, and keeping them as non-pointers helps with that (though not all the way, as slices for example are still mutable, but regardless it helps). The Reset() function could still take a pointer receiver but everything else can and arguably should stay the same. Pointer-fying also comes with baggage like needing to check for nils, so ideally we don't change anything that isn't already working. What is the API for unmarshaling any, |
@jaekwon I believe specifically because:
I advocated for clear semantics -- pointer or non-pointer only, don't mix. @aaronc brought to my attention that in order for a |
Protobuf code generation only supports pointer types. That is why the change is needed. In the future making changes to the code generation itself could be an option, but it isn't right now. So unfortunately l don't think the migration can proceed without this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conceptACK
ref: #6213
This PR makes
sdk.Msg
inheritproto.Message
. As a result allsdk.Msg
types now must use pointer sematnics.This is necessary for packing
sdk.Msg
s in protoAny
s and in particular to complete #6216.In some cases I forced certain Msg's to implement
proto.Message
with dummy implementations (in particular ibc/07-tendermint and a few test Msg's), but I tried to avoid this where possible and added a proto file for ibc/09-localhostMsgCreateClient
.For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorer