Skip to content

Commit

Permalink
Add rules guards in doSubmit() if transaction is not signed.
Browse files Browse the repository at this point in the history 
Fix Amount parsing if transaction is not signed.
Extend unit-tests.
Consolidate errors. Remove redundant checks.
  • Loading branch information
@gregtatcam
gregtatcam committed Aug 13, 2024
1 parent 749a80a commit a80a8cd
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 9 deletions.
12 changes: 9 additions & 3 deletions src/libxrpl/protocol/STEitherAmount.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
//==============================================================================

#include <xrpl/basics/Log.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Rules.h>
#include <xrpl/protocol/STEitherAmount.h>
#include <xrpl/protocol/SystemParameters.h>
#include <xrpl/protocol/jss.h>
Expand Down Expand Up @@ -244,10 +246,14 @@ amountFromJson(SField const& name, Json::Value const& v)
value = v[jss::value];
if (v.isMember(jss::mpt_issuance_id))
{
isMPT = true;
currencyOrMPTID = v[jss::mpt_issuance_id];
std::optional<Rules> const& rules = getCurrentTransactionRules();
if (!rules || (rules && rules->enabled(featureMPTokensV1)))
{
isMPT = true;
currencyOrMPTID = v[jss::mpt_issuance_id];
}
}
else
if (!isMPT)
{
currencyOrMPTID = v[jss::currency];
issuer = v[jss::issuer];
Expand Down
6 changes: 5 additions & 1 deletion src/libxrpl/protocol/STObject.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,11 @@ STObject::applyTemplate(const SOTemplate& type)
if (auto const v = dynamic_cast<STEitherAmount*>(&iter->get());
v && v->isMPT())
{
throwFieldErr(e.sField().fieldName, "doesn't support MPT");
std::optional<Rules> const& rules =
getCurrentTransactionRules();
if (!rules || rules->enabled(featureMPTokensV1))
throwFieldErr(
e.sField().fieldName, "doesn't support MPT");
}
}
v.emplace_back(std::move(*iter));
Expand Down
97 changes: 97 additions & 0 deletions src/test/app/MPToken_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,54 @@ class MPToken_test : public beast::unit_test::suite
delivermin(mptAlice.mpt(100)),
ter(temMALFORMED));
}

// Unsigned request
{
Env env{*this, features};
Account const alice("alice");
Account const carol("carol");

MPTTester mptAlice(env, alice, {.holders = {&bob, &carol}});

mptAlice.create({.ownerCount = 1, .holderCount = 0});

mptAlice.authorize({.account = &carol});

Json::Value payment;
payment[jss::secret] = alice.name();
payment[jss::tx_json] = pay(alice, carol, mptAlice.mpt(100));
payment[jss::tx_json][jss::SendMax] =
mptAlice.mpt(100).getJson(JsonOptions::none);
payment[jss::tx_json][jss::Fee] =
to_string(env.current()->fees().base);
auto jrr = env.rpc("json", "submit", to_string(payment));
BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::result][jss::error_message] ==
"Field 'SendMax' doesn't support MPT");

payment[jss::tx_json].removeMember(jss::SendMax);
payment[jss::tx_json][jss::DeliverMin] =
mptAlice.mpt(100).getJson(JsonOptions::none);
jrr = env.rpc("json", "submit", to_string(payment));
BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::result][jss::error_message] ==
"Field 'DeliverMin' doesn't support MPT");

payment[jss::tx_json].removeMember(jss::DeliverMin);
payment[jss::build_path] = true;
jrr = env.rpc("json", "submit", to_string(payment));
BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::result][jss::error_message] ==
"Field 'build_path' not allowed in this context.");

payment.removeMember(jss::build_path);
jrr = env.rpc("json", "submit", to_string(payment));
BEAST_EXPECT(jrr[jss::result][jss::status] == "success");
BEAST_EXPECT(jrr[jss::result][jss::engine_result] == "tesSUCCESS");
}
}

void
Expand All @@ -919,6 +967,34 @@ class MPToken_test : public beast::unit_test::suite
BEAST_EXPECT(expectOffers(env, alice, 0));
}

// Unsigned request
{
Env env{*this, features};
Account const alice("alice"); // issuer
auto const USD = alice["USD"];

MPTTester mptAlice(env, alice);

mptAlice.create();

Json::Value jv;
jv[jss::secret] = alice.name();
jv[jss::tx_json] = offer(alice, USD(100), mptAlice.mpt(100));
jv[jss::tx_json][jss::Fee] = to_string(env.current()->fees().base);
auto jrr = env.rpc("json", "submit", to_string(jv));
BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::result][jss::error_message] ==
"Field 'TakerGets' doesn't support MPT");

jv[jss::tx_json] = offer(alice, mptAlice.mpt(100), USD(100));
jrr = env.rpc("json", "submit", to_string(jv));
BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::result][jss::error_message] ==
"Field 'TakerPays' doesn't support MPT");
}

{
Env env{*this, features - featureMPTokensV1};
Account const alice("alice");
Expand All @@ -932,6 +1008,27 @@ class MPToken_test : public beast::unit_test::suite

BEAST_EXPECT(expectOffers(env, alice, 0));
}

// Unsigned request, MPT amendment not enabled
{
Env env{*this, features - featureMPTokensV1};
Account const alice("alice"); // issuer
auto const USD = alice["USD"];

env.fund(XRP(1'000), alice);
STMPTAmount mpt{
MPTIssue{std::make_pair(1, alice.id())}, UINT64_C(100)};

Json::Value jv;
jv[jss::secret] = alice.name();
jv[jss::tx_json] = offer(alice, USD(100), mpt);
jv[jss::tx_json][jss::Fee] = to_string(env.current()->fees().base);
auto jrr = env.rpc("json", "submit", to_string(jv));
BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::result][jss::error_message] ==
"Field 'tx_json.TakerGets' has invalid data.");
}
}

void
Expand Down
2 changes: 1 addition & 1 deletion src/xrpld/app/tx/detail/Transactor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ Transactor::operator()()
// fixSTAmountCanonicalize predate the rulesGuard and should be replaced.
STAmountSO stAmountSO{view().rules().enabled(fixSTAmountCanonicalize)};
NumberSO stNumberSO{view().rules().enabled(fixUniversalNumber)};
CurrentTransactionRulesGuard currentTransctionRulesGuard(view().rules());
CurrentTransactionRulesGuard currentTransactionRulesGuard(view().rules());

#ifdef DEBUG
{
Expand Down
9 changes: 5 additions & 4 deletions src/xrpld/rpc/detail/TransactionSign.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ checkPayment(
if (!tx_json.isMember(jss::Amount))
return RPC::missing_field_error("tx_json.Amount");

STAmount amount;
STEitherAmount amount;

if (!amountFromJsonNoThrow(amount, tx_json[jss::Amount]))
return RPC::invalid_field_error("tx_json.Amount");
Expand All @@ -213,7 +213,8 @@ checkPayment(
if (!dstAccountID)
return RPC::invalid_field_error("tx_json.Destination");

if ((doPath == false) && params.isMember(jss::build_path))
if (((doPath == false) && params.isMember(jss::build_path)) ||
(params.isMember(jss::build_path) && !amount.isIssue()))
return RPC::make_error(
rpcINVALID_PARAMS,
"Field 'build_path' not allowed in this context.");
Expand All @@ -235,7 +236,7 @@ checkPayment(
else
{
// If no SendMax, default to Amount with sender as issuer.
sendMax = amount;
sendMax = get<STAmount>(amount);
sendMax.setIssuer(srcAddressID);
}

Expand All @@ -259,7 +260,7 @@ checkPayment(
*dstAccountID,
sendMax.issue().currency,
sendMax.issue().account,
amount,
get<STAmount>(amount),
std::nullopt,
app);
if (pf.findPaths(app.config().PATH_SEARCH_OLD))
Expand Down
2 changes: 2 additions & 0 deletions src/xrpld/rpc/handlers/Submit.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ doSubmit(RPC::JsonContext& context)

if (!context.params.isMember(jss::tx_blob))
{
CurrentTransactionRulesGuard currentTransactionRulesGuard(
context.ledgerMaster.getCurrentLedger()->rules());
auto const failType = getFailHard(context);

if (context.role != Role::ADMIN && !context.app.config().canSign())
Expand Down

0 comments on commit a80a8cd

Please sign in to comment.