dev: add definitions.json generation script #772
Conversation
This comment was marked as spam.
This comment was marked as spam.
![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 (5)
tools/generate_definitions.py (4)
6-15: Add docstring to explain CAPITALIZATION_EXCEPTIONS dictionaryThe dictionary's purpose and when/how exceptions are applied should be documented for maintainability.
Add a docstring explaining the purpose:
CAPITALIZATION_EXCEPTIONS = { + """Mapping of special case strings to their proper capitalization in the XRPL. + + These exceptions override the default word capitalization rules when processing + field names and types from the rippled source code. + """ "NFTOKEN": "NFToken",
17-19: Enhance error handling with descriptive messageThe error message could be more informative about what the rippled path should contain.
if len(sys.argv) != 2: - print("Usage: python " + sys.argv[0] + " path/to/rippled") + print(f"Usage: {sys.argv[0]} PATH_TO_RIPPLED\n" + f"PATH_TO_RIPPLED should point to the root of the rippled source code containing the 'include' directory") sys.exit(1)
87-93: Document and simplify complex regex patternsThe regex patterns are complex and would benefit from documentation and named groups.
+# Pattern to match STYPE definitions in two possible formats: +# 1. STYPE(STI_NAME, NUMBER) +# 2. STI_NAME = NUMBER +TYPE_PATTERN = r""" + ^[ ]* # Start of line with optional spaces + (?:STYPE\(STI_ # First format: STYPE(STI_ + (?P<name1>[^ ]*?) # Capture name + [ ]*,[ ]* # Comma separator + (?P<num1>[0-9-]+) # Capture number + [ ]*\) # Closing parenthesis + | # OR + STI_ # Second format: STI_ + (?P<name2>[^ ]*?) # Capture name + [ ]*=[ ]* # Equals sign + (?P<num2>[0-9-]+) # Capture number + ) + [ ]*,?[ ]*$ # Optional comma and end of line +""" + type_hits = re.findall( - r"^ *STYPE\(STI_([^ ]*?) *, *([0-9-]+) *\) *\\?$", sfield_h, re.MULTILINE + TYPE_PATTERN, sfield_h, re.MULTILINE | re.VERBOSE )
1-326: Add unit tests for the scriptThe script performs critical data processing but lacks tests to verify its correctness.
Would you like me to help create a test suite for this script? The tests would cover:
- File reading and error handling
- String translation logic
- Regex pattern matching
- Data validation
- JSON output formatting
xrpl/core/binarycodec/definitions/definitions.json (1)
Line range hint
1-1068: Well-structured definitions file with clear organizationThe file maintains a clean and consistent structure with:
- Clear separation of concerns between different type definitions
- Consistent formatting and indentation
- Valid JSON syntax
- Logical grouping of related entries
This organization makes the file easy to maintain and extend.
Consider adding a schema file to formally validate the structure and prevent accidental malformation during future updates.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
tools/generate_definitions.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(1 hunks)
🔇 Additional comments (1)
xrpl/core/binarycodec/definitions/definitions.json (1)
33-57: LGTM: LEDGER_ENTRY_TYPES changes are well-structured
The new ledger entry types are properly organized with:
- Consistent numerical ordering of type IDs
- No duplicate IDs
- Clear semantic naming that reflects their purpose
tools/generate_definitions.py
Outdated
| def _read_file(filename: str) -> str: | ||
| with open(filename, "r") as f: | ||
| return f.read() | ||
|
|
There was a problem hiding this comment.
Add error handling for file operations
The file reading function should handle potential IOErrors and provide meaningful error messages.
def _read_file(filename: str) -> str:
+ """Read and return the contents of a file.
+
+ Args:
+ filename: Path to the file to read
+
+ Returns:
+ The contents of the file as a string
+
+ Raises:
+ SystemExit: If the file cannot be read
+ """
try:
with open(filename, "r") as f:
return f.read()
+ except IOError as e:
+ print(f"Error reading {filename}: {e}", file=sys.stderr)
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
tools/generate_definitions.py
Outdated
| sfield_h_fn = sys.argv[1] + "/include/xrpl/protocol/SField.h" | ||
| sfield_macro_fn = sys.argv[1] + "/include/xrpl/protocol/detail/sfields.macro" | ||
| ledger_entries_macro_fn = ( | ||
| sys.argv[1] + "/include/xrpl/protocol/detail/ledger_entries.macro" | ||
| ) | ||
| ter_h_fn = sys.argv[1] + "/include/xrpl/protocol/TER.h" | ||
| transactions_macro_fn = sys.argv[1] + "/include/xrpl/protocol/detail/transactions.macro" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pathlib for robust path handling
Using string concatenation for paths is error-prone. Consider using pathlib for more robust path handling.
+from pathlib import Path
+
+rippled_path = Path(sys.argv[1])
+include_path = rippled_path / "include" / "xrpl" / "protocol"
+
-sfield_h_fn = sys.argv[1] + "/include/xrpl/protocol/SField.h"
-sfield_macro_fn = sys.argv[1] + "/include/xrpl/protocol/detail/sfields.macro"
+sfield_h_fn = include_path / "SField.h"
+sfield_macro_fn = include_path / "detail" / "sfields.macro"Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I also like this suggestion. It could make developing on non-Unix systems less cumbersome.
There was a problem hiding this comment.
@ckeshava Thanks for confirming! Yes, pathlib is indeed the more robust solution for cross-platform path handling.
(｀°ω°)⊃━☆ﾟ.*･｡ﾟ
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:07:34.153Z
Learning: Use `pathlib` instead of string concatenation for path handling in Python scripts to ensure better cross-platform compatibility, especially for development on non-Unix systems.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
See the most recent version of the code - this was handled.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
xrpl/core/binarycodec/definitions/definitions.json (4)
914-922: Consistency Check: "IssuerNode" Field Update
The tilde markers on the"isVLEncoded"and"nth"properties for"IssuerNode"indicate an update. Please confirm that these values (setting"isVLEncoded": falseand"nth": 27) are consistent with similar fields in this array, ensuring a uniform approach to field serialization.
924-932: Consistency Check: "SubjectNode" Field Update
The updated"SubjectNode"block now also explicitly sets"isVLEncoded": falseand"nth": 28". Verify that this change mirrors the"IssuerNode"settings and that the ordering is intentional and consistent across the definitions.
2543-2552: Formatting Update: "UnauthorizeCredentials" Block
Minor formatting adjustments (as seen by the tilde markers) have been applied to the"UnauthorizeCredentials"entry. Please review these changes to ensure they do not alter the intended behavior of this field.
2744-2752: Update: "CredentialIDs" Block
The"CredentialIDs"block shows minor updates (with tilde markers adjusting"isVLEncoded": trueand the"nth"value) to ensure consistency with similar vector types. Please confirm that these changes correctly reflect the new encoding requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
xrpl/core/binarycodec/definitions/definitions.json(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (9)
xrpl/core/binarycodec/definitions/definitions.json (9)
1253-1262: New Field Addition: "DomainID" in FIELDS
A new field named"DomainID"with type"Hash256"is added. Please double-check that the assigned"nth": 34does not conflict with any existing field ordering and that its type is appropriate for representing a domain identifier per protocol specifications.
2892-2897: Ledger Entry Type Update: "PermissionedDomain"
The new ledger entry type"PermissionedDomain"is added with the numeric code130. Ensure that this code is unique within the"LEDGER_ENTRY_TYPES"mapping and that its integration aligns with the overall protocol definitions.
2923-2925: New Transaction Result: "tecHOOK_REJECTED"
A new transaction result"tecHOOK_REJECTED"with value153has been introduced. Confirm that this result code does not conflict with existing codes and that its semantics are properly documented in the protocol references.
3092-3099: New Transaction Type: "AMMClawback"
The transaction type"AMMClawback"with code31is now part of the"TRANSACTION_TYPES". Please ensure that its addition is consistent with other AMM-related types and that the chosen numeric value is unique and appropriate per protocol guidelines.
3125-3127: New Transaction Type: "NFTokenModify"
The"NFTokenModify"transaction type (code61) is newly added. Verify that its code does not interfere with other NFToken transaction codes and that its behavior is clearly defined in downstream processing.
3135-3137: New Domain-Related Transaction Types: "PermissionedDomainDelete" and "PermissionedDomainSet"
The new entries"PermissionedDomainDelete"(code63) and"PermissionedDomainSet"(code62) have been introduced. Please check that these types are consistently implemented and that any components relying on domain transactions are updated accordingly.
3165-3170: New Type Definition: "Number" in TYPES
A new type"Number"has been added with the mapping9in the"TYPES"section. Confirm that the semantic meaning of"Number"is clearly defined and that its numeric mapping aligns with its usage—particularly with the newly introduced"Number"field in FIELDS.
2553-2562: New Addition: "AcceptedCredentials" Block
The"AcceptedCredentials"block is newly added with"nth": 28. Verify that this addition is intentional, that it follows the established structure for credentials-related fields, and that its ordering is compatible with existing field definitions.
2033-2042:⚠️ Potential issueNew Field Addition: "Number" in FIELDS
The"Number"field is introduced with type"Number"and"nth": 1. Since there are existing entries (like"ObjectEndMarker") already using an"nth"value of 1, please verify that this value is correctly assigned and does not lead to a collision during serialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tools/generate_definitions.py (2)
63-93: Improve the translation function with a lookup table.The
_translatefunction has many if-else conditions that could be simplified using a lookup table.Apply this diff to improve the function:
+# Define translation mapping for direct conversions +DIRECT_TRANSLATIONS = { + "OBJECT": "STObject", + "ARRAY": "STArray", + "ACCOUNT": "AccountID", + "LEDGERENTRY": "LedgerEntry", + "NOTPRESENT": "NotPresent", + "PATHSET": "PathSet", + "VL": "Blob", + "DIR_NODE": "DirectoryNode", + "PAYCHAN": "PayChannel", +} + def _translate(inp: str) -> str: + """Translate rippled type names to binary codec format. + + Args: + inp: Input string in rippled format (e.g., 'UINT256', 'ACCOUNT') + + Returns: + Translated string in binary codec format (e.g., 'Hash256', 'AccountID') + """ + # Handle direct translations first + if inp in DIRECT_TRANSLATIONS: + return DIRECT_TRANSLATIONS[inp] + if re.match(r"^UINT", inp): if re.search(r"256|160|128|192", inp): return inp.replace("UINT", "Hash") else: return inp.replace("UINT", "UInt") - if inp == "OBJECT" or inp == "ARRAY": - return "ST" + inp[0:1].upper() + inp[1:].lower() - if inp == "ACCOUNT": - return "AccountID" - if inp == "LEDGERENTRY": - return "LedgerEntry" - if inp == "NOTPRESENT": - return "NotPresent" - if inp == "PATHSET": - return "PathSet" - if inp == "VL": - return "Blob" - if inp == "DIR_NODE": - return "DirectoryNode" - if inp == "PAYCHAN": - return "PayChannel" parts = inp.split("_") result = "" for part in parts: if part in CAPITALIZATION_EXCEPTIONS: result += CAPITALIZATION_EXCEPTIONS[part] else: result += part[0:1].upper() + part[1:].lower() return result
186-206: Add validation for field properties.The helper functions for field properties should validate their inputs.
Apply this diff to add validation:
def _is_vl_encoded(t: str) -> str: + """Check if a type is variable-length encoded. + + Args: + t: Type string to check + + Returns: + "true" if the type is variable-length encoded, "false" otherwise + """ + if not isinstance(t, str): + raise TypeError("Type must be a string") if t == "VL" or t == "ACCOUNT" or t == "VECTOR256": return "true" return "false" def _is_serialized(t: str, name: str) -> str: + """Check if a field is serialized. + + Args: + t: Type string to check + name: Field name to check + + Returns: + "true" if the field is serialized, "false" otherwise + """ + if not isinstance(t, str) or not isinstance(name, str): + raise TypeError("Type and name must be strings") if t == "LEDGERENTRY" or t == "TRANSACTION" or t == "VALIDATION" or t == "METADATA": return "false" if name == "hash" or name == "index": return "false" return "true" def _is_signing_field(t: str, not_signing_field: str) -> str: + """Check if a field is a signing field. + + Args: + t: Type string to check + not_signing_field: String indicating if the field is not for signing + + Returns: + "true" if the field is a signing field, "false" otherwise + """ + if not isinstance(t, str) or not isinstance(not_signing_field, str): + raise TypeError("Type and not_signing_field must be strings") if not_signing_field == "notSigning": return "false" if t == "LEDGERENTRY" or t == "TRANSACTION" or t == "VALIDATION" or t == "METADATA": return "false" return "true"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyproject.toml(1 hunks)tools/generate_definitions.py(1 hunks)tools/generate_tx_models.py(4 hunks)
🔇 Additional comments (4)
tools/generate_tx_models.py (1)
74-74: LGTM!The addition of
"UINT192": "str"toTYPE_MAPis correct and consistent with the existing type mappings.tools/generate_definitions.py (2)
34-41: Add error handling for HTTP requests.The
_read_file_from_githubfunction should handle potential HTTP errors and provide meaningful error messages.Apply this diff to add error handling:
def _read_file_from_github(repo: str, filename: str) -> str: + """Read and return the contents of a file from GitHub. + + Args: + repo: GitHub repository URL + filename: Path to the file within the repository + + Returns: + The contents of the file as a string + + Raises: + SystemExit: If the file cannot be read from GitHub + """ url = repo.replace("github.com", "raw.githubusercontent.com") url = url.replace("tree", "refs/heads") url += filename if not url.startswith("http"): url = "https://" + url - response = httpx.get(url) - return response.text + try: + response = httpx.get(url) + response.raise_for_status() + return response.text + except httpx.HTTPError as e: + print(f"Error reading {url}: {e}", file=sys.stderr) + sys.exit(1)
44-46: Add error handling for file operations.The
_read_filefunction should handle potential IOErrors and provide meaningful error messages.Apply this diff to add error handling:
def _read_file(folder: str, filename: str) -> str: - with open(folder + filename, "r") as f: - return f.read() + """Read and return the contents of a file. + + Args: + folder: Path to the folder containing the file + filename: Name of the file to read + + Returns: + The contents of the file as a string + + Raises: + SystemExit: If the file cannot be read + """ + try: + with open(folder + filename, "r") as f: + return f.read() + except IOError as e: + print(f"Error reading {folder + filename}: {e}", file=sys.stderr) + sys.exit(1)pyproject.toml (1)
86-92: LGTM!The new
generatetask is well-structured and correctly configured to run both scripts in sequence. The help text and argument configuration are clear and appropriate.
|
@mvadari Can you add an example usage of this new command into Should it be |
|
If I specify a github repository in its default branch, I get a cryptic error message: (I inserted the DEBUG print statement) I observed this issue with Is it possible to parse the default branches of a fork too? |
Added |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CONTRIBUTING.md (2)
184-189: Add error handling guidance and clarify branch usage.Based on the reported issues in the PR:
- Document the known issue with default branch usage and provide a workaround.
- Consider adding examples for common use cases (e.g., using develop branch, local path).
Apply this diff to enhance the documentation:
To update just the `definitions.json` file: ```bash poetry run poe definitions https://github.com/XRPLF/rippled/tree/developAny Github branch link or local path to rippled will work here.
+
+Note: When using a GitHub repository link:
+- Specify the branch explicitly (e.g.,.../tree/develop) for best results
+- If you encounter aKeyError: 'UINT256'error, try using the develop branch instead of the default branch
+
+Examples:
+bash +# Using local rippled repository +poetry run poe definitions /path/to/local/rippled + +# Using specific GitHub branch +poetry run poe definitions https://github.com/XRPLF/rippled/tree/develop +<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 185-185: Code block style Expected: indented; Actual: fenced (MD046, code-block-style) </details> </details> --- `191-194`: **Add context about model updates.** Consider providing more information about what gets updated when running the generate command, as this is a new feature. Apply this diff to add context: ```diff To update the models as well: + +This command will: +1. Update the `definitions.json` file +2. Generate new model files based on the updated definitions + ```bash poetry run poe generate https://github.com/XRPLF/rippled/tree/develop
+The generated models will reflect any new or updated transaction types, ledger entry types, and fields from the rippled source.
<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 192-192: Code block style Expected: indented; Actual: fenced (MD046, code-block-style) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e6c174237fb61a2e1d264ae1bf53ca30e5d781cc and 201820b0e792ab606f84204b700eb47555698091. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `CONTRIBUTING.md` (1 hunks) * `pyproject.toml` (1 hunks) * `tools/generate_definitions.py` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * tools/generate_definitions.py * pyproject.toml </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>CONTRIBUTING.md</summary> 185-185: Code block style Expected: indented; Actual: fenced (MD046, code-block-style) --- 192-192: Code block style Expected: indented; Actual: fenced (MD046, code-block-style) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (12)</summary> * GitHub Check: Snippet test (3.13) * GitHub Check: Integration test (3.13) * GitHub Check: Integration test (3.12) * GitHub Check: Snippet test (3.12) * GitHub Check: Integration test (3.11) * GitHub Check: Snippet test (3.11) * GitHub Check: Snippet test (3.10) * GitHub Check: Integration test (3.10) * GitHub Check: Integration test (3.9) * GitHub Check: Snippet test (3.9) * GitHub Check: Integration test (3.8) * GitHub Check: Snippet test (3.8) </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>CONTRIBUTING.md (1)</summary> `182-182`: **LGTM!** The updated section header accurately reflects the expanded scope of the section to include both `definitions.json` and model updates. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Added. |
tools/generate_definitions.py
Outdated
| # Example line: | ||
| # LEDGER_ENTRY(ltNFTOKEN_OFFER, 0x0037, NFTokenOffer, nft_offer, ({ | ||
| lt_hits = re.findall( | ||
| r"^ *LEDGER_ENTRY[A-Z_]*\(lt[A-Z_]+ *, *([x0-9a-f]+) *, *([^,]+), *([^,]+), \({$", |
There was a problem hiding this comment.
May I also suggest another update? Since we are not making use of the match groups after indices 0, 1, we could use a simpler regex:
| r"^ *LEDGER_ENTRY[A-Z_]*\(lt[A-Z_]+ *, *([x0-9a-f]+) *, *([^,]+), *([^,]+), \({$", | |
| r"^ *LEDGER_ENTRY[A-Z_]*\(lt[A-Z_]+ *, *([x0-9a-f]+) *, *([^,]+),.*$", |
There was a problem hiding this comment.
It may not be used now, but it could be useful later - e.g. to add support for the account_objects/ledger_entry filters. I don't see any reason to remove it.
| sfield_hits = re.findall( | ||
| r'^ *CONSTRUCT_[^\_]+_SFIELD *\( *[^,\n]*,[ \n]*"([^\"\n ]+)"[ \n]*,[ \n]*' | ||
| + r"([^, \n]+)[ \n]*,[ \n]*([0-9]+)(,.*?(notSigning))?", | ||
| ( |
There was a problem hiding this comment.
| ( | |
| # Typical structure of Sfield declaractions: | |
| # TYPED_SFIELD(sfTxnSignature, VL, 4, SField::sMD_Default, SField::notSigning) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
High Level Overview of Change
This PR adds a script to generate the
definitions.jsonfile from rippled source code or from a rippled Github tree branch.It also updates the model generation scripts to support a Github tree branch, and adds a
poescript to generate all of this in one go.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.