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

Added Error messages on Gateway_Balance and Channel_Authorize #4577

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f7c5c65
Error messages for ApiV2
PeterChen13579 Jun 20, 2023
2b78768
added clang format
PeterChen13579 Jun 20, 2023
2eb175e
changed ledger->read to exist
PeterChen13579 Jun 20, 2023
e493534
quick fix
PeterChen13579 Jun 20, 2023
690ce23
changed env unit test based on ED's commit
PeterChen13579 Jun 22, 2023
0cd5145
Revert "changed env unit test based on ED's commit"
PeterChen13579 Jun 22, 2023
c843248
Merge remote-tracking branch 'upstream/master' into Error_messages
PeterChen13579 Jun 22, 2023
614cea4
Merge remote-tracking branch 'upstream/develop' into Error_messages
PeterChen13579 Jun 22, 2023
29971ca
changed unit test based on ED's API Beta commit
PeterChen13579 Jun 22, 2023
311e66c
Update src/test/app/PayChan_test.cpp
PeterChen13579 Jun 22, 2023
3adae5b
changed unittest based on Arihant's comment
PeterChen13579 Jun 27, 2023
dad2e8e
added clang format
PeterChen13579 Jun 27, 2023
7344b30
Fixed issue #4548 as dealing with same rpc
PeterChen13579 Jun 28, 2023
b1bae4d
Fix for arihant's comment
PeterChen13579 Jun 28, 2023
da46159
compare to 2u, for faster step
PeterChen13579 Jun 29, 2023
41fcf31
sign commit attempt
PeterChen13579 Jun 30, 2023
682a1a4
quick fix on Arihant's comment
PeterChen13579 Jul 5, 2023
920cd81
Merge remote-tracking branch 'upstream/develop' into Error_messages
PeterChen13579 Jul 10, 2023
21c5ebd
fixes based on Greg's comments
PeterChen13579 Jul 10, 2023
38e7f8e
move account/HW outside of loop
PeterChen13579 Jul 10, 2023
ce680b5
typo fix
PeterChen13579 Jul 10, 2023
092f6c3
refactor based on shawn's comments
PeterChen13579 Jul 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/ripple/rpc/handlers/GatewayBalances.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ doGatewayBalances(RPC::JsonContext& context)

result[jss::account] = toBase58(accountID);

if (!ledger->exists(keylet::account(accountID)) && context.apiVersion > 1u)
{
RPC::inject_error(rpcACT_NOT_FOUND, result);
return result;
}

// Parse the specified hotwallet(s), if any
std::set<AccountID> hotWallets;

Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add rpcINALID_HOT_WALLET error and inject it into the result.

{
result[jss::error] = "invalidHotWallet";
}
else
{
result[jss::error] = "invalidParams";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please inject rpcINVALID_PARAMS into result.

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

brackets here as well

return result;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/handlers/PayChanClaim.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider passing context.apiVersion instead.

if (RPC::contains_error(result))
return result;

Expand Down
14 changes: 12 additions & 2 deletions src/ripple/rpc/impl/RPCHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,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,
std::optional<std::reference_wrapper<JsonContext>> context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider std::optional<uint> const& apiVersion instead.

{
bool const has_key_type = params.isMember(jss::key_type);

Expand Down Expand Up @@ -900,7 +903,14 @@ keypairForSignature(Json::Value const& params, Json::Value& error)

if (!keyType)
{
error = RPC::invalid_field_error(jss::key_type);
if (context.has_value() && context.value().get().apiVersion > 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: keep it consistent - use 1u if you are using it everywhere else in the PR. Or keep it 1 if you want to use that everywhere but keep it consistent.

{
error = RPC::make_error(rpcBAD_KEY_TYPE);
}
else
{
error = RPC::invalid_field_error(jss::key_type);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the brackets needed here?

return {};
}

Expand Down
5 changes: 4 additions & 1 deletion src/ripple/rpc/impl/RPCHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,10 @@ std::optional<Seed>
parseRippleLibSeed(Json::Value const& params);

std::pair<PublicKey, SecretKey>
keypairForSignature(Json::Value const& params, Json::Value& error);
keypairForSignature(
Json::Value const& params,
Json::Value& error,
std::optional<std::reference_wrapper<JsonContext>> context = std::nullopt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider changing to std::optional<uint> const& apiVersion. Actually, maybe we can even do this: uint apiVersion = RPC::apiVersionIfUnspecified?


/**
* API version numbers used in API version 1
Expand Down
54 changes: 52 additions & 2 deletions src/test/app/PayChan_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change the headers order? The original order and the new line looks right to me.

#include <chrono>
#include <test/jtx.h>

namespace ripple {
namespace test {
Expand Down Expand Up @@ -1160,6 +1160,50 @@ struct PayChan_test : public beast::unit_test::suite
bob.human());
}

void
testAccountChannelAuthorize(FeatureBitset features, unsigned int apiVersion)
{
testcase("PayChan Channel_Auth RPC Api " + std::to_string(apiVersion));
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();

// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider moving the testVersion loop inside this function. The channel above doesn't need to be created twice. Then this is going to be:

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);
   }

{
auto const rs = env.rpc(
"json",
"channel_authorize",
args.toStyledString())[jss::result];
BEAST_EXPECT(rs[jss::error] == "invalidParams");
}
else
{
auto const rs = env.rpc(
"json",
"channel_authorize",
args.toStyledString())[jss::result];
BEAST_EXPECT(rs[jss::error] == "badKeyType");
}
}

void
testAuthVerifyRPC(FeatureBitset features)
{
Expand Down Expand Up @@ -2139,6 +2183,12 @@ struct PayChan_test : public beast::unit_test::suite
testAccountChannelsRPC(features);
testAccountChannelsRPCMarkers(features);
testAccountChannelsRPCSenderOnly(features);
for (auto testVersion = RPC::apiMinimumSupportedVersion;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider moving the loop inside the function.

testVersion <= RPC::apiBetaVersion;
++testVersion)
{
testAccountChannelAuthorize(features, testVersion);
}
testAuthVerifyRPC(features);
testOptionalFields(features);
testMalformedPK(features);
Expand Down
Loading