diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h index dcd7fd4b76b..d4e4965ee10 100644 --- a/include/xrpl/protocol/Indexes.h +++ b/include/xrpl/protocol/Indexes.h @@ -290,6 +290,8 @@ oracle(AccountID const& account, std::uint32_t const& documentID) 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 0eddfdc6e2c..36b9a55a139 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -460,6 +460,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..22cd7d0f846 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -52,9 +52,8 @@ 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); @@ -70,9 +69,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 +96,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 +113,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 +125,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 +139,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 +168,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 +196,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 +237,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 +273,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 +283,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 +349,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 +381,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 +466,7 @@ class PermissionedDomains_test : public beast::unit_test::suite testDisabled(); testSet(); testDelete(); + testAccountReserve(); } }; 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);