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 long-deprecated account_tx switch #2926

Closed
mDuo13 opened this issue May 6, 2019 · 5 comments
Closed

Remove long-deprecated account_tx switch #2926

mDuo13 opened this issue May 6, 2019 · 5 comments
Labels
API Change Reviewed Tech Debt Non-urgent improvements

Comments

@mDuo13
Copy link
Collaborator

mDuo13 commented May 6, 2019

(Porting this issue from Ripple's internal Jira ticket RIPD-1569)

Description

The account_tx method has an "old" mode that's been deprecated since at least 2013, with a "temporary switch" to use the old mode if people provide certain parameters (offset, count, descending, ledger_max, or ledger_min) in the request, and the new mode otherwise.

I'd say it's about time to remove the old mode.

Blocker

Per @scottschurr:

I made a run at this fix, but there's a complication. The command-line version of the account_tx command relies on the deprecated form of the RPC command. Until we come up with a new version of the command line (and figure out a transition process between the old and new forms) we're stuck with the deprecated version of the RPC command.

From a development perspective, the simplest possible change to the command line form would be to simply remove it and tell folks they must use the json command-line command to do account_tx queries. This approach requires no transition process and requires no new code.

I'm unsure just how user hostile that approach would be, however.

I welcome other suggestions.

@cjcobb23
Copy link
Contributor

@mDuo13 @scottschurr For the command line, there are really only two fields that require calling the legacy handler: count and offset. I propose we simply ignore these fields if they are present, and include a warning in the response.

The other legacy fields can be easily mapped to a non-legacy field: descending is the same as forward. ledger_min and ledger_max are the same as ledger_index_min and ledger_index_max. Since the command line version parses arguments based on position, as opposed to the fields being named, we can just interpret these legacy values as their non-legacy analogues.

@scottschurr
Copy link
Collaborator

@cjcobb23, thanks for looking at this. Your proposal seems reasonable to me. Let's see what @mDuo13 thinks.

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Feb 7, 2020

I'm all in favor of it.

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Mar 2, 2021

The switch for the legacy code was removed as part of #3609. However, some of the old code is still lingering around, e.g. https://github.com/ripple/rippled/blob/master/src/ripple/rpc/handlers/AccountTxOld.cpp

ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jul 20, 2022
ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jul 21, 2022
ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jul 21, 2022
legleux pushed a commit to legleux/rippled that referenced this issue Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Reviewed Tech Debt Non-urgent improvements
Projects
None yet
Development

No branches or pull requests

5 participants
@mDuo13 @scottschurr @carlhua @cjcobb23 and others