Fix: parseTransactionFlags unintentionally modifies transaction #2825
Conversation
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/src/models/utils/flags.ts (2)
75-79: Enhance error message for invalid flags.The error message could be more helpful by including the transaction type and the actual flag value.
- `flag ${flag} doesn't exist in flagEnum: ${JSON.stringify(flagEnum)}`, + `Invalid flag '${flag}' for transaction type '${tx.TransactionType}'. Valid flags are: ${Object.keys(flagEnum).join(", ")}`,
95-114: Consider improving type safety and documentation.The function implementation looks good but could benefit from improved type safety and documentation updates:
- Update the JSDoc to reflect that the function now uses
convertTxFlagsToNumber:/** * Convert a Transaction flags property into a map for easy interpretation. * + * This function first converts the transaction's flags to their numeric representation + * using convertTxFlagsToNumber, then maps them to boolean values. * * @param tx - A transaction to parse flags for. - * @returns A map with all flags as booleans. + * @returns An object mapping flag names to boolean values, or an empty object if no flags are set. */
- Add type safety for the flag map:
- const booleanFlagMap = {} + const booleanFlagMap: Record<string, boolean> = {} - const transactionTypeFlags = txToFlag[tx.TransactionType] + const transactionTypeFlags = txToFlag[tx.TransactionType as keyof typeof txToFlag] + if (!transactionTypeFlags) { + return {} + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/xrpl/src/client/index.ts(2 hunks)packages/xrpl/src/models/utils/flags.ts(2 hunks)
🔇 Additional comments (3)
packages/xrpl/src/models/utils/flags.ts (1)
64-85: LGTM! Successfully fixes the mutation issue.
The refactoring from setTransactionFlagsToNumber to convertTxFlagsToNumber correctly addresses the unintended mutation of the transaction object by returning a new value instead of modifying the input. This aligns with functional programming principles and fixes the issue described in PR #2823.
packages/xrpl/src/client/index.ts (2)
50-50: LGTM: Import change aligns with the fix.
The import change from setTransactionFlagsToNumber to convertTxFlagsToNumber reflects the intended behavior of converting flags without modifying the transaction object.
668-668: LGTM: Flag conversion change fixes the modification issue.
The change from setTransactionFlagsToNumber to convertTxFlagsToNumber ensures that transaction flags are converted without modifying the original transaction object, fixing the issue described in PR #2823.
Let's verify the transaction submission flow still works:
✅ Verification successful
Based on the extensive test results, I can now generate a final response. The tests show that the flag conversion is well-tested and the change from setTransactionFlagsToNumber to convertTxFlagsToNumber maintains the expected behavior while fixing the modification issue.
LGTM: Flag conversion change is safe and well-tested
The change to use convertTxFlagsToNumber is verified to be safe through extensive test coverage:
- Multiple transaction types test flag conversion (Payment, OfferCreate, TrustSet, etc.)
- Tests verify both numeric and boolean flag handling
- Integration tests confirm transaction submission flow with flags
- The function maintains the same behavior as the previous implementation but avoids modifying the original transaction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any transaction submission tests or examples that might be affected
# Look for transaction submission tests
rg -A 5 "submit.*transaction" "**/*test*"
# Look for flag-related tests
rg -A 5 "transaction.*flags" "**/*test*"
# Look for examples using transaction flags
rg -A 5 "Flags.*=.*" "**/*example*"
Length of output: 277
Script:
#!/bin/bash
# Let's try more specific paths and patterns
# Look for transaction submission tests
rg -A 5 "submit.*transaction" packages/xrpl/test/
rg -A 5 "submit.*tx" packages/xrpl/test/
# Look for flag-related tests
rg -A 5 "transaction.*flags" packages/xrpl/test/
rg -A 5 "tx.*flags" packages/xrpl/test/
# Look for examples using transaction flags
fd -t f example packages/xrpl/ -x rg -A 5 "Flags.*=.*" {}
# Check test files specifically related to flags or transactions
fd -t f "(flag|transaction|tx).*test" packages/xrpl/test/
Length of output: 16123
Script:
#!/bin/bash
# Let's check the implementation and tests of flag conversion functions
# Look for flag conversion function implementations
ast-grep --pattern 'function convertTxFlagsToNumber($$$)'
# Check for flag-related tests specifically
rg -A 5 "convertTxFlagsToNumber" packages/xrpl/test/
# Look for any test files that might test flag conversion
fd -t f "flags.*test" packages/xrpl/test/
Length of output: 156
Script:
#!/bin/bash
# Let's try a broader search for flag conversion and tests
# Look for flag conversion implementations with different patterns
rg -A 10 "convertTxFlagsToNumber" packages/xrpl/src/
rg -A 10 "function.*Flags" packages/xrpl/src/
# Check for flag-related tests in models
rg -A 5 "flags" packages/xrpl/test/models/
rg -A 5 "Flags" packages/xrpl/test/models/
# Look for transaction flag tests
rg -A 5 "test.*flags" packages/xrpl/test/
Length of output: 58976
|
Will push test changes once it is decided if/how to deprecate setTransactionFlagsToNumber |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
packages/xrpl/src/models/utils/flags.ts (3)
61-75: Enhance deprecation notice documentation.Consider adding more details to help users migrate:
- Add
@sincetag to indicate when it was deprecated- Add a link to migration guide if available
- Consider adding an example of how to migrate to the new function
/** * Sets a transaction's flags to its numeric representation. * * @deprecated * This utility function is deprecated. * Use convertTxFlagsToNumber() instead and use the returned value to modify the tx.Flags from the caller. + * @since 2.x.x + * @see https://xrpl.org/blog/migration-guide (if available) + * @example + * // Old way + * setTransactionFlagsToNumber(tx); + * // New way + * tx.Flags = convertTxFlagsToNumber(tx); * * @param tx - A transaction to set its flags to its numeric representation. */
95-98: Improve error message clarity.The error message could be more descriptive to help users understand and fix the issue.
- `flag ${flag} doesn't exist in flagEnum: ${JSON.stringify(flagEnum)}`, + `Invalid flag '${flag}' for transaction type '${tx.TransactionType}'. ` + + `Allowed flags are: ${Object.keys(flagEnum).join(', ')}`,
113-133: Consider using TypeScript generics for better type safety.The function could be made more type-safe by using generics to specify the transaction type.
-export function parseTransactionFlags(tx: Transaction): object { +export function parseTransactionFlags<T extends Transaction>(tx: T): Partial<Record<keyof typeof txToFlag[T['TransactionType']], boolean>> { const flags = convertTxFlagsToNumber(tx) if (flags === 0) { return {} } - const booleanFlagMap = {} + const booleanFlagMap: Partial<Record<keyof typeof txToFlag[T['TransactionType']], boolean>> = {} const transactionTypeFlags = txToFlag[tx.TransactionType] Object.values(transactionTypeFlags).forEach((flag) => { if ( typeof flag === 'string' && isFlagEnabled(flags, transactionTypeFlags[flag]) ) { booleanFlagMap[flag] = true } }) return booleanFlagMap }packages/xrpl/test/models/utils.test.ts (3)
74-75: Test cases updated consistently, consider adding edge casesAll test cases have been properly updated to use
convertTxFlagsToNumber. While the current test coverage is good, consider adding these edge cases:
- Test with undefined flags
- Test with null flags
- Test with empty transaction object
Example test case:
it('handles undefined flags gracefully', function () { const tx: OfferCreate = { Account: 'r3rhWeE31Jt5sWmi4QiGLMZnY3ENgqw96W', TransactionType: 'OfferCreate', // Flags intentionally omitted } const flagNum = convertTxFlagsToNumber(tx) assert.strictEqual(flagNum, 0) })Also applies to: 93-94, 113-114, 140-141, 151-152
Line range hint
234-236: Fix incorrect assertion in parseTransactionFlags testsThe tests for parseTransactionFlags are using
assert.notStrictEqual, which is incorrect as it would pass even when the flags are equal. This should beassert.deepStrictEqualfor object comparison.Apply this fix:
- assert.notStrictEqual(flagsMap, expected) + assert.deepStrictEqual(flagsMap, expected)Also applies to: 257-259
Based on the gathered information, I can now provide the final response:
Test coverage missing for several transaction types with flags
The current test suite only covers flag conversion for:
- OfferCreate
- PaymentChannelClaim
- Payment
- TrustSet
However, the codebase defines flags for several other transaction types that lack test coverage:
- AMMWithdraw
- AMMDeposit
- EnableAmendment
- NFTokenCreateOffer
- NFTokenMint
- XChainModifyBridge
- AccountSet (both AsfFlags and TfFlags)
These transaction types should have their flag conversion and parsing logic tested to ensure reliable behavior.
🔗 Analysis chain
Line range hint
1-276: Verify test coverage for all transaction typesLet's ensure we have test coverage for all transaction types that support flags.
Let me gather more specific information about the transaction types and their flag coverage.
Let me try a different approach to find the flag definitions and their test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all transaction types with flags are covered in tests # Search for all transaction types that have flags echo "Searching for transaction types with flags..." rg -l "Flags" --type ts | grep -v "test" # Search for corresponding test coverage echo "Checking test coverage..." rg -l "TransactionType.*Flags" --type ts | grep "test"Length of output: 2514
Script:
#!/bin/bash # Check transaction types with flags and their test coverage # Find all transaction types that define flags echo "Transaction types with flag definitions:" ast-grep --pattern 'export const enum $_Flags { $$$ }' # Find flag usage in tests echo -e "\nFlag testing in test files:" rg "describe.*Flags" --type tsLength of output: 349
Script:
#!/bin/bash echo "Finding transaction flag definitions:" rg "enum.*Flags" --type ts -A 5 --glob "!**/*.test.ts" echo -e "\nFinding flag tests in test files:" rg "sets.*Flags.*numeric" --type ts -B 1 -A 1Length of output: 9297
packages/xrpl/HISTORY.md (1)
Line range hint
1-24: Consider adding SHA-256 checksums for browser buildsPrevious releases included SHA-256 checksums for browser builds. Consider adding these for the latest changes to maintain consistency with historical releases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/index.ts(1 hunks)packages/xrpl/src/models/utils/flags.ts(2 hunks)packages/xrpl/test/models/utils.test.ts(6 hunks)
🔇 Additional comments (4)
packages/xrpl/src/models/index.ts (1)
12-13: Consider adding JSDoc comments for the exported functions.
While the changes align with the PR objectives, adding JSDoc comments for both functions would help users understand:
- That
setTransactionFlagsToNumberis deprecated - Why they should use
convertTxFlagsToNumberinstead - The behavioral difference between the two functions (mutation vs. return)
Let's verify the deprecation warning implementation:
packages/xrpl/src/models/utils/flags.ts (1)
114-114: Changes successfully address the unintended modification issue.
The implementation now correctly uses convertTxFlagsToNumber without modifying the input transaction object, which aligns with the PR objectives.
packages/xrpl/test/models/utils.test.ts (1)
18-18: LGTM: Import statement updated correctly
The import statement has been properly updated to use the new convertTxFlagsToNumber function, aligning with the PR's objective.
packages/xrpl/HISTORY.md (1)
6-8: LGTM! The changes improve transaction flag handling
The changes look good and follow best practices:
- Fix prevents unintended mutation of transaction objects
- Clear deprecation notice for old function
- New utility function with clearer naming
packages/xrpl/HISTORY.md
Outdated
| @@ -3,6 +3,9 @@ | |||
| Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xrpl-announce) for release announcements. We recommend that xrpl.js (ripple-lib) users stay up-to-date with the latest stable release. | |||
|
|
|||
| ## Unreleased Changes | |||
| * Fix for parseTransactionFlags to no longer modify a passed in transaction when returning a parsed mapping | |||
There was a problem hiding this comment.
This should be under a heading that matches the actions - see https://keepachangelog.com/en/1.1.0/
|
Looks like you'll need to ignore deprecation warnings like this: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/xrpl/test/models/utils.test.ts (3)
Line range hint
46-165: Fix assertion in parseTransactionFlags testsThe tests for
parseTransactionFlagsuseassert.notStrictEqualfor comparing objects, which might not work as intended. For object comparisons, consider usingassert.deepEqualorassert.deepStrictEqual.Apply this change to the assertions:
- assert.notStrictEqual(flagsMap, expected) + assert.deepStrictEqual(flagsMap, expected)
Line range hint
167-214: Simplify complex assertion in parseAccountRootFlags testThe current assertion chain using multiple
&&operators could be simplified for better readability and maintenance.Consider refactoring the assertion to use
Object.entries:- assert.isTrue( - parsed.lsfDefaultRipple && - parsed.lsfDepositAuth && - parsed.lsfDisableMaster && - parsed.lsfDisallowXRP && - parsed.lsfGlobalFreeze && - parsed.lsfNoFreeze && - parsed.lsfPasswordSpent && - parsed.lsfRequireAuth && - parsed.lsfRequireDestTag && - parsed.lsfDisallowIncomingNFTokenOffer && - parsed.lsfDisallowIncomingCheck && - parsed.lsfDisallowIncomingPayChan && - parsed.lsfDisallowIncomingTrustline && - parsed.lsfAllowTrustLineClawback, - ) + Object.entries(parsed).forEach(([key, value]) => { + assert.isTrue(value, `Flag ${key} should be enabled`); + });
Line range hint
216-286: Fix inconsistencies in parseTransactionFlags testsThere are several issues in these tests:
- Test descriptions don't match the assertions (e.g., "all enabled" but only testing one flag)
- Using
notStrictEqualinstead of proper object comparison- Missing test cases for other transaction types
Consider these improvements:
- Fix test descriptions:
- it('parseTransactionFlags all enabled', function () { + it('parseTransactionFlags with tfRenew enabled', function () {
- Fix assertions:
- assert.notStrictEqual(flagsMap, expected) + assert.deepStrictEqual(flagsMap, expected)
- Add test cases for other transaction types to match the coverage in
setTransactionFlagsToNumbertests.Would you like me to help generate additional test cases for other transaction types?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/xrpl/src/models/utils/flags.ts(2 hunks)packages/xrpl/test/models/utils.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/utils/flags.ts
🔇 Additional comments (2)
packages/xrpl/test/models/utils.test.ts (2)
Line range hint 1-19: LGTM: Appropriate handling of deprecated function
The ESLint directive and comment clearly explain why the deprecated setTransactionFlagsToNumber is still being used in tests. This aligns with the discussion in previous reviews about maintaining backward compatibility.
Line range hint 28-44: LGTM: Comprehensive flag testing
The tests for isFlagEnabled are well-structured with:
- Clear setup using
beforeEach - Coverage of both enabled and disabled flag scenarios
- Proper use of bitwise operations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xrpl/src/models/utils/flags.ts (1)
113-113: Consider documenting the fallback behaviorThe function returns 0 when no flags are present or when the transaction type is not in
txToFlag. Consider adding a comment explaining this behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/utils/flags.ts(2 hunks)
🔇 Additional comments (5)
packages/xrpl/src/models/utils/flags.ts (5)
58-62: Well-implemented type guard function!
The new type guard function improves type safety by properly narrowing transaction types, replacing previous type assertions. This is a good TypeScript pattern.
91-94: Good type check for numeric flags
The early return for numeric flags is a good optimization and maintains backward compatibility.
130-140: Improved flag mapping logic
The new implementation with booleanFlagMap is more readable and maintainable. The type checking and flag enabling logic is well-structured.
123-126: Consider preserving null flag behavior
Based on previous review discussions, we should verify if returning an empty object for zero flags maintains backward compatibility with the old behavior where null flags were handled differently.
✅ Verification successful
Let's also check the actual implementation and tests:
Based on the evidence from the test files and implementation, I can now provide the final response:
Returning empty object for zero flags is correct behavior
The current implementation's behavior of returning an empty object for zero flags is correct and maintains backward compatibility. This is evidenced by:
- The test case
parseTransactionFlags all falseexplicitly verifies that zero flags should return an empty object. - The codebase handles
nullflags differently in validation (e.g., in Payment transactions) rather than in flag parsing. - The
parseTransactionFlagsfunction is meant to return a boolean map of enabled flags, and when no flags are enabled (zero), an empty object is the appropriate representation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or usages of null flags
rg "tx\.Flags\s*=\s*null" --type ts -A 2
Length of output: 42
Script:
#!/bin/bash
# Let's search for tests related to flag handling and null values
rg "Flags.*null|null.*Flags" --type ts -A 2
# Also search for flag-related test files
fd "flags?.test" -e ts
# Look for the previous implementation to understand the old behavior
git log -p -- "**/flags.ts"
Length of output: 591
Script:
# Check the current implementation
cat packages/xrpl/src/models/utils/flags.ts
# Look for test files that might verify flag behavior
fd test -e ts -x grep -l "Flags" {}
Length of output: 7117
Script:
# Let's check the test file for flag parsing behavior
cat packages/xrpl/test/models/utils.test.ts
# Also check the payment transaction test since it has explicit null flag handling
cat packages/xrpl/src/models/transactions/payment.ts
Length of output: 16416
96-111: Verify the validation logic completeness
The validation logic looks good, but we should verify that all error cases are properly handled:
- Invalid transaction types
- Invalid flag values
- Null/undefined checks
✅ Verification successful
Based on the code analysis results, I can now generate the final response:
Validation logic is properly implemented with complete error handling
The flag validation logic is comprehensive and handles all error cases:
- Invalid transaction types are checked via
istxToFlagKey(tx.TransactionType) - Invalid flag values are validated against the transaction-specific flag enum with proper error handling:
if (flagEnum[flag] == null) { throw new ValidationError(...) } - Null/undefined checks are in place:
- Checks if
tx.Flagsexists before processing - Handles both numeric and object flag representations
- Validates against transaction-specific flag enums
- Checks if
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing error handling patterns
rg "ValidationError.*flag" --type ts -A 2
Length of output: 43
Script:
#!/bin/bash
# Let's first check the ValidationError class and its usage
rg "class ValidationError" --type ts -A 5
# Then check for any flag-related error handling in the codebase
rg "throw.*flag" --type ts -A 2
# Let's also check the transaction type and flag enums
ast-grep --pattern 'export type $_ = {
$$$
}'
# And check how flags are validated in other parts
rg "Flags\?" --type ts -A 2
Length of output: 3763
Script:
#!/bin/bash
# Let's check the transaction type enum and flag mapping
ast-grep --pattern 'const $_ = {
$$$
}' --lang ts
# Check for null/undefined handling patterns
rg "if.*tx\.Flags" --type ts -A 3
# Let's also look at the istxToFlagKey function implementation
ast-grep --pattern 'function istxToFlagKey($_): $_ {
$$$
}'
Length of output: 66915
Script:
#!/bin/bash
# Let's check the istxToFlagKey function and txToFlag mapping
rg "istxToFlagKey|txToFlag" --type ts -A 5
# Check for flag validation patterns
rg "if.*Flags" --type ts -A 3
# Look for flag-related error handling
rg "throw.*ValidationError.*flag" --type ts -A 2
Length of output: 11362
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/src/models/utils/flags.ts (2)
67-83: Enhance deprecation notice with standard JSDoc format.While the deprecation is well-documented, consider enhancing the JSDoc comment to follow standard format:
/** * Sets a transaction's flags to its numeric representation. * - * @deprecated - * This utility function is deprecated. - * Use convertTxFlagsToNumber() instead and use the returned value to modify the tx.Flags from the caller. + * @deprecated since version X.Y.Z + * @see convertTxFlagsToNumber() - Use this instead and modify tx.Flags with the returned value * * @param tx - A transaction to set its flags to its numeric representation. */
102-106: Improve error message clarity.The error message could be more descriptive to help users understand and fix the issue:
- `flag ${flag} doesn't exist in flagEnum: ${JSON.stringify(flagEnum)}`, + `Invalid flag '${flag}' for transaction type '${tx.TransactionType}'. Valid flags are: ${Object.keys(flagEnum).join(", ")}`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/utils/flags.ts(2 hunks)
🔇 Additional comments (3)
packages/xrpl/src/models/utils/flags.ts (3)
58-62: LGTM! Well-implemented type guard.
The type guard improves type safety and follows TypeScript best practices.
122-141: LGTM! Improved clarity and efficiency.
The changes improve the function by:
- Using a more descriptive variable name
booleanFlagMap - Adding an early return optimization for zero flags
- Simplifying the flag parsing logic
67-83: Verify migration to new function across codebase.
Let's ensure all usages of the deprecated function are updated:
✅ Verification successful
Migration to new function is properly implemented
The verification shows a clean migration pattern:
- The deprecated function is only present in:
flags.ts: The deprecation implementation itselfindex.ts: Just the export statementutils.test.ts: Tests specifically checking backward compatibility
The new convertTxFlagsToNumber function is properly integrated in core functionality:
- Used in transaction processing (
transaction.ts) - Integrated in client operations (
client/index.ts) - Properly exported (
models/index.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find remaining usages of deprecated function
echo "Searching for remaining usages of deprecated function..."
rg "setTransactionFlagsToNumber" --type ts -l
# Find usages of new function
echo "Verifying adoption of new function..."
rg "convertTxFlagsToNumber" --type ts -l
# Check for any TODO comments related to migration
echo "Checking for migration TODOs..."
rg "TODO.*setTransactionFlagsToNumber" --type ts
Length of output: 693
Script:
#!/bin/bash
# Let's check the actual usage context of the deprecated function
echo "Checking usage context of deprecated function in index.ts..."
rg "setTransactionFlagsToNumber" packages/xrpl/src/models/index.ts -B 2 -A 2
echo "Checking test usage..."
rg "setTransactionFlagsToNumber" packages/xrpl/test/models/utils.test.ts -B 2 -A 2
echo "Checking usage of new function..."
rg "convertTxFlagsToNumber" packages/xrpl/src/models/transactions/transaction.ts -B 2 -A 2
rg "convertTxFlagsToNumber" packages/xrpl/src/client/index.ts -B 2 -A 2
Length of output: 2686
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/xrpl/test/models/utils.test.ts (1)
Line range hint
217-273: Fix incorrect assertions in parseTransactionFlags testsThe test suite contains critical issues:
assert.notStrictEqualis used whereassert.deepStrictEqualshould be used- This means the tests are passing for the wrong reason - they're asserting the objects are different when they should be equal
Apply these changes to fix the assertions:
- assert.notStrictEqual(flagsMap, expected) + assert.deepStrictEqual(flagsMap, expected)
🧹 Nitpick comments (3)
packages/xrpl/test/models/utils.test.ts (3)
51-108: Enhance test coverage for parseAccountRootFlagsWhile the current tests cover the extremes (all flags enabled/disabled), consider adding:
- Individual flag tests for better isolation of failures
- Edge cases with specific flag combinations
- Break down the complex assertion for better error messages
Example test case to add:
it('parses individual flags correctly', function () { const flags = AccountRootFlags.lsfDefaultRipple const parsed = parseAccountRootFlags(flags) assert.isTrue(parsed.lsfDefaultRipple) assert.isUndefined(parsed.lsfDepositAuth) // ... other flags should be undefined })
274-379: Add error handling tests for convertTxFlagsToNumberThe test suite should include cases for error conditions and edge cases:
- Invalid flag combinations
- Undefined/null flags
- Invalid transaction types
Example test case to add:
it('handles invalid flag combinations gracefully', function () { const tx: Payment = { TransactionType: 'Payment', Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo', Amount: '1234', Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy', Flags: { tfNoRippleDirect: true, tfPartialPayment: true, invalidFlag: true, // should be ignored }, } const { tfNoRippleDirect, tfPartialPayment } = PaymentFlags const expected: number = tfNoRippleDirect | tfPartialPayment const result = convertTxFlagsToNumber(tx) assert.strictEqual(result, expected) })
Line range hint
1-379: Consider reorganizing test file for better maintainabilityThe test file maintains good coverage but could benefit from:
- Shared test fixtures to reduce duplication between
setTransactionFlagsToNumberandconvertTxFlagsToNumbertest suites- Helper functions for common assertion patterns
- Consistent error case coverage across all test suites
Example shared fixture:
const TEST_TRANSACTIONS = { offerCreate: { Account: 'r3rhWeE31Jt5sWmi4QiGLMZnY3ENgqw96W', // ... other common fields Flags: { tfPassive: true, tfImmediateOrCancel: false, tfFillOrKill: true, tfSell: false, }, }, // ... other test transactions }
|
@achowdhry-ripple this looks good. I'd like to see the CI/CD test results. They will be executed once the merge conflicts are resolved. |
High Level Overview of Change
the parseTransactionFlags method unintentionally edited the Transaction object's flags when passed in as a parameter, because it used setTransactionFlagsToNumber inside. Instead, now it uses convertTxFlagsToNumber, which passes the numerical representation of flags back to the caller instead of automatically modifying the passed in object. This is clearer for a user, and avoids unexpected modification and reduces redundant need for both functions.
As cleanup, all uses of the setter were removed and moving forward we will be using the converter instead. The setter remains as a deprecated function for compatibility support since it is an exported function.
convertFlagsToNumber flags has been renamed to convertTxFlagsToNumber for clarity
Context of Change
#2823
Type of Change
Did you update HISTORY.md?
Test Plan