search

cosmos / ibc-proto-rs

Rust Protobuf definitions and gRPC clients for interacting with Cosmos SDK, IBC and Interchain Security
Apache License 2.0
45 stars 26 forks source link

Include "host-functions" feature of ics23 under the features #108

Closed Farhad-Shabani closed 1 year ago

Farhad-Shabani commented 1 year ago

Background

Needed in some places of ics23_commitment within ibc-rs. So then, we can completely move away from ics23 dependency there and only import ics23 types from ibc-proto-rs.

romac commented 1 year ago

I think it should be up to ibc-rs to depend on both inc-proto and ics23 and enable that feature for the latter. Verification of commitment proofs is imho not the responsibility of this crate and should be left up to the dépendants of this crate.

Farhad-Shabani commented 1 year ago

I think it should be up to ibc-rs to depend on both inc-proto and ics23 and enable that feature for the latter. Verification of commitment proofs is imho not the responsibility of this crate and should be left up to the dépendants of this crate.

Very true, my bad! was looking at the cosmos/ics23 as the only proto types' provider, and thought of the host-functions so. Thanks for pointing that out!

Though, to clear up the point of confusion. Apart from the proto types from the ics23 crate, we can actually access bunch of other types through the ibc-proto-rs. Perhaps, ideally, we should only re-export the proto types of ics23?

romac commented 1 year ago

Though, to clear up the point of confusion. Apart from the proto types from the ics23 crate, we can actually access bunch of other types through the ibc-proto-rs. Perhaps, ideally, we should only re-export the proto types of ics23?

Good question! On one hand it's kinda handy to have these on hand for when the client code defines its own HostFunctionsProvider and therefore would not need to explicitly add in ics23 as a dependency with the host-functions feature enabled. On the other hand, as point out, it goes against the argument I was making above and conflates multiple concerns together.

In the end, I don't feel super strongly either way, but would slightly favor exporting only the protos and leaving verification up to the client.

Alternatively, we could split the protos out of ics23 and into their own crate and re export that only, but it's perhaps a bit overkill.

Farhad-Shabani commented 1 year ago

In the end, I don't feel super strongly either way, but would slightly favor exporting only the protos and leaving verification up to the client.

👍🏻

Alternatively, we could split the protos out of ics23 and into their own crate and re export that only, but it's perhaps a bit overkill.

Yes, looks unnecessary right now. Maybe to keep it in mind for future improvements.