feat(dev): add support for pulling definitions from Github #3046
+362
−298
Conversation
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
|
""" WalkthroughThe scripts in both Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/xrpl/tools/generateModels.js (1)
204-287: Fix potential race condition with async forEach.Using
forEachwith async callbacks doesn't wait for the operations to complete, which could lead to race conditions when multiple transactions update the same files (transaction.ts and index.ts).Apply this diff to ensure sequential processing:
- txsToAdd.forEach(async (tx) => { + for (const tx of txsToAdd) {And ensure the main function is async and awaited:
- main(process.argv[2]) + main(process.argv[2]).catch(err => { + console.error('Error:', err) + process.exit(1) + })
🧹 Nitpick comments (2)
packages/xrpl/tools/generateModels.js (2)
10-41: Consider extracting shared GitHub/file reading logic.The
readFileFromGitHubandreadFilefunctions are duplicated between this file andgenerateDefinitions.js. Consider creating a shared utility module to follow the DRY principle.Create a new file
tools/fileUtils.js:const fs = require('fs/promises') const path = require('path') async function readFileFromGitHub(repo, filename) { // ... existing implementation } async function readFile(folder, filename) { // ... existing implementation } module.exports = { readFileFromGitHub, readFile }Then import and use these functions in both scripts.
64-64: Remove debug logging.This console.log appears to be debug code that should be removed.
Apply this diff:
- console.log(txFormatsHits)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ripple-binary-codec/tools/generateDefinitions.js(2 hunks)packages/xrpl/tools/generateModels.js(11 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:18-18
Timestamp: 2024-10-08T16:29:11.194Z
Learning: `SField.cpp` and `TxFormats.cpp` are located in the `rippled` repository (`https://github.com/XRPLF/rippled/`), not in the current repository.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:36-36
Timestamp: 2024-11-18T23:29:19.882Z
Learning: In the file `packages/xrpl/tools/generateModels.js`, it's acceptable for the script to crash when the regex doesn't match as expected.
packages/ripple-binary-codec/tools/generateDefinitions.js (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:18-18
Timestamp: 2024-10-08T16:29:11.194Z
Learning: `SField.cpp` and `TxFormats.cpp` are located in the `rippled` repository (`https://github.com/XRPLF/rippled/`), not in the current repository.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
packages/xrpl/tools/generateModels.js (18)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:36-36
Timestamp: 2024-11-18T23:29:19.882Z
Learning: In the file `packages/xrpl/tools/generateModels.js`, it's acceptable for the script to crash when the regex doesn't match as expected.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:18-18
Timestamp: 2024-10-08T16:29:11.194Z
Learning: `SField.cpp` and `TxFormats.cpp` are located in the `rippled` repository (`https://github.com/XRPLF/rippled/`), not in the current repository.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/src/Wallet/batchSigner.ts:0-0
Timestamp: 2025-04-16T15:55:50.121Z
Learning: When using `encodeForSigningBatch` for Batch transactions in the XRPL.js library, the field names should use camelCase (`flags`, `txIDs`) even though the transaction object properties themselves use PascalCase (`Flags`, `TxIDs`).
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
🧬 Code Graph Analysis (2)
packages/ripple-binary-codec/tools/generateDefinitions.js (1)
packages/xrpl/tools/generateModels.js (9)
fs(5-5)url(14-14)response(23-23)filePath(35-35)path(6-6)read(44-44)sfieldMacroFile(45-48)transactionsMacroFile(57-60)sfieldHits(49-51)
packages/xrpl/tools/generateModels.js (3)
packages/ripple-binary-codec/tools/generateDefinitions.js (8)
fs(16-16)path(15-15)url(22-22)response(31-31)filePath(43-43)read(51-53)sfieldMacroFile(60-63)transactionsMacroFile(69-72)packages/xrpl/tools/createValidateTests.js (3)
fs(1-1)path(2-2)filename(23-26)packages/xrpl/tools/createValidate.js (3)
fs(6-6)path(7-7)filename(15-18)
🔇 Additional comments (6)
packages/ripple-binary-codec/tools/generateDefinitions.js (4)
16-16: LGTM!Correct usage of
fs/promisesfor async file operations.
18-40: LGTM!The GitHub URL transformation and error handling are correctly implemented.
42-49: LGTM!Proper async implementation with informative error messages.
51-53: LGTM!Clean conditional selection between GitHub and local file readers.
packages/xrpl/tools/generateModels.js (2)
5-5: LGTM!Consistent use of
fs/promisesfor async operations.
62-62: LGTM!The regex enhancement correctly handles the
Delegation::namespace pattern.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/xrpl/tools/generateModels.js (1)
201-289: Address concurrency issue with forEach and async callbacks.The main function has a potential issue with concurrent async operations that could lead to race conditions.
The
forEachwith async callbacks on line 204 doesn't wait for completion and could cause race conditions when multiple transactions update the same files simultaneously.- txsToAdd.forEach(async (tx) => { + for (const tx of txsToAdd) { // ... existing logic ... - }) + }This ensures transactions are processed sequentially, preventing race conditions when updating shared files like
transaction.tsandindex.ts.The rest of the async conversions (lines 258, 267, 277, 282, 284) are correct and maintain proper error handling.
🧹 Nitpick comments (1)
packages/xrpl/tools/generateModels.js (1)
43-93: Review GitHub URL detection and function signature.The function correctly implements conditional reading based on URL type, but the GitHub URL detection could be more robust.
Consider improving the GitHub URL detection:
- const folderUrl = new URL(folder) - const read = folderUrl.host === 'github.com' ? readFileFromGitHub : readFile + const isGitHubUrl = folder.includes('github.com') + const read = isGitHubUrl ? readFileFromGitHub : readFileThis avoids potential URL constructor errors for local paths and is more straightforward.
The regex update on line 63 correctly adds support for the
Delegation::namespace segment as mentioned in the AI summary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ripple-binary-codec/tools/generateDefinitions.js(2 hunks)packages/xrpl/tools/generateModels.js(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ripple-binary-codec/tools/generateDefinitions.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:18-18
Timestamp: 2024-10-08T16:29:11.194Z
Learning: `SField.cpp` and `TxFormats.cpp` are located in the `rippled` repository (`https://github.com/XRPLF/rippled/`), not in the current repository.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:36-36
Timestamp: 2024-11-18T23:29:19.882Z
Learning: In the file `packages/xrpl/tools/generateModels.js`, it's acceptable for the script to crash when the regex doesn't match as expected.
packages/xrpl/tools/generateModels.js (18)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:36-36
Timestamp: 2024-11-18T23:29:19.882Z
Learning: In the file `packages/xrpl/tools/generateModels.js`, it's acceptable for the script to crash when the regex doesn't match as expected.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:18-18
Timestamp: 2024-10-08T16:29:11.194Z
Learning: `SField.cpp` and `TxFormats.cpp` are located in the `rippled` repository (`https://github.com/XRPLF/rippled/`), not in the current repository.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/src/Wallet/batchSigner.ts:0-0
Timestamp: 2025-04-16T15:55:50.121Z
Learning: When using `encodeForSigningBatch` for Batch transactions in the XRPL.js library, the field names should use camelCase (`flags`, `txIDs`) even though the transaction object properties themselves use PascalCase (`Flags`, `TxIDs`).
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/xrpl/tools/generateModels.js (5)
5-5: LGTM! Proper async module import.The change from synchronous
fstofs/promisesis appropriate for the async refactoring.
34-41: LGTM! Proper async file reading with error handling.The conversion to async is correct and maintains appropriate error handling.
124-176: LGTM! Proper async file operations.The conversion to async file writing is correct and maintains the existing logic for updating transaction files.
178-192: LGTM! Consistent async pattern.The function correctly uses async file operations while maintaining the existing logic.
10-32: Enforce Input Validation in readFileFromGitHubTo prevent host-spoofing and path-traversal attacks, validate both the
repoandfilenameinputs before constructing the raw.githubusercontent URL.• packages/xrpl/tools/generateModels.js (lines 10–32)
• Add a helper to parse & verify the GitHub URL:function parseGitHubRepo(repo) { // Ensure a protocol is present for URL parsing const url = new URL(repo.startsWith('http') ? repo : `https://${repo}`) if (url.hostname !== 'github.com') { throw new Error(`Invalid GitHub host: ${url.hostname}`) } const parts = url.pathname.split('/').filter(Boolean) if (parts.length < 2) { throw new Error(`Invalid repository format: ${url.pathname}`) } return { owner: parts[0], name: parts[1], ref: parts[3] || 'HEAD', } } function validateFilename(filename) { if (filename.includes('..') || filename.startsWith('/')) { throw new Error(`Invalid filename path: ${filename}`) } return filename.split('/').map(encodeURIComponent).join('/') } async function readFileFromGitHub(repo, filename) { const { owner, name, ref } = parseGitHubRepo(repo) const safeFilename = validateFilename(filename) const url = `https://raw.githubusercontent.com/${owner}/${name}/${ref}/${safeFilename}` try { const response = await fetch(url) if (!response.ok) { throw new Error(`${response.status} ${response.statusText}`) } return await response.text() } catch (e) { console.error(`Error fetching ${url}: ${e.message}`) process.exit(1) } }• Key changes:
- Use
URLto enforcegithub.comhost.- Ensure the repo path has at least
owner/name.- Reject filenames containing
..or leading/.- Encode each path segment via
encodeURIComponent.⛔ Skipped due to learnings
Learnt from: mvadari PR: XRPLF/xrpl.js#2826 File: packages/xrpl/tools/generateModels.js:36-36 Timestamp: 2024-11-18T23:29:19.882Z Learning: In the file `packages/xrpl/tools/generateModels.js`, it's acceptable for the script to crash when the regex doesn't match as expected.Learnt from: ckeshava PR: XRPLF/xrpl.js#3032 File: packages/xrpl/src/models/transactions/common.ts:689-698 Timestamp: 2025-07-10T22:04:59.994Z Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.Learnt from: mvadari PR: XRPLF/xrpl.js#2895 File: packages/xrpl/test/models/clawback.test.ts:0-0 Timestamp: 2025-02-12T23:28:55.377Z Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.Learnt from: achowdhry-ripple PR: XRPLF/xrpl.js#2661 File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102 Timestamp: 2024-12-05T16:48:12.951Z Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.Learnt from: mvadari PR: XRPLF/xrpl.js#2690 File: packages/xrpl/tools/generateModels.js:52-52 Timestamp: 2024-10-02T15:47:02.491Z Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.Learnt from: mvadari PR: XRPLF/xrpl.js#2690 File: packages/xrpl/tools/generateModels.js:52-52 Timestamp: 2024-10-08T16:29:11.194Z Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.Learnt from: ckeshava PR: XRPLF/xrpl.js#2826 File: packages/xrpl/tools/generateModels.js:25-25 Timestamp: 2025-01-13T22:38:40.085Z Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
ckeshava
approved these changes
Aug 19, 2025
Patel-Raj11
approved these changes
Aug 19, 2025
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
This PR adds support for
generateDefinitions.jsandgenerateModels.jsto also pull from rippled on Github, removing the need to have a local clone.Context of Change
Ease of use for tooling
I wrote a similar script for ripple/craft#100, porting it over to xrpl.js wasn't too big a lift
Type of Change
Did you update HISTORY.md?
Test Plan
Works locally