-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Remove tx_history and ledger_header in API version 2 #4759
Conversation
1577d88
to
0124517
Compare
0124517
to
dc1af06
Compare
Fixes #3638 |
P.S. while we're at it, maybe do #3539 also? |
355cf6c
to
981c606
Compare
01242b7
to
2b18a03
Compare
There was a problem hiding this 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!
a636e73
to
2593b11
Compare
Apologies to reviewers about all these |
5faa60c
to
ca63bdc
Compare
18503e7
to
ffd95a8
Compare
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. |
@ckeshava Change |
There was a problem hiding this 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.
@Bronek please comment here to let us know when you think all suggestions have been addressed |
4a9922f
to
dff8113
Compare
Yes, all addressed |
This is ready to merge |
awaiting updated review from ximinez (verifying the changes that were made) |
Well in that case I will have one more minor improvement ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Suggested commit message:
|
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
High Level Overview of Change
Remove
tx_history
andledger_header
methods from API version 2. Also updateRPC::Handler
to allow for methods (or methods implementation) to be version specific. This partially resolves #4727 and fully resolves #3638 and #3539Context 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 #4727Type of Change
Before / After
Before
In any API version
tx_history
andledger_header
are recognised methodsAfter
In API version 1
tx_history
andledger_header
are a valid API methodIn API version 2
tx_history
andledger_header
are not recognised method;rippled
will return errorunknownCmd
if they are called.