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

Return error when the address is an empty string #6907

Merged
merged 3 commits into from
Aug 4, 2020

Conversation

dauTT
Copy link
Contributor

@dauTT dauTT commented Jul 31, 2020

Description

closes: #6760


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #6907 into master will decrease coverage by 0.04%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master    #6907      +/-   ##
==========================================
- Coverage   61.82%   61.78%   -0.05%     
==========================================
  Files         520      520              
  Lines       32129    32147      +18     
==========================================
- Hits        19865    19861       -4     
- Misses      10656    10676      +20     
- Partials     1608     1610       +2     

@alexanderbez alexanderbez added A:automerge Automatically merge PR once all prerequisites pass. C:Types labels Jul 31, 2020
@alexanderbez
Copy link
Contributor

ACK @dauTT, but we're 100% going to have to fix some tests.

@jgimeno
Copy link
Contributor

jgimeno commented Jul 31, 2020

Looks good! As @alexanderbez mentioned some tests fail now, should not be a big issue to solve them.

@dauTT
Copy link
Contributor Author

dauTT commented Aug 1, 2020

In many places we do allow to use nil address in a query like in this case:

image

nil address have actually well define empty string representation:

image

In the end the tests fails because the unmarshalling of empty string address does not work anymore:

image

So it seems, after all, not a bad idea to allow empty string in AccAddressFromBech32. What do you think?

@alexanderbez
Copy link
Contributor

AccAddressFromBech32 should return an error if the string is empty. "" is not a valid address.

@dauTT
Copy link
Contributor Author

dauTT commented Aug 4, 2020

Some tests are failing, but I think they are not related to this PR.

@alexanderbez alexanderbez merged commit f11d052 into cosmos:master Aug 4, 2020
@dauTT dauTT deleted the issue-6760 branch August 4, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Types
Projects
None yet
3 participants