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

API does not accept seed or public key for account #4404

Merged
merged 9 commits into from
May 17, 2023
Merged

API does not accept seed or public key for account #4404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d183bf6
API does not interpret "account" as seed or public key
drlongle Jan 31, 2023
f028527
Fix clang-format
drlongle Jan 31, 2023
b3cf64a
Use Account.human() instead of Account.id() in unit tests
drlongle Feb 21, 2023
aa420cf
Drop non-strict account parsing #3330
drlongle Mar 1, 2023
958a3e6
Do not default construct account ids
drlongle Mar 30, 2023
0263106
Merge branch 'develop' into do-not-interpret-account-as-seed
drlongle May 9, 2023
4f970ef
Address reviewer's comments
drlongle May 9, 2023
8997cb7
Address review comments
drlongle May 9, 2023
7d93021
Address review comments
drlongle May 9, 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
10 changes: 4 additions & 6 deletions src/ripple/app/main/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,12 @@ printHelp(const po::options_description& desc)
<< systemName() << "d [options] <command> <params>\n"
<< desc << std::endl
<< "Commands: \n"
" account_currencies <account> [<ledger>] [strict]\n"
" account_info <account>|<seed>|<pass_phrase>|<key> [<ledger>] "
"[strict]\n"
" account_currencies <account> [<ledger>]\n"
" account_info <account>|<key> [<ledger>]\n"
" account_lines <account> <account>|\"\" [<ledger>]\n"
" account_channels <account> <account>|\"\" [<ledger>]\n"
" account_objects <account> [<ledger>] [strict]\n"
" account_offers <account>|<account_public_key> [<ledger>] "
"[strict]\n"
" account_objects <account> [<ledger>]\n"
" account_offers <account>|<account_public_key> [<ledger>]\n"
" account_tx accountID [ledger_index_min [ledger_index_max "
"[limit "
"]]] [binary]\n"
Expand Down
26 changes: 5 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 @@ -775,11 +775,9 @@ class RPCParser
return jvRequest;
}

// owner_info <account>|<account_public_key> [strict]
// owner_info <seed>|<pass_phrase>|<key> [<ledger>] [strict]
// account_info <account>|<account_public_key> [strict]
// account_info <seed>|<pass_phrase>|<key> [<ledger>] [strict]
// account_offers <account>|<account_public_key> [<ledger>] [strict]
// owner_info <account>
// account_info <account> [<ledger>]
// account_offers <account> [<ledger>]
Json::Value
parseAccountItems(Json::Value const& jvParams)
{
Expand Down Expand Up @@ -895,10 +893,7 @@ class RPCParser
// Parameters 0 and 1 are accounts
if (i < 2)
{
if (parseBase58<PublicKey>(
TokenType::AccountPublic, strParam) ||
parseBase58<AccountID>(strParam) ||
parseGenericSeed(strParam))
if (parseBase58<AccountID>(strParam))
{
jvRequest[accFields[i]] = std::move(strParam);
}
Expand All @@ -924,26 +919,15 @@ class RPCParser
{
std::string strIdent = jvParams[0u].asString();
unsigned int iCursor = jvParams.size();
bool bStrict = false;

if (iCursor >= 2 && jvParams[iCursor - 1] == jss::strict)
{
bStrict = true;
--iCursor;
}

if (!parseBase58<PublicKey>(TokenType::AccountPublic, strIdent) &&
!parseBase58<AccountID>(strIdent) && !parseGenericSeed(strIdent))
if (!parseBase58<AccountID>(strIdent))
return rpcError(rpcACT_MALFORMED);

// Get info on account.
Json::Value jvRequest(Json::objectValue);

jvRequest[jss::account] = strIdent;

if (bStrict)
jvRequest[jss::strict] = 1;

if (iCursor == 2 && !jvParseLedger(jvRequest, jvParams[1u].asString()))
return rpcError(rpcLGR_IDX_MALFORMED);

Expand Down
34 changes: 16 additions & 18 deletions src/ripple/rpc/handlers/AccountChannels.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ addChannel(Json::Value& jsonLines, SLE const& line)
}

// {
// account: <account>|<account_public_key>
// account: <account>
// ledger_hash : <ledger>
// ledger_index : <ledger_index>
// limit: integer // optional
Expand All @@ -76,26 +76,25 @@ doAccountChannels(RPC::JsonContext& context)
if (!ledger)
return result;

std::string strIdent(params[jss::account].asString());
AccountID accountID;

if (auto const err = RPC::accountFromString(accountID, strIdent))
return err;
auto id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
return rpcError(rpcACT_MALFORMED);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

accountID is not really an optional. It's always seated if valid. Consider something like this:

   auto const id = parseBase58<AccountID>(params[jss::account].asString());
   if (!id)
   {
       rpcError(rpcACT_MALFORMED);
   }
  AccountID const accountID{std::move(id.value())};

It also results in fewer code changes. Other handlers have a similar code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

AccountID const accountID{std::move(id.value())};

if (!ledger->exists(keylet::account(accountID)))
return rpcError(rpcACT_NOT_FOUND);

std::string strDst;
if (params.isMember(jss::destination_account))
strDst = params[jss::destination_account].asString();
auto hasDst = !strDst.empty();

AccountID raDstAccount;
if (hasDst)
{
if (auto const err = RPC::accountFromString(raDstAccount, strDst))
return err;
}
auto const raDstAccount = [&]() -> std::optional<AccountID> {
return strDst.empty() ? std::nullopt : parseBase58<AccountID>(strDst);
}();
if (!strDst.empty() && !raDstAccount)
return rpcError(rpcACT_MALFORMED);

unsigned int limit;
if (auto err = readLimitField(limit, RPC::Tuning::accountChannels, context))
Expand All @@ -109,10 +108,9 @@ doAccountChannels(RPC::JsonContext& context)
{
std::vector<std::shared_ptr<SLE const>> items;
AccountID const& accountID;
bool hasDst;
AccountID const& raDstAccount;
std::optional<AccountID> const& raDstAccount;
};
VisitData visitData = {{}, accountID, hasDst, raDstAccount};
VisitData visitData = {{}, accountID, raDstAccount};
visitData.items.reserve(limit);
uint256 startAfter = beast::zero;
std::uint64_t startHint = 0;
Expand Down Expand Up @@ -180,8 +178,8 @@ doAccountChannels(RPC::JsonContext& context)

if (count <= limit && sleCur->getType() == ltPAYCHAN &&
(*sleCur)[sfAccount] == accountID &&
(!visitData.hasDst ||
visitData.raDstAccount == (*sleCur)[sfDestination]))
(!visitData.raDstAccount ||
*visitData.raDstAccount == (*sleCur)[sfDestination]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can simplify:

visitData.raDstAccount <= (*sleCur)[sfDestination])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we check if DstAccount is provided or DstAccount == (*sleCur)[sfDestination].

Your suggestion seems to change the intention of this check. Would it work as expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. I'm taking back this suggestion.

{
visitData.items.emplace_back(sleCur);
}
Expand Down
13 changes: 7 additions & 6 deletions src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,14 @@ doAccountCurrencies(RPC::JsonContext& context)
params.isMember(jss::account) ? params[jss::account].asString()
: params[jss::ident].asString());

bool const bStrict =
params.isMember(jss::strict) && params[jss::strict].asBool();

// Get info on account.
AccountID accountID; // out param
if (auto jvAccepted = RPC::accountFromString(accountID, strIdent, bStrict))
return jvAccepted;
auto id = parseBase58<AccountID>(strIdent);
if (!id)
{
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};

if (!ledger->exists(keylet::account(accountID)))
return rpcError(rpcACT_NOT_FOUND);
Expand Down
18 changes: 8 additions & 10 deletions src/ripple/rpc/handlers/AccountInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ namespace ripple {

// {
// account: <ident>,
// strict: <bool> // optional (default false)
// // if true only allow public keys and addresses.
// ledger_hash : <ledger>
// ledger_index : <ledger_index>
// signer_lists : <bool> // optional (default false)
Expand Down Expand Up @@ -67,15 +65,14 @@ doAccountInfo(RPC::JsonContext& context)
if (!ledger)
return result;

bool bStrict = params.isMember(jss::strict) && params[jss::strict].asBool();
AccountID accountID;

// Get info on account.

auto jvAccepted = RPC::accountFromString(accountID, strIdent, bStrict);

if (jvAccepted)
return jvAccepted;
auto id = parseBase58<AccountID>(strIdent);
if (!id)
{
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};

static constexpr std::
array<std::pair<std::string_view, LedgerSpecificFlags>, 9>
Expand Down Expand Up @@ -113,6 +110,7 @@ doAccountInfo(RPC::JsonContext& context)
return result;
}

Json::Value jvAccepted(Json::objectValue);
RPC::injectSLE(jvAccepted, *sleAccepted);
result[jss::account_data] = jvAccepted;

Expand Down
53 changes: 22 additions & 31 deletions src/ripple/rpc/handlers/AccountLines.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,6 @@

namespace ripple {

struct VisitData
{
std::vector<RPCTrustLine> items;
AccountID const& accountID;
bool hasPeer;
AccountID const& raPeerAccount;

bool ignoreDefault;
uint32_t foundCount;
};

void
addLine(Json::Value& jsonLines, RPCTrustLine const& line)
{
Expand Down Expand Up @@ -76,7 +65,7 @@ addLine(Json::Value& jsonLines, RPCTrustLine const& line)
}

// {
// account: <account>|<account_public_key>
// account: <account>
// ledger_hash : <ledger>
// ledger_index : <ledger_index>
// limit: integer // optional
Expand All @@ -96,33 +85,28 @@ doAccountLines(RPC::JsonContext& context)
if (!ledger)
return result;

std::string strIdent(params[jss::account].asString());
AccountID accountID;

if (auto jv = RPC::accountFromString(accountID, strIdent))
auto id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
for (auto it = jv.begin(); it != jv.end(); ++it)
justinr1234 marked this conversation as resolved.
Show resolved Hide resolved
result[it.memberName()] = *it;
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};

if (!ledger->exists(keylet::account(accountID)))
return rpcError(rpcACT_NOT_FOUND);

std::string strPeer;
if (params.isMember(jss::peer))
strPeer = params[jss::peer].asString();
auto hasPeer = !strPeer.empty();

AccountID raPeerAccount;
if (hasPeer)
auto const raPeerAccount = [&]() -> std::optional<AccountID> {
return strPeer.empty() ? std::nullopt : parseBase58<AccountID>(strPeer);
}();
if (!strPeer.empty() && !raPeerAccount)
{
if (auto jv = RPC::accountFromString(raPeerAccount, strPeer))
{
for (auto it = jv.begin(); it != jv.end(); ++it)
result[it.memberName()] = *it;
return result;
}
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}

unsigned int limit;
Expand All @@ -138,8 +122,15 @@ doAccountLines(RPC::JsonContext& context)
params[jss::ignore_default].asBool();

Json::Value& jsonLines(result[jss::lines] = Json::arrayValue);
VisitData visitData = {
{}, accountID, hasPeer, raPeerAccount, ignoreDefault, 0};
struct VisitData
{
std::vector<RPCTrustLine> items;
AccountID const& accountID;
std::optional<AccountID> const& raPeerAccount;
bool ignoreDefault;
uint32_t foundCount;
};
VisitData visitData = {{}, accountID, raPeerAccount, ignoreDefault, 0};
uint256 startAfter = beast::zero;
std::uint64_t startHint = 0;

Expand Down Expand Up @@ -227,8 +218,8 @@ doAccountLines(RPC::JsonContext& context)
RPCTrustLine::makeItem(visitData.accountID, sleCur);

if (line &&
(!visitData.hasPeer ||
visitData.raPeerAccount ==
(!visitData.raPeerAccount ||
*visitData.raPeerAccount ==
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can simplify: visitData.raPeerAccount <= line->getAccountIDPeer()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, we check if peerAccount is not provided or peerAccount == line->getAccountIDPeer().

Your suggestion seems to change the intention of this check. Would it work as expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. I'm taking back this suggestion.

line->getAccountIDPeer()))
{
visitData.items.emplace_back(*line);
Expand Down
30 changes: 11 additions & 19 deletions src/ripple/rpc/handlers/AccountObjects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace ripple {

/** General RPC command that can retrieve objects in the account root.
{
account: <account>|<account_public_key>
account: <account>
ledger_hash: <string> // optional
ledger_index: <string | unsigned integer> // optional
type: <string> // optional, defaults to all account objects types
Expand All @@ -60,17 +60,13 @@ doAccountNFTs(RPC::JsonContext& context)
if (ledger == nullptr)
return result;

AccountID accountID;
auto id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
auto const strIdent = params[jss::account].asString();
if (auto jv = RPC::accountFromString(accountID, strIdent))
{
for (auto it = jv.begin(); it != jv.end(); ++it)
result[it.memberName()] = *it;

return result;
}
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};

if (!ledger->exists(keylet::account(accountID)))
return rpcError(rpcACT_NOT_FOUND);
Expand Down Expand Up @@ -177,17 +173,13 @@ doAccountObjects(RPC::JsonContext& context)
if (ledger == nullptr)
return result;

AccountID accountID;
auto const id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
auto const strIdent = params[jss::account].asString();
if (auto jv = RPC::accountFromString(accountID, strIdent))
{
for (auto it = jv.begin(); it != jv.end(); ++it)
result[it.memberName()] = *it;

return result;
}
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};

if (!ledger->exists(keylet::account(accountID)))
return rpcError(rpcACT_NOT_FOUND);
Expand Down
13 changes: 5 additions & 8 deletions src/ripple/rpc/handlers/AccountOffers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ appendOfferJson(std::shared_ptr<SLE const> const& offer, Json::Value& offers)
};

// {
// account: <account>|<account_public_key>
// account: <account>
// ledger_hash : <ledger>
// ledger_index : <ledger_index>
// limit: integer // optional
Expand All @@ -65,16 +65,13 @@ doAccountOffers(RPC::JsonContext& context)
if (!ledger)
return result;

std::string strIdent(params[jss::account].asString());
AccountID accountID;

if (auto jv = RPC::accountFromString(accountID, strIdent))
auto id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
for (auto it = jv.begin(); it != jv.end(); ++it)
result[it.memberName()] = (*it);

RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};

// Get info on account.
result[jss::account] = toBase58(accountID);
Expand Down
Loading