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

feat: add rippled API v2 support and use as default #2656

Merged
merged 57 commits into from
Jun 28, 2024

Conversation

khancode
Copy link
Collaborator

@khancode khancode commented Mar 4, 2024

High Level Overview of Change

Add rippled api_version support for v2 and use as default while maintaining support for v1.

Context of Change

  • rippled api_version 2 is supported by 100% of rippled servers now that numerous v2 amendments have activated. (as of April 8, 2024.). Therefore, we should default to using v2 and maintain support of v1 response types.
  • This feature will be released as a major bump version since it's a significant breaking change.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update HISTORY.md?

  • Yes
  • No, this change does not impact library users

Test Plan

Updated existing unit/integ tests and added new ones.

@khancode khancode changed the title add apiVersion support to requests and AccountInfoResponse v1/v2 types Add support for rippled apiVersion requests and AccountInfoResponse v1/v2 types Mar 11, 2024
@khancode khancode changed the title Add support for rippled apiVersion requests and AccountInfoResponse v1/v2 types Add support for rippled apiVersion requests and AccountInfoResponse type Mar 25, 2024
@khancode khancode changed the title Add support for rippled apiVersion requests and AccountInfoResponse type Add rippled apiVersion support for requests and AccountInfoResponse type Mar 25, 2024
@khancode khancode changed the title Add rippled apiVersion support for requests and AccountInfoResponse type feat: add rippled apiVersion support for requests and AccountInfoResponse type Mar 25, 2024
@khancode khancode changed the title feat: add rippled apiVersion support for requests and AccountInfoResponse type feat: add rippled api_version support for requests and AccountInfoResponse type Mar 27, 2024
@khancode khancode marked this pull request as ready for review March 27, 2024 19:06
@khancode khancode changed the title feat: add rippled api_version support for requests and AccountInfoResponse type feat: add rippled api_version support for v2 requests and AccountInfoResponse type Mar 27, 2024
@khancode khancode requested a review from ckeshava June 24, 2024 21:07
Copy link
Collaborator

@justinr1234 justinr1234 left a comment

Choose a reason for hiding this comment

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

  • For all your tests, shouldn't we be keeping v1 tests? I know you added some v1 tests, but you changed a lot of tests to use tx_json but I didn't see tests for making sure v1 still works
  • I think we should add a warning to people when they're trying to access v1 data but the library is defaulting to v2 -- Likely we can use a class getter than throws an error when they try to access the wrong tx or tx_json

@khancode
Copy link
Collaborator Author

  • For all your tests, shouldn't we be keeping v1 tests? I know you added some v1 tests, but you changed a lot of tests to use tx_json but I didn't see tests for making sure v1 still works
  • I think we should add a warning to people when they're trying to access v1 data but the library is defaulting to v2 -- Likely we can use a class getter than throws an error when they try to access the wrong tx or tx_json
  1. I only kept v1 tests specifically for the RPC responses that changed. I thought it would be repetitive to test for v1 in all other tests that indirectly use these RPC responses. So the v1 response tests are still covered.
  2. I don't think we shouldn't add this complexity since that's what the updated/different TS models are for and virtually all validators run v2 now.

@khancode khancode merged commit 8e2aba3 into main Jun 28, 2024
15 checks passed
@khancode khancode deleted the rippled-v2-account-info-response branch June 28, 2024 12:26
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.

6 participants