Skip to content

Conversation

@radik878
Copy link

@radik878 radik878 commented Oct 5, 2025

  • Replace Buffer.from(signature.toDER(), 'hex') with direct Array.from(signature.toDER()) in genMockPassportData.ts and genMockIdDoc.ts.
  • Remove ts-ignore comments tied to incorrect hex encoding.
  • Keeps signatures in DER as expected by downstream parsers (e.g., extractRSFromSignature).

Summary by CodeRabbit

  • Refactor
    • Streamlined ECDSA signature byte handling by using a direct DER-bytes approach, removing unnecessary conversions.
    • Improves type-safety, consistency across environments, and reduces potential edge-case errors in signature generation.
    • Aligns behavior in mock ID document and passport data generation for greater consistency.
    • No functional changes expected; existing flows and outputs should remain the same for end users.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 5, 2025

Walkthrough

Replaced hex-based DER signature conversion with direct number[] casting in ECDSA signing paths within two utilities. No control-flow or API changes; only the method of constructing signature bytes was updated. Removed TypeScript ignore comments and Buffer.from usage.

Changes

Cohort / File(s) Summary
ECDSA DER conversion update
common/src/utils/passports/genMockIdDoc.ts, common/src/utils/passports/genMockPassportData.ts
In ECDSA sign paths, switched from Buffer.from(signature.toDER(), 'hex') to Array.from(signature.toDER() as number[]), removing TS ignore and hex decoding; logic flow unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • remicolin

Poem

Two bytes diverged in a cryptic DER,
We chose the path of number arrays there.
No buffers to hex, no comments to fear,
Just cleaner casts, the intent now clear.
Signatures stride with lighter gear.

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ 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 clearly describes the core change—using elliptic-generated DER bytes directly for ECDSA signatures—and matches the modifications in the signature serialization code. It is concise, specific, and accurately reflects the main functionality updated without extraneous detail. This phrasing makes it easy for teammates to understand the primary fix when scanning history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
common/src/utils/passports/genMockIdDoc.ts (1)

226-270: Consider extracting duplicate sign() logic.

The sign() function here duplicates logic from genMockPassportData.ts (lines 143-188). Consider extracting to a shared utility module for maintainability.

common/src/utils/passports/genMockPassportData.ts (1)

179-179: Approve correct ECDSA DER byte usage

  • genMockPassportData and genMockIdDoc now correctly emit raw DER bytes.
  • Recommend adding TS-level unit tests to cover ECDSA mock data generation and parsing via extractRSFromSignature.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48f8d79 and 0e05c5c.

📒 Files selected for processing (2)
  • common/src/utils/passports/genMockIdDoc.ts (1 hunks)
  • common/src/utils/passports/genMockPassportData.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx,sol,nr}

📄 CodeRabbit inference engine (.cursorrules)

**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g., ***-***-1234 for passport numbers, J*** D*** for names).

Files:

  • common/src/utils/passports/genMockPassportData.ts
  • common/src/utils/passports/genMockIdDoc.ts
common/src/**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

common/src/**/*.{ts,tsx,js,jsx}: Review shared utilities for:

  • Reusability and modular design
  • Type safety and error handling
  • Side-effect management
  • Documentation and naming clarity

Files:

  • common/src/utils/passports/genMockPassportData.ts
  • common/src/utils/passports/genMockIdDoc.ts
🔇 Additional comments (1)
common/src/utils/passports/genMockIdDoc.ts (1)

261-261: Critical fix approved: consistent with genMockPassportData.ts.

The change correctly handles DER bytes from elliptic. Same validation recommendations apply.

@transphorm transphorm changed the base branch from main to dev October 6, 2025 21:58
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.

2 participants