Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixUniversalNumber: Number merge #4192

Merged
merged 13 commits into from
Feb 7, 2023
Merged

Conversation

HowardHinnant
Copy link
Contributor

High Level Overview of Change

Use Number for IOUAmount and STAmount arithmetic

* Guarded by amendment fixUniversalNumber
* Produces slightly better accuracy in some computations.

Context of Change

There are two many decimal floating point classes in rippled, and AMM needed a third. This merges common code. A side effect is that some transaction arithmetic becomes more accurate in the final digit or two. Therefore this change is guarded by an amendment.

The first many commits introduce Number. Only the final commit merges the common code with IOUAmount and STAmount, and introduces the amendment.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Test Plan

Some tests are updated with the slightly more accurate results.

@zhangdongwei123
Copy link

这个是关于ripple还是radar?

@scottschurr
Copy link
Collaborator

@zhangdongwei123, this repository is for the XRP Ledger and has nothing to do with radar.

@zhangdongwei123
Copy link

雷达还开吗?

@scottschurr
Copy link
Collaborator

@zhangdongwei123, you are asking at the wrong site. This site has no connection with radar. We don't know whether radar is working or not. Please find somewhere else to ask.

@seelabs
Copy link
Collaborator

seelabs commented Jun 29, 2022

@HowardHinnant I'm seeing two unit tests fail (the first is this one: #2 failed: Taker_test.cpp(238)) I haven't dug into what that test is doing, but I wanted to make a note before I dug into the code.

@HowardHinnant
Copy link
Contributor Author

Looking into it now, thanks.

@HowardHinnant
Copy link
Contributor Author

This new commit fixes the failures you observed @seelabs. For reasons that are not clear to me, the Taker test ran with the amendment on in non-unity build, and with the amendment off in unity build. The patch forces the amendment on in the Taker test for both non-unity and unity builds.

@HowardHinnant
Copy link
Contributor Author

Rebased to 1.9.2-rc1.

@HowardHinnant
Copy link
Contributor Author

Rebased to 1.9.2.

return 1;
return -1;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler warning: Number.cpp:159:1: warning: control reaches end of non-void function [-Wreturn-type]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f099878

Number y{1'000'000'000'000'001'500, 32000};
BEAST_EXPECT((y == Number{1'000'000'000'000'002, 32003}));
Number m{std::numeric_limits<std::int64_t>::min()};
BEAST_EXPECT((m == Number{-9'223'372'036'854'776, 3}));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on Ubuntu 20.04.1 (ARM64). m is 0. Should this architecture be supported?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it also failed on Ubuntu 20.04.4 Intel architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this report. Is m == 0 on the Intel architecture as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. I'm reproducing it now. This is a bug somewhere in Number as it only reproduces with optimizations on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7e10665.

Taking the negative of the most negative signed value is undefined behavior, not identity. However one can cast negative signed to unsigned and then negate, getting the desired result without UB.

The existing unit tests caught this, and should now pass.

#include <type_traits>
#include <utility>

#ifdef _MSVC_LANG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use BOOST_COMP_MSVC if possible; standardizing on that makes it easier to find all instances of this kind of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

Number::Guard::round() noexcept
{
auto mode = Number::getround();
switch (mode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change needed, but I think this would be more readable as standalone if statements:

    auto mode = Number::getround();

    if (mode == towards_zero)
        return -1;

    if (mode == downward)
    {
        if (sbit_)
        {
            if (digits_ > 0 || xbit_)
                return 1;
        }
        return -1;
    }

    if (mode == upward)
    {
        if (sbit_)
            return -1;
        if (digits_ > 0 || xbit_)
            return 1;
        return -1;
    }

    // assume round to nearest if mode is not one of the predefined values
    if (digits_ > 0x5000'0000'0000'0000)
        return 1;
    if (digits_ < 0x5000'0000'0000'0000)
        return -1;
    if (xbit_)
        return 1;
    return 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

assert(isnormal() && y.isnormal());
auto xm = mantissa();
auto xe = exponent();
int xn = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you plan on using 0 (i.e. having a tribool like thing) then I'd recommend turning this to a bool (same for yn) for better self-documenting code.

Copy link
Contributor Author

@HowardHinnant HowardHinnant Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I like the -1/1 int is because of this line:

    mantissa_ = xm * xn;

Yes, I could rewrite it with:

    if (xn)
        mantissa_ = -xm;
    else
        mantissa_ = xm;

but I really like the conciseness, and potential branch elimination of this formulation.

Comment on lines 433 to 441
do
{
rm2 = rm1;
rm1 = r;
r = r + r * (one - d * r);
} while (r != rm1 && r != rm2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we confident that this will always terminate for every possible input? Should we have a counter to place some reasonable upper limit on the number of iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, both by theory and testing.

Theory: The Newton algorithm will reliably point towards the right answer. If that right answer is between two representable values, and the starting guess is one of those representable values, then the algorithm either gives back the same answer, or the other representable value closest to the exact answer. It never "wanders" away.

Practice: The set up to this look reduces the domain to [1, 10). Very significant testing was done throughout this small finite range, especially near the endpoints, and at the point where the difference between the initial guess and the exact answer is at a maximum.

I also tried going with a fixed number of iterations. However testing revealed that the required number of iterations varied enough that I felt it was too expensive to hard code the maximum number of iterations when many inputs in the range [1, 10) required fewer than the maximum.

In the end, among these four approaches:

  1. Hard code the number of iterations: Rejected because there was no one number of iterations that was acceptable.
  2. Stop when r == rm1. Rejected because the loop sometimes did not terminate because the algorithm would bounce between two values.
  3. Stop when r == rm1 || r == rm2. Accepted as the highest performance alternative while maintaining correctness.
  4. Stop when r == rm1 || r == rm2 || r == rm3. Although correct, a history of three values was never observed to be needed. Rejected.

@@ -2117,7 +2117,7 @@ class Offer_test : public beast::unit_test::suite
jrr = ledgerEntryState(env, bob, gw, "USD");
BEAST_EXPECT(
jrr[jss::node][sfBalance.fieldName][jss::value] ==
"-0.966500000033334");
"-0.9665000000333333");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concerns me: should this change even if the new Number logic amendment is not activated? Can we check both with and without please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will attempt to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/ripple/basics/Number.h Show resolved Hide resolved
src/ripple/basics/Number.h Show resolved Hide resolved
@@ -43,6 +45,17 @@ IOUAmount::minPositiveAmount()
void
IOUAmount::normalize()
{
if (*stNumberSwitchover)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the check for if (mantissa_ == 0) can safely be above this. Not sure if it's worth doing, but there's nothing that the Number back-and-forth will achieve in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

IOUAmount&
IOUAmount::operator+=(IOUAmount const& other)
{
if (other == beast::zero)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before: if there's nothing to be done, we should void round-tripping via Number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

inline constexpr Number
Number::operator+() const noexcept
{
return *this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL... or at least was reminded: the unary + operator, unlike unary -, leaves its argument completely unchanged.

if (x.mantissa_ == 0)
return y.mantissa_ > 0;

// Both have same sign, the right is zero and the left is non-zero.
Copy link
Contributor

@greg7mdp greg7mdp Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should comment be // Both are positive ... instead of // Both have same sign ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. At this point they both could be negative, or they both could be non-negative. I.e. lneg == rneg.

Copy link
Contributor

@greg7mdp greg7mdp Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the same comment says "the right is zero" which means the sign they both have is positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not married to this code. I lifted it straight from IOUAmount.cpp:

bool
IOUAmount::operator<(IOUAmount const& other) const
{
// If the two amounts have different signs (zero is treated as positive)
// then the comparison is true iff the left is negative.
bool const lneg = mantissa_ < 0;
bool const rneg = other.mantissa_ < 0;
if (lneg != rneg)
return lneg;
// Both have same sign and the left is zero: the right must be
// greater than 0.
if (mantissa_ == 0)
return other.mantissa_ > 0;
// Both have same sign, the right is zero and the left is non-zero.
if (other.mantissa_ == 0)
return false;
// Both have the same sign, compare by exponents:
if (exponent_ > other.exponent_)
return lneg;
if (exponent_ < other.exponent_)
return !lneg;
// If equal exponents, compare mantissas
return mantissa_ < other.mantissa_;
}

I suppose a more precise comment might be:

Both x and y have the same sign. If the left is 0, then y must also be non-negative. So y is either equal to x (equal to 0), or y is positive and thus greater than x (which is 0).

If the comment is really causing problems, I'd be more in favor of just removing it. For my money the code speaks clearly enough for itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally up to you, Howard. I kinda like your latest comment but it is a mouthful.

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, you write outstanding code. Always fun to review it.

I left a couple comments.

src/ripple/basics/Number.h Show resolved Hide resolved
src/ripple/basics/Number.h Show resolved Hide resolved
src/ripple/basics/Number.h Show resolved Hide resolved
setround(rounding_mode mode);

private:
static thread_local rounding_mode mode_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about co-routines what might resume on different threads. If that happens they can be resumed with a different rounding mode than what they were suspended with. If this can never happen, we should at least have a comment explaining why. If it can, we probably need to handle that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions here.

It looks like co-routines resuming on different threads is an explicit design for the co-routine author, and not something that happens by accident. So we could just document "don't do that" for the Number class.

Changing rounding_mode to be static doesn't seem like a good idea because that means all computations using Number (on different threads) will be forced to share the same rounding mode.

Changing rounding_mode to be a non-static data member isn't practical as there will be many Number instances participating in a single expression.

Copy link
Collaborator

@seelabs seelabs Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the rounding mode is used exactly once in non-test code: in AMM_formula.h. How would the code look if we made the rounding mode a template parameter and made it part of the type instead of a thread local? It sounds like all but one place uses the same rounding mode. If this is true, that might work out well.

Edit: I think the only code that changes rounding mode is here:

Number::setround(mode);
auto const res = STAmount{issue, (std::int64_t)n};
Number::setround(Number::rounding_mode::to_nearest);
return res;

And I'm assuming the ending Number::setround(Number::rounding_mode::to_nearest) is unintentional and it's supposed to set to the previous rounding mode. If it's intentional, then my suggestion doesn't work well. Although if it's intentional then I don't love that code in AMM_formulae` and will probably see what we can do to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If to_nearest is the only mode ever set, we don't need this facility because to_nearest is the default, and what was in place prior to the introduction of Number::setround.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other rounding modes are used in AMM when swapping in/out of the pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks.

bool const negative = (mantissa_ < 0);
auto m = static_cast<std::make_unsigned_t<rep>>(mantissa_);
if (negative)
m = -m;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In several algorithms, we negate a number. Negating the largest negative value doesn't actually negate it. Can you add a comment near each negation explaining why we never see the largest negative value or what the consequences of that value would be for the algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just recently became aware that negating the most negative signed value is undefined behavior, and the gcc optimizer takes advantage of that fact, producing code that changes behavior compared to non-optimized code.

Because of this I inserted line 179:

    auto m = static_cast<std::make_unsigned_t<rep>>(mantissa_);

I'm negating an unsigned integral, not a signed integral. And this is well-defined for all values.

This language design makes no sense to me at all, resulting in code that is much more difficult to read and reason about. However I am now using this poorly designed part of the language correctly.

src/ripple/basics/impl/IOUAmount.cpp Show resolved Hide resolved
*this = Number{};
return *this;
}
assert(isnormal() && y.isnormal());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to remove creating unnormalized numbers? If we can't, maybe we should change this assert so it works on normalized numbers if we ever hit this case. (here and other operators that assume normalized inputs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this definition of isnormal() the only de-normalized number that is allowed is 0, which should be taken care of by the checks above this assert. If this assert fires, there is a serious bug in Number and should be investigated immediately.

One way for de-normalized numbers to arise is via the "unchecked" constructor. Clients that use this constructor should take care to only construct normalized numbers. This constructor is an optimization for creating constants, while avoiding the normalization process which the coder can do manually, for example:

    constexpr Number a2{1405502114116773, -17, unchecked{}};
    static_assert(a2.isnormal());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HowardHinnant , maybe you you use if (std::is_constant_evaluated()) within the "unchecked" constructor to perform the static_assert(isnormal());?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe just static_assert(!std::is_constant_evaluated() || isnormal());?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a possibility, but doing so would not eliminate the motivation for these asserts.

src/ripple/basics/impl/Number.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/impl/ShardArchiveHandler.cpp Show resolved Hide resolved
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 7, 2023
@intelliot intelliot assigned HowardHinnant and unassigned seelabs Feb 7, 2023
@intelliot
Copy link
Collaborator

Just needs conflicts to be resolved @HowardHinnant . There is no need to rebase (although that is fine if that's what you prefer) - merge commits are OK since they will be squashed anyway.

* Conversions to Number are implicit
* Conversions away from Number are explicit and potentially lossy
* If lossy, round to nearest, and to even on tie
* Return 0 if abs(x) < limit, else returns x
* Guarded by amendment fixUniversalNumber
* Produces slightly better accuracy in some computations.
You can set a thread-local flag to direct Number how to round
non-exact results with the syntax:

    Number::rounding_mode prev_mode = Number::setround(Number::towards_zero);

This flag will stay in effect for this thread only until another call
to setround.  The previously set rounding mode is returned.

You can also retrieve the current rounding mode with:

    Number::rounding_mode current_mode = Number::getround();

The available rounding modes are:

* to_nearest : Rounds to nearest representable value.  On tie, rounds
               to even.
* towards_zero : Rounds towards zero.
* downward : Rounds towards negative infinity.
* upward : Rounds towards positive infinity.

The default rounding mode is to_nearest.
* Taking the negative of a signed negative is UB, but
  taking the negative of an unsigned is not.
* Replace division with faster algorithm.
* Correct some rounding bugs in multiplication.
* Add tests for rounding bugs.
* Optimization includes computing remainder from division.
* Used only within Number::operator*=.
@HowardHinnant HowardHinnant force-pushed the NumberMerge branch 2 times, most recently from 4dc0206 to 2f11eb3 Compare February 7, 2023 20:16
@HowardHinnant
Copy link
Contributor Author

Squashed and rebased to Develop (1.10.0-rc1). Builds and passes --unittest.

Three static member functions are introduced with
definitions consistent with std::numeric_limits:

static constexpr Number min() noexcept;

  Returns: The minimum positive value.  This is the value closest to zero.

static constexpr Number max() noexcept;

  Returns: The maximum possible value.

static constexpr Number lowest() noexcept;

  Returns: The negative value which is less than all other values.

friend constexpr bool
operator==(Number const& x, Number const& y) noexcept
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect the user to normalize the arguments before calling this operator? Would the following code work?

Number a{1,0};
Number b{1,-1}, c{9,-1};
assert(a == b+c);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Numbers are always normalized as a class invariant. There's a way to break that invariant by using unchecked in the constructor, but one shouldn't. unchecked is used as a tool to normalize manually at coding time in order to form a constexpr Number .

The assert passes in this example.

return false;

// Both have the same sign, compare by exponents:
if (x.exponent_ > y.exponent_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect that x and y have been normalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.


inline Number
operator+(Number const& x, Number const& y)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if x and y have the same exponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do not have to have the same exponent. Different exponents are handled within the += operator here: https:/XRPLF/rippled/blob/develop/src/ripple/basics/impl/Number.cpp#L256-L277

throw std::overflow_error("Number::power nan");
if (d == 0)
{
if (f == -one)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (f == -one)
if (f == abs(one))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case f == one is already handled on line 731. And f == -one must be handled separately because power(1, 0, 0) == 1 but power(-1, 0, 0) is nan and should throw an exception. Only if either n or d is not 0 (the exponent is not nan) should power(-1, n, d) == 1.

These corner cases were designed to be consistent with Annex F of the C standard, which is itself designed to be consistent with ISO/IEC 60559.

intelliot pushed a commit that referenced this pull request Jul 12, 2023
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]>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
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]>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Amendment Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants