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 GetSigners from sdk.Msg implementations #4687

Closed
3 tasks
damiannolan opened this issue Sep 18, 2023 · 13 comments · Fixed by #5352
Closed
3 tasks

Remove GetSigners from sdk.Msg implementations #4687

damiannolan opened this issue Sep 18, 2023 · 13 comments · Fixed by #5352
Assignees
Labels
good first issue Good for newcomers type: code hygiene Clean up code but without changing functionality or interfaces
Milestone

Comments

@damiannolan
Copy link
Contributor

Summary

The GetSigners method has been deprecated in favour of proto annotations. It can be removed.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@damiannolan damiannolan added good first issue Good for newcomers type: code hygiene Clean up code but without changing functionality or interfaces labels Sep 18, 2023
@damiannolan damiannolan moved this to Backlog in ibc-go Sep 18, 2023
@CyberGA
Copy link
Contributor

CyberGA commented Sep 19, 2023

Hi @damiannolan
I would like to work on this. Is it just to remove the GetSigners?
Am I replacing it with anything?

@damiannolan
Copy link
Contributor Author

damiannolan commented Sep 20, 2023

Hey @CyberGA, feel free to work on this issue! I can assign you. Thanks a lot!

Yes we can just remove the GetSigners method for each implementation of the sdk.Msg. The msgs are maintained under the types package of each module (transfer, interchain-accounts, 29-fee, ibc core). You can find them in the msgs.go file.

There no need to do any to replace them. The functionality has already been replaced using protobuf annotations. See here for example

@damiannolan damiannolan moved this from Backlog to In progress in ibc-go Sep 20, 2023
@CyberGA
Copy link
Contributor

CyberGA commented Sep 20, 2023

Okay I will work on it

@DimitrisJim
Copy link
Contributor

ayo @CyberGA still have time to work on this? If not, we can assign it elsewhere. An example of how this should look can be seen in linked PR.

@CyberGA
Copy link
Contributor

CyberGA commented Oct 19, 2023

ayo @CyberGA still have time to work on this? If not, we can assign it elsewhere. An example of how this should look can be seen in linked PR.

Please assign it to someone else. Thanks

@damiannolan
Copy link
Contributor Author

I can do it 👍

@ThanhNhann
Copy link
Contributor

Hey @damiannolan, do you still work on this? if you don't have time, I think I can handle it 😄

@damiannolan
Copy link
Contributor Author

@ThanhNhann be my guest! I'll assign you :)

@DimitrisJim
Copy link
Contributor

small note: we shouldn't close this issue after #5305, these should also be removed for new messages added for channel upgradability.

@hoangdv2429
Copy link
Contributor

@damiannolan @DimitrisJim sirs can I continue the work on this ?

@DimitrisJim
Copy link
Contributor

Depends on if @ThanhNhann is still working on it, if not, I don't see why not!

@ThanhNhann
Copy link
Contributor

thank @hoangdv2429 but I'm working on it, you can check other issues and solve them too 🤝

@DimitrisJim DimitrisJim linked a pull request Dec 11, 2023 that will close this issue
9 tasks
@DimitrisJim
Copy link
Contributor

completed in #5352, thank you for taking this on @ThanhNhann 💪

@github-project-automation github-project-automation bot moved this from In progress to Done in ibc-go Dec 11, 2023
@crodriguezvega crodriguezvega added this to the v9.0.0 milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Status: Done 🥳
Development

Successfully merging a pull request may close this issue.

6 participants