Skip to content

fix: make Directory.dir_root and Directory.owner optional in LedgerEntry#943

Open
m1lestones wants to merge 1 commit intoXRPLF:mainfrom
m1lestones:fix/directory-dir-root-optional
Open

fix: make Directory.dir_root and Directory.owner optional in LedgerEntry#943
m1lestones wants to merge 1 commit intoXRPLF:mainfrom
m1lestones:fix/directory-dir-root-optional

Conversation

@m1lestones
Copy link
Copy Markdown

Description

Fixes #885.

Both dir_root and owner were marked as REQUIRED in the Directory model used by LedgerEntry, but the XRPL ledger_entry docs specify that only one of the two is needed — you can look up a DirectoryNode by owner alone or by dir_root alone.

This caused a spurious XRPLModelException: {'dir_root': 'dir_root is not set'} when constructing a valid Directory with only owner provided, even though the same query works correctly when passed as a raw dict.

Changes

xrpl/models/requests/ledger_entry.py

  • Directory.owner: str = REQUIREDOptional[str] = None
  • Directory.dir_root: str = REQUIREDOptional[str] = None
  • Added Directory._get_errors() to enforce that at least one of owner or dir_root is provided

tests/unit/models/requests/test_ledger_entry.py

  • Added Directory to imports
  • Added test_directory_with_owner_only_is_valid — regression test for Request model error xrpl.models.requests.ledger_entry.Directory #885
  • Added test_directory_with_dir_root_only_is_valid — confirms dir_root-only is also valid
  • Added test_directory_with_neither_owner_nor_dir_root_is_invalid — confirms validation catches missing fields

Test plan

  • test_directory_with_owner_only_is_valid — the exact case from issue Request model error xrpl.models.requests.ledger_entry.Directory #885 now passes
  • test_directory_with_dir_root_only_is_valid — dir_root-only lookup is valid
  • test_directory_with_neither_owner_nor_dir_root_is_invalid — raises XRPLModelException as expected
  • All existing TestLedgerEntry tests continue to pass

Both `dir_root` and `owner` were marked as REQUIRED in the Directory
model, but the XRPL ledger_entry API only requires one of the two.
This caused a spurious XRPLModelException when constructing a Directory
with only `owner` (a valid and commonly used pattern).

- Make both `owner` and `dir_root` Optional[str] = None
- Add _get_errors validation ensuring at least one is provided
- Add regression tests covering owner-only, dir_root-only, and
  neither-provided cases

Fixes XRPLF#885
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 55597edc-d822-45f9-a659-af568c05e8ac

📥 Commits

Reviewing files that changed from the base of the PR and between 8879e95 and ce294b4.

📒 Files selected for processing (2)
  • tests/unit/models/requests/test_ledger_entry.py
  • xrpl/models/requests/ledger_entry.py

Walkthrough

The Directory request model's owner and dir_root fields were changed from required to optional, with a custom validation method added to enforce that at least one field must be provided. Three corresponding test cases were added to verify the new validation behavior.

Changes

Cohort / File(s) Summary
Model Validation Logic
xrpl/models/requests/ledger_entry.py
Made owner and dir_root fields optional (Optional[str] = None). Added _get_errors() override to validate that at least one field is provided, raising an error when both are None.
Test Coverage
tests/unit/models/requests/test_ledger_entry.py
Added three test cases: (1) Directory with owner and sub_index without dir_root; (2) Directory with dir_root alone; (3) Directory with neither field raises XRPLModelException. Imported Directory model into test module.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops through validation schemes,
Where fields once rigid now flow free,
At least one path, the garden deems,
Makes Directory what it should be! 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: making Directory.dir_root and Directory.owner optional in LedgerEntry, addressing issue #885.
Description check ✅ Passed The PR description is well-structured, includes context (reference to issue #885 and XRPL docs), specifies the exact changes, and documents the test plan with all test cases covered.
Linked Issues check ✅ Passed The PR fully addresses all objectives from issue #885: makes both fields optional, enforces that at least one is provided via _get_errors(), accepts owner-only and dir_root-only cases, and rejects neither-provided cases.
Out of Scope Changes check ✅ Passed All changes are strictly within scope: the model field changes, validation logic, updated docstring, and comprehensive test coverage directly address issue #885 with no extraneous modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request model error xrpl.models.requests.ledger_entry.Directory

1 participant