Add elgamal#617
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a new ChangesElGamal Module
Sequence Diagram(s)sequenceDiagram
participant Test as ElGamal.test.ts
participant Sim as ElGamalSimulator
participant Mock as MockElGamal.compact
participant Core as ElGamal.compact
Test->>Sim: encrypt(pk, value, r)
Sim->>Mock: circuits.pure.encrypt(pk, value, r)
Mock->>Core: ElGamal_encrypt(pk, value, r)
Core-->>Mock: Ciphertext
Mock-->>Sim: Ciphertext
Sim-->>Test: Ciphertext
Test->>Sim: assertDecryptsTo(ct, pk, ek, claimedValue)
Sim->>Mock: circuits.pure.assertDecryptsTo(ct, pk, ek, claimedValue)
Mock->>Core: ElGamal_assertDecryptsTo(...)
Core->>Core: ek_scalar = secretToScalar(ek)
Core->>Core: m = c2 - ek_scalar*c1
Core->>Core: assert m == g^claimedValue
Core-->>Mock: pass/throw
Mock-->>Sim: pass/throw
Sim-->>Test: pass/throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/src/crypto/test/ElGamal.test.ts (1)
33-33: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
constfor immutable binding.The
contractvariable is assigned once and never reassigned. Declare it withconstinstead ofletto clearly signal immutability.♻️ Proposed fix
-let contract: ElGamalSimulator; +const contract = new ElGamalSimulator();Then remove the assignment on line 36, which would now be redundant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/crypto/test/ElGamal.test.ts` at line 33, The contract variable is declared with let but is assigned only once and never reassigned, so it should be declared as const instead. Change the let keyword to const for the contract variable of type ElGamalSimulator, and then remove the redundant assignment statement on line 36 that would now be unnecessary since const bindings must be initialized at declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: The changelog entry on line 12 contains an unresolved placeholder
`#???` in the ElGamal module entry that needs to be replaced with the actual
GitHub issue or pull request number to make the release note traceable and
actionable. Locate the line containing "Add ElGamal module (#???)" and replace
the `#???` placeholder with the correct issue or PR reference number.
In `@contracts/src/crypto/ElGamal.compact`:
- Around line 187-193: The encryptPoint circuit function currently accepts a
zero value for the randomness parameter r, which creates a security
vulnerability by allowing plaintext exposure when r equals 0. Add an assertion
at the beginning of the encryptPoint function to validate that r is not zero,
similar to the existing identity check for pk. This guard should reject any
attempt to encrypt with zero randomness and will automatically protect all
functions that depend on encryptPoint (such as encrypt, addEncrypted,
subEncrypted, and rerandomize) from this vulnerability.
In `@contracts/src/crypto/test/mocks/MockElGamal.compact`:
- Around line 99-119: Remove the `pure` qualifier from the circuit declarations
in MockElGamal.compact to match the upstream ElGamal.compact signatures.
Specifically, change `export pure circuit` to `export circuit` for the three
assertion circuits: assertKeyPair, assertDecryptsToPoint, and assertDecryptsTo.
Then update ElGamalSimulator.ts to access these circuits correctly by replacing
`this.circuits.pure.assertKeyPair`, `this.circuits.pure.assertDecryptsToPoint`,
and `this.circuits.pure.assertDecryptsTo` with `this.circuits.assertKeyPair`,
`this.circuits.assertDecryptsToPoint`, and `this.circuits.assertDecryptsTo`
respectively.
---
Nitpick comments:
In `@contracts/src/crypto/test/ElGamal.test.ts`:
- Line 33: The contract variable is declared with let but is assigned only once
and never reassigned, so it should be declared as const instead. Change the let
keyword to const for the contract variable of type ElGamalSimulator, and then
remove the redundant assignment statement on line 36 that would now be
unnecessary since const bindings must be initialized at declaration.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dad3aa24-f757-430d-9e53-a11b8b4078d4
📒 Files selected for processing (9)
CHANGELOG.mdcontracts/package.jsoncontracts/src/crypto/ElGamal.compactcontracts/src/crypto/test/ElGamal.test.tscontracts/src/crypto/test/mocks/MockElGamal.compactcontracts/src/crypto/test/simulators/ElGamalSimulator.tscontracts/src/crypto/test/witnesses/ElGamalWitnesses.tspackage.jsonturbo.json
Fixes #606
Summary by CodeRabbit
New Features
Tests