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

Improve validation of address with VerifyAddressFormat #677

Merged
merged 3 commits into from Jun 19, 2020

Conversation

akhilkumarpilli
Copy link
Contributor

Fixes #571

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #677 into master will decrease coverage by 0.08%.
The diff coverage is 18.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
- Coverage   30.99%   30.91%   -0.09%     
==========================================
  Files         144      144              
  Lines        6849     6858       +9     
==========================================
- Hits         2123     2120       -3     
- Misses       4620     4632      +12     
  Partials      106      106              
Impacted Files Coverage Δ
x/deployment/query/path.go 31.91% <0.00%> (-1.42%) ⬇️
x/deployment/query/types.go 0.00% <ø> (ø)
x/deployment/types/id.go 48.14% <0.00%> (-1.86%) ⬇️
x/deployment/types/msgs.go 0.00% <0.00%> (ø)
x/market/query/path.go 17.24% <0.00%> (-1.28%) ⬇️
x/market/types/id.go 61.11% <0.00%> (ø)
x/market/types/msgs.go 0.00% <0.00%> (ø)
x/provider/types/msgs.go 76.59% <100.00%> (-6.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3aacaf3...cb95c67. Read the comment docs.

@akhilkumarpilli akhilkumarpilli changed the title Improve parsing addresses with VerifyAddressFormat Improve validation of address with VerifyAddressFormat Jun 18, 2020
@akhilkumarpilli akhilkumarpilli marked this pull request as ready for review June 18, 2020 12:25
@akhilkumarpilli akhilkumarpilli requested a review from boz June 18, 2020 12:25
Copy link
Contributor

@boz boz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, thank you.

Can you make sure that there's adequate test coverage for the message validations?

See my note about the filters - I think that they can probably be left out of this PR. The semantics of querying should be:

  1. if the address is empty, don't filter on address
  2. if the address is non-empty and invalid, return an error
  3. if the address is non-empty and valid, filter on the address

x/deployment/query/types.go Outdated Show resolved Hide resolved
@akhilkumarpilli
Copy link
Contributor Author

akhilkumarpilli commented Jun 19, 2020

Looking great, thank you.

Can you make sure that there's adequate test coverage for the message validations?

See my note about the filters - I think that they can probably be left out of this PR. The semantics of querying should be:

1. if the address is empty, don't filter on address

2. if the address is non-empty and invalid, return an error

3. if the address is non-empty and valid, filter on the address

Added few more cli tests for testing filters. Also verifying given owner address and if invalid, throwing error.

This function will not change as I am validating given owner address before coming to this function.

@boz
Copy link
Contributor

boz commented Jun 19, 2020

This function will not change as I am validating given owner address before coming to this function.

Perfect, thank you.

Copy link
Contributor

@boz boz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@boz boz merged commit 1f291c4 into master Jun 19, 2020
@boz boz deleted the akhil/parse-addresses branch June 19, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants