fixAMMClawbackRounding: adjust last holder's LPToken balance by yinyiqian1 · Pull Request #5513 · XRPLF/rippled

yinyiqian1 · 2025-06-25T20:15:07Z

Overview of fixAMMClawbackRounding: Due to rounding, the LPTokenBalance of the last LP might not match the LP's trustline balance, this was fixed for AMMWithdraw in fixAMMv1_1 by adjusting the LPTokenBalance to be the same as trustline balance.
Since AMMClawback is also performing a withdrawal, we need to adjust LPTokenBalance as well in AMMClawback.

This PR includes:

refactored verifyAndAdjustLPTokenBalance function to AMMUtils. AMMWithdraw and AMMClawback both call this function to adjust LPTokenBalance.
Added unit test testLastHolderLPTokenBalance to test the scenario.
Modify the existing unit tests for fixAMMClawbackRounding. Added some minor changes, for example, variable aliceXrpBalance is updated after each transaction to make checking xrp balance easier.

High Level Overview of Change

Context of Change

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)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
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

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)

codecov · 2025-06-26T17:10:35Z

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.0%. Comparing base (258ba71) to head (51aa456).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/misc/detail/AMMUtils.cpp	83.3%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5513     +/-   ##
=========================================
- Coverage     79.1%   79.0%   -0.0%     
=========================================
  Files          816     816             
  Lines        71632   71652     +20     
  Branches      8237    8263     +26     
=========================================
- Hits         56648   56639      -9     
- Misses       14984   15013     +29

Files with missing lines	Coverage Δ
src/xrpld/app/tx/detail/AMMClawback.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/AMMWithdraw.cpp	`95.5% <100.0%> (+0.2%)`	⬆️
src/xrpld/app/misc/detail/AMMUtils.cpp	`96.5% <83.3%> (-0.8%)`	⬇️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

include/xrpl/protocol/detail/features.macro

gregtatcam

AMMClawback logs a bunch of errors like

ERR:OpenLedger AMM 31 invariant failed: 8A5A159FDA51F5E9841D2C80FD488DD47014FFE4F5948235D7B5AE7B957D248D true 565.685424949238/USD/r9QxhA9RghPZBbUchA9HkrmLKaWvkLXU29 70710678/XRP 199999.9998321968 200000 0.0000000008390160007039478

This is normal but it makes parsing unit-tests output difficult. These errors can be inhibited like this

Env env(*this, features, std::make_unique<CaptureLogs>(&logs));

You can search CaptureLogs for examples.

src/xrpld/app/misc/AMMUtils.h

src/xrpld/app/tx/detail/AMMClawback.cpp

src/test/app/AMMClawback_test.cpp

shawnxie999

👍

gregtatcam

LGTM 👍

include/xrpl/protocol/detail/features.macro

* XRPLF/develop: fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526)

…refactoring-1 * upstream/develop: (56 commits) Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) chore: Fix compilation error with clang-20 and cleanup (#5543) test: Remove circular jtx.h dependencies (#5544) Decouple CredentialHelpers from xrpld/app/tx (#5487) fix: crash when trace-logging in tests (#5529) test: switch some unit tests to doctest (#5383) ...

…actoring-2 * ximinez/lending-refactoring-1: (57 commits) Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) chore: Fix compilation error with clang-20 and cleanup (#5543) test: Remove circular jtx.h dependencies (#5544) Decouple CredentialHelpers from xrpld/app/tx (#5487) fix: crash when trace-logging in tests (#5529) ...

…actoring-3 * ximinez/lending-refactoring-2: (57 commits) Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) chore: Fix compilation error with clang-20 and cleanup (#5543) test: Remove circular jtx.h dependencies (#5544) Decouple CredentialHelpers from xrpld/app/tx (#5487) fix: crash when trace-logging in tests (#5529) ...

…actoring-4 * ximinez/lending-refactoring-3: (61 commits) fixup! Rename Transactor preflight functions Rename Transactor preflight functions fixup! Make preflight1 and preflight2 private static Transactor functions Make preflight1 and preflight2 private static Transactor functions Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) ...

…actoring-4 * ximinez/lending-refactoring-3: (61 commits) fixup! Rename Transactor preflight functions Rename Transactor preflight functions fixup! Make preflight1 and preflight2 private static Transactor functions Make preflight1 and preflight2 private static Transactor functions Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) ... Co-authored-by: Bronek Kozicki <brok@incorrekt.com>

