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.

Sign up for GitHub

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

Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

Closed
wants to merge 2 commits into from
Closed

Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df8ac42
Add validated field to account_channels command (RIPD-1466)
seelabs Jul 24, 2017
cdcf1e6
Allow channel_verify to specify public key in hex (RIPD-1467)
seelabs Jul 28, 2017
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
73 changes: 52 additions & 21 deletions src/ripple/net/impl/RPCCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <BeastConfig.h>
#include <ripple/app/main/Application.h>
#include <ripple/basics/StringUtilities.h>
#include <ripple/net/RPCCall.h>
#include <ripple/net/RPCErr.h>
#include <ripple/basics/contract.h>
Expand All @@ -37,6 +38,7 @@
#include <ripple/beast/core/LexicalCast.h>
#include <beast/core/string.hpp>
#include <boost/asio/streambuf.hpp>
#include <boost/optional.hpp>
#include <boost/regex.hpp>
#include <array>
#include <iostream>
Expand Down Expand Up @@ -82,6 +84,33 @@ std::string createHTTPPost (
return s.str ();
}

static
boost::optional<std::uint64_t>
to_uint64(std::string const& s)
{
if (s.empty())
return boost::none;

for (auto c : s)
{
if (!isdigit(c))
return boost::none;
}

try
{
std::size_t pos{};
auto const drops = std::stoul(s, &pos);
if (s.size() != pos)
return boost::none;
return drops;
}
catch (std::exception const&)
{
return boost::none;
}
}

class RPCParser
{
private:
Expand Down Expand Up @@ -680,16 +709,9 @@ class RPCParser
}
jvRequest[jss::channel_id] = jvParams[1u].asString ();

try
{
auto const drops = std::stoul (jvParams[2u].asString ());
(void)drops; // just used for error checking
jvRequest[jss::amount] = jvParams[2u];
}
catch (std::exception const&)
{
return rpcError (rpcCHANNEL_AMT_MALFORMED);
}
if (!jvParams[2u].isString() || !to_uint64(jvParams[2u].asString()))
return rpcError(rpcCHANNEL_AMT_MALFORMED);
jvRequest[jss::amount] = jvParams[2u];

return jvRequest;
}
Expand All @@ -699,7 +721,21 @@ class RPCParser
{
std::string const strPk = jvParams[0u].asString ();

if (!parseBase58<PublicKey> (TokenType::TOKEN_ACCOUNT_PUBLIC, strPk))
bool const validPublicKey = [&strPk]{
if (parseBase58<PublicKey> (TokenType::TOKEN_ACCOUNT_PUBLIC, strPk))
return true;

std::pair<Blob, bool> pkHex(strUnHex (strPk));
if (!pkHex.second)
return false;
Copy link
Contributor

@miguelportilla miguelportilla Aug 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion: This code path doesn't seem to be covered by unit test. We can improve the coverage and test.


if (!publicKeyType(makeSlice(pkHex.first)))
return false;
Copy link
Contributor

@miguelportilla miguelportilla Aug 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Same here


return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

}();

if (!validPublicKey)
return rpcError (rpcPUBLIC_MALFORMED);

Json::Value jvRequest (Json::objectValue);
Expand All @@ -712,16 +748,11 @@ class RPCParser
return rpcError (rpcCHANNEL_MALFORMED);
}
jvRequest[jss::channel_id] = jvParams[1u].asString ();
try
{
auto const drops = std::stoul (jvParams[2u].asString ());
(void)drops; // just used for error checking
jvRequest[jss::amount] = jvParams[2u];
}
catch (std::exception const&)
{
return rpcError (rpcCHANNEL_AMT_MALFORMED);
}

if (!jvParams[2u].isString() || !to_uint64(jvParams[2u].asString()))
return rpcError(rpcCHANNEL_AMT_MALFORMED);
jvRequest[jss::amount] = jvParams[2u];

jvRequest[jss::signature] = jvParams[3u].asString ();

return jvRequest;
Expand Down
10 changes: 4 additions & 6 deletions src/ripple/rpc/handlers/AccountChannels.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,8 @@ Json::Value doAccountChannels (RPC::Context& context)
std::string strIdent (params[jss::account].asString ());
AccountID accountID;

result = RPC::accountFromString (accountID, strIdent);
if (result)
return result;
if (auto const actResult = RPC::accountFromString (accountID, strIdent))
return actResult;

if (! ledger->exists(keylet::account (accountID)))
return rpcError (rpcACT_NOT_FOUND);
Expand All @@ -93,9 +92,8 @@ Json::Value doAccountChannels (RPC::Context& context)
AccountID raDstAccount;
if (hasDst)
{
result = RPC::accountFromString (raDstAccount, strDst);
if (result)
return result;
if (auto const actResult = RPC::accountFromString (raDstAccount, strDst))
return actResult;
}

unsigned int limit;
Expand Down
84 changes: 62 additions & 22 deletions src/ripple/rpc/handlers/PayChanClaim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,37 @@
#include <ripple/rpc/impl/RPCHelpers.h>
#include <ripple/rpc/impl/Tuning.h>

#include <boost/optional.hpp>

namespace ripple {

static
boost::optional<std::uint64_t>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there are now two identical implementations of this function, how would you feel about putting it in basics/StringUtilities? That way if there are future bugs to fix they will only need to be fixed in one place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered that, but I decided they shouldn't be project-wide utilities. I agree code duplication like this is bad, but too many project-wide utility functions has a cost too. It makes finding the right utility function hard to find because the useful ones are mixed in with ones like this that are only used in two files.

If the two places were in the same subsystem, I'd look for a place in that subsystem to put them. As is, they are in ripple/net and ripple/rpc, so the util would have to live in ripple.

I'm OK promoting this project-wide if you feel strongly, but I still vote for the two (small) duplicated functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't feel strongly about it. I have a lot of sympathy for the costs associated with too many project-wide utility functions. I regularly thrash about trying to find where some utility function is hidden. I also probably accidentally create duplicate code, since I have no idea that a function already exists that does what I want to do. I'm not sure precisely what to do about either of those problems.

At any rate, my personal take is that this is not an overly specialized function, so I'd be fine with promoting it as a general facility (since it took you a couple of tries to get it to its current state of correctness). But I'm also okay with duplicating it.

to_uint64(std::string const& s)
{
if (s.empty())
return boost::none;

for (auto c : s)
{
if (!isdigit(c))
return boost::none;
}

try
{
std::size_t pos{};
auto const drops = std::stoul(s, &pos);
if (s.size() != pos)
return boost::none;
return drops;
}
catch (std::exception const&)
{
return boost::none;
}
}

// {
// secret_key: <signing_secret_key>
// channel_id: 256-bit channel id
Expand All @@ -54,15 +83,15 @@ Json::Value doChannelAuthorize (RPC::Context& context)
if (!channelId.SetHexExact (params[jss::channel_id].asString ()))
return rpcError (rpcCHANNEL_MALFORMED);

std::uint64_t drops = 0;
try
{
drops = std::stoul (params[jss::amount].asString ());
}
catch (std::exception const&)
{
boost::optional<std::uint64_t> const optDrops =
params[jss::amount].isString()
? to_uint64(params[jss::amount].asString())
: boost::none;

if (!optDrops)
return rpcError (rpcCHANNEL_AMT_MALFORMED);
}

std::uint64_t const drops = *optDrops;

Serializer msg;
serializePayChanAuthorization (msg, channelId, XRPAmount (drops));
Expand Down Expand Up @@ -90,29 +119,40 @@ Json::Value doChannelVerify (RPC::Context& context)
{
auto const& params (context.params);
for (auto const& p :
{jss::public_key, jss::channel_id, jss::amount, jss::signature})
{jss::public_key, jss::channel_id, jss::amount, jss::signature})
if (!params.isMember (p))
return RPC::missing_field_error (p);

std::string const strPk = params[jss::public_key].asString ();
auto const pk =
parseBase58<PublicKey> (TokenType::TOKEN_ACCOUNT_PUBLIC, strPk);
if (!pk)
return rpcError (rpcPUBLIC_MALFORMED);
boost::optional<PublicKey> pk;
{
std::string const strPk = params[jss::public_key].asString();
pk = parseBase58<PublicKey>(TokenType::TOKEN_ACCOUNT_PUBLIC, strPk);

if (!pk)
{
std::pair<Blob, bool> pkHex(strUnHex (strPk));
if (!pkHex.second)
return rpcError(rpcPUBLIC_MALFORMED);
Copy link
Contributor

@miguelportilla miguelportilla Aug 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion: This code path doesn't seem to be covered by unit test. We can improve the coverage and test.

auto const pkType = publicKeyType(makeSlice(pkHex.first));
if (!pkType)
return rpcError(rpcPUBLIC_MALFORMED);
Copy link
Contributor

@miguelportilla miguelportilla Aug 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Same here

pk.emplace(makeSlice(pkHex.first));
}
}

uint256 channelId;
if (!channelId.SetHexExact (params[jss::channel_id].asString ()))
return rpcError (rpcCHANNEL_MALFORMED);

std::uint64_t drops = 0;
try
{
drops = std::stoul (params[jss::amount].asString ());
}
catch (std::exception const&)
{
boost::optional<std::uint64_t> const optDrops =
params[jss::amount].isString()
? to_uint64(params[jss::amount].asString())
: boost::none;

if (!optDrops)
return rpcError (rpcCHANNEL_AMT_MALFORMED);
}

std::uint64_t const drops = *optDrops;

std::pair<Blob, bool> sig(strUnHex (params[jss::signature].asString ()));
if (!sig.second || !sig.first.size ())
Expand Down
Loading