Include vendored proto definitions in the proto_descriptor file #4425
Conversation
There was a problem hiding this comment.
This is a slick change, I'm bullish on merge.
It's possible that judicious use of vendoring here may affect downstream consumers of our protos: I'm thinking in particular of interchaintest pulling in copies of buf.build/penumbra-zone and also its own imports of the cosmos deps. In order to evaluate behavior there, we need to press on with merging, pushing to buf, and re-pulling in those other repos, which is too long of a round trip to block merge on now, given the priority on unblocking the Skip IBC work.
| @@ -338,6 +338,8 @@ impl Opt { | |||
| .add_service(tonic_web::enable(stake_query_proxy)) | |||
| .add_service(tonic_web::enable(compact_block_query_proxy)) | |||
| .add_service(tonic_web::enable(tendermint_proxy_proxy)) | |||
| // TODO: should we add the IBC services here as well? they will appear | |||
| // in reflection but not be available. | |||
There was a problem hiding this comment.
As a general rule, I'd say any services that can be exposed in both pd and pclientd, meaning they'll work equally well in both contexts, probably should be. Otherwise we're starting down a path where there are odd discrepancies that are hard to document. Not worth blocking merge here though, and I appreciate you adding this TODO.
|
Confirmed that the new IBC methods are exposed via reflection by testing locally: whereas they are not present on main: N.B. the older |
Describe your changes
This changes the proto build process to include cosmos/ibc-go proto files in the vendored proto file list used to generate the proto_descriptor file.
As-implemented, there are some duplicate paths that appear both as dependencies of the penumbra protos and in ibc-go. The ibc-go protos will be included first so the penumbra protos take priority. This shouldn't be an issue in the way we currently use the protos but this seems important to document for future reference.
Issue ticket number and link
Closes #4422
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: