fix: AMM swap rounding #5002

seelabs · 2024-04-25T11:00:44Z

High Level Overview of Change

fix amendment: AMM swap should honor invariants:

The AMM has an invariant for swaps where:

new_balance_1*new_balance_2 >= old_balance_1*old_balance_2

Due to rounding, this invariant could sometimes be violated (although by very small amounts).

This patch introduces an amendment fixAMMRounding that changes the rounding to always favor the AMM. Doing this should maintain the invariant.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[ x] 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)
[ 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

codecov-commenter · 2024-04-25T11:25:55Z

Codecov Report

Attention: Patch coverage is 60.52632% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 70.9%. Comparing base (b422e71) to head (b90f58d).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5002     +/-   ##
=========================================
- Coverage     70.9%   70.9%   -0.0%     
=========================================
  Files          796     796             
  Lines        66727   66792     +65     
  Branches     10976   10996     +20     
=========================================
+ Hits         47339   47378     +39     
- Misses       19388   19414     +26

Files	Coverage Δ
src/ripple/app/tx/impl/Transactor.cpp	`84.5% <100.0%> (+<0.1%)`	⬆️
src/ripple/basics/Number.h	`100.0% <100.0%> (ø)`
src/ripple/protocol/Feature.h	`100.0% <ø> (ø)`
src/ripple/protocol/Rules.h	`100.0% <100.0%> (ø)`
src/ripple/protocol/impl/Feature.cpp	`93.9% <ø> (ø)`
src/ripple/protocol/impl/Rules.cpp	`97.9% <100.0%> (+0.4%)`	⬆️
src/ripple/ledger/impl/View.cpp	`91.6% <75.0%> (-0.1%)`	⬇️
src/ripple/protocol/AmountConversions.h	`85.7% <0.0%> (-10.3%)`	⬇️
src/ripple/app/misc/AMMHelpers.h	`69.0% <53.1%> (-16.1%)`	⬇️

... and 5 files with indirect coverage changes

src/test/app/AMM_test.cpp

gregtatcam · 2024-04-25T13:20:35Z

src/ripple/protocol/AmountConversions.h

+        static_assert(alwaysFalse, "Unsupported type for toAmount");
+    }
+}
+


How about using concept instead of the static_assert here and other places?

template <typename T> concept ValidAmount = std::is_same_v<T, IOUAmount> || std::is_same_v<T, XRPAmount> || std::is_same_v<T, STAmount>; template <ValidAmount T> T toMaxAmount(Issue const& issue) { ... }

I don't think max Number is applicable, at least not in the context that toMaxAmount() is used.

I thought I got a compile error without the supporting Number.

I don't object to the concept, but I weakly prefer the code without it. I don't love that the is_same is repeated in both the body of the function and the concept, and the concept isn't important for distinguishing between overloads. I'll change it if you or others feel strongly, but I think I prefer the non-concept version.

It builds for me without Number on OSX, Linux, and Windows.

I prefer concept since it's clearly defines the type constraint. I don't feel strongly about it though.

I hear you. I don't think this is worth spending too much time on. I'm going to keep it as is.

gregtatcam · 2024-04-25T14:50:56Z

src/ripple/app/misc/AMMHelpers.h

+
+        Number::setround(Number::upward);
+        auto const numerator = pool.in * pool.out;
+        auto const fee = getFee(tfee);


Does it make sense to always round upwards getFee() and downward feeMult(). The fee is added to the pool so we want always to maximize it's effect. Can have the default just in case it needs to be changed in some instances:

inline Number getFee(std::uint16_t tfee, Number::rounding_mode rounding = Number::rounding_mode::upward) { if (auto const& rules = getCurrentTransactionRules(); rules && rules->enabled(fixAMMRounding)) { saveNumberRoundMode rm(Number::setround(rounding)); return Number{tfee} / AUCTION_SLOT_FEE_SCALE_FACTOR; } return Number{tfee} / AUCTION_SLOT_FEE_SCALE_FACTOR; } inline Number feeMult(std::uint16_t tfee, Number::rounding_mode rounding = Number::rounding_mode::downward) { if (auto const& rules = getCurrentTransactionRules(); rules && rules->enabled(fixAMMRounding)) { saveNumberRoundMode rm(Number::setround(rounding)); return 1 - getFee(tfee); } return 1 - getFee(tfee); }

gregtatcam · 2024-04-25T18:08:49Z

src/test/app/AMM_test.cpp

