Fix serialization. Extend unit-tests.

include/xrpl/basics/MPTAmount.h

@@ -42,7 +42,6 @@ class MPTAmount : private boost::totally_ordered<MPTAmount>,
 {
 public:
     using value_type = std::int64_t;
-    static constexpr std::uint64_t cMaxMPTValue = 0x8000000000000000;
 
 protected:
     value_type value_;

include/xrpl/protocol/STAmount.h

@@ -79,7 +79,6 @@ class STAmount final
     static std::uint64_t const uRateOne;
 
     //--------------------------------------------------------------------------
-    STAmount(std::uint64_t value, SerialIter& sit);
     STAmount(SerialIter& sit);
 
     struct unchecked

include/xrpl/protocol/STMPTAmount.h

@@ -35,10 +35,15 @@ class STMPTAmount final : public MPTAmount
     MPTIssue issue_;
 
 public:
-    static constexpr std::uint64_t cMPToken = 0x2000000000000000ull;
-
-    STMPTAmount(std::uint64_t value, SerialIter& sit);
-    STMPTAmount(MPTIssue const& issue, std::uint64_t value);
+    static constexpr std::uint8_t cMPToken = 0x20;
+    static constexpr std::uint8_t cPositive = 0x40;
+    static constexpr std::uint8_t cMask = cMPToken | cPositive;
+
+    STMPTAmount(SerialIter& sit);
+    STMPTAmount(
+        MPTIssue const& issue,
+        std::uint64_t value,
+        bool negative = false);
     STMPTAmount(MPTIssue const& issue, std::int64_t value = 0);
     explicit STMPTAmount(value_type value = 0);
 

include/xrpl/protocol/Serializer.h

@@ -344,6 +344,10 @@ class SerialIter
         return static_cast<int>(remain_);
     }
 
+    // peek function, throw on error
+    unsigned char
+    peek8();
+
     // get functions throw on error
     unsigned char
     get8();

src/libxrpl/protocol/STAmount.cpp

@@ -85,8 +85,9 @@ areComparable(STAmount const& v1, STAmount const& v2)
         v1.issue().currency == v2.issue().currency;
 }
 
-STAmount::STAmount(std::uint64_t value, SerialIter& sit)
+STAmount::STAmount(SerialIter& sit)
 {
+    std::uint64_t value = sit.get64();
     // native
     if ((value & cNotNative) == 0)
     {
@@ -156,10 +157,6 @@ STAmount::STAmount(std::uint64_t value, SerialIter& sit)
     canonicalize();
 }
 
-STAmount::STAmount(ripple::SerialIter& sit) : STAmount(sit.get64(), sit)
-{
-}
-
 STAmount::STAmount(
     Issue const& issue,
     mantissa_type mantissa,

src/libxrpl/protocol/STEitherAmount.cpp

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include <xrpl/basics/Log.h>
+#include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/STEitherAmount.h>
 #include <xrpl/protocol/SystemParameters.h>
 #include <xrpl/protocol/jss.h>
@@ -29,12 +30,12 @@ namespace ripple {
 STEitherAmount::STEitherAmount(SerialIter& sit, SField const& name)
     : STBase(name)
 {
-    auto const value = sit.get64();
-    if ((value & STAmount::cNotNative) == 0 &&
-        (value & STMPTAmount::cMPToken) != 0)
-        amount_.emplace<STMPTAmount>(value, sit);
+    auto const u8 = sit.peek8();
+    if (((static_cast<std::uint64_t>(u8) << 56) & STAmount::cNotNative) == 0 &&
+        (u8 & STMPTAmount::cMPToken) != 0)
+        amount_.emplace<STMPTAmount>(sit);
     else
-        amount_.emplace<STAmount>(value, sit);
+        amount_.emplace<STAmount>(sit);
 }
 
 STEitherAmount::STEitherAmount(XRPAmount const& amount) : amount_{amount}
@@ -346,9 +347,9 @@ amountFromJson(SField const& name, Json::Value const& v)
         {
             STMPTAmount const ret =
                 amountFromString(std::get<MPTIssue>(issue), value.asString());
-            mantissa = ret.value();
+            negative = ret.value() < 0;
+            mantissa = !negative ? ret.value() : -ret.value();
             exponent = 0;
-            negative = false;
         }
     }
     else
@@ -363,12 +364,10 @@ amountFromJson(SField const& name, Json::Value const& v)
                 std::get<Issue>(issue), mantissa, exponent, native, negative}};
     while (exponent-- > 0)
         mantissa *= 10;
-    if (mantissa > STMPTAmount::cMaxMPTValue)
+    if (mantissa > maxMPTokenAmount)
         Throw<std::runtime_error>("MPT amount out of range");
     return STEitherAmount{
-        name,
-        STMPTAmount{
-            std::get<MPTIssue>(issue), static_cast<std::int64_t>(mantissa)}};
+        name, STMPTAmount{std::get<MPTIssue>(issue), mantissa, negative}};
 }
 
 }  // namespace detail

src/libxrpl/protocol/STMPTAmount.cpp

@@ -18,19 +18,24 @@
 //==============================================================================
 
 #include <xrpl/beast/core/LexicalCast.h>
+#include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/STMPTAmount.h>
 #include <xrpl/protocol/jss.h>
 
 #include <boost/regex.hpp>
 
 namespace ripple {
 
-STMPTAmount::STMPTAmount(std::uint64_t value, SerialIter& sit)
+STMPTAmount::STMPTAmount(SerialIter& sit)
 {
-    assert(value & cMPToken);
-    value_ = (value << 8) | sit.get8();
-    value_ &= ~cMPToken;
+    auto const mask = sit.get8();
+    assert((mask & cMPToken) && (mask & ~cMask) == 0);
+    if (((mask & cMPToken) == 0) || (mask & ~cMask))
+        Throw<std::logic_error>("Not MPT Amount.");
 
+    value_ = sit.get64();
+    if ((mask & cPositive) == 0)
+        value_ = -value_;
     issue_ = sit.get192();
 }
 
@@ -39,12 +44,17 @@ STMPTAmount::STMPTAmount(MPTIssue const& issue, value_type value)
 {
 }
 
-STMPTAmount::STMPTAmount(MPTIssue const& issue, std::uint64_t value)
+STMPTAmount::STMPTAmount(
+    MPTIssue const& issue,
+    std::uint64_t value,
+    bool negative)
     : issue_(issue)
 {
-    if (value > cMaxMPTValue)
+    if (value > maxMPTokenAmount)
         Throw<std::logic_error>("MPTAmount is out of range");
     value_ = static_cast<std::int64_t>(value);
+    if (negative)
+        value_ = -value_;
 }
 
 STMPTAmount::STMPTAmount(value_type value) : MPTAmount(value)
@@ -90,9 +100,11 @@ STMPTAmount::setJson(Json::Value& elem) const
 void
 STMPTAmount::add(Serializer& s) const
 {
-    auto u8 = static_cast<unsigned char>(cMPToken >> 56);
+    auto u8 = cMPToken;
+    if (value_ >= 0)
+        u8 |= cPositive;
     s.add8(u8);
-    s.add64(value_);
+    s.add64(value_ >= 0 ? value_ : -value_);
     s.addBitString(issue_.getMptID());
 }
 
@@ -150,7 +162,7 @@ amountFromString(MPTIssue const& issue, std::string const& amount)
 {
     static boost::regex const reNumber(
         "^"                       // the beginning of the string
-        "([+]?)"                  // (optional) + character (MPT is positive)
+        "([+-]?)"                 // (optional) + character
         "(0|[1-9][0-9]*)"         // a number (no leading zeroes, unless 0)
         "(\\.([0-9]+))?"          // (optional) period followed by any number
         "([eE]([+-]?)([0-9]+))?"  // (optional) E, optional + or -, any number
@@ -180,9 +192,11 @@ amountFromString(MPTIssue const& issue, std::string const& amount)
     if (match[3].matched)
         Throw<std::runtime_error>("MPT must be specified as integral.");
 
-    std::int64_t mantissa;
+    std::uint64_t mantissa;
     int exponent;
 
+    bool negative = (match[1].matched && (match[1] == "-"));
+
     if (!match[4].matched)  // integer only
     {
         mantissa =
@@ -208,7 +222,7 @@ amountFromString(MPTIssue const& issue, std::string const& amount)
     while (exponent-- > 0)
         mantissa *= 10;
 
-    return {issue, mantissa};
+    return {issue, mantissa, negative};
 }
 
 }  // namespace ripple

src/libxrpl/protocol/Serializer.cpp

@@ -358,6 +358,15 @@ SerialIter::skip(int length)
     remain_ -= length;
 }
 
+unsigned char
+SerialIter::peek8()
+{
+    if (remain_ < 1)
+        Throw<std::runtime_error>("invalid SerialIter peek8");
+    unsigned char t = *p_;
+    return t;
+}
+
 unsigned char
 SerialIter::get8()
 {

src/test/app/MPToken_test.cpp

@@ -787,25 +787,53 @@ class MPToken_test : public beast::unit_test::suite
             mptAlice.pay(alice, bob, 1, tecMPT_MAX_AMOUNT_EXCEEDED);
         }
 
-        // TODO: This test is currently failing! Modify the STAmount to change
-        // the range
         // Issuer fails trying to send more than the default maximum
         // amount allowed
-        // {
-        //     Env env{*this, features};
+        {
+            Env env{*this, features};
+
+            MPTTester mptAlice(env, alice, {.holders = {&bob}});
+
+            mptAlice.create({.ownerCount = 1, .holderCount = 0});
+
+            mptAlice.authorize({.account = &bob});
+
+            // issuer sends holder the default max amount allowed
+            mptAlice.pay(alice, bob, maxMPTokenAmount);
+
+            // issuer tries to exceed max amount
+            mptAlice.pay(alice, bob, 1, tecMPT_MAX_AMOUNT_EXCEEDED);
+        }
 
-        //     MPTTester mptAlice(env, alice, {.holders = {&bob}});
+        // Can't pay negative amount
+        {
+            Env env{*this, features};
 
-        //     mptAlice.create({.ownerCount = 1, .holderCount = 0});
+            MPTTester mptAlice(env, alice, {.holders = {&bob}});
 
-        //     mptAlice.authorize({.account = &bob});
+            mptAlice.create({.ownerCount = 1, .holderCount = 0});
 
-        //     // issuer sends holder the default max amount allowed
-        //     mptAlice.pay(alice, bob, maxMPTokenAmount);
+            mptAlice.authorize({.account = &bob});
 
-        //     // issuer tries to exceed max amount
-        //     mptAlice.pay(alice, bob, 1, tecMPT_MAX_AMOUNT_EXCEEDED);
-        // }
+            mptAlice.pay(alice, bob, -1, temBAD_AMOUNT);
+        }
+
+        // pay more than max amount
+        // fails in the json parser before
+        // transactor is called
+        {
+            Env env{*this, features};
+            env.fund(XRP(1'000), alice, bob);
+            STMPTAmount mpt{
+                MPTIssue{std::make_pair(1, alice.id())}, UINT64_C(100)};
+            Json::Value jv;
+            jv[jss::secret] = alice.name();
+            jv[jss::tx_json] = pay(alice, bob, mpt);
+            jv[jss::tx_json][jss::Amount][jss::value] =
+                to_string(maxMPTokenAmount + 1);
+            auto const jrr = env.rpc("json", "submit", to_string(jv));
+            BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
+        }
 
         // Transfer fee
         {
@@ -1090,10 +1118,6 @@ class MPToken_test : public beast::unit_test::suite
                 jv[jss::SendMax] = mpt.getJson(JsonOptions::none);
                 test(jv);
             }
-            // Clawback
-            if (!feature[featureMPTokensV1])
-            {
-            }
             // EscrowCreate
             {
                 Json::Value jv;
@@ -1348,9 +1372,9 @@ class MPToken_test : public beast::unit_test::suite
             env(claw(alice, mpt(5), alice), ter(temMALFORMED));
             env.close();
 
-            // TODO: uncomment after stamount changes
-            // env(claw(alice, mpt(maxMPTokenAmount), bob), ter(temBAD_AMOUNT));
-            // env.close();
+            // can't clawback negative amount
+            env(claw(alice, mpt(-1), bob), ter(temBAD_AMOUNT));
+            env.close();
         }
 
         // Preclaim - clawback fails when MPTCanClawback is disabled on issuance
@@ -1415,6 +1439,29 @@ class MPToken_test : public beast::unit_test::suite
             // issuer
             mptAlice.claw(carol, bob, 1, tecNO_PERMISSION);
         }
+
+        // clawback more than max amount
+        // fails in the json parser before
+        // transactor is called
+        {
+            Env env(*this, features);
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+
+            env.fund(XRP(1000), alice, bob);
+            env.close();
+
+            auto const mpt = ripple::test::jtx::MPT(
+                alice.name(), std::make_pair(env.seq(alice), alice.id()));
+
+            Json::Value jv = claw(alice, mpt(1), bob);
+            jv[jss::Amount][jss::value] = to_string(maxMPTokenAmount + 1);
+            Json::Value jv1;
+            jv1[jss::secret] = alice.name();
+            jv1[jss::tx_json] = jv;
+            auto const jrr = env.rpc("json", "submit", to_string(jv1));
+            BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
+        }
     }
 
     void
@@ -1564,11 +1611,6 @@ class MPToken_test : public beast::unit_test::suite
         testMPTInvalidInTx(all);
 
         // Test parsed MPTokenIssuanceID in API response metadata
-        // TODO: This test exercises the parsing logic of mptID in `tx`,
-        // but,
-        //       mptID is also parsed in different places like `account_tx`,
-        //       `subscribe`, `ledger`. We should create test for these
-        //       occurances (lower prioirity).
         testTxJsonMetaFields(all);
     }
 };