APIv2(gateway_balances, channel_authorize): update errors (#4618)

gateway_balances
* When `account` does not exist in the ledger, return `actNotFound`
  * (Previously, a normal response was returned)
  * Fix #4290
* When required field(s) are missing, return `invalidParams`
  * (Previously, `invalidHotWallet` was incorrectly returned)
  * Fix #4548

channel_authorize
* When the specified `key_type` is invalid, return `badKeyType`
  * (Previously, `invalidParams` was returned)
  * Fix #4289

Since these are breaking changes, they apply only to API version 2.

Supersedes #4577

src/ripple/protocol/ErrorCodes.h

@@ -79,7 +79,7 @@ enum error_code_i {
     // unused                  27,
     // unused                  28,
     rpcTXN_NOT_FOUND = 29,
-    // unused                  30,
+    rpcINVALID_HOTWALLET = 30,
 
     // Malformed command
     rpcINVALID_PARAMS = 31,

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 hotwallet.", 400},
     {rpcISSUE_MALFORMED,        "issueMalformed",       "Issue is malformed.", 400},
     {rpcJSON_RPC,               "json_rpc",             "JSON-RPC transport error.", 500},
     {rpcLGR_IDXS_INVALID,       "lgrIdxsInvalid",       "Ledger indexes invalid.", 400},

src/ripple/rpc/handlers/GatewayBalances.cpp

@@ -78,6 +78,12 @@ doGatewayBalances(RPC::JsonContext& context)
 
     result[jss::account] = toBase58(accountID);
 
+    if (context.apiVersion > 1u && !ledger->exists(keylet::account(accountID)))
+    {
+        RPC::inject_error(rpcACT_NOT_FOUND, result);
+        return result;
+    }
+
     // Parse the specified hotwallet(s), if any
     std::set<AccountID> hotWallets;
 
@@ -116,7 +122,18 @@ doGatewayBalances(RPC::JsonContext& context)
 
         if (!valid)
         {
-            result[jss::error] = "invalidHotWallet";
+            // The documentation states that invalidParams is used when
+            // One or more fields are specified incorrectly.
+            // invalidHotwallet should be used when the account exists, but does
+            // not have currency issued by the account from the request.
+            if (context.apiVersion < 2u)
+            {
+                RPC::inject_error(rpcINVALID_HOTWALLET, result);
+            }
+            else
+            {
+                RPC::inject_error(rpcINVALID_PARAMS, result);
+            }
             return result;
         }
     }

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);
+    auto const [pk, sk] =
+        RPC::keypairForSignature(params, result, context.apiVersion);
     if (RPC::contains_error(result))
         return result;
 

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -972,7 +972,10 @@ getSeedFromRPC(Json::Value const& params, Json::Value& error)
 }
 
 std::pair<PublicKey, SecretKey>
-keypairForSignature(Json::Value const& params, Json::Value& error)
+keypairForSignature(
+    Json::Value const& params,
+    Json::Value& error,
+    unsigned int apiVersion)
 {
     bool const has_key_type = params.isMember(jss::key_type);
 
@@ -1026,7 +1029,10 @@ keypairForSignature(Json::Value const& params, Json::Value& error)
 
         if (!keyType)
         {
-            error = RPC::invalid_field_error(jss::key_type);
+            if (apiVersion > 1u)
+                error = RPC::make_error(rpcBAD_KEY_TYPE);
+            else
+                error = RPC::invalid_field_error(jss::key_type);
             return {};
         }
 

src/ripple/rpc/impl/RPCHelpers.h

@@ -225,9 +225,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);
-
 /**
  * API version numbers used in API version 1
  */
@@ -312,6 +309,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
 

src/test/app/PayChan_test.cpp

@@ -24,9 +24,9 @@
 #include <ripple/protocol/PayChan.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
-#include <test/jtx.h>
-
+#include <ripple/rpc/impl/RPCHelpers.h>
 #include <chrono>
+#include <test/jtx.h>
 
 namespace ripple {
 namespace test {
@@ -1144,6 +1144,47 @@ struct PayChan_test : public beast::unit_test::suite
             bob.human());
     }
 
+    void
+    testAccountChannelAuthorize(FeatureBitset features)
+    {
+        using namespace jtx;
+        using namespace std::literals::chrono_literals;
+
+        Env env{*this, features};
+        auto const alice = Account("alice");
+        auto const bob = Account("bob");
+        auto const charlie = Account("charlie", KeyType::ed25519);
+        env.fund(XRP(10000), alice, bob, charlie);
+        auto const pk = alice.pk();
+        auto const settleDelay = 3600s;
+        auto const channelFunds = XRP(1000);
+        auto const chan1Str = to_string(channel(alice, bob, env.seq(alice)));
+        env(create(alice, bob, channelFunds, settleDelay, pk));
+        env.close();
+
+        Json::Value args{Json::objectValue};
+        args[jss::channel_id] = chan1Str;
+        args[jss::key_type] = "ed255191";
+        args[jss::seed] = "snHq1rzQoN2qiUkC3XF5RyxBzUtN";
+        args[jss::amount] = 51110000;
+
+        // 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];
+            auto const error = apiVersion < 2u ? "invalidParams" : "badKeyType";
+            BEAST_EXPECT(rs[jss::error] == error);
+        }
+    }
+
     void
     testAuthVerifyRPC(FeatureBitset features)
     {
@@ -5509,6 +5550,7 @@ struct PayChan_test : public beast::unit_test::suite
         testAccountChannelsRPC(features);
         testAccountChannelsRPCMarkers(features);
         testAccountChannelsRPCSenderOnly(features);
+        testAccountChannelAuthorize(features);
         testAuthVerifyRPC(features);
         testOptionalFields(features);
         testMalformedPK(features);

src/test/rpc/GatewayBalances_test.cpp

@@ -18,6 +18,7 @@
 #include <ripple/beast/unit_test.h>
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/jss.h>
+#include <ripple/rpc/impl/RPCHelpers.h>
 #include <test/jtx.h>
 #include <test/jtx/WSClient.h>
 
@@ -34,117 +35,155 @@ public:
         using namespace jtx;
         Env env(*this, features);
 
+        {
+            // Gateway account and assets
+            Account const alice{"alice"};
+            env.fund(XRP(10000), "alice");
+            auto USD = alice["USD"];
+            auto CNY = alice["CNY"];
+            auto JPY = alice["JPY"];
+
+            // Create a hotwallet
+            Account const hw{"hw"};
+            env.fund(XRP(10000), "hw");
+            env(trust(hw, USD(10000)));
+            env(trust(hw, JPY(10000)));
+            env(pay(alice, hw, USD(5000)));
+            env(pay(alice, hw, JPY(5000)));
+
+            // Create some clients
+            Account const bob{"bob"};
+            env.fund(XRP(10000), "bob");
+            env(trust(bob, USD(100)));
+            env(trust(bob, CNY(100)));
+            env(pay(alice, bob, USD(50)));
+
+            Account const charley{"charley"};
+            env.fund(XRP(10000), "charley");
+            env(trust(charley, CNY(500)));
+            env(trust(charley, JPY(500)));
+            env(pay(alice, charley, CNY(250)));
+            env(pay(alice, charley, JPY(250)));
+
+            Account const dave{"dave"};
+            env.fund(XRP(10000), "dave");
+            env(trust(dave, CNY(100)));
+            env(pay(alice, dave, CNY(30)));
+
+            // give the gateway an asset
+            env(trust(alice, charley["USD"](50)));
+            env(pay(charley, alice, USD(10)));
+
+            // freeze dave
+            env(trust(alice, dave["CNY"](0), dave, tfSetFreeze));
+
+            env.close();
+
+            auto wsc = makeWSClient(env.app().config());
+
+            Json::Value qry;
+            qry[jss::account] = alice.human();
+            qry[jss::hotwallet] = hw.human();
+
+            auto jv = wsc->invoke("gateway_balances", qry);
+            expect(jv[jss::status] == "success");
+            if (wsc->version() == 2)
+            {
+                expect(jv.isMember(jss::jsonrpc) && jv[jss::jsonrpc] == "2.0");
+                expect(
+                    jv.isMember(jss::ripplerpc) && jv[jss::ripplerpc] == "2.0");
+                expect(jv.isMember(jss::id) && jv[jss::id] == 5);
+            }
+
+            auto const& result = jv[jss::result];
+            expect(result[jss::account] == alice.human());
+            expect(result[jss::status] == "success");
+
+            {
+                auto const& balances = result[jss::balances];
+                expect(balances.isObject(), "balances is not an object");
+                expect(balances.size() == 1, "balances size is not 1");
+
+                auto const& hwBalance = balances[hw.human()];
+                expect(hwBalance.isArray(), "hwBalance is not an array");
+                expect(hwBalance.size() == 2);
+                auto c1 = hwBalance[0u][jss::currency];
+                auto c2 = hwBalance[1u][jss::currency];
+                expect(c1 == "USD" || c2 == "USD");
+                expect(c1 == "JPY" || c2 == "JPY");
+                expect(
+                    hwBalance[0u][jss::value] == "5000" &&
+                    hwBalance[1u][jss::value] == "5000");
+            }
+
+            {
+                auto const& fBalances = result[jss::frozen_balances];
+                expect(fBalances.isObject());
+                expect(fBalances.size() == 1);
+
+                auto const& fBal = fBalances[dave.human()];
+                expect(fBal.isArray());
+                expect(fBal.size() == 1);
+                expect(fBal[0u].isObject());
+                expect(fBal[0u][jss::currency] == "CNY");
+                expect(fBal[0u][jss::value] == "30");
+            }
+
+            {
+                auto const& assets = result[jss::assets];
+                expect(assets.isObject(), "assets it not an object");
+                expect(assets.size() == 1, "assets size is not 1");
+
+                auto const& cAssets = assets[charley.human()];
+                expect(cAssets.isArray());
+                expect(cAssets.size() == 1);
+                expect(cAssets[0u][jss::currency] == "USD");
+                expect(cAssets[0u][jss::value] == "10");
+            }
+
+            {
+                auto const& obligations = result[jss::obligations];
+                expect(obligations.isObject(), "obligations is not an object");
+                expect(obligations.size() == 3);
+                expect(obligations["CNY"] == "250");
+                expect(obligations["JPY"] == "250");
+                expect(obligations["USD"] == "50");
+            }
+        }
+    }
+
+    void
+    testGWBApiVersions(FeatureBitset features)
+    {
+        using namespace std::chrono_literals;
+        using namespace jtx;
+        Env env(*this, features);
+
         // Gateway account and assets
         Account const alice{"alice"};
-        env.fund(XRP(10000), "alice");
-        auto USD = alice["USD"];
-        auto CNY = alice["CNY"];
-        auto JPY = alice["JPY"];
-
-        // Create a hotwallet
+        env.fund(XRP(10000), alice);
         Account const hw{"hw"};
-        env.fund(XRP(10000), "hw");
-        env(trust(hw, USD(10000)));
-        env(trust(hw, JPY(10000)));
-        env(pay(alice, hw, USD(5000)));
-        env(pay(alice, hw, JPY(5000)));
-
-        // Create some clients
-        Account const bob{"bob"};
-        env.fund(XRP(10000), "bob");
-        env(trust(bob, USD(100)));
-        env(trust(bob, CNY(100)));
-        env(pay(alice, bob, USD(50)));
-
-        Account const charley{"charley"};
-        env.fund(XRP(10000), "charley");
-        env(trust(charley, CNY(500)));
-        env(trust(charley, JPY(500)));
-        env(pay(alice, charley, CNY(250)));
-        env(pay(alice, charley, JPY(250)));
-
-        Account const dave{"dave"};
-        env.fund(XRP(10000), "dave");
-        env(trust(dave, CNY(100)));
-        env(pay(alice, dave, CNY(30)));
-
-        // give the gateway an asset
-        env(trust(alice, charley["USD"](50)));
-        env(pay(charley, alice, USD(10)));
-
-        // freeze dave
-        env(trust(alice, dave["CNY"](0), dave, tfSetFreeze));
-
+        env.fund(XRP(10000), hw);
         env.close();
 
         auto wsc = makeWSClient(env.app().config());
 
-        Json::Value qry;
-        qry[jss::account] = alice.human();
-        qry[jss::hotwallet] = hw.human();
+        Json::Value qry2;
+        qry2[jss::account] = alice.human();
+        qry2[jss::hotwallet] = "asdf";
 
-        auto jv = wsc->invoke("gateway_balances", qry);
-        expect(jv[jss::status] == "success");
-        if (wsc->version() == 2)
+        for (auto apiVersion = RPC::apiMinimumSupportedVersion;
+             apiVersion <= RPC::apiBetaVersion;
+             ++apiVersion)
         {
-            expect(jv.isMember(jss::jsonrpc) && jv[jss::jsonrpc] == "2.0");
-            expect(jv.isMember(jss::ripplerpc) && jv[jss::ripplerpc] == "2.0");
-            expect(jv.isMember(jss::id) && jv[jss::id] == 5);
-        }
+            qry2[jss::api_version] = apiVersion;
+            auto jv = wsc->invoke("gateway_balances", qry2);
+            expect(jv[jss::status] == "error");
 
-        auto const& result = jv[jss::result];
-        expect(result[jss::account] == alice.human());
-        expect(result[jss::status] == "success");
-
-        {
-            auto const& balances = result[jss::balances];
-            expect(balances.isObject(), "balances is not an object");
-            expect(balances.size() == 1, "balances size is not 1");
-
-            auto const& hwBalance = balances[hw.human()];
-            expect(hwBalance.isArray(), "hwBalance is not an array");
-            expect(hwBalance.size() == 2);
-            auto c1 = hwBalance[0u][jss::currency];
-            auto c2 = hwBalance[1u][jss::currency];
-            expect(c1 == "USD" || c2 == "USD");
-            expect(c1 == "JPY" || c2 == "JPY");
-            expect(
-                hwBalance[0u][jss::value] == "5000" &&
-                hwBalance[1u][jss::value] == "5000");
-        }
-
-        {
-            auto const& fBalances = result[jss::frozen_balances];
-            expect(fBalances.isObject());
-            expect(fBalances.size() == 1);
-
-            auto const& fBal = fBalances[dave.human()];
-            expect(fBal.isArray());
-            expect(fBal.size() == 1);
-            expect(fBal[0u].isObject());
-            expect(fBal[0u][jss::currency] == "CNY");
-            expect(fBal[0u][jss::value] == "30");
-        }
-
-        {
-            auto const& assets = result[jss::assets];
-            expect(assets.isObject(), "assets it not an object");
-            expect(assets.size() == 1, "assets size is not 1");
-
-            auto const& cAssets = assets[charley.human()];
-            expect(cAssets.isArray());
-            expect(cAssets.size() == 1);
-            expect(cAssets[0u][jss::currency] == "USD");
-            expect(cAssets[0u][jss::value] == "10");
-        }
-
-        {
-            auto const& obligations = result[jss::obligations];
-            expect(obligations.isObject(), "obligations is not an object");
-            expect(obligations.size() == 3);
-            expect(obligations["CNY"] == "250");
-            expect(obligations["JPY"] == "250");
-            expect(obligations["USD"] == "50");
+            auto response = jv[jss::result];
+            auto const error =
+                apiVersion < 2u ? "invalidHotWallet" : "invalidParams";
+            BEAST_EXPECT(response[jss::error] == error);
         }
     }
 
@@ -207,8 +246,11 @@ public:
     {
         using namespace jtx;
         auto const sa = supported_amendments();
-        testGWB(sa - featureFlowCross);
-        testGWB(sa);
+        for (auto feature : {sa - featureFlowCross, sa})
+        {
+            testGWB(feature);
+            testGWBApiVersions(feature);
+        }
 
         testGWBOverflow();
     }