Fix offer crossing with transfer fee over AMM and other fixes #5016

gregtatcam · 2024-05-13T19:33:18Z

High Level Overview of Change

This PR addresses three issues:

Offer crossing with the transfer fee via AMM
LPToken adjustment to factor in the LPTokenBalance rounding
Possible conflict between Liquidity Provider trustline LPToken balance
and LPTokenBalance when last Liquidity Provider withdraws all tokens

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

Before / After

qualityUpperBound() and QualityFunction calculation doesn't factor in the transfer fee on offer crossing.
This is correct behavior when crossing an account offer. Single-path with AMM limits the out value based
on the limitQuality. If the transfer fee is not factored in then the out value is going to be such that
the generated AMM offer quality matches the limitQuality. But the effective quality is smaller because
takerPays amount factors in the transfer fee when the strand's quality is calculated. The fix factors in
the transfer fee when the quality is adjusted for the fees on offer crossing.
adjustAmountsByLPTokens() function is not returning adjusted tokens and amounts in some cases.
The fix returns the adjusted LPToken and adjusted amounts.
Liquidity Provider's (LP) LPToken trustline balances might not add up to the total LPTokenBalance due to the rounding.
This results in a dust amount left in the balances on the last LP withdrawal or the withdrawal transaction failing. The fix
sets the LPTokenBalance to the trustline balance if LP is the only remaining LP on withdrawal transaction.

Test Plan

Updated AMM_test and AMMExtended unit-tests.

codecov-commenter · 2024-05-13T19:55:57Z

Codecov Report

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

Project coverage is 71.1%. Comparing base (2705109) to head (15390be).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5016     +/-   ##
=========================================
+ Coverage     71.0%   71.1%   +0.1%     
=========================================
  Files          796     796             
  Lines        66912   66997     +85     
  Branches     10988   10979      -9     
=========================================
+ Hits         47540   47635     +95     
+ Misses       19372   19362     -10

Files	Coverage Δ
src/ripple/app/misc/impl/AMMHelpers.cpp	`97.6% <100.0%> (+0.4%)`	⬆️
src/ripple/app/misc/impl/AMMUtils.cpp	`99.4% <100.0%> (+0.2%)`	⬆️
src/ripple/protocol/impl/Feature.cpp	`93.9% <ø> (ø)`
src/ripple/app/paths/impl/BookStep.cpp	`93.1% <95.0%> (+1.1%)`	⬆️
src/ripple/app/tx/impl/AMMWithdraw.cpp	`91.2% <66.7%> (+0.8%)`	⬆️

... and 2 files with indirect coverage changes

scottschurr · 2024-05-14T01:25:15Z

src/ripple/app/misc/impl/AMMUtils.cpp

+        auto const uNodeNext = ownerDir->getFieldU64(sfIndexNext);
+        if (uNodeNext == 0)
+            return nTrustLines == 2 || nTrustLines == 3;
+        currentIndex = keylet::page(root, uNodeNext);


This linear search is kinda terrifying. I looked in a recent ledger and the largest number of owner directory pages owned by a single account was 284,820. But there were quite a few accounts with over 10,000 owner directory pages. This linear search signs us up for a potentially huge amount of computing. Calling this function from a transactor looks really scary to me.

Is there anything we know about AMM Liquidity Providers that will limit the number of directory pages that they can own? Is there any way that could be leveraged to limit this linear search to just a few directory nodes?

There are two things in this function that prevent this possible huge number of iterations. One is that the limit is set to at most 10 iterations. Two is that if there is more than one Liquidity Provider (LP) then we'll break out of the loop a lot sooner because any LP can have at most three trustlines to AMM: one for LPToken and at most two for IOU. So we should break out after at most four iterations in the worst case scenario if there are more than one LP.

if ((lowLimit.getIssuer() != lpAccount && highLimit.getIssuer() != lpAccount)) return false;

Sorry. I missed what limit was up to when I first looked at the while loop. But I still don't understand why 10 directory pages is a legit upper bound.

Owner directories can hold a lot of things other than trust lines. The number of Offers (both IOU and NFT), Escrows, PayChans, and DepositPreauths they can hold has a very large upper bound. Additionally, an account can own up to 250 tickets.

Now, maybe it's impossible for an AMM Liquidity Provider account to own any of these kinds of objects. In which case 10 iterations is (probably?) plenty. But I am not familiar enough with the AMM to know whether that's the case. If it is the case I think there should be a comment explaining why 10 iterations will always be sufficient.

There should probably also be some kind of error indication if the number of expected directory pages is ever surpassed. It may not be possible for an AMM Liquidity Provider to ever exceed that number of pages today. But people are excited about AMMs, which means rules will be changing in the future. So either the function must throw or the return value needs to be more complicated than a bool. Maybe an Expected?

Does that make any sense?

We are iterating over the trustlines for AMM account. AMM account holds one AMM object and the rest are LP trustlines. Each LP has one trustline for LPToken. If an issuer created AMM then the issuer could have two more trustlines for IOU. Actually the function should return true if trustlines >= 1 && truslines <= 3, not 2 or 3.

