Add more LoanSet unit tests, added LoanBroker LoanSequence field

- LoanSequence will prevent loan key collisions

include/xrpl/protocol/STTx.h

@@ -133,15 +133,15 @@ public:
         @param requireCanonicalSig If `true`, check that the signature is fully
             canonical. If `false`, only check that the signature is valid.
         @param rules The current ledger rules.
-        @param sigObject The object that contains the signature fields. Will
+        @param pSig Pointer to object that contains the signature fields, if not
-       most often be *this.
+            using "this". Will most often be null
         @return `true` if valid signature. If invalid, the error message string.
     */
     Expected<void, std::string>
     checkSign(
         RequireFullyCanonicalSig requireCanonicalSig,
         Rules const& rules,
-        STObject const& sigObject) const;
+        STObject const* pSig) const;
 
     /** Check the signature.
         @param requireCanonicalSig If `true`, check that the signature is fully
@@ -172,13 +172,13 @@ private:
     Expected<void, std::string>
     checkSingleSign(
         RequireFullyCanonicalSig requireCanonicalSig,
-        STObject const& sigObject) const;
+        STObject const* pSig) const;
 
     Expected<void, std::string>
     checkMultiSign(
         RequireFullyCanonicalSig requireCanonicalSig,
         Rules const& rules,
-        STObject const& sigObject) const;
+        STObject const* pSig) const;
 
     STBase*
     copy(std::size_t n, void* buf) const override;

include/xrpl/protocol/detail/ledger_entries.macro

@@ -501,6 +501,7 @@ LEDGER_ENTRY(ltLOAN_BROKER, 0x0088, LoanBroker, loan_broker, ({
     {sfVaultID,              soeREQUIRED},
     {sfAccount,              soeREQUIRED},
     {sfOwner,                soeREQUIRED},
+    {sfLoanSequence,         soeREQUIRED},
     {sfData,                 soeDEFAULT},
     {sfManagementFeeRate,    soeDEFAULT},
     {sfOwnerCount,           soeDEFAULT},

include/xrpl/protocol/detail/sfields.macro

@@ -125,15 +125,16 @@ TYPED_SFIELD(sfPreviousPaymentDate,      UINT32,    55)
 TYPED_SFIELD(sfNextPaymentDueDate,       UINT32,    56)
 TYPED_SFIELD(sfPaymentRemaining,         UINT32,    57)
 TYPED_SFIELD(sfPaymentTotal,             UINT32,    58)
-
+TYPED_SFIELD(sfLoanSequence,             UINT32,    59)
 
 // 32-bit integers represented as 1/10 basis points (bips)
 TYPED_SFIELD(sfCoverRateMinimum,         TENTHBIPS32, 1)
 TYPED_SFIELD(sfCoverRateLiquidation,     TENTHBIPS32, 2)
-TYPED_SFIELD(sfInterestRate,             TENTHBIPS32, 3)
+TYPED_SFIELD(sfOverpaymentFee,           TENTHBIPS32, 3)
-TYPED_SFIELD(sfLateInterestRate,         TENTHBIPS32, 4)
+TYPED_SFIELD(sfInterestRate,             TENTHBIPS32, 4)
-TYPED_SFIELD(sfCloseInterestRate,        TENTHBIPS32, 5)
+TYPED_SFIELD(sfLateInterestRate,         TENTHBIPS32, 5)
-TYPED_SFIELD(sfOverpaymentInterestRate,  TENTHBIPS32, 6)
+TYPED_SFIELD(sfCloseInterestRate,        TENTHBIPS32, 6)
+TYPED_SFIELD(sfOverpaymentInterestRate,  TENTHBIPS32, 7)
 
 // 64-bit integers (common)
 TYPED_SFIELD(sfIndexNext,                UINT64,     1)
@@ -236,9 +237,8 @@ TYPED_SFIELD(sfLoanOriginationFee,       NUMBER,     9)
 TYPED_SFIELD(sfLoanServiceFee,           NUMBER,     10)
 TYPED_SFIELD(sfLatePaymentFee,           NUMBER,     11)
 TYPED_SFIELD(sfClosePaymentFee,          NUMBER,     12)
-TYPED_SFIELD(sfOverpaymentFee,           NUMBER,     13)
+TYPED_SFIELD(sfPrincipalOutstanding,     NUMBER,     13)
-TYPED_SFIELD(sfPrincipalOutstanding,     NUMBER,     14)
+TYPED_SFIELD(sfPrincipalRequested,       NUMBER,     14)
-TYPED_SFIELD(sfPrincipalRequested,       NUMBER,     15)
 
 // currency amount (common)
 TYPED_SFIELD(sfAmount,                   AMOUNT,     1)

src/libxrpl/protocol/InnerObjectFormats.cpp

@@ -158,7 +158,7 @@ InnerObjectFormats::InnerObjectFormats()
     add(sfCounterpartySignature.jsonName,
         sfCounterpartySignature.getCode(),
         {
-            {sfSigningPubKey, soeREQUIRED},
+            {sfSigningPubKey, soeOPTIONAL},
             {sfTxnSignature, soeOPTIONAL},
             {sfSigners, soeOPTIONAL},
         });

src/libxrpl/protocol/STTx.cpp

@@ -245,17 +245,19 @@ Expected<void, std::string>
 STTx::checkSign(
     RequireFullyCanonicalSig requireCanonicalSig,
     Rules const& rules,
-    STObject const& sigObject) const
+    STObject const* pSig) const
 {
     try
     {
         // Determine whether we're single- or multi-signing by looking
         // at the SigningPubKey.  If it's empty we must be
         // multi-signing.  Otherwise we're single-signing.
+        STObject const& sigObject{pSig ? *pSig : *this};
+
         Blob const& signingPubKey = sigObject.getFieldVL(sfSigningPubKey);
         return signingPubKey.empty()
-            ? checkMultiSign(requireCanonicalSig, rules, sigObject)
+            ? checkMultiSign(requireCanonicalSig, rules, pSig)
-            : checkSingleSign(requireCanonicalSig, sigObject);
+            : checkSingleSign(requireCanonicalSig, pSig);
     }
     catch (std::exception const&)
     {
@@ -268,13 +270,13 @@ STTx::checkSign(
     RequireFullyCanonicalSig requireCanonicalSig,
     Rules const& rules) const
 {
-    if (auto const ret = checkSign(requireCanonicalSig, rules, *this); !ret)
+    if (auto const ret = checkSign(requireCanonicalSig, rules, nullptr); !ret)
         return ret;
 
     if (isFieldPresent(sfCounterpartySignature))
     {
         auto const counterSig = getFieldObject(sfCounterpartySignature);
-        if (auto const ret = checkSign(requireCanonicalSig, rules, counterSig);
+        if (auto const ret = checkSign(requireCanonicalSig, rules, &counterSig);
             !ret)
             return Unexpected("Counterparty: " + ret.error());
     }
@@ -363,8 +365,10 @@ STTx::getMetaSQL(
 Expected<void, std::string>
 STTx::checkSingleSign(
     RequireFullyCanonicalSig requireCanonicalSig,
-    STObject const& sigObject) const
+    STObject const* pSig) const
 {
+    STObject const& sigObject{pSig ? *pSig : *this};
+
     // We don't allow both a non-empty sfSigningPubKey and an sfSigners.
     // That would allow the transaction to be signed two ways.  So if both
     // fields are present the signature is invalid.
@@ -406,8 +410,10 @@ Expected<void, std::string>
 STTx::checkMultiSign(
     RequireFullyCanonicalSig requireCanonicalSig,
     Rules const& rules,
-    STObject const& sigObject) const
+    STObject const* pSig) const
 {
+    STObject const& sigObject{pSig ? *pSig : *this};
+
     // Make sure the MultiSigners are present.  Otherwise they are not
     // attempting multi-signing and we just have a bad SigningPubKey.
     if (!sigObject.isFieldPresent(sfSigners))
@@ -444,8 +450,8 @@ STTx::checkMultiSign(
     {
         auto const accountID = signer.getAccountID(sfAccount);
 
-        // The account owner may not multisign for themselves.
+        // The account owner may not usually multisign for themselves.
-        if (accountID == txnAccountID)
+        if (!pSig && accountID == txnAccountID)
             return Unexpected("Invalid multisigner.");
 
         // No duplicate signers allowed.

src/test/app/LoanBroker_test.cpp

@@ -148,6 +148,7 @@ class LoanBroker_test : public beast::unit_test::suite
             BEAST_EXPECT(broker->at(sfFlags) == 0);
             BEAST_EXPECT(broker->at(sfSequence) == env.seq(alice) - 1);
             BEAST_EXPECT(broker->at(sfOwnerCount) == 0);
+            BEAST_EXPECT(broker->at(sfLoanSequence) == 1);
             BEAST_EXPECT(broker->at(sfDebtTotal) == 0);
             BEAST_EXPECT(broker->at(sfCoverAvailable) == 0);
             if (checkBroker)

src/test/app/Loan_test.cpp

@@ -23,6 +23,8 @@
 #include <test/jtx/mpt.h>
 #include <test/jtx/vault.h>
 
+#include <xrpld/app/tx/detail/LoanSet.h>
+
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/json/json_forwards.h>
@@ -354,34 +356,40 @@ class Loan_test : public beast::unit_test::suite
         // MPT. That'll require three corresponding SAVs.
         Env env(*this, all);
 
-        Account issuer{"issuer"};
+        Account const issuer{"issuer"};
-        // For simplicity, alice will be the sole actor for the vault & brokers.
+        // For simplicity, lender will be the sole actor for the vault &
-        Account alice{"alice"};
+        // brokers.
+        Account const lender{"lender"};
+        // Borrower only wants to borrow
+        Account const borrower{"borrower"};
         // Evan will attempt to be naughty
-        Account evan{"evan"};
+        Account const evan{"evan"};
+        // Do not fund alice
+        Account const alice{"alice"};
         Vault vault{env};
 
         // Fund the accounts and trust lines with the same amount so that tests
         // can use the same values regardless of the asset.
-        env.fund(XRP(100'000), issuer, noripple(alice, evan));
+        env.fund(XRP(100'000), issuer, noripple(lender, borrower, evan));
         env.close();
 
         // Create assets
         PrettyAsset const xrpAsset{xrpIssue(), 1'000'000};
-        PrettyAsset const iouAsset = issuer["IOU"];
+        std::string const iouCurrency{"IOU"};
-        env(trust(alice, iouAsset(1'000'000)));
+        PrettyAsset const iouAsset = issuer[iouCurrency];
+        env(trust(lender, iouAsset(1'000'000)));
         env(trust(evan, iouAsset(1'000'000)));
         env(pay(issuer, evan, iouAsset(100'000)));
-        env(pay(issuer, alice, iouAsset(100'000)));
+        env(pay(issuer, lender, iouAsset(100'000)));
         env.close();
 
         MPTTester mptt{env, issuer, mptInitNoFund};
         mptt.create(
             {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock});
         PrettyAsset const mptAsset = mptt.issuanceID();
-        mptt.authorize({.account = alice});
+        mptt.authorize({.account = lender});
         mptt.authorize({.account = evan});
-        env(pay(issuer, alice, mptAsset(100'000)));
+        env(pay(issuer, lender, mptAsset(100'000)));
         env(pay(issuer, evan, mptAsset(100'000)));
         env.close();
 
@@ -392,114 +400,259 @@ class Loan_test : public beast::unit_test::suite
         for (auto const& asset : assets)
         {
             auto [tx, vaultKeylet] =
-                vault.create({.owner = alice, .asset = asset});
+                vault.create({.owner = lender, .asset = asset});
             env(tx);
             env.close();
             BEAST_EXPECT(env.le(vaultKeylet));
 
             env(vault.deposit(
-                {.depositor = alice,
+                {.depositor = lender,
                  .id = vaultKeylet.key,
-                 .amount = asset(5000)}));
+                 .amount = asset(50'000)}));
             env.close();
 
-            auto const keylet = keylet::loanbroker(alice.id(), env.seq(alice));
+            auto const keylet =
+                keylet::loanbroker(lender.id(), env.seq(lender));
             auto const testData = "spam spam spam spam";
 
             using namespace loanBroker;
-            env(set(alice, vaultKeylet.key),
+            env(set(lender, vaultKeylet.key),
                 fee(increment),
                 data(testData),
-                managementFeeRate(TenthBips16(123)),
+                managementFeeRate(TenthBips16(100)),
-                debtMaximum(Number(9)),
+                debtMaximum(Number(25'000)),
-                coverRateMinimum(TenthBips32(100)),
+                coverRateMinimum(TenthBips32(percentageToTenthBips(10))),
-                coverRateLiquidation(TenthBips32(200)));
+                coverRateLiquidation(TenthBips32(percentageToTenthBips(25))));
+
+            env(coverDeposit(lender, keylet.key, asset(1000)));
 
             brokers.emplace_back(asset, keylet.key);
         }
 
-#if 0
+        // Create and update Loans
-        // Create and update Loan Brokers
+        for (auto const& broker : brokers)
-        for (auto const& vault : vaults)
         {
-            using namespace loanBroker;
+            using namespace loan;
+            using namespace std::chrono_literals;
 
-            auto badKeylet = keylet::vault(alice.id(), env.seq(alice));
+            auto const principalRequested = Number(1000);
+            auto const startDate = env.now() + 3600s;
+            auto const loanSetFee = fee(env.current()->fees().base * 2);
+
+            auto badKeylet = keylet::vault(lender.id(), env.seq(lender));
             // Try some failure cases
-            // insufficient fee
+            // insufficient fee - single sign
-            env(set(evan, vault.vaultID), ter(telINSUF_FEE_P));
+            env(set(borrower, broker.brokerID, principalRequested, startDate),
-            // not the vault owner
+                sig(sfCounterpartySignature, lender),
-            env(set(evan, vault.vaultID),
+                ter(telINSUF_FEE_P));
-                fee(increment),
+            // insufficient fee - multisign
+            env(set(borrower, broker.brokerID, principalRequested, startDate),
+                counterparty(lender),
+                msig(evan, lender),
+                msig(sfCounterpartySignature, evan, borrower),
+                fee(env.current()->fees().base * 5 - 1),
+                ter(telINSUF_FEE_P));
+            // multisign sufficient fee, but no signers set up
+            env(set(borrower, broker.brokerID, principalRequested, startDate),
+                counterparty(lender),
+                msig(evan, lender),
+                msig(sfCounterpartySignature, evan, borrower),
+                fee(env.current()->fees().base * 5),
+                ter(tefNOT_MULTI_SIGNING));
+            // not the broker owner, no counterparty, not signed by broker owner
+            env(set(borrower, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, evan),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // not the broker owner, counterparty is borrower
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                counterparty(borrower),
+                sig(sfCounterpartySignature, borrower),
+                loanSetFee,
                 ter(tecNO_PERMISSION));
-            // not a vault
+            // not a LoanBroker object, no counterparty
-            env(set(alice, badKeylet.key), fee(increment), ter(tecNO_ENTRY));
+            env(set(lender, badKeylet.key, principalRequested, startDate),
+                sig(sfCounterpartySignature, evan),
+                loanSetFee,
+                ter(temBAD_SIGNER));
+            // not a LoanBroker object, counterparty is valid
+            env(set(lender, badKeylet.key, principalRequested, startDate),
+                counterparty(borrower),
+                sig(sfCounterpartySignature, borrower),
+                loanSetFee,
+                ter(tecNO_ENTRY));
+            // borrower doesn't exist
+            env(set(lender, broker.brokerID, principalRequested, startDate),
+                counterparty(alice),
+                sig(sfCounterpartySignature, alice),
+                loanSetFee,
+                ter(terNO_ACCOUNT));
             // flags are checked first
-            env(set(evan, vault.vaultID, ~tfUniversal),
+            env(set(evan,
-                fee(increment),
+                    broker.brokerID,
+                    principalRequested,
+                    startDate,
+                    tfLoanSetMask),
+                sig(sfCounterpartySignature, lender),
+                loanSetFee,
                 ter(temINVALID_FLAG));
+
+            // Frozen trust line / locked MPT issuance
+            // XRP can not be frozen
+            if (!broker.asset.raw().native())
+            {
+                auto const brokerSLE =
+                    env.le(keylet::loanbroker(broker.brokerID));
+                if (!BEAST_EXPECT(brokerSLE))
+                    return;
+                auto const brokerPseudo = brokerSLE->at(sfAccount);
+                auto const pseudoAcct =
+                    Account("Broker pseudo-account", brokerPseudo);
+
+                std::function<void(Account const& holder)> freeze;
+                std::function<void(Account const& holder)> unfreeze;
+                // Freeze / lock the asset
+                if (broker.asset.raw().holds<Issue>())
+                {
+                    freeze = [&](Account const& holder) {
+                        env(trust(issuer, holder[iouCurrency](0), tfSetFreeze));
+                    };
+                    unfreeze = [&](Account const& holder) {
+                        env(trust(
+                            issuer, holder[iouCurrency](0), tfClearFreeze));
+                    };
+                }
+                else
+                {
+                    freeze = [&](Account const& holder) {
+                        mptt.set(
+                            {.account = issuer,
+                             .holder = holder,
+                             .flags = tfMPTLock});
+                    };
+                    unfreeze = [&](Account const& holder) {
+                        mptt.set(
+                            {.account = issuer,
+                             .holder = holder,
+                             .flags = tfMPTUnlock});
+                    };
+                }
+            }
+
             // field length validation
             // sfData: good length, bad account
-            env(set(evan, vault.vaultID),
+            env(set(evan, broker.brokerID, principalRequested, startDate),
-                fee(increment),
+                sig(sfCounterpartySignature, borrower),
                 data(std::string(maxDataPayloadLength, 'X')),
-                ter(tecNO_PERMISSION));
+                loanSetFee,
+                ter(tefBAD_AUTH));
             // sfData: too long
-            env(set(evan, vault.vaultID),
+            env(set(evan, broker.brokerID, principalRequested, startDate),
-                fee(increment),
+                sig(sfCounterpartySignature, lender),
                 data(std::string(maxDataPayloadLength + 1, 'Y')),
-                ter(temINVALID));
+                loanSetFee,
-            // sfManagementFeeRate: good value, bad account
-            env(set(evan, vault.vaultID),
-                managementFeeRate(maxManagementFeeRate),
-                fee(increment),
-                ter(tecNO_PERMISSION));
-            // sfManagementFeeRate: too big
-            env(set(evan, vault.vaultID),
-                managementFeeRate(maxManagementFeeRate + TenthBips16(10)),
-                fee(increment),
-                ter(temINVALID));
-            // sfCoverRateMinimum: good value, bad account
-            env(set(evan, vault.vaultID),
-                coverRateMinimum(maxCoverRate),
-                fee(increment),
-                ter(tecNO_PERMISSION));
-            // sfCoverRateMinimum: too big
-            env(set(evan, vault.vaultID),
-                coverRateMinimum(maxCoverRate + 1),
-                fee(increment),
-                ter(temINVALID));
-            // sfCoverRateLiquidation: good value, bad account
-            env(set(evan, vault.vaultID),
-                coverRateLiquidation(maxCoverRate),
-                fee(increment),
-                ter(tecNO_PERMISSION));
-            // sfCoverRateLiquidation: too big
-            env(set(evan, vault.vaultID),
-                coverRateLiquidation(maxCoverRate + 1),
-                fee(increment),
-                ter(temINVALID));
-            // sfDebtMaximum: good value, bad account
-            env(set(evan, vault.vaultID),
-                debtMaximum(Number(0)),
-                fee(increment),
-                ter(tecNO_PERMISSION));
-            // sfDebtMaximum: overflow
-            env(set(evan, vault.vaultID),
-                debtMaximum(Number(1, 100)),
-                fee(increment),
-                ter(temINVALID));
-            // sfDebtMaximum: negative
-            env(set(evan, vault.vaultID),
-                debtMaximum(Number(-1)),
-                fee(increment),
                 ter(temINVALID));
 
+            // field range validation
+            // sfOverpaymentFee: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                overpaymentFee(maxOverpaymentFee),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfOverpaymentFee: too big
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                overpaymentFee(maxOverpaymentFee + 1),
+                loanSetFee,
+                ter(temINVALID));
+
+            // sfLateInterestRate: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                lateInterestRate(maxLateInterestRate),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfLateInterestRate: too big
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                lateInterestRate(maxLateInterestRate + 1),
+                loanSetFee,
+                ter(temINVALID));
+
+            // sfCloseInterestRate: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                closeInterestRate(maxCloseInterestRate),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfCloseInterestRate: too big
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                closeInterestRate(maxCloseInterestRate + 1),
+                loanSetFee,
+                ter(temINVALID));
+
+            // sfOverpaymentInterestRate: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                overpaymentInterestRate(maxOverpaymentInterestRate),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfOverpaymentInterestRate: too big
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                overpaymentInterestRate(maxOverpaymentInterestRate + 1),
+                loanSetFee,
+                ter(temINVALID));
+
+            // sfPaymentTotal: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                paymentTotal(LoanSet::minPaymentTotal),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfPaymentTotal: too small (there is no max)
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                paymentTotal(LoanSet::minPaymentTotal - 1),
+                loanSetFee,
+                ter(temINVALID));
+
+            // sfPaymentInterval: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                paymentInterval(LoanSet::minPaymentInterval),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfPaymentInterval: too small (there is no max)
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                paymentInterval(LoanSet::minPaymentInterval - 1),
+                loanSetFee,
+                ter(temINVALID));
+
+            // sfGracePeriod: good value, bad account
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, borrower),
+                paymentInterval(LoanSet::minPaymentInterval * 2),
+                gracePeriod(LoanSet::minPaymentInterval * 2),
+                loanSetFee,
+                ter(tefBAD_AUTH));
+            // sfGracePeriod: larger than paymentInterval
+            env(set(evan, broker.brokerID, principalRequested, startDate),
+                sig(sfCounterpartySignature, lender),
+                paymentInterval(LoanSet::minPaymentInterval * 2),
+                gracePeriod(LoanSet::minPaymentInterval * 3),
+                loanSetFee,
+                ter(temINVALID));
+
+#if 0
             std::string testData;
             lifecycle(
                 "default fields",
                 env,
-                alice,
+                lender,
                 evan,
                 vault,
                 // No modifications
@@ -521,33 +674,33 @@ class Loan_test : public beast::unit_test::suite
 
                     // Update the fields
                     auto const nextKeylet =
-                        keylet::loanbroker(alice.id(), env.seq(alice));
+                        keylet::loanbroker(lender.id(), env.seq(lender));
 
                     // fields that can't be changed
                     // LoanBrokerID
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(nextKeylet.key),
                         ter(tecNO_ENTRY));
                     // VaultID
-                    env(set(alice, nextKeylet.key),
+                    env(set(lender, nextKeylet.key),
                         loanBrokerID(broker->key()),
                         ter(tecNO_PERMISSION));
                     // Owner
-                    env(set(evan, vault.vaultID),
+                    env(set(evan, broker.brokerID),
                         loanBrokerID(broker->key()),
                         ter(tecNO_PERMISSION));
                     // ManagementFeeRate
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         managementFeeRate(maxManagementFeeRate),
                         ter(temINVALID));
                     // CoverRateMinimum
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         coverRateMinimum(maxManagementFeeRate),
                         ter(temINVALID));
                     // CoverRateLiquidation
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         coverRateLiquidation(maxManagementFeeRate),
                         ter(temINVALID));
@@ -555,18 +708,18 @@ class Loan_test : public beast::unit_test::suite
                     // fields that can be changed
                     testData = "Test Data 1234";
                     // Bad data: too long
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         data(std::string(maxDataPayloadLength + 1, 'W')),
                         ter(temINVALID));
 
                     // Bad debt maximum
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         debtMaximum(Number(-175, -1)),
                         ter(temINVALID));
                     // Data & Debt maximum
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         data(testData),
                         debtMaximum(Number(175, -1)));
@@ -580,7 +733,7 @@ class Loan_test : public beast::unit_test::suite
             lifecycle(
                 "non-default fields",
                 env,
-                alice,
+                lender,
                 evan,
                 vault,
                 [&](jtx::JTx const& jv) {
@@ -605,7 +758,7 @@ class Loan_test : public beast::unit_test::suite
                 },
                 [&](SLE::const_ref broker) {
                     // Reset Data & Debt maximum to default values
-                    env(set(alice, vault.vaultID),
+                    env(set(lender, broker.brokerID),
                         loanBrokerID(broker->key()),
                         data(""),
                         debtMaximum(Number(0)));
@@ -615,8 +768,8 @@ class Loan_test : public beast::unit_test::suite
                     BEAST_EXPECT(!broker->isFieldPresent(sfData));
                     BEAST_EXPECT(!broker->isFieldPresent(sfDebtMaximum));
                 });
-        }
 #endif
+        }
     }
 
 public:

src/test/jtx/TestHelpers.h

@@ -145,6 +145,29 @@ public:
     }
 };
 
+struct accountIDField : public JTxField<SF_ACCOUNT, AccountID, std::string>
+{
+    using SF = SF_ACCOUNT;
+    using SV = AccountID;
+    using OV = std::string;
+    using base = JTxField<SF, SV, OV>;
+
+protected:
+    using base::value_;
+
+public:
+    explicit accountIDField(SF const& sfield, SV const& value)
+        : JTxField(sfield, value)
+    {
+    }
+
+    OV
+    value() const override
+    {
+        return toBase58(value_);
+    }
+};
+
 struct blobField : public JTxField<SF_VL, std::string>
 {
     using SF = SF_VL;
@@ -258,6 +281,10 @@ using valueUnitWrapper =
 template <class SField, class StoredValue = typename SField::type::value_type>
 using simpleField = JTxFieldWrapper<JTxField<SField, StoredValue>>;
 
+/** General field definitions, or fields used in multiple transaction namespaces
+ */
+auto const data = JTxFieldWrapper<blobField>(sfData);
+
 // TODO We only need this long "requires" clause as polyfill, for C++20
 // implementations which are missing <ranges> header. Replace with
 // `std::ranges::range<Input>`, and accordingly use std::ranges::begin/end
@@ -659,8 +686,6 @@ coverWithdraw(
 
 auto const loanBrokerID = JTxFieldWrapper<uint256Field>(sfLoanBrokerID);
 
-auto const data = JTxFieldWrapper<blobField>(sfData);
-
 auto const managementFeeRate =
     valueUnitWrapper<SF_TENTHBIPS16>(sfManagementFeeRate);
 
@@ -685,7 +710,37 @@ set(AccountID const& account,
     NetClock::time_point const& startDate,
     uint32_t flags = 0);
 
-}
+auto const counterparty = JTxFieldWrapper<accountIDField>(sfCounterparty);
+
+// For `CounterPartySignature`, use `sig(sfCounterpartySignature, ...)`
+
+auto const loanOriginationFee = simpleField<SF_NUMBER>(sfLoanOriginationFee);
+
+auto const loanServiceFee = simpleField<SF_NUMBER>(sfLoanServiceFee);
+
+auto const latePaymentFee = simpleField<SF_NUMBER>(sfLatePaymentFee);
+
+auto const closePaymentFee = simpleField<SF_NUMBER>(sfClosePaymentFee);
+
+auto const overpaymentFee = valueUnitWrapper<SF_TENTHBIPS32>(sfOverpaymentFee);
+
+auto const interestRate = valueUnitWrapper<SF_TENTHBIPS32>(sfInterestRate);
+
+auto const lateInterestRate =
+    valueUnitWrapper<SF_TENTHBIPS32>(sfLateInterestRate);
+
+auto const closeInterestRate =
+    valueUnitWrapper<SF_TENTHBIPS32>(sfCloseInterestRate);
+
+auto const overpaymentInterestRate =
+    valueUnitWrapper<SF_TENTHBIPS32>(sfOverpaymentInterestRate);
+
+auto const paymentTotal = simpleField<SF_UINT32>(sfPaymentTotal);
+
+auto const paymentInterval = simpleField<SF_UINT32>(sfPaymentInterval);
+
+auto const gracePeriod = simpleField<SF_UINT32>(sfGracePeriod);
+}  // namespace loan
 
 }  // namespace jtx
 }  // namespace test

src/test/jtx/impl/multisign.cpp

@@ -85,7 +85,11 @@ msig::operator()(Env& env, JTx& jt) const
         // Where to put the signature. Supports sfCounterPartySignature.
         auto& sigObject = subField ? jtx[*subField] : jtx.jv;
 
-        sigObject[sfSigningPubKey] = "";
+        // The signing pub key is only required at the top level.
+        if (!subField)
+            sigObject[sfSigningPubKey] = "";
+        else if (sigObject.isNull())
+            sigObject = Json::Value(Json::objectValue);
         std::optional<STObject> st;
         try
         {

src/test/ledger/Invariants_test.cpp

@@ -1642,13 +1642,6 @@ class Invariants_test : public beast::unit_test::suite
                 return std::make_pair(slePseudo, sleDir);
             };
 
-            // JLOG(j.fatal()) << "Invariant failed:";
-            // JLOG(j.fatal()) << "Invariant failed: ";
-            // JLOG(j.fatal())
-            //     << "Invariant failed: ";
-            // JLOG(j.fatal())
-            //     << "Invariant failed: ";
-
             doInvariantCheck(
                 {{"Loan Broker with zero OwnerCount has multiple directory "
                   "pages"}},
@@ -1763,6 +1756,29 @@ class Invariants_test : public beast::unit_test::suite
                 STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}},
                 {tecINVARIANT_FAILED, tefINVARIANT_FAILED},
                 createLoanBroker);
+
+            doInvariantCheck(
+                {{"Loan Broker sequence number decreased"}},
+                [&](Account const& A1, Account const& A2, ApplyContext& ac) {
+                    if (loanBrokerKeylet.type != ltLOAN_BROKER)
+                        return false;
+                    auto sleBroker = ac.view().peek(loanBrokerKeylet);
+                    if (!sleBroker)
+                        return false;
+                    if (!BEAST_EXPECT(sleBroker->at(sfLoanSequence) > 0))
+                        return false;
+                    // Need to touch sleBroker so that it is included in the
+                    // modified entries for the invariant to find
+                    ac.view().update(sleBroker);
+
+                    sleBroker->at(sfLoanSequence) -= 1;
+
+                    return true;
+                },
+                XRPAmount{},
+                STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}},
+                {tecINVARIANT_FAILED, tefINVARIANT_FAILED},
+                createLoanBroker);
         }
     }
 

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

@@ -1846,7 +1846,7 @@ ValidLoanBroker::visitEntry(
 {
     if (after && after->getType() == ltLOAN_BROKER)
     {
-        brokers_.emplace_back(after);
+        brokers_.emplace_back(before, after);
     }
 }
 
@@ -1904,15 +1904,15 @@ ValidLoanBroker::finalize(
 {
     bool const enforce = view.rules().enabled(featureLendingProtocol);
 
-    for (auto const& broker : brokers_)
+    for (auto const& [before, after] : brokers_)
     {
         // https://github.com/Tapanito/XRPL-Standards/blob/xls-66-lending-protocol/XLS-0066d-lending-protocol/README.md#3123-invariants
         // If `LoanBroker.OwnerCount = 0` the `DirectoryNode` will have at most
         // one node (the root), which will only hold entries for `RippleState`
         // or `MPToken` objects.
-        if (broker->at(sfOwnerCount) == 0)
+        if (after->at(sfOwnerCount) == 0)
         {
-            auto const dir = view.read(keylet::ownerDir(broker->at(sfAccount)));
+            auto const dir = view.read(keylet::ownerDir(after->at(sfAccount)));
             if (dir)
             {
                 if (!goodZeroDirectory(view, dir, j))
@@ -1920,12 +1920,23 @@ ValidLoanBroker::finalize(
                     XRPL_ASSERT(
                         enforce,
                         "ripple::ValidLoanBroker::finalize : Enforcing "
-                        "invariant");
+                        "invariant: directory");
                     if (enforce)
                         return false;
                 }
             }
         }
+        if (before && before->at(sfLoanSequence) > after->at(sfLoanSequence))
+        {
+            JLOG(j.fatal()) << "Invariant failed: Loan Broker sequence number "
+                               "decreased";
+            XRPL_ASSERT(
+                enforce,
+                "ripple::ValidLoanBroker::finalize : Enforcing "
+                "invariant: loan sequence");
+            if (enforce)
+                return false;
+        }
     }
     return true;
 }

src/xrpld/app/tx/detail/InvariantCheck.h

@@ -634,6 +634,7 @@ public:
  */
 class NoModifiedUnmodifiableFields
 {
+    // Pair is <before, after>.
     std::set<std::pair<SLE::const_pointer, SLE::const_pointer>> changedEntries_;
 
 public:
@@ -690,7 +691,9 @@ public:
  */
 class ValidLoanBroker
 {
-    std::vector<SLE::const_pointer> brokers_;
+    // Pair is <before, after>. After is used for most of the checks, except
+    // those that check changed values.
+    std::vector<std::pair<SLE::const_pointer, SLE::const_pointer>> brokers_;
 
     bool
     goodZeroDirectory(

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

@@ -209,6 +209,7 @@ LoanBrokerSet::doApply()
         broker->at(sfVaultID) = vaultID;
         broker->at(sfOwner) = account_;
         broker->at(sfAccount) = pseudoId;
+        broker->at(sfLoanSequence) = 1;
         if (auto const data = tx[~sfData])
             broker->at(sfData) = *data;
         if (auto const rate = tx[~sfManagementFeeRate])

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

@@ -71,6 +71,8 @@ LoanSet::doPreflight(PreflightContext const& ctx)
     if (auto const data = tx[~sfData]; data && !data->empty() &&
         !validDataLength(tx[~sfData], maxDataPayloadLength))
         return temINVALID;
+    if (!validNumericRange(tx[~sfOverpaymentFee], maxOverpaymentFee))
+        return temINVALID;
     if (!validNumericRange(tx[~sfLateInterestRate], maxLateInterestRate))
         return temINVALID;
     if (!validNumericRange(tx[~sfCloseInterestRate], maxCloseInterestRate))
@@ -84,7 +86,7 @@ LoanSet::doPreflight(PreflightContext const& ctx)
         return temINVALID;
     if (auto const paymentInterval =
             tx[~sfPaymentInterval].value_or(LoanSet::defaultPaymentInterval);
-        paymentInterval < LoanSet::defaultPaymentInterval)
+        paymentInterval < LoanSet::minPaymentInterval)
         return temINVALID;
     else if (auto const gracePeriod =
                  tx[~sfGracePeriod].value_or(LoanSet::defaultGracePeriod);
@@ -160,18 +162,13 @@ LoanSet::preclaim(PreclaimContext const& ctx)
     auto const brokerSle = ctx.view.read(keylet::loanbroker(brokerID));
     if (!brokerSle)
     {
+        // This can only be hit if there's a counterparty specified, otherwise
+        // it'll fail in the signature check
         JLOG(ctx.j.warn()) << "LoanBroker does not exist.";
         return tecNO_ENTRY;
     }
     auto const brokerOwner = brokerSle->at(sfOwner);
-    if (!tx.isFieldPresent(sfCounterparty) && account != brokerOwner)
-    {
-        JLOG(ctx.j.warn())
-            << "Counterparty is not the owner of the LoanBroker.";
-        return tecNO_PERMISSION;
-    }
     auto const counterparty = tx[~sfCounterparty].value_or(brokerOwner);
-    auto const borrower = counterparty == brokerOwner ? account : counterparty;
     if (account != brokerOwner && counterparty != brokerOwner)
     {
         JLOG(ctx.j.warn()) << "Neither Account nor Counterparty are the owner "
@@ -179,11 +176,14 @@ LoanSet::preclaim(PreclaimContext const& ctx)
         return tecNO_PERMISSION;
     }
 
+    auto const borrower = counterparty == brokerOwner ? account : counterparty;
     if (auto const borrowerSle = ctx.view.read(keylet::account(borrower));
         !borrowerSle)
     {
+        // It may not be possible to hit this case, because it'll fail the
+        // signature check with terNO_ACCOUNT.
         JLOG(ctx.j.warn()) << "Borrower does not exist.";
-        return tecNO_ENTRY;
+        return terNO_ACCOUNT;
     }
 
     auto const brokerPseudo = brokerSle->at(sfAccount);
@@ -332,10 +332,11 @@ LoanSet::doApply()
     // interest minus the management fee)
     auto const loanInterestToVault = loanInterest -
         tenthBipsOfValue(loanInterest, managementFeeRate.value());
-    auto const loanSequence = tx.getSeqValue();
     auto const startDate = tx[sfStartDate];
     auto const paymentInterval =
         tx[~sfPaymentInterval].value_or(defaultPaymentInterval);
+    auto const loanSequence = brokerSle->at(sfLoanSequence).value();
+    brokerSle->at(sfLoanSequence) += 1;
 
     // Create the loan
     auto loan =

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

@@ -54,10 +54,17 @@ public:
     TER
     doApply() override;
 
-private:
+public:
+    static std::uint32_t constexpr minPaymentTotal = 1;
     static std::uint32_t constexpr defaultPaymentTotal = 1;
+    static_assert(defaultPaymentTotal >= minPaymentTotal);
+
+    static std::uint32_t constexpr minPaymentInterval = 60;
     static std::uint32_t constexpr defaultPaymentInterval = 60;
+    static_assert(defaultPaymentInterval >= minPaymentInterval);
+
     static std::uint32_t constexpr defaultGracePeriod = 60;
+    static_assert(defaultGracePeriod >= minPaymentInterval);
 };
 
 //------------------------------------------------------------------------------