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

fixPreviousTxnID: add sfPreviousTxnID/sfPreviousTxnLgrSequence to all ledger objects #4751

Merged
merged 28 commits into from
Apr 18, 2024

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Oct 5, 2023

High Level Overview of Change

This PR adds sfPreviousTxnID/sfPreviousTxnLgrSequence as fields to all ledger objects that don't already have them (except LedgerHashes, because that seems a bit redundant and also those objects aren't modified via transaction).

The affected objects are:

  • DirectoryNode
  • Amendments
  • FeeSettings
  • NegativeUNL
  • AMM

Context of Change

It makes it a lot easier to go through the history of a ledger object. Related to #4602.

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)

Test Plan

Tests have been added to ensure that everything behaves as-is prior to the amendment and transitions seamlessly when the amendment is activated.

@mvadari mvadari changed the title fix amendment for adding sfPreviousTxnID/sfPreviousTxnLgrSequence to all ledger objects amendment fix to add sfPreviousTxnID/sfPreviousTxnLgrSequence to all ledger objects Oct 5, 2023
@mvadari mvadari marked this pull request as ready for review November 2, 2023 19:29
@mvadari mvadari requested a review from ximinez November 2, 2023 19:29
@mvadari
Copy link
Collaborator Author

mvadari commented Nov 2, 2023

@Silkjaer @RichardAH @nixer89 @dangell7 this is ready for review now.

@intelliot intelliot added this to the 2.1.0 (Mar 2024) milestone Jan 31, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

Attention: Patch coverage is 90.19608% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 76.97%. Comparing base (c88166e) to head (c8af202).

Files Patch % Lines
src/test/ledger/Directory_test.cpp 92.68% 0 Missing and 3 partials ⚠️
src/ripple/ledger/impl/ApplyStateTable.cpp 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4751   +/-   ##
========================================
  Coverage    76.97%   76.97%           
========================================
  Files         1129     1129           
  Lines       131914   131959   +45     
  Branches     39629    39687   +58     
========================================
+ Hits        101539   101574   +35     
- Misses       24459    24505   +46     
+ Partials      5916     5880   -36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvadari
Copy link
Collaborator Author

mvadari commented Mar 19, 2024

Proposed commit message:

fix amendment to add `PreviousTxnID`/`PreviousTxnLgrSequence`

This amendment, `fixPreviousTxnID`, adds `PreviousTxnID` and
`PreviousTxnLgrSequence` as fields to all ledger objects that did
not already have them included (`DirectoryNode`, `Amendments`,
`FeeSettings`, `NegativeUNL`, and `AMM`). This makes it much easier
to go through the history of these ledger objects.

@mvadari mvadari requested a review from ximinez April 10, 2024 16:33
src/ripple/protocol/impl/STLedgerEntry.cpp Show resolved Hide resolved
src/ripple/protocol/impl/STLedgerEntry.cpp Outdated Show resolved Hide resolved
src/test/rpc/TransactionEntry_test.cpp Show resolved Hide resolved
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.

Because I can't help myself, I made all the changes from my feedback, except the feature name, in a branch: mvadari/rippled@fix-prev-txn-id...ximinez:rippled:mv/4751-prev-txn-id

Pick and choose as you see fit.

src/ripple/ledger/impl/ApplyStateTable.cpp Outdated Show resolved Hide resolved
src/ripple/protocol/STLedgerEntry.h Outdated Show resolved Hide resolved
src/ripple/protocol/impl/STLedgerEntry.cpp Outdated Show resolved Hide resolved
src/ripple/protocol/impl/STLedgerEntry.cpp Outdated Show resolved Hide resolved
src/ripple/ledger/impl/ApplyStateTable.cpp Outdated Show resolved Hide resolved
src/ripple/protocol/impl/Feature.cpp Show resolved Hide resolved
src/test/ledger/Directory_test.cpp Outdated Show resolved Hide resolved
src/test/ledger/Directory_test.cpp Outdated Show resolved Hide resolved
src/test/ledger/Directory_test.cpp Outdated Show resolved Hide resolved
@mvadari mvadari requested a review from ximinez April 12, 2024 13:51
@Bronek Bronek self-requested a review April 12, 2024 14:16
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Looking good !

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.

🎉 :shipit:

@mvadari
Copy link
Collaborator Author

mvadari commented Apr 17, 2024

I'm not sure who to tag to indicate that this is ready to go (maybe @seelabs) - the Mac test failure looks like the known flakiness, not an actual failure.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Apr 18, 2024
@ximinez
Copy link
Collaborator

ximinez commented Apr 18, 2024

I see you proposed a commit message (#4751 (comment)), and I just added the "Passed" label. One of us will merge it soon.

@ximinez ximinez merged commit 659bd99 into XRPLF:develop Apr 18, 2024
16 of 17 checks passed
@mvadari mvadari deleted the fix-prev-txn-id branch April 18, 2024 15:05
@ximinez ximinez mentioned this pull request Apr 18, 2024
1 task
@seelabs seelabs mentioned this pull request Jun 4, 2024
1 task
@intelliot
Copy link
Collaborator

API Change: this adds fields to objects. I don't think it's a breaking change. But it would still make sense to update the models in the client libraries (@mvadari has noted this).

@intelliot intelliot changed the title amendment fix to add sfPreviousTxnID/sfPreviousTxnLgrSequence to all ledger objects fixPreviousTxnID: add sfPreviousTxnID/sfPreviousTxnLgrSequence to all ledger objects Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Amendment API Change Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Status: 🚢 Released in 2.2.0
Development

Successfully merging this pull request may close these issues.

5 participants