hermes icon indicating copy to clipboard operation
hermes copied to clipboard

Rename Raw types into Proto types

Open adizere opened this issue 4 years ago • 2 comments

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

adizere avatar Nov 26 '20 14:11 adizere

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.

plafer avatar Mar 10 '22 16:03 plafer

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.

adizere avatar Mar 14 '22 09:03 adizere

Closing as stale.

adizere avatar Sep 30 '22 10:09 adizere