Thoughts on addresses #802

webmaster128 · 2021-02-25T23:31:33Z

Some thoughts on the future of addresses in CosmWasm. This prepares a renaming from HumanAddr to just Addr as the primary address type.

What I learned:

info.sender -> JSON state becomes much nicer
JSON state -> query response becomes much nicer
Using CanonicalAddr for storage keys (such as bucket) remains possible
Address validation works, but is easier to forget than before

Thinking about HumanAddr in 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:

Closes Improved address handling #856

ethanfrey · 2021-02-26T13:01:21Z

Thinking about HumanAddr in 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?

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

Nice idea. A few comments to give feedback on finishing it

contracts/hackatom/src/contract.rs

contracts/hackatom/tests/integration.rs

packages/std/src/traits.rs

webmaster128 · 2021-03-02T08:38:29Z

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.

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

What this preserves is the data part of a bech32 address, not its prefix and encoded representation. Addresses are stored as 20 bytes in a lot of places in the Cosmos SDK.

Wouldn't a chain change chain ID and prefix when they fork, keeping the same keypairs but different addresses?

ethanfrey · 2021-03-30T12:30:46Z

Wouldn't a chain change chain ID and prefix when they fork, keeping the same keypairs but different addresses?

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 validate(String) -> HumanAddr method as well and use that in the sample contracts

webmaster128 · 2021-03-30T17:29:35Z

Good. I'll update this PR then and will ping you for a review when done.

packages/std/src/query.rs

ethanfrey · 2021-04-06T20:27:20Z

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?
The update has already started: CosmWasm/cw-plus#260

maurolacy

Particularly for newcomers, I think this new format / API will be simpler / clearer. Thanks for doing this.

CHANGELOG.md

MIGRATING.md

maurolacy · 2021-04-07T08:08:10Z

packages/std/src/addresses.rs

+/// 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)`


Here, briefly detailing uses cases would be nice IMO.

packages/std/src/coins.rs

packages/std/src/errors/system_error.rs

packages/std/src/mock.rs

packages/vm/src/backend.rs

ethanfrey

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.

MIGRATING.md

contracts/hackatom/schema/state.json

contracts/hackatom/src/contract.rs

contracts/ibc-reflect-send/src/contract.rs

packages/std/src/mock.rs

packages/std/src/traits.rs

packages/std/src/types.rs

packages/vm/src/imports.rs

contracts/ibc-reflect/src/contract.rs

ethanfrey · 2021-04-07T15:59:03Z

Good fixes. Glad to see this.

webmaster128 requested a review from ethanfrey February 25, 2021 23:31

webmaster128 marked this pull request as draft February 25, 2021 23:31

ethanfrey reviewed Feb 26, 2021

View reviewed changes

contracts/hackatom/src/contract.rs Outdated Show resolved Hide resolved

contracts/hackatom/src/contract.rs Show resolved Hide resolved

contracts/hackatom/tests/integration.rs Outdated Show resolved Hide resolved

packages/std/src/traits.rs Outdated Show resolved Hide resolved

webmaster128 force-pushed the primary-address branch from b0cf519 to 92cae24 Compare March 2, 2021 08:33

webmaster128 force-pushed the primary-address branch from 92cae24 to 81fd75c Compare March 2, 2021 09:40

webmaster128 force-pushed the primary-address branch 2 times, most recently from acbb132 to c31519a Compare March 30, 2021 21:25

webmaster128 mentioned this pull request Mar 31, 2021

Improved address handling #856

Closed

5 tasks

webmaster128 force-pushed the primary-address branch 6 times, most recently from 66ad164 to 2a8b36d Compare March 31, 2021 16:51

ethanfrey reviewed Mar 31, 2021

View reviewed changes

packages/std/src/query.rs Show resolved Hide resolved

webmaster128 added 10 commits April 1, 2021 20:50

Split scripts in check/test workspace

0cee708

Sort tests

5e377fd

Add addr_validate and addr_ prefixed humanize/canonicalize

1d5ca28

Let addr_validate return StdResult<()>

7f93bcd

Let addr_canonicalize accept &str inputs

d81d404

Let addr_validate return HumanAddr

3aeaafe

Rename imports to addr_canonicalize/addr_humanize

0224fa5

Update testing contracts

09a3fe0

Add addr_validate import

d42a2ce

Use addr_validate import

98cd3a3

webmaster128 force-pushed the primary-address branch from de45064 to 5b4f7d0 Compare April 1, 2021 19:09

webmaster128 added 10 commits April 6, 2021 14:11

Replace HumanAddr with String in BankQuery

9d3fdd9

Replace HumanAddr with String in WasmQuery

d45dcd5

Replace HumanAddr with String in StakingQuery

e6a7943

Make adresses in FullDelegation/Delegation/Validator Addr

b134ccd

Change address types in CosmosMsg

44aa457

Simplify mock_info tests

ad6d0bd

Implement PartialEq for Addr and String

89c1b99

Let Api::addr_humanize return Addr

7aa03ab

Deprecarte HumanAddr

58be130

Add MIGRATING entry

238a976

webmaster128 force-pushed the primary-address branch from f348fa4 to 238a976 Compare April 6, 2021 12:11

webmaster128 requested review from ethanfrey and maurolacy April 6, 2021 12:12

webmaster128 added 3 commits April 6, 2021 14:17

Bring back state schemas for hackatom

0d24683

Bring back state schema for reflect

91d1f90

Restore state schema for staking contract

b84f5a6

maurolacy approved these changes Apr 7, 2021

View reviewed changes

ethanfrey reviewed Apr 7, 2021

View reviewed changes

webmaster128 added 5 commits April 7, 2021 14:52

Avoid unnecessary as_ref() for Addr == Addr and Addr == String

c9e1377

Fix and improve MIGRATING text

ec6a16f

Fix typo in NoSuchContract::addr comment

8eb6d4d

Make remote_addr a String in ibc-reflect-send

d9b4283

Implement From<&Addr> for String and From<&Addr> for HumanAddr

d0298f2

webmaster128 force-pushed the primary-address branch from 82def5e to d0298f2 Compare April 7, 2021 14:26

Add doc comment to Addr::unchecked

e5271a9

webmaster128 added the automerge See mergify.io label Apr 7, 2021

mergify bot merged commit 0413557 into main Apr 7, 2021

mergify bot deleted the primary-address branch April 7, 2021 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thoughts on addresses #802

Thoughts on addresses #802

webmaster128 commented Feb 25, 2021 •

edited

Loading

ethanfrey commented Feb 26, 2021

ethanfrey left a comment

webmaster128 commented Mar 2, 2021

ethanfrey commented Mar 30, 2021 •

edited

Loading

webmaster128 commented Mar 30, 2021

ethanfrey commented Apr 6, 2021 •

edited

Loading

maurolacy left a comment

maurolacy Apr 7, 2021

ethanfrey left a comment

ethanfrey commented Apr 7, 2021

Thoughts on addresses #802

Thoughts on addresses #802

Conversation

webmaster128 commented Feb 25, 2021 • edited Loading

ethanfrey commented Feb 26, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

webmaster128 commented Mar 2, 2021

ethanfrey commented Mar 30, 2021 • edited Loading

webmaster128 commented Mar 30, 2021

ethanfrey commented Apr 6, 2021 • edited Loading

maurolacy left a comment

Choose a reason for hiding this comment

maurolacy Apr 7, 2021

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey commented Apr 7, 2021

webmaster128 commented Feb 25, 2021 •

edited

Loading

ethanfrey commented Mar 30, 2021 •

edited

Loading

ethanfrey commented Apr 6, 2021 •

edited

Loading