Skip to content

Conversation

@Vishalkulkarni45
Copy link
Collaborator

@Vishalkulkarni45 Vishalkulkarni45 commented Oct 3, 2025

support for serbia dsc

Summary by CodeRabbit

  • New Features

    • Added support for an additional DSC circuit variant exposing a public merkle_root input for verification workflows.
  • Tests

    • Added RSA‑PSS test vector (SHA‑384, 3072‑bit key, exponent 65537, 48‑byte salt).
    • Standardized test dependency resolution to use top-level node_modules paths for more reliable test loading.

@cursor
Copy link

cursor bot commented Oct 3, 2025

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 17.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Walkthrough

Adds a new Circom DSC instance exposing a public merkle_root (DSC(18,120,35)); extends RSAPSS test vectors with sha384/saltLen=48/e=65537/keyLen=3072; and updates many test file include/path resolutions to reference top-level node_modules and adjusted relative key paths.

Changes

Cohort / File(s) Summary
New DSC circuit instance
circuits/circuits/dsc/instances/dsc_sha384_rsapss_65537_48_3072.circom
New Circom v2.1.9 circuit file that includes dsc.circom, declares component main with a single public input merkle_root, and instantiates DSC(18, 120, 35).
Test vectors update
circuits/tests/dsc/test_cases.ts
Adds an RSA-PSS entry to fullSigAlgs: { sigAlg: 'rsapss', hashFunction: 'sha384', saltLen: '48', domainParameter: '65537', keyLength: '3072' }.
Test path / include adjustments (multiple tests)
circuits/tests/disclose/vc_and_disclose_aadhaar.test.ts, circuits/tests/other_circuits/qrdata_extractor.test.ts, circuits/tests/register/register_aadhaar.test.ts, circuits/tests/disclose/vc_and_disclose.test.ts, circuits/tests/disclose/vc_and_disclose_id.test.ts, circuits/tests/dsc/dsc.test.ts, circuits/tests/ofac/ofac.test.ts, circuits/tests/other_circuits/custom_hasher.test.ts, circuits/tests/other_circuits/is_older_than.test.ts, circuits/tests/other_circuits/is_valid.test.ts, circuits/tests/other_circuits/prove_country_is_not_in_list.test.ts, circuits/tests/register/register.test.ts, circuits/tests/register_id/register_id.test.ts, circuits/tests/utils/ecdsa.test.ts, circuits/tests/utils/rsaPkcs1v1_5.test.ts, circuits/tests/utils/rsapss.test.ts
Updated test resource paths: some testPrivateKey.pem/testPublicKey.pem relative paths changed (../../ vs ../../../) and many wasm_tester include arrays switched from relative ../node_modules variants to top-level node_modules and explicit node_modules/* subpaths (e.g., node_modules/circomlib/circuits, node_modules/@zk-kit/binary-merkle-root.circom/src). No test logic or assertions changed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant DevTests as Test Suite
  participant Wasm as wasm_tester
  participant Circom as Circom Compiler
  participant DSC as DSC Circuit (DSC(18,120,35))

  rect rgba(210,235,255,0.6)
    DevTests->>Wasm: invoke wasm_tester(include: ['node_modules', ...])
    note right of Wasm #D0EBFF: includes resolved from top-level node_modules
  end

  Wasm->>Circom: compile circuit files (including new DSC instance)
  Circom->>DSC: instantiate DSC(18,120,35)
  DSC-->>Circom: expose public input `merkle_root`
  Circom-->>Wasm: compiled wasm + r1cs
  Wasm-->>DevTests: test harness ready
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • remicolin
  • hackertron

Poem

A root appears in wire and gate,
PSS and SHA feed the proof's fate.
Paths trimmed, vectors grown in kind,
Tests align and links unwind.
Small edits hum — the circuits wait.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the main feature addition by specifying the new DSC variant’s identifier. It is concise, uses a standard “feat:” prefix, and avoids unnecessary detail. A teammate scanning history would understand that the pull request introduces the dsc_sha384_rsapss_65537_48_3072 circuit.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/serbia-dsc

📜 Recent 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 a14d816 and fe78f77.

📒 Files selected for processing (13)
  • circuits/tests/disclose/vc_and_disclose.test.ts (1 hunks)
  • circuits/tests/disclose/vc_and_disclose_id.test.ts (1 hunks)
  • circuits/tests/dsc/dsc.test.ts (1 hunks)
  • circuits/tests/ofac/ofac.test.ts (6 hunks)
  • circuits/tests/other_circuits/custom_hasher.test.ts (1 hunks)
  • circuits/tests/other_circuits/is_older_than.test.ts (1 hunks)
  • circuits/tests/other_circuits/is_valid.test.ts (1 hunks)
  • circuits/tests/other_circuits/prove_country_is_not_in_list.test.ts (1 hunks)
  • circuits/tests/register/register.test.ts (1 hunks)
  • circuits/tests/register_id/register_id.test.ts (1 hunks)
  • circuits/tests/utils/ecdsa.test.ts (4 hunks)
  • circuits/tests/utils/rsaPkcs1v1_5.test.ts (3 hunks)
  • circuits/tests/utils/rsapss.test.ts (7 hunks)
✅ Files skipped from review due to trivial changes (6)
  • circuits/tests/disclose/vc_and_disclose.test.ts
  • circuits/tests/dsc/dsc.test.ts
  • circuits/tests/utils/ecdsa.test.ts
  • circuits/tests/other_circuits/is_valid.test.ts
  • circuits/tests/other_circuits/is_older_than.test.ts
  • circuits/tests/ofac/ofac.test.ts
🧰 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:

  • circuits/tests/other_circuits/prove_country_is_not_in_list.test.ts
  • circuits/tests/utils/rsaPkcs1v1_5.test.ts
  • circuits/tests/utils/rsapss.test.ts
  • circuits/tests/other_circuits/custom_hasher.test.ts
  • circuits/tests/register/register.test.ts
  • circuits/tests/register_id/register_id.test.ts
  • circuits/tests/disclose/vc_and_disclose_id.test.ts
**/*.{test,spec}.{ts,js,tsx,jsx}

⚙️ CodeRabbit configuration file

**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:

  • Test coverage completeness
  • Test case quality and edge cases
  • Mock usage appropriateness
  • Test readability and maintainability

Files:

  • circuits/tests/other_circuits/prove_country_is_not_in_list.test.ts
  • circuits/tests/utils/rsaPkcs1v1_5.test.ts
  • circuits/tests/utils/rsapss.test.ts
  • circuits/tests/other_circuits/custom_hasher.test.ts
  • circuits/tests/register/register.test.ts
  • circuits/tests/register_id/register_id.test.ts
  • circuits/tests/disclose/vc_and_disclose_id.test.ts
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: run_circuit_tests
🔇 Additional comments (7)
circuits/tests/disclose/vc_and_disclose_id.test.ts (1)

69-71: LGTM! Include paths correctly updated to node_modules resolution.

The switch from relative paths to centralized node_modules-based includes aligns with repository-wide standardization. The paths are correct:

  • node_modules/@zk-kit/binary-merkle-root.circom/src matches the v2.0.0 package structure
  • node_modules/circomlib/circuits is the standard circomlib template location

Based on learnings.

circuits/tests/utils/rsapss.test.ts (1)

33-33: LGTM: Path refactor aligns module resolution.

The update from relative to absolute node_modules paths is consistent across all test instantiations and aligns with the project-wide module resolution strategy.

Also applies to: 71-71, 104-104, 136-136, 166-166, 194-194, 225-225

circuits/tests/register/register.test.ts (1)

65-67: LGTM: Consistent module resolution update.

The path refactor to absolute node_modules references matches the project-wide standardization without affecting test logic.

circuits/tests/other_circuits/prove_country_is_not_in_list.test.ts (1)

24-26: LGTM: Module path standardization applied.

The include path update maintains consistency with the codebase-wide refactor to absolute node_modules references.

circuits/tests/utils/rsaPkcs1v1_5.test.ts (1)

40-40: LGTM: Consistent path resolution across test cases.

All three circuit instantiations now use absolute node_modules paths, aligning with the project-wide module resolution update.

Also applies to: 64-64, 86-86

circuits/tests/other_circuits/custom_hasher.test.ts (1)

26-28: LGTM: Module resolution standardized for both circuits.

Both circuit initializations now use absolute node_modules paths, maintaining consistency with the project-wide refactor.

Also applies to: 33-35

circuits/tests/register_id/register_id.test.ts (1)

62-64: LGTM: Final file completes the module path refactor.

The include path update to absolute node_modules references completes the consistent standardization across the test suite.


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.

@Vishalkulkarni45
Copy link
Collaborator Author

@remicolin ci are passing , going to merge it

@Vishalkulkarni45 Vishalkulkarni45 merged commit b3f93af into dev Oct 3, 2025
15 checks passed
@Vishalkulkarni45 Vishalkulkarni45 deleted the feat/serbia-dsc branch October 3, 2025 14:09
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.

3 participants