Update ADR 019 to reflect usage of Any #6081
Conversation
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
aaronc
requested review from
alessio,
alexanderbez,
fedekunze,
hschoenburg and
nylira
as code owners
alexanderbez
added
T: ADR
Codecov Report
@@ Coverage Diff @@
## master #6081 +/- ##
==========================================
- Coverage 54.78% 54.57% -0.21%
==========================================
Files 437 436 -1
Lines 26436 26296 -140
==========================================
- Hits 14482 14352 -130
+ Misses 10958 10952 -6
+ Partials 996 992 -4 |
|
|
||
| Note that `InterfaceContext` usage does not deviate from standard protobuf |
There was a problem hiding this comment.
How and where is this type used? It seems it's used in a concrete codec type. I'd note that here.
There was a problem hiding this comment.
Does this addition explain things clearly enough: 0470e47 ?
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
There was a problem hiding this comment.
conceptACK. I do not hold strong opinions on if this is truly the best approach or not, I just simply don't have enough context and experience from the client-side POV. That being said, the design looks reasonable to me. Finally, I would like to note, something feels "off" to me about the UnpackInterfacesMsg. Maybe once I see it in practice I'll be able to understand its purpose and use better 👍
Also, I'd prefer to see more 👀 on this (as much as possible) and more approvals before proceeding.
It's poorly named for sure, just couldn't think of something better yet. It could be generalized to a "post-unmarshal" hook - maybe something like |
| // interface passed in as iface | ||
| // Ex: | ||
| // ctx.RegisterInterface("cosmos_sdk.Msg", (*sdk.Msg)(nil)) | ||
| RegisterInterface(protoName string, iface interface{}) |
There was a problem hiding this comment.
q: Why do you need to register a name to an interface?
pp: this feels over complicated and may create some confusion for example which module/ code is responsible to register an interface. Would there be sanity checks if all interfaces have an implementation for example, failures on duplicates or multiple names, ... must register interface be called before register implementation? It would be easier to drop this functionality.
There was a problem hiding this comment.
q: Why do you need to register a
nameto an interface?
This is so that we can communicate which types satisfy which interface to clients. It is a usability thing. Since interfaces do not have a proto message name, we are giving them one here rather than making the client UX dependent on the golang name.
pp: this feels over complicated and may create some confusion for example which module/ code is responsible to register an interface. Would there be sanity checks if all interfaces have an implementation for example, failures on duplicates or multiple names, ... must register interface be called before register implementation? It would be easier to drop this functionality.
There should be failures on duplicates but it would not need to be called before registering implementations.
Yes it would be easier to drop this and dropping it would also diminish client UX IMHO.
There was a problem hiding this comment.
Not depending on the Golang name is a useful feature when integrating message protos across an application with many pieces outside of the blockchain written in other languages. It simplifies the ability to release proto bindings from a single shared reference project.
| about all the required types. Note, the use of `interface_type` allows us to avoid a significant | ||
| amount of code boilerplate when implementing the `Codec`. | ||
| The module manager will include a method to call `RegisterInterfaceTypes` on | ||
| every module that implements in order to populate the `InterfaceRegistry`. |
There was a problem hiding this comment.
q: Are there modules that do not register any new type for IO or persistence? Just curious if it is worth to make it optional
There was a problem hiding this comment.
All modules would need it to register their sdk.Msg types, should it not be optional?
| GetSubmitter() sdk.AccAddress | ||
| To implement the `UnpackInterfaces` phase of deserialization which unpacks | ||
| interfaces wrapped in `Any` before they're needed, we create an interface | ||
| that `sdk.Msg`s and other types can implement: |
There was a problem hiding this comment.
pp: The UnpackInterfaces adds some complexity and may be easily forgotten or done wrong.
I am wondering if it would be easier to have a reflection based approach that recursively scans for fields of types Any in the decoded protobuf. There needs to be a cast to the concrete interface or type later anyway.
Just learned about DynamicAny type in Gogo. That feels like something that goes into the same direction.
There was a problem hiding this comment.
This would be possible via reflection if we didn't care about making sure that the packed type was registered for the requested interface. I think there is a subtle difference but my preference is to be more explicit about which types satisfy which interfaces. For instance, say we have two interfaces Msg and PubKey and some type MsgX happens to satisfy both because of either programmer error or pure maliciousness, would we want to allow MsgX to be used as a PubKey? I would prefer a system that only allowed things that were properly registered.
We could implement all of the UnpackInterfaces methods in a way that checks for the right type via a gogo proto plugin. It's not that hard and I've done it before. I was just trying to avoid needing write a new plugin for this. But I can if it's helpful.
DynamicAny uses the global registry I'm trying to avoid.
There was a problem hiding this comment.
I would prefer a system that only allowed things that were properly registered.
I also prefer this ... portions of the code base that depend on understanding the message format should fail quickly ... other areas that can delegate processing should be able to pass off the message without "opening that box".
|
I've been following this development in various places (referenced issues and PRs as linked above). The implementation specified in this ADR looks very workable from my perspective as a client/module developer building on the SDK. |
This PR updates ADR 019 to reflect the usage of
Anyoveroneofas discussed in #6030.ADR 020 will be updated in a second PR after discussion in #6078 proceeds towards a solution.
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)