Make LoanSet.CounterpartySignature optional in the Tx layout

- Still required for the transaction to succeed (except inside a Batch,
  because the batch signers take care of that).
- Started adding tests for Loan-related RPC and low-level math checks.
  Currently only implemented "sign" on a LoanSet to verify it can be
  done.

include/xrpl/protocol/detail/transactions.macro

@@ -904,7 +904,7 @@ TRANSACTION(ttLOAN_SET, 80, LoanSet,
     {sfLoanBrokerID, soeREQUIRED},
     {sfData, soeOPTIONAL},
     {sfCounterparty, soeOPTIONAL},
-    {sfCounterpartySignature, soeREQUIRED},
+    {sfCounterpartySignature, soeOPTIONAL},
     {sfLoanOriginationFee, soeOPTIONAL},
     {sfLoanServiceFee, soeOPTIONAL},
     {sfLatePaymentFee, soeOPTIONAL},

include/xrpl/protocol/jss.h

@@ -59,6 +59,7 @@ JSS(BaseAsset);            // in: Oracle
 JSS(BidMax);               // in: AMM Bid
 JSS(BidMin);               // in: AMM Bid
 JSS(ClearFlag);            // field.
+JSS(CounterpartySignature);// field.
 JSS(DeliverMax);           // out: alias to Amount
 JSS(DeliverMin);           // in: TransactionSign
 JSS(Destination);          // in: TransactionSign; field.

src/test/app/Batch_test.cpp

@@ -2648,30 +2648,6 @@ class Batch_test : public beast::unit_test::suite
                             STAmount{asset, asset(500).value()}),
                         lenderSeq + 2));
             }
-            {
-                auto const [txIDs, batchID] = submitBatch(
-                    env,
-                    telENV_RPC_FAILED,
-                    batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing),
-                    batch::inner(
-                        env.json(
-                            set(lender,
-                                brokerKeylet.key,
-                                asset(1000).value(),
-                                env.now() + 3600s),
-                            // Must include a CounterpartySignature field.
-                            // Transaction will not even parse at the RPC layer
-                            sig(none),
-                            fee(none),
-                            seq(none)),
-                        lenderSeq + 1),
-                    batch::inner(
-                        draw(
-                            lender,
-                            loanKeylet.key,
-                            STAmount{asset, asset(500).value()}),
-                        lenderSeq + 2));
-            }
             {
                 auto const [txIDs, batchID] = submitBatch(
                     env,
@@ -2683,7 +2659,6 @@ class Batch_test : public beast::unit_test::suite
                                 brokerKeylet.key,
                                 asset(1000).value(),
                                 env.now() + 3600s),
-                            json(sfCounterpartySignature, Json::objectValue),
                             // Counterparty must be set
                             sig(none),
                             fee(none),
@@ -2709,7 +2684,6 @@ class Batch_test : public beast::unit_test::suite
                                 env.now() + 3600s),
                             // Counterparty must sign the outer transaction
                             counterparty(borrower.id()),
-                            json(sfCounterpartySignature, Json::objectValue),
                             sig(none),
                             fee(none),
                             seq(none)),
@@ -2737,7 +2711,6 @@ class Batch_test : public beast::unit_test::suite
                                 asset(1000).value(),
                                 env.now() + 3600s),
                             counterparty(borrower.id()),
-                            json(sfCounterpartySignature, Json::objectValue),
                             sig(none),
                             fee(none),
                             seq(none)),
@@ -2773,7 +2746,6 @@ class Batch_test : public beast::unit_test::suite
                                 asset(1000).value(),
                                 env.now() + 3600s),
                             counterparty(borrower.id()),
-                            json(sfCounterpartySignature, Json::objectValue),
                             sig(none),
                             fee(none),
                             seq(none)),

src/test/app/Loan_test.cpp

