Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promote API version 2 to supported #4803

Merged
merged 12 commits into from
Nov 13, 2023

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Nov 3, 2023

High Level Overview of Change

This PR version essentially bumps apiMaximumSupportedVersion to 2, whilst ensuring that the command line interface (both rippled and the one used in unit tests) stays at version 1. We plan to bump the command line interface to API version 2, but not in this PR (probably also not in the nearest release). We also bump apiBetaVersion to 3 and add apiCommandLineVersion, used by both command line parsing and these unit tests which rely on command line parsing.

Note, this change removes remnants of a non-functional method tx_account. The method has no implementation and no mapping in Handler.cpp, however until this PR it did have command line parsing (only used in unit tests and otherwise useless). This is now removed.

Large diff is result of the switch to forAllApiVersions in RPCall_test.cpp. This test was failing because of hardcoded dependency on apiMaximumSupportedVersion (which before this change was incidentally the same as apiMinimumSupportedVersion), expressed both as %MAX_API_VER% and as a conditional on expected output (near lines 1353 - 1369). Since change was needed to fix the failing test, and large diff would be unavoidable to remove the misleading %MAX_API_VER% name, I decided to do it whilst improving test design and coverage.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

@Bronek Bronek force-pushed the feature/promote_API_V2_to_supported branch 2 times, most recently from fd7e0db to 3801410 Compare November 4, 2023 14:50
@Bronek Bronek marked this pull request as ready for review November 4, 2023 20:27
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I do have some feedback.

src/ripple/net/impl/RPCCall.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/impl/RPCHelpers.h Outdated Show resolved Hide resolved
src/test/jtx/Env.h Outdated Show resolved Hide resolved
src/test/jtx/Env.h Outdated Show resolved Hide resolved
src/test/jtx/Env.h Outdated Show resolved Hide resolved
src/test/rpc/LedgerRequestRPC_test.cpp Outdated Show resolved Hide resolved
@Bronek Bronek force-pushed the feature/promote_API_V2_to_supported branch from 271ad5c to bdb123d Compare November 7, 2023 00:28
@Bronek Bronek requested a review from ximinez November 7, 2023 00:31
@Bronek Bronek force-pushed the feature/promote_API_V2_to_supported branch from bdb123d to 9865328 Compare November 8, 2023 20:36
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@@ -131,7 +131,16 @@ else ()
>)
endif ()

if (use_gold AND is_gcc)
if (use_mold)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these changes required to allow version 2?

@manojsdoshi manojsdoshi merged commit ac27089 into XRPLF:develop Nov 13, 2023
16 checks passed
manojsdoshi added a commit that referenced this pull request Nov 20, 2023
* Support for the mold linker (#4807)

* Promote API version 2 to supported (#4803)

* Promote API version 2 to be supported

* Switch the command line to API version 1

* Fix LedgerRequestRPC test

* Remove obsolete tx_account method

This method is not implemented, the only parts which are removed are related to command-line parsing

* Fix RPCCall test

* Reduce diff size, small test improvements

* Minor fixes

* Support for the mold linker

* Fix TransactionEntry_test

* Fix AccountTx_test

---------

Co-authored-by: seelabs <[email protected]>

* Update Linux smoketest distros (#4813)

* Fix 2.0 regression in tx method with binary output (#4812)

* Fix binary output from tx method

* Formatting fix

* Minor test improvement

* Minor test improvements

* Optimize calculation of close time to avoid impasse and minimize gratuitous proposal changes (#4760)

* Optimize the calculation of close time to avoid
impasse and minimize gratuitous proposal changes.

* git apply clang-format.patch

* Scott S review fixes. Also clang-format.

* Set version to 2.0.0-rc2

---------

Co-authored-by: manoj <[email protected]>
Co-authored-by: Scott Determan <[email protected]>
Co-authored-by: Bronek Kozicki <[email protected]>
Co-authored-by: Michael Legleux <[email protected]>
Co-authored-by: Mark Travis <[email protected]>
ximinez pushed a commit to ximinez/rippled that referenced this pull request Nov 21, 2023
* Promote API version 2 to supported

* Switch command line to API version 1

* Fix LedgerRequestRPC test

* Remove obsolete tx_account method

This method is not implemented, the only parts which are removed are related to command-line parsing

* Fix RPCCall test

* Reduce diff size, small test improvements

* Minor fixes

* Support for the mold linker

* [fold] handle case where both mold and gold are installed

* [fold] Use first non-default linker

* Fix TransactionEntry_test

* Fix AccountTx_test

---------

Co-authored-by: seelabs <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Nov 21, 2023
* Promote API version 2 to supported

* Switch command line to API version 1

* Fix LedgerRequestRPC test

* Remove obsolete tx_account method

This method is not implemented, the only parts which are removed are related to command-line parsing

* Fix RPCCall test

* Reduce diff size, small test improvements

* Minor fixes

* Support for the mold linker

* [fold] handle case where both mold and gold are installed

* [fold] Use first non-default linker

* Fix TransactionEntry_test

* Fix AccountTx_test

---------

Co-authored-by: seelabs <[email protected]>
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
* Promote API version 2 to supported

* Switch command line to API version 1

* Fix LedgerRequestRPC test

* Remove obsolete tx_account method

This method is not implemented, the only parts which are removed are related to command-line parsing

* Fix RPCCall test

* Reduce diff size, small test improvements

* Minor fixes

* Support for the mold linker

* [fold] handle case where both mold and gold are installed

* [fold] Use first non-default linker

* Fix TransactionEntry_test

* Fix AccountTx_test

---------

Co-authored-by: seelabs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants