Thoughts on addresses#802
Conversation
It has never been done yet. And as they were working on ADR 028, care was taken to not change existing addresses. Furthermore, if a chain forks, the standard way of avoiding replay attacks is to change the chainId on one (or both) side. Since ChainID is included in the SignBytes, this chain alone avoids replay attacks, but allows existing wallets to work on either chain with the same key pair and address |
ethanfrey
left a comment
There was a problem hiding this comment.
Nice idea. A few comments to give feedback on finishing it
b0cf519 to
92cae24
Compare
|
I added all the validation features now, but removed the change from CanonicalAddr -> HumanAddr in state as I'm still unsure about the potential profix change.
What this preserves is the Wouldn't a chain change chain ID and prefix when they fork, keeping the same keypairs but different addresses? |
92cae24 to
81fd75c
Compare
One might think that would be the desired path. chain id is enough to avoid reply attacks, the bech32 prefix helps with ux confusion. However, the Cosmos SDK stores addresses as bech32 internally already (example staking), so I think we should just go with the flow and store them as bech32 strings as well. It will clearly be a major issue to change bech32 prefix and I doubt anyone would undertake it. If they did, migrating contract state should also be a feasible solution - it's not like it is that much harder than migrating everything else. (And unlikely that either happen... like everyone still uses the cosmos hub's bip44 derivation path) That said, I fully support updating this PR and getting it in as the standard way to use things. Providing Human <-> Canonical is good, but let's have some |
|
Good. I'll update this PR then and will ping you for a review when done. |
acbb132 to
c31519a
Compare
66ad164 to
2a8b36d
Compare
de45064 to
5b4f7d0
Compare
f348fa4 to
238a976
Compare
|
First glance looks good. I did not have time to review today, it is high on my list for tomorrow. Once this is in, do you want to cut a 0.14.0-beta2 so we can try this out in cosmwasm-plus? |
maurolacy
left a comment
There was a problem hiding this comment.
Particularly for newcomers, I think this new format / API will be simpler / clearer. Thanks for doing this.
| /// assumptions should be made other than being UTF-8 encoded and of reasonable length. | ||
| /// | ||
| /// This type represents a validated address. It can be created in the following ways | ||
| /// 1. Use `Addr::unchecked(input)` |
There was a problem hiding this comment.
Here, briefly detailing uses cases would be nice IMO.
ethanfrey
left a comment
There was a problem hiding this comment.
Nice stuff.
I left lots of comments (looking closely at everything) and found a few issues with the ibc contracts and one small enhancement request. Otherwise, looks good to me.
82def5e to
d0298f2
Compare
|
Good fixes. Glad to see this. |
Some thoughts on the future of addresses in CosmWasm. This prepares a renaming from
HumanAddrto justAddras the primary address type.What I learned:
info.sender-> JSON state becomes much nicerCanonicalAddrfor storage keys (such as bucket) remains possibleThinking about
HumanAddrin state again: Is it realistic that the human readable address format changes over the lifetime of a chain? Should we avoid the human readable address format in state? How do native modules handle hat?Updates: