Introduce AMM support (XLS-30d) #4294

gregtatcam · 2022-09-06T16:39:44Z

High Level Overview of Change

Adds AMM core functionality InstanceCreate, Deposit, Withdraw, Governance, Auctioning
and payment engine integration.

Context of Change

Adds Transactors for InstanceCreate, Deposit, Withdraw, Governance, and Auctioning.
Adds AMM Root Account, trustline for each IOU AMM token, trustline to track Liquidity Provider Tokens (LPT),
and ltAMM object to track fee votes, auction slot bids, AMM tokens pair, total outstanding tokens balance, and
AMMID to AMM RootAccountID mapping.
New classes are added to facilitate AMM integration into the payment engine. BookStep uses these classes
to infer if AMM liquidity can be consumed.
A new RPC method amm_info is added to fetch the pool and
LPT balances.
A new Number class is added to facilitate AMM formula implementation. IOUAmount and STAmount use
Number arithmetic. A fixUniversalNumber amendment is added.
featureAMM, fixUniversalNumber, and featureFlowCross amendments are required to support AMM.

Type of Change

[ x] New feature (non-breaking change which adds functionality)

Test Plan

AMM unit tests are added for all core functionality features.

nbougalis · 2022-09-07T17:38:47Z

Requesting reviews from @seelabs and Aanchal Malhotra; I can't mark them as reviewers.

wojake · 2022-09-07T17:50:43Z

@aanchal4: Please review the PR

sublimator · 2022-09-20T01:31:38Z

What's AMM ? Any docs for the curious?

wojake · 2022-09-20T06:31:35Z

