Skip to content

Commit

Permalink
[FOLD] Fix some issues with implicit XRPAmount conversions
Browse files Browse the repository at this point in the history
  • Loading branch information
ximinez committed Oct 2, 2019
1 parent 9e1e12a commit 0d64a99
Show file tree
Hide file tree
Showing 10 changed files with 43 additions and 30 deletions.
9 changes: 6 additions & 3 deletions src/ripple/app/misc/FeeVoteImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ FeeVoteImpl::doVoting(
{
auto const vote = val->getFieldU64 (sfBaseFee);
if (isLegalAmount(vote))
baseFeeVote.addVote (vote);
baseFeeVote.addVote(XRPAmount{
unsafe_cast<XRPAmount::value_type>(vote)});
else
// Invalid amounts will be treated as if they're
// not provided. Don't throw because this value is
Expand All @@ -186,7 +187,8 @@ FeeVoteImpl::doVoting(

if (val->isFieldPresent (sfReserveBase))
{
baseReserveVote.addVote (val->getFieldU32 (sfReserveBase));
baseReserveVote.addVote(XRPAmount{
val->getFieldU32(sfReserveBase)});
}
else
{
Expand All @@ -195,7 +197,8 @@ FeeVoteImpl::doVoting(

if (val->isFieldPresent (sfReserveIncrement))
{
incReserveVote.addVote (val->getFieldU32 (sfReserveIncrement));
incReserveVote.addVote (XRPAmount{
val->getFieldU32 (sfReserveIncrement)});
}
else
{
Expand Down
6 changes: 3 additions & 3 deletions src/ripple/app/misc/impl/LoadFeeTrack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<XRPAmount::value_type>::max();
constexpr XRPAmount max{
std::numeric_limits<XRPAmount::value_type>::max()};
if (baseFee > max / feeFactor)
Throw<std::overflow_error> ("scaleFeeLoad");
baseFee *= feeFactor;
Expand Down
32 changes: 17 additions & 15 deletions src/ripple/basics/XRPAmount.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ struct dropTag;

class XRPAmount
: private boost::totally_ordered <XRPAmount>
, private boost::less_than_comparable <XRPAmount, std::uint64_t>
, private boost::additive <XRPAmount>
, private boost::equality_comparable <XRPAmount, std::int64_t>
, private boost::additive <XRPAmount, std::int64_t>
, private boost::dividable <XRPAmount, std::int64_t>
, private boost::modable <XRPAmount, std::int64_t>
, private boost::unit_steppable <XRPAmount>
Expand Down Expand Up @@ -94,7 +94,7 @@ class XRPAmount
XRPAmount
operator*(value_type const& rhs) const
{
return { drops_ * rhs };
return XRPAmount{ drops_ * rhs };
}

friend
Expand Down Expand Up @@ -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)
{
Expand All @@ -165,7 +179,7 @@ class XRPAmount
XRPAmount
operator- () const
{
return { -drops_ };
return XRPAmount{ -drops_ };
}

bool
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/ledger/ReadView.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ struct Fees
std::pair<bool, XRPAmount>
toDrops(FeeUnit64 const& fee) const
{
return mulDiv(base.drops(), fee, units);
return mulDiv(base, fee, units);
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/ledger/detail/ApplyStateTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class ApplyStateTable
std::pair<Action, std::shared_ptr<SLE>>>;

items_t items_;
XRPAmount dropsDestroyed_ = 0;
XRPAmount dropsDestroyed_{0};

public:
ApplyStateTable() = default;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/ledger/detail/RawStateTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class RawStateTable
std::pair<Action, std::shared_ptr<SLE>>>, false>>;

items_t items_;
XRPAmount dropsDestroyed_ = 0;
XRPAmount dropsDestroyed_{0};
};

} // detail
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/protocol/SystemParameters.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::uint64_t>();
assert(initial);
return initial && amount <= *initial;
}

/* The currency code for the native currency. */
Expand Down
6 changes: 5 additions & 1 deletion src/ripple/protocol/impl/STAmount.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ STAmount::STAmount (SField const& name,
, mIsNative (true)
, mIsNegative (negative)
{
assert(mValue <= std::numeric_limits<std::int64_t>::max());
}

STAmount::STAmount (SField const& name, Issue const& issue,
Expand All @@ -215,6 +216,7 @@ STAmount::STAmount (SField const& name, Issue const& issue,
, mOffset (exponent)
, mIsNegative (negative)
{
assert(mValue <= std::numeric_limits<std::int64_t>::max());
canonicalize ();
}

Expand All @@ -226,6 +228,7 @@ STAmount::STAmount (std::uint64_t mantissa, bool negative)
, mIsNative (true)
, mIsNegative (mantissa != 0 && negative)
{
assert(mValue <= std::numeric_limits<std::int64_t>::max());
}

STAmount::STAmount (Issue const& issue,
Expand All @@ -235,6 +238,7 @@ STAmount::STAmount (Issue const& issue,
, mOffset (exponent)
, mIsNegative (negative)
{
assert(mValue <= std::numeric_limits<std::int64_t>::max());
canonicalize ();
}

Expand Down Expand Up @@ -310,7 +314,7 @@ STAmount::xrp () const
if (mIsNegative)
drops = -drops;

return { drops };
return XRPAmount{ drops };
}

IOUAmount
Expand Down
2 changes: 1 addition & 1 deletion src/test/jtx/amount.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions src/test/ledger/Invariants_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand All @@ -257,20 +257,22 @@ 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;
});

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;
});
Expand Down

0 comments on commit 0d64a99

Please sign in to comment.