refactor: Use isFlag where possible instead of bitwise math - #7278
Merged
Conversation
mvadari
added
the
Trivial
Contributor
There was a problem hiding this comment.
Pull request overview
A pure refactor that replaces verbose bitwise expressions on sfFlags (e.g., (sle->getFlags() & lsfX) != 0u) with the more readable sle->isFlag(lsfX) helper across transactors, ledger helpers, RPC handlers, and tests. No behavior changes.
Changes:
- Substitutes
(getFlags()/getFieldU32(sfFlags) & FLAG)patterns withisFlag(FLAG)throughout the codebase. - In
RippleStateHelpers.cpp::updateTrustLine, factors out per-side flag selectors into named locals and replaces asetFieldU32(sfFlags, flags & ~mask)withclearFlag(mask). - Updates relevant tests to use
isFlagfor clarity.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/rpc/handlers/orderbook/DepositAuthorized.cpp | Use isFlag for deposit/credential flag checks |
| src/xrpld/rpc/handlers/account/NoRippleCheck.cpp | Use isFlag for default-ripple/no-ripple checks |
| src/xrpld/rpc/handlers/account/AccountLines.cpp | Use isFlag for low/high reserve checks |
| src/xrpld/rpc/detail/PathRequest.cpp | Use isFlag for lsfDisallowXRP |
| src/xrpld/rpc/detail/Pathfinder.cpp | Use isFlag for noRipple/requireAuth checks |
| src/test/jtx/impl/flags.cpp | Replace (flags & mask) == mask with isFlag(mask) |
| src/test/app/SetRegularKey_test.cpp | Use isFlag(lsfPasswordSpent) in assertions |
| src/test/app/DepositAuth_test.cpp | Use isFlag(lsfDepositAuth) |
| src/test/app/Credentials_test.cpp | Use isFlag(lsfAccepted) in assertions |
| src/libxrpl/tx/transactors/vault/VaultSet.cpp | Use isFlag(lsfMPTRequireAuth) |
| src/libxrpl/tx/transactors/vault/VaultCreate.cpp | Use isFlag(tfVaultPrivate) |
| src/libxrpl/tx/transactors/token/TrustSet.cpp | Use isFlag for require-auth/disallow-incoming/default-ripple |
| src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp | Use isFlag(tfMPTRequireAuth) |
| src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp | Use isFlag(tfMPTUnauthorize) |
| src/libxrpl/tx/transactors/token/Clawback.cpp | Use isFlag(lsfMPTCanClawback) |
| src/libxrpl/tx/transactors/system/Batch.cpp | Use isFlag(tfInnerBatchTxn) |
| src/libxrpl/tx/transactors/payment/Payment.cpp | Use isFlag for require-dest-tag/password-spent |
| src/libxrpl/tx/transactors/payment_channel/PaymentChannelClaim.cpp | Use isFlag(tfRenew/tfClose) |
| src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp | Use isFlag(lsfSellNFToken) |
| src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp | Use isFlag(lsfRequireDestTag) |
| src/libxrpl/tx/transactors/dex/OfferCreate.cpp | Use isFlag for auth checks |
| src/libxrpl/tx/transactors/dex/AMMDeposit.cpp | Use isFlag for AMM deposit flags |
| src/libxrpl/tx/transactors/dex/AMMCreate.cpp | Use isFlag(lsfDefaultRipple) |
| src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp | Use isFlag(lsfAccepted) |
| src/libxrpl/tx/transactors/check/CheckCash.cpp | Use isFlag plus comment alignment update |
| src/libxrpl/tx/transactors/bridge/XChainBridge.cpp | Use isFlag across multiple bridge flag checks |
| src/libxrpl/tx/transactors/account/SignerListSet.cpp | Use isFlag(lsfOneOwnerCount) |
| src/libxrpl/tx/transactors/account/SetRegularKey.cpp | Use isFlag(lsfPasswordSpent) |
| src/libxrpl/tx/transactors/account/AccountDelete.cpp | Use isFlag for require-dest-tag/deposit-auth |
| src/libxrpl/tx/paths/DirectStep.cpp | Use isFlag for require-auth/no-ripple |
| src/libxrpl/tx/paths/BookStep.cpp | Use isFlag for no-ripple checks |
| src/libxrpl/tx/invariants/AMMInvariant.cpp | Use isFlag(lsfAMMNode) |
| src/libxrpl/ledger/helpers/TokenHelpers.cpp | Use isFlag(lsfDefaultRipple) for the SLE-side check |
| src/libxrpl/ledger/helpers/RippleStateHelpers.cpp | Refactor to named flag selectors, isFlag/clearFlag |
| src/libxrpl/ledger/helpers/NFTokenHelpers.cpp | Use isFlag for sell-NFT/disallow-incoming-offer |
| src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | Use isFlag(lsfMPTRequireAuth) in assert |
| src/libxrpl/ledger/helpers/CredentialHelpers.cpp | Use isFlag for accepted/deposit-auth checks |
| include/xrpl/tx/paths/detail/StepChecks.h | Use isFlag for no-ripple checks in checkNoRipple |
| include/xrpl/ledger/helpers/EscrowHelpers.h | Use isFlag(lsfDefaultRipple) and reformat comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7278 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.0%
=========================================
Files 1010 1010
Lines 76268 76224 -44
Branches 7445 7413 -32
=========================================
- Hits 62608 62555 -53
- Misses 13660 13669 +9
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
bthomee
approved these changes
May 14, 2026
bthomee
reviewed
May 14, 2026
Comment on lines
580
to
581
| // FIXME This NEEDS to be cleaned up and simplified. It's impossible | ||
| // for anyone to understand. |
Collaborator
There was a problem hiding this comment.
Maybe this comment can now be removed after the improvements made in this PR?
Contributor
Author
There was a problem hiding this comment.
I would say there's still room for improvement
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (3)
src/libxrpl/tx/transactors/token/MPTokenIssuanceSet.cpp:160
isFlag(tfMPTokenIssuanceSetMask)is true only if all mask bits are set, but the original(txFlags & tfMPTokenIssuanceSetMask) != 0uwas true if any masked bit was set. This refactor changes the meaning of the permission check. Use(tx.getFlags() & tfMPTokenIssuanceSetMask) != 0u.
src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp:300isFlag(tfLPToken | tfWithdrawAll)requires both bits to be set, but the original(ctx.tx.getFlags() & (tfLPToken | tfWithdrawAll)) != 0uwas true if either was set. The two flags are mutually exclusive, so this branch will no longer execute, skipping thecheckAmountvalidation. Usectx.tx.isFlag(tfLPToken) || ctx.tx.isFlag(tfWithdrawAll).
if (ctx.tx.isFlag(tfLPToken | tfWithdrawAll))
src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp:1144
isFlag(tfWithdrawAll | tfOneAssetWithdrawAll)requires both bits to be set, while the original(tx[sfFlags] & (tfWithdrawAll | tfOneAssetWithdrawAll)) != 0uwas true when either was set. Because these flags are mutually exclusive,isWithdrawAllwill now always returnWithdrawAll::No, which is a silent regression. Usetx.isFlag(tfWithdrawAll) || tx.isFlag(tfOneAssetWithdrawAll).
if (tx.isFlag(tfWithdrawAll | tfOneAssetWithdrawAll))
bthomee
approved these changes
May 15, 2026
|
- Mayukha proposed a pure refactor to replace complex bitwise math with
the more readable isFlag helper across 39 files in rippled.
- The pull request was reviewed and updated by Mayukha to address
suggestions and concerns about unintended semantic changes when using
isFlag with multiple flags.
…On Fri, May 15, 2026, 8:36 AM Bart ***@***.***> wrote:
***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub
<#7278 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B4O437A73Y57BKOESPAXPOT424MNFAVCNFSM6AAAAACY6KJ5BCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DEOJYGUYDKOJVHE>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
mvadari
added
the
Ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Title pretty much says it all. I did some additional refactors to remove
flagsvariables where relevant. A few ternaries of the form!bool ? X : Ywere also flipped (tobool ? Y : X).No functionality changes, this is a pure refactor.
Context of Change
Bitwise math is harder to read/reason about
API Impact
N/A