diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index 5e6fb8f5ce..fc16d53e61 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -352,7 +352,7 @@ accountHolds( // // <-- saAmount: amount of currency held by account. May be negative. [[nodiscard]] STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, Currency const& currency, @@ -361,7 +361,7 @@ accountCanSend( beast::Journal j); [[nodiscard]] STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, Issue const& issue, @@ -369,7 +369,7 @@ accountCanSend( beast::Journal j); [[nodiscard]] STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, MPTIssue const& mptIssue, @@ -378,7 +378,7 @@ accountCanSend( beast::Journal j); [[nodiscard]] STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, Asset const& asset, @@ -753,6 +753,17 @@ canWithdraw( [[nodiscard]] TER canWithdraw(ReadView const& view, STTx const& tx); +[[nodiscard]] TER +doWithdraw( + ApplyView& view, + STTx const& tx, + AccountID const& senderAcct, + AccountID const& dstAcct, + AccountID const& sourceAcct, + XRPAmount priorBalance, + STAmount const& amount, + beast::Journal j); + /// Any transactors that call addEmptyHolding() in doApply must call /// canAddHolding() in preflight with the same View and Asset [[nodiscard]] TER diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index 6765f7cf7d..7ad1e70256 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -81,13 +81,27 @@ public: bool native() const { - return holds() && get().native(); + return std::visit( + [&](TIss const& issue) { + if constexpr (std::is_same_v) + return issue.native(); + if constexpr (std::is_same_v) + return false; + }, + issue_); } bool integral() const { - return !holds() || get().native(); + return std::visit( + [&](TIss const& issue) { + if constexpr (std::is_same_v) + return issue.native(); + if constexpr (std::is_same_v) + return true; + }, + issue_); } friend constexpr bool diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 2487d29c9b..ded16cad6d 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -110,6 +110,7 @@ tenthBipsOfValue(T value, TenthBips bips) return value * bips.value() / tenthBipsPerUnity.value(); } +namespace Lending { /** The maximum management fee rate allowed by a loan broker in 1/10 bips. Valid values are between 0 and 10% inclusive. @@ -197,6 +198,7 @@ static constexpr int loanPaymentsPerFeeIncrement = 5; * without an amendment */ static constexpr int loanMaximumPaymentsPerTransaction = 100; +} // namespace Lending /** The maximum length of a URI inside an NFT */ std::size_t constexpr maxTokenURILength = 256; diff --git a/include/xrpl/protocol/PublicKey.h b/include/xrpl/protocol/PublicKey.h index 445e0e3b97..8d8062408b 100644 --- a/include/xrpl/protocol/PublicKey.h +++ b/include/xrpl/protocol/PublicKey.h @@ -231,11 +231,7 @@ verifyDigest( SHA512-Half, and the resulting digest is signed. */ [[nodiscard]] bool -verify( - PublicKey const& publicKey, - Slice const& m, - Slice const& sig, - bool mustBeFullyCanonical = true) noexcept; +verify(PublicKey const& publicKey, Slice const& m, Slice const& sig) noexcept; /** Calculate the 160-bit node ID from a node public key. */ NodeID diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index f3f285249c..3ea45cc05b 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -564,7 +564,7 @@ STAmount::clear() { // The -100 is used to allow 0 to sort less than a small positive values // which have a negative exponent. - mOffset = native() ? 0 : -100; + mOffset = integral() ? 0 : -100; mValue = 0; mIsNegative = false; } @@ -701,7 +701,7 @@ getRate(STAmount const& offerOut, STAmount const& offerIn); */ STAmount roundToScale( - STAmount value, + STAmount const& value, std::int32_t scale, Number::rounding_mode rounding = Number::getround()); @@ -729,7 +729,7 @@ roundToAsset( STAmount const ret{asset, value}; if (ret.integral()) return ret; - // Not that the ctor will round integral types (XRP, MPT) via canonicalize, + // Note that the ctor will round integral types (XRP, MPT) via canonicalize, // so no extra work is needed for those. return roundToScale(ret, scale); } diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index 716881d910..02f865e349 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -103,22 +103,15 @@ public: std::optional> signatureTarget = {}); - enum class RequireFullyCanonicalSig : bool { no, yes }; - /** Check the signature. - @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. @return `true` if valid signature. If invalid, the error message string. */ Expected - checkSign(RequireFullyCanonicalSig requireCanonicalSig, Rules const& rules) - const; + checkSign(Rules const& rules) const; Expected - checkBatchSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules) const; + checkBatchSign(Rules const& rules) const; // SQL Functions with metadata. static std::string const& @@ -140,40 +133,25 @@ public: private: /** Check the signature. - @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 Reference to object that contains the signature fields. Will be *this more often than not. @return `true` if valid signature. If invalid, the error message string. */ Expected - checkSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules, - STObject const& sigObject) const; + checkSign(Rules const& rules, STObject const& sigObject) const; Expected - checkSingleSign( - RequireFullyCanonicalSig requireCanonicalSig, - STObject const& sigObject) const; + checkSingleSign(STObject const& sigObject) const; Expected - checkMultiSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules, - STObject const& sigObject) const; + checkMultiSign(Rules const& rules, STObject const& sigObject) const; Expected - checkBatchSingleSign( - STObject const& batchSigner, - RequireFullyCanonicalSig requireCanonicalSig) const; + checkBatchSingleSign(STObject const& batchSigner) const; Expected - checkBatchMultiSign( - STObject const& batchSigner, - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules) const; + checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const; STBase* copy(std::size_t n, void* buf) const override; diff --git a/include/xrpl/protocol/TxFlags.h b/include/xrpl/protocol/TxFlags.h index 0f3ea94b21..d4faed192c 100644 --- a/include/xrpl/protocol/TxFlags.h +++ b/include/xrpl/protocol/TxFlags.h @@ -272,8 +272,14 @@ constexpr std::uint32_t const tfBatchMask = // as an overpayment. False, no overpayments will be taken. constexpr std::uint32_t const tfLoanOverpayment = 0x00010000; // LoanPay exclusive flags: -// tfLoanFullPayment: True, indicates that the payment is +// tfLoanFullPayment: True, indicates that the payment is an early +// full payment. It must pay the entire loan including close +// interest and fees, or it will fail. False: Not a full payment. constexpr std::uint32_t const tfLoanFullPayment = 0x00020000; +// tfLoanLatePayment: True, indicates that the payment is late, +// and includes late iterest and fees. If the loan is not late, +// it will fail. False: not a late payment. If the current payment +// is overdue, the transaction will fail. constexpr std::uint32_t const tfLoanLatePayment = 0x00040000; constexpr std::uint32_t const tfLoanSetMask = ~(tfUniversal | tfLoanOverpayment); diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index b6d0cf105b..c36b282835 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -68,8 +68,6 @@ XRPL_FIX (UniversalNumber, Supported::yes, VoteBehavior::DefaultNo XRPL_FEATURE(XRPFees, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (RemoveNFTokenAutoTrustLine, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) // The following amendments are obsolete, but must remain supported @@ -82,9 +80,9 @@ XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYe // enabled (added to the ledger). // // If a feature remains obsolete for long enough that no clients are able -// to vote for it, the feature can be removed (entirely?) from the code. -XRPL_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete) - +// to vote for it, the feature can be removed entirely from the code. Until +// then the feature needs to be marked explicitly as obsolete, e.g. +// XRPL_FEATURE(Example, Supported::yes, VoteBehavior::Obsolete) // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. // All known amendments and amendments that may appear in a validated ledger @@ -120,6 +118,8 @@ XRPL_RETIRE_FIX(TrustLinesToSelf) XRPL_RETIRE_FEATURE(Checks) XRPL_RETIRE_FEATURE(CheckCashMakesTrustLine) XRPL_RETIRE_FEATURE(CryptoConditions) +XRPL_RETIRE_FEATURE(CryptoConditionsSuite) +XRPL_RETIRE_FEATURE(DeletableAccounts) XRPL_RETIRE_FEATURE(DepositAuth) XRPL_RETIRE_FEATURE(DepositPreauth) XRPL_RETIRE_FEATURE(DisallowIncoming) @@ -135,6 +135,7 @@ XRPL_RETIRE_FEATURE(MultiSignReserve) XRPL_RETIRE_FEATURE(NegativeUNL) XRPL_RETIRE_FEATURE(NonFungibleTokensV1_1) XRPL_RETIRE_FEATURE(PayChan) +XRPL_RETIRE_FEATURE(RequireFullyCanonicalSig) XRPL_RETIRE_FEATURE(SortedDirectories) XRPL_RETIRE_FEATURE(TicketBatch) XRPL_RETIRE_FEATURE(TickSize) diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 5f6d1b844e..1034c35895 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -572,7 +572,7 @@ LEDGER_ENTRY(ltLOAN, 0x0089, Loan, loan, ({ // // - InterestOwedToVault = InterestOutstanding - ManagementFeeOutstanding // The amount of the total interest that is owed to the vault, and - // will be sent to as part of a payment. + // will be sent to it as part of a payment. // // - TrueTotalLoanValue = PaymentRemaining * PeriodicPayment // The unrounded true total value of the loan. diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index dd1b52b355..6d2d833440 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -297,7 +297,7 @@ TRANSACTION(ttTRUST_SET, 20, TrustSet, #endif TRANSACTION(ttACCOUNT_DELETE, 21, AccountDelete, Delegation::notDelegatable, - featureDeletableAccounts, + uint256{}, mustDeleteAcct, ({ {sfDestination, soeREQUIRED}, diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 28f6bb89be..33c6cf69ec 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -564,7 +564,7 @@ accountHolds( } STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, Currency const& currency, @@ -589,19 +589,19 @@ accountCanSend( } STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, Issue const& issue, FreezeHandling zeroIfFrozen, beast::Journal j) { - return accountCanSend( + return accountSpendable( view, account, issue.currency, issue.account, zeroIfFrozen, j); } STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, MPTIssue const& mptIssue, @@ -631,7 +631,7 @@ accountCanSend( } [[nodiscard]] STAmount -accountCanSend( +accountSpendable( ReadView const& view, AccountID const& account, Asset const& asset, @@ -645,9 +645,9 @@ accountCanSend( std::remove_cvref_t, Issue>) { - return accountCanSend(view, account, value, zeroIfFrozen, j); + return accountSpendable(view, account, value, zeroIfFrozen, j); } - return accountCanSend( + return accountSpendable( view, account, value, zeroIfFrozen, zeroIfUnauthorized, j); }, asset.value()); @@ -1386,6 +1386,55 @@ canWithdraw(ReadView const& view, STTx const& tx) return canWithdraw(from, view, to, tx.isFieldPresent(sfDestinationTag)); } +TER +doWithdraw( + ApplyView& view, + STTx const& tx, + AccountID const& senderAcct, + AccountID const& dstAcct, + AccountID const& sourceAcct, + XRPAmount priorBalance, + STAmount const& amount, + beast::Journal j) +{ + // Create trust line or MPToken for the receiving account + if (dstAcct == senderAcct) + { + if (auto const ter = addEmptyHolding( + view, senderAcct, priorBalance, amount.asset(), j); + !isTesSuccess(ter) && ter != tecDUPLICATE) + return ter; + } + else + { + auto dstSle = view.peek(keylet::account(dstAcct)); + if (auto err = + verifyDepositPreauth(tx, view, senderAcct, dstAcct, dstSle, j)) + return err; + } + + // Sanity check + if (accountHolds( + view, + sourceAcct, + amount.asset(), + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j) < amount) + { + // LCOV_EXCL_START + JLOG(j.error()) << "LoanBrokerCoverWithdraw: negative balance of " + "broker cover assets."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + // Move the funds directly from the broker's pseudo-account to the + // dstAcct + return accountSend( + view, sourceAcct, dstAcct, amount, j, WaiveTransferFee::Yes); +} + [[nodiscard]] TER addEmptyHolding( ApplyView& view, @@ -1730,10 +1779,13 @@ removeEmptyHolding( } // `asset` is an IOU. + // If the account is the issuer, then no line should exist. Check anyway. If + // a line does exist, it will get deleted. If not, return success. + bool const accountIsIssuer = accountID == issue.account; auto const line = view.peek(keylet::line(accountID, issue)); if (!line) - return tecOBJECT_NOT_FOUND; - if (line->at(sfBalance)->iou() != beast::zero) + return accountIsIssuer ? (TER)tesSUCCESS : (TER)tecOBJECT_NOT_FOUND; + if (!accountIsIssuer && line->at(sfBalance)->iou() != beast::zero) return tecHAS_OBLIGATIONS; // Adjust the owner count(s) @@ -1782,10 +1834,18 @@ removeEmptyHolding( MPTIssue const& mptIssue, beast::Journal journal) { + // If the account is the issuer, then no token should exist. MPTs do not + // have the legacy ability to create such a situation, but check anyway. If + // a token does exist, it will get deleted. If not, return success. + bool const accountIsIssuer = accountID == mptIssue.getIssuer(); auto const& mptID = mptIssue.getMptID(); auto const mptoken = view.peek(keylet::mptoken(mptID, accountID)); if (!mptoken) - return tecOBJECT_NOT_FOUND; + return accountIsIssuer ? (TER)tesSUCCESS : (TER)tecOBJECT_NOT_FOUND; + // Unlike a trust line, if the account is the issuer, and the token has a + // balance, it can not just be deleted, because that will throw the issuance + // accounting out of balance, so fail. Since this should be impossible + // anyway, I'm not going to put any effort into it. if (mptoken->at(sfMPTAmount) != 0) return tecHAS_OBLIGATIONS; @@ -2064,7 +2124,7 @@ rippleSendIOU( beast::Journal j, WaiveTransferFee waiveFee) { - auto const issuer = saAmount.getIssuer(); + auto const& issuer = saAmount.getIssuer(); XRPL_ASSERT( !isXRP(uSenderID) && !isXRP(uReceiverID), @@ -2078,7 +2138,7 @@ rippleSendIOU( // Direct send: redeeming IOUs and/or sending own IOUs. auto const ter = rippleCreditIOU(view, uSenderID, uReceiverID, saAmount, false, j); - if (view.rules().enabled(featureDeletableAccounts) && ter != tesSUCCESS) + if (ter != tesSUCCESS) return ter; saActual = saAmount; return tesSUCCESS; @@ -2119,7 +2179,7 @@ rippleSendMultiIOU( beast::Journal j, WaiveTransferFee waiveFee) { - auto const issuer = issue.getIssuer(); + auto const& issuer = issue.getIssuer(); XRPL_ASSERT( !isXRP(senderID), "ripple::rippleSendMultiIOU : sender is not XRP"); @@ -2459,7 +2519,7 @@ rippleCreditMPT( { // Do not check MPT authorization here - it must have been checked earlier auto const mptID = keylet::mptIssuance(saAmount.get().getMptID()); - auto const issuer = saAmount.getIssuer(); + auto const& issuer = saAmount.getIssuer(); auto sleIssuance = view.peek(mptID); if (!sleIssuance) return tecOBJECT_NOT_FOUND; @@ -2526,7 +2586,7 @@ rippleSendMPT( "ripple::rippleSendMPT : sender is not receiver"); // Safe to get MPT since rippleSendMPT is only called by accountSendMPT - auto const issuer = saAmount.getIssuer(); + auto const& issuer = saAmount.getIssuer(); auto const sle = view.read(keylet::mptIssuance(saAmount.get().getMptID())); @@ -2589,7 +2649,7 @@ rippleSendMultiMPT( { // Safe to get MPT since rippleSendMultiMPT is only called by // accountSendMultiMPT - auto const issuer = mptIssue.getIssuer(); + auto const& issuer = mptIssue.getIssuer(); auto const sle = view.read(keylet::mptIssuance(mptIssue.getMptID())); if (!sle) @@ -2621,12 +2681,16 @@ rippleSendMultiMPT( // not exceed MaximumAmount if (senderID == issuer) { + XRPL_ASSERT_PARTS( + takeFromSender == beast::zero, + "rippler::rippleSendMultiMPT", + "sender == issuer, takeFromSender == zero"); auto const sendAmount = amount.mpt().value(); auto const maximumAmount = sle->at(~sfMaximumAmount).value_or(maxMPTokenAmount); - if (sendAmount > maximumAmount - takeFromSender || + if (sendAmount > maximumAmount || sle->getFieldU64(sfOutstandingAmount) > - maximumAmount - sendAmount - takeFromSender) + maximumAmount - sendAmount) return tecPATH_DRY; } @@ -3287,7 +3351,7 @@ canTransfer( if (issue.native()) return tesSUCCESS; - auto const issuerId = issue.getIssuer(); + auto const& issuerId = issue.getIssuer(); if (issuerId == from || issuerId == to) return tesSUCCESS; auto const sleIssuer = view.read(keylet::account(issuerId)); diff --git a/src/libxrpl/protocol/PublicKey.cpp b/src/libxrpl/protocol/PublicKey.cpp index 0a29af7d32..f1794331ad 100644 --- a/src/libxrpl/protocol/PublicKey.cpp +++ b/src/libxrpl/protocol/PublicKey.cpp @@ -267,18 +267,13 @@ verifyDigest( } bool -verify( - PublicKey const& publicKey, - Slice const& m, - Slice const& sig, - bool mustBeFullyCanonical) noexcept +verify(PublicKey const& publicKey, Slice const& m, Slice const& sig) noexcept { if (auto const type = publicKeyType(publicKey)) { if (*type == KeyType::secp256k1) { - return verifyDigest( - publicKey, sha512Half(m), sig, mustBeFullyCanonical); + return verifyDigest(publicKey, sha512Half(m), sig); } else if (*type == KeyType::ed25519) { diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 566957509c..8b714193cc 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -830,7 +830,7 @@ STAmount::isDefault() const void STAmount::canonicalize() { - if (native() || mAsset.holds()) + if (integral()) { // native and MPT currency amounts should always have an offset of zero // log(2^64,10) ~ 19.2 @@ -859,8 +859,10 @@ STAmount::canonicalize() }; if (native()) set(XRPAmount{num}); - else + else if (mAsset.holds()) set(MPTAmount{num}); + else + Throw("Unknown integral asset type"); mOffset = 0; } else @@ -1462,9 +1464,12 @@ canonicalizeRoundStrict( } STAmount -roundToScale(STAmount value, std::int32_t scale, Number::rounding_mode rounding) +roundToScale( + STAmount const& value, + std::int32_t scale, + Number::rounding_mode rounding) { - // Nothing to do for intgral types. + // Nothing to do for integral types. if (value.integral()) return value; @@ -1474,16 +1479,15 @@ roundToScale(STAmount value, std::int32_t scale, Number::rounding_mode rounding) if (value.exponent() >= scale) return value; - STAmount referenceValue{ + STAmount const referenceValue{ value.asset(), STAmount::cMinValue, scale, value.negative()}; NumberRoundModeGuard mg(rounding); - // With an IOU, the total will be truncated to the precision of the - // larger value: referenceValue - value += referenceValue; - // Remove the reference value, and we're left with the rounded value. - value -= referenceValue; - return value; + // With an IOU, the the result of addition will be truncated to the + // precision of the larger value, which in this case is referenceValue. Then + // remove the reference value via subtraction, and we're left with the + // rounded value. + return (value + referenceValue) - referenceValue; } namespace { diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 006f4fee7a..d3b3ab4a88 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -237,10 +237,7 @@ STTx::sign( } Expected -STTx::checkSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules, - STObject const& sigObject) const +STTx::checkSign(Rules const& rules, STObject const& sigObject) const { try { @@ -249,9 +246,8 @@ STTx::checkSign( // multi-signing. Otherwise we're single-signing. Blob const& signingPubKey = sigObject.getFieldVL(sfSigningPubKey); - return signingPubKey.empty() - ? checkMultiSign(requireCanonicalSig, rules, sigObject) - : checkSingleSign(requireCanonicalSig, sigObject); + return signingPubKey.empty() ? checkMultiSign(rules, sigObject) + : checkSingleSign(sigObject); } catch (std::exception const&) { @@ -260,27 +256,22 @@ STTx::checkSign( } Expected -STTx::checkSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules) const +STTx::checkSign(Rules const& rules) const { - if (auto const ret = checkSign(requireCanonicalSig, rules, *this); !ret) + if (auto const ret = checkSign(rules, *this); !ret) return ret; if (isFieldPresent(sfCounterpartySignature)) { auto const counterSig = getFieldObject(sfCounterpartySignature); - if (auto const ret = checkSign(requireCanonicalSig, rules, counterSig); - !ret) + if (auto const ret = checkSign(rules, counterSig); !ret) return Unexpected("Counterparty: " + ret.error()); } return {}; } Expected -STTx::checkBatchSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules) const +STTx::checkBatchSign(Rules const& rules) const { try { @@ -297,8 +288,8 @@ STTx::checkBatchSign( { Blob const& signingPubKey = signer.getFieldVL(sfSigningPubKey); auto const result = signingPubKey.empty() - ? checkBatchMultiSign(signer, requireCanonicalSig, rules) - : checkBatchSingleSign(signer, requireCanonicalSig); + ? checkBatchMultiSign(signer, rules) + : checkBatchSingleSign(signer); if (!result) return result; @@ -393,10 +384,7 @@ STTx::getMetaSQL( } static Expected -singleSignHelper( - STObject const& sigObject, - Slice const& data, - bool const fullyCanonical) +singleSignHelper(STObject const& sigObject, Slice const& data) { // 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 @@ -411,11 +399,8 @@ singleSignHelper( if (publicKeyType(makeSlice(spk))) { Blob const signature = sigObject.getFieldVL(sfTxnSignature); - validSig = verify( - PublicKey(makeSlice(spk)), - data, - makeSlice(signature), - fullyCanonical); + validSig = + verify(PublicKey(makeSlice(spk)), data, makeSlice(signature)); } } catch (std::exception const&) @@ -430,33 +415,24 @@ singleSignHelper( } Expected -STTx::checkSingleSign( - RequireFullyCanonicalSig requireCanonicalSig, - STObject const& sigObject) const +STTx::checkSingleSign(STObject const& sigObject) const { auto const data = getSigningData(*this); - bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || - (requireCanonicalSig == STTx::RequireFullyCanonicalSig::yes); - return singleSignHelper(sigObject, makeSlice(data), fullyCanonical); + return singleSignHelper(sigObject, makeSlice(data)); } Expected -STTx::checkBatchSingleSign( - STObject const& batchSigner, - RequireFullyCanonicalSig requireCanonicalSig) const +STTx::checkBatchSingleSign(STObject const& batchSigner) const { Serializer msg; serializeBatch(msg, getFlags(), getBatchTransactionIDs()); - bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || - (requireCanonicalSig == STTx::RequireFullyCanonicalSig::yes); - return singleSignHelper(batchSigner, msg.slice(), fullyCanonical); + return singleSignHelper(batchSigner, msg.slice()); } Expected multiSignHelper( STObject const& sigObject, std::optional txnAccountID, - bool const fullyCanonical, std::function makeMsg, Rules const& rules) { @@ -513,8 +489,7 @@ multiSignHelper( validSig = verify( PublicKey(makeSlice(spk)), makeMsg(accountID).slice(), - makeSlice(signature), - fullyCanonical); + makeSlice(signature)); } } catch (std::exception const& e) @@ -533,14 +508,8 @@ multiSignHelper( } Expected -STTx::checkBatchMultiSign( - STObject const& batchSigner, - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules) const +STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const { - bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || - (requireCanonicalSig == RequireFullyCanonicalSig::yes); - // We can ease the computational load inside the loop a bit by // pre-constructing part of the data that we hash. Fill a Serializer // with the stuff that stays constant from signature to signature. @@ -549,7 +518,6 @@ STTx::checkBatchMultiSign( return multiSignHelper( batchSigner, std::nullopt, - fullyCanonical, [&dataStart](AccountID const& accountID) -> Serializer { Serializer s = dataStart; finishMultiSigningData(accountID, s); @@ -559,14 +527,8 @@ STTx::checkBatchMultiSign( } Expected -STTx::checkMultiSign( - RequireFullyCanonicalSig requireCanonicalSig, - Rules const& rules, - STObject const& sigObject) const +STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const { - bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || - (requireCanonicalSig == RequireFullyCanonicalSig::yes); - // Used inside the loop in multiSignHelper to enforce that // the account owner may not multisign for themselves. auto const txnAccountID = &sigObject != this @@ -580,7 +542,6 @@ STTx::checkMultiSign( return multiSignHelper( sigObject, txnAccountID, - fullyCanonical, [&dataStart](AccountID const& accountID) -> Serializer { Serializer s = dataStart; finishMultiSigningData(accountID, s); diff --git a/src/test/app/AccountDelete_test.cpp b/src/test/app/AccountDelete_test.cpp index 9d5d1cd877..f22f9c4165 100644 --- a/src/test/app/AccountDelete_test.cpp +++ b/src/test/app/AccountDelete_test.cpp @@ -418,60 +418,6 @@ public: env.close(); } - void - testAmendmentEnable() - { - // Start with the featureDeletableAccounts amendment disabled. - // Then enable the amendment and delete an account. - using namespace jtx; - - testcase("Amendment enable"); - - Env env{*this, testable_amendments() - featureDeletableAccounts}; - Account const alice("alice"); - Account const becky("becky"); - - env.fund(XRP(10000), alice, becky); - env.close(); - - // Close enough ledgers to be able to delete alice's account. - incLgrSeqForAccDel(env, alice); - - // Verify that alice's account root is present. - Keylet const aliceAcctKey{keylet::account(alice.id())}; - BEAST_EXPECT(env.closed()->exists(aliceAcctKey)); - - auto const alicePreDelBal{env.balance(alice)}; - auto const beckyPreDelBal{env.balance(becky)}; - - auto const acctDelFee{drops(env.current()->fees().increment)}; - env(acctdelete(alice, becky), fee(acctDelFee), ter(temDISABLED)); - env.close(); - - // Verify that alice's account root is still present and alice and - // becky both have their XRP. - BEAST_EXPECT(env.current()->exists(aliceAcctKey)); - BEAST_EXPECT(env.balance(alice) == alicePreDelBal); - BEAST_EXPECT(env.balance(becky) == beckyPreDelBal); - - // When the amendment is enabled the previous transaction is - // retried into the new open ledger and succeeds. - env.enableFeature(featureDeletableAccounts); - env.close(); - - // alice's account is still in the most recently closed ledger. - BEAST_EXPECT(env.closed()->exists(aliceAcctKey)); - - // Verify that alice's account root is gone from the current ledger - // and becky has alice's XRP. - BEAST_EXPECT(!env.current()->exists(aliceAcctKey)); - BEAST_EXPECT( - env.balance(becky) == alicePreDelBal + beckyPreDelBal - acctDelFee); - - env.close(); - BEAST_EXPECT(!env.closed()->exists(aliceAcctKey)); - } - void testTooManyOffers() { @@ -1148,7 +1094,6 @@ public: testBasics(); testDirectories(); testOwnedTypes(); - testAmendmentEnable(); testTooManyOffers(); testImplicitlyCreatedTrustline(); testBalanceTooSmallForFee(); diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 2f3b9337e0..f2fae58a9b 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -2295,12 +2295,9 @@ class Batch_test : public beast::unit_test::suite Serializer s; parsed.object->add(s); auto const jrr = env.rpc("submit", strHex(s.slice()))[jss::result]; - BEAST_EXPECTS( - jrr[jss::status] == "error" && - jrr[jss::error] == "invalidTransaction" && - jrr[jss::error_exception] == - "fails local checks: Empty SigningPubKey.", - to_string(jrr)); + BEAST_EXPECT( + jrr[jss::status] == "success" && + jrr[jss::engine_result] == "temINVALID_FLAG"); env.close(); } diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index aa5b5519b9..66ce525e8c 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -638,6 +638,7 @@ class LoanBroker_test : public beast::unit_test::suite } using namespace loanBroker; + using namespace ripple::Lending; TenthBips32 const tenthBipsZero{0}; @@ -672,11 +673,16 @@ class LoanBroker_test : public beast::unit_test::suite coverRateMinimum(maxCoverRate), coverRateLiquidation(maxCoverRate), ter(tecNO_PERMISSION)); - // Cover: too big + // CoverMinimum: too big env(set(evan, vault.vaultID), coverRateMinimum(maxCoverRate + 1), coverRateLiquidation(maxCoverRate + 1), ter(temINVALID)); + // CoverLiquidation: too big + env(set(evan, vault.vaultID), + coverRateMinimum(maxCoverRate / 2), + coverRateLiquidation(maxCoverRate + 1), + ter(temINVALID)); // Cover: zero min, non-zero liquidation - implicit and // explicit zero values. env(set(evan, vault.vaultID), @@ -1057,6 +1063,20 @@ class LoanBroker_test : public beast::unit_test::suite // preclaim: tecHAS_OBLIGATIONS env(del(alice, brokerKeylet.key), ter(tecHAS_OBLIGATIONS)); + + // Repay and delete the loan + auto const loanKeylet = keylet::loan(brokerKeylet.key, 1); + env(loan::pay(borrower, loanKeylet.key, asset(50).value())); + env(loan::del(alice, loanKeylet.key)); + + env(trust(issuer, asset(0), alice, tfSetFreeze | tfSetDeepFreeze)); + // preclaim: tecFROZEN (deep frozen) + env(del(alice, brokerKeylet.key), ter(tecFROZEN)); + env(trust( + issuer, asset(0), alice, tfClearFreeze | tfClearDeepFreeze)); + + // successful delete the loan broker object + env(del(alice, brokerKeylet.key), ter(tesSUCCESS)); } else env(del(alice, brokerKeylet.key)); diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index fa4ed233b4..b2ad47c2b4 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -141,11 +141,7 @@ protected: using namespace jtx; auto const vaultSle = env.le(keylet::vault(vaultID)); - if (!vaultSle) - // This function is not important enough to return an optional. - // Return an impossibly small number - return STAmount::cMinOffset - 1; - return vaultSle->at(sfAssetsTotal).exponent(); + return getVaultScale(vaultSle); } }; @@ -910,12 +906,12 @@ protected: state.loanScale, Number::upward); - auto currentRoundedState = constructRoundedLoanState( + auto currentRoundedState = constructLoanState( state.totalValue, state.principalOutstanding, state.managementFeeOutstanding); { - auto const raw = calculateRawLoanState( + auto const raw = computeRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining, @@ -968,7 +964,7 @@ protected: Number totalFeesPaid = 0; std::size_t totalPaymentsMade = 0; - ripple::LoanState currentTrueState = calculateRawLoanState( + ripple::LoanState currentTrueState = computeRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining, @@ -1023,12 +1019,13 @@ protected: paymentComponents.trackedInterestPart() + paymentComponents.trackedManagementFeeDelta); - ripple::LoanState const nextTrueState = calculateRawLoanState( + ripple::LoanState const nextTrueState = computeRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining - 1, broker.params.managementFeeRate); - detail::LoanDeltas const deltas = currentTrueState - nextTrueState; + detail::LoanStateDeltas const deltas = + currentTrueState - nextTrueState; BEAST_EXPECT( deltas.total() == deltas.principal + deltas.interest + deltas.managementFee); @@ -1639,6 +1636,7 @@ protected: int interestExponent) { using namespace jtx; + using namespace Lending; auto const& asset = broker.asset.raw(); auto const currencyLabel = getCurrencyLabel(asset); @@ -2391,7 +2389,7 @@ protected: // the below BEAST_EXPECTs may not hold across assets. Number const interval = state.paymentInterval; auto const periodicRate = - interval * Number(12, -2) / (365 * 24 * 60 * 60); + interval * Number(12, -2) / secondsInYear; BEAST_EXPECT( periodicRate == Number(2283105022831050, -21, Number::unchecked{})); @@ -2623,7 +2621,7 @@ protected: // the below BEAST_EXPECTs may not hold across assets. Number const interval = state.paymentInterval; auto const periodicRate = - interval * Number(12, -2) / (365 * 24 * 60 * 60); + interval * Number(12, -2) / secondsInYear; BEAST_EXPECT( periodicRate == Number(2283105022831050, -21, Number::unchecked{})); @@ -2664,12 +2662,12 @@ protected: Number::upward)); { - auto const raw = calculateRawLoanState( + auto const raw = computeRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining, broker.params.managementFeeRate); - auto const rounded = constructRoundedLoanState( + auto const rounded = constructLoanState( state.totalValue, state.principalOutstanding, state.managementFeeOutstanding); @@ -2707,7 +2705,7 @@ protected: Number totalInterestPaid = 0; std::size_t totalPaymentsMade = 0; - ripple::LoanState currentTrueState = calculateRawLoanState( + ripple::LoanState currentTrueState = computeRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining, @@ -2732,13 +2730,12 @@ protected: paymentComponents.trackedValueDelta <= roundedPeriodicPayment); - ripple::LoanState const nextTrueState = - calculateRawLoanState( - state.periodicPayment, - periodicRate, - state.paymentRemaining - 1, - broker.params.managementFeeRate); - detail::LoanDeltas const deltas = + ripple::LoanState const nextTrueState = computeRawLoanState( + state.periodicPayment, + periodicRate, + state.paymentRemaining - 1, + broker.params.managementFeeRate); + detail::LoanStateDeltas const deltas = currentTrueState - nextTrueState; testcase @@ -4843,6 +4840,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; + using namespace Lending; Env env(*this, all); Account const issuer{"issuer"}; @@ -5016,6 +5014,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; + using namespace Lending; Env env(*this, all); Account const issuer{"issuer"}; @@ -5099,6 +5098,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; + using namespace Lending; Env env(*this, all); Account const issuer{"issuer"}; @@ -5211,6 +5211,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; + using namespace Lending; Env env(*this, all); Account const issuer{"issuer"}; @@ -5305,6 +5306,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; + using namespace Lending; Env env(*this, all); Account const issuer{"issuer"}; @@ -5841,7 +5843,7 @@ protected: Number const latePaymentFeeRounded = roundToAsset( broker.asset, loanSle->at(sfLatePaymentFee), state.loanScale); - auto const roundedLoanState = constructRoundedLoanState( + auto const roundedLoanState = constructLoanState( state.totalValue, state.principalOutstanding, state.managementFeeOutstanding); @@ -5849,7 +5851,7 @@ protected: auto const periodicRate = loanPeriodicRate(interestRateValue, state.paymentInterval); - auto const rawLoanState = calculateRawLoanState( + auto const rawLoanState = computeRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining, @@ -5859,7 +5861,7 @@ protected: auto const startDateSeconds = static_cast( state.startDate.time_since_epoch().count()); - Number const fullPaymentInterest = calculateFullPaymentInterest( + Number const fullPaymentInterest = computeFullPaymentInterest( rawLoanState.principalOutstanding, periodicRate, parentCloseTime, @@ -6141,7 +6143,7 @@ protected: loanPeriodicRate(after.interestRate, after.paymentInterval); // Accrued + prepayment-penalty interest based on current periodic // schedule - auto const fullPaymentInterest = calculateFullPaymentInterest( + auto const fullPaymentInterest = computeFullPaymentInterest( after.periodicPayment, periodicRate2, after.paymentRemaining, @@ -6177,7 +6179,7 @@ protected: // Reference (clamped) computation: emulate a non-negative accrual // window by clamping prevPaymentDate to 'now' for the full-pay path. auto const prevClamped = std::min(after.previousPaymentDate, nowSecs); - auto const fullPaymentInterestClamped = calculateFullPaymentInterest( + auto const fullPaymentInterestClamped = computeFullPaymentInterest( after.periodicPayment, periodicRate2, after.paymentRemaining, diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 12ac309acb..5f57322be1 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -5720,7 +5720,6 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // Close the ledger until the ledger sequence is large enough to delete // the account (no longer within ) - // This is enforced by the featureDeletableAccounts amendment auto incLgrSeqForAcctDel = [&](Env& env, Account const& acct) { int const delta = [&]() -> int { if (env.seq(acct) + 255 > openLedgerSeq(env)) @@ -5839,8 +5838,6 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } env.close(); - // Increment ledger sequence to the number that is - // enforced by the featureDeletableAccounts amendment incLgrSeqForAcctDel(env, alice); // Verify that alice's account root is present. @@ -5943,8 +5940,6 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ticketCount(env, alice) == 0); - // Increment ledger sequence to the number that is - // enforced by the featureDeletableAccounts amendment incLgrSeqForAcctDel(env, alice); // Verify that alice's account root is present. @@ -6053,8 +6048,6 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ticketCount(env, minter) == 0); - // Increment ledger sequence to the number that is - // enforced by the featureDeletableAccounts amendment incLgrSeqForAcctDel(env, alice); // Verify that alice's account root is present. diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 4b7f156738..8e78969397 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -4330,7 +4330,7 @@ public: Account const ellie("ellie"); Account const fiona("fiona"); - constexpr int ledgersInQueue = 10; + constexpr int ledgersInQueue = 20; auto cfg = makeConfig( {{"minimum_txn_in_ledger_standalone", "1"}, {"ledgers_in_queue", std::to_string(ledgersInQueue)}, diff --git a/src/test/app/tx/apply_test.cpp b/src/test/app/tx/apply_test.cpp index 236905cefd..308ad3d3b8 100644 --- a/src/test/app/tx/apply_test.cpp +++ b/src/test/app/tx/apply_test.cpp @@ -35,23 +35,6 @@ public: SerialIter sitTrans(makeSlice(*ret)); STTx const tx = *std::make_shared(std::ref(sitTrans)); - { - test::jtx::Env no_fully_canonical( - *this, - test::jtx::testable_amendments() - - featureRequireFullyCanonicalSig); - - Validity valid = checkValidity( - no_fully_canonical.app().getHashRouter(), - tx, - no_fully_canonical.current()->rules(), - no_fully_canonical.app().config()) - .first; - - if (valid != Validity::Valid) - fail("Non-Fully canonical signature was not permitted"); - } - { test::jtx::Env fully_canonical( *this, test::jtx::testable_amendments()); diff --git a/src/test/ledger/View_test.cpp b/src/test/ledger/View_test.cpp index b7dbf4ec9e..ab978fda59 100644 --- a/src/test/ledger/View_test.cpp +++ b/src/test/ledger/View_test.cpp @@ -1114,10 +1114,10 @@ class GetAmendments_test : public beast::unit_test::suite break; } - // There should be at least 5 amendments. Don't do exact comparison + // There should be at least 3 amendments. Don't do exact comparison // to avoid maintenance as more amendments are added in the future. BEAST_EXPECT(i == 254); - BEAST_EXPECT(majorities.size() >= 5); + BEAST_EXPECT(majorities.size() >= 3); // None of the amendments should be enabled yet. auto enableds = getEnabledAmendments(*env.closed()); @@ -1135,7 +1135,7 @@ class GetAmendments_test : public beast::unit_test::suite break; } BEAST_EXPECT(i == 255); - BEAST_EXPECT(enableds.size() >= 5); + BEAST_EXPECT(enableds.size() >= 3); } void diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 8a434486ea..50cd8c511f 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -1615,8 +1615,7 @@ public: BEAST_EXPECT(!defaultRules.enabled(featureAMM)); unexpected( - !j.checkSign(STTx::RequireFullyCanonicalSig::yes, defaultRules), - "Transaction fails signature test"); + !j.checkSign(defaultRules), "Transaction fails signature test"); Serializer rawTxn; j.add(rawTxn); diff --git a/src/test/protocol/SecretKey_test.cpp b/src/test/protocol/SecretKey_test.cpp index bd7e2ccfe4..803988e9d5 100644 --- a/src/test/protocol/SecretKey_test.cpp +++ b/src/test/protocol/SecretKey_test.cpp @@ -145,7 +145,7 @@ public: auto sig = sign(pk, sk, makeSlice(data)); BEAST_EXPECT(sig.size() != 0); - BEAST_EXPECT(verify(pk, makeSlice(data), sig, true)); + BEAST_EXPECT(verify(pk, makeSlice(data), sig)); // Construct wrong data: auto badData = data; @@ -156,17 +156,17 @@ public: std::max_element(badData.begin(), badData.end())); // Wrong data: should fail - BEAST_EXPECT(!verify(pk, makeSlice(badData), sig, true)); + BEAST_EXPECT(!verify(pk, makeSlice(badData), sig)); // Slightly change the signature: if (auto ptr = sig.data()) ptr[j % sig.size()]++; // Wrong signature: should fail - BEAST_EXPECT(!verify(pk, makeSlice(data), sig, true)); + BEAST_EXPECT(!verify(pk, makeSlice(data), sig)); // Wrong data and signature: should fail - BEAST_EXPECT(!verify(pk, makeSlice(badData), sig, true)); + BEAST_EXPECT(!verify(pk, makeSlice(badData), sig)); } } } diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 10f82ac88d..e01f7566ac 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -124,8 +124,7 @@ class Feature_test : public beast::unit_test::suite featureToName(fixRemoveNFTokenAutoTrustLine) == "fixRemoveNFTokenAutoTrustLine"); BEAST_EXPECT(featureToName(featureFlow) == "Flow"); - BEAST_EXPECT( - featureToName(featureDeletableAccounts) == "DeletableAccounts"); + BEAST_EXPECT(featureToName(featureDID) == "DID"); BEAST_EXPECT( featureToName(fixIncludeKeyletFields) == "fixIncludeKeyletFields"); BEAST_EXPECT(featureToName(featureTokenEscrow) == "TokenEscrow"); @@ -184,16 +183,16 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this}; - auto jrr = env.rpc("feature", "RequireFullyCanonicalSig")[jss::result]; + auto jrr = env.rpc("feature", "Flow")[jss::result]; BEAST_EXPECTS(jrr[jss::status] == jss::success, "status"); jrr.removeMember(jss::status); BEAST_EXPECT(jrr.size() == 1); BEAST_EXPECT( - jrr.isMember("00C1FC4A53E60AB02C864641002B3172F38677E29C26C54066851" - "79B37E1EDAC")); + jrr.isMember("740352F2412A9909880C23A559FCECEDA3BE2126FED62FC7660D6" + "28A06927F11")); auto feature = *(jrr.begin()); - BEAST_EXPECTS(feature[jss::name] == "RequireFullyCanonicalSig", "name"); + BEAST_EXPECTS(feature[jss::name] == "Flow", "name"); BEAST_EXPECTS(!feature[jss::enabled].asBool(), "enabled"); BEAST_EXPECTS( feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), @@ -201,7 +200,7 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS(feature[jss::supported].asBool(), "supported"); // feature names are case-sensitive - expect error here - jrr = env.rpc("feature", "requireFullyCanonicalSig")[jss::result]; + jrr = env.rpc("feature", "flow")[jss::result]; BEAST_EXPECT(jrr[jss::error] == "badFeature"); BEAST_EXPECT(jrr[jss::error_message] == "Feature unknown or invalid."); } @@ -420,9 +419,9 @@ class Feature_test : public beast::unit_test::suite break; } - // There should be at least 5 amendments. Don't do exact comparison + // There should be at least 3 amendments. Don't do exact comparison // to avoid maintenance as more amendments are added in the future. - BEAST_EXPECT(majorities.size() >= 5); + BEAST_EXPECT(majorities.size() >= 3); std::map const& votes = ripple::detail::supportedAmendments(); @@ -477,8 +476,8 @@ class Feature_test : public beast::unit_test::suite testcase("Veto"); using namespace test::jtx; - Env env{*this, FeatureBitset{featureRequireFullyCanonicalSig}}; - constexpr char const* featureName = "RequireFullyCanonicalSig"; + Env env{*this, FeatureBitset{featureFlow}}; + constexpr char const* featureName = "Flow"; auto jrr = env.rpc("feature", featureName)[jss::result]; if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) @@ -529,7 +528,22 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this}; - constexpr char const* featureName = "CryptoConditionsSuite"; + + auto const& supportedAmendments = detail::supportedAmendments(); + auto obsoleteFeature = std::find_if( + std::begin(supportedAmendments), + std::end(supportedAmendments), + [](auto const& pair) { + return pair.second == VoteBehavior::Obsolete; + }); + + if (obsoleteFeature == std::end(supportedAmendments)) + { + pass(); + return; + } + + auto const featureName = obsoleteFeature->first; auto jrr = env.rpc("feature", featureName)[jss::result]; if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) diff --git a/src/test/rpc/LedgerClosed_test.cpp b/src/test/rpc/LedgerClosed_test.cpp index b3a5e60afd..ba25a115c3 100644 --- a/src/test/rpc/LedgerClosed_test.cpp +++ b/src/test/rpc/LedgerClosed_test.cpp @@ -38,7 +38,7 @@ public: lc_result = env.rpc("ledger_closed")[jss::result]; BEAST_EXPECT( lc_result[jss::ledger_hash] == - "E86DE7F3D7A4D9CE17EF7C8BA08A8F4D8F643B9552F0D895A31CDA78F541DE4E"); + "0F1A9E0C109ADEF6DA2BDE19217C12BBEC57174CBDBD212B0EBDC1CEDB853185"); BEAST_EXPECT(lc_result[jss::ledger_index] == 3); } diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 2d9daa5d9d..57a20527d0 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -307,8 +307,8 @@ class LedgerRPC_test : public beast::unit_test::suite { std::string const hash3{ - "E86DE7F3D7A4D9CE17EF7C8BA08A8F4D" - "8F643B9552F0D895A31CDA78F541DE4E"}; + "0F1A9E0C109ADEF6DA2BDE19217C12BBEC57174CBDBD212B0EBDC1CEDB8531" + "85"}; // access via the ledger_hash field Json::Value jvParams; jvParams[jss::ledger_hash] = hash3; diff --git a/src/test/rpc/LedgerRequestRPC_test.cpp b/src/test/rpc/LedgerRequestRPC_test.cpp index 5917dcedf2..0751b120aa 100644 --- a/src/test/rpc/LedgerRequestRPC_test.cpp +++ b/src/test/rpc/LedgerRequestRPC_test.cpp @@ -200,7 +200,7 @@ public: result = env.rpc("ledger_request", "3")[jss::result]; constexpr char const* hash3 = - "8D631B20BC989AF568FBA97375290544B0703A5ADC1CF9E9053580461690C9EE"; + "9FFD8AE09190D5002FE4252A1B29EABCF40DABBCE3B42619C6BD0BE381D51103"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "3"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "99999999999999980"); @@ -209,14 +209,14 @@ public: BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash2); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "BC9EF2A16BFF80BCFABA6FA84688D858D33BD0FA0435CAA9DF6DA4105A39A29E"); + "35738B8517F37D08983AF6BC7DA483CCA9CF6B41B1FECB31A20028D78FE0BB22"); BEAST_EXPECT( result[jss::ledger][jss::transaction_hash] == - "0213EC486C058B3942FBE3DAC6839949A5C5B02B8B4244C8998EFDF04DBD8222"); + "CBD7F0948EBFA2241DE4EA627939A0FFEE6B80A90FE09C42C825DA546E9B73FF"); result = env.rpc("ledger_request", "4")[jss::result]; constexpr char const* hash4 = - "1A8E7098B23597E73094DADA58C9D62F3AB93A12C6F7666D56CA85A6CFDE530F"; + "7C9B614445517B8C6477E0AB10A35FFC1A23A34FEA41A91ECBDE884CC097C6E1"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "4"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "99999999999999960"); @@ -225,14 +225,14 @@ public: BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash3); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "C690188F123C91355ADA8BDF4AC5B5C927076D3590C215096868A5255264C6DD"); + "1EE701DD2A150205173E1EDE8D474DF6803EC95253DAAEE965B9D896CFC32A04"); BEAST_EXPECT( result[jss::ledger][jss::transaction_hash] == - "3CBDB8F42E04333E1642166BFB93AC9A7E1C6C067092CD5D881D6F3AB3D67E76"); + "9BBDDBF926100DFFF364E16268F544B19F5B9BC6ECCBBC104F98D13FA9F3BC35"); result = env.rpc("ledger_request", "5")[jss::result]; constexpr char const* hash5 = - "C6A222D71AE65D7B4F240009EAD5DEB20D7EEDE5A4064F28BBDBFEEB6FBE48E5"; + "98885D02145CCE4AD2605F1809F17188DB2053B14ED399CAC985DD8E03DCA8C0"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "5"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "99999999999999940"); @@ -241,10 +241,10 @@ public: BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash4); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "EA81CD9D36740736F00CB747E0D0E32D3C10B695823D961F0FB9A1CE7133DD4D"); + "41D64D64796468DEA7AE2A7282C0BB525D6FD7ABC29453C5E5BC6406E947CBCE"); BEAST_EXPECT( result[jss::ledger][jss::transaction_hash] == - "C3D086CD6BDB9E97AD1D513B2C049EF2840BD21D0B3E22D84EBBB89B6D2EF59D"); + "8FE8592EF22FBC2E8C774C7A1ED76AA3FCE64BED17D748CBA9AFDF7072FE36C7"); result = env.rpc("ledger_request", "6")[jss::result]; BEAST_EXPECT(result[jss::error] == "invalidParams"); diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index 28a8ad0829..93dc2d7f66 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -12,6 +12,8 @@ struct PreflightContext; bool checkLendingProtocolDependencies(PreflightContext const& ctx); +static constexpr std::uint32_t secondsInYear = 365 * 24 * 60 * 60; + Number loanPeriodicRate(TenthBips32 interestRate, std::uint32_t paymentInterval); @@ -102,6 +104,31 @@ struct LoanState } }; +// Some values get re-rounded to the vault scale any time they are adjusted. In +// addition, they are prevented from ever going below zero. This helps avoid +// accumulated rounding errors and leftover dust amounts. +template +void +adjustImpreciseNumber( + NumberProxy value, + Number const& adjustment, + Asset const& asset, + int vaultScale) +{ + value = roundToAsset(asset, value + adjustment, vaultScale); + + if (*value < beast::zero) + value = 0; +} + +inline int +getVaultScale(SLE::const_ref vaultSle) +{ + if (!vaultSle) + return Number::minExponent - 1; // LCOV_EXCL_LINE + return vaultSle->at(sfAssetsTotal).exponent(); +} + TER checkLoanGuards( Asset const& vaultAsset, @@ -112,26 +139,29 @@ checkLoanGuards( beast::Journal j); LoanState -calculateRawLoanState( +computeRawLoanState( Number const& periodicPayment, Number const& periodicRate, std::uint32_t const paymentRemaining, TenthBips32 const managementFeeRate); LoanState -calculateRawLoanState( +computeRawLoanState( Number const& periodicPayment, TenthBips32 interestRate, std::uint32_t paymentInterval, std::uint32_t const paymentRemaining, TenthBips32 const managementFeeRate); +// Constructs a valid LoanState object from arbitrary inputs LoanState -constructRoundedLoanState( +constructLoanState( Number const& totalValueOutstanding, Number const& principalOutstanding, Number const& managementFeeOutstanding); +// Constructs a valid LoanState object from a Loan object, which always has +// rounded values LoanState constructRoundedLoanState(SLE::const_ref loan); @@ -143,7 +173,7 @@ computeManagementFee( std::int32_t scale); Number -calculateFullPaymentInterest( +computeFullPaymentInterest( Number const& rawPrincipalOutstanding, Number const& periodicRate, NetClock::time_point parentCloseTime, @@ -153,7 +183,7 @@ calculateFullPaymentInterest( TenthBips32 closeInterestRate); Number -calculateFullPaymentInterest( +computeFullPaymentInterest( Number const& periodicPayment, Number const& periodicRate, std::uint32_t paymentRemaining, @@ -190,7 +220,10 @@ struct PaymentComponents trackedInterestPart() const; }; -struct LoanDeltas +// This structure describes the difference between two LoanState objects so that +// the differences between components don't have to be tracked individually, +// risking more errors. How that difference is used depends on the context. +struct LoanStateDeltas { Number principal; Number interest; @@ -220,14 +253,14 @@ computePaymentComponents( } // namespace detail -detail::LoanDeltas +detail::LoanStateDeltas operator-(LoanState const& lhs, LoanState const& rhs); LoanState -operator-(LoanState const& lhs, detail::LoanDeltas const& rhs); +operator-(LoanState const& lhs, detail::LoanStateDeltas const& rhs); LoanState -operator+(LoanState const& lhs, detail::LoanDeltas const& rhs); +operator+(LoanState const& lhs, detail::LoanStateDeltas const& rhs); LoanProperties computeLoanProperties( diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index e5eac77c1b..8ba49c787c 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -54,7 +54,7 @@ loanPeriodicRate(TenthBips32 interestRate, std::uint32_t paymentInterval) * other places. */ return tenthBipsOfValue(Number(paymentInterval), interestRate) / - (365 * 24 * 60 * 60); + secondsInYear; } bool @@ -179,13 +179,14 @@ loanLatePaymentInterest( * The spec is to be updated to base the duration on the next due date */ + auto const now = parentCloseTime.time_since_epoch().count(); + // If the payment is not late by any amount of time, then there's no late // interest - if (parentCloseTime.time_since_epoch().count() <= nextPaymentDueDate) + if (now <= nextPaymentDueDate) return 0; - auto const secondsOverdue = - parentCloseTime.time_since_epoch().count() - nextPaymentDueDate; + auto const secondsOverdue = now - nextPaymentDueDate; auto const rate = loanPeriodicRate(lateInterestRate, secondsOverdue); @@ -205,15 +206,18 @@ loanAccruedInterest( * This formula is from the XLS-66 spec, section 3.2.4.1.4 (Early Full * Repayment), specifically "accruedInterest = ...". */ + if (periodicRate == beast::zero) + return numZero; + auto const lastPaymentDate = std::max(prevPaymentDate, startDate); + auto const now = parentCloseTime.time_since_epoch().count(); // If the loan has been paid ahead, then "lastPaymentDate" is in the future, // and no interest has accrued. - if (parentCloseTime.time_since_epoch().count() <= lastPaymentDate) - return 0; + if (now <= lastPaymentDate) + return numZero; - auto const secondsSinceLastPayment = - parentCloseTime.time_since_epoch().count() - lastPaymentDate; + auto const secondsSinceLastPayment = now - lastPaymentDate; return principalOutstanding * periodicRate * secondsSinceLastPayment / paymentInterval; @@ -367,9 +371,9 @@ tryOverpayment( TenthBips16 const managementFeeRate, beast::Journal j) { - auto const raw = calculateRawLoanState( + auto const raw = computeRawLoanState( periodicPayment, periodicRate, paymentRemaining, managementFeeRate); - auto const rounded = constructRoundedLoanState( + auto const rounded = constructLoanState( totalValueOutstanding, principalOutstanding, managementFeeOutstanding); auto const errors = rounded - raw; @@ -394,7 +398,7 @@ tryOverpayment( << ", first payment principal: " << newLoanProperties.firstPaymentPrincipal; - auto const newRaw = calculateRawLoanState( + auto const newRaw = computeRawLoanState( newLoanProperties.periodicPayment, periodicRate, paymentRemaining, @@ -423,7 +427,7 @@ tryOverpayment( numZero, rounded.managementFeeDue); - auto const newRounded = constructRoundedLoanState( + auto const newRounded = constructLoanState( totalValueOutstanding, principalOutstanding, managementFeeOutstanding); newLoanProperties.totalValueOutstanding = newRounded.valueOutstanding; @@ -751,8 +755,7 @@ computeFullPayment( if (paymentRemaining <= 1) { // If this is the last payment, it has to be a regular payment - JLOG(j.warn()) << "Full payment requested when only final " - << "payment remains."; + JLOG(j.warn()) << "Last payment cannot be a full payment."; return Unexpected(tecKILLED); } @@ -761,7 +764,7 @@ computeFullPayment( // Full payment interest consists of accrued normal interest and the // prepayment penalty, as computed by 3.2.4.1.4. - auto const fullPaymentInterest = calculateFullPaymentInterest( + auto const fullPaymentInterest = computeFullPaymentInterest( rawPrincipalOutstanding, periodicRate, view.parentCloseTime(), @@ -771,8 +774,8 @@ computeFullPayment( closeInterestRate); auto const [roundedFullInterest, roundedFullManagementFee] = [&]() { - auto const interest = - roundToAsset(asset, fullPaymentInterest, loanScale); + auto const interest = roundToAsset( + asset, fullPaymentInterest, loanScale, Number::downward); auto const parts = computeInterestAndFeeParts( asset, interest, managementFeeRate, loanScale); // Apply as much of the fee to the outstanding fee, but no @@ -832,7 +835,7 @@ PaymentComponents::trackedInterestPart() const } void -LoanDeltas::nonNegative() +LoanStateDeltas::nonNegative() { if (principal < beast::zero) principal = numZero; @@ -871,7 +874,19 @@ computePaymentComponents( auto const roundedPeriodicPayment = roundPeriodicPayment(asset, periodicPayment, scale); - LoanState const trueTarget = calculateRawLoanState( + if (paymentRemaining == 1 || + totalValueOutstanding <= roundedPeriodicPayment) + { + // If there's only one payment left, we need to pay off each of the loan + // parts. + return PaymentComponents{ + .trackedValueDelta = totalValueOutstanding, + .trackedPrincipalDelta = principalOutstanding, + .trackedManagementFeeDelta = managementFeeOutstanding, + .specialCase = PaymentSpecialCase::final}; + } + + LoanState const trueTarget = computeRawLoanState( periodicPayment, periodicRate, paymentRemaining - 1, managementFeeRate); LoanState const roundedTarget = LoanState{ .valueOutstanding = @@ -881,10 +896,10 @@ computePaymentComponents( .interestDue = roundToAsset(asset, trueTarget.interestDue, scale), .managementFeeDue = roundToAsset(asset, trueTarget.managementFeeDue, scale)}; - LoanState const currentLedgerState = constructRoundedLoanState( + LoanState const currentLedgerState = constructLoanState( totalValueOutstanding, principalOutstanding, managementFeeOutstanding); - LoanDeltas deltas = currentLedgerState - roundedTarget; + LoanStateDeltas deltas = currentLedgerState - roundedTarget; deltas.nonNegative(); // Adjust the deltas if necessary for data integrity @@ -916,33 +931,6 @@ computePaymentComponents( roundedPeriodicPayment - (deltas.principal + deltas.interest), currentLedgerState.managementFeeDue}); - if (paymentRemaining == 1 || - totalValueOutstanding <= roundedPeriodicPayment) - { - // If there's only one payment left, we need to pay off each of the loan - // parts. - - XRPL_ASSERT_PARTS( - deltas.total() <= totalValueOutstanding, - "ripple::detail::computePaymentComponents", - "last payment total value agrees"); - XRPL_ASSERT_PARTS( - deltas.principal <= principalOutstanding, - "ripple::detail::computePaymentComponents", - "last payment principal agrees"); - XRPL_ASSERT_PARTS( - deltas.managementFee <= managementFeeOutstanding, - "ripple::detail::computePaymentComponents", - "last payment management fee agrees"); - - // Pay everything off - return PaymentComponents{ - .trackedValueDelta = totalValueOutstanding, - .trackedPrincipalDelta = principalOutstanding, - .trackedManagementFeeDelta = managementFeeOutstanding, - .specialCase = PaymentSpecialCase::final}; - } - // The shortage must never be negative, which indicates that the parts are // trying to take more than the whole payment. The excess can be positive, // which indicates that we're not going to take the whole payment amount, @@ -963,7 +951,7 @@ computePaymentComponents( "ripple::detail::computePaymentComponents", "excess non-negative"); }; - auto addressExcess = [&takeFrom](LoanDeltas& deltas, Number& excess) { + auto addressExcess = [&takeFrom](LoanStateDeltas& deltas, Number& excess) { // This order is based on where errors are the least problematic takeFrom(deltas.interest, excess); takeFrom(deltas.managementFee, excess); @@ -1059,7 +1047,7 @@ computeOverpaymentComponents( "ripple::detail::computeOverpaymentComponents : valid overpayment " "amount"); - Number const fee = roundToAsset( + Number const overpaymentFee = roundToAsset( asset, tenthBipsOfValue(overpayment, overpaymentFeeRate), loanScale); auto const [rawOverpaymentInterest, _] = [&]() { @@ -1077,12 +1065,12 @@ computeOverpaymentComponents( auto const result = detail::ExtendedPaymentComponents{ detail::PaymentComponents{ - .trackedValueDelta = overpayment - fee, + .trackedValueDelta = overpayment - overpaymentFee, .trackedPrincipalDelta = overpayment - roundedOverpaymentInterest - - roundedOverpaymentManagementFee - fee, + roundedOverpaymentManagementFee - overpaymentFee, .trackedManagementFeeDelta = roundedOverpaymentManagementFee, .specialCase = detail::PaymentSpecialCase::extra}, - fee, + overpaymentFee, roundedOverpaymentInterest}; XRPL_ASSERT_PARTS( result.trackedInterestPart() == roundedOverpaymentInterest, @@ -1093,10 +1081,10 @@ computeOverpaymentComponents( } // namespace detail -detail::LoanDeltas +detail::LoanStateDeltas operator-(LoanState const& lhs, LoanState const& rhs) { - detail::LoanDeltas result{ + detail::LoanStateDeltas result{ .principal = lhs.principalOutstanding - rhs.principalOutstanding, .interest = lhs.interestDue - rhs.interestDue, .managementFee = lhs.managementFeeDue - rhs.managementFeeDue, @@ -1106,7 +1094,7 @@ operator-(LoanState const& lhs, LoanState const& rhs) } LoanState -operator-(LoanState const& lhs, detail::LoanDeltas const& rhs) +operator-(LoanState const& lhs, detail::LoanStateDeltas const& rhs) { LoanState result{ .valueOutstanding = lhs.valueOutstanding - rhs.total(), @@ -1119,7 +1107,7 @@ operator-(LoanState const& lhs, detail::LoanDeltas const& rhs) } LoanState -operator+(LoanState const& lhs, detail::LoanDeltas const& rhs) +operator+(LoanState const& lhs, detail::LoanStateDeltas const& rhs) { LoanState result{ .valueOutstanding = lhs.valueOutstanding + rhs.total(), @@ -1213,7 +1201,7 @@ checkLoanGuards( } Number -calculateFullPaymentInterest( +computeFullPaymentInterest( Number const& rawPrincipalOutstanding, Number const& periodicRate, NetClock::time_point parentCloseTime, @@ -1235,8 +1223,10 @@ calculateFullPaymentInterest( accruedInterest >= 0, "ripple::detail::computeFullPaymentInterest : valid accrued interest"); - auto const prepaymentPenalty = - tenthBipsOfValue(rawPrincipalOutstanding, closeInterestRate); + auto const prepaymentPenalty = closeInterestRate == beast::zero + ? Number{} + : tenthBipsOfValue(rawPrincipalOutstanding, closeInterestRate); + XRPL_ASSERT( prepaymentPenalty >= 0, "ripple::detail::computeFullPaymentInterest : valid prepayment " @@ -1246,7 +1236,7 @@ calculateFullPaymentInterest( } Number -calculateFullPaymentInterest( +computeFullPaymentInterest( Number const& periodicPayment, Number const& periodicRate, std::uint32_t paymentRemaining, @@ -1260,7 +1250,7 @@ calculateFullPaymentInterest( detail::loanPrincipalFromPeriodicPayment( periodicPayment, periodicRate, paymentRemaining); - return calculateFullPaymentInterest( + return computeFullPaymentInterest( rawPrincipalOutstanding, periodicRate, parentCloseTime, @@ -1271,7 +1261,7 @@ calculateFullPaymentInterest( } LoanState -calculateRawLoanState( +computeRawLoanState( Number const& periodicPayment, Number const& periodicRate, std::uint32_t const paymentRemaining, @@ -1302,14 +1292,14 @@ calculateRawLoanState( }; LoanState -calculateRawLoanState( +computeRawLoanState( Number const& periodicPayment, TenthBips32 interestRate, std::uint32_t paymentInterval, std::uint32_t const paymentRemaining, TenthBips32 const managementFeeRate) { - return calculateRawLoanState( + return computeRawLoanState( periodicPayment, loanPeriodicRate(interestRate, paymentInterval), paymentRemaining, @@ -1317,7 +1307,7 @@ calculateRawLoanState( } LoanState -constructRoundedLoanState( +constructLoanState( Number const& totalValueOutstanding, Number const& principalOutstanding, Number const& managementFeeOutstanding) @@ -1335,7 +1325,7 @@ constructRoundedLoanState( LoanState constructRoundedLoanState(SLE::const_ref loan) { - return constructRoundedLoanState( + return constructLoanState( loan->at(sfTotalValueOutstanding), loan->at(sfPrincipalOutstanding), loan->at(sfManagementFeeOutstanding)); @@ -1422,12 +1412,12 @@ computeLoanProperties( auto const firstPaymentPrincipal = [&]() { // Compute the parts for the first payment. Ensure that the // principal payment will actually change the principal. - auto const startingState = calculateRawLoanState( + auto const startingState = computeRawLoanState( periodicPayment, periodicRate, paymentsRemaining, managementFeeRate); - auto const firstPaymentState = calculateRawLoanState( + auto const firstPaymentState = computeRawLoanState( periodicPayment, periodicRate, paymentsRemaining - 1, @@ -1457,6 +1447,8 @@ loanMakePayment( LoanPaymentType const paymentType, beast::Journal j) { + using namespace Lending; + /* * This function is an implementation of the XLS-66 spec, * section 3.2.4.3 (Transaction Pseudo-code) @@ -1535,7 +1527,7 @@ loanMakePayment( Number const closePaymentFee = roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale); - LoanState const roundedLoanState = constructRoundedLoanState( + LoanState const roundedLoanState = constructLoanState( totalValueOutstandingProxy, principalOutstandingProxy, managementFeeOutstandingProxy); @@ -1661,7 +1653,7 @@ loanMakePayment( Number totalPaid; std::size_t numPayments = 0; - while (amount >= totalPaid + periodic.totalDue && + while ((amount >= (totalPaid + periodic.totalDue)) && paymentRemainingProxy > 0 && numPayments < loanMaximumPaymentsPerTransaction) { diff --git a/src/xrpld/app/rdb/backend/detail/Node.cpp b/src/xrpld/app/rdb/backend/detail/Node.cpp index ed8c5c06aa..04f328390d 100644 --- a/src/xrpld/app/rdb/backend/detail/Node.cpp +++ b/src/xrpld/app/rdb/backend/detail/Node.cpp @@ -171,7 +171,7 @@ getRowsMinMax(soci::session& session, TableType type) bool saveValidatedLedger( DatabaseCon& ldgDB, - DatabaseCon& txnDB, + std::unique_ptr const& txnDB, Application& app, std::shared_ptr const& ledger, bool current) @@ -254,7 +254,15 @@ saveValidatedLedger( if (app.config().useTxTables()) { - auto db = txnDB.checkoutDb(); + if (!txnDB) + { + // LCOV_EXCL_START + JLOG(j.fatal()) << "TxTables db isn't available"; + Throw("TxTables db isn't available"); + // LCOV_EXCL_STOP + } + + auto db = txnDB->checkoutDb(); soci::transaction tr(*db); diff --git a/src/xrpld/app/rdb/backend/detail/Node.h b/src/xrpld/app/rdb/backend/detail/Node.h index 412631571e..2c0fd69445 100644 --- a/src/xrpld/app/rdb/backend/detail/Node.h +++ b/src/xrpld/app/rdb/backend/detail/Node.h @@ -111,7 +111,7 @@ getRowsMinMax(soci::session& session, TableType type); bool saveValidatedLedger( DatabaseCon& ldgDB, - DatabaseCon& txnDB, + std::unique_ptr const& txnDB, Application& app, std::shared_ptr const& ledger, bool current); diff --git a/src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp b/src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp index 4934205bfd..944ed814f5 100644 --- a/src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp +++ b/src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp @@ -392,8 +392,7 @@ SQLiteDatabaseImp::saveValidatedLedger( { if (existsLedger()) { - if (!detail::saveValidatedLedger( - *lgrdb_, *txdb_, app_, ledger, current)) + if (!detail::saveValidatedLedger(*lgrdb_, txdb_, app_, ledger, current)) return false; } diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index 9d2fe4e47d..8bf0c7fe02 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -456,8 +456,7 @@ Batch::preflightSigValidated(PreflightContext const& ctx) } // Check the batch signers signatures. - auto const sigResult = ctx.tx.checkBatchSign( - STTx::RequireFullyCanonicalSig::yes, ctx.rules); + auto const sigResult = ctx.tx.checkBatchSign(ctx.rules); if (!sigResult) { diff --git a/src/xrpld/app/tx/detail/DeleteAccount.cpp b/src/xrpld/app/tx/detail/DeleteAccount.cpp index 0654c8dbce..c9fd0cc75e 100644 --- a/src/xrpld/app/tx/detail/DeleteAccount.cpp +++ b/src/xrpld/app/tx/detail/DeleteAccount.cpp @@ -22,9 +22,6 @@ namespace ripple { bool DeleteAccount::checkExtraFeatures(PreflightContext const& ctx) { - if (!ctx.rules.enabled(featureDeletableAccounts)) - return false; - if (ctx.tx.isFieldPresent(sfCredentialIDs) && !ctx.rules.enabled(featureCredentials)) return false; diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index 5cf90809a2..c22b8145c6 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -153,12 +153,6 @@ EscrowCreate::preflight(PreflightContext const& ctx) << ec.message(); return temMALFORMED; } - - // Conditions other than PrefixSha256 require the - // "CryptoConditionsSuite" amendment: - if (condition->type != Type::preimageSha256 && - !ctx.rules.enabled(featureCryptoConditionsSuite)) - return temDISABLED; } return tesSUCCESS; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index a23b4bcb47..04f28352b8 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -1057,12 +1057,7 @@ ValidNewAccountRoot::finalize( return false; } - std::uint32_t const startingSeq = // - pseudoAccount // - ? 0 // - : view.rules().enabled(featureDeletableAccounts) // - ? view.seq() // - : 1; + std::uint32_t const startingSeq = pseudoAccount ? 0 : view.seq(); if (accountSeq_ != startingSeq) { @@ -2342,9 +2337,30 @@ ValidLoanBroker::visitEntry( std::shared_ptr const& before, std::shared_ptr const& after) { - if (after && after->getType() == ltLOAN_BROKER) + if (after) { - brokers_.emplace_back(before, after); + if (after->getType() == ltLOAN_BROKER) + { + auto& broker = brokers_[after->key()]; + broker.brokerBefore = before; + broker.brokerAfter = after; + } + else if ( + after->getType() == ltACCOUNT_ROOT && + after->isFieldPresent(sfLoanBrokerID)) + { + auto const& loanBrokerID = after->at(sfLoanBrokerID); + // create an entry if one doesn't already exist + brokers_.emplace(loanBrokerID, BrokerInfo{}); + } + else if (after->getType() == ltRIPPLE_STATE) + { + lines_.emplace_back(after); + } + else if (after->getType() == ltMPTOKEN) + { + mpts_.emplace_back(after); + } } } @@ -2403,8 +2419,51 @@ ValidLoanBroker::finalize( // Loan Brokers will not exist on ledger if the Lending Protocol amendment // is not enabled, so there's no need to check it. - for (auto const& [before, after] : brokers_) + for (auto const& line : lines_) { + for (auto const& field : {&sfLowLimit, &sfHighLimit}) + { + auto const account = + view.read(keylet::account(line->at(*field).getIssuer())); + // This Invariant doesn't know about the rules for Trust Lines, so + // if the account is missing, don't treat it as an error. This + // loop is only concerned with finding Broker pseudo-accounts + if (account && account->isFieldPresent(sfLoanBrokerID)) + { + auto const& loanBrokerID = account->at(sfLoanBrokerID); + // create an entry if one doesn't already exist + brokers_.emplace(loanBrokerID, BrokerInfo{}); + } + } + } + for (auto const& mpt : mpts_) + { + auto const account = view.read(keylet::account(mpt->at(sfAccount))); + // This Invariant doesn't know about the rules for MPTokens, so + // if the account is missing, don't treat is as an error. This + // loop is only concerned with finding Broker pseudo-accounts + if (account && account->isFieldPresent(sfLoanBrokerID)) + { + auto const& loanBrokerID = account->at(sfLoanBrokerID); + // create an entry if one doesn't already exist + brokers_.emplace(loanBrokerID, BrokerInfo{}); + } + } + + for (auto const& [brokerID, broker] : brokers_) + { + auto const& after = broker.brokerAfter + ? broker.brokerAfter + : view.read(keylet::loanbroker(brokerID)); + + if (!after) + { + JLOG(j.fatal()) << "Invariant failed: Loan Broker missing"; + return false; + } + + auto const& before = broker.brokerBefore; + // 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` @@ -2438,6 +2497,26 @@ ValidLoanBroker::finalize( << "Invariant failed: Loan Broker cover available is negative"; return false; } + auto const vault = view.read(keylet::vault(after->at(sfVaultID))); + if (!vault) + { + JLOG(j.fatal()) + << "Invariant failed: Loan Broker vault ID is invalid"; + return false; + } + auto const& vaultAsset = vault->at(sfAsset); + if (after->at(sfCoverAvailable) < accountHolds( + view, + after->at(sfAccount), + vaultAsset, + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j)) + { + JLOG(j.fatal()) << "Invariant failed: Loan Broker cover available " + "is less than pseudo-account asset balance"; + return false; + } } return true; } @@ -2470,10 +2549,25 @@ ValidLoan::finalize( for (auto const& [before, after] : loans_) { // https://github.com/Tapanito/XRPL-Standards/blob/xls-66-lending-protocol/XLS-0066d-lending-protocol/README.md#3223-invariants - // If `Loan.PaymentRemaining = 0` then `Loan.PrincipalOutstanding = 0` + // If `Loan.PaymentRemaining = 0` then the loan MUST be fully paid off if (after->at(sfPaymentRemaining) == 0 && - after->at(sfPrincipalOutstanding) != 0) + (after->at(sfTotalValueOutstanding) != beast::zero || + after->at(sfPrincipalOutstanding) != beast::zero || + after->at(sfManagementFeeOutstanding) != beast::zero)) { + JLOG(j.fatal()) << "Invariant failed: Loan with zero payments " + "remaining has not been paid off"; + return false; + } + // If `Loan.PaymentRemaining != 0` then the loan MUST NOT be fully paid + // off + if (after->at(sfPaymentRemaining) != 0 && + after->at(sfTotalValueOutstanding) == beast::zero && + after->at(sfPrincipalOutstanding) == beast::zero && + after->at(sfManagementFeeOutstanding) == beast::zero) + { + JLOG(j.fatal()) << "Invariant failed: Loan with zero payments " + "remaining has not been paid off"; return false; } if (before && diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 4f81141300..ffe8e4ca5e 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -763,9 +763,25 @@ public: */ class ValidLoanBroker { - // Pair is . After is used for most of the checks, except - // those that check changed values. - std::vector> brokers_; + // Not all of these elements will necessarily be populated. Remaining items + // will be looked up as needed. + struct BrokerInfo + { + SLE::const_pointer brokerBefore = nullptr; + // After is used for most of the checks, except + // those that check changed values. + SLE::const_pointer brokerAfter = nullptr; + }; + // Collect all the LoanBrokers found directly or indirectly through + // pseudo-accounts. Key is the brokerID / index. It will be used to find the + // LoanBroker object if brokerBefore and brokerAfter are nullptr + std::map brokers_; + // Collect all the modified trust lines. Their high and low accounts will be + // loaded to look for LoanBroker pseudo-accounts. + std::vector lines_; + // Collect all the modified MPTokens. Their accounts will be loaded to look + // for LoanBroker pseudo-accounts. + std::vector mpts_; bool goodZeroDirectory( diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp index f31520e42b..8a1d8eb8f9 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp @@ -226,7 +226,13 @@ LoanBrokerCoverClawback::preclaim(PreclaimContext const& ctx) auto const vault = ctx.view.read(keylet::vault(sleBroker->at(sfVaultID))); if (!vault) - return tecINTERNAL; + { + // LCOV_EXCL_START + JLOG(ctx.j.fatal()) << "Vault is missing for Broker " << brokerID; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } + auto const vaultAsset = vault->at(sfAsset); @@ -269,7 +275,7 @@ LoanBrokerCoverClawback::preclaim(PreclaimContext const& ctx) STAmount const clawAmount = *findClawAmount; // Explicitly check the balance of the trust line / MPT to make sure the - // balance is actually there. It should always match `stCoverAvailable`, so + // balance is actually there. It should always match `sfCoverAvailable`, so // if there isn't, this is an internal error. if (accountHolds( ctx.view, diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp index 974bb36a93..4e9e0e9c05 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp @@ -49,8 +49,10 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) auto const vault = ctx.view.read(keylet::vault(sleBroker->at(sfVaultID))); if (!vault) { + // LCOV_EXCL_START JLOG(ctx.j.fatal()) << "Vault is missing for Broker " << brokerID; return tefBAD_LEDGER; + // LCOV_EXCL_STOP } auto const vaultAsset = vault->at(sfAsset); diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index 58204b70d5..1fd5a1a471 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp @@ -3,6 +3,8 @@ #include #include +#include + namespace ripple { bool @@ -59,7 +61,12 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) } auto const vault = ctx.view.read(keylet::vault(sleBroker->at(sfVaultID))); if (!vault) - return tefBAD_LEDGER; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + JLOG(ctx.j.fatal()) << "Vault is missing for Broker " << brokerID; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } auto const vaultAsset = vault->at(sfAsset); if (amount.asset() != vaultAsset) @@ -150,69 +157,15 @@ LoanBrokerCoverWithdraw::doApply() broker->at(sfCoverAvailable) -= amount; view().update(broker); - // Create trust line or MPToken for the receiving account - if (dstAcct == account_) - { - if (auto const ter = addEmptyHolding( - view(), account_, mPriorBalance, amount.asset(), j_); - !isTesSuccess(ter) && ter != tecDUPLICATE) - return ter; - } - - // Move the funds from the broker's pseudo-account to the dstAcct - if (dstAcct == account_ || amount.native()) - { - // Transfer assets directly from pseudo-account to depositor. - // Because this is either a self-transfer or an XRP payment, there is no - // need to use the payment engine. - return accountSend( - view(), brokerPseudoID, dstAcct, amount, j_, WaiveTransferFee::Yes); - } - - { - // If sending the Cover to a different account, then this is - // effectively a payment. Use the Payment transaction code to call - // the payment engine, though only a subset of the functionality is - // supported in this transaction. e.g. No paths, no partial - // payments. - bool const mptDirect = amount.holds(); - STAmount const maxSourceAmount = - Payment::getMaxSourceAmount(brokerPseudoID, amount); - SLE::pointer sleDst = view().peek(keylet::account(dstAcct)); - if (!sleDst) - return tecINTERNAL; // LCOV_EXCL_LINE - - Payment::RipplePaymentParams paymentParams{ - .ctx = ctx_, - .maxSourceAmount = maxSourceAmount, - .srcAccountID = brokerPseudoID, - .dstAccountID = dstAcct, - .sleDst = sleDst, - .dstAmount = amount, - .paths = STPathSet{}, - .deliverMin = std::nullopt, - .j = j_}; - - TER ret; - if (mptDirect) - { - ret = Payment::makeMPTDirectPayment(paymentParams); - } - else - { - ret = Payment::makeRipplePayment(paymentParams); - } - // Always claim a fee - if (!isTesSuccess(ret) && !isTecClaim(ret)) - { - JLOG(j_.info()) - << "LoanBrokerCoverWithdraw: changing result from " - << transToken(ret) - << " to tecPATH_DRY for IOU payment with Destination"; - return tecPATH_DRY; - } - return ret; - } + return doWithdraw( + view(), + tx, + account_, + dstAcct, + brokerPseudoID, + mPriorBalance, + amount, + j_); } //------------------------------------------------------------------------------ diff --git a/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp b/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp index bae806b45e..f3dd781bb5 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp @@ -33,7 +33,10 @@ LoanBrokerDelete::preclaim(PreclaimContext const& ctx) JLOG(ctx.j.warn()) << "LoanBroker does not exist."; return tecNO_ENTRY; } - if (account != sleBroker->at(sfOwner)) + + auto const brokerOwner = sleBroker->at(sfOwner); + + if (account != brokerOwner) { JLOG(ctx.j.warn()) << "Account is not the owner of the LoanBroker."; return tecNO_PERMISSION; @@ -43,6 +46,54 @@ LoanBrokerDelete::preclaim(PreclaimContext const& ctx) JLOG(ctx.j.warn()) << "LoanBrokerDelete: Owner count is " << ownerCount; return tecHAS_OBLIGATIONS; } + if (auto const debtTotal = sleBroker->at(sfDebtTotal); + debtTotal != beast::zero) + { + // Any remaining debt should have been wiped out by the last Loan + // Delete. This check is purely defensive. + auto const vault = + ctx.view.read(keylet::vault(sleBroker->at(sfVaultID))); + if (!vault) + return tefINTERNAL; // LCOV_EXCL_LINE + auto const asset = vault->at(sfAsset); + auto const scale = getVaultScale(vault); + + auto const rounded = + roundToAsset(asset, debtTotal, scale, Number::towards_zero); + + if (rounded != beast::zero) + { + // LCOV_EXCL_START + JLOG(ctx.j.warn()) << "LoanBrokerDelete: Debt total is " + << debtTotal << ", which rounds to " << rounded; + return tecHAS_OBLIGATIONS; + // LCOV_EXCL_START + } + } + + auto const vault = ctx.view.read(keylet::vault(sleBroker->at(sfVaultID))); + if (!vault) + { + // LCOV_EXCL_START + JLOG(ctx.j.fatal()) << "Vault is missing for Broker " << brokerID; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } + + Asset const asset = vault->at(sfAsset); + + auto const coverAvailable = + STAmount{asset, sleBroker->at(sfCoverAvailable)}; + // If there are assets in the cover, broker will receive them on deletion. + // So we need to check if the broker owner is deep frozen for that asset. + if (coverAvailable > beast::zero) + { + if (auto const ret = checkDeepFrozen(ctx.view, brokerOwner, asset)) + { + JLOG(ctx.j.warn()) << "Broker owner account is frozen."; + return ret; + } + } return tesSUCCESS; } @@ -133,6 +184,8 @@ LoanBrokerDelete::doApply() if (!owner) return tefBAD_LEDGER; // LCOV_EXCL_LINE + // Decreases the owner count by two: one for the LoanBroker object, and + // one for the pseudo-account. adjustOwnerCount(view(), owner, -2, j_); } diff --git a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp index c5b6891cb8..c2e6effd7a 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp @@ -13,6 +13,8 @@ LoanBrokerSet::checkExtraFeatures(PreflightContext const& ctx) NotTEC LoanBrokerSet::preflight(PreflightContext const& ctx) { + using namespace Lending; + auto const& tx = ctx.tx; if (auto const data = tx[~sfData]; data && !data->empty() && !validDataLength(tx[~sfData], maxDataPayloadLength)) @@ -118,7 +120,13 @@ LoanBrokerSet::doApply() // Modify an existing LoanBroker auto broker = view.peek(keylet::loanbroker(*brokerID)); if (!broker) - return tefBAD_LEDGER; // LCOV_EXCL_LINE + { + // This should be impossible + // LCOV_EXCL_START + JLOG(j_.fatal()) << "LoanBroker does not exist."; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } if (auto const data = tx[~sfData]) broker->at(sfData) = *data; @@ -132,20 +140,36 @@ LoanBrokerSet::doApply() // Create a new LoanBroker pointing back to the given Vault auto const vaultID = tx[sfVaultID]; auto const sleVault = view.read(keylet::vault(vaultID)); + if (!sleVault) + { + // This should be impossible + // LCOV_EXCL_START + JLOG(j_.fatal()) << "Vault does not exist."; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } auto const vaultPseudoID = sleVault->at(sfAccount); auto const sequence = tx.getSeqValue(); auto owner = view.peek(keylet::account(account_)); if (!owner) - return tefBAD_LEDGER; // LCOV_EXCL_LINE + { + // This should be impossible + // LCOV_EXCL_START + JLOG(j_.fatal()) << "Account does not exist."; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } auto broker = std::make_shared(keylet::loanbroker(account_, sequence)); if (auto const ter = dirLink(view, account_, broker)) - return ter; + return ter; // LCOV_EXCL_LINE if (auto const ter = dirLink(view, vaultPseudoID, broker, sfVaultNode)) - return ter; + return ter; // LCOV_EXCL_LINE + // Increases the owner count by two: one for the LoanBroker object, and + // one for the pseudo-account. adjustOwnerCount(view, owner, 2, j_); auto const ownerCount = owner->at(sfOwnerCount); if (mPriorBalance < view.fees().accountReserve(ownerCount)) @@ -154,7 +178,7 @@ LoanBrokerSet::doApply() auto maybePseudo = createPseudoAccount(view, broker->key(), sfLoanBrokerID); if (!maybePseudo) - return maybePseudo.error(); + return maybePseudo.error(); // LCOV_EXCL_LINE auto& pseudo = *maybePseudo; auto pseudoId = pseudo->at(sfAccount); @@ -167,6 +191,7 @@ LoanBrokerSet::doApply() broker->at(sfVaultID) = vaultID; broker->at(sfOwner) = account_; broker->at(sfAccount) = pseudoId; + // The LoanSequence indexes loans created by this broker, starting at 1 broker->at(sfLoanSequence) = 1; if (auto const data = tx[~sfData]) broker->at(sfData) = *data; diff --git a/src/xrpld/app/tx/detail/LoanDelete.cpp b/src/xrpld/app/tx/detail/LoanDelete.cpp index 084de2cf3f..87ff4d594b 100644 --- a/src/xrpld/app/tx/detail/LoanDelete.cpp +++ b/src/xrpld/app/tx/detail/LoanDelete.cpp @@ -101,7 +101,27 @@ LoanDelete::doApply() view.erase(loanSle); // Decrement the LoanBroker's owner count. + // The broker's owner count is solely for the number of outstanding loans, + // and is distinct from the broker's pseudo-account's owner count adjustOwnerCount(view, brokerSle, -1, j_); + // If there are no loans left, then any remaining debt must be forgiven, + // because there is no other way to pay it back. + if (brokerSle->at(sfOwnerCount) == 0) + { + auto debtTotalProxy = brokerSle->at(sfDebtTotal); + if (*debtTotalProxy != beast::zero) + { + XRPL_ASSERT_PARTS( + roundToAsset( + vaultSle->at(sfAsset), + debtTotalProxy, + getVaultScale(vaultSle), + Number::towards_zero) == beast::zero, + "ripple::LoanDelete::doApply", + "last loan, remaining debt rounds to zero"); + debtTotalProxy = 0; + } + } // Decrement the borrower's owner count adjustOwnerCount(view, borrowerSle, -1, j_); diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index 73754404e5..adf08d71bf 100644 --- a/src/xrpld/app/tx/detail/LoanManage.cpp +++ b/src/xrpld/app/tx/detail/LoanManage.cpp @@ -114,9 +114,20 @@ LoanManage::preclaim(PreclaimContext const& ctx) return tesSUCCESS; } -Number +static Number owedToVault(SLE::ref loanSle) { + // Spec section 3.2.3.2, defines the default amount as + // + // DefaultAmount = (Loan.PrincipalOutstanding + Loan.InterestOutstanding) + // + // Loan.InterestOutstanding is not stored directly on ledger. + // It is computed as + // + // Loan.TotalValueOutstanding - Loan.PrincipalOutstanding - + // Loan.ManagementFeeOutstanding + // + // Add that to the original formula, and you get this: return loanSle->at(sfTotalValueOutstanding) - loanSle->at(sfManagementFeeOutstanding); } @@ -135,13 +146,6 @@ LoanManage::defaultLoan( std::int32_t const loanScale = loanSle->at(sfLoanScale); auto brokerDebtTotalProxy = brokerSle->at(sfDebtTotal); - auto principalOutstandingProxy = loanSle->at(sfPrincipalOutstanding); - auto managementFeeOutstandingProxy = - loanSle->at(sfManagementFeeOutstanding); - - auto totalValueOutstandingProxy = loanSle->at(sfTotalValueOutstanding); - auto paymentRemainingProxy = loanSle->at(sfPaymentRemaining); - Number const totalDefaultAmount = owedToVault(loanSle); // Apply the First-Loss Capital to the Default Amount @@ -171,16 +175,16 @@ LoanManage::defaultLoan( // Update the Vault object: + // The vault may be at a different scale than the loan. Reduce rounding + // errors during the accounting by rounding some of the values to that + // scale. + auto const vaultScale = getVaultScale(vaultSle); + { // Decrease the Total Value of the Vault: auto vaultTotalProxy = vaultSle->at(sfAssetsTotal); auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable); - // The vault may be at a different scale than the loan. Reduce rounding - // errors during the accounting by rounding some of the values to that - // scale. - auto const vaultScale = vaultTotalProxy.value().exponent(); - if (vaultTotalProxy < vaultDefaultAmount) { // LCOV_EXCL_START @@ -246,16 +250,11 @@ LoanManage::defaultLoan( // Update the LoanBroker object: { + auto const asset = *vaultSle->at(sfAsset); + // Decrease the Debt of the LoanBroker: - if (brokerDebtTotalProxy < totalDefaultAmount) - { - // LCOV_EXCL_START - JLOG(j.warn()) - << "LoanBroker debt total is less than the default amount"; - return tefBAD_LEDGER; - // LCOV_EXCL_STOP - } - brokerDebtTotalProxy -= totalDefaultAmount; + adjustImpreciseNumber( + brokerDebtTotalProxy, -totalDefaultAmount, asset, vaultScale); // Decrease the First-Loss Capital Cover Available: auto coverAvailableProxy = brokerSle->at(sfCoverAvailable); if (coverAvailableProxy < defaultCovered) @@ -272,10 +271,11 @@ LoanManage::defaultLoan( // Update the Loan object: loanSle->setFlag(lsfLoanDefault); - totalValueOutstandingProxy = 0; - paymentRemainingProxy = 0; - principalOutstandingProxy = 0; - managementFeeOutstandingProxy = 0; + + loanSle->at(sfTotalValueOutstanding) = 0; + loanSle->at(sfPaymentRemaining) = 0; + loanSle->at(sfPrincipalOutstanding) = 0; + loanSle->at(sfManagementFeeOutstanding) = 0; // Zero out the next due date. Since it's default, it'll be removed from // the object. loanSle->at(sfNextPaymentDueDate) = 0; diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 39a93f634a..4bc0dc6188 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -7,6 +7,8 @@ #include #include +#include + namespace ripple { bool @@ -32,14 +34,11 @@ LoanPay::preflight(PreflightContext const& ctx) // The loan payment flags are all mutually exclusive. If more than one is // set, the tx is malformed. - int flagsSet = 0; - for (auto const flag : - {tfLoanLatePayment, tfLoanFullPayment, tfLoanOverpayment}) - { - if (ctx.tx.isFlag(flag)) - ++flagsSet; - } - if (flagsSet > 1) + static_assert( + (tfLoanLatePayment | tfLoanFullPayment | tfLoanOverpayment) == + ~(tfLoanPayMask | tfUniversal)); + auto const flagsSet = ctx.tx.getFlags() & ~(tfLoanPayMask | tfUniversal); + if (std::popcount(flagsSet) > 1) { JLOG(ctx.j.warn()) << "Only one LoanPay flag can be set per tx. " << flagsSet << " is too many."; @@ -52,6 +51,8 @@ LoanPay::preflight(PreflightContext const& ctx) XRPAmount LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) { + using namespace Lending; + auto const normalCost = Transactor::calculateBaseFee(view, tx); if (tx.isFlag(tfLoanFullPayment) || tx.isFlag(tfLoanLatePayment)) @@ -210,7 +211,7 @@ LoanPay::preclaim(PreclaimContext const& ctx) // Do not support "partial payments" - if the transaction says to pay X, // then the account must have X available, even if the loan payment takes // less. - if (auto const balance = accountCanSend( + if (auto const balance = accountSpendable( ctx.view, account, asset, @@ -386,6 +387,11 @@ LoanPay::doApply() !asset.integral() || totalPaidToVaultRaw == totalPaidToVaultRounded, "ripple::LoanPay::doApply", "rounding does nothing for integral asset"); + // Account for value changes when reducing the broker's debt: + // - Positive value change (from full/late/overpayments): Subtract from the + // amount credited toward debt to avoid over-reducing the debt. + // - Negative value change (from full/overpayments): Add to the amount + // credited toward debt,effectively increasing the debt reduction. auto const totalPaidToVaultForDebt = totalPaidToVaultRaw - paymentParts->valueChange; @@ -407,11 +413,9 @@ LoanPay::doApply() "totalPaidToVaultForDebt rounding good"); // Despite our best efforts, it's possible for rounding errors to accumulate // in the loan broker's debt total. This is because the broker may have more - // that one loan with significantly different scales. - if (totalPaidToVaultForDebt >= debtTotalProxy) - debtTotalProxy = 0; - else - debtTotalProxy -= totalPaidToVaultForDebt; + // than one loan with significantly different scales. + adjustImpreciseNumber( + debtTotalProxy, -totalPaidToVaultForDebt, asset, vaultScale); //------------------------------------------------------ // Vault object state changes @@ -470,11 +474,11 @@ LoanPay::doApply() } #if !NDEBUG - auto const accountBalanceBefore = accountCanSend( + auto const accountBalanceBefore = accountSpendable( view, account_, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_); auto const vaultBalanceBefore = account_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountCanSend( + : accountSpendable( view, vaultPseudoAccount, asset, @@ -483,7 +487,7 @@ LoanPay::doApply() j_); auto const brokerBalanceBefore = account_ == brokerPayee ? STAmount{asset, 0} - : accountCanSend( + : accountSpendable( view, brokerPayee, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_); #endif @@ -539,11 +543,11 @@ LoanPay::doApply() "vault pseudo balance agrees after"); #if !NDEBUG - auto const accountBalanceAfter = accountCanSend( + auto const accountBalanceAfter = accountSpendable( view, account_, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_); auto const vaultBalanceAfter = account_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountCanSend( + : accountSpendable( view, vaultPseudoAccount, asset, @@ -552,7 +556,7 @@ LoanPay::doApply() j_); auto const brokerBalanceAfter = account_ == brokerPayee ? STAmount{asset, 0} - : accountCanSend( + : accountSpendable( view, brokerPayee, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_); XRPL_ASSERT_PARTS( diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index d9e5f8b981..e73f685bf8 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -21,6 +21,8 @@ LoanSet::getFlagsMask(PreflightContext const& ctx) NotTEC LoanSet::preflight(PreflightContext const& ctx) { + using namespace Lending; + auto const& tx = ctx.tx; // Special case for Batch inner transactions @@ -157,9 +159,13 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx) // for the transaction. Note that unlike the base class, the single signer // is counted if present. It will only be absent in a batch inner // transaction. - std::size_t const signerCount = counterSig.isFieldPresent(sfSigners) - ? counterSig.getFieldArray(sfSigners).size() - : (counterSig.isFieldPresent(sfTxnSignature) ? 1 : 0); + std::size_t const signerCount = [&counterSig]() { + // Compute defensively. Assure that "tx" cannot be accessed and cause + // confusion or miscalculations. + return counterSig.isFieldPresent(sfSigners) + ? counterSig.getFieldArray(sfSigners).size() + : (counterSig.isFieldPresent(sfTxnSignature) ? 1 : 0); + }(); return normalCost + (signerCount * baseFee); } @@ -179,7 +185,7 @@ LoanSet::getValueFields() return valueFields; } -std::uint32_t +static std::uint32_t getStartDate(ReadView const& view) { return view.info().closeTime.time_since_epoch().count(); @@ -213,18 +219,32 @@ LoanSet::preclaim(PreclaimContext const& ctx) // The grace period can't be larger than the interval. Check it first, // mostly so that unit tests can test that specific case. if (grace > timeAvailable) + { + JLOG(ctx.j.warn()) << "Grace period exceeds protocol time limit."; return tecKILLED; + } if (interval > timeAvailable) + { + JLOG(ctx.j.warn()) + << "Payment interval exceeds protocol time limit."; return tecKILLED; + } if (total > timeAvailable) + { + JLOG(ctx.j.warn()) << "Payment total exceeds protocol time limit."; return tecKILLED; + } auto const timeLastPayment = timeAvailable - grace; if (timeLastPayment / interval < total) + { + JLOG(ctx.j.warn()) << "Last payment due date, or grace period for " + "last payment exceeds protocol time limit."; return tecKILLED; + } } auto const account = tx[sfAccount]; @@ -246,6 +266,7 @@ LoanSet::preclaim(PreclaimContext const& ctx) "of the LoanBroker."; return tecNO_PERMISSION; } + auto const brokerPseudo = brokerSle->at(sfAccount); auto const borrower = counterparty == brokerOwner ? account : counterparty; if (auto const borrowerSle = ctx.view.read(keylet::account(borrower)); @@ -292,6 +313,16 @@ LoanSet::preclaim(PreclaimContext const& ctx) JLOG(ctx.j.warn()) << "Vault pseudo-account is frozen."; return ret; } + + // brokerPseudo is the fallback account to receive LoanPay fees, even if the + // broker owner is unable to accept them. Don't create the loan if it is + // deep frozen. + if (auto const ret = checkDeepFrozen(ctx.view, brokerPseudo, asset)) + { + JLOG(ctx.j.warn()) << "Broker pseudo-account is frozen."; + return ret; + } + // borrower is eventually going to have to pay back the loan, so it can't be // frozen now. It is also going to receive funds, so it can't be deep // frozen, but being frozen is a prerequisite for being deep frozen, so @@ -352,7 +383,7 @@ LoanSet::doApply() auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable); auto vaultTotalProxy = vaultSle->at(sfAssetsTotal); - auto const vaultScale = vaultTotalProxy.value().exponent(); + auto const vaultScale = getVaultScale(vaultSle); if (vaultAvailableProxy < principalRequested) { JLOG(j_.warn()) @@ -414,7 +445,7 @@ LoanSet::doApply() // LCOV_EXCL_STOP } - LoanState const state = constructRoundedLoanState( + LoanState const state = constructLoanState( properties.totalValueOutstanding, principalRequested, properties.managementFeeOwedToBroker); @@ -572,11 +603,16 @@ LoanSet::doApply() view.update(vaultSle); // Update the balances in the loan broker - brokerSle->at(sfDebtTotal) += newDebtDelta; + adjustImpreciseNumber( + brokerSle->at(sfDebtTotal), newDebtDelta, vaultAsset, vaultScale); // The broker's owner count is solely for the number of outstanding loans, // and is distinct from the broker's pseudo-account's owner count adjustOwnerCount(view, brokerSle, 1, j_); loanSequenceProxy += 1; + // The sequence should be extremely unlikely to roll over, but fail if it + // does + if (loanSequenceProxy == 0) + return tecMAX_SEQUENCE_REACHED; view.update(brokerSle); // Put the loan into the pseudo-account's directory diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index b52ae78523..ae47036ab6 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -440,7 +440,7 @@ PayChanClaim::preflight(PreflightContext const& ctx) PublicKey const pk(ctx.tx[sfPublicKey]); Serializer msg; serializePayChanAuthorization(msg, k.key, authAmt); - if (!verify(pk, msg.slice(), *sig, /*canonical*/ true)) + if (!verify(pk, msg.slice(), *sig)) return temBAD_SIGNATURE; } diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 9d6a5e5783..56aeb1b0aa 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -29,8 +29,8 @@ Payment::makeTxConsequences(PreflightContext const& ctx) } STAmount -Payment::getMaxSourceAmount( - AccountID const& senderAccount, +getMaxSourceAmount( + AccountID const& account, STAmount const& dstAmount, std::optional const& sendMax) { @@ -40,7 +40,7 @@ Payment::getMaxSourceAmount( return dstAmount; else return STAmount( - Issue{dstAmount.get().currency, senderAccount}, + Issue{dstAmount.get().currency, account}, dstAmount.mantissa(), dstAmount.exponent(), dstAmount < beast::zero); @@ -398,14 +398,10 @@ Payment::doApply() if (!sleDst) { - std::uint32_t const seqno{ - view().rules().enabled(featureDeletableAccounts) ? view().seq() - : 1}; - // Create the account. sleDst = std::make_shared(k); sleDst->setAccountID(sfAccount, dstAccountID); - sleDst->setFieldU32(sfSequence, seqno); + sleDst->setFieldU32(sfSequence, view().seq()); view().insert(sleDst); } @@ -422,35 +418,155 @@ Payment::doApply() if (ripple) { - return makeRipplePayment(RipplePaymentParams{ - .ctx = ctx_, - .maxSourceAmount = maxSourceAmount, - .srcAccountID = account_, - .dstAccountID = dstAccountID, - .sleDst = sleDst, - .dstAmount = dstAmount, - .paths = ctx_.tx.getFieldPathSet(sfPaths), - .deliverMin = deliverMin, - .partialPaymentAllowed = partialPaymentAllowed, - .defaultPathsAllowed = defaultPathsAllowed, - .limitQuality = limitQuality, - .j = j_}); + // Ripple payment with at least one intermediate step and uses + // transitive balances. + + // An account that requires authorization has two ways to get an + // IOU Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination. + + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; + + path::RippleCalc::Input rcInput; + rcInput.partialPaymentAllowed = partialPaymentAllowed; + rcInput.defaultPathsAllowed = defaultPathsAllowed; + rcInput.limitQuality = limitQuality; + rcInput.isLedgerOpen = view().open(); + + path::RippleCalc::Output rc; + { + PaymentSandbox pv(&view()); + JLOG(j_.debug()) << "Entering RippleCalc in payment: " + << ctx_.tx.getTransactionID(); + rc = path::RippleCalc::rippleCalculate( + pv, + maxSourceAmount, + dstAmount, + dstAccountID, + account_, + ctx_.tx.getFieldPathSet(sfPaths), + ctx_.tx[~sfDomainID], + ctx_.app.logs(), + &rcInput); + // VFALCO NOTE We might not need to apply, depending + // on the TER. But always applying *should* + // be safe. + pv.apply(ctx_.rawView()); + } + + // TODO: is this right? If the amount is the correct amount, was + // the delivered amount previously set? + if (rc.result() == tesSUCCESS && rc.actualAmountOut != dstAmount) + { + if (deliverMin && rc.actualAmountOut < *deliverMin) + rc.setResult(tecPATH_PARTIAL); + else + ctx_.deliver(rc.actualAmountOut); + } + + auto terResult = rc.result(); + + // Because of its overhead, if RippleCalc + // fails with a retry code, claim a fee + // instead. Maybe the user will be more + // careful with their path spec next time. + if (isTerRetry(terResult)) + terResult = tecPATH_DRY; + return terResult; } else if (mptDirect) { - return makeMPTDirectPayment(RipplePaymentParams{ - .ctx = ctx_, - .maxSourceAmount = maxSourceAmount, - .srcAccountID = account_, - .dstAccountID = dstAccountID, - .sleDst = sleDst, - .dstAmount = dstAmount, - .paths = ctx_.tx.getFieldPathSet(sfPaths), - .deliverMin = deliverMin, - .partialPaymentAllowed = partialPaymentAllowed, - .defaultPathsAllowed = defaultPathsAllowed, - .limitQuality = limitQuality, - .j = j_}); + JLOG(j_.trace()) << " dstAmount=" << dstAmount.getFullText(); + auto const& mptIssue = dstAmount.get(); + + if (auto const ter = requireAuth(view(), mptIssue, account_); + ter != tesSUCCESS) + return ter; + + if (auto const ter = requireAuth(view(), mptIssue, dstAccountID); + ter != tesSUCCESS) + return ter; + + if (auto const ter = + canTransfer(view(), mptIssue, account_, dstAccountID); + ter != tesSUCCESS) + return ter; + + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; + + auto const& issuer = mptIssue.getIssuer(); + + // Transfer rate + Rate rate{QUALITY_ONE}; + // Payment between the holders + if (account_ != issuer && dstAccountID != issuer) + { + // If globally/individually locked then + // - can't send between holders + // - holder can send back to issuer + // - issuer can send to holder + if (isAnyFrozen(view(), {account_, dstAccountID}, mptIssue)) + return tecLOCKED; + + // Get the rate for a payment between the holders. + rate = transferRate(view(), mptIssue.getMptID()); + } + + // Amount to deliver. + STAmount amountDeliver = dstAmount; + // Factor in the transfer rate. + // No rounding. It'll change once MPT integrated into DEX. + STAmount requiredMaxSourceAmount = multiply(dstAmount, rate); + + // Send more than the account wants to pay or less than + // the account wants to deliver (if no SendMax). + // Adjust the amount to deliver. + if (partialPaymentAllowed && requiredMaxSourceAmount > maxSourceAmount) + { + requiredMaxSourceAmount = maxSourceAmount; + // No rounding. It'll change once MPT integrated into DEX. + amountDeliver = divide(maxSourceAmount, rate); + } + + if (requiredMaxSourceAmount > maxSourceAmount || + (deliverMin && amountDeliver < *deliverMin)) + return tecPATH_PARTIAL; + + PaymentSandbox pv(&view()); + auto res = accountSend( + pv, account_, dstAccountID, amountDeliver, ctx_.journal); + if (res == tesSUCCESS) + { + pv.apply(ctx_.rawView()); + + // If the actual amount delivered is different from the original + // amount due to partial payment or transfer fee, we need to update + // DelieveredAmount using the actual delivered amount + if (view().rules().enabled(fixMPTDeliveredAmount) && + amountDeliver != dstAmount) + ctx_.deliver(amountDeliver); + } + else if (res == tecINSUFFICIENT_FUNDS || res == tecPATH_DRY) + res = tecPATH_PARTIAL; + + return res; } XRPL_ASSERT(dstAmount.native(), "ripple::Payment::doApply : amount is XRP"); @@ -543,199 +659,4 @@ Payment::doApply() return tesSUCCESS; } -// Reusable helpers -TER -Payment::makeRipplePayment(Payment::RipplePaymentParams const& p) -{ - // Set up some copies/references so the code can be moved from - // Payment::doApply otherwise unmodified - // - // Note that some of these variable names use trailing '_', which is - // usually reserved for member variables. After, or just before these - // changes are merged, a follow-up can clean up the names for consistency. - ApplyContext& ctx_ = p.ctx; - STAmount const& maxSourceAmount = p.maxSourceAmount; - AccountID const& account_ = p.srcAccountID; - AccountID const& dstAccountID = p.dstAccountID; - SLE::pointer sleDst = p.sleDst; - STAmount const& dstAmount = p.dstAmount; - STPathSet const& paths = p.paths; - std::optional const& deliverMin = p.deliverMin; - bool partialPaymentAllowed = p.partialPaymentAllowed; - bool defaultPathsAllowed = p.defaultPathsAllowed; - bool limitQuality = p.limitQuality; - beast::Journal j_ = p.j; - - auto view = [&p]() -> ApplyView& { return p.ctx.view(); }; - - // Below this line, copied straight from Payment::doApply - // except `ctx_.tx.getFieldPathSet(sfPaths)` replaced with `paths`, - // because not all transactions have that field available, and using - // it will throw - //------------------------------------------------------- - // Ripple payment with at least one intermediate step and uses - // transitive balances. - - // An account that requires authorization has two ways to get an - // IOU Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination. - - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, dstAccountID, sleDst, ctx_.journal); - !isTesSuccess(err)) - return err; - - path::RippleCalc::Input rcInput; - rcInput.partialPaymentAllowed = partialPaymentAllowed; - rcInput.defaultPathsAllowed = defaultPathsAllowed; - rcInput.limitQuality = limitQuality; - rcInput.isLedgerOpen = view().open(); - - path::RippleCalc::Output rc; - { - PaymentSandbox pv(&view()); - JLOG(j_.debug()) << "Entering RippleCalc in payment: " - << ctx_.tx.getTransactionID(); - rc = path::RippleCalc::rippleCalculate( - pv, - maxSourceAmount, - dstAmount, - dstAccountID, - account_, - paths, - ctx_.tx[~sfDomainID], - ctx_.app.logs(), - &rcInput); - // VFALCO NOTE We might not need to apply, depending - // on the TER. But always applying *should* - // be safe. - pv.apply(ctx_.rawView()); - } - - // TODO: is this right? If the amount is the correct amount, was - // the delivered amount previously set? - if (rc.result() == tesSUCCESS && rc.actualAmountOut != dstAmount) - { - if (deliverMin && rc.actualAmountOut < *deliverMin) - rc.setResult(tecPATH_PARTIAL); - else - ctx_.deliver(rc.actualAmountOut); - } - - auto terResult = rc.result(); - - // Because of its overhead, if RippleCalc - // fails with a retry code, claim a fee - // instead. Maybe the user will be more - // careful with their path spec next time. - if (isTerRetry(terResult)) - terResult = tecPATH_DRY; - return terResult; -} - -TER -Payment::makeMPTDirectPayment(Payment::RipplePaymentParams const& p) -{ - // Set up some copies/references so the code can be moved from - // Payment::doApply otherwise unmodified - // - // Note that some of these variable names use trailing '_', which is - // usually reserved for member variables. After, or just before these - // changes are merged, a follow-up can clean up the names for consistency. - ApplyContext& ctx_ = p.ctx; - STAmount const& maxSourceAmount = p.maxSourceAmount; - AccountID const& account_ = p.srcAccountID; - AccountID const& dstAccountID = p.dstAccountID; - SLE::pointer sleDst = p.sleDst; - STAmount const& dstAmount = p.dstAmount; - // STPathSet const& paths = p.paths; - std::optional const& deliverMin = p.deliverMin; - bool partialPaymentAllowed = p.partialPaymentAllowed; - // bool defaultPathsAllowed = p.defaultPathsAllowed; - // bool limitQuality = p.limitQuality; - beast::Journal j_ = p.j; - - auto view = [&p]() -> ApplyView& { return p.ctx.view(); }; - - // Below this line, copied straight from Payment::doApply - //------------------------------------------------------- - JLOG(j_.trace()) << " dstAmount=" << dstAmount.getFullText(); - auto const& mptIssue = dstAmount.get(); - - if (auto const ter = requireAuth(view(), mptIssue, account_); - ter != tesSUCCESS) - return ter; - - if (auto const ter = requireAuth(view(), mptIssue, dstAccountID); - ter != tesSUCCESS) - return ter; - - if (auto const ter = canTransfer(view(), mptIssue, account_, dstAccountID); - ter != tesSUCCESS) - return ter; - - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, dstAccountID, sleDst, ctx_.journal); - !isTesSuccess(err)) - return err; - - auto const& issuer = mptIssue.getIssuer(); - - // Transfer rate - Rate rate{QUALITY_ONE}; - // Payment between the holders - if (account_ != issuer && dstAccountID != issuer) - { - // If globally/individually locked then - // - can't send between holders - // - holder can send back to issuer - // - issuer can send to holder - if (isAnyFrozen(view(), {account_, dstAccountID}, mptIssue)) - return tecLOCKED; - - // Get the rate for a payment between the holders. - rate = transferRate(view(), mptIssue.getMptID()); - } - - // Amount to deliver. - STAmount amountDeliver = dstAmount; - // Factor in the transfer rate. - // No rounding. It'll change once MPT integrated into DEX. - STAmount requiredMaxSourceAmount = multiply(dstAmount, rate); - - // Send more than the account wants to pay or less than - // the account wants to deliver (if no SendMax). - // Adjust the amount to deliver. - if (partialPaymentAllowed && requiredMaxSourceAmount > maxSourceAmount) - { - requiredMaxSourceAmount = maxSourceAmount; - // No rounding. It'll change once MPT integrated into DEX. - amountDeliver = divide(maxSourceAmount, rate); - } - - if (requiredMaxSourceAmount > maxSourceAmount || - (deliverMin && amountDeliver < *deliverMin)) - return tecPATH_PARTIAL; - - PaymentSandbox pv(&view()); - auto res = - accountSend(pv, account_, dstAccountID, amountDeliver, ctx_.journal); - if (res == tesSUCCESS) - { - pv.apply(ctx_.rawView()); - - // If the actual amount delivered is different from the original - // amount due to partial payment or transfer fee, we need to update - // DelieveredAmount using the actual delivered amount - if (view().rules().enabled(fixMPTDeliveredAmount) && - amountDeliver != dstAmount) - ctx_.deliver(amountDeliver); - } - else if (res == tecINSUFFICIENT_FUNDS || res == tecPATH_DRY) - res = tecPATH_PARTIAL; - - return res; -} - } // namespace ripple diff --git a/src/xrpld/app/tx/detail/Payment.h b/src/xrpld/app/tx/detail/Payment.h index c532b6efb2..f41326ecf6 100644 --- a/src/xrpld/app/tx/detail/Payment.h +++ b/src/xrpld/app/tx/detail/Payment.h @@ -40,37 +40,6 @@ public: TER doApply() override; - - // Helpers for this class and other transactors that make "Payments" - struct RipplePaymentParams - { - ApplyContext& ctx; - STAmount const& maxSourceAmount; - AccountID const& srcAccountID; - AccountID const& dstAccountID; - SLE::pointer sleDst; - STAmount const& dstAmount; - // Paths need to be explicitly included because other transactions don't - // have them defined - STPathSet const& paths; - std::optional const& deliverMin; - bool partialPaymentAllowed = false; - bool defaultPathsAllowed = true; - bool limitQuality = false; - beast::Journal j; - }; - - static STAmount - getMaxSourceAmount( - AccountID const& senderAccount, - STAmount const& dstAmount, - std::optional const& sendMax = {}); - - static TER - makeRipplePayment(RipplePaymentParams const& p); - - static TER - makeMPTDirectPayment(RipplePaymentParams const& p); }; } // namespace ripple diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index de84a372e7..a3bef88d49 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -83,6 +83,8 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (auto const ret = checkFrozen(ctx.view, dstAcct, vaultAsset)) return ret; + // Cannot return shares to the vault, if the underlying asset was frozen for + // the submitter if (auto const ret = checkFrozen(ctx.view, account, vaultShare)) return ret; @@ -237,42 +239,15 @@ VaultWithdraw::doApply() auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_); - // Create trust line or MPToken for the receiving account - if (dstAcct == account_) - { - if (auto const ter = addEmptyHolding( - view(), account_, mPriorBalance, vaultAsset, j_); - !isTesSuccess(ter) && ter != tecDUPLICATE) - return ter; - } - - // Transfer assets from vault to depositor or destination account. - if (auto const ter = accountSend( - view(), - vaultAccount, - dstAcct, - assetsWithdrawn, - j_, - WaiveTransferFee::Yes); - !isTesSuccess(ter)) - return ter; - - // Sanity check - if (accountHolds( - view(), - vaultAccount, - assetsWithdrawn.asset(), - FreezeHandling::fhIGNORE_FREEZE, - AuthHandling::ahIGNORE_AUTH, - j_) < beast::zero) - { - // LCOV_EXCL_START - JLOG(j_.error()) << "VaultWithdraw: negative balance of vault assets."; - return tefINTERNAL; - // LCOV_EXCL_STOP - } - - return tesSUCCESS; + return doWithdraw( + view(), + ctx_.tx, + account_, + dstAcct, + vaultAccount, + mPriorBalance, + assetsWithdrawn, + j_); } } // namespace ripple diff --git a/src/xrpld/app/tx/detail/XChainBridge.cpp b/src/xrpld/app/tx/detail/XChainBridge.cpp index 554d4a8436..e555dca0a1 100644 --- a/src/xrpld/app/tx/detail/XChainBridge.cpp +++ b/src/xrpld/app/tx/detail/XChainBridge.cpp @@ -464,12 +464,9 @@ transferHelper( } // Create the account. - std::uint32_t const seqno{ - psb.rules().enabled(featureDeletableAccounts) ? psb.seq() : 1}; - sleDst = std::make_shared(dstK); sleDst->setAccountID(sfAccount, dst); - sleDst->setFieldU32(sfSequence, seqno); + sleDst->setFieldU32(sfSequence, psb.seq()); psb.insert(sleDst); } diff --git a/src/xrpld/app/tx/detail/apply.cpp b/src/xrpld/app/tx/detail/apply.cpp index d6967e333f..06be378c2b 100644 --- a/src/xrpld/app/tx/detail/apply.cpp +++ b/src/xrpld/app/tx/detail/apply.cpp @@ -40,6 +40,16 @@ checkValidity( return { Validity::SigBad, "Malformed: Invalid inner batch transaction."}; + + std::string reason; + if (!passesLocalChecks(tx, reason)) + { + router.setFlags(id, SF_LOCALBAD); + return {Validity::SigGoodOnly, reason}; + } + + router.setFlags(id, SF_SIGGOOD); + return {Validity::Valid, ""}; } if (any(flags & SF_SIGBAD)) @@ -48,13 +58,7 @@ checkValidity( if (!any(flags & SF_SIGGOOD)) { - // Don't know signature state. Check it. - auto const requireCanonicalSig = - rules.enabled(featureRequireFullyCanonicalSig) - ? STTx::RequireFullyCanonicalSig::yes - : STTx::RequireFullyCanonicalSig::no; - - auto const sigVerify = tx.checkSign(requireCanonicalSig, rules); + auto const sigVerify = tx.checkSign(rules); if (!sigVerify) { router.setFlags(id, SF_SIGBAD); diff --git a/src/xrpld/rpc/handlers/PayChanClaim.cpp b/src/xrpld/rpc/handlers/PayChanClaim.cpp index dd36a873f0..95c4ba5f3d 100644 --- a/src/xrpld/rpc/handlers/PayChanClaim.cpp +++ b/src/xrpld/rpc/handlers/PayChanClaim.cpp @@ -137,8 +137,7 @@ doChannelVerify(RPC::JsonContext& context) serializePayChanAuthorization(msg, channelId, XRPAmount(drops)); Json::Value result; - result[jss::signature_verified] = - verify(*pk, msg.slice(), makeSlice(*sig), /*canonical*/ true); + result[jss::signature_verified] = verify(*pk, msg.slice(), makeSlice(*sig)); return result; }