fixes based on Greg's comments

XRPLF · Jul 10, 2023 · 21c5ebd · gregtatcam · Jul 10, 2023 · PeterChen13579
1 parent 920cd81
commit 21c5ebd
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 66 deletions.
diff --git a/src/ripple/protocol/ErrorCodes.h b/src/ripple/protocol/ErrorCodes.h
@@ -78,7 +78,7 @@ enum error_code_i {
     // unused                  27,
     // unused                  28,
     rpcTXN_NOT_FOUND = 29,
-    // unused                  30,
+    rpcINVALID_HOTWALLET = 30,
 
     // Malformed command
     rpcINVALID_PARAMS = 31,

diff --git a/src/ripple/protocol/impl/ErrorCodes.cpp b/src/ripple/protocol/impl/ErrorCodes.cpp
@@ -76,6 +76,7 @@ constexpr static ErrorInfo unorderedErrorInfos[]{
     {rpcINTERNAL,               "internal",             "Internal error.", 500},
     {rpcINVALID_LGR_RANGE,      "invalidLgrRange",      "Ledger range is invalid.", 400},
     {rpcINVALID_PARAMS,         "invalidParams",        "Invalid parameters.", 400},
+    {rpcINVALID_HOTWALLET,      "invalidHotWallet",     "Invalid hotwalloet.", 400},
     {rpcJSON_RPC,               "json_rpc",             "JSON-RPC transport error.", 500},
     {rpcLGR_IDXS_INVALID,       "lgrIdxsInvalid",       "Ledger indexes invalid.", 400},
     {rpcLGR_IDX_MALFORMED,      "lgrIdxMalformed",      "Ledger index malformed.", 400},

diff --git a/src/ripple/rpc/handlers/GatewayBalances.cpp b/src/ripple/rpc/handlers/GatewayBalances.cpp
@@ -128,11 +128,11 @@ doGatewayBalances(RPC::JsonContext& context)
             // not have currency issued by the account from the request.
             if (context.apiVersion < 2u)
             {
-                result[jss::error] = "invalidHotWallet";
+                RPC::inject_error(rpcINVALID_HOTWALLET, result);
             }
             else
             {
-                result[jss::error] = "invalidParams";
+                RPC::inject_error(rpcINVALID_PARAMS, result);
             }
             return result;
         }

diff --git a/src/ripple/rpc/handlers/PayChanClaim.cpp b/src/ripple/rpc/handlers/PayChanClaim.cpp
@@ -55,7 +55,8 @@ doChannelAuthorize(RPC::JsonContext& context)
         return RPC::missing_field_error(jss::secret);
 
     Json::Value result;
-    auto const [pk, sk] = RPC::keypairForSignature(params, result, context);
+    auto const [pk, sk] =
+        RPC::keypairForSignature(params, result, context.apiVersion);
     if (RPC::contains_error(result))
         return result;
 

diff --git a/src/ripple/rpc/impl/RPCHelpers.cpp b/src/ripple/rpc/impl/RPCHelpers.cpp
@@ -849,7 +849,7 @@ std::pair<PublicKey, SecretKey>
 keypairForSignature(
     Json::Value const& params,
     Json::Value& error,
-    std::optional<std::reference_wrapper<JsonContext>> context)
+    uint apiVersion)
 {
     bool const has_key_type = params.isMember(jss::key_type);
 
@@ -903,7 +903,7 @@ keypairForSignature(
 
         if (!keyType)
         {
-            if (context.has_value() && context.value().get().apiVersion > 1u)
+            if (apiVersion > 1u)
             {
                 error = RPC::make_error(rpcBAD_KEY_TYPE);
             }

diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h
@@ -206,12 +206,6 @@ getSeedFromRPC(Json::Value const& params, Json::Value& error);
 std::optional<Seed>
 parseRippleLibSeed(Json::Value const& params);
 
-std::pair<PublicKey, SecretKey>
-keypairForSignature(
-    Json::Value const& params,
-    Json::Value& error,
-    std::optional<std::reference_wrapper<JsonContext>> context = std::nullopt);
-
 /**
  * API version numbers used in API version 1
  */
@@ -296,6 +290,12 @@ getAPIVersionNumber(const Json::Value& value, bool betaEnabled);
 std::variant<std::shared_ptr<Ledger const>, Json::Value>
 getLedgerByContext(RPC::JsonContext& context);
 
+std::pair<PublicKey, SecretKey>
+keypairForSignature(
+    Json::Value const& params,
+    Json::Value& error,
+    uint apiVersion = apiVersionIfUnspecified);
+
 }  // namespace RPC
 }  // namespace ripple
 

diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp
@@ -1161,9 +1161,8 @@ struct PayChan_test : public beast::unit_test::suite
     }
 
     void