@sublimator:
Proposal: XRPLF/XRPL-Standards#78
Doc (request access and you'll be granted in): https://docs.google.com/document/d/1cPnAtdt6ekCc7tQCA7UQQhKD3W3dNQr3fFzPyQGQ5-M/edit#heading=h.ls4o33ii8o1

sublimator · 2022-09-21T07:40:54Z

@wojake
Thanks

seelabs

Nice job on this!

Let me start by staying that since there are a lot of comments, they can come across as abrupt or demanding. I don't mean them as such. Mentally put a "Please consider..." in front of every comment and a "...if I understood correctly" at the end. It gets too wordy if I actually do that, but I also don't want to the comments to be read harshly - I don't mean them to be.

Some thoughts apart from the inline code comments:

Is there a paper explaining why offers are sized by a Fibonacci sequence? I understand it may be beneficial to create synthetic offers with increasing sizes, but I don't understand why Fibonacci was chosen. Why not double every time? Why not some other function? How was growth by Fibonacci chosen? Was there a mathematical exploration of the different growth functions and what the trade-offs were? What are the trade-offs that were considered?

The handling of const gave me pause in this patch. There are things marked const that are do not appear to be "logical const". Mutable is used to work around this. I think it's important to resolve this.

Our style guide says we shouldn't trigger exceptions on user input. This patch probably violates this, but I don't actually object too strongly (I think we should relax that style guide). However, we do need to make sure the new exceptions are handled correctly.

Whenever we change a sle, we need to make sure we call update. I'm not sure this is done in all cases. I tagged one possible case, but it's worth it to audit the other transactors to make sure we're doing this.

I understand why the AMM is a regular account, but I don't love that it acts in special ways that are fragile. In particular, it doesn't use a reserve, and more problematically, it can't be deposited to.

Don't define function inline in classes - it makes it harder to see the interface.

In optional, prefer * to .value

src/ripple/app/misc/AMM.h

src/ripple/protocol/SField.h

src/ripple/protocol/impl/QualityFunction.cpp

src/ripple/rpc/handlers/AMMInfo.cpp

src/test/app/AMM_test.cpp

src/ripple/app/misc/AMM_formulae.h

cjcobb23 · 2022-10-12T15:46:30Z

src/ripple/app/misc/AMM.h

@@ -0,0 +1,160 @@
+//------------------------------------------------------------------------------


Can we move this file to xrpl_core, so we can access the functions in clio?

Actually, I realized we can't move everything in this file into xrpl_core, but we can probably move things like calcAccountID and calcAMMGroupHash

Moved relevant API's to xrpl_core.

wojake · 2022-10-29T14:37:42Z

I'd like to propose an additional check on AMMCreate transactions:
If Asset1 || Asset2 == AMMToken: throw temBAD_CURRENCY

We shouldn't allow AMM tokens to be a depositable asset on an AMM Instance as they wouldn't be volatile and it's odd to offer them for sale at an algorithmic price that doesn't know their underlying value in their respective AMM.

While an AMM Instance isn't able to construct an AMMCreate transaction and create a new pool for their AMM token (i.e. xAMMToken & USD/Bistamp), an IOU issuer could construct an AMMCreate transaction and create a pool for their IOU with an AMM token (i.e. USD/Bitstamp & xAMMToken).

To add on to my suggestion, there actually might be a small number of users who would use this pool (AMMToken&IOU) but it would be incredibly inconsistent for us to facilitate it as a feature. Disk space is a commodity on the ledger and it should be treated as such.

effofexx · 2022-10-30T19:02:58Z

I'd like to suggest an alternate method of calculating the amount of LPTokens being burned and refunded to the previous slot-holder in AMMBid.cpp. I apologize in advance for the lengthy message but I'm a little rusty with C++ and want to ensure my understanding of the code is correct, as well as explain my line of thinking.

First, I'd like to explain my understanding/assumptions. As an example, I will use the case where a slot-holder is outbid during the first interval, but I think my suggestion at the end would still be applicable for the other intervals as well.

My Understanding/Assumptions:

During the first interval:

timeSlot = 0
fractionUsed = 0.05
fractionRemaining = 0.95
payPrice = pricePurchased * 1.05 (i.e. X * 1.05)
Amount of LPTokens burned (line 283) = payPrice * 0.05
Amount of LPTokens refunded previous to slot-holder (line 292) = payPrice * 0.95

However, when representing the refund in terms of what the previous slot-holder paid (X), the refund is:
fractionRemaining * payPrice = 0.95 * X * 1.05 = X * 0.9975

Potential Issue:

Since timeSlot is an int, the values above are true at any point during the first interval the current slot-holder was outbid, even if only seconds after acquiring the slot. Is that the intent? The comment on line 286 suggests the previous owner should get a full refund, and the proposal suggests the refund is calculated on a pro-rata basis without mentioning interval increments.

I think the way the refund is being calculated has potential to introduce issues caused by arbitragers unfairly losing a portion of their LPTokens - possibly within mere seconds of acquiring the slot - before they have an opportunity to utlize their slot privileges. It's hard to say how much value could be lost, given we don't know what the slots will cost, or even the value of an LPToken, but this could deter potential arbitragers if the value is sufficiently high.

Suggestion:

In order to maintain the spirit of the proposal while also ensuring the previous slot-holder doesn't get refunded more than they paid, I suggest we consider calculating the amount refunded to previous owner using values based on what they paid and a more accurate representation of the amount of time they've held the slot. So the refunded amount would be something like this:
refundAmount = (1 - ((diff / intervalDuration) / nIntervals)) * pricePurchased

And line 292 would become:
toSTAmount(lpTokens.issue(), refundAmount),

Likewise, the amount to be burned on line 283 would be something like:
res = updateSlot(0, *payPrice, *payPrice - refundAmount);

There may be a more efficient way of implementing this change that I'm not aware of, but hopefully my thoughts are presented clear enough that you understand my concern.

dangell7 · 2022-10-30T21:39:46Z

fractionRemaining * payPrice = 0.95 * X * 1.05 = X * 0.9975

Your calculation is off. fractionRemaining * payPrice = X * 0.95. You're adding an additional 1.05

effofexx · 2022-10-30T21:55:10Z

@dangell7 but payPrice ≠ X. What I'm doing in the bit your quoted is expanding the equation after the first equals sign.

pricePurchased is the amount the previous slot-holder paid (X). Then on line 245 computedPrice is calculated by multiplying pricePurchased by 1.05 (if the bid is made during the first interval). Then beginning on line 251 payPrice is finally defined as equal to computedPrice after checking that computedPrice is within the bounds of the defined min and max bids.

godexsoft

Adding a few suggestions after reviewing the transactors-related parts.

godexsoft · 2022-11-15T18:42:47Z

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

+        return err;
+    }
+
+    if (ctx.tx[sfTradingFee] > TradingFeeThreshold)


TradingFeeThreshold is set to 1000 (1%) but the whitepaper says valid value is up to 65000 (65%). I may be misunderstanding the purpose of this threshold but i thought i'd ask just in case.

Nevermind - this got updated in the whitepaper since i had this question. Seems to match now.

godexsoft · 2022-11-16T23:53:31Z

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

+    }
+
+    if (auto const res =
+            invalidAMMAmount(amount, {{asset, asset2}}, ePrice.has_value()))


When dealing with optionals, constructing like {{asset, asset2}} constructs the pair by copying the assets and then moves it into the optional. This is minor but it's generally better to use std::make_optional(asset, asset2) here (and everywhere) as that guarantees copy elision and all you get is just a constructor call without having to move anything.

godexsoft · 2022-11-18T20:25:01Z

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

+         &amountBalance = amountBalance,
+         &amount2Balance = amount2Balance,
+         &lptAMMBalance = lptAMMBalance]() -> std::pair<TER, STAmount> {
+        if (subTxType & tfTwoAsset)


I wonder, why & here but == for other ifs below?

Thanks, changed all to &.

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

godexsoft · 2022-11-18T20:48:40Z

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

+    return deposit(view, ammAccount, amountDeposit, std::nullopt, tokens);
+}
+
+}  // namespace ripple


Missing empty line - makes github very sad

godexsoft · 2022-11-21T23:52:08Z

src/ripple/app/tx/impl/AMMVote.h

+
+namespace ripple {
+
+class Sandbox;


Is this needed here?

No, removed.

godexsoft · 2022-11-22T00:08:16Z

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

+    auto const ammAccountID = (**ammSle)[sfAMMAccount];
+    auto const lpTokens =
+        ammLPHolds(ctx_.view(), **ammSle, ctx_.tx[sfAccount], ctx_.journal);
+    auto const lpTokensWithdraw =


This looks familiar. Consider refactoring this out as a helper function?

godexsoft · 2022-11-22T00:15:17Z

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

+    return {tecAMM_FAILED_WITHDRAW, STAmount{}};
+}
+
+}  // namespace ripple


Newline needed

godexsoft · 2022-11-22T00:27:08Z

src/ripple/basics/Number.h

+    constexpr static int minExponent = -32768;
+    constexpr static int maxExponent = 32768;
+
+    class Guard;


Out of interest, what is this for?

Nvm. I found it while looking thru the rest of the code. Apparently non-expanded files are not searchable (what a surprise 🤣) so i could not see it before.

godexsoft · 2022-11-23T17:02:56Z

src/ripple/app/tx/impl/AMMBid.h

+
+namespace ripple {
+
+class Sandbox;


Is this needed?

No, removed.

godexsoft

The code looks well tested and inline with the codebase's overall style.
I have left a few minor comments. Most are negligible and can be ignored. Nothing noted is a show stopper IMO.

godexsoft · 2022-11-28T18:45:12Z

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

+    auto const [minC, maxC] = std::minmax(cur1, cur2);
+    auto const hash = sha512Half(minC, maxC);
+    Currency currency;
+    *currency.begin() = AMMCurrencyCode;


Is this the best way this exact behaviour can be expressed in code?

Do you have suggestions? LPToken is 0x03 plus 19 bytes from the hash. I added a comment.

godexsoft · 2022-11-28T18:47:09Z

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

+    return std::make_pair(assetInBalance, assetOutBalance);
+}
+
+Expected<std::tuple<STAmount, STAmount, STAmount>, TER>


Consider using a struct for the 3 amounts to make it more obvious what they are meant to represent

I'd like to keep it. It's pretty simple pool properties - amount, amount2, lptoken. There is also another function (above) that uses a subset - amount, amount2.

godexsoft · 2022-11-28T19:31:04Z

src/ripple/app/paths/AMMContext.h

+class AMMContext
+{
+private:
+    constexpr inline static std::uint8_t MaxIterations = 4;


Minor note: MaxIterations is implicitly inline (due to constexpr). No need to explicitly seqckfu inline

Thanks, updated.

godexsoft · 2022-11-28T19:38:26Z

src/ripple/app/paths/impl/AMMLiquidity.cpp

+template class AMMLiquidity<XRPAmount, IOUAmount>;
+template class AMMLiquidity<IOUAmount, XRPAmount>;
+
+}  // namespace ripple


Newline needed

godexsoft · 2022-11-28T19:39:36Z

src/ripple/app/paths/impl/AMMLiquidity.cpp

+
+    cur.in = toAmount<TIn>(
+        getIssue(balances.in),
+        (Number(5) / 20000) * initialBalances_.in,


Consider moving magic numbers into some sort of constant on the class?

godexsoft · 2022-11-28T22:26:13Z

src/test/app/TrustAndBalance_test.cpp

            env.require(balance(
                alice,
                STAmount(
                    carol["USD"].issue(),
-                    6500000000000001ull,
+                    6500000000000000ull,


Does this change not break anything in the rest of the system? i mean if it was expected to be one thing before and now it's a different thing it may lead to some issues with already existing data and what not. Just wanted to make sure you checked and confirmed this

@HowardHinnant , maybe you can clarify this. Thanks

Yes, this is a breaking change. And therefore this change is guarded by an amendment that must be approved prior to going into effect.

godexsoft · 2022-11-28T22:29:55Z

src/test/basics/Number_test.cpp

+    {
+        testcase("test_limits");
+        bool caught = false;
+        try


Consider adding BEAST_EXPECT_THROW_ANY(...) or similar to simplify all the boilerplate

godexsoft · 2022-11-28T23:16:43Z

src/test/jtx/impl/AMM.cpp

+    setTokens(jv, assets);
+    jv[jss::TransactionType] = jss::AMMWithdraw;
+    if (log_)
+        std::cout << jv.toStyledString();


Is it allowed to have cout logs anywhere? I'd expect only jlog logs to be pushed to the repo

Also more couts in other places in this file.

This is for debugging. There is also a manual AMMCalc test, which is more of a tool to check the state of AMM after swap, etc.

godexsoft · 2022-11-29T16:16:14Z

src/test/jtx/impl/AMMTest.cpp

+
+}  // namespace jtx
+}  // namespace test
+}  // namespace ripple


Newline needed

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

seelabs

Still 👍 after the latest patches

ximinez

Merge looks good!

…#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected]

ledger state: (XRPLF#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected]

ledger state: (XRPLF#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected] Signed-off-by: Manoj Doshi <[email protected]>

…#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected]

ledger state: (XRPLF#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected] Signed-off-by: Manoj Doshi <[email protected]>

…#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected] Signed-off-by: Manoj Doshi <[email protected]>

Add AMM functionality: - InstanceCreate - Deposit - Withdraw - Governance - Auctioning - payment engine integration To support this functionality, add: - New RPC method, `amm_info`, to fetch pool and LPT balances - AMM Root Account - trust line for each IOU AMM token - trust line to track Liquidity Provider Tokens (LPT) - `ltAMM` object The `ltAMM` object tracks: - fee votes - auction slot bids - AMM tokens pair - total outstanding tokens balance - `AMMID` to AMM `RootAccountID` mapping Add new classes to facilitate AMM integration into the payment engine. `BookStep` uses these classes to infer if AMM liquidity can be consumed. The AMM formula implementation uses the new Number class added in XRPLF#4192. IOUAmount and STAmount use Number arithmetic. Add AMM unit tests for all features. AMM requires the following amendments: - featureAMM - fixUniversalNumber - featureFlowCross Notes: - Current trading fee threshold is 1% - AMM currency is generated by: 0x03 + 152 bits of sha256{cur1, cur2} - Current max AMM Offers is 30 --------- Co-authored-by: Howard Hinnant <[email protected]>

…#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected] Signed-off-by: Manoj Doshi <[email protected]>

Add AMM functionality: - InstanceCreate - Deposit - Withdraw - Governance - Auctioning - payment engine integration To support this functionality, add: - New RPC method, `amm_info`, to fetch pool and LPT balances - AMM Root Account - trust line for each IOU AMM token - trust line to track Liquidity Provider Tokens (LPT) - `ltAMM` object The `ltAMM` object tracks: - fee votes - auction slot bids - AMM tokens pair - total outstanding tokens balance - `AMMID` to AMM `RootAccountID` mapping Add new classes to facilitate AMM integration into the payment engine. `BookStep` uses these classes to infer if AMM liquidity can be consumed. The AMM formula implementation uses the new Number class added in XRPLF#4192. IOUAmount and STAmount use Number arithmetic. Add AMM unit tests for all features. AMM requires the following amendments: - featureAMM - fixUniversalNumber - featureFlowCross Notes: - Current trading fee threshold is 1% - AMM currency is generated by: 0x03 + 152 bits of sha256{cur1, cur2} - Current max AMM Offers is 30 --------- Co-authored-by: Howard Hinnant <[email protected]>

…#4626) When an AMM account is deleted, the owner directory entries must be deleted in order to ensure consistent ledger state. * When deleting AMM account: * Clean up AMM owner dir, linking AMM account and AMM object * Delete trust lines to AMM * Disallow `CheckCreate` to AMM accounts * AMM cannot cash a check * Constrain entries in AuthAccounts array to be accounts * AuthAccounts is an array of objects for the AMMBid transaction * SetTrust (TrustSet): Allow on AMM only for LP tokens * If the destination is an AMM account and the trust line doesn't exist, then: * If the asset is not the AMM LP token, then fail the tx with `tecNO_PERMISSION` * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY` * This disallows trustlines to AMM in empty state * Add AMMID to AMM root account * Remove lsfAMM flag and use sfAMMID instead * Remove owner dir entry for ltAMM * Add `AMMDelete` transaction type to handle amortized deletion * Limit number of trust lines to delete on final withdraw + AMMDelete * Put AMM in empty state when LPTokens is 0 upon final withdraw * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are deleted (i.e. partial deletion) * This is handled in Transactor similar to deleted offers * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr * Don't validate for invalid asset pair in AMMDelete * AMMWithdraw deletes AMM trust lines and AMM account/object only if the number of trust lines is less than max * Current `maxDeletableAMMTrustLines` = 512 * Check no directory left after AMM trust lines are deleted * Enable partial trustline deletion in AMMWithdraw * Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in empty state * Clawback considerations * Disallow clawback out of AMM account * Disallow AMM create if issuer can claw back This patch applies to the AMM implementation in XRPLF#4294. Acknowledgements: Richard Holland and Nik Bougalis for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the project code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: [email protected] Signed-off-by: Manoj Doshi <[email protected]>

gregtatcam force-pushed the amm-core-functionality branch 2 times, most recently from ad655ce to c1e4bfb Compare September 7, 2022 14:15

nbougalis requested a review from RichardAH September 7, 2022 17:37

seelabs requested review from seelabs, nbougalis and aanchal4 September 22, 2022 14:13

seelabs assigned seelabs, nbougalis and aanchal4 Sep 22, 2022

tequdev mentioned this pull request Sep 24, 2022

Add AMM liquidity pool support #3908

Open

seelabs suggested changes Oct 7, 2022

View reviewed changes

seelabs reviewed Oct 11, 2022

View reviewed changes

src/ripple/app/misc/AMM_formulae.h Outdated Show resolved Hide resolved

cjcobb23 suggested changes Oct 12, 2022

View reviewed changes

seelabs requested a review from greg7mdp October 17, 2022 20:13

seelabs assigned greg7mdp Oct 17, 2022

seelabs unassigned greg7mdp Nov 3, 2022

gregtatcam force-pushed the amm-core-functionality branch 2 times, most recently from fe128fa to 2fa59f1 Compare November 10, 2022 20:43

seelabs removed the request for review from greg7mdp November 15, 2022 16:41

godexsoft reviewed Nov 23, 2022

View reviewed changes

godexsoft reviewed Nov 29, 2022

View reviewed changes

intelliot assigned seelabs and ximinez and unassigned nbougalis and aanchal4 Jul 12, 2023

seelabs approved these changes Jul 12, 2023

View reviewed changes

intelliot requested a review from ximinez July 12, 2023 15:11

ximinez approved these changes Jul 12, 2023

View reviewed changes

intelliot merged commit 3c9db4b into XRPLF:develop Jul 12, 2023

This was referenced Jul 17, 2023

fix(AMM): prevent orphaned objects, inconsistent ledger state: (updates XLS-30) #4626

Closed

Proposed 1.12.0-b2 #4627

Merged

ximinez mentioned this pull request Aug 1, 2023

Add an Invariant check that a deleted account has no traceable objects left on the ledger. #4638

Closed

intelliot added the XLS-30 label Oct 20, 2023

thejohnfreeman mentioned this pull request Apr 23, 2024

Raise test coverage of AMM to 100% #5000

Open

		@@ -0,0 +1,160 @@
		//------------------------------------------------------------------------------

Introduce AMM support (XLS-30d) #4294

Introduce AMM support (XLS-30d) #4294

Conversation

gregtatcam commented Sep 6, 2022 • edited by intelliot Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

nbougalis commented Sep 7, 2022

wojake commented Sep 7, 2022

sublimator commented Sep 20, 2022

wojake commented Sep 20, 2022

sublimator commented Sep 21, 2022

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

wojake commented Oct 29, 2022 • edited Loading

effofexx commented Oct 30, 2022 • edited Loading

dangell7 commented Oct 30, 2022

effofexx commented Oct 30, 2022

godexsoft 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

godexsoft 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

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

ximinez left a comment

Choose a reason for hiding this comment

gregtatcam commented Sep 6, 2022 •

edited by intelliot

Loading

wojake commented Oct 29, 2022 •

edited

Loading

effofexx commented Oct 30, 2022 •

edited

Loading