Fix unit test api_version to enable api_version 2 #4785

pwang200 · 2023-10-25T04:19:04Z

This PR attempts to address the unit test api_version issues discussed in a previous PR #4568

the command line API still uses apiMaximumSupportedVersion.
the unit test RPCs will uses apiMinimumSupportedVersion if unspecified.

arihantkothari · 2023-10-25T15:12:36Z

src/test/rpc/RPCCall_test.cpp

@@ -6403,7 +6403,7 @@ std::string
 updateAPIVersionString(const char* const req)
 {
    static std::string version_str =
-        std::to_string(RPC::apiMaximumSupportedVersion);
+        std::to_string(RPC::apiMinimumSupportedVersion);


Should minimum version be default or apiVersionIfUnspecified ? Both of them = 1 right now, so it doesn't really matter. But I am just curious.

I know most of the times (maybe all the times), apiVersionIfUnspecified = apiMinimumSupportedVersion will be true.

I cannot say no. I.e., probably make sense to use apiVersionIfUnspecified as default.

arihantkothari · 2023-10-25T15:13:43Z

src/test/rpc/RPCCall_test.cpp

@@ -6403,7 +6403,7 @@ std::string
 updateAPIVersionString(const char* const req)
 {
    static std::string version_str =
-        std::to_string(RPC::apiMaximumSupportedVersion);
+        std::to_string(RPC::apiMinimumSupportedVersion);
    static auto place_holder = "%MAX_API_VER%";


If we are putting minimum version as version_str. Should this line instead be static auto place_holder = "%MIN_API_VER%"; ? [line 6407]

I changed it back to RPC::apiMaximumSupportedVersion and we will see what we eventually decide to use as default. Could be apiVersionIfUnspecified as you said earlier.

src/ripple/net/RPCCall.h

Bronek · 2023-10-25T15:35:38Z

src/ripple/net/impl/RPCCall.cpp

@@ -1561,7 +1563,8 @@ rpcClient(
    try
    {
        Json::Value jvRpc = Json::Value(Json::objectValue);
-        jvRequest = rpcCmdLineToJson(args, jvRpc, logs.journal("RPCParser"));
+        auto apiVersion = fromCmdLine? RPC::apiMaximumSupportedVersion : RPC::apiMinimumSupportedVersion;


I would rather not have this logic hardcoded in this location; it would be easier to understand if rpcClient took explicitly unsigned apiVersion parameter instead, and then fromCommandLine passes RPC::apiMaximumSupportedVersion. That maintains isolation of concerns and also enables unit tests to test different versions (by which, I mean more than two) of command line generated parameters.

Also, if you go this way, I would add assert(apiVersion <= RPC::apiBetaVersion);

Thanks, changed.

Bronek · 2023-10-25T15:36:20Z

src/ripple/net/impl/RPCCall.cpp

@@ -1698,7 +1701,7 @@ fromCommandLine(
    const std::vector<std::string>& vCmd,
    Logs& logs)
 {
-    auto const result = rpcClient(vCmd, config, logs);
+    auto const result = rpcClient(vCmd, config, logs, true);


as mentioned above, instead of true pass RPC::apiMaximumSupportedVersion here.

ximinez · 2023-10-25T17:14:05Z

IMHO, this solution is backwards. When a new API version becomes officially supported, we want incompatible unit tests to break. That is a signal that they are version dependent and need to be updated to test all API versions (preferably), or a least updated to the latest API version.

Bronek · 2023-10-26T10:15:44Z

IMHO, this solution is backwards. When a new API version becomes officially supported, we want incompatible unit tests to break. That is a signal that they are version dependent and need to be updated to test all API versions (preferably), or a least updated to the latest API version.

Agree. Exposing apiVersion parameter, where appropriate, to unit tests, would make that easier.

ximinez · 2023-10-26T15:17:34Z

Agree. Exposing apiVersion parameter, where appropriate, to unit tests, would make that easier.

There's a helper function, forAllApiVersions for just this purpose.

pwang200 · 2023-10-26T21:22:03Z

I kind of agree with what @ximinez said about "this solution is backwards". That could be the reason we had apiMaximumSupportedVersion as default. I changed the default back to apiMaximumSupportedVersion. (shows my (lack of) understanding of the issue.)

I addressed @Bronek comments. Now the apiVersion is unit tests' choice, instead of having to use what the command line use.

A bigger discussion is what should be used as unit test default. What @arihantkothari said (apiVersionIfUnspecified) also make sense in some cases.

intelliot · 2023-10-31T03:36:27Z

@pwang200 - from your perspective, is this PR ready for review + merge?

src/ripple/net/RPCCall.h

Bronek · 2023-10-31T14:22:19Z

I addressed @Bronek comments. Now the apiVersion is unit tests' choice, instead of having to use what the command line use.

Now we can start using forAllApiVersions in unit tests for command line tests, as suggested by @ximinez, because with this change we have the ability to pass the API version down to rpcClient function.

Bronek

Nice, that's an overall improvement.

Bronek · 2023-10-31T12:15:24Z

src/ripple/net/RPCCall.h


 /** Internal invocation of RPC client.
+ *  Used by both rippled command line as well as rippled unit tests


I like this comment ! 👍

Bronek · 2023-10-31T14:45:17Z

src/ripple/net/impl/RPCCall.cpp

@@ -1698,7 +1672,8 @@ fromCommandLine(
    const std::vector<std::string>& vCmd,
    Logs& logs)
 {
-    auto const result = rpcClient(vCmd, config, logs);
+    auto const result =
+        rpcClient(vCmd, config, logs, RPC::apiMaximumSupportedVersion);


Note, this means that as soon as we bump apiMaximumSupportedVersion, the tx_history and ledger_header commands will stop working (as they are no longer recognized in API version 2). Ideally at this point we should remove them from parseCommand in this file (and also remove function RPCParser::parseTxHistory). This probably needs its own issue.

pwang200 · 2023-10-31T17:49:11Z

@pwang200 - from your perspective, is this PR ready for review + merge?

yes

seelabs

👍 This looks good. Approving as-is, but left two nits to consider. (I'm fine if the nits are not addressed).

seelabs · 2023-10-31T18:54:01Z

src/ripple/net/RPCCall.h

+rpcCmdToJson(
+    std::vector<std::string> const& args,
+    Json::Value& retParams,
+    beast::Journal j,


Usually we make journal the last parameter.

seelabs · 2023-10-31T18:55:05Z

src/ripple/net/RPCCall.h

+    std::vector<std::string> const& args,
+    Json::Value& retParams,
+    beast::Journal j,
+    unsigned int apiVersion);


Maybe not for this PR, but we should consider making apiVersion either an enum type with one enum per API version or maybe a strongly-typed int (int with a tag, like we do with base_uint).

arihantkothari

lgtm 👍

intelliot · 2023-10-31T21:59:52Z

re-requested review since a new commit was pushed after the last review.

Also, should we try to merge #4775 prior to this one?

Bronek · 2023-11-02T11:44:58Z

Also, should we try to merge #4775 prior to this one?

There's no need to delay this PR

The command line API still uses `apiMaximumSupportedVersion`. The unit test RPCs use `apiMinimumSupportedVersion` if unspecified. Context: - XRPLF#4568 - XRPLF#4552

Fix unit test api_version to enable api_version 2

737a89f

intelliot requested review from Bronek and arihantkothari October 25, 2023 05:33

arihantkothari mentioned this pull request Oct 25, 2023

Enable api_version 2 #4568

Closed

arihantkothari reviewed Oct 25, 2023

View reviewed changes

intelliot added this to the 2.0 milestone Oct 25, 2023

Bronek requested changes Oct 25, 2023

View reviewed changes

intelliot assigned pwang200 Oct 25, 2023

move unit test specific to src/test/jtx, default apiVersion to max

c15fd32

intelliot requested review from Bronek and arihantkothari October 27, 2023 05:41

intelliot assigned Bronek and arihantkothari Oct 27, 2023

intelliot reviewed Oct 31, 2023

View reviewed changes

src/ripple/net/RPCCall.h Outdated Show resolved Hide resolved

clang-format

2d3d91c

intelliot requested a review from seelabs October 31, 2023 03:38

Bronek approved these changes Oct 31, 2023

View reviewed changes

Bronek reviewed Oct 31, 2023

View reviewed changes

intelliot added API Change API V2 Testing labels Oct 31, 2023

kennyzlei added the Clio Reviewed label Oct 31, 2023

seelabs approved these changes Oct 31, 2023

View reviewed changes

arihantkothari approved these changes Oct 31, 2023

View reviewed changes

make journal the last parameter

e1d1c27

intelliot requested a review from Bronek October 31, 2023 21:59

Bronek approved these changes Nov 1, 2023

View reviewed changes

Merge branch 'develop' into api_version-fix

3e9b260

intelliot merged commit 056255e into XRPLF:develop Nov 2, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unit test api_version to enable api_version 2 #4785

Fix unit test api_version to enable api_version 2 #4785

pwang200 commented Oct 25, 2023

arihantkothari Oct 25, 2023

arihantkothari Oct 25, 2023 •

edited

Loading

pwang200 Oct 26, 2023

arihantkothari Oct 25, 2023 •

edited

Loading

pwang200 Oct 26, 2023

Bronek Oct 25, 2023

Bronek Oct 25, 2023

pwang200 Oct 26, 2023

Bronek Oct 25, 2023

pwang200 Oct 26, 2023

ximinez commented Oct 25, 2023

Bronek commented Oct 26, 2023

ximinez commented Oct 26, 2023

pwang200 commented Oct 26, 2023 •

edited

Loading

intelliot commented Oct 31, 2023

Bronek commented Oct 31, 2023

Bronek left a comment

Bronek Oct 31, 2023

Bronek Oct 31, 2023

pwang200 commented Oct 31, 2023

seelabs left a comment

seelabs Oct 31, 2023

seelabs Oct 31, 2023

arihantkothari left a comment

intelliot commented Oct 31, 2023

Bronek commented Nov 2, 2023


		/** Internal invocation of RPC client.
		* Used by both rippled command line as well as rippled unit tests

Fix unit test api_version to enable api_version 2 #4785

Fix unit test api_version to enable api_version 2 #4785

Conversation

pwang200 commented Oct 25, 2023

Choose a reason for hiding this comment

arihantkothari Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arihantkothari Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez commented Oct 25, 2023

Bronek commented Oct 26, 2023

ximinez commented Oct 26, 2023

pwang200 commented Oct 26, 2023 • edited Loading

intelliot commented Oct 31, 2023

Bronek commented Oct 31, 2023

Bronek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwang200 commented Oct 31, 2023

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arihantkothari left a comment

Choose a reason for hiding this comment

intelliot commented Oct 31, 2023

Bronek commented Nov 2, 2023

arihantkothari Oct 25, 2023 •

edited

Loading

arihantkothari Oct 25, 2023 •

edited

Loading

pwang200 commented Oct 26, 2023 •

edited

Loading