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

Convert sdk.AccAddress to be a string, not a []byte #10838

Closed
4 tasks
sunnya97 opened this issue Dec 25, 2021 · 12 comments
Closed
4 tasks

Convert sdk.AccAddress to be a string, not a []byte #10838

sunnya97 opened this issue Dec 25, 2021 · 12 comments

Comments

@sunnya97
Copy link
Member

Summary

This way it will keep the bech32 prefix information, rather than stripping it out. This will be massively helpful for building out features like bech32-ibc (https://github.com/osmosis-labs/bech32-ibc)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ValarDragon
Copy link
Contributor

Honestly I think sdk.AccAddress should be a struct, with a method for GetBech32Address, GetRawAddress, and some private fields for raw_address and bech32_address for caching purposes.

@robert-zaremba
Copy link
Collaborator

@ValarDragon there is a big refactor planned together with global bech32 removal.

The Bech32* functions require a config, so those will be rather some factory method

@robert-zaremba
Copy link
Collaborator

@sunnya97 could you explain bech32ibc ?

@sunnya97
Copy link
Member Author

sunnya97 commented Jan 3, 2022

bech32ibc is primarily a modified bank keeper that allows sends to an address with a bech32 prefix different than the chain's native one, to automatically be converted to an IBC transfer to the right chain. It's the implementation of the proposal started here: #9948

I'd like this to be better generalized, however, to be usable for things other than just bank sends. For example, other modules / contracts should also be able to make use of it.

@aaronc
Copy link
Member

aaronc commented Jan 3, 2022

I think there's maybe some mixing of separate concerns here. An AccAddress is primarily for x/auth to perform public key-like authentication so it is naturally prefix free - i.e. related to the pubkey not bech32. Having x/bank store balances for local (x/auth) and x/ibc addresses is a separate concern, and strings addresses already could be used for that if this is the desired solution (although I have my doubts because of the limitations of IBC -> prefix mapping). A struct wouldn't work directly because you need some way to construct a prefix key in the store.

@robert-zaremba
Copy link
Collaborator

I thought we can implement #9948 as:

  1. a gov managed (or, in a simple version, managed through software upgrade) mapping of bech32 prefix -> IBC channels
  2. update to the RPC Msg/Send (it would need to call IBC module if the receiver prefix is different then the chain prefix - with parameters queried using data from 1.) without changing the low level keeper interface.

@sunnya97
Copy link
Member Author

sunnya97 commented Jan 4, 2022

@robert-zaremba this is what the bech32-ibc implementation does

However, my point is more just the notion that using prefix-free addresses through the SDK, especially in the bank module is messy because we're moving towards a world where the prefix is a meaningful information-carrying part of an address.

@sunnya97
Copy link
Member Author

sunnya97 commented Jan 4, 2022

@aaronc well AccAddress is used everywhere throughout the SDK, not just in auth. For example, all the Bank Keeper's function interfaces use AccAddress as inputs. I think we should still use a special type, not just a plain string, so that it can assert the validity of bech32 encoding for example.

@aaronc
Copy link
Member

aaronc commented Jan 4, 2022

Either way there are still two separate types - address string (with prefix) and address bytes (without prefix). I have thought about adding a type alias for an address string and methods could use that instead of bytes.

@robert-zaremba
Copy link
Collaborator

So, for proof of concept, of having a bigger type containing both an IBC ID (eg: bech32 prefix, channel ID...) and the address bytes, do you have an idea where we can use it to experiment? Currently in SDK, it seams that all keepers are happy with bytes only (AccAddress).

@tac0turtle
Copy link
Member

we are working on this in the auth wroking group. let me know if youd like to join

@tac0turtle
Copy link
Member

we spent some time removing bech32 global and using the address codec. This treats addresses as string or bytes, the default string form is bech32 but it can be swapped out fairly easily. Closing this issue, please reopen it sunny if you think we should do something else and we can discuss

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

No branches or pull requests

5 participants