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

Remove tx_history and ledger_header in API version 2 #4759

Merged
merged 17 commits into from
Oct 24, 2023

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Oct 11, 2023

High Level Overview of Change

Remove tx_history and ledger_header methods from API version 2. Also update RPC::Handler to allow for methods (or methods implementation) to be version specific. This partially resolves #4727 and fully resolves #3638 and #3539

Context of Change

Note design change in RPC::Handler - we can now store multiple handlers with the same name, as long as they belong to different (non-overlapping) API versions. This necessarily impacts the handler lookup algorithm and its complexity, however there is no performance loss on x86_64 architecture, and only minimal performance loss on arm64 (around 10ns). This design change gives us extra flexibility evolving API in the future, including other parts of #4727

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

Before / After

Before

In any API version tx_history and ledger_header are recognised methods

After

In API version 1 tx_history and ledger_header are a valid API method
In API version 2 tx_history and ledger_header are not recognised method; rippled will return error unknownCmd if they are called.

@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch 2 times, most recently from 1577d88 to 0124517 Compare October 12, 2023 13:44
@Bronek Bronek marked this pull request as ready for review October 12, 2023 14:06
@Bronek Bronek changed the title [DRAFT] Remove tx_history in API version 2 Remove tx_history in API version 2 Oct 12, 2023
@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch from 0124517 to dc1af06 Compare October 12, 2023 17:44
@mDuo13
Copy link
Collaborator

mDuo13 commented Oct 13, 2023

Fixes #3638

@mDuo13
Copy link
Collaborator

mDuo13 commented Oct 13, 2023

P.S. while we're at it, maybe do #3539 also?

@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch 2 times, most recently from 355cf6c to 981c606 Compare October 13, 2023 14:24
@Bronek Bronek changed the title Remove tx_history in API version 2 Remove tx_history and ledger_header in API version 2 Oct 13, 2023
@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch from 01242b7 to 2b18a03 Compare October 13, 2023 16:35
Copy link
Collaborator

@mDuo13 mDuo13 left a comment

Choose a reason for hiding this comment

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

Excited to finally get rid of some of this old cruft!

API-CHANGELOG.md Outdated Show resolved Hide resolved
API-CHANGELOG.md Outdated Show resolved Hide resolved
@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch 3 times, most recently from a636e73 to 2593b11 Compare October 13, 2023 19:10
@Bronek
Copy link
Collaborator Author

Bronek commented Oct 13, 2023

Apologies to reviewers about all these forced push, I was trying to make the micro-benchmark Handler_test.cpp little less stupid. Which happens to be first commit in the series because I use it to establish baseline ...

@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch from 5faa60c to ca63bdc Compare October 13, 2023 19:58
@intelliot intelliot added this to the 2.0 milestone Oct 16, 2023
@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch from 18503e7 to ffd95a8 Compare October 16, 2023 13:16
@intelliot
Copy link
Collaborator

Yes, I think we can always default to the most recent API version for command line. As stated here, the command line is intended for ad-hoc usage by humans, not programs or automated scripts. The command line is not meant for use in production code.

I don't think anyone is using tx_history, not even from the command line. If they are, they are welcome to later open an issue, where we can direct them to use alternative method to get the data they're looking for.

@ximinez
Copy link
Collaborator

ximinez commented Oct 19, 2023

@ckeshava Change apiMaximumSupportedVersion to 2 and rebuild. The command line uses the maximum non-beta API version.
https://github.com/XRPLF/rippled/blob/1.12.0/src/ripple/net/impl/RPCCall.cpp#L1485

@intelliot
Copy link
Collaborator

note: we intend to promote API version 2 to non-beta after #4733 is merged, and maybe after #4775; and almost certainly before the first 2.0.0-rc

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.

A few minor comments. Overall looks great.

src/ripple/rpc/handlers/LedgerHandler.h Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/Version.h Outdated Show resolved Hide resolved
src/ripple/rpc/impl/Handler.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/impl/Handler.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/impl/Handler.cpp Show resolved Hide resolved
src/test/rpc/Handler_test.cpp Outdated Show resolved Hide resolved
src/test/rpc/Handler_test.cpp Outdated Show resolved Hide resolved
src/test/basics/PerfLog_test.cpp Outdated Show resolved Hide resolved
src/test/rpc/Handler_test.cpp Show resolved Hide resolved
@intelliot
Copy link
Collaborator

@Bronek please comment here to let us know when you think all suggestions have been addressed

@Bronek Bronek force-pushed the feature/remove_tx_history_APIv2 branch from 4a9922f to dff8113 Compare October 23, 2023 14:48
@Bronek
Copy link
Collaborator Author

Bronek commented Oct 23, 2023

@Bronek please comment here to let us know when you think all suggestions have been addressed

Yes, all addressed

@Bronek
Copy link
Collaborator Author

Bronek commented Oct 23, 2023

This is ready to merge

@intelliot
Copy link
Collaborator

awaiting updated review from ximinez (verifying the changes that were made)

@Bronek
Copy link
Collaborator Author

Bronek commented Oct 23, 2023

awaiting updated review from ximinez (verifying the changes that were made)

Well in that case I will have one more minor improvement ...

@ckeshava ckeshava mentioned this pull request Oct 23, 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.

Looks good!

@intelliot
Copy link
Collaborator

Suggested commit message:

APIv2: remove tx_history and ledger_header (#4759)

Remove `tx_history` and `ledger_header` methods from API version 2.

Update `RPC::Handler` to allow for methods (or method implementations)
to be API version specific. This partially resolves #4727. We can now
store multiple handlers with the same name, as long as they belong to
different (non-overlapping) API versions. This necessarily impacts the
handler lookup algorithm and its complexity; however, there is no
performance loss on x86_64 architecture, and only minimal performance
loss on arm64 (around 10ns). This design change gives us extra
flexibility evolving the API in the future, including other parts of
#4727.

In API version 2, `tx_history` and `ledger_header` are no longer
recognised; if they are called, `rippled` will return error
`unknownCmd`

Resolve #3638

Resolve #3539

@intelliot intelliot merged commit 1eac4d2 into XRPLF:develop Oct 24, 2023
16 checks passed
@intelliot intelliot mentioned this pull request Nov 3, 2023
1 task
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
Remove `tx_history` and `ledger_header` methods from API version 2.

Update `RPC::Handler` to allow for methods (or method implementations)
to be API version specific. This partially resolves XRPLF#4727. We can now
store multiple handlers with the same name, as long as they belong to
different (non-overlapping) API versions. This necessarily impacts the
handler lookup algorithm and its complexity; however, there is no
performance loss on x86_64 architecture, and only minimal performance
loss on arm64 (around 10ns). This design change gives us extra
flexibility evolving the API in the future, including other parts of
XRPLF#4727.

In API version 2, `tx_history` and `ledger_header` are no longer
recognised; if they are called, `rippled` will return error
`unknownCmd`

Resolve XRPLF#3638

Resolve XRPLF#3539
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
6 participants