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

Fix limit parameter for certain RPC calls #4610

Closed
wants to merge 3 commits into from
Closed

Fix limit parameter for certain RPC calls #4610

wants to merge 3 commits into from

Conversation

PeterChen13579
Copy link
Contributor

High Level Overview of Change

Fix limit error for #4540, #4541 and #4315

Context of Change

limit value should be inclusive between 10-400. Any range outside will be considered an error for api version 2 onwards.
More discussion can be found here:

Type of Change

  • [ X] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ X] Tests (You added tests for code that already exists, or your new feature included in this PR)

Added unit test that calls limit outside of valid range for api V2

@PeterChen13579 PeterChen13579 changed the title Fix limit Fix limit parameter for certain RPC calls Jul 6, 2023
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Two objections to this.

  1. Deal breaker: Don't use hard-coded values. The function has a range parameter. Use it.
  2. Opinion: As indicated by the description in Docs: clarify that limit values below 10 will be set to 10, and values above 400 will be set to 400 xrpl-dev-portal#1985, the docs need to be clarified, not the code. In rippled-land, docs reflect the code, not the other way around, except in the case of a bug. This is not a bug - it's an intentional design decision. Better to have one result with more or fewer results than requested than to have the client send a bunch of "broken" requests trying to figure out the valid range because they haven't read the docs.

@PeterChen13579
Copy link
Contributor Author

@ximinez I had a discussion with @godexsoft. He mentioned they were following the Rippled Documentation(limit) for Clio, and to change it on their side takes a lot more work than changing it on rippled's side. As rippled should match Clio, we both agreed to change it on rippled. Maybe @godexsoft can give more insight?

@ximinez
Copy link
Collaborator

ximinez commented Jul 6, 2023

@ximinez I had a discussion with @godexsoft. He mentioned they were following the Rippled Documentation(limit) for Clio, and to change it on their side takes a lot more work than changing it on rippled's side. As rippled should match Clio, we both agreed to change it on rippled. Maybe @godexsoft can give more insight?

I replied to @godexsoft's comment at XRPLF/xrpl-dev-portal#1985 (comment) with the reasons that I disagree with this decision. Most significantly, I don't think the docs say anything about needing to return an error. Clio should match rippled.

@ximinez ximinez self-requested a review July 7, 2023 20:56
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

This looks fine if we decide this is the correct solution, but I'm going to hold off approving, until a final decision is made.

@godexsoft
Copy link
Collaborator

Looks good to me too @PeterChen13579 👍 But we did end up implementing the clamping behaviour in Clio instead. Will have to discard this PR.

@PeterChen13579
Copy link
Contributor Author

Discarded PR. Clamping supported on Clio.
Reference: XRPLF/clio#740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants