Fix: parseTransactionFlags unintentionally modifies transaction #2825
Conversation
|
Warning Rate limit exceeded@achowdhry-ripple has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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
|
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
|
@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. |
| assert.isUndefined(parsed.lsfDefaultRipple) | ||
| assert.isUndefined(parsed.lsfDepositAuth) | ||
| assert.isUndefined(parsed.lsfDisableMaster) | ||
| assert.isUndefined(parsed.lsfDisallowXRP) | ||
| assert.isUndefined(parsed.lsfGlobalFreeze) | ||
| assert.isUndefined(parsed.lsfNoFreeze) | ||
| assert.isUndefined(parsed.lsfPasswordSpent) | ||
| assert.isUndefined(parsed.lsfRequireAuth) | ||
| assert.isUndefined(parsed.lsfRequireDestTag) | ||
| assert.isUndefined(parsed.lsfDisallowIncomingNFTokenOffer) | ||
| assert.isUndefined(parsed.lsfDisallowIncomingCheck) | ||
| assert.isUndefined(parsed.lsfDisallowIncomingPayChan) | ||
| assert.isUndefined(parsed.lsfDisallowIncomingTrustline) | ||
| assert.isUndefined(parsed.lsfAllowTrustLineClawback) |
There was a problem hiding this comment.
This test is misleading because its title implies set to false but really they're undefined. undefined != false in JS. Instead, you can rename the test to all flags not set.
There was a problem hiding this comment.
Also, it's worth creating another test for explicitly setting flags to false. That should cover the remaining cases.
There was a problem hiding this comment.
Since we are parsing "0" as the numerical form, we are checking an all-false flagset in this test. Also because the definition of the flag utility function returns only true flags, an empty object would by definition be equivalent to all false flags, without it being explicitly stated per each flag. So this logic is also equivalent to checking for an empty flag set object returned, but this seems clearer to show all the expected flags cannot be found. To me, I'd prefer to leave it as this since logically it is correct, and adding 2 different tests for false and undefined would make it look like there is difference in meaning with flags for the 2 values when they are interpreted the same in these utilities. Also this was an existing test, not something I modified
There was a problem hiding this comment.
I see. I do agree in the application sense being undefined is also considered to be false but in the JS context these are inequivalent. And I would also consider passing 0 as not setting any flags. Therefore, I still do think we should rename the test to "no flags set"
There was a problem hiding this comment.
sure can rename it
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
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