R4R: Bech32 Empty Addresses #2601

sunnya97 · 2018-10-26T07:11:24Z

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-10-26T07:22:17Z

Codecov Report

Merging #2601 into develop will increase coverage by 0.08%.
The diff coverage is 81.57%.

@@             Coverage Diff             @@
##           develop    #2601      +/-   ##
===========================================
+ Coverage     57.3%   57.39%   +0.08%     
===========================================
  Files          179      179              
  Lines        14125    14143      +18     
===========================================
+ Hits          8094     8117      +23     
+ Misses        5546     5531      -15     
- Partials       485      495      +10

cwgoes

utACK, 🌱

cwgoes

One concern. If we want to keep it this way we have to ensure these cases are handled elsewhere.

cwgoes · 2018-10-28T21:31:21Z

types/address.go

@@ -55,6 +56,10 @@ func AccAddressFromHex(address string) (addr AccAddress, err error) {

 // AccAddressFromBech32 creates an AccAddress from a Bech32 string.
 func AccAddressFromBech32(address string) (addr AccAddress, err error) {
+	if len(strings.TrimSpace(address)) == 0 {


Are we sure we want to do this direction too (decoding empty strings as valid bech32-addresses)? That seems like it could lead to UX confusion or griefing (tricking users into sending coins to empty addresses, for example).

Good point. @sunnya97 what's the new empty address in bech32 ?

That should be caught by the ValidateBasic of the Msgs.

I feel like its reasonable to make all msg types ensure that the address is tmhash.size digest in their validate basics.

Really, that seems kinda annoying, why not ensure it at the type level instead?

Lets make a new issue for this though, its basically entirely independent of this PR.

I don't quite follow - I'm concerned about this particular addition, which will accept "" (the empty string) as a valid bech32-address. Previously that wasn't the case, and any address when de-serialized in GetFromBech32 would be automatically checked for the correct length and checksum. Addresses can still be []byte, that's not my concern.

i think the more underlying problem is that check tx may pass a tx with invalid address. That should ideally be caught asap. Then any built tx would fail in it's check tx phase.

Can we not just do the length check in these function calls (TM address length)?

I think we should return an error for invalid addresses here, rather than returning an invalid AccAddress{}.
For simplicity we should use a single global constant tmhash.TruncatedSize (definitely not 32 bytes, but 20).
And if the length is invalid, this function should return an error immediately.

cwgoes · 2018-10-28T21:31:29Z

types/address.go

@@ -169,6 +178,10 @@ func ValAddressFromHex(address string) (addr ValAddress, err error) {

 // ValAddressFromBech32 creates a ValAddress from a Bech32 string.
 func ValAddressFromBech32(address string) (addr ValAddress, err error) {
+	if len(strings.TrimSpace(address)) == 0 {


Same as above.

cwgoes · 2018-10-28T21:31:36Z

types/address.go

@@ -284,6 +301,10 @@ func ConsAddressFromHex(address string) (addr ConsAddress, err error) {

 // ConsAddressFromBech32 creates a ConsAddress from a Bech32 string.
 func ConsAddressFromBech32(address string) (addr ConsAddress, err error) {
+	if len(strings.TrimSpace(address)) == 0 {


Same as above.

ValarDragon

LGTM

fedekunze · 2018-10-30T12:13:39Z

I think i'd be helpful (eg. for burning) to add sdk.Bech32EmptyAddr and sdk.Bech32EmptyConsPubKey as constants to types/address.go. Or same as solidity something like address(0)

ValarDragon · 2018-10-30T15:12:04Z

I don't think that should be done here, or that burning should be done like that. We don't need total burned in state forever. Let's introduce things for burning after we've decided on how to do it.

alexanderbez

utACK

jaekwon

Needs to return error for anything that isn't 20 bytes.

fedekunze · 2018-11-15T15:50:56Z

status of this PR ?

alexanderbez · 2018-11-15T18:32:24Z

Comments need to be addressed :-/

sunnya97 · 2018-11-16T00:03:15Z

Needs to return error for anything that isn't 20 bytes.

@jaekwon Why is that something we want enforced by the Address type? If the .Address() function of a PubKey returns something of a different size, isn't that fine?

cwgoes · 2018-11-16T11:26:37Z

@jaekwon Why is that something we want enforced by the Address type? If the .Address() function of a PubKey returns something of a different size, isn't that fine?

Because we use these functions to sanitize user input.

sunnya97 · 2018-11-17T01:10:54Z

Shouldn't user input should be sanitized by where the input is received? Why should the SDK itself enforce a specific Address size, maybe different apps want different Address lengths or something.

cwgoes · 2018-11-19T11:54:49Z

Shouldn't user input should be sanitized by where the input is received? Why should the SDK itself enforce a specific Address size, maybe different apps want different Address lengths or something.

Maybe different address sizes are a useful option - then the address size could be part of the application configuration, like the bech-prefixes already are. I still think we should sanitize addresses in the bech32-from-string functions, it substantially reduces the possibility of accidentally unsanitized input.

ValarDragon · 2018-11-19T16:43:28Z

different address sizes are a useful option

They should totally be a thing imo. I feel kinda convinced that they should ideally be 21 or 22 bytes lol.

alexanderbez · 2018-11-19T17:01:57Z

I don't see a reason to allow flexibility here directly. They should be a fixed size as @cwgoes and @ValarDragon suggested. If other applications need different address/account types, they should be able to define and use those modularly. No?

ValarDragon · 2018-11-19T17:10:31Z

I think its reasonable to have multiple sized addresses in one chain, not sure why that would need to be restricted. For example z-addr's are 96 bytes, whilst t-addr's are 36 bytes in their bech32 forms.

cwgoes · 2018-11-20T10:11:07Z

I think its reasonable to have multiple sized addresses in one chain, not sure why that would need to be restricted. For example z-addr's are 96 bytes, whilst t-addr's are 36 bytes in their bech32 forms.

Seems a bit niche but sure - if they're different sizes, though, they should definitely have different bech32-forms and thus different bech32-conversion functions, so the bech32-conversion functions can still check the size (AFAIK that's the only debate we're having right now).

rigelrozanski · 2018-11-29T01:18:33Z

have Jae's comments been addressed? This PR looks close

I haven't kept up with this PR

cwgoes · 2018-12-04T15:25:57Z

Have Jae's comments been addressed? This PR looks close.

Looks like not yet.

jaekwon · 2018-12-05T09:18:52Z

Can we make MarshalJSON and UnmarshalJSON allow for nil & empty addresses, AND make the functions AccAddressFromBech32 etc expect a non-nil/empty address? I think that would be the best of both worlds.

I want structures to have nil/empty addresses sometimes. MarshalJSON/UnmarshalJSON should support that. Yet the function "AccAddressFromBech32" sounds like it expects a valid Bech32, and "" is not a valid Bech32.

I don't really like the idea of having .String() return a "". Even a []byte{} or []byte(nil) returns "[]" when formatted... .String() return "" can be confusing because rather than showing something descriptive like "<nil>" or whatever, it's returning nothing.

I'd prefer that .String() return "<nil>" and that we use another function like .Bech32() that panics when the address is nil/empty. The user would first check that the address is valid via .Valid() before calling .Bech32() or .String().

Validate methods would have to manually check whether an address is .Valid() or not, but that's OK... it should, because address can be nil in some contexts, and not in others.

fedekunze · 2018-12-11T11:12:18Z

I agree with @jaekwon's comments. I still think an empty address .String() should be set to something like sdk.EmptyAddr = cosmos10000000000000000...

cwgoes · 2018-12-12T12:31:28Z

@sunnya97 Did we come to consensus on what to do here?

sunnya97 · 2018-12-17T15:08:10Z

@cwgoes I'm not sure...

@jaekwon I don't think I understood your post.

I don't really like the idea of having .String() return a "".

Next paragraph:

I'd prefer that .String() return "" ...

uhhhhh 😅😅 😅 😅 😅

I still think an empty address .String() should be set to something like sdk.EmptyAddr = cosmos10000000000000000...

We want the string/bech32 of an empty/nil sdk.AccAddress to be empty string, because then in JSONs, URL query params, etc, you can drop the param. Let's say to get all the redelegations, instead of having to do GET /stake/redelegations?delegator=cosmos10000000000000000&from_validator=cosmos10000000000000000&to_validator=cosmos10000000000000000, it can just be GET /stake/redelegations. Because omission == empty string.

Also, keep in mind, it wouldn't be cosmos10000000000000000000 as that wouldn't be valid bech32. It would actually have to be cosmos100000000000000000002sdj93 (or something, didn't actually check what actual bech32 checksum of the 0 address is 😛). Keep in mind, this whole PR started because the current string of the empty address is cosmos1550dq7, which we thought was ugly/confusing/took up space when it could be empty.

fedekunze · 2019-01-12T00:04:37Z

This PR has 3 mo already... any updates?

sunnya97 · 2019-01-13T06:58:12Z

@jaekwon

jaekwon · 2019-01-17T18:57:31Z

Oops it was formatting. Fixed to say "<nil>". I think that's preferable to a special nil bech32 address which can still be confusing... e.g. would users be forced to type/paste "cosmos100000000000000000002sdj93" when they mean to say that no address exists? Or that "cosmos100000000000000000002sdj93" be displayed to represent that no address exists? That doesn't seem good. I'd rather we only represent nil addresses by their absence in struct fields, printable/debuggable via .String() returning "<nil>" but not going the other way around, e.g. "<nil>" shouldn't parse to a nil address.

cwgoes · 2019-02-05T18:44:01Z

Bump @sunnya97.

jackzampolin · 2019-02-08T20:39:10Z

types/address.go

@@ -203,6 +207,10 @@ func ValAddressFromBech32(address string) (addr ValAddress, err error) {
 		return nil, err
 	}

+	if len(bz) != AddrLen {
+		return nil, errors.New("Incorrect address length")


can this common error live somewhere?

cwgoes

Changes look fine, but we should update all the == nil checks to instead call Empty(), and see if we need any additional Empty() checks where addresses should not be empty.

Comments addressed, I think - we decided not to go with ""

dsgnsupporter · 2019-02-08T22:42:27Z

sunnya97 requested review from cwgoes, ebuchman and rigelrozanski as code owners October 26, 2018 07:11

sunnya97 changed the title ~~WIP: Bech32 Empty Addresses~~ R4R: Bech32 Empty Addresses Oct 27, 2018

sunnya97 force-pushed the sunny/bech32_empty_address branch from 5a1aad9 to 63cb867 Compare October 27, 2018 00:08

sunnya97 added the ready-for-review label Oct 27, 2018

sunnya97 added 2 commits October 26, 2018 17:09

empty addresses

2720883

PENDING

bdce019

sunnya97 force-pushed the sunny/bech32_empty_address branch from 63cb867 to bdce019 Compare October 27, 2018 00:09

sunnya97 assigned jackzampolin, cwgoes and fedekunze Oct 28, 2018

cwgoes approved these changes Oct 28, 2018

View reviewed changes

cwgoes suggested changes Oct 28, 2018

View reviewed changes

ValarDragon previously approved these changes Oct 30, 2018

View reviewed changes

alexanderbez approved these changes Oct 31, 2018

View reviewed changes

ValarDragon mentioned this pull request Oct 31, 2018

Make address [addressHash.Size]byte instead of []byte #2650

Closed

jaekwon previously requested changes Nov 6, 2018

View reviewed changes

Merge branch 'develop' into sunny/bech32_empty_address

cef9ea2

fixed addr len

2039080

asdf

0090817

merged develop

0f92f6a

jackzampolin reviewed Feb 8, 2019

View reviewed changes

cwgoes suggested changes Feb 8, 2019

View reviewed changes

cwgoes added this to the v0.31.0 (Launch RC) milestone Feb 8, 2019

works?

7cdcc7a

cwgoes approved these changes Feb 8, 2019

View reviewed changes

cwgoes merged commit da0426a into develop Feb 8, 2019

cwgoes deleted the sunny/bech32_empty_address branch February 8, 2019 23:54

odeke-em mentioned this pull request Jul 17, 2020

types: *FromBech32 functions return zero value and nil error on the blank string yet consumers blindly use them to search #6760

Closed

4 tasks

R4R: Bech32 Empty Addresses #2601

R4R: Bech32 Empty Addresses #2601

Conversation

sunnya97 commented Oct 26, 2018 • edited Loading

codecov bot commented Oct 26, 2018 • edited Loading

Codecov Report

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

fedekunze commented Oct 30, 2018

ValarDragon commented Oct 30, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

jaekwon left a comment

Choose a reason for hiding this comment

fedekunze commented Nov 15, 2018

alexanderbez commented Nov 15, 2018

sunnya97 commented Nov 16, 2018

cwgoes commented Nov 16, 2018

sunnya97 commented Nov 17, 2018

cwgoes commented Nov 19, 2018 • edited Loading

ValarDragon commented Nov 19, 2018

alexanderbez commented Nov 19, 2018 • edited Loading

ValarDragon commented Nov 19, 2018

cwgoes commented Nov 20, 2018

rigelrozanski commented Nov 29, 2018

cwgoes commented Dec 4, 2018

jaekwon commented Dec 5, 2018 • edited Loading

fedekunze commented Dec 11, 2018 • edited Loading

cwgoes commented Dec 12, 2018

sunnya97 commented Dec 17, 2018 • edited Loading

fedekunze commented Jan 12, 2019

sunnya97 commented Jan 13, 2019

jaekwon commented Jan 17, 2019 • edited Loading

cwgoes commented Feb 5, 2019

Choose a reason for hiding this comment

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

dsgnsupporter commented Feb 8, 2019

sunnya97 commented Oct 26, 2018 •

edited

Loading

codecov bot commented Oct 26, 2018 •

edited

Loading

cwgoes commented Nov 19, 2018 •

edited

Loading

alexanderbez commented Nov 19, 2018 •

edited

Loading

jaekwon commented Dec 5, 2018 •

edited

Loading

fedekunze commented Dec 11, 2018 •

edited

Loading

sunnya97 commented Dec 17, 2018 •

edited

Loading

jaekwon commented Jan 17, 2019 •

edited

Loading

cwgoes left a comment •

edited

Loading