From 8d2dff2e483e4799bf2f5988e78df115e5a00976 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 10 Nov 2025 15:32:08 +0000 Subject: [PATCH 1/2] chore: Unify build & test, add ctest to coverage (#6013) This change unifies the build and test jobs into a single job, and adds `ctest` to coverage reporting. The mechanics of coverage reporting is slightly complex and most of it is encapsulated in the `coverage` target. The status quo way of preparing coverage reports involves running a single target `cmake --build . --target coverage`, which does three things: * Build the `rippled` binary (via target dependency) * Prepare coverage reports: * Run `./rippled -u` unit tests. * Gather test output and build reports. This makes it awkward to add an additional `ctest` step between build and coverage reporting steps. The better solution is to split `coverage` target into separate build, followed by `ctest`, followed by test generation. Luckily, the `coverage` target has been designed specifically to support such case; it does not need to build `rippled`, it's just a dependency. Similarly it allows additional tests to be run before gathering test outputs; in principle we could even strip it from running tests and run them separately instead. This means we can keep build, `ctest` and generation of coverage reports as separate steps, as long as the state of build directory is fully (including file timestamps, additional coverage files etc.) preserved between the steps. This means that in order to run `ctest` for coverage reporting we need to integrate build and test into a single job, which this change does. --- .github/scripts/strategy-matrix/generate.py | 2 - .../workflows/reusable-build-test-config.yml | 186 +++++++++++++++--- .github/workflows/reusable-build.yml | 154 --------------- .github/workflows/reusable-test.yml | 111 ----------- cmake/XrplAddTest.cmake | 16 -- src/tests/libxrpl/CMakeLists.txt | 8 +- 6 files changed, 169 insertions(+), 308 deletions(-) delete mode 100644 .github/workflows/reusable-build.yml delete mode 100644 .github/workflows/reusable-test.yml diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 025d553b5e..c762d61ccd 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -138,8 +138,6 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: # Unity on linux/amd64 if f'{os['compiler_name']}-{os['compiler_version']}' == 'gcc-15' and build_type == 'Debug' and '-Dunity=OFF' in cmake_args and architecture['platform'] == 'linux/amd64': cmake_args = f'-Dcoverage=ON -Dcoverage_format=xml -DCODE_COVERAGE_VERBOSE=ON -DCMAKE_C_FLAGS=-O0 -DCMAKE_CXX_FLAGS=-O0 {cmake_args}' - cmake_target = 'coverage' - build_only = True # Generate a unique name for the configuration, e.g. macos-arm64-debug # or debian-bookworm-gcc-12-amd64-release-unity. diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index a59dbda71b..4a8bc71321 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -7,19 +7,23 @@ on: description: "The directory where to build." required: true type: string + build_only: description: 'Whether to only build or to build and test the code ("true", "false").' required: true type: boolean + build_type: description: 'The build type to use ("Debug", "Release").' type: string required: true + cmake_args: description: "Additional arguments to pass to CMake." required: false type: string default: "" + cmake_target: description: "The CMake target to build." type: string @@ -29,6 +33,7 @@ on: description: Runner to run the job on as a JSON string required: true type: string + image: description: "The image to run in (leave empty to run natively)" required: true @@ -51,27 +56,162 @@ on: required: true jobs: - build: - uses: ./.github/workflows/reusable-build.yml - with: - build_dir: ${{ inputs.build_dir }} - build_type: ${{ inputs.build_type }} - cmake_args: ${{ inputs.cmake_args }} - cmake_target: ${{ inputs.cmake_target }} - runs_on: ${{ inputs.runs_on }} - image: ${{ inputs.image }} - config_name: ${{ inputs.config_name }} - nproc_subtract: ${{ inputs.nproc_subtract }} - secrets: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + build-and-test: + name: ${{ inputs.config_name }} + runs-on: ${{ fromJSON(inputs.runs_on) }} + container: ${{ inputs.image != '' && inputs.image || null }} + timeout-minutes: 60 + env: + 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 - test: - needs: build - uses: ./.github/workflows/reusable-test.yml - with: - run_tests: ${{ !inputs.build_only }} - verify_voidstar: ${{ contains(inputs.cmake_args, '-Dvoidstar=ON') }} - runs_on: ${{ inputs.runs_on }} - image: ${{ inputs.image }} - config_name: ${{ inputs.config_name }} - nproc_subtract: ${{ inputs.nproc_subtract }} + - name: Checkout repository + uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + + - name: Prepare runner + uses: XRPLF/actions/.github/actions/prepare-runner@99685816bb60a95a66852f212f382580e180df3a + with: + disable_ccache: false + + - name: Print build environment + uses: ./.github/actions/print-env + + - name: Get number of processors + uses: XRPLF/actions/.github/actions/get-nproc@046b1620f6bfd6cd0985dc82c3df02786801fe0a + id: nproc + with: + subtract: ${{ inputs.nproc_subtract }} + + - name: Setup Conan + uses: ./.github/actions/setup-conan + + - name: Build dependencies + uses: ./.github/actions/build-deps + with: + build_dir: ${{ inputs.build_dir }} + build_nproc: ${{ steps.nproc.outputs.nproc }} + build_type: ${{ inputs.build_type }} + # Set the verbosity to "quiet" for Windows to avoid an excessive + # amount of logs. For other OSes, the "verbose" logs are more useful. + log_verbosity: ${{ runner.os == 'Windows' && 'quiet' || 'verbose' }} + + - name: Configure CMake + shell: bash + working-directory: ${{ inputs.build_dir }} + env: + BUILD_TYPE: ${{ inputs.build_type }} + CMAKE_ARGS: ${{ inputs.cmake_args }} + run: | + cmake \ + -G '${{ runner.os == 'Windows' && 'Visual Studio 17 2022' || 'Ninja' }}' \ + -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \ + -DCMAKE_BUILD_TYPE="${BUILD_TYPE}" \ + ${CMAKE_ARGS} \ + .. + + - name: Build the binary + shell: bash + working-directory: ${{ inputs.build_dir }} + env: + BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} + BUILD_TYPE: ${{ inputs.build_type }} + CMAKE_TARGET: ${{ inputs.cmake_target }} + run: | + cmake \ + --build . \ + --config "${BUILD_TYPE}" \ + --parallel "${BUILD_NPROC}" \ + --target "${CMAKE_TARGET}" + + - name: Upload rippled artifact (Linux) + if: ${{ runner.os == 'Linux' }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + env: + BUILD_DIR: ${{ inputs.build_dir }} + with: + name: rippled-${{ inputs.config_name }} + path: ${{ env.BUILD_DIR }}/rippled + retention-days: 3 + if-no-files-found: error + + - 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 + echo 'The binary is statically linked.' + else + echo 'The binary is dynamically linked.' + exit 1 + fi + + - 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 + + - name: Run the separate tests + if: ${{ !inputs.build_only }} + working-directory: ${{ inputs.build_dir }} + # Windows locks some of the build files while running tests, and parallel jobs can collide + 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: 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 . \ + --config "${BUILD_TYPE}" \ + --parallel "${BUILD_NPROC}" \ + --target coverage + + - name: Upload coverage report + if: ${{ github.repository_owner == 'XRPLF' && !inputs.build_only && env.ENABLED_COVERAGE == 'true' }} + uses: codecov/codecov-action@18283e04ce6e62d37312384ff67231eb8fd56d24 # v5.4.3 + with: + disable_search: true + disable_telem: true + fail_ci_if_error: true + files: ${{ inputs.build_dir }}/coverage.xml + 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-build.yml b/.github/workflows/reusable-build.yml deleted file mode 100644 index a9d5fd7f7c..0000000000 --- a/.github/workflows/reusable-build.yml +++ /dev/null @@ -1,154 +0,0 @@ -name: Build rippled - -on: - workflow_call: - inputs: - build_dir: - description: "The directory where to build." - required: true - type: string - build_type: - description: 'The build type to use ("Debug", "Release").' - required: true - type: string - cmake_args: - description: "Additional arguments to pass to CMake." - required: true - type: string - cmake_target: - description: "The CMake target to build." - required: true - type: string - - runs_on: - description: Runner to run the job on as a JSON string - required: true - type: string - image: - description: "The image to run in (leave empty to run natively)" - required: true - type: string - - config_name: - description: "The name of the configuration." - required: true - type: string - - nproc_subtract: - description: "The number of processors to subtract when calculating parallelism." - required: true - type: number - - secrets: - CODECOV_TOKEN: - description: "The Codecov token to use for uploading coverage reports." - required: true - -defaults: - run: - shell: bash - -jobs: - build: - name: Build ${{ inputs.config_name }} - runs-on: ${{ fromJSON(inputs.runs_on) }} - container: ${{ inputs.image != '' && inputs.image || null }} - timeout-minutes: 60 - steps: - - name: Cleanup workspace - if: ${{ runner.os == 'macOS' }} - uses: XRPLF/actions/.github/actions/cleanup-workspace@3f044c7478548e3c32ff68980eeb36ece02b364e - - - name: Checkout repository - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - - - name: Prepare runner - uses: XRPLF/actions/.github/actions/prepare-runner@99685816bb60a95a66852f212f382580e180df3a - with: - disable_ccache: false - - - name: Print build environment - uses: ./.github/actions/print-env - - - name: Get number of processors - uses: XRPLF/actions/.github/actions/get-nproc@046b1620f6bfd6cd0985dc82c3df02786801fe0a - id: nproc - with: - subtract: ${{ inputs.nproc_subtract }} - - - name: Setup Conan - uses: ./.github/actions/setup-conan - - - name: Build dependencies - uses: ./.github/actions/build-deps - with: - build_dir: ${{ inputs.build_dir }} - build_nproc: ${{ steps.nproc.outputs.nproc }} - build_type: ${{ inputs.build_type }} - # Set the verbosity to "quiet" for Windows to avoid an excessive - # amount of logs. For other OSes, the "verbose" logs are more useful. - log_verbosity: ${{ runner.os == 'Windows' && 'quiet' || 'verbose' }} - - - name: Configure CMake - shell: bash - working-directory: ${{ inputs.build_dir }} - env: - BUILD_TYPE: ${{ inputs.build_type }} - CMAKE_ARGS: ${{ inputs.cmake_args }} - run: | - cmake \ - -G '${{ runner.os == 'Windows' && 'Visual Studio 17 2022' || 'Ninja' }}' \ - -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \ - -DCMAKE_BUILD_TYPE="${BUILD_TYPE}" \ - ${CMAKE_ARGS} \ - .. - - - name: Build the binary - shell: bash - working-directory: ${{ inputs.build_dir }} - env: - BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} - BUILD_TYPE: ${{ inputs.build_type }} - CMAKE_TARGET: ${{ inputs.cmake_target }} - run: | - cmake \ - --build . \ - --config "${BUILD_TYPE}" \ - --parallel ${BUILD_NPROC} \ - --target "${CMAKE_TARGET}" - - - name: Put built binaries in one location - shell: bash - working-directory: ${{ inputs.build_dir }} - env: - BUILD_TYPE_DIR: ${{ runner.os == 'Windows' && inputs.build_type || '' }} - CMAKE_TARGET: ${{ inputs.cmake_target }} - run: | - mkdir -p ./binaries/doctest/ - - cp ./${BUILD_TYPE_DIR}/rippled* ./binaries/ - if [ "${CMAKE_TARGET}" != 'coverage' ]; then - cp ./src/tests/libxrpl/${BUILD_TYPE_DIR}/xrpl.test.* ./binaries/doctest/ - fi - - - name: Upload rippled artifact - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 - env: - BUILD_DIR: ${{ inputs.build_dir }} - with: - name: rippled-${{ inputs.config_name }} - path: ${{ env.BUILD_DIR }}/binaries/ - retention-days: 3 - if-no-files-found: error - - - name: Upload coverage report - if: ${{ github.repository_owner == 'XRPLF' && inputs.cmake_target == 'coverage' }} - uses: codecov/codecov-action@18283e04ce6e62d37312384ff67231eb8fd56d24 # v5.4.3 - with: - disable_search: true - disable_telem: true - fail_ci_if_error: true - files: ${{ inputs.build_dir }}/coverage.xml - plugins: noop - token: ${{ secrets.CODECOV_TOKEN }} - verbose: true diff --git a/.github/workflows/reusable-test.yml b/.github/workflows/reusable-test.yml deleted file mode 100644 index 8d4a4a8d33..0000000000 --- a/.github/workflows/reusable-test.yml +++ /dev/null @@ -1,111 +0,0 @@ -name: Test rippled - -on: - workflow_call: - inputs: - verify_voidstar: - description: "Whether to verify the presence of voidstar instrumentation." - required: true - type: boolean - run_tests: - description: "Whether to run unit tests" - required: true - type: boolean - - runs_on: - description: Runner to run the job on as a JSON string - required: true - type: string - image: - description: "The image to run in (leave empty to run natively)" - required: true - type: string - - config_name: - description: "The name of the configuration." - required: true - type: string - - nproc_subtract: - description: "The number of processors to subtract when calculating parallelism." - required: true - type: number - -jobs: - test: - name: Test ${{ inputs.config_name }} - runs-on: ${{ fromJSON(inputs.runs_on) }} - container: ${{ inputs.image != '' && inputs.image || null }} - timeout-minutes: 30 - steps: - - name: Cleanup workspace - if: ${{ runner.os == 'macOS' }} - uses: XRPLF/actions/.github/actions/cleanup-workspace@3f044c7478548e3c32ff68980eeb36ece02b364e - - - name: Get number of processors - uses: XRPLF/actions/.github/actions/get-nproc@046b1620f6bfd6cd0985dc82c3df02786801fe0a - id: nproc - with: - subtract: ${{ inputs.nproc_subtract }} - - - name: Download rippled artifact - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 - with: - name: rippled-${{ inputs.config_name }} - - - name: Make binary executable (Linux and macOS) - shell: bash - if: ${{ runner.os == 'Linux' || runner.os == 'macOS' }} - run: | - chmod +x ./rippled - - - name: Check linking (Linux) - if: ${{ runner.os == 'Linux' }} - shell: bash - run: | - ldd ./rippled - if [ "$(ldd ./rippled | grep -E '(libstdc\+\+|libgcc)' | wc -l)" -eq 0 ]; then - echo 'The binary is statically linked.' - else - echo 'The binary is dynamically linked.' - exit 1 - fi - - - name: Verifying presence of instrumentation - if: ${{ inputs.verify_voidstar }} - shell: bash - run: | - ./rippled --version | grep libvoidstar - - - name: Run the embedded tests - if: ${{ inputs.run_tests }} - shell: bash - env: - BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} - run: | - ./rippled --unittest --unittest-jobs ${BUILD_NPROC} - - - name: Run the separate tests - if: ${{ inputs.run_tests }} - env: - EXT: ${{ runner.os == 'Windows' && '.exe' || '' }} - shell: bash - run: | - for test_file in ./doctest/*${EXT}; do - echo "Executing $test_file" - chmod +x "$test_file" - if [[ "${{ runner.os }}" == "Windows" && "$test_file" == "./doctest/xrpl.test.net.exe" ]]; then - echo "Skipping $test_file on Windows" - else - "$test_file" - fi - done - - - name: Debug failure (Linux) - if: ${{ failure() && runner.os == 'Linux' && inputs.run_tests }} - shell: bash - run: | - echo "IPv4 local port range:" - cat /proc/sys/net/ipv4/ip_local_port_range - echo "Netstat:" - netstat -an diff --git a/cmake/XrplAddTest.cmake b/cmake/XrplAddTest.cmake index a1dd3847bd..191a25c467 100644 --- a/cmake/XrplAddTest.cmake +++ b/cmake/XrplAddTest.cmake @@ -22,20 +22,4 @@ function(xrpl_add_test name) UNITY_BUILD_BATCH_SIZE 0) # Adjust as needed add_test(NAME ${target} COMMAND ${target}) - set_tests_properties( - ${target} PROPERTIES - FIXTURES_REQUIRED ${target}_fixture - ) - - add_test( - NAME ${target}.build - COMMAND - ${CMAKE_COMMAND} - --build ${CMAKE_BINARY_DIR} - --config $ - --target ${target} - ) - set_tests_properties(${target}.build PROPERTIES - FIXTURES_SETUP ${target}_fixture - ) endfunction() diff --git a/src/tests/libxrpl/CMakeLists.txt b/src/tests/libxrpl/CMakeLists.txt index 5bae91c929..880fdb2948 100644 --- a/src/tests/libxrpl/CMakeLists.txt +++ b/src/tests/libxrpl/CMakeLists.txt @@ -14,5 +14,9 @@ xrpl_add_test(crypto) target_link_libraries(xrpl.test.crypto PRIVATE xrpl.imports.test) xrpl_add_test(json) target_link_libraries(xrpl.test.json PRIVATE xrpl.imports.test) -xrpl_add_test(net) -target_link_libraries(xrpl.test.net PRIVATE xrpl.imports.test) + +# 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) +endif() From 12c629a1d2e676848ae185306ab2d28660cfc74f Mon Sep 17 00:00:00 2001 From: Jingchen Date: Mon, 10 Nov 2025 16:03:10 +0000 Subject: [PATCH 2/2] refactor: Retire CheckCashMakesTrustLine amendment (#5974) Amendments activated for more than 2 years can be retired. This change retires the CheckCashMakesTrustLine amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/Check_test.cpp | 105 +++++--------------- src/xrpld/app/tx/detail/CashCheck.cpp | 38 ++----- 3 files changed, 35 insertions(+), 110 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 6ac33f3e78..53da9eb38d 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -63,7 +63,6 @@ XRPL_FEATURE(XRPFees, Supported::yes, VoteBehavior::DefaultNo XRPL_FEATURE(DisallowIncoming, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (RemoveNFTokenAutoTrustLine, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(ExpandedSignerList, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FEATURE(CheckCashMakesTrustLine, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYes) @@ -119,6 +118,7 @@ XRPL_RETIRE(fixRmSmallIncreasedQOffers) XRPL_RETIRE(fixSTAmountCanonicalize) XRPL_RETIRE(fixTakerDryOfferRemoval) XRPL_RETIRE(fixTrustLinesToSelf) +XRPL_RETIRE(CheckCashMakesTrustLine) XRPL_RETIRE(CryptoConditions) XRPL_RETIRE(Escrow) XRPL_RETIRE(EnforceInvariants) diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 9fec61da7b..a9534bf5f9 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -679,9 +679,6 @@ class Check_test : public beast::unit_test::suite using namespace test::jtx; - bool const cashCheckMakesTrustLine = - features[featureCheckCashMakesTrustLine]; - Account const gw{"gateway"}; Account const alice{"alice"}; Account const bob{"bob"}; @@ -714,26 +711,10 @@ class Check_test : public beast::unit_test::suite // and fails because he hasn't got a trust line for USD. env(pay(gw, alice, USD(0.5))); env.close(); - if (!cashCheckMakesTrustLine) - { - // If cashing a check automatically creates a trustline then - // this returns tesSUCCESS and the check is removed from the - // ledger which would mess up later tests. - env(check::cash(bob, chkId1, USD(10)), ter(tecNO_LINE)); - env.close(); - } // bob sets up the trust line, but not at a high enough limit. env(trust(bob, USD(9.5))); env.close(); - if (!cashCheckMakesTrustLine) - { - // If cashing a check is allowed to exceed the trust line - // limit then this returns tesSUCCESS and the check is - // removed from the ledger which would mess up later tests. - env(check::cash(bob, chkId1, USD(10)), ter(tecPATH_PARTIAL)); - env.close(); - } // bob sets the trust line limit high enough but asks for more // than the check's SendMax. @@ -808,34 +789,31 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, alice) == 2); BEAST_EXPECT(ownerCount(env, bob) == 1); - if (cashCheckMakesTrustLine) - { - // Automatic trust lines are enabled. But one aspect of - // automatic trust lines is that they allow the account - // cashing a check to exceed their trust line limit. Show - // that at work. - // - // bob's trust line limit is currently USD(10.5). Show that - // a payment to bob cannot exceed that trust line, but cashing - // a check can. + // Automatic trust lines are enabled. But one aspect of + // automatic trust lines is that they allow the account + // cashing a check to exceed their trust line limit. Show + // that at work. + // + // bob's trust line limit is currently USD(10.5). Show that + // a payment to bob cannot exceed that trust line, but cashing + // a check can. - // Payment of 20 USD fails. - env(pay(gw, bob, USD(20)), ter(tecPATH_PARTIAL)); - env.close(); + // Payment of 20 USD fails. + env(pay(gw, bob, USD(20)), ter(tecPATH_PARTIAL)); + env.close(); - uint256 const chkId20{getCheckIndex(gw, env.seq(gw))}; - env(check::create(gw, bob, USD(20))); - env.close(); + uint256 const chkId20{getCheckIndex(gw, env.seq(gw))}; + env(check::create(gw, bob, USD(20))); + env.close(); - // However cashing a check for 20 USD succeeds. - env(check::cash(bob, chkId20, USD(20))); - env.close(); - env.require(balance(bob, USD(30))); + // However cashing a check for 20 USD succeeds. + env(check::cash(bob, chkId20, USD(20))); + env.close(); + env.require(balance(bob, USD(30))); - // Clean up this most recent experiment so the rest of the - // tests work. - env(pay(bob, gw, USD(20))); - } + // Clean up this most recent experiment so the rest of the + // tests work. + env(pay(bob, gw, USD(20))); // ... so bob cancels alice's remaining check. env(check::cancel(bob, chkId3)); @@ -950,8 +928,7 @@ class Check_test : public beast::unit_test::suite env(check::create(alice, bob, USD(7))); env.close(); - env(check::cash(bob, chkId, USD(7)), - ter(cashCheckMakesTrustLine ? tecNO_AUTH : tecNO_LINE)); + env(check::cash(bob, chkId, USD(7)), ter(tecNO_AUTH)); env.close(); // Now give bob a trustline for USD. bob still can't cash the @@ -966,17 +943,6 @@ class Check_test : public beast::unit_test::suite env(trust(gw, bob["USD"](1)), txflags(tfSetfAuth)); env.close(); - // bob tries to cash the check again but fails because his trust - // limit is too low. - if (!cashCheckMakesTrustLine) - { - // If cashing a check is allowed to exceed the trust line - // limit then this returns tesSUCCESS and the check is - // removed from the ledger which would mess up later tests. - env(check::cash(bob, chkId, USD(7)), ter(tecPATH_PARTIAL)); - env.close(); - } - // Two possible outcomes here depending on whether cashing a // check can build a trust line: // o If it can't build a trust line, then since bob set his @@ -985,7 +951,7 @@ class Check_test : public beast::unit_test::suite // o If it can build a trust line, then the check is allowed to // exceed the trust limit and bob gets the full transfer. env(check::cash(bob, chkId, check::DeliverMin(USD(4)))); - STAmount const bobGot = cashCheckMakesTrustLine ? USD(7) : USD(5); + STAmount const bobGot = USD(7); verifyDeliveredAmount(env, bobGot); env.require(balance(alice, USD(8) - bobGot)); env.require(balance(bob, bobGot)); @@ -1368,23 +1334,6 @@ class Check_test : public beast::unit_test::suite env(pay(gw, alice, USD(20))); env.close(); - // Before bob gets a trustline, have him try to cash a check. - // Should fail. - { - uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; - env(check::create(alice, bob, USD(20))); - env.close(); - - if (!features[featureCheckCashMakesTrustLine]) - { - // If cashing a check automatically creates a trustline then - // this returns tesSUCCESS and the check is removed from the - // ledger which would mess up later tests. - env(check::cash(bob, chkId, USD(20)), ter(tecNO_LINE)); - env.close(); - } - } - // Now set up bob's trustline. env(trust(bob, USD(20))); env.close(); @@ -1984,10 +1933,6 @@ class Check_test : public beast::unit_test::suite { // Explore automatic trust line creation when a check is cashed. // - // This capability is enabled by the featureCheckCashMakesTrustLine - // amendment. So this test executes only when that amendment is - // active. - assert(features[featureCheckCashMakesTrustLine]); testcase("Trust Line Creation"); @@ -2688,11 +2633,9 @@ public: { using namespace test::jtx; auto const sa = testable_amendments(); - testWithFeats(sa - featureCheckCashMakesTrustLine); testWithFeats(sa - disallowIncoming); testWithFeats(sa); - - testTrustLineCreation(sa); // Test with featureCheckCashMakesTrustLine + testTrustLineCreation(sa); } }; diff --git a/src/xrpld/app/tx/detail/CashCheck.cpp b/src/xrpld/app/tx/detail/CashCheck.cpp index a62dd51a63..4010aa0714 100644 --- a/src/xrpld/app/tx/detail/CashCheck.cpp +++ b/src/xrpld/app/tx/detail/CashCheck.cpp @@ -156,17 +156,6 @@ CashCheck::preclaim(PreclaimContext const& ctx) // An issuer can always accept their own currency. if (!value.native() && (value.getIssuer() != dstId)) { - auto const sleTrustLine = - ctx.view.read(keylet::line(dstId, issuerId, currency)); - - if (!sleTrustLine && - !ctx.view.rules().enabled(featureCheckCashMakesTrustLine)) - { - JLOG(ctx.j.warn()) - << "Cannot cash check for IOU without trustline."; - return tecNO_LINE; - } - auto const sleIssuer = ctx.view.read(keylet::account(issuerId)); if (!sleIssuer) { @@ -178,6 +167,9 @@ CashCheck::preclaim(PreclaimContext const& ctx) if (sleIssuer->at(sfFlags) & lsfRequireAuth) { + auto const sleTrustLine = + ctx.view.read(keylet::line(dstId, issuerId, currency)); + if (!sleTrustLine) { // We can only create a trust line if the issuer does not @@ -325,10 +317,7 @@ CashCheck::doApply() Keylet const trustLineKey = keylet::line(truster, trustLineIssue); bool const destLow = issuer > account_; - bool const checkCashMakesTrustLine = - psb.rules().enabled(featureCheckCashMakesTrustLine); - - if (checkCashMakesTrustLine && !psb.exists(trustLineKey)) + if (!psb.exists(trustLineKey)) { // 1. Can the check casher meet the reserve for the trust line? // 2. Create trust line between destination (this) account @@ -401,14 +390,11 @@ CashCheck::doApply() sleTrustLine->at(tweakedLimit) = savedLimit; }); - if (checkCashMakesTrustLine) - { - // Set the trust line limit to the highest possible value - // while flow runs. - STAmount const bigAmount( - trustLineIssue, STAmount::cMaxValue, STAmount::cMaxOffset); - sleTrustLine->at(tweakedLimit) = bigAmount; - } + // Set the trust line limit to the highest possible value + // while flow runs. + STAmount const bigAmount( + trustLineIssue, STAmount::cMaxValue, STAmount::cMaxOffset); + sleTrustLine->at(tweakedLimit) = bigAmount; // Let flow() do the heavy lifting on a check for an IOU. auto const result = flow( @@ -441,15 +427,11 @@ CashCheck::doApply() << "flow did not produce DeliverMin."; return tecPATH_PARTIAL; } - if (!checkCashMakesTrustLine) - // Set the delivered_amount metadata. - ctx_.deliver(result.actualAmountOut); } // Set the delivered amount metadata in all cases, not just // for DeliverMin. - if (checkCashMakesTrustLine) - ctx_.deliver(result.actualAmountOut); + ctx_.deliver(result.actualAmountOut); sleCheck = psb.peek(keylet::check(ctx_.tx[sfCheckID])); }