cosmos-sdk-proto: add serde derive macros #471
Conversation
|
Be warned that |
|
@romac whatever is used to solve informalsystems/tendermint-rs#1445 we can use here too |
|
@ash-burnt now that #482 is merged, if you rebase there should be a way forward here |
cosmos-sdk-proto/Cargo.toml
Outdated
| tendermint-proto = "0.39" | ||
| serde = "1.0.203" | ||
| pbjson = { git = "https://github.com/informalsystems/pbjson.git", package = "informalsystems-pbjson" } | ||
| pbjson-types = { git = "https://github.com/informalsystems/pbjson.git", package = "informalsystems-pbjson-types" } |
There was a problem hiding this comment.
Instead of these, use https://docs.rs/tendermint-proto/latest/tendermint_proto/google/protobuf/index.html
There was a problem hiding this comment.
Done, the extern_paths have been updated to point to the new tendermint rs version, and these deps have been removed
proto-build/src/main.rs
Outdated
| const COSMOS_SDK_REV: &str = "v0.47.10"; | ||
|
|
||
| /// The Cosmos ibc-go commit or tag to be cloned and used to build the proto files | ||
| const IBC_REV: &str = "v3.0.0"; | ||
|
|
||
| /// The wasmd commit or tag to be cloned and used to build the proto files | ||
| const WASMD_REV: &str = "v0.29.2"; | ||
| const WASMD_REV: &str = "v0.45.0"; |
There was a problem hiding this comment.
Can you bump these in a separate PR? There's already quite a bit going on in this one
There was a problem hiding this comment.
I have rolled back the version updates
| pub config: ::core::option::Option<::tendermint_proto::google::protobuf::Any>, | ||
| pub config: ::core::option::Option<::pbjson_types::Any>, |
There was a problem hiding this comment.
These should stay as they are. Instead of using pbjson-types, you can leverage the newly added pbjson support in tendermint-proto (which solves every downstream crate having to figure out this problem)
There was a problem hiding this comment.
After updating the extern_paths, these now generate as they did before
Co-authored-by: Tony Arcieri (iqlusion) <tony@iqlusion.io>
|
@ash-burnt looks like there are conflicts that need resolved |
|
@tony-iqlusion they should be addressed now |
|
Looks like these serde generations still require pbjson for serialization and deserialization of non-proto values. Seems like numbers and binary types |
|
@ash-burnt yes, the (informalsystems-) |
Co-authored-by: Tony Arcieri (iqlusion) <tony@iqlusion.io>
Co-authored-by: Tony Arcieri (iqlusion) <tony@iqlusion.io>
|
Aah, so the problem isn't I think we're going to need to add a feature for this and have it enable the Either that, or we're going to need to add a set of imports to the top of each of the generated |
|
What should the feature be called? |
|
Sure, that's fine |
|
Went with "serialization" since it can't conflict with an import name |
cosmos-sdk-proto/Cargo.toml
Outdated
|
|
||
| [features] | ||
| default = ["grpc-transport"] | ||
| std = ["prost/std", "tendermint-proto/std"] | ||
| grpc = ["std", "tonic"] | ||
| grpc-transport = ["grpc", "tonic/transport"] | ||
| cosmwasm = [] | ||
| serialization = ["serde", "tendermint-proto/std", "pbjson"] |
There was a problem hiding this comment.
There's a lot of serialization going on in this crate, both protobuf and now JSON.
How about:
| serialization = ["serde", "tendermint-proto/std", "pbjson"] | |
| serde = ["dep:serde", "tendermint-proto/std", "pbjson"] |
There was a problem hiding this comment.
Done, and adjusted the code gen
tony-iqlusion
changed the title
|
@ash-burnt needs rustfmt but otherwise looks good |
|
@tony-iqlusion formatted, let me know if there is anything else that needs doing |
| // add the feature flag to the serde definitions | ||
| ( | ||
| "impl serde::Serialize for", | ||
| "#[cfg(feature = \"serde\")]\n\ | ||
| impl serde::Serialize for", | ||
| ), | ||
| ( | ||
| "impl<'de> serde::Deserialize<'de> for", | ||
| "#[cfg(feature = \"serde\")]\n\ | ||
| impl<'de> serde::Deserialize<'de> for", | ||
| ), |
There was a problem hiding this comment.
These can probably be gated on the module level but I don't see any reason to block the PR over that
Hello,
I am currently trying to use cosmos protos for storing information inside of cosmwasm smart contracts, but the existing definitions don't have the required Serialize and Deserialize traits. This should fix #409 and #83
I have extended the proto-build buf configuration to run an additional neoeinstein plugin to generate these traits.