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

APIv2(account_info): handle invalid "signer_lists" value #4585

Merged
merged 5 commits into from
Jun 30, 2023
Merged

APIv2(account_info): handle invalid "signer_lists" value #4585

merged 5 commits into from
Jun 30, 2023

Conversation

PeterChen13579
Copy link
Contributor

High Level Overview of Change

Signer_list with invalid input should have error'ed out. Fixed this issue in this PR. (fix for issue #4539)

Context of Change

For API version 2, if signer_list has a value not of type boolean, it will return invalidParam error

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)

Refer to Github link above.

Added own unit test to test for signer_list in API v2

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for adding the test! I did leave one nit comment, but I'm good with this pull request either way. Thanks for the contribution!

Comment on lines 129 to 130
// assigning any string value works. Do not allow this
// This for api v2 onwards
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tiniest of nits: The comment says "... allow this This for ...". Consider removing the capitalized "This".

// The document states that signer_lists is a bool, however
// assigning any string value works. Do not allow this
// This for api v2 onwards
if (!params[jss::signer_lists].isBool() && context.apiVersion > 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to check the context.apiVersion here? We assume that all the validators are running the latest version of the rippled codebase. Otherwise, the laggards will be incompatible with the rest of the network right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a situation where we don't need to worry about validators very much, at least on the main net. This is an RPC query. It's not typical for main net validators to be required also handle very many RPC queries.

The context.api can be a value passed in by the caller as shown in the test where it says "\"api_version\": 2. So, yes, we should check the api_version. It can be different from one RPC call to the next.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see okay, thanks for the clarification 👍
I'm guessing the version of rippled software is preserved in a different variable right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the software version (e.g., 1.12.0-b1) is elsewhere. The code never needs to check the software version because it is compiled into the binary with whatever software version it has. The api_version is a different kind of beast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see, okay

@@ -127,7 +127,7 @@ doAccountInfo(RPC::JsonContext& context)

// The document states that signer_lists is a bool, however
// assigning any string value works. Do not allow this
// This for api v2 onwards
// this for api v2 onwards
Copy link
Collaborator

Choose a reason for hiding this comment

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

😄 Close. If you read the sentence, it now reads "... allow this this for ...". The word "this" appears two times in a row. Now that neither of the instances of "this" are capitalized feel free to remove either one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I meant to write "this is for api v2 onwards". I just updated the comment for more clarity. Hope the updated one is fine 😄

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

The revised comment looks great. Still 👍

@intelliot intelliot merged commit f18c6df into XRPLF:develop Jun 30, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Jul 10, 2023
When requesting `account_info` with an invalid `signer_lists` value, the
API should return an "invalidParams" error.

`signer_lists` should have a value of type boolean. If it is not a
boolean, then it is invalid input. The response now indicates that.

* This is an API breaking change, so the change is only reflected for
  requests containing `"api_version": 2`
* Fix XRPLF#4539
@intelliot intelliot mentioned this pull request Jul 18, 2023
1 task
@intelliot intelliot changed the title account_info with Invalid signer list returns invalid params APIv2(account_info): handle invalid "signer_lists" value Jul 25, 2023
intelliot added a commit that referenced this pull request Jul 25, 2023
intelliot added a commit that referenced this pull request Sep 19, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
When requesting `account_info` with an invalid `signer_lists` value, the
API should return an "invalidParams" error.

`signer_lists` should have a value of type boolean. If it is not a
boolean, then it is invalid input. The response now indicates that.

* This is an API breaking change, so the change is only reflected for
  requests containing `"api_version": 2`
* Fix XRPLF#4539
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
When requesting `account_info` with an invalid `signer_lists` value, the
API should return an "invalidParams" error.

`signer_lists` should have a value of type boolean. If it is not a
boolean, then it is invalid input. The response now indicates that.

* This is an API breaking change, so the change is only reflected for
  requests containing `"api_version": 2`
* Fix XRPLF#4539
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants