hermes
hermes copied to clipboard
Rename Raw types into Proto types
Crate
mostly ibc
Summary
Looking at the code now that this new Protobuf
type is kicking in, I'm wondering if we should not adapt our practice of calling serialization types with the "Raw" prefix. Instead, we could use the "Proto" prefix or something similar. So for example we'd have the following:
impl Protobuf<ProtoClientConnections> for ConnectionIds {}
instead of https://github.com/informalsystems/ibc-rs/blob/9963c4a0f275eef401dcefa6b0f159dda93fe2b4/modules/src/ics02_client/raw.rs#L18
The idea is to make the connection between tendermint_proto::Protobuf
and the underlying type more explicit. It's mostly aesthetics, but might make the codebase simpler to read & understand.
Originally posted by @adizere in https://github.com/informalsystems/ibc-rs/pull/364#discussion_r526342477
For Admin Use
- [X] Not duplicate issue
- [X] Appropriate labels applied
- [X] Appropriate milestone (priority) applied
- [ ] Appropriate contributors tagged
- [ ] Contributor assigned/self-assigned
Is now a good time for me to go for this one? It could create some pretty bad merge conflicts if other big PRs are currently being worked on.
This issue is mostly aesthetic and for resolving a drift in our terminology. I would be in favor of going forward with the modifications, it would be nice to have this sometime before v1.
That being said, I had a closer look through the codebase and we never define any domain type to have the 'raw' prefix. We only rename upon importing various protobuf-types, example:
https://github.com/informalsystems/ibc-rs/blob/01c2983ab8b005d8194518176da6fcb827ca2ed0/modules/src/core/ics04_channel/msgs/chan_open_try.rs#L11
We also use it as an associated type in a case: https://github.com/informalsystems/ibc-rs/blob/37d54d4d851d3d7af394845e05b17b4d0e66afd7/modules/src/tx_msg.rs#L7
But for the most part, the 'raw' prefix appears peppered in our error subtypes, example:
- https://github.com/informalsystems/ibc-rs/blob/241c1846d180aad5d6a03f66e142575411c07fc8/modules/src/core/ics02_client/error.rs#L120
- https://docs.rs/ibc/0.12.0/ibc/core/ics03_connection/connection/index.html?search=raw
It could create some pretty bad merge conflicts if other big PRs are currently being worked on.
I agree working on this can bring significant conflicts, so let's try to be mindful of critical PRs on the modules side, in particular this one: https://github.com/informalsystems/ibc-rs/pull/1838. It would be a good idea to work on the present issue after we're done merging #1838.
Closing as stale.