Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #354] Introduce decode_ledger_data utility method #785

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Fix #354] Introduce decode_ledger_data utility method #785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29a708e
[Fix #354] Introduce decode_ledger_data utility method
ckeshava Dec 20, 2024
6a4df65
Merge branch 'main' into decodeLedgerData
ckeshava Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [[Unreleased]]

### Added
- decode_ledger_data utility method is useful for parsing serialized ledger-headers

## [4.0.0] - 2024-12-23

### Added
Expand Down Expand Up @@ -95,6 +98,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [2.0.0] - 2023-07-05
### BREAKING CHANGE
- The default signing algorithm in the `Wallet` was changed from secp256k1 to ed25519

### Added:
- Wallet support for regular key compatibility
- Added new ways of wallet generation: `from_seed`, `from_secret`, `from_entropy`, `from_secret_numbers`
Expand Down
28 changes: 27 additions & 1 deletion tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4842,7 +4842,7 @@
}
],
"ledgerData": [{
"binary": "01E91435016340767BF1C4A3EACEB081770D8ADE216C85445DD6FB002C6B5A2930F2DECE006DA18150CB18F6DD33F6F0990754C962A7CCE62F332FF9C13939B03B864117F0BDA86B6E9B4F873B5C3E520634D343EF5D9D9A4246643D64DAD278BA95DC0EAC6EB5350CF970D521276CDE21276CE60A00",
"serialized_data": "01E91435016340767BF1C4A3EACEB081770D8ADE216C85445DD6FB002C6B5A2930F2DECE006DA18150CB18F6DD33F6F0990754C962A7CCE62F332FF9C13939B03B864117F0BDA86B6E9B4F873B5C3E520634D343EF5D9D9A4246643D64DAD278BA95DC0EAC6EB5350CF970D521276CDE21276CE60A00",
"json": {
"account_hash": "3B5C3E520634D343EF5D9D9A4246643D64DAD278BA95DC0EAC6EB5350CF970D5",
"close_flags": 0,
Expand All @@ -4854,5 +4854,31 @@
"total_coins": "99994494362043555",
"transaction_hash": "DD33F6F0990754C962A7CCE62F332FF9C13939B03B864117F0BDA86B6E9B4F87"
}
}, {
"serialized_data": "058973980163396C087559B1361DA99E35E60A7A769473C536F4D2218C75872CD23FB76701C0C4A7D5DDD34B52F011B54E84BEC1E84C509ADEB64111AE2D7B6082B2065A70A1538678DF1ED14E8D02783F8E4F47A636BB73264EEADAAF93FA1DC10E8BFA9FEDE1E436313EE22EF7E1202EF7E1210A00",
"json": {
"account_hash": "4E8D02783F8E4F47A636BB73264EEADAAF93FA1DC10E8BFA9FEDE1E436313EE2",
"close_flags": 0,
"close_time": 787996961,
"close_time_resolution": 10,
"parent_close_time": 787996960,
"parent_hash": "361DA99E35E60A7A769473C536F4D2218C75872CD23FB76701C0C4A7D5DDD34B",
"total_coins": "99986752893442481",
"transaction_hash": "52F011B54E84BEC1E84C509ADEB64111AE2D7B6082B2065A70A1538678DF1ED1",
"ledger_index": 92894104
}
}, {
"serialized_data": "058974040163396C0704FF8BBB7676027C39A0BF54ACA8B472CD2D39CBB83AE9612FF6E129E1FE6237A7D75AE1D62183ED82F39825A20199251427D1DEBFD46C1AABAA473DDFDB93ABBAF9CDD7C48AD76715EE257DBF64BE4248827A147E59F90D1905174C47D6878C278AB12EF7E2C42EF7E2C50A00",
"json": {
"account_hash": "D7C48AD76715EE257DBF64BE4248827A147E59F90D1905174C47D6878C278AB1",
"close_flags": 0,
"close_time": 787997381,
"close_time_resolution": 10,
"parent_close_time": 787997380,
"parent_hash": "BB7676027C39A0BF54ACA8B472CD2D39CBB83AE9612FF6E129E1FE6237A7D75A",
"total_coins": "99986752869302155",
"transaction_hash": "E1D62183ED82F39825A20199251427D1DEBFD46C1AABAA473DDFDB93ABBAF9CD",
"ledger_index": 92894212
}
}]
}
14 changes: 14 additions & 0 deletions tests/unit/core/binarycodec/fixtures/data_driven_fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@
# top level keys: ['types', 'fields_tests', 'whole_objects', 'values_tests']


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
Comment on lines +12 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Align implementation with existing fixture loading pattern.

The current implementation has several issues that should be addressed:

  1. Hardcoded path could break in different environments
  2. Missing error handling
  3. 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 e

Committable suggestion skipped: line range outside the PR's diff.



def get_field_tests():
"""
Constructs and returns a list of FieldTest objects after parsing JSON data
Expand Down
9 changes: 9 additions & 0 deletions tests/unit/core/binarycodec/test_field_id_codec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 TestFieldIDCodec(TestCase):
Expand All @@ -21,3 +22,11 @@ def test_encode(self):
def test_decode(self):
for test in self.field_tests:
self.assertEqual(test.name, field_id_codec.decode(test.expected_hex))


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"])
)
Comment on lines +27 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage with additional test cases.

The current test implementation only covers the happy path. Consider adding tests for:

  1. Error cases (malformed input)
  2. Edge cases (minimum/maximum values)
  3. 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.

Suggested change
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

35 changes: 35 additions & 0 deletions xrpl/core/binarycodec/field_id_codec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,44 @@
`Field IDs <https://xrpl.org/serialization.html#field-ids>`_
"""

from typing import Any, Dict

from xrpl.core.binarycodec.binary_wrappers import BinaryParser
from xrpl.core.binarycodec.definitions import definitions
from xrpl.core.binarycodec.definitions.field_header import FieldHeader
from xrpl.core.binarycodec.exceptions import XRPLBinaryCodecException
from xrpl.core.binarycodec.types.hash256 import Hash256
from xrpl.core.binarycodec.types.uint64 import UInt64


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(),
}
Comment on lines +16 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 encode(field_name: str) -> bytes:
Expand Down
Loading