[FOLD] Review feedback #1: First batch from @SEELab:

* Move IOUAmount to basics * Rename units namespace to feeunit * Fix some FeeLevel unit handling in server_info
ximinez · Aug 8, 2019 · ccd3d54 · ccd3d54 · ripplelabs-jenkins · Aug 9, 2019
1 parent 3b93c0e
commit ccd3d54
Show file tree

Hide file tree

Showing 32 changed files with 79 additions and 77 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1631,6 +1631,7 @@ else ()
     src/ripple/basics/impl/contract.cpp
     src/ripple/basics/impl/CountedObject.cpp
     src/ripple/basics/impl/FileUtilities.cpp
+    src/ripple/basics/impl/IOUAmount.cpp
     src/ripple/basics/impl/Log.cpp
     src/ripple/basics/impl/strHex.cpp
     src/ripple/basics/impl/StringUtilities.cpp
@@ -1657,7 +1658,6 @@ else ()
     src/ripple/protocol/impl/ErrorCodes.cpp
     src/ripple/protocol/impl/Feature.cpp
     src/ripple/protocol/impl/HashPrefix.cpp
-    src/ripple/protocol/impl/IOUAmount.cpp
     src/ripple/protocol/impl/Indexes.cpp
     src/ripple/protocol/impl/InnerObjectFormats.cpp
     src/ripple/protocol/impl/Issue.cpp
@@ -1730,6 +1730,7 @@ install (
     src/ripple/basics/Buffer.h
     src/ripple/basics/CountedObject.h
     src/ripple/basics/FileUtilities.h
+    src/ripple/basics/IOUAmount.h
     src/ripple/basics/LocalValue.h
     src/ripple/basics/Log.h
     src/ripple/basics/safe_cast.h
@@ -1784,7 +1785,6 @@ install (
     src/ripple/protocol/ErrorCodes.h
     src/ripple/protocol/Feature.h
     src/ripple/protocol/HashPrefix.h
-    src/ripple/protocol/IOUAmount.h
     src/ripple/protocol/Indexes.h
     src/ripple/protocol/InnerObjectFormats.h
     src/ripple/protocol/Issue.h
@@ -2338,12 +2338,14 @@ else ()
     src/test/basics/Buffer_test.cpp
     src/test/basics/DetectCrash_test.cpp
     src/test/basics/FileUtilities_test.cpp
+    src/test/basics/IOUAmount_test.cpp
     src/test/basics/KeyCache_test.cpp
     src/test/basics/PerfLog_test.cpp
     src/test/basics/RangeSet_test.cpp
     src/test/basics/Slice_test.cpp
     src/test/basics/StringUtilities_test.cpp
     src/test/basics/TaggedCache_test.cpp
+    src/test/basics/XRPAmount_test.cpp
     src/test/basics/base64_test.cpp
     src/test/basics/base_uint_test.cpp
     src/test/basics/contract_test.cpp
@@ -2498,7 +2500,6 @@ else ()
          subdir: protocol
     #]===============================]
     src/test/protocol/BuildInfo_test.cpp
-    src/test/protocol/IOUAmount_test.cpp
     src/test/protocol/InnerObjectFormats_test.cpp
     src/test/protocol/Issue_test.cpp
     src/test/protocol/PublicKey_test.cpp
@@ -2511,7 +2512,6 @@ else ()
     src/test/protocol/SecretKey_test.cpp
     src/test/protocol/Seed_test.cpp
     src/test/protocol/TER_test.cpp
-    src/test/protocol/XRPAmount_test.cpp
     src/test/protocol/digest_test.cpp
     src/test/protocol/types_test.cpp
     #[===============================[

diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp
@@ -2231,14 +2231,15 @@ Json::Value NetworkOPsImp::getServerInfo (bool human, bool admin, bool counters)
 
     auto const loadFactorServer = app_.getFeeTrack().getLoadFactor();
     auto const loadBaseServer = app_.getFeeTrack().getLoadBase();
-    auto const loadFactorFeeEscalation =
-        escalationMetrics.openLedgerFeeLevel;
-    auto const loadBaseFeeEscalation =
-        escalationMetrics.referenceFeeLevel;
+    /* Scale the escalated fee level to unitless "load factor".
+       In practice, this just strips the units, but it will continue
+       to work correctly if either base value ever changes. */
+    auto const loadFactorFeeEscalation = mulDiv(
+        escalationMetrics.openLedgerFeeLevel, loadBaseServer,
+        escalationMetrics.referenceFeeLevel).second;
 
     auto const loadFactor = std::max(safe_cast<std::uint64_t>(loadFactorServer),
-        mulDiv(loadFactorFeeEscalation, loadBaseServer,
-            loadBaseFeeEscalation).second);
+        loadFactorFeeEscalation);
 
     if (!human)
     {
@@ -2252,11 +2253,11 @@ Json::Value NetworkOPsImp::getServerInfo (bool human, bool admin, bool counters)
             that high.
         */
         info[jss::load_factor_fee_escalation] =
-            loadFactorFeeEscalation.json();
+            escalationMetrics.openLedgerFeeLevel.json();
         info[jss::load_factor_fee_queue] =
             escalationMetrics.minProcessingFeeLevel.json();
         info[jss::load_factor_fee_reference] =
-            loadBaseFeeEscalation.json();
+            escalationMetrics.referenceFeeLevel.json();
     }
     else
     {
@@ -2281,13 +2282,14 @@ Json::Value NetworkOPsImp::getServerInfo (bool human, bool admin, bool counters)
                 info[jss::load_factor_cluster] =
                     static_cast<double> (fee) / loadBaseServer;
         }
-        if (loadFactorFeeEscalation !=
+        if (escalationMetrics.openLedgerFeeLevel !=
                 escalationMetrics.referenceFeeLevel &&
                 (admin ||
-                loadFactorFeeEscalation != FeeLevel64{ loadFactor }))
+                loadFactorFeeEscalation != loadFactor))
             info[jss::load_factor_fee_escalation] =
