rippled
rippled copied to clipboard
XRPLF
Introduce AMM support (XLS-30d)
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.
Future Tasks
The unit-tests is a work in progress.
Requesting reviews from @seelabs and Aanchal Malhotra; I can't mark them as reviewers.
@sublimator: Proposal: https://github.com/XRPLF/XRPL-Standards/discussions/78 Doc (request access and you'll be granted in): https://docs.google.com/document/d/1cPnAtdt6ekCc7tQCA7UQQhKD3W3dNQr3fFzPyQGQ5-M/edit#heading=h.ls4o33ii8o1
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.
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.
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
@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.
Feel free to say no, but: Is there a way to divide this into 2 smaller PRs so that review is easier?
We can have a feature branch (similar to feature/nft-fixes, maybe feature/amm) to use as a staging area for code that has been reviewed/approved but is not yet ready to be merged into develop.
Thoughts @gregtatcam ?
Feel free to say no, but: Is there a way to divide this into 2 smaller PRs so that review is easier?
We can have a feature branch (similar to
feature/nft-fixes, maybefeature/amm) to use as a staging area for code that has been reviewed/approved but is not yet ready to be merged intodevelop.Thoughts @gregtatcam ?
I'm worried about making any changes now when we are planning to release within a few weeks. Also I'm on PTO next week so rushing this in the next few days might not be ideal. I'd rather focus on getting the feedback addressed as soon as possible. This might be a good idea for future projects. I think it's a bit late to change the process now.
Given the fact that this is a very large code base that touches on the payment engine at multiple points, I would not be comfortable with this being merged without an external code audit. We’ve been bitten badly with XLS20 sped up merging. This, IMO, is enormously more impactful on the ledger. So I do think this external audit needs to be commissioned by the authors, Ripple.
So I do think this external audit needs to be commissioned by the authors, Ripple.
I am also strongly in favour of an external code / security review. The independent maintainers on this repository (myself included) simply do not have the time to review 16K LOC (AMM) and 13k LOC (XChain) ... meaning these amendments are written by Ripple engineers and reviewed only by Ripple engineers. Please get a third party review.
Given the fact that this is a very large code base that touches on the payment engine at multiple points, I would not be comfortable with this being merged without an external code audit. We’ve been bitten badly with XLS20 sped up merging. This, IMO, is enormously more impactful on the ledger. So I do think this external audit needs to be commissioned by the authors, Ripple.
Do Ripple engineers think this doesn’t merit a reply? @JoelKatz ? If merging happens without an external review I will take it that Ripple does not put network and user safety first. And we will review merge permissions.
Given the fact that this is a very large code base that touches on the payment engine at multiple points, I would not be comfortable with this being merged without an external code audit. We’ve been bitten badly with XLS20 sped up merging. This, IMO, is enormously more impactful on the ledger. So I do think this external audit needs to be commissioned by the authors, Ripple.
Do Ripple engineers think this doesn’t merit a reply? @JoelKatz ? If merging happens without an external review I will take it that Ripple does not put network and user safety first. And we will review merge permissions.
@alloynetworks @RichardAH @WietseWind I apologize if you felt that we were ignoring you. We were not at all. We had selected a 3rd party security review vendor and have a first kick off meeting on Monday. We hope to have further details on the security review plan next week. I will update this thread regardless of whether I get meaningful details to share from the 3rd party to communicate expectations and timelines.
We want to emphasis that we do care about the network and its safety. We are part of the ecosystem and we are not in any way going to put us and the entire network at peril.
So stay tuned for updates.
@alloynetworks @RichardAH @WietseWind I apologize if you felt that we were ignoring you. We were not at all.
Yeah, I totally thumbs upped your comments! :grinning:
So stay tuned for updates.
Just an update on our end:
- We had a kick off meeting with our first security vendor today 3/27 Monday
- The vendor will provide us with an estimate on the timeline and the quotes in the near future
- We are looking for other potential security vendors at the same time as well to determine the best fit for reviewing such DeFi protocol / features
(cc: @alloynetworks @RichardAH @WietseWind )
So stay tuned for updates.
Just an update on our end:
We had a kick off meeting with our first security vendor today 3/27 Monday
- The vendor will provide us with an estimate on the timeline and the quotes in the near future
We are looking for other potential security vendors at the same time as well to determine the best fit for reviewing such DeFi protocol / features
(cc: @alloynetworks @RichardAH @WietseWind )
After careful review, we have chosen a 3rd party security vendor that is a best fit for auditing the AMM on the XRPL. We are in the process of onboarding the vendor to get the work started. (cc: @alloynetworks @RichardAH @WietseWind )
Hey, did you notice that all of the unity builds had a single test failure? https://github.com/XRPLF/rippled/actions/runs/4948980242/jobs/8852057301?pr=4294
Hey, did you notice that all of the unity builds had a single test failure? https://github.com/XRPLF/rippled/actions/runs/4948980242/jobs/8852057301?pr=4294
I have not paid attention to the latest because previously I saw "failed in 0 seconds" failures. I tested with unity OFF on Ubuntu, Windows, and ON/OFF on OSX. It worked on OSX. This test fails on Ubuntu/ON:
ammAlice.withdraw(
alice,
STAmount{USD, UINT64_C(99999999999999999), -13},
std::nullopt,
std::nullopt,
ter(tecAMM_FAILED_WITHDRAW));
On Ubuntu/OFF, STAmount is rounded to USD(10000) but on Ubuntu with unity ON it's 9999.999999999999.
It's a bit puzzling. I'm investigating. Thanks for pointing this out.
@gregtatcam Could you please update the TER code for:
tecHOOK_ERROR [[maybe_unused]] = 153,
the correct field is:
tecHOOK_REJECTED [[maybe_unused]] = 153,
Also it is in use on the hooks network.
Hi @dangell7. Why does it have to be updated? rippled develop branch has tecHOOK_ERROR field and not tecHOOK_REJECTED.
tecHOOK_ERROR
The correct field on the hooks amendment is tecHOOK_REJECTED. I can make a PR to change this on develop that way its not part of your PR.
tecHOOK_ERROR
The correct field on the hooks amendment is
tecHOOK_REJECTED. I can make a PR to change this ondevelopthat way its not part of your PR.
Right. It should be PR on develop. I'll then merge.
I'd like to propose an additional check on AMMCreate transactions: If
Asset1 || Asset2 == AMMToken: throw temBAD_CURRENCYWe 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 rate 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 might be a small number of users who would use this pool (AMMToken&IOU) but it would be incredibly small for us to facilitate it as a feature. Disk space is a commodity on the ledger and it should be treated as such.
As per the suggestion of @ximinez, re-posted as a review 😄
@wojake , this request is addressed in the latest commit.
If and when this PR is merged:
- It will be squashed into a single commit.
- The commit will have the following commit message:
Introduce AMM support (XLS-30d): (#4294)
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 #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]>
If anyone prefers otherwise and/or would like to suggest any changes to this commit message, please comment here with your recommended alternative. @injaelee @gregtatcam @HowardHinnant
per community discussions, this will not be merged until after the results of the security review are published (cc: @injaelee )
Given the fact that this is a very large code base that touches on the payment engine at multiple points, I would not be comfortable with this being merged without an external code audit. We’ve been bitten badly with XLS20 sped up merging. This, IMO, is enormously more impactful on the ledger. So I do think this external audit needs to be commissioned by the authors, Ripple.
Do Ripple engineers think this doesn’t merit a reply? @JoelKatz ? If merging happens without an external review I will take it that Ripple does not put network and user safety first. And we will review merge permissions.
@alloynetworks @RichardAH @WietseWind I apologize if you felt that we were ignoring you. We were not at all. We had selected a 3rd party security review vendor and have a first kick off meeting on Monday. We hope to have further details on the security review plan next week. I will update this thread regardless of whether I get meaningful details to share from the 3rd party to communicate expectations and timelines.
We want to emphasis that we do care about the network and its safety. We are part of the ecosystem and we are not in any way going to put us and the entire network at peril.
So stay tuned for updates.
Just an update on the security review.
Our security audit report will be published on 6/26 Monday through CertiK's Skynet.
Stay tuned.
Given the fact that this is a very large code base that touches on the payment engine at multiple points, I would not be comfortable with this being merged without an external code audit. We’ve been bitten badly with XLS20 sped up merging. This, IMO, is enormously more impactful on the ledger. So I do think this external audit needs to be commissioned by the authors, Ripple.
Do Ripple engineers think this doesn’t merit a reply? @JoelKatz ? If merging happens without an external review I will take it that Ripple does not put network and user safety first. And we will review merge permissions.
@alloynetworks @RichardAH @WietseWind I apologize if you felt that we were ignoring you. We were not at all. We had selected a 3rd party security review vendor and have a first kick off meeting on Monday. We hope to have further details on the security review plan next week. I will update this thread regardless of whether I get meaningful details to share from the 3rd party to communicate expectations and timelines. We want to emphasis that we do care about the network and its safety. We are part of the ecosystem and we are not in any way going to put us and the entire network at peril. So stay tuned for updates.
Just an update on the security review.
Our security audit report will be published on 6/26 Monday through CertiK's Skynet.
Stay tuned.
The audit report is now ready. Please take a look.
- https://skynet.certik.com/projects/xrp-ledger
- Go to the "Code Security" section and click on the "View PDF" button
It might be a good idea if someone could write a blog on the Dev.to website and maybe a tweet to go along with it on the RippleX Twitter account regarding the AMM security audit results. Not many, outside of developers pay attention to Github posts.
