refactor: Remove seq from TMGetObjectByHash#6976
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the peer-protocol TMGetObjectByHash message by removing the obsolete seq field and cleaning up related code paths and tests.
Changes:
- Remove
seq(field/tag 3) fromTMGetObjectByHashinxrpl.proto. - Stop copying
seqfrom request to reply inPeerImpandLedgerMasterhandlers. - Update overlay compression test to no longer set
seqonTMGetObjectByHash.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/xrpld/overlay/detail/PeerImp.cpp |
Removes propagation of obsolete seq into TMGetObjectByHash replies and drops an outdated comment. |
src/xrpld/app/ledger/detail/LedgerMaster.cpp |
Removes propagation of seq when constructing fetch-pack replies. |
src/test/overlay/compression_test.cpp |
Updates test setup to stop setting removed seq field. |
include/xrpl/proto/xrpl.proto |
Deletes the seq field from the protobuf definition of TMGetObjectByHash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6976 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 1010 1010
Lines 75992 75988 -4
Branches 7605 7595 -10
=======================================
+ Hits 62002 62017 +15
+ Misses 13990 13971 -19
🚀 New features to boost your workflow:
|
|
What is the most recent version that used this field, and is it amendment or otherwise blocked? |
We were doing this initially And then we removed it 11 years ago in the commit f946d7b by Vinnie. Can confirm we don't use this field in the past 11 years. |
High Level Overview of Change
This PR removes the seq field from
TMGetObjectByHashas it's not used and obsolete.Context of Change
As Vinie suggested in the comment that the
seqfield is obsolete, we did some research and confirmed that it's indeed not used and it's safe to remove.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)