Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow creating PDs if credentials are not enabled #5275

Merged
merged 3 commits into from
Feb 7, 2025
+22 −3
Merged

Do not allow creating PDs if credentials are not enabled #5275

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a38c94
Do not allow creating PDs if credentials are not enabled
Bronek Feb 4, 2025
4185428
Remove "delete PD" from test
Bronek Feb 7, 2025
cce6ead
Merge branch 'develop' into bkozicki/permissioned_domains_depend_on_c…
Bronek Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions src/test/app/PermissionedDomains_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@ exceptionExpected(Env& env, Json::Value const& jv)

class PermissionedDomains_test : public beast::unit_test::suite
{
FeatureBitset withFeature_{
supported_amendments() | featurePermissionedDomains};
FeatureBitset withoutFeature_{supported_amendments()};
FeatureBitset withFeature_{
supported_amendments() //
| featurePermissionedDomains | featureCredentials};

// Verify that each tx type can execute if the feature is enabled.
void
Expand All @@ -77,6 +78,22 @@ class PermissionedDomains_test : public beast::unit_test::suite
env(pdomain::deleteTx(alice, domain));
}

// Verify that PD cannot be created or updated if credentials are disabled
void
testCredentialsDisabled()
{
auto amendments = supported_amendments();
amendments.set(featurePermissionedDomains);
amendments.reset(featureCredentials);
testcase("Credentials disabled");
Account const alice("alice");
Env env(*this, amendments);
env.fund(XRP(1000), alice);
pdomain::Credentials credentials{{alice, "first credential"}};
env(pdomain::setTx(alice, credentials), ter(temDISABLED));
env(pdomain::deleteTx(alice, uint256(75)), ter(tecNO_ENTRY));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be temDISABLED?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The change is meant to prevent creation of PDs (if credentials cannot be used). If a PD cannot be created, then an obvious error on trying to do anything with it is tecNO_ENTRY. I do not insist this is best, but the idea is to keep coupling between different amendments to minimum - if we chose a different pattern (check every single transaction type) then I feel we might fall into ambiguities.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So your argument is that you can't create a PD because it requires credentials, but technically if PDs had other rules those would still be allowed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - if hypothetically PD could make sense without referring to credentials (because e.g. it would refer to something else) then I think we should be able to create it; which makes me think I ought to change the check to

PermissionedDomainSet::preflight(PreflightContext const& ctx)
{
    if (!ctx.rules.enabled(featurePermissionedDomains))
        return temDISABLED;

    if (ctx.tx.isFieldPresent(sfAcceptedCredentials) &&
        !ctx.rules.enabled(featureCredentials))
        return temDISABLED;

what do you think ? This looks a bit strange, given that sfAcceptedCredentials is a required field

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's sort of unnecessary and if we happen to make sfAcceptedCredentials optional we can make that change then.

}

// Verify that each tx does not execute if feature is disabled
void
testDisabled()
Expand Down Expand Up @@ -556,6 +573,7 @@ class PermissionedDomains_test : public beast::unit_test::suite
run() override
{
testEnabled();
testCredentialsDisabled();
testDisabled();
testSet();
testDelete();
Expand Down
4 changes: 3 additions & 1 deletion src/xrpld/app/tx/detail/PermissionedDomainSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ namespace ripple {
NotTEC
PermissionedDomainSet::preflight(PreflightContext const& ctx)
{
if (!ctx.rules.enabled(featurePermissionedDomains))
if (!ctx.rules.enabled(featurePermissionedDomains) ||
!ctx.rules.enabled(featureCredentials))
return temDISABLED;

if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
return ret;

Expand Down
Loading