Remove OwnerPaysFee as it's never fully supported
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 false for flow(), but this flag is sometimes hardcoded as true in 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
- [ ] 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)
- [x] Refactor (non-breaking change that only restructures code)
- [ ] Performance (increase or change in throughput and/or latency)
- [x] Tests (you added tests for code that already exists, or your new feature included in this PR)
- [ ] Documentation update
- [ ] Chore (no impact to binary, e.g.
.gitignore, formatting, dropping support for older tooling) - [ ] Release
API Impact
This change doesn't affect any public protocol.
- [ ] Public API: New feature (new methods and/or new fields)
- [ ] Public API: Breaking change (in general, breaking changes should only impact the next api_version)
- [ ]
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl) - [ ] Peer protocol change (must be backward compatible or bump the peer protocol version)
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/
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 79.1%. Comparing base (
42fd74b) to head (e955f18). Report is 1 commits behind head on develop.
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
| Files with missing lines | Coverage Δ | |
|---|---|---|
| include/xrpl/protocol/Feature.h | 100.0% <ø> (ø) |
|
| src/libxrpl/protocol/Feature.cpp | 94.8% <100.0%> (ø) |
|
| src/xrpld/app/paths/RippleCalc.cpp | 82.9% <ø> (-0.4%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
@a1q123456 you can fix the formatting errors with
find src include -type f \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' -o -name '*.ipp' \) -exec clang-format -i {} +
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 pip, e.g.
pip install clang-format==18.1.8
@a1q123456 you can fix the formatting errors with
find src include -type f \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' -o -name '*.ipp' \) -exec clang-format -i {} +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
pip, e.g.pip install clang-format==18.1.8
Thanks Bronek! Will do.
@a1q123456 is this ready to merge or are you still working on addressing some comments?
@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.