EPIC: remove global bech32

[tac0turtle]

Summary

Currently bech32 prefixes is a global in the cosmos-sdk and for applications as well. We should aim to remove most if not all globals within the sdk.

Problem Definition

Globals should be removed.

Work Breakdown

• replace account bech32 calls with calls into address codec for the prefix(s)
• replace validator key bech32 calls
• remove sdk.bech32 from types and outside modules
• Remove address verifier from global config, put it as argument to NewAccountKeeper
• Support cosmos.msg.v1.signer as alternative to Msg.GetSigners #11275
• Add a way for clients to get the bech32 prefix
• Go through every module and remove calls to .String() (this may require state migrations)
  • auth
  • authz
  • bank
  • feegrant
  • gov
  • groups
  • nft
  • distribution
  • staking
  • slashing
  • upgrade
• Deprecate all .String calls on addresses
• remove the need to set global codecs

Notes:

• do not call address.String() (SDK should not call address.String()  #8332)

Is there any other work we should mention here?

ref #8332

[faddat]

@marbar3778 this is an epic epic. I am a huge fan.

if I get time might look at closing it or seeing if we've got someone at Notional feeling up to the task.

[itsdevbear]

app.ModuleBasics another global.

[alexanderbez]

ModuleBasics is fine IMO

[robert-zaremba]

In #1533 I read about account.Codec. Copying here my comment about reusing codec.Codec for AccAddress.

We can combine account.Codec with codec.Codec. We could have a rule there for AccAddress type (as well for other types, VaAddress).

[itsdevbear]

How do we feel about deprecating bech32 entirely and just using byte addresses (maybe EIP-55), whats the advantage to the human readable prefix bech32, many nontechnical users find it confusing that their address changes when they switch chains and is a huge confusion point from many members in our community.

[yihuang]

Bech32 could be a purely client-side or rpc side thing, no need to exist in chain state I think. We can use raw bytes in state, but convert to bech32 in rpc.

[tac0turtle]

Personally i think we should kill accaddress type as well. It should all be bytes. The decode and encode are handled by the address codec. We shouldnt combine it with the codec.codec since they do two separate things and some modules dont need codec.codec.

Bech32 or any address encoding is only an client layer thing, it shouldnt be used in state

[alexanderbez]

Bech32 could be a purely client-side or rpc side thing, no need to exist in chain state I think. We can use raw bytes in state, but convert to bech32 in rpc.

This is already the case. Bech32 isn't used in the state-machine. Modules store bytes, not strings. Bech32 is used for clients and tests primarily.

[aaronc]

It is used in the state machine in so much as transaction addresses are encoded in bech32 and need to be decoded in modules. I agree with @yihuang that we should evaluate making it client side only, but as an option. Removing it from the state machine entirely would be too breaking for clients. But we could add support in sign mode textual for rendering bytes addresses as bech32. Any thoughts @AmauryM ?

[alexanderbez]

That only happens in message validation, the state machine does not rely on any string interpretations.

[aaronc]

That only happens in message validation, the state machine does not rely on any string interpretations.

Message validation is part of the state machine. But in addition, all msg servers decode bech32 addresses. Just look at any msg server implementation - even the simplest bank msgServer.Send. It calls sdk.AccAddressFromBech32 twice

[alexanderbez]

Yes, but they're never read or written to/from storage is what I meant.

[aaronc]

Yes they are usually not used in storage, but some chains might store them.

[yihuang]

Bech32 could be a purely client-side or rpc side thing, no need to exist in chain state I think. We can use raw bytes in state, but convert to bech32 in rpc.

This is already the case. Bech32 isn't used in the state-machine. Modules store bytes, not strings. Bech32 is used for clients and tests primarily.

There are several sdk modules store bech32 into the state, I did a search in the change set file:

$ pigz -d -c staking/block-7000001.zz | strings | grep crc | head -n 1
1crcvaloper1efw0q0ggzxtmuf80wpcgr77h70c3avpdt6zv2l
$ pigz -d -c acc/block-7000001.zz | strings | grep crc | head -n 1
*crc1qgjqzzaz64nllgq5yghdjcx3lfpm3j8p2cy428
$ pigz -d -c slashing/block-7000001.zz | strings | grep crc | head -n 1
1crcvalcons1r6svmr6jnkt27mjzv3ggs2cj5wcfurnz2kuaxl
$ pigz -d -c feegrant/block-7000001.zz | strings | grep crc | head -n 1
*crc1gp7w5j69nzrrsnnjx0938gz5w74lkn3zdh7axr

[alexanderbez]

I both stand corrected and am very disappointed. Thanks for pointing that out @yihuang!

[tac0turtle]

dam this was my worry, we will have to approach this issue based on where it is used. Some places like staking could only be removed in a staking v2 design.

[alexanderbez]

We'll need migrations then. Which shouldn't be hard. Just take string to bytes. Still a bummer though.

[aaronc]

Why is it a problem that they're stored in state though? It doesn't affect our ability to remove the global.

[tac0turtle]

We'll need migrations then. Which shouldn't be hard. Just take string to bytes. Still a bummer though.

main issue is how long migrations would take, but something we can test.

Why is it a problem that they're stored in state though? It doesn't affect our ability to remove the global.

if a user wants to use something other than bech32 then they still need to use bech32 for this layer.

[aaronc]

if a user wants to use something other than bech32 then they still need to use bech32 for this layer.

as long as the modules storing string addresses in state use address.Codec, the encoding should be swappable on a per-chain basis

[JulianToledano]

Hey guys!!

A lot of String calls have been removed from x packages. Some interfaces may need to be refactored to remove internal calls to it, such as ValidateBasic and I'm still looking for any calls to Stringer that may have been missed.

However, my current concern is the String method, has a cache for Bech32 addresses. I think it makes sense to create an address codec with a cache to maintain performance.

@tac0turtle

[tac0turtle]

yea the cache makes sense. for the breaking of interfaces i think we can leave them on the global for now. We have plans on changing things in the future so would prefer to avoid breaking things for users

[tac0turtle]

closing this, the cache is still being worked on but this is a nice to have performance improvement