-                static_cast<FeeLevelDouble>(loadFactorFeeEscalation) /
-                    escalationMetrics.referenceFeeLevel;
+                static_cast<FeeLevelDouble>(
+                    escalationMetrics.openLedgerFeeLevel) /
+                        escalationMetrics.referenceFeeLevel;
         if (escalationMetrics.minProcessingFeeLevel !=
                 escalationMetrics.referenceFeeLevel)
             info[jss::load_factor_fee_queue] =
@@ -2324,16 +2326,16 @@ Json::Value NetworkOPsImp::getServerInfo (bool human, bool admin, bool counters)
         {
             // Make a local specialization of the TaggedFee class, using
             // XRPAmount as the "unit" to make some of the math here easier.
-            using DropsDouble = units::TaggedFee<XRPAmount, double>;
+            using XRPDouble = feeunit::TaggedFee<XRPAmount, double>;
 
             l[jss::base_fee_xrp] =
-                static_cast<DropsDouble>(baseFee) /
+                static_cast<XRPDouble>(baseFee) /
                     DROPS_PER_XRP;
             l[jss::reserve_base_xrp]   =
-                static_cast<DropsDouble>(
+                static_cast<XRPDouble>(
                     lpClosed->fees().accountReserve(0)) / DROPS_PER_XRP;
             l[jss::reserve_inc_xrp]    =
-                static_cast<DropsDouble>(
+                static_cast<XRPDouble>(
                     lpClosed->fees().increment) / DROPS_PER_XRP;
 
             auto const nowOffset = app_.timeKeeper().nowOffset();

diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp
@@ -127,7 +127,7 @@ namespace detail
 struct xrp_unit_product_tag;
 
 using xrp_unit_product =
-    units::TaggedFee<detail::xrp_unit_product_tag, std::uint64_t>;
+    feeunit::TaggedFee<detail::xrp_unit_product_tag, std::uint64_t>;
 
 } // detail
 

diff --git a/src/ripple/app/paths/Credit.h b/src/ripple/app/paths/Credit.h
@@ -20,9 +20,9 @@
 #ifndef RIPPLE_APP_PATHS_CREDIT_H_INCLUDED
 #define RIPPLE_APP_PATHS_CREDIT_H_INCLUDED
 
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/ledger/View.h>
 #include <ripple/protocol/STAmount.h>
-#include <ripple/protocol/IOUAmount.h>
 
 namespace ripple {
 

diff --git a/src/ripple/app/paths/Flow.cpp b/src/ripple/app/paths/Flow.cpp
@@ -23,7 +23,7 @@
 #include <ripple/app/paths/impl/StrandFlow.h>
 #include <ripple/app/paths/impl/Steps.h>
 #include <ripple/basics/Log.h>
-#include <ripple/protocol/IOUAmount.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/XRPAmount.h>
 
 #include <boost/container/flat_set.hpp>

diff --git a/src/ripple/app/paths/impl/AmountSpec.h b/src/ripple/app/paths/impl/AmountSpec.h
@@ -20,7 +20,7 @@
 #ifndef RIPPLE_PATH_IMPL_AMOUNTSPEC_H_INCLUDED
 #define RIPPLE_PATH_IMPL_AMOUNTSPEC_H_INCLUDED
 
-#include <ripple/protocol/IOUAmount.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/XRPAmount.h>
 #include <ripple/protocol/STAmount.h>
 

diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp
@@ -23,14 +23,14 @@
 #include <ripple/app/paths/NodeDirectory.h>
 #include <ripple/app/tx/impl/OfferStream.h>
 #include <ripple/basics/contract.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/Log.h>
+#include <ripple/basics/XRPAmount.h>
 #include <ripple/ledger/Directory.h>
 #include <ripple/ledger/PaymentSandbox.h>
 #include <ripple/protocol/Book.h>
 #include <ripple/protocol/Feature.h>
-#include <ripple/protocol/IOUAmount.h>
 #include <ripple/protocol/Quality.h>
-#include <ripple/basics/XRPAmount.h>
 
 #include <boost/container/flat_set.hpp>
 

diff --git a/src/ripple/app/paths/impl/DirectStep.cpp b/src/ripple/app/paths/impl/DirectStep.cpp
@@ -20,9 +20,9 @@
 #include <ripple/app/paths/Credit.h>
 #include <ripple/app/paths/impl/StepChecks.h>
 #include <ripple/app/paths/impl/Steps.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/Log.h>
 #include <ripple/ledger/PaymentSandbox.h>
-#include <ripple/protocol/IOUAmount.h>
 #include <ripple/protocol/Quality.h>
 
 #include <boost/container/flat_set.hpp>

diff --git a/src/ripple/app/paths/impl/FlowDebugInfo.h b/src/ripple/app/paths/impl/FlowDebugInfo.h
@@ -21,9 +21,9 @@
 #define RIPPLE_PATH_IMPL_FLOWDEBUGINFO_H_INCLUDED
 
 #include <ripple/app/paths/impl/AmountSpec.h>
-#include <ripple/ledger/PaymentSandbox.h>
-#include <ripple/protocol/IOUAmount.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/XRPAmount.h>
+#include <ripple/ledger/PaymentSandbox.h>
 
 #include <boost/container/flat_map.hpp>
 #include <boost/optional.hpp>

diff --git a/src/ripple/app/paths/impl/PaySteps.cpp b/src/ripple/app/paths/impl/PaySteps.cpp
@@ -19,11 +19,11 @@
 
 #include <ripple/app/paths/impl/Steps.h>
 #include <ripple/basics/contract.h>
+#include <ripple/basics/IOUAmount.h>
+#include <ripple/basics/XRPAmount.h>
 #include <ripple/json/json_writer.h>
 #include <ripple/ledger/ReadView.h>
 #include <ripple/protocol/Feature.h>
-#include <ripple/protocol/IOUAmount.h>
-#include <ripple/basics/XRPAmount.h>
 
 #include <algorithm>
 #include <numeric>

diff --git a/src/ripple/app/paths/impl/StrandFlow.h b/src/ripple/app/paths/impl/StrandFlow.h
@@ -26,9 +26,9 @@
 #include <ripple/app/paths/impl/FlatSets.h>
 #include <ripple/app/paths/impl/FlowDebugInfo.h>
 #include <ripple/app/paths/impl/Steps.h>
-#include <ripple/basics/Log.h>
-#include <ripple/protocol/IOUAmount.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/XRPAmount.h>
+#include <ripple/basics/Log.h>
 
 #include <boost/container/flat_set.hpp>
 

diff --git a/src/ripple/app/paths/impl/XRPEndpointStep.cpp b/src/ripple/app/paths/impl/XRPEndpointStep.cpp
@@ -21,11 +21,11 @@
 #include <ripple/app/paths/impl/AmountSpec.h>
 #include <ripple/app/paths/impl/Steps.h>
 #include <ripple/app/paths/impl/StepChecks.h>
+#include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/Log.h>
+#include <ripple/basics/XRPAmount.h>
 #include <ripple/ledger/PaymentSandbox.h>
-#include <ripple/protocol/IOUAmount.h>
 #include <ripple/protocol/Quality.h>
-#include <ripple/basics/XRPAmount.h>
 
 #include <boost/container/flat_set.hpp>
 

diff --git a/src/ripple/app/tx/impl/ApplyContext.h b/src/ripple/app/tx/impl/ApplyContext.h
@@ -22,9 +22,9 @@
 
 #include <ripple/app/main/Application.h>
 #include <ripple/ledger/ApplyViewImpl.h>
+#include <ripple/basics/XRPAmount.h>
 #include <ripple/core/Config.h>
 #include <ripple/protocol/STTx.h>
-#include <ripple/basics/XRPAmount.h>
 #include <ripple/beast/utility/Journal.h>
 #include <boost/optional.hpp>
 #include <utility>

diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp
@@ -23,6 +23,7 @@
 #include <ripple/basics/chrono.h>
 #include <ripple/basics/Log.h>
 #include <ripple/basics/safe_cast.h>
+#include <ripple/basics/XRPAmount.h>
 #include <ripple/conditions/Condition.h>
 #include <ripple/conditions/Fulfillment.h>
 #include <ripple/ledger/ApplyView.h>
@@ -32,7 +33,6 @@
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/Indexes.h>
 #include <ripple/protocol/TxFlags.h>
-#include <ripple/basics/XRPAmount.h>
 
 // During an EscrowFinish, the transaction must specify both
 // a condition and a fulfillment. We track whether that

diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp
@@ -20,6 +20,7 @@
 #include <ripple/app/tx/impl/PayChan.h>
 #include <ripple/basics/chrono.h>
 #include <ripple/basics/Log.h>
+#include <ripple/basics/XRPAmount.h>
 #include <ripple/ledger/ApplyView.h>
 #include <ripple/ledger/View.h>
 #include <ripple/protocol/digest.h>
@@ -29,7 +30,6 @@
 #include <ripple/protocol/PublicKey.h>
 #include <ripple/protocol/st.h>
 #include <ripple/protocol/TxFlags.h>
-#include <ripple/basics/XRPAmount.h>
 
 namespace ripple {
 

diff --git a/src/ripple/protocol/IOUAmount.h → src/ripple/basics/IOUAmount.h b/src/ripple/protocol/IOUAmount.h → src/ripple/basics/IOUAmount.h
@@ -17,8 +17,8 @@
 */
 //==============================================================================
 
-#ifndef RIPPLE_PROTOCOL_IOUAMOUNT_H_INCLUDED
-#define RIPPLE_PROTOCOL_IOUAMOUNT_H_INCLUDED
+#ifndef RIPPLE_BASICS_IOUAMOUNT_H_INCLUDED
+#define RIPPLE_BASICS_IOUAMOUNT_H_INCLUDED
 
 #include <ripple/beast/utility/Zero.h>
 #include <boost/operators.hpp>
Build Type	Log	Result	Status
msvc.Debug	logfile	1179 cases, 0 failed, t: 570s	PASS ✅
msvc.Debug, NINJA_BUILD=true	logfile	1179 cases, 0 failed, t: 594s	PASS ✅
gcc.Release -Dassert=ON, MANUAL_TESTS=true	logfile	915 cases, 0 failed, t: 5m2s	PASS ✅
gcc.Debug -Dcoverage=ON, TARGET=coverage_report, SKIP_TESTS=true	logfile	1179 cases, 0 failed, t: 17m3s	PASS ✅
docs, TARGET=docs	logfile	1 cases, 0 failed, t: 0m1s	PASS ✅
msvc.Debug -Dunity=OFF	logfile	1178 cases, 0 failed, t: 1108s	PASS ✅
clang.Debug	logfile	1179 cases, 0 failed, t: 2m59s	PASS ✅
msvc.Release	logfile	1179 cases, 0 failed, t: 406s	PASS ✅
clang.Debug -Dunity=OFF	logfile	1178 cases, 0 failed, t: 11m25s	PASS ✅
gcc.Debug	logfile	1179 cases, 0 failed, t: 2m59s	PASS ✅
gcc.Debug -Dunity=OFF	logfile	1178 cases, 0 failed, t: 11m57s	PASS ✅
clang.Release -Dassert=ON	logfile	1179 cases, 0 failed, t: 5m12s	PASS ✅
gcc.Release -Dassert=ON	logfile	1179 cases, 0 failed, t: 5m0s	PASS ✅
gcc.Debug -Dstatic=OFF	logfile	1179 cases, 0 failed, t: 3m0s	PASS ✅
gcc.Debug -Dstatic=OFF -DBUILD_SHARED_LIBS=ON	logfile	1179 cases, 0 failed, t: 2m56s	PASS ✅
gcc.Debug, NINJA_BUILD=true	logfile	1179 cases, 0 failed, t: 2m53s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=address, PARALLEL_TESTS=false, DEBUGGER=false	logfile	1178 cases, 0 failed, t: 11m53s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=undefined, PARALLEL_TESTS=false	logfile	1178 cases, 0 failed, t: 13m21s	PASS ✅