@@ -86,10 +86,11 @@ class Loan_test : public beast::unit_test::suite
             using namespace std::chrono_literals;
             using namespace loan;
 
-            // counter party signature is required on LoanSet
+            // counter party signature is optional on LoanSet. Confirm that by
+            // sending transaction without one.
             auto setTx = env.jt(
                 set(alice, keylet.key, Number(10000), env.now() + 720h),
-                ter(temMALFORMED));
+                ter(temDISABLED));
             env(setTx);
 
             // All loan transactions are disabled.
@@ -1822,23 +1823,35 @@ class Loan_test : public beast::unit_test::suite
         auto const startDate = env.now() + 60s;
 
         // The LoanSet json can be created without a counterparty signature, but
-        // it is malformed.
+        // it will not pass preflight
         auto createJson = env.json(
             set(lender,
                 broker.brokerID,
                 broker.asset(principalRequest).value(),
                 startDate),
             fee(loanSetFee));
+        env(createJson, ter(temBAD_SIGNER));
 
-        env(createJson, ter(temMALFORMED));
-
-        // Adding an empty counterparty signature object is also malformed, but
-        // fails at the RPC level.
+        // Adding an empty counterparty signature object also fails, but
+        // at the RPC level.
         createJson = env.json(
             createJson, json(sfCounterpartySignature, Json::objectValue));
-
         env(createJson, ter(telENV_RPC_FAILED));
 
+        if (auto const jt = env.jt(createJson); BEAST_EXPECT(jt.stx))
+        {
+            Serializer s;
+            jt.stx->add(s);
+            auto const jr = env.rpc("submit", strHex(s.slice()));
+
+            BEAST_EXPECT(jr.isMember(jss::result));
+            auto const jResult = jr[jss::result];
+            BEAST_EXPECT(jResult[jss::error] == "invalidTransaction");
+            BEAST_EXPECT(
+                jResult[jss::error_exception] ==
+                "fails local checks: Transaction has bad signature.");
+        }
+
         // Copy the transaction signature into the counterparty signature.
         Json::Value counterpartyJson{Json::objectValue};
         counterpartyJson[sfTxnSignature] = createJson[sfTxnSignature];
@@ -2137,6 +2150,55 @@ class Loan_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testRPC()
+    {
+        // This will expand as more test cases are added. Some functionality is
+        // tested in other test functions.
+        testcase("RPC");
+
+        using namespace jtx;
+
+        Env env(*this, all);
+
+        Account const alice{"alice"};
+
+        {
+            auto const sk = alice.sk();
+            auto const jr = env.rpc(
+                "sign",
+                encodeBase58Token(TokenType::FamilySeed, sk.data(), sk.size()),
+                R"({ "TransactionType" : "LoanSet", )"
+                R"("Account" : "rHP9W1SByc8on4vfFsBdt5sun2gZTnBhkx", )"
+                R"("Counterparty" : "rHP9W1SByc8on4vfFsBdt5sun2gZTnBhkx", )"
+                R"("LoanBrokerID" : )"
+                R"("EAFD2D37FE12815F00705F1B57173A16F94EE15E02D53AF5B683942B57ED53E9", )"
+                R"("PrincipalRequested" : "100000000", )"
+                R"("StartDate" : "807730340", "PaymentTotal" : 10000, )"
+                R"("PaymentInterval" : 1, "GracePeriod" : 300, )"
+                R"("Flags" : 65536, "Fee" : "24", "Sequence" : 1 })",
+                "offline");
+
+            BEAST_EXPECT(
+                jr.isMember(jss::result) &&
+                jr[jss::result].isMember(jss::tx_json));
+            auto const& tx = jr[jss::result][jss::tx_json];
+            BEAST_EXPECT(!tx.isMember(jss::CounterpartySignature));
+            BEAST_EXPECT(
+                tx.isMember(jss::TxnSignature) &&
+                tx[jss::TxnSignature].asString().length() == 142);
+        }
+    }
+
+    void
+    testBasicMath()
+    {
+        // Test the functions defined in LendingHelpers.h
+        testcase("Basic Math");
+
+        pass();
+    }
+
 public:
     void
     run() override
