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 5 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
44 changes: 21 additions & 23 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 const accountID =
parseBase58<AccountID>(params[jss::account].asString());
if (!accountID)
{
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.


if (!ledger->exists(keylet::account(accountID)))
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 @@ -151,7 +149,7 @@ doAccountChannels(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

if (!RPC::isRelatedToAccount(*ledger, sle, accountID))
if (!RPC::isRelatedToAccount(*ledger, sle, *accountID))
return rpcError(rpcINVALID_PARAMS);
}

Expand All @@ -160,7 +158,7 @@ doAccountChannels(RPC::JsonContext& context)
std::uint64_t nextHint = 0;
if (!forEachItemAfter(
*ledger,
accountID,
*accountID,
startAfter,
startHint,
limit + 1,
Expand All @@ -179,9 +177,9 @@ doAccountChannels(RPC::JsonContext& context)
}

if (count <= limit && sleCur->getType() == ltPAYCHAN &&
(*sleCur)[sfAccount] == accountID &&
(!visitData.hasDst ||
visitData.raDstAccount == (*sleCur)[sfDestination]))
(*sleCur)[sfAccount] == *accountID &&
(!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 All @@ -202,7 +200,7 @@ doAccountChannels(RPC::JsonContext& context)
to_string(*marker) + "," + std::to_string(nextHint);
}

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

for (auto const& item : visitData.items)
addChannel(jsonChannels, *item);
Expand Down
16 changes: 8 additions & 8 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,19 +46,19 @@ 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 const accountID = parseBase58<AccountID>(strIdent);
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 optional. Please consider the updated suggested in AccountChannels.cpp comment. I will not repeat this comment for other files. All changed files under rpc/handlers have a similar code block: AccountInfo.cpp, AccountLines.cpp, AccountObjects.cpp, AccountOffers.cpp, DepositAuthorized.cpp, GatewayBalances.cpp, NoRipplecheck.cpp, OwnerInfo.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have applied your suggested changes across the board. Let me know if you find any further issues.

if (!accountID)
{
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}

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

std::set<Currency> send, receive;
for (auto const& rspEntry : RPCTrustLine::getItems(accountID, *ledger))
for (auto const& rspEntry : RPCTrustLine::getItems(*accountID, *ledger))
{
STAmount const& saBalance = rspEntry.getBalance();

Expand Down
25 changes: 11 additions & 14 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,13 @@ 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 const accountID = parseBase58<AccountID>(strIdent);
if (!accountID)
{
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}

static constexpr std::
array<std::pair<std::string_view, LedgerSpecificFlags>, 9>
Expand All @@ -99,7 +95,7 @@ doAccountInfo(RPC::JsonContext& context)
{"disallowIncomingPayChan", lsfDisallowIncomingPayChan},
{"disallowIncomingTrustline", lsfDisallowIncomingTrustline}}};

auto const sleAccepted = ledger->read(keylet::account(accountID));
auto const sleAccepted = ledger->read(keylet::account(*accountID));
if (sleAccepted)
{
auto const queue =
Expand All @@ -113,6 +109,7 @@ doAccountInfo(RPC::JsonContext& context)
return result;
}

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

Expand All @@ -137,7 +134,7 @@ doAccountInfo(RPC::JsonContext& context)

// This code will need to be revisited if in the future we support
// multiple SignerLists on one account.
auto const sleSigners = ledger->read(keylet::signers(accountID));
auto const sleSigners = ledger->read(keylet::signers(*accountID));
if (sleSigners)
jvSignerList.append(sleSigners->getJson(JsonOptions::none));

Expand All @@ -160,7 +157,7 @@ doAccountInfo(RPC::JsonContext& context)
{
Json::Value jvQueueData = Json::objectValue;

auto const txs = context.app.getTxQ().getAccountTxs(accountID);
auto const txs = context.app.getTxQ().getAccountTxs(*accountID);
if (!txs.empty())
{
jvQueueData[jss::txn_count] =
Expand Down Expand Up @@ -247,7 +244,7 @@ doAccountInfo(RPC::JsonContext& context)
}
else
{
result[jss::account] = toBase58(accountID);
result[jss::account] = toBase58(*accountID);
RPC::inject_error(rpcACT_NOT_FOUND, result);
}

Expand Down
Loading