Remove OwnerPaysFee as it's never fully supported#5435
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5435 +/- ##
=======================================
Coverage 79.1% 79.1%
=======================================
Files 817 817
Lines 71704 71703 -1
Branches 8238 8236 -2
=======================================
+ Hits 56720 56722 +2
+ Misses 14984 14981 -3
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@a1q123456 you can fix the formatting errors with You will need clang-format version 18.1.8 in your path. Since you do not need full clang/llvm for formatting only, a simpler way to get it is with |
Thanks Bronek! Will do. |
vvysokikh1
left a comment
There was a problem hiding this comment.
General question: are all tests that are being removed failing? If some of them can succeed, they should stay
Bronek
left a comment
There was a problem hiding this comment.
I spent some time with this PR and I think I see a potential problem here.
The established semantics of "Retired" feature is that it is permanently enabled and the code does not support it being disabled. However when we examine how amendment support is handled in FeatureCollections::registerFeature, the body of lambda getAmendmentSupport makes implied assumption that an Obsolete feature is always Retired and I think that's incorrect and may potentially cause problems in the future.
I suggest the following change:
--- a/src/libxrpl/protocol/Feature.cpp
+++ b/src/libxrpl/protocol/Feature.cpp
@@ -276,7 +276,7 @@ FeatureCollections::registerFeature(
features.emplace_back(name, f);
auto const getAmendmentSupport = [=]() {
- if (vote == VoteBehavior::Obsolete)
+ if (vote == VoteBehavior::Obsolete && support == Supported::yes)
return AmendmentSupport::Retired;
return support == Supported::yes ? AmendmentSupport::Supported
: AmendmentSupport::Unsupported;
... and then you will find that you can undo changes in Feature_test.cpp and also remove numObsoleteUnsupportedAmendments (as they will be now simply considered "unsupported") from Feature.cpp and Feature.h.
You might also add point 5) in the large comment near the top of Feature.h to explain that a feature might be marked Obsolete, and that can mean one of two things, depending on Supported - either it is in the ledger and it is on its way to become Retired (if it is Supported::yes) or the feature is not in the ledger and the code to support it has been removed (if it is Supported::no). Feel free to phrase it differently, just note the comment above retireFeature in Feature.cpp
…-unused-feature-owner-pays-fee
Bronek
left a comment
There was a problem hiding this comment.
This is good, just please improve the comment in Feature.h
|
@a1q123456 is this ready to merge or are you still working on addressing some comments? |
Hey Bart, you're faster than me :). It's ready to merge now. |
…e-owner-pays-fee' into a1q123456/remove-unused-feature-owner-pays-fee
High Level Overview of Change
As instructed, the OwnerPaysFee amendment is never fully supported and therefore we should remove it. However, after some investigation, I found that we can't fully purge everything relating to it because some other features are relying on it. I've removed the feature from RippleCalc.cpp and set the ownerPaysFee flag to
falseforflow(), but this flag is sometimes hardcoded astruein some contexts, e.g. XChainBridge.cpp at line 502, CashCheck.cpp at line 448, and CreateOffer.cpp at line 804, which implies we shouldn't fully remove all the business logic relating to it.I've also removed some unit test cases because they were written for the OwnerPaysFee feature and there are already some test cases for the regular scenario with OwnerPaysFee disabled.
Context of Change
This change removes unneeded logic and unit tests.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
This change doesn't affect any public protocol.
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
As this is the first time we have a feature that is both obsolete and unsupported, we should check if any related systems support it. e.g. https://xrpscan.com/amendments and https://js.xrpl.org/