Removed calls to deprecated utils function in tests #2845
Conversation
…omorphic/utils`'s `stringToHex`
…morphic/utils`'s `hexToString`
WalkthroughThe pull request introduces changes across multiple files to replace the Changes
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: 3
🧹 Outside diff range and nitpick comments (1)
packages/xrpl/test/utils/hexConversion.test.ts (1)
1-1: LGTM! Consider adding edge case tests.The migration from deprecated utils to
@xrplf/isomorphic/utilsis correctly implemented. The test cases maintain proper coverage for both ASCII and Unicode strings.Consider adding test cases for:
- Empty strings
- Strings with special characters
- Very long strings
- Invalid hex strings (for
hexToString)Also applies to: 7-7, 13-13, 23-23, 25-25
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
packages/xrpl/snippets/src/multisigning.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(1 hunks)packages/xrpl/test/integration/integration.test.ts(2 hunks)packages/xrpl/test/integration/regularKey.test.ts(5 hunks)packages/xrpl/test/integration/requests/submit.test.ts(2 hunks)packages/xrpl/test/integration/requests/submitMultisigned.test.ts(3 hunks)packages/xrpl/test/integration/requests/tx.test.ts(3 hunks)packages/xrpl/test/integration/submitAndWait.test.ts(5 hunks)packages/xrpl/test/integration/transactions/nftokenMint.test.ts(2 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(4 hunks)packages/xrpl/test/utils/hexConversion.test.ts(2 hunks)
🔇 Additional comments (12)
packages/xrpl/snippets/src/multisigning.ts (1)
1-1: LGTM! Import and usage correctly updated.
The migration to stringToHex from @xrplf/isomorphic/utils is properly implemented for the Domain field conversion.
Also applies to: 54-54
packages/xrpl/test/integration/transactions/nftokenMint.test.ts (1)
1-1: LGTM! Verify URI handling across NFT-related tests.
The migration to stringToHex is correctly implemented. Since URI handling is critical for NFT minting, let's verify consistency across related files.
Also applies to: 36-36
✅ Verification successful
Let me check the actual URI handling implementation in these files to verify consistency.
Let me try a different approach to check the URI handling implementation.
URI handling is consistent across NFT-related files
The verification shows that:
- The model properly validates URI in hex format
- Both model tests and integration tests consistently use
stringToHexfrom@xrplf/isomorphic/utilsfor URI conversion - No deprecated conversion functions are used in NFT-related code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check URI handling consistency across NFT-related files
# Search for URI-related hex conversions in NFT files
echo "Checking URI conversions in NFT-related files:"
rg -l "URI.*[Hh]ex.*" --type ts | grep -i "nft"
# Search for any remaining usage of deprecated conversion functions
echo -e "\nChecking for any remaining deprecated conversion calls:"
rg -i "convert(String|Hex)To(String|Hex)" --type ts
Length of output: 1019
Script:
#!/bin/bash
# Check URI handling implementation in NFT-related files
echo "NFTokenMint.ts implementation:"
ast-grep --pattern 'URI = $$$' packages/xrpl/src/models/transactions/NFTokenMint.ts
echo -e "\nNFTokenMint.test.ts model test:"
rg -A 5 "URI.*=" packages/xrpl/test/models/NFTokenMint.test.ts
echo -e "\nNFTokenMint integration test:"
rg -A 5 "URI.*=" packages/xrpl/test/integration/transactions/nftokenMint.test.ts
Length of output: 458
Script:
#!/bin/bash
# Check URI handling in NFT-related files with broader patterns
echo "NFTokenMint.ts implementation:"
rg -B2 -A2 "URI" packages/xrpl/src/models/transactions/NFTokenMint.ts
echo -e "\nModel test URI handling:"
rg -B2 -A2 "stringToHex" packages/xrpl/test/models/NFTokenMint.test.ts
echo -e "\nIntegration test URI handling:"
rg -B2 -A2 "stringToHex" packages/xrpl/test/integration/transactions/nftokenMint.test.ts
Length of output: 2007
packages/xrpl/test/integration/requests/submit.test.ts (1)
1-1: LGTM: Correctly migrated to stringToHex
The deprecated convertStringToHex function has been properly replaced with stringToHex from @xrplf/isomorphic/utils. The import and usage are correct.
Also applies to: 38-38
packages/xrpl/test/models/NFTokenMint.test.ts (2)
1-1: LGTM: Consistent migration to stringToHex across test cases
The deprecated function has been properly replaced with stringToHex across all test cases. The changes maintain the test coverage for URI validation, including hex format verification.
Also applies to: 28-28, 43-43, 63-63
Line range hint 73-108: Well-structured test coverage for URI validation
Excellent test coverage with cases for:
- Empty URI validation
- Non-hex format validation
This ensures robust validation of the URI field format requirements.
packages/xrpl/test/integration/requests/tx.test.ts (1)
1-1: LGTM: Comprehensive migration across API versions
The deprecated function has been properly replaced with stringToHex in both the base test and API version 1 test. The changes maintain compatibility across different API versions while modernizing the code.
Also applies to: 38-38, 85-85
packages/xrpl/test/integration/integration.test.ts (1)
1-1: LGTM! Successfully migrated to stringToHex
The changes correctly replace the deprecated convertStringToHex with stringToHex from the isomorphic utils package, maintaining the same functionality.
Also applies to: 77-77
packages/xrpl/src/models/transactions/NFTokenMint.ts (1)
95-97: LGTM! Documentation updated correctly
The URI field documentation has been properly updated to reference the new stringToHex function from @xrplf/isomorphic/utils, maintaining consistency with the codebase changes.
packages/xrpl/test/integration/requests/submitMultisigned.test.ts (2)
1-1: LGTM: Import statement correctly added
The import of stringToHex from @xrplf/isomorphic/utils is properly placed at the top of the file.
78-78: LGTM: Consistent replacement of deprecated function
The Domain field in both test cases has been correctly updated to use stringToHex instead of the deprecated convertStringToHex function. The change maintains the same functionality while using the recommended utility function.
Also applies to: 151-151
packages/xrpl/test/integration/regularKey.test.ts (2)
1-1: LGTM: Import statement correctly added
The import of stringToHex from @xrplf/isomorphic/utils is properly placed at the top of the file.
88-88: LGTM: Consistent replacement of deprecated function across all test cases
The Domain field has been correctly updated to use stringToHex instead of the deprecated convertStringToHex function in all four test cases:
- "sign and submit with a regular key"
- "sign and submit using the master key of an account with a regular key"
- "submit_multisigned transaction with regular keys set"
- "try multisigning with the account address used to set up a regular key"
The changes maintain consistency across all test scenarios while using the recommended utility function.
Also applies to: 106-106, 262-262, 322-322
| @@ -1,3 +1,4 @@ | |||
| import { stringToHex } from '@xrplf/isomorphic/utils' | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove deprecated convertStringToHex function and its exports
The deprecated convertStringToHex function is still present in the codebase:
packages/xrpl/src/utils/stringConversion.ts: Function definition and exportpackages/xrpl/src/utils/index.ts: Import and re-export
While the function internally uses the new stringToHex from @xrplf/isomorphic/utils, it should be removed to avoid confusion and maintain consistency. The PR's changes should include removing this deprecated function since direct usage of stringToHex from @xrplf/isomorphic/utils is now the standard.
🔗 Analysis chain
Verify complete removal of deprecated function
Let's ensure all instances of the deprecated convertStringToHex function have been removed from the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of convertStringToHex
rg "convertStringToHex" --type ts
# Search for all string-to-hex conversion patterns to ensure consistency
rg "(?i)(convert|to).*hex.*string" --type ts
Length of output: 4665
| @@ -1,6 +1,7 @@ | |||
| import { stringToHex } from '@xrplf/isomorphic/utils' | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to stringToHex is incomplete - Found additional instances to update
The deprecated convertStringToHex function is still present in the codebase:
- It's defined in
packages/xrpl/src/utils/stringConversion.ts(though it's now just a wrapper aroundstringToHex) - It's exported in
packages/xrpl/src/utils/index.ts
These instances should also be removed to complete the migration to stringToHex.
🔗 Analysis chain
LGTM! Consistent migration across all test cases
The changes correctly replace all instances of the deprecated convertStringToHex with stringToHex from the isomorphic utils package. The migration is consistent across all test scenarios:
- Unsigned transaction test
- Validation error test
- Signed transaction test
- Extended wait test
Let's verify that we haven't missed any instances of the deprecated function:
Also applies to: 39-39, 93-93, 111-111, 134-134
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of convertStringToHex
rg "convertStringToHex" --type ts
Length of output: 389
Script:
#!/bin/bash
# Let's check if there are any remaining usages of convertStringToHex in the codebase
rg "convertStringToHex\(" --type ts -A 2
# Also check for any imports of convertStringToHex
rg "import.*convertStringToHex" --type ts
Length of output: 412
| @@ -1,3 +1,4 @@ | |||
| import { stringToHex } from '@xrplf/isomorphic/utils' | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration is incomplete - convertStringToHex is still present in the codebase
The deprecated convertStringToHex function is still present in:
packages/xrpl/src/utils/stringConversion.ts(function definition)packages/xrpl/src/utils/index.ts(import and re-export)
While the function internally uses stringToHex, its presence might confuse developers. Consider:
- Removing the deprecated function
- Updating any remaining imports to use
stringToHexdirectly
🔗 Analysis chain
Verify complete migration from deprecated function
Let's ensure there are no remaining instances of the deprecated convertStringToHex function in the codebase.
Let me gather more information to understand if the old function is still being used or just defined.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of the deprecated function
# Expected: No results, indicating complete migration
# Search for any remaining instances of convertStringToHex
rg "convertStringToHex" --type-add 'ts:*.{ts,tsx}' --type ts
# Search for new stringToHex usage to confirm migration
rg "stringToHex.*example\.com" --type-add 'ts:*.{ts,tsx}' --type ts
Length of output: 1937
Script:
#!/bin/bash
# Search for actual usage of convertStringToHex (not just definitions)
rg "convertStringToHex\(" --type-add 'ts:*.{ts,tsx}' --type ts -C 2
# Check if there are any imports of convertStringToHex in the codebase
rg "import.*convertStringToHex" --type-add 'ts:*.{ts,tsx}' --type ts
Length of output: 581
|
Would be great if we can change the call in this snippet as well: |
It is included on the first commit of this pull request ! @pdp2121 |
|
IMO deprecated functions shouldn't be removed from tests until the functions are fully removed - just because they're deprecated doesn't mean they should fail. |
|
I simply replaced them with the up-to-date functions. The deprecated functions themselves pointed to the new ones that I used, so nothing is failing ! I will update my pull request text, because I realise I was not specific enough. |
I agree with @mvadari on this one. Maybe at least have one or two of the tests for the deprecated ones. It wouldn't hurt. |
If you roll back the changes to |
…xrplf/isomorphic/utils`'s `stringToHex`" This reverts commit f5a06d7.
|
Done ! Thanks for the feedback @mvadari , very appreciated |
|
What is the expected notice period before a deprecated method can be removed from the codebase? Is it one major-version release cycle ? |
High Level Overview of Change
See #2698 .
Removed multiple calls to deprecated functions in the tests (convertStringToHex and convertHexToString) and replaced them with the new version (@xrplf/isomorphic/util's stringToHex and hexToString).
The deprecated functions where already just pointing to the new one.
Also updated the models/transactions/NFTokenMint.ts comments to point at the correct functions.
Context of Change
Simple refactor to follow best practices. The tests could be referenced by a user and he should always have the correct function used by default.
Type of Change
Did you update HISTORY.md?