-            BEAST_EXPECT(ammAlice.expectBalances(
-                XRP(11'000), USD(11'000), IOUAmount{10'999'890, 0}));
-        });
+        testAMM(


Here and a few other places the tests are executed twice with/without fixAMMRounding and there is no change to the tests with fixAMMRounding. Should we limit this to only the tests that have different results with fixAMMRounding? This impacts the run-time of the tests. testAMM() can be changed to take a vector of features and could also add a helper function to iterate over a vector of features in cases where testAMM() is not used.

gregtatcam

👍 LGTM

HowardHinnant

Approved, but I would like to make CurrentTransactionRulesGuard not copyable either now or in the near future. All it needs is deleted copy members. We already do not copy it, so this won't break anything.

HowardHinnant · 2024-04-25T19:09:00Z

src/ripple/protocol/Rules.h

+class CurrentTransactionRulesGuard
+{
+public:
+    explicit CurrentTransactionRulesGuard(Rules r)


This class should not be CopyConstructible nor CopyAssignable.

Yep, agreed, thanks! Fixed in 94bbc9c8a9 [fold] CurrentTransactionRulesGuard should not be copyable or assignable

scottschurr

👍 LGTM. I left one note that might be a small improvement for future maintainability. But, in general, it looks like it should do the job.

scottschurr · 2024-04-25T18:51:36Z

src/ripple/ledger/impl/View.cpp

+    if (auto const& rules = getCurrentTransactionRules();
+        rules && rules->enabled(fixAMMRounding))


There are now two sources of "truth" regarding the Rules: the Rules in the current ledger or the Rules supplied by getCurrentTransactionRules(). And, interestingly, those two perspectives are only in alignment if the code being executed is inside the scope of a transactor. Outside the scope of a transactor, getCurrentTransactionRules() returns a nullopt_t.

I think the safest approach everywhere in this file is to call view.rules().enabled() to get the Rules that are explicitly associated with the ledger that was explicitly passed in. That way the code that is likely to be copied and pasted will work in both ApplyViews and ReadViews. So I suggest changing these lines to:

if (view.rules().enabled(fixAMMRounding))

But that's just looking forward to when this is copied and pasted within this file. The current implementation should also work just fine in this particular function.

It can be difficult to make transaction breaking changes to low level code because the low level code does not have access to a ledger and the current activated amendments in that ledger (the "rules"). This patch adds global access to the current ledger rules as a `std::optional`. If the optional is not seated, then there is no active transaction.

The AMM has an invariant for swaps where: new_balance_1*new_balance_2 >= old_balance_1*old_balance_2 Due to rounding, this invariant could sometimes be violated (although by very small amounts). This patch introduces an amendment `fixAMMRounding` that changes the rounding to always favor the AMM. Doing this should maintain the invariant. Co-authored-by: Bronek Kozicki Co-authored-by: thejohnfreeman

seelabs requested review from gregtatcam, HowardHinnant and scottschurr April 25, 2024 11:00

gregtatcam requested changes Apr 25, 2024

View reviewed changes

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

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

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

gregtatcam reviewed Apr 25, 2024

View reviewed changes

gregtatcam approved these changes Apr 25, 2024

View reviewed changes

HowardHinnant approved these changes Apr 25, 2024

View reviewed changes

scottschurr approved these changes Apr 25, 2024

View reviewed changes

seelabs force-pushed the amm_rounding branch 4 times, most recently from e751e72 to 3cfc52f Compare April 25, 2024 23:49

seelabs added 2 commits April 25, 2024 20:17

seelabs force-pushed the amm_rounding branch from 3cfc52f to b90f58d Compare April 26, 2024 00:21

seelabs merged commit 3f7ce93 into XRPLF:develop Apr 26, 2024
17 checks passed

ximinez mentioned this pull request Apr 26, 2024

Proposed 2.2.0-rc1 #5003

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: AMM swap rounding #5002

fix: AMM swap rounding #5002

seelabs commented Apr 25, 2024

codecov-commenter commented Apr 25, 2024 •

edited

Loading

gregtatcam Apr 25, 2024

seelabs Apr 25, 2024

gregtatcam Apr 25, 2024

seelabs Apr 25, 2024

gregtatcam Apr 25, 2024

gregtatcam Apr 25, 2024

gregtatcam left a comment

HowardHinnant left a comment

HowardHinnant Apr 25, 2024

seelabs Apr 25, 2024

scottschurr left a comment

scottschurr Apr 25, 2024

		if (auto const& rules = getCurrentTransactionRules();
		rules && rules->enabled(fixAMMRounding))

fix: AMM swap rounding #5002

fix: AMM swap rounding #5002

Conversation

seelabs commented Apr 25, 2024

High Level Overview of Change

Type of Change

codecov-commenter commented Apr 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

HowardHinnant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 25, 2024 •

edited

Loading