Skip to content

Commit

Permalink
[FOLD] Review feedback #3: More from @seelabs:
Browse files Browse the repository at this point in the history 
* Make a tag specifically for drop types (XRPAmount)
* Make the drops to decimal XRP conversion an XRPAmount member function.
  Requires moving DROPS_PER_XRP to XRPAmount.h. Includes unit tests.
* Fix some other problematic XRPAmount unit tests.
  • Loading branch information
@ximinez
ximinez committed Aug 19, 2019
1 parent cce64fa commit 915d2a8
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 37 deletions.
17 changes: 5 additions & 12 deletions src/ripple/app/misc/NetworkOPs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2319,19 +2319,12 @@ Json::Value NetworkOPsImp::getServerInfo (bool human, bool admin, bool counters)
}
else
{
// Make a local specialization of the TaggedFee class, using
// XRPAmount as the "unit" to make some of the math here easier.
using XRPDouble = feeunit::TaggedFee<XRPAmount, double>;

l[jss::base_fee_xrp] =
static_cast<XRPDouble>(baseFee) /
DROPS_PER_XRP;
l[jss::reserve_base_xrp] =
static_cast<XRPDouble>(
lpClosed->fees().accountReserve(0)) / DROPS_PER_XRP;
l[jss::reserve_inc_xrp] =
static_cast<XRPDouble>(
lpClosed->fees().increment) / DROPS_PER_XRP;
baseFee.decimalXRP();
l[jss::reserve_base_xrp] =
lpClosed->fees().accountReserve(0).decimalXRP();
l[jss::reserve_inc_xrp] =
lpClosed->fees().increment.decimalXRP();

auto const nowOffset = app_.timeKeeper().nowOffset();
if (std::abs (nowOffset.count()) >= 60)
Expand Down
29 changes: 26 additions & 3 deletions src/ripple/basics/XRPAmount.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@

namespace ripple {

namespace feeunit {
struct drop_tag;
} // feeunit

class XRPAmount
: private boost::totally_ordered <XRPAmount>
, private boost::additive <XRPAmount>
Expand All @@ -39,7 +43,7 @@ class XRPAmount
, private boost::unit_steppable <XRPAmount>
{
public:
using unit_type = XRPAmount;
using unit_type = feeunit::drop_tag;
using value_type = std::int64_t;
private:
value_type drops_;
Expand Down Expand Up @@ -210,6 +214,10 @@ class XRPAmount
return drops_;
}

constexpr
double
decimalXRP () const;

Json::Value
json () const
{
Expand Down Expand Up @@ -246,6 +254,20 @@ class XRPAmount

};

/** Number of drops per 1 XRP */
static
constexpr
XRPAmount
DROPS_PER_XRP{1'000'000};

inline
constexpr
double
XRPAmount::decimalXRP () const
{
return static_cast<double>(drops_) / DROPS_PER_XRP.drops();
}

// Output XRPAmount as just the drops value.
template<class Char, class Traits>
std::basic_ostream<Char, Traits>&
Expand Down Expand Up @@ -288,8 +310,9 @@ mulRatio (
}
if (r > std::numeric_limits<XRPAmount::value_type>::max ())
Throw<std::overflow_error> ("XRP mulRatio overflow");
if (r < std::numeric_limits<XRPAmount::value_type>::min ())
Throw<std::overflow_error> ("XRP mulRatio underflow");
// TODO: DO WE NEED AN AMENDMENT TO ADD THIS CHECK
//if (r < std::numeric_limits<XRPAmount::value_type>::min ())
// Throw<std::overflow_error> ("XRP mulRatio underflow");
return XRPAmount (r.convert_to<XRPAmount::value_type> ());
}

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/basics/feeunits.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ constexpr bool is_usable_unit_v =
std::is_same_v<typename T::unit_type, feeunit_tag> ||
std::is_same_v<typename T::unit_type, feelevel_tag> ||
std::is_same_v<typename T::unit_type, unitless_tag> ||
std::is_same_v<typename T::unit_type, XRPAmount>;
std::is_same_v<typename T::unit_type, drop_tag>;


template<class UnitTag, class T>
Expand Down
6 changes: 0 additions & 6 deletions src/ripple/protocol/SystemParameters.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,6 @@ systemName ()

/** Configure the native currency. */

/** Number of drops per 1 XRP */
static
constexpr
XRPAmount
DROPS_PER_XRP{ 1'000'000 };

/** Number of drops in the genesis account. */
static
constexpr
Expand Down
40 changes: 33 additions & 7 deletions src/test/basics/XRPAmount_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,25 +110,44 @@ class XRPAmount_test : public beast::unit_test::suite
}
}

void testDecimal ()
{
// Tautology
BEAST_EXPECT(DROPS_PER_XRP.decimalXRP() == 1);

XRPAmount test{1};
BEAST_EXPECT(test.decimalXRP() == 0.000001);

test = -test;
BEAST_EXPECT(test.decimalXRP() == -0.000001);

test = 100'000'000;
BEAST_EXPECT(test.decimalXRP() == 100);

test = -test;
BEAST_EXPECT(test.decimalXRP() == -100);
}

void testMulRatio()
{
testcase ("mulRatio");

constexpr auto maxUInt32 = std::numeric_limits<std::uint32_t>::max ();
constexpr auto maxUInt64 = std::numeric_limits<std::uint64_t>::max ();
constexpr auto maxXRP = std::numeric_limits<XRPAmount::value_type>::max ();
constexpr auto minXRP = std::numeric_limits<XRPAmount::value_type>::min ();

{
// multiply by a number that would overflow then divide by the same
// number, and check we didn't lose any value
XRPAmount big (maxUInt64);
XRPAmount big (maxXRP);
BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, true));
// rounding mode shouldn't matter as the result is exact
BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, false));
}

{
// Similar test as above, but for neative values
XRPAmount big (maxUInt64);
// Similar test as above, but for negative values
XRPAmount big (minXRP);
BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, true));
// rounding mode shouldn't matter as the result is exact
BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, false));
Expand Down Expand Up @@ -163,7 +182,7 @@ class XRPAmount_test : public beast::unit_test::suite
}

{
XRPAmount big (maxUInt64);
XRPAmount big (maxXRP);
auto const rup = mulRatio (big, maxUInt32 - 1, maxUInt32, true);
auto const rdown = mulRatio (big, maxUInt32 - 1, maxUInt32, false);
BEAST_EXPECT(rup.drops () - rdown.drops () == 1);
Expand All @@ -185,8 +204,14 @@ class XRPAmount_test : public beast::unit_test::suite

{
// overflow
XRPAmount big (maxUInt64);
except ([&] {mulRatio (big, 2, 0, true);});
XRPAmount big (maxXRP);
except ([&] {mulRatio (big, 2, 1, true);});
}

{
// underflow
XRPAmount bigNegative (minXRP + 10);
BEAST_EXPECT(mulRatio(bigNegative, 2, 1, true) == minXRP);
}
}

Expand All @@ -198,6 +223,7 @@ class XRPAmount_test : public beast::unit_test::suite
testBeastZero ();
testComparisons ();
testAddSub ();
testDecimal ();
testMulRatio ();
}
};
Expand Down
16 changes: 8 additions & 8 deletions src/test/basics/feeunits_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ class feeunits_test
XRPAmount x{ 100 };
BEAST_EXPECT(x.drops() == 100);
BEAST_EXPECT((std::is_same_v<decltype(x)::unit_type,
XRPAmount>));
feeunit::drop_tag>));
auto y = 4u * x;
BEAST_EXPECT(y.value() == 400);
BEAST_EXPECT((std::is_same_v<decltype(y)::unit_type,
XRPAmount>));
feeunit::drop_tag>));

auto z = 4 * y;
BEAST_EXPECT(z.value() == 1600);
BEAST_EXPECT((std::is_same_v<decltype(z)::unit_type,
XRPAmount>));
feeunit::drop_tag>));

FeeUnit32 f{ 10 };
FeeUnit32 baseFee{ 100 };
Expand All @@ -57,18 +57,18 @@ class feeunits_test

BEAST_EXPECT(drops.value() == 1000);
BEAST_EXPECT((std::is_same_v<decltype(drops)::unit_type,
XRPAmount>));
feeunit::drop_tag>));
BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
}
{
XRPAmount x{ 100 };
BEAST_EXPECT(x.value() == 100);
BEAST_EXPECT((std::is_same_v<decltype(x)::unit_type,
XRPAmount>));
feeunit::drop_tag>));
auto y = 4u * x;
BEAST_EXPECT(y.value() == 400);
BEAST_EXPECT((std::is_same_v<decltype(y)::unit_type,
XRPAmount>));
feeunit::drop_tag>));

FeeUnit64 f{ 10 };
FeeUnit64 baseFee{ 100 };
Expand All @@ -77,7 +77,7 @@ class feeunits_test

BEAST_EXPECT(drops.value() == 1000);
BEAST_EXPECT((std::is_same_v<decltype(drops)::unit_type,
XRPAmount>));
feeunit::drop_tag>));
BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
}
{
Expand All @@ -98,7 +98,7 @@ class feeunits_test

BEAST_EXPECT(drops.value() == 40);
BEAST_EXPECT((std::is_same_v<decltype(drops)::unit_type,
XRPAmount>));
feeunit::drop_tag>));
BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
}
}
Expand Down

0 comments on commit 915d2a8

Please sign in to comment.