Skip to content

Commit

Permalink
Review fixes 8
Browse files Browse the repository at this point in the history
  • Loading branch information
@oleks-rip
oleks-rip committed Dec 17, 2024
1 parent ab0290f commit bc8f025
Show file tree
Hide file tree
Showing 5 changed files with 215 additions and 15 deletions.
61 changes: 57 additions & 4 deletions src/test/app/PermissionedDomains_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,22 @@ class PermissionedDomains_test : public beast::unit_test::suite
BEAST_EXPECT(
exceptionExpected(env, txJsonMutable).starts_with("invalidParams"));

txJsonMutable["AcceptedCredentials"][2u] = credentialOrig;
// Make an empty CredentialType.
txJsonMutable["AcceptedCredentials"][2u] = credentialOrig;
txJsonMutable["AcceptedCredentials"][2u][jss::Credential]
["CredentialType"] = "";
env(txJsonMutable, ter(temMALFORMED));

// Make too long CredentialType.
constexpr std::string_view longCredentialType =
"Cred0123456789012345678901234567890123456789012345678901234567890";
static_assert(longCredentialType.size() == maxCredentialTypeLength + 1);
txJsonMutable["AcceptedCredentials"][2u] = credentialOrig;
txJsonMutable["AcceptedCredentials"][2u][jss::Credential]
["CredentialType"] = std::string(longCredentialType);
BEAST_EXPECT(
exceptionExpected(env, txJsonMutable).starts_with("invalidParams"));

// Remove Credentialtype from a credential and apply.
txJsonMutable["AcceptedCredentials"][2u][jss::Credential].removeMember(
"CredentialType");
Expand All @@ -182,8 +192,8 @@ class PermissionedDomains_test : public beast::unit_test::suite
BEAST_EXPECT(
exceptionExpected(env, txJsonMutable).starts_with("invalidParams"));

// Make 2 identical credentials. The duplicate should be silently
// removed.
// Make 2 identical credentials. Duplicates are not supported by
// permissioned domains, so transactions should return errors
{
pdomain::Credentials const credentialsDup{
{alice7, "credential6"},
Expand Down Expand Up @@ -299,6 +309,44 @@ class PermissionedDomains_test : public beast::unit_test::suite
credentials1);
}

// Make longest possible CredentialType.
{
constexpr std::string_view longCredentialType =
"Cred0123456789012345678901234567890123456789012345678901234567"
"89";
static_assert(longCredentialType.size() == maxCredentialTypeLength);
pdomain::Credentials const longCredentials{
{alice[1], std::string(longCredentialType)}};

env(pdomain::setTx(alice[0], longCredentials));

// One account can create multiple domains
BEAST_EXPECT(env.ownerCount(alice[0]) == 2);

auto tx = env.tx()->getJson(JsonOptions::none);
BEAST_EXPECT(tx[jss::TransactionType] == "PermissionedDomainSet");
BEAST_EXPECT(tx["Account"] == alice[0].human());

bool findSeq = false;
for (auto const& [domain, object] :
pdomain::getObjects(alice[0], env))
{
findSeq = object["Sequence"] == tx["Sequence"];
if (findSeq)
{
BEAST_EXPECT(domain.isNonZero());
BEAST_EXPECT(
object["LedgerEntryType"] == "PermissionedDomain");
BEAST_EXPECT(object["Owner"] == alice[0].human());
BEAST_EXPECT(
pdomain::credentialsFromJson(object, pubKey2Acc) ==
longCredentials);
break;
}
}
BEAST_EXPECT(findSeq);
}

