Skip to content
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

extend cosmos reflection to provide information that would allow reflection clients to send transactions in sdk based apps #8959

Closed
fdymylja opened this issue Mar 23, 2021 · 11 comments · Fixed by #8965
Assignees

Comments

@fdymylja
Copy link
Contributor

Now that the gRPC reflection server is working we can allow clients to query any chain without codec context.

What's left now is allowing clients to write on any chain too without having context on the codec and the configuration.

This can be and should be achieved via the cosmos reflection service.

@fdymylja fdymylja changed the title extend cosmos reflection to provide information that would allow reflection clients to send transactions in the sdk extend cosmos reflection to provide information that would allow reflection clients to send transactions in sdk based apps Mar 23, 2021
@fdymylja fdymylja self-assigned this Mar 23, 2021
@aaronc
Copy link
Member

aaronc commented Mar 23, 2021

@fdymylja do you have a suggested API for this?

@fdymylja
Copy link
Contributor Author

@aaronc currently working on it, i'll have it ready in few hours for a first review

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 23, 2021

My general idea is to extend the cosmos reflection service with a new RPC endpoint called GetAppDescriptor this would return an AppDescriptor type which would provide the required information to build transactions in a reflective way.

The number of assumptions (of clients) should be minimal.

On the PoC I had built the required sdk types for building transactions were limited to the public key interface, and the tx.Tx type.

@robert-zaremba
Copy link
Collaborator

Why this is a problem? proto.Marshal(m proto.Message) works out of the box and doesn't need any context.

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 24, 2021

@robert-zaremba it doesn't work if you don't have the proto.Message to work with.

What we're proposing here is to enable clients to work without having context on all (if not any) proto.Message type or registry.

And also enable them to gather information (sequence, acc number) and sign.

You can find an example here of a PoC I did in which we create an app codec at runtime: https://github.com/cosmos/cosmos-sdk/pull/8827/files#diff-4ef9592981ad3c238542407085dc4f7bec5b3aab8fd7fe486aa3e76659230ec3R33

Then it's possible to use dynamic protobuf to interact with any chain, or even some types. This whilst providing anypb marshalling/unmarshalling safety

@robert-zaremba
Copy link
Collaborator

@fdymylja , thanks for explanation. Are you going to handle Any wrapping as well? Because now it's problematic if someone wants to interact with gRPC. He firstly needs to serialize the message and then wrap the bytes into Any with a proper type URL.

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 25, 2021

@fdymylja , thanks for explanation. Are you going to handle Any wrapping as well? Because now it's problematic if someone wants to interact with gRPC. He firstly needs to serialize the message and then wrap the bytes into Any with a proper type URL.

@robert-zaremba, during the tests that I've done, thanks to the protoregistry.Types the google.protobuf.Any types were being marshalled and unmarshalled with their concrete types. So no any wrapping, and whilst printing a jsonified Account you would see the concrete in the output and not the base64 encoded bytes as value.

The only short-coming we have right now is that anything which has a registered type URL in the protoregistry.Types can be unmarshalled as any. This is not optimal because we have only some specific types which should unmarshal to some messages containing our google.protobuf.Any interfaces.

I've added this as a supported case in the proposed API. Since in the future we plan to indicate the implementing interfaces in the tx.Tx type (for example), then I can use the protov2 custom marshal options to provide a limited type registry (which is used to fetch a proto message given it's type URL) to make sure that if a type URL does not belong to that specific google.Protobuf.Any it's gonna be rejected.

@robert-zaremba
Copy link
Collaborator

This is not optimal because we have only some specific types which should unmarshal to some messages containing our google.protobuf.Any interfaces.

Do you mean that, if we explicitly put some other type in any, then the App will panic because it won't be able to unmarshal to expected type? If we have a good error messages, then, I think, it's not bad. WDYT?

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 25, 2021

Do you mean that, if we explicitly put some other type in any, then the App will panic because it won't be able to unmarshal to expected type? If we have a good error messages, then, I think, it's not bad. WDYT?

No sorry, I think I didn't make myself clear, I'll make a concrete example.

Let's assume the reflection client consumer is rosetta API (rosetta does not have the app.Codec, so it has 0 context on anything). You tell rosetta you wanna call (call in rosetta is an arbitrary query):

a call in rosetta roughly looks like this: (json)call{ method: string, metadata: map[string]interface{} }

  • Invoked function: (gRPC method or TM query path) GetStringifiedPubKey
  • Accepted input:
    message GetStringifiedPubKeyRequest {
    
    google.protobuf.Any pub_key = 1 [(cosmos_proto.any_interface) = "crypto.PubKey"];
    }

Expected Output:

  message GetStringifiedPubKeyResposne {
  string pub_key_string = 1;
  }

Now, due to how any works, I could fill anything I want into the pub_key field, and then rely on the server to return me an error. This is fine but we don't need to rely on the server to tell us that the request is malformed because I put a proto.Message in the any which is not expected for the given request.

Instead the protov2 API (which is what we rely on for building reflection clients), provides us marshal and unmarshal options for messages. Specifically the Resolver interface:

Resolver interface {
		protoregistry.ExtensionTypeResolver
		protoregistry.MessageTypeResolver
	}

Each proto Message containing an google.protobuf.Any would have its own custom Resolver, which would then allow the client to know if a request (or response too) is malformed due to unexpected google.protobuf.Any concrete type. This is thanks to the option cosmos_proto.any_interface which would allow us to know the concrete proto messages for the crypto.PubKey interface.

@robert-zaremba
Copy link
Collaborator

Now, due to how any works, I could fill anything I want into the pub_key field, and then rely on the server to return me an error.

yes, this is what I had in mind.

Thank you for more details. Awesome initiative. There is lot of work with regards to v2 migration. So probably we will need a dedicated team for that. If we have stronger use cases then it's worth to plan for it.

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 25, 2021

Thank you @robert-zaremba. BTW, just to point this for people who might not have enough context on proto vs protov2 semantics in the sdk.

The SDK can move forward by using protov1 and gogoproto.

Reflection clients can rely on the provided proto descriptors and protov2 to work (descriptors are backwards compatible between v1 and v2).

Bonus:

Another use case: you could build a python library which uses the gRPC reflection + cosmos reflection to query and sign transactions on any chain, similar to how web3.py handles it with contracts. Example:

cosmos_app = cosmos.LoadApp(grpc_endpoint)

Then this client would build a class which would expose all the possible queries and allow you to build transactions with messages accepted by the target chain in a dynamic python way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants