fix: handle hex string signatures and OverflowError in is_valid_message

[m1lestones]

Description

Fixes #861.

sign() returns a hex-encoded uppercase string. is_valid_message() expected raw bytes. Passing the hex string directly — or calling .encode() on it as a first-time user might — caused a cryptic OverflowError from ecpy's internal EdDSA verifier instead of returning False or raising a descriptive exception.

The natural usage pattern:

signature = keypairs.sign(message, private_key)       # returns str
is_valid  = keypairs.is_valid_message(message, signature, public_key)  # OverflowError

...raised an unhandled exception rather than working as expected.

Changes

xrpl/core/keypairs/main.py

• is_valid_message signature changed from signature: bytes → signature: Union[str, bytes]
• If a str is passed, it is decoded with bytes.fromhex() before being forwarded — matching the same pattern sign() already uses for the message parameter

xrpl/core/keypairs/ed25519.py

• Wrapped _SIGNER.verify() in try/except (OverflowError, ValueError) returning False
• Ensures malformed inputs never surface as an unhandled exception regardless of what the caller passes

tests/unit/core/keypairs/test_main.py

• test_is_valid_message_ed25519_hex_string_signature — the exact pattern from OverflowError exception when verifying a signed message #861: sign() output passed directly to is_valid_message()
• test_is_valid_message_ed25519_malformed_sig_returns_false — confirms OverflowError is caught and returns False

Test plan

• test_is_valid_message_ed25519_hex_string_signature passes — hex string accepted directly
• test_is_valid_message_ed25519_malformed_sig_returns_false passes — no OverflowError
• All existing TestMain tests continue to pass

[coderabbitai]

Warning

Rate limit exceeded

@m1lestones has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 20 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 20 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8c1bcb6d-788a-4eb0-971a-9367e298f77e

📥 Commits

Reviewing files that changed from the base of the PR and between 27cc376 and f08ceb2.

📒 Files selected for processing (4)

• tests/unit/core/keypairs/test_main.py
• xrpl/core/keypairs/ed25519.py
• xrpl/core/keypairs/main.py
• xrpl/core/keypairs/secp256k1.py

Walkthrough

This PR addresses issue #861 by enhancing keypairs signature validation to handle hex-encoded string signatures and gracefully handle malformed signatures with exception handling. It also updates Directory ledger-entry validation to make owner and dir_root optional fields with mutual exclusivity checks, plus comprehensive unit tests.

Changes

Cohort / File(s)	Summary
Keypairs Signature Validation xrpl/core/keypairs/main.py, xrpl/core/keypairs/ed25519.py	Modified is_valid_message to accept both hex-encoded string and raw bytes signatures; added exception handling in ed25519 to catch OverflowError and ValueError, returning False instead of propagating exceptions.
Directory Ledger-Entry Validation xrpl/models/requests/ledger_entry.py	Made Directory.owner and Directory.dir_root optional fields (changed from REQUIRED to Optional[str] = None) and added _get_errors() validation method requiring at least one field to be provided.
Test Coverage tests/unit/core/keypairs/test_main.py, tests/unit/models/requests/test_ledger_entry.py	Added Ed25519 signature validation tests covering hex-string signatures and malformed signature handling; expanded Directory validation tests for owner, dir\_root, and error cases.

Possibly related issues

• Request model error xrpl.models.requests.ledger_entry.Directory #885: The changes to Directory validation in xrpl/models/requests/ledger_entry.py make owner and dir\_root optional with mutual validation, directly addressing the issue about dir\_root being unnecessarily required.

Poem

🐰 A signature's hex, now wrapped with care,
No overflow terrors floating in air!
Directory paths need not both exist,
Either root or owner—flexibility kissed! ✨

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Changes to LedgerEntry Directory validation are unrelated to issue #861 (OverflowError in is_valid_message) and appear to be out of scope for this PR.	Remove the LedgerEntry Directory-related changes from this PR; those should be addressed in a separate PR focused on Directory validation.
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The PR description addresses the core issue but lacks formal structure matching the repository's required template (High Level Overview, Context, Type of Change checkboxes, CHANGELOG.md confirmation, Test Plan section).	Reorganize the description to match the required template format, including the Type of Change checkboxes and explicit CHANGELOG.md confirmation section.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main changes: handling hex string signatures and the OverflowError in is_valid_message function.
Linked Issues check	✅ Passed	The PR fully addresses issue #861 by accepting hex-string signatures and preventing OverflowError from propagating, meeting all stated objectives.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

