rippled icon indicating copy to clipboard operation
rippled copied to clipboard

amendment fix to add `sfPreviousTxnID`/`sfPreviousTxnLgrSequence` to all ledger objects

Open mvadari opened this issue 1 year ago • 2 comments

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

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [x] 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 avatar Oct 05 '23 18:10 mvadari

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

mvadari avatar Nov 02 '23 19:11 mvadari

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 :warning:
src/ripple/ledger/impl/ApplyStateTable.cpp 33.33% 0 Missing and 2 partials :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 01 '24 19:02 codecov-commenter

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 avatar Mar 19 '24 17:03 mvadari

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.

mvadari avatar Apr 17 '24 19:04 mvadari

I see you proposed a commit message (https://github.com/XRPLF/rippled/pull/4751#issuecomment-2007734702), and I just added the "Passed" label. One of us will merge it soon.

ximinez avatar Apr 18 '24 00:04 ximinez

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 avatar Jun 27 '24 19:06 intelliot