// Create new from existing account with 10 credentials.
pdomain::Credentials const credentials10{
{alice[2], "credential1"},
Expand Down Expand Up @@ -396,9 +444,11 @@ class PermissionedDomains_test : public beast::unit_test::suite

env.fund(XRP(1000), alice);
auto const setFee(drops(env.current()->fees().increment));

pdomain::Credentials credentials{{alice, "first credential"}};
env(pdomain::setTx(alice, credentials));
env.close();

auto objects = pdomain::getObjects(alice, env);
BEAST_EXPECT(objects.size() == 1);
auto const domain = objects.begin()->first;
Expand All @@ -423,12 +473,15 @@ class PermissionedDomains_test : public beast::unit_test::suite
BEAST_EXPECT(env.ownerCount(alice) == 1);
auto const objID = pdomain::getObjects(alice, env).begin()->first;
BEAST_EXPECT(pdomain::objectExists(objID, env));

// Delete domain that belongs to user.
env(pdomain::deleteTx(alice, domain), ter(tesSUCCESS));
env(pdomain::deleteTx(alice, domain));
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(env.ownerCount(alice) == 0);

// The object needs to be gone.
BEAST_EXPECT(pdomain::getObjects(alice, env).empty());
BEAST_EXPECT(!pdomain::objectExists(objID, env));
Expand Down
6 changes: 3 additions & 3 deletions src/test/jtx/impl/permissioned_domains.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ setTx(
if (domain)
jv[sfDomainID] = to_string(*domain);

Json::Value credentials2(Json::arrayValue);
Json::Value acceptedCredentials(Json::arrayValue);
for (auto const& credential : credentials)
{
Json::Value object(Json::objectValue);
object[sfCredential] = credential.toJson();
credentials2.append(std::move(object));
acceptedCredentials.append(std::move(object));
}

jv[sfAcceptedCredentials] = credentials2;
jv[sfAcceptedCredentials] = acceptedCredentials;
return jv;
}

Expand Down
146 changes: 146 additions & 0 deletions src/test/ledger/Invariants_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,152 @@ class Invariants_test : public beast::unit_test::suite
XRPAmount{},
STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}},
{tecINVARIANT_FAILED, tecINVARIANT_FAILED});

auto const createPD = [](ApplyContext& ac,
std::shared_ptr<SLE>& sle,
Account const& A1,
Account const& A2) {
sle->setAccountID(sfOwner, A1);
sle->setFieldU32(sfSequence, 10);

STArray credentials(sfAcceptedCredentials, 2);
for (std::size_t n = 0; n < 2; ++n)
{
auto cred = STObject::makeInnerObject(sfCredential);
cred.setAccountID(sfIssuer, A2);
auto credType = "cred_type" + std::to_string(n);
cred.setFieldVL(
sfCredentialType, Slice(credType.c_str(), credType.size()));
credentials.push_back(std::move(cred));
}
sle->setFieldArray(sfAcceptedCredentials, credentials);
ac.view().insert(sle);
};

testcase << "PermissionedDomain Set 1";
doInvariantCheck(
{{"permissioned domain with no rules."}},
[createPD](Account const& A1, Account const& A2, ApplyContext& ac) {
Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
auto slePd = std::make_shared<SLE>(pdKeylet);

// create PD
createPD(ac, slePd, A1, A2);

// update PD with empty rules
{
STArray credentials(sfAcceptedCredentials, 2);
slePd->setFieldArray(sfAcceptedCredentials, credentials);
ac.view().update(slePd);
}

return true;
},
XRPAmount{},
STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}},
{tecINVARIANT_FAILED, tecINVARIANT_FAILED});

testcase << "PermissionedDomain Set 2";
doInvariantCheck(
{{"permissioned domain bad credentials size " +
std::to_string(tooBig)}},
[createPD](Account const& A1, Account const& A2, ApplyContext& ac) {
Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
auto slePd = std::make_shared<SLE>(pdKeylet);

// create PD
createPD(ac, slePd, A1, A2);

// update PD
{
STArray credentials(sfAcceptedCredentials, tooBig);

for (std::size_t n = 0; n < tooBig; ++n)
{
auto cred = STObject::makeInnerObject(sfCredential);
cred.setAccountID(sfIssuer, A2);
auto credType = "cred_type2" + std::to_string(n);
cred.setFieldVL(
sfCredentialType,
Slice(credType.c_str(), credType.size()));
credentials.push_back(std::move(cred));
}

slePd->setFieldArray(sfAcceptedCredentials, credentials);
ac.view().update(slePd);
}

return true;
},
XRPAmount{},
STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}},
{tecINVARIANT_FAILED, tecINVARIANT_FAILED});

testcase << "PermissionedDomain Set 3";
doInvariantCheck(
{{"permissioned domain credentials aren't sorted"}},
[createPD](Account const& A1, Account const& A2, ApplyContext& ac) {
Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
auto slePd = std::make_shared<SLE>(pdKeylet);

// create PD
createPD(ac, slePd, A1, A2);

// update PD
{
STArray credentials(sfAcceptedCredentials, 2);
for (std::size_t n = 0; n < 2; ++n)
{
auto cred = STObject::makeInnerObject(sfCredential);
cred.setAccountID(sfIssuer, A2);
auto credType =
std::string("cred_type2") + std::to_string(9 - n);
cred.setFieldVL(
sfCredentialType,
Slice(credType.c_str(), credType.size()));
credentials.push_back(std::move(cred));
}

slePd->setFieldArray(sfAcceptedCredentials, credentials);
ac.view().update(slePd);
}

return true;
},
XRPAmount{},
STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}},
{tecINVARIANT_FAILED, tecINVARIANT_FAILED});

testcase << "PermissionedDomain Set 4";
doInvariantCheck(
{{"permissioned domain credentials aren't unique"}},
[createPD](Account const& A1, Account const& A2, ApplyContext& ac) {
Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
auto slePd = std::make_shared<SLE>(pdKeylet);

// create PD
createPD(ac, slePd, A1, A2);

// update PD
{
STArray credentials(sfAcceptedCredentials, 2);
for (std::size_t n = 0; n < 2; ++n)
{
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().update(slePd);
}

return true;
},
XRPAmount{},
STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}},
{tecINVARIANT_FAILED, tecINVARIANT_FAILED});
}

public:
Expand Down
6 changes: 3 additions & 3 deletions src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ PermissionedDomainDelete::doApply()

if (!view().dirRemove(keylet::ownerDir(account_), page, slePd->key(), true))
{
JLOG(j_.fatal())
<< "Unable to delete permissioned domain directory entry.";
return tefBAD_LEDGER;
JLOG(j_.fatal()) // LCOV_EXCL_LINE
<< "Unable to delete permissioned domain directory entry."; // LCOV_EXCL_LINE
return tefBAD_LEDGER; // LCOV_EXCL_LINE
}

auto const ownerSle = view().peek(keylet::account(account_));
Expand Down
11 changes: 6 additions & 5 deletions src/xrpld/app/tx/detail/PermissionedDomainSet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ PermissionedDomainSet::preclaim(PreclaimContext const& ctx)
auto const account = ctx.tx.getAccountID(sfAccount);

if (!ctx.view.exists(keylet::account(account)))
return tefINTERNAL;
return tefINTERNAL; // LCOV_EXCL_LINE

auto const& credentials = ctx.tx.getFieldArray(sfAcceptedCredentials);
for (auto const& credential : credentials)
Expand Down Expand Up @@ -84,7 +84,7 @@ PermissionedDomainSet::doApply()
{
auto const ownerSle = view().peek(keylet::account(account_));
if (!ownerSle)
return tefINTERNAL;
return tefINTERNAL; // LCOV_EXCL_LINE

auto const sortedTxCredentials =
credentials::makeSorted(ctx_.tx.getFieldArray(sfAcceptedCredentials));
Expand All @@ -103,7 +103,7 @@ PermissionedDomainSet::doApply()
auto slePd = view().peek(
keylet::permissionedDomain(ctx_.tx.getFieldH256(sfDomainID)));
if (!slePd)
return tefINTERNAL;
return tefINTERNAL; // LCOV_EXCL_LINE
slePd->peekFieldArray(sfAcceptedCredentials) = std::move(sortedLE);
view().update(slePd);
}
Expand All @@ -121,15 +121,16 @@ PermissionedDomainSet::doApply()
account_, ctx_.tx.getFieldU32(sfSequence));
auto slePd = std::make_shared<SLE>(pdKeylet);
if (!slePd)
return tefINTERNAL;
return tefINTERNAL; // LCOV_EXCL_LINE

slePd->setAccountID(sfOwner, account_);
slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence));
slePd->peekFieldArray(sfAcceptedCredentials) = std::move(sortedLE);
auto const page = view().dirInsert(
keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_));
if (!page)
return tecDIR_FULL;
return tecDIR_FULL; // LCOV_EXCL_LINE

slePd->setFieldU64(sfOwnerNode, *page);
// If we succeeded, the new entry counts against the creator's reserve.
adjustOwnerCount(view(), ownerSle, 1, ctx_.journal);
Expand Down

0 comments on commit bc8f025

Please sign in to comment.