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

Remove GetSignBytes with Eden upgrade #3911

Closed
3 tasks
Tracked by #3916 ...
colin-axner opened this issue Jun 21, 2023 · 6 comments · Fixed by #4570
Closed
3 tasks
Tracked by #3916 ...

Remove GetSignBytes with Eden upgrade #3911

colin-axner opened this issue Jun 21, 2023 · 6 comments · Fixed by #4570
Assignees
Labels
deprecated Issues to remove deprecated code type: code hygiene Clean up code but without changing functionality or interfaces
Milestone

Comments

@colin-axner
Copy link
Contributor

Summary

GetSignBytes is being removed as an interface as apart of the Eden SDK upgrade. It can be removed after we have updated to SDK v0.50.x

Proposal

Remove GetSignBytes function from 29-fee msgs and transfer msgs. Please do this pr after the upgrade to SDK v0.50.x has occurred to reduce changes to review in the pr which bumps the SDK version


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added type: code hygiene Clean up code but without changing functionality or interfaces deprecated Issues to remove deprecated code labels Jun 21, 2023
@tac0turtle
Copy link
Member

in 0.50 this is automatically inferred based off the proto name, but for backwards compatability we introduced a few proto annotations to recreate the same bytes as before.

Amino name annotation: https://github.com/cosmos/cosmos-sdk/blob/b990cb783a25fbcb9c95da570a7918ec29172514/proto/cosmos/auth/v1beta1/auth.proto#L15
Amino message encoding: https://github.com/cosmos/cosmos-sdk/blob/b990cb783a25fbcb9c95da570a7918ec29172514/proto/cosmos/auth/v1beta1/auth.proto#L35

We are working on documentation for this as well

This was referenced Jul 17, 2023
@crodriguezvega crodriguezvega added this to the v8.0.0 milestone Jul 17, 2023
@crodriguezvega crodriguezvega moved this to Todo in ibc-go Jul 17, 2023
@damiannolan damiannolan self-assigned this Sep 5, 2023
@damiannolan damiannolan moved this from Todo to In review in ibc-go Sep 5, 2023
@damiannolan
Copy link
Member

Looks like there is some Route() and Type() methods on these msgs left hanging around. Should we remove them as well here? What do you think?

@tac0turtle
Copy link
Member

tac0turtle commented Sep 5, 2023

they are not needed anymore

@damiannolan
Copy link
Member

Indeed! Figured as much, thanks @tac0turtle

@damiannolan
Copy link
Member

I'm not sure I understand what this documentation is recommending -> https://docs.cosmos.network/v0.50/core/encoding#authz-authorizations-and-govgroup-proposals

Should we be doing this amino codec registration on pkg init or is it not required?

@tac0turtle
Copy link
Member

its not needed anymore, we still need to clean up docs for 0.50

@damiannolan damiannolan added the v8 label Sep 6, 2023
@github-project-automation github-project-automation bot moved this from In review to Done in ibc-go Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated Issues to remove deprecated code type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants