code review changes

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>

include/xrpl/protocol/IOUAmount.h

@@ -179,36 +179,4 @@ to_string(IOUAmount const& amount);
 IOUAmount
 mulRatio(IOUAmount const& amt, std::uint32_t num, std::uint32_t den, bool roundUp);
 
-// Since many uses of the number class do not have access to a ledger,
-// getSTNumberSwitchover needs to be globally accessible.
-
-bool
-getSTNumberSwitchover();
-
-void
-setSTNumberSwitchover(bool v);
-
-/** RAII class to set and restore the Number switchover.
- */
-
-class NumberSO
-{
-    bool saved_;
-
-public:
-    ~NumberSO()
-    {
-        setSTNumberSwitchover(saved_);
-    }
-
-    NumberSO(NumberSO const&) = delete;
-    NumberSO&
-    operator=(NumberSO const&) = delete;
-
-    explicit NumberSO(bool v) : saved_(getSTNumberSwitchover())
-    {
-        setSTNumberSwitchover(v);
-    }
-};
-
 }  // namespace xrpl

include/xrpl/protocol/detail/features.macro

@@ -68,7 +68,6 @@ XRPL_FIX    (DisallowIncomingV1,          Supported::yes, VoteBehavior::DefaultN
 XRPL_FEATURE(XChainBridge,                Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(AMM,                         Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(Clawback,                    Supported::yes, VoteBehavior::DefaultNo)
-XRPL_FIX    (UniversalNumber,             Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(XRPFees,                     Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (RemoveNFTokenAutoTrustLine,  Supported::yes, VoteBehavior::DefaultYes)
 
@@ -116,6 +115,7 @@ XRPL_RETIRE_FIX(RmSmallIncreasedQOffers)
 XRPL_RETIRE_FIX(STAmountCanonicalize)
 XRPL_RETIRE_FIX(TakerDryOfferRemoval)
 XRPL_RETIRE_FIX(TrustLinesToSelf)
+XRPL_RETIRE_FIX(UniversalNumber)
 
 XRPL_RETIRE_FEATURE(Checks)
 XRPL_RETIRE_FEATURE(CheckCashMakesTrustLine)

src/libxrpl/protocol/AMMCore.cpp

@@ -128,7 +128,7 @@ ammAuctionTimeSlot(std::uint64_t current, STObject const& auctionSlot)
 bool
 ammEnabled(Rules const& rules)
 {
-    return rules.enabled(featureAMM) && rules.enabled(fixUniversalNumber);
+    return rules.enabled(featureAMM);
 }
 
 }  // namespace xrpl

src/libxrpl/protocol/IOUAmount.cpp

@@ -17,29 +17,6 @@
 
 namespace xrpl {
 
-namespace {
-
-// Use a static inside a function to help prevent order-of-initialization issues
-LocalValue<bool>&
-getStaticSTNumberSwitchover()
-{
-    static LocalValue<bool> r{true};
-    return r;
-}
-}  // namespace
-
-bool
-getSTNumberSwitchover()
-{
-    return *getStaticSTNumberSwitchover();
-}
-
-void
-setSTNumberSwitchover(bool v)
-{
-    *getStaticSTNumberSwitchover() = v;
-}
-
 /* The range for the mantissa when normalized */
 // log(2^63,10) ~ 18.96
 //
@@ -139,37 +116,7 @@ IOUAmount::operator+=(IOUAmount const& other)
         return *this;
     }
 
-    if (getSTNumberSwitchover())
-    {
-        *this = IOUAmount{Number{*this} + Number{other}};
-        return *this;
-    }
-    auto m = other.mantissa_;
-    auto e = other.exponent_;
-
-    while (exponent_ < e)
-    {
-        mantissa_ /= 10;
-        ++exponent_;
-    }
-
-    while (e < exponent_)
-    {
-        m /= 10;
-        ++e;
-    }
-
-    // This addition cannot overflow an std::int64_t but we may throw from
-    // normalize if the result isn't representable.
-    mantissa_ += m;
-
-    if (mantissa_ >= -10 && mantissa_ <= 10)
-    {
-        *this = beast::zero;
-        return *this;
-    }
-
-    normalize();
+    *this = IOUAmount{Number{*this} + Number{other}};
     return *this;
 }
 

src/libxrpl/protocol/STAmount.cpp

@@ -386,47 +386,9 @@ operator+(STAmount const& v1, STAmount const& v2)
     if (v1.holds<MPTIssue>())
         return {v1.mAsset, v1.mpt().value() + v2.mpt().value()};
 
-    if (getSTNumberSwitchover())
-    {
-        auto x = v1;
-        x = v1.iou() + v2.iou();
-        return x;
-    }
-
-    int ov1 = v1.exponent(), ov2 = v2.exponent();
-    std::int64_t vv1 = static_cast<std::int64_t>(v1.mantissa());
-    std::int64_t vv2 = static_cast<std::int64_t>(v2.mantissa());
-
-    if (v1.negative())
-        vv1 = -vv1;
-
-    if (v2.negative())
-        vv2 = -vv2;
-
-    while (ov1 < ov2)
-    {
-        vv1 /= 10;
-        ++ov1;
-    }
-
-    while (ov2 < ov1)
-    {
-        vv2 /= 10;
-        ++ov2;
-    }
-
-    // This addition cannot overflow an std::int64_t. It can overflow an
-    // STAmount and the constructor will throw.
-
-    std::int64_t const fv = vv1 + vv2;
-
-    if ((fv >= -10) && (fv <= 10))
-        return {v1.getFName(), v1.asset()};
-
-    if (fv >= 0)
-        return STAmount{v1.getFName(), v1.asset(), static_cast<std::uint64_t>(fv), ov1, false};
-
-    return STAmount{v1.getFName(), v1.asset(), static_cast<std::uint64_t>(-fv), ov1, true};
+    STAmount x{};
+    x = v1.iou() + v2.iou();
+    return x;
 }
 
 STAmount
@@ -875,53 +837,16 @@ STAmount::canonicalize()
         if (mAsset.holds<MPTIssue>() && mOffset > 18)
             Throw<std::runtime_error>("MPT amount out of range");
 
-        if (getSTNumberSwitchover())
-        {
-            Number const num(mIsNegative, mValue, mOffset, Number::unchecked{});
-            auto set = [&](auto const& val) {
-                auto const value = val.value();
-                mIsNegative = value < 0;
-                mValue = mIsNegative ? -value : value;
-            };
-            if (native())
-            {
-                set(XRPAmount{num});
-            }
-            else if (mAsset.holds<MPTIssue>())
-            {
-                set(MPTAmount{num});
-            }
-            else
-            {
-                Throw<std::runtime_error>("Unknown integral asset type");
-            }
-            mOffset = 0;
-        }
+        Number num(mIsNegative ? -mValue : mValue, mOffset, Number::unchecked{});
+        auto set = [&](auto const& val) {
+            mIsNegative = val.value() < 0;
+            mValue = mIsNegative ? -val.value() : val.value();
+        };
+        if (native())
+            set(XRPAmount{num});
         else
-        {
-            while (mOffset < 0)
-            {
-                mValue /= 10;
-                ++mOffset;
-            }
-
-            while (mOffset > 0)
-            {
-                // N.B. do not move the overflow check to after the
-                // multiplication
-                if (native() && mValue > cMaxNativeN)
-                {
-                    Throw<std::runtime_error>("Native currency amount out of range");
-                }
-                else if (!native() && mValue > maxMPTokenAmount)
-                {
-                    Throw<std::runtime_error>("MPT amount out of range");
-                }
-
-                mValue *= 10;
-                --mOffset;
-            }
-        }
+            set(MPTAmount{num});
+        mOffset = 0;
 
         if (native() && mValue > cMaxNativeN)
         {
@@ -935,53 +860,7 @@ STAmount::canonicalize()
         return;
     }
 
-    if (getSTNumberSwitchover())
-    {
-        *this = iou();
-        return;
-    }
-
-    if (mValue == 0)
-    {
-        mOffset = -100;
-        mIsNegative = false;
-        return;
-    }
-
-    while ((mValue < cMinValue) && (mOffset > cMinOffset))
-    {
-        mValue *= 10;
-        --mOffset;
-    }
-
-    while (mValue > cMaxValue)
-    {
-        if (mOffset >= cMaxOffset)
-            Throw<std::runtime_error>("value overflow");
-
-        mValue /= 10;
-        ++mOffset;
-    }
-
-    if ((mOffset < cMinOffset) || (mValue < cMinValue))
-    {
-        mValue = 0;
-        mIsNegative = false;
-        mOffset = -100;
-        return;
-    }
-
-    if (mOffset > cMaxOffset)
-        Throw<std::runtime_error>("value overflow");
-
-    XRPL_ASSERT(
-        (mValue == 0) || ((mValue >= cMinValue) && (mValue <= cMaxValue)),
-        "xrpl::STAmount::canonicalize : value inside range");
-    XRPL_ASSERT(
-        (mValue == 0) || ((mOffset >= cMinOffset) && (mOffset <= cMaxOffset)),
-        "xrpl::STAmount::canonicalize : offset inside range");
-    XRPL_ASSERT(
-        (mValue != 0) || (mOffset != -100), "xrpl::STAmount::canonicalize : value or offset set");
+    *this = iou();
 }
 
 void
@@ -1349,44 +1228,8 @@ multiply(STAmount const& v1, STAmount const& v2, Asset const& asset)
         return STAmount(asset, minV * maxV);
     }
 
-    if (getSTNumberSwitchover())
-    {
-        auto const r = Number{v1} * Number{v2};
-        return STAmount{asset, r};
-    }
-
-    std::uint64_t value1 = v1.mantissa();
-    std::uint64_t value2 = v2.mantissa();
-    int offset1 = v1.exponent();
-    int offset2 = v2.exponent();
-
-    if (v1.integral())
-    {
-        while (value1 < STAmount::cMinValue)
-        {
-            value1 *= 10;
-            --offset1;
-        }
-    }
-
-    if (v2.integral())
-    {
-        while (value2 < STAmount::cMinValue)
-        {
-            value2 *= 10;
-            --offset2;
-        }
-    }
-
-    // We multiply the two mantissas (each is between 10^15
-    // and 10^16), so their product is in the 10^30 to 10^32
-    // range. Dividing their product by 10^14 maintains the
-    // precision, by scaling the result to 10^16 to 10^18.
-    return STAmount(
-        asset,
-        muldiv(value1, value2, tenTo14) + 7,
-        offset1 + offset2 + 14,
-        v1.negative() != v2.negative());
+    auto const r = Number{v1} * Number{v2};
+    return STAmount{asset, r.mantissa(), r.exponent()};
 }
 
 // This is the legacy version of canonicalizeRound.  It's been in use

src/libxrpl/tx/Transactor.cpp

@@ -1178,13 +1178,6 @@ Transactor::operator()()
 {
     JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID();
 
-    // These global updates really should have been for every Transaction
-    // step: preflight, preclaim, and doApply. And even calculateBaseFee. See
-    // with_txn_type().
-    //
-    // raii classes for the current ledger rules.
-    // fixUniversalNumber predate the rulesGuard and should be replaced.
-    NumberSO const stNumberSO{view().rules().enabled(fixUniversalNumber)};
     CurrentTransactionRulesGuard const currentTransactionRulesGuard(view().rules());
 
 #ifdef DEBUG

src/test/app/AMM_test.cpp

@@ -4339,26 +4339,19 @@ private:
     testAmendment()
     {
         testcase("Amendment");
-        FeatureBitset const all{testable_amendments()};
-        FeatureBitset const noAMM{all - featureAMM};
-        FeatureBitset const noNumber{all - fixUniversalNumber};
-        FeatureBitset const noAMMAndNumber{all - featureAMM - fixUniversalNumber};
-        using namespace jtx;
+        FeatureBitset const noAMM{testable_amendments() - featureAMM};
 
-        for (auto const& feature : {noAMM, noNumber, noAMMAndNumber})
-        {
-            Env env{*this, feature};
-            fund(env, gw, {alice}, {USD(1'000)}, Fund::All);
-            AMM amm(env, alice, XRP(1'000), USD(1'000), ter(temDISABLED));
+        Env env{*this, noAMM};
+        fund(env, gw, {alice}, {USD(1'000)}, Fund::All);
+        AMM amm(env, alice, XRP(1'000), USD(1'000), ter(temDISABLED));
 
-            env(amm.bid({.bidMax = 1000}), ter(temMALFORMED));
-            env(amm.bid({}), ter(temDISABLED));
-            amm.vote(VoteArg{.tfee = 100, .err = ter(temDISABLED)});
-            amm.withdraw(WithdrawArg{.tokens = 100, .err = ter(temMALFORMED)});
-            amm.withdraw(WithdrawArg{.err = ter(temDISABLED)});
-            amm.deposit(DepositArg{.asset1In = USD(100), .err = ter(temDISABLED)});
-            amm.ammDelete(alice, ter(temDISABLED));
-        }
+        env(amm.bid({.bidMax = 1000}), ter(temMALFORMED));
+        env(amm.bid({}), ter(temDISABLED));
+        amm.vote(VoteArg{.tfee = 100, .err = ter(temDISABLED)});
+        amm.withdraw(WithdrawArg{.tokens = 100, .err = ter(temMALFORMED)});
+        amm.withdraw(WithdrawArg{.err = ter(temDISABLED)});
+        amm.deposit(DepositArg{.asset1In = USD(100), .err = ter(temDISABLED)});
+        amm.ammDelete(alice, ter(temDISABLED));
     }
 
     void

src/test/app/NFToken_test.cpp

@@ -2218,17 +2218,7 @@ class NFTokenBaseUtil_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);
             env(token::mint(alice), txflags(tfTransferable), token::xferFee(1));
@@ -2250,7 +2240,7 @@ class NFTokenBaseUtil_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);
+            auto pmt = drops(50000);
             STAmount carolBalance = env.balance(carol);
             uint256 const minterSellOfferIndex = keylet::nftoffer(minter, env.seq(minter)).key;
             env(token::createOffer(minter, nftID, pmt), txflags(tfSellNFToken));

src/test/app/Offer_test.cpp

@@ -1968,54 +1968,36 @@ public:
 
         using namespace jtx;
 
-        for (auto NumberSwitchOver : {false, true})
-        {
-            Env env{*this, features};
-            if (NumberSwitchOver)
-            {
-                env.enableFeature(fixUniversalNumber);
-            }
-            else
-            {
-                env.disableFeature(fixUniversalNumber);
-            }
+        Env env{*this, features};
 
-            auto const gw = Account{"gateway"};
-            auto const alice = Account{"alice"};
-            auto const bob = Account{"bob"};
-            auto const USD = gw["USD"];
+        auto const gw = Account{"gateway"};
+        auto const alice = Account{"alice"};
+        auto const bob = Account{"bob"};
+        auto const USD = gw["USD"];
 
-            env.fund(XRP(10000), gw, alice, bob);
-            env.close();
+        env.fund(XRP(10000), gw, alice, bob);
+        env.close();
 
-            env(rate(gw, 1.005));
+        env(rate(gw, 1.005));
 
-            env(trust(alice, USD(1000)));
-            env(trust(bob, USD(1000)));
-            env(trust(gw, alice["USD"](50)));
+        env(trust(alice, USD(1000)));
+        env(trust(bob, USD(1000)));
+        env(trust(gw, alice["USD"](50)));
 
-            env(pay(gw, bob, bob["USD"](1)));
-            env(pay(alice, gw, USD(50)));
+        env(pay(gw, bob, bob["USD"](1)));
+        env(pay(alice, gw, USD(50)));
 
-            env(trust(gw, alice["USD"](0)));
+        env(trust(gw, alice["USD"](0)));
 
-            env(offer(alice, USD(50), XRP(150000)));
-            env(offer(bob, XRP(100), USD(0.1)));
+        env(offer(alice, USD(50), XRP(150000)));
+        env(offer(bob, XRP(100), USD(0.1)));
 
-            auto jrr = ledgerEntryState(env, alice, gw, "USD");
-            BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "49.96666666666667");
+        auto jrr = ledgerEntryState(env, alice, gw, "USD");
+        BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "49.96666666666667");
 
-            jrr = ledgerEntryState(env, bob, gw, "USD");
-            Json::Value const bobUSD = jrr[jss::node][sfBalance.fieldName][jss::value];
-            if (!NumberSwitchOver)
-            {
-                BEAST_EXPECT(bobUSD == "-0.966500000033334");
-            }
-            else
-            {
-                BEAST_EXPECT(bobUSD == "-0.9665000000333333");
-            }
-        }
+        jrr = ledgerEntryState(env, bob, gw, "USD");
+        Json::Value const bobsUSD = jrr[jss::node][sfBalance.fieldName][jss::value];
+        BEAST_EXPECT(bobsUSD == "-0.9665000000333333");
     }
 
     void

src/test/basics/Number_test.cpp

@@ -1345,7 +1345,6 @@ public:
     void
     test_toSTAmount()
     {
-        NumberSO const stNumberSO{true};
         Issue const issue;
         Number const n{7'518'783'80596, -5};
         saveNumberRoundMode const save{Number::setround(Number::to_nearest)};

src/xrpld/app/misc/detail/TxQ.cpp

@@ -305,7 +305,6 @@ TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j)
 {
     // If the rules or flags change, preflight again
     XRPL_ASSERT(pfResult, "xrpl::TxQ::MaybeTx::apply : preflight result is set");
-    NumberSO const stNumberSO{view.rules().enabled(fixUniversalNumber)};
 
     // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above
     if (pfResult->rules != view.rules() || pfResult->flags != flags)
@@ -730,8 +729,6 @@ TxQ::apply(
     ApplyFlags flags,
     beast::Journal j)
 {
-    NumberSO const stNumberSO{view.rules().enabled(fixUniversalNumber)};
-
     // See if the transaction is valid, properly formed,
     // etc. before doing potentially expensive queue
     // replace and multi-transaction operations.