It makes sense to return an error in some cases: there are 0 trustlines, non-issuer LP has more than 1 trustline, an issuer LP has more than 3 trustlines, there are more than 1 AMM object. Expected with tecINTERNAL works.

I think it is sufficient to just look for LPToken trustlines and return an error if it is 0 or greater than 1.

I just did a little more reading about the AMM implementation. I'm sorry that a) I'm not really up on how the AMM works and b) that I did not look more closely at the loop before I commented.

Given the constraints as I (finally) understand them, we can have high confidence that the number of loops will be very bounded by adding one constraint to the loop. If nTrustLines > 3 return false. I think if that constraint is added then (assuming there's only one AMM object) the number of directory pages examined cannot exceed 6.

Yeah, I don't love this. I don't see a better solution though.

I like all the error checking added to this function. Good changes!

seelabs

This looks OK to me, but I want to take another look tomorrow before I approve it.

seelabs · 2024-05-14T22:24:34Z

src/test/jtx/impl/AMMTest.cpp

@@ -103,44 +103,45 @@ AMMTestBase::testAMM(
    std::optional<std::pair<STAmount, STAmount>> const& pool,
    std::uint16_t tfee,
    std::optional<jtx::ter> const& ter,
-    std::optional<FeatureBitset> const& features)
+    std::vector<FeatureBitset> const& vfeatures)


I don't love this interface - running the same test with a different set of features seems like you'd want another function call (with possibly another ter). I'm not that picky about test code, so no change needed if you prefer this.

This ter is for AMMCreate failure, in which case the call back is not very useful. If ter is included and a different behavior (AMMCreate fails or succeeds) is expected for different features then testAMM should be called separately for each feature. Actually, ter was not used at all in this function. I updated to use it.
The vector of features is intended for a case when AMMCreate succeeds, there are a number of transactions in the call back, and the transactions may behave differently or have different results for different features.

seelabs · 2024-05-14T22:26:09Z

src/ripple/app/misc/impl/AMMUtils.cpp

+        auto const uNodeNext = ownerDir->getFieldU64(sfIndexNext);
+        if (uNodeNext == 0)
+            return nTrustLines == 2 || nTrustLines == 3;
+        currentIndex = keylet::page(root, uNodeNext);


Yeah, I don't love this. I don't see a better solution though.

scottschurr · 2024-05-15T16:19:43Z

src/ripple/app/misc/impl/AMMHelpers.cpp

@@ -165,31 +165,48 @@ adjustAmountsByLPTokens(

    if (lpTokensActual < lpTokens)
    {
+        bool const ammRoundingEnabled = [&]() {
+            if (auto const& rules = getCurrentTransactionRules();
+                rules && rules->enabled(fixAMMRounding))


When this is merged with develop will fixAMMRounding be an additional amendment? Or will you be leveraging the recently added fixAMMv1_1? I'm fine either way, but it's worth being aware of the merge consequences.

I'll leverage fixAMMv1_1.

We should probably update this PR to use fixAMMv1_1.

I need to merge with the other AMM PR #4983.

seelabs · 2024-05-15T17:09:13Z

src/ripple/app/tx/impl/AMMWithdraw.cpp

+            return {res.error(), false};
+        else if (
+            res.value() &&
+            withinRelativeDistance(


I'm not understanding the reasoning behind the withinRelativeDistance check. If it's the only trust line why do we check if there's a relative distance? I'm guessing this is as a sanity check. But I would probably make the sanity check looser if this is the case.

Right, it's a sanity check and I made it more restrictive. Do you feel e-7 is sufficient?

I don't think there's a value where we can safely say "the sanity check must always be below this value". This sanity check is really protecting against a bug in isOnlyLiquidityProvider. As long as that's solid we don't need the sanity check. Thinking...

If this is the last remaining trust line and the sanity check fails, what should happen? If we return a tec we may be saving ourselves against an exploit/bad bug at the cost of not allowing the last person to get their funds. If we don't have a check, we open ourselves up to bugs in isOnlyLiqueidityProvider. If we keep it as-is, then what? Isn't that still a bug if the sanity check fails and we keep the code as-is?

The only reason to have the sanity check is in case isOnlyLiquidityProvider is not solid. I think it looks right but then essentially all tokens are assigned to the user. I don't see a way around it.

If the sanity check fails then it's still a bug, only more obscure. The transaction might succeed or might fail. It might make sense to return tec. The user can still withdraw with a fewer tokens.

I vote that we loosen the constraint considerably - maybe 10^-1? 10^-2? - and also change this to return a tec if the sanity check fails.

I was thinking e-4 at least to prevent/minimize a possible exploit if there is one. I'm fine with e-2 if everyone votes for it.

It's your call. I'm also find with e-4.

Let's wait for @scottschurr vote.

I don't really have an opinion, but I trust @gregtatcam's take. Shall we go with e-4?

Let's do e-3.

scottschurr

👍 Looks good to me. I found one typo in a comment, but nothing that critically needs to be changed as far as I was able to see.

scottschurr · 2024-05-15T21:42:28Z

src/ripple/app/misc/impl/AMMUtils.cpp

-    std::uint8_t nTrustLines = 0;
+    // Liquidity Provider (LP) must have one LPToken trustline
+    std::uint8_t nLPTokenTrustLines = 0;
+    // There are at most two IOU truslines. One or both could be to the LP


Typo: trusline -> trustline

scottschurr · 2024-05-15T21:43:47Z

src/ripple/app/misc/impl/AMMUtils.cpp

+    // AMM LP has at most three trustlines and only one AMM object must exist.
+    // If there are more than five objects then it's either an error or
+    // there are more than one LP. Ten pages should be sufficient to include
+    // five objects.


Thanks for these comments! They helped me a lot.

scottschurr · 2024-05-15T21:52:36Z

src/ripple/app/misc/impl/AMMUtils.cpp

 isOnlyLiquidityProvider(
    ReadView const& view,
-    AccountID const& ammAccount,
+    Issue const& ammIssue,


Good call on passing in the Issue. Thanks.

scottschurr · 2024-05-15T22:17:55Z

src/ripple/app/misc/impl/AMMUtils.cpp

+        auto const uNodeNext = ownerDir->getFieldU64(sfIndexNext);
+        if (uNodeNext == 0)
+            return nTrustLines == 2 || nTrustLines == 3;
+        currentIndex = keylet::page(root, uNodeNext);


I like all the error checking added to this function. Good changes!

scottschurr · 2024-05-16T00:14:41Z

src/ripple/app/tx/impl/AMMWithdraw.cpp

+            return {res.error(), false};
+        else if (
+            res.value() &&
+            withinRelativeDistance(


I don't really have an opinion, but I trust @gregtatcam's take. Shall we go with e-4?

The fix is to return the actual adjusted lp tokens and amounts by the function.

Single path AMM offer has to factor in the transfer in rate when calculating the upper bound quality and the quality function because single path AMM's offer quality is not constant. This fix factors in the transfer fee in BookStep::adjustQualityWithFees().

Due to the rounding, LPTokenBalance of the last Liquidity Provider (LP), might not match this LP's trustline balance. This fix sets LPTokenBalance on last LP withdrawal to this LP's LPToken trustline balance.

gregtatcam requested a review from seelabs May 13, 2024 19:42

gregtatcam requested a review from scottschurr May 13, 2024 20:41

scottschurr reviewed May 14, 2024

View reviewed changes

seelabs reviewed May 14, 2024

View reviewed changes

scottschurr reviewed May 15, 2024

View reviewed changes

seelabs reviewed May 15, 2024

View reviewed changes

scottschurr approved these changes May 16, 2024

View reviewed changes

seelabs approved these changes May 16, 2024

View reviewed changes

gregtatcam added 3 commits May 16, 2024 13:32

Fix adjustAmountsByLPTokens():

2a25f58

The fix is to return the actual adjusted lp tokens and amounts by the function.

Fix last Liquidity Provider withdrawal:

15390be

Due to the rounding, LPTokenBalance of the last Liquidity Provider (LP), might not match this LP's trustline balance. This fix sets LPTokenBalance on last LP withdrawal to this LP's LPToken trustline balance.

gregtatcam force-pushed the amm-fix-offercross-trfee branch from 261e750 to 15390be Compare May 16, 2024 17:36

seelabs merged commit 15390be into XRPLF:develop May 16, 2024
17 checks passed

seelabs mentioned this pull request May 16, 2024

Proposed 2.2.0-rc2 #5018

Merged

1 task

intelliot mentioned this pull request May 30, 2024

fixAMMv1_1: improve the quality of the generated AMM offer in certain cases #4983

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix offer crossing with transfer fee over AMM and other fixes #5016

Fix offer crossing with transfer fee over AMM and other fixes #5016

gregtatcam commented May 13, 2024

codecov-commenter commented May 13, 2024 •

edited

scottschurr May 14, 2024

gregtatcam May 14, 2024

scottschurr May 14, 2024

gregtatcam May 14, 2024

gregtatcam May 14, 2024

scottschurr May 14, 2024

seelabs May 14, 2024

scottschurr May 15, 2024

seelabs left a comment

seelabs May 14, 2024

gregtatcam May 15, 2024

seelabs May 14, 2024

scottschurr May 15, 2024

gregtatcam May 15, 2024

seelabs May 15, 2024

gregtatcam May 15, 2024

seelabs May 15, 2024

gregtatcam May 15, 2024

seelabs May 15, 2024

gregtatcam May 15, 2024

seelabs May 15, 2024

gregtatcam May 15, 2024

seelabs May 15, 2024

gregtatcam May 15, 2024

scottschurr May 16, 2024

gregtatcam May 16, 2024

scottschurr left a comment

scottschurr May 15, 2024

scottschurr May 15, 2024

scottschurr May 15, 2024

scottschurr May 15, 2024

scottschurr May 16, 2024

Fix offer crossing with transfer fee over AMM and other fixes #5016

Fix offer crossing with transfer fee over AMM and other fixes #5016

Conversation

gregtatcam commented May 13, 2024

High Level Overview of Change

Type of Change

Before / After

Test Plan

codecov-commenter commented May 13, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a 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

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

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

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

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

codecov-commenter commented May 13, 2024 •

edited