From 27f9f726c32e31ddf806f5a8d1efda1b8190fc58 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Thu, 5 Sep 2024 16:37:32 -0700 Subject: [PATCH 01/11] Permissioned Domains: https://github.com/XRPLF/XRPL-Standards/discussions/228 --- include/xrpl/protocol/Feature.h | 2 +- include/xrpl/protocol/Indexes.h | 3 + include/xrpl/protocol/detail/features.macro | 1 + .../xrpl/protocol/detail/ledger_entries.macro | 13 + include/xrpl/protocol/detail/sfields.macro | 2 + .../xrpl/protocol/detail/transactions.macro | 11 + include/xrpl/protocol/jss.h | 2 + src/libxrpl/protocol/Indexes.cpp | 9 + src/test/app/PermissionedDomains_test.cpp | 478 ++++++++++++++++++ src/test/ledger/Invariants_test.cpp | 41 ++ src/xrpld/app/tx/detail/InvariantCheck.cpp | 44 ++ src/xrpld/app/tx/detail/InvariantCheck.h | 31 +- .../tx/detail/PermissionedDomainDelete.cpp | 75 +++ .../app/tx/detail/PermissionedDomainDelete.h | 49 ++ .../app/tx/detail/PermissionedDomainSet.cpp | 147 ++++++ .../app/tx/detail/PermissionedDomainSet.h | 54 ++ src/xrpld/app/tx/detail/applySteps.cpp | 2 + src/xrpld/rpc/handlers/AccountObjects.cpp | 3 +- 18 files changed, 964 insertions(+), 3 deletions(-) create mode 100644 src/test/app/PermissionedDomains_test.cpp create mode 100644 src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp create mode 100644 src/xrpld/app/tx/detail/PermissionedDomainDelete.h create mode 100644 src/xrpld/app/tx/detail/PermissionedDomainSet.cpp create mode 100644 src/xrpld/app/tx/detail/PermissionedDomainSet.h diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 90a81c55ef4..df071c69864 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -80,7 +80,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 83; +static constexpr std::size_t numFeatures = 84; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h index 72cf0b527b1..f327412e72e 100644 --- a/include/xrpl/protocol/Indexes.h +++ b/include/xrpl/protocol/Indexes.h @@ -329,6 +329,9 @@ mptoken(uint256 const& mptokenKey) Keylet mptoken(uint256 const& issuanceKey, AccountID const& holder) noexcept; +Keylet +permissionedDomain(AccountID const& account, std::uint32_t seq) noexcept; + } // namespace keylet // Everything below is deprecated and should be removed in favor of keylets: diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 24c6e72ae34..e2d1fb4ed68 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -98,6 +98,7 @@ XRPL_FIX (1513, Supported::yes, VoteBehavior::DefaultYe XRPL_FEATURE(FlowCross, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(OwnerPaysFee, Supported::no, VoteBehavior::DefaultNo) +XRPL_FEATURE(PermissionedDomains, Supported::no, VoteBehavior::DefaultNo) // The following amendments are obsolete, but must remain supported diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 0cb1ec3416a..b75fb2dd84d 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -436,3 +436,16 @@ LEDGER_ENTRY(ltCREDENTIAL, 0x0081, Credential, ({ {sfPreviousTxnID, soeREQUIRED}, {sfPreviousTxnLgrSeq, soeREQUIRED}, })) + +/** A ledger object which tracks PermissionedDomain + \sa keylet::permissionedDomain + */ + +LEDGER_ENTRY(ltPERMISSIONED_DOMAIN, 0x0082, PermissionedDomain, ({ + {sfOwner, soeREQUIRED}, + {sfSequence, soeREQUIRED}, + {sfAcceptedCredentials, soeREQUIRED}, + {sfOwnerNode, soeREQUIRED}, + {sfPreviousTxnID, soeREQUIRED}, + {sfPreviousTxnLgrSeq, soeREQUIRED}, +})) diff --git a/include/xrpl/protocol/detail/sfields.macro b/include/xrpl/protocol/detail/sfields.macro index ccf6350cbfc..b51b52c9682 100644 --- a/include/xrpl/protocol/detail/sfields.macro +++ b/include/xrpl/protocol/detail/sfields.macro @@ -190,6 +190,7 @@ TYPED_SFIELD(sfHookStateKey, UINT256, 30) TYPED_SFIELD(sfHookHash, UINT256, 31) TYPED_SFIELD(sfHookNamespace, UINT256, 32) TYPED_SFIELD(sfHookSetTxnID, UINT256, 33) +TYPED_SFIELD(sfDomainID, UINT256, 34) // currency amount (common) TYPED_SFIELD(sfAmount, AMOUNT, 1) @@ -372,3 +373,4 @@ UNTYPED_SFIELD(sfPriceDataSeries, ARRAY, 24) UNTYPED_SFIELD(sfAuthAccounts, ARRAY, 25) UNTYPED_SFIELD(sfAuthorizeCredentials, ARRAY, 26) UNTYPED_SFIELD(sfUnauthorizeCredentials, ARRAY, 27) +UNTYPED_SFIELD(sfAcceptedCredentials, ARRAY, 28) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 4f4c8f12595..6925b36778a 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -447,6 +447,17 @@ TRANSACTION(ttCREDENTIAL_DELETE, 60, CredentialDelete, ({ {sfCredentialType, soeREQUIRED}, })) +/** This transaction type creates or modifies a Permissioned Domain */ +TRANSACTION(ttPERMISSIONED_DOMAIN_SET, 61, PermissionedDomainSet, ({ + {sfDomainID, soeOPTIONAL}, + {sfAcceptedCredentials, soeREQUIRED}, +})) + +/** This transaction type deletes a Permissioned Domain */ +TRANSACTION(ttPERMISSIONED_DOMAIN_DELETE, 62, PermissionedDomainDelete, ({ + {sfDomainID, soeREQUIRED}, +})) + /** This system-generated transaction type is used to update the status of the various amendments. diff --git a/include/xrpl/protocol/jss.h b/include/xrpl/protocol/jss.h index f9e0db24949..20466f3e9c7 100644 --- a/include/xrpl/protocol/jss.h +++ b/include/xrpl/protocol/jss.h @@ -96,6 +96,7 @@ JSS(OracleDocumentID); // field JSS(Owner); // field JSS(Paths); // in/out: TransactionSign JSS(PayChannel); // ledger type. +JSS(PermissionedDomain); // ledger type. JSS(PriceDataSeries); // field. JSS(PriceData); // field. JSS(Provider); // field. @@ -521,6 +522,7 @@ JSS(peers); // out: InboundLedger, handlers/Peers, Overlay JSS(peer_disconnects); // Severed peer connection counter. JSS(peer_disconnects_resources); // Severed peer connections because of // excess resource consumption. +JSS(permissioned_domain); // out: AccountObjects JSS(port); // in: Connect, out: NetworkOPs JSS(ports); // out: NetworkOPs JSS(previous); // out: Reservations diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index 12142879ad5..5905bc3f1fc 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -77,6 +77,7 @@ enum class LedgerNameSpace : std::uint16_t { MPTOKEN_ISSUANCE = '~', MPTOKEN = 't', CREDENTIAL = 'D', + PERMISSIONED_DOMAIN = 'm', // No longer used or supported. Left here to reserve the space // to avoid accidental reuse. @@ -519,6 +520,14 @@ credential( indexHash(LedgerNameSpace::CREDENTIAL, subject, issuer, credType)}; } +Keylet +permissionedDomain(AccountID const& account, std::uint32_t seq) noexcept +{ + return { + ltPERMISSIONED_DOMAIN, + indexHash(LedgerNameSpace::PERMISSIONED_DOMAIN, account, seq)}; +} + } // namespace keylet } // namespace ripple diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp new file mode 100644 index 00000000000..cacbf0b46a7 --- /dev/null +++ b/src/test/app/PermissionedDomains_test.cpp @@ -0,0 +1,478 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace ripple { +namespace test { +namespace jtx { + +class PermissionedDomains_test : public beast::unit_test::suite +{ + FeatureBitset withFeature_{ + supported_amendments() | featurePermissionedDomains}; + FeatureBitset withoutFeature_{supported_amendments()}; + + using Credential = std::pair; + using Credentials = std::vector; + + // helpers + // Make json for PermissionedDomainSet transaction + static Json::Value + setTx( + AccountID const& account, + Credentials const& credentials, + std::optional domain = std::nullopt) + { + Json::Value jv; + jv[sfTransactionType.jsonName] = jss::PermissionedDomainSet; + jv[sfAccount.jsonName] = to_string(account); + if (domain) + jv[sfDomainID.jsonName] = to_string(*domain); + Json::Value a(Json::arrayValue); + for (auto const& credential : credentials) + { + Json::Value obj(Json::objectValue); + obj[sfIssuer.jsonName] = to_string(credential.first); + obj[sfCredentialType.jsonName] = strHex( + Slice{credential.second.data(), credential.second.size()}); + Json::Value o2(Json::objectValue); + o2[sfAcceptedCredential.jsonName] = obj; + a.append(o2); + } + jv[sfAcceptedCredentials.jsonName] = a; + return jv; + } + + // Make json for PermissionedDomainDelete transaction + static Json::Value + deleteTx(AccountID const& account, uint256 const& domain) + { + Json::Value jv{Json::objectValue}; + jv[sfTransactionType.jsonName] = jss::PermissionedDomainDelete; + jv[sfAccount.jsonName] = to_string(account); + jv[sfDomainID.jsonName] = to_string(domain); + return jv; + } + + // Get PermissionedDomain objects from account_objects rpc call + static std::map + getObjects(Account const& account, Env& env) + { + std::map ret; + Json::Value params; + params[jss::account] = account.human(); + auto const& resp = + env.rpc("json", "account_objects", to_string(params)); + Json::Value a(Json::arrayValue); + a = resp[jss::result][jss::account_objects]; + for (auto const& object : a) + { + if (object["LedgerEntryType"] != "PermissionedDomain") + continue; + uint256 index; + std::ignore = index.parseHex(object[jss::index].asString()); + ret[index] = object; + } + return ret; + } + + // Convert string to Blob + static Blob + toBlob(std::string const& input) + { + Blob ret; + for (auto const& c : input) + ret.push_back(c); + return ret; + } + + // Extract credentials from account_object object + static Credentials + credentialsFromJson(Json::Value const& object) + { + Credentials ret; + Json::Value a(Json::arrayValue); + a = object["AcceptedCredentials"]; + for (auto const& credential : a) + { + Json::Value obj(Json::objectValue); + obj = credential["AcceptedCredential"]; + auto const issuer = obj["Issuer"]; + auto const credentialType = obj["CredentialType"]; + auto aid = parseBase58(issuer.asString()); + auto ct = strUnHex(credentialType.asString()); + ret.emplace_back( + *parseBase58(issuer.asString()), + strUnHex(credentialType.asString()).value()); + } + return ret; + } + + // Sort credentials the same way as PermissionedDomainSet + static Credentials + sortCredentials(Credentials const& input) + { + Credentials ret = input; + std::sort( + ret.begin(), + ret.end(), + [](Credential const& left, Credential const& right) -> bool { + return left.first < right.first; + }); + return ret; + } + + // Get account_info + static Json::Value + ownerInfo(Account const& account, Env& env) + { + Json::Value params; + params[jss::account] = account.human(); + auto const& resp = env.rpc("json", "account_info", to_string(params)); + return env.rpc( + "json", + "account_info", + to_string(params))["result"]["account_data"]; + } + + // tests + // Verify that each tx type can execute if the feature is enabled. + void + testEnabled() + { + testcase("Enabled"); + Account const alice("alice"); + Env env(*this, withFeature_); + env.fund(XRP(1000), alice); + auto const setFee(drops(env.current()->fees().increment)); + Credentials credentials{{alice, toBlob("first credential")}}; + env(setTx(alice, credentials), fee(setFee)); + BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + auto objects = getObjects(alice, env); + BEAST_EXPECT(objects.size() == 1); + auto const domain = objects.begin()->first; + env(deleteTx(alice, domain)); + } + + // Verify that each tx does not execute if feature is disabled + void + testDisabled() + { + testcase("Disabled"); + Account const alice("alice"); + Env env(*this, withoutFeature_); + env.fund(XRP(1000), alice); + auto const setFee(drops(env.current()->fees().increment)); + Credentials credentials{{alice, toBlob("first credential")}}; + env(setTx(alice, credentials), fee(setFee), ter(temDISABLED)); + env(deleteTx(alice, uint256(75)), ter(temDISABLED)); + } + + // Verify that bad inputs fail for each of create new and update + // behaviors of PermissionedDomainSet + void + testBadData( + Account const& account, + Env& env, + std::optional domain = std::nullopt) + { + Account const alice2("alice2"); + Account const alice3("alice3"); + Account const alice4("alice4"); + Account const alice5("alice5"); + Account const alice6("alice6"); + Account const alice7("alice7"); + Account const alice8("alice8"); + Account const alice9("alice9"); + Account const alice10("alice10"); + Account const alice11("alice11"); + Account const alice12("alice12"); + auto const setFee(drops(env.current()->fees().increment)); + + // Test empty credentials. + env(setTx(account, Credentials(), domain), + fee(setFee), + ter(temMALFORMED)); + + // Test 11 credentials. + Credentials const credentials11{ + {alice2, toBlob("credential1")}, + {alice3, toBlob("credential2")}, + {alice4, toBlob("credential3")}, + {alice5, toBlob("credential4")}, + {alice6, toBlob("credential5")}, + {alice7, toBlob("credential6")}, + {alice8, toBlob("credential7")}, + {alice9, toBlob("credential8")}, + {alice10, toBlob("credential9")}, + {alice11, toBlob("credential10")}, + {alice12, toBlob("credential11")}}; + BEAST_EXPECT( + credentials11.size() == PermissionedDomainSet::PD_ARRAY_MAX + 1); + env(setTx(account, credentials11, domain), + fee(setFee), + ter(temMALFORMED)); + + // Test credentials including non-existent issuer. + Account const nobody("nobody"); + Credentials const credentialsNon{ + {alice2, toBlob("credential1")}, + {alice3, toBlob("credential2")}, + {alice4, toBlob("credential3")}, + {nobody, toBlob("credential4")}, + {alice5, toBlob("credential5")}, + {alice6, toBlob("credential6")}, + {alice7, toBlob("credential7")}}; + env(setTx(account, credentialsNon, domain), + fee(setFee), + ter(temBAD_ISSUER)); + + Credentials const credentials4{ + {alice2, toBlob("credential1")}, + {alice3, toBlob("credential2")}, + {alice4, toBlob("credential3")}, + {alice5, toBlob("credential4")}, + }; + auto txJsonMutable = setTx(account, credentials4, domain); + auto const credentialOrig = txJsonMutable["AcceptedCredentials"][2u]; + + // Remove Issuer from the 3rd credential and apply. + txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] + .removeMember("Issuer"); + env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + + txJsonMutable["AcceptedCredentials"][2u] = credentialOrig; + // Remove Credentialtype from the 3rd credential and apply. + txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] + .removeMember("CredentialType"); + env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + + // Remove both + txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] + .removeMember("Issuer"); + env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + } + + // Test PermissionedDomainSet + void + testSet() + { + testcase("Set"); + Env env(*this, withFeature_); + Account const alice("alice"); + env.fund(XRP(1000), alice); + Account const alice2("alice2"); + env.fund(XRP(1000), alice2); + Account const alice3("alice3"); + env.fund(XRP(1000), alice3); + Account const alice4("alice4"); + env.fund(XRP(1000), alice4); + Account const alice5("alice5"); + env.fund(XRP(1000), alice5); + Account const alice6("alice6"); + env.fund(XRP(1000), alice6); + Account const alice7("alice7"); + env.fund(XRP(1000), alice7); + Account const alice8("alice8"); + env.fund(XRP(1000), alice8); + Account const alice9("alice9"); + env.fund(XRP(1000), alice9); + Account const alice10("alice10"); + env.fund(XRP(1000), alice10); + Account const alice11("alice11"); + env.fund(XRP(1000), alice11); + Account const alice12("alice12"); + env.fund(XRP(1000), alice12); + auto const dropsFee = env.current()->fees().increment; + auto const setFee(drops(dropsFee)); + + // Create new from existing account with a single credential. + Credentials const credentials1{{alice2, toBlob("credential1")}}; + { + env(setTx(alice, credentials1), fee(setFee)); + BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + auto tx = env.tx()->getJson(JsonOptions::none); + BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainSet"); + BEAST_EXPECT(tx["Account"] == alice.human()); + auto objects = getObjects(alice, env); + auto domain = objects.begin()->first; + auto object = objects.begin()->second; + BEAST_EXPECT(object["LedgerEntryType"] == "PermissionedDomain"); + BEAST_EXPECT(object["Owner"] == alice.human()); + BEAST_EXPECT(object["Sequence"] == tx["Sequence"]); + BEAST_EXPECT(credentialsFromJson(object) == credentials1); + } + + // Create new from existing account with 10 credentials. + Credentials const credentials10{ + {alice2, toBlob("credential1")}, + {alice3, toBlob("credential2")}, + {alice4, toBlob("credential3")}, + {alice5, toBlob("credential4")}, + {alice6, toBlob("credential5")}, + {alice7, toBlob("credential6")}, + {alice8, toBlob("credential7")}, + {alice9, toBlob("credential8")}, + {alice10, toBlob("credential9")}, + {alice11, toBlob("credential10")}, + }; + uint256 domain2; + { + BEAST_EXPECT( + credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); + BEAST_EXPECT(credentials10 != sortCredentials(credentials10)); + env(setTx(alice, credentials10), fee(setFee)); + auto tx = env.tx()->getJson(JsonOptions::none); + auto meta = env.meta()->getJson(JsonOptions::none); + Json::Value a(Json::arrayValue); + a = meta["AffectedNodes"]; + + for (auto const& node : a) + { + if (!node.isMember("CreatedNode") || + node["CreatedNode"]["LedgerEntryType"] != + "PermissionedDomain") + { + continue; + } + std::ignore = domain2.parseHex( + node["CreatedNode"]["LedgerIndex"].asString()); + } + auto objects = getObjects(alice, env); + auto object = objects[domain2]; + BEAST_EXPECT( + credentialsFromJson(object) == sortCredentials(credentials10)); + } + + // Make a new domain with insufficient fee. + env(setTx(alice, credentials10), + fee(drops(dropsFee - 1)), + ter(telINSUF_FEE_P)); + + // Update with 1 credential. + env(setTx(alice, credentials1, domain2)); + BEAST_EXPECT( + credentialsFromJson(getObjects(alice, env)[domain2]) == + credentials1); + + // Update with 10 credentials. + env(setTx(alice, credentials10, domain2)); + env.close(); + BEAST_EXPECT( + credentialsFromJson(getObjects(alice, env)[domain2]) == + sortCredentials(credentials10)); + + // Test bad data when creating a domain. + testBadData(alice, env); + // Test bad data when updating a domain. + testBadData(alice, env, domain2); + + // Try to delete the account with domains. + auto const acctDelFee(drops(env.current()->fees().increment)); + constexpr std::size_t deleteDelta = 255; + { + // Close enough ledgers to make it potentially deletable if empty. + std::size_t ownerSeq = ownerInfo(alice, env)["Sequence"].asUInt(); + while (deleteDelta + ownerSeq > env.current()->seq()) + env.close(); + env(acctdelete(alice, alice2), + fee(acctDelFee), + ter(tecHAS_OBLIGATIONS)); + } + + { + // Delete the domains and then the owner account. + for (auto const& objs : getObjects(alice, env)) + env(deleteTx(alice, objs.first)); + env.close(); + std::size_t ownerSeq = ownerInfo(alice, env)["Sequence"].asUInt(); + while (deleteDelta + ownerSeq > env.current()->seq()) + env.close(); + env(acctdelete(alice, alice2), fee(acctDelFee)); + } + } + + // Test PermissionedDomainDelete + void + testDelete() + { + testcase("Delete"); + Env env(*this, withFeature_); + Account const alice("alice"); + + env.fund(XRP(1000), alice); + auto const setFee(drops(env.current()->fees().increment)); + Credentials credentials{{alice, toBlob("first credential")}}; + env(setTx(alice, credentials), fee(setFee)); + env.close(); + auto objects = getObjects(alice, env); + BEAST_EXPECT(objects.size() == 1); + auto const domain = objects.begin()->first; + + // Delete a domain that doesn't belong to the account. + Account const bob("bob"); + env.fund(XRP(1000), bob); + env(deleteTx(bob, domain), ter(temINVALID_ACCOUNT_ID)); + + // Delete a non-existent domain. + env(deleteTx(alice, uint256(75)), ter(tecNO_ENTRY)); + + // Delete a zero domain. + env(deleteTx(alice, uint256(0)), ter(temMALFORMED)); + + // Make sure owner count reflects the existing domain. + BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + // Delete domain that belongs to user. + env(deleteTx(alice, domain), ter(tesSUCCESS)); + auto const tx = env.tx()->getJson(JsonOptions::none); + BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainDelete"); + // Make sure the owner count goes back to 0. + BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); + } + +public: + void + run() override + { + testEnabled(); + testDisabled(); + testSet(); + testDelete(); + } +}; + +BEAST_DEFINE_TESTSUITE_PRIO(PermissionedDomains, app, ripple, 2); + +} // namespace jtx +} // namespace test +} // namespace ripple diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 8d7b08fa1ab..765b47e9796 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -798,6 +798,46 @@ class Invariants_test : public beast::unit_test::suite }); } + void + testPermissionedDomainInvariants() + { + using namespace test::jtx; + testcase << "PermissionedDomain"; + doInvariantCheck( + {{"permissioned domain with no rules"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); + auto slePd = std::make_shared(pdKeylet); + slePd->setAccountID(sfOwner, A1); + slePd->setFieldU32(sfSequence, 10); + + ac.view().insert(slePd); + return true; + }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + + doInvariantCheck( + {{"permissioned domain bad credentials size 11"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); + auto slePd = std::make_shared(pdKeylet); + slePd->setAccountID(sfOwner, A1); + slePd->setFieldU32(sfSequence, 10); + + STArray credentials(sfAcceptedCredentials); + for (std::size_t n = 0; n < 11; ++n) + credentials.push_back(STObject(sfSequence)); + slePd->setFieldArray(sfAcceptedCredentials, credentials); + ac.view().insert(slePd); + return true; + }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + } + public: void run() override @@ -813,6 +853,7 @@ class Invariants_test : public beast::unit_test::suite testNoZeroEscrow(); testValidNewAccountRoot(); testNFTokenPageInvariants(); + testPermissionedDomainInvariants(); } }; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 90fc399b344..2002c8911b3 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -482,6 +483,7 @@ LedgerEntryTypesMatch::visitEntry( case ltMPTOKEN_ISSUANCE: case ltMPTOKEN: case ltCREDENTIAL: + case ltPERMISSIONED_DOMAIN: break; default: invalidTypeAdded_ = true; @@ -1120,4 +1122,46 @@ ValidMPTIssuance::finalize( mptokensCreated_ == 0 && mptokensDeleted_ == 0; } +//------------------------------------------------------------------------------ + +void +ValidPermissionedDomain::visitEntry( + bool, + std::shared_ptr const& before, + std::shared_ptr const& after) +{ + if (after->getType() != ltPERMISSIONED_DOMAIN) + return; + credentialsSize_ = after->getFieldArray(sfAcceptedCredentials).size(); +} + +bool +ValidPermissionedDomain::finalize( + STTx const& tx, + TER const result, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) +{ + if (tx.getTxnType() != ttPERMISSIONED_DOMAIN_SET || result != tesSUCCESS) + return true; + + if (!credentialsSize_) + { + JLOG(j.fatal()) << "Invariant failed: permissioned domain with " + "no rules."; + return false; + } + + if (credentialsSize_ > PermissionedDomainSet::PD_ARRAY_MAX) + { + JLOG(j.fatal()) << "Invariant failed: permissioned domain bad " + "credentials size " + << credentialsSize_; + return false; + } + + return true; +} + } // namespace ripple diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 23ec8005556..13ccc5fb179 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -28,6 +28,7 @@ #include #include +#include #include #include @@ -475,6 +476,33 @@ class ValidMPTIssuance beast::Journal const&); }; +/** + * @brief Invariants: Permissioned Domains must have some rules and + * AcceptedCredentials must have length between 1 and 10 inclusive. + * + * Since only permissions constitute rules, an empty credentials list + * means that there are no rules and the invariant is violated. + */ +class ValidPermissionedDomain +{ + std::size_t credentialsSize_{0}; + +public: + void + visitEntry( + bool, + std::shared_ptr const&, + std::shared_ptr const&); + + bool + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); +}; + // additional invariant checks can be declared above and then added to this // tuple using InvariantChecks = std::tuple< @@ -491,7 +519,8 @@ using InvariantChecks = std::tuple< ValidNFTokenPage, NFTokenCountTracking, ValidClawback, - ValidMPTIssuance>; + ValidMPTIssuance, + ValidPermissionedDomain>; /** * @brief get a tuple of all invariant checks diff --git a/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp new file mode 100644 index 00000000000..b737af5e570 --- /dev/null +++ b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp @@ -0,0 +1,75 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include + +namespace ripple { + +NotTEC +PermissionedDomainDelete::preflight(PreflightContext const& ctx) +{ + if (!ctx.rules.enabled(featurePermissionedDomains)) + return temDISABLED; + if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) + return ret; + if (!ctx.tx.isFieldPresent(sfDomainID)) + return temMALFORMED; + return preflight2(ctx); +} + +TER +PermissionedDomainDelete::preclaim(PreclaimContext const& ctx) +{ + assert(ctx.tx.isFieldPresent(sfDomainID)); + auto const domain = ctx.tx.getFieldH256(sfDomainID); + if (domain == beast::zero) + return temMALFORMED; + auto const sleDomain = ctx.view.read({ltPERMISSIONED_DOMAIN, domain}); + if (!sleDomain) + return tecNO_ENTRY; + assert( + sleDomain->isFieldPresent(sfOwner) && ctx.tx.isFieldPresent(sfAccount)); + if (sleDomain->getAccountID(sfOwner) != ctx.tx.getAccountID(sfAccount)) + return temINVALID_ACCOUNT_ID; + return tesSUCCESS; +} + +/** Attempt to delete the Permissioned Domain. */ +TER +PermissionedDomainDelete::doApply() +{ + assert(ctx_.tx.isFieldPresent(sfDomainID)); + auto const slePd = + view().peek({ltPERMISSIONED_DOMAIN, ctx_.tx.at(sfDomainID)}); + auto const page{(*slePd)[sfOwnerNode]}; + if (!view().dirRemove(keylet::ownerDir(account_), page, slePd->key(), true)) + { + JLOG(j_.fatal()) + << "Unable to delete permissioned domain directory entry."; + return tefBAD_LEDGER; + } + auto const ownerSle = view().peek(keylet::account(account_)); + assert(ownerSle && ownerSle->getFieldU32(sfOwnerCount) > 0); + adjustOwnerCount(view(), ownerSle, -1, ctx_.journal); + view().erase(slePd); + return tesSUCCESS; +} + +} // namespace ripple diff --git a/src/xrpld/app/tx/detail/PermissionedDomainDelete.h b/src/xrpld/app/tx/detail/PermissionedDomainDelete.h new file mode 100644 index 00000000000..3fdf50ed298 --- /dev/null +++ b/src/xrpld/app/tx/detail/PermissionedDomainDelete.h @@ -0,0 +1,49 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TX_PERMISSIONEDDOMAINDELETE_H_INCLUDED +#define RIPPLE_TX_PERMISSIONEDDOMAINDELETE_H_INCLUDED + +#include + +namespace ripple { + +class PermissionedDomainDelete : public Transactor +{ +public: + static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; + + explicit PermissionedDomainDelete(ApplyContext& ctx) : Transactor(ctx) + { + } + + static NotTEC + preflight(PreflightContext const& ctx); + + static TER + preclaim(PreclaimContext const& ctx); + + /** Attempt to create the Permissioned Domain. */ + TER + doApply() override; +}; + +} // namespace ripple + +#endif // RIPPLE_TX_PERMISSIONEDDOMAINDELETE_H_INCLUDED diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp new file mode 100644 index 00000000000..e7cf94f0b64 --- /dev/null +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -0,0 +1,147 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include + +#include + +namespace ripple { + +NotTEC +PermissionedDomainSet::preflight(PreflightContext const& ctx) +{ + if (!ctx.rules.enabled(featurePermissionedDomains)) + return temDISABLED; + if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) + return ret; + + if (!ctx.tx.isFieldPresent(sfAcceptedCredentials)) + return temMALFORMED; + auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); + // TODO check to see if we should disallow duplicate issuers. + // If so, it probably means sorting on the CredentialType field + // for identical issuers. + if (credentials.empty() || credentials.size() > PD_ARRAY_MAX) + return temMALFORMED; + + return preflight2(ctx); +} + +XRPAmount +PermissionedDomainSet::calculateBaseFee(ReadView const& view, STTx const& tx) +{ + if (tx.isFieldPresent(sfDomainID)) + return Transactor::calculateBaseFee(view, tx); + // The fee required for a new PermissionedDomain is one owner reserve. + return view.fees().increment; +} + +TER +PermissionedDomainSet::preclaim(PreclaimContext const& ctx) +{ + if (!ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount)))) + return tefINTERNAL; + + assert(ctx.tx.isFieldPresent(sfAcceptedCredentials)); + auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); + for (auto const& credential : credentials) + { + if (!credential.isFieldPresent(sfIssuer) || + !credential.isFieldPresent(sfCredentialType)) + { + return temMALFORMED; + } + if (!ctx.view.read(keylet::account(credential.getAccountID(sfIssuer)))) + return temBAD_ISSUER; + } + + if (!ctx.tx.isFieldPresent(sfDomainID)) + return tesSUCCESS; + auto const domain = ctx.tx.getFieldH256(sfDomainID); + if (domain == beast::zero) + return temMALFORMED; + auto const sleDomain = ctx.view.read({ltPERMISSIONED_DOMAIN, domain}); + if (!sleDomain) + return tecNO_ENTRY; + auto const owner = sleDomain->getAccountID(sfOwner); + auto account = ctx.tx.getAccountID(sfAccount); + if (owner != account) + return temINVALID_ACCOUNT_ID; + + return tesSUCCESS; +} + +/** Attempt to create the Permissioned Domain. */ +TER +PermissionedDomainSet::doApply() +{ + auto const ownerSle = view().peek(keylet::account(account_)); + + // All checks have already been done. Just update credentials. Same logic + // for either new domain or updating existing. + auto updateSle = [this](std::shared_ptr const& sle) { + auto credentials = ctx_.tx.getFieldArray(sfAcceptedCredentials); + // TODO when credentials are merged, it is possible that this will + // also need to sort on the CredentialType field in case there + // are multiple issuers in each set of credentials. The spec + // needs to be ironed out. + credentials.sort( + [](STObject const& left, STObject const& right) -> bool { + return dynamic_cast(&left)->getAccountID( + sfIssuer) < + dynamic_cast(&right)->getAccountID( + sfIssuer); + }); + sle->setFieldArray(sfAcceptedCredentials, credentials); + }; + + if (ctx_.tx.isFieldPresent(sfDomainID)) + { + // Modify existing permissioned domain. + auto sleUpdate = view().peek( + {ltPERMISSIONED_DOMAIN, ctx_.tx.getFieldH256(sfDomainID)}); + updateSle(sleUpdate); + view().update(sleUpdate); + } + else + { + // Create new permissioned domain. + Keylet const pdKeylet = keylet::permissionedDomain( + account_, ctx_.tx.getFieldU32(sfSequence)); + auto slePd = std::make_shared(pdKeylet); + slePd->setAccountID(sfOwner, account_); + slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); + updateSle(slePd); + view().insert(slePd); + auto const page = view().dirInsert( + keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_)); + if (!page) + return tecDIR_FULL; + slePd->setFieldU64(sfOwnerNode, *page); + // If we succeeded, the new entry counts against the creator's reserve. + adjustOwnerCount(view(), ownerSle, 1, ctx_.journal); + } + + return tesSUCCESS; +} + +} // namespace ripple diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.h b/src/xrpld/app/tx/detail/PermissionedDomainSet.h new file mode 100644 index 00000000000..e1f105b1d15 --- /dev/null +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.h @@ -0,0 +1,54 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TX_PERMISSIONEDDOMAINSET_H_INCLUDED +#define RIPPLE_TX_PERMISSIONEDDOMAINSET_H_INCLUDED + +#include +#include + +namespace ripple { + +class PermissionedDomainSet : public Transactor +{ +public: + static constexpr std::size_t PD_ARRAY_MAX = 10; + static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; + + explicit PermissionedDomainSet(ApplyContext& ctx) : Transactor(ctx) + { + } + + static NotTEC + preflight(PreflightContext const& ctx); + + static XRPAmount + calculateBaseFee(ReadView const& view, STTx const& tx); + + static TER + preclaim(PreclaimContext const& ctx); + + /** Attempt to create the Permissioned Domain. */ + TER + doApply() override; +}; + +} // namespace ripple + +#endif // RIPPLE_TX_PERMISSIONEDDOMAINSET_H_INCLUDED diff --git a/src/xrpld/app/tx/detail/applySteps.cpp b/src/xrpld/app/tx/detail/applySteps.cpp index b3c711084dc..9ce776931a5 100644 --- a/src/xrpld/app/tx/detail/applySteps.cpp +++ b/src/xrpld/app/tx/detail/applySteps.cpp @@ -52,6 +52,8 @@ #include #include #include +#include +#include #include #include #include diff --git a/src/xrpld/rpc/handlers/AccountObjects.cpp b/src/xrpld/rpc/handlers/AccountObjects.cpp index 538b1d79424..1bdd95a74ca 100644 --- a/src/xrpld/rpc/handlers/AccountObjects.cpp +++ b/src/xrpld/rpc/handlers/AccountObjects.cpp @@ -224,7 +224,8 @@ doAccountObjects(RPC::JsonContext& context) ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID}, {jss::bridge, ltBRIDGE}, {jss::mpt_issuance, ltMPTOKEN_ISSUANCE}, - {jss::mptoken, ltMPTOKEN}}; + {jss::mptoken, ltMPTOKEN}, + {jss::permissioned_domain, ltPERMISSIONED_DOMAIN}}; typeFilter.emplace(); typeFilter->reserve(std::size(deletionBlockers)); From e1718e762f8018f79b0c2e05667f3183bb6798ec Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Tue, 1 Oct 2024 15:10:24 -0700 Subject: [PATCH 02/11] Review fixes from Olek. --- src/test/app/PermissionedDomains_test.cpp | 352 +++++++----------- src/test/jtx/PermissionedDomains.h | 78 ++++ src/test/jtx/impl/PermissionedDomains.cpp | 151 ++++++++ .../tx/detail/PermissionedDomainDelete.cpp | 2 - .../app/tx/detail/PermissionedDomainSet.cpp | 46 ++- 5 files changed, 390 insertions(+), 239 deletions(-) create mode 100644 src/test/jtx/PermissionedDomains.h create mode 100644 src/test/jtx/impl/PermissionedDomains.cpp diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index cacbf0b46a7..e96c88d55e9 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include #include @@ -26,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -42,130 +44,6 @@ class PermissionedDomains_test : public beast::unit_test::suite supported_amendments() | featurePermissionedDomains}; FeatureBitset withoutFeature_{supported_amendments()}; - using Credential = std::pair; - using Credentials = std::vector; - - // helpers - // Make json for PermissionedDomainSet transaction - static Json::Value - setTx( - AccountID const& account, - Credentials const& credentials, - std::optional domain = std::nullopt) - { - Json::Value jv; - jv[sfTransactionType.jsonName] = jss::PermissionedDomainSet; - jv[sfAccount.jsonName] = to_string(account); - if (domain) - jv[sfDomainID.jsonName] = to_string(*domain); - Json::Value a(Json::arrayValue); - for (auto const& credential : credentials) - { - Json::Value obj(Json::objectValue); - obj[sfIssuer.jsonName] = to_string(credential.first); - obj[sfCredentialType.jsonName] = strHex( - Slice{credential.second.data(), credential.second.size()}); - Json::Value o2(Json::objectValue); - o2[sfAcceptedCredential.jsonName] = obj; - a.append(o2); - } - jv[sfAcceptedCredentials.jsonName] = a; - return jv; - } - - // Make json for PermissionedDomainDelete transaction - static Json::Value - deleteTx(AccountID const& account, uint256 const& domain) - { - Json::Value jv{Json::objectValue}; - jv[sfTransactionType.jsonName] = jss::PermissionedDomainDelete; - jv[sfAccount.jsonName] = to_string(account); - jv[sfDomainID.jsonName] = to_string(domain); - return jv; - } - - // Get PermissionedDomain objects from account_objects rpc call - static std::map - getObjects(Account const& account, Env& env) - { - std::map ret; - Json::Value params; - params[jss::account] = account.human(); - auto const& resp = - env.rpc("json", "account_objects", to_string(params)); - Json::Value a(Json::arrayValue); - a = resp[jss::result][jss::account_objects]; - for (auto const& object : a) - { - if (object["LedgerEntryType"] != "PermissionedDomain") - continue; - uint256 index; - std::ignore = index.parseHex(object[jss::index].asString()); - ret[index] = object; - } - return ret; - } - - // Convert string to Blob - static Blob - toBlob(std::string const& input) - { - Blob ret; - for (auto const& c : input) - ret.push_back(c); - return ret; - } - - // Extract credentials from account_object object - static Credentials - credentialsFromJson(Json::Value const& object) - { - Credentials ret; - Json::Value a(Json::arrayValue); - a = object["AcceptedCredentials"]; - for (auto const& credential : a) - { - Json::Value obj(Json::objectValue); - obj = credential["AcceptedCredential"]; - auto const issuer = obj["Issuer"]; - auto const credentialType = obj["CredentialType"]; - auto aid = parseBase58(issuer.asString()); - auto ct = strUnHex(credentialType.asString()); - ret.emplace_back( - *parseBase58(issuer.asString()), - strUnHex(credentialType.asString()).value()); - } - return ret; - } - - // Sort credentials the same way as PermissionedDomainSet - static Credentials - sortCredentials(Credentials const& input) - { - Credentials ret = input; - std::sort( - ret.begin(), - ret.end(), - [](Credential const& left, Credential const& right) -> bool { - return left.first < right.first; - }); - return ret; - } - - // Get account_info - static Json::Value - ownerInfo(Account const& account, Env& env) - { - Json::Value params; - params[jss::account] = account.human(); - auto const& resp = env.rpc("json", "account_info", to_string(params)); - return env.rpc( - "json", - "account_info", - to_string(params))["result"]["account_data"]; - } - - // tests // Verify that each tx type can execute if the feature is enabled. void testEnabled() @@ -175,13 +53,13 @@ class PermissionedDomains_test : public beast::unit_test::suite Env env(*this, withFeature_); env.fund(XRP(1000), alice); auto const setFee(drops(env.current()->fees().increment)); - Credentials credentials{{alice, toBlob("first credential")}}; - env(setTx(alice, credentials), fee(setFee)); - BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); - auto objects = getObjects(alice, env); + pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + env(pd::setTx(alice, credentials), fee(setFee)); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + auto objects = pd::getObjects(alice, env); BEAST_EXPECT(objects.size() == 1); auto const domain = objects.begin()->first; - env(deleteTx(alice, domain)); + env(pd::deleteTx(alice, domain)); } // Verify that each tx does not execute if feature is disabled @@ -193,9 +71,9 @@ class PermissionedDomains_test : public beast::unit_test::suite Env env(*this, withoutFeature_); env.fund(XRP(1000), alice); auto const setFee(drops(env.current()->fees().increment)); - Credentials credentials{{alice, toBlob("first credential")}}; - env(setTx(alice, credentials), fee(setFee), ter(temDISABLED)); - env(deleteTx(alice, uint256(75)), ter(temDISABLED)); + pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + env(pd::setTx(alice, credentials), fee(setFee), ter(temDISABLED)); + env(pd::deleteTx(alice, uint256(75)), ter(temDISABLED)); } // Verify that bad inputs fail for each of create new and update @@ -220,59 +98,64 @@ class PermissionedDomains_test : public beast::unit_test::suite auto const setFee(drops(env.current()->fees().increment)); // Test empty credentials. - env(setTx(account, Credentials(), domain), + env(pd::setTx(account, pd::Credentials(), domain), fee(setFee), ter(temMALFORMED)); // Test 11 credentials. - Credentials const credentials11{ - {alice2, toBlob("credential1")}, - {alice3, toBlob("credential2")}, - {alice4, toBlob("credential3")}, - {alice5, toBlob("credential4")}, - {alice6, toBlob("credential5")}, - {alice7, toBlob("credential6")}, - {alice8, toBlob("credential7")}, - {alice9, toBlob("credential8")}, - {alice10, toBlob("credential9")}, - {alice11, toBlob("credential10")}, - {alice12, toBlob("credential11")}}; + pd::Credentials const credentials11{ + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice4, pd::toBlob("credential3")}, + {alice5, pd::toBlob("credential4")}, + {alice6, pd::toBlob("credential5")}, + {alice7, pd::toBlob("credential6")}, + {alice8, pd::toBlob("credential7")}, + {alice9, pd::toBlob("credential8")}, + {alice10, pd::toBlob("credential9")}, + {alice11, pd::toBlob("credential10")}, + {alice12, pd::toBlob("credential11")}}; BEAST_EXPECT( credentials11.size() == PermissionedDomainSet::PD_ARRAY_MAX + 1); - env(setTx(account, credentials11, domain), + env(pd::setTx(account, credentials11, domain), fee(setFee), ter(temMALFORMED)); // Test credentials including non-existent issuer. Account const nobody("nobody"); - Credentials const credentialsNon{ - {alice2, toBlob("credential1")}, - {alice3, toBlob("credential2")}, - {alice4, toBlob("credential3")}, - {nobody, toBlob("credential4")}, - {alice5, toBlob("credential5")}, - {alice6, toBlob("credential6")}, - {alice7, toBlob("credential7")}}; - env(setTx(account, credentialsNon, domain), + pd::Credentials const credentialsNon{ + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice4, pd::toBlob("credential3")}, + {nobody, pd::toBlob("credential4")}, + {alice5, pd::toBlob("credential5")}, + {alice6, pd::toBlob("credential6")}, + {alice7, pd::toBlob("credential7")}}; + env(pd::setTx(account, credentialsNon, domain), fee(setFee), ter(temBAD_ISSUER)); - Credentials const credentials4{ - {alice2, toBlob("credential1")}, - {alice3, toBlob("credential2")}, - {alice4, toBlob("credential3")}, - {alice5, toBlob("credential4")}, + pd::Credentials const credentials4{ + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice4, pd::toBlob("credential3")}, + {alice5, pd::toBlob("credential4")}, }; - auto txJsonMutable = setTx(account, credentials4, domain); + auto txJsonMutable = pd::setTx(account, credentials4, domain); auto const credentialOrig = txJsonMutable["AcceptedCredentials"][2u]; - // Remove Issuer from the 3rd credential and apply. + // Remove Issuer from a credential and apply. txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] .removeMember("Issuer"); env(txJsonMutable, fee(setFee), ter(temMALFORMED)); txJsonMutable["AcceptedCredentials"][2u] = credentialOrig; - // Remove Credentialtype from the 3rd credential and apply. + // Make an empty CredentialType. + txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] + ["CredentialType"] = ""; + env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + + // Remove Credentialtype from a credential and apply. txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] .removeMember("CredentialType"); env(txJsonMutable, fee(setFee), ter(temMALFORMED)); @@ -281,6 +164,17 @@ class PermissionedDomains_test : public beast::unit_test::suite txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] .removeMember("Issuer"); env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + + // Make 2 identical credentials. + pd::Credentials const credentialsDup{ + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice2, pd::toBlob("credential1")}, + {alice5, pd::toBlob("credential4")}, + }; + env(pd::setTx(account, credentialsDup, domain), + fee(setFee), + ter(temMALFORMED)); } // Test PermissionedDomainSet @@ -289,69 +183,63 @@ class PermissionedDomains_test : public beast::unit_test::suite { testcase("Set"); Env env(*this, withFeature_); - Account const alice("alice"); - env.fund(XRP(1000), alice); - Account const alice2("alice2"); - env.fund(XRP(1000), alice2); - Account const alice3("alice3"); - env.fund(XRP(1000), alice3); - Account const alice4("alice4"); - env.fund(XRP(1000), alice4); - Account const alice5("alice5"); - env.fund(XRP(1000), alice5); - Account const alice6("alice6"); - env.fund(XRP(1000), alice6); - Account const alice7("alice7"); - env.fund(XRP(1000), alice7); - Account const alice8("alice8"); - env.fund(XRP(1000), alice8); - Account const alice9("alice9"); - env.fund(XRP(1000), alice9); - Account const alice10("alice10"); - env.fund(XRP(1000), alice10); - Account const alice11("alice11"); - env.fund(XRP(1000), alice11); - Account const alice12("alice12"); - env.fund(XRP(1000), alice12); + Account const alice("alice"), alice2("alice2"), alice3("alice3"), + alice4("alice4"), alice5("alice5"), alice6("alice6"), + alice7("alice7"), alice8("alice8"), alice9("alice9"), + alice10("alice10"), alice11("alice11"), alice12("alice12"); + env.fund( + XRP(1000), + alice, + alice2, + alice3, + alice4, + alice5, + alice6, + alice7, + alice8, + alice9, + alice10, + alice11, + alice12); auto const dropsFee = env.current()->fees().increment; auto const setFee(drops(dropsFee)); // Create new from existing account with a single credential. - Credentials const credentials1{{alice2, toBlob("credential1")}}; + pd::Credentials const credentials1{{alice2, pd::toBlob("credential1")}}; { - env(setTx(alice, credentials1), fee(setFee)); - BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + env(pd::setTx(alice, credentials1), fee(setFee)); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); auto tx = env.tx()->getJson(JsonOptions::none); BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainSet"); BEAST_EXPECT(tx["Account"] == alice.human()); - auto objects = getObjects(alice, env); + auto objects = pd::getObjects(alice, env); auto domain = objects.begin()->first; auto object = objects.begin()->second; BEAST_EXPECT(object["LedgerEntryType"] == "PermissionedDomain"); BEAST_EXPECT(object["Owner"] == alice.human()); BEAST_EXPECT(object["Sequence"] == tx["Sequence"]); - BEAST_EXPECT(credentialsFromJson(object) == credentials1); + BEAST_EXPECT(pd::credentialsFromJson(object) == credentials1); } // Create new from existing account with 10 credentials. - Credentials const credentials10{ - {alice2, toBlob("credential1")}, - {alice3, toBlob("credential2")}, - {alice4, toBlob("credential3")}, - {alice5, toBlob("credential4")}, - {alice6, toBlob("credential5")}, - {alice7, toBlob("credential6")}, - {alice8, toBlob("credential7")}, - {alice9, toBlob("credential8")}, - {alice10, toBlob("credential9")}, - {alice11, toBlob("credential10")}, + pd::Credentials const credentials10{ + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice4, pd::toBlob("credential3")}, + {alice5, pd::toBlob("credential4")}, + {alice6, pd::toBlob("credential5")}, + {alice7, pd::toBlob("credential6")}, + {alice8, pd::toBlob("credential7")}, + {alice9, pd::toBlob("credential8")}, + {alice10, pd::toBlob("credential9")}, + {alice11, pd::toBlob("credential10")}, }; uint256 domain2; { BEAST_EXPECT( credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); - BEAST_EXPECT(credentials10 != sortCredentials(credentials10)); - env(setTx(alice, credentials10), fee(setFee)); + BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); + env(pd::setTx(alice, credentials10), fee(setFee)); auto tx = env.tx()->getJson(JsonOptions::none); auto meta = env.meta()->getJson(JsonOptions::none); Json::Value a(Json::arrayValue); @@ -368,29 +256,34 @@ class PermissionedDomains_test : public beast::unit_test::suite std::ignore = domain2.parseHex( node["CreatedNode"]["LedgerIndex"].asString()); } - auto objects = getObjects(alice, env); + auto objects = pd::getObjects(alice, env); auto object = objects[domain2]; BEAST_EXPECT( - credentialsFromJson(object) == sortCredentials(credentials10)); + pd::credentialsFromJson(object) == + pd::sortCredentials(credentials10)); } // Make a new domain with insufficient fee. - env(setTx(alice, credentials10), + env(pd::setTx(alice, credentials10), fee(drops(dropsFee - 1)), ter(telINSUF_FEE_P)); // Update with 1 credential. - env(setTx(alice, credentials1, domain2)); + env(pd::setTx(alice, credentials1, domain2)); BEAST_EXPECT( - credentialsFromJson(getObjects(alice, env)[domain2]) == + pd::credentialsFromJson(pd::getObjects(alice, env)[domain2]) == credentials1); // Update with 10 credentials. - env(setTx(alice, credentials10, domain2)); + env(pd::setTx(alice, credentials10, domain2)); env.close(); BEAST_EXPECT( - credentialsFromJson(getObjects(alice, env)[domain2]) == - sortCredentials(credentials10)); + pd::credentialsFromJson(pd::getObjects(alice, env)[domain2]) == + pd::sortCredentials(credentials10)); + + // Update from the wrong owner. + env(pd::setTx(alice2, credentials1, domain2), + ter(temINVALID_ACCOUNT_ID)); // Test bad data when creating a domain. testBadData(alice, env); @@ -402,7 +295,8 @@ class PermissionedDomains_test : public beast::unit_test::suite constexpr std::size_t deleteDelta = 255; { // Close enough ledgers to make it potentially deletable if empty. - std::size_t ownerSeq = ownerInfo(alice, env)["Sequence"].asUInt(); + std::size_t ownerSeq = + pd::ownerInfo(alice, env)["Sequence"].asUInt(); while (deleteDelta + ownerSeq > env.current()->seq()) env.close(); env(acctdelete(alice, alice2), @@ -412,10 +306,11 @@ class PermissionedDomains_test : public beast::unit_test::suite { // Delete the domains and then the owner account. - for (auto const& objs : getObjects(alice, env)) - env(deleteTx(alice, objs.first)); + for (auto const& objs : pd::getObjects(alice, env)) + env(pd::deleteTx(alice, objs.first)); env.close(); - std::size_t ownerSeq = ownerInfo(alice, env)["Sequence"].asUInt(); + std::size_t ownerSeq = + pd::ownerInfo(alice, env)["Sequence"].asUInt(); while (deleteDelta + ownerSeq > env.current()->seq()) env.close(); env(acctdelete(alice, alice2), fee(acctDelFee)); @@ -432,32 +327,37 @@ class PermissionedDomains_test : public beast::unit_test::suite env.fund(XRP(1000), alice); auto const setFee(drops(env.current()->fees().increment)); - Credentials credentials{{alice, toBlob("first credential")}}; - env(setTx(alice, credentials), fee(setFee)); + pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + env(pd::setTx(alice, credentials), fee(setFee)); env.close(); - auto objects = getObjects(alice, env); + auto objects = pd::getObjects(alice, env); BEAST_EXPECT(objects.size() == 1); auto const domain = objects.begin()->first; // Delete a domain that doesn't belong to the account. Account const bob("bob"); env.fund(XRP(1000), bob); - env(deleteTx(bob, domain), ter(temINVALID_ACCOUNT_ID)); + env(pd::deleteTx(bob, domain), ter(temINVALID_ACCOUNT_ID)); // Delete a non-existent domain. - env(deleteTx(alice, uint256(75)), ter(tecNO_ENTRY)); + env(pd::deleteTx(alice, uint256(75)), ter(tecNO_ENTRY)); // Delete a zero domain. - env(deleteTx(alice, uint256(0)), ter(temMALFORMED)); + env(pd::deleteTx(alice, uint256(0)), ter(temMALFORMED)); // Make sure owner count reflects the existing domain. - BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + auto const objID = pd::getObjects(alice, env).begin()->first; + BEAST_EXPECT(pd::objectExists(objID, env)); // Delete domain that belongs to user. - env(deleteTx(alice, domain), ter(tesSUCCESS)); + env(pd::deleteTx(alice, domain), ter(tesSUCCESS)); auto const tx = env.tx()->getJson(JsonOptions::none); BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainDelete"); // Make sure the owner count goes back to 0. - BEAST_EXPECT(ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); + // The object needs to be gone. + BEAST_EXPECT(pd::getObjects(alice, env).empty()); + BEAST_EXPECT(!pd::objectExists(objID, env)); } public: diff --git a/src/test/jtx/PermissionedDomains.h b/src/test/jtx/PermissionedDomains.h new file mode 100644 index 00000000000..ae7b673a946 --- /dev/null +++ b/src/test/jtx/PermissionedDomains.h @@ -0,0 +1,78 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED +#define RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED + +#include + +namespace ripple { +namespace test { +namespace jtx { +namespace pd { + +// Helpers for PermissionedDomains testing +using Credential = std::pair; +using Credentials = std::vector; + +// helpers +// Make json for PermissionedDomainSet transaction +Json::Value +setTx( + AccountID const& account, + Credentials const& credentials, + std::optional domain = std::nullopt); + +// Make json for PermissionedDomainDelete transaction +Json::Value +deleteTx(AccountID const& account, uint256 const& domain); + +// Get PermissionedDomain objects from account_objects rpc call +std::map +getObjects(Account const& account, Env& env); + +// Check if ledger object is there +bool +objectExists(uint256 const& objID, Env& env); + +// Convert string to Blob +inline Blob +toBlob(std::string const& input) +{ + return Blob(input.begin(), input.end()); +} + +// Extract credentials from account_object object +Credentials +credentialsFromJson(Json::Value const& object); + +// Sort credentials the same way as PermissionedDomainSet +Credentials +sortCredentials(Credentials const& input); + +// Get account_info +Json::Value +ownerInfo(Account const& account, Env& env); + +} // namespace pd +} // namespace jtx +} // namespace test +} // namespace ripple + +#endif // RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/PermissionedDomains.cpp new file mode 100644 index 00000000000..7b27263e990 --- /dev/null +++ b/src/test/jtx/impl/PermissionedDomains.cpp @@ -0,0 +1,151 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include + +namespace ripple { +namespace test { +namespace jtx { +namespace pd { + +// helpers +// Make json for PermissionedDomainSet transaction +Json::Value +setTx( + AccountID const& account, + Credentials const& credentials, + std::optional domain) +{ + Json::Value jv; + jv[sfTransactionType.jsonName] = jss::PermissionedDomainSet; + jv[sfAccount.jsonName] = to_string(account); + if (domain) + jv[sfDomainID.jsonName] = to_string(*domain); + Json::Value a(Json::arrayValue); + for (auto const& credential : credentials) + { + Json::Value obj(Json::objectValue); + obj[sfIssuer.jsonName] = to_string(credential.first); + obj[sfCredentialType.jsonName] = + strHex(Slice{credential.second.data(), credential.second.size()}); + Json::Value o2(Json::objectValue); + o2[sfAcceptedCredential.jsonName] = obj; + a.append(o2); + } + jv[sfAcceptedCredentials.jsonName] = a; + return jv; +} + +// Make json for PermissionedDomainDelete transaction +Json::Value +deleteTx(AccountID const& account, uint256 const& domain) +{ + Json::Value jv{Json::objectValue}; + jv[sfTransactionType.jsonName] = jss::PermissionedDomainDelete; + jv[sfAccount.jsonName] = to_string(account); + jv[sfDomainID.jsonName] = to_string(domain); + return jv; +} + +// Get PermissionedDomain objects from account_objects rpc call +std::map +getObjects(Account const& account, Env& env) +{ + std::map ret; + Json::Value params; + params[jss::account] = account.human(); + auto const& resp = env.rpc("json", "account_objects", to_string(params)); + Json::Value a(Json::arrayValue); + a = resp[jss::result][jss::account_objects]; + for (auto const& object : a) + { + if (object["LedgerEntryType"] != "PermissionedDomain") + continue; + uint256 index; + std::ignore = index.parseHex(object[jss::index].asString()); + ret[index] = object; + } + return ret; +} + +// Check if ledger object is there +bool +objectExists(uint256 const& objID, Env& env) +{ + Json::Value params; + params[jss::index] = to_string(objID); + auto const& resp = + env.rpc("json", "ledger_entry", to_string(params))["result"]["status"] + .asString(); + if (resp == "success") + return true; + if (resp == "error") + return false; + throw std::runtime_error("Error getting ledger_entry RPC result."); +} + +// Extract credentials from account_object object +Credentials +credentialsFromJson(Json::Value const& object) +{ + Credentials ret; + Json::Value a(Json::arrayValue); + a = object["AcceptedCredentials"]; + for (auto const& credential : a) + { + Json::Value obj(Json::objectValue); + obj = credential["AcceptedCredential"]; + auto const& issuer = obj["Issuer"]; + auto const& credentialType = obj["CredentialType"]; + ret.emplace_back( + *parseBase58(issuer.asString()), + strUnHex(credentialType.asString()).value()); + } + return ret; +} + +// Sort credentials the same way as PermissionedDomainSet +Credentials +sortCredentials(Credentials const& input) +{ + Credentials ret = input; + std::sort( + ret.begin(), + ret.end(), + [](Credential const& left, Credential const& right) -> bool { + return left.first < right.first; + }); + return ret; +} + +// Get account_info +Json::Value +ownerInfo(Account const& account, Env& env) +{ + Json::Value params; + params[jss::account] = account.human(); + auto const& resp = env.rpc("json", "account_info", to_string(params)); + return env.rpc( + "json", "account_info", to_string(params))["result"]["account_data"]; +} + +} // namespace pd +} // namespace jtx +} // namespace test +} // namespace ripple diff --git a/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp index b737af5e570..a1a5418f5c9 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp @@ -29,8 +29,6 @@ PermissionedDomainDelete::preflight(PreflightContext const& ctx) return temDISABLED; if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (!ctx.tx.isFieldPresent(sfDomainID)) - return temMALFORMED; return preflight2(ctx); } diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index e7cf94f0b64..42a91bb0c33 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -34,12 +34,7 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - if (!ctx.tx.isFieldPresent(sfAcceptedCredentials)) - return temMALFORMED; auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); - // TODO check to see if we should disallow duplicate issuers. - // If so, it probably means sorting on the CredentialType field - // for identical issuers. if (credentials.empty() || credentials.size() > PD_ARRAY_MAX) return temMALFORMED; @@ -70,6 +65,8 @@ PermissionedDomainSet::preclaim(PreclaimContext const& ctx) { return temMALFORMED; } + if (credential.getFieldVL(sfCredentialType).empty()) + return temMALFORMED; if (!ctx.view.read(keylet::account(credential.getAccountID(sfIssuer)))) return temBAD_ISSUER; } @@ -106,10 +103,23 @@ PermissionedDomainSet::doApply() // needs to be ironed out. credentials.sort( [](STObject const& left, STObject const& right) -> bool { - return dynamic_cast(&left)->getAccountID( - sfIssuer) < - dynamic_cast(&right)->getAccountID( - sfIssuer); + if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) + return true; + if (left.getAccountID(sfIssuer) == right.getAccountID(sfIssuer)) + { + if (left.getFieldVL(sfCredentialType) < + right.getFieldVL(sfCredentialType)) + { + return true; + } + if (left.getFieldVL(sfCredentialType) == + right.getFieldVL(sfCredentialType)) + { + throw std::runtime_error("duplicate credentials"); + } + return false; + } + return false; }); sle->setFieldArray(sfAcceptedCredentials, credentials); }; @@ -119,7 +129,14 @@ PermissionedDomainSet::doApply() // Modify existing permissioned domain. auto sleUpdate = view().peek( {ltPERMISSIONED_DOMAIN, ctx_.tx.getFieldH256(sfDomainID)}); - updateSle(sleUpdate); + try + { + updateSle(sleUpdate); + } + catch (...) + { + return temMALFORMED; + } view().update(sleUpdate); } else @@ -130,7 +147,14 @@ PermissionedDomainSet::doApply() auto slePd = std::make_shared(pdKeylet); slePd->setAccountID(sfOwner, account_); slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); - updateSle(slePd); + try + { + updateSle(slePd); + } + catch (...) + { + return temMALFORMED; + } view().insert(slePd); auto const page = view().dirInsert( keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_)); From 4dce61747add6fb863f96cfef7c6e169c50bb12b Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Tue, 1 Oct 2024 18:21:23 -0700 Subject: [PATCH 03/11] Test for 2 types of bad DomainIDs in PermissionedDomainSet. Also, test sorting of permissions. --- src/test/app/PermissionedDomains_test.cpp | 52 ++++++++---- src/test/jtx/PermissionedDomains.h | 7 +- src/test/jtx/impl/PermissionedDomains.cpp | 81 ++++++++++++++++--- .../app/tx/detail/PermissionedDomainSet.cpp | 1 - 4 files changed, 113 insertions(+), 28 deletions(-) diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index e96c88d55e9..f1016f3558b 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -175,6 +175,34 @@ class PermissionedDomains_test : public beast::unit_test::suite env(pd::setTx(account, credentialsDup, domain), fee(setFee), ter(temMALFORMED)); + + // Have equal issuers but different credentials and make sure they + // sort correctly. + { + pd::Credentials const credentialsSame{ + {alice2, pd::toBlob("credential3")}, + {alice3, pd::toBlob("credential2")}, + {alice2, pd::toBlob("credential9")}, + {alice5, pd::toBlob("credential4")}, + {alice2, pd::toBlob("credential6")}, + }; + BEAST_EXPECT( + credentialsSame != *pd::sortCredentials(credentialsSame)); + env(pd::setTx(account, credentialsSame, domain), fee(setFee)); + + uint256 d; + if (domain) + d = *domain; + else + d = pd::getNewDomain(env.meta()); + env.close(); + auto objects = pd::getObjects(account, env); + auto const fromObject = pd::credentialsFromJson(objects[d]); + auto const sortedCreds = *pd::sortCredentials(credentialsSame); + BEAST_EXPECT( + pd::credentialsFromJson(objects[d]) == + *pd::sortCredentials(credentialsSame)); + } } // Test PermissionedDomainSet @@ -238,24 +266,10 @@ class PermissionedDomains_test : public beast::unit_test::suite { BEAST_EXPECT( credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); - BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); + BEAST_EXPECT(credentials10 != *pd::sortCredentials(credentials10)); env(pd::setTx(alice, credentials10), fee(setFee)); auto tx = env.tx()->getJson(JsonOptions::none); - auto meta = env.meta()->getJson(JsonOptions::none); - Json::Value a(Json::arrayValue); - a = meta["AffectedNodes"]; - - for (auto const& node : a) - { - if (!node.isMember("CreatedNode") || - node["CreatedNode"]["LedgerEntryType"] != - "PermissionedDomain") - { - continue; - } - std::ignore = domain2.parseHex( - node["CreatedNode"]["LedgerIndex"].asString()); - } + domain2 = pd::getNewDomain(env.meta()); auto objects = pd::getObjects(alice, env); auto object = objects[domain2]; BEAST_EXPECT( @@ -285,6 +299,12 @@ class PermissionedDomains_test : public beast::unit_test::suite env(pd::setTx(alice2, credentials1, domain2), ter(temINVALID_ACCOUNT_ID)); + // Update a uint256(0) domain + env(pd::setTx(alice, credentials1, uint256(0)), ter(temMALFORMED)); + + // Update non-existent domain + env(pd::setTx(alice, credentials1, uint256(75)), ter(tecNO_ENTRY)); + // Test bad data when creating a domain. testBadData(alice, env); // Test bad data when updating a domain. diff --git a/src/test/jtx/PermissionedDomains.h b/src/test/jtx/PermissionedDomains.h index ae7b673a946..95e4541ee66 100644 --- a/src/test/jtx/PermissionedDomains.h +++ b/src/test/jtx/PermissionedDomains.h @@ -21,6 +21,7 @@ #define RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED #include +#include namespace ripple { namespace test { @@ -63,13 +64,17 @@ Credentials credentialsFromJson(Json::Value const& object); // Sort credentials the same way as PermissionedDomainSet -Credentials +std::optional sortCredentials(Credentials const& input); // Get account_info Json::Value ownerInfo(Account const& account, Env& env); +// Get newly created domain from transaction metadata. +uint256 +getNewDomain(std::shared_ptr const& meta); + } // namespace pd } // namespace jtx } // namespace test diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/PermissionedDomains.cpp index 7b27263e990..f773140b266 100644 --- a/src/test/jtx/impl/PermissionedDomains.cpp +++ b/src/test/jtx/impl/PermissionedDomains.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include namespace ripple { namespace test { @@ -120,18 +121,56 @@ credentialsFromJson(Json::Value const& object) return ret; } -// Sort credentials the same way as PermissionedDomainSet -Credentials +// Sort credentials the same way as PermissionedDomainSet. None can +// be identical. +std::optional sortCredentials(Credentials const& input) { - Credentials ret = input; - std::sort( - ret.begin(), - ret.end(), - [](Credential const& left, Credential const& right) -> bool { - return left.first < right.first; - }); - return ret; + try + { + Credentials ret = input; + std::sort( + ret.begin(), + ret.end(), + [](Credential const& left, Credential const& right) -> bool { + if (left.first < right.first) + return true; + if (left.first == right.first) + { + if (left.second < right.second) + return true; + if (left.second == right.second) + throw std::runtime_error("duplicate"); + } + return false; + /* + if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) + return true; + if (left.getAccountID(sfIssuer) == right.getAccountID(sfIssuer)) + { + if (left.getFieldVL(sfCredentialType) < + right.getFieldVL(sfCredentialType)) + { + return true; + } + if (left.getFieldVL(sfCredentialType) == + right.getFieldVL(sfCredentialType)) + { + throw std::runtime_error("duplicate credentials"); + } + return false; + } + return false; + return left.first < right.first; + */ + }); + return ret; + } + catch (...) + { + std::cerr << "wtf\n"; + return std::nullopt; + } } // Get account_info @@ -145,6 +184,28 @@ ownerInfo(Account const& account, Env& env) "json", "account_info", to_string(params))["result"]["account_data"]; } +uint256 +getNewDomain(std::shared_ptr const& meta) +{ + uint256 ret; + auto metaJson = meta->getJson(JsonOptions::none); + Json::Value a(Json::arrayValue); + a = metaJson["AffectedNodes"]; + + for (auto const& node : a) + { + if (!node.isMember("CreatedNode") || + node["CreatedNode"]["LedgerEntryType"] != "PermissionedDomain") + { + continue; + } + std::ignore = + ret.parseHex(node["CreatedNode"]["LedgerIndex"].asString()); + } + + return ret; +} + } // namespace pd } // namespace jtx } // namespace test diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index 42a91bb0c33..f3a5c2aaac7 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -117,7 +117,6 @@ PermissionedDomainSet::doApply() { throw std::runtime_error("duplicate credentials"); } - return false; } return false; }); From cde5ae4ae9463fba416f4591efa4c5dc770e38a9 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Mon, 7 Oct 2024 17:04:29 -0700 Subject: [PATCH 04/11] Silently remove duplicate credentials instead of failing the transaction. Conforms to Credentials spec. --- src/test/app/PermissionedDomains_test.cpp | 44 +++++++---- src/test/jtx/PermissionedDomains.h | 3 +- src/test/jtx/impl/PermissionedDomains.cpp | 73 +++++++------------ .../app/tx/detail/PermissionedDomainSet.cpp | 40 ++++------ 4 files changed, 72 insertions(+), 88 deletions(-) diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index f1016f3558b..6b1454f30d3 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -165,16 +165,32 @@ class PermissionedDomains_test : public beast::unit_test::suite .removeMember("Issuer"); env(txJsonMutable, fee(setFee), ter(temMALFORMED)); - // Make 2 identical credentials. - pd::Credentials const credentialsDup{ - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice2, pd::toBlob("credential1")}, - {alice5, pd::toBlob("credential4")}, - }; - env(pd::setTx(account, credentialsDup, domain), - fee(setFee), - ter(temMALFORMED)); + // Make 2 identical credentials. The duplicate should be silently + // removed. + { + pd::Credentials const credentialsDup{ + {alice7, pd::toBlob("credential6")}, + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice2, pd::toBlob("credential1")}, + {alice5, pd::toBlob("credential4")}, + }; + BEAST_EXPECT(pd::sortCredentials(credentialsDup).size() == 4); + env(pd::setTx(account, credentialsDup, domain), fee(setFee)); + + uint256 d; + if (domain) + d = *domain; + else + d = pd::getNewDomain(env.meta()); + env.close(); + auto objects = pd::getObjects(account, env); + auto const fromObject = pd::credentialsFromJson(objects[d]); + auto const sortedCreds = pd::sortCredentials(credentialsDup); + BEAST_EXPECT( + pd::credentialsFromJson(objects[d]) == + pd::sortCredentials(credentialsDup)); + } // Have equal issuers but different credentials and make sure they // sort correctly. @@ -187,7 +203,7 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice2, pd::toBlob("credential6")}, }; BEAST_EXPECT( - credentialsSame != *pd::sortCredentials(credentialsSame)); + credentialsSame != pd::sortCredentials(credentialsSame)); env(pd::setTx(account, credentialsSame, domain), fee(setFee)); uint256 d; @@ -198,10 +214,10 @@ class PermissionedDomains_test : public beast::unit_test::suite env.close(); auto objects = pd::getObjects(account, env); auto const fromObject = pd::credentialsFromJson(objects[d]); - auto const sortedCreds = *pd::sortCredentials(credentialsSame); + auto const sortedCreds = pd::sortCredentials(credentialsSame); BEAST_EXPECT( pd::credentialsFromJson(objects[d]) == - *pd::sortCredentials(credentialsSame)); + pd::sortCredentials(credentialsSame)); } } @@ -266,7 +282,7 @@ class PermissionedDomains_test : public beast::unit_test::suite { BEAST_EXPECT( credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); - BEAST_EXPECT(credentials10 != *pd::sortCredentials(credentials10)); + BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); env(pd::setTx(alice, credentials10), fee(setFee)); auto tx = env.tx()->getJson(JsonOptions::none); domain2 = pd::getNewDomain(env.meta()); diff --git a/src/test/jtx/PermissionedDomains.h b/src/test/jtx/PermissionedDomains.h index 95e4541ee66..20c5467b128 100644 --- a/src/test/jtx/PermissionedDomains.h +++ b/src/test/jtx/PermissionedDomains.h @@ -21,7 +21,6 @@ #define RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED #include -#include namespace ripple { namespace test { @@ -64,7 +63,7 @@ Credentials credentialsFromJson(Json::Value const& object); // Sort credentials the same way as PermissionedDomainSet -std::optional +Credentials sortCredentials(Credentials const& input); // Get account_info diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/PermissionedDomains.cpp index f773140b266..5405ada76b1 100644 --- a/src/test/jtx/impl/PermissionedDomains.cpp +++ b/src/test/jtx/impl/PermissionedDomains.cpp @@ -121,56 +121,37 @@ credentialsFromJson(Json::Value const& object) return ret; } -// Sort credentials the same way as PermissionedDomainSet. None can -// be identical. -std::optional +// Sort credentials the same way as PermissionedDomainSet. Silently +// remove duplicates. +Credentials sortCredentials(Credentials const& input) { - try - { - Credentials ret = input; - std::sort( - ret.begin(), - ret.end(), - [](Credential const& left, Credential const& right) -> bool { - if (left.first < right.first) - return true; - if (left.first == right.first) - { - if (left.second < right.second) - return true; - if (left.second == right.second) - throw std::runtime_error("duplicate"); - } - return false; - /* - if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) - return true; - if (left.getAccountID(sfIssuer) == right.getAccountID(sfIssuer)) - { - if (left.getFieldVL(sfCredentialType) < - right.getFieldVL(sfCredentialType)) - { - return true; - } - if (left.getFieldVL(sfCredentialType) == - right.getFieldVL(sfCredentialType)) - { - throw std::runtime_error("duplicate credentials"); - } - return false; - } - return false; - return left.first < right.first; - */ - }); - return ret; - } - catch (...) + Credentials ret = input; + + std::set cSet; + for (auto const& c : ret) + cSet.insert(c); + if (ret.size() > cSet.size()) { - std::cerr << "wtf\n"; - return std::nullopt; + ret = Credentials(); + for (auto const& c : cSet) + ret.push_back(c); } + + std::sort( + ret.begin(), + ret.end(), + [](Credential const& left, Credential const& right) -> bool { + if (left.first < right.first) + return true; + if (left.first == right.first) + { + if (left.second < right.second) + return true; + } + return false; + }); + return ret; } // Get account_info diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index f3a5c2aaac7..b3c4fa2f684 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -21,7 +21,7 @@ #include #include #include - +#include #include namespace ripple { @@ -95,12 +95,19 @@ PermissionedDomainSet::doApply() // All checks have already been done. Just update credentials. Same logic // for either new domain or updating existing. + // Silently remove duplicates. auto updateSle = [this](std::shared_ptr const& sle) { auto credentials = ctx_.tx.getFieldArray(sfAcceptedCredentials); - // TODO when credentials are merged, it is possible that this will - // also need to sort on the CredentialType field in case there - // are multiple issuers in each set of credentials. The spec - // needs to be ironed out. + std::map hashed; + for (auto const& c : credentials) + hashed.insert({c.getHash(HashPrefix::transactionID), c}); + if (credentials.size() > hashed.size()) + { + credentials = STArray(); + for (auto const& e : hashed) + credentials.push_back(e.second); + } + credentials.sort( [](STObject const& left, STObject const& right) -> bool { if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) @@ -112,11 +119,6 @@ PermissionedDomainSet::doApply() { return true; } - if (left.getFieldVL(sfCredentialType) == - right.getFieldVL(sfCredentialType)) - { - throw std::runtime_error("duplicate credentials"); - } } return false; }); @@ -128,14 +130,7 @@ PermissionedDomainSet::doApply() // Modify existing permissioned domain. auto sleUpdate = view().peek( {ltPERMISSIONED_DOMAIN, ctx_.tx.getFieldH256(sfDomainID)}); - try - { - updateSle(sleUpdate); - } - catch (...) - { - return temMALFORMED; - } + updateSle(sleUpdate); view().update(sleUpdate); } else @@ -146,14 +141,7 @@ PermissionedDomainSet::doApply() auto slePd = std::make_shared(pdKeylet); slePd->setAccountID(sfOwner, account_); slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); - try - { - updateSle(slePd); - } - catch (...) - { - return temMALFORMED; - } + updateSle(slePd); view().insert(slePd); auto const page = view().dirInsert( keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_)); From ce083299e2fd7b9422d7a9e564969caf4d6329fb Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Mon, 14 Oct 2024 13:24:23 -0700 Subject: [PATCH 05/11] Review fixes from Denis. --- include/xrpl/protocol/Indexes.h | 2 + src/libxrpl/protocol/Indexes.cpp | 6 + src/test/app/PermissionedDomains_test.cpp | 123 +++++++++++++----- src/test/jtx/PermissionedDomains.h | 2 +- src/test/jtx/impl/PermissionedDomains.cpp | 6 +- src/test/rpc/AccountObjects_test.cpp | 1 + .../app/tx/detail/PermissionedDomainSet.cpp | 53 ++++---- .../app/tx/detail/PermissionedDomainSet.h | 3 - src/xrpld/rpc/detail/RPCHelpers.cpp | 5 +- 9 files changed, 142 insertions(+), 59 deletions(-) diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h index f327412e72e..3aaedb46eed 100644 --- a/include/xrpl/protocol/Indexes.h +++ b/include/xrpl/protocol/Indexes.h @@ -332,6 +332,8 @@ mptoken(uint256 const& issuanceKey, AccountID const& holder) noexcept; Keylet permissionedDomain(AccountID const& account, std::uint32_t seq) noexcept; +Keylet +permissionedDomain(uint256 const& domainID) noexcept; } // namespace keylet // Everything below is deprecated and should be removed in favor of keylets: diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index 5905bc3f1fc..261c253c8cb 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -528,6 +528,12 @@ permissionedDomain(AccountID const& account, std::uint32_t seq) noexcept indexHash(LedgerNameSpace::PERMISSIONED_DOMAIN, account, seq)}; } +Keylet +permissionedDomain(uint256 const& domainID) noexcept +{ + return {ltPERMISSIONED_DOMAIN, domainID}; +} + } // namespace keylet } // namespace ripple diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index 6b1454f30d3..03dd6e98eb7 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -52,12 +52,13 @@ class PermissionedDomains_test : public beast::unit_test::suite Account const alice("alice"); Env env(*this, withFeature_); env.fund(XRP(1000), alice); - auto const setFee(drops(env.current()->fees().increment)); pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; - env(pd::setTx(alice, credentials), fee(setFee)); + env(pd::setTx(alice, credentials)); BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); auto objects = pd::getObjects(alice, env); BEAST_EXPECT(objects.size() == 1); + // Test that account_objects is correct without passing it the type + BEAST_EXPECT(objects == pd::getObjects(alice, env, false)); auto const domain = objects.begin()->first; env(pd::deleteTx(alice, domain)); } @@ -70,9 +71,8 @@ class PermissionedDomains_test : public beast::unit_test::suite Account const alice("alice"); Env env(*this, withoutFeature_); env.fund(XRP(1000), alice); - auto const setFee(drops(env.current()->fees().increment)); pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; - env(pd::setTx(alice, credentials), fee(setFee), ter(temDISABLED)); + env(pd::setTx(alice, credentials), ter(temDISABLED)); env(pd::deleteTx(alice, uint256(75)), ter(temDISABLED)); } @@ -98,9 +98,7 @@ class PermissionedDomains_test : public beast::unit_test::suite auto const setFee(drops(env.current()->fees().increment)); // Test empty credentials. - env(pd::setTx(account, pd::Credentials(), domain), - fee(setFee), - ter(temMALFORMED)); + env(pd::setTx(account, pd::Credentials(), domain), ter(temMALFORMED)); // Test 11 credentials. pd::Credentials const credentials11{ @@ -117,9 +115,7 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice12, pd::toBlob("credential11")}}; BEAST_EXPECT( credentials11.size() == PermissionedDomainSet::PD_ARRAY_MAX + 1); - env(pd::setTx(account, credentials11, domain), - fee(setFee), - ter(temMALFORMED)); + env(pd::setTx(account, credentials11, domain), ter(temMALFORMED)); // Test credentials including non-existent issuer. Account const nobody("nobody"); @@ -131,9 +127,7 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice5, pd::toBlob("credential5")}, {alice6, pd::toBlob("credential6")}, {alice7, pd::toBlob("credential7")}}; - env(pd::setTx(account, credentialsNon, domain), - fee(setFee), - ter(temBAD_ISSUER)); + env(pd::setTx(account, credentialsNon, domain), ter(temBAD_ISSUER)); pd::Credentials const credentials4{ {alice2, pd::toBlob("credential1")}, @@ -147,23 +141,23 @@ class PermissionedDomains_test : public beast::unit_test::suite // Remove Issuer from a credential and apply. txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] .removeMember("Issuer"); - env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + env(txJsonMutable, ter(temMALFORMED)); txJsonMutable["AcceptedCredentials"][2u] = credentialOrig; // Make an empty CredentialType. txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] ["CredentialType"] = ""; - env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + env(txJsonMutable, ter(temMALFORMED)); // Remove Credentialtype from a credential and apply. txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] .removeMember("CredentialType"); - env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + env(txJsonMutable, ter(temMALFORMED)); // Remove both txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] .removeMember("Issuer"); - env(txJsonMutable, fee(setFee), ter(temMALFORMED)); + env(txJsonMutable, ter(temMALFORMED)); // Make 2 identical credentials. The duplicate should be silently // removed. @@ -176,7 +170,7 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice5, pd::toBlob("credential4")}, }; BEAST_EXPECT(pd::sortCredentials(credentialsDup).size() == 4); - env(pd::setTx(account, credentialsDup, domain), fee(setFee)); + env(pd::setTx(account, credentialsDup, domain)); uint256 d; if (domain) @@ -204,7 +198,7 @@ class PermissionedDomains_test : public beast::unit_test::suite }; BEAST_EXPECT( credentialsSame != pd::sortCredentials(credentialsSame)); - env(pd::setTx(account, credentialsSame, domain), fee(setFee)); + env(pd::setTx(account, credentialsSame, domain)); uint256 d; if (domain) @@ -245,13 +239,11 @@ class PermissionedDomains_test : public beast::unit_test::suite alice10, alice11, alice12); - auto const dropsFee = env.current()->fees().increment; - auto const setFee(drops(dropsFee)); // Create new from existing account with a single credential. pd::Credentials const credentials1{{alice2, pd::toBlob("credential1")}}; { - env(pd::setTx(alice, credentials1), fee(setFee)); + env(pd::setTx(alice, credentials1)); BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); auto tx = env.tx()->getJson(JsonOptions::none); BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainSet"); @@ -283,7 +275,7 @@ class PermissionedDomains_test : public beast::unit_test::suite BEAST_EXPECT( credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); - env(pd::setTx(alice, credentials10), fee(setFee)); + env(pd::setTx(alice, credentials10)); auto tx = env.tx()->getJson(JsonOptions::none); domain2 = pd::getNewDomain(env.meta()); auto objects = pd::getObjects(alice, env); @@ -293,11 +285,6 @@ class PermissionedDomains_test : public beast::unit_test::suite pd::sortCredentials(credentials10)); } - // Make a new domain with insufficient fee. - env(pd::setTx(alice, credentials10), - fee(drops(dropsFee - 1)), - ter(telINSUF_FEE_P)); - // Update with 1 credential. env(pd::setTx(alice, credentials1, domain2)); BEAST_EXPECT( @@ -364,7 +351,7 @@ class PermissionedDomains_test : public beast::unit_test::suite env.fund(XRP(1000), alice); auto const setFee(drops(env.current()->fees().increment)); pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; - env(pd::setTx(alice, credentials), fee(setFee)); + env(pd::setTx(alice, credentials)); env.close(); auto objects = pd::getObjects(alice, env); BEAST_EXPECT(objects.size() == 1); @@ -396,6 +383,83 @@ class PermissionedDomains_test : public beast::unit_test::suite BEAST_EXPECT(!pd::objectExists(objID, env)); } + void + testAccountReserve() + { + // Verify that the reserve behaves as expected for minting. + testcase("Account Reserve"); + + using namespace test::jtx; + + Env env(*this, withFeature_); + Account const alice("alice"); + + // Fund alice enough to exist, but not enough to meet + // the reserve. + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + env.fund(acctReserve, alice); + env.close(); + BEAST_EXPECT(env.balance(alice) == acctReserve); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); + + // alice does not have enough XRP to cover the reserve. + pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + env(pd::setTx(alice, credentials), ter(tecINSUFFICIENT_RESERVE)); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); + BEAST_EXPECT(pd::getObjects(alice, env).size() == 0); + env.close(); + + // Pay alice almost enough to make the reserve. + env(pay(env.master, alice, incReserve + drops(19))); + BEAST_EXPECT(env.balance(alice) == acctReserve + incReserve + drops(9)); + env.close(); + + // alice still does not have enough XRP for the reserve. + env(pd::setTx(alice, credentials), ter(tecINSUFFICIENT_RESERVE)); + env.close(); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); + + // Pay alice enough to make the reserve. + env(pay(env.master, alice, drops(11))); + env.close(); + + // Now alice can create a PermissionedDomain. + env(pd::setTx(alice, credentials)); + env.close(); + BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + + /* + env(did::setValid(alice), ter(tecINSUFFICIENT_RESERVE)); + env.close(); + BEAST_EXPECT(ownerCount(env, alice) == 0); + + // Pay alice almost enough to make the reserve for a DID. + env(pay(env.master, alice, incReserve + drops(19))); + BEAST_EXPECT(env.balance(alice) == acctReserve + incReserve + + drops(9)); env.close(); + + // alice still does not have enough XRP for the reserve of a + DID. env(did::setValid(alice), ter(tecINSUFFICIENT_RESERVE)); + env.close(); + BEAST_EXPECT(ownerCount(env, alice) == 0); + + // Pay alice enough to make the reserve for a DID. + env(pay(env.master, alice, drops(11))); + env.close(); + + // Now alice can create a DID. + env(did::setValid(alice)); + env.close(); + BEAST_EXPECT(ownerCount(env, alice) == 1); + + // alice deletes her DID. + env(did::del(alice)); + BEAST_EXPECT(ownerCount(env, alice) == 0); + env.close(); + */ + } + public: void run() override @@ -404,6 +468,7 @@ class PermissionedDomains_test : public beast::unit_test::suite testDisabled(); testSet(); testDelete(); + testAccountReserve(); } }; diff --git a/src/test/jtx/PermissionedDomains.h b/src/test/jtx/PermissionedDomains.h index 20c5467b128..3ddf08096f7 100644 --- a/src/test/jtx/PermissionedDomains.h +++ b/src/test/jtx/PermissionedDomains.h @@ -45,7 +45,7 @@ deleteTx(AccountID const& account, uint256 const& domain); // Get PermissionedDomain objects from account_objects rpc call std::map -getObjects(Account const& account, Env& env); +getObjects(Account const& account, Env& env, bool withType = true); // Check if ledger object is there bool diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/PermissionedDomains.cpp index 5405ada76b1..632efb7e485 100644 --- a/src/test/jtx/impl/PermissionedDomains.cpp +++ b/src/test/jtx/impl/PermissionedDomains.cpp @@ -64,13 +64,15 @@ deleteTx(AccountID const& account, uint256 const& domain) return jv; } -// Get PermissionedDomain objects from account_objects rpc call +// Get PermissionedDomain objects by type from account_objects rpc call std::map -getObjects(Account const& account, Env& env) +getObjects(Account const& account, Env& env, bool withType) { std::map ret; Json::Value params; params[jss::account] = account.human(); + if (withType) + params[jss::type] = jss::permissioned_domain; auto const& resp = env.rpc("json", "account_objects", to_string(params)); Json::Value a(Json::arrayValue); a = resp[jss::result][jss::account_objects]; diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 7326fff0c76..69f47e084f2 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -627,6 +627,7 @@ class AccountObjects_test : public beast::unit_test::suite BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::ticket), 0)); BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::amm), 0)); BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::did), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::permissioned_domain), 0)); // we expect invalid field type reported for the following types BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::amendments))); diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index b3c4fa2f684..c7c088d9121 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -37,17 +37,22 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx) auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); if (credentials.empty() || credentials.size() > PD_ARRAY_MAX) return temMALFORMED; + for (auto const& credential : credentials) + { + if (!credential.isFieldPresent(sfIssuer) || + !credential.isFieldPresent(sfCredentialType)) + { + return temMALFORMED; + } + if (credential.getFieldVL(sfCredentialType).empty()) + return temMALFORMED; + } - return preflight2(ctx); -} + auto const domain = ctx.tx.at(~sfDomainID); + if (domain && *domain == beast::zero) + return temMALFORMED; -XRPAmount -PermissionedDomainSet::calculateBaseFee(ReadView const& view, STTx const& tx) -{ - if (tx.isFieldPresent(sfDomainID)) - return Transactor::calculateBaseFee(view, tx); - // The fee required for a new PermissionedDomain is one owner reserve. - return view.fees().increment; + return preflight2(ctx); } TER @@ -56,17 +61,9 @@ PermissionedDomainSet::preclaim(PreclaimContext const& ctx) if (!ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount)))) return tefINTERNAL; - assert(ctx.tx.isFieldPresent(sfAcceptedCredentials)); auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); for (auto const& credential : credentials) { - if (!credential.isFieldPresent(sfIssuer) || - !credential.isFieldPresent(sfCredentialType)) - { - return temMALFORMED; - } - if (credential.getFieldVL(sfCredentialType).empty()) - return temMALFORMED; if (!ctx.view.read(keylet::account(credential.getAccountID(sfIssuer)))) return temBAD_ISSUER; } @@ -74,9 +71,7 @@ PermissionedDomainSet::preclaim(PreclaimContext const& ctx) if (!ctx.tx.isFieldPresent(sfDomainID)) return tesSUCCESS; auto const domain = ctx.tx.getFieldH256(sfDomainID); - if (domain == beast::zero) - return temMALFORMED; - auto const sleDomain = ctx.view.read({ltPERMISSIONED_DOMAIN, domain}); + auto const sleDomain = ctx.view.read(keylet::permissionedDomain(domain)); if (!sleDomain) return tecNO_ENTRY; auto const owner = sleDomain->getAccountID(sfOwner); @@ -93,6 +88,18 @@ PermissionedDomainSet::doApply() { auto const ownerSle = view().peek(keylet::account(account_)); + // Check reserve availability for new object creation + { + auto const balance = STAmount((*ownerSle)[sfBalance]).xrp(); + auto const reserve = + ctx_.view().fees().accountReserve((*ownerSle)[sfOwnerCount] + 1); + + if (balance < reserve) + return tecINSUFFICIENT_RESERVE; + } + + // The purpose of this lambda is to modify the SLE for either creating a + // new or updating an existing object, to reduce code repetition. // All checks have already been done. Just update credentials. Same logic // for either new domain or updating existing. // Silently remove duplicates. @@ -129,7 +136,9 @@ PermissionedDomainSet::doApply() { // Modify existing permissioned domain. auto sleUpdate = view().peek( - {ltPERMISSIONED_DOMAIN, ctx_.tx.getFieldH256(sfDomainID)}); + keylet::permissionedDomain(ctx_.tx.getFieldH256(sfDomainID))); + // It should already be checked in preclaim(). + assert(sleUpdate); updateSle(sleUpdate); view().update(sleUpdate); } @@ -142,7 +151,6 @@ PermissionedDomainSet::doApply() slePd->setAccountID(sfOwner, account_); slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); updateSle(slePd); - view().insert(slePd); auto const page = view().dirInsert( keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_)); if (!page) @@ -150,6 +158,7 @@ PermissionedDomainSet::doApply() slePd->setFieldU64(sfOwnerNode, *page); // If we succeeded, the new entry counts against the creator's reserve. adjustOwnerCount(view(), ownerSle, 1, ctx_.journal); + view().insert(slePd); } return tesSUCCESS; diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.h b/src/xrpld/app/tx/detail/PermissionedDomainSet.h index e1f105b1d15..fcc507d1867 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.h +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.h @@ -38,9 +38,6 @@ class PermissionedDomainSet : public Transactor static NotTEC preflight(PreflightContext const& ctx); - static XRPAmount - calculateBaseFee(ReadView const& view, STTx const& tx); - static TER preclaim(PreclaimContext const& ctx); diff --git a/src/xrpld/rpc/detail/RPCHelpers.cpp b/src/xrpld/rpc/detail/RPCHelpers.cpp index af204eaedf7..82569745e1b 100644 --- a/src/xrpld/rpc/detail/RPCHelpers.cpp +++ b/src/xrpld/rpc/detail/RPCHelpers.cpp @@ -931,7 +931,7 @@ chooseLedgerEntryType(Json::Value const& params) std::pair result{RPC::Status::OK, ltANY}; if (params.isMember(jss::type)) { - static constexpr std::array, 25> + static constexpr std::array, 26> types{ {{jss::account, ltACCOUNT_ROOT}, {jss::amendments, ltAMENDMENTS}, @@ -958,7 +958,8 @@ chooseLedgerEntryType(Json::Value const& params) {jss::xchain_owned_create_account_claim_id, ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID}, {jss::mpt_issuance, ltMPTOKEN_ISSUANCE}, - {jss::mptoken, ltMPTOKEN}}}; + {jss::mptoken, ltMPTOKEN}, + {jss::permissioned_domain, ltPERMISSIONED_DOMAIN}}}; auto const& p = params[jss::type]; if (!p.isString()) From 27e713399839e60c7e34ce55a83c698d8f1c098a Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Tue, 15 Oct 2024 04:19:46 -0700 Subject: [PATCH 06/11] Check for reserve only when creating a new object. --- .../app/tx/detail/PermissionedDomainSet.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index c7c088d9121..7d296599cbc 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -88,16 +88,6 @@ PermissionedDomainSet::doApply() { auto const ownerSle = view().peek(keylet::account(account_)); - // Check reserve availability for new object creation - { - auto const balance = STAmount((*ownerSle)[sfBalance]).xrp(); - auto const reserve = - ctx_.view().fees().accountReserve((*ownerSle)[sfOwnerCount] + 1); - - if (balance < reserve) - return tecINSUFFICIENT_RESERVE; - } - // The purpose of this lambda is to modify the SLE for either creating a // new or updating an existing object, to reduce code repetition. // All checks have already been done. Just update credentials. Same logic @@ -145,6 +135,13 @@ PermissionedDomainSet::doApply() else { // Create new permissioned domain. + // Check reserve availability for new object creation + auto const balance = STAmount((*ownerSle)[sfBalance]).xrp(); + auto const reserve = + ctx_.view().fees().accountReserve((*ownerSle)[sfOwnerCount] + 1); + if (balance < reserve) + return tecINSUFFICIENT_RESERVE; + Keylet const pdKeylet = keylet::permissionedDomain( account_, ctx_.tx.getFieldU32(sfSequence)); auto slePd = std::make_shared(pdKeylet); From f281504330f41e51ad398b5c906f8c300961fcf7 Mon Sep 17 00:00:00 2001 From: Oleksandr <115580134+oleks-rip@users.noreply.github.com> Date: Sat, 26 Oct 2024 01:17:08 -0400 Subject: [PATCH 07/11] RXI-1265 review fixes --- src/test/app/PermissionedDomains_test.cpp | 41 ++------- src/test/jtx.h | 1 + src/test/jtx/impl/PermissionedDomains.cpp | 30 +------ src/test/rpc/LedgerRPC_test.cpp | 90 +++++++++++++++++++ .../app/tx/detail/PermissionedDomainSet.cpp | 57 ++++++------ src/xrpld/rpc/handlers/LedgerEntry.cpp | 30 +++++++ 6 files changed, 158 insertions(+), 91 deletions(-) diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index 03dd6e98eb7..126e47aef9a 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -18,7 +18,6 @@ //============================================================================== #include -#include #include #include #include @@ -410,9 +409,13 @@ class PermissionedDomains_test : public beast::unit_test::suite BEAST_EXPECT(pd::getObjects(alice, env).size() == 0); env.close(); + auto const baseFee = env.current()->fees().base.drops(); + // Pay alice almost enough to make the reserve. - env(pay(env.master, alice, incReserve + drops(19))); - BEAST_EXPECT(env.balance(alice) == acctReserve + incReserve + drops(9)); + env(pay(env.master, alice, incReserve + drops(2 * baseFee) - drops(1))); + BEAST_EXPECT( + env.balance(alice) == + acctReserve + incReserve + drops(baseFee) - drops(1)); env.close(); // alice still does not have enough XRP for the reserve. @@ -421,43 +424,13 @@ class PermissionedDomains_test : public beast::unit_test::suite BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); // Pay alice enough to make the reserve. - env(pay(env.master, alice, drops(11))); + env(pay(env.master, alice, drops(baseFee) + drops(1))); env.close(); // Now alice can create a PermissionedDomain. env(pd::setTx(alice, credentials)); env.close(); BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); - - /* - env(did::setValid(alice), ter(tecINSUFFICIENT_RESERVE)); - env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 0); - - // Pay alice almost enough to make the reserve for a DID. - env(pay(env.master, alice, incReserve + drops(19))); - BEAST_EXPECT(env.balance(alice) == acctReserve + incReserve + - drops(9)); env.close(); - - // alice still does not have enough XRP for the reserve of a - DID. env(did::setValid(alice), ter(tecINSUFFICIENT_RESERVE)); - env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 0); - - // Pay alice enough to make the reserve for a DID. - env(pay(env.master, alice, drops(11))); - env.close(); - - // Now alice can create a DID. - env(did::setValid(alice)); - env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 1); - - // alice deletes her DID. - env(did::del(alice)); - BEAST_EXPECT(ownerCount(env, alice) == 0); - env.close(); - */ } public: diff --git a/src/test/jtx.h b/src/test/jtx.h index b7b9a9fa05c..d9ef285a61d 100644 --- a/src/test/jtx.h +++ b/src/test/jtx.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/PermissionedDomains.cpp index 632efb7e485..7e7eb964415 100644 --- a/src/test/jtx/impl/PermissionedDomains.cpp +++ b/src/test/jtx/impl/PermissionedDomains.cpp @@ -128,32 +128,10 @@ credentialsFromJson(Json::Value const& object) Credentials sortCredentials(Credentials const& input) { - Credentials ret = input; - - std::set cSet; - for (auto const& c : ret) - cSet.insert(c); - if (ret.size() > cSet.size()) - { - ret = Credentials(); - for (auto const& c : cSet) - ret.push_back(c); - } - - std::sort( - ret.begin(), - ret.end(), - [](Credential const& left, Credential const& right) -> bool { - if (left.first < right.first) - return true; - if (left.first == right.first) - { - if (left.second < right.second) - return true; - } - return false; - }); - return ret; + std::set credentialsSet; + for (auto const& c : input) + credentialsSet.insert(c); + return {credentialsSet.begin(), credentialsSet.end()}; } // Get account_info diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 41657468666..611bc62991f 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -3055,6 +3055,95 @@ class LedgerRPC_test : public beast::unit_test::suite } } + void + testLedgerEntryPermissionedDomain() + { + testcase("ledger_entry PermissionedDomain"); + + using namespace test::jtx; + + Env env(*this, supported_amendments() | featurePermissionedDomains); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + env.fund(XRP(5000), issuer, alice, bob); + env.close(); + + auto const seq = env.seq(alice); + env(pd::setTx(alice, {{alice, pd::toBlob("first credential")}})); + env.close(); + auto const objects = pd::getObjects(alice, env); + BEAST_EXPECT(objects.size() == 1); + if (objects.empty()) + return; + // env(pd::deleteTx(alice, domain)); + + { + // Succeed + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain][jss::account] = alice.human(); + params[jss::permissioned_domain][jss::seq] = seq; + auto jv = env.rpc("json", "ledger_entry", to_string(params)); + BEAST_EXPECT( + jv.isObject() && jv.isMember(jss::result) && + !jv[jss::result].isMember(jss::error) && + jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::node].isMember( + sfLedgerEntryType.jsonName) && + jv[jss::result][jss::node][sfLedgerEntryType.jsonName] == + jss::PermissionedDomain); + + std::string const pdIdx = jv[jss::result][jss::index].asString(); + BEAST_EXPECT( + strHex(keylet::permissionedDomain(alice, seq).key) == pdIdx); + + params.clear(); + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain] = pdIdx; + jv = env.rpc("json", "ledger_entry", to_string(params)); + BEAST_EXPECT( + jv.isObject() && jv.isMember(jss::result) && + !jv[jss::result].isMember(jss::error) && + jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::node].isMember( + sfLedgerEntryType.jsonName) && + jv[jss::result][jss::node][sfLedgerEntryType.jsonName] == + jss::PermissionedDomain); + } + + { + // Fail, invalid account + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain][jss::account] = 1; + params[jss::permissioned_domain][jss::seq] = seq; + auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); + checkErrorValue(jrr[jss::result], "malformedRequest", ""); + } + + { + // Fail, no account + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain][jss::account] = ""; + params[jss::permissioned_domain][jss::seq] = seq; + auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); + checkErrorValue(jrr[jss::result], "malformedRequest", ""); + } + + { + // Fail, invalid sequence + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain][jss::account] = alice.human(); + params[jss::permissioned_domain][jss::seq] = "12g"; + auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); + checkErrorValue(jrr[jss::result], "malformedRequest", ""); + } + } + public: void run() override @@ -3085,6 +3174,7 @@ class LedgerRPC_test : public beast::unit_test::suite testInvalidOracleLedgerEntry(); testOracleLedgerEntry(); testLedgerEntryMPT(); + testLedgerEntryPermissionedDomain(); forAllApiVersions(std::bind_front( &LedgerRPC_test::testLedgerEntryInvalidParams, this)); diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index 7d296599cbc..f64cef0a00c 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -44,7 +44,8 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx) { return temMALFORMED; } - if (credential.getFieldVL(sfCredentialType).empty()) + auto sz = credential.getFieldVL(sfCredentialType).size(); + if (!sz || sz > 64) return temMALFORMED; } @@ -94,43 +95,34 @@ PermissionedDomainSet::doApply() // for either new domain or updating existing. // Silently remove duplicates. auto updateSle = [this](std::shared_ptr const& sle) { - auto credentials = ctx_.tx.getFieldArray(sfAcceptedCredentials); - std::map hashed; - for (auto const& c : credentials) - hashed.insert({c.getHash(HashPrefix::transactionID), c}); - if (credentials.size() > hashed.size()) - { - credentials = STArray(); - for (auto const& e : hashed) - credentials.push_back(e.second); - } + auto const& credentials = ctx_.tx.getFieldArray(sfAcceptedCredentials); - credentials.sort( - [](STObject const& left, STObject const& right) -> bool { - if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) - return true; - if (left.getAccountID(sfIssuer) == right.getAccountID(sfIssuer)) - { - if (left.getFieldVL(sfCredentialType) < - right.getFieldVL(sfCredentialType)) - { - return true; - } - } - return false; - }); - sle->setFieldArray(sfAcceptedCredentials, credentials); + std::map< + std::pair, + std::reference_wrapper> + credMap; + for (auto const& c : credentials) + credMap.insert( + {{c.getAccountID(sfIssuer), c.getFieldVL(sfCredentialType)}, + c}); + + STArray credSorted; + credSorted.reserve(credMap.size()); + for (auto const& [k, v] : credMap) + credSorted.push_back(v); + sle->setFieldArray(sfAcceptedCredentials, credSorted); }; if (ctx_.tx.isFieldPresent(sfDomainID)) { // Modify existing permissioned domain. - auto sleUpdate = view().peek( + auto slePd = view().peek( keylet::permissionedDomain(ctx_.tx.getFieldH256(sfDomainID))); - // It should already be checked in preclaim(). - assert(sleUpdate); - updateSle(sleUpdate); - view().update(sleUpdate); + if (!slePd) + return tefINTERNAL; + + updateSle(slePd); + view().update(slePd); } else { @@ -145,6 +137,9 @@ PermissionedDomainSet::doApply() Keylet const pdKeylet = keylet::permissionedDomain( account_, ctx_.tx.getFieldU32(sfSequence)); auto slePd = std::make_shared(pdKeylet); + if (!slePd) + return tefINTERNAL; + slePd->setAccountID(sfOwner, account_); slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); updateSle(slePd); diff --git a/src/xrpld/rpc/handlers/LedgerEntry.cpp b/src/xrpld/rpc/handlers/LedgerEntry.cpp index 5d03bbb189d..8cb99acb231 100644 --- a/src/xrpld/rpc/handlers/LedgerEntry.cpp +++ b/src/xrpld/rpc/handlers/LedgerEntry.cpp @@ -807,6 +807,36 @@ doLedgerEntry(RPC::JsonContext& context) } } } + else if (context.params.isMember(jss::permissioned_domain)) + { + uNodeIndex = beast::zero; + auto const& pd = context.params[jss::permissioned_domain]; + if (pd.isString()) + { + if (!uNodeIndex.parseHex(pd.asString())) + { + uNodeIndex = beast::zero; + jvResult[jss::error] = "malformedRequest"; + } + } + else if ( + !pd.isObject() || !pd.isMember(jss::account) || + !pd[jss::account].isString() || !pd.isMember(jss::seq) || + (!pd[jss::seq].isInt() && !pd[jss::seq].isUInt())) + { + jvResult[jss::error] = "malformedRequest"; + } + else + { + auto const account = + parseBase58(pd[jss::account].asString()); + unsigned const seq = pd[jss::seq].asUInt(); + if (account) + uNodeIndex = keylet::permissionedDomain(*account, seq).key; + else + jvResult[jss::error] = "malformedRequest"; + } + } else { if (context.params.isMember("params") && From 0e949ea54ae5646b5115b9264702ce826a8c0497 Mon Sep 17 00:00:00 2001 From: Oleksandr <115580134+oleks-rip@users.noreply.github.com> Date: Thu, 7 Nov 2024 00:33:27 -0500 Subject: [PATCH 08/11] Rebase fixes --- src/test/app/PermissionedDomains_test.cpp | 246 ++++++++++-------- src/test/jtx.h | 2 +- src/test/jtx/Account.h | 6 +- src/test/jtx/deposit.h | 3 + ...edDomains.cpp => permissioned_domains.cpp} | 25 +- ...sionedDomains.h => permissioned_domains.h} | 19 +- src/test/rpc/LedgerRPC_test.cpp | 2 +- 7 files changed, 165 insertions(+), 138 deletions(-) rename src/test/jtx/impl/{PermissionedDomains.cpp => permissioned_domains.cpp} (88%) rename src/test/jtx/{PermissionedDomains.h => permissioned_domains.h} (85%) diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index 126e47aef9a..7ac6eaad73a 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -25,6 +25,7 @@ #include #include #include + #include #include #include @@ -37,6 +38,20 @@ namespace ripple { namespace test { namespace jtx { +static std::string +exceptionExpected(Env& env, Json::Value const& jv) +{ + try + { + env(jv, ter(temMALFORMED)); + } + catch (std::exception const& ex) + { + return ex.what(); + } + return {}; +} + class PermissionedDomains_test : public beast::unit_test::suite { FeatureBitset withFeature_{ @@ -51,7 +66,7 @@ class PermissionedDomains_test : public beast::unit_test::suite Account const alice("alice"); Env env(*this, withFeature_); env.fund(XRP(1000), alice); - pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + pd::Credentials credentials{{alice, "first credential"}}; env(pd::setTx(alice, credentials)); BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); auto objects = pd::getObjects(alice, env); @@ -70,7 +85,7 @@ class PermissionedDomains_test : public beast::unit_test::suite Account const alice("alice"); Env env(*this, withoutFeature_); env.fund(XRP(1000), alice); - pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + pd::Credentials credentials{{alice, "first credential"}}; env(pd::setTx(alice, credentials), ter(temDISABLED)); env(pd::deleteTx(alice, uint256(75)), ter(temDISABLED)); } @@ -101,17 +116,17 @@ class PermissionedDomains_test : public beast::unit_test::suite // Test 11 credentials. pd::Credentials const credentials11{ - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice4, pd::toBlob("credential3")}, - {alice5, pd::toBlob("credential4")}, - {alice6, pd::toBlob("credential5")}, - {alice7, pd::toBlob("credential6")}, - {alice8, pd::toBlob("credential7")}, - {alice9, pd::toBlob("credential8")}, - {alice10, pd::toBlob("credential9")}, - {alice11, pd::toBlob("credential10")}, - {alice12, pd::toBlob("credential11")}}; + {alice2, "credential1"}, + {alice3, "credential2"}, + {alice4, "credential3"}, + {alice5, "credential4"}, + {alice6, "credential5"}, + {alice7, "credential6"}, + {alice8, "credential7"}, + {alice9, "credential8"}, + {alice10, "credential9"}, + {alice11, "credential10"}, + {alice12, "credential11"}}; BEAST_EXPECT( credentials11.size() == PermissionedDomainSet::PD_ARRAY_MAX + 1); env(pd::setTx(account, credentials11, domain), ter(temMALFORMED)); @@ -119,55 +134,63 @@ class PermissionedDomains_test : public beast::unit_test::suite // Test credentials including non-existent issuer. Account const nobody("nobody"); pd::Credentials const credentialsNon{ - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice4, pd::toBlob("credential3")}, - {nobody, pd::toBlob("credential4")}, - {alice5, pd::toBlob("credential5")}, - {alice6, pd::toBlob("credential6")}, - {alice7, pd::toBlob("credential7")}}; + {alice2, "credential1"}, + {alice3, "credential2"}, + {alice4, "credential3"}, + {nobody, "credential4"}, + {alice5, "credential5"}, + {alice6, "credential6"}, + {alice7, "credential7"}}; env(pd::setTx(account, credentialsNon, domain), ter(temBAD_ISSUER)); pd::Credentials const credentials4{ - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice4, pd::toBlob("credential3")}, - {alice5, pd::toBlob("credential4")}, + {alice2, "credential1"}, + {alice3, "credential2"}, + {alice4, "credential3"}, + {alice5, "credential4"}, }; auto txJsonMutable = pd::setTx(account, credentials4, domain); auto const credentialOrig = txJsonMutable["AcceptedCredentials"][2u]; // Remove Issuer from a credential and apply. - txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] - .removeMember("Issuer"); - env(txJsonMutable, ter(temMALFORMED)); + txJsonMutable["AcceptedCredentials"][2u][jss::Credential].removeMember( + jss::Issuer); + BEAST_EXPECT( + exceptionExpected(env, txJsonMutable).starts_with("invalidParams")); txJsonMutable["AcceptedCredentials"][2u] = credentialOrig; // Make an empty CredentialType. - txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] + txJsonMutable["AcceptedCredentials"][2u][jss::Credential] ["CredentialType"] = ""; env(txJsonMutable, ter(temMALFORMED)); // Remove Credentialtype from a credential and apply. - txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] - .removeMember("CredentialType"); - env(txJsonMutable, ter(temMALFORMED)); + txJsonMutable["AcceptedCredentials"][2u][jss::Credential].removeMember( + "CredentialType"); + BEAST_EXPECT( + exceptionExpected(env, txJsonMutable).starts_with("invalidParams")); // Remove both - txJsonMutable["AcceptedCredentials"][2u]["AcceptedCredential"] - .removeMember("Issuer"); - env(txJsonMutable, ter(temMALFORMED)); + txJsonMutable["AcceptedCredentials"][2u][jss::Credential].removeMember( + jss::Issuer); + BEAST_EXPECT( + exceptionExpected(env, txJsonMutable).starts_with("invalidParams")); // Make 2 identical credentials. The duplicate should be silently // removed. { pd::Credentials const credentialsDup{ - {alice7, pd::toBlob("credential6")}, - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice2, pd::toBlob("credential1")}, - {alice5, pd::toBlob("credential4")}, + {alice7, "credential6"}, + {alice2, "credential1"}, + {alice3, "credential2"}, + {alice2, "credential1"}, + {alice5, "credential4"}, }; + + std::unordered_map pubKey2Acc; + for (auto const& c : credentialsDup) + pubKey2Acc.emplace(c.issuer.human(), c.issuer); + BEAST_EXPECT(pd::sortCredentials(credentialsDup).size() == 4); env(pd::setTx(account, credentialsDup, domain)); @@ -178,23 +201,26 @@ class PermissionedDomains_test : public beast::unit_test::suite d = pd::getNewDomain(env.meta()); env.close(); auto objects = pd::getObjects(account, env); - auto const fromObject = pd::credentialsFromJson(objects[d]); + auto const fromObject = + pd::credentialsFromJson(objects[d], pubKey2Acc); auto const sortedCreds = pd::sortCredentials(credentialsDup); - BEAST_EXPECT( - pd::credentialsFromJson(objects[d]) == - pd::sortCredentials(credentialsDup)); + BEAST_EXPECT(fromObject == sortedCreds); } // Have equal issuers but different credentials and make sure they // sort correctly. { pd::Credentials const credentialsSame{ - {alice2, pd::toBlob("credential3")}, - {alice3, pd::toBlob("credential2")}, - {alice2, pd::toBlob("credential9")}, - {alice5, pd::toBlob("credential4")}, - {alice2, pd::toBlob("credential6")}, + {alice2, "credential3"}, + {alice3, "credential2"}, + {alice2, "credential9"}, + {alice5, "credential4"}, + {alice2, "credential6"}, }; + std::unordered_map pubKey2Acc; + for (auto const& c : credentialsSame) + pubKey2Acc.emplace(c.issuer.human(), c.issuer); + BEAST_EXPECT( credentialsSame != pd::sortCredentials(credentialsSame)); env(pd::setTx(account, credentialsSame, domain)); @@ -206,11 +232,10 @@ class PermissionedDomains_test : public beast::unit_test::suite d = pd::getNewDomain(env.meta()); env.close(); auto objects = pd::getObjects(account, env); - auto const fromObject = pd::credentialsFromJson(objects[d]); + auto const fromObject = + pd::credentialsFromJson(objects[d], pubKey2Acc); auto const sortedCreds = pd::sortCredentials(credentialsSame); - BEAST_EXPECT( - pd::credentialsFromJson(objects[d]) == - pd::sortCredentials(credentialsSame)); + BEAST_EXPECT(fromObject == sortedCreds); } } @@ -220,97 +245,104 @@ class PermissionedDomains_test : public beast::unit_test::suite { testcase("Set"); Env env(*this, withFeature_); - Account const alice("alice"), alice2("alice2"), alice3("alice3"), - alice4("alice4"), alice5("alice5"), alice6("alice6"), - alice7("alice7"), alice8("alice8"), alice9("alice9"), - alice10("alice10"), alice11("alice11"), alice12("alice12"); - env.fund( - XRP(1000), - alice, - alice2, - alice3, - alice4, - alice5, - alice6, - alice7, - alice8, - alice9, - alice10, - alice11, - alice12); + + const int accNum = 12; + Account const alice[accNum] = { + "alice", + "alice2", + "alice3", + "alice4", + "alice5", + "alice6", + "alice7", + "alice8", + "alice9", + "alice10", + "alice11", + "alice12"}; + std::unordered_map pubKey2Acc; + for (auto const& c : alice) + pubKey2Acc.emplace(c.human(), c); + + for (int i = 0; i < accNum; ++i) + env.fund(XRP(1000), alice[i]); // Create new from existing account with a single credential. - pd::Credentials const credentials1{{alice2, pd::toBlob("credential1")}}; + pd::Credentials const credentials1{{alice[2], "credential1"}}; { - env(pd::setTx(alice, credentials1)); - BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1); + env(pd::setTx(alice[0], credentials1)); + BEAST_EXPECT( + pd::ownerInfo(alice[0], env)["OwnerCount"].asUInt() == 1); auto tx = env.tx()->getJson(JsonOptions::none); BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainSet"); - BEAST_EXPECT(tx["Account"] == alice.human()); - auto objects = pd::getObjects(alice, env); + BEAST_EXPECT(tx["Account"] == alice[0].human()); + auto objects = pd::getObjects(alice[0], env); auto domain = objects.begin()->first; auto object = objects.begin()->second; BEAST_EXPECT(object["LedgerEntryType"] == "PermissionedDomain"); - BEAST_EXPECT(object["Owner"] == alice.human()); + BEAST_EXPECT(object["Owner"] == alice[0].human()); BEAST_EXPECT(object["Sequence"] == tx["Sequence"]); - BEAST_EXPECT(pd::credentialsFromJson(object) == credentials1); + BEAST_EXPECT( + pd::credentialsFromJson(object, pubKey2Acc) == credentials1); } // Create new from existing account with 10 credentials. pd::Credentials const credentials10{ - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice4, pd::toBlob("credential3")}, - {alice5, pd::toBlob("credential4")}, - {alice6, pd::toBlob("credential5")}, - {alice7, pd::toBlob("credential6")}, - {alice8, pd::toBlob("credential7")}, - {alice9, pd::toBlob("credential8")}, - {alice10, pd::toBlob("credential9")}, - {alice11, pd::toBlob("credential10")}, + {alice[2], "credential1"}, + {alice[3], "credential2"}, + {alice[4], "credential3"}, + {alice[5], "credential4"}, + {alice[6], "credential5"}, + {alice[7], "credential6"}, + {alice[8], "credential7"}, + {alice[9], "credential8"}, + {alice[10], "credential9"}, + {alice[11], "credential10"}, }; uint256 domain2; { BEAST_EXPECT( credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); - env(pd::setTx(alice, credentials10)); + env(pd::setTx(alice[0], credentials10)); auto tx = env.tx()->getJson(JsonOptions::none); domain2 = pd::getNewDomain(env.meta()); - auto objects = pd::getObjects(alice, env); + auto objects = pd::getObjects(alice[0], env); auto object = objects[domain2]; BEAST_EXPECT( - pd::credentialsFromJson(object) == + pd::credentialsFromJson(object, pubKey2Acc) == pd::sortCredentials(credentials10)); } // Update with 1 credential. - env(pd::setTx(alice, credentials1, domain2)); + env(pd::setTx(alice[0], credentials1, domain2)); BEAST_EXPECT( - pd::credentialsFromJson(pd::getObjects(alice, env)[domain2]) == + pd::credentialsFromJson( + pd::getObjects(alice[0], env)[domain2], pubKey2Acc) == credentials1); // Update with 10 credentials. - env(pd::setTx(alice, credentials10, domain2)); + env(pd::setTx(alice[0], credentials10, domain2)); env.close(); BEAST_EXPECT( - pd::credentialsFromJson(pd::getObjects(alice, env)[domain2]) == + pd::credentialsFromJson( + pd::getObjects(alice[0], env)[domain2], pubKey2Acc) == pd::sortCredentials(credentials10)); // Update from the wrong owner. - env(pd::setTx(alice2, credentials1, domain2), + env(pd::setTx(alice[2], credentials1, domain2), ter(temINVALID_ACCOUNT_ID)); // Update a uint256(0) domain - env(pd::setTx(alice, credentials1, uint256(0)), ter(temMALFORMED)); + env(pd::setTx(alice[0], credentials1, uint256(0)), ter(temMALFORMED)); // Update non-existent domain - env(pd::setTx(alice, credentials1, uint256(75)), ter(tecNO_ENTRY)); + env(pd::setTx(alice[0], credentials1, uint256(75)), ter(tecNO_ENTRY)); // Test bad data when creating a domain. - testBadData(alice, env); + testBadData(alice[0], env); // Test bad data when updating a domain. - testBadData(alice, env, domain2); + testBadData(alice[0], env, domain2); // Try to delete the account with domains. auto const acctDelFee(drops(env.current()->fees().increment)); @@ -318,24 +350,24 @@ class PermissionedDomains_test : public beast::unit_test::suite { // Close enough ledgers to make it potentially deletable if empty. std::size_t ownerSeq = - pd::ownerInfo(alice, env)["Sequence"].asUInt(); + pd::ownerInfo(alice[0], env)["Sequence"].asUInt(); while (deleteDelta + ownerSeq > env.current()->seq()) env.close(); - env(acctdelete(alice, alice2), + env(acctdelete(alice[0], alice[2]), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); } { // Delete the domains and then the owner account. - for (auto const& objs : pd::getObjects(alice, env)) - env(pd::deleteTx(alice, objs.first)); + for (auto const& objs : pd::getObjects(alice[0], env)) + env(pd::deleteTx(alice[0], objs.first)); env.close(); std::size_t ownerSeq = - pd::ownerInfo(alice, env)["Sequence"].asUInt(); + pd::ownerInfo(alice[0], env)["Sequence"].asUInt(); while (deleteDelta + ownerSeq > env.current()->seq()) env.close(); - env(acctdelete(alice, alice2), fee(acctDelFee)); + env(acctdelete(alice[0], alice[2]), fee(acctDelFee)); } } @@ -349,7 +381,7 @@ class PermissionedDomains_test : public beast::unit_test::suite env.fund(XRP(1000), alice); auto const setFee(drops(env.current()->fees().increment)); - pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + pd::Credentials credentials{{alice, "first credential"}}; env(pd::setTx(alice, credentials)); env.close(); auto objects = pd::getObjects(alice, env); @@ -403,7 +435,7 @@ class PermissionedDomains_test : public beast::unit_test::suite BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); // alice does not have enough XRP to cover the reserve. - pd::Credentials credentials{{alice, pd::toBlob("first credential")}}; + pd::Credentials credentials{{alice, "first credential"}}; env(pd::setTx(alice, credentials), ter(tecINSUFFICIENT_RESERVE)); BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0); BEAST_EXPECT(pd::getObjects(alice, env).size() == 0); diff --git a/src/test/jtx.h b/src/test/jtx.h index d9ef285a61d..f3c69ce33c3 100644 --- a/src/test/jtx.h +++ b/src/test/jtx.h @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -51,6 +50,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/jtx/Account.h b/src/test/jtx/Account.h index bcf117d9357..f49070dff03 100644 --- a/src/test/jtx/Account.h +++ b/src/test/jtx/Account.h @@ -158,10 +158,10 @@ hash_append(Hasher& h, Account const& v) noexcept hash_append(h, v.id()); } -inline bool -operator<(Account const& lhs, Account const& rhs) noexcept +inline auto +operator<=>(Account const& lhs, Account const& rhs) noexcept { - return lhs.id() < rhs.id(); + return lhs.id() <=> rhs.id(); } } // namespace jtx diff --git a/src/test/jtx/deposit.h b/src/test/jtx/deposit.h index 9de3416367c..9bd73d383dd 100644 --- a/src/test/jtx/deposit.h +++ b/src/test/jtx/deposit.h @@ -43,6 +43,9 @@ struct AuthorizeCredentials jtx::Account issuer; std::string credType; + auto + operator<=>(const AuthorizeCredentials&) const = default; + Json::Value toJson() const { diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/permissioned_domains.cpp similarity index 88% rename from src/test/jtx/impl/PermissionedDomains.cpp rename to src/test/jtx/impl/permissioned_domains.cpp index 7e7eb964415..8ec4f9842c6 100644 --- a/src/test/jtx/impl/PermissionedDomains.cpp +++ b/src/test/jtx/impl/permissioned_domains.cpp @@ -17,7 +17,7 @@ */ //============================================================================== -#include +#include #include namespace ripple { @@ -42,12 +42,8 @@ setTx( for (auto const& credential : credentials) { Json::Value obj(Json::objectValue); - obj[sfIssuer.jsonName] = to_string(credential.first); - obj[sfCredentialType.jsonName] = - strHex(Slice{credential.second.data(), credential.second.size()}); - Json::Value o2(Json::objectValue); - o2[sfAcceptedCredential.jsonName] = obj; - a.append(o2); + obj[sfCredential.jsonName] = credential.toJson(); + a.append(std::move(obj)); } jv[sfAcceptedCredentials.jsonName] = a; return jv; @@ -105,7 +101,9 @@ objectExists(uint256 const& objID, Env& env) // Extract credentials from account_object object Credentials -credentialsFromJson(Json::Value const& object) +credentialsFromJson( + Json::Value const& object, + std::unordered_map const& pubKey2Acc) { Credentials ret; Json::Value a(Json::arrayValue); @@ -113,12 +111,13 @@ credentialsFromJson(Json::Value const& object) for (auto const& credential : a) { Json::Value obj(Json::objectValue); - obj = credential["AcceptedCredential"]; - auto const& issuer = obj["Issuer"]; + obj = credential[jss::Credential]; + auto const& issuer = obj[jss::Issuer]; auto const& credentialType = obj["CredentialType"]; - ret.emplace_back( - *parseBase58(issuer.asString()), - strUnHex(credentialType.asString()).value()); + auto blob = strUnHex(credentialType.asString()).value(); + ret.push_back( + {pubKey2Acc.at(issuer.asString()), + std::string(blob.begin(), blob.end())}); } return ret; } diff --git a/src/test/jtx/PermissionedDomains.h b/src/test/jtx/permissioned_domains.h similarity index 85% rename from src/test/jtx/PermissionedDomains.h rename to src/test/jtx/permissioned_domains.h index 3ddf08096f7..ba610ec3ecc 100644 --- a/src/test/jtx/PermissionedDomains.h +++ b/src/test/jtx/permissioned_domains.h @@ -17,10 +17,10 @@ */ //============================================================================== -#ifndef RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED -#define RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED +#pragma once #include +#include namespace ripple { namespace test { @@ -28,7 +28,7 @@ namespace jtx { namespace pd { // Helpers for PermissionedDomains testing -using Credential = std::pair; +using Credential = ripple::test::jtx::deposit::AuthorizeCredentials; using Credentials = std::vector; // helpers @@ -51,16 +51,11 @@ getObjects(Account const& account, Env& env, bool withType = true); bool objectExists(uint256 const& objID, Env& env); -// Convert string to Blob -inline Blob -toBlob(std::string const& input) -{ - return Blob(input.begin(), input.end()); -} - // Extract credentials from account_object object Credentials -credentialsFromJson(Json::Value const& object); +credentialsFromJson( + Json::Value const& object, + std::unordered_map const& pubKey2Acc); // Sort credentials the same way as PermissionedDomainSet Credentials @@ -78,5 +73,3 @@ getNewDomain(std::shared_ptr const& meta); } // namespace jtx } // namespace test } // namespace ripple - -#endif // RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 611bc62991f..76e9feeb83b 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -3071,7 +3071,7 @@ class LedgerRPC_test : public beast::unit_test::suite env.close(); auto const seq = env.seq(alice); - env(pd::setTx(alice, {{alice, pd::toBlob("first credential")}})); + env(pd::setTx(alice, {{alice, "first credential"}})); env.close(); auto const objects = pd::getObjects(alice, env); BEAST_EXPECT(objects.size() == 1); From 7de5cb53ed4637024e4a5734ea1d3760473a1bf0 Mon Sep 17 00:00:00 2001 From: Oleksandr <115580134+oleks-rip@users.noreply.github.com> Date: Wed, 13 Nov 2024 13:04:08 -0500 Subject: [PATCH 09/11] Align with XLS-70 --- include/xrpl/protocol/Protocol.h | 4 + src/test/app/PermissionedDomains_test.cpp | 21 +++-- src/xrpld/app/misc/CredentialHelpers.cpp | 26 ++++++ src/xrpld/app/misc/CredentialHelpers.h | 9 +- src/xrpld/app/tx/detail/DepositPreauth.cpp | 48 ++-------- src/xrpld/app/tx/detail/InvariantCheck.cpp | 35 ++++++- src/xrpld/app/tx/detail/InvariantCheck.h | 1 + .../tx/detail/PermissionedDomainDelete.cpp | 11 ++- .../app/tx/detail/PermissionedDomainDelete.h | 7 +- .../app/tx/detail/PermissionedDomainSet.cpp | 92 ++++++++----------- .../app/tx/detail/PermissionedDomainSet.h | 8 +- 11 files changed, 137 insertions(+), 125 deletions(-) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index a9bd10a6fd1..dda8c24238f 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -104,6 +104,10 @@ std::size_t constexpr maxCredentialTypeLength = 64; /** The maximum number of credentials can be passed in array */ std::size_t constexpr maxCredentialsArraySize = 8; +/** The maximum number of credentials can be passed in array for permissioned + * domain */ +std::size_t constexpr maxPermissionedDomainCredentialsArraySize = 10; + /** The maximum length of MPTokenMetadata */ std::size_t constexpr maxMPTokenMetadataLength = 1024; diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index 7ac6eaad73a..7955aa7fe02 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -128,7 +128,8 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice11, "credential10"}, {alice12, "credential11"}}; BEAST_EXPECT( - credentials11.size() == PermissionedDomainSet::PD_ARRAY_MAX + 1); + credentials11.size() == + maxPermissionedDomainCredentialsArraySize + 1); env(pd::setTx(account, credentials11, domain), ter(temMALFORMED)); // Test credentials including non-existent issuer. @@ -141,7 +142,7 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice5, "credential5"}, {alice6, "credential6"}, {alice7, "credential7"}}; - env(pd::setTx(account, credentialsNon, domain), ter(temBAD_ISSUER)); + env(pd::setTx(account, credentialsNon, domain), ter(tecNO_ISSUER)); pd::Credentials const credentials4{ {alice2, "credential1"}, @@ -191,8 +192,12 @@ class PermissionedDomains_test : public beast::unit_test::suite for (auto const& c : credentialsDup) pubKey2Acc.emplace(c.issuer.human(), c.issuer); - BEAST_EXPECT(pd::sortCredentials(credentialsDup).size() == 4); - env(pd::setTx(account, credentialsDup, domain)); + auto const sorted = pd::sortCredentials(credentialsDup); + BEAST_EXPECT(sorted.size() == 4); + env(pd::setTx(account, credentialsDup, domain), ter(temMALFORMED)); + + env.close(); + env(pd::setTx(account, sorted, domain)); uint256 d; if (domain) @@ -302,7 +307,8 @@ class PermissionedDomains_test : public beast::unit_test::suite uint256 domain2; { BEAST_EXPECT( - credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); + credentials10.size() == + maxPermissionedDomainCredentialsArraySize); BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); env(pd::setTx(alice[0], credentials10)); auto tx = env.tx()->getJson(JsonOptions::none); @@ -330,8 +336,7 @@ class PermissionedDomains_test : public beast::unit_test::suite pd::sortCredentials(credentials10)); // Update from the wrong owner. - env(pd::setTx(alice[2], credentials1, domain2), - ter(temINVALID_ACCOUNT_ID)); + env(pd::setTx(alice[2], credentials1, domain2), ter(tecNO_PERMISSION)); // Update a uint256(0) domain env(pd::setTx(alice[0], credentials1, uint256(0)), ter(temMALFORMED)); @@ -391,7 +396,7 @@ class PermissionedDomains_test : public beast::unit_test::suite // Delete a domain that doesn't belong to the account. Account const bob("bob"); env.fund(XRP(1000), bob); - env(pd::deleteTx(bob, domain), ter(temINVALID_ACCOUNT_ID)); + env(pd::deleteTx(bob, domain), ter(tecNO_PERMISSION)); // Delete a non-existent domain. env(pd::deleteTx(alice, uint256(75)), ter(tecNO_ENTRY)); diff --git a/src/xrpld/app/misc/CredentialHelpers.cpp b/src/xrpld/app/misc/CredentialHelpers.cpp index 08b5d804d4b..81ba6aa0b13 100644 --- a/src/xrpld/app/misc/CredentialHelpers.cpp +++ b/src/xrpld/app/misc/CredentialHelpers.cpp @@ -19,6 +19,7 @@ #include #include +#include #include @@ -225,6 +226,31 @@ makeSorted(STArray const& in) return out; } +NotTEC +checkArray(STArray const& in, unsigned maxSize) +{ + if (in.empty() || (in.size() > maxSize)) + return temMALFORMED; + + std::unordered_set duplicates; + for (auto const& credential : in) + { + auto const& issuer(credential[sfIssuer]); + if (!issuer) + return temINVALID_ACCOUNT_ID; + + auto const ct = credential[sfCredentialType]; + if (ct.empty() || (ct.size() > maxCredentialTypeLength)) + return temMALFORMED; + + auto [it, ins] = duplicates.insert(sha512Half(issuer, ct)); + if (!ins) + return temMALFORMED; + } + + return tesSUCCESS; +} + } // namespace credentials TER diff --git a/src/xrpld/app/misc/CredentialHelpers.h b/src/xrpld/app/misc/CredentialHelpers.h index 3291fc1daa6..97b8436b30b 100644 --- a/src/xrpld/app/misc/CredentialHelpers.h +++ b/src/xrpld/app/misc/CredentialHelpers.h @@ -21,8 +21,6 @@ #include -#include - namespace ripple { namespace credentials { @@ -60,10 +58,15 @@ valid(PreclaimContext const& ctx, AccountID const& src); TER authorized(ApplyContext const& ctx, AccountID const& dst); -// return empty set if there are duplicates +// Sort credentials array, return empty set if there are duplicates std::set> makeSorted(STArray const& in); +// Check credentials array passed to DepositPreauth/PermissionedDomainSet +// transactions +NotTEC +checkArray(STArray const& in, unsigned maxSize); + } // namespace credentials // Check expired credentials and for existing DepositPreauth ledger object diff --git a/src/xrpld/app/tx/detail/DepositPreauth.cpp b/src/xrpld/app/tx/detail/DepositPreauth.cpp index 73cd19e4120..df19a8aa34d 100644 --- a/src/xrpld/app/tx/detail/DepositPreauth.cpp +++ b/src/xrpld/app/tx/detail/DepositPreauth.cpp @@ -24,11 +24,9 @@ #include #include #include -#include #include #include -#include namespace ripple { @@ -94,45 +92,13 @@ DepositPreauth::preflight(PreflightContext const& ctx) } else { - STArray const& arr(ctx.tx.getFieldArray( - authArrPresent ? sfAuthorizeCredentials - : sfUnauthorizeCredentials)); - if (arr.empty() || (arr.size() > maxCredentialsArraySize)) - { - JLOG(ctx.j.trace()) << "Malformed transaction: " - "Invalid AuthorizeCredentials size: " - << arr.size(); - return temMALFORMED; - } - - std::unordered_set duplicates; - for (auto const& o : arr) - { - auto const& issuer(o[sfIssuer]); - if (!issuer) - { - JLOG(ctx.j.trace()) - << "Malformed transaction: " - "AuthorizeCredentials Issuer account is invalid."; - return temINVALID_ACCOUNT_ID; - } - - auto const ct = o[sfCredentialType]; - if (ct.empty() || (ct.size() > maxCredentialTypeLength)) - { - JLOG(ctx.j.trace()) - << "Malformed transaction: invalid size of CredentialType."; - return temMALFORMED; - } - - auto [it, ins] = duplicates.insert(sha512Half(issuer, ct)); - if (!ins) - { - JLOG(ctx.j.trace()) - << "Malformed transaction: duplicates in credentials."; - return temMALFORMED; - } - } + if (auto err = credentials::checkArray( + ctx.tx.getFieldArray( + authArrPresent ? sfAuthorizeCredentials + : sfUnauthorizeCredentials), + maxCredentialsArraySize); + !isTesSuccess(err)) + return err; } return preflight2(ctx); diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 2002c8911b3..99d8aa5c17b 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -19,6 +19,7 @@ #include +#include #include #include #include @@ -1132,7 +1133,23 @@ ValidPermissionedDomain::visitEntry( { if (after->getType() != ltPERMISSIONED_DOMAIN) return; - credentialsSize_ = after->getFieldArray(sfAcceptedCredentials).size(); + auto const& credentials = after->getFieldArray(sfAcceptedCredentials); + credentialsSize_ = credentials.size(); + auto const sorted = credentials::makeSorted(credentials); + isUnique_ = !sorted.empty(); + + if (isUnique_) + { + unsigned i = 0; + for (auto const& cred : sorted) + { + auto const& credTx(credentials[i++]); + isSorted_ = (cred.first == credTx[sfIssuer]) && + (cred.second == credTx[sfCredentialType]); + if (!isSorted_) + break; + } + } } bool @@ -1153,7 +1170,7 @@ ValidPermissionedDomain::finalize( return false; } - if (credentialsSize_ > PermissionedDomainSet::PD_ARRAY_MAX) + if (credentialsSize_ > maxPermissionedDomainCredentialsArraySize) { JLOG(j.fatal()) << "Invariant failed: permissioned domain bad " "credentials size " @@ -1161,6 +1178,20 @@ ValidPermissionedDomain::finalize( return false; } + if (!isSorted_) + { + JLOG(j.fatal()) << "Invariant failed: permissioned domain credentials " + "aren't sorted"; + return false; + } + + if (!isUnique_) + { + JLOG(j.fatal()) << "Invariant failed: permissioned domain credentials " + "aren't unique"; + return false; + } + return true; } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 13ccc5fb179..d8f2dece0ec 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -486,6 +486,7 @@ class ValidMPTIssuance class ValidPermissionedDomain { std::size_t credentialsSize_{0}; + bool isSorted_ = false, isUnique_ = false; public: void diff --git a/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp index a1a5418f5c9..321f054dbcd 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp @@ -29,23 +29,26 @@ PermissionedDomainDelete::preflight(PreflightContext const& ctx) return temDISABLED; if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; + + assert(ctx.tx.isFieldPresent(sfDomainID)); + auto const domain = ctx.tx.getFieldH256(sfDomainID); + if (domain == beast::zero) + return temMALFORMED; + return preflight2(ctx); } TER PermissionedDomainDelete::preclaim(PreclaimContext const& ctx) { - assert(ctx.tx.isFieldPresent(sfDomainID)); auto const domain = ctx.tx.getFieldH256(sfDomainID); - if (domain == beast::zero) - return temMALFORMED; auto const sleDomain = ctx.view.read({ltPERMISSIONED_DOMAIN, domain}); if (!sleDomain) return tecNO_ENTRY; assert( sleDomain->isFieldPresent(sfOwner) && ctx.tx.isFieldPresent(sfAccount)); if (sleDomain->getAccountID(sfOwner) != ctx.tx.getAccountID(sfAccount)) - return temINVALID_ACCOUNT_ID; + return tecNO_PERMISSION; return tesSUCCESS; } diff --git a/src/xrpld/app/tx/detail/PermissionedDomainDelete.h b/src/xrpld/app/tx/detail/PermissionedDomainDelete.h index 3fdf50ed298..ce0d1f14d86 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainDelete.h +++ b/src/xrpld/app/tx/detail/PermissionedDomainDelete.h @@ -17,8 +17,7 @@ */ //============================================================================== -#ifndef RIPPLE_TX_PERMISSIONEDDOMAINDELETE_H_INCLUDED -#define RIPPLE_TX_PERMISSIONEDDOMAINDELETE_H_INCLUDED +#pragma once #include @@ -39,11 +38,9 @@ class PermissionedDomainDelete : public Transactor static TER preclaim(PreclaimContext const& ctx); - /** Attempt to create the Permissioned Domain. */ + /** Attempt to delete the Permissioned Domain. */ TER doApply() override; }; } // namespace ripple - -#endif // RIPPLE_TX_PERMISSIONEDDOMAINDELETE_H_INCLUDED diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index f64cef0a00c..73c3b7478ae 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -17,11 +17,12 @@ */ //============================================================================== +#include #include #include #include #include -#include + #include namespace ripple { @@ -34,20 +35,11 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) return ret; - auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); - if (credentials.empty() || credentials.size() > PD_ARRAY_MAX) - return temMALFORMED; - for (auto const& credential : credentials) - { - if (!credential.isFieldPresent(sfIssuer) || - !credential.isFieldPresent(sfCredentialType)) - { - return temMALFORMED; - } - auto sz = credential.getFieldVL(sfCredentialType).size(); - if (!sz || sz > 64) - return temMALFORMED; - } + if (auto err = credentials::checkArray( + ctx.tx.getFieldArray(sfAcceptedCredentials), + maxPermissionedDomainCredentialsArraySize); + !isTesSuccess(err)) + return err; auto const domain = ctx.tx.at(~sfDomainID); if (domain && *domain == beast::zero) @@ -59,26 +51,28 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx) TER PermissionedDomainSet::preclaim(PreclaimContext const& ctx) { - if (!ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount)))) + auto const account = ctx.tx.getAccountID(sfAccount); + + if (!ctx.view.exists(keylet::account(account))) return tefINTERNAL; - auto const credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); + auto const& credentials = ctx.tx.getFieldArray(sfAcceptedCredentials); for (auto const& credential : credentials) { - if (!ctx.view.read(keylet::account(credential.getAccountID(sfIssuer)))) - return temBAD_ISSUER; + if (!ctx.view.exists( + keylet::account(credential.getAccountID(sfIssuer)))) + return tecNO_ISSUER; } - if (!ctx.tx.isFieldPresent(sfDomainID)) - return tesSUCCESS; - auto const domain = ctx.tx.getFieldH256(sfDomainID); - auto const sleDomain = ctx.view.read(keylet::permissionedDomain(domain)); - if (!sleDomain) - return tecNO_ENTRY; - auto const owner = sleDomain->getAccountID(sfOwner); - auto account = ctx.tx.getAccountID(sfAccount); - if (owner != account) - return temINVALID_ACCOUNT_ID; + if (ctx.tx.isFieldPresent(sfDomainID)) + { + auto const sleDomain = ctx.view.read( + keylet::permissionedDomain(ctx.tx.getFieldH256(sfDomainID))); + if (!sleDomain) + return tecNO_ENTRY; + if (sleDomain->getAccountID(sfOwner) != account) + return tecNO_PERMISSION; + } return tesSUCCESS; } @@ -88,30 +82,19 @@ TER PermissionedDomainSet::doApply() { auto const ownerSle = view().peek(keylet::account(account_)); + if (!ownerSle) + return tefINTERNAL; - // The purpose of this lambda is to modify the SLE for either creating a - // new or updating an existing object, to reduce code repetition. - // All checks have already been done. Just update credentials. Same logic - // for either new domain or updating existing. - // Silently remove duplicates. - auto updateSle = [this](std::shared_ptr const& sle) { - auto const& credentials = ctx_.tx.getFieldArray(sfAcceptedCredentials); - - std::map< - std::pair, - std::reference_wrapper> - credMap; - for (auto const& c : credentials) - credMap.insert( - {{c.getAccountID(sfIssuer), c.getFieldVL(sfCredentialType)}, - c}); - - STArray credSorted; - credSorted.reserve(credMap.size()); - for (auto const& [k, v] : credMap) - credSorted.push_back(v); - sle->setFieldArray(sfAcceptedCredentials, credSorted); - }; + auto const sortedTX = + credentials::makeSorted(ctx_.tx.getFieldArray(sfAcceptedCredentials)); + STArray sortedLE(sfAcceptedCredentials, sortedTX.size()); + for (auto const& p : sortedTX) + { + auto cred = STObject::makeInnerObject(sfCredential); + cred.setAccountID(sfIssuer, p.first); + cred.setFieldVL(sfCredentialType, p.second); + sortedLE.push_back(std::move(cred)); + } if (ctx_.tx.isFieldPresent(sfDomainID)) { @@ -120,8 +103,7 @@ PermissionedDomainSet::doApply() keylet::permissionedDomain(ctx_.tx.getFieldH256(sfDomainID))); if (!slePd) return tefINTERNAL; - - updateSle(slePd); + slePd->peekFieldArray(sfAcceptedCredentials) = std::move(sortedLE); view().update(slePd); } else @@ -142,7 +124,7 @@ PermissionedDomainSet::doApply() slePd->setAccountID(sfOwner, account_); slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); - updateSle(slePd); + slePd->peekFieldArray(sfAcceptedCredentials) = std::move(sortedLE); auto const page = view().dirInsert( keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_)); if (!page) diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.h b/src/xrpld/app/tx/detail/PermissionedDomainSet.h index fcc507d1867..7419770946f 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.h +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.h @@ -16,19 +16,15 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ //============================================================================== - -#ifndef RIPPLE_TX_PERMISSIONEDDOMAINSET_H_INCLUDED -#define RIPPLE_TX_PERMISSIONEDDOMAINSET_H_INCLUDED +#pragma once #include -#include namespace ripple { class PermissionedDomainSet : public Transactor { public: - static constexpr std::size_t PD_ARRAY_MAX = 10; static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; explicit PermissionedDomainSet(ApplyContext& ctx) : Transactor(ctx) @@ -47,5 +43,3 @@ class PermissionedDomainSet : public Transactor }; } // namespace ripple - -#endif // RIPPLE_TX_PERMISSIONEDDOMAINSET_H_INCLUDED From e8ca0196eb3eb4180d35296a32efe1da1cfda0bf Mon Sep 17 00:00:00 2001 From: Oleksandr <115580134+oleks-rip@users.noreply.github.com> Date: Wed, 13 Nov 2024 13:49:13 -0500 Subject: [PATCH 10/11] Added more UT --- src/test/app/PermissionedDomains_test.cpp | 9 ++++++++- src/test/jtx/fee.h | 3 ++- src/test/rpc/LedgerRPC_test.cpp | 20 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index 7955aa7fe02..52c9e02aa03 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -26,7 +26,6 @@ #include #include -#include #include #include #include @@ -144,6 +143,11 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice7, "credential7"}}; env(pd::setTx(account, credentialsNon, domain), ter(tecNO_ISSUER)); + // Test bad fee + env(pd::setTx(account, credentials11, domain), + fee(1, true), + ter(temBAD_FEE)); + pd::Credentials const credentials4{ {alice2, "credential1"}, {alice3, "credential2"}, @@ -401,6 +405,9 @@ class PermissionedDomains_test : public beast::unit_test::suite // Delete a non-existent domain. env(pd::deleteTx(alice, uint256(75)), ter(tecNO_ENTRY)); + // Test bad fee + env(pd::deleteTx(alice, uint256(75)), ter(temBAD_FEE), fee(1, true)); + // Delete a zero domain. env(pd::deleteTx(alice, uint256(0)), ter(temMALFORMED)); diff --git a/src/test/jtx/fee.h b/src/test/jtx/fee.h index c671e0b2a1e..4e29fad1521 100644 --- a/src/test/jtx/fee.h +++ b/src/test/jtx/fee.h @@ -53,7 +53,8 @@ class fee Throw("fee: not XRP"); } - explicit fee(std::uint64_t amount) : fee{STAmount{amount}} + explicit fee(std::uint64_t amount, bool negative = false) + : fee{STAmount{amount, negative}} { } diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 76e9feeb83b..99b0f0a23fc 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -3113,6 +3113,26 @@ class LedgerRPC_test : public beast::unit_test::suite jss::PermissionedDomain); } + { + // Fail, invalid permissioned domain index + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain] = + "12F1F1F1F180D67377B2FAB292A31C922470326268D2B9B74CD1E582645B9A" + "DE"; + auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); + checkErrorValue(jrr[jss::result], "entryNotFound", ""); + } + + { + // Fail, invalid permissioned domain index + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain] = "NotAHexString"; + auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); + checkErrorValue(jrr[jss::result], "malformedRequest", ""); + } + { // Fail, invalid account Json::Value params; From ceeec9f680e7b471e6eb27ce6d61b50885c5849f Mon Sep 17 00:00:00 2001 From: Oleksandr <115580134+oleks-rip@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:48:28 -0500 Subject: [PATCH 11/11] Invariant tests check --- src/test/ledger/Invariants_test.cpp | 13 ++++++--- src/xrpld/app/misc/CredentialHelpers.cpp | 29 +++++++++++++++---- src/xrpld/app/misc/CredentialHelpers.h | 4 +-- src/xrpld/app/tx/detail/DepositPreauth.cpp | 3 +- .../app/tx/detail/PermissionedDomainSet.cpp | 3 +- 5 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 765b47e9796..52263d2dd80 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -804,7 +804,7 @@ class Invariants_test : public beast::unit_test::suite using namespace test::jtx; testcase << "PermissionedDomain"; doInvariantCheck( - {{"permissioned domain with no rules"}}, + {{"permissioned domain with no rules."}}, [](Account const& A1, Account const&, ApplyContext& ac) { Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); auto slePd = std::make_shared(pdKeylet); @@ -820,15 +820,20 @@ class Invariants_test : public beast::unit_test::suite doInvariantCheck( {{"permissioned domain bad credentials size 11"}}, - [](Account const& A1, Account const&, ApplyContext& ac) { + [](Account const& A1, Account const& A2, ApplyContext& ac) { Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); auto slePd = std::make_shared(pdKeylet); slePd->setAccountID(sfOwner, A1); slePd->setFieldU32(sfSequence, 10); - STArray credentials(sfAcceptedCredentials); + STArray credentials(sfAcceptedCredentials, 11); for (std::size_t n = 0; n < 11; ++n) - credentials.push_back(STObject(sfSequence)); + { + auto cred = STObject::makeInnerObject(sfCredential); + cred.setAccountID(sfIssuer, A2); + cred.setFieldVL(sfCredentialType, Slice("cred_type", 9)); + credentials.push_back(std::move(cred)); + } slePd->setFieldArray(sfAcceptedCredentials, credentials); ac.view().insert(slePd); return true; diff --git a/src/xrpld/app/misc/CredentialHelpers.cpp b/src/xrpld/app/misc/CredentialHelpers.cpp index 81ba6aa0b13..8e9a902a20a 100644 --- a/src/xrpld/app/misc/CredentialHelpers.cpp +++ b/src/xrpld/app/misc/CredentialHelpers.cpp @@ -214,10 +214,10 @@ authorized(ApplyContext const& ctx, AccountID const& dst) } std::set> -makeSorted(STArray const& in) +makeSorted(STArray const& credentials) { std::set> out; - for (auto const& cred : in) + for (auto const& cred : credentials) { auto [it, ins] = out.emplace(cred[sfIssuer], cred[sfCredentialType]); if (!ins) @@ -227,25 +227,44 @@ makeSorted(STArray const& in) } NotTEC -checkArray(STArray const& in, unsigned maxSize) +checkArray(STArray const& credentials, unsigned maxSize, beast::Journal j) { - if (in.empty() || (in.size() > maxSize)) + if (credentials.empty() || (credentials.size() > maxSize)) + { + JLOG(j.trace()) << "Malformed transaction: " + "Invalid credentials size: " + << credentials.size(); return temMALFORMED; + } std::unordered_set duplicates; - for (auto const& credential : in) + for (auto const& credential : credentials) { auto const& issuer(credential[sfIssuer]); if (!issuer) + { + JLOG(j.trace()) << "Malformed transaction: " + "Issuer account is invalid: " + << to_string(issuer); return temINVALID_ACCOUNT_ID; + } auto const ct = credential[sfCredentialType]; if (ct.empty() || (ct.size() > maxCredentialTypeLength)) + { + JLOG(j.trace()) << "Malformed transaction: " + "Invalid credentialType size: " + << ct.size(); return temMALFORMED; + } auto [it, ins] = duplicates.insert(sha512Half(issuer, ct)); if (!ins) + { + JLOG(j.trace()) << "Malformed transaction: " + "duplicates in credenentials."; return temMALFORMED; + } } return tesSUCCESS; diff --git a/src/xrpld/app/misc/CredentialHelpers.h b/src/xrpld/app/misc/CredentialHelpers.h index 97b8436b30b..acc4f2621db 100644 --- a/src/xrpld/app/misc/CredentialHelpers.h +++ b/src/xrpld/app/misc/CredentialHelpers.h @@ -60,12 +60,12 @@ authorized(ApplyContext const& ctx, AccountID const& dst); // Sort credentials array, return empty set if there are duplicates std::set> -makeSorted(STArray const& in); +makeSorted(STArray const& credentials); // Check credentials array passed to DepositPreauth/PermissionedDomainSet // transactions NotTEC -checkArray(STArray const& in, unsigned maxSize); +checkArray(STArray const& credentials, unsigned maxSize, beast::Journal j); } // namespace credentials diff --git a/src/xrpld/app/tx/detail/DepositPreauth.cpp b/src/xrpld/app/tx/detail/DepositPreauth.cpp index df19a8aa34d..599fcd60526 100644 --- a/src/xrpld/app/tx/detail/DepositPreauth.cpp +++ b/src/xrpld/app/tx/detail/DepositPreauth.cpp @@ -96,7 +96,8 @@ DepositPreauth::preflight(PreflightContext const& ctx) ctx.tx.getFieldArray( authArrPresent ? sfAuthorizeCredentials : sfUnauthorizeCredentials), - maxCredentialsArraySize); + maxCredentialsArraySize, + ctx.j); !isTesSuccess(err)) return err; } diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index 73c3b7478ae..d0ec770e06e 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -37,7 +37,8 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx) if (auto err = credentials::checkArray( ctx.tx.getFieldArray(sfAcceptedCredentials), - maxPermissionedDomainCredentialsArraySize); + maxPermissionedDomainCredentialsArraySize, + ctx.j); !isTesSuccess(err)) return err;