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

Replace deprecated usage of AddressFromBech32 with address.Codec #7835

Closed
gjermundgaraba opened this issue Jan 10, 2025 · 7 comments · Fixed by #7896
Closed

Replace deprecated usage of AddressFromBech32 with address.Codec #7835

gjermundgaraba opened this issue Jan 10, 2025 · 7 comments · Fixed by #7896
Assignees
Labels
good first issue Good for newcomers

Comments

@gjermundgaraba
Copy link
Contributor

gjermundgaraba commented Jan 10, 2025

We use a few different versions of *AddressFromBech32 in the code base, which is now deprecated. This should be replaced with use of an address.Codec as per the documentation:

// Deprecated: Use an address.Codec to convert addresses from and to string/bytes.
func AccAddressFromBech32(address string) (addr AccAddress, err error) {

Replace all the instance of these deprecated functions.

Look at how this is done in the SDK for a good pointer on how it should be done here.

@gjermundgaraba gjermundgaraba added the good first issue Good for newcomers label Jan 10, 2025
@github-project-automation github-project-automation bot moved this to Backlog in IBC Jan 10, 2025
@gjermundgaraba
Copy link
Contributor Author

@duvbell, I added a link to the SDK where you should go look for the different ways they get the correct codec at any given time. It is usually attached to a client context, so you shouldn't have to create it anywhere yourself.

If you want to work on this issue I can assign it to you (that's usually what we want before a PR, so people don't overlap work).

@hungdinh82
Copy link
Contributor

@gjermundgaraba I would like to work on this issue.

@duvbell
Copy link
Contributor

duvbell commented Jan 17, 2025

helo @gjermundgaraba , i was kind of busy the last few days. Lets @hungdinh82 take on this, i will do other issues later

@hungdinh82
Copy link
Contributor

Thank you so much @duvbell.

@technicallyty
Copy link
Contributor

Would like to start a discussion on what to do forValidateBasic and other stateless methods who's interface cannot be changed, but would need access to an address codec somehow.

There may be a path where an address codec is stored in a global, but I'm unsure if that's ideal here. cc @gjermundgaraba

I don't think it would be the worst thing in the world to keep the deprecated calls around, but just wanted to flag this in case.

@gjermundgaraba
Copy link
Contributor Author

Hmm, yes, that is a tricky one. One option would be to move all the ValidateBasic logic into some other interface that we need to call from the message handlers. That is the "future" way of doing this, correct?

It would be more work, but could also be a good opportunity to make sure we have test coverage for those things in the message handlers.

The global path is something I would certainly prefer to avoid if possible.

@github-project-automation github-project-automation bot moved this from Backlog to Done in IBC Jan 29, 2025
@technicallyty
Copy link
Contributor

Hmm, yes, that is a tricky one. One option would be to move all the ValidateBasic logic into some other interface that we need to call from the message handlers. That is the "future" way of doing this, correct?

It would be more work, but could also be a good opportunity to make sure we have test coverage for those things in the message handlers.

The global path is something I would certainly prefer to avoid if possible.

yes, SDK was moving in a direction that suggested validation be done in the message servers instead of using stateless ValidateBasic checks. we could do that here if you think that would be good for the modules.

i think ICL SDK team needs to further discuss and see if thats something we want to continue pushing for cc: @aljo242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment