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

Add additional derive macros to Prost generated types #409

Open
ewoolsey opened this issue Jun 27, 2023 · 19 comments · May be fixed by #471
Open

Add additional derive macros to Prost generated types #409

ewoolsey opened this issue Jun 27, 2023 · 19 comments · May be fixed by #471

Comments

@ewoolsey
Copy link

If you take a look at osmosis-std You'll see they have derived some additional traits on many of their types, which allow you get access to type urls, and request responses automatically. This is super powerful, because then you can automatically generate utility functions to send/receive messages using a client. This is what the generated code looks like:

/// QueryParamsRequest is the request type for the Query/Params RPC method.
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(
    Clone,
    PartialEq,
    Eq,
    ::prost::Message,
    ::serde::Serialize,
    ::serde::Deserialize,
    ::schemars::JsonSchema,
    CosmwasmExt,
)]
#[proto_message(type_url = "/cosmos.auth.v1beta1.QueryParamsRequest")]
#[proto_query(
    path = "/cosmos.auth.v1beta1.Query/Params",
    response_type = QueryParamsResponse
)]
pub struct QueryParamsRequest {}

The macros are already built so they could simply be used in this crate almost without changes.

@tony-iqlusion
Copy link
Member

What's generating the type URLs?

@ewoolsey
Copy link
Author

What's generating the type URLs?

I'm assuming they're just being scraped from the .proto files. Likely whatever template they're using to generate the code includes those extra derives

@tony-iqlusion
Copy link
Member

I mainly ask because I've been very much interested in getting that upstreamed to Prost: tokio-rs/prost#858

@ewoolsey
Copy link
Author

Ah yes I see that would be pretty convenient. Perhaps you can comment more on this, but I've wondered why cosmrs redefines many of the types in cosmos-idk-proto. Adding these extra derives IMO would mean that a new version of cosmrs could directly use the types from cosmos-sdk-proto rather than redefining them.

@tony-iqlusion
Copy link
Member

CosmRS defines "domain types" which wrap the protos and enforce various invariants, similar to how gogoproto is used in the upstream Cosmos SDK. It also has custom logic to handle field evolution between versions, as well as custom serializations for various types.

A similar pattern is used in tendermint-rs and ibc-rs (as well as various other Cosmos-related projects like Penumbra).

@ewoolsey
Copy link
Author

Ah gotcha! I see how that could be useful. So currently I'm maintaining a crate called cosm-utils which consists of 80% boilerplate that could be completely removed if we were able to get these response and type_url traits on all of the proto types. It would make maintaining it so much easier.

@ewoolsey
Copy link
Author

I see that cosmos_sdk_proto has a TypeUrl trait. Is there any chance we could get a Request trait that looks something like this?

pub trait Request {
    type Response: TypeUrl + MessageExt
}

This would be massively powerful. Adding a macro to make these for us wouldn't be too difficult either. What do you think?

@tony-iqlusion
Copy link
Member

@ewoolsey FWIW I'd like to move to a trait like this in the next release: tokio-rs/prost#896

@ewoolsey
Copy link
Author

@ewoolsey FWIW I'd like to move to a trait like this in the next release: tokio-rs/prost#896

Awesome yeah I just saw that PR go up. That looks like a perfect solution. Any thoughts on a Request style trait that provides a Response associated type?

@tony-iqlusion
Copy link
Member

What's the use case?

@ewoolsey
Copy link
Author

@tony-iqlusion It allows you to build queries automatically. Essentially you could write a single function like this (coded from my head please ignore errors):

impl Client {
    pub async fn query<T: Request>(&self, msg: T) -> Result<<T as Request>::Response> {
        let res = self.query(msg)?;
        let response: <T as Request>::Response = serde_json::from_bytes(res.value)?;
        Ok(response)
    }
}

You could do something similar with execute message as well, imagine a message like MsgStoreCode, the Response could be

pub struct MsgStoreCodeResponse {
    code_id: u32
}

impl TryFrom<Vec<Events>> for MsgStoreCodeResponse {
    ...
}

@tony-iqlusion
Copy link
Member

That's what tonic already does with gRPC endpoints

@ewoolsey
Copy link
Author

ewoolsey commented Aug 14, 2023

That's what tonic already does with gRPC endpoints

Yes that's true for tonic and gRPC, but not true for other clients(tendermint_rpc, http, websocket, etc.)

@tony-iqlusion
Copy link
Member

We don't even support gRPC yet. That seems like the logical place to start.

(Note: it's already implemented downstream in Ocular)

@ewoolsey
Copy link
Author

(Note: it's already implemented downstream in Ocular)

Yeah I also have support for it in the library I'm maintaining for my work. It's currently very cumbersome to maintain manually, hence the request for this trait which would make that work a breeze.

@penso
Copy link
Contributor

penso commented Sep 14, 2023

I have similar need where I'd like to get JSON version of cosmrs::bank::MsgSend and other proto related structs and having serde derives on those would make my life way easier.

What's the best way for now to do that?

@tony-iqlusion
Copy link
Member

See #83 for JSON-related discussion

@tony-iqlusion
Copy link
Member

In #432 we migrated to the new prost::Name trait.

It should eventually get first-class support in prost-build: tokio-rs/prost#926

@ewoolsey
Copy link
Author

ewoolsey commented Oct 3, 2023

In #432 we migrated to the new prost::Name trait.

It should eventually get first-class support in prost-build: tokio-rs/prost#926

This is fantastic news! Thanks for the hard work. I'll look into migrating over to this soon.

@ash-burnt ash-burnt linked a pull request Jun 1, 2024 that will close this issue
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