feat(ring): introduce RingElement API and bump version to 1.4.0 - #53
Conversation
- Add `RingElement<C>` for polynomial arithmetic in $R_q = \mathbb{Z}_q[X]/(X^N + 1)$ with dual-state (coefficient/NTT) representation and lazy conversion.
- Make `lumen_math::ring` module public for direct access.
- Update CHANGELOG, README, and tests to document and validate the new API.
- Bump crate version to `1.4.0`.
📝 WalkthroughWalkthroughThis PR adds a new public Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @Cargo.toml:
- Line 4: The Cargo.toml edition value is invalid ("2024"); update the edition
entry in Cargo.toml (the line setting edition = "...") to a supported Rust
edition such as "2021" (or "2018") so Cargo can build; modify the edition value
accordingly and save the file.
🧹 Nitpick comments (2)
src/ring/element.rs (2)
227-234: Consider restrictingdata_mutvisibility or adding safeguards.Exposing mutable access to raw data while maintaining internal state tracking is risky. A user could modify the data in ways that invalidate the
statefield (e.g., manually apply partial NTT, leaving data in an inconsistent state).Consider:
- Making this
pub(crate)if only needed internally- Adding a method that takes a closure and resets state to a known value after mutation
- Documenting specific safe usage patterns
272-286: Consider preserving NTT state in mixed-state addition.When adding NTT + Coefficient, the current implementation converts both to Coefficient form (lines 282-285). However, for consistency and potential performance benefits, you might consider always converting to NTT form instead, or at least documenting this choice.
Current behavior:
- Coefficient + Coefficient → Coefficient ✓
- NTT + NTT → NTT ✓
- Coefficient + NTT → Coefficient (converts RHS)
- NTT + Coefficient → Coefficient (converts LHS)
This asymmetry means the result state depends on operand order, which could be surprising.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGELOG.mdCargo.tomlREADME.mdTHEORY.mdsrc/lib.rssrc/ring/element.rssrc/ring/mod.rstests/ring_element_test.rs
🧰 Additional context used
🧬 Code graph analysis (2)
tests/ring_element_test.rs (1)
src/ring/element.rs (9)
make_ctx(491-493)new(76-87)zero(111-117)is_zero(237-239)state(133-135)degree(139-141)one(120-129)coefficients(199-206)from_ntt(97-108)
src/ring/element.rs (1)
tests/ring_element_test.rs (4)
make_ctx(9-11)test_subtraction(82-94)test_negation(97-105)test_scale(143-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (17)
Cargo.toml (1)
3-3: Version bump to 1.4.0 is appropriate.The minor version increment correctly reflects the addition of new public API (
RingElement<C>and the publicringmodule) without breaking existing functionality.THEORY.md (1)
252-268: Ring Elements documentation is clear and well-placed.The new subsection appropriately documents the dual-state design, lazy conversion strategy, and efficiency optimizations via shared
Arc<NttContext<C>>. Placement after the Negacyclic NTT section provides good contextual flow.CHANGELOG.md (1)
8-22: Changelog entry is well-structured and comprehensive.The 1.4.0 entry clearly documents the new
RingElement<C>API, its dual-state representation, arithmetic operations, and efficiency optimizations. Separating the new type features from the module-visibility improvement in distinct subsections follows best practices.README.md (1)
99-116: LGTM! Clear documentation with practical example.The Ring Elements section provides a good introduction to the API. The example demonstrates the key concepts: shared NTT context via
Arc, element construction, and automatic state conversion during arithmetic.src/ring/element.rs (6)
50-65: Well-designed struct with appropriate ownership semantics.The use of
Arc<NttContext<C>>for shared NTT context is appropriate for the use case where multiple ring elements share the same ring parameters. The dual-state design is clean.
76-87: LGTM! Proper validation in constructors.The assertion ensures data integrity by requiring the coefficient vector length to match the NTT context degree. This prevents silent bugs from mismatched dimensions.
149-193: Clean state conversion API with appropriate variants.The three conversion patterns (in-place, consuming, cloning) provide flexibility for different use cases. The no-op behavior when already in the target state is efficient.
450-479: LGTM! Standard polynomial display format.The Display implementation correctly shows polynomials in descending degree order with appropriate handling of the zero polynomial case.
485-610: Good unit test coverage.The tests cover the essential functionality including state conversions, arithmetic operations, and cross-state equality. The NTT roundtrip test is particularly important for verifying the correctness of state transformations.
420-435: Do not simplify usinglhs.data == rhs.data.The explicit call to
to_u1024()applies Montgomery reduction before comparison. UsingFieldElement'sPartialEqdirectly would compare raw values without this reduction, which could lead to incorrect equality checks if the values are stored in Montgomery form internally. The current implementation is correct and necessary.Likely an incorrect or invalid review comment.
src/lib.rs (2)
15-15: LGTM! Clean module integration.The new
ringmodule follows the established pattern of public module declarations in the crate root.
49-51: LGTM! Appropriate public re-exports.The re-exports of
RingElementandRingElementStateat the crate root level provide convenient access while maintaining the option to use the full pathlumen_math::ring::*for more explicit imports.src/ring/mod.rs (1)
1-8: LGTM! Clean module organization.The module structure is well-organized with clear documentation and appropriate re-exports.
tests/ring_element_test.rs (4)
13-62: Good coverage of construction and state conversion APIs.The tests thoroughly verify element construction, state transitions, and the important property that NTT roundtrips preserve coefficient values.
202-278: Excellent algebraic property tests.Testing commutativity and distributivity provides strong confidence that the ring operations are mathematically correct. These properties are fundamental to ring arithmetic and catch subtle implementation bugs.
280-297: LGTM! Critical edge case validation.These panic tests verify that the API correctly enforces invariants:
- Coefficient length must match context degree
- Operations require the same NTT context (via
Arc::ptr_eq)The
expectedstrings match the panic messages in the implementation.
185-200: Good test for mixed-state operations.This test verifies that adding elements in different states produces correct results, which is crucial for the lazy conversion feature.
| name = "lumen-math" | ||
| version = "1.3.0" | ||
| version = "1.4.0" | ||
| edition = "2024" |
There was a problem hiding this comment.
Invalid Rust edition "2024"—must be 2015, 2018, or 2021.
Rust does not have a 2024 edition. This will cause a cargo build failure. Correct this to a valid edition (typically "2021" for modern Rust).
🔎 Proposed fix
-edition = "2024"
+edition = "2021"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| edition = "2024" | |
| edition = "2021" |
🤖 Prompt for AI Agents
In @Cargo.toml at line 4, The Cargo.toml edition value is invalid ("2024");
update the edition entry in Cargo.toml (the line setting edition = "...") to a
supported Rust edition such as "2021" (or "2018") so Cargo can build; modify the
edition value accordingly and save the file.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
…actor addition/subtraction logic, and expand tests - Introduced `modify_data` method for safe mutation of polynomial coefficients, enforcing state consistency. - Refactored addition and subtraction methods to simplify RHS state conversion logic. - Expanded test coverage to validate edge cases, state transitions, format implementations, and API behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ring/element.rs (1)
237-256: Safe mutation design with minor doc clarification needed.The
modify_datamethod correctly ensures coefficient form before and after mutation, providing a safe API. However, the doc comment "resetting state afterward" (line 238) is slightly ambiguous—it might be clearer to say "ensuring coefficient form" since the state remains Coefficient rather than being "reset" to some default.Optional: Clarify doc comment
- /// Safely modifies the data using a closure, resetting state afterward. + /// Safely modifies the data using a closure, ensuring coefficient form. /// - /// After the mutation, the element is marked as being in coefficient form. + /// The element is converted to coefficient form before mutation and remains + /// in coefficient form afterward.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ring/element.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/ring/element.rs (1)
tests/ring_element_test.rs (4)
make_ctx(9-11)test_subtraction(82-94)test_negation(97-105)test_scale(143-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (8)
src/ring/element.rs (8)
1-65: Excellent module documentation and type design.The dual-state representation with lazy conversion is well-explained, and the use of
Arc<NttContext<C>>for shared context is an efficient design choice for lattice-based cryptography. The documentation clearly explains the purpose and usage patterns.
67-147: Well-designed constructors and accessors.Input validation with clear panic messages, appropriate use of
#[inline]for simple accessors, and correct initialization of special values (zero and one).
149-193: Efficient lazy state conversion implementation.The no-op checks (lines 153, 163) prevent unnecessary transformations, and providing multiple conversion strategies (in-place, consuming, and cloning) gives users flexibility based on their ownership needs.
258-271: Correct utility implementations.The
is_zeromethod works correctly in both states (since NTT is linear), andscaleappropriately preserves the current state as scalar multiplication is valid in both coefficient and NTT domains.
277-359: Correct cross-state arithmetic implementation.Addition and subtraction properly handle operands in different states by converting RHS to match LHS, preserving the LHS state in the result. The
Arc::ptr_eqcheck ensures context consistency, and the owned trait implementations appropriately delegate to the reference implementations.
361-424: Efficient multiplication using NTT.The multiplication implementation correctly leverages NTT for O(n log n) polynomial multiplication instead of O(n²), and returns the result in NTT form to avoid unnecessary inverse transforms. Negation correctly preserves state since NTT is a linear transformation.
430-489: Well-implemented comparison and formatting.Equality comparison correctly normalizes both operands to coefficient form for comparison, and the
Displayimplementation produces readable polynomial notation (highest degree first, with special handling for the zero polynomial).
495-952: Exemplary test coverage.The test suite is comprehensive and well-structured, covering constructors, state transitions, arithmetic operations (including cross-state scenarios), edge cases, panic conditions, accessors, formatting, and no-op optimizations. This thorough testing validates both the correctness and API usability of the implementation.
1 participant
RingElement<C>for polynomial arithmetic inlumen_math::ringmodule public for direct access.1.4.0.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.