Skip to content

Commit

Permalink
[FOLD] Review feedback #2: Second batch from @seelabs:
Browse files Browse the repository at this point in the history
* Simplify the calculations in LoadFeeTrack.
* Make commutative operator* a friend instead of a free function in
  XRPAmount and TaggedFee.
  • Loading branch information
ximinez committed Aug 26, 2019
1 parent 81295bb commit 3355ce5
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 63 deletions.
48 changes: 5 additions & 43 deletions src/ripple/app/misc/impl/LoadFeeTrack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,29 +84,6 @@ LoadFeeTrack::lowerLocalFee ()

//------------------------------------------------------------------------------

namespace detail
{

struct xrp_unit_product_tag;

using xrp_unit_product =
feeunit::TaggedFee<detail::xrp_unit_product_tag, std::uint64_t>;

} // detail

detail::xrp_unit_product
operator* (FeeUnit64 lhs, XRPAmount rhs)
{
return detail::xrp_unit_product{ lhs.fee() * rhs.drops() };
}

XRPAmount
operator/ (detail::xrp_unit_product lhs, FeeUnit64 rhs)
{
return{ lhs.fee() / rhs.fee() };
}


// Scale using load as well as base rate
XRPAmount
scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack,
Expand Down Expand Up @@ -182,26 +159,11 @@ scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack,
if (baseFee > max / feeFactor)
Throw<std::overflow_error> ("scaleFeeLoad");
baseFee *= feeFactor;
// Reorder fee and baseFee
maybe_swap(fee, baseFee);
// If fee * baseFee overflows, do the division first
if (fee > FeeUnit64{ max / baseFee })
{
// Do the division first, on the larger of fee and baseFee
auto const factor = fee / den;
// If factor * basefee ( == fee / den * baseFee ) will overflow,
// throw
if (factor > max / baseFee)
Throw<std::overflow_error> ("scaleFeeLoad");
return factor * baseFee;
}
else
{
// Otherwise fee * baseFee won't overflow,
// so do it prior to the division.
auto const product = fee * baseFee;
return product / den;
}

auto const result = mulDiv(fee, baseFee, den);
if (!result.first)
Throw<std::overflow_error> ("scaleFeeLoad");
return result.second;
}

} // ripple
2 changes: 1 addition & 1 deletion src/ripple/app/misc/impl/TxQ.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1493,7 +1493,7 @@ TxQ::doRPC(Application& app) const
levels[jss::median_level] = to_string(metrics.medFeeLevel);
levels[jss::open_ledger_level] = to_string(metrics.openLedgerFeeLevel);

auto const& baseFee = view->fees().base;
auto const baseFee = view->fees().base;
auto& drops = ret[jss::drops] = Json::Value();

// Don't care about the overflow flags
Expand Down
18 changes: 9 additions & 9 deletions src/ripple/basics/XRPAmount.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,15 @@ class XRPAmount
return { drops_ * rhs };
}

friend
constexpr
XRPAmount
operator*(value_type lhs, XRPAmount const& rhs)
{
// multiplication is commutative
return rhs * lhs;
}

constexpr
value_type
operator/(XRPAmount const& rhs) const
Expand Down Expand Up @@ -253,15 +262,6 @@ to_string (XRPAmount const& amount)
return std::to_string (amount.drops ());
}

inline
constexpr
XRPAmount
operator*(XRPAmount::value_type lhs, XRPAmount const& rhs)
{
// multiplication is commutative
return rhs * lhs;
}

inline
XRPAmount
mulRatio (
Expand Down
18 changes: 9 additions & 9 deletions src/ripple/basics/feeunits.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,15 @@ class TaggedFee
return TaggedFee{ fee_ * rhs };
}

friend
constexpr
TaggedFee
operator*(value_type lhs, TaggedFee const& rhs)
{
// multiplication is commutative
return rhs * lhs;
}

constexpr
value_type
operator/ (TaggedFee const& rhs) const
Expand Down Expand Up @@ -322,15 +331,6 @@ to_string (TaggedFee<UnitTag, T> const& amount)
return std::to_string (amount.fee ());
}

template<class UnitTag, class T>
constexpr
TaggedFee<UnitTag, T>
operator*(T lhs, TaggedFee<UnitTag, T> const& rhs)
{
// multiplication is commutative
return rhs * lhs;
}

template<class Source, class = enable_if_unit_t<Source> >
constexpr bool can_muldiv_source_v =
std::is_convertible_v<typename Source::value_type, std::uint64_t>;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/protocol/Quality.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ operator!= (
//------------------------------------------------------------------------------

// Ripple specific constant used for parsing qualities and other things
#define QUALITY_ONE 1000000000
#define QUALITY_ONE 1'000'000'000

/** Represents the logical ratio of output currency to input currency.
Internally this is stored using a custom floating point representation,
Expand Down

0 comments on commit 3355ce5

Please sign in to comment.