Conversation
Add new conventions to ARCHITECTURE.md - dependencies / supply chain risk - dynamic memory management - constant-time and side-channel resistance
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the ARCHITECTURE.MD document by formalizing crucial development conventions for the Constantine project. It establishes clear guidelines for managing external dependencies to enhance supply chain security, outlines strict policies for dynamic memory allocation to safeguard sensitive data, and details robust strategies for implementing constant-time operations and mitigating side-channel attack risks in cryptographic routines. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📝 WalkthroughWalkthroughThe ARCHITECTURE.md document is expanded and reorganized: "APIs" is renamed to "APIs & Conventions" and new sections cover Dependencies, Argument ordering, Return values, Memory allocation policies, and Constant-time/side-channel resistance details. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds important architectural conventions to the ARCHITECTURE.md file regarding dependencies, memory management, and resistance to side-channel attacks. The new sections are well-written and provide valuable guidance for developers. My review includes a few minor suggestions to correct some typos in the documentation to improve its quality.
ARCHITECTURE.md
Outdated
| - std/atomics | ||
| - Anything used at compile-time only | ||
| - std/macros | ||
| - std/os and std/strutils to create compiletime paths relative to the currentSourcePath |
There was a problem hiding this comment.
There's a small typo here. compiletime should be compile-time for better readability and consistency with how such terms are usually written.
| - std/os and std/strutils to create compiletime paths relative to the currentSourcePath | |
| - std/os and std/strutils to create compile-time paths relative to the currentSourcePath |
ARCHITECTURE.md
Outdated
| ### Constant-time and side-channel resistance | ||
|
|
||
| Low-level operations that can handle secrets SHOULD be constant-time by default. | ||
| Constantine assumes that addition, substraction (including carries/borrows), multiplication, bit operations, shifts are constant-time in hardware. |
There was a problem hiding this comment.
There's a typo in substraction. It should be subtraction.
| Constantine assumes that addition, substraction (including carries/borrows), multiplication, bit operations, shifts are constant-time in hardware. | |
| Constantine assumes that addition, subtraction (including carries/borrows), multiplication, bit operations, shifts are constant-time in hardware. |
ARCHITECTURE.md
Outdated
|
|
||
| We use Nim's effect tracking `{.tags[VarTime].}` so we can ensure that in protocols that handle secrets it is a compile-time error to have VarTime anywhere in the call stack via `{.tags:[].}`. | ||
|
|
||
| High-level protocols or subroutines for high-level protocols do not need this `_vartime` suffix hence this mostly concerns bigint, fields, elliptic curves adn polynomial arithmetic. |
There was a problem hiding this comment.
There's a typo here. adn should be and.
| High-level protocols or subroutines for high-level protocols do not need this `_vartime` suffix hence this mostly concerns bigint, fields, elliptic curves adn polynomial arithmetic. | |
| High-level protocols or subroutines for high-level protocols do not need this `_vartime` suffix hence this mostly concerns bigint, fields, elliptic curves and polynomial arithmetic. |
Greptile SummaryThis PR is a documentation-only update to Key additions:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Protocol Operation] --> B{Handles Secrets?}
B -- Yes --> C[Avoid Dynamic Allocation]
B -- No --> D[Custom Allocators\nconstantine/platforms/allocs.nim]
C --> E{Low-level op?}
E -- Yes --> F[Use SecretWord / SecretBool\nct_routines.nim]
F --> G{Compiler may introduce branches?}
G -- Yes --> H[Assembly path\nx86-64 / ARM64]
G -- No --> I[Nim implementation]
E -- No --> J[High-level protocol\nno _vartime suffix needed]
H --> K{Variable-time variant needed?}
I --> K
K -- Yes --> L[Suffix with _vartime\nTagged with VarTime effect]
K -- No --> M[Default constant-time path]
L --> N[Blocked at compile-time\nin secret-handling call stacks]
M --> O{Table access?}
O -- Yes --> P[Use secretLookup or ccopy\nfor uniform cache access]
O -- No --> Q[Safe to use]
Last reviewed commit: a90eaaf |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ARCHITECTURE.md
Outdated
| ### Constant-time and side-channel resistance | ||
|
|
||
| Low-level operations that can handle secrets SHOULD be constant-time by default. | ||
| Constantine assumes that addition, substraction (including carries/borrows), multiplication, bit operations, shifts are constant-time in hardware. |
There was a problem hiding this comment.
Typo: "substraction" should be "subtraction"
substraction is a misspelling of "subtraction".
| Constantine assumes that addition, substraction (including carries/borrows), multiplication, bit operations, shifts are constant-time in hardware. | |
| Constantine assumes that addition, subtraction (including carries/borrows), multiplication, bit operations, shifts are constant-time in hardware. |
ARCHITECTURE.md
Outdated
|
|
||
| Some primitives may have a variable time optimization (for example field inversion or elliptic curve scalar multiplication) or may be variable-time only (for example multi-scalar multiplication), in that case they are suffixed `_vartime`. | ||
|
|
||
| We use Nim's effect tracking `{.tags[VarTime].}` so we can ensure that in protocols that handle secrets it is a compile-time error to have VarTime anywhere in the call stack via `{.tags:[].}`. |
There was a problem hiding this comment.
Missing colon in effect-tracking pragma
{.tags[VarTime].} is missing the colon after tags. The correct Nim pragma syntax — as used consistently throughout the actual codebase (e.g., in constantine/platforms/intrinsics/extended_precision_vartime.nim) — is {.tags:[VarTime].}.
| We use Nim's effect tracking `{.tags[VarTime].}` so we can ensure that in protocols that handle secrets it is a compile-time error to have VarTime anywhere in the call stack via `{.tags:[].}`. | |
| We use Nim's effect tracking `{.tags:[VarTime].}` so we can ensure that in protocols that handle secrets it is a compile-time error to have VarTime anywhere in the call stack via `{.tags:[].}`. |
ARCHITECTURE.md
Outdated
|
|
||
| We use Nim's effect tracking `{.tags[VarTime].}` so we can ensure that in protocols that handle secrets it is a compile-time error to have VarTime anywhere in the call stack via `{.tags:[].}`. | ||
|
|
||
| High-level protocols or subroutines for high-level protocols do not need this `_vartime` suffix hence this mostly concerns bigint, fields, elliptic curves adn polynomial arithmetic. |
There was a problem hiding this comment.
Typo: "adn" should be "and"
adn is a misspelling of "and".
| High-level protocols or subroutines for high-level protocols do not need this `_vartime` suffix hence this mostly concerns bigint, fields, elliptic curves adn polynomial arithmetic. | |
| High-level protocols or subroutines for high-level protocols do not need this `_vartime` suffix hence this mostly concerns bigint, fields, elliptic curves and polynomial arithmetic. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Line 111: Fix two typos in the ARCHITECTURE.md security guidance: replace the
word "substraction" with "subtraction" and replace "adn" with "and" (search for
the exact tokens "substraction" and "adn" to locate the occurrences in the
conventions section and update them accordingly).
- Line 122: The Nim pragma example for VarTime is using malformed syntax
`{.tags[VarTime].}`; update the example to the correct Nim pragma format
`{.tags:[VarTime].}` so it matches all other usages and is safe to
copy/paste—specifically replace the occurrence of `{.tags[VarTime].}` with
`{.tags:[VarTime].}` in the VarTime/example line and ensure the brackets and
colon are exactly as shown.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ARCHITECTURE.md (1)
75-75:⚠️ Potential issue | 🟡 MinorFix spelling: "Guarenteed" → "Guaranteed"
The abbreviation GCE is misspelled as "Guarenteed Copy Elision" when it should be "Guaranteed Copy Elision".
📝 Proposed spelling fix
- - And they are struggling with GCE (Guarenteed Copy Elision) and NRVO/RVO(Named) Return Value Optimization + - And they are struggling with GCE (Guaranteed Copy Elision) and NRVO/RVO(Named) Return Value Optimization🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` at line 75, Replace the misspelled phrase "Guarenteed Copy Elision" with the correct spelling "Guaranteed Copy Elision" where it appears (specifically in the text "GCE (Guarenteed Copy Elision) and NRVO/RVO(Named) Return Value Optimization") so the line reads "GCE (Guaranteed Copy Elision) and NRVO/RVO (Named) Return Value Optimization".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Line 106: The pragma spacing is inconsistent: change the `{.tags: [].}`
instance to match the established `{.tags:[...].}` style used elsewhere (e.g.,
`{.tags:[HeapAlloc].}` and `{.tags:[Alloca].}`) so all occurrences use no space
after `tags:`; locate the `{.tags: [].}` snippet in the ARCHITECTURE.md text and
remove the space to make it `{.tags:[].}`.
- Line 122: The pragma syntax is inconsistent: update the occurrence of `{.tags:
[VarTime].}` so it matches the project's `{.tags:[].}` style by removing the
space after the colon (change to `{.tags:[VarTime].}`), and scan for other
`{.tags: ...}` occurrences (e.g., `{.tags:[].}`) to ensure all use the
`{.tags:[...].}` format for consistency across the doc.
---
Outside diff comments:
In `@ARCHITECTURE.md`:
- Line 75: Replace the misspelled phrase "Guarenteed Copy Elision" with the
correct spelling "Guaranteed Copy Elision" where it appears (specifically in the
text "GCE (Guarenteed Copy Elision) and NRVO/RVO(Named) Return Value
Optimization") so the line reads "GCE (Guaranteed Copy Elision) and NRVO/RVO
(Named) Return Value Optimization".
Corrected spelling of 'Guaranteed' and adjusted formatting in multiple sections of the architecture document.
1 participant
Add new conventions to ARCHITECTURE.md
Summary by CodeRabbit