@@ -2147,6 +2209,9 @@ public:
         testBatchBypassCounterparty();
         testWrongMaxDebtBehavior();
         testLoanPayComputePeriodicPaymentValidRateInvariant();
+
+        testRPC();
+        testBasicMath();
     }
 };
 

src/xrpld/app/tx/detail/Batch.cpp

@@ -310,6 +310,8 @@ Batch::preflight(PreflightContext const& ctx)
         if (auto const ret = checkSignatureFields(stx, hash))
             return ret;
 
+        // Note that the CounterpartySignature is optional, and should not be
+        // included, but if it is, ensure it doesn't contain a signature.
         if (stx.isFieldPresent(sfCounterpartySignature))
         {
             auto const counterpartySignature =

src/xrpld/app/tx/detail/LoanSet.cpp

@@ -73,11 +73,25 @@ LoanSet::preflight(PreflightContext const& ctx)
         return temBAD_SIGNER;
     }
 
-    auto const counterPartySig = ctx.tx.getFieldObject(sfCounterpartySignature);
+    // These extra hoops are because STObjects cannot be Proxy'd from STObject.
+    auto const counterPartySig = [&tx]() -> std::optional<STObject const> {
+        if (tx.isFieldPresent(sfCounterpartySignature))
+            return tx.getFieldObject(sfCounterpartySignature);
+        return std::nullopt;
+    }();
+    if (!tx.isFlag(tfInnerBatchTxn) && !counterPartySig)
+    {
+        JLOG(ctx.j.warn())
+            << "LoanSet transaction must have a CounterpartySignature.";
+        return temBAD_SIGNER;
+    }
 
-    if (auto const ret =
-            ripple::detail::preflightCheckSigningKey(counterPartySig, ctx.j))
-        return ret;
+    if (counterPartySig)
+    {
+        if (auto const ret = ripple::detail::preflightCheckSigningKey(
+                *counterPartySig, ctx.j))
+            return ret;
+    }
 
     if (auto const data = tx[~sfData]; data && !data->empty() &&
         !validDataLength(tx[~sfData], maxDataPayloadLength))
@@ -109,9 +123,12 @@ LoanSet::preflight(PreflightContext const& ctx)
         return temINVALID;
 
     // Copied from preflight2
-    if (auto const ret = ripple::detail::preflightCheckSimulateKeys(
-            ctx.flags, counterPartySig, ctx.j))
-        return *ret;
+    if (counterPartySig)
+    {
+        if (auto const ret = ripple::detail::preflightCheckSimulateKeys(
+                ctx.flags, *counterPartySig, ctx.j))
+            return *ret;
+    }
 
     return tesSUCCESS;
 }
@@ -136,7 +153,10 @@ LoanSet::checkSign(PreclaimContext const& ctx)
     }();
     if (!counterSigner)
         return temBAD_SIGNER;
-    // Counterparty signature is required
+
+    // Counterparty signature is optional. Presence is checked in preflight.
+    if (!ctx.tx.isFieldPresent(sfCounterpartySignature))
+        return tesSUCCESS;
     auto const counterSig = ctx.tx.getFieldObject(sfCounterpartySignature);
     return Transactor::checkSign(ctx, *counterSigner, counterSig);
 }
@@ -150,6 +170,8 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx)
     // CounterpartySignature, whether a single signature or a multisignature
     XRPAmount const baseFee = view.fees().base;
 
+    // Counterparty signature is optional, but getFieldObject will return an
+    // empty object if it's not present.
     auto const counterSig = tx.getFieldObject(sfCounterpartySignature);
     // Each signer adds one more baseFee to the minimum required fee
     // for the transaction. Note that unlike the base class, the single signer