Skip to content

Commit

Permalink
Silently remove duplicate credentials instead of
Browse files Browse the repository at this point in the history 
failing the transaction. Conforms to Credentials spec.
  • Loading branch information
@mtrippled
mtrippled committed Oct 8, 2024
1 parent 70dfad9 commit 49d1890
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 88 deletions.
45 changes: 31 additions & 14 deletions src/test/app/PermissionedDomains_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,16 +165,33 @@ 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.
Expand All @@ -187,7 +204,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;
Expand All @@ -198,10 +215,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));
}
}

Expand Down Expand Up @@ -266,7 +283,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());
Expand Down
3 changes: 1 addition & 2 deletions src/test/jtx/PermissionedDomains.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#define RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED

#include <test/jtx.h>
#include <optional>

namespace ripple {
namespace test {
Expand Down Expand Up @@ -64,7 +63,7 @@ Credentials
credentialsFromJson(Json::Value const& object);

// Sort credentials the same way as PermissionedDomainSet
std::optional<Credentials>
Credentials
sortCredentials(Credentials const& input);

// Get account_info
Expand Down
73 changes: 27 additions & 46 deletions src/test/jtx/impl/PermissionedDomains.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,56 +121,37 @@ credentialsFromJson(Json::Value const& object)
return ret;
}

// Sort credentials the same way as PermissionedDomainSet. None can
// be identical.
std::optional<Credentials>
// 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<Credential> 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
Expand Down
40 changes: 14 additions & 26 deletions src/xrpld/app/tx/detail/PermissionedDomainSet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include <xrpld/ledger/View.h>
#include <xrpl/protocol/STObject.h>
#include <xrpl/protocol/TxFlags.h>

#include <map>
#include <optional>

namespace ripple {
Expand Down Expand Up @@ -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<STLedgerEntry> 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<uint256, STObject> 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))
Expand All @@ -112,11 +119,6 @@ PermissionedDomainSet::doApply()
{
return true;
}
if (left.getFieldVL(sfCredentialType) ==
right.getFieldVL(sfCredentialType))
{
throw std::runtime_error("duplicate credentials");
}
}
return false;
});
Expand All @@ -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
Expand All @@ -146,14 +141,7 @@ PermissionedDomainSet::doApply()
auto slePd = std::make_shared<SLE>(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_));
Expand Down

0 comments on commit 49d1890

Please sign in to comment.