fix: Backport Permissioned Domains fixes - #7016
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7016 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 1010 1010
Lines 76205 76205
Branches 7407 7401 -6
=======================================
Hits 62571 62571
Misses 13634 13634 🚀 New features to boost your workflow:
|
|
Worth calling out in the PR description that withFix_ and withFeature_ were identical before this change — that's the actual substantive fix here. |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
We recently merged a refactor to One-time setupIf you don't already have clang-tidy working in your env, on macOS: brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.Workflow on your branch (before merging develop)1. Grab the new git remote -v # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy2. Reconfigure conan/cmake so 3. Apply renames for the files modified in your PR: git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
| grep -E '\.(cpp|h|hpp|ipp)$' \
| xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called4. Build + test, then commit as a single dedicated commit: cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"5. Now merge develop: git merge upstream/developExtraRun clang-tidy once more after the merge to catch any stragglers introduced from develop's side: run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
| FeatureBitset withFeature_{ | ||
| FeatureBitset withoutFeature_{ | ||
| testableAmendments() // | ||
| | featurePermissionedDomains | featureCredentials}; | ||
|
|
||
| - featurePermissionedDomains - fixPermissionedDomainInvariant}; | ||
| FeatureBitset withFeature_{ | ||
| (testableAmendments() // | ||
| | featurePermissionedDomains | featureCredentials) - | ||
| fixPermissionedDomainInvariant}; | ||
| FeatureBitset withFix_{ | ||
| testableAmendments() // | ||
| | featurePermissionedDomains | featureCredentials}; | ||
| | featurePermissionedDomains | featureCredentials | fixPermissionedDomainInvariant}; |
There was a problem hiding this comment.
The featurePermissionedDomains, featureCredentials, and fixPermissionedDomainInvariant amendments are marked as being supported in features.macro, so are included in testableAmendments() by default. In essence, the list above translates to:
- without feature: all amendments minus permissioned domains and invariant.
- with feature: all amendments minus invariant.
- with fix: all amendments.
Is a separate FeatureBitset needed for "all amendments minus permissioned domains" (so, including the invariant)?
There was a problem hiding this comment.
This code is moved from another branch, and, probably, written before amendments were accepted. So it is here in this form just to be the same, and to provide exact clarity on which amendments enabled / disabed
There was a problem hiding this comment.
After looking more closely, the way the current tests are set up, and where these withX and withoutX are referenced, I notice my source of feeling uneasy with the current code (not your changes specifically).
I suggest to simply remove the withoutFeature_ completely. It's only used in one location, on line 101. You can just replace the withoutFeature_ there by testableAmendments() - featurePermissionedDomains.
Then you can keep everything else as-is for now. I think adding more combinations would make sense, to make sure nothing breaks if the fix is enabled but the amendment isn't, for instance. It's unlikely Credentials will become unsupported, so you probably don't have to test for that.
bthomee
left a comment
There was a problem hiding this comment.
Very nice, thanks for making the changes.
High Level Overview of Change
Some fixes for Permissioned Domains that where applied to staging-3.1.3 but not present in develop