Skip to content

return sig len on ksafe sign der#247

Merged
klever-patrick merged 1 commit into
developfrom
fix/xrp-ksafe
Mar 25, 2026
Merged

return sig len on ksafe sign der#247
klever-patrick merged 1 commit into
developfrom
fix/xrp-ksafe

Conversation

@klever-patrick
Copy link
Copy Markdown
Contributor

@klever-patrick klever-patrick commented Mar 25, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced cryptographic signature handling with improved validation and error detection for signing operations.
    • Signatures now return only the actual signature data rather than fixed-size buffers, improving efficiency.

@github-actions github-actions Bot added the rust label Mar 25, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 25, 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: f7f13144-a8f9-4b69-8cf0-1039ca9f03dd

📥 Commits

Reviewing files that changed from the base of the PR and between bfe36c1 and 3d47910.

📒 Files selected for processing (1)
  • packages/kos/src/crypto/secp256k1.rs

📝 Walkthrough

Walkthrough

Updated the secp256k1 signature function to properly return and validate the actual signature length instead of always returning a fixed 72-byte buffer. Added error handling for invalid signature lengths in the sign_der method and extended the error enum accordingly.

Changes

Cohort / File(s) Summary
Secp256k1 Signature Validation
packages/kos/src/crypto/secp256k1.rs
Added SignError variant to Secp256Err enum with Display formatting. Updated FFI binding c_ecdsa_secp256k1_sign_der to return u32 (signature length). Modified sign_der method to capture returned length, validate it (len != 0 and len <= 72), reject invalid lengths, and return only the actual signature bytes instead of the full 72-byte buffer.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15 minutes

Poem

🐰 A signature once padded and plump,
Now trimmed to its actual length—
No wasted bytes in our crypto leap,
Each hop more lean, each proof more deep! 🔐

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: returning signature length from the ksafe sign_der function, which is the primary modification across the changeset.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/xrp-ksafe

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

@klever-patrick klever-patrick merged commit ace9c82 into develop Mar 25, 2026
6 checks passed
@klever-patrick klever-patrick deleted the fix/xrp-ksafe branch March 25, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants