Include rounding mode in XRPAmount to STAmount conversion.

XRPLF · Oct 19, 2022 · 717ff40 · 717ff40
1 parent efd5187
commit 717ff40
Show file tree

Hide file tree

Showing 7 changed files with 1,037 additions and 661 deletions.
diff --git a/src/ripple/basics/IOUAmount.h b/src/ripple/basics/IOUAmount.h
@@ -186,11 +186,7 @@ mulRatio(
     std::uint32_t den,
     bool roundUp);
 
-// Since IOUAmount and STAmount do not have access to a ledger, this
-// is needed to put low-level routines on an amendment switch. Only
-// transactions need to use this switchover. Outside of a transaction
-// it's safe to unconditionally use the new behavior.
-extern LocalValue<bool> stNumberSwitchover;
+extern std::atomic<bool> stNumberSwitchover;
 
 /** RAII class to set and restore the Number switchover.
  */
@@ -202,16 +198,16 @@ class NumberSO
 public:
     ~NumberSO()
     {
-        *stNumberSwitchover = saved_;
+        stNumberSwitchover = saved_;
     }
 
     NumberSO(NumberSO const&) = delete;
     NumberSO&
     operator=(NumberSO const&) = delete;
 
-    explicit NumberSO(bool v) : saved_(*stNumberSwitchover)
+    explicit NumberSO(bool v) : saved_(stNumberSwitchover)
     {
-        *stNumberSwitchover = v;
+        stNumberSwitchover = v;
     }
 };
 

diff --git a/src/ripple/basics/Number.h b/src/ripple/basics/Number.h
@@ -337,6 +337,24 @@ squelch(Number const& x, Number const& limit) noexcept
     return x;
 }
 
+class saveNumberRoundMode
+{
+    Number::rounding_mode mode_;
+
+public:
+    ~saveNumberRoundMode()
+    {
+        Number::setround(mode_);
+    }
+    explicit saveNumberRoundMode(Number::rounding_mode mode) noexcept
+        : mode_{mode}
+    {
+    }
+    saveNumberRoundMode(saveNumberRoundMode const&) = delete;
+    saveNumberRoundMode&
+    operator=(saveNumberRoundMode const&) = delete;
+};
+
 }  // namespace ripple
 
 #endif  // RIPPLE_BASICS_NUMBER_H_INCLUDED
diff --git a/src/ripple/basics/impl/IOUAmount.cpp b/src/ripple/basics/impl/IOUAmount.cpp
@@ -27,7 +27,7 @@
 
 namespace ripple {
 
-LocalValue<bool> stNumberSwitchover(true);
+std::atomic<bool> stNumberSwitchover(true);
 
 /* The range for the mantissa when normalized */
 static std::int64_t constexpr minMantissa = 1000000000000000ull;
@@ -51,7 +51,7 @@ IOUAmount::normalize()
         return;
     }
 
-    if (*stNumberSwitchover)
+    if (stNumberSwitchover)
     {
         Number v{mantissa_, exponent_};
         mantissa_ = v.mantissa();
@@ -117,7 +117,7 @@ IOUAmount::operator+=(IOUAmount const& other)
         return *this;
     }
 
-    if (*stNumberSwitchover)
+    if (stNumberSwitchover)
     {
         *this = IOUAmount{Number{*this} + Number{other}};
         return *this;

diff --git a/src/ripple/protocol/impl/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp
@@ -395,7 +395,7 @@ operator+(STAmount const& v1, STAmount const& v2)
     if (v1.native())
         return {v1.getFName(), getSNValue(v1) + getSNValue(v2)};
 
-    if (*stNumberSwitchover)
+    if (stNumberSwitchover)
     {
         auto x = v1;
         x = v1.iou() + v2.iou();
@@ -725,24 +725,35 @@ STAmount::canonicalize()
                     "Native currency amount out of range");
         }
 
-        while (mOffset < 0)
+        if (stNumberSwitchover && *stAmountCanonicalizeSwitchover)
         {
-            mValue /= 10;
-            ++mOffset;
+            Number num(mIsNegative ? -mValue : mValue, mOffset, Number::unchecked{});
+            XRPAmount xrp{num};
+            mIsNegative = xrp.drops() < 0;
+            mValue = mIsNegative ? -xrp.drops() : xrp.drops();
+            mOffset = 0;
         }
-
-        while (mOffset > 0)
+        else
         {
-            if (*stAmountCanonicalizeSwitchover)
+            while (mOffset < 0)
+            {
+                mValue /= 10;
+                ++mOffset;
+            }
+
+            while (mOffset > 0)
             {
-                // N.B. do not move the overflow check to after the
-                // multiplication
-                if (mValue > cMaxNativeN)
-                    Throw<std::runtime_error>(
-                        "Native currency amount out of range");
+                if (*stAmountCanonicalizeSwitchover)
+                {
+                    // N.B. do not move the overflow check to after the
+                    // multiplication
+                    if (mValue > cMaxNativeN)
+                        Throw<std::runtime_error>(
+                            "Native currency amount out of range");
+                }
+                mValue *= 10;
+                --mOffset;
             }
-            mValue *= 10;
-            --mOffset;
         }
 
         if (mValue > cMaxNativeN)
@@ -753,7 +764,7 @@ STAmount::canonicalize()
 
     mIsNative = false;
 
-    if (*stNumberSwitchover)
+    if (stNumberSwitchover)
     {
         *this = iou();
         return;
@@ -1196,7 +1207,7 @@ multiply(STAmount const& v1, STAmount const& v2, Issue const& issue)
         return STAmount(v1.getFName(), minV * maxV);
     }
 
-    if (*stNumberSwitchover)
+    if (stNumberSwitchover)
         return {IOUAmount{Number{v1} * Number{v2}}, issue};
 
     std::uint64_t value1 = v1.mantissa();

diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp
@@ -2335,7 +2335,13 @@ class NFToken_test : public beast::unit_test::suite
 
         // See the impact of rounding when the nft is sold for small amounts
         // of drops.
+        for (auto NumberSwitchOver : {true})
         {
+            if (NumberSwitchOver)
+                env.enableFeature(fixUniversalNumber);
+            else
+                env.disableFeature(fixUniversalNumber);
+
             // An nft with a transfer fee of 1 basis point.
             uint256 const nftID =
                 token::getNextID(env, alice, 0u, tfTransferable, 1);
@@ -2360,16 +2366,17 @@ class NFToken_test : public beast::unit_test::suite
 
             // minter sells to carol.  The payment is just small enough that
             // alice does not get any transfer fee.
+            auto pmt = NumberSwitchOver ? drops(50000) : drops(99999);
             STAmount carolBalance = env.balance(carol);
             uint256 const minterSellOfferIndex =
                 keylet::nftoffer(minter, env.seq(minter)).key;
-            env(token::createOffer(minter, nftID, drops(99999)),
+            env(token::createOffer(minter, nftID, pmt),
                 txflags(tfSellNFToken));
             env.close();
             env(token::acceptSellOffer(carol, minterSellOfferIndex));
             env.close();
-            minterBalance += drops(99999) - fee;
-            carolBalance -= drops(99999) + fee;
+            minterBalance += pmt - fee;
+            carolBalance -= pmt + fee;
             BEAST_EXPECT(env.balance(alice) == aliceBalance);
             BEAST_EXPECT(env.balance(minter) == minterBalance);
             BEAST_EXPECT(env.balance(carol) == carolBalance);
@@ -2379,13 +2386,14 @@ class NFToken_test : public beast::unit_test::suite
             STAmount beckyBalance = env.balance(becky);
             uint256 const beckyBuyOfferIndex =
                 keylet::nftoffer(becky, env.seq(becky)).key;
-            env(token::createOffer(becky, nftID, drops(100000)),
+            pmt = NumberSwitchOver ? drops(50001) : drops(100000);
+            env(token::createOffer(becky, nftID, pmt),
                 token::owner(carol));
             env.close();
             env(token::acceptBuyOffer(carol, beckyBuyOfferIndex));
             env.close();
-            carolBalance += drops(99999) - fee;
-            beckyBalance -= drops(100000) + fee;
+            carolBalance += pmt - drops(1) - fee;
+            beckyBalance -= pmt + fee;
             aliceBalance += drops(1);
 
             BEAST_EXPECT(env.balance(alice) == aliceBalance);