Skip to content

Commit

Permalink
Improved error message on mistyped command [RIPD-1527]:
Browse files Browse the repository at this point in the history 
Previously if you mistyped the "submit_multisigned" command as
"submit_multisign", the returned message was "Internal error".  Not
very helpful.  It turns out this was caused by a small amount of
code in RPCCall.cpp.  Removing that code improves two situations:

1. It improves the situation with a mistyped command.  Now the
   command returns "Unknown method" and provides the string of
   the mistyped command.

2. The "transaction_entry", if properly entered in its command
   line form, would fire an assert.  That assert is now removed.

In the process, it was discovered that the command line form of
the "transaction_entry" command has not worked correctly for at
least a year.  Therefore support for that the command line form
of "transaction_entry" is added along with appropriate unit
tests.
  • Loading branch information
@scottschurr
scottschurr committed Nov 23, 2017
1 parent a0e55dd commit 613dc6e
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 19 deletions.
33 changes: 26 additions & 7 deletions src/ripple/net/impl/RPCCall.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,31 @@ class RPCParser
return rpcError (rpcINVALID_PARAMS);
}

// transaction_entry <tx_hash> <ledger_hash/ledger_index>
Json::Value parseTransactionEntry (Json::Value const& jvParams)
{
// Parameter count should have already been verified.
assert (jvParams.size() == 2);

std::string const txHash = jvParams[0u].asString();
if (txHash.length() != 64)
return rpcError (rpcINVALID_PARAMS);

Json::Value jvRequest;
jvRequest[jss::tx_hash] = txHash;

jvParseLedger (jvRequest, jvParams[1u].asString());

// jvParseLedger inserts a "ledger_index" of 0 if it doesn't
// find a match.
if (jvRequest.isMember(jss::ledger_index) &&
jvRequest[jss::ledger_index] == 0)
return rpcError (rpcINVALID_PARAMS);

return jvRequest;
}


// tx <transaction_id>
Json::Value parseTx (Json::Value const& jvParams)
{
Expand Down Expand Up @@ -1073,7 +1098,7 @@ class RPCParser
{ "server_info", &RPCParser::parseAsIs, 0, 0 },
{ "server_state", &RPCParser::parseAsIs, 0, 0 },
{ "stop", &RPCParser::parseAsIs, 0, 0 },
// { "transaction_entry", &RPCParser::parseTransactionEntry, -1, -1 },
{ "transaction_entry", &RPCParser::parseTransactionEntry, 2, 2 },
{ "tx", &RPCParser::parseTx, 1, 2 },
{ "tx_account", &RPCParser::parseTxAccount, 1, 7 },
{ "tx_history", &RPCParser::parseTxHistory, 1, 1 },
Expand Down Expand Up @@ -1323,12 +1348,6 @@ rpcClient(std::vector<std::string> const& args,
jvParams.append(jvRequest[i]);
}

if (jvRequest.isMember(jss::params))
{
auto const& params = jvRequest[jss::params];
assert(params.size() == 1);
jvParams.append(params[0u]);
}
{
boost::asio::io_service isService;
RPCCall::fromNetwork (
Expand Down
12 changes: 12 additions & 0 deletions src/test/rpc/JSONRPC_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1840,6 +1840,17 @@ R"({
class JSONRPC_test : public beast::unit_test::suite
{
public:
void testBadRpcCommand ()
{
test::jtx::Env env(*this);
Json::Value const result {
env.rpc ("bad_command", R"({"MakingThisUp": 0})")};

BEAST_EXPECT (result[jss::result][jss::error] == "unknownCmd");
BEAST_EXPECT (
result[jss::result][jss::request][jss::command] == "bad_command");
}

void testAutoFillFees ()
{
test::jtx::Env env(*this);
Expand Down Expand Up @@ -2346,6 +2357,7 @@ class JSONRPC_test : public beast::unit_test::suite

void run ()
{
testBadRpcCommand ();
testAutoFillFees ();
testAutoFillEscalatedFees ();
testTransactionRPC ();
Expand Down
113 changes: 101 additions & 12 deletions src/test/rpc/TransactionEntry_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,76 @@ class TransactionEntry_test : public beast::unit_test::suite
BEAST_EXPECT(result[jss::error] == "transactionNotFound");
BEAST_EXPECT(result[jss::status] == "error");
}

std::string const txHash {
"E2FE8D4AF3FCC3944DDF6CD8CDDC5E3F0AD50863EF8919AFEF10CB6408CD4D05"};

// Command line format
{
// No arguments
Json::Value const result {env.rpc ("transaction_entry")};
BEAST_EXPECT(result[jss::ledger_hash].asString().empty());
BEAST_EXPECT(result[jss::error] == "badSyntax");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// One argument
Json::Value const result {env.rpc ("transaction_entry", txHash)};
BEAST_EXPECT(result[jss::error] == "badSyntax");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// First argument with too few characters
Json::Value const result {env.rpc (
"transaction_entry", txHash.substr (1), "closed")};
BEAST_EXPECT(result[jss::error] == "invalidParams");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// First argument with too many characters
Json::Value const result {env.rpc (
"transaction_entry", txHash + "A", "closed")};
BEAST_EXPECT(result[jss::error] == "invalidParams");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// Second argument not valid
Json::Value const result {env.rpc (
"transaction_entry", txHash, "closer")};
BEAST_EXPECT(result[jss::error] == "invalidParams");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// Ledger index of 0 is not valid
Json::Value const result {env.rpc (
"transaction_entry", txHash, "0")};
BEAST_EXPECT(result[jss::error] == "invalidParams");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// Three arguments
Json::Value const result {env.rpc (
"transaction_entry", txHash, "closed", "extra")};
BEAST_EXPECT(result[jss::error] == "badSyntax");
BEAST_EXPECT(result[jss::status] == "error");
}

{
// Valid structure, but transaction not found.
Json::Value const result {env.rpc (
"transaction_entry", txHash, "closed")};
BEAST_EXPECT(
! result[jss::result][jss::ledger_hash].asString().empty());
BEAST_EXPECT(
result[jss::result][jss::error] == "transactionNotFound");
BEAST_EXPECT(result[jss::result][jss::status] == "error");
}
}

void testRequest()
Expand All @@ -78,24 +148,28 @@ class TransactionEntry_test : public beast::unit_test::suite
Env env {*this};

auto check_tx = [this, &env]
(int index, std::string txhash, std::string type = "")
(int index, std::string const txhash, std::string const type = "")
{
Json::Value resIndex, resHash;
// first request using ledger_index to lookup
Json::Value const resIndex {[&env, index, &txhash] ()
{
Json::Value params {Json::objectValue};
params[jss::ledger_index] = index;
params[jss::tx_hash] = txhash;
resIndex = env.client()
return env.client()
.invoke("transaction_entry", params)[jss::result];
if(! BEAST_EXPECTS(resIndex.isMember(jss::tx_json), txhash))
return;
BEAST_EXPECT(resIndex[jss::tx_json][jss::hash] == txhash);
if(! type.empty())
BEAST_EXPECTS(
resIndex[jss::tx_json][jss::TransactionType] == type,
txhash + " is " +
resIndex[jss::tx_json][jss::TransactionType].asString());
}()};

if(! BEAST_EXPECTS(resIndex.isMember(jss::tx_json), txhash))
return;

BEAST_EXPECT(resIndex[jss::tx_json][jss::hash] == txhash);
if(! type.empty())
{
BEAST_EXPECTS(
resIndex[jss::tx_json][jss::TransactionType] == type,
txhash + " is " +
resIndex[jss::tx_json][jss::TransactionType].asString());
}

// second request using ledger_hash to lookup and verify
Expand All @@ -104,10 +178,25 @@ class TransactionEntry_test : public beast::unit_test::suite
Json::Value params {Json::objectValue};
params[jss::ledger_hash] = resIndex[jss::ledger_hash];
params[jss::tx_hash] = txhash;
resHash = env.client()
Json::Value const resHash = env.client()
.invoke("transaction_entry", params)[jss::result];
BEAST_EXPECT(resHash == resIndex);
}

// Use the command line form with the index.
{
Json::Value const clIndex {env.rpc (
"transaction_entry", txhash, std::to_string (index))};
BEAST_EXPECT (clIndex["result"] == resIndex);
}

// Use the command line form with the ledger_hash.
{
Json::Value const clHash {env.rpc (
"transaction_entry", txhash,
resIndex[jss::ledger_hash].asString())};
BEAST_EXPECT (clHash["result"] == resIndex);
}
};

Account A1 {"A1"};
Expand Down

0 comments on commit 613dc6e

Please sign in to comment.