Skip to content

Commit

Permalink
fix amendment: AMM swap should honor invariants: (XRPLF#5002)
Browse files Browse the repository at this point in the history
The AMM has an invariant for swaps where:
new_balance_1*new_balance_2 >= old_balance_1*old_balance_2

Due to rounding, this invariant could sometimes be violated (although by
very small amounts).

This patch introduces an amendment `fixAMMRounding` that changes the
rounding to always favor the AMM. Doing this should maintain the
invariant.

Co-authored-by: Bronek Kozicki
Co-authored-by: thejohnfreeman
  • Loading branch information
seelabs authored and ximinez committed Apr 26, 2024
1 parent b65cea1 commit bd56a00
Show file tree
Hide file tree
Showing 10 changed files with 1,604 additions and 704 deletions.
133 changes: 119 additions & 14 deletions src/ripple/app/misc/AMMHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
#include <ripple/basics/Number.h>
#include <ripple/protocol/AMMCore.h>
#include <ripple/protocol/AmountConversions.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/Issue.h>
#include <ripple/protocol/Quality.h>
#include <ripple/protocol/Rules.h>
#include <ripple/protocol/STAccount.h>
#include <ripple/protocol/STAmount.h>

Expand Down Expand Up @@ -228,10 +230,62 @@ swapAssetIn(
TIn const& assetIn,
std::uint16_t tfee)
{
return toAmount<TOut>(
getIssue(pool.out),
pool.out - (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
Number::rounding_mode::downward);
if (auto const& rules = getCurrentTransactionRules();
rules && rules->enabled(fixAMMRounding))
{
// set rounding to always favor the amm. Clip to zero.
// calculate:
// pool.out -
// (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
// and explicitly set the rounding modes
// Favoring the amm means we should:
// minimize:
// pool.out -
// (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
// maximize:
// (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
// (pool.in * pool.out)
// minimize:
// (pool.in + assetIn * feeMult(tfee)),
// minimize:
// assetIn * feeMult(tfee)
// feeMult is: (1-fee), fee is tfee/100000
// minimize:
// 1-fee
// maximize:
// fee
saveNumberRoundMode _{Number::getround()};

Number::setround(Number::upward);
auto const numerator = pool.in * pool.out;
auto const fee = getFee(tfee);

Number::setround(Number::downward);
auto const denom = pool.in + assetIn * (1 - fee);

if (denom.signum() <= 0)
return toAmount<TOut>(getIssue(pool.out), 0);

Number::setround(Number::upward);
auto const ratio = numerator / denom;

Number::setround(Number::downward);
auto const swapOut = pool.out - ratio;

if (swapOut.signum() < 0)
return toAmount<TOut>(getIssue(pool.out), 0);

return toAmount<TOut>(
getIssue(pool.out), swapOut, Number::rounding_mode::downward);
}
else
{
return toAmount<TOut>(
getIssue(pool.out),
pool.out -
(pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
Number::rounding_mode::downward);
}
}

/** Swap assetOut out of the pool and swap in a proportional amount
Expand All @@ -250,11 +304,62 @@ swapAssetOut(
TOut const& assetOut,
std::uint16_t tfee)
{
return toAmount<TIn>(
getIssue(pool.in),
((pool.in * pool.out) / (pool.out - assetOut) - pool.in) /
feeMult(tfee),
Number::rounding_mode::upward);
if (auto const& rules = getCurrentTransactionRules();
rules && rules->enabled(fixAMMRounding))
{
// set rounding to always favor the amm. Clip to zero.
// calculate:
// ((pool.in * pool.out) / (pool.out - assetOut) - pool.in) /
// (1-tfee/100000)
// maximize:
// ((pool.in * pool.out) / (pool.out - assetOut) - pool.in)
// maximize:
// (pool.in * pool.out) / (pool.out - assetOut)
// maximize:
// (pool.in * pool.out)
// minimize
// (pool.out - assetOut)
// minimize:
// (1-tfee/100000)
// maximize:
// tfee/100000

saveNumberRoundMode _{Number::getround()};

Number::setround(Number::upward);
auto const numerator = pool.in * pool.out;

Number::setround(Number::downward);
auto const denom = pool.out - assetOut;
if (denom.signum() <= 0)
{
return toMaxAmount<TIn>(getIssue(pool.in));
}

Number::setround(Number::upward);
auto const ratio = numerator / denom;
auto const numerator2 = ratio - pool.in;
auto const fee = getFee(tfee);

Number::setround(Number::downward);
auto const feeMult = 1 - fee;

Number::setround(Number::upward);
auto const swapIn = numerator2 / feeMult;
if (swapIn.signum() < 0)
return toAmount<TIn>(getIssue(pool.in), 0);

return toAmount<TIn>(
getIssue(pool.in), swapIn, Number::rounding_mode::upward);
}
else
{
return toAmount<TIn>(
getIssue(pool.in),
((pool.in * pool.out) / (pool.out - assetOut) - pool.in) /
feeMult(tfee),
Number::rounding_mode::upward);
}
}

/** Return square of n.
Expand All @@ -263,12 +368,12 @@ Number
square(Number const& n);

/** Adjust LP tokens to deposit/withdraw.
* Amount type keeps 16 digits. Maintaining the LP balance by adding deposited
* tokens or subtracting withdrawn LP tokens from LP balance results in
* losing precision in LP balance. I.e. the resulting LP balance
* Amount type keeps 16 digits. Maintaining the LP balance by adding
* deposited tokens or subtracting withdrawn LP tokens from LP balance
* results in losing precision in LP balance. I.e. the resulting LP balance
* is less than the actual sum of LP tokens. To adjust for this, subtract
* old tokens balance from the new one for deposit or vice versa for withdraw
* to cancel out the precision loss.
* old tokens balance from the new one for deposit or vice versa for
* withdraw to cancel out the precision loss.
* @param lptAMMBalance LPT AMM Balance
* @param lpTokens LP tokens to deposit or withdraw
* @param isDeposit true if deposit, false if withdraw
Expand Down
7 changes: 7 additions & 0 deletions src/ripple/basics/Number.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,13 @@ class Number
return x.mantissa_ < y.mantissa_;
}

/** Return the sign of the amount */
constexpr int
signum() const noexcept
{
return (mantissa_ < 0) ? -1 : (mantissa_ ? 1 : 0);
}

friend constexpr bool
operator>(Number const& x, Number const& y) noexcept
{
Expand Down
12 changes: 11 additions & 1 deletion src/ripple/ledger/impl/View.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1147,7 +1147,17 @@ accountSend(
beast::Journal j,
WaiveTransferFee waiveFee)
{
assert(saAmount >= beast::zero);
if (view.rules().enabled(fixAMMRounding))
{
if (saAmount < beast::zero)
{
return tecINTERNAL;
}
}
else
{
assert(saAmount >= beast::zero);
}

/* If we aren't sending anything or if the sender is the same as the
* receiver then we don't need to do anything.
Expand Down
52 changes: 46 additions & 6 deletions src/ripple/protocol/AmountConversions.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include <ripple/basics/XRPAmount.h>
#include <ripple/protocol/STAmount.h>

#include <type_traits>

namespace ripple {

inline STAmount
Expand Down Expand Up @@ -130,16 +132,44 @@ toAmount(
saveNumberRoundMode rm(Number::getround());
if (isXRP(issue))
Number::setround(mode);

if constexpr (std::is_same_v<IOUAmount, T>)
return IOUAmount(n);
if constexpr (std::is_same_v<XRPAmount, T>)
else if constexpr (std::is_same_v<XRPAmount, T>)
return XRPAmount(static_cast<std::int64_t>(n));
if constexpr (std::is_same_v<STAmount, T>)
else if constexpr (std::is_same_v<STAmount, T>)
{
if (isXRP(issue))
return STAmount(issue, static_cast<std::int64_t>(n));
return STAmount(issue, n.mantissa(), n.exponent());
}
else
{
constexpr bool alwaysFalse = !std::is_same_v<T, T>;
static_assert(alwaysFalse, "Unsupported type for toAmount");
}
}

template <typename T>
T
toMaxAmount(Issue const& issue)
{
if constexpr (std::is_same_v<IOUAmount, T>)
return IOUAmount(STAmount::cMaxValue, STAmount::cMaxOffset);
else if constexpr (std::is_same_v<XRPAmount, T>)
return XRPAmount(static_cast<std::int64_t>(STAmount::cMaxNativeN));
else if constexpr (std::is_same_v<STAmount, T>)
{
if (isXRP(issue))
return STAmount(
issue, static_cast<std::int64_t>(STAmount::cMaxNativeN));
return STAmount(issue, STAmount::cMaxValue, STAmount::cMaxOffset);
}
else
{
constexpr bool alwaysFalse = !std::is_same_v<T, T>;
static_assert(alwaysFalse, "Unsupported type for toMaxAmount");
}
}

inline STAmount
Expand All @@ -157,10 +187,15 @@ getIssue(T const& amt)
{
if constexpr (std::is_same_v<IOUAmount, T>)
return noIssue();
if constexpr (std::is_same_v<XRPAmount, T>)
else if constexpr (std::is_same_v<XRPAmount, T>)
return xrpIssue();
if constexpr (std::is_same_v<STAmount, T>)
else if constexpr (std::is_same_v<STAmount, T>)
return amt.issue();
else
{
constexpr bool alwaysFalse = !std::is_same_v<T, T>;
static_assert(alwaysFalse, "Unsupported type for getIssue");
}
}

template <typename T>
Expand All @@ -169,10 +204,15 @@ get(STAmount const& a)
{
if constexpr (std::is_same_v<IOUAmount, T>)
return a.iou();
if constexpr (std::is_same_v<XRPAmount, T>)
else if constexpr (std::is_same_v<XRPAmount, T>)
return a.xrp();
if constexpr (std::is_same_v<STAmount, T>)
else if constexpr (std::is_same_v<STAmount, T>)
return a;
else
{
constexpr bool alwaysFalse = !std::is_same_v<T, T>;
static_assert(alwaysFalse, "Unsupported type for get");
}
}

} // namespace ripple
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 72;
static constexpr std::size_t numFeatures = 73;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -359,6 +359,7 @@ extern uint256 const featurePriceOracle;
extern uint256 const fixEmptyDID;
extern uint256 const fixXChainRewardRounding;
extern uint256 const fixPreviousTxnID;
extern uint256 const fixAMMRounding;

} // namespace ripple

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ REGISTER_FEATURE(PriceOracle, Supported::yes, VoteBehavior::De
REGISTER_FIX (fixEmptyDID, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixXChainRewardRounding, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixAMMRounding, Supported::yes, VoteBehavior::DefaultNo);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/protocol/impl/Rules.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
namespace ripple {

namespace {
// Use a static inisde a function to help prevent order-of-initialization issues
// Use a static inside a function to help prevent order-of-initialization issues
LocalValue<std::optional<Rules>>&
getCurrentTransactionRulesRef()
{
Expand Down
Loading

0 comments on commit bd56a00

Please sign in to comment.