Co-authored-by: Jingchen <a1q123456@users.noreply.github.com> Co-authored-by: Bronek Kozicki <brok@incorrekt.com> Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com> Co-authored-by: Denis Angell <dangell@transia.co> Co-authored-by: Vlad <129996061+vvysokikh1@users.noreply.github.com> Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> Co-authored-by: yinyiqian1 <yqian@ripple.com> Co-authored-by: Michael Legleux <legleux@users.noreply.github.com> Co-authored-by: Bart <bthomee@users.noreply.github.com> Co-authored-by: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Co-authored-by: Luc des Trois Maisons <3maisons@gmail.com> Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> fix: crash when trace-logging in tests (#5529) Fix compilation error with clang-20 and cleanup (#5543) fix: Link with boost libraries explicitly (#5546) fix: add allowTrustLineLocking flag for account_info (#5525) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) Fix macos runner (#5585) Fix clang-format CI job (#5598) resolved actions/runner#2058.

Bronek · 2025-09-01T10:24:52Z

@yinyiqian1 would you like to create a new PR to switch Supported::no to yes for this amendment, before the 3.0.0 release ?

yinyiqian1 · 2025-09-01T15:34:28Z

@yinyiqian1 would you like to create a new PR to switch Supported::no to yes for this amendment, before the 3.0.0 release ?

Sure. Just created: #5750

yinyiqian1 force-pushed the fix_ammclawback branch 2 times, most recently from d83e7c9 to 8058476 Compare June 26, 2025 16:01

yinyiqian1 changed the title ~~fix last holder's LPToken balance for AMMClawback~~ fixAMMClawback: adjust last holder's LPToken balance Jun 26, 2025

yinyiqian1 marked this pull request as ready for review June 26, 2025 16:15

yinyiqian1 requested review from a team, gregtatcam and shawnxie999 June 26, 2025 16:15

fix last holder's LPToken balance for AMMClawback

5a645ff

yinyiqian1 force-pushed the fix_ammclawback branch from 8058476 to 5a645ff Compare June 26, 2025 16:43

exclude coverage for unreachable lines

c5ec622

shawnxie999 reviewed Jun 26, 2025

View reviewed changes

include/xrpl/protocol/detail/features.macro Outdated Show resolved Hide resolved

gregtatcam reviewed Jul 1, 2025

View reviewed changes

Addressing comments

851156d

yinyiqian1 changed the title ~~fixAMMClawback: adjust last holder's LPToken balance~~ fixAMMClawbackRounding: adjust last holder's LPToken balance Jul 3, 2025

shawnxie999 reviewed Jul 3, 2025

View reviewed changes

src/xrpld/app/tx/detail/AMMClawback.cpp Show resolved Hide resolved

src/xrpld/app/tx/detail/AMMClawback.cpp Show resolved Hide resolved

src/xrpld/app/tx/detail/AMMClawback.cpp Outdated Show resolved Hide resolved

retrieve LP token balance inside amendment gate

021dd78

yinyiqian1 requested review from gregtatcam and shawnxie999 July 3, 2025 19:20

shawnxie999 reviewed Jul 4, 2025

View reviewed changes

src/test/app/AMMClawback_test.cpp Show resolved Hide resolved

shawnxie999 reviewed Jul 4, 2025

View reviewed changes

src/test/app/AMMClawback_test.cpp Outdated Show resolved Hide resolved

yinyiqian1 force-pushed the fix_ammclawback branch from f13a220 to ee978e1 Compare July 4, 2025 18:22

Check last holder's lptoken balance in unit test

61a702d

yinyiqian1 force-pushed the fix_ammclawback branch from ee978e1 to 61a702d Compare July 4, 2025 18:23

shawnxie999 approved these changes Jul 4, 2025

View reviewed changes

gregtatcam approved these changes Jul 5, 2025

View reviewed changes

bthomee reviewed Jul 6, 2025

View reviewed changes

include/xrpl/protocol/detail/features.macro Outdated Show resolved Hide resolved

yinyiqian1 added 3 commits July 10, 2025 10:12

Set AMMClawbackRounding to Supported::no

03fd53c

Merge branch 'develop' into fix_ammclawback

65ebc46

Merge branch 'develop' into fix_ammclawback

677fa91

Add fixAMMClawbackRounding to FeatureBitset

43dbcc1

kennyzlei added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jul 11, 2025

Merge branch 'develop' into fix_ammclawback

51aa456

bthomee enabled auto-merge (squash) July 11, 2025 19:30

bthomee merged commit 8aa94ea into XRPLF:develop Jul 11, 2025
2 checks passed

yinyiqian1 deleted the fix_ammclawback branch July 25, 2025 14:36

legleux mentioned this pull request Aug 27, 2025

Release 2.6.0 #5741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixAMMClawbackRounding: adjust last holder's LPToken balance#5513

fixAMMClawbackRounding: adjust last holder's LPToken balance#5513
bthomee merged 10 commits intoXRPLF:developfrom
yinyiqian1:fix_ammclawback

yinyiqian1 commented Jun 25, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 26, 2025 •

edited

Loading

Uh oh!

Uh oh!

gregtatcam left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shawnxie999 left a comment

Uh oh!

gregtatcam left a comment

Uh oh!

Uh oh!

Uh oh!

Bronek commented Sep 1, 2025

Uh oh!

yinyiqian1 commented Sep 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

yinyiqian1 commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

codecov bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shawnxie999 left a comment

Choose a reason for hiding this comment

Uh oh!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bronek commented Sep 1, 2025

Uh oh!

yinyiqian1 commented Sep 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yinyiqian1 commented Jun 25, 2025 •

edited

Loading

codecov bot commented Jun 26, 2025 •

edited

Loading