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.

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

Rename Raw types into Proto types #423

Closed
3 of 5 tasks
adizere opened this issue Nov 26, 2020 · 3 comments
Closed
3 of 5 tasks

Rename Raw types into Proto types #423

adizere opened this issue Nov 26, 2020 · 3 comments
Labels
A: good-first-issue Admin: good for newcomers O: code-hygiene Objective: cause to improve code hygiene

Comments

@adizere
Copy link
Member

adizere commented Nov 26, 2020

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 informalsystems/ibc-rs#364 (comment)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added the O: code-hygiene Objective: cause to improve code hygiene label Nov 26, 2020
@adizere adizere added this to the v0.0.6 milestone Nov 26, 2020
@adizere adizere modified the milestones: v0.0.6, v0.0.7 Dec 23, 2020
@adizere adizere modified the milestones: v0.0.7, v0.0.8 Jan 6, 2021
@ancazamfir ancazamfir modified the milestones: v0.1.1, v0.1.2 Feb 11, 2021
@plafer
Copy link
Contributor

plafer commented Mar 10, 2022

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.

@adizere
Copy link
Member Author

adizere commented Mar 14, 2022

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:

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 adizere modified the milestones: Backlog, v1.0.0 Mar 14, 2022
@adizere adizere moved this to Backlog in IBC-rs: the road to v1 Mar 14, 2022
@romac romac added the A: good-first-issue Admin: good for newcomers label Mar 25, 2022
@adizere adizere modified the milestones: v1.0.0, v2 Apr 28, 2022
@adizere adizere modified the milestones: v2, Backlog Jun 29, 2022
@adizere
Copy link
Member Author

adizere commented Sep 30, 2022

Closing as stale.

@adizere adizere closed this as completed Sep 30, 2022
@adizere adizere removed this from the Backlog milestone Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: good-first-issue Admin: good for newcomers O: code-hygiene Objective: cause to improve code hygiene
Projects
None yet
Development

No branches or pull requests

4 participants