tests/unit/core/keypairs/test_main.py (1)

237-242: Consider adding test coverage for additional edge cases.

The current test covers malformed bytes passed as signature. Consider adding tests for:

1. Invalid hex string — verifies behavior when a non-hex string like "not-valid-hex" is passed as signature (this would currently raise ValueError per my earlier comment).

2. secp256k1 malformed signature — verifies consistent behavior across both crypto backends (currently secp256k1 lacks the exception handling).

Suggested additional test

def test_is_valid_message_invalid_hex_string_signature(self):
    # Non-hex string should return False (or raise clear exception), not crash
    public_key = "ED01FA53FA5A7E77798F882ECE20B1ABC00BB358A9E55A202D0D0676BD0CE37A63"
    invalid_hex = "not-valid-hex"
    # This test will fail until main.py handles ValueError from bytes.fromhex()
    self.assertFalse(keypairs.is_valid_message(b"test message", invalid_hex, public_key))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/core/keypairs/test_main.py` around lines 237 - 242, The
is_valid_message path currently lets non-hex signatures and malformed secp256k1
signatures raise (e.g., ValueError from bytes.fromhex or backend-specific
exceptions); update keypairs.is_valid_message to defensively parse the signature
inside a try/except and to return False on bytes.fromhex ValueError/TypeError
and on verification exceptions (e.g., OverflowError or the secp256k1 library's
signature errors) rather than letting them propagate; ensure both Ed25519 and
secp256k1 verification branches are wrapped so malformed hex and malformed
signature bytes both yield False; add unit tests mirroring the provided
malformed-sig test: one for a non-hex string signature and one for a secp256k1
malformed signature to assert False.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@xrpl/core/keypairs/ed25519.py`:
- Around line 92-95: The SECP256K1.is_valid_message method must mirror the
Ed25519 behavior: wrap the verification call in a try/except that catches
OverflowError and ValueError and returns False on those exceptions so the method
always returns a bool. Locate SECP256K1.is_valid_message and the call to the
secp verification routine (the code that currently performs the verify and can
raise) and add the same try/except pattern used in ed25519.is_valid_message to
return False for OverflowError and ValueError.

In `@xrpl/core/keypairs/main.py`:
- Around line 141-142: The conversion bytes.fromhex(signature) can raise
ValueError for invalid hex strings; wrap that conversion in a try/except
ValueError inside the function that handles signature verification (where the
local variable signature is converted) and return False for malformed hex inputs
to match the ed25519.py behavior; ensure the except only catches ValueError from
bytes.fromhex and does not swallow other exceptions, keeping the rest of the
verification flow unchanged.

---

Nitpick comments:
In `@tests/unit/core/keypairs/test_main.py`:
- Around line 237-242: The is_valid_message path currently lets non-hex
signatures and malformed secp256k1 signatures raise (e.g., ValueError from
bytes.fromhex or backend-specific exceptions); update keypairs.is_valid_message
to defensively parse the signature inside a try/except and to return False on
bytes.fromhex ValueError/TypeError and on verification exceptions (e.g.,
OverflowError or the secp256k1 library's signature errors) rather than letting
them propagate; ensure both Ed25519 and secp256k1 verification branches are
wrapped so malformed hex and malformed signature bytes both yield False; add
unit tests mirroring the provided malformed-sig test: one for a non-hex string
signature and one for a secp256k1 malformed signature to assert False.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: de1fc31d-64c9-48e9-8f34-d92075480db0

📥 Commits

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

📒 Files selected for processing (5)

• tests/unit/core/keypairs/test_main.py
• tests/unit/models/requests/test_ledger_entry.py
• xrpl/core/keypairs/ed25519.py
• xrpl/core/keypairs/main.py
• xrpl/models/requests/ledger_entry.py