Conversation
WalkthroughThis change fixes a validation error in the Directory model for LedgerEntry requests by making the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (3)
xrpl/models/requests/ledger_entry.py (1)
119-139: Missing validation for mutually-required fields.The docstring states that the input "requires either dir_root or owner", but there's no
_get_errorsmethod to enforce this constraint. ADirectory()with neither field set will pass model validation but fail at the XRPL server.Consider adding validation similar to other models in this file:
🔎 Proposed validation method
@require_kwargs_on_init @dataclass(frozen=True, **KW_ONLY_DATACLASS) class Directory(BaseModel): """ The input requires either dir_root or owner as a sub-field, plus optionally a sub_index sub-field. """ owner: Optional[str] = None """ (Optional) Unique address of the account associated with this directory. """ dir_root: Optional[str] = None """ (Optional) Unique index identifying the directory to retrieve, as a hex string. """ sub_index: Optional[int] = None """ (Optional) If provided, jumps to a later "page" of the DirectoryNode. """ + + def _get_errors(self: Self) -> Dict[str, str]: + errors = super()._get_errors() + if self.owner is None and self.dir_root is None: + errors["Directory"] = "Must provide either `owner` or `dir_root`." + return errorsNote: You'll need to add
Selfto the imports fromtyping_extensionsandDictfromtypingif not already available in the class scope.tests/integration/reqs/test_ledger_entry.py (2)
43-43: Use lowercase for local variables.
DIR_NODE_INDEXusesSCREAMING_SNAKE_CASE, which by Python convention is reserved for module-level constants. Since this is a local variable assigned at runtime, usesnake_case.🔎 Proposed fix
- DIR_NODE_INDEX = response.result["index"] + dir_node_index = response.result["index"]And update line 58:
- response = await client.request(LedgerEntry(index=DIR_NODE_INDEX)) + response = await client.request(LedgerEntry(index=dir_node_index))
36-60: Consider adding test coverage fordir_rootretrieval.The test validates fetching by
owner, but per the updated model,dir_rootis an equally valid alternative. Consider adding a test case that fetches the directory usingdir_rootto ensure both code paths work correctly.🔎 Example addition after line 42
# fetch the directory using dir_root response = await client.request( LedgerEntry(directory=Directory(dir_root=dir_node_index)) ) self.assertTrue(response.is_successful()) self.assertEqual(response.result["node"]["LedgerEntryType"], "DirectoryNode")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)tests/integration/reqs/test_ledger_entry.py(1 hunks)xrpl/models/requests/ledger_entry.py(1 hunks)
⏰ 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.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.9)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
CHANGELOG.md (1)
10-12: LGTM!The changelog entry is clear, properly placed under the Unreleased/Fixed section, and follows the existing format.
tests/integration/reqs/test_ledger_entry.py (1)
16-60: Good test coverage for the fix.The test properly validates the fix by:
- Creating a directory entry via DIDSet
- Fetching using the Directory dataclass with optional fields
- Fetching using a JSON dict
- Fetching by ledger index
This confirms the model now accepts
ownerwithout requiringdir_root.
| """ | ||
| Required fields for requesting a DirectoryNode if not querying by | ||
| object ID. | ||
| The input requires either dir_root or owner as a sub-field, plus optionally a |
There was a problem hiding this comment.
This should probably be checked in the model validation
| The input requires either dir_root or owner as a sub-field, plus optionally a | ||
| sub_index sub-field. |
There was a problem hiding this comment.
| The input requires either dir_root or owner as a sub-field, plus optionally a | |
| sub_index sub-field. | |
| Required fields for requesting a DirectoryNode if not querying by | |
| object ID. | |
| The input requires either dir_root or owner as a sub-field, plus optionally a | |
| sub_index sub-field. |
3 participants
High Level Overview of Change
fix #885: Fix error in the Directory model of LedgerEntry request
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added new integration test to validate the correctness of the change.