diff --git a/.github/scripts/rename/copyright.sh b/.github/scripts/rename/copyright.sh index c0f64de496..41dbdc1f94 100755 --- a/.github/scripts/rename/copyright.sh +++ b/.github/scripts/rename/copyright.sh @@ -70,9 +70,6 @@ fi if ! grep -q 'Dev Null' src/test/app/tx/apply_test.cpp; then echo -e "// Copyright (c) 2020 Dev Null Productions\n\n$(cat src/test/app/tx/apply_test.cpp)" > src/test/app/tx/apply_test.cpp fi -if ! grep -q 'Dev Null' src/test/app/NetworkOPs_test.cpp; then - echo -e "// Copyright (c) 2020 Dev Null Productions\n\n$(cat src/test/app/NetworkOPs_test.cpp)" > src/test/app/NetworkOPs_test.cpp -fi if ! grep -q 'Dev Null' src/test/rpc/ManifestRPC_test.cpp; then echo -e "// Copyright (c) 2020 Dev Null Productions\n\n$(cat src/test/rpc/ManifestRPC_test.cpp)" > src/test/rpc/ManifestRPC_test.cpp fi diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 768897f665..8ce810aa2e 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -130,7 +130,7 @@ jobs: --target "${CMAKE_TARGET}" - name: Upload rippled artifact (Linux) - if: ${{ runner.os == 'Linux' }} + if: ${{ github.repository_owner == 'XRPLF' && runner.os == 'Linux' }} uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 env: BUILD_DIR: ${{ inputs.build_dir }} diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 115f013817..b6d0cf105b 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -66,12 +66,10 @@ XRPL_FEATURE(AMM, Supported::yes, VoteBehavior::DefaultNo XRPL_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (UniversalNumber, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(XRPFees, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FEATURE(DisallowIncoming, 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(Checks, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) // The following amendments are obsolete, but must remain supported @@ -119,10 +117,12 @@ XRPL_RETIRE_FIX(STAmountCanonicalize) XRPL_RETIRE_FIX(TakerDryOfferRemoval) XRPL_RETIRE_FIX(TrustLinesToSelf) +XRPL_RETIRE_FEATURE(Checks) XRPL_RETIRE_FEATURE(CheckCashMakesTrustLine) XRPL_RETIRE_FEATURE(CryptoConditions) XRPL_RETIRE_FEATURE(DepositAuth) XRPL_RETIRE_FEATURE(DepositPreauth) +XRPL_RETIRE_FEATURE(DisallowIncoming) XRPL_RETIRE_FEATURE(Escrow) XRPL_RETIRE_FEATURE(EnforceInvariants) XRPL_RETIRE_FEATURE(ExpandedSignerList) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 9ba32042b4..dd1b52b355 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -226,7 +226,7 @@ TRANSACTION(ttPAYCHAN_CLAIM, 15, PaymentChannelClaim, #endif TRANSACTION(ttCHECK_CREATE, 16, CheckCreate, Delegation::delegatable, - featureChecks, + uint256{}, noPriv, ({ {sfDestination, soeREQUIRED}, @@ -242,7 +242,7 @@ TRANSACTION(ttCHECK_CREATE, 16, CheckCreate, #endif TRANSACTION(ttCHECK_CASH, 17, CheckCash, Delegation::delegatable, - featureChecks, + uint256{}, noPriv, ({ {sfCheckID, soeREQUIRED}, @@ -256,7 +256,7 @@ TRANSACTION(ttCHECK_CASH, 17, CheckCash, #endif TRANSACTION(ttCHECK_CANCEL, 18, CheckCancel, Delegation::delegatable, - featureChecks, + uint256{}, noPriv, ({ {sfCheckID, soeREQUIRED}, diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 61e20a0098..b7d158041b 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -67,8 +67,6 @@ public: class Check_test : public beast::unit_test::suite { - FeatureBitset const disallowIncoming{featureDisallowIncoming}; - static uint256 getCheckIndex(AccountID const& account, std::uint32_t uSequence) { @@ -125,25 +123,6 @@ class Check_test : public beast::unit_test::suite using namespace test::jtx; Account const alice{"alice"}; - { - // If the Checks amendment is not enabled, you should not be able - // to create, cash, or cancel checks. - Env env{*this, features - featureChecks}; - - env.fund(XRP(1000), alice); - env.close(); - - uint256 const checkId{ - getCheckIndex(env.master, env.seq(env.master))}; - env(check::create(env.master, alice, XRP(100)), ter(temDISABLED)); - env.close(); - - env(check::cash(alice, checkId, XRP(100)), ter(temDISABLED)); - env.close(); - - env(check::cancel(alice, checkId), ter(temDISABLED)); - env.close(); - } { // If the Checks amendment is enabled all check-related // facilities should be available. @@ -277,24 +256,12 @@ class Check_test : public beast::unit_test::suite using namespace test::jtx; - // test flag doesn't set unless amendment enabled - { - Env env{*this, features - disallowIncoming}; - Account const alice{"alice"}; - env.fund(XRP(10000), alice); - env(fset(alice, asfDisallowIncomingCheck)); - env.close(); - auto const sle = env.le(alice); - uint32_t flags = sle->getFlags(); - BEAST_EXPECT(!(flags & lsfDisallowIncomingCheck)); - } - Account const gw{"gateway"}; Account const alice{"alice"}; Account const bob{"bob"}; IOU const USD{gw["USD"]}; - Env env{*this, features | disallowIncoming}; + Env env{*this, features}; STAmount const startBalance{XRP(1000).value()}; env.fund(startBalance, gw, alice, bob); @@ -2613,7 +2580,6 @@ public: { using namespace test::jtx; auto const sa = testable_amendments(); - testWithFeats(sa - disallowIncoming); testWithFeats(sa); testTrustLineCreation(sa); } diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 9e1c777a91..e36ad9a550 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -1703,9 +1703,6 @@ class Delegate_test : public beast::unit_test::suite // NFTokenMint, NFTokenBurn, NFTokenCreateOffer, NFTokenCancelOffer, // NFTokenAcceptOffer are not included, they are tested separately. std::unordered_map txRequiredFeatures{ - {"CheckCreate", featureChecks}, - {"CheckCash", featureChecks}, - {"CheckCancel", featureChecks}, {"Clawback", featureClawback}, {"AMMClawback", featureAMMClawback}, {"AMMCreate", featureAMM}, diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 1db46cf91d..fa4ed233b4 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -83,6 +83,7 @@ protected: TenthBips16 managementFeeRate{100}; TenthBips32 coverRateLiquidation = percentageToTenthBips(25); std::string data{}; + std::uint32_t flags = 0; Number maxCoveredLoanValue(Number const& currentDebt) const @@ -223,6 +224,22 @@ protected: } }; + struct PaymentParameters + { + Number overpaymentFactor = Number{1}; + std::optional overpaymentExtra = std::nullopt; + std::uint32_t flags = 0; + bool showStepBalances = false; + bool validateBalances = true; + + static PaymentParameters const& + defaults() + { + static PaymentParameters const result{}; + return result; + } + }; + struct LoanState { std::uint32_t previousPaymentDate = 0; @@ -465,7 +482,7 @@ protected: auto const keylet = keylet::loanbroker(lender.id(), env.seq(lender)); using namespace loanBroker; - env(set(lender, vaultKeylet.key), + env(set(lender, vaultKeylet.key, params.flags), data(params.data), managementFeeRate(params.managementFeeRate), debtMaximum(debtMaximumValue), @@ -700,10 +717,10 @@ protected: << std::endl; // checkGuards returns a TER, so success is 0 - BEAST_EXPECT(!LoanSet::checkGuards( + BEAST_EXPECT(!checkLoanGuards( asset, asset(loanParams.principalRequest).number(), - loanParams.interest.value_or(TenthBips32{}), + loanParams.interest.value_or(TenthBips32{}) != beast::zero, loanParams.payTotal.value_or(LoanSet::defaultPaymentTotal), props, env.journal)); @@ -835,7 +852,7 @@ protected: jtx::Account const& issuer, jtx::Account const& lender, jtx::Account const& borrower, - bool showStepBalances = false) + PaymentParameters const& paymentParams = PaymentParameters::defaults()) { // Make all the individual payments using namespace jtx; @@ -846,6 +863,8 @@ protected: // Account const evan{"evan"}; // Account const alice{"alice"}; + bool const showStepBalances = paymentParams.showStepBalances; + auto const currencyLabel = getCurrencyLabel(broker.asset); auto const baseFee = env.current()->fees().base; @@ -891,24 +910,25 @@ protected: state.loanScale, Number::upward); + auto currentRoundedState = constructRoundedLoanState( + state.totalValue, + state.principalOutstanding, + state.managementFeeOutstanding); { auto const raw = calculateRawLoanState( state.periodicPayment, periodicRate, state.paymentRemaining, broker.params.managementFeeRate); - auto const rounded = constructRoundedLoanState( - state.totalValue, - state.principalOutstanding, - state.managementFeeOutstanding); if (showStepBalances) { log << currencyLabel << " Starting loan balances: " - << "\n\tTotal value: " << rounded.valueOutstanding - << "\n\tPrincipal: " << rounded.principalOutstanding - << "\n\tInterest: " << rounded.interestDue - << "\n\tMgmt fee: " << rounded.managementFeeDue + << "\n\tTotal value: " + << currentRoundedState.valueOutstanding << "\n\tPrincipal: " + << currentRoundedState.principalOutstanding + << "\n\tInterest: " << currentRoundedState.interestDue + << "\n\tMgmt fee: " << currentRoundedState.managementFeeDue << "\n\tPayments remaining " << state.paymentRemaining << std::endl; } @@ -918,18 +938,24 @@ protected: << " Loan starting state: " << state.paymentRemaining << ", " << raw.interestDue << ", " << raw.principalOutstanding << ", " << raw.managementFeeDue - << ", " << rounded.valueOutstanding << ", " - << rounded.principalOutstanding << ", " - << rounded.interestDue << ", " << rounded.managementFeeDue - << std::endl; + << ", " << currentRoundedState.valueOutstanding << ", " + << currentRoundedState.principalOutstanding << ", " + << currentRoundedState.interestDue << ", " + << currentRoundedState.managementFeeDue << std::endl; } } // Try to pay a little extra to show that it's _not_ // taken - STAmount const transactionAmount = STAmount{broker.asset, totalDue} + - std::min(broker.asset(10).value(), - STAmount{broker.asset, totalDue / 20}); + auto const extraAmount = paymentParams.overpaymentExtra + ? broker.asset(*paymentParams.overpaymentExtra).value() + : std::min( + broker.asset(10).value(), + STAmount{broker.asset, totalDue / 20}); + + STAmount const transactionAmount = + STAmount{broker.asset, totalDue * paymentParams.overpaymentFactor} + + extraAmount; auto const borrowerInitialBalance = env.balance(borrower, broker.asset).number(); @@ -949,7 +975,7 @@ protected: broker.params.managementFeeRate); auto validateBorrowerBalance = [&]() { - if (borrower == issuer) + if (borrower == issuer || !paymentParams.validateBalances) return; auto const totalSpent = (totalPaid.trackedValueDelta + totalFeesPaid + @@ -1035,54 +1061,64 @@ protected: auto const totalDueAmount = STAmount{ broker.asset, paymentComponents.trackedValueDelta + serviceFee}; - // Due to the rounding algorithms to keep the interest and - // principal in sync with "true" values, the computed amount - // may be a little less than the rounded fixed payment - // amount. For integral types, the difference should be < 3 - // (1 unit for each of the interest and management fee). For - // IOUs, the difference should be dust. - Number const diff = totalDue - totalDueAmount; - BEAST_EXPECT( - paymentComponents.specialCase == - detail::PaymentSpecialCase::final || - diff == beast::zero || - (diff > beast::zero && - ((broker.asset.integral() && - (static_cast(diff) < 3)) || - (state.loanScale - diff.exponent() > 13)))); + if (paymentParams.validateBalances) + { + // Due to the rounding algorithms to keep the interest and + // principal in sync with "true" values, the computed amount + // may be a little less than the rounded fixed payment + // amount. For integral types, the difference should be < 3 + // (1 unit for each of the interest and management fee). For + // IOUs, the difference should be dust. + Number const diff = totalDue - totalDueAmount; + BEAST_EXPECT( + paymentComponents.specialCase == + detail::PaymentSpecialCase::final || + diff == beast::zero || + (diff > beast::zero && + ((broker.asset.integral() && + (static_cast(diff) < 3)) || + (state.loanScale - diff.exponent() > 13)))); - BEAST_EXPECT( - paymentComponents.trackedPrincipalDelta >= beast::zero && - paymentComponents.trackedPrincipalDelta <= - state.principalOutstanding); - BEAST_EXPECT( - paymentComponents.specialCase != - detail::PaymentSpecialCase::final || - paymentComponents.trackedPrincipalDelta == - state.principalOutstanding); + BEAST_EXPECT( + paymentComponents.trackedPrincipalDelta >= beast::zero && + paymentComponents.trackedPrincipalDelta <= + state.principalOutstanding); + BEAST_EXPECT( + paymentComponents.specialCase != + detail::PaymentSpecialCase::final || + paymentComponents.trackedPrincipalDelta == + state.principalOutstanding); + } auto const borrowerBalanceBeforePayment = env.balance(borrower, broker.asset); // Make the payment - env(pay(borrower, loanKeylet.key, transactionAmount)); + env( + pay(borrower, + loanKeylet.key, + transactionAmount, + paymentParams.flags)); env.close(d{state.paymentInterval / 2}); - // Need to account for fees if the loan is in XRP - PrettyAmount adjustment = broker.asset(0); - if (broker.asset.native()) + if (paymentParams.validateBalances) { - adjustment = env.current()->fees().base; - } + // Need to account for fees if the loan is in XRP + PrettyAmount adjustment = broker.asset(0); + if (broker.asset.native()) + { + adjustment = env.current()->fees().base; + } - // Check the result - verifyLoanStatus.checkPayment( - state.loanScale, - borrower, - borrowerBalanceBeforePayment, - totalDueAmount, - adjustment); + // Check the result + verifyLoanStatus.checkPayment( + state.loanScale, + borrower, + borrowerBalanceBeforePayment, + totalDueAmount, + adjustment); + } if (showStepBalances) { @@ -1110,6 +1146,8 @@ protected: << ", error: " << truncate(errors.managementFee) << ")\n\tPayments remaining " << loanSle->at(sfPaymentRemaining) << std::endl; + + currentRoundedState = current; } --state.paymentRemaining; @@ -1130,7 +1168,8 @@ protected: paymentComponents.trackedManagementFeeDelta; state.totalValue -= paymentComponents.trackedValueDelta; - verifyLoanStatus(state); + if (paymentParams.validateBalances) + verifyLoanStatus(state); totalPaid.trackedValueDelta += paymentComponents.trackedValueDelta; totalPaid.trackedPrincipalDelta += @@ -1149,21 +1188,25 @@ protected: BEAST_EXPECT(state.paymentRemaining == 0); BEAST_EXPECT(state.principalOutstanding == 0); - // Make sure all the payments add up - BEAST_EXPECT(totalPaid.trackedValueDelta == initialState.totalValue); - BEAST_EXPECT( - totalPaid.trackedPrincipalDelta == - initialState.principalOutstanding); - BEAST_EXPECT( - totalPaid.trackedManagementFeeDelta == - initialState.managementFeeOutstanding); - // This is almost a tautology given the previous checks, but - // check it anyway for completeness. auto const initialInterestDue = initialState.totalValue - (initialState.principalOutstanding + initialState.managementFeeOutstanding); - BEAST_EXPECT(totalInterestPaid == initialInterestDue); - BEAST_EXPECT(totalPaymentsMade == initialState.paymentRemaining); + if (paymentParams.validateBalances) + { + // Make sure all the payments add up + BEAST_EXPECT( + totalPaid.trackedValueDelta == initialState.totalValue); + BEAST_EXPECT( + totalPaid.trackedPrincipalDelta == + initialState.principalOutstanding); + BEAST_EXPECT( + totalPaid.trackedManagementFeeDelta == + initialState.managementFeeOutstanding); + // This is almost a tautology given the previous checks, but + // check it anyway for completeness. + BEAST_EXPECT(totalInterestPaid == initialInterestDue); + BEAST_EXPECT(totalPaymentsMade == initialState.paymentRemaining); + } if (showStepBalances) { @@ -6514,7 +6557,7 @@ protected: issuer, lender, borrower, - true); + PaymentParameters{.showStepBalances = true}); if (auto const brokerSle = env.le(broker.brokerKeylet()); BEAST_EXPECT(brokerSle)) @@ -6640,7 +6683,7 @@ protected: env.le(keylet::loanbroker(brokerInfo.brokerID)); BEAST_EXPECT(brokerSle)) { - std::cout << *brokerSle << std::endl; + log << *brokerSle << std::endl; BEAST_EXPECT(brokerSle->at(sfDebtTotal) == Number(804)); } @@ -6662,7 +6705,7 @@ protected: env.le(keylet::loanbroker(brokerInfo.brokerID)); BEAST_EXPECT(brokerSle)) { - std::cout << *brokerSle << std::endl; + log << *brokerSle << std::endl; BEAST_EXPECT( brokerSle->at(sfCoverAvailable) == xrpAsset(81).value()); BEAST_EXPECT(brokerSle->at(sfDebtTotal) == Number(804)); @@ -6670,8 +6713,7 @@ protected: // Also demonstrate that the true minimum (804 * 10%) exceeds 80 auto const theoreticalMin = tenthBipsOfValue(Number(804), TenthBips32(10'000)); - std::cout << "Theoretical min cover: " << theoreticalMin - << std::endl; + log << "Theoretical min cover: " << theoreticalMin << std::endl; BEAST_EXPECT(Number(804, -1) == theoreticalMin); } } @@ -6727,7 +6769,7 @@ protected: issuer, lender, borrower, - true); + PaymentParameters{.showStepBalances = true}); } void @@ -6901,7 +6943,85 @@ protected: issuer, lender, issuer, - true); + PaymentParameters{.showStepBalances = true}); + } + + void + testLimitExceeded() + { + testcase("RIPD-4125 - overpayment"); + + using namespace jtx; + + Account const issuer("issuer"); + Account const lender("lender"); + Account const borrower("borrower"); + + BrokerParameters const brokerParams{ + .vaultDeposit = 100'000, + .debtMax = 0, + .coverRateMin = TenthBips32{0}, + .managementFeeRate = TenthBips16{0}, + .coverRateLiquidation = TenthBips32{0}}; + LoanParameters const loanParams{ + .account = lender, + .counter = borrower, + .principalRequest = Number{200000, -6}, + .interest = TenthBips32{50000}, + .payTotal = 3, + .payInterval = 200, + .gracePd = 60, + .flags = tfLoanOverpayment, + }; + + auto const assetType = AssetType::XRP; + + Env env( + *this, + makeConfig(), + all, + nullptr, + beast::severities::Severity::kWarning); + + auto loanResult = createLoan( + env, assetType, brokerParams, loanParams, issuer, lender, borrower); + + if (!BEAST_EXPECT(loanResult)) + return; + + auto broker = std::get(*loanResult); + auto loanKeylet = std::get(*loanResult); + auto pseudoAcct = std::get(*loanResult); + + VerifyLoanStatus verifyLoanStatus(env, broker, pseudoAcct, loanKeylet); + + auto const state = getCurrentState(env, broker, loanKeylet); + + env(loan::pay( + borrower, + loanKeylet.key, + STAmount{broker.asset, state.periodicPayment * 3 / 2 + 1}, + tfLoanOverpayment)); + env.close(); + + PaymentParameters paymentParams{ + //.overpaymentFactor = Number{15, -1}, + //.overpaymentExtra = Number{1, -6}, + //.flags = tfLoanOverpayment, + .showStepBalances = true, + //.validateBalances = false, + }; + + makeLoanPayments( + env, + broker, + loanParams, + loanKeylet, + verifyLoanStatus, + issuer, + lender, + borrower, + paymentParams); } public: @@ -6951,6 +7071,7 @@ public: testRoundingAllowsUndercoverage(); testBorrowerIsBroker(); testIssuerIsBorrower(); + testLimitExceeded(); } }; diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index a11446df71..12ac309acb 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -12,8 +12,6 @@ namespace ripple { class NFTokenBaseUtil_test : public beast::unit_test::suite { - FeatureBitset const disallowIncoming{featureDisallowIncoming}; - // Helper function that returns the number of NFTs minted by an issuer. static std::uint32_t mintedCount(test::jtx::Env const& env, test::jtx::Account const& issuer) @@ -2960,19 +2958,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite using namespace test::jtx; - // test flag doesn't set unless amendment enabled - { - Env env{*this, features - disallowIncoming}; - Account const alice{"alice"}; - env.fund(XRP(10000), alice); - env(fset(alice, asfDisallowIncomingNFTokenOffer)); - env.close(); - auto const sle = env.le(alice); - uint32_t flags = sle->getFlags(); - BEAST_EXPECT(!(flags & lsfDisallowIncomingNFTokenOffer)); - } - - Env env{*this, features | disallowIncoming}; + Env env{*this, features}; Account const issuer{"issuer"}; Account const minter{"minter"}; @@ -7624,8 +7610,8 @@ class NFTokenDisallowIncoming_test : public NFTokenBaseUtil_test run() override { testWithFeats( - allFeatures - featureDisallowIncoming - fixNFTokenReserve - - featureNFTokenMintOffer - featureDynamicNFT); + allFeatures - fixNFTokenReserve - featureNFTokenMintOffer - + featureDynamicNFT); } }; diff --git a/src/test/app/NetworkOPs_test.cpp b/src/test/app/NetworkOPs_test.cpp index d9afe6d628..3965778221 100644 --- a/src/test/app/NetworkOPs_test.cpp +++ b/src/test/app/NetworkOPs_test.cpp @@ -1,5 +1,3 @@ -// Copyright (c) 2020 Dev Null Productions - #include #include #include diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index bee3167b29..59c404ae28 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -14,8 +14,6 @@ using namespace jtx::paychan; struct PayChan_test : public beast::unit_test::suite { - FeatureBitset const disallowIncoming{featureDisallowIncoming}; - static std::pair> channelKeyAndSle( ReadView const& view, @@ -242,20 +240,8 @@ struct PayChan_test : public beast::unit_test::suite testcase("Disallow Incoming Flag"); using namespace jtx; - // test flag doesn't set unless amendment enabled - { - Env env{*this, features - disallowIncoming}; - Account const alice{"alice"}; - env.fund(XRP(10000), alice); - env(fset(alice, asfDisallowIncomingPayChan)); - env.close(); - auto const sle = env.le(alice); - uint32_t flags = sle->getFlags(); - BEAST_EXPECT(!(flags & lsfDisallowIncomingPayChan)); - } - using namespace std::literals::chrono_literals; - Env env{*this, features | disallowIncoming}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto const cho = Account("cho"); @@ -2127,7 +2113,6 @@ public: { using namespace test::jtx; FeatureBitset const all{testable_amendments()}; - testWithFeats(all - disallowIncoming); testWithFeats(all); testDepositAuthCreds(); testMetaAndOwnership(all - fixIncludeKeyletFields); diff --git a/src/test/app/SetTrust_test.cpp b/src/test/app/SetTrust_test.cpp index 88e359ab23..07573cf13f 100644 --- a/src/test/app/SetTrust_test.cpp +++ b/src/test/app/SetTrust_test.cpp @@ -9,8 +9,6 @@ namespace test { class SetTrust_test : public beast::unit_test::suite { - FeatureBitset const disallowIncoming{featureDisallowIncoming}; - public: void testTrustLineDelete() @@ -478,25 +476,12 @@ public: using namespace test::jtx; - // test flag doesn't set unless amendment enabled - { - Env env{*this, features - disallowIncoming}; - Account const alice{"alice"}; - env.fund(XRP(10000), alice); - env(fset(alice, asfDisallowIncomingTrustline)); - env.close(); - auto const sle = env.le(alice); - uint32_t flags = sle->getFlags(); - BEAST_EXPECT(!(flags & lsfDisallowIncomingTrustline)); - } - // fixDisallowIncomingV1 { for (bool const withFix : {true, false}) { - auto const amend = withFix - ? features | disallowIncoming - : (features | disallowIncoming) - fixDisallowIncomingV1; + auto const amend = + withFix ? features : features - fixDisallowIncomingV1; Env env{*this, amend}; auto const dist = Account("dist"); @@ -532,7 +517,7 @@ public: } } - Env env{*this, features | disallowIncoming}; + Env env{*this, features}; auto const gw = Account{"gateway"}; auto const alice = Account{"alice"}; @@ -630,7 +615,6 @@ public: { using namespace test::jtx; auto const sa = testable_amendments(); - testWithFeats(sa - disallowIncoming); testWithFeats(sa); } }; diff --git a/src/test/rpc/AccountInfo_test.cpp b/src/test/rpc/AccountInfo_test.cpp index 86d0a4b4f6..5e2fc608b3 100644 --- a/src/test/rpc/AccountInfo_test.cpp +++ b/src/test/rpc/AccountInfo_test.cpp @@ -615,33 +615,23 @@ public: {"disallowIncomingTrustline", asfDisallowIncomingTrustline}}}; - if (features[featureDisallowIncoming]) + for (auto& asf : disallowIncomingFlags) { - for (auto& asf : disallowIncomingFlags) - { - // Clear a flag and check that account_info returns results - // as expected - env(fclear(alice, asf.second)); - env.close(); - auto const f1 = getAccountFlag(asf.first, alice); - BEAST_EXPECT(f1.has_value()); - BEAST_EXPECT(!f1.value()); + // Clear a flag and check that account_info returns results + // as expected + env(fclear(alice, asf.second)); + env.close(); + auto const f1 = getAccountFlag(asf.first, alice); + BEAST_EXPECT(f1.has_value()); + BEAST_EXPECT(!f1.value()); - // Set a flag and check that account_info returns results - // as expected - env(fset(alice, asf.second)); - env.close(); - auto const f2 = getAccountFlag(asf.first, alice); - BEAST_EXPECT(f2.has_value()); - BEAST_EXPECT(f2.value()); - } - } - else - { - for (auto& asf : disallowIncomingFlags) - { - BEAST_EXPECT(!getAccountFlag(asf.first, alice)); - } + // Set a flag and check that account_info returns results + // as expected + env(fset(alice, asf.second)); + env.close(); + auto const f2 = getAccountFlag(asf.first, alice); + BEAST_EXPECT(f2.has_value()); + BEAST_EXPECT(f2.value()); } static constexpr std::pair @@ -706,12 +696,8 @@ public: FeatureBitset const allFeatures{ ripple::test::jtx::testable_amendments()}; testAccountFlags(allFeatures); - testAccountFlags(allFeatures - featureDisallowIncoming); - testAccountFlags( - allFeatures - featureDisallowIncoming - featureClawback); - testAccountFlags( - allFeatures - featureDisallowIncoming - featureClawback - - featureTokenEscrow); + testAccountFlags(allFeatures - featureClawback); + testAccountFlags(allFeatures - featureClawback - featureTokenEscrow); } }; diff --git a/src/test/rpc/LedgerEntry_test.cpp b/src/test/rpc/LedgerEntry_test.cpp index fdd3838e89..a0fde0f457 100644 --- a/src/test/rpc/LedgerEntry_test.cpp +++ b/src/test/rpc/LedgerEntry_test.cpp @@ -1103,7 +1103,7 @@ class LedgerEntry_test : public beast::unit_test::suite checkErrorValue( jrr[jss::result], "malformedAuthorizedCredentials", - "Invalid field 'authorized_credentials', not array."); + "Invalid field 'authorized_credentials', array empty."); } { @@ -1144,7 +1144,7 @@ class LedgerEntry_test : public beast::unit_test::suite checkErrorValue( jrr[jss::result], "malformedAuthorizedCredentials", - "Invalid field 'authorized_credentials', not array."); + "Invalid field 'authorized_credentials', array too long."); } } diff --git a/src/test/rpc/RPCCall_test.cpp b/src/test/rpc/RPCCall_test.cpp index 07129e4bae..7647dc5f65 100644 --- a/src/test/rpc/RPCCall_test.cpp +++ b/src/test/rpc/RPCCall_test.cpp @@ -1584,8 +1584,6 @@ static RPCCallTestData const rpcCallTestArray[] = { "EUR/rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789", - "junk", // Note: indexing bug in parseBookOffers() requires junk - // param. "200", }, RPCCallTestData::no_exception, @@ -1597,7 +1595,6 @@ static RPCCallTestData const rpcCallTestArray[] = { "issuer" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "ledger_hash" : "ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789", "limit" : 200, - "proof" : true, "taker_gets" : { "currency" : "EUR", "issuer" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA" @@ -1617,8 +1614,8 @@ static RPCCallTestData const rpcCallTestArray[] = { "EUR/rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789", - "junk", // Note: indexing bug in parseBookOffers() requires junk param. "200", + "0", "MyMarker"}, RPCCallTestData::no_exception, R"({ @@ -1630,7 +1627,6 @@ static RPCCallTestData const rpcCallTestArray[] = { "ledger_hash" : "ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789", "limit" : 200, "marker" : "MyMarker", - "proof" : true, "taker_gets" : { "currency" : "EUR", "issuer" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA" @@ -1665,8 +1661,8 @@ static RPCCallTestData const rpcCallTestArray[] = { "EUR/rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789", - "junk", // Note: indexing bug in parseBookOffers() requires junk param. "200", + "0", "MyMarker", "extra"}, RPCCallTestData::no_exception, @@ -1770,12 +1766,19 @@ static RPCCallTestData const rpcCallTestArray[] = { "EUR/rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789", - "junk", // Note: indexing bug in parseBookOffers() requires junk - // param. "not_a_number", }, - RPCCallTestData::bad_cast, - R"()"}, + RPCCallTestData::no_exception, + R"({ + "method" : "book_offers", + "params" : [ + { + "error" : "invalidParams", + "error_code" : 31, + "error_message" : "Invalid field 'limit'." + } + ] + })"}, // can_delete // ------------------------------------------------------------------ diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index f430a0f0d5..28a8ad0829 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -102,6 +102,15 @@ struct LoanState } }; +TER +checkLoanGuards( + Asset const& vaultAsset, + Number const& principalRequested, + bool expectInterest, + std::uint32_t paymentTotal, + LoanProperties const& properties, + beast::Journal j); + LoanState calculateRawLoanState( Number const& periodicPayment, @@ -217,6 +226,9 @@ operator-(LoanState const& lhs, LoanState const& rhs); LoanState operator-(LoanState const& lhs, detail::LoanDeltas const& rhs); +LoanState +operator+(LoanState const& lhs, detail::LoanDeltas const& rhs); + LoanProperties computeLoanProperties( Asset const& asset, diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index 62f567e7c0..e5eac77c1b 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -372,9 +372,7 @@ tryOverpayment( auto const rounded = constructRoundedLoanState( totalValueOutstanding, principalOutstanding, managementFeeOutstanding); - auto const totalValueError = totalValueOutstanding - raw.valueOutstanding; - auto const principalError = principalOutstanding - raw.principalOutstanding; - auto const feeError = managementFeeOutstanding - raw.managementFeeDue; + auto const errors = rounded - raw; auto const newRawPrincipal = std::max( raw.principalOutstanding - overpaymentComponents.trackedPrincipalDelta, @@ -389,33 +387,68 @@ tryOverpayment( managementFeeRate, loanScale); - auto const newRaw = calculateRawLoanState( - newLoanProperties.periodicPayment, - periodicRate, - paymentRemaining, - managementFeeRate); + JLOG(j.debug()) << "new periodic payment: " + << newLoanProperties.periodicPayment + << ", new total value: " + << newLoanProperties.totalValueOutstanding + << ", first payment principal: " + << newLoanProperties.firstPaymentPrincipal; - totalValueOutstanding = roundToAsset( - asset, newRaw.valueOutstanding + totalValueError, loanScale); - principalOutstanding = roundToAsset( - asset, - newRaw.principalOutstanding + principalError, - loanScale, - Number::downward); - managementFeeOutstanding = - roundToAsset(asset, newRaw.managementFeeDue + feeError, loanScale); + auto const newRaw = calculateRawLoanState( + newLoanProperties.periodicPayment, + periodicRate, + paymentRemaining, + managementFeeRate) + + errors; + + JLOG(j.debug()) << "new raw value: " << newRaw.valueOutstanding + << ", principal: " << newRaw.principalOutstanding + << ", interest gross: " << newRaw.interestOutstanding(); + + principalOutstanding = std::clamp( + roundToAsset( + asset, newRaw.principalOutstanding, loanScale, Number::upward), + numZero, + rounded.principalOutstanding); + totalValueOutstanding = std::clamp( + roundToAsset( + asset, + principalOutstanding + newRaw.interestOutstanding(), + loanScale, + Number::upward), + numZero, + rounded.valueOutstanding); + managementFeeOutstanding = std::clamp( + roundToAsset(asset, newRaw.managementFeeDue, loanScale), + numZero, + rounded.managementFeeDue); + + auto const newRounded = constructRoundedLoanState( + totalValueOutstanding, principalOutstanding, managementFeeOutstanding); + + newLoanProperties.totalValueOutstanding = newRounded.valueOutstanding; + + JLOG(j.debug()) << "new rounded value: " << newRounded.valueOutstanding + << ", principal: " << newRounded.principalOutstanding + << ", interest gross: " << newRounded.interestOutstanding(); periodicPayment = newLoanProperties.periodicPayment; // check that the loan is still valid - if (newLoanProperties.firstPaymentPrincipal <= 0 && - principalOutstanding > 0) + if (auto const ter = checkLoanGuards( + asset, + principalOutstanding, + // The loan may have been created with interest, but for + // small interest amounts, that may have already been paid + // off. Check what's still outstanding. This should + // guarantee that the interest checks pass. + newRounded.interestOutstanding() != beast::zero, + paymentRemaining, + newLoanProperties, + j)) { - // The overpayment has caused the loan to be in a state - // where no further principal can be paid. - JLOG(j.warn()) - << "Loan overpayment would cause loan to be stuck. " - "Rejecting overpayment, but normal payments are unaffected."; + JLOG(j.warn()) << "Principal overpayment would cause the loan to be in " + "an invalid state. Ignore the overpayment"; return Unexpected(tesSUCCESS); } @@ -437,21 +470,27 @@ tryOverpayment( // LCOV_EXCL_STOP } - auto const newRounded = constructRoundedLoanState( - totalValueOutstanding, principalOutstanding, managementFeeOutstanding); + auto const deltas = rounded - newRounded; + + auto const hypotheticalValueOutstanding = + rounded.valueOutstanding - deltas.principal; + auto const valueChange = - newRounded.interestOutstanding() - rounded.interestOutstanding(); - XRPL_ASSERT_PARTS( - valueChange <= beast::zero, - "ripple::detail::tryOverpayment", - "principal overpayment did not increase value of loan"); + newRounded.valueOutstanding - hypotheticalValueOutstanding; + if (valueChange > 0) + { + JLOG(j.warn()) << "Principal overpayment would increase the value of " + "the loan. Ignore the overpayment"; + return Unexpected(tesSUCCESS); + } return LoanPaymentParts{ - .principalPaid = - rounded.principalOutstanding - newRounded.principalOutstanding, - .interestPaid = rounded.interestDue - newRounded.interestDue, - .valueChange = valueChange + overpaymentComponents.untrackedInterest, - .feePaid = rounded.managementFeeDue - newRounded.managementFeeDue + + .principalPaid = deltas.principal, + .interestPaid = + deltas.interest + overpaymentComponents.untrackedInterest, + .valueChange = + valueChange + overpaymentComponents.trackedInterestPart(), + .feePaid = deltas.managementFee + overpaymentComponents.untrackedManagementFee}; } @@ -481,6 +520,17 @@ doOverpayment( Number managementFeeOutstanding = managementFeeOutstandingProxy; Number periodicPayment = periodicPaymentProxy; + JLOG(j.debug()) + << "overpayment components:" + << ", totalValue before: " << *totalValueOutstandingProxy + << ", valueDelta: " << overpaymentComponents.trackedValueDelta + << ", principalDelta: " << overpaymentComponents.trackedPrincipalDelta + << ", managementFeeDelta: " + << overpaymentComponents.trackedManagementFeeDelta + << ", interestPart: " << overpaymentComponents.trackedInterestPart() + << ", untrackedInterest: " << overpaymentComponents.untrackedInterest + << ", totalDue: " << overpaymentComponents.totalDue + << ", payments remaining :" << paymentRemaining; auto const ret = tryOverpayment( asset, loanScale, @@ -527,12 +577,29 @@ doOverpayment( "ripple::detail::doOverpayment", "no fee change"); + // I'm not 100% sure the following asserts are correct. If in doubt, and + // everything else works, remove any that cause trouble. + + JLOG(j.debug()) << "valueChange: " << loanPaymentParts.valueChange + << ", totalValue before: " << *totalValueOutstandingProxy + << ", totalValue after: " << totalValueOutstanding + << ", totalValue delta: " + << (totalValueOutstandingProxy - totalValueOutstanding) + << ", principalDelta: " + << overpaymentComponents.trackedPrincipalDelta + << ", principalPaid: " << loanPaymentParts.principalPaid + << ", Computed difference: " + << overpaymentComponents.trackedPrincipalDelta - + (totalValueOutstandingProxy - totalValueOutstanding); + XRPL_ASSERT_PARTS( - overpaymentComponents.untrackedInterest == - totalValueOutstandingProxy - totalValueOutstanding - - overpaymentComponents.trackedPrincipalDelta, + loanPaymentParts.valueChange == + totalValueOutstanding - + (totalValueOutstandingProxy - + overpaymentComponents.trackedPrincipalDelta) + + overpaymentComponents.trackedInterestPart(), "ripple::detail::doOverpayment", - "value change agrees"); + "interest paid agrees"); XRPL_ASSERT_PARTS( overpaymentComponents.trackedPrincipalDelta == @@ -995,11 +1062,9 @@ computeOverpaymentComponents( Number const fee = roundToAsset( asset, tenthBipsOfValue(overpayment, overpaymentFeeRate), loanScale); - Number const payment = overpayment - fee; - - auto const [rawOverpaymentInterest, rawOverpaymentManagementFee] = [&]() { + auto const [rawOverpaymentInterest, _] = [&]() { Number const interest = - tenthBipsOfValue(payment, overpaymentInterestRate); + tenthBipsOfValue(overpayment, overpaymentInterestRate); return detail::computeInterestAndFeeParts(interest, managementFeeRate); }(); auto const [roundedOverpaymentInterest, roundedOverpaymentManagementFee] = @@ -1010,15 +1075,20 @@ computeOverpaymentComponents( asset, interest, managementFeeRate, loanScale); }(); - return detail::ExtendedPaymentComponents{ + auto const result = detail::ExtendedPaymentComponents{ detail::PaymentComponents{ - .trackedValueDelta = payment, - .trackedPrincipalDelta = payment - roundedOverpaymentInterest - - roundedOverpaymentManagementFee, + .trackedValueDelta = overpayment - fee, + .trackedPrincipalDelta = overpayment - roundedOverpaymentInterest - + roundedOverpaymentManagementFee - fee, .trackedManagementFeeDelta = roundedOverpaymentManagementFee, .specialCase = detail::PaymentSpecialCase::extra}, fee, roundedOverpaymentInterest}; + XRPL_ASSERT_PARTS( + result.trackedInterestPart() == roundedOverpaymentInterest, + "ripple::detail::computeOverpaymentComponents", + "valid interest computation"); + return result; } } // namespace detail @@ -1048,6 +1118,100 @@ operator-(LoanState const& lhs, detail::LoanDeltas const& rhs) return result; } +LoanState +operator+(LoanState const& lhs, detail::LoanDeltas const& rhs) +{ + LoanState result{ + .valueOutstanding = lhs.valueOutstanding + rhs.total(), + .principalOutstanding = lhs.principalOutstanding + rhs.principal, + .interestDue = lhs.interestDue + rhs.interest, + .managementFeeDue = lhs.managementFeeDue + rhs.managementFee, + }; + + return result; +} + +TER +checkLoanGuards( + Asset const& vaultAsset, + Number const& principalRequested, + bool expectInterest, + std::uint32_t paymentTotal, + LoanProperties const& properties, + beast::Journal j) +{ + auto const totalInterestOutstanding = + properties.totalValueOutstanding - principalRequested; + // Guard 1: if there is no computed total interest over the life of the + // loan for a non-zero interest rate, we cannot properly amortize the + // loan + if (expectInterest && totalInterestOutstanding <= 0) + { + // Unless this is a zero-interest loan, there must be some interest + // due on the loan, even if it's (measurable) dust + JLOG(j.warn()) << "Loan for " << principalRequested + << " with interest has no interest due"; + return tecPRECISION_LOSS; + } + // Guard 1a: If there is any interest computed over the life of the + // loan, for a zero interest rate, something went sideways. + if (!expectInterest && totalInterestOutstanding > 0) + { + // LCOV_EXCL_START + JLOG(j.warn()) << "Loan for " << principalRequested + << " with no interest has interest due"; + return tecINTERNAL; + // LCOV_EXCL_STOP + } + + // Guard 2: if the principal portion of the first periodic payment is + // too small to be accurately represented with the given rounding mode, + // raise an error + if (properties.firstPaymentPrincipal <= 0) + { + // Check that some true (unrounded) principal is paid each period. + // Since the first payment pays the least principal, if it's good, + // they'll all be good. Note that the outstanding principal is + // rounded, and may not change right away. + JLOG(j.warn()) << "Loan is unable to pay principal."; + return tecPRECISION_LOSS; + } + + // Guard 3: If the periodic payment is so small that it can't even be + // rounded to a representable value, then the loan can't be paid. Also, + // avoids dividing by 0. + auto const roundedPayment = roundPeriodicPayment( + vaultAsset, properties.periodicPayment, properties.loanScale); + if (roundedPayment == beast::zero) + { + JLOG(j.warn()) << "Loan Periodic payment (" + << properties.periodicPayment << ") rounds to 0. "; + return tecPRECISION_LOSS; + } + + // Guard 4: if the rounded periodic payment is large enough that the + // loan can't be amortized in the specified number of payments, raise an + // error + { + NumberRoundModeGuard mg(Number::upward); + + if (std::int64_t const computedPayments{ + properties.totalValueOutstanding / roundedPayment}; + computedPayments != paymentTotal) + { + JLOG(j.warn()) << "Loan Periodic payment (" + << properties.periodicPayment << ") rounding (" + << roundedPayment << ") on a total value of " + << properties.totalValueOutstanding + << " can not complete the loan in the specified " + "number of payments (" + << computedPayments << " != " << paymentTotal << ")"; + return tecPRECISION_LOSS; + } + } + return tesSUCCESS; +} + Number calculateFullPaymentInterest( Number const& rawPrincipalOutstanding, diff --git a/src/xrpld/app/tx/detail/CreateCheck.cpp b/src/xrpld/app/tx/detail/CreateCheck.cpp index f5a36e6ac5..df859c4364 100644 --- a/src/xrpld/app/tx/detail/CreateCheck.cpp +++ b/src/xrpld/app/tx/detail/CreateCheck.cpp @@ -61,8 +61,7 @@ CreateCheck::preclaim(PreclaimContext const& ctx) auto const flags = sleDst->getFlags(); // Check if the destination has disallowed incoming checks - if (ctx.view.rules().enabled(featureDisallowIncoming) && - (flags & lsfDisallowIncomingCheck)) + if (flags & lsfDisallowIncomingCheck) return tecNO_PERMISSION; // Pseudo-accounts cannot cash checks. Note, this is not amendment-gated diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 0f3a6a3819..39a93f634a 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -361,6 +361,12 @@ LoanPay::doApply() // LCOV_EXCL_STOP } + JLOG(j_.debug()) << "Loan Pay: principal paid: " + << paymentParts->principalPaid + << ", interest paid: " << paymentParts->interestPaid + << ", fee paid: " << paymentParts->feePaid + << ", value change: " << paymentParts->valueChange; + //------------------------------------------------------ // LoanBroker object state changes view.update(brokerSle); @@ -442,6 +448,12 @@ LoanPay::doApply() } } + JLOG(j_.debug()) << "total paid to vault raw: " << totalPaidToVaultRaw + << ", total paid to vault rounded: " + << totalPaidToVaultRounded + << ", total paid to broker: " << totalPaidToBroker + << ", amount from transaction: " << amount; + // Move funds XRPL_ASSERT_PARTS( totalPaidToVaultRounded + totalPaidToBroker <= amount, diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index f72ad33cee..d9e5f8b981 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -312,87 +312,6 @@ LoanSet::preclaim(PreclaimContext const& ctx) return tesSUCCESS; } -TER -LoanSet::checkGuards( - Asset const& vaultAsset, - Number const& principalRequested, - TenthBips32 interestRate, - std::uint32_t paymentTotal, - LoanProperties const& properties, - beast::Journal j) -{ - auto const totalInterestOutstanding = - properties.totalValueOutstanding - principalRequested; - // Guard 1: if there is no computed total interest over the life of the - // loan for a non-zero interest rate, we cannot properly amortize the - // loan - if (interestRate > TenthBips32{0} && totalInterestOutstanding <= 0) - { - // Unless this is a zero-interest loan, there must be some interest - // due on the loan, even if it's (measurable) dust - JLOG(j.warn()) << "Loan for " << principalRequested << " with " - << interestRate << "% interest has no interest due"; - return tecPRECISION_LOSS; - } - // Guard 1a: If there is any interest computed over the life of the - // loan, for a zero interest rate, something went sideways. - if (interestRate == TenthBips32{0} && totalInterestOutstanding > 0) - { - // LCOV_EXCL_START - JLOG(j.warn()) << "Loan for " << principalRequested - << " with 0% interest has interest due"; - return tecINTERNAL; - // LCOV_EXCL_STOP - } - - // Guard 2: if the principal portion of the first periodic payment is - // too small to be accurately represented with the given rounding mode, - // raise an error - if (properties.firstPaymentPrincipal <= 0) - { - // Check that some true (unrounded) principal is paid each period. - // Since the first payment pays the least principal, if it's good, - // they'll all be good. Note that the outstanding principal is - // rounded, and may not change right away. - JLOG(j.warn()) << "Loan is unable to pay principal."; - return tecPRECISION_LOSS; - } - - // Guard 3: If the periodic payment is so small that it can't even be - // rounded to a representable value, then the loan can't be paid. Also, - // avoids dividing by 0. - auto const roundedPayment = roundPeriodicPayment( - vaultAsset, properties.periodicPayment, properties.loanScale); - if (roundedPayment == beast::zero) - { - JLOG(j.warn()) << "Loan Periodic payment (" - << properties.periodicPayment << ") rounds to 0. "; - return tecPRECISION_LOSS; - } - - // Guard 4: if the rounded periodic payment is large enough that the - // loan can't be amortized in the specified number of payments, raise an - // error - { - NumberRoundModeGuard mg(Number::upward); - - if (std::int64_t const computedPayments{ - properties.totalValueOutstanding / roundedPayment}; - computedPayments != paymentTotal) - { - JLOG(j.warn()) << "Loan Periodic payment (" - << properties.periodicPayment << ") rounding (" - << roundedPayment << ") on a total value of " - << properties.totalValueOutstanding - << " can not complete the loan in the specified " - "number of payments (" - << computedPayments << " != " << paymentTotal << ")"; - return tecPRECISION_LOSS; - } - } - return tesSUCCESS; -} - TER LoanSet::doApply() { @@ -474,10 +393,10 @@ LoanSet::doApply() } } - if (auto const ret = checkGuards( + if (auto const ret = checkLoanGuards( vaultAsset, principalRequested, - interestRate, + interestRate != beast::zero, paymentTotal, properties, j_)) diff --git a/src/xrpld/app/tx/detail/LoanSet.h b/src/xrpld/app/tx/detail/LoanSet.h index e7f4ef5503..91f3960891 100644 --- a/src/xrpld/app/tx/detail/LoanSet.h +++ b/src/xrpld/app/tx/detail/LoanSet.h @@ -36,15 +36,6 @@ public: static TER preclaim(PreclaimContext const& ctx); - static TER - checkGuards( - Asset const& vaultAsset, - Number const& principalRequested, - TenthBips32 interestRate, - std::uint32_t paymentTotal, - LoanProperties const& properties, - beast::Journal j); - TER doApply() override; diff --git a/src/xrpld/app/tx/detail/NFTokenUtils.cpp b/src/xrpld/app/tx/detail/NFTokenUtils.cpp index 67f3a0de5f..db2b00cb2c 100644 --- a/src/xrpld/app/tx/detail/NFTokenUtils.cpp +++ b/src/xrpld/app/tx/detail/NFTokenUtils.cpp @@ -919,31 +919,21 @@ tokenOfferCreatePreclaim( return tecNO_DST; // check if the destination has disallowed incoming offers - if (view.rules().enabled(featureDisallowIncoming)) - { - // flag cannot be set unless amendment is enabled but - // out of an abundance of caution check anyway - - if (sleDst->getFlags() & lsfDisallowIncomingNFTokenOffer) - return tecNO_PERMISSION; - } + if (sleDst->getFlags() & lsfDisallowIncomingNFTokenOffer) + return tecNO_PERMISSION; } if (owner) { - // Check if the owner (buy offer) has disallowed incoming offers - if (view.rules().enabled(featureDisallowIncoming)) - { - auto const sleOwner = view.read(keylet::account(*owner)); + auto const sleOwner = view.read(keylet::account(*owner)); - // defensively check - // it should not be possible to specify owner that doesn't exist - if (!sleOwner) - return tecNO_TARGET; + // defensively check + // it should not be possible to specify owner that doesn't exist + if (!sleOwner) + return tecNO_TARGET; - if (sleOwner->getFlags() & lsfDisallowIncomingNFTokenOffer) - return tecNO_PERMISSION; - } + if (sleOwner->getFlags() & lsfDisallowIncomingNFTokenOffer) + return tecNO_PERMISSION; } if (view.rules().enabled(fixEnforceNFTokenTrustlineV2) && !amount.native()) diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index c3dc99e7fb..b52ae78523 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -202,8 +202,7 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) auto const flags = sled->getFlags(); // Check if they have disallowed incoming payment channels - if (ctx.view.rules().enabled(featureDisallowIncoming) && - (flags & lsfDisallowIncomingPayChan)) + if (flags & lsfDisallowIncomingPayChan) return tecNO_PERMISSION; if ((flags & lsfRequireDestTag) && !ctx.tx[~sfDestinationTag]) diff --git a/src/xrpld/app/tx/detail/SetAccount.cpp b/src/xrpld/app/tx/detail/SetAccount.cpp index db513d9f05..e860eca22d 100644 --- a/src/xrpld/app/tx/detail/SetAccount.cpp +++ b/src/xrpld/app/tx/detail/SetAccount.cpp @@ -595,29 +595,25 @@ SetAccount::doApply() sle->isFieldPresent(sfNFTokenMinter)) sle->makeFieldAbsent(sfNFTokenMinter); - // Set or clear flags for disallowing various incoming instruments - if (ctx_.view().rules().enabled(featureDisallowIncoming)) - { - if (uSetFlag == asfDisallowIncomingNFTokenOffer) - uFlagsOut |= lsfDisallowIncomingNFTokenOffer; - else if (uClearFlag == asfDisallowIncomingNFTokenOffer) - uFlagsOut &= ~lsfDisallowIncomingNFTokenOffer; + if (uSetFlag == asfDisallowIncomingNFTokenOffer) + uFlagsOut |= lsfDisallowIncomingNFTokenOffer; + else if (uClearFlag == asfDisallowIncomingNFTokenOffer) + uFlagsOut &= ~lsfDisallowIncomingNFTokenOffer; - if (uSetFlag == asfDisallowIncomingCheck) - uFlagsOut |= lsfDisallowIncomingCheck; - else if (uClearFlag == asfDisallowIncomingCheck) - uFlagsOut &= ~lsfDisallowIncomingCheck; + if (uSetFlag == asfDisallowIncomingCheck) + uFlagsOut |= lsfDisallowIncomingCheck; + else if (uClearFlag == asfDisallowIncomingCheck) + uFlagsOut &= ~lsfDisallowIncomingCheck; - if (uSetFlag == asfDisallowIncomingPayChan) - uFlagsOut |= lsfDisallowIncomingPayChan; - else if (uClearFlag == asfDisallowIncomingPayChan) - uFlagsOut &= ~lsfDisallowIncomingPayChan; + if (uSetFlag == asfDisallowIncomingPayChan) + uFlagsOut |= lsfDisallowIncomingPayChan; + else if (uClearFlag == asfDisallowIncomingPayChan) + uFlagsOut &= ~lsfDisallowIncomingPayChan; - if (uSetFlag == asfDisallowIncomingTrustline) - uFlagsOut |= lsfDisallowIncomingTrustline; - else if (uClearFlag == asfDisallowIncomingTrustline) - uFlagsOut &= ~lsfDisallowIncomingTrustline; - } + if (uSetFlag == asfDisallowIncomingTrustline) + uFlagsOut |= lsfDisallowIncomingTrustline; + else if (uClearFlag == asfDisallowIncomingTrustline) + uFlagsOut &= ~lsfDisallowIncomingTrustline; // Set or clear flags for disallowing escrow if (ctx_.view().rules().enabled(featureTokenEscrow)) diff --git a/src/xrpld/app/tx/detail/SetTrust.cpp b/src/xrpld/app/tx/detail/SetTrust.cpp index 8d5f335f47..4e8f20f3ae 100644 --- a/src/xrpld/app/tx/detail/SetTrust.cpp +++ b/src/xrpld/app/tx/detail/SetTrust.cpp @@ -200,31 +200,27 @@ SetTrust::preclaim(PreclaimContext const& ctx) // This might be nullptr auto const sleDst = ctx.view.read(keylet::account(uDstAccountID)); - if ((ctx.view.rules().enabled(featureDisallowIncoming) || - ammEnabled(ctx.view.rules()) || + if ((ammEnabled(ctx.view.rules()) || ctx.view.rules().enabled(featureSingleAssetVault)) && sleDst == nullptr) return tecNO_DST; // If the destination has opted to disallow incoming trustlines // then honour that flag - if (ctx.view.rules().enabled(featureDisallowIncoming)) + if (sleDst->getFlags() & lsfDisallowIncomingTrustline) { - if (sleDst->getFlags() & lsfDisallowIncomingTrustline) + // The original implementation of featureDisallowIncoming was + // too restrictive. If + // o fixDisallowIncomingV1 is enabled and + // o The trust line already exists + // Then allow the TrustSet. + if (ctx.view.rules().enabled(fixDisallowIncomingV1) && + ctx.view.exists(keylet::line(id, uDstAccountID, currency))) { - // The original implementation of featureDisallowIncoming was - // too restrictive. If - // o fixDisallowIncomingV1 is enabled and - // o The trust line already exists - // Then allow the TrustSet. - if (ctx.view.rules().enabled(fixDisallowIncomingV1) && - ctx.view.exists(keylet::line(id, uDstAccountID, currency))) - { - // pass - } - else - return tecNO_PERMISSION; + // pass } + else + return tecNO_PERMISSION; } // In general, trust lines to pseudo accounts are not permitted, unless diff --git a/src/xrpld/rpc/detail/RPCCall.cpp b/src/xrpld/rpc/detail/RPCCall.cpp index 915f951749..185043302a 100644 --- a/src/xrpld/rpc/detail/RPCCall.cpp +++ b/src/xrpld/rpc/detail/RPCCall.cpp @@ -332,15 +332,31 @@ private: if (jvParams.size() >= 5) { - int iLimit = jvParams[5u].asInt(); + try + { + int iLimit = jvParams[4u].asInt(); - if (iLimit > 0) - jvRequest[jss::limit] = iLimit; + if (iLimit > 0) + jvRequest[jss::limit] = iLimit; + } + catch (std::exception const&) + { + return RPC::invalid_field_error(jss::limit); + } } - if (jvParams.size() >= 6 && jvParams[5u].asInt()) + if (jvParams.size() >= 6) { - jvRequest[jss::proof] = true; + try + { + int bProof = jvParams[5u].asInt(); + if (bProof) + jvRequest[jss::proof] = true; + } + catch (std::exception const&) + { + return RPC::invalid_field_error(jss::proof); + } } if (jvParams.size() == 7) diff --git a/src/xrpld/rpc/handlers/AccountInfo.cpp b/src/xrpld/rpc/handlers/AccountInfo.cpp index 683077be6e..e9f95d450b 100644 --- a/src/xrpld/rpc/handlers/AccountInfo.cpp +++ b/src/xrpld/rpc/handlers/AccountInfo.cpp @@ -116,11 +116,8 @@ doAccountInfo(RPC::JsonContext& context) for (auto const& lsf : lsFlags) acctFlags[lsf.first.data()] = sleAccepted->isFlag(lsf.second); - if (ledger->rules().enabled(featureDisallowIncoming)) - { - for (auto const& lsf : disallowIncomingFlags) - acctFlags[lsf.first.data()] = sleAccepted->isFlag(lsf.second); - } + for (auto const& lsf : disallowIncomingFlags) + acctFlags[lsf.first.data()] = sleAccepted->isFlag(lsf.second); if (ledger->rules().enabled(featureClawback)) acctFlags[allowTrustLineClawbackFlag.first.data()] = diff --git a/src/xrpld/rpc/handlers/LedgerEntry.cpp b/src/xrpld/rpc/handlers/LedgerEntry.cpp index b8b17e6b59..5a1dfb4ace 100644 --- a/src/xrpld/rpc/handlers/LedgerEntry.cpp +++ b/src/xrpld/rpc/handlers/LedgerEntry.cpp @@ -16,8 +16,6 @@ #include #include -#include - namespace ripple { static Expected @@ -178,18 +176,41 @@ static Expected parseAuthorizeCredentials(Json::Value const& jv) { if (!jv.isArray()) + { return LedgerEntryHelpers::invalidFieldError( "malformedAuthorizedCredentials", jss::authorized_credentials, "array"); - STArray arr(sfAuthorizeCredentials, jv.size()); + } + + std::uint32_t const n = jv.size(); + if (n > maxCredentialsArraySize) + { + return Unexpected(LedgerEntryHelpers::malformedError( + "malformedAuthorizedCredentials", + "Invalid field '" + std::string(jss::authorized_credentials) + + "', array too long.")); + } + + if (n == 0) + { + return Unexpected(LedgerEntryHelpers::malformedError( + "malformedAuthorizedCredentials", + "Invalid field '" + std::string(jss::authorized_credentials) + + "', array empty.")); + } + + STArray arr(sfAuthorizeCredentials, n); for (auto const& jo : jv) { if (!jo.isObject()) + { return LedgerEntryHelpers::invalidFieldError( "malformedAuthorizedCredentials", jss::authorized_credentials, "array"); + } + if (auto const value = LedgerEntryHelpers::hasRequired( jo, {jss::issuer, jss::credential_type}, @@ -260,13 +281,6 @@ parseDepositPreauth(Json::Value const& dp, Json::StaticString const fieldName) auto const arr = parseAuthorizeCredentials(ac); if (!arr.has_value()) return Unexpected(arr.error()); - if (arr->empty() || (arr->size() > maxCredentialsArraySize)) - { - return LedgerEntryHelpers::invalidFieldError( - "malformedAuthorizedCredentials", - jss::authorized_credentials, - "array"); - } auto const& sorted = credentials::makeSorted(arr.value()); if (sorted.empty())