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.

Sign up for GitHub

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

Jump to bottom

remove deprecated fields in ledger command, fix for #3214 #4244

Merged
merged 3 commits into from
Jun 28, 2023
Merged

remove deprecated fields in ledger command, fix for #3214 #4244

merged 3 commits into from
Jun 28, 2023

Conversation

ckeshava
Copy link
Collaborator

@ckeshava ckeshava commented Jul 20, 2022 •
edited by mDuo13
Loading

I removed the following fields from the ledger command - accepted, seqNum, hash and totalCoins.

Some of these variable names (totalCoins, seqNum) are used in managing the database but I don't think there is any redundancy in the creation/updation/deletion of the tables.

(Note: Edited to correct seqNum field name. --@mDuo13)

@ckeshava ckeshava closed this Jul 20, 2022
@ckeshava ckeshava reopened this Jul 20, 2022
@nbougalis
Copy link
Contributor

I don't know if we can just remove the deprecated field; someone might still be relying on it. Do we know how long it's been marked DEPRECATED for? @mDuo13, any objections?

@mDuo13
Copy link
Collaborator

mDuo13 commented Nov 28, 2022

Per #3214, the fields have been marked as deprecated dating back to at least 2015. I think we can just remove them.

intelliot
intelliot approved these changes Jan 12, 2023
View reviewed changes
Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

I'm fine with just removing these, but let's get at least one more review.

@intelliot intelliot requested a review from mtrippled February 14, 2023 03:33
gregtatcam
gregtatcam reviewed Feb 20, 2023
View reviewed changes
Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

It looks ok but jss::hash is listed in the ledger_request documentation: https://xrpl.org/ledger_request.html.

@intelliot intelliot requested review from seelabs and ledhed2222 April 6, 2023 23:21
@intelliot
Copy link
Collaborator

@mDuo13 any thoughts on jss::hash?

@mDuo13
Copy link
Collaborator

mDuo13 commented Apr 18, 2023

The hash field in ledger_request should stay (for now). That command has its own problems, but I don't think it uses the shared ledger deserialization that this ticket is about.

@intelliot intelliot requested review from gregtatcam and removed request for seelabs April 20, 2023 15:18
@intelliot intelliot requested a review from drlongle May 25, 2023 05:55
@intelliot intelliot added this to the 1.12 milestone May 25, 2023
@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 25, 2023
@intelliot
Copy link
Collaborator

This is slated for inclusion in 1.12 (not 1.11) unless anyone comments here to request otherwise.

@intelliot intelliot merged commit 0e98352 into XRPLF:develop Jun 28, 2023
ckeshava added a commit to ckeshava/rippled that referenced this pull request Jul 10, 2023
@ckeshava
fix: remove deprecated fields in ledger method (XRPLF#4244)
284bc4a 
Remove deprecated fields from the ledger command:
* accepted
* hash (use ledger_hash instead)
* seqNum (use ledger_index instead)
* totalCoins (use total_coins instead)

Update SHAMapStore unit tests to use `jss:ledger_hash` instead of the
deprecated `hash` field.

Fix XRPLF#3214
@intelliot intelliot mentioned this pull request Jul 18, 2023
Merged
1 task
@intelliot intelliot mentioned this pull request Aug 1, 2023
Closed
ckeshava added a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
@ckeshava
fix: remove deprecated fields in ledger method (XRPLF#4244)
301e27c 
Remove deprecated fields from the ledger command:
* accepted
* hash (use ledger_hash instead)
* seqNum (use ledger_index instead)
* totalCoins (use total_coins instead)

Update SHAMapStore unit tests to use `jss:ledger_hash` instead of the
deprecated `hash` field.

Fix XRPLF#3214
@mDuo13 mDuo13 mentioned this pull request Sep 22, 2023
Closed
@intelliot
Copy link
Collaborator

Released in 1.12.0.

ckeshava added a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
@ckeshava
fix: remove deprecated fields in ledger method (XRPLF#4244)
9c58f61 
Remove deprecated fields from the ledger command:
* accepted
* hash (use ledger_hash instead)
* seqNum (use ledger_index instead)
* totalCoins (use total_coins instead)

Update SHAMapStore unit tests to use `jss:ledger_hash` instead of the
deprecated `hash` field.

Fix XRPLF#3214
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drlongle drlongle drlongle approved these changes

@ledhed2222 ledhed2222 ledhed2222 approved these changes

@intelliot intelliot intelliot approved these changes

@mDuo13 mDuo13 Awaiting requested review from mDuo13

@nbougalis nbougalis Awaiting requested review from nbougalis

@mtrippled mtrippled Awaiting requested review from mtrippled

@gregtatcam gregtatcam Awaiting requested review from gregtatcam

Assignees

@gregtatcam gregtatcam

@mDuo13 mDuo13

Labels
API Change Clio Reviewed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Tech Debt Non-urgent improvements Testable Will Need Documentation
Projects
Core Ledger
Archived in project
Milestone
1.12
Development

Successfully merging this pull request may close these issues.
7 participants
@ckeshava @nbougalis @mDuo13 @intelliot @gregtatcam @ledhed2222 @drlongle