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

[v11] [Decision needed] Do we revert the proto snake case changes #1696

Closed
3 tasks
ValarDragon opened this issue Jun 7, 2022 · 7 comments
Closed
3 tasks

Comments

@ValarDragon
Copy link
Member

ValarDragon commented Jun 7, 2022

Background

In PR #1656 we fixed legacy issues we had, with certain fields being camelCase when they should have been snake case the whole time. However this changes the .proto files.

It did not change the generated go fields, but it does change the generated typescript code that front-ends use at the moment.

We need to decide:

  1. Is this worth being a breaking change for frontends
  2. If so do we do it in v11
  3. Is it a Typescript codegen bug that we don't enforce a particular casing? (So then this is a bug fix)

Acceptance Criteria

  • Understand how significant of a break this is
  • Decide on do we do it evenetually or not
  • Decide on if it goes into v10 or a future upgrade

cc @czarcas7ic @pyramation @jonator

@pyramation
Copy link

Seems that we should eventually aim for this based on the Protocol Buffers naming conventions:

Use CamelCase (with an initial capital) for message names – for example, SongServerRequest.

Use underscore_separated_names for field names (including oneof field and extension names) – for example, song_name.

Given current events, If v10 is very soon, it could make more sense to try and get into v10 and if not v11 to have space allow FE to work on more pressing issues.

@pyramation
Copy link

From what I looked at with @Thunnini and @mattverse I learned that if you run the make proto-all proto gen command in osmosis, it should generate a bunch of files name like protofilename.pb.go that shows what the changes are that the FE should have to account for.

I'm pretty sure that we have to update the FE everywhere we call this.base.sendMsgs with a aminoMsgs array for all messages. I think @jonator may have already put a lot of the message construction in a single package after the refactor which could make this more simple.

@ValarDragon ValarDragon changed the title [v10] [Decision needed] Do we revert the proto snake case changes [v11] [Decision needed] Do we revert the proto snake case changes Jun 14, 2022
@alexanderbez
Copy link
Contributor

IMO we should follow Protobuf convention for field names and stick with snake-case.

@pyramation
Copy link

One thought about the fact that this was already merged — it may be best to revert so it doesn't block other features that need to ship from main? I think it's safer and then we merge back in when we're ready and tested.

@mattverse
Copy link
Member

Its highly preferred not to switch back and forth with the protos as they can be fragile to state breaking changes.
However, if there's a final decision made between camel case and snake case, I think it should be no biggie to revert the changes.

@p0mvn p0mvn added the Stale label Jul 15, 2022
@p0mvn
Copy link
Member

p0mvn commented Jul 15, 2022

Based on my understanding of the discussion here:

The agreement is to use the protobuf convention where messages are camelCase and fields are underscore_case.

Look like we already follow this convention in our .proto files. Is there anything that needs to be decided here? Can this be closed?

@github-actions github-actions bot removed the Stale label Jul 16, 2022
@mattverse
Copy link
Member

mattverse commented Jul 18, 2022

This can be closed. AFAIK, we had a PR for changing the protos to follow protobuf convention

@p0mvn p0mvn closed this as completed Jul 19, 2022
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants