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

Consistently show ledger_index as integer on JSON output #4820

Merged
merged 12 commits into from
Nov 29, 2023

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Nov 17, 2023

High Level Overview of Change

Several methods in API version 1 return ledger_index as a quoted number, i.e. string type. This value should be consistently returned as a number. It was reported in XRPLF/clio#722 (comment) and I missed it in earlier changes meant for API version 2

Context 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 update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Before / After

Before:

JSON output from ledger would contain ledger as string e.g. "ledger_index": "54300932"

After

JSON output from ledger would contain ledger as number e.g. "ledger_index": 54300932

@mvadari
Copy link
Collaborator

mvadari commented Nov 20, 2023

This resolves this issue: #3652

@Bronek Bronek force-pushed the feature/unify_json_ledger_index_int branch from 761104f to 4548a30 Compare November 20, 2023 18:08
@Bronek Bronek force-pushed the feature/unify_json_ledger_index_int branch from 4548a30 to 7a1ab1f Compare November 20, 2023 18:11
@Bronek Bronek marked this pull request as ready for review November 21, 2023 09:39
May want to revert this commit, if we decide to keep it at API version 3
@HowardHinnant HowardHinnant self-requested a review November 21, 2023 17:38
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.

Looks good. I left one suggestion, but it's not a biggie.

src/ripple/app/misc/NetworkOPs.cpp Show resolved Hide resolved
@Bronek Bronek force-pushed the feature/unify_json_ledger_index_int branch from b3cb87e to ff9e616 Compare November 22, 2023 12:53
Bronek added a commit to Bronek/rippled that referenced this pull request Nov 22, 2023
@intelliot intelliot added this to the 2.1.0 (Feb 2024) milestone Nov 29, 2023
@intelliot
Copy link
Collaborator

@Bronek please update this PR so the change applies in api_version 2

@intelliot intelliot modified the milestones: 2.1.0 (Feb 2024), 2.0.0 Nov 29, 2023
@Bronek
Copy link
Collaborator Author

Bronek commented Nov 29, 2023

@Bronek please update this PR so the change applies in api_version 2

Done. Only change in change description was needed; I switched it to API version 2 last week in 62d51a0

@intelliot intelliot merged commit c045060 into XRPLF:develop Nov 29, 2023
8 checks passed
@kennyzlei
Copy link
Collaborator

We have this corresponding issue for Clio to maintain parity XRPLF/clio#1014

@Bronek @intelliot would be appreciated if we can have a rippled issue created with the API Change label in the future so we can catch these breaking API changes quicker for Clio folks to work on that side 🙏

sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
For api_version 2, always return ledger_index as integer in JSON output.

api_version 1 retains prior behavior.
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.

6 participants