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
Merged

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

Show file tree
Hide file tree
Changes from all commits
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
21 changes: 19 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,21 @@ 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()
Copy link
Collaborator

@mvadari mvadari Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit: would prefer if this was a part of testDisabled instead of separate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the reason why this is separate is because the normal test pattern is to use one FeatureBitset per test (or per test run). I like sticking to patterns, makes things more predictable.

{
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));
}

// Verify that each tx does not execute if feature is disabled
void
testDisabled()
Expand Down Expand Up @@ -556,6 +572,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) ||
oleks-rip marked this conversation as resolved.
Show resolved Hide resolved
!ctx.rules.enabled(featureCredentials))
return temDISABLED;

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

Expand Down
Loading