-    testAccountChannelAuthorize(FeatureBitset features, unsigned int apiVersion)
+    testAccountChannelAuthorize(FeatureBitset features)
     {
-        testcase("PayChan Channel_Auth RPC Api " + std::to_string(apiVersion));
         using namespace jtx;
         using namespace std::literals::chrono_literals;
 
@@ -1179,28 +1178,26 @@ struct PayChan_test : public beast::unit_test::suite
         env(create(alice, bob, channelFunds, settleDelay, pk));
         env.close();
 
-        // test for api_version 2
         Json::Value args{Json::objectValue};
-        args[jss::api_version] = apiVersion;
         args[jss::channel_id] = chan1Str;
         args[jss::key_type] = "ed255191";
         args[jss::seed] = "snHq1rzQoN2qiUkC3XF5RyxBzUtN";
         args[jss::amount] = 51110000;
-        if (apiVersion < 2u)
-        {
-            auto const rs = env.rpc(
-                "json",
-                "channel_authorize",
-                args.toStyledString())[jss::result];
-            BEAST_EXPECT(rs[jss::error] == "invalidParams");
-        }
-        else
+
+        // test for all api versions
+        for (auto apiVersion = RPC::apiMinimumSupportedVersion;
+             apiVersion <= RPC::apiBetaVersion;
+             ++apiVersion)
         {
+            testcase(
+                "PayChan Channel_Auth RPC Api " + std::to_string(apiVersion));
+            args[jss::api_version] = apiVersion;
             auto const rs = env.rpc(
                 "json",
                 "channel_authorize",
                 args.toStyledString())[jss::result];
-            BEAST_EXPECT(rs[jss::error] == "badKeyType");
+            auto const error = apiVersion < 2u ? "invalidParams" : "badKeyType";
+            BEAST_EXPECT(rs[jss::error] == error);
         }
     }
 
@@ -2183,12 +2180,7 @@ struct PayChan_test : public beast::unit_test::suite
         testAccountChannelsRPC(features);
         testAccountChannelsRPCMarkers(features);
         testAccountChannelsRPCSenderOnly(features);
-        for (auto testVersion = RPC::apiMinimumSupportedVersion;
-             testVersion <= RPC::apiBetaVersion;
-             ++testVersion)
-        {
-            testAccountChannelAuthorize(features, testVersion);
-        }
+        testAccountChannelAuthorize(features);
         testAuthVerifyRPC(features);
         testOptionalFields(features);
         testMalformedPK(features);

diff --git a/src/test/rpc/GatewayBalances_test.cpp b/src/test/rpc/GatewayBalances_test.cpp
@@ -29,13 +29,12 @@ class GatewayBalances_test : public beast::unit_test::suite
 {
 public:
     void
-    testGWB(FeatureBitset features, unsigned int apiVersion)
+    testGWB(FeatureBitset features)
     {
         using namespace std::chrono_literals;
         using namespace jtx;
         Env env(*this, features);
 
-        if (apiVersion < 2u)
         {
             // Gateway account and assets
             Account const alice{"alice"};
@@ -151,37 +150,39 @@ class GatewayBalances_test : public beast::unit_test::suite
                 expect(obligations["USD"] == "50");
             }
         }
-        else
-        {
-            // Gateway account and assets
-            Account const alice{"alice"};
-            env.fund(XRP(10000), alice);
-            Account const hw{"hw"};
-            env.fund(XRP(10000), hw);
-            env.close();
+    }
 
-            auto wsc = makeWSClient(env.app().config());
+    void
+    testGWBApiVersions(FeatureBitset features)
+    {
+        using namespace std::chrono_literals;
+        using namespace jtx;
+        Env env(*this, features);
 
-            Json::Value qry2;
-            qry2[jss::api_version] = apiVersion;
-            {
-                qry2[jss::account] = "rNGQLojaFxTYphuQUJ24QUhyGBUMMbqBMx";
-                qry2[jss::hotwallet] = hw.human();
-                auto jv = wsc->invoke("gateway_balances", qry2);
-                expect(jv[jss::status] == "error");
+        // Gateway account and assets
+        Account const alice{"alice"};
+        env.fund(XRP(10000), alice);
+        Account const hw{"hw"};
+        env.fund(XRP(10000), hw);
+        env.close();
 
-                auto response = jv[jss::result];
-                expect(response[jss::error] == "actNotFound");
-            }
-            {
-                qry2[jss::account] = alice.human();
-                qry2[jss::hotwallet] = "asdf";
-                auto jv = wsc->invoke("gateway_balances", qry2);
-                expect(jv[jss::status] == "error");
+        auto wsc = makeWSClient(env.app().config());
 
-                auto response = jv[jss::result];
-                expect(response[jss::error] == "invalidParams");
-            }
+        Json::Value qry2;
+        for (auto apiVersion = RPC::apiMinimumSupportedVersion;
+             apiVersion <= RPC::apiBetaVersion;
+             ++apiVersion)
+        {
+            qry2[jss::account] = alice.human();
+            qry2[jss::hotwallet] = "asdf";
+            qry2[jss::api_version] = apiVersion;
+            auto jv = wsc->invoke("gateway_balances", qry2);
+            expect(jv[jss::status] == "error");
+
+            auto response = jv[jss::result];
+            auto const error =
+                apiVersion < 2u ? "invalidHotWallet" : "invalidParams";
+            BEAST_EXPECT(response[jss::error] == error);
         }
     }
 
@@ -244,12 +245,10 @@ class GatewayBalances_test : public beast::unit_test::suite
     {
         using namespace jtx;
         auto const sa = supported_amendments();
-        for (auto testVersion = RPC::apiMinimumSupportedVersion;
-             testVersion <= RPC::apiBetaVersion;
-             ++testVersion)
+        for (auto feature : {sa - featureFlowCross, sa})
         {
-            testGWB(sa - featureFlowCross, testVersion);
-            testGWB(sa, testVersion);
+            testGWB(feature);
+            testGWBApiVersions(feature);
         }
 
         testGWBOverflow();