cosmos-reflection: extend to support writing by reflection clients #8965
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8965 +/- ##
==========================================
- Coverage 58.99% 58.95% -0.04%
==========================================
Files 575 574 -1
Lines 32192 32213 +21
==========================================
Hits 18992 18992
- Misses 10984 11002 +18
- Partials 2216 2219 +3
|
|
pinging also @robert-zaremba in case you wanna provide some feedback and insights on the proposed API |
# Conflicts: # x/staking/types/staking.pb.go
…reflection' into frojdi/issue-8959-extend-cosmos-reflection
fdymylja
requested review from
robert-zaremba,
amaury1093,
jgimeno and
sahith-narahari
|
introduces a breaking change in proto because golang package has changed, to make it consistent to where gRPC reflection lives. on a second note, I had tried a lot to give even stronger reflection to the point in which client would not need to rely on auth for building txs. unfortunately current SDK abstractions and structure of signing (partly in types, partly in auth) does not allow for stronger reflection capabilities. they could have been achieved but the approach would have not been as clean as I would have liked. Hence post-poning this when we do more refactoring work on the sdk. For instance the missing part is given a sign mode, what is the endpoint that I need to query to get authentication info (sequence, acc num), and where do I put this data? I thought of creating 2 new RPCs, one which would build an unsigned tx for you given an intent (sdk.Msgs, fees, etc) and it would return you the unsigned bytes of the TX (with filled sequence and acc num) and the expected signatures (as a combo of bytes to sign, pub key that has to sign); the other RPC would combine the unsigned bytes and the signatures and return you the signed tx. But then trust node issues appeared (how can I make sure that the node actually returned my intent as TX and not lets say send all my coins to another address), and the solution would need to bring into reflection scope auth, which would turn redundant again. This PR can be merged and this would allow clients to write on any chain, just by relying on the tx.Tx type and auth. |
…reflection' into frojdi/issue-8959-extend-cosmos-reflection
|
@aaronc I've addressed your feedbacks. The only piece which I'm kind of not happy with is the |
| // fullname is the protobuf queryable name of the interface implementer | ||
| string fullname = 1; | ||
| // type_url defines the type URL used when marshalling the type as any | ||
| // this is required so we can provide type safe google.protobuf.Any marshalling and | ||
| // unmarshalling, making sure that we don't accept just 'any' type | ||
| // in our interface fields | ||
| string type_url = 2; |
There was a problem hiding this comment.
I'm confused in general by your usage of both fullname and type_url here and in other places. From my perspective, there is only "type URL" OR "full name" in the protobuf world and we should use that consistently. There should not be cases where we have both a type URL and full name that could be different but refer to the same thing.
There was a problem hiding this comment.
@aaronc we do need both unfortunately.
When we build a protoregistry from gRPC reflection, the file descriptor will define, for example, the fullname of MsgSend as cosmos.bank.v1beta1.MsgSend, now if I want to resolve that type if it's wrapped as google.protobuf.Any I need to feed my protov2 type registry a way to give me my proto.MessageType from the given type URL which is going to be /cosmos.bank.v1beta1.Msg/Send.
Note: when I register a dynamic type from a file descriptor in the type registry, I cannot use a custom name. It's just going to take my MessageDescriptor from my FileDescriptor which defines its name.
There was a problem hiding this comment.
This is not quite right I believe. As I said before /cosmos.bank.v1beta1.Msg/Send is the type URL for the method. cosmos.bank.v1beta1.MsgSend is the type URL for the request type. So what you want to register in the Any resolver is the request type of the service method. It is incorrect to say that the " fullname of MsgSend as cosmos.bank.v1beta1.MsgSend".
There was a problem hiding this comment.
Then I'm not understanding sorry, as per ADR-031, I need to encode my MsgSend as:
any{typeURL: /cosmos.bank.v1beta1.Msg/Send, value: proto.Marshal(MsgSend)}
In the TX type, hence I need some way to feed that info when I'm creating the any type.
Same thing goes for when I need to decode a transaction, I need to have a custom resolver that returns me the MessageType (whose fullname is cosmos.v1beta1.bank.MsgSend, as per file descriptor) but is encoded as any with tpye URL: /cosmos.bank.v1beta1.Msg/Send
There was a problem hiding this comment.
This is the quote from ADR-031:
In order to encode service methods in transactions, we encode them as Anys in the same TxBody.messages field as other Msgs. We simply set Any.type_url to the full-qualified method name (ex. /cosmos.gov.Msg/SubmitProposal) and set Any.value to the protobuf encoding of the request message (MsgSubmitProposal in this case).
There was a problem hiding this comment.
Yeah, you encode and decode the request type of the method which happens to be cosmos.v1beta1.bank.MsgSend. That's the convention of ADR 031. We write rpc(RequestType) returns (ResponseType) for service methods. So as a descriptor you would have:
- method type URL
- request type URL
- response type URL
And you encode Any(TypeURL: methodTypeUrl, Value: proto.Marshal(requestType))
Does that help?
My general preference is to come up with APIs that we more or less feel good about before we commit to them... If we want to mark things as experimental and unstable, I think the right way would be a |
There was a problem hiding this comment.
I kind of share the same feeling as @aaronc. This looks like a good start, but contains some confusing parts, which doesn't make an ideal user-facing API imo. To unblock immediate work on rosetta, we can merge this as v1alpha1.
| // msg contains a descriptor of sdk.ServiceMsg, note: sdk.Msg is not supported | ||
| // as every sdk.Msg is already an sdk.ServiceMsg. It is defined as a oneof in case | ||
| // different representation of a msg will be implemented. | ||
| oneof msg { | ||
| // service_msg is used when the message is an sdk.ServiceMsg type | ||
| ServiceMsgDescriptor service_msg = 1; | ||
| } |
There was a problem hiding this comment.
Not sure about the oneof, maybe we should just use ServiceMsgDescriptor and drop legacy Msgs altogether?
| // name defines the unique name of the signing mode | ||
| string name = 1; |
There was a problem hiding this comment.
I always understood "reflection" as "a proto service returning something about the proto files themselves". So strictly speaking, every field in this file should be a proto fullname string. With this mindset, I would also put this info in the TxService.
For example, here the TxDescriptor would only return proto fullnames:
fullname string-> returncosmos.tx.v1beta1.Txservice_name string-> returncosmos.tx.v1beta1.Servicerepeated MsgDescriptor msgs-> return all the Msg Services fullnames
and other normal services can return whatever they want.
It seems that this PR understands "reflection" as "proto reflection + any chain config that's not queryable from state". Which is fine, but yeah, creates scoping issues as whether chain-id/codecs/sign_modes/bech32 belong to normal services or reflection services.
| } | ||
|
|
||
| // QueryServiceDescriptor describes a cosmos-sdk queryable service | ||
| message QueryServiceDescriptor { |
There was a problem hiding this comment.
it only adds one more thing: the tendermint query path, which can be used if a client desires to query via tendermint rather than gRPC.
But the tendermint query path (full_query_path below) is exactly the gRPC method fullname.
I would remove repeated QueryMethodDescriptor methods = 2, we just need string fullname = 1 and you can further reflection to list all methods.
| message ServiceMsgDescriptor { | ||
| // request_fullname is the protobuf fullname of the given sdk.ServiceMsg request | ||
| // this is the protobuf message type which should be used as google.protobuf.Any.value | ||
| // when delivering the msg to the DeliverTx endpoint | ||
| string request_fullname = 1; | ||
| // request_route is the sdk.ServiceMsg route, it is equal to type_url | ||
| string request_route = 2; | ||
| // request_type_url is the identifier that should be used as google.protobuf.Any.type_url | ||
| // when delivering the msg to the DeliverTx endpoint | ||
| string request_type_url = 3; | ||
| // response_fullname is the protobuf fullname of the given sdk.ServiceMsg response | ||
| string response_fullname = 4; | ||
| } |
There was a problem hiding this comment.
IMO we don't need all this. You just need one field string fullname of the whole service. Then, you can use a separate reflection to loop through its methods and query the request_fullname and the response_fullname of each method
There was a problem hiding this comment.
The configurator is not inside the BaseApp, hence we have no access to MsgServer descriptors. This is documented in the implementation.
|
@AmauryM, @aaronc, after further investigation the following are my conclusions: TypeURL (we can discuss if we want to keep the field name as type_url or call it any_type_url, to further clarify its role) is a must that must be returned when we're querying for interfaces implementations:
How is any compatibility broken? Any spec states the following: Now this aside making type resolving for any hard (which is why need to rely on typeURL field in the API proposed here), it breaks the functionalities of any itself. This is always gonna return false for sdk.ServiceMsgs even if the type we're trying to compare it to is correct. This spec is broken for each proto.Message in the sdk that returns sdk.ServiceMsg interface wrapped as any. I suggest to revisit this part in ADR-031, and maybe opt for a custom type url which includes both the method and the correct fullname of the message. As for:
I do agree, but it's too much of a struggle to propose 4 different services or changes in current APIs and then coordinate. Just agreeing on the API would block work which depends on this for months. |
|
I'm fine with reverting current changes to the reflection service and proposing this as a v2alpha1. WDYT? |
OK 👍 |
| return nil, fmt.Errorf("unable to get *tx.Tx protobuf name") | ||
| } | ||
| // get msgs | ||
| msgImplementers := ir.ListImplementations(sdk.MsgInterfaceProtoName) |
There was a problem hiding this comment.
One idea I just thought (for the future) is creating new Protobuf options used for reflection, e.g.
// cosmos.bank.v1beta1
service Msg {
option cosmos_msg_service = true;
// all methods
}
service Query {
option cosmos_query_service = true;
// all methods
}Which would avoid using ListImplementations for getting all ServiceMsgs, and could potentially be useful for reflection-less clients (like regen-js).
There was a problem hiding this comment.
Yes, having this would sure help. Although I feel the best would be that the configurator becomes part of *BaseApp and we explicitly mark query services and msg services available to the application.
aaronc
dismissed
their stale review
Dismissing my change request because the switch to an alpha package meets my needs to spend more time on API design.
…ue-8959-extend-cosmos-reflection
|
Thanks so much @sahith-narahari! I'll merge as soon as I can |
Description
closes: #8959
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes