Add fixAMMv1_3 amendment #5203

gregtatcam · 2024-11-25T16:32:37Z

High Level Overview of Change

Add AMM bid/create/deposit/swap/withdraw/vote invariants:
- Deposit, Withdrawal invariants: sqrt(asset1Balance * asset2Balance) >= LPTokens.
- Bid: sqrt(asset1Balance * asset2Balance) > LPTokens and the pool balances don't change.
- Create: sqrt(asset1Balance * assetBalance2) == LPTokens.
- Swap: asset1BalanceAfter * asset2BalanceAfter >= asset1BalanceBefore * asset2BalanceBefore
  and LPTokens don't change.
- Vote: LPTokens and pool balances don't change.
- All AMM and swap transactions: amounts and tokens are greater than zero, except on withdrawal if all tokens
  are withdrawn.
Add AMM deposit and withdraw rounding to ensure AMM invariant:
- On deposit, tokens out are rounded downward and deposit amount is rounded upward.
- On withdrawal, tokens in are rounded upward and withdrawal amount is rounded downward.
Add Order Book Offer invariant to verify consumed amounts. Consumed amounts are less than the offer.
Fix Bid validation. AuthAccount can't have duplicate accounts or the submitter account.

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

Test Plan

AMM, AMMExtended, AMMClawback, AMMInfo unit-tests are updated

* Add AMM deposit and withdraw rounding to ensure AMM invariant * Add AMM bid/create/deposit/swap/withdraw/vote invariants * Add Offer invariant to verify consumed amounts * Fix Bid validation

vvysokikh1 · 2025-02-06T11:36:12Z

@gregtatcam could you please merge latest develop so it would be easier to review? Also build seems to be broken

vvysokikh1

Initial review of the invariant implementation

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

src/xrpld/app/tx/detail/InvariantCheck.h

vvysokikh1 · 2025-02-06T13:14:40Z

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

+        if ((txType == ttAMM_DEPOSIT || txType == ttAMM_WITHDRAW ||
+             txType == ttAMM_CLAWBACK || txType == ttAMM_BID) &&
+            ammAccount_)
+        {


I would actually split these if blocks into 4 separate functions, each checking their own condition. When named properly that would improve readability.

IMO it's a lot of code repetition. I added comments and broke the if expression, which checks the invariant. It should be a lot better now.

gregtatcam · 2025-02-06T15:31:57Z

@gregtatcam could you please merge latest develop so it would be easier to review? Also build seems to be broken

done

vvysokikh1 · 2025-02-06T13:24:52Z

src/xrpld/app/tx/detail/Offer.h

+    checkInvariant(TAmounts<TIn, TOut> const& consumed, beast::Journal j) const
    {
+        if (!isFeatureEnabled(fixAMMv1_3))
+            return true;
+
+        if (consumed.in > m_amounts.in || consumed.out > m_amounts.out)
+        {
+            // LCOV_EXCL_START
+            JLOG(j.error())
+                << "AMMOffer::checkInvariant failed: consumed "
+                << to_string(consumed.in) << " " << to_string(consumed.out)
+                << " amounts " << to_string(m_amounts.in) << " "
+                << to_string(m_amounts.out);
+
+            return false;
+            // LCOV_EXCL_STOP
+        }
+


Is this a separate invariant from usual ones? Why?

vvysokikh1 · 2025-02-11T16:49:24Z

include/xrpl/protocol/detail/features.macro

-XRPL_FEATURE(DynamicNFT,                 Supported::yes, VoteBehavior::DefaultNo)
-XRPL_FEATURE(Credentials,                Supported::yes, VoteBehavior::DefaultNo)
-XRPL_FEATURE(AMMClawback,                Supported::yes, VoteBehavior::DefaultNo)
-XRPL_FIX    (AMMv1_2,                    Supported::yes, VoteBehavior::DefaultNo)


Maybe should add //clang-format off for this file and revert this change? I personally don't really see this as a problem though

This file does not have an extension expected by clang-format, so it will not be automatically formatted by it. No need for //clang-format off here.

My VS code setup has application of clang-format on save. So when I add new feature to this file, it reformats it on save regardless of it's extension.

vvysokikh1 · 2025-02-11T17:22:14Z

include/xrpl/protocol/Rules.h

+bool
+isFeatureEnabled(uint256 const& feature);
+


Maybe worth to put on the top of the file (same as other free functions)?

vvysokikh1 · 2025-02-11T17:23:45Z

src/libxrpl/protocol/Rules.cpp

+bool
+isFeatureEnabled(uint256 const& feature)
+{
+    auto const rules = getCurrentTransactionRules();


maybe worth using auto const& to avoid copying the rules?

vvysokikh1 · 2025-02-11T17:24:44Z

src/test/jtx/Env.h

+    /** Check if feature is enabled
+     */


nit: this comment doesn't bring any value

vvysokikh1 · 2025-03-20T10:48:56Z

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

+void
+ValidAMM::visitEntry(
+    bool isDelete,
+    std::shared_ptr<SLE const> const& before,
+    std::shared_ptr<SLE const> const& after)
+{
+    if (isDelete)
+        return;
+
+    auto set = [&](std::shared_ptr<SLE const> const& sle, auto&& cb) {
+        auto const type = sle->getType();
+        if (type == ltAMM)
+        {
+            cb();
+        }
+        else if (
+            sle->isFieldPresent(sfBalance) &&
+            ((type == ltRIPPLE_STATE && sle->getFlags() & lsfAMMNode) ||
+             (type == ltACCOUNT_ROOT && sle->isFieldPresent(sfAMMID))))
+        {
+            ammPoolChanged_ = true;
+        }
+    };
+
+    if (after)
+        set(after, [&] {
+            ammAccount_ = after->getAccountID(sfAccount);
+            lptAMMBalanceAfter_ = after->getFieldAmount(sfLPTokenBalance);
+        });
+    if (before)
+        set(before, [&] {
+            lptAMMBalanceBefore_ = before->getFieldAmount(sfLPTokenBalance);
+        });
+}


I appreciate deduplication, but I find this code too difficult to read. Lambda + callback is too much for my taste.

I'd suggest something like this:

void ValidAMM::visitEntry( bool isDelete, std::shared_ptr<SLE const> const& before, std::shared_ptr<SLE const> const& after) { if (isDelete) return; // Check if pool changed auto ammPoolChanged = [&](std::shared_ptr<SLE const> const& sle) { auto const type = sle->getType(); return sle->isFieldPresent(sfBalance) && ((type == ltRIPPLE_STATE && sle->getFlags() & lsfAMMNode) || (type == ltACCOUNT_ROOT && sle->isFieldPresent(sfAMMID))); }; // Check if entry is AMM auto isAMM = [](std::shared_ptr<SLE const> const& sle) { auto const type = after->getType(); return type == ltAMM; }; if (after) { if (isAMM(after)) { ammAccount_ = after->getAccountID(sfAccount); lptAMMBalanceAfter_ = after->getFieldAmount(sfLPTokenBalance); } else if (ammPoolChanged(after)) { ammPoolChanged_ = true; } } if (before) { if (isAMM(before)) { lptAMMBalanceBefore_ = before->getFieldAmount(sfLPTokenBalance); } else if (ammPoolChanged(before)) { ammPoolChanged_ = true; } } }

I also wonder if ammPoolChanged should be checked for before. To me it seems like both lsfAMMNode and sfAMMID flags can be only set and not removed. And if this is a deletion, we shortcut early. Also I've not found a single occurence of isFieldPresent(sfBalance), whole codebase assumes it's always present.

So I assume it's sufficient to check these flags only for after and not check sfBalance flag existence (assuming it's always present for both trust line and amm node), thus we could simplify the code further

vvysokikh1 · 2025-03-20T11:26:49Z

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

+    if (result == tesSUCCESS)
+    {


I'd suggest a shortcut here.
if (result != tesSUCCESS)
return true;

vvysokikh1 · 2025-03-20T12:08:28Z

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

+        auto const txType = tx.getTxnType();
+        if ((txType == ttAMM_DEPOSIT || txType == ttAMM_WITHDRAW ||
+             txType == ttAMM_CLAWBACK || txType == ttAMM_BID) &&
+            ammAccount_)


ammAccount_ is checked on every if/else statement, maybe take it out? Is that even possible to have a situation where ammAccount_ would not be present? Should it be an assert of some sort?

vvysokikh1 · 2025-03-20T14:19:25Z

src/xrpld/app/misc/AMMHelpers.h

+/** Round AMM single deposit/withdrawal amount.
+ * The lambda's are used to delay evaluation until the function
+ * is executed so that the calculation is not done twice. noRoundCb() is
+ * called if AMMv1_3 is disabled. Otherwise, the rounding is set and
+ * the amount is:
+ *   isDeposit is Yes - the balance multiplied by productCb()
+ *   isDeposit is No - the result of productCb(). The rounding is
+ *     the same for all calculations in productCb()
+ */
+STAmount
+getRoundedAsset(
+    Rules const& rules,
+    std::function<Number()>&& noRoundCb,
+    STAmount const& balance,
+    std::function<Number()>&& productCb,
+    IsDeposit isDeposit);


I'm not convinced that these callbacks are required here. Callback is not even a correct term here.

All those callbacks are either identical or calculable from one another:

auto amtNoRoundCb = [&] { return tokensAdj / ePrice; }; auto amtProdCb = [&] { return tokensAdj / ePrice; };

or

auto tokNoRoundCb = [&] { return lptAMMBalance * **(lptAMMBalance + ae * (f - 2)) / (lptAMMBalance * f - ae);** }; auto tokProdCb = [&] { return **(lptAMMBalance + ae * (f - 2)) / (lptAMMBalance * f - ae)**; }; where tokNoRoundCb() == tokProdCb() * lptAMMBalance;

So if we don't want to do calculation twice, we can do that without lambdas relatively easy.

Overall I would suggest revisiting all such callback taking functions. They're probably not needed and could be simplified/refactored into more easy readable code

vvysokikh1 · 2025-03-20T14:26:26Z

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

+bool
+ValidAMM::finalize(
+    STTx const& tx,
+    TER const result,
+    XRPAmount const fee,
+    ReadView const& view,
+    beast::Journal const& j)


I would like to see unit tests checking this invariant. I understand it's probably going to take a lot of effort thought.

While the current version of the code is definitely much more readable that previous, I would still suggest splitting finalize into a couple of different functions each covering certain scenario (or group of scenarios). That would make writing and reading tests much easier for this invariant

Bronek · 2025-03-24T14:15:00Z

@gregtatcam I want to bring this commit 1e565e8 in a different PR #5224 to your attention. You might (or might not - up to you) want to copy & alter it to check the fixAMMv1_3 feature.

Add fixAMMv1_3 amendment

c789b5f

* Add AMM deposit and withdraw rounding to ensure AMM invariant * Add AMM bid/create/deposit/swap/withdraw/vote invariants * Add Offer invariant to verify consumed amounts * Fix Bid validation

gregtatcam force-pushed the amendment/fix-amm-v1-3 branch from 5f8bd94 to c789b5f Compare November 25, 2024 16:35

tequdev mentioned this pull request Dec 5, 2024

AuthAccounts with AMMBid should be unique and !=sfAccount [Version: rippled-2.2.2] #5184

Open

mvadari linked an issue Dec 5, 2024 that may be closed by this pull request

AuthAccounts with AMMBid should be unique and !=sfAccount [Version: rippled-2.2.2] #5184

Open

gregtatcam mentioned this pull request Dec 12, 2024

Add MPTIssue to STIssue #5200

Merged

9 tasks

gregtatcam requested review from ximinez and Bronek January 6, 2025 18:53

kennyzlei added Bug Amendment labels Jan 27, 2025

kennyzlei requested review from vvysokikh1 and removed request for Bronek and ximinez January 27, 2025 21:25

kennyzlei added this to the 2.4.0 (2025) milestone Feb 4, 2025

vvysokikh1 requested changes Feb 6, 2025

View reviewed changes

gregtatcam added 2 commits February 6, 2025 15:19

Merge with the latest develop

bb972e9

Address reviewer's feedback

5bf582f

kennyzlei modified the milestones: 2.4.0 (Q1 2025), 2.5.0 (2025) Feb 11, 2025

vvysokikh1 reviewed Mar 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fixAMMv1_3 amendment #5203

Add fixAMMv1_3 amendment #5203

gregtatcam commented Nov 25, 2024

vvysokikh1 commented Feb 6, 2025 •

edited

Loading

vvysokikh1 left a comment

vvysokikh1 Feb 6, 2025

gregtatcam Feb 11, 2025

gregtatcam commented Feb 6, 2025

vvysokikh1 Feb 6, 2025

vvysokikh1 Feb 11, 2025

Bronek Mar 24, 2025

vvysokikh1 Mar 24, 2025 •

edited

Loading

vvysokikh1 Feb 11, 2025

vvysokikh1 Feb 11, 2025

vvysokikh1 Feb 11, 2025

vvysokikh1 Mar 20, 2025

vvysokikh1 Mar 20, 2025

vvysokikh1 Mar 20, 2025

vvysokikh1 Mar 20, 2025

vvysokikh1 Mar 20, 2025

Bronek commented Mar 24, 2025 •

edited

Loading

Add fixAMMv1_3 amendment #5203

Are you sure you want to change the base?

Add fixAMMv1_3 amendment #5203

Conversation

gregtatcam commented Nov 25, 2024

High Level Overview of Change

Type of Change

Test Plan

vvysokikh1 commented Feb 6, 2025 • edited Loading

vvysokikh1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysokikh1 Mar 24, 2025 • edited Loading

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

Bronek commented Mar 24, 2025 • edited Loading

vvysokikh1 commented Feb 6, 2025 •

edited

Loading

vvysokikh1 Mar 24, 2025 •

edited

Loading

Bronek commented Mar 24, 2025 •

edited

Loading