From d094c42324d5f4ed8bbad04e683c018f59b4b1b7 Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Thu, 21 Sep 2023 16:33:24 -0400 Subject: [PATCH] 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 | 2 +- src/ripple/protocol/impl/ErrorCodes.cpp | 1 + src/ripple/rpc/handlers/GatewayBalances.cpp | 19 +- src/ripple/rpc/handlers/PayChanClaim.cpp | 3 +- src/ripple/rpc/impl/RPCHelpers.cpp | 10 +- src/ripple/rpc/impl/RPCHelpers.h | 8 +- src/test/app/PayChan_test.cpp | 46 +++- src/test/rpc/GatewayBalances_test.cpp | 250 ++++++++++++-------- 8 files changed, 225 insertions(+), 114 deletions(-) diff --git a/src/ripple/protocol/ErrorCodes.h b/src/ripple/protocol/ErrorCodes.h index 87323b0dea8..8319b69c8c2 100644 --- 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 index 603888827b2..319bd8e28c2 100644 --- 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 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}, diff --git a/src/ripple/rpc/handlers/GatewayBalances.cpp b/src/ripple/rpc/handlers/GatewayBalances.cpp index 77cec496ed0..fc6a7b49fd8 100644 --- a/src/ripple/rpc/handlers/GatewayBalances.cpp +++ b/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 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; } } diff --git a/src/ripple/rpc/handlers/PayChanClaim.cpp b/src/ripple/rpc/handlers/PayChanClaim.cpp index 6353124cb39..23a4041bb35 100644 --- 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); + 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 index 898755bd5ab..f082f8913ca 100644 --- a/src/ripple/rpc/impl/RPCHelpers.cpp +++ b/src/ripple/rpc/impl/RPCHelpers.cpp @@ -847,7 +847,10 @@ getSeedFromRPC(Json::Value const& params, Json::Value& error) } std::pair -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); @@ -901,7 +904,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 {}; } diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h index 5fa7ae804a2..096ab7849ba 100644 --- a/src/ripple/rpc/impl/RPCHelpers.h +++ b/src/ripple/rpc/impl/RPCHelpers.h @@ -208,9 +208,6 @@ getSeedFromRPC(Json::Value const& params, Json::Value& error); std::optional parseRippleLibSeed(Json::Value const& params); -std::pair -keypairForSignature(Json::Value const& params, Json::Value& error); - /** * API version numbers used in API version 1 */ @@ -295,6 +292,11 @@ getAPIVersionNumber(const Json::Value& value, bool betaEnabled); std::variant, Json::Value> getLedgerByContext(RPC::JsonContext& context); +std::pair +keypairForSignature( + Json::Value const& params, + Json::Value& error, + uint apiVersion = apiVersionIfUnspecified); /** Helper to parse submit_mode parameter to RPC submit. * * @param params RPC parameters diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index f8162dd107d..02a13de5c21 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -24,9 +24,9 @@ #include #include #include -#include - +#include #include +#include namespace ripple { namespace test { @@ -1063,6 +1063,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) { @@ -2042,6 +2083,7 @@ struct PayChan_test : public beast::unit_test::suite testAccountChannelsRPC(features); testAccountChannelsRPCMarkers(features); testAccountChannelsRPCSenderOnly(features); + testAccountChannelAuthorize(features); testAuthVerifyRPC(features); testOptionalFields(features); testMalformedPK(features); diff --git a/src/test/rpc/GatewayBalances_test.cpp b/src/test/rpc/GatewayBalances_test.cpp index c14ec0f043c..091b9f51686 100644 --- a/src/test/rpc/GatewayBalances_test.cpp +++ b/src/test/rpc/GatewayBalances_test.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -34,117 +35,155 @@ class GatewayBalances_test : public beast::unit_test::suite 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(); - - 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"); - } + Json::Value qry2; + qry2[jss::account] = alice.human(); + qry2[jss::hotwallet] = "asdf"; + for (auto apiVersion = RPC::apiMinimumSupportedVersion; + apiVersion <= RPC::apiBetaVersion; + ++apiVersion) { - 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"); + 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); } } @@ -207,8 +246,11 @@ class GatewayBalances_test : public beast::unit_test::suite { using namespace jtx; auto const sa = supported_amendments(); - testGWB(sa - featureFlowCross); - testGWB(sa); + for (auto feature : {sa - featureFlowCross, sa}) + { + testGWB(feature); + testGWBApiVersions(feature); + } testGWBOverflow(); }