From 5b77635dbd5c4709d0726a6f3c8f9b517ec8b36f Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 5 Aug 2024 19:39:06 -0400 Subject: [PATCH] Add invalid MPT amount check in STTx::passesLocalChecks() to provide one single check for all tx. Update unit-tests. Fix payment for unauthorized holder --- include/xrpl/protocol/MPTIssue.h | 3 + include/xrpl/protocol/SOTemplate.h | 8 +- include/xrpl/protocol/STEitherAmount.h | 3 - include/xrpl/protocol/STObject.h | 2 + include/xrpl/protocol/XChainAttestations.h | 6 +- src/libxrpl/protocol/MPTIssue.cpp | 9 + src/libxrpl/protocol/STObject.cpp | 15 +- src/libxrpl/protocol/STTx.cpp | 33 ++ src/libxrpl/protocol/XChainAttestations.cpp | 8 +- src/test/app/MPToken_test.cpp | 407 ++++++++++++++++-- src/test/jtx/attester.h | 6 +- src/test/jtx/impl/attester.cpp | 6 +- src/test/jtx/impl/xchain_bridge.cpp | 8 +- src/test/jtx/xchain_bridge.h | 2 +- src/xrpld/app/tx/detail/AMMCreate.cpp | 3 - src/xrpld/app/tx/detail/AMMDeposit.cpp | 3 - src/xrpld/app/tx/detail/AMMWithdraw.cpp | 3 - src/xrpld/app/tx/detail/CashCheck.cpp | 3 - .../app/tx/detail/NFTokenCreateOffer.cpp | 3 - src/xrpld/app/tx/detail/NFTokenMint.cpp | 3 - src/xrpld/app/tx/detail/Transactor.cpp | 2 +- src/xrpld/app/tx/detail/XChainBridge.cpp | 12 - src/xrpld/ledger/detail/View.cpp | 4 + src/xrpld/rpc/detail/TransactionSign.cpp | 9 +- 24 files changed, 453 insertions(+), 108 deletions(-) diff --git a/include/xrpl/protocol/MPTIssue.h b/include/xrpl/protocol/MPTIssue.h index dc16d207a87..6c01baccf08 100644 --- a/include/xrpl/protocol/MPTIssue.h +++ b/include/xrpl/protocol/MPTIssue.h @@ -84,6 +84,9 @@ isXRP(uint192 const&) return false; } +Json::Value +to_json(MPTIssue const& issue); + } // namespace ripple #endif // RIPPLE_PROTOCOL_MPTISSUE_H_INCLUDED diff --git a/include/xrpl/protocol/SOTemplate.h b/include/xrpl/protocol/SOTemplate.h index 0ea420a483b..56a0d471150 100644 --- a/include/xrpl/protocol/SOTemplate.h +++ b/include/xrpl/protocol/SOTemplate.h @@ -69,9 +69,7 @@ class SOElement public: SOElement(SField const& fieldName, SOEStyle style) - : sField_(fieldName) - , style_(style) - , supportMpt_(SOETxMPTAmount::soeMPTNone) + : sField_(fieldName), style_(style), supportMpt_(soeMPTNone) { init(fieldName); } @@ -101,10 +99,10 @@ class SOElement return style_; } - bool + SOETxMPTAmount supportMPT() const { - return supportMpt_ == soeMPTSupported; + return supportMpt_; } }; diff --git a/include/xrpl/protocol/STEitherAmount.h b/include/xrpl/protocol/STEitherAmount.h index 3c62b42986c..c16c8a1d6a1 100644 --- a/include/xrpl/protocol/STEitherAmount.h +++ b/include/xrpl/protocol/STEitherAmount.h @@ -205,9 +205,6 @@ operator==(STEitherAmount const& lhs, STEitherAmount const& rhs) }, lhs.getValue(), rhs.getValue()); - if (lhs.isIssue() == rhs.isIssue()) - return lhs.getValue() == rhs.getValue(); - return false; } inline bool diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index fb7d15d3630..26cdf5a55ea 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -240,6 +240,8 @@ class STObject : public STBase, public CountedObject Blob getFieldVL(SField const& field) const; STEitherAmount const& + getFieldEitherAmount(SField const& field) const; + STEitherAmount const& getFieldAmount(SField const& field) const; STAmount const& getFieldAmount(TypedFieldAmount const& field) const; diff --git a/include/xrpl/protocol/XChainAttestations.h b/include/xrpl/protocol/XChainAttestations.h index 721950ca9c1..ea715aa30ab 100644 --- a/include/xrpl/protocol/XChainAttestations.h +++ b/include/xrpl/protocol/XChainAttestations.h @@ -143,7 +143,7 @@ struct AttestationClaim : AttestationBase message( STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, + STEitherAmount const& sendingAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t claimID, @@ -226,8 +226,8 @@ struct AttestationCreateAccount : AttestationBase message( STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, - STAmount const& rewardAmount, + STEitherAmount const& sendingAmount, + STEitherAmount const& rewardAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t createCount, diff --git a/src/libxrpl/protocol/MPTIssue.cpp b/src/libxrpl/protocol/MPTIssue.cpp index 28806dce3aa..4737f46a938 100644 --- a/src/libxrpl/protocol/MPTIssue.cpp +++ b/src/libxrpl/protocol/MPTIssue.cpp @@ -19,6 +19,7 @@ #include #include +#include namespace ripple { @@ -67,4 +68,12 @@ getMPT(uint192 const& id) return std::make_pair(sequence, account); } +Json::Value +to_json(MPTIssue const& issue) +{ + Json::Value jv; + jv[jss::mpt_issuance_id] = to_string(issue.getMptID()); + return jv; +} + } // namespace ripple diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index 6542424e372..ec23b16793d 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -165,14 +165,6 @@ STObject::applyTemplate(const SOTemplate& type) e.sField().fieldName, "may not be explicitly set to default."); } - if (iter->get().getSType() == STI_AMOUNT && !e.supportMPT()) - { - if (auto const v = dynamic_cast(&iter->get()); - v && v->isMPT()) - { - throwFieldErr(e.sField().fieldName, "doesn't support MPT"); - } - } v.emplace_back(std::move(*iter)); v_.erase(iter); } @@ -638,6 +630,13 @@ STObject::getFieldVL(SField const& field) const return Blob(b.data(), b.data() + b.size()); } +STEitherAmount const& +STObject::getFieldEitherAmount(SField const& field) const +{ + static STEitherAmount const empty{}; + return getFieldByConstRef(field, empty); +} + STEitherAmount const& STObject::getFieldAmount(SField const& field) const { diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 149186d43ce..d9726ccbd06 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -543,6 +543,32 @@ isAccountFieldOkay(STObject const& st) return true; } +static bool +invalidMPTAmountInTx(STObject const& tx) +{ + auto const txType = tx[~sfTransactionType]; + if (!txType) + return false; + if (auto const* item = + TxFormats::getInstance().findByType(safe_cast(*txType))) + { + for (auto const& e : item->getSOTemplate()) + { + if (tx.isFieldPresent(e.sField()) && e.supportMPT() != soeMPTNone) + { + if (auto const& field = tx.peekAtField(e.sField()); + field.getSType() == STI_AMOUNT && + static_cast(field).isMPT()) + { + if (e.supportMPT() == soeMPTNotSupported) + return true; + } + } + } + } + return false; +} + bool passesLocalChecks(STObject const& st, std::string& reason) { @@ -560,6 +586,13 @@ passesLocalChecks(STObject const& st, std::string& reason) reason = "Cannot submit pseudo transactions."; return false; } + + if (invalidMPTAmountInTx(st)) + { + reason = "Amount can not be MPT."; + return false; + } + return true; } diff --git a/src/libxrpl/protocol/XChainAttestations.cpp b/src/libxrpl/protocol/XChainAttestations.cpp index 2b0c1004a3a..2d736ebff43 100644 --- a/src/libxrpl/protocol/XChainAttestations.cpp +++ b/src/libxrpl/protocol/XChainAttestations.cpp @@ -216,7 +216,7 @@ std::vector AttestationClaim::message( STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, + STEitherAmount const& sendingAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t claimID, @@ -225,7 +225,7 @@ AttestationClaim::message( STObject o{sfGeneric}; // Serialize in SField order to make python serializers easier to write o[sfXChainClaimID] = claimID; - o[sfAmount] = STEitherAmount{sendingAmount}; + o[sfAmount] = sendingAmount; if (dst) o[sfDestination] = *dst; o[sfOtherChainSource] = sendingAccount; @@ -361,8 +361,8 @@ std::vector AttestationCreateAccount::message( STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, - STAmount const& rewardAmount, + STEitherAmount const& sendingAmount, + STEitherAmount const& rewardAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t createCount, diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 94cbe5a616b..f68f1c65bcf 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -640,6 +641,25 @@ class MPToken_test : public beast::unit_test::suite mptAlice.pay(bob, carol, 50); } + // Holder is not authorized + { + Env env{*this, features}; + + MPTTester mptAlice(env, alice, {.holders = {&bob, &carol}}); + + mptAlice.create( + {.ownerCount = 1, .holderCount = 0, .flags = tfMPTCanTransfer}); + + // issuer to holder + mptAlice.pay(alice, bob, 100, tecNO_AUTH); + + // holder to issuer + mptAlice.pay(bob, alice, 100, tecNO_AUTH); + + // holder to holder + mptAlice.pay(bob, carol, 50, tecNO_AUTH); + } + // If allowlisting is enabled, Payment fails if the receiver is not // authorized { @@ -862,40 +882,70 @@ class MPToken_test : public beast::unit_test::suite env(pay(alice, bob, mpt), ter(temDISABLED)); } + // MPT is disabled, unsigned request + { + Env env{*this, features - featureMPTokensV1}; + Account const alice("alice"); // issuer + Account const carol("carol"); + auto const USD = alice["USD"]; + + env.fund(XRP(1'000), alice); + env.fund(XRP(1'000), carol); + STMPTAmount mpt{ + MPTIssue{std::make_pair(1, alice.id())}, UINT64_C(100)}; + + Json::Value jv; + jv[jss::secret] = alice.name(); + jv[jss::tx_json][jss::Fee] = to_string(env.current()->fees().base); + jv[jss::tx_json] = pay(alice, carol, mpt); + auto const jrr = env.rpc("json", "submit", to_string(jv)); + BEAST_EXPECT(jrr[jss::result][jss::engine_result] == "temDISABLED"); + } + // Invalid combination of send, sendMax, deliverMin { Env env{*this, features}; Account const alice("alice"); Account const carol("carol"); - Account const bob("bob"); - MPTTester mptAlice(env, alice, {.holders = {&bob, &carol}}); + MPTTester mptAlice(env, alice, {.holders = {&carol}}); mptAlice.create({.ownerCount = 1, .holderCount = 0}); - mptAlice.authorize({.account = &bob}); - mptAlice.authorize({.account = &carol}); - env(pay(alice, bob, mptAlice.mpt(100)), + // sendMax and DeliverMin are valid XRP amount, + // but is invalid combination with MPT amount + env(pay(alice, carol, mptAlice.mpt(100)), sendmax(XRP(100)), - delivermin(XRP(100)), ter(temMALFORMED)); - - env(pay(alice, bob, mptAlice.mpt(100)), - sendmax(mptAlice.mpt(100)), + env(pay(alice, carol, mptAlice.mpt(100)), delivermin(XRP(100)), ter(temMALFORMED)); + } - env(pay(alice, bob, XRP(100)), - sendmax(mptAlice.mpt(100)), - delivermin(XRP(100)), - ter(temMALFORMED)); + // build_path is invalid if MPT + { + Env env{*this, features}; + Account const alice("alice"); + Account const carol("carol"); - env(pay(alice, bob, XRP(100)), - sendmax(XRP(100)), - delivermin(mptAlice.mpt(100)), - ter(temMALFORMED)); + 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::build_path] = true; + 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 'build_path' not allowed in this context."); } } @@ -905,33 +955,312 @@ class MPToken_test : public beast::unit_test::suite testcase("MPT Amount Invalid in Transaction"); using namespace test::jtx; + std::set txWithAmounts; + for (auto const& format : TxFormats::getInstance()) { - Env env{*this, features}; - Account const alice("alice"); // issuer - - MPTTester mptAlice(env, alice); - - mptAlice.create(); - - env(offer(alice, mptAlice.mpt(100), XRP(100)), ter(temMALFORMED)); - env.close(); - - BEAST_EXPECT(expectOffers(env, alice, 0)); + for (auto const& e : format.getSOTemplate()) + { + // Transaction has amount fields. + // Exclude Clawback, which only supports sfAmount and is checked + // in the transactor for amendment enable/disable. Exclude + // pseudo-transaction SetFee. Don't consider the Fee field since + // it's included in every transaction. + if (e.supportMPT() != soeMPTNone && + e.sField().getName() != jss::Fee && + format.getName() != jss::Clawback && + format.getName() != jss::SetFee) + { + txWithAmounts.insert(format.getName()); + break; + } + } } + Account const alice("alice"); + auto const USD = alice["USD"]; + Account const carol("carol"); + MPTIssue issue(std::make_pair(1, alice.id())); + STMPTAmount mpt{issue, UINT64_C(100)}; + auto const jvb = bridge(alice, USD, alice, USD); + for (auto const& feature : {features, features - featureMPTokensV1}) { - Env env{*this, features - featureMPTokensV1}; - Account const alice("alice"); - + Env env{*this, feature}; env.fund(XRP(1'000), alice); - STMPTAmount mpt{ - MPTIssue{std::make_pair(1, alice.id())}, UINT64_C(100)}; - - env(offer(alice, mpt, XRP(100)), ter(temMALFORMED)); - env.close(); - - BEAST_EXPECT(expectOffers(env, alice, 0)); + env.fund(XRP(1'000), carol); + auto test = [&](Json::Value const& jv) { + txWithAmounts.erase(jv[jss::TransactionType].asString()); + + // tx is signed + auto jtx = env.jt(jv); + Serializer s; + jtx.stx->add(s); + auto jrr = env.rpc("submit", strHex(s.slice())); + BEAST_EXPECT( + jrr[jss::result][jss::error] == "invalidTransaction"); + + // tx is unsigned + Json::Value jv1; + jv1[jss::secret] = alice.name(); + jv1[jss::tx_json] = jv; + jrr = env.rpc("json", "submit", to_string(jv1)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + }; + // All transactions with sfAmount, which don't support MPT + // and transactions with amount fields, which can't be MPT + + // AMMCreate + auto ammCreate = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::AMMCreate; + jv[jss::Account] = alice.human(); + jv[jss::Amount] = (field.fieldName == sfAmount.fieldName) + ? mpt.getJson(JsonOptions::none) + : "100000000"; + jv[jss::Amount2] = (field.fieldName == sfAmount2.fieldName) + ? mpt.getJson(JsonOptions::none) + : "100000000"; + jv[jss::TradingFee] = 0; + test(jv); + }; + ammCreate(sfAmount); + ammCreate(sfAmount2); + // AMMDeposit + auto ammDeposit = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::AMMDeposit; + jv[jss::Account] = alice.human(); + jv[jss::Asset] = to_json(xrpIssue()); + jv[jss::Asset2] = to_json(USD.issue()); + jv[field.fieldName] = mpt.getJson(JsonOptions::none); + jv[jss::Flags] = tfSingleAsset; + test(jv); + }; + ammDeposit(sfAmount); + for (SField const& field : + {std::ref(sfAmount2), + std::ref(sfEPrice), + std::ref(sfLPTokenOut)}) + ammDeposit(field); + // AMMWithdraw + auto ammWithdraw = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::AMMWithdraw; + jv[jss::Account] = alice.human(); + jv[jss::Asset] = to_json(xrpIssue()); + jv[jss::Asset2] = to_json(USD.issue()); + jv[jss::Flags] = tfSingleAsset; + jv[field.fieldName] = mpt.getJson(JsonOptions::none); + test(jv); + }; + ammWithdraw(sfAmount); + for (SField const& field : + {std::ref(sfAmount2), + std::ref(sfEPrice), + std::ref(sfLPTokenIn)}) + ammWithdraw(field); + // AMMBid + auto ammBid = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::AMMBid; + jv[jss::Account] = alice.human(); + jv[jss::Asset] = to_json(xrpIssue()); + jv[jss::Asset2] = to_json(USD.issue()); + jv[field.fieldName] = mpt.getJson(JsonOptions::none); + test(jv); + }; + ammBid(sfBidMin); + ammBid(sfBidMax); + // CheckCash + auto checkCash = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::CheckCash; + jv[jss::Account] = alice.human(); + jv[sfCheckID.fieldName] = to_string(uint256{1}); + jv[field.fieldName] = mpt.getJson(JsonOptions::none); + test(jv); + }; + checkCash(sfAmount); + checkCash(sfDeliverMin); + // CheckCreate + { + Json::Value jv; + jv[jss::TransactionType] = jss::CheckCreate; + jv[jss::Account] = alice.human(); + jv[jss::Destination] = carol.human(); + jv[jss::SendMax] = mpt.getJson(JsonOptions::none); + test(jv); + } + // Clawback + if (!feature[featureMPTokensV1]) + { + } + // EscrowCreate + { + Json::Value jv; + jv[jss::TransactionType] = jss::EscrowCreate; + jv[jss::Account] = alice.human(); + jv[jss::Destination] = carol.human(); + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + test(jv); + } + // OfferCreate + { + Json::Value const jv = offer(alice, USD(100), mpt); + test(jv); + } + // PaymentChannelCreate + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelCreate; + jv[jss::Account] = alice.human(); + jv[jss::Destination] = carol.human(); + jv[jss::SettleDelay] = 1; + jv[sfPublicKey.fieldName] = strHex(alice.pk().slice()); + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + test(jv); + } + // PaymentChannelFund + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelFund; + jv[jss::Account] = alice.human(); + jv[sfChannel.fieldName] = to_string(uint256{1}); + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + test(jv); + } + // PaymentChannelClaim + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelClaim; + jv[jss::Account] = alice.human(); + jv[sfChannel.fieldName] = to_string(uint256{1}); + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + test(jv); + } + // Payment + auto payment = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::Payment; + jv[jss::Account] = alice.human(); + jv[jss::Destination] = carol.human(); + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + if (field == sfSendMax) + jv[jss::SendMax] = mpt.getJson(JsonOptions::none); + else + jv[jss::DeliverMin] = mpt.getJson(JsonOptions::none); + test(jv); + }; + payment(sfSendMax); + payment(sfDeliverMin); + // NFTokenCreateOffer + { + Json::Value jv; + jv[jss::TransactionType] = jss::NFTokenCreateOffer; + jv[jss::Account] = alice.human(); + jv[sfNFTokenID.fieldName] = to_string(uint256{1}); + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + test(jv); + } + // NFTokenAcceptOffer + { + Json::Value jv; + jv[jss::TransactionType] = jss::NFTokenAcceptOffer; + jv[jss::Account] = alice.human(); + jv[sfNFTokenBrokerFee.fieldName] = + mpt.getJson(JsonOptions::none); + test(jv); + } + // NFTokenMint + { + Json::Value jv; + jv[jss::TransactionType] = jss::NFTokenMint; + jv[jss::Account] = alice.human(); + jv[sfNFTokenTaxon.fieldName] = 1; + jv[jss::Amount] = mpt.getJson(JsonOptions::none); + test(jv); + } + // TrustSet + auto trustSet = [&](SField const& field) { + Json::Value jv; + jv[jss::TransactionType] = jss::TrustSet; + jv[jss::Account] = alice.human(); + jv[jss::Flags] = 0; + jv[field.fieldName] = mpt.getJson(JsonOptions::none); + test(jv); + }; + trustSet(sfLimitAmount); + trustSet(sfFee); + // XChainCommit + { + Json::Value const jv = xchain_commit(alice, jvb, 1, mpt); + test(jv); + } + // XChainClaim + { + Json::Value const jv = xchain_claim(alice, jvb, 1, mpt, alice); + test(jv); + } + // XChainCreateClaimID + { + Json::Value const jv = + xchain_create_claim_id(alice, jvb, mpt, alice); + test(jv); + } + // XChainAddClaimAttestation + { + Json::Value const jv = claim_attestation( + alice, + jvb, + alice, + mpt, + alice, + true, + 1, + alice, + signer(alice)); + test(jv); + } + // XChainAddAccountCreateAttestation + { + Json::Value const jv = create_account_attestation( + alice, + jvb, + alice, + mpt, + XRP(10), + alice, + false, + 1, + alice, + signer(alice)); + test(jv); + } + // XChainAccountCreateCommit + { + Json::Value const jv = sidechain_xchain_account_create( + alice, jvb, alice, mpt, XRP(10)); + test(jv); + } + // XChain[Create|Modify]Bridge + auto bridgeTx = [&](Json::StaticString const& tt, + bool minAmount = false) { + Json::Value jv; + jv[jss::TransactionType] = tt; + jv[jss::Account] = alice.human(); + jv[sfXChainBridge.fieldName] = jvb; + jv[sfSignatureReward.fieldName] = + mpt.getJson(JsonOptions::none); + if (minAmount) + jv[sfMinAccountCreateAmount.fieldName] = + mpt.getJson(JsonOptions::none); + test(jv); + }; + bridgeTx(jss::XChainCreateBridge); + bridgeTx(jss::XChainCreateBridge, true); + bridgeTx(jss::XChainModifyBridge); + bridgeTx(jss::XChainModifyBridge, true); } + BEAST_EXPECT(txWithAmounts.empty()); } void @@ -1231,7 +1560,7 @@ class MPToken_test : public beast::unit_test::suite // Test Direct Payment testPayment(all); - // Test MPT Amount is invalid in non-Payment Tx + // Test MPT Amount is invalid in Tx, which don't support MPT testMPTInvalidInTx(all); // Test parsed MPTokenIssuanceID in API response metadata diff --git a/src/test/jtx/attester.h b/src/test/jtx/attester.h index 327fb2e4873..8bb5d57196a 100644 --- a/src/test/jtx/attester.h +++ b/src/test/jtx/attester.h @@ -42,7 +42,7 @@ sign_claim_attestation( SecretKey const& sk, STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, + STEitherAmount const& sendingAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t claimID, @@ -54,8 +54,8 @@ sign_create_account_attestation( SecretKey const& sk, STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, - STAmount const& rewardAmount, + STEitherAmount const& sendingAmount, + STEitherAmount const& rewardAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t createCount, diff --git a/src/test/jtx/impl/attester.cpp b/src/test/jtx/impl/attester.cpp index 66be9da83b3..02a61ffe9bd 100644 --- a/src/test/jtx/impl/attester.cpp +++ b/src/test/jtx/impl/attester.cpp @@ -35,7 +35,7 @@ sign_claim_attestation( SecretKey const& sk, STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, + STEitherAmount const& sendingAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t claimID, @@ -58,8 +58,8 @@ sign_create_account_attestation( SecretKey const& sk, STXChainBridge const& bridge, AccountID const& sendingAccount, - STAmount const& sendingAmount, - STAmount const& rewardAmount, + STEitherAmount const& sendingAmount, + STEitherAmount const& rewardAmount, AccountID const& rewardAccount, bool wasLockingChainSend, std::uint64_t createCount, diff --git a/src/test/jtx/impl/xchain_bridge.cpp b/src/test/jtx/impl/xchain_bridge.cpp index 53b972288a6..32d24c0027a 100644 --- a/src/test/jtx/impl/xchain_bridge.cpp +++ b/src/test/jtx/impl/xchain_bridge.cpp @@ -115,7 +115,7 @@ Json::Value xchain_create_claim_id( Account const& acc, Json::Value const& bridge, - STAmount const& reward, + STEitherAmount const& reward, Account const& otherChainSource) { Json::Value jv; @@ -216,7 +216,7 @@ claim_attestation( sk, stBridge, sendingAccount, - get(sendingAmount.value), + sendingAmount.value, rewardAccount, wasLockingChainSend, claimID, @@ -269,8 +269,8 @@ create_account_attestation( sk, stBridge, sendingAccount, - get(sendingAmount.value), - get(rewardAmount.value), + sendingAmount.value, + rewardAmount.value, rewardAccount, wasLockingChainSend, createCount, diff --git a/src/test/jtx/xchain_bridge.h b/src/test/jtx/xchain_bridge.h index 9968317c8de..dd219d0dfcd 100644 --- a/src/test/jtx/xchain_bridge.h +++ b/src/test/jtx/xchain_bridge.h @@ -61,7 +61,7 @@ Json::Value xchain_create_claim_id( Account const& acc, Json::Value const& bridge, - STAmount const& reward, + STEitherAmount const& reward, Account const& otherChainSource); Json::Value diff --git a/src/xrpld/app/tx/detail/AMMCreate.cpp b/src/xrpld/app/tx/detail/AMMCreate.cpp index 12fee14e6ec..3ed35d72656 100644 --- a/src/xrpld/app/tx/detail/AMMCreate.cpp +++ b/src/xrpld/app/tx/detail/AMMCreate.cpp @@ -41,9 +41,6 @@ AMMCreate::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[sfAmount])) - return temMALFORMED; - if (ctx.tx.getFlags() & tfUniversalMask) { JLOG(ctx.j.debug()) << "AMM Instance: invalid flags."; diff --git a/src/xrpld/app/tx/detail/AMMDeposit.cpp b/src/xrpld/app/tx/detail/AMMDeposit.cpp index be6f2303e52..75993ad4189 100644 --- a/src/xrpld/app/tx/detail/AMMDeposit.cpp +++ b/src/xrpld/app/tx/detail/AMMDeposit.cpp @@ -41,9 +41,6 @@ AMMDeposit::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[~sfAmount])) - return temMALFORMED; - auto const flags = ctx.tx.getFlags(); if (flags & tfDepositMask) { diff --git a/src/xrpld/app/tx/detail/AMMWithdraw.cpp b/src/xrpld/app/tx/detail/AMMWithdraw.cpp index 110291e9ac7..394fd79dd8c 100644 --- a/src/xrpld/app/tx/detail/AMMWithdraw.cpp +++ b/src/xrpld/app/tx/detail/AMMWithdraw.cpp @@ -41,9 +41,6 @@ AMMWithdraw::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[~sfAmount])) - return temMALFORMED; - auto const flags = ctx.tx.getFlags(); if (flags & tfWithdrawMask) { diff --git a/src/xrpld/app/tx/detail/CashCheck.cpp b/src/xrpld/app/tx/detail/CashCheck.cpp index 37b9c1348b9..585bf9dcc99 100644 --- a/src/xrpld/app/tx/detail/CashCheck.cpp +++ b/src/xrpld/app/tx/detail/CashCheck.cpp @@ -42,9 +42,6 @@ CashCheck::preflight(PreflightContext const& ctx) if (!isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[~sfAmount])) - return temMALFORMED; - if (ctx.tx.getFlags() & tfUniversalMask) { // There are no flags (other than universal) for CashCheck yet. diff --git a/src/xrpld/app/tx/detail/NFTokenCreateOffer.cpp b/src/xrpld/app/tx/detail/NFTokenCreateOffer.cpp index 4321253204d..c132df1ffda 100644 --- a/src/xrpld/app/tx/detail/NFTokenCreateOffer.cpp +++ b/src/xrpld/app/tx/detail/NFTokenCreateOffer.cpp @@ -36,9 +36,6 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[sfAmount])) - return temMALFORMED; - auto const txFlags = ctx.tx.getFlags(); if (txFlags & tfNFTokenCreateOfferMask) diff --git a/src/xrpld/app/tx/detail/NFTokenMint.cpp b/src/xrpld/app/tx/detail/NFTokenMint.cpp index bea1fee4b7e..06ff1932f3f 100644 --- a/src/xrpld/app/tx/detail/NFTokenMint.cpp +++ b/src/xrpld/app/tx/detail/NFTokenMint.cpp @@ -53,9 +53,6 @@ NFTokenMint::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[~sfAmount])) - return temMALFORMED; - // Prior to fixRemoveNFTokenAutoTrustLine, transfer of an NFToken between // accounts allowed a TrustLine to be added to the issuer of that token // without explicit permission from that issuer. This was enabled by diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 42e9f0677ab..bf52a66debe 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -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 { diff --git a/src/xrpld/app/tx/detail/XChainBridge.cpp b/src/xrpld/app/tx/detail/XChainBridge.cpp index f611aed7d48..3154d605ca1 100644 --- a/src/xrpld/app/tx/detail/XChainBridge.cpp +++ b/src/xrpld/app/tx/detail/XChainBridge.cpp @@ -1214,9 +1214,6 @@ attestationPreflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[sfAmount])) - return temMALFORMED; - if (ctx.tx.getFlags() & tfUniversalMask) return temINVALID_FLAG; @@ -1675,9 +1672,6 @@ XChainClaim::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[sfAmount])) - return temMALFORMED; - if (ctx.tx.getFlags() & tfUniversalMask) return temINVALID_FLAG; @@ -1916,9 +1910,6 @@ XChainCommit::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[sfAmount])) - return temMALFORMED; - if (ctx.tx.getFlags() & tfUniversalMask) return temINVALID_FLAG; @@ -2190,9 +2181,6 @@ XChainCreateAccountCommit::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (ctx.rules.enabled(featureMPTokensV1) && isMPT(ctx.tx[sfAmount])) - return temMALFORMED; - if (ctx.tx.getFlags() & tfUniversalMask) return temINVALID_FLAG; diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 6021ca75688..98d8d5b59ee 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -1893,6 +1893,8 @@ rippleCredit( else return tecINSUFFICIENT_FUNDS; } + else + return tecNO_AUTH; } if (uReceiverID == issuer) @@ -1921,6 +1923,8 @@ rippleCredit( sfMPTAmount, sle->getFieldU64(sfMPTAmount) + saAmount.value()); view.update(sle); } + else + return tecNO_AUTH; } return tesSUCCESS; } diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 1fee84c683b..77938b4924a 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -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"); @@ -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."); @@ -235,7 +236,7 @@ checkPayment( else { // If no SendMax, default to Amount with sender as issuer. - sendMax = amount; + sendMax = get(amount); sendMax.setIssuer(srcAddressID); } @@ -259,7 +260,7 @@ checkPayment( *dstAccountID, sendMax.issue().currency, sendMax.issue().account, - amount, + get(amount), std::nullopt, app); if (pf.findPaths(app.config().PATH_SEARCH_OLD))