fixAMMv1_1: improve the quality of the generated AMM offer in certain cases
#4983
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4983 +/- ##
=========================================
+ Coverage 71.0% 71.0% +0.1%
=========================================
Files 796 796
Lines 66793 66912 +119
Branches 10987 10988 +1
=========================================
+ Hits 47420 47539 +119
Misses 19373 19373
... and 4 files with indirect coverage changes |
src/ripple/app/misc/AMMHelpers.h
Outdated
| * x = (-b - sqrt(b**2 - 4*a*c))/(2*a), if b < 0 | ||
| */ | ||
| inline std::optional<Number> | ||
| solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c) |
There was a problem hiding this comment.
Should this function be placed next to
solveQuadraticEq(Number const& a, Number const& b, Number const& c);
at the bottom of this file or vice versa? That second function is not inlined and also not used by this function when it could be. Means that a simple piece of logic is split over three file locations. Hard to follow by code outsiders. Also corrected comment below shows the same for insiders :-)
There was a problem hiding this comment.
These two functions use different solutions and the rounding strategy could be also different.
gregtatcam
force-pushed
the
amm-offer-rounding-fix
branch
2 times, most recently
from
a3e7adf to
99390cb
Compare
| Number::setround(mode_); | ||
| return n; | ||
| } | ||
| }; |
There was a problem hiding this comment.
These are clever. And I like the way they clean up the code. But @scottschurr pointed out to me a potential to cause a run-time error: Subexpressions in C++ are not guaranteed to be executed in any given order. See: https://en.cppreference.com/w/cpp/language/eval_order#:~:text=Order%20of%20evaluation%20of%20any,same%20expression%20is%20evaluated%20again.
For example in:
z = downward()(a * b) / upward()(c - d);I am concerned that the order could be:
- construct downward
- construct upward
- execute a * b
- execute c - d
- call the downward() with a*b
- call the upward with c-d
- do the division
When I tried to simulate such an order with clang, I failed to do so. But even if all of our compilers don't create a run-time error now, if they started to in the future, the error would be silent, and extremely subtle, hard to detect and hard to find and fix.
There was a problem hiding this comment.
Thanks for investigating. So have to break expressions into subexpressions. The functor would still work to make it less verbose, right? I.e.
num = downard()(a * b);
den = upward()(c - d);
res = downard() (num / den);
There was a problem hiding this comment.
Yes, that will work. We will have to watch out for maintenance in the future to make sure it doesn't combine such expressions.
For other's, here's what the above looks like without these helpers:
saveNumberRoundMode _{Number::setround(Number::downward)};
num = a * b;
Number::setround(Number::upward);
den = c - d;
Number::setround(Number::downward);
res = num / den;The basic rule for both of these styles is that we need a ; between calls to Number::setround.
There was a problem hiding this comment.
I'm wondering if parenthesis are going to force the order, for instance. Also note additional downward() to set the division rounding.
z = downward()( (downward()(a * b) ) / ( upward()(c - d) ) )
There was a problem hiding this comment.
Well, if you can find a place in the standard that says parenthesis will guarantee that order, please add that reference to the source code in a comment. I don't know the standard well enough to have a sense for where you might find such a guarantee. However there are several places in the standard that say, to a large extent, order of evaluation within an expression is unspecified.
The standard has specific rules about constructors and destructors. That's why RAII works. But here we have a constructor and the function call operator. It's not the same thing.
There was a problem hiding this comment.
Yes, I'm pretty sure that in your example downward() will be evaluated first.
However I'm most concerned about @HowardHinnant's example at the top of this discussion. If both upward() and downward() are constructed before either of the functors are called then one of the computations will round the wrong way. The only way I personally know of to guarantee order of construction is with a sequence point.
The order of evaluation discussion says very little about when constructors will be called. My experience with undefined behavior has made me leery of surprises that optimizers can bring. But I haven't found wording that proves that the compiler will do the wrong thing.
There was a problem hiding this comment.
I give up (for now)!
There was a problem hiding this comment.
It seems like the direction we're going is that upward() and downward() will be a lot less important. That's probably for the best. However I cobbled up an approach that is not as pretty as the original upward() and downward() but includes the necessary sequence points. It leans heavily on lambdas. The code looks like this:
template<Number::rounding_mode Rm, typename Functor>
requires std::is_invocable_v<Functor>
auto
numberRounder(Functor&& f)
{
saveNumberRoundMode guard(Number::getround());
Number::setround(Rm);
return std::forward<Functor>(f)();
}
template<typename Functor>
requires std::is_invocable_v<Functor>
auto
roundUp(Functor&& f)
{
return numberRounder<Number::upward>(std::forward<Functor>(f));
}
template<typename Functor>
requires std::is_invocable_v<Functor>
auto
roundDn(Functor&& f)
{
return numberRounder<Number::downward>(std::forward<Functor>(f));
}
Using it looks like this (including clang-formatting):
auto const nTakerPaysConstraint = roundDn([&] {
return pool.out * targetQuality.rate() -
roundUp([&] { return pool.in / f; });
});
Just for your consideration.
Also, I've found the two-phase use of saveNumberRoundMode to be a bit more awkward than necessary. So a while back I cobbled up something I find easier to use. The implementation is in STAmount.cpp, since that's the only place it's currently used:
class NumberRoundModeGuard
{
saveNumberRoundMode saved_;
public:
explicit NumberRoundModeGuard(Number::rounding_mode mode) noexcept
: saved_{Number::setround(mode)}
{
}
NumberRoundModeGuard(NumberRoundModeGuard const&) = delete;
NumberRoundModeGuard&
operator=(NumberRoundModeGuard const&) = delete;
};
I personally find NumberRoundModeGuard to be easier to think about and use than saveNumberRoundMode. If that's true for you as well, then I encourage you to move NumberRoundModeGuard into the Number.h header and use it freely.
There was a problem hiding this comment.
Thanks for both suggestions! I was considering lambdas but thought it would not be easy to read. But now that I look at it, I think it's a good alternative.
src/ripple/app/misc/AMMHelpers.h
Outdated
| * The above equation produces the quadratic equation: | ||
| * i^2*(1-fee) + i*I*(2-fee) + I^2 - I*O/quality, | ||
| * which is solved for i, and o is found with swapAssetIn(). | ||
| namespace { |
There was a problem hiding this comment.
Anonymous namespaces in header files aren't that useful. Consider giving this a name like AMMHelpersDetail or somesuch ("detail" should be in the name though to signal it's private)
There was a problem hiding this comment.
Another choice that's been used elsewhere in the code base would be to use namespace detail, rather than the anonymous namespace. I don't feel strongly about it, just presenting an option.
There was a problem hiding this comment.
I feel pretty strongly against anonymous namespaces in header files, because of internal linkage. Fine if everything inside such a namespace is explicitly static but otherwise can lead to ODR.
| Number::setround(mode_); | ||
| return n; | ||
| } | ||
| }; |
There was a problem hiding this comment.
It's a shame the functor syntax won't work, I liked that a lot.
Doesn't everything except:
Number z = div(mul(a, b, downward), sub(c, d, upward), downward);
Run into the order of operations problem? (Even if it doesn't I don't love the dwn(dwn(a) + b)) / dwn(up(c) - d) syntax).
One thought: if we used expression templates we could use:
z = downward(downward(a * b) / upward(c - d))
And get around the order of operations issue. However, expression templates aren't a lot of fun to write or debug. There are serious downsides to going that way. I'm probably opposed to doing that, but maybe it will spark an idea from someone else. (ref: expression templates: https://en.wikipedia.org/wiki/Expression_templates)
| if (d < 0) | ||
| return std::nullopt; | ||
| return downward()( | ||
| downward()(-b + downward()(root2(d))) / upward()(2 * a)); |
There was a problem hiding this comment.
There are more numerically stable forms of the quadratic formula. For example, look for the "Citardauq Formula"
|
|
||
| // Might fail due to finite precision. Should the relative difference be | ||
| // allowed? | ||
| if (Quality{*amounts} < quality) |
There was a problem hiding this comment.
Since we're defining rounding to favor the AMM, won't this trigger almost all the time? I.e. I'd expect the computed quality to be less than the requested quality (by tiny amounts, of course). I might be wrong on that, but if I am can you leave a comment in the code explaining why we don't expect to hit this very often?
There was a problem hiding this comment.
There are two scenario. One is - AMM offer is generated so that the new Spot Price Quality is equal to the requested quality. In this case the offer's quality is better than updated SPQ. The second is - if there is a trading fee then matching the new SPQ to the target quality will push AMM offer quality bellow the target quality. In this case the AMM offer is generated to match the target quality and SPQ quality is going to be better than the target quality. In both cases we start the offer generation with the XRP side (if applicable) and we round it downward. This minimizes the offer size and maximizes the quality. If the offer is IOU/IOU, we still round downward the side of the offer that we start generating first. This again minimizes the offer and maximizes the quality. So the scenario when we succeed in the offer generation but it's quality is less than the target quality should be pretty rare. It's still possible though and perhaps we should add some error allowance.
src/ripple/app/misc/AMMHelpers.h
Outdated
| Quality const& targetQuality, | ||
| std::uint16_t const& tfee) | ||
| { | ||
| assert(targetQuality.rate() != beast::zero); |
There was a problem hiding this comment.
In addition to asserting, consider returning a nullopt as well, so release builds don't crash.
There was a problem hiding this comment.
I recently discovered that order book offers with actual rates of zero exist in the wild! One such offer was XRP(5) for WTF(1e-80). There are other examples in the ledger.
I'm not really in on how the targetQuality passed into this function is calculated. But if it has any resemblance to order book offer quality calculations, then an assert would be wrong -- it's not a programming error, it's just an extreme exchange rate. And people horsing around with the ledger can produce these.
But if it's really impossible for the AMM to produce a targetQuality rate of zero, then the assert is okay. Your call.
There was a problem hiding this comment.
Not related to the actual quality recorded in the directory (or this PR), but the values of TakerGets and TakerPays both being zero, here are a set of example Offer Ledger Entries where that condition is true:
408991C44992F2C09EF01D87F66B1690C9B420339D36ABB3DB95B6B11E12C60E
ECAF61B8D247751276754D7534368AB1FD67AB19CB085E5F7F3D65858556EBDC
2E596CDA5D5CB57A8A32809EC151F9B6204ABB2ADD0E66E3EF98EB5F8D940EE2
0415142A08035D2851C8FC082585AB39A281A7D9A3A5ED486DE75DBA8EF638C9
870C021307BC50F2B81F610F88C6EEF7427C7A60F5FAEEA065CEFAE509EBB280
F2AD2E9FA24C7F1337C6D25D422C648799400023DB0D13A6AD4431C7DCD902C8
DE627244B71834D41B4CADC665FC4C9BABD750CD10F98D9EFD6CFAA82FD6FD09
3473D44B0E170E012DE021A619342B672F2A09BC2BE26EACA7EB3D3292C716C6
4A995EA0EEA4A299A0C22EE356E667D5E506F6E475E01F80B9E9DF1200469A3A
B547CE784C3081D3758FC80196AFD1A2B01A0E3CC679B4D75570A1D6F61EFB6D
0BFF9D102B5BF605D69347DA5575FD19C4B804D01D11569BD7C2D730214832B8
B7159EDEC0001D0EA57299C1068C7CBD0982FFC7EF1733D73C86B2FDE872976B
0D0AF5A21BC7E4195E33F3626DF8243421EED537C5EC33806C9A291B9B5FF98F
0E3637D85C28DF22B8372FC15238564A12689D12F074817C81998D423AA8797B
243112506E7075F25C87FD4853632AE7311A3280EFDE3C8043BF5E2116502480
BBD613C560E2382ACA38518679B86D0FA9193D9A576081125F17536DE8BE6D3F
A696015F48F3F1F39BB0DAD7268909B5B800951DD842752E038AD5A4A10F082A
7BFF910E30475CB1030AB2B3A899D255326E63AA24B8DEC29C7CF8CBE8D25E72
273FAC5F744BE3DCC8632AD9CF510FEC24E762D5387CF6144D6D3838C88DC2F4
7E80B182412941CE63AFDD1478104FB20670E839326194C0A0E0E3FC9E46246A
For example:
rippled -q ledger_entry DE627244B71834D41B4CADC665FC4C9BABD750CD10F98D9EFD6CFAA82FD6FD09
{
"result" : {
"index" : "DE627244B71834D41B4CADC665FC4C9BABD750CD10F98D9EFD6CFAA82FD6FD09",
"ledger_current_index" : 87841488,
"node" : {
"Account" : "rbdssUA9ztShhv8ZA2zqd5W44VrJ5xUpK",
"BookDirectory" : "24F71D31B9A8F0F1A4E9F19530986702713CE751513AFB324F06A02F816907EC",
"BookNode" : "0",
"Flags" : 0,
"LedgerEntryType" : "Offer",
"OwnerNode" : "0",
"PreviousTxnID" : "A51D818EAF5541D2D724BEA95FF3835F0607C8BB46DDCB93DCA407AE43BB255A",
"PreviousTxnLgrSeq" : 10428569,
"Sequence" : 5,
"TakerGets" : "0",
"TakerPays" : {
"currency" : "JPY",
"issuer" : "rbdssUA9ztShhv8ZA2zqd5W44VrJ5xUpK",
"value" : "0"
},
"index" : "DE627244B71834D41B4CADC665FC4C9BABD750CD10F98D9EFD6CFAA82FD6FD09"
},
"status" : "success",
"validated" : false
}
}
src/ripple/app/misc/AMMHelpers.h
Outdated
| // a | ||
| // a must be maximized and minimized. a is 1, no rounding | ||
| // b must be maximized and minimized. choose minimize since the effect of | ||
| // the rounding outside of root2 is greater than the inside of root2. |
There was a problem hiding this comment.
Except b is squared inside the root2. What you're saying may still be true, but it's not immediately clear to me that it is.
There was a problem hiding this comment.
To put it differently, we maximize the left side of the expression in several ways: b * b is maximized, 4 * a * c is minimized, b * b - 4 * a * c is maximized and then the root is maximized as well. So I think the overall minimization effect is going to be greater if b is rounded downward. If this the right way, then b has to be actually rounded upward because b is negative in this case; i.e. we need to make the absolute b to be smaller.
There was a problem hiding this comment.
You may be right, but let me think this through carefully and let's leave this issue open for now.
There was a problem hiding this comment.
Here's what I did in sage:
sage: def q(a,b,c): return -b+sqrt(b*b-4*a*c)
sage: small_delta = 10^-20
sage: large_delta = 9*10^-20
sage: R=RealField(400)
sage: R(q(1,10+large_delta,1)) > R(q(1,10-small_delta,1))
True
I think that shows that if we round up we can end up with larger values than if we round down. In this case, it's simulating rounding 10.00...001 up to 10.00...01 or down to 10.00...000). When I round b down I get a smaller value. It's possible I messed up this calculation (please dummy check me), but I'm still not convinced we'll find a single rounding for b that minimizes this. We may need one rounding inside the root2 and one outside the root2.
There was a problem hiding this comment.
A few things:
- The result is negative given
a,b,cvalues. - There is a relation between
bandc. I think a real test should take it into consideration. - How does this test factor in other rounding that I mentioned?
- Doesn't having different
bkind of breaks the basic algebra?bmust be the same in quadratic equation solution . x = (-b1 +- sqrt(b2*b2 - 4 * a *c)) / (2 * a) is not a valid quadratic equation solution even ifb1andb2differ by a small delta. Am I missing something?
There was a problem hiding this comment.
This makes sense to me.
There was a problem hiding this comment.
I'm really poking at this statement:
b must be maximized and minimized. choose minimize since the effect of
the rounding outside of root2 is greater than the inside of root2
It's not obvious to me that that's true, and I think the test I wrote shows that it's not always true. If we really do want to choose the rounding that minimizes the whole expression sometimes we'll have to round up, and sometimes we'll have to round down (I think). There may be other constraints that I'm not considering (although I don't think a relationship between b and c would matter, but maybe it does.)
So my real concern is that we always honor the invariant. If we can show that the invariant is honored in all cases, then we're good. An easy way to do that is to show we always set roundings to favor the AMM. If we can't do that then we have a much harder analysis job to do. (or we can give up trying to honor the invariant, but I'd rather not do that). Anyway, let's talk some more tomorrow about this.
There was a problem hiding this comment.
The invariant is honored in all cases. The reason for the rounding in this function is to minimize the generated offer so that the quality matches the provided target quality. This is accomplished by rounding downward the side of the offer that we start generating first. It doesn't matter which side it is - the smaller the amount the better the quality. The other side of the offer is calculated with either swapAssetIn or swapAssetOut. Those functions ensure the invariant is honored and round the result downward (to minimize takerGets) or upward (to maximize takerPays) respectively.
There was a problem hiding this comment.
As long as the invariant is honored I'm good. And I agree we want the most accurate result while honoring the invariant.
There was a problem hiding this comment.
I'm going to change the rounding per @HowardHinnant suggestion and address all current comments.
| // limit quality to prevent AMM being blocked by a lower quality | ||
| // LOB. | ||
| auto const bestQuality = [&]() -> std::optional<Quality> { | ||
| if (sb.rules().enabled(fixAMMRounding) && lobQuality) |
There was a problem hiding this comment.
This change doesn't relate to "AMMRounding". We might want to rename the feature.
There was a problem hiding this comment.
For NFTs there was a rollup of three different fixes named fixNonFungibleTokensV1_2. Perhaps something like fixAMMv1_1 would work?
There was a problem hiding this comment.
We could think about rolling up the other AMM rounding amendment that we added to 2.2 into this one as well.
There was a problem hiding this comment.
We only have fixAMMRounding in 2.2, no?
src/ripple/app/misc/AMMHelpers.h
Outdated
| // fee is always maximized | ||
| // feeMult is always minimized | ||
| auto const fee = upward()(getFee(tfee)); | ||
| auto const feeMult = downward()(1 - fee); |
There was a problem hiding this comment.
Just something to consider that might improve readability. There's a feeMult() function called in this file (defined in AMMCore.h). There are also just a few locals in this file named feeMult, like this one. Also, it seems to me that, throughout most of this file, locals filling the role of feeMult are named f.
Consider renaming those few locals in this file named feeMult to f. This may increase naming consistency in the file. It might also reduce cognitive dissonance caused by a local name and a function name being the same.
|
I pushed commit, which should address all feedback. The commit message details all the updates, but the main change is that the rounding is changed to |
src/ripple/app/misc/AMMHelpers.h
Outdated
| Quality const& targetQuality, | ||
| std::uint16_t const& tfee) | ||
| { | ||
| assert(targetQuality.rate() != beast::zero); |
There was a problem hiding this comment.
I recently discovered that order book offers with actual rates of zero exist in the wild! One such offer was XRP(5) for WTF(1e-80). There are other examples in the ledger.
I'm not really in on how the targetQuality passed into this function is calculated. But if it has any resemblance to order book offer quality calculations, then an assert would be wrong -- it's not a programming error, it's just an extreme exchange rate. And people horsing around with the ledger can produce these.
But if it's really impossible for the AMM to produce a targetQuality rate of zero, then the assert is okay. Your call.
src/ripple/app/misc/AMMHelpers.h
Outdated
| if (nTakerPaysConstraint < *nTakerPays) | ||
| nTakerPays = nTakerPaysConstraint; | ||
|
|
||
| auto getAmounts = [&](Number const& nTakerPays) { |
There was a problem hiding this comment.
This param shadows the local variable. Consider changing the name.
src/ripple/app/misc/AMMHelpers.h
Outdated
| if (nTakerGetsConstraint < *nTakerGets) | ||
| nTakerGets = nTakerGetsConstraint; | ||
|
|
||
| auto getAmounts = [&](Number const& nTakerGets) { |
There was a problem hiding this comment.
This param shadows the local variable, consider changing the name.
src/ripple/app/misc/AMMHelpers.h
Outdated
| if (nTakerGetsConstraint < *nTakerGets) | ||
| nTakerGets = nTakerGetsConstraint; | ||
|
|
||
| auto getAmounts = [&pool, &tfee](Number const& nTakerGets_) { |
There was a problem hiding this comment.
I think we need a different fix here. Trailing underscores are for member variables.
src/test/app/AMMExtended_test.cpp
Outdated
| @@ -1441,7 +1458,7 @@ struct AMMExtended_test : public jtx::AMMTest | |||
| testOfferCreateThenCross(all); | |||
| testSellFlagExceedLimit(all); | |||
| testGatewayCrossCurrency(all); | |||
| testGatewayCrossCurrency(all - fixAMMRounding); | |||
| testGatewayCrossCurrency(all - fixAMMv1_1); | |||
| // testPartialCross | |||
There was a problem hiding this comment.
There are a number of commented out lines that appear to invoke tests that no longer exist? It would probably be good to remove those commented out lines. In total I see:
// testPartialCross
// testXRPDirectCross
// testDirectCross
// testSellOffer
// testSelfCrossLowQualityOffer
// testOfferInScaling
// testOfferInScalingWithXferRate
// testOfferThresholdWithReducedFunds
// testTinyOffer
// testSelfPayXferFeeOffer
// testSelfPayXferFeeOffer
// testRCSmoketest
I would guess that all of those commented out lines should be removed.
src/test/app/AMM_test.cpp
Outdated
| @@ -5557,6 +5651,217 @@ struct AMM_test : public jtx::AMMTest | |||
| false); | |||
| } | |||
|
|
|||
| void | |||
| testFixAMMOfferRounding(FeatureBitset features) | |||
There was a problem hiding this comment.
Since the amendment was renamed, is this still the right name for the test? If you do change the name consider also changing the testcase() text.
src/test/app/AMM_test.cpp
Outdated
| // error allowance, succeed post-fix | ||
| // because of offer resizing | ||
| FailShouldSucceed, // Fail in pre-fix due to rounding, | ||
| // succeed after fix because of XRP is |
There was a problem hiding this comment.
Nit: typo: "...because of XRP is side is generated first" -> "...because XRP side is generated first"
src/test/app/AMM_test.cpp
Outdated
| }; | ||
| // clang-format off | ||
| std::vector<std::tuple<std::string, std::string, Quality, std::uint16_t, Status>> tests = { | ||
| {"0.001519763260828713", "1558701", Quality{5414253689393440221}, 1000, Status::FailShouldSucceed}, |
There was a problem hiding this comment.
I find this table really hard to read. Personally, I prefer to format wide data tables like this into columns and put headings at the top. Unfortunately the Quality column gets really wide here because of the TAmounts. But, even with that, I still think it improves readability.
You can also get rid of the repeated Status:: entry in each line by putting...
using enum Status;
immediately below the enum declaration.
With these suggestions, then the first three lines of the table might look like this:
std::vector<std::tuple<std::string, std::string, Quality, std::uint16_t, Status>> tests = {
// Pool In, Pool Out, Quality, Fee, Status
{"0.001519763260828713", "1558701", Quality{ 5414253689393440221}, 1000, FailShouldSucceed},
I find it easier to read and think about. But your milage may vary. I'm not insisting on this change. Your call.
A single-path AMM offer with account offer on DEX, is always generated starting with the takerPays first, which is rounded up, and then the takerGets, which is rounded down. This rounding ensures that the pool's product invariant is maintained. However, when one of the offer's side is XRP, this rounding can result in the AMM offer having a lower quality, potentially causing offer generation to fail if the quality is lower than the account's offer quality. To address this issue, the proposed fix adjusts the offer generation process to start with the XRP side first and always rounds it down. This results in a smaller offer size, improving the offer's quality. Regardless if the offer has XRP or not, the rounding is done so that the offer size is minimized. This change still ensures the product invariant, as the other generated side is the exact result of the swap-in or swap-out equations. If a liquidity can be provided by both AMM and LOB offer on offer crossing then AMM offer is generated so that it matches LOB offer quality. If LOB offer quality is less than limit quality then generated AMM offer quality is also less than limit quality and the offer doesn't cross. To address this issue, if LOB quality is better than limit quality then use LOB quality to generate AMM offer. Otherwise, don't use the quality to generate AMM offer. In this case, limitOut() function in StrandFlow limits the out amount to match strand's quality to limit quality and consume maximum AMM liquidity.
If a liquidity can be provided by both AMM and LOB offer on offer crossing then AMM offer is generated so that it matches LOB offer quality. If LOB offer quality is less than limit quality then generated AMM offer quality is also less than limit quality and the offer doesn't cross. This commit addresses this issue but selecting the best quality out of LOB and limit quality when generating AMM offer.
* Change rounding to nearest in getAMMOfferStartWithTaker[Gets,Pays]() * Reduce offer size to 99.99% if fail to match targetQuality * Use numerically stable citardauq formulas to solve quadratic equation * Rename amendment to fixAMMv1_1 * Move NumberRoundModeGuard to Number.h * Update testFixAMMOfferRounding() unit-tests * Update comments
* Check targetQuality rate is not zero * Shorten rounding mode reference * Update comments * Rename shadowed variable
limitQuality value defines the strand's average quality. If it is used to generate the AMM offer than less AMM liquidity is going to be consumed since AMM's spot price quality is going to match limitQuality and the generated offer's quality is going to be better than limitQuality. The fix is - if LOB quality is better than limitQuality then use LOB quality to generate AMM offer. Otherwise, don't use the quality to generate AMM offer. In this case, limitOut() function in StrandFlow limits the out amount to match strand's quality to limitQuality and consume maximum AMM liquidity.
gregtatcam
force-pushed
the
amm-offer-rounding-fix
branch
from
11f7b6d to
106982f
Compare
intelliot
changed the title
fixAMMv1_1: Add the fixAMMOfferRounding amendment (subsequently renamed to fixAMMv1_1)
intelliot
changed the title
fixAMMv1_1: Add the fixAMMOfferRounding amendment (subsequently renamed to fixAMMv1_1)fixAMMv1_1: improve the quality of the generated AMM offer in certain cases
High Level Overview of Change
Introduces
fixAMMOfferRounding(subsequently combined with #5016 and renamed tofixAMMv1_1) amendment to improve quality of the generated AMM offer for a single path payment when one side of the offer is XRP and there is a Limit Order Book (LOB) offer for the same asset pair on the DEX.Context of Change
A single-path AMM offer with account offer on DEX, is always generated
starting with the takerPays first, which is rounded up, and then
the takerGets, which is rounded down. This rounding ensures that the pool's
product invariant is maintained. However, when one of the offer's side
is XRP, this rounding can result in the AMM offer having a lower
quality, potentially causing offer generation to fail if the quality
is lower than the account's offer quality.
To address this issue, the proposed fix adjusts the offer generation process
to start with the XRP side first and always rounds it down. This results
in a smaller offer size, improving the offer's quality.
This change still ensures the product invariant, as the other generated
side is the exact result of the swap-in or swap-out equations.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
A unit-test
testFixAMMOfferRounding()is added to AMM_test to verify this amendment