feat: Enhance parseTransactionFlags with string overload and includeAll option (#1903)#3224
Open
slurpyone wants to merge 2 commits intoXRPLF:mainfrom
Open
feat: Enhance parseTransactionFlags with string overload and includeAll option (#1903)#3224slurpyone wants to merge 2 commits intoXRPLF:mainfrom
slurpyone wants to merge 2 commits intoXRPLF:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughparseTransactionFlags now accepts either a Transaction object or a (txType, flagsNum) pair, adds an optional includeAll option to include disabled flags, registers EnableAmendmentFlags and LoanSetFlags in the txToFlag mapping, and always returns a Record<string, boolean> map including global flags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xrpl/test/models/utils.test.ts (1)
397-471: Add one regression case per newtxToFlagentry.These cases cover the new overload mechanics, but none of them prove the new
EnableAmendmentandLoanSetmappings work. Dropping either key frompackages/xrpl/src/models/utils/flags.tswould still leave this suite green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/utils.test.ts` around lines 397 - 471, Add explicit regression tests that exercise the new txToFlag mappings for the EnableAmendment and LoanSet entries: call parseTransactionFlags using the transaction type string (e.g., 'EnableAmendment' and 'LoanSet') with relevant numeric flags (and with includeAll true where appropriate) and assert the resulting flagsMap contains the expected mapped keys; ensure each new txToFlag entry has at least one test that would fail if its mapping in txToFlag were removed or incorrect so the suite actually verifies the new overload mechanics (reference parseTransactionFlags and txToFlag, and the specific keys EnableAmendment and LoanSet).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/utils/flags.ts`:
- Around line 170-179: The public signature parseTransactionFlags currently uses
a union that allows invalid call shapes (e.g., parseTransactionFlags(tx, 123))
and forces callers to pass undefined for options; replace it with two TypeScript
overloads matching the docblock: one overload parseTransactionFlags(tx:
Transaction, options?: { includeAll?: boolean }) and a second overload
parseTransactionFlags(txType: string, flagsNum: number, options?: { includeAll?:
boolean }), then keep a single implementation signature (e.g.,
parseTransactionFlags(txOrType: Transaction | string, flagsNumOrOptions?: number
| { includeAll?: boolean }, maybeOptions?: { includeAll?: boolean })) and adapt
the body to detect whether the second argument is a number or options and to
require flagsNum when txOrType is a string; ensure the JSDoc and exported types
(if you introduce ParseTransactionFlagsOptions) are updated accordingly.
---
Nitpick comments:
In `@packages/xrpl/test/models/utils.test.ts`:
- Around line 397-471: Add explicit regression tests that exercise the new
txToFlag mappings for the EnableAmendment and LoanSet entries: call
parseTransactionFlags using the transaction type string (e.g., 'EnableAmendment'
and 'LoanSet') with relevant numeric flags (and with includeAll true where
appropriate) and assert the resulting flagsMap contains the expected mapped
keys; ensure each new txToFlag entry has at least one test that would fail if
its mapping in txToFlag were removed or incorrect so the suite actually verifies
the new overload mechanics (reference parseTransactionFlags and txToFlag, and
the specific keys EnableAmendment and LoanSet).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f4049c1-9130-4018-a18a-976695add183
📒 Files selected for processing (2)
packages/xrpl/src/models/utils/flags.tspackages/xrpl/test/models/utils.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/utils/flags.ts`:
- Around line 173-175: The example for parseTransactionFlags showing includeAll:
true is inaccurate because includeAll merges transaction-specific flags with
global flags; update the example return value to include those global flags (the
same fix for the duplicate example at lines 228–235). Locate the
parseTransactionFlags usage in flags.ts and modify the example comment to show
the full returned object (transaction flags plus global flags names returned by
the code path that combines GLOBAL flags), or alternatively adjust the example
wording to state that global flags are included when includeAll is true.
- Around line 202-205: When txOrType is a string (the code path that sets
transactionType = txOrType), validate that flagsNumOrOptions is actually a
number before using it as flags: check typeof flagsNumOrOptions === 'number' and
assign flags = flagsNumOrOptions, otherwise throw a clear TypeError (or similar)
explaining the second argument must be a numeric flags bitmask; still assign
options = maybeOptions as before. Update the logic around transactionType,
flags, flagsNumOrOptions, and maybeOptions so callers get a deterministic
runtime error instead of silently getting 0/invalid flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51909c26-0041-4b05-9536-8bd06f7e4e76
📒 Files selected for processing (2)
packages/xrpl/src/models/utils/flags.tspackages/xrpl/test/models/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/models/utils.test.ts
Comment on lines
+202
to
+205
| if (typeof txOrType === 'string') { | ||
| transactionType = txOrType | ||
| flags = (flagsNumOrOptions as number) ?? 0 | ||
| options = maybeOptions |
Contributor
There was a problem hiding this comment.
Validate flagsNum at runtime for string-based calls.
When txOrType is a string, Line 204 currently accepts a missing or mis-typed second argument and falls through as 0/an invalid bit source. That makes JS callers and any-typed TS callers get an empty flag map instead of a contract error, which is hard to debug for the new overload.
Suggested fix
if (typeof txOrType === 'string') {
transactionType = txOrType
- flags = (flagsNumOrOptions as number) ?? 0
+ if (typeof flagsNumOrOptions !== 'number') {
+ throw new ValidationError(
+ 'flagsNum is required and must be a number when txOrType is a string.',
+ )
+ }
+ flags = flagsNumOrOptions
options = maybeOptions
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (typeof txOrType === 'string') { | |
| transactionType = txOrType | |
| flags = (flagsNumOrOptions as number) ?? 0 | |
| options = maybeOptions | |
| if (typeof txOrType === 'string') { | |
| transactionType = txOrType | |
| if (typeof flagsNumOrOptions !== 'number') { | |
| throw new ValidationError( | |
| 'flagsNum is required and must be a number when txOrType is a string.', | |
| ) | |
| } | |
| flags = flagsNumOrOptions | |
| options = maybeOptions | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/utils/flags.ts` around lines 202 - 205, When
txOrType is a string (the code path that sets transactionType = txOrType),
validate that flagsNumOrOptions is actually a number before using it as flags:
check typeof flagsNumOrOptions === 'number' and assign flags =
flagsNumOrOptions, otherwise throw a clear TypeError (or similar) explaining the
second argument must be a numeric flags bitmask; still assign options =
maybeOptions as before. Update the logic around transactionType, flags,
flagsNumOrOptions, and maybeOptions so callers get a deterministic runtime error
instead of silently getting 0/invalid flags.
slurpyone
force-pushed
the
feat/parse-transaction-flags
branch
from
278fbfd to
81adabe
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/xrpl/src/models/utils/flags.ts (2)
173-175:⚠️ Potential issue | 🟡 Minor
includeAllexample is still missing global flags in the return shape.When
includeAll: true, implementation also includes global flags (for exampletfInnerBatchTxn: false), so this example is currently misleading.✏️ Suggested doc fix
- * // => { tfNoRippleDirect: false, tfPartialPayment: true, tfLimitQuality: false } + * // => { tfNoRippleDirect: false, tfPartialPayment: true, tfLimitQuality: false, tfInnerBatchTxn: false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/utils/flags.ts` around lines 173 - 175, The example in parseTransactionFlags's docstring is misleading because when includeAll: true the function also returns global flags (e.g., tfInnerBatchTxn), so update the example and/or text in the comment near the parseTransactionFlags function to show that global flags are included in the returned shape (for example include tfInnerBatchTxn: false alongside tfNoRippleDirect/tfPartialPayment/tfLimitQuality) or explicitly state that global flags are present when includeAll is true.
202-205:⚠️ Potential issue | 🟠 MajorEnforce numeric
flagsNumfor string-based calls at runtime.This path currently defaults missing
flagsNumto0and accepts mis-typed values, which violates the documented contract and can hide caller mistakes.🛠️ Suggested fix
if (typeof txOrType === 'string') { transactionType = txOrType - flags = (flagsNumOrOptions as number) ?? 0 + if (typeof flagsNumOrOptions !== 'number' || Number.isNaN(flagsNumOrOptions)) { + throw new ValidationError( + 'flagsNum is required and must be a number when txOrType is a string.', + ) + } + flags = flagsNumOrOptions options = maybeOptions } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/utils/flags.ts` around lines 202 - 205, When txOrType is a string (the "string-based" overload), validate that flagsNumOrOptions is a numeric flags value before assigning: instead of defaulting to 0, test typeof flagsNumOrOptions === 'number' and if not throw a TypeError (or similar) indicating a required numeric flagsNum; then set flags = flagsNumOrOptions and options = maybeOptions. Update the branch that currently sets transactionType = txOrType, flags = (flagsNumOrOptions as number) ?? 0, options = maybeOptions to perform this runtime type-check and error on missing/mistyped flagsNum to enforce the documented contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/xrpl/src/models/utils/flags.ts`:
- Around line 173-175: The example in parseTransactionFlags's docstring is
misleading because when includeAll: true the function also returns global flags
(e.g., tfInnerBatchTxn), so update the example and/or text in the comment near
the parseTransactionFlags function to show that global flags are included in the
returned shape (for example include tfInnerBatchTxn: false alongside
tfNoRippleDirect/tfPartialPayment/tfLimitQuality) or explicitly state that
global flags are present when includeAll is true.
- Around line 202-205: When txOrType is a string (the "string-based" overload),
validate that flagsNumOrOptions is a numeric flags value before assigning:
instead of defaulting to 0, test typeof flagsNumOrOptions === 'number' and if
not throw a TypeError (or similar) indicating a required numeric flagsNum; then
set flags = flagsNumOrOptions and options = maybeOptions. Update the branch that
currently sets transactionType = txOrType, flags = (flagsNumOrOptions as number)
?? 0, options = maybeOptions to perform this runtime type-check and error on
missing/mistyped flagsNum to enforce the documented contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7fba20b8-edf2-401f-bae5-af8b08de76c7
📒 Files selected for processing (2)
packages/xrpl/src/models/utils/flags.tspackages/xrpl/test/models/utils.test.ts
slurpyone
force-pushed
the
feat/parse-transaction-flags
branch
from
81adabe to
bb8603d
Compare
slurpyone
force-pushed
the
feat/parse-transaction-flags
branch
5 times, most recently
from
b3da2f7 to
f080931
Compare
slurpyone
force-pushed
the
feat/parse-transaction-flags
branch
4 times, most recently
from
86f3f3e to
d01618d
Compare
slurpyone
force-pushed
the
feat/parse-transaction-flags
branch
3 times, most recently
from
9c9f87a to
6d59daa
Compare
…ll option (XRPLF#1903) - Add overload accepting (txType: string, flags: number) for parsing numeric flags directly from raw API responses without constructing a full Transaction object - Add optional { includeAll: true } parameter to include all possible flags for a transaction type with their boolean values (not just enabled flags) - Improve return type from object to Record<string, boolean> - Add missing EnableAmendment and LoanSet to txToFlag mapping - Add comprehensive tests for all new functionality
Address CodeRabbit review feedback: replace union signature with proper overloads to prevent invalid call shapes. - Add ParseTransactionFlagsOptions interface - Overload 1: parseTransactionFlags(tx, options?) for Transaction objects - Overload 2: parseTransactionFlags(txType, flagsNum, options?) for strings - Update tests to use new overload-friendly calling convention
slurpyone
force-pushed
the
feat/parse-transaction-flags
branch
from
6d59daa to
cefea2a
Compare
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
1 participant
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.
Summary
Enhances the existing
parseTransactionFlagsutility to address the usability gaps described in #1903.Changes
(txType: string, flags: number)directly, so users working with raw API responses can parse flags without constructing a fullTransactionobject:includeAlloption: Optionally include all possible flags for a transaction type (withfalsefor disabled ones):Improved typing: Return type changed from
objecttoRecord<string, boolean>.Missing flag mappings: Added
EnableAmendmentandLoanSetto thetxToFlagmap.Backward Compatibility
Fully backward compatible — the existing
parseTransactionFlags(tx: Transaction)signature continues to work identically.Tests
Added 7 new test cases covering:
includeAlloption with both string and Transaction overloadsAll existing tests continue to pass.
Closes #1903