From 0d64a990f655fc17f288e6a13e6d2971239222bd Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 28 Aug 2019 14:01:15 -0400 Subject: [PATCH] [FOLD] Fix some issues with implicit XRPAmount conversions --- src/ripple/app/misc/FeeVoteImpl.cpp | 9 ++++-- src/ripple/app/misc/impl/LoadFeeTrack.cpp | 6 ++-- src/ripple/basics/XRPAmount.h | 32 ++++++++++++---------- src/ripple/ledger/ReadView.h | 2 +- src/ripple/ledger/detail/ApplyStateTable.h | 2 +- src/ripple/ledger/detail/RawStateTable.h | 2 +- src/ripple/protocol/SystemParameters.h | 4 ++- src/ripple/protocol/impl/STAmount.cpp | 6 +++- src/test/jtx/amount.h | 2 +- src/test/ledger/Invariants_test.cpp | 8 ++++-- 10 files changed, 43 insertions(+), 30 deletions(-) diff --git a/src/ripple/app/misc/FeeVoteImpl.cpp b/src/ripple/app/misc/FeeVoteImpl.cpp index 94513fe7dad..c8723e21942 100644 --- a/src/ripple/app/misc/FeeVoteImpl.cpp +++ b/src/ripple/app/misc/FeeVoteImpl.cpp @@ -172,7 +172,8 @@ FeeVoteImpl::doVoting( { auto const vote = val->getFieldU64 (sfBaseFee); if (isLegalAmount(vote)) - baseFeeVote.addVote (vote); + baseFeeVote.addVote(XRPAmount{ + unsafe_cast(vote)}); else // Invalid amounts will be treated as if they're // not provided. Don't throw because this value is @@ -186,7 +187,8 @@ FeeVoteImpl::doVoting( if (val->isFieldPresent (sfReserveBase)) { - baseReserveVote.addVote (val->getFieldU32 (sfReserveBase)); + baseReserveVote.addVote(XRPAmount{ + val->getFieldU32(sfReserveBase)}); } else { @@ -195,7 +197,8 @@ FeeVoteImpl::doVoting( if (val->isFieldPresent (sfReserveIncrement)) { - incReserveVote.addVote (val->getFieldU32 (sfReserveIncrement)); + incReserveVote.addVote (XRPAmount{ + val->getFieldU32 (sfReserveIncrement)}); } else { diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp index 8b891a040d3..f0ccf652558 100644 --- a/src/ripple/app/misc/impl/LoadFeeTrack.cpp +++ b/src/ripple/app/misc/impl/LoadFeeTrack.cpp @@ -90,7 +90,7 @@ scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack, Fees const& fees, bool bUnlimited) { if (fee == 0) - return 0; + return XRPAmount{0}; // Normally, types with different units wouldn't be mathematically // compatible. This function is an exception. @@ -149,8 +149,8 @@ scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack, assert(fee.value() >= baseFee.value()); // If baseFee * feeFactor overflows, the final result will overflow - constexpr XRPAmount max = - std::numeric_limits::max(); + constexpr XRPAmount max{ + std::numeric_limits::max()}; if (baseFee > max / feeFactor) Throw ("scaleFeeLoad"); baseFee *= feeFactor; diff --git a/src/ripple/basics/XRPAmount.h b/src/ripple/basics/XRPAmount.h index a82d42c74b6..7347e028888 100644 --- a/src/ripple/basics/XRPAmount.h +++ b/src/ripple/basics/XRPAmount.h @@ -45,9 +45,9 @@ struct dropTag; class XRPAmount : private boost::totally_ordered - , private boost::less_than_comparable , private boost::additive , private boost::equality_comparable + , private boost::additive , private boost::dividable , private boost::modable , private boost::unit_steppable @@ -94,7 +94,7 @@ class XRPAmount XRPAmount operator*(value_type const& rhs) const { - return { drops_ * rhs }; + return XRPAmount{ drops_ * rhs }; } friend @@ -141,6 +141,20 @@ class XRPAmount return *this; } + XRPAmount& + operator+= (value_type const& rhs) + { + drops_ += rhs; + return *this; + } + + XRPAmount& + operator-= (value_type const& rhs) + { + drops_ -= rhs; + return *this; + } + XRPAmount& operator*= (value_type const& rhs) { @@ -165,7 +179,7 @@ class XRPAmount XRPAmount operator- () const { - return { -drops_ }; + return XRPAmount{ -drops_ }; } bool @@ -186,18 +200,6 @@ class XRPAmount return drops_ < other.drops_; } - bool - operator<(std::uint64_t other) const - { - return drops_ < other; - } - - bool - operator>(std::uint64_t other) const - { - return drops_ > other; - } - /** Returns true if the amount is not zero */ explicit constexpr diff --git a/src/ripple/ledger/ReadView.h b/src/ripple/ledger/ReadView.h index 8b4872db1f3..15c5c79e017 100644 --- a/src/ripple/ledger/ReadView.h +++ b/src/ripple/ledger/ReadView.h @@ -69,7 +69,7 @@ struct Fees std::pair toDrops(FeeUnit64 const& fee) const { - return mulDiv(base.drops(), fee, units); + return mulDiv(base, fee, units); } }; diff --git a/src/ripple/ledger/detail/ApplyStateTable.h b/src/ripple/ledger/detail/ApplyStateTable.h index 38fde05d051..1bbfbe30e1c 100644 --- a/src/ripple/ledger/detail/ApplyStateTable.h +++ b/src/ripple/ledger/detail/ApplyStateTable.h @@ -51,7 +51,7 @@ class ApplyStateTable std::pair>>; items_t items_; - XRPAmount dropsDestroyed_ = 0; + XRPAmount dropsDestroyed_{0}; public: ApplyStateTable() = default; diff --git a/src/ripple/ledger/detail/RawStateTable.h b/src/ripple/ledger/detail/RawStateTable.h index 651b6b60ce5..fcd2d2836ce 100644 --- a/src/ripple/ledger/detail/RawStateTable.h +++ b/src/ripple/ledger/detail/RawStateTable.h @@ -95,7 +95,7 @@ class RawStateTable std::pair>>, false>>; items_t items_; - XRPAmount dropsDestroyed_ = 0; + XRPAmount dropsDestroyed_{0}; }; } // detail diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index 310219f180d..2bbd9ac74b8 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -55,7 +55,9 @@ bool isLegalAmount (XRPAmount const& amount) inline bool isLegalAmount (std::uint64_t amount) { - return amount <= INITIAL_XRP; + auto const initial = INITIAL_XRP.dropsAs(); + assert(initial); + return initial && amount <= *initial; } /* The currency code for the native currency. */ diff --git a/src/ripple/protocol/impl/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp index d5e06326d55..9b653ff0094 100644 --- a/src/ripple/protocol/impl/STAmount.cpp +++ b/src/ripple/protocol/impl/STAmount.cpp @@ -205,6 +205,7 @@ STAmount::STAmount (SField const& name, , mIsNative (true) , mIsNegative (negative) { + assert(mValue <= std::numeric_limits::max()); } STAmount::STAmount (SField const& name, Issue const& issue, @@ -215,6 +216,7 @@ STAmount::STAmount (SField const& name, Issue const& issue, , mOffset (exponent) , mIsNegative (negative) { + assert(mValue <= std::numeric_limits::max()); canonicalize (); } @@ -226,6 +228,7 @@ STAmount::STAmount (std::uint64_t mantissa, bool negative) , mIsNative (true) , mIsNegative (mantissa != 0 && negative) { + assert(mValue <= std::numeric_limits::max()); } STAmount::STAmount (Issue const& issue, @@ -235,6 +238,7 @@ STAmount::STAmount (Issue const& issue, , mOffset (exponent) , mIsNegative (negative) { + assert(mValue <= std::numeric_limits::max()); canonicalize (); } @@ -310,7 +314,7 @@ STAmount::xrp () const if (mIsNegative) drops = -drops; - return { drops }; + return XRPAmount{ drops }; } IOUAmount diff --git a/src/test/jtx/amount.h b/src/test/jtx/amount.h index 49fe293e81d..245c42c5443 100644 --- a/src/test/jtx/amount.h +++ b/src/test/jtx/amount.h @@ -64,7 +64,7 @@ struct None // This value is also defined in SystemParameters.h. It's // duplicated here to catch any possible future errors that // could change that value (however unlikely). -constexpr XRPAmount dropsPerXRP = { 1'000'000 }; +constexpr XRPAmount dropsPerXRP{ 1'000'000 }; /** Represents an XRP or IOU quantity This customizes the string conversion and supports diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index fcbb7ba4968..fea4ac8682b 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -247,7 +247,7 @@ class Invariants_test : public beast::unit_test::suite doInvariantCheck (enabled, {{ "incorrect account XRP balance" }, { "XRP net change was positive: 99999999000000001" }}, - [](Account const& A1, Account const&, ApplyContext& ac) + [self = this](Account const& A1, Account const&, ApplyContext& ac) { // balance exceeds genesis amount auto const sle = ac.view().peek (keylet::account(A1.id())); @@ -257,6 +257,7 @@ class Invariants_test : public beast::unit_test::suite // with an invalid value sle->setFieldAmount (sfBalance, INITIAL_XRP + drops(1)); + self->BEAST_EXPECT(!sle->getFieldAmount(sfBalance).negative()); ac.view().update (sle); return true; }); @@ -264,13 +265,14 @@ class Invariants_test : public beast::unit_test::suite doInvariantCheck (enabled, {{ "incorrect account XRP balance" }, { "XRP net change of -1000000001 doesn't match fee 0" }}, - [](Account const& A1, Account const&, ApplyContext& ac) + [self = this](Account const& A1, Account const&, ApplyContext& ac) { // balance is negative auto const sle = ac.view().peek (keylet::account(A1.id())); if(! sle) return false; - sle->setFieldAmount (sfBalance, -1); + sle->setFieldAmount (sfBalance, {1, true}); + self->BEAST_EXPECT(sle->getFieldAmount(sfBalance).negative()); ac.view().update (sle); return true; });