diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index f7285e249..d0d319d9f 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -235,52 +235,14 @@ EscrowCreate::doApply() return tecUNFUNDED; else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens)) { - // find the user's trustline - auto const currency = amount.getCurrency(); - auto const issuer = amount.getIssuer(); - - auto& view = ctx_.view(); - auto& j = ctx_.journal; - - if (!view.peek(keylet::account(issuer))) - { - JLOG(j.warn()) - << "Issuer not found for escrow create source line"; - return tecNO_ISSUER; - } - - if (isGlobalFrozen(view, issuer)) - { - JLOG(j.warn()) << "Creating escrow for frozen asset"; - return tecFROZEN; - } - sleLine = view.peek(keylet::line(account, issuer, currency)); + // RH TODO: does doing a dry run here add any value? it should be done in + // preclaim but there is no preclaim in escrow. - if (!sleLine) - return tecUNFUNDED_PAYMENT; - - if (sleLine->isFlag(ctx_.tx[sfDestination] > issuer ? lsfHighFreeze : lsfLowFreeze)) - { - JLOG(j.warn()) << "Creating escrow for destination frozen trustline"; - return tecFROZEN; - } - - bool high = account > issuer; - - STAmount balance = (*sleLine)[sfBalance]; - - STAmount lockedBalance {sfLockedBalance, amount.issue()}; - if (sleLine->isFieldPresent(sfLockedBalance)) - lockedBalance = (*sleLine)[sfLockedBalance]; - - auto spendableBalance = balance - lockedBalance; - - if (high) - spendableBalance = -spendableBalance; - - if (amount > spendableBalance) - return tecUNFUNDED_PAYMENT; + // perform the lock as a dry run first + if (TER result = trustAdjustLockedBalance(ctx_.view(), sleLine, amount, true); + result != tesSUCCESS) + return result; } else return tecINTERNAL; // should never happen @@ -342,17 +304,11 @@ EscrowCreate::doApply() (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount]; else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens) && sleLine) { - // update trustline to reflect locked up balance - bool high = account > amount.getIssuer(); - - STAmount lockedBalance {sfLockedBalance, amount.issue()}; - - if (sleLine->isFieldPresent(sfLockedBalance)) - lockedBalance = (*sleLine)[sfLockedBalance]; - - lockedBalance += high ? -amount : amount; - sleLine->setFieldAmount(sfLockedBalance, lockedBalance); - ctx_.view().update(sleLine); + // do the lock-up for real now + TER result = + trustAdjustLockedBalance(ctx_.view(), sleLine, amount); + if (result != tesSUCCESS) + return result; } else return tecINTERNAL; // should never happen @@ -587,226 +543,21 @@ EscrowFinish::doApply() (*sled)[sfBalance] = (*sled)[sfBalance] + (*slep)[sfAmount]; else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens)) { - // There are three involved accounts and two trustlines (at the end) - // Src - Account which created the escrow - // Dst - Account which will receive a payout in the event of success - // Issuer - Account which issued the IOU the escrow locks - auto issuerAccID = amount.getIssuer(); - auto currency = amount.getCurrency(); + // 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 = + trustXferLockedBalance( + ctx_.view(), + account_, // txn signing account + sle, // src account + sled, // dst account + amount, // xfer amount + ctx_.journal); - Keylet klIssuer = keylet::account(issuerAccID); - - // amendments should only change as much code as is necessary to make the amendment work - // these renames should be applied to the whole class in a later, non-amendment, commit - auto& view = ctx_.view(); - - auto& srcAccID = account; - auto& dstAccID = destID; - - bool dstLow = dstAccID < issuerAccID; - bool srcLow = srcAccID < issuerAccID; - - auto& sleSrcAcc = sle; - auto& sleDstAcc = sled; - - auto& j = ctx_.journal; - - if (!sleSrcAcc) - { - JLOG(j.warn()) - << "Src account not found in escrow finish"; - return tecINTERNAL; - } - - STAmount dstBalanceDrops = sleDstAcc->getFieldAmount(sfBalance); - - // check if the destination has a trustline already - Keylet klDstLine = keylet::line(dstAccID, issuerAccID, currency); - - auto sleSrcLine = view.peek(keylet::line(srcAccID, issuerAccID, currency)); - auto sleDstLine = view.peek(klDstLine); - - // check the issuer exists - auto sleIssuer = view.peek(klIssuer); - if (!sleIssuer) - { - JLOG(j.warn()) - << "Cannot finish escrow for token from non-existent issuer: " - << to_string(issuerAccID); - return tecNO_ISSUER; - } - - // check the trustline isn't frozen - if (isGlobalFrozen(view, issuerAccID)) - { - JLOG(j.warn()) << "Cannot finish an escrow for frozen issuer"; - return tecFROZEN; - } - - // check the source line exists - if (!sleSrcLine) - { - JLOG(j.error()) - << "Cannot finish an escrow where the source line does not exist: " - << "src: " << to_string(srcAccID) << " " - << "iss: " << to_string(issuerAccID) << " " - << "cur: " << to_string(currency); - return tefINTERNAL; - } - - // check if rippling is allowed on the source line - { - auto const flag {srcLow ? lsfLowNoRipple : lsfHighNoRipple}; - bool tlNoRipple = sleSrcLine->getFieldU32(sfFlags) & flag; - if (tlNoRipple) - { - JLOG(j.warn()) << "EscrowFinish would violate noripple stauts on issuer."; - return tecPATH_DRY; - } - } - - // dstLow XNOR srcLow tells us if we need to flip the balance amount - // on the destination line - bool flipDstAmt = !((dstLow && srcLow) || (!dstLow && !srcLow)); - - // compute transfer fee, if any - auto xferRate = transferRate(view, issuerAccID); - - // the destination will sometimes get less depending on xfer rate - // with any difference in tokens burned - auto dstAmt = - xferRate == parityRate - ? amount - : multiplyRound(amount, xferRate, amount.issue(), true); - - // check if the dest line exists, and if it doesn't create it if we're allowed to - if (!sleDstLine) - { - // if a line doesn't already exist between issuer and destination then - // such a line can now be created but only if either the person who signed - // for this txn is the destination account or the source and dest are the same - if (srcAccID != dstAccID && account_ != dstAccID) - return tecNO_PERMISSION; - - // create trustline - - if (std::uint32_t const ownerCount = {sleDstAcc->at(sfOwnerCount)}; - dstBalanceDrops < view.fees().accountReserve(ownerCount + 1)) - { - JLOG(j.trace()) << "Dest Trust line does not exist. " - "Insufficent reserve to create line."; - return tecNO_LINE_INSUF_RESERVE; - } - - // clang-format off - if (TER const ter = trustCreate( - view, - dstLow, // is dest low? - issuerAccID, // source - dstAccID, // destination - klDstLine.key, // ledger index - sleDstAcc, // Account to add to - false, // authorize account - (sleDstAcc->getFlags() & lsfDefaultRipple) == 0, - false, // freeze trust line - flipDstAmt ? -dstAmt : dstAmt, // initial balance - Issue(currency, account_), // limit of zero - 0, // quality in - 0, // quality out - j); // journal - !isTesSuccess(ter)) - { - return ter; - } - // clang-format on - } - else - { - // trustline already exists - // check is it's frozen - if (dstAccID != issuerAccID && - sleDstLine->isFlag(dstLow ? lsfLowFreeze : lsfHighFreeze)) - { - JLOG(j.warn()) - << "Finishing an escrow for destination frozen trustline."; - return tecFROZEN; - } - - // increment dest balance - { - // if balance is higher than limit then only allow if dest is signer - STAmount dstLimit = - dstLow ? (*sleDstLine)[sfLowLimit] : (*sleDstLine)[sfHighLimit]; - - STAmount priorBalance = - dstLow ? (*sleDstLine)[sfBalance] : -((*sleDstLine)[sfBalance]); - - STAmount finalBalance = priorBalance + (flipDstAmt ? -dstAmt : dstAmt); - - if (finalBalance < priorBalance) - { - JLOG(j.warn()) - << "Escrow finish resulted in a lower final balance on dest line"; - return tecINTERNAL; - } - - if (finalBalance > dstLimit && account_ != dstAccID) - { - JLOG(j.trace()) - << "Escrow finish would increase dest line above limit without permission"; - return tecPATH_DRY; - } - - sleDstLine->setFieldAmount(sfBalance, dstLow ? finalBalance : -finalBalance); - } - - } - - // decrement source balance - { - STAmount priorBalance = - srcLow ? (*sleSrcLine)[sfBalance] : -((*sleSrcLine)[sfBalance]); - - STAmount finalBalance = priorBalance - amount; - if (finalBalance < beast::zero) - { - JLOG(j.warn()) - << "Escrow finish results in a negative balance on source line"; - return tecINTERNAL; - } - - if (!sleSrcLine->isFieldPresent(sfLockedBalance)) - { - JLOG(j.warn()) - << "Escrow finish could not find sfLockedBalance on source line"; - return tecINTERNAL; - } - - STAmount priorLockedBalance = - srcLow ? (*sleSrcLine)[sfLockedBalance] : -((*sleSrcLine)[sfLockedBalance]); - - STAmount finalLockedBalance = priorLockedBalance - amount; - if (finalLockedBalance < beast::zero) - { - JLOG(j.warn()) - << "Escrow finish results in a negative locked balance on source line"; - return tecINTERNAL; - } - - sleSrcLine->setFieldAmount(sfBalance, srcLow ? finalBalance : -finalBalance); - - if (finalLockedBalance == beast::zero) - sleSrcLine->makeFieldAbsent(sfLockedBalance); - else - sleSrcLine->setFieldAmount(sfLockedBalance, srcLow ? finalLockedBalance : -finalLockedBalance); - } - - // update source and dest lines to reflect balance mutation - view.update(sleSrcLine); - - if (sleDstLine) - view.update(sleDstLine); + if (result != tesSUCCESS) + return result; } else return tecINTERNAL; // should never happen @@ -902,30 +653,14 @@ EscrowCancel::doApply() (*sle)[sfBalance] = (*sle)[sfBalance] + (*slep)[sfAmount]; else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens)) { - auto& view = ctx_.view(); - - auto issuerAccID = amount.getIssuer(); - auto currency = amount.getCurrency(); - auto sleSrcLine = view.peek(keylet::line(account, issuerAccID, currency)); - bool isLow = account < issuerAccID; - - STAmount priorLockedBalance = - isLow ? (*sleSrcLine)[sfLockedBalance] : -(*sleSrcLine)[sfLockedBalance]; - - STAmount finalLockedBalance = priorLockedBalance - amount; - if (finalLockedBalance < beast::zero) - { - JLOG(j_.warn()) - << "Escrow cancel would force locked balance below zero"; - return tecINTERNAL; - } - - if (finalLockedBalance == beast::zero) - sleSrcLine->makeFieldAbsent(sfLockedBalance); - else - sleSrcLine->setFieldAmount(sfLockedBalance, isLow ? finalLockedBalance : -finalLockedBalance); - - view.update(sleSrcLine); + // unlock previously locked tokens from source line + auto line = view.peek(keylet::line(account, issuerAccID, currency)); + TER result = trustAdjustLockedBalance( + ctx_.view(), + line, + -amount); + if (result != tesSUCCESS) + return result; } else return tecINTERNAL; diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index f130725bc..8e37e6027 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -158,31 +158,14 @@ closeChannel( } else if (view.rules().enabled(featurePaychanAndEscrowForTokens)) { - // unlock the TL balance that was locked by this channel - auto& account = src; - auto issuerAccID = amount.getIssuer(); - auto currency = amount.getCurrency(); - auto sleSrcLine = view.peek(keylet::line(account, issuerAccID, currency)); - bool isLow = account < issuerAccID; - - STAmount priorLockedBalance = - isLow ? (*sleSrcLine)[sfLockedBalance] : -(*sleSrcLine)[sfLockedBalance]; - - STAmount finalLockedBalance = priorLockedBalance - amount; - if (finalLockedBalance < beast::zero) - { - JLOG(j.warn()) - << "Paychanel close would force locked balance below zero"; - return tecINTERNAL; - } - - if (finalLockedBalance == beast::zero) - sleSrcLine->makeFieldAbsent(sfLockedBalance); - else - sleSrcLine->setFieldAmount(sfLockedBalance, isLow ? finalLockedBalance : -finalLockedBalance); - - view.update(sleSrcLine); - + auto line = view.peek(keylet::line(src, amount.getIssuer(), amount.getCurrency())); + TER result = + trustAdjustLockedBalance( + view, + line, + -amount); + if (result != tesSUCCESS) + return result; } else return tefINTERNAL; @@ -263,62 +246,39 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) if (balance < reserve) return tecINSUFFICIENT_RESERVE; + auto const dst = ctx.tx[sfDestination]; + // Check reserve and funds availability if (isXRP(amount) && balance < reserve + ctx.tx[sfAmount]) return tecUNFUNDED; - else if (ctx.view.rules().enabled(featurePaychanAndEscrowForTokens)) - { - // find the user's trustline - auto const currency = amount.getCurrency(); - auto const issuer = amount.getIssuer(); - - auto& view = ctx.view; - auto& j = ctx.j; - - if (!view.read(keylet::account(issuer))) - { - JLOG(j.warn()) - << "Issuer not found for paychan create source line"; - return tecNO_ISSUER; - } - - if (isGlobalFrozen(view, issuer)) - { - JLOG(j.warn()) << "Creating paychan for frozen asset"; - return tecFROZEN; - } - - auto const sleLine = view.read(keylet::line(account, issuer, currency)); - - if (!sleLine) - return tecUNFUNDED_PAYMENT; - - if (sleLine->isFlag(ctx.tx[sfDestination] > issuer ? lsfHighFreeze : lsfLowFreeze)) - { - JLOG(j.warn()) << "Creating paychan for destination frozen trustline"; - return tecFROZEN; - } - - bool high = account > issuer; - - STAmount balance = (*sleLine)[sfBalance]; - - STAmount lockedBalance {sfLockedBalance, amount.issue()}; - if (sleLine->isFieldPresent(sfLockedBalance)) - lockedBalance = (*sleLine)[sfLockedBalance]; - - auto spendableBalance = balance - lockedBalance; - - if (high) - spendableBalance = -spendableBalance; - - if (amount > spendableBalance) - return tecUNFUNDED; - } else - return tecINTERNAL; + { + if (!ctx.view.rules().enabled(featurePaychanAndEscrowForTokens)) + return tecINTERNAL; - auto const dst = ctx.tx[sfDestination]; + // check for any possible bars to a channel existing + // between these accounts for this asset + if (TER result = + trustXferAllowed( + ctx.view, + {account, dst}, + amount.issue); + result != tesSUCCESS) + return result; + + // check if the amount can be locked + auto sleLine = ctx.view.read(keylet::line(account, amount.issuer(), amount.currency())); + if (TER result = + trustAdjustLockedBalance( + ctx.view, + sleLine, + amount, + true); + result != tesSUCCESS) + return result; + + // all good! + } { // Check destination account @@ -397,41 +357,28 @@ PayChanCreate::doApply() // Deduct owner's balance, increment owner count if (isXRP(amount)) (*sle)[sfBalance] = (*sle)[sfBalance] - amount; - else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens)) + else { - // find the user's trustline - auto const currency = amount.getCurrency(); - auto const issuer = amount.getIssuer(); + if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens)) + return tefINTERNAL; - auto& view = ctx_.view(); - auto& j = ctx_.journal; + auto sleLine = + view.peek(keylet::line(account, amount.getIssuer(), amount.getCurrency())); - auto sleLine = view.peek(keylet::line(account, issuer, currency)); if (!sleLine) return tecUNFUNDED_PAYMENT; - bool high = account > issuer; + TER result = + trustAdjustLockedBalance( + ctx_.view(), + sleLine, + amount, + false); - STAmount balance = (*sleLine)[sfBalance]; - - STAmount lockedBalance {sfLockedBalance, amount.issue()}; - if (sleLine->isFieldPresent(sfLockedBalance)) - lockedBalance = (*sleLine)[sfLockedBalance]; - - auto spendableBalance = balance - lockedBalance; - - if (high) - spendableBalance = -spendableBalance; - - if (amount > spendableBalance) - return tecUNFUNDED_PAYMENT; - - lockedBalance += high ? -amount : amount; - sleLine->setFieldAmount(sfLockedBalance, lockedBalance); - ctx_.view().update(sleLine); + if (result != tesSUCCESS) + return tefINTERNAL; } - else - return tecINTERNAL; + adjustOwnerCount(ctx_.view(), sle, 1, ctx_.journal); ctx_.view().update(sle); @@ -483,6 +430,7 @@ PayChanFund::preflight(PreflightContext const& ctx) return preflight2(ctx); } +// RH UPTO TER PayChanFund::doApply() { diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 26aa8f03d..89d2ef2a4 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -962,9 +962,13 @@ bool isTrustDefault( TER trustXferAllowed( ReadView& view, - std::vector parties, + std::vector const& parties, Issue const& issue) { + + if (isFakeXRP(issue.currency)) + return tecNO_PERMISSION; + auto const sleIssuerAcc = view.peek(keylet::account(issue.account)); bool lockedBalanceAllowed = @@ -979,6 +983,7 @@ trustXferAllowed( return tecFROZEN; uint32_t issuerFlags = sleIssuerAcc->getFieldU32(sfFlags); + bool requireAuth = issuerFlags & lsfRequireAuth; for (AccountID const& p: parties) @@ -1056,18 +1061,47 @@ trustXferAllowed( return tesSUCCESS; } +// Type juggling to allow dry runs of helper functions from preclaim +// context, without forcing the caller to recast or change their view. +// ApplyView is a subclass of ReadView so this logic works fine and +// allows a helper to be called explicitly and without confusion. +using ReadOrApplyView = + std::variant< + std::reference_wrapper, + std::reference_wrapper>; + +inline ReadView const& +forceReadView(ReadOrApplyView& view) +{ + return + *(std::holds_alternative(view) + ? &(std::get(view).get()) + : std::reintrepret_cast( + &(std::get(view).get()))); +} + // Modify a locked trustline balance, creating one if none exists // or removing one if no longer needed. deltaAmt is in absolute terms // positive means increment locked balance and negative decrement. TER -trustModifyLockedBalance( - ApplyView& view, +trustAdjustLockedBalance( + ReadOrApplyView view_, std::shared_ptr& sleLine, - STAmount const& deltaAmt) + STAmount const& deltaAmt, + bool dryRun = false /* don't actually update, just try the delta */) { - if (!view.rules().enabled(featurePaychanAndEscrowForTokens)) + bool bReadView = std::holds_alternative(view_); + + // dry runs are explicit in code, but really the view type determines + // what occurs here, so this combination is invalid. + if (bReadView && !dryRun) return tefINTERNAL; + ReadView const& view = forceReadView(view_); + + if (!view.rules.enabled(featurePaychanAndEscrowForTokens)) + return tefINTERNAL; + if (!sleLine) return tecUNFUNDED_PAYMENT; @@ -1081,13 +1115,17 @@ trustModifyLockedBalance( // issuer then the high side is the account. bool high = lowLimit.issuer() == issuer; + std::vector parties + {high ? sleLine->getFieldAmount(sfHighLimit).issuer(): lowLimit.issuer()}; + // check for freezes & auth if (TER result = - trustXferAllowed(view, - {high ? sleLine->getFieldAmount(sfHighLimit).issuer() - : lowLimit.issuer()}, deltaAmt.issue()); - result != tesSUCCESS) - return result; + trustXferAllowed( + view, + parties, + deltaAmt.issue()) + result != tesSUCCESS) + return result; // pull the TL balance from the account's perspective STAmount balance = @@ -1114,12 +1152,18 @@ trustModifyLockedBalance( if (lockedBalance < beast::zero) return tecINTERNAL; + // we won't update any SLEs if it is a dry run + if (dryRun) + return tesSUCCESS; + if (lockedBalance == beast::zero) sleLine->makeFieldAbsent(sfLockedBalance); else sleLine->setFieldAmount(sfLockedBalance, high ? -lockedBalance : lockedBalance); - view.update(sleLine); + // dryRun must be true if we have a ReadView in the variant + // therefore this is safe to execute + std::get>(view_).get().update(sleLine); return tesSUCCESS; } @@ -1133,6 +1177,9 @@ trustXferLockedBalance( STAmount const& amount, // issuer, currency are in this field Journal& j) { + if (!view.rules().enabled(featurePaychanAndEscrowForTokens)) + return tefINTERNAL; + if (!sleSrcAcc || !sleDstAcc) { JLOG(j.warn()) diff --git a/src/ripple/protocol/STAmount.h b/src/ripple/protocol/STAmount.h index 181fec248..dd844ee6e 100644 --- a/src/ripple/protocol/STAmount.h +++ b/src/ripple/protocol/STAmount.h @@ -525,7 +525,6 @@ isFakeXRP(STAmount const& amount) return isFakeXRP(amount.issue().currency); } - // Since `canonicalize` does not have access to a ledger, this is needed to put // the low-level routine stAmountCanonicalize on an amendment switch. Only // transactions need to use this switchover. Outside of a transaction it's safe