rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Remove OwnerPaysFee as it's never fully supported

Open a1q123456 opened this issue 7 months ago • 3 comments

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)
  • [ ] libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • [ ] 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/

a1q123456 avatar May 19 '25 13:05 a1q123456

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

Impacted file tree graph

@@           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:

... and 6 files with indirect coverage changes

Impacted file tree graph

: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.

codecov[bot] avatar May 19 '25 14:05 codecov[bot]

@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

Bronek avatar May 19 '25 16:05 Bronek

@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 avatar May 19 '25 17:05 a1q123456

@a1q123456 is this ready to merge or are you still working on addressing some comments?

bthomee avatar Jun 24 '25 16:06 bthomee

@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.

a1q123456 avatar Jun 24 '25 16:06 a1q123456