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

Enable api_version 2 #4568

Closed
wants to merge 3 commits into from

Conversation

arihantkothari
Copy link
Collaborator

High Level Overview of Change

Enabling api_version 2 by bumping up apiMaximumSupportedVersion to 2 and fixing AccountInfo tests that were failing as a consequence.

Context of Change

Changing error code leads to breaking changes for error handling in applications relying on those error codes. Hence, all these changes should be upgraded to the next api_version, so that existing users get the existing behaviour, and users who request a higher api_version get the new behaviour.
More context: #4552

Type of Change

  • [x ] New feature (non-breaking change which adds functionality)
  • [ x] Tests (updated existing tests)

@intelliot intelliot added this to the 1.12 milestone Jun 20, 2023
@arihantkothari arihantkothari marked this pull request as draft June 22, 2023 21:08
@intelliot intelliot modified the milestones: 1.12, 1.13 Aug 23, 2023
@intelliot
Copy link
Collaborator

Discussed today and decided to hold this for 1.13 or later.

@arihantkothari
Copy link
Collaborator Author

Discussed today and decided to hold this for 1.13 or later.

Sorry, I missed this comment. Okay! Thanks for the update @intelliot !

@arihantkothari
Copy link
Collaborator Author

arihantkothari commented Oct 23, 2023

Leaving this comment here (this is from a while back):

Changing apiMaximumSupportedVersion to 2 would result in the failure of the account_info version 1 test because the unit tests typically operate based on apiMaximumSupportedVersion by default. This behavior is due to the calls made within the code flow, specifically Env::do_rpc -> rpcClient -> rpcCmdLineToJson -> insert_api_version, where the api_version is set to RPC::apiMaximumSupportedVersion. It appears that the command line specific function rpcCmdLineToJson is called in order to keep the unit tests aligned with the latest supported API version by default, because the command line exhibits different behavior. The question arises as to whether we should maintain this behavior or not.

Let me be clear here: this PR will still pass all the tests because I added api_version 1 in the test. But the underlying problem STILL exists.

@intelliot
Copy link
Collaborator

update: @pwang200 will take a look at this and potentially open a new PR

@arihantkothari
Copy link
Collaborator Author

Closing this PR since #4785 is now open.

@arihantkothari arihantkothari deleted the enable-api-v2 branch October 26, 2023 01:56
intelliot pushed a commit that referenced this pull request Nov 2, 2023
The command line API still uses `apiMaximumSupportedVersion`.
The unit test RPCs use `apiMinimumSupportedVersion` if unspecified.

Context:
- #4568
- #4552
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
The command line API still uses `apiMaximumSupportedVersion`.
The unit test RPCs use `apiMinimumSupportedVersion` if unspecified.

Context:
- XRPLF#4568
- XRPLF#4552
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants