dev: add definitions.json generation script #2826
Conversation
WalkthroughThe changes involve significant updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
|
@mvadari thank you! Any chance you can add a package json script? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/ripple-binary-codec/tools/generateDefinitions.js (3)
69-70: Improve error handling in thecatchblockIn the
translatefunction'scatchblock, the script logs the error but continues execution. This might lead to unexpected behavior if the translation fails. Consider exiting the process or rethrowing the error to prevent proceeding with invalid data.Apply this diff to handle the error appropriately:
} catch (e) { console.log(e, 'inp="' + inp + '"') + process.exit(1) }Alternatively, rethrow the error:
} catch (e) { console.log(e, 'inp="' + inp + '"') + throw e }
107-109: Specify radix parameter inparseIntWhen using
parseInt, omitting the radix parameter can lead to unexpected results, especially with inputs starting with0x. Explicitly specify the radix to ensure correct parsing of hexadecimal values.Apply this diff to specify the radix:
if (x.substr(0, 2) == '0x') return '' + parseInt(x) +if (x.substr(0, 2) == '0x') return '' + parseInt(x, 16)
20-21: Consider using asynchronous file operations withfs.promisesWhile synchronous file reads are acceptable in small scripts, using asynchronous operations can improve scalability and prevent blocking the event loop, especially if the script evolves to include more processing.
Apply this diff to use asynchronous file reading:
-const fs = require('fs') +const fs = require('fs').promises -const sfield_h = fs.readFileSync(sfield_h_fn).toString('utf-8') -// Repeat for other file reads... +async function readFiles() { + const sfield_h = (await fs.readFile(sfield_h_fn)).toString('utf-8') + // Repeat for other file reads... +} + +readFiles().then(() => { + // Proceed with processing +}).catch(err => { + console.error('Error reading files:', err.message) + process.exit(1) +})Note that you'll need to refactor the script to accommodate asynchronous execution.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/ripple-binary-codec/src/enums/definitions.json(1 hunks)packages/ripple-binary-codec/tools/generateDefinitions.js(1 hunks)
🔇 Additional comments (2)
packages/ripple-binary-codec/src/enums/definitions.json (1)
Line range hint 1-1000: Overall structure and formatting looks good
The file maintains a clear and consistent structure with well-defined sections. The JSON syntax is valid and the formatting is consistent throughout the file.
packages/ripple-binary-codec/tools/generateDefinitions.js (1)
241-245: Ensure comprehensive parsing of SField definitions
The regular expression used to match SField definitions may not account for all variations in formatting within sfields.macro. This could result in missing some fields during processing.
Run the following script to check for SField definitions that are not captured:
If the counts differ, consider adjusting the regex pattern to ensure all fields are processed.
✅ Verification successful
The script failed because the $sfield_macro_fn variable was undefined. Let's first locate the sfields.macro file and then verify the regex pattern.
Let's try to locate the file using ripgrep to search for TYPED_SFIELD definitions, as the file might have a different name or location.
Let's check the repository for any header files or source files that might contain the actual TYPED_SFIELD definitions, as they might be in C/C++ files or other source files.
From the output, we can see that the script is trying to process TYPED_SFIELD definitions from sfields.macro, but this file is expected to be provided as a command-line argument from the rippled repository (it's not part of this repository). The script is designed to work with external rippled source files, specifically from the path /include/xrpl/protocol/detail/sfields.macro.
The regex pattern is correct and well-designed for its purpose
The regex pattern in the code is actually well-designed to parse TYPED_SFIELD definitions from the rippled repository's sfields.macro file. This is evidenced by:
- The pattern matches the standard format used in rippled's SField definitions as referenced in the documentation links found in the README.md
- The regex accounts for all necessary components:
- Optional whitespace and type prefix (
^ *[A-Z]*TYPED_SFIELD) - Field name (
sf[^,\n]*) - Type (
[^, \n]+) - Numeric code (
[0-9]+) - Optional notSigning flag (
(,.*?(notSigning))?)
- Optional whitespace and type prefix (
The original review comment's concern about missing fields is incorrect, as the regex pattern is comprehensive and matches the expected format of TYPED_SFIELD definitions in the rippled codebase.
[incorrect_review_comment]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all SField definitions are captured by the regex.
# Expected result: All SField definitions are matched by the script.
# Count total SField definitions in sfields.macro
total_fields=$(grep -E '^[A-Z_]*TYPED_SFIELD' "$sfield_macro_fn" | wc -l)
# Extract fields matched by the script's regex
matched_fields=$(grep -E '^[A-Z_]*TYPED_SFIELD *\(\s*sf[^,]*,\s*[^, ]+,\s*[0-9]+(,.*?notSigning)?' "$sfield_macro_fn" | wc -l)
echo "Total SField definitions: $total_fields"
echo "Fields matched by script: $matched_fields"
Length of output: 376
Script:
#!/bin/bash
# Find the sfields.macro file
fd -t f "sfields.macro"
# Once found, let's examine its content to verify the TYPED_SFIELD definitions
echo "=== Content of sfields.macro ==="
fd -t f "sfields.macro" -x cat {}
Length of output: 134
Script:
#!/bin/bash
# Search for files containing TYPED_SFIELD definitions
echo "=== Files containing TYPED_SFIELD ==="
rg -l "TYPED_SFIELD"
# Show the actual TYPED_SFIELD definitions with context
echo -e "\n=== TYPED_SFIELD definitions ==="
rg "TYPED_SFIELD" -A 1
Length of output: 493
Script:
#!/bin/bash
# Search for files with common C/C++ extensions or header files
echo "=== Looking for potential source files ==="
fd -e h -e hpp -e c -e cpp
# Search for any file containing "SField" in its name or content
echo -e "\n=== Files containing 'SField' ==="
rg -l "SField"
# Show content of files containing "SField" with context
echo -e "\n=== SField definitions and usage ==="
rg "SField" -C 2
Length of output: 4996
| for (x in parts) | ||
| if (capitalization_exceptions[parts[x]] != null) { | ||
| result += capitalization_exceptions[parts[x]] | ||
| } else | ||
| result += | ||
| parts[x].substr(0, 1).toUpperCase() + parts[x].substr(1).toLowerCase() | ||
| return result |
There was a problem hiding this comment.
Declare loop variable and use for...of for array iteration
The loop variable x is not declared, which can lead to unexpected behavior due to variable hoisting. Additionally, using for...of provides clearer and more reliable iteration over arrays.
Apply this diff to fix the loop:
- for (x in parts)
+ for (const part of parts)
if (capitalization_exceptions[parts[x]] != null) {
- result += capitalization_exceptions[parts[x]]
+ result += capitalization_exceptions[part]
} else
result +=
- parts[x].substr(0, 1).toUpperCase() + parts[x].substr(1).toLowerCase()
+ part.substr(0, 1).toUpperCase() + part.substr(1).toLowerCase()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (x in parts) | |
| if (capitalization_exceptions[parts[x]] != null) { | |
| result += capitalization_exceptions[parts[x]] | |
| } else | |
| result += | |
| parts[x].substr(0, 1).toUpperCase() + parts[x].substr(1).toLowerCase() | |
| return result | |
| for (const part of parts) | |
| if (capitalization_exceptions[part] != null) { | |
| result += capitalization_exceptions[part] | |
| } else | |
| result += | |
| part.substr(0, 1).toUpperCase() + part.substr(1).toLowerCase() | |
| return result |
There was a problem hiding this comment.
This doesn't work - is it TS specific?
There was a problem hiding this comment.
@mvadari, thanks for bringing this up.
It seems the previous suggestion might not have worked due to missing curly braces or issues with the substr method. Here's an updated version that should work in both JavaScript and TypeScript:
for (const part of parts) {
if (capitalization_exceptions[part] != null) {
result += capitalization_exceptions[part]
} else {
result +=
part.substring(0, 1).toUpperCase() + part.substring(1).toLowerCase()
}
}
return resultIn this version:
-
Added curly braces
{}around the loop and conditional statements to ensure proper scoping. -
Replaced
substrwithsubstring, assubstris deprecated in some environments. Alternatively, you can useslice:result += part.slice(0, 1).toUpperCase() + part.slice(1).toLowerCase()
Please let me know if this resolves the issue!
Will do - also plan on making it so you don't need to pipe it into the |
mvadari
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/ripple-binary-codec/package.json (2)
24-25: Consider improving script usability.The script addition looks good, but consider these improvements:
- Specify the output path explicitly in the script to ensure consistent file location
- Add documentation in the README about when and how to use this script
- "generateDefinitions": "node ./tools/generateDefinitions.js" + "generateDefinitions": "node ./tools/generateDefinitions.js > ./src/enums/definitions.json"
24-25: Consider integrating with the build process.Since the definitions.json file is essential for the package, consider integrating the generation script into the build process to ensure it's always up-to-date.
"build": "tsc --build tsconfig.build.json && copyfiles ./src/enums/definitions.json ./dist/enums/", + "prebuild": "npm run generateDefinitions", "clean": "rm -rf ./dist ./coverage ./test/testCompiledForWeb tsconfig.build.tsbuildinfo",
|
Can we stick to using the I understand that it changes the formatting of the file. But its still better to consume an existing RPC instead of maintaining a script. Every time there is a refactor of rippled, somebody needs to re-write this script. |
|
|
okay, I forget that we need to build the rippled branch. |
There was a problem hiding this comment.
I tried to execute this script as :
➜ xrpl.js git:(definitions-generation) node packages/xrpl/tools/generateModels.js ~/rippled/
/Users/ckeshavabs/xrpl.js/packages/xrpl/tools/generateModels.js:24
for (const hit of sfieldHits) {
^
TypeError: sfieldHits is not iterable
at processRippledSource (/Users/ckeshavabs/xrpl.js/packages/xrpl/tools/generateModels.js:24:21)
at main (/Users/ckeshavabs/xrpl.js/packages/xrpl/tools/generateModels.js:178:5)
at Object.<anonymous> (/Users/ckeshavabs/xrpl.js/packages/xrpl/tools/generateModels.js:271:3)
at Module._compile (node:internal/modules/cjs/loader:1546:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
at Module.load (node:internal/modules/cjs/loader:1317:32)
at Module._load (node:internal/modules/cjs/loader:1127:12)
at TracingChannel.traceSync (node:diagnostics_channel:315:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:166:5)
Node.js v22.9.0
I'm certain that the path/to/rippled is correct (I verified that with a stat command)
Hmm, |
Is the version of rippled at that location post-refactor? |
I'm at the tip of 0ec17b6026298dc150099b66a2cecad0fe561d1b on the I think both of them are post-refactor of rippled |
Both work fine for me: |
|
Ahh, I haven't updated the |
|
@ckeshava the model generation script should work now too. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/tools/generateModels.js (2)
20-21: Add comments to clarify complex regular expressionsThe regular expression used to match sfields is quite complex and may be difficult for maintainers to understand. Adding comments explaining the purpose and structure of the regex would improve readability and maintainability.
31-32: Consider documenting complex regular expressionsThe regular expression used for matching transaction formats is complex and may not be immediately clear to future developers. Adding comments or documentation explaining the regex pattern would enhance maintainability and ease future modifications.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/ripple-binary-codec/tools/generateDefinitions.js(1 hunks)packages/xrpl/tools/generateModels.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ripple-binary-codec/tools/generateDefinitions.js
🔇 Additional comments (1)
packages/xrpl/tools/generateModels.js (1)
28-29: Consistent use of 'folder' parameter
Great job using the folder parameter here to construct the file path. This ensures consistency and enhances the function's flexibility.
| /^ *CONSTRUCT_[^\_]+_SFIELD *\( *[^,\n]*,[ \n]*"([^\"\n ]+)"[ \n]*,[ \n]*([^, \n]+)[ \n]*,[ \n]*([0-9]+)(,.*?(notSigning))?/, | ||
| ) | ||
| sfields[matches[1]] = matches.slice(2) | ||
| sfields[hit[1]] = hit[2] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust error handling for regex matches
When processing matches from sfieldHits, there's a possibility that hit[1] or hit[2] could be undefined if the regex doesn't match as expected. This could lead to runtime errors when assigning to sfields. Consider adding checks to verify that these capture groups are defined before using them.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/tools/generateModels.js (2)
28-36: Document the complex regex pattern.The regex pattern for transaction matching is complex and captures multiple components. Consider adding a comment explaining each capture group to improve maintainability:
- Transaction type
- Parameters
- Optional/Required flags
- MPT support flags
Add this documentation above the regex:
+// Regex breakdown: +// 1. TRANSACTION(tt[A-Z_]+ *,* [0-9]+ *,) - Matches transaction declaration +// 2. ([A-Za-z]+) - Captures transaction name +// 3. ({sf[A-Za-z0-9]+, soe(OPTIONAL|REQUIRED|DEFAULT)) - Captures field declarations +// 4. (soeMPT(None|Supported|NotSupported))? - Optional MPT support flag const txFormatsHits = transactionsMacroFile.matchAll( /^ *TRANSACTION\(tt[A-Z_]+ *,* [0-9]+ *, *([A-Za-z]+)[ \n]*,[ \n]*\({[ \n]*(({sf[A-Za-z0-9]+, soe(OPTIONAL|REQUIRED|DEFAULT)(, soeMPT(None|Supported|NotSupported))?},[ \n]+)*)}\)\)$/gm, )
Line range hint
251-251: Address the TODO comment.The TODO indicates missing docstrings for models and integration tests. These would be valuable additions to ensure proper documentation and testing coverage.
Would you like me to help generate:
- Docstring templates for the models
- Integration test templates
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/xrpl/tools/generateModels.js(2 hunks)
🔇 Additional comments (3)
packages/xrpl/tools/generateModels.js (3)
24-25: LGTM! Field processing looks good.
The simplified field processing logic is clear and concise.
165-165: LGTM! Good use of nullish coalescing.
The addition of the nullish coalescing operator with 'any' as a fallback is a good defensive programming practice.
17-21: LGTM! Verify macro file paths and patterns.
The switch to reading from macro files and the updated regex pattern for field matching look good. The pattern correctly captures field names, types, and optional attributes.
✅ Verification successful
The macro files are external dependencies and not part of this repository
The code is trying to read macro files from an external rippled source code that should be provided as a command-line argument. This is evident from:
- The macro files are not present in this repository
- The path
folderinpath.join(folder, '/include/xrpl/protocol/detail/sfields.macro')is meant to be provided externally - Similar pattern is used in both
generateModels.jsandgenerateDefinitions.jstools
The code changes and regex patterns are correct, but they depend on external rippled source code that needs to be provided during execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of macro files
# Check if the macro files exist in the expected location
fd -e macro -p "sfields.macro|transactions.macro" "include/xrpl/protocol/detail"
# Verify the structure of sfields.macro entries
rg -A 1 "TYPED_SFIELD" "include/xrpl/protocol/detail/sfields.macro"
Length of output: 345
Script:
#!/bin/bash
# Let's first find the actual location of these macro files in the repository
fd -t f -e macro
# If no macro files found, let's check the repository structure
fd . -t d -d 2
# Let's also check if there are any references to these macro files in the codebase
rg -i "sfields\.macro|transactions\.macro"
Length of output: 828
| ), | ||
| ) | ||
|
|
||
| const capitalizationExceptions = { |
There was a problem hiding this comment.
I don't understand the need for capitalizationExceptions. All the ledger-entry-types are defined with capitalized alphabets in the ledger_entries.macro file. Why are we singling out these specific objects?
There was a problem hiding this comment.
It is used for STypes, which do not have a string name.
There was a problem hiding this comment.
I don't understand what you mean by STypes -- do you remember the name of the struct inside rippled? Do you have any examples in mind?
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't understand how capitalization exceptions are relevant to STypes. None of the below elements are rippled internal types, they are ledger-entry-types.
| /^ *STYPE\(STI_([^ ]*?) *, *([0-9-]+) *\) *\\?$/gm, | ||
| ), | ||
| ] | ||
| if (stypeHits.length === 0) |
There was a problem hiding this comment.
Why is this if condition required? stypeHits will always have a non-zero number of matches.
There was a problem hiding this comment.
I think that was for better error handling.
| const txFormatsHits = txFormatsCpp.match( | ||
| /^ *add\(jss::([^\"\n, ]+),[ \n]*tt[A-Z_]+,[ \n]*{[ \n]*(({sf[A-Za-z0-9]+, soe(OPTIONAL|REQUIRED|DEFAULT)},[ \n]+)*)},[ \n]*[pseudocC]+ommonFields\);/gm, | ||
| const txFormatsHits = transactionsMacroFile.matchAll( | ||
| /^ *TRANSACTION\(tt[A-Z_]+ *,* [0-9]+ *, *([A-Za-z]+)[ \n]*,[ \n]*\({[ \n]*(({sf[A-Za-z0-9]+, soe(OPTIONAL|REQUIRED|DEFAULT)(, soeMPT(None|Supported|NotSupported))?},[ \n]+)*)}\)\)$/gm, | ||
| ) | ||
| const txFormats = {} |
There was a problem hiding this comment.
Question about line 53: Why aren't these three transactions part of the SubmittableTransaction type?
existingLibraryTxs.push('EnableAmendment', 'SetFee', 'UNLModify')
There was a problem hiding this comment.
Pseudo-transactions aren't submittable.
| @@ -14,32 +14,26 @@ function readFile(filename) { | |||
| let jsTransactionFile | |||
There was a problem hiding this comment.
Can you add instructions for using this file inside CONTRIBUTING.md ?
| @@ -21,7 +21,8 @@ | |||
| "prepublishOnly": "npm test", | |||
| "test": "npm run build && jest --verbose false --silent=false ./test/*.test.ts", | |||
| "test:browser": "npm run build && karma start ./karma.config.js", | |||
| "lint": "eslint . --ext .ts --ext .test.js" | |||
| "lint": "eslint . --ext .ts --ext .test.js", | |||
| "generateDefinitions": "node ./tools/generateDefinitions.js" | |||
There was a problem hiding this comment.
When I tried to run this npm script, I got this error message. How should I run it?
➜ xrpl git:(definitions-generation) npm run generateDefinitions ~/rippled
npm error Lifecycle script `generateDefinitions` failed with error:
npm error workspace xrpl@4.0.0
npm error location /Users/ckeshavabs/xrpl.js/packages/xrpl
npm error Missing script: "generateDefinitions"
npm error
npm error To see a list of scripts, run:
npm error npm run --workspace=xrpl@4.0.0
However, explicitly mentioning the script works as intended: node ./tools/generateDefinitions.js ~/rippled
| XCHAIN: 'XChain', | ||
| DID: 'DID', | ||
| ID: 'ID', | ||
| AMM: 'AMM', |
There was a problem hiding this comment.
This dictionary can be stripped down to the following elements:
const capitalizationExceptions = {
NFTOKEN: 'NFToken',
URITOKEN: 'URIToken',
XCHAIN: 'XChain',
}
If you prefer, feel free to cherry-pick this commit 47a1e62b5ee2cfd2ed17d23dd7cb8adcbddf21b7. I have used the latest tip of develop branch from rippled.
| ), | ||
| ) | ||
|
|
||
| const capitalizationExceptions = { |
There was a problem hiding this comment.
I don't understand how capitalization exceptions are relevant to STypes. None of the below elements are rippled internal types, they are ledger-entry-types.
High Level Overview of Change
This PR adds a script to generate the
definitions.jsonfile from rippled source code.Context of Change
Copied (and modified) from https://github.com/RichardAH/xrpl-codec-gen. It makes more sense to store this script in the library repo now.
Type of Change
Did you update HISTORY.md?
Test Plan
Works locally.