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

Proto: camelCase to snake_case #1656

Merged
merged 11 commits into from
Jun 7, 2022
Merged

Proto: camelCase to snake_case #1656

merged 11 commits into from
Jun 7, 2022

Conversation

hieuvubk
Copy link
Contributor

@hieuvubk hieuvubk commented Jun 2, 2022

Closes: #701

Change camelCase to snake_case in gamm proto

@hieuvubk hieuvubk requested a review from a team June 2, 2022 21:25
@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Jun 2, 2022
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK (did a quick skim)

Thank you so much for helping :)

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, ty!!

@ValarDragon
Copy link
Member

We should check with @pyramation about whether this breaks js before merging tho

@hieuvubk hieuvubk changed the title Issue 701 Proto: camelCase to snake_case Jun 3, 2022
@ValarDragon
Copy link
Member

Can you add a breaking change entry in the changelog? (Also in the future can you fill out the PR template please!)

Btw, also added you to the 'notional' github team, so in the future you can branch off the repo directly

@pyramation
Copy link

utACK as well! @Thunnini can you take a look to verify this will change amino encoding on FE? I have a pretty good idea of the implications but have not tested.

This change would like require we update FE amino converter implementations (to be more like how it's done with cosmos package with underscore<->camel instead of camel<->camel). So in a sense, this is a good move in the right direction, and how other packages seem to be doing it.

@hieuvubk
Copy link
Contributor Author

hieuvubk commented Jun 4, 2022

Can you add a breaking change entry in the changelog? (Also in the future can you fill out the PR template please!)

Btw, also added you to the 'notional' github team, so in the future you can branch off the repo directly

thank u, i edited changelog

@pyramation
Copy link

thanks for updating all of these! super helpful. Let's coordinate a migration plan with @Thunnini @jonator for v9 or v10 so we can test this with FE.

@hieuvubk
Copy link
Contributor Author

hieuvubk commented Jun 5, 2022

@ValarDragon Va @pyramation
I just gone through proto files and encountered some special cases in the txfees module
Ảnh chụp Màn hình 2022-06-05 lúc 13 04 04
Ảnh chụp Màn hình 2022-06-05 lúc 13 04 25
Neither camelCase nor snake_case, so any change will affect to other code. What do u think?

@pyramation
Copy link

@ValarDragon Va @pyramation I just gone through proto files and encountered some special cases in the txfees module Ảnh chụp Màn hình 2022-06-05 lúc 13 04 04 Ảnh chụp Màn hình 2022-06-05 lúc 13 04 25 Neither camelCase nor snake_case, so any change will affect to other code. What do u think?

I don't believe we're using those fields in FE yet — would it make sense to rename those to fee_token and pool_id?

@hieuvubk
Copy link
Contributor Author

hieuvubk commented Jun 5, 2022

@ValarDragon Va @pyramation I just gone through proto files and encountered some special cases in the txfees module Ảnh chụp Màn hình 2022-06-05 lúc 13 04 04 Ảnh chụp Màn hình 2022-06-05 lúc 13 04 25 Neither camelCase nor snake_case, so any change will affect to other code. What do u think?

I don't believe we're using those fields in FE yet — would it make sense to rename those to fee_token and pool_id?

Oh i mean in chain code

@hieuvubk hieuvubk merged commit a1ca194 into osmosis-labs:main Jun 7, 2022
@ValarDragon
Copy link
Member

ValarDragon commented Jun 7, 2022

Hey, this probably shouldn't have been merged yet, adjusting perms to improve this in the future. (My bad on hitting approve)

Going to make a follow-up issue, to ensure we have a plan for how significant is this breaking change, and if we should revert it or bundle it with another large breaking change in the future.

@hieuvubk
Copy link
Contributor Author

hieuvubk commented Jun 7, 2022

my bad, should we revert to previous commit?

@ValarDragon
Copy link
Member

Eh, its not that huge of a deal.

Going to make the issue for us to decide in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The proto structs for Balancer should use snake_case
4 participants