diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 4a8bc71321..768897f665 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -55,6 +55,10 @@ on: description: "The Codecov token to use for uploading coverage reports." required: true +defaults: + run: + shell: bash + jobs: build-and-test: name: ${{ inputs.config_name }} @@ -65,9 +69,9 @@ jobs: ENABLED_VOIDSTAR: ${{ contains(inputs.cmake_args, '-Dvoidstar=ON') }} ENABLED_COVERAGE: ${{ contains(inputs.cmake_args, '-Dcoverage=ON') }} steps: - - name: Cleanup workspace - if: ${{ runner.os == 'macOS' }} - uses: XRPLF/actions/.github/actions/cleanup-workspace@3f044c7478548e3c32ff68980eeb36ece02b364e + - name: Cleanup workspace (macOS and Windows) + if: ${{ runner.os == 'macOS' || runner.os == 'Windows' }} + uses: XRPLF/actions/.github/actions/cleanup-workspace@01b244d2718865d427b499822fbd3f15e7197fcc - name: Checkout repository uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 @@ -100,7 +104,6 @@ jobs: log_verbosity: ${{ runner.os == 'Windows' && 'quiet' || 'verbose' }} - name: Configure CMake - shell: bash working-directory: ${{ inputs.build_dir }} env: BUILD_TYPE: ${{ inputs.build_type }} @@ -114,7 +117,6 @@ jobs: .. - name: Build the binary - shell: bash working-directory: ${{ inputs.build_dir }} env: BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} @@ -141,7 +143,6 @@ jobs: - name: Check linking (Linux) if: ${{ runner.os == 'Linux' }} working-directory: ${{ inputs.build_dir }} - shell: bash run: | ldd ./rippled if [ "$(ldd ./rippled | grep -E '(libstdc\+\+|libgcc)' | wc -l)" -eq 0 ]; then @@ -154,7 +155,6 @@ jobs: - name: Verify presence of instrumentation (Linux) if: ${{ runner.os == 'Linux' && env.ENABLED_VOIDSTAR == 'true' }} working-directory: ${{ inputs.build_dir }} - shell: bash run: | ./rippled --version | grep libvoidstar @@ -165,20 +165,34 @@ jobs: env: BUILD_TYPE: ${{ inputs.build_type }} PARALLELISM: ${{ runner.os == 'Windows' && '1' || steps.nproc.outputs.nproc }} - shell: bash run: | ctest \ --output-on-failure \ -C "${BUILD_TYPE}" \ -j "${PARALLELISM}" + - name: Run the embedded tests + if: ${{ !inputs.build_only }} + working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', inputs.build_dir, inputs.build_type) || inputs.build_dir }} + env: + BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} + run: | + ./rippled --unittest --unittest-jobs "${BUILD_NPROC}" + + - name: Debug failure (Linux) + if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }} + run: | + echo "IPv4 local port range:" + cat /proc/sys/net/ipv4/ip_local_port_range + echo "Netstat:" + netstat -an + - name: Prepare coverage report if: ${{ !inputs.build_only && env.ENABLED_COVERAGE == 'true' }} working-directory: ${{ inputs.build_dir }} env: BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} BUILD_TYPE: ${{ inputs.build_type }} - shell: bash run: | cmake \ --build . \ @@ -197,21 +211,3 @@ jobs: plugins: noop token: ${{ secrets.CODECOV_TOKEN }} verbose: true - - - name: Run the embedded tests - if: ${{ !inputs.build_only && env.ENABLED_COVERAGE != 'true' }} - working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', inputs.build_dir, inputs.build_type) || inputs.build_dir }} - shell: bash - env: - BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} - run: | - ./rippled --unittest --unittest-jobs "${BUILD_NPROC}" - - - name: Debug failure (Linux) - if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }} - shell: bash - run: | - echo "IPv4 local port range:" - cat /proc/sys/net/ipv4/ip_local_port_range - echo "Netstat:" - netstat -an diff --git a/.github/workflows/reusable-strategy-matrix.yml b/.github/workflows/reusable-strategy-matrix.yml index 129f65938b..c975347307 100644 --- a/.github/workflows/reusable-strategy-matrix.yml +++ b/.github/workflows/reusable-strategy-matrix.yml @@ -18,6 +18,10 @@ on: description: "The generated strategy matrix." value: ${{ jobs.generate-matrix.outputs.matrix }} +defaults: + run: + shell: bash + jobs: generate-matrix: runs-on: ubuntu-latest diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index f6262a2f1f..320396c899 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -40,6 +40,10 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +defaults: + run: + shell: bash + jobs: # Generate the strategy matrix to be used by the following job. generate-matrix: @@ -58,9 +62,9 @@ jobs: runs-on: ${{ matrix.architecture.runner }} container: ${{ contains(matrix.architecture.platform, 'linux') && format('ghcr.io/xrplf/ci/{0}-{1}:{2}-{3}-sha-{4}', matrix.os.distro_name, matrix.os.distro_version, matrix.os.compiler_name, matrix.os.compiler_version, matrix.os.image_sha) || null }} steps: - - name: Cleanup workspace - if: ${{ runner.os == 'macOS' }} - uses: XRPLF/actions/.github/actions/cleanup-workspace@3f044c7478548e3c32ff68980eeb36ece02b364e + - name: Cleanup workspace (macOS and Windows) + if: ${{ runner.os == 'macOS' || runner.os == 'Windows' }} + uses: XRPLF/actions/.github/actions/cleanup-workspace@01b244d2718865d427b499822fbd3f15e7197fcc - name: Checkout repository uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 diff --git a/BUILD.md b/BUILD.md index f13204ea4c..39570edbd3 100644 --- a/BUILD.md +++ b/BUILD.md @@ -495,18 +495,18 @@ A coverage report is created when the following steps are completed, in order: 1. `rippled` binary built with instrumentation data, enabled by the `coverage` option mentioned above -2. completed run of unit tests, which populates coverage capture data +2. completed one or more run of the unit tests, which populates coverage capture data 3. completed run of the `gcovr` tool (which internally invokes either `gcov` or `llvm-cov`) to assemble both instrumentation data and the coverage capture data into a coverage report -The above steps are automated into a single target `coverage`. The instrumented +The last step of the above is automated into a single target `coverage`. The instrumented `rippled` binary can also be used for regular development or testing work, at the cost of extra disk space utilization and a small performance hit -(to store coverage capture). In case of a spurious failure of unit tests, it is -possible to re-run the `coverage` target without rebuilding the `rippled` binary -(since it is simply a dependency of the coverage report target). It is also possible -to select only specific tests for the purpose of the coverage report, by setting -the `coverage_test` variable in `cmake` +(to store coverage capture data). Since `rippled` binary is simply a dependency of the +coverage report target, it is possible to re-run the `coverage` target without +rebuilding the `rippled` binary. Note, running of the unit tests before the `coverage` +target is left to the developer. Each such run will append to the coverage data +collected in the build directory. The default coverage report format is `html-details`, but the user can override it to any of the formats listed in `Builds/CMake/CodeCoverage.cmake` @@ -515,11 +515,6 @@ to generate more than one format at a time by setting the `coverage_extra_args` variable in `cmake`. The specific command line used to run the `gcovr` tool will be displayed if the `CODE_COVERAGE_VERBOSE` variable is set. -By default, the code coverage tool runs parallel unit tests with `--unittest-jobs` -set to the number of available CPU cores. This may cause spurious test -errors on Apple. Developers can override the number of unit test jobs with -the `coverage_test_parallelism` variable in `cmake`. - Example use with some cmake variables set: ``` diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake index c2b66c9cac..e1b44e656d 100644 --- a/cmake/CodeCoverage.cmake +++ b/cmake/CodeCoverage.cmake @@ -109,6 +109,9 @@ # - add a new function add_code_coverage_to_target # - remove some unused code # +# 2025-11-11, Bronek Kozicki +# - make EXECUTABLE and EXECUTABLE_ARGS optional +# # USAGE: # # 1. Copy this file into your cmake modules path. @@ -317,6 +320,10 @@ function(setup_target_for_coverage_gcovr) set(Coverage_FORMAT xml) endif() + if(NOT DEFINED Coverage_EXECUTABLE AND DEFINED Coverage_EXECUTABLE_ARGS) + message(FATAL_ERROR "EXECUTABLE_ARGS must not be set if EXECUTABLE is not set") + endif() + if("--output" IN_LIST GCOVR_ADDITIONAL_ARGS) message(FATAL_ERROR "Unsupported --output option detected in GCOVR_ADDITIONAL_ARGS! Aborting...") else() @@ -398,17 +405,18 @@ function(setup_target_for_coverage_gcovr) endforeach() # Set up commands which will be run to generate coverage data - # Run tests - set(GCOVR_EXEC_TESTS_CMD - ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} - ) + # If EXECUTABLE is not set, the user is expected to run the tests manually + # before running the coverage target NAME + if(DEFINED Coverage_EXECUTABLE) + set(GCOVR_EXEC_TESTS_CMD + ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} + ) + endif() # Create folder if(DEFINED GCOVR_CREATE_FOLDER) set(GCOVR_FOLDER_CMD ${CMAKE_COMMAND} -E make_directory ${GCOVR_CREATE_FOLDER}) - else() - set(GCOVR_FOLDER_CMD echo) # dummy endif() # Running gcovr @@ -425,11 +433,13 @@ function(setup_target_for_coverage_gcovr) if(CODE_COVERAGE_VERBOSE) message(STATUS "Executed command report") - message(STATUS "Command to run tests: ") - string(REPLACE ";" " " GCOVR_EXEC_TESTS_CMD_SPACED "${GCOVR_EXEC_TESTS_CMD}") - message(STATUS "${GCOVR_EXEC_TESTS_CMD_SPACED}") + if(NOT "${GCOVR_EXEC_TESTS_CMD}" STREQUAL "") + message(STATUS "Command to run tests: ") + string(REPLACE ";" " " GCOVR_EXEC_TESTS_CMD_SPACED "${GCOVR_EXEC_TESTS_CMD}") + message(STATUS "${GCOVR_EXEC_TESTS_CMD_SPACED}") + endif() - if(NOT GCOVR_FOLDER_CMD STREQUAL "echo") + if(NOT "${GCOVR_FOLDER_CMD}" STREQUAL "") message(STATUS "Command to create a folder: ") string(REPLACE ";" " " GCOVR_FOLDER_CMD_SPACED "${GCOVR_FOLDER_CMD}") message(STATUS "${GCOVR_FOLDER_CMD_SPACED}") diff --git a/cmake/XrplCov.cmake b/cmake/XrplCov.cmake index 288f74c0ef..b212d60b64 100644 --- a/cmake/XrplCov.cmake +++ b/cmake/XrplCov.cmake @@ -11,6 +11,9 @@ if(CMAKE_CXX_COMPILER_ID MATCHES "MSVC") return() endif() +include(ProcessorCount) +ProcessorCount(PROCESSOR_COUNT) + include(CodeCoverage) # The instructions for these commands come from the `CodeCoverage` module, @@ -26,15 +29,13 @@ list(APPEND GCOVR_ADDITIONAL_ARGS --exclude-throw-branches --exclude-noncode-lines --exclude-unreachable-branches -s - -j ${coverage_test_parallelism}) + -j ${PROCESSOR_COUNT}) setup_target_for_coverage_gcovr( NAME coverage FORMAT ${coverage_format} - EXECUTABLE xrpld - EXECUTABLE_ARGS --unittest$<$:=${coverage_test}> --unittest-jobs ${coverage_test_parallelism} --quiet --unittest-log EXCLUDE "src/test" "src/tests" "include/xrpl/beast/test" "include/xrpl/beast/unit_test" "${CMAKE_BINARY_DIR}/pb-xrpl.libpb" - DEPENDENCIES xrpld + DEPENDENCIES xrpld xrpl.tests ) add_code_coverage_to_target(opts INTERFACE) diff --git a/cmake/XrplSettings.cmake b/cmake/XrplSettings.cmake index be339c135e..a16513afc5 100644 --- a/cmake/XrplSettings.cmake +++ b/cmake/XrplSettings.cmake @@ -51,17 +51,10 @@ if(is_gcc OR is_clang) option(coverage "Generates coverage info." OFF) option(profile "Add profiling flags" OFF) - set(coverage_test_parallelism "${PROCESSOR_COUNT}" CACHE STRING - "Unit tests parallelism for the purpose of coverage report.") set(coverage_format "html-details" CACHE STRING "Output format of the coverage report.") set(coverage_extra_args "" CACHE STRING "Additional arguments to pass to gcovr.") - set(coverage_test "" CACHE STRING - "On gcc & clang, the specific unit test(s) to run for coverage. Default is all tests.") - if(coverage_test AND NOT coverage) - set(coverage ON CACHE BOOL "gcc/clang only" FORCE) - endif() option(wextra "compile with extra gcc/clang warnings enabled" ON) else() set(profile OFF CACHE BOOL "gcc/clang only" FORCE) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 42f2db41f2..193f0665dc 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -78,12 +78,15 @@ namespace detail { #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX -#pragma push_macro("XRPL_RETIRE") -#undef XRPL_RETIRE +#pragma push_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FEATURE +#pragma push_macro("XRPL_RETIRE_FIX") +#undef XRPL_RETIRE_FIX #define XRPL_FEATURE(name, supported, vote) +1 #define XRPL_FIX(name, supported, vote) +1 -#define XRPL_RETIRE(name) +1 +#define XRPL_RETIRE_FEATURE(name) +1 +#define XRPL_RETIRE_FIX(name) +1 // This value SHOULD be equal to the number of amendments registered in // Feature.cpp. Because it's only used to reserve storage, and determine how @@ -94,8 +97,10 @@ static constexpr std::size_t numFeatures = #include ); -#undef XRPL_RETIRE -#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_RETIRE_FEATURE +#pragma pop_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FIX +#pragma pop_macro("XRPL_RETIRE_FIX") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE @@ -339,17 +344,22 @@ foreachFeature(FeatureBitset bs, F&& f) #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX -#pragma push_macro("XRPL_RETIRE") -#undef XRPL_RETIRE +#pragma push_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FEATURE +#pragma push_macro("XRPL_RETIRE_FIX") +#undef XRPL_RETIRE_FIX #define XRPL_FEATURE(name, supported, vote) extern uint256 const feature##name; #define XRPL_FIX(name, supported, vote) extern uint256 const fix##name; -#define XRPL_RETIRE(name) +#define XRPL_RETIRE_FEATURE(name) +#define XRPL_RETIRE_FIX(name) #include -#undef XRPL_RETIRE -#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_RETIRE_FEATURE +#pragma pop_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FIX +#pragma pop_macro("XRPL_RETIRE_FIX") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE diff --git a/include/xrpl/protocol/TxFlags.h b/include/xrpl/protocol/TxFlags.h index d3a2fecf45..0f3ea94b21 100644 --- a/include/xrpl/protocol/TxFlags.h +++ b/include/xrpl/protocol/TxFlags.h @@ -274,10 +274,11 @@ constexpr std::uint32_t const tfLoanOverpayment = 0x00010000; // LoanPay exclusive flags: // tfLoanFullPayment: True, indicates that the payment is constexpr std::uint32_t const tfLoanFullPayment = 0x00020000; +constexpr std::uint32_t const tfLoanLatePayment = 0x00040000; constexpr std::uint32_t const tfLoanSetMask = ~(tfUniversal | tfLoanOverpayment); constexpr std::uint32_t const tfLoanPayMask = ~(tfUniversal | - tfLoanOverpayment | tfLoanFullPayment); + tfLoanOverpayment | tfLoanFullPayment | tfLoanLatePayment); // LoanManage flags: constexpr std::uint32_t const tfLoanDefault = 0x00010000; diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index eb68006428..493c5a034d 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -4,8 +4,11 @@ #if !defined(XRPL_FIX) #error "undefined macro: XRPL_FIX" #endif -#if !defined(XRPL_RETIRE) -#error "undefined macro: XRPL_RETIRE" +#if !defined(XRPL_RETIRE_FEATURE) +#error "undefined macro: XRPL_RETIRE_FEATURE" +#endif +#if !defined(XRPL_RETIRE_FIX) +#error "undefined macro: XRPL_RETIRE_FIX" #endif // clang-format off @@ -71,11 +74,8 @@ XRPL_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYe XRPL_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (PayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) // The following amendments are obsolete, but must remain supported @@ -96,44 +96,48 @@ XRPL_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete) // All known amendments and amendments that may appear in a validated ledger // must be registered either here or above with the "active" amendments // -// Please keep this list sorted alphabetically for convenience. -XRPL_RETIRE(fix1201) -XRPL_RETIRE(fix1368) -XRPL_RETIRE(fix1373) -XRPL_RETIRE(fix1512) -XRPL_RETIRE(fix1513) -XRPL_RETIRE(fix1515) -XRPL_RETIRE(fix1523) -XRPL_RETIRE(fix1528) -XRPL_RETIRE(fix1543) -XRPL_RETIRE(fix1571) -XRPL_RETIRE(fix1578) -XRPL_RETIRE(fix1623) -XRPL_RETIRE(fix1781) -XRPL_RETIRE(fixAmendmentMajorityCalc) -XRPL_RETIRE(fixCheckThreading) -XRPL_RETIRE(fixNonFungibleTokensV1_2) -XRPL_RETIRE(fixNFTokenRemint) -XRPL_RETIRE(fixMasterKeyAsRegularKey) -XRPL_RETIRE(fixQualityUpperBound) -XRPL_RETIRE(fixReducedOffersV1) -XRPL_RETIRE(fixRmSmallIncreasedQOffers) -XRPL_RETIRE(fixSTAmountCanonicalize) -XRPL_RETIRE(fixTakerDryOfferRemoval) -XRPL_RETIRE(fixTrustLinesToSelf) -XRPL_RETIRE(CheckCashMakesTrustLine) -XRPL_RETIRE(CryptoConditions) -XRPL_RETIRE(Escrow) -XRPL_RETIRE(EnforceInvariants) -XRPL_RETIRE(FeeEscalation) -XRPL_RETIRE(FlowCross) -XRPL_RETIRE(HardenedValidations) -XRPL_RETIRE(ImmediateOfferKilled) -XRPL_RETIRE(MultiSign) -XRPL_RETIRE(NonFungibleTokensV1_1) -XRPL_RETIRE(PayChan) -XRPL_RETIRE(SortedDirectories) -XRPL_RETIRE(TickSize) -XRPL_RETIRE(TrustSetAuth) +// Please keep both lists sorted alphabetically for convenience. +XRPL_RETIRE_FIX(1201) +XRPL_RETIRE_FIX(1368) +XRPL_RETIRE_FIX(1373) +XRPL_RETIRE_FIX(1512) +XRPL_RETIRE_FIX(1513) +XRPL_RETIRE_FIX(1515) +XRPL_RETIRE_FIX(1523) +XRPL_RETIRE_FIX(1528) +XRPL_RETIRE_FIX(1543) +XRPL_RETIRE_FIX(1571) +XRPL_RETIRE_FIX(1578) +XRPL_RETIRE_FIX(1623) +XRPL_RETIRE_FIX(1781) +XRPL_RETIRE_FIX(AmendmentMajorityCalc) +XRPL_RETIRE_FIX(CheckThreading) +XRPL_RETIRE_FIX(MasterKeyAsRegularKey) +XRPL_RETIRE_FIX(NonFungibleTokensV1_2) +XRPL_RETIRE_FIX(NFTokenRemint) +XRPL_RETIRE_FIX(PayChanRecipientOwnerDir) +XRPL_RETIRE_FIX(QualityUpperBound) +XRPL_RETIRE_FIX(ReducedOffersV1) +XRPL_RETIRE_FIX(RmSmallIncreasedQOffers) +XRPL_RETIRE_FIX(STAmountCanonicalize) +XRPL_RETIRE_FIX(TakerDryOfferRemoval) +XRPL_RETIRE_FIX(TrustLinesToSelf) + +XRPL_RETIRE_FEATURE(CheckCashMakesTrustLine) +XRPL_RETIRE_FEATURE(CryptoConditions) +XRPL_RETIRE_FEATURE(DepositAuth) +XRPL_RETIRE_FEATURE(DepositPreauth) +XRPL_RETIRE_FEATURE(Escrow) +XRPL_RETIRE_FEATURE(EnforceInvariants) +XRPL_RETIRE_FEATURE(FeeEscalation) +XRPL_RETIRE_FEATURE(FlowCross) +XRPL_RETIRE_FEATURE(HardenedValidations) +XRPL_RETIRE_FEATURE(ImmediateOfferKilled) +XRPL_RETIRE_FEATURE(MultiSign) +XRPL_RETIRE_FEATURE(NonFungibleTokensV1_1) +XRPL_RETIRE_FEATURE(PayChan) +XRPL_RETIRE_FEATURE(SortedDirectories) +XRPL_RETIRE_FEATURE(TickSize) +XRPL_RETIRE_FEATURE(TrustSetAuth) // clang-format on diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 9e9a0c17a4..5f6d1b844e 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -542,7 +542,7 @@ LEDGER_ENTRY(ltLOAN, 0x0089, Loan, loan, ({ {sfPaymentInterval, soeREQUIRED}, {sfGracePeriod, soeDEFAULT}, {sfPreviousPaymentDate, soeDEFAULT}, - {sfNextPaymentDueDate, soeOPTIONAL}, + {sfNextPaymentDueDate, soeDEFAULT}, // The loan object tracks these values: // // - PaymentRemaining: The number of payments left in the loan. When it diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index fb99e9e3d0..e922813cca 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -268,7 +268,7 @@ TRANSACTION(ttCHECK_CANCEL, 18, CheckCancel, #endif TRANSACTION(ttDEPOSIT_PREAUTH, 19, DepositPreauth, Delegation::delegatable, - featureDepositPreauth, + uint256{}, noPriv, ({ {sfAuthorize, soeOPTIONAL}, diff --git a/src/libxrpl/basics/BasicConfig.cpp b/src/libxrpl/basics/BasicConfig.cpp index 280c660794..63a001bcc3 100644 --- a/src/libxrpl/basics/BasicConfig.cpp +++ b/src/libxrpl/basics/BasicConfig.cpp @@ -30,7 +30,7 @@ Section::append(std::vector const& lines) // '=' static boost::regex const re1( "^" // start of line - "(?:\\s*)" // whitespace (optonal) + "(?:\\s*)" // whitespace (optional) "([a-zA-Z][_a-zA-Z0-9]*)" // "(?:\\s*)" // whitespace (optional) "(?:=)" // '=' diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index deaf53c05b..fa9e71f113 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2113,26 +2113,26 @@ static TER rippleSendMultiIOU( ApplyView& view, AccountID const& senderID, - Asset const& asset, + Issue const& issue, MultiplePaymentDestinations const& receivers, STAmount& actual, beast::Journal j, WaiveTransferFee waiveFee) { - auto const issuer = asset.getIssuer(); + auto const issuer = issue.getIssuer(); XRPL_ASSERT( !isXRP(senderID), "ripple::rippleSendMultiIOU : sender is not XRP"); // These may diverge - STAmount takeFromSender{asset}; + STAmount takeFromSender{issue}; actual = takeFromSender; // Failures return immediately. for (auto const& r : receivers) { auto const& receiverID = r.first; - STAmount amount{asset, r.second}; + STAmount amount{issue, r.second}; /* If we aren't sending anything or if the sender is the same as the * receiver then we don't need to do anything. @@ -2315,7 +2315,7 @@ static TER accountSendMultiIOU( ApplyView& view, AccountID const& senderID, - Asset const& asset, + Issue const& issue, MultiplePaymentDestinations const& receivers, beast::Journal j, WaiveTransferFee waiveFee) @@ -2325,27 +2325,14 @@ accountSendMultiIOU( "ripple::accountSendMultiIOU", "multiple recipients provided"); - if (view.rules().enabled(fixAMMv1_1)) - { - if (asset.holds()) - { - return tecINTERNAL; - } - } - else - { - XRPL_ASSERT( - !asset.holds(), "ripple::accountSendMultiIOU : not MPT"); - } - - if (!asset.native()) + if (!issue.native()) { STAmount actual; JLOG(j.trace()) << "accountSendMultiIOU: " << to_string(senderID) << " sending " << receivers.size() << " IOUs"; return rippleSendMultiIOU( - view, senderID, asset, receivers, actual, j, waiveFee); + view, senderID, issue, receivers, actual, j, waiveFee); } /* XRP send which does not check reserve and can do pure adjustment. @@ -2370,26 +2357,15 @@ accountSendMultiIOU( } // Failures return immediately. - STAmount takeFromSender{asset}; + STAmount takeFromSender{issue}; for (auto const& r : receivers) { auto const& receiverID = r.first; - STAmount amount{asset, r.second}; + STAmount amount{issue, r.second}; - takeFromSender += amount; - - if (view.rules().enabled(fixAMMv1_1)) + if (amount < beast::zero) { - if (amount < beast::zero) - { - return tecINTERNAL; - } - } - else - { - XRPL_ASSERT( - amount >= beast::zero, - "ripple::accountSendMultiIOU : minimum amount"); + return tecINTERNAL; // LCOV_EXCL_LINE } /* If we aren't sending anything or if the sender is the same as the @@ -2423,6 +2399,9 @@ accountSendMultiIOU( view.creditHook(xrpAccount(), receiverID, amount, -rcvBal); view.update(receiver); + + // Take what is actually sent + takeFromSender += amount; } if (auto stream = j.trace()) @@ -2602,7 +2581,7 @@ static TER rippleSendMultiMPT( ApplyView& view, AccountID const& senderID, - Asset const& asset, + MPTIssue const& mptIssue, MultiplePaymentDestinations const& receivers, STAmount& actual, beast::Journal j, @@ -2610,29 +2589,25 @@ rippleSendMultiMPT( { // Safe to get MPT since rippleSendMultiMPT is only called by // accountSendMultiMPT - auto const issuer = asset.getIssuer(); + auto const issuer = mptIssue.getIssuer(); - auto const sle = - view.read(keylet::mptIssuance(asset.get().getMptID())); + auto const sle = view.read(keylet::mptIssuance(mptIssue.getMptID())); if (!sle) return tecOBJECT_NOT_FOUND; // These may diverge - STAmount takeFromSender{asset}; + STAmount takeFromSender{mptIssue}; actual = takeFromSender; for (auto const& r : receivers) { auto const& receiverID = r.first; - STAmount amount{asset, r.second}; + STAmount amount{mptIssue, r.second}; - XRPL_ASSERT( - senderID != receiverID, - "ripple::rippleSendMultiMPT : sender is not receiver"); - - XRPL_ASSERT( - amount >= beast::zero, - "ripple::rippleSendMultiMPT : minimum amount "); + if (amount < beast::zero) + { + return tecINTERNAL; // LCOV_EXCL_LINE + } /* If we aren't sending anything or if the sender is the same as the * receiver then we don't need to do anything. @@ -2723,17 +2698,15 @@ static TER accountSendMultiMPT( ApplyView& view, AccountID const& senderID, - Asset const& asset, + MPTIssue const& mptIssue, MultiplePaymentDestinations const& receivers, beast::Journal j, WaiveTransferFee waiveFee) { - XRPL_ASSERT(asset.holds(), "ripple::accountSendMultiMPT : MPT"); - STAmount actual; return rippleSendMultiMPT( - view, senderID, asset, receivers, actual, j, waiveFee); + view, senderID, mptIssue, receivers, actual, j, waiveFee); } TER @@ -2774,10 +2747,10 @@ accountSendMulti( [&](TIss const& issue) { if constexpr (std::is_same_v) return accountSendMultiIOU( - view, senderID, asset, receivers, j, waiveFee); + view, senderID, issue, receivers, j, waiveFee); else return accountSendMultiMPT( - view, senderID, asset, receivers, j, waiveFee); + view, senderID, issue, receivers, j, waiveFee); }, asset.value()); } @@ -3494,7 +3467,7 @@ assetsToSharesDeposit( Number const shareTotal{ unsafe_cast(issuance->at(sfOutstandingAmount)), Number::strong}; - shares = (shareTotal * (assets / assetTotal)).truncate(); + shares = ((shareTotal * assets) / assetTotal).truncate(); return shares; } @@ -3528,7 +3501,7 @@ sharesToAssetsDeposit( Number const shareTotal{ unsafe_cast(issuance->at(sfOutstandingAmount)), Number::strong}; - assets = assetTotal * (shares / shareTotal); + assets = (assetTotal * shares) / shareTotal; return assets; } @@ -3559,7 +3532,7 @@ assetsToSharesWithdraw( Number const shareTotal{ unsafe_cast(issuance->at(sfOutstandingAmount)), Number::strong}; - Number result = shareTotal * (assets / assetTotal); + Number result = (shareTotal * assets) / assetTotal; if (truncate == TruncateShares::yes) result = result.truncate(); shares = result; @@ -3592,7 +3565,7 @@ sharesToAssetsWithdraw( Number const shareTotal{ unsafe_cast(issuance->at(sfOutstandingAmount)), Number::strong}; - assets = assetTotal * (shares / shareTotal); + assets = (assetTotal * shares) / shareTotal; return assets; } diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index af21ce5985..7690ae586b 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -17,7 +17,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "3.0.0-b1" +char const* const versionString = "3.1.0-b0" // clang-format on #if defined(DEBUG) || defined(SANITIZER) diff --git a/src/libxrpl/protocol/Feature.cpp b/src/libxrpl/protocol/Feature.cpp index 8d09378fc7..10c42ccb8a 100644 --- a/src/libxrpl/protocol/Feature.cpp +++ b/src/libxrpl/protocol/Feature.cpp @@ -411,8 +411,10 @@ featureToName(uint256 const& f) #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX -#pragma push_macro("XRPL_RETIRE") -#undef XRPL_RETIRE +#pragma push_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FEATURE +#pragma push_macro("XRPL_RETIRE_FIX") +#undef XRPL_RETIRE_FIX #define XRPL_FEATURE(name, supported, vote) \ uint256 const feature##name = registerFeature(#name, supported, vote); @@ -420,16 +422,23 @@ featureToName(uint256 const& f) uint256 const fix##name = registerFeature("fix" #name, supported, vote); // clang-format off -#define XRPL_RETIRE(name) \ - [[deprecated("The referenced amendment has been retired")]] \ - [[maybe_unused]] \ - uint256 const retired##name = retireFeature(#name); +#define XRPL_RETIRE_FEATURE(name) \ + [[deprecated("The referenced feature amendment has been retired")]] \ + [[maybe_unused]] \ + uint256 const retiredFeature##name = retireFeature(#name); + +#define XRPL_RETIRE_FIX(name) \ + [[deprecated("The referenced fix amendment has been retired")]] \ + [[maybe_unused]] \ + uint256 const retiredFix##name = retireFeature("fix" #name); // clang-format on #include -#undef XRPL_RETIRE -#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_RETIRE_FEATURE +#pragma pop_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FIX +#pragma pop_macro("XRPL_RETIRE_FIX") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 2cfd690183..6b09ea0ce5 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -3039,8 +3039,6 @@ private: using namespace jtx; Account const becky{"becky"}; - bool const supportsPreauth = {features[featureDepositPreauth]}; - // The initial implementation of DepositAuth had a bug where an // account with the DepositAuth flag set could not make a payment // to itself. That bug was fixed in the DepositPreauth amendment. @@ -3068,15 +3066,11 @@ private: env(fset(becky, asfDepositAuth)); env.close(); - // becky pays herself again. Whether it succeeds depends on - // whether featureDepositPreauth is enabled. - TER const expect{ - supportsPreauth ? TER{tesSUCCESS} : TER{tecNO_PERMISSION}}; - + // becky pays herself again. env(pay(becky, becky, USD(10)), path(~USD), sendmax(XRP(10)), - ter(expect)); + ter(tesSUCCESS)); env.close(); } @@ -3784,9 +3778,7 @@ private: void testDepositAuth() { - auto const supported{jtx::testable_amendments()}; - testPayment(supported - featureDepositPreauth); - testPayment(supported); + testPayment(jtx::testable_amendments()); testPayIOU(); } diff --git a/src/test/app/AccountDelete_test.cpp b/src/test/app/AccountDelete_test.cpp index 9328b4e017..9d5d1cd877 100644 --- a/src/test/app/AccountDelete_test.cpp +++ b/src/test/app/AccountDelete_test.cpp @@ -268,12 +268,8 @@ public: testcase("Owned types"); - // We want to test both... - // o Old-style PayChannels without a recipient backlink as well as - // o New-styled PayChannels with the backlink. - // So we start the test using old-style PayChannels. Then we pass - // the amendment to get new-style PayChannels. - Env env{*this, testable_amendments() - fixPayChanRecipientOwnerDir}; + // We want to test PayChannels with the backlink. + Env env{*this, testable_amendments()}; Account const alice("alice"); Account const becky("becky"); Account const gw("gw"); @@ -374,16 +370,14 @@ public: alice, becky, XRP(57), 4s, env.now() + 2s, alice.pk())); env.close(); - // An old-style PayChannel does not add a back link from the - // destination. So with the PayChannel in place becky should be - // able to delete her account, but alice should not. + // With the PayChannel in place becky and alice should not be + // able to delete her account auto const beckyBalance{env.balance(becky)}; env(acctdelete(alice, gw), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); - env(acctdelete(becky, gw), fee(acctDelFee)); - verifyDeliveredAmount(env, beckyBalance - acctDelFee); + env(acctdelete(becky, gw), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); env.close(); - // Alice cancels her PayChannel which will leave her with only offers + // Alice cancels her PayChannel, which will leave her with only offers // in her directory. // Lambda to close a PayChannel. @@ -401,14 +395,8 @@ public: env(payChanClose(alice, alicePayChanKey, alice.pk())); env.close(); - // Now enable the amendment so PayChannels add a backlink from the - // destination. - env.enableFeature(fixPayChanRecipientOwnerDir); - env.close(); - - // gw creates a PayChannel with alice as the destination. With the - // amendment passed this should prevent alice from deleting her - // account. + // gw creates a PayChannel with alice as the destination, this should + // prevent alice from deleting her account. Keylet const gwPayChanKey{keylet::payChan(gw, alice, env.seq(gw))}; env(payChanCreate(gw, alice, XRP(68), 4s, env.now() + 2s, alice.pk())); @@ -430,84 +418,6 @@ public: env.close(); } - void - testResurrection() - { - // Create an account with an old-style PayChannel. Delete the - // destination of the PayChannel then resurrect the destination. - // The PayChannel should still work. - using namespace jtx; - - testcase("Resurrection"); - - // We need an old-style PayChannel that doesn't provide a backlink - // from the destination. So don't enable the amendment with that fix. - Env env{*this, testable_amendments() - fixPayChanRecipientOwnerDir}; - Account const alice("alice"); - Account const becky("becky"); - - env.fund(XRP(10000), alice, becky); - env.close(); - - // Verify that becky's account root is present. - Keylet const beckyAcctKey{keylet::account(becky.id())}; - BEAST_EXPECT(env.closed()->exists(beckyAcctKey)); - - using namespace std::chrono_literals; - Keylet const payChanKey{keylet::payChan(alice, becky, env.seq(alice))}; - auto const payChanXRP = XRP(37); - - env(payChanCreate( - alice, becky, payChanXRP, 4s, env.now() + 1h, alice.pk())); - env.close(); - BEAST_EXPECT(env.closed()->exists(payChanKey)); - - // Close enough ledgers to be able to delete becky's account. - incLgrSeqForAccDel(env, becky); - - auto const beckyPreDelBalance{env.balance(becky)}; - - auto const acctDelFee{drops(env.current()->fees().increment)}; - env(acctdelete(becky, alice), fee(acctDelFee)); - verifyDeliveredAmount(env, beckyPreDelBalance - acctDelFee); - env.close(); - - // Verify that becky's account root is gone. - BEAST_EXPECT(!env.closed()->exists(beckyAcctKey)); - - // All it takes is a large enough XRP payment to resurrect - // becky's account. Try too small a payment. - env(pay(alice, - becky, - drops(env.current()->fees().accountReserve(0)) - XRP(1)), - ter(tecNO_DST_INSUF_XRP)); - env.close(); - - // Actually resurrect becky's account. - env(pay(alice, becky, XRP(10))); - env.close(); - - // becky's account root should be back. - BEAST_EXPECT(env.closed()->exists(beckyAcctKey)); - BEAST_EXPECT(env.balance(becky) == XRP(10)); - - // becky's resurrected account can be the destination of alice's - // PayChannel. - auto payChanClaim = [&]() { - Json::Value jv; - jv[jss::TransactionType] = jss::PaymentChannelClaim; - jv[jss::Account] = alice.human(); - jv[sfChannel.jsonName] = to_string(payChanKey.key); - jv[sfBalance.jsonName] = - payChanXRP.value().getJson(JsonOptions::none); - return jv; - }; - env(payChanClaim()); - env.close(); - - BEAST_EXPECT(env.balance(becky) == XRP(10) + payChanXRP); - } - void testAmendmentEnable() { @@ -1238,7 +1148,6 @@ public: testBasics(); testDirectories(); testOwnedTypes(); - testResurrection(); testAmendmentEnable(); testTooManyOffers(); testImplicitlyCreatedTrustline(); diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index b74d492cc1..2f3b9337e0 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -2543,6 +2543,11 @@ class Batch_test : public beast::unit_test::suite { testcase("loan"); + bool const lendingBatchEnabled = !std::any_of( + Batch::disabledTxTypes.begin(), + Batch::disabledTxTypes.end(), + [](auto const& disabled) { return disabled == ttLOAN_BROKER_SET; }); + using namespace test::jtx; test::jtx::Env env{ @@ -2609,7 +2614,8 @@ class Batch_test : public beast::unit_test::suite { auto const [txIDs, batchID] = submitBatch( env, - temBAD_SIGNATURE, + lendingBatchEnabled ? temBAD_SIGNATURE + : temINVALID_INNER_BATCH, batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2648,7 +2654,8 @@ class Batch_test : public beast::unit_test::suite { auto const [txIDs, batchID] = submitBatch( env, - temBAD_SIGNER, + lendingBatchEnabled ? temBAD_SIGNER + : temINVALID_INNER_BATCH, batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2672,7 +2679,8 @@ class Batch_test : public beast::unit_test::suite auto const batchFee = batch::calcBatchFee(env, 1, 2); auto const [txIDs, batchID] = submitBatch( env, - tesSUCCESS, + lendingBatchEnabled ? TER(tesSUCCESS) + : TER(temINVALID_INNER_BATCH), batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2704,7 +2712,8 @@ class Batch_test : public beast::unit_test::suite auto const batchFee = batch::calcBatchFee(env, 1, 2); auto const [txIDs, batchID] = submitBatch( env, - tesSUCCESS, + lendingBatchEnabled ? TER(tesSUCCESS) + : TER(temINVALID_INNER_BATCH), batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2721,7 +2730,9 @@ class Batch_test : public beast::unit_test::suite } env.close(); BEAST_EXPECT(env.le(brokerKeylet)); - if (auto const sleLoan = env.le(loanKeylet); BEAST_EXPECT(sleLoan)) + if (auto const sleLoan = env.le(loanKeylet); lendingBatchEnabled + ? BEAST_EXPECT(sleLoan) + : !BEAST_EXPECT(!sleLoan)) { BEAST_EXPECT(sleLoan->isFlag(lsfLoanImpaired)); } diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index bec720a556..c0fd4b1f3b 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -1707,7 +1707,6 @@ class Delegate_test : public beast::unit_test::suite {"CheckCreate", featureChecks}, {"CheckCash", featureChecks}, {"CheckCancel", featureChecks}, - {"DepositPreauth", featureDepositPreauth}, {"Clawback", featureClawback}, {"AMMClawback", featureAMMClawback}, {"AMMCreate", featureAMM}, diff --git a/src/test/app/DepositAuth_test.cpp b/src/test/app/DepositAuth_test.cpp index 8d12a31088..54d5dd6254 100644 --- a/src/test/app/DepositAuth_test.cpp +++ b/src/test/app/DepositAuth_test.cpp @@ -33,21 +33,6 @@ struct DepositAuth_test : public beast::unit_test::suite Account const alice{"alice"}; { - // featureDepositAuth is disabled. - Env env(*this, testable_amendments() - featureDepositAuth); - env.fund(XRP(10000), alice); - - // Note that, to support old behavior, invalid flags are ignored. - env(fset(alice, asfDepositAuth)); - env.close(); - BEAST_EXPECT(!hasDepositAuth(env, alice)); - - env(fclear(alice, asfDepositAuth)); - env.close(); - BEAST_EXPECT(!hasDepositAuth(env, alice)); - } - { - // featureDepositAuth is enabled. Env env(*this); env.fund(XRP(10000), alice); @@ -281,8 +266,6 @@ struct DepositAuth_test : public beast::unit_test::suite bool noRipplePrev, bool noRippleNext, bool withDepositAuth) { - assert(!withDepositAuth || features[featureDepositAuth]); - Env env(*this, features); env.fund(XRP(10000), gw1, alice, bob); @@ -305,8 +288,6 @@ struct DepositAuth_test : public beast::unit_test::suite bool noRipplePrev, bool noRippleNext, bool withDepositAuth) { - assert(!withDepositAuth || features[featureDepositAuth]); - Env env(*this, features); env.fund(XRP(10000), gw1, gw2, alice); @@ -333,30 +314,16 @@ struct DepositAuth_test : public beast::unit_test::suite auto const noRippleNext = i & 0x2; auto const withDepositAuth = i & 0x4; testIssuer( - testable_amendments() | featureDepositAuth, + testable_amendments(), noRipplePrev, noRippleNext, withDepositAuth); - if (!withDepositAuth) - testIssuer( - testable_amendments() - featureDepositAuth, - noRipplePrev, - noRippleNext, - withDepositAuth); - testNonIssuer( - testable_amendments() | featureDepositAuth, + testable_amendments(), noRipplePrev, noRippleNext, withDepositAuth); - - if (!withDepositAuth) - testNonIssuer( - testable_amendments() - featureDepositAuth, - noRipplePrev, - noRippleNext, - withDepositAuth); } } @@ -400,26 +367,6 @@ struct DepositPreauth_test : public beast::unit_test::suite Account const alice{"alice"}; Account const becky{"becky"}; { - // featureDepositPreauth is disabled. - Env env(*this, testable_amendments() - featureDepositPreauth); - env.fund(XRP(10000), alice, becky); - env.close(); - - // Should not be able to add a DepositPreauth to alice. - env(deposit::auth(alice, becky), ter(temDISABLED)); - env.close(); - env.require(owners(alice, 0)); - env.require(owners(becky, 0)); - - // Should not be able to remove a DepositPreauth from alice. - env(deposit::unauth(alice, becky), ter(temDISABLED)); - env.close(); - env.require(owners(alice, 0)); - env.require(owners(becky, 0)); - } - { - // featureDepositPreauth is enabled. The valid case is really - // simple: // o We should be able to add and remove an entry, and // o That entry should cost one reserve. // o The reserve should be returned when the entry is removed. @@ -602,8 +549,6 @@ struct DepositPreauth_test : public beast::unit_test::suite Account const gw{"gw"}; IOU const USD(gw["USD"]); - bool const supportsPreauth = {features[featureDepositPreauth]}; - { // The initial implementation of DepositAuth had a bug where an // account with the DepositAuth flag set could not make a payment @@ -632,15 +577,11 @@ struct DepositPreauth_test : public beast::unit_test::suite env(fset(becky, asfDepositAuth)); env.close(); - // becky pays herself again. Whether it succeeds depends on - // whether featureDepositPreauth is enabled. - TER const expect{ - supportsPreauth ? TER{tesSUCCESS} : TER{tecNO_PERMISSION}}; - + // becky pays herself again. env(pay(becky, becky, USD(10)), path(~USD), sendmax(XRP(10)), - ter(expect)); + ter(tesSUCCESS)); env.close(); { @@ -652,29 +593,17 @@ struct DepositPreauth_test : public beast::unit_test::suite bool const supportsCredentials = features[featureCredentials]; - TER const expectCredentials( - supportsCredentials ? TER(tesSUCCESS) : TER(temDISABLED)); - TER const expectPayment( - !supportsCredentials - ? TER(temDISABLED) - : (!supportsPreauth ? TER(tecNO_PERMISSION) - : TER(tesSUCCESS))); - TER const expectDP( - !supportsPreauth - ? TER(temDISABLED) - : (!supportsCredentials ? TER(temDISABLED) - : TER(tesSUCCESS))); + TER const expectTer( + !supportsCredentials ? TER(temDISABLED) : TER(tesSUCCESS)); env(deposit::authCredentials(becky, {{carol, credType}}), - ter(expectDP)); + ter(expectTer)); env.close(); // gw accept credentials - env(credentials::create(gw, carol, credType), - ter(expectCredentials)); + env(credentials::create(gw, carol, credType), ter(expectTer)); env.close(); - env(credentials::accept(gw, carol, credType), - ter(expectCredentials)); + env(credentials::accept(gw, carol, credType), ter(expectTer)); env.close(); auto jv = credentials::ledgerEntry(env, gw, carol, credType); @@ -685,115 +614,94 @@ struct DepositPreauth_test : public beast::unit_test::suite env(pay(gw, becky, USD(100)), credentials::ids({credIdx}), - ter(expectPayment)); + ter(expectTer)); env.close(); } - - { - using namespace std::chrono; - - if (!supportsPreauth) - { - auto const seq1 = env.seq(alice); - env(escrow::create(alice, becky, XRP(100)), - escrow::finish_time(env.now() + 1s)); - env.close(); - - // Failed as rule is disabled - env(escrow::finish(gw, alice, seq1), - fee(1500), - ter(tecNO_PERMISSION)); - env.close(); - } - } } - if (supportsPreauth) - { - // Make sure DepositPreauthorization works for payments. + // Make sure DepositPreauthorization works for payments. - Account const carol{"carol"}; + Account const carol{"carol"}; - Env env(*this, features); - env.fund(XRP(5000), alice, becky, carol, gw); - env.close(); + Env env(*this, features); + env.fund(XRP(5000), alice, becky, carol, gw); + env.close(); - env.trust(USD(1000), alice); - env.trust(USD(1000), becky); - env.trust(USD(1000), carol); - env.close(); + env.trust(USD(1000), alice); + env.trust(USD(1000), becky); + env.trust(USD(1000), carol); + env.close(); - env(pay(gw, alice, USD(1000))); - env.close(); + env(pay(gw, alice, USD(1000))); + env.close(); - // Make XRP and IOU payments from alice to becky. Should be fine. - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + // Make XRP and IOU payments from alice to becky. Should be fine. + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // becky decides to require authorization for deposits. - env(fset(becky, asfDepositAuth)); - env.close(); + // becky decides to require authorization for deposits. + env(fset(becky, asfDepositAuth)); + env.close(); - // alice can no longer pay becky. - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // alice can no longer pay becky. + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky preauthorizes carol for deposit, which doesn't provide - // authorization for alice. - env(deposit::auth(becky, carol)); - env.close(); + // becky preauthorizes carol for deposit, which doesn't provide + // authorization for alice. + env(deposit::auth(becky, carol)); + env.close(); - // alice still can't pay becky. - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // alice still can't pay becky. + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky preauthorizes alice for deposit. - env(deposit::auth(becky, alice)); - env.close(); + // becky preauthorizes alice for deposit. + env(deposit::auth(becky, alice)); + env.close(); - // alice can now pay becky. - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + // alice can now pay becky. + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // alice decides to require authorization for deposits. - env(fset(alice, asfDepositAuth)); - env.close(); + // alice decides to require authorization for deposits. + env(fset(alice, asfDepositAuth)); + env.close(); - // Even though alice is authorized to pay becky, becky is not - // authorized to pay alice. - env(pay(becky, alice, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(becky, alice, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // Even though alice is authorized to pay becky, becky is not + // authorized to pay alice. + env(pay(becky, alice, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(becky, alice, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky unauthorizes carol. Should have no impact on alice. - env(deposit::unauth(becky, carol)); - env.close(); + // becky unauthorizes carol. Should have no impact on alice. + env(deposit::unauth(becky, carol)); + env.close(); - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // becky unauthorizes alice. alice now can't pay becky. - env(deposit::unauth(becky, alice)); - env.close(); + // becky unauthorizes alice. alice now can't pay becky. + env(deposit::unauth(becky, alice)); + env.close(); - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky decides to remove authorization for deposits. Now - // alice can pay becky again. - env(fclear(becky, asfDepositAuth)); - env.close(); + // becky decides to remove authorization for deposits. Now + // alice can pay becky again. + env(fclear(becky, asfDepositAuth)); + env.close(); - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); - } + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); } void @@ -1545,8 +1453,6 @@ struct DepositPreauth_test : public beast::unit_test::suite testEnable(); testInvalid(); auto const supported{jtx::testable_amendments()}; - testPayment(supported - featureDepositPreauth - featureCredentials); - testPayment(supported - featureDepositPreauth); testPayment(supported - featureCredentials); testPayment(supported); testCredentialsPayment(); diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 22e81ccca4..e3b2340022 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -252,16 +252,6 @@ struct Escrow_test : public beast::unit_test::suite using namespace jtx; using namespace std::chrono; - { - // Respect the "asfDisallowXRP" account flag: - Env env(*this, features - featureDepositAuth); - - env.fund(XRP(5000), "bob", "george"); - env(fset("george", asfDisallowXRP)); - env(escrow::create("bob", "george", XRP(10)), - escrow::finish_time(env.now() + 1s), - ter(tecNO_TARGET)); - } { // Ignore the "asfDisallowXRP" account flag, which we should // have been doing before. diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 1778e60b37..da8b964c94 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -361,11 +362,8 @@ protected: loan->at(sfPreviousPaymentDate) == previousPaymentDate); env.test.BEAST_EXPECT( loan->at(sfPaymentRemaining) == paymentRemaining); - if (paymentRemaining == 0) - env.test.BEAST_EXPECT(!loan->at(~sfNextPaymentDueDate)); - else - env.test.BEAST_EXPECT( - loan->at(sfNextPaymentDueDate) == nextPaymentDate); + env.test.BEAST_EXPECT( + loan->at(sfNextPaymentDueDate) == nextPaymentDate); env.test.BEAST_EXPECT(loan->at(sfLoanScale) == loanScale); env.test.BEAST_EXPECT( loan->at(sfTotalValueOutstanding) == totalValue); @@ -498,7 +496,7 @@ protected: return LoanState{ .previousPaymentDate = loan->at(sfPreviousPaymentDate), .startDate = tp{d{loan->at(sfStartDate)}}, - .nextPaymentDate = loan->at(~sfNextPaymentDueDate).value_or(0), + .nextPaymentDate = loan->at(sfNextPaymentDueDate), .paymentRemaining = loan->at(sfPaymentRemaining), .loanScale = loan->at(sfLoanScale), .totalValue = loan->at(sfTotalValueOutstanding), @@ -1059,8 +1057,7 @@ protected: // Make the payment env(pay(borrower, loanKeylet.key, transactionAmount)); - env.close( - d{(state.previousPaymentDate + state.nextPaymentDate) / 2}); + env.close(d{state.paymentInterval / 2}); // Need to account for fees if the loan is in XRP PrettyAmount adjustment = broker.asset(0); @@ -1111,6 +1108,7 @@ protected: detail::PaymentSpecialCase::final) { state.paymentRemaining = 0; + state.nextPaymentDate = 0; } else { @@ -2170,6 +2168,7 @@ protected: state.totalValue = 0; state.principalOutstanding = 0; state.managementFeeOutstanding = 0; + state.nextPaymentDate = 0; verifyLoanStatus(state); // Once a loan is defaulted, it can't be managed @@ -2227,14 +2226,29 @@ protected: (Number{15, -1} / loanPaymentsPerFeeIncrement + 1)}), ter(temINVALID_FLAG)); } - // Try to send a payment marked as both full payment and - // overpayment. Do not include `txFlags`, so we don't duplicate the - // prior test transaction. + // Try to send a payment marked as multiple mutually exclusive + // payment types. Do not include `txFlags`, so we don't duplicate + // the prior test transaction. + env(pay(borrower, + loanKeylet.key, + broker.asset(state.periodicPayment * 2), + tfLoanLatePayment | tfLoanFullPayment), + ter(temINVALID_FLAG)); + env(pay(borrower, + loanKeylet.key, + broker.asset(state.periodicPayment * 2), + tfLoanLatePayment | tfLoanOverpayment), + ter(temINVALID_FLAG)); env(pay(borrower, loanKeylet.key, broker.asset(state.periodicPayment * 2), tfLoanOverpayment | tfLoanFullPayment), ter(temINVALID_FLAG)); + env(pay(borrower, + loanKeylet.key, + broker.asset(state.periodicPayment * 2), + tfLoanLatePayment | tfLoanOverpayment | tfLoanFullPayment), + ter(temINVALID_FLAG)); { auto const otherAsset = broker.asset.raw() == assets[0].raw() @@ -2299,6 +2313,7 @@ protected: state.managementFeeOutstanding = 0; state.previousPaymentDate = state.nextPaymentDate + state.paymentInterval * (numPayments - 1); + state.nextPaymentDate = 0; verifyLoanStatus(state); verifyLoanStatus.checkPayment( @@ -2801,6 +2816,7 @@ protected: detail::PaymentSpecialCase::final) { state.paymentRemaining = 0; + state.nextPaymentDate = 0; } else { @@ -3786,6 +3802,11 @@ protected: // From FIND-001 testcase << "Batch Bypass Counterparty"; + bool const lendingBatchEnabled = !std::any_of( + Batch::disabledTxTypes.begin(), + Batch::disabledTxTypes.end(), + [](auto const& disabled) { return disabled == ttLOAN_BROKER_SET; }); + using namespace jtx; using namespace std::chrono_literals; Env env(*this, all); @@ -3831,7 +3852,8 @@ protected: env(batch::outer(borrower, seq, batchFee, tfAllOrNothing), batch::inner(forgedLoanSet, seq + 1), batch::inner(pay(borrower, lender, XRP(1)), seq + 2), - ter(temBAD_SIGNATURE)); + ter(lendingBatchEnabled ? temBAD_SIGNATURE + : temINVALID_INNER_BATCH)); env.close(); // ? Check that the loan was NOT created @@ -4562,7 +4584,11 @@ protected: issuer, lender["IOU"](1'000), tfClearFreeze | tfClearDeepFreeze)); env.close(); - env(pay(borrower, loanKeylet.key, debtMaximumRequest)); + // The payment is late by this point + env(pay(borrower, loanKeylet.key, debtMaximumRequest), ter(tecEXPIRED)); + env.close(); + env(pay( + borrower, loanKeylet.key, debtMaximumRequest, tfLoanLatePayment)); env.close(); // preclaim: tecKILLED @@ -6523,8 +6549,6 @@ protected: Account const lender{"lender"}; Account const borrower{"borrower"}; - auto const asset = xrpIssue(); - env.fund(XRP(200'000), lender, borrower); env.close(); @@ -6644,6 +6668,131 @@ protected: env, broker, loanParams, loanKeylet, verifyLoanStatus, true); } + void + testBorrowerIsBroker() + { + testcase("Test Borrower is Broker"); + using namespace jtx; + using namespace loan; + Account const broker{"broker"}; + Account const issuer{"issuer"}; + Account const borrower_{"borrower"}; + Account const depositor{"depositor"}; + + auto testLoanAsset = [&](auto&& getMaxDebt, auto const& borrower) { + Env env(*this); + Vault vault(env); + + if (borrower == broker) + env.fund(XRP(10'000), broker, issuer, depositor); + else + env.fund(XRP(10'000), broker, borrower, issuer, depositor); + env.close(); + + auto const xrpFee = XRP(100); + auto const txFee = fee(xrpFee); + + STAmount const debtMaximumRequest = getMaxDebt(env); + + auto const& asset = debtMaximumRequest.asset(); + auto const initialVault = asset(debtMaximumRequest * 100); + + auto [tx, vaultKeylet] = + vault.create({.owner = broker, .asset = asset}); + env(tx, txFee); + env.close(); + + env(vault.deposit( + {.depositor = depositor, + .id = vaultKeylet.key, + .amount = initialVault}), + txFee); + env.close(); + + auto const brokerKeylet = + keylet::loanbroker(broker.id(), env.seq(broker)); + + env(loanBroker::set(broker, vaultKeylet.key), txFee); + env.close(); + + auto const serviceFee = 101; + + env(set(broker, brokerKeylet.key, debtMaximumRequest), + counterparty(borrower), + sig(sfCounterpartySignature, borrower), + loanServiceFee(serviceFee), + paymentTotal(10), + txFee); + env.close(); + + std::uint32_t const loanSequence = 1; + auto const loanKeylet = + keylet::loan(brokerKeylet.key, loanSequence); + + auto const brokerBalanceBefore = env.balance(broker, asset); + + if (auto const loanSle = env.le(loanKeylet); + env.test.BEAST_EXPECT(loanSle)) + { + auto const payment = loanSle->at(sfPeriodicPayment); + auto const totalPayment = payment + serviceFee; + env(loan::pay(borrower, loanKeylet.key, asset(totalPayment)), + txFee); + env.close(); + if (auto const vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle)) + { + auto const expected = [&]() { + // The service fee is transferred to the broker if + // a borrower is not the broker + if (borrower != broker) + return brokerBalanceBefore.number() + serviceFee; + // Since a borrower is the broker, the payment is + // transferred to the Vault from the broker but not + // the service fee. + // If the asset is XRP then the broker pays the txfee. + if (asset.native()) + return brokerBalanceBefore.number() - payment - + xrpFee.number(); + return brokerBalanceBefore.number() - payment; + }(); + BEAST_EXPECT( + env.balance(broker, asset).value() == + asset(expected).value()); + } + } + }; + // Test when a borrower is the broker and is not to verify correct + // service fee transfer in both cases. + for (auto const& borrowerAcct : {broker, borrower_}) + { + testLoanAsset( + [&](Env&) -> STAmount { return STAmount{XRPAmount{200'000}}; }, + borrowerAcct); + testLoanAsset( + [&](Env& env) -> STAmount { + auto const IOU = issuer["USD"]; + env(trust(broker, IOU(1'000'000'000))); + env(trust(depositor, IOU(1'000'000'000))); + env(pay(issuer, broker, IOU(100'000'000))); + env(pay(issuer, depositor, IOU(100'000'000))); + env.close(); + return IOU(200'000); + }, + borrowerAcct); + testLoanAsset( + [&](Env& env) -> STAmount { + MPTTester mpt( + {.env = env, + .issuer = issuer, + .holders = {broker, depositor}, + .pay = 100'000'000}); + return mpt(200'000); + }, + borrowerAcct); + } + } + public: void run() override @@ -6654,11 +6803,10 @@ public: testPoC_UnsignedUnderflowOnFullPayAfterEarlyPeriodic(); testLoanCoverMinimumRoundingExploit(); #endif - testDustManipulation(); - testIssuerLoan(); testDisabled(); testSelfLoan(); + testIssuerLoan(); testLoanSet(); testLifecycle(); testServiceFeeOnBrokerDeepFreeze(); @@ -6683,12 +6831,14 @@ public: testLoanNextPaymentDueDateOverflow(); testRequireAuth(); + testDustManipulation(); testRIPD3831(); testRIPD3459(); testRIPD3901(); testRIPD3902(); testRoundingAllowsUndercoverage(); + testBorrowerIsBroker(); } }; diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 238042d388..a11446df71 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6808,7 +6808,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite mintAndCreateSellOffer(env, alice, XRP(0)); // Bob can accept the offer because the new NFT is stored in - // an existing NFTokenPage so no new reserve is requried + // an existing NFTokenPage so no new reserve is required env(token::acceptSellOffer(bob, sellOfferIndex)); env.close(); } diff --git a/src/test/app/NetworkOPs_test.cpp b/src/test/app/NetworkOPs_test.cpp index 166c3140d4..d9afe6d628 100644 --- a/src/test/app/NetworkOPs_test.cpp +++ b/src/test/app/NetworkOPs_test.cpp @@ -21,7 +21,7 @@ public: void testAllBadHeldTransactions() { - // All trasactions are already marked as SF_BAD, and we should be able + // All transactions are already marked as SF_BAD, and we should be able // to handle the case properly without an assertion failure testcase("No valid transactions in batch"); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index aff617fdba..2cbf2598e1 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -180,13 +180,10 @@ public: // an offer. Show that the attempt to remove the offer fails. env.require(offers(alice, 2)); - // featureDepositPreauths changes the return code on an expired Offer. - // Adapt to that. - bool const featPreauth{features[featureDepositPreauth]}; env(offer(alice, XRP(5), USD(2)), json(sfExpiration.fieldName, lastClose(env)), json(jss::OfferSequence, offer2Seq), - ter(featPreauth ? TER{tecEXPIRED} : TER{tesSUCCESS})); + ter(tecEXPIRED)); env.close(); env.require(offers(alice, 2)); @@ -1082,13 +1079,9 @@ public: offers(alice, 0), owners(alice, 1)); - // Place an offer that should have already expired. - // The DepositPreauth amendment changes the return code; adapt to that. - bool const featPreauth{features[featureDepositPreauth]}; - env(offer(alice, xrpOffer, usdOffer), json(sfExpiration.fieldName, lastClose(env)), - ter(featPreauth ? TER{tecEXPIRED} : TER{tesSUCCESS})); + ter(tecEXPIRED)); env.require( balance(alice, startBalance - f - f), @@ -4499,21 +4492,13 @@ public: env(fset(gw, asfRequireAuth)); env.close(); - // The test behaves differently with or without DepositPreauth. - bool const preauth = features[featureDepositPreauth]; - // Before DepositPreauth an account with lsfRequireAuth set could not // create an offer to buy their own currency. After DepositPreauth // they can. - env(offer(gw, gwUSD(40), XRP(4000)), - ter(preauth ? TER{tesSUCCESS} : TER{tecNO_LINE})); + env(offer(gw, gwUSD(40), XRP(4000)), ter(tesSUCCESS)); env.close(); - env.require(offers(gw, preauth ? 1 : 0)); - - if (!preauth) - // The rest of the test verifies DepositPreauth behavior. - return; + env.require(offers(gw, 1)); // Set up an authorized trust line and pay alice gwUSD 50. env(trust(gw, aliceUSD(100)), txflags(tfSetfAuth)); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 650542f827..4a1f1c14eb 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -656,16 +656,6 @@ struct PayChan_test : public beast::unit_test::suite auto const alice = Account("alice"); auto const bob = Account("bob"); - { - // Create a channel where dst disallows XRP - Env env(*this, features - featureDepositAuth); - env.fund(XRP(10000), alice, bob); - env(fset(bob, asfDisallowXRP)); - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), 3600s, alice.pk()), - ter(tecNO_TARGET)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - } { // Create a channel where dst disallows XRP. Ignore that flag, // since it's just advisory. @@ -677,19 +667,6 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(channelExists(*env.current(), chan)); } - { - // Claim to a channel where dst disallows XRP - // (channel is created before disallow xrp is set) - Env env(*this, features - featureDepositAuth); - env.fund(XRP(10000), alice, bob); - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), 3600s, alice.pk())); - BEAST_EXPECT(channelExists(*env.current(), chan)); - - env(fset(bob, asfDisallowXRP)); - auto const reqBal = XRP(500).value(); - env(claim(alice, chan, reqBal, reqBal), ter(tecNO_TARGET)); - } { // Claim to a channel where dst disallows XRP (channel is // created before disallow xrp is set). Ignore that flag @@ -1820,36 +1797,6 @@ struct PayChan_test : public beast::unit_test::suite return std::distance(ownerDir.begin(), ownerDir.end()); }; - { - // Test without adding the paychan to the recipient's owner - // directory - Env env(*this, features - fixPayChanRecipientOwnerDir); - env.fund(XRP(10000), alice, bob); - env(create(alice, bob, XRP(1000), settleDelay, pk)); - env.close(); - auto const [chan, chanSle] = - channelKeyAndSle(*env.current(), alice, bob); - BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); - if (features[fixIncludeKeyletFields]) - { - BEAST_EXPECT((*chanSle)[sfSequence] == env.seq(alice) - 1); - } - else - { - BEAST_EXPECT(!chanSle->isFieldPresent(sfSequence)); - } - // close the channel - env(claim(bob, chan), txflags(tfClose)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 0); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); - } - { // Test with adding the paychan to the recipient's owner directory Env env{*this, features}; @@ -1874,7 +1821,7 @@ struct PayChan_test : public beast::unit_test::suite { // Test removing paychans created before adding to the recipient's // owner directory - Env env(*this, features - fixPayChanRecipientOwnerDir); + Env env(*this, features); env.fund(XRP(10000), alice, bob); // create the channel before the amendment activates env(create(alice, bob, XRP(1000), settleDelay, pk)); @@ -1883,21 +1830,9 @@ struct PayChan_test : public beast::unit_test::suite channelKeyAndSle(*env.current(), alice, bob); BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); - env.enableFeature(fixPayChanRecipientOwnerDir); - env.close(); - BEAST_EXPECT( - env.current()->rules().enabled(fixPayChanRecipientOwnerDir)); - // These checks look redundant, but if you don't `close` after the - // `create` these checks will fail. I believe this is due to the - // create running with one set of amendments initially, then with a - // different set with the ledger closes (tho I haven't dug into it) - BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + BEAST_EXPECT(inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 1); - // close the channel after the amendment activates env(claim(bob, chan), txflags(tfClose)); BEAST_EXPECT(!channelExists(*env.current(), chan)); BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); @@ -1939,12 +1874,8 @@ struct PayChan_test : public beast::unit_test::suite auto const bob = Account("bob"); auto const carol = Account("carol"); - for (bool const withOwnerDirFix : {false, true}) { - auto const amd = withOwnerDirFix - ? features - : features - fixPayChanRecipientOwnerDir; - Env env{*this, amd}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -1958,12 +1889,8 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(channelAmount(*env.current(), chan) == XRP(1000)); rmAccount(env, alice, carol, tecHAS_OBLIGATIONS); - // can only remove bob if the channel isn't in their owner direcotry - rmAccount( - env, - bob, - carol, - withOwnerDirFix ? TER(tecHAS_OBLIGATIONS) : TER(tesSUCCESS)); + // can only remove bob if the channel isn't in their owner directory + rmAccount(env, bob, carol, TER(tecHAS_OBLIGATIONS)); auto const feeDrops = env.current()->fees().base; auto chanBal = channelBalance(*env.current(), chan); @@ -1978,158 +1905,27 @@ struct PayChan_test : public beast::unit_test::suite assert(reqBal <= chanAmt); // claim should fail if the dst was removed - if (withOwnerDirFix) - { - env(claim(alice, chan, reqBal, authAmt)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob + delta); - chanBal = reqBal; - } - else - { - auto const preAlice = env.balance(alice); - env(claim(alice, chan, reqBal, authAmt), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == chanBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - } + env(claim(alice, chan, reqBal, authAmt)); + env.close(); + BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob + delta); + chanBal = reqBal; // fund should fail if the dst was removed - if (withOwnerDirFix) - { - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000))); - env.close(); - BEAST_EXPECT( - env.balance(alice) == preAlice - XRP(1000) - feeDrops); - BEAST_EXPECT( - channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); - chanAmt = chanAmt + XRP(1000); - } - else - { - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000)), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - } + auto const preAlice = env.balance(alice); + env(fund(alice, chan, XRP(1000))); + env.close(); + BEAST_EXPECT(env.balance(alice) == preAlice - XRP(1000) - feeDrops); + BEAST_EXPECT( + channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); + chanAmt = chanAmt + XRP(1000); { // Owner closes, will close after settleDelay env(claim(alice, chan), txflags(tfClose)); env.close(); - // settle delay hasn't ellapsed. Channels should exist. - BEAST_EXPECT(channelExists(*env.current(), chan)); - auto const closeTime = env.current()->info().parentCloseTime; - auto const minExpiration = closeTime + settleDelay; - env.close(minExpiration); - env(claim(alice, chan), txflags(tfClose)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - } - } - - { - // test resurrected account - Env env{*this, features - fixPayChanRecipientOwnerDir}; - env.fund(XRP(10000), alice, bob, carol); - env.close(); - - // Create a channel from alice to bob - auto const pk = alice.pk(); - auto const settleDelay = 100s; - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), settleDelay, pk)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == XRP(0)); - BEAST_EXPECT(channelAmount(*env.current(), chan) == XRP(1000)); - - // Since `fixPayChanRecipientOwnerDir` is not active, can remove bob - rmAccount(env, bob, carol); - BEAST_EXPECT(!env.closed()->exists(keylet::account(bob.id()))); - - auto const feeDrops = env.current()->fees().base; - auto chanBal = channelBalance(*env.current(), chan); - auto chanAmt = channelAmount(*env.current(), chan); - BEAST_EXPECT(chanBal == XRP(0)); - BEAST_EXPECT(chanAmt == XRP(1000)); - auto preBob = env.balance(bob); - auto const delta = XRP(50); - auto reqBal = chanBal + delta; - auto authAmt = reqBal + XRP(100); - assert(reqBal <= chanAmt); - - { - // claim should fail, since bob doesn't exist - auto const preAlice = env.balance(alice); - env(claim(alice, chan, reqBal, authAmt), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == chanBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - } - - { - // fund should fail, sincebob doesn't exist - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000)), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - } - - // resurrect bob - env(pay(alice, bob, XRP(20))); - env.close(); - BEAST_EXPECT(env.closed()->exists(keylet::account(bob.id()))); - - { - // alice should be able to claim - preBob = env.balance(bob); - reqBal = chanBal + delta; - authAmt = reqBal + XRP(100); - env(claim(alice, chan, reqBal, authAmt)); - BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob + delta); - chanBal = reqBal; - } - - { - // bob should be able to claim - preBob = env.balance(bob); - reqBal = chanBal + delta; - authAmt = reqBal + XRP(100); - auto const sig = - signClaimAuth(alice.pk(), alice.sk(), chan, authAmt); - env(claim(bob, chan, reqBal, authAmt, Slice(sig), alice.pk())); - BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob + delta - feeDrops); - chanBal = reqBal; - } - - { - // alice should be able to fund - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000))); - BEAST_EXPECT( - env.balance(alice) == preAlice - XRP(1000) - feeDrops); - BEAST_EXPECT( - channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); - chanAmt = chanAmt + XRP(1000); - } - - { - // Owner closes, will close after settleDelay - env(claim(alice, chan), txflags(tfClose)); - env.close(); - // settle delay hasn't ellapsed. Channels should exist. + // settle delay hasn't elapsed. Channels should exist. BEAST_EXPECT(channelExists(*env.current(), chan)); auto const closeTime = env.current()->info().parentCloseTime; auto const minExpiration = closeTime + settleDelay; diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index d176744c5c..4b7f156738 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -4330,9 +4330,10 @@ public: Account const ellie("ellie"); Account const fiona("fiona"); + constexpr int ledgersInQueue = 10; auto cfg = makeConfig( {{"minimum_txn_in_ledger_standalone", "1"}, - {"ledgers_in_queue", "5"}, + {"ledgers_in_queue", std::to_string(ledgersInQueue)}, {"maximum_txn_per_account", "10"}}, {{"account_reserve", "1000"}, {"owner_reserve", "50"}}); @@ -4358,7 +4359,9 @@ public: env.close(); env.fund(XRP(10000), fiona); env.close(); - checkMetrics(*this, env, 0, 10, 0, 2); + + auto const metrics = env.app().getTxQ().getMetrics(*env.current()); + checkMetrics(*this, env, 0, ledgersInQueue * metrics.txPerLedger, 0, 2); // Close ledgers until the amendments show up. int i = 0; @@ -4370,7 +4373,12 @@ public: } auto expectedPerLedger = ripple::detail::numUpVotedAmendments() + 1; checkMetrics( - *this, env, 0, 5 * expectedPerLedger, 0, expectedPerLedger); + *this, + env, + 0, + ledgersInQueue * expectedPerLedger, + 0, + expectedPerLedger); // Now wait 2 weeks modulo 256 ledgers for the amendments to be // enabled. Speed the process by closing ledgers every 80 minutes, @@ -4389,7 +4397,7 @@ public: *this, env, 0, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedPerLedger + 1, expectedPerLedger); @@ -4435,12 +4443,12 @@ public: prepareFee(++multiplier), ter(terQUEUED)); } - std::size_t expectedInQueue = 60; + std::size_t expectedInQueue = multiplier; checkMetrics( *this, env, expectedInQueue, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedPerLedger + 1, expectedPerLedger); @@ -4467,7 +4475,7 @@ public: *this, env, expectedInQueue, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedInLedger, expectedPerLedger); { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index cf6a6584cb..f823c56a72 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -2489,6 +2489,7 @@ class Vault_test : public beast::unit_test::suite struct CaseArgs { int initialXRP = 1000; + Number initialIOU = 200; double transferRate = 1.0; }; @@ -2516,7 +2517,7 @@ class Vault_test : public beast::unit_test::suite PrettyAsset const asset = issuer["IOU"]; env.trust(asset(1000), owner); env.trust(asset(1000), charlie); - env(pay(issuer, owner, asset(200))); + env(pay(issuer, owner, asset(args.initialIOU))); env(rate(issuer, args.transferRate)); env.close(); @@ -2894,6 +2895,86 @@ class Vault_test : public beast::unit_test::suite env(tx1); }); + testCase( + [&, this]( + Env& env, + Account const& owner, + Account const& issuer, + Account const& charlie, + auto const& vaultAccount, + Vault& vault, + PrettyAsset const& asset, + auto&&...) { + testcase("IOU calculation rounding"); + + auto [tx, keylet] = + vault.create({.owner = owner, .asset = asset}); + tx[sfScale] = 1; + env(tx); + env.close(); + + auto const startingOwnerBalance = env.balance(owner, asset); + BEAST_EXPECT( + (startingOwnerBalance.value() == + STAmount{asset, 11875, -2})); + + // This operation (first deposit 100, then 3.75 x 5) is known to + // have triggered calculation rounding errors in Number + // (addition and division), causing the last deposit to be + // blocked by Vault invariants. + env(vault.deposit( + {.depositor = owner, + .id = keylet.key, + .amount = asset(100)})); + + auto const tx1 = vault.deposit( + {.depositor = owner, + .id = keylet.key, + .amount = asset(Number(375, -2))}); + for (auto i = 0; i < 5; ++i) + { + env(tx1); + } + env.close(); + + { + STAmount const xfer{asset, 1185, -1}; + BEAST_EXPECT( + env.balance(owner, asset) == + startingOwnerBalance.value() - xfer); + BEAST_EXPECT( + env.balance(vaultAccount(keylet), asset) == xfer); + + auto const vault = env.le(keylet); + BEAST_EXPECT(vault->at(sfAssetsAvailable) == xfer); + BEAST_EXPECT(vault->at(sfAssetsTotal) == xfer); + } + + // Total vault balance should be 118.5 IOU. Withdraw and delete + // the vault to verify this exact amount was deposited and the + // owner has matching shares + env(vault.withdraw( + {.depositor = owner, + .id = keylet.key, + .amount = asset(Number(1000 + 37 * 5, -1))})); + + { + BEAST_EXPECT( + env.balance(owner, asset) == + startingOwnerBalance.value()); + BEAST_EXPECT( + env.balance(vaultAccount(keylet), asset) == + beast::zero); + auto const vault = env.le(keylet); + BEAST_EXPECT(vault->at(sfAssetsAvailable) == beast::zero); + BEAST_EXPECT(vault->at(sfAssetsTotal) == beast::zero); + } + + env(vault.del({.owner = owner, .id = keylet.key})); + env.close(); + }, + {.initialIOU = Number(11875, -2)}); + auto const [acctReserve, incReserve] = [this]() -> std::pair { Env env{*this, testable_amendments()}; return { diff --git a/src/test/csf/Peer.h b/src/test/csf/Peer.h index 3a76e7b841..5f6f006b0f 100644 --- a/src/test/csf/Peer.h +++ b/src/test/csf/Peer.h @@ -404,7 +404,7 @@ struct Peer { minDuration = std::min(minDuration, link.data.delay); - // Send a messsage to neighbors to find the ledger + // Send a message to neighbors to find the ledger net.send( this, link.target, [to = link.target, from = this, ledgerID]() { if (auto it = to->ledgers.find(ledgerID); diff --git a/src/test/csf/collectors.h b/src/test/csf/collectors.h index c361701b4e..53494dad44 100644 --- a/src/test/csf/collectors.h +++ b/src/test/csf/collectors.h @@ -28,7 +28,7 @@ namespace csf { /** Group of collectors. Presents a group of collectors as a single collector which process an event - by calling each collector sequentially. This is analagous to CollectorRefs + by calling each collector sequentially. This is analogous to CollectorRefs in CollectorRef.h, but does *not* erase the type information of the combined collectors. */ diff --git a/src/test/csf/events.h b/src/test/csf/events.h index bf01de4eb2..b25dc4b339 100644 --- a/src/test/csf/events.h +++ b/src/test/csf/events.h @@ -13,7 +13,7 @@ namespace test { namespace csf { // Events are emitted by peers at a variety of points during the simulation. -// Each event is emitted by a particlar peer at a particular time. Collectors +// Each event is emitted by a particular peer at a particular time. Collectors // process these events, perhaps calculating statistics or storing events to // a log for post-processing. // diff --git a/src/test/csf/ledgers.h b/src/test/csf/ledgers.h index 26b416a2fb..80e431c1e2 100644 --- a/src/test/csf/ledgers.h +++ b/src/test/csf/ledgers.h @@ -35,7 +35,7 @@ namespace csf { Ledgers are immutable value types. All ledgers with the same sequence number, transactions, close time, etc. will have the same ledger ID. The - LedgerOracle class below manges ID assignments for a simulation and is the + LedgerOracle class below manages ID assignments for a simulation and is the only way to close and create a new ledger. Since the parent ledger ID is part of type, this also means ledgers with distinct histories will have distinct ids, even if they have the same set of transactions, sequence diff --git a/src/test/nodestore/TestBase.h b/src/test/nodestore/TestBase.h index c1401abc33..0675ad85d4 100644 --- a/src/test/nodestore/TestBase.h +++ b/src/test/nodestore/TestBase.h @@ -81,7 +81,7 @@ public: case 3: return hotUNKNOWN; } - // will never happen, but make static analysys tool happy. + // will never happen, but make static analysis tool happy. return hotUNKNOWN; }(); diff --git a/src/test/nodestore/import_test.cpp b/src/test/nodestore/import_test.cpp index 69ef50d453..30aaaa2ea9 100644 --- a/src/test/nodestore/import_test.cpp +++ b/src/test/nodestore/import_test.cpp @@ -235,7 +235,7 @@ parse_args(std::string const& s) // '=' static boost::regex const re1( "^" // start of line - "(?:\\s*)" // whitespace (optonal) + "(?:\\s*)" // whitespace (optional) "([a-zA-Z][_a-zA-Z0-9]*)" // "(?:\\s*)" // whitespace (optional) "(?:=)" // '=' diff --git a/src/test/overlay/tx_reduce_relay_test.cpp b/src/test/overlay/tx_reduce_relay_test.cpp index 3eafa08713..1ca9e7aac6 100644 --- a/src/test/overlay/tx_reduce_relay_test.cpp +++ b/src/test/overlay/tx_reduce_relay_test.cpp @@ -245,7 +245,7 @@ private: // (20+0.25*(60-20)-5=25), queue the rest, skip counts towards relayed // (60-25-5=30) testRelay("skip", true, 60, 0, 20, 25, 25, 30, skip); - // relay to minPeers + disabled + 25% of (nPeers - minPeers - disalbed) + // relay to minPeers + disabled + 25% of (nPeers - minPeers - disabled) // (20+10+0.25*(70-20-10)=40), queue the rest (30) testRelay("disabled", true, 70, 10, 20, 25, 40, 30); // relay to minPeers + disabled-not-in-skip + 25% of (nPeers - minPeers diff --git a/src/test/rpc/AccountSet_test.cpp b/src/test/rpc/AccountSet_test.cpp index 31fc345862..3df3606a03 100644 --- a/src/test/rpc/AccountSet_test.cpp +++ b/src/test/rpc/AccountSet_test.cpp @@ -35,8 +35,7 @@ public: using namespace test::jtx; Account const alice("alice"); - // Test without DepositAuth enabled initially. - Env env(*this, testable_amendments() - featureDepositAuth); + Env env(*this, testable_amendments()); env.fund(XRP(10000), noripple(alice)); // Give alice a regular key so she can legally set and clear @@ -116,19 +115,6 @@ public: } } }; - - // Test with featureDepositAuth disabled. - testFlags( - {asfRequireDest, - asfRequireAuth, - asfDisallowXRP, - asfGlobalFreeze, - asfDisableMaster, - asfDefaultRipple}); - - // Enable featureDepositAuth and retest. - env.enableFeature(featureDepositAuth); - env.close(); testFlags( {asfRequireDest, asfRequireAuth, diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 1a53f39f60..13d5fd3969 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -322,8 +322,7 @@ class Feature_test : public beast::unit_test::suite testcase("No Params, Some Enabled"); using namespace test::jtx; - Env env{ - *this, FeatureBitset(featureDepositAuth, featureDepositPreauth)}; + Env env{*this, FeatureBitset{}}; std::map const& votes = ripple::detail::supportedAmendments(); diff --git a/src/test/rpc/NoRippleCheck_test.cpp b/src/test/rpc/NoRippleCheck_test.cpp index 94c85a59ef..81ec097181 100644 --- a/src/test/rpc/NoRippleCheck_test.cpp +++ b/src/test/rpc/NoRippleCheck_test.cpp @@ -252,7 +252,7 @@ class NoRippleCheckLimits_test : public beast::unit_test::suite auto checkBalance = [&env]() { // this is endpoint drop prevention. Non admin ports will drop // requests if they are coming too fast, so we manipulate the - // resource manager here to reset the enpoint balance (for + // resource manager here to reset the endpoint balance (for // localhost) if we get too close to the drop limit. It would // be better if we could add this functionality to Env somehow // or otherwise disable endpoint charging for certain test diff --git a/src/tests/libxrpl/CMakeLists.txt b/src/tests/libxrpl/CMakeLists.txt index 880fdb2948..a2374698d9 100644 --- a/src/tests/libxrpl/CMakeLists.txt +++ b/src/tests/libxrpl/CMakeLists.txt @@ -3,6 +3,9 @@ include(XrplAddTest) # Test requirements. find_package(doctest REQUIRED) +# Custom target for all tests defined in this file +add_custom_target(xrpl.tests) + # Common library dependencies for the rest of the tests. add_library(xrpl.imports.test INTERFACE) target_link_libraries(xrpl.imports.test INTERFACE doctest::doctest xrpl.libxrpl) @@ -10,13 +13,19 @@ target_link_libraries(xrpl.imports.test INTERFACE doctest::doctest xrpl.libxrpl) # One test for each module. xrpl_add_test(basics) target_link_libraries(xrpl.test.basics PRIVATE xrpl.imports.test) +add_dependencies(xrpl.tests xrpl.test.basics) + xrpl_add_test(crypto) target_link_libraries(xrpl.test.crypto PRIVATE xrpl.imports.test) +add_dependencies(xrpl.tests xrpl.test.crypto) + xrpl_add_test(json) target_link_libraries(xrpl.test.json PRIVATE xrpl.imports.test) +add_dependencies(xrpl.tests xrpl.test.json) # Network unit tests are currently not supported on Windows if(NOT WIN32) xrpl_add_test(net) target_link_libraries(xrpl.test.net PRIVATE xrpl.imports.test) + add_dependencies(xrpl.tests xrpl.test.net) endif() diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index d9e27414fa..c32b578d0b 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -30,21 +30,24 @@ roundPeriodicPayment( struct LoanPaymentParts { /// principal_paid is the amount of principal that the payment covered. - Number principalPaid; + Number principalPaid = numZero; /// interest_paid is the amount of interest that the payment covered. - Number interestPaid; + Number interestPaid = numZero; /** * value_change is the amount by which the total value of the Loan changed. * If value_change < 0, Loan value decreased. * If value_change > 0, Loan value increased. * This is 0 for regular payments. */ - Number valueChange; + Number valueChange = numZero; /// feePaid is amount of fee that is paid to the broker - Number feePaid; + Number feePaid = numZero; LoanPaymentParts& operator+=(LoanPaymentParts const& other); + + bool + operator==(LoanPaymentParts const& other) const; }; /** This structure describes the initial "computed" properties of a loan. @@ -240,15 +243,11 @@ computeLoanProperties( bool isRounded(Asset const& asset, Number const& value, std::int32_t scale); -Expected -loanMakeFullPayment( - Asset const& asset, - ApplyView& view, - SLE::ref loan, - SLE::const_ref brokerSle, - STAmount const& amount, - bool const overpaymentAllowed, - beast::Journal j); +// Indicates what type of payment is being made. +// regular, late, and full are mutually exclusive. +// overpayment is an "add on" to a regular payment, and follows that path with +// potential extra work at the end. +enum class LoanPaymentType { regular = 0, late, full, overpayment }; Expected loanMakePayment( @@ -257,7 +256,7 @@ loanMakePayment( SLE::ref loan, SLE::const_ref brokerSle, STAmount const& amount, - bool const overpaymentAllowed, + LoanPaymentType const paymentType, beast::Journal j); } // namespace ripple diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index 8c349aafa2..eac586d1a0 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -35,6 +35,14 @@ LoanPaymentParts::operator+=(LoanPaymentParts const& other) return *this; } +bool +LoanPaymentParts::operator==(LoanPaymentParts const& other) const +{ + return principalPaid == other.principalPaid && + interestPaid == other.interestPaid && + valueChange == other.valueChange && feePaid == other.feePaid; +} + Number loanPeriodicRate(TenthBips32 interestRate, std::uint32_t paymentInterval) { @@ -170,6 +178,12 @@ loanLatePaymentInterest( * * The spec is to be updated to base the duration on the next due date */ + + // If the payment is not late by any amount of time, then there's no late + // interest + if (parentCloseTime.time_since_epoch().count() <= nextPaymentDueDate) + return 0; + auto const secondsOverdue = parentCloseTime.time_since_epoch().count() - nextPaymentDueDate; @@ -193,6 +207,11 @@ loanAccruedInterest( */ auto const lastPaymentDate = std::max(prevPaymentDate, startDate); + // 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; + auto const secondsSinceLastPayment = parentCloseTime.time_since_epoch().count() - lastPaymentDate; @@ -499,9 +518,9 @@ doPayment( paymentRemainingProxy = 0; prevPaymentDateProxy = *nextDueDateProxy; - // Remove the field. This is the only condition where nextDueDate is - // allowed to be removed. - nextDueDateProxy = std::nullopt; + // Zero out the next due date. Since it's default, it'll be removed from + // the object. + nextDueDateProxy = 0; // Always zero out the the tracked values on a final payment principalOutstandingProxy = 0; @@ -514,10 +533,8 @@ doPayment( { paymentRemainingProxy -= 1; - prevPaymentDateProxy = *nextDueDateProxy; - // STObject::OptionalField does not define operator+=, so do it the - // old-fashioned way. - nextDueDateProxy = *nextDueDateProxy + paymentInterval; + prevPaymentDateProxy = nextDueDateProxy; + nextDueDateProxy += paymentInterval; } XRPL_ASSERT_PARTS( principalOutstandingProxy > payment.trackedPrincipalDelta, @@ -818,7 +835,7 @@ computeLatePayment( beast::Journal j) { if (!hasExpired(view, nextDueDate)) - return Unexpected(tesSUCCESS); + return Unexpected(tecTOO_SOON); // the payment is late // Late payment interest is only the part of the interest that comes @@ -972,9 +989,19 @@ computeFullPayment( "ripple::detail::computeFullPayment", "total due is rounded"); + JLOG(j.trace()) << "computeFullPayment result: periodicPayment: " + << periodicPayment << ", periodicRate: " << periodicRate + << ", paymentRemaining: " << paymentRemaining + << ", rawPrincipalOutstanding: " << rawPrincipalOutstanding + << ", fullPaymentInterest: " << fullPaymentInterest + << ", roundedFullInterest: " << roundedFullInterest + << ", roundedFullManagementFee: " + << roundedFullManagementFee + << ", untrackedInterest: " << full.untrackedInterest; + if (amount < full.totalDue) // If the payment is less than the full payment amount, it's not - // sufficient to be a full payment, but that's not an error. + // sufficient to be a full payment. return Unexpected(tecINSUFFICIENT_PAYMENT); return full; @@ -1639,111 +1666,6 @@ computeLoanProperties( .firstPaymentPrincipal = firstPaymentPrincipal}; } -Expected -loanMakeFullPayment( - Asset const& asset, - ApplyView& view, - SLE::ref loan, - SLE::const_ref brokerSle, - STAmount const& amount, - bool const overpaymentAllowed, - beast::Journal j) -{ - auto principalOutstandingProxy = loan->at(sfPrincipalOutstanding); - auto paymentRemainingProxy = loan->at(sfPaymentRemaining); - - if (paymentRemainingProxy == 0 || principalOutstandingProxy == 0) - { - // Loan complete - JLOG(j.warn()) << "Loan is already paid off."; - return Unexpected(tecKILLED); - } - - auto totalValueOutstandingProxy = loan->at(sfTotalValueOutstanding); - auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding); - - // Next payment due date must be set unless the loan is complete - auto nextDueDateProxy = loan->at(~sfNextPaymentDueDate); - if (!nextDueDateProxy) - { - JLOG(j.warn()) << "Loan next payment due date is not set."; - return Unexpected(tecINTERNAL); - } - - std::int32_t const loanScale = loan->at(sfLoanScale); - - TenthBips32 const interestRate{loan->at(sfInterestRate)}; - TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)}; - - Number const closePaymentFee = - roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale); - TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)}; - - auto const periodicPayment = loan->at(sfPeriodicPayment); - - auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDate); - std::uint32_t const startDate = loan->at(sfStartDate); - - std::uint32_t const paymentInterval = loan->at(sfPaymentInterval); - // Compute the normal periodic rate, payment, etc. - // We'll need it in the remaining calculations - Number const periodicRate = loanPeriodicRate(interestRate, paymentInterval); - XRPL_ASSERT( - interestRate == 0 || periodicRate > 0, - "ripple::loanMakeFullPayment : valid rate"); - - XRPL_ASSERT( - *totalValueOutstandingProxy > 0, - "ripple::loanMakeFullPayment : valid total value"); - - view.update(loan); - - // ------------------------------------------------------------- - // full payment handling - LoanState const roundedLoanState = calculateRoundedLoanState( - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy); - - if (auto const fullPaymentComponents = detail::computeFullPayment( - asset, - view, - principalOutstandingProxy, - managementFeeOutstandingProxy, - periodicPayment, - paymentRemainingProxy, - prevPaymentDateProxy, - startDate, - paymentInterval, - closeInterestRate, - loanScale, - roundedLoanState.interestDue, - periodicRate, - closePaymentFee, - amount, - managementFeeRate, - j)) - return doPayment( - *fullPaymentComponents, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - else if (fullPaymentComponents.error()) - // error() will be the TER returned if a payment is not made. It - // will only evaluate to true if it's unsuccessful. Otherwise, - // tesSUCCESS means nothing was done, so continue. - return Unexpected(fullPaymentComponents.error()); - - // LCOV_EXCL_START - UNREACHABLE("ripple::loanMakeFullPayment : invalid result"); - return Unexpected(tecINTERNAL); - // LCOV_EXCL_STOP -} - Expected loanMakePayment( Asset const& asset, @@ -1751,7 +1673,7 @@ loanMakePayment( SLE::ref loan, SLE::const_ref brokerSle, STAmount const& amount, - bool const overpaymentAllowed, + LoanPaymentType const paymentType, beast::Journal j) { /* @@ -1775,8 +1697,8 @@ loanMakePayment( auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding); // Next payment due date must be set unless the loan is complete - auto nextDueDateProxy = loan->at(~sfNextPaymentDueDate); - if (!nextDueDateProxy) + auto nextDueDateProxy = loan->at(sfNextPaymentDueDate); + if (*nextDueDateProxy == 0) { JLOG(j.warn()) << "Loan next payment due date is not set."; return Unexpected(tecINTERNAL); @@ -1785,16 +1707,14 @@ loanMakePayment( std::int32_t const loanScale = loan->at(sfLoanScale); TenthBips32 const interestRate{loan->at(sfInterestRate)}; - TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)}; - TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)}; Number const serviceFee = loan->at(sfLoanServiceFee); - Number const latePaymentFee = loan->at(sfLatePaymentFee); TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)}; Number const periodicPayment = loan->at(sfPeriodicPayment); auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDate); + std::uint32_t const startDate = loan->at(sfStartDate); std::uint32_t const paymentInterval = loan->at(sfPaymentInterval); // Compute the normal periodic rate, payment, etc. @@ -1810,7 +1730,80 @@ loanMakePayment( view.update(loan); - detail::PaymentComponentsPlus const periodic{ + // ------------------------------------------------------------- + // A late payment not flagged as late overrides all other options. + if (paymentType != LoanPaymentType::late && + hasExpired(view, nextDueDateProxy)) + { + // If the payment is late, and the late flag was not set, it's not valid + JLOG(j.warn()) + << "Loan payment is overdue. Use the tfLoanLatePayment transaction " + "flag to make a late payment. Loan was created on " + << startDate << ", prev payment due date is " + << prevPaymentDateProxy << ", next payment due date is " + << nextDueDateProxy << ", ledger time is " + << view.parentCloseTime().time_since_epoch().count(); + return Unexpected(tecEXPIRED); + } + + // ------------------------------------------------------------- + // full payment handling + if (paymentType == LoanPaymentType::full) + { + TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)}; + Number const closePaymentFee = + roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale); + + LoanState const roundedLoanState = calculateRoundedLoanState( + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy); + + if (auto const fullPaymentComponents = detail::computeFullPayment( + asset, + view, + principalOutstandingProxy, + managementFeeOutstandingProxy, + periodicPayment, + paymentRemainingProxy, + prevPaymentDateProxy, + startDate, + paymentInterval, + closeInterestRate, + loanScale, + roundedLoanState.interestDue, + periodicRate, + closePaymentFee, + amount, + managementFeeRate, + j)) + { + return doPayment( + *fullPaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + paymentRemainingProxy, + prevPaymentDateProxy, + nextDueDateProxy, + paymentInterval); + } + else if (fullPaymentComponents.error()) + // error() will be the TER returned if a payment is not made. It + // will only evaluate to true if it's unsuccessful. Otherwise, + // tesSUCCESS means nothing was done, so continue. + return Unexpected(fullPaymentComponents.error()); + + // LCOV_EXCL_START + UNREACHABLE("ripple::loanMakePayment : invalid full payment result"); + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + + // ------------------------------------------------------------- + // compute the periodic payment info that will be needed whether the payment + // is late or regular + detail::PaymentComponentsPlus periodic{ detail::computePaymentComponents( asset, loanScale, @@ -1829,21 +1822,75 @@ loanMakePayment( // ------------------------------------------------------------- // late payment handling - if (auto const latePaymentComponents = detail::computeLatePayment( - asset, - view, - principalOutstandingProxy, - *nextDueDateProxy, - periodic, - lateInterestRate, - loanScale, - latePaymentFee, - amount, - managementFeeRate, - j)) + if (paymentType == LoanPaymentType::late) { - return doPayment( - *latePaymentComponents, + TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)}; + Number const latePaymentFee = loan->at(sfLatePaymentFee); + + if (auto const latePaymentComponents = detail::computeLatePayment( + asset, + view, + principalOutstandingProxy, + nextDueDateProxy, + periodic, + lateInterestRate, + loanScale, + latePaymentFee, + amount, + managementFeeRate, + j)) + { + return doPayment( + *latePaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + paymentRemainingProxy, + prevPaymentDateProxy, + nextDueDateProxy, + paymentInterval); + } + else if (latePaymentComponents.error()) + { + // error() will be the TER returned if a payment is not made. It + // will only evaluate to true if it's unsuccessful. + return Unexpected(latePaymentComponents.error()); + } + + // LCOV_EXCL_START + UNREACHABLE("ripple::loanMakePayment : invalid late payment result"); + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + + // ------------------------------------------------------------- + // regular periodic payment handling + + XRPL_ASSERT_PARTS( + paymentType == LoanPaymentType::regular || + paymentType == LoanPaymentType::overpayment, + "ripple::loanMakePayment", + "regular payment type"); + + // This will keep a running total of what is actually paid, if the payment + // is sufficient for any payment + LoanPaymentParts totalParts; + Number totalPaid; + std::size_t numPayments = 0; + + while (amount >= totalPaid + periodic.totalDue && + paymentRemainingProxy > 0 && + numPayments < loanMaximumPaymentsPerTransaction) + { + // Try to make more payments + XRPL_ASSERT_PARTS( + periodic.trackedPrincipalDelta >= 0, + "ripple::loanMakePayment", + "payment pays non-negative principal"); + + totalPaid += periodic.totalDue; + totalParts += detail::doPayment( + periodic, totalValueOutstandingProxy, principalOutstandingProxy, managementFeeOutstandingProxy, @@ -1851,47 +1898,19 @@ loanMakePayment( prevPaymentDateProxy, nextDueDateProxy, paymentInterval); - } - else if (latePaymentComponents.error()) - // error() will be the TER returned if a payment is not made. It will - // only evaluate to true if it's unsuccessful. Otherwise, tesSUCCESS - // means nothing was done, so continue. - return Unexpected(latePaymentComponents.error()); + ++numPayments; - // ------------------------------------------------------------- - // regular periodic payment handling + XRPL_ASSERT_PARTS( + (periodic.specialCase == detail::PaymentSpecialCase::final) == + (paymentRemainingProxy == 0), + "ripple::loanMakePayment", + "final payment is the final payment"); - // if the payment is not late nor if it's a full payment, then it must - // be a periodic one, with possible overpayments + // Don't compute the next payment if this was the last payment + if (periodic.specialCase == detail::PaymentSpecialCase::final) + break; - // This will keep a running total of what is actually paid, if the payment - // is sufficient for a single payment - Number totalPaid = periodic.totalDue; - - if (amount < totalPaid) - { - JLOG(j.warn()) << "Periodic loan payment amount is insufficient. Due: " - << totalPaid << ", paid: " << amount; - return Unexpected(tecINSUFFICIENT_PAYMENT); - } - - LoanPaymentParts totalParts = detail::doPayment( - periodic, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - - std::size_t numPayments = 1; - - while (totalPaid < amount && paymentRemainingProxy > 0 && - numPayments < loanMaximumPaymentsPerTransaction) - { - // Try to make more payments - detail::PaymentComponentsPlus const nextPayment{ + periodic = detail::PaymentComponentsPlus{ detail::computePaymentComponents( asset, loanScale, @@ -1903,40 +1922,13 @@ loanMakePayment( paymentRemainingProxy, managementFeeRate), serviceFee}; - XRPL_ASSERT_PARTS( - nextPayment.trackedPrincipalDelta >= 0, - "ripple::loanMakePayment", - "additional payment pays non-negative principal"); -#if LOANCOMPLETE - XRPL_ASSERT( - nextPayment.rawInterest <= periodic.rawInterest, - "ripple::loanMakePayment : decreasing interest"); - XRPL_ASSERT( - nextPayment.rawPrinicpal >= periodic.rawPrincipal, - "ripple::loanMakePayment : increasing principal"); -#endif + } - if (amount < totalPaid + nextPayment.totalDue) - // We're done making payments. - break; - - totalPaid += nextPayment.totalDue; - totalParts += detail::doPayment( - nextPayment, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - ++numPayments; - - XRPL_ASSERT_PARTS( - (nextPayment.specialCase == detail::PaymentSpecialCase::final) == - (paymentRemainingProxy == 0), - "ripple::loanMakePayment", - "final payment is the final payment"); + if (numPayments == 0) + { + JLOG(j.warn()) << "Regular loan payment amount is insufficient. Due: " + << periodic.totalDue << ", paid: " << amount; + return Unexpected(tecINSUFFICIENT_PAYMENT); } XRPL_ASSERT_PARTS( @@ -1952,9 +1944,9 @@ loanMakePayment( // ------------------------------------------------------------- // overpayment handling - if (overpaymentAllowed && loan->isFlag(lsfLoanOverpayment) && - paymentRemainingProxy > 0 && nextDueDateProxy && totalPaid < amount && - numPayments < loanMaximumPaymentsPerTransaction) + if (paymentType == LoanPaymentType::overpayment && + loan->isFlag(lsfLoanOverpayment) && paymentRemainingProxy > 0 && + totalPaid < amount && numPayments < loanMaximumPaymentsPerTransaction) { TenthBips32 const overpaymentInterestRate{ loan->at(sfOverpaymentInterestRate)}; diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index 052e90e19e..9d2fe4e47d 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -263,7 +263,8 @@ Batch::preflight(PreflightContext const& ctx) return temREDUNDANT; } - if (stx.getFieldU16(sfTransactionType) == ttBATCH) + auto const txType = stx.getFieldU16(sfTransactionType); + if (txType == ttBATCH) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " << "batch cannot have an inner batch txn. " @@ -271,6 +272,14 @@ Batch::preflight(PreflightContext const& ctx) return temINVALID; } + if (std::any_of( + disabledTxTypes.begin(), + disabledTxTypes.end(), + [txType](auto const& disabled) { return txType == disabled; })) + { + return temINVALID_INNER_BATCH; + } + if (!(stx.getFlags() & tfInnerBatchTxn)) { JLOG(ctx.j.debug()) diff --git a/src/xrpld/app/tx/detail/Batch.h b/src/xrpld/app/tx/detail/Batch.h index 1b1d7614d5..7889e91bdc 100644 --- a/src/xrpld/app/tx/detail/Batch.h +++ b/src/xrpld/app/tx/detail/Batch.h @@ -35,6 +35,24 @@ public: TER doApply() override; + + static constexpr auto disabledTxTypes = std::to_array({ + ttVAULT_CREATE, + ttVAULT_SET, + ttVAULT_DELETE, + ttVAULT_DEPOSIT, + ttVAULT_WITHDRAW, + ttVAULT_CLAWBACK, + ttLOAN_BROKER_SET, + ttLOAN_BROKER_DELETE, + ttLOAN_BROKER_COVER_DEPOSIT, + ttLOAN_BROKER_COVER_WITHDRAW, + ttLOAN_BROKER_COVER_CLAWBACK, + ttLOAN_SET, + ttLOAN_DELETE, + ttLOAN_MANAGE, + ttLOAN_PAY, + }); }; } // namespace ripple diff --git a/src/xrpld/app/tx/detail/CreateOffer.cpp b/src/xrpld/app/tx/detail/CreateOffer.cpp index a1179da81b..fe96436f2f 100644 --- a/src/xrpld/app/tx/detail/CreateOffer.cpp +++ b/src/xrpld/app/tx/detail/CreateOffer.cpp @@ -177,13 +177,7 @@ CreateOffer::preclaim(PreclaimContext const& ctx) { // Note that this will get checked again in applyGuts, but it saves // us a call to checkAcceptAsset and possible false negative. - // - // The return code change is attached to featureDepositPreauth as a - // convenience, as the change is not big enough to deserve its own - // amendment. - return ctx.view.rules().enabled(featureDepositPreauth) - ? TER{tecEXPIRED} - : TER{tesSUCCESS}; + return tecEXPIRED; } // Make sure that we are authorized to hold what the taker will pay us. @@ -235,10 +229,7 @@ CreateOffer::checkAcceptAsset( return (flags & tapRETRY) ? TER{terNO_ACCOUNT} : TER{tecNO_ISSUER}; } - // This code is attached to the DepositPreauth amendment as a matter of - // convenience. The change is not significant enough to deserve its - // own amendment. - if (view.rules().enabled(featureDepositPreauth) && (issue.account == id)) + if (issue.account == id) // An account can always accept its own issuance. return tesSUCCESS; @@ -599,13 +590,7 @@ CreateOffer::applyGuts(Sandbox& sb, Sandbox& sbCancel) { // If the offer has expired, the transaction has successfully // done nothing, so short circuit from here. - // - // The return code change is attached to featureDepositPreauth as a - // convenience. The change is not big enough to deserve a fix code. - TER const ter{ - sb.rules().enabled(featureDepositPreauth) ? TER{tecEXPIRED} - : TER{tesSUCCESS}}; - return {ter, true}; + return {tecEXPIRED, true}; } bool const bOpenLedger = sb.open(); diff --git a/src/xrpld/app/tx/detail/DeleteAccount.cpp b/src/xrpld/app/tx/detail/DeleteAccount.cpp index ed72900248..0654c8dbce 100644 --- a/src/xrpld/app/tx/detail/DeleteAccount.cpp +++ b/src/xrpld/app/tx/detail/DeleteAccount.cpp @@ -232,8 +232,7 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) if (!ctx.tx.isFieldPresent(sfCredentialIDs)) { // Check whether the destination account requires deposit authorization. - if (ctx.view.rules().enabled(featureDepositAuth) && - (sleDst->getFlags() & lsfDepositAuth)) + if (sleDst->getFlags() & lsfDepositAuth) { if (!ctx.view.exists(keylet::depositPreauth(dst, account))) return tecNO_PERMISSION; @@ -353,8 +352,7 @@ DeleteAccount::doApply() if (!src || !dst) return tefBAD_LEDGER; // LCOV_EXCL_LINE - if (ctx_.view().rules().enabled(featureDepositAuth) && - ctx_.tx.isFieldPresent(sfCredentialIDs)) + if (ctx_.tx.isFieldPresent(sfCredentialIDs)) { if (auto err = verifyDepositPreauth( ctx_.tx, ctx_.view(), account_, dstID, dst, ctx_.journal); diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index fb3e3c509e..5cf90809a2 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -459,12 +459,6 @@ EscrowCreate::doApply() if (((*sled)[sfFlags] & lsfRequireDestTag) && !ctx_.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - if (!ctx_.view().rules().enabled(featureDepositAuth) && - ((*sled)[sfFlags] & lsfDisallowXRP)) - return tecNO_TARGET; } // Create escrow in ledger. Note that we we use the value from the @@ -1041,13 +1035,10 @@ EscrowFinish::doApply() if (!sled) return tecNO_DST; - if (ctx_.view().rules().enabled(featureDepositAuth)) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, destID, sled, ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), account_, destID, sled, ctx_.journal); + !isTesSuccess(err)) + return err; AccountID const account = (*slep)[sfAccount]; diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index c45d317dea..73754404e5 100644 --- a/src/xrpld/app/tx/detail/LoanManage.cpp +++ b/src/xrpld/app/tx/detail/LoanManage.cpp @@ -276,7 +276,9 @@ LoanManage::defaultLoan( paymentRemainingProxy = 0; principalOutstandingProxy = 0; managementFeeOutstandingProxy = 0; - loanSle->at(~sfNextPaymentDueDate) = std::nullopt; + // Zero out the next due date. Since it's default, it'll be removed from + // the object. + loanSle->at(sfNextPaymentDueDate) = 0; view.update(loanSle); // Return funds from the LoanBroker pseudo-account to the diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index fe19ed1c0d..64e18016e1 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -30,9 +30,21 @@ LoanPay::preflight(PreflightContext const& ctx) if (ctx.tx[sfAmount] <= beast::zero) return temBAD_AMOUNT; - // isFlag requires an exact match - all flags to be set - to return true. - if (ctx.tx.isFlag(tfLoanOverpayment | tfLoanFullPayment)) + // 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) + { + JLOG(ctx.j.warn()) << "Only one LoanPay flag can be set per tx. " + << flagsSet << " is too many."; return temINVALID_FLAG; + } return tesSUCCESS; } @@ -42,8 +54,8 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) { auto const normalCost = Transactor::calculateBaseFee(view, tx); - if (tx.isFlag(tfLoanFullPayment)) - // The loan will be making one set of calculations for one (large) + if (tx.isFlag(tfLoanFullPayment) || tx.isFlag(tfLoanLatePayment)) + // The loan will be making one set of calculations for one full or late // payment return normalCost; @@ -65,7 +77,8 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) } if (hasExpired(view, loanSle->at(sfNextPaymentDueDate))) - // If the payment is late, it'll only make one payment + // If the payment is late, and the late payment flag is not set, it'll + // fail return normalCost; auto const brokerSle = @@ -97,6 +110,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) // Estimate how many payments will be made Number const numPaymentEstimate = static_cast(amount / regularPayment); + // Charge one base fee per paymentsPerFeeIncrement payments, rounding up. Number::setround(Number::upward); auto const feeIncrements = std::max( @@ -291,23 +305,19 @@ LoanPay::doApply() LoanManage::unimpairLoan(view, loanSle, vaultSle, j_); } - Expected const paymentParts = - tx.isFlag(tfLoanFullPayment) ? loanMakeFullPayment( - asset, - view, - loanSle, - brokerSle, - amount, - tx.isFlag(tfLoanOverpayment), - j_) - : loanMakePayment( - asset, - view, - loanSle, - brokerSle, - amount, - tx.isFlag(tfLoanOverpayment), - j_); + LoanPaymentType const paymentType = [&tx]() { + // preflight already checked that at most one flag is set. + if (tx.isFlag(tfLoanLatePayment)) + return LoanPaymentType::late; + if (tx.isFlag(tfLoanFullPayment)) + return LoanPaymentType::full; + if (tx.isFlag(tfLoanOverpayment)) + return LoanPaymentType::overpayment; + return LoanPaymentType::regular; + }(); + + Expected const paymentParts = loanMakePayment( + asset, view, loanSle, brokerSle, amount, paymentType, j_); if (!paymentParts) { diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index 27b8380f5f..c3dc99e7fb 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -116,8 +116,7 @@ closeChannel( } // Remove PayChan from recipient's owner directory, if present. - if (auto const page = (*slep)[~sfDestinationNode]; - page && view.rules().enabled(fixPayChanRecipientOwnerDir)) + if (auto const page = (*slep)[~sfDestinationNode]) { auto const dst = (*slep)[sfDestination]; if (!view.dirRemove(keylet::ownerDir(dst), *page, key, true)) @@ -210,12 +209,6 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) if ((flags & lsfRequireDestTag) && !ctx.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - if (!ctx.view.rules().enabled(featureDepositAuth) && - (flags & lsfDisallowXRP)) - return tecNO_TARGET; - // Pseudo-accounts cannot receive payment channels, other than native // to their underlying ledger object - implemented in their respective // transaction types. Note, this is not amendment-gated because all @@ -284,7 +277,6 @@ PayChanCreate::doApply() } // Add PayChan to the recipient's owner directory - if (ctx_.view().rules().enabled(fixPayChanRecipientOwnerDir)) { auto const page = ctx_.view().dirInsert( keylet::ownerDir(dst), payChanKeylet, describeOwnerDir(dst)); @@ -527,20 +519,10 @@ PayChanClaim::doApply() if (!sled) return tecNO_DST; - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - bool const depositAuth{ctx_.view().rules().enabled(featureDepositAuth)}; - if (!depositAuth && - (txAccount == src && (sled->getFlags() & lsfDisallowXRP))) - return tecNO_TARGET; - - if (depositAuth) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal); + !isTesSuccess(err)) + return err; (*slep)[sfBalance] = ctx_.tx[sfBalance]; XRPAmount const reqDelta = reqBalance - chanBalance; diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 9ab9dafd28..931048d1a9 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -417,21 +417,9 @@ Payment::doApply() view().update(sleDst); } - // Determine whether the destination requires deposit authorization. - bool const depositAuth = view().rules().enabled(featureDepositAuth); - bool const reqDepositAuth = - sleDst->getFlags() & lsfDepositAuth && depositAuth; - - bool const depositPreauth = view().rules().enabled(featureDepositPreauth); - bool const ripple = (hasPaths || sendMax || !dstAmount.native()) && !mptDirect; - // If the destination has lsfDepositAuth set, then only direct XRP - // payments (no intermediate steps) are allowed to the destination. - if (!depositPreauth && ripple && reqDepositAuth) - return tecNO_PERMISSION; - if (ripple) { return makeRipplePayment(RipplePaymentParams{ @@ -507,43 +495,40 @@ Payment::doApply() // The source account does have enough money. Make sure the // source account has authority to deposit to the destination. - if (depositAuth) + // An account that requires authorization has three ways to get an XRP + // Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination, or + // 3. If the destination's XRP balance is + // a. less than or equal to the base reserve and + // b. the deposit amount is less than or equal to the base reserve, + // then we allow the deposit. + // + // Rule 3 is designed to keep an account from getting wedged + // in an unusable state if it sets the lsfDepositAuth flag and + // then consumes all of its XRP. Without the rule if an + // account with lsfDepositAuth set spent all of its XRP, it + // would be unable to acquire more XRP required to pay fees. + // + // We choose the base reserve as our bound because it is + // a small number that seldom changes but is always sufficient + // to get the account un-wedged. + + // Get the base reserve. + XRPAmount const dstReserve{view().fees().reserve}; + + if (dstAmount > dstReserve || + sleDst->getFieldAmount(sfBalance) > dstReserve) { - // If depositPreauth is enabled, then an account that requires - // authorization has three ways to get an XRP Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination, or - // 3. If the destination's XRP balance is - // a. less than or equal to the base reserve and - // b. the deposit amount is less than or equal to the base reserve, - // then we allow the deposit. - // - // Rule 3 is designed to keep an account from getting wedged - // in an unusable state if it sets the lsfDepositAuth flag and - // then consumes all of its XRP. Without the rule if an - // account with lsfDepositAuth set spent all of its XRP, it - // would be unable to acquire more XRP required to pay fees. - // - // We choose the base reserve as our bound because it is - // a small number that seldom changes but is always sufficient - // to get the account un-wedged. - - // Get the base reserve. - XRPAmount const dstReserve{view().fees().reserve}; - - if (dstAmount > dstReserve || - sleDst->getFieldAmount(sfBalance) > dstReserve) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, - ctx_.view(), - account_, - dstAccountID, - sleDst, - ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; } // Do the arithmetic for the transfer and make the ledger change. @@ -594,23 +579,15 @@ Payment::makeRipplePayment(Payment::RipplePaymentParams const& p) // Ripple payment with at least one intermediate step and uses // transitive balances. - if (depositPreauth && depositAuth) - { - // If depositPreauth is enabled, then 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. + // 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; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), account_, dstAccountID, sleDst, ctx_.journal); + !isTesSuccess(err)) + return err; path::RippleCalc::Input rcInput; rcInput.partialPaymentAllowed = partialPaymentAllowed; diff --git a/src/xrpld/app/tx/detail/SetAccount.cpp b/src/xrpld/app/tx/detail/SetAccount.cpp index e966dba8f5..db513d9f05 100644 --- a/src/xrpld/app/tx/detail/SetAccount.cpp +++ b/src/xrpld/app/tx/detail/SetAccount.cpp @@ -463,18 +463,15 @@ SetAccount::doApply() // // DepositAuth // - if (view().rules().enabled(featureDepositAuth)) + if (uSetFlag == asfDepositAuth) { - if (uSetFlag == asfDepositAuth) - { - JLOG(j_.trace()) << "Set lsfDepositAuth."; - uFlagsOut |= lsfDepositAuth; - } - else if (uClearFlag == asfDepositAuth) - { - JLOG(j_.trace()) << "Clear lsfDepositAuth."; - uFlagsOut &= ~lsfDepositAuth; - } + JLOG(j_.trace()) << "Set lsfDepositAuth."; + uFlagsOut |= lsfDepositAuth; + } + else if (uClearFlag == asfDepositAuth) + { + JLOG(j_.trace()) << "Clear lsfDepositAuth."; + uFlagsOut &= ~lsfDepositAuth; } //