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.

Sign up for GitHub

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

Jump to bottom

Osmosis Proto #236 #239

Closed
wants to merge 44 commits into from
Closed

Osmosis Proto #236 #239

wants to merge 44 commits into from

Conversation

Philipp-Sc
Copy link
Contributor

Hi,

this is the initial version I got working.
Please let me know if something needs be improved/fixed.

There will be quite a few improvements over the coming days, to make osmosis-proto more convenient to use.

Best regards,
Philipp

@Philipp-Sc Philipp-Sc mentioned this pull request Jun 2, 2022
Closed
@Philipp-Sc
Copy link
Contributor Author

conflict resolved in a better way important to get all types.

@Philipp-Sc
Copy link
Contributor Author

added osmosis specific cosmrs customization (custom_cosmrs) it requires one change in /cosmrs/.../lib.rs:

mod prost_ext; --> pub mod prost_ext;

this way one can add the necessary types without the need to change the main cosmrs crate.

@Philipp-Sc
Copy link
Contributor Author

regarding commit 72c14dd this needs to be merged into the main branch. (not part of osmosis-proto, I put this here to be able to continue working on cosmos-rust-bot with just using a single source for cosmos-rust and not over complicating things)

@tony-iqlusion
Copy link
Member

@Philipp-Sc can you open a separate PR for 72c14dd ?

Regarding the current dependency relationships, this may need some upstream refactoring.

Ideally osmosis-proto would only need to rely on cosmos-sdk-proto rather than also relying on cosmrs, and there shouldn't be any Osmosis-related changes to cosmos-sdk-proto or cosmrs.

chris-ricketts and others added 8 commits June 9, 2022 12:36
@Philipp-Sc
Copy link
Contributor Author

@tony-iqlusion I see, the only part that depends on cosmrs is osmosis-proto/src/custom_cosmrs/mod.rs which provides google.protobuf.Any to rust type conversion. It is needed to actually use the osmosis-proto crate. (it is enabled by this)
Another way to do it would be for cosmrs to import osmosis-proto and add the code directly into cosmrs, but that makes it harder to maintain.
Alternatively a new package could be created osmosis-rs.

Let me know what you prefer.
Best regards,
Philipp

@tony-iqlusion
Copy link
Member

I think it would make sense to move the relevant traits into cosmos-sdk-proto so there is no dependency relationship between cosmrs and osmosis-proto.

@Philipp-Sc Philipp-Sc mentioned this pull request Jun 21, 2022
Closed
@Philipp-Sc
Copy link
Contributor Author

I created an issue for it. #248
For the time being I will work on Philipp-Sc:osmosis-proto improving it for my needs.
Once issue #248 is resolved, and the main branch refactored I will follow up here.

neacsu and others added 2 commits July 22, 2022 17:03
@neacsu
cosmrs: add feegrant module support (cosmos#250)
4c4e3ea 
Expose MsgGrantAllowance, MsgRevokeAllowance, BasicAllowance and
AllowedMsgAllowance from cosmrs.
@tony-iqlusion
Bump tendermint-rs crates to v0.23.8 (cosmos#253)
2fe22e5 
This includes the following additional dependency upgrades:

- `k256` v0.11
- `ecdsa` v0.14
tony-iqlusion added a commit that referenced this pull request Jul 25, 2022
@tony-iqlusion
Move protobuf traits from cosmrs => cosmos-sdk-proto
5e8a763 
When trying to define an `osmosis-proto` crate (#239), we ran into the
problem that it needed to import `cosmrs` to be able to impl the
`MsgProto` trait.

We couldn't follow the same pattern as `cosmrs` defining the type URLs
for `cosmos-sdk-proto`, which it could only do because it defined the
`MsgProto` trait as well.

Knowledge of the type URLs is necessary to convert to/from `Any`, which
Cosmos SDK uses all over the place.

So far there isn't a good upstream solution to this problem in `prost`
or AFAICT in `tendermint-proto` either. There's an upstream tracking
issue for `prost` here:

tokio-rs/prost#299

The ideal solution to this problem seems to be adding a `TYPE_URL` to
`prost::Message`, and automatically populating them with `prost-build`.

Failing that, this commit introduces a `TypeUrl` trait with an
associated `TYPE_URL` const (previously provided by the `MsgProto`
trait).

The `from_any` and `to_any` methods have been moved to `MessageExt`.
@tony-iqlusion tony-iqlusion mentioned this pull request Jul 25, 2022
Merged
tony-iqlusion added a commit that referenced this pull request Jul 25, 2022
@tony-iqlusion
Move protobuf traits from cosmrs => cosmos-sdk-proto
0232c6e 
When trying to define an `osmosis-proto` crate (#239), we ran into the
problem that it needed to import `cosmrs` to be able to impl the
`MsgProto` trait.

We couldn't follow the same pattern as `cosmrs` defining the type URLs
for `cosmos-sdk-proto`, which it could only do because it defined the
`MsgProto` trait as well.

Knowledge of the type URLs is necessary to convert to/from `Any`, which
Cosmos SDK uses all over the place.

So far there isn't a good upstream solution to this problem in `prost`
or AFAICT in `tendermint-proto` either. There's an upstream tracking
issue for `prost` here:

tokio-rs/prost#299

The ideal solution to this problem seems to be adding a `TYPE_URL` to
`prost::Message`, and automatically populating them with `prost-build`.

Failing that, this commit introduces a `TypeUrl` trait with an
associated `TYPE_URL` const (previously provided by the `MsgProto`
trait).

The `from_any` and `to_any` methods have been moved to `MessageExt`.
tony-iqlusion and others added 19 commits July 25, 2022 16:24
@tony-iqlusion
cosmrs: document re-exports (cosmos#256)
72f3242 
The re-exports CosmRS provides are easily overlooked, and many
downstream users explicitly include `cosmrs`, `cosmos-sdk-proto`, and
`tendermint` in their Cargo.toml.

This documentation should help steer people towards using the
re-exports.
@tony-iqlusion
cosmrs: MSRV 1.57 (cosmos#257)
9eb479e 
Required by tendermint 0.23.8: cosmos#253
@tony-iqlusion
Move protobuf traits from cosmrs => cosmos-sdk-proto (cosmos#255)
5d3bed0 
When trying to define an `osmosis-proto` crate (cosmos#239), we ran into the
problem that it needed to import `cosmrs` to be able to impl the
`MsgProto` trait.

We couldn't follow the same pattern as `cosmrs` defining the type URLs
for `cosmos-sdk-proto`, which it could only do because it defined the
`MsgProto` trait as well.

Knowledge of the type URLs is necessary to convert to/from `Any`, which
Cosmos SDK uses all over the place.

So far there isn't a good upstream solution to this problem in `prost`
or AFAICT in `tendermint-proto` either. There's an upstream tracking
issue for `prost` here:

tokio-rs/prost#299

The ideal solution to this problem seems to be adding a `TYPE_URL` to
`prost::Message`, and automatically populating them with `prost-build`.

Failing that, this commit introduces a `TypeUrl` trait with an
associated `TYPE_URL` const (previously provided by the `MsgProto`
trait).

The `from_any` and `to_any` methods have been moved to `MessageExt`.
@tony-iqlusion
cosmrs: add grpc features (cosmos#258)
b9d82af 
Transitively enables gRPC support in `cosmos-sdk-proto`
@tony-iqlusion
cosmos-sdk-proto v0.13.0 (cosmos#260)
cc83b5a
@tony-iqlusion
cosmrs v0.8.0 (cosmos#261)
7c4bb6f
@tony-iqlusion
cosmrs: used badge from buildstats.info
0476181
@tony-iqlusion
cosmos-sdk-proto: remove pub from mod type_urls (cosmos#263)
ab33607 
This module contains only trait impls and is therefore empty. It was
mistakenly made `pub`.
@Philipp-Sc
Merge branch 'osmosis-proto' into main
5935473
@Philipp-Sc
Update Cargo.toml
753f8d3
@Philipp-Sc
Merge pull request #2 from Philipp-Sc/main
06ab6b8 
Merge upstream cosmos-rust
update
025ed01
update
d35233a
update
d9c2b5c
update
ff41ff0
update
f54f6bf
update
6947cae
update
acd63ef
update, removed cosmrs from Cargo.toml
28363c0
@Philipp-Sc
Copy link
Contributor Author

@tony-iqlusion I updated the branch.
Now there is no dependency relationship between cosmrs and osmosis-proto.
Note: It also includes some additional type_urls for cosmos-sdk-proto in cosmos-sdk-proto/src/type_urls.rs.

It should be ready to be merged. Let me know what you think.

Best regards,
Philipp

@Philipp-Sc
Update Cargo.toml
63d42e3 
osmosis added to default features
@tony-iqlusion
Copy link
Member

Can you open a new PR that targets the main branch, rather than the osmosis-proto branch?

I just pushed that up as an example of how to structure the crate.

@tony-iqlusion tony-iqlusion deleted the branch cosmos:osmosis-proto August 2, 2022 12:43
@Philipp-Sc Philipp-Sc mentioned this pull request Aug 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tony-iqlusion tony-iqlusion tony-iqlusion left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Philipp-Sc @tony-iqlusion @cbrit @jstuczyn @iboss-ptk @chris-ricketts @neacsu