[Fix #354] Introduce decode_ledger_data utility method #785
Conversation
WalkthroughThis pull request introduces updates to the XRPL Python library, including the addition of the Changes
Possibly Related PRs
Suggested Reviewers
Poem
📜 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json (2)
Line range hint
2315-2882: Consider adding more edge cases for AMM operations.While the AMM operation test cases cover the basic functionality, consider adding edge cases like:
- Maximum/minimum values for trading fees
- Edge cases for deposit/withdraw amounts
- Error cases with invalid asset combinations
Line range hint
1-10: Add documentation for test data organization.Consider adding a header comment explaining:
- The structure and organization of the test data
- Purpose of each section (accountState, transactions, ledgerData)
- How to add new test cases
Example:
+ /* + * Binary codec test fixtures containing: + * 1. accountState - Ledger entry test data + * 2. transactions - Transaction test data + * 3. ledgerData - Ledger header test data + * + * Each section contains both binary and JSON representations + * for testing serialization/deserialization. + */ { "accountState": [
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json(2 hunks)tests/unit/core/binarycodec/fixtures/data_driven_fixtures.py(1 hunks)tests/unit/core/binarycodec/test_field_id_codec.py(2 hunks)xrpl/core/binarycodec/field_id_codec.py(1 hunks)
🔇 Additional comments (3)
xrpl/core/binarycodec/field_id_codec.py (1)
6-13: LGTM: Imports are properly organized and typed.
The new imports are correctly organized and include necessary types for the implementation.
CHANGELOG.md (1)
Line range hint 11-24: LGTM! The changelog structure follows best practices.
The changes are well-organized under appropriate categories (Added, BREAKING CHANGE, Fixed) and follow the Keep a Changelog format.
🧰 Tools
🪛 Markdownlint (0.37.0)
15-15: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json (1)
Line range hint 1-4882: LGTM! Comprehensive test data coverage for binary codec functionality.
The test fixtures provide excellent coverage across different ledger entry types, transaction types, and ledger headers. The data is well structured with both binary and JSON representations.
| def decode_ledger_header(serialized_str: str) -> Dict[str, Any]: | ||
| """ | ||
| Decodes a serialized ledger header. | ||
| Note: The file located at xrpl/core/binarycodec/definitions/definitions.json file | ||
| is used to parse the serialized data. If developers need custom definitions, | ||
| please update that file. | ||
|
|
||
| Args: | ||
| serialized_str: A serialized ledger header, represented as a hexa-decimal string | ||
|
|
||
| Returns: | ||
| A Dict object describing a ledger header | ||
| """ | ||
| parser = BinaryParser(serialized_str) | ||
|
|
||
| return { | ||
| "ledger_index": parser.read_uint32(), | ||
| # Uint64 types are represented as hex-strings for preserving precision | ||
| # For ease of use, explicitly type-cast this value into base-10 | ||
| "total_coins": str(int(parser.read_type(UInt64).to_hex(), base=16)), | ||
| "parent_hash": parser.read_type(Hash256).to_hex(), | ||
| "transaction_hash": parser.read_type(Hash256).to_hex(), | ||
| "account_hash": parser.read_type(Hash256).to_hex(), | ||
| "parent_close_time": parser.read_uint32(), | ||
| "close_time": parser.read_uint32(), | ||
| "close_time_resolution": parser.read_uint8(), | ||
| "close_flags": parser.read_uint8(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for malformed input.
The implementation looks solid and handles uint64 precision correctly. However, it should handle potential errors from malformed input.
Consider wrapping the parsing in a try-except block:
def decode_ledger_header(serialized_str: str) -> Dict[str, Any]:
"""
Decodes a serialized ledger header.
Note: The file located at xrpl/core/binarycodec/definitions/definitions.json file
is used to parse the serialized data. If developers need custom definitions,
please update that file.
Args:
serialized_str: A serialized ledger header, represented as a hexa-decimal string
Returns:
A Dict object describing a ledger header
+
+ Raises:
+ XRPLBinaryCodecException: If the input string is malformed or incomplete
"""
+ if not isinstance(serialized_str, str):
+ raise XRPLBinaryCodecException("serialized_str must be a string")
+
parser = BinaryParser(serialized_str)
+ try:
return {
"ledger_index": parser.read_uint32(),
# Uint64 types are represented as hex-strings for preserving precision
# For ease of use, explicitly type-cast this value into base-10
"total_coins": str(int(parser.read_type(UInt64).to_hex(), base=16)),
"parent_hash": parser.read_type(Hash256).to_hex(),
"transaction_hash": parser.read_type(Hash256).to_hex(),
"account_hash": parser.read_type(Hash256).to_hex(),
"parent_close_time": parser.read_uint32(),
"close_time": parser.read_uint32(),
"close_time_resolution": parser.read_uint8(),
"close_flags": parser.read_uint8(),
}
+ except Exception as e:
+ raise XRPLBinaryCodecException(f"Failed to decode ledger header: {str(e)}")📝 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.
| def decode_ledger_header(serialized_str: str) -> Dict[str, Any]: | |
| """ | |
| Decodes a serialized ledger header. | |
| Note: The file located at xrpl/core/binarycodec/definitions/definitions.json file | |
| is used to parse the serialized data. If developers need custom definitions, | |
| please update that file. | |
| Args: | |
| serialized_str: A serialized ledger header, represented as a hexa-decimal string | |
| Returns: | |
| A Dict object describing a ledger header | |
| """ | |
| parser = BinaryParser(serialized_str) | |
| return { | |
| "ledger_index": parser.read_uint32(), | |
| # Uint64 types are represented as hex-strings for preserving precision | |
| # For ease of use, explicitly type-cast this value into base-10 | |
| "total_coins": str(int(parser.read_type(UInt64).to_hex(), base=16)), | |
| "parent_hash": parser.read_type(Hash256).to_hex(), | |
| "transaction_hash": parser.read_type(Hash256).to_hex(), | |
| "account_hash": parser.read_type(Hash256).to_hex(), | |
| "parent_close_time": parser.read_uint32(), | |
| "close_time": parser.read_uint32(), | |
| "close_time_resolution": parser.read_uint8(), | |
| "close_flags": parser.read_uint8(), | |
| } | |
| def decode_ledger_header(serialized_str: str) -> Dict[str, Any]: | |
| """ | |
| Decodes a serialized ledger header. | |
| Note: The file located at xrpl/core/binarycodec/definitions/definitions.json file | |
| is used to parse the serialized data. If developers need custom definitions, | |
| please update that file. | |
| Args: | |
| serialized_str: A serialized ledger header, represented as a hexa-decimal string | |
| Returns: | |
| A Dict object describing a ledger header | |
| Raises: | |
| XRPLBinaryCodecException: If the input string is malformed or incomplete | |
| """ | |
| if not isinstance(serialized_str, str): | |
| raise XRPLBinaryCodecException("serialized_str must be a string") | |
| parser = BinaryParser(serialized_str) | |
| try: | |
| return { | |
| "ledger_index": parser.read_uint32(), | |
| # Uint64 types are represented as hex-strings for preserving precision | |
| # For ease of use, explicitly type-cast this value into base-10 | |
| "total_coins": str(int(parser.read_type(UInt64).to_hex(), base=16)), | |
| "parent_hash": parser.read_type(Hash256).to_hex(), | |
| "transaction_hash": parser.read_type(Hash256).to_hex(), | |
| "account_hash": parser.read_type(Hash256).to_hex(), | |
| "parent_close_time": parser.read_uint32(), | |
| "close_time": parser.read_uint32(), | |
| "close_time_resolution": parser.read_uint8(), | |
| "close_flags": parser.read_uint8(), | |
| } | |
| except Exception as e: | |
| raise XRPLBinaryCodecException(f"Failed to decode ledger header: {str(e)}") |
| def get_ledger_data_codec_test(): | ||
| """ | ||
| Fetch the (serialized, de-serialized) pairs of ledger_data from the | ||
| codec-fixtures.json file | ||
| """ | ||
|
|
||
| with open( | ||
| "tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json" | ||
| ) as codec_tests: | ||
| ledger_data = json.load(codec_tests)["ledgerData"] | ||
|
|
||
| return ledger_data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align implementation with existing fixture loading pattern.
The current implementation has several issues that should be addressed:
- Hardcoded path could break in different environments
- Missing error handling
- Inconsistent with other fixture loading patterns in this file
Consider refactoring to match the existing pattern:
+_CODEC_FILENAME = "./data/codec-fixtures.json"
+_codec_absolute_path = os.path.join(dirname, _CODEC_FILENAME)
+with open(_codec_absolute_path) as codec_tests:
+ _CODEC_FIXTURES_JSON = json.load(codec_tests)
def get_ledger_data_codec_test():
"""
Fetch the (serialized, de-serialized) pairs of ledger_data from the
codec-fixtures.json file
+
+ Returns:
+ List of ledger data test fixtures
+
+ Raises:
+ KeyError: If ledgerData key is missing from fixtures
"""
- with open(
- "tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json"
- ) as codec_tests:
- ledger_data = json.load(codec_tests)["ledgerData"]
-
- return ledger_data
+ try:
+ return _CODEC_FIXTURES_JSON["ledgerData"]
+ except KeyError as e:
+ raise KeyError("Missing ledgerData in codec-fixtures.json") from eCommittable suggestion skipped: line range outside the PR's diff.
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| import xrpl.core.binarycodec.field_id_codec as field_id_codec | |||
| from tests.unit.core.binarycodec.fixtures import data_driven_fixtures | |||
| from xrpl.core.binarycodec.field_id_codec import decode_ledger_header | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing import for exception handling.
For the new test cases, we need to import XRPLBinaryCodecException.
Add this import:
from unittest import TestCase
import xrpl.core.binarycodec.field_id_codec as field_id_codec
+from xrpl.core.binarycodec.exceptions import XRPLBinaryCodecException
from tests.unit.core.binarycodec.fixtures import data_driven_fixtures
from xrpl.core.binarycodec.field_id_codec import decode_ledger_header📝 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.
| from xrpl.core.binarycodec.field_id_codec import decode_ledger_header | |
| from xrpl.core.binarycodec.field_id_codec import decode_ledger_header | |
| from xrpl.core.binarycodec.exceptions import XRPLBinaryCodecException |
| class TestDecodeLedgerHeader(TestCase): | ||
| def test_decode_ledger_header_valid(self): | ||
| for test in data_driven_fixtures.get_ledger_data_codec_test(): | ||
| self.assertEqual( | ||
| test["json"], decode_ledger_header(test["serialized_data"]) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test implementation only covers the happy path. Consider adding tests for:
- Error cases (malformed input)
- Edge cases (minimum/maximum values)
- Field-specific validations
Add these test methods to improve coverage:
class TestDecodeLedgerHeader(TestCase):
def test_decode_ledger_header_valid(self):
for test in data_driven_fixtures.get_ledger_data_codec_test():
self.assertEqual(
test["json"], decode_ledger_header(test["serialized_data"])
)
+
+ def test_decode_ledger_header_invalid_input(self):
+ """Test that invalid inputs raise appropriate exceptions."""
+ invalid_inputs = [
+ None,
+ 123,
+ "",
+ "invalid hex",
+ "0x1234", # incomplete data
+ ]
+ for invalid_input in invalid_inputs:
+ with self.assertRaises(XRPLBinaryCodecException):
+ decode_ledger_header(invalid_input)
+
+ def test_decode_ledger_header_field_validation(self):
+ """Test specific field values and types."""
+ test_data = data_driven_fixtures.get_ledger_data_codec_test()[0]
+ result = decode_ledger_header(test_data["serialized_data"])
+
+ # Validate field types
+ self.assertIsInstance(result["ledger_index"], int)
+ self.assertIsInstance(result["total_coins"], str)
+ self.assertIsInstance(result["parent_hash"], str)
+ self.assertTrue(result["parent_hash"].isalnum())
+ self.assertEqual(len(result["parent_hash"]), 64) # 32 bytes in hex📝 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.
| class TestDecodeLedgerHeader(TestCase): | |
| def test_decode_ledger_header_valid(self): | |
| for test in data_driven_fixtures.get_ledger_data_codec_test(): | |
| self.assertEqual( | |
| test["json"], decode_ledger_header(test["serialized_data"]) | |
| ) | |
| class TestDecodeLedgerHeader(TestCase): | |
| def test_decode_ledger_header_valid(self): | |
| for test in data_driven_fixtures.get_ledger_data_codec_test(): | |
| self.assertEqual( | |
| test["json"], decode_ledger_header(test["serialized_data"]) | |
| ) | |
| def test_decode_ledger_header_invalid_input(self): | |
| """Test that invalid inputs raise appropriate exceptions.""" | |
| invalid_inputs = [ | |
| None, | |
| 123, | |
| "", | |
| "invalid hex", | |
| "0x1234", # incomplete data | |
| ] | |
| for invalid_input in invalid_inputs: | |
| with self.assertRaises(XRPLBinaryCodecException): | |
| decode_ledger_header(invalid_input) | |
| def test_decode_ledger_header_field_validation(self): | |
| """Test specific field values and types.""" | |
| test_data = data_driven_fixtures.get_ledger_data_codec_test()[0] | |
| result = decode_ledger_header(test_data["serialized_data"]) | |
| # Validate field types | |
| self.assertIsInstance(result["ledger_index"], int) | |
| self.assertIsInstance(result["total_coins"], str) | |
| self.assertIsInstance(result["parent_hash"], str) | |
| self.assertTrue(result["parent_hash"].isalnum()) | |
| self.assertEqual(len(result["parent_hash"]), 64) # 32 bytes in hex |
CHANGELOG.md
Outdated
| @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ### Added | |||
| - Support for the Multi-Purpose Tokens (MPT) amendment (XLS-33) | |||
| - Add `include_deleted` to ledger_entry request | |||
| - decode_ledger_data utility method is useful for parsing serialized ledger-headers | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the changelog entry for better clarity and context.
The current entry is vague and could be more informative. Consider revising it to better describe the feature's purpose and scope:
-decode_ledger_data utility method is useful for parsing serialized ledger-headers
+Add `decode_ledger_data` utility method for parsing serialized hexadecimal ledger data, including ledger headers. This method provides functionality similar to the one in xrpl.js library.This revision:
- Uses consistent formatting with other entries
- Clearly states the method's purpose
- Provides context about its relationship to xrpl.js
- Uses proper punctuation
📝 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.
| - decode_ledger_data utility method is useful for parsing serialized ledger-headers | |
| - Add `decode_ledger_data` utility method for parsing serialized hexadecimal ledger data, including ledger headers. This method provides functionality similar to the one in xrpl.js library. |
High Level Overview of Change
This PR introduces a utility method for parsing serialized hexa-decimal ledger-data.
Context of Change
This PR is similar to this method in xrpl.js.
Type of Change
Did you update CHANGELOG.md?
Test Plan
Three new unit tests are used to validate the correct execution of this new method.