XRPLF review of XLS34 (XRPLF#69)

* add amendment guard for sfTransferRate

* make `sfTransferRate` optional

* split tests into existing / xls34

* make variables `const`

* clang-format

* chage error code

* guard optional `sfTransferRate`

* rename tests

* Guard Optional sfTransferRate

* clang-format

* fixup tests

Author: dangell7

src/ripple/app/tx/impl/Escrow.cpp

@@ -218,7 +218,7 @@ EscrowCreate::doApply()
     auto const balance = STAmount((*sle)[sfBalance]).xrp();
     auto const reserve =
         ctx_.view().fees().accountReserve((*sle)[sfOwnerCount] + 1);
-    bool isIssuer = amount.getIssuer() == account;
+    bool const isIssuer = amount.getIssuer() == account;
 
     if (balance < reserve)
         return tecINSUFFICIENT_RESERVE;
@@ -237,7 +237,7 @@ EscrowCreate::doApply()
         if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
             return temDISABLED;
 
-        TER result = trustTransferAllowed(
+        TER const result = trustTransferAllowed(
             ctx_.view(),
             {account, ctx_.tx[sfDestination]},
             amount.issue(),
@@ -263,7 +263,7 @@ EscrowCreate::doApply()
                 return tecNO_LINE;
 
             {
-                TER result = trustAdjustLockedBalance(
+                TER const result = trustAdjustLockedBalance(
                     ctx_.view(), sleLine, amount, 1, ctx_.journal, DryRun);
 
                 JLOG(ctx_.journal.trace())
@@ -296,20 +296,24 @@ EscrowCreate::doApply()
 
     // Create escrow in ledger.  Note that we we use the value from the
     // sequence or ticket.  For more explanation see comments in SeqProxy.h.
-    auto xferRate = transferRate(view(), amount.getIssuer());
     Keylet const escrowKeylet =
         keylet::escrow(account, seqID(ctx_));
     auto const slep = std::make_shared<SLE>(escrowKeylet);
     (*slep)[sfAmount] = ctx_.tx[sfAmount];
     (*slep)[sfAccount] = account;
-    (*slep)[sfTransferRate] = xferRate.value;
     (*slep)[~sfCondition] = ctx_.tx[~sfCondition];
     (*slep)[~sfSourceTag] = ctx_.tx[~sfSourceTag];
     (*slep)[sfDestination] = ctx_.tx[sfDestination];
     (*slep)[~sfCancelAfter] = ctx_.tx[~sfCancelAfter];
     (*slep)[~sfFinishAfter] = ctx_.tx[~sfFinishAfter];
     (*slep)[~sfDestinationTag] = ctx_.tx[~sfDestinationTag];
 
+    if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
+    {
+        auto const xferRate = transferRate(view(), amount.getIssuer());
+        (*slep)[~sfTransferRate] = xferRate.value;
+    }
+
     ctx_.view().insert(slep);
 
     // Add escrow to sender's owner directory
@@ -346,7 +350,7 @@ EscrowCreate::doApply()
                 return tecNO_LINE;
 
             // do the lock-up for real now
-            TER result = trustAdjustLockedBalance(
+            TER const result = trustAdjustLockedBalance(
                 ctx_.view(), sleLine, amount, 1, ctx_.journal, WetRun);
 
             JLOG(ctx_.journal.trace())
@@ -469,7 +473,7 @@ EscrowFinish::doApply()
 
     AccountID const account = (*slep)[sfAccount];
     auto const sle = ctx_.view().peek(keylet::account(account));
-    auto amount = slep->getFieldAmount(sfAmount);
+    auto const amount = slep->getFieldAmount(sfAmount);
 
     // If a cancel time is present, a finish operation should only succeed prior
     // to that time. fix1571 corrects a logic error in the check that would make
@@ -576,16 +580,19 @@ EscrowFinish::doApply()
         if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
             return temDISABLED;
 
+        if (!slep->isFieldPresent(sfTransferRate))
+            return tecINTERNAL;
+
         Rate lockedRate = ripple::Rate(slep->getFieldU32(sfTransferRate));
-        auto issuerAccID = amount.getIssuer();
+        auto const issuerAccID = amount.getIssuer();
         auto const xferRate = transferRate(view(), issuerAccID);
         // update if issuer rate is less than locked rate
         if (xferRate < lockedRate)
             lockedRate = xferRate;
 
         // perform a dry run of the transfer before we
         // change anything on-ledger
-        TER result = trustTransferLockedBalance(
+        TER const result = trustTransferLockedBalance(
             ctx_.view(),
             account_,  // txn signing account
             sle,       // src account
@@ -632,8 +639,11 @@ EscrowFinish::doApply()
     else
     {
         // compute transfer fee, if any
+        if (!slep->isFieldPresent(sfTransferRate))
+            return tecINTERNAL;
+
         Rate lockedRate = ripple::Rate(slep->getFieldU32(sfTransferRate));
-        auto issuerAccID = amount.getIssuer();
+        auto const issuerAccID = amount.getIssuer();
         auto const xferRate = transferRate(view(), issuerAccID);
         // update if issuer rate is less than locked rate
         if (xferRate < lockedRate)
@@ -642,7 +652,7 @@ EscrowFinish::doApply()
         // all the significant complexity of checking the validity of this
         // transfer and ensuring the lines exist etc is hidden away in this
         // function, all we need to do is call it and return if unsuccessful.
-        TER result = trustTransferLockedBalance(
+        TER const result = trustTransferLockedBalance(
             ctx_.view(),
             account_,  // txn signing account
             sle,       // src account
@@ -733,8 +743,8 @@ EscrowCancel::doApply()
 
     AccountID const account = (*slep)[sfAccount];
     auto const sle = ctx_.view().peek(keylet::account(account));
-    auto amount = slep->getFieldAmount(sfAmount);
-    bool isIssuer = amount.getIssuer() == account;
+    auto const amount = slep->getFieldAmount(sfAmount);
+    bool const isIssuer = amount.getIssuer() == account;
 
     std::shared_ptr<SLE> sleLine;
 
@@ -750,7 +760,7 @@ EscrowCancel::doApply()
                 account, amount.getIssuer(), amount.getCurrency()));
 
             // dry run before we make any changes to ledger
-            if (TER result = trustAdjustLockedBalance(
+            if (TER const result = trustAdjustLockedBalance(
                     ctx_.view(), sleLine, -amount, -1, ctx_.journal, DryRun);
                 result != tesSUCCESS)
                 return result;
@@ -794,7 +804,7 @@ EscrowCancel::doApply()
         if (!isIssuer)
         {
             // unlock previously locked tokens from source line
-            TER result = trustAdjustLockedBalance(
+            TER const result = trustAdjustLockedBalance(
                 ctx_.view(), sleLine, -amount, -1, ctx_.journal, WetRun);
 
             JLOG(ctx_.journal.trace())

src/ripple/app/tx/impl/InvariantCheck.cpp

@@ -361,7 +361,7 @@ NoZeroEscrow::finalize(
     if (bad_ && rv.rules().enabled(featurePaychanAndEscrowForTokens) &&
         txn.isFieldPresent(sfTransactionType))
     {
-        uint16_t tt = txn.getFieldU16(sfTransactionType);
+        uint16_t const tt = txn.getFieldU16(sfTransactionType);
         if (tt == ttESCROW_CANCEL || tt == ttESCROW_FINISH)
             return true;
 

src/ripple/app/tx/impl/PayChan.cpp

@@ -133,7 +133,7 @@ closeChannel(
             keylet::line(src, amount.getIssuer(), amount.getCurrency()));
 
         // dry run
-        TER result =
+        TER const result =
             trustAdjustLockedBalance(view, sleLine, -amount, -1, j, DryRun);
 
         JLOG(j.trace()) << "closeChannel: trustAdjustLockedBalance(dry) result="
@@ -178,7 +178,7 @@ closeChannel(
         (*sle)[sfBalance] = (*sle)[sfBalance] + amount;
     else
     {
-        TER result =
+        TER const result =
             trustAdjustLockedBalance(view, sleLine, -amount, -1, j, WetRun);
 
         JLOG(j.trace()) << "closeChannel: trustAdjustLockedBalance(wet) result="
@@ -257,7 +257,7 @@ PayChanCreate::preclaim(PreclaimContext const& ctx)
         return tecINSUFFICIENT_RESERVE;
 
     auto const dst = ctx.tx[sfDestination];
-    bool isIssuer = amount.getIssuer() == account;
+    bool const isIssuer = amount.getIssuer() == account;
 
     // Check reserve and funds availability
     if (isXRP(amount) && balance < reserve + amount)
@@ -272,7 +272,7 @@ PayChanCreate::preclaim(PreclaimContext const& ctx)
         // check for any possible bars to a channel existing
         // between these accounts for this asset
         {
-            TER result = trustTransferAllowed(
+            TER const result = trustTransferAllowed(
                 ctx.view, {account, dst}, amount.issue(), ctx.j);
             JLOG(ctx.j.trace())
                 << "PayChanCreate::preclaim trustTransferAllowed result="
@@ -287,7 +287,7 @@ PayChanCreate::preclaim(PreclaimContext const& ctx)
         {
             auto sleLine = ctx.view.read(keylet::line(
                 account, amount.getIssuer(), amount.getCurrency()));
-            TER result = trustAdjustLockedBalance(
+            TER const result = trustAdjustLockedBalance(
                 ctx.view, sleLine, amount, 1, ctx.j, DryRun);
             JLOG(ctx.j.trace()) << "PayChanCreate::preclaim "
                                    "trustAdjustLockedBalance(dry) result="
@@ -334,8 +334,7 @@ PayChanCreate::doApply()
     auto const dst = ctx_.tx[sfDestination];
 
     STAmount const amount{ctx_.tx[sfAmount]};
-    bool isIssuer = amount.getIssuer() == account;
-    auto xferRate = transferRate(view(), amount.getIssuer());
+    bool const isIssuer = amount.getIssuer() == account;
 
     // Create PayChan in ledger.
     //
@@ -353,12 +352,17 @@ PayChanCreate::doApply()
     (*slep)[sfAccount] = account;
     (*slep)[sfDestination] = dst;
     (*slep)[sfSettleDelay] = ctx_.tx[sfSettleDelay];
-    (*slep)[sfTransferRate] = xferRate.value;
     (*slep)[sfPublicKey] = ctx_.tx[sfPublicKey];
     (*slep)[~sfCancelAfter] = ctx_.tx[~sfCancelAfter];
     (*slep)[~sfSourceTag] = ctx_.tx[~sfSourceTag];
     (*slep)[~sfDestinationTag] = ctx_.tx[~sfDestinationTag];
 
+    if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
+    {
+        auto xferRate = transferRate(view(), amount.getIssuer());
+        (*slep)[~sfTransferRate] = xferRate.value;
+    }
+
     ctx_.view().insert(slep);
 
     // Add PayChan to owner directory
@@ -398,7 +402,7 @@ PayChanCreate::doApply()
             if (!sleLine)
                 return tecNO_LINE;
 
-            TER result = trustAdjustLockedBalance(
+            TER const result = trustAdjustLockedBalance(
                 ctx_.view(), sleLine, amount, 1, ctx_.journal, WetRun);
 
             JLOG(ctx_.journal.trace())
@@ -470,24 +474,28 @@ PayChanFund::doApply()
     AccountID const src = (*slep)[sfAccount];
     auto const txAccount = ctx_.tx[sfAccount];
     auto const expiration = (*slep)[~sfExpiration];
-    bool isIssuer = amount.getIssuer() == txAccount;
-    // auto const chanFunds = (*slep)[sfAmount];
-
-    // adjust transfer rate
-    Rate lockedRate = ripple::Rate(slep->getFieldU32(sfTransferRate));
-    auto issuerAccID = amount.getIssuer();
-    auto const xferRate = transferRate(view(), issuerAccID);
-    // update if issuer rate less than locked rate
-    if (xferRate < lockedRate)
-        (*slep)[sfTransferRate] = xferRate.value;
-    // throw if issuer rate greater than locked rate
-    if (xferRate > lockedRate)
-        return temBAD_TRANSFER_RATE;
+    bool const isIssuer = amount.getIssuer() == txAccount;
 
     // if this is a Fund operation on an IOU then perform a dry run here
     if (!isXRP(amount) &&
         ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
     {
+        // adjust transfer rate
+        if (!slep->isFieldPresent(sfTransferRate))
+            return tecINTERNAL;
+
+        Rate lockedRate = ripple::Rate(slep->getFieldU32(sfTransferRate));
+        auto const issuerAccID = amount.getIssuer();
+        auto const xferRate = transferRate(view(), issuerAccID);
+
+        // update if issuer rate less than locked rate
+        if (xferRate < lockedRate)
+            (*slep)[~sfTransferRate] = xferRate.value;
+
+        // throw if issuer rate greater than locked rate
+        if (xferRate > lockedRate)
+            return temBAD_TRANSFER_RATE;
+
         // issuer does not need to lock anything
         if (!isIssuer)
         {
@@ -497,7 +505,7 @@ PayChanFund::doApply()
             sleLine = ctx_.view().peek(keylet::line(
                 (*slep)[sfAccount], amount.getIssuer(), amount.getCurrency()));
 
-            TER result = trustAdjustLockedBalance(
+            TER const result = trustAdjustLockedBalance(
                 ctx_.view(), sleLine, amount, 1, ctx_.journal, DryRun);
 
             JLOG(ctx_.journal.trace())
@@ -571,7 +579,7 @@ PayChanFund::doApply()
         // issuer does not need to lock anything
         if (!isIssuer)
         {
-            TER result = trustAdjustLockedBalance(
+            TER const result = trustAdjustLockedBalance(
                 ctx_.view(), sleLine, amount, 1, ctx_.journal, WetRun);
 
             JLOG(ctx_.journal.trace())
@@ -715,11 +723,11 @@ PayChanClaim::doApply()
         }
 
         if (reqBalance > chanFunds)
-            return tecINSUFFICIENT_FUNDS;
+            return tecUNFUNDED_PAYMENT;
 
         if (reqBalance <= chanBalance)
             // nothing requested
-            return tecINSUFFICIENT_FUNDS;
+            return tecUNFUNDED_PAYMENT;
 
         auto sled = ctx_.view().peek(keylet::account(dst));
         if (!sled)
@@ -746,17 +754,6 @@ PayChanClaim::doApply()
             }
         }
 
-        // compute transfer fee, if any
-        Rate lockedRate = ripple::Rate(slep->getFieldU32(sfTransferRate));
-        auto issuerAccID = chanFunds.getIssuer();
-        auto const xferRate = transferRate(view(), issuerAccID);
-        // update if issuer rate is less than locked rate
-        if (xferRate < lockedRate)
-        {
-            (*slep)[sfTransferRate] = xferRate.value;
-            lockedRate = xferRate;
-        }
-
         (*slep)[sfBalance] = ctx_.tx[sfBalance];
         STAmount const reqDelta = reqBalance - chanBalance;
         assert(reqDelta >= beast::zero);
@@ -770,8 +767,22 @@ PayChanClaim::doApply()
             if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
                 return temDISABLED;
 
+            // compute transfer fee, if any
+            if (!slep->isFieldPresent(sfTransferRate))
+                return tecINTERNAL;
+
+            Rate lockedRate = ripple::Rate(slep->getFieldU32(sfTransferRate));
+            auto const issuerAccID = chanFunds.getIssuer();
+            auto const xferRate = transferRate(view(), issuerAccID);
+            // update if issuer rate is less than locked rate
+            if (xferRate < lockedRate)
+            {
+                (*slep)[~sfTransferRate] = xferRate.value;
+                lockedRate = xferRate;
+            }
+
             auto sleSrcAcc = ctx_.view().peek(keylet::account(src));
-            TER result = trustTransferLockedBalance(
+            TER const result = trustTransferLockedBalance(
                 ctx_.view(),
                 txAccount,
                 sleSrcAcc,