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

client: fix Paginate's arguments validation #6205

Merged
merged 3 commits into from
May 13, 2020

Conversation

alessio
Copy link
Contributor

@alessio alessio commented May 12, 2020

Check both page and defLimit against negative values.

Follow up of #6163

@auto-comment
Copy link

auto-comment bot commented May 12, 2020

👋 Thanks for creating a PR!

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.


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Thank you for your contribution to the Cosmos-SDK! 🚀

@alessio alessio changed the title Alessio/fix client paginate client: fix Paginate's arguments validation May 12, 2020
@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #6205 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6205      +/-   ##
==========================================
+ Coverage   54.85%   54.86%   +0.01%     
==========================================
  Files         444      444              
  Lines       26785    26787       +2     
==========================================
+ Hits        14693    14697       +4     
+ Misses      11049    11048       -1     
+ Partials     1043     1042       -1     

Check both page and defLimit against negative values.

Follow up of #6205
@alessio alessio force-pushed the alessio/fix-client-paginate branch from 438e893 to 9416388 Compare May 12, 2020 23:04
@alessio alessio marked this pull request as ready for review May 12, 2020 23:04
@alessio alessio added R4R and removed WIP labels May 12, 2020
@alessio alessio requested a review from colin-axner May 12, 2020 23:06

// fallback to default limit if supplied limit is invalid
if limit <= 0 {
if defLimit < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm shouldn't this be defLimit <= 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll, in case defLimit is 0 and applied as the actual limit though I think 0 results would be returned. That should be ok.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK

@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label May 13, 2020
@mergify mergify bot merged commit 9b6e694 into master May 13, 2020
@mergify mergify bot deleted the alessio/fix-client-paginate branch May 13, 2020 13:14
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants