Skip to content

Commit

Permalink
Review fixes from Denis.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mtrippled
mtrippled committed Oct 14, 2024
1 parent e08bef8 commit 7587f0e
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 54 deletions.
2 changes: 2 additions & 0 deletions include/xrpl/protocol/Indexes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 6 additions & 0 deletions src/libxrpl/protocol/Indexes.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
121 changes: 92 additions & 29 deletions src/test/app/PermissionedDomains_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
}

Expand All @@ -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{
Expand All @@ -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");
Expand All @@ -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")},
Expand All @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -404,6 +466,7 @@ class PermissionedDomains_test : public beast::unit_test::suite
testDisabled();
testSet();
testDelete();
testAccountReserve();
}
};

Expand Down
53 changes: 31 additions & 22 deletions src/xrpld/app/tx/detail/PermissionedDomainSet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -56,27 +61,17 @@ 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;
}

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);
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}
Expand All @@ -142,14 +151,14 @@ 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)
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);
view().insert(slePd);
}

return tesSUCCESS;
Expand Down
3 changes: 0 additions & 3 deletions src/xrpld/app/tx/detail/PermissionedDomainSet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit 7587f0e

Please sign in to comment.