Do not allow creating PDs if credentials are not enabled

[Bronek]

High Level Overview of Change

This solves the problem of permissioned domains dependency on credentials.

Context of Change

If the permissioned domains amendment XLS-80 were enabled before credentials XLS-70, then the permissioned domain users will not be able to match any credentials, i.e. the amendment will be useless. The simplest solution, proposed in this PR, is to prevent creation of any PD objects if credentials are not enabled.

Similarly, amendments which depend on permissioned domains (e.g. single asset vault) will have to check if permissioned domain amendment XLS-80 is enabled, and act accordingly. This is similar in spirit to how we handle addition of new optional fields to existing transactions, returning temDISABLED if the newly added features wanted by the transaction are not enabled.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.2%. Comparing base (d9e4009) to head (cce6ead).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5275     +/-   ##
=========================================
- Coverage     78.2%   78.2%   -0.0%     
=========================================
  Files          790     790             
  Lines        67619   67623      +4     
  Branches      8161    8163      +2     
=========================================
  Hits         52848   52848             
- Misses       14771   14775      +4     

Files with missing lines	Coverage Δ
src/xrpld/app/tx/detail/PermissionedDomainSet.cpp	100.0% <100.0%> (ø)

... and 4 files with indirect coverage changes

[mvadari]

Shouldn't this also be temDISABLED?

[Bronek]

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.

[mvadari]

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?

[Bronek]

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

[mvadari]

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

[mvadari]

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

[Bronek]

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.

[oleks-rip]

lgtm