Fix Request.from_xrpl by aliasing it to Request.from_dict
#892
Conversation
WalkthroughAdds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
xrpl/models/requests/request.py (1)
166-180: Improve docstring clarity and consider JSON error handling.The implementation correctly delegates to
from_dict, but the docstring describes it as an "Alias offrom_dict" without mentioning that it additionally accepts JSON strings. Consider updating the docstring to explicitly state this capability.Additionally,
json.loads()will raiseJSONDecodeErrorfor invalid JSON strings, which may not provide the clearest error message to users expecting XRPL-specific validation errors.Note on WebSocket format support:
According to issue #891,
from_xrplshould handle both JSON-RPC format (method+params) and WebSocket format (command+ params). The current implementation only handles dicts with amethodkey (since it delegates tofrom_dict), not thecommandkey used in WebSocket-style requests. The TODO comment at line 139 also mentions this gap.While
GenericRequest.to_dict()convertscommandtomethod, the reverse conversion isn't implemented infrom_dict. This means WebSocket-style dicts will still fail with "Request does not include method."Is this partial fix intentional as a first step, with full XRPL format support planned for a follow-up? If so, consider updating the method name or docstring to clarify the current scope.
Apply this diff to improve the docstring:
@classmethod def from_xrpl(cls: Type[Self], value: Union[str, Dict[str, Any]]) -> Self: """ - Construct a new Request from a dictionary of parameters. Alias of `from_dict`. + Construct a new Request from a dictionary or JSON string. Alias of `from_dict` + with additional support for JSON string parsing. Args: - value: The value to construct the Request from. + value: A dictionary or JSON string to construct the Request from. Returns: A new Request object, constructed using the given parameters. Raises: XRPLModelException: If the dictionary provided is invalid. + JSONDecodeError: If the string provided is not valid JSON. """ return cls.from_dict(value if isinstance(value, dict) else json.loads(value))Optional: To improve error handling for invalid JSON, consider wrapping
json.loads()in a try-except block:@classmethod def from_xrpl(cls: Type[Self], value: Union[str, Dict[str, Any]]) -> Self: """ Construct a new Request from a dictionary or JSON string. Alias of `from_dict` with additional support for JSON string parsing. Args: value: A dictionary or JSON string to construct the Request from. Returns: A new Request object, constructed using the given parameters. Raises: XRPLModelException: If the dictionary provided is invalid or if the string is not valid JSON. """ if isinstance(value, dict): return cls.from_dict(value) try: parsed = json.loads(value) except json.JSONDecodeError as e: raise XRPLModelException(f"Invalid JSON string: {e}") return cls.from_dict(parsed)tests/unit/models/requests/test_requests.py (1)
32-42: Consider testing JSON string input.The test validates that
from_xrplworks with dictionary input, which is good. However,from_xrpladds the capability to accept JSON strings (not just dicts), and this new functionality isn't currently tested.Consider adding a test for JSON string input:
def test_from_xrpl_json_string(self): req_str = '{"method": "account_tx", "account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj"}' obj = Request.from_xrpl(req_str) self.assertEqual(obj.__class__.__name__, "AccountTx") expected = { "method": "account_tx", "account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj", "binary": False, "forward": False, "api_version": _DEFAULT_API_VERSION, } self.assertDictEqual(obj.to_dict(), expected)Optionally, you could also add tests for error cases (invalid JSON, unsupported types) to ensure proper error handling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)tests/unit/models/requests/test_requests.py(1 hunks)xrpl/models/requests/request.py(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/requests/vault_info.py:16-31
Timestamp: 2025-06-04T22:17:47.822Z
Learning: In the xrpl-py library, Request classes do not perform client-side validation on their contents. The library relies on the rippled node to return appropriate error codes for malformed requests, maintaining a consistent pattern across all Request models.
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 726
File: tests/unit/models/requests/test_requests.py:16-18
Timestamp: 2024-11-01T20:40:21.354Z
Learning: In the xrpl-py codebase, the `to_dict` method of classes inheriting from `Request` automatically includes the `api_version` key in the resulting dictionary. When reviewing tests that use `to_dict()`, expect `api_version` to be present in the output.
📚 Learning: 2024-11-01T20:40:21.354Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 726
File: tests/unit/models/requests/test_requests.py:16-18
Timestamp: 2024-11-01T20:40:21.354Z
Learning: In the xrpl-py codebase, the `to_dict` method of classes inheriting from `Request` automatically includes the `api_version` key in the resulting dictionary. When reviewing tests that use `to_dict()`, expect `api_version` to be present in the output.
Applied to files:
CHANGELOG.mdtests/unit/models/requests/test_requests.pyxrpl/models/requests/request.py
📚 Learning: 2025-06-04T22:17:47.822Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/requests/vault_info.py:16-31
Timestamp: 2025-06-04T22:17:47.822Z
Learning: In the xrpl-py library, Request classes do not perform client-side validation on their contents. The library relies on the rippled node to return appropriate error codes for malformed requests, maintaining a consistent pattern across all Request models.
Applied to files:
CHANGELOG.mdtests/unit/models/requests/test_requests.pyxrpl/models/requests/request.py
📚 Learning: 2024-11-01T18:53:01.394Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Applied to files:
tests/unit/models/requests/test_requests.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
tests/unit/models/requests/test_requests.py
📚 Learning: 2024-11-01T16:20:50.192Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Applied to files:
tests/unit/models/requests/test_requests.py
📚 Learning: 2024-11-01T20:41:08.207Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_credential_create.py:13-22
Timestamp: 2024-11-01T20:41:08.207Z
Learning: In Python test files in the xrpl-py codebase (e.g., `tests/unit/models/transactions/test_credential_create.py`), docstrings are not required for test classes and methods if their names are self-explanatory enough.
Applied to files:
tests/unit/models/requests/test_requests.py
🧬 Code graph analysis (1)
tests/unit/models/requests/test_requests.py (2)
xrpl/models/requests/request.py (3)
Request(103-213)from_xrpl(167-180)to_dict(204-213)xrpl/models/requests/generic_request.py (1)
to_dict(80-99)
⏰ 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). (8)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.12)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
xrpl/models/requests/request.py (1)
8-8: LGTM!The
jsonimport is necessary for parsing JSON strings in the newfrom_xrplmethod.CHANGELOG.md (1)
10-12: LGTM!The changelog entry accurately documents the fix and is appropriately categorized under "Fixed" for the Unreleased version.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where Request.from_xrpl was incorrectly using the inherited BaseModel.from_xrpl method, which expected PascalCase fields and converted them to snake_case. Since XRPL request objects already use snake_case for their top-level fields (unlike transactions which use PascalCase), this conversion was incorrect. The fix aliases Request.from_xrpl to Request.from_dict to properly handle snake_case request fields.
- Creates a new
Request.from_xrplmethod that delegates toRequest.from_dict - Adds support for both dictionary and JSON string inputs via type checking
- Includes a basic test case to verify the functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| xrpl/models/requests/request.py | Adds json import and implements from_xrpl method as an alias to from_dict with support for both dict and string inputs |
| tests/unit/models/requests/test_requests.py | Adds test case test_from_xrpl to verify the method works correctly with dictionary input |
| CHANGELOG.md | Documents the bug fix in the Unreleased section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_from_xrpl(self): | ||
| req = {"method": "account_tx", "account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj"} | ||
| obj = Request.from_xrpl(req) | ||
| self.assertEqual(obj.__class__.__name__, "AccountTx") | ||
| expected = { | ||
| **req, | ||
| "binary": False, | ||
| "forward": False, | ||
| "api_version": _DEFAULT_API_VERSION, | ||
| } | ||
| self.assertDictEqual(obj.to_dict(), expected) |
There was a problem hiding this comment.
The test only verifies from_xrpl with a dictionary input. According to the method signature and documentation, from_xrpl also accepts a JSON string. Consider adding a test case that passes a JSON string to ensure both input types work correctly. For example: obj = Request.from_xrpl(json.dumps(req)).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
Title pretty much says it all.
Request.from_xrplwas originally pointing to thefrom_xrplofBaseModel, so it was assuming it would be fullyPascalCasefields that would need to becamel_case'd. This is incorrect for requests.Context of Change
Fixes #891
Type of Change
Did you update CHANGELOG.md?
Test Plan
CI passes. Added basic test.