refactor: add polynomial crate [skip-line-limit]#1195
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
Contributor
📝 WalkthroughWalkthroughThis pull request integrates the external Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
cedoor
changed the title
cedoor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@crates/polynomial/benches/polynomial.rs`:
- Line 1: Add a single-line SPDX header comment at the very top of
crates/polynomial/benches/polynomial.rs (above the existing use
criterion::{black_box, criterion_group, criterion_main, Criterion}; line) using
the repository's SPDX identifier (for example "SPDX-License-Identifier:
Apache-2.0" or the project's chosen identifier) so the license check passes;
ensure the header is the first line in the file.
In `@crates/polynomial/Cargo.toml`:
- Around line 6-7: Update the Cargo.toml "repository" field so it points to a
valid repository URL for crates.io metadata rather than the current non-standard
GitHub path; edit the repository entry in crates/polynomial/Cargo.toml (the
repository key) to either the repo root (e.g.,
https://github.com/gnosisguild/enclave) or a proper tree link for the crate
directory (e.g.,
https://github.com/gnosisguild/enclave/tree/main/crates/polynomial).
In `@crates/polynomial/README.md`:
- Around line 22-25: The fenced code block showing the polynomial representation
in the README is missing a language tag (triggers MD040); update the
triple-backtick fence that surrounds "a_n * x^n + a_{n-1} * x^{n-1} + ... + a_1
* x + a_0" to include a language identifier (e.g., "text") so the fence becomes
```text ... ```, ensuring markdownlint passes.
- Line 78: Replace the non‑standard space after the heading hashes in the
"### Quick Start" line with a standard ASCII space so the heading reads "###
Quick Start" (ensure the three hashes are followed by U+0020, not a non‑breaking
or other Unicode space); locate the heading text "### Quick Start" in README.md
and update the spacing to satisfy markdownlint MD018.
In `@crates/polynomial/src/errors.rs`:
- Around line 1-2: Add the project's standard SPDX license header at the very
top of the file errors.rs (module polynomial::errors) so the file starts with
the required header comment; update the header exactly to match the project's
standard SPDX text used across the repository and ensure there is a blank line
after the header before the existing module doc comment //! Error types for
polynomial operations.
In `@crates/polynomial/src/lib.rs`:
- Line 1: Add the repo-standard SPDX identifier as the very first line of the
file before the existing module doc comment (the line starting with "//! #
Polynomial Library"); e.g. insert a single-line comment like "//
SPDX-License-Identifier: <REPO_STANDARD>" at the top so the SPDX header check
passes.
In `@crates/polynomial/src/polynomial.rs`:
- Line 1: Add the required SPDX license identifier as a file header in
polynomial.rs: insert a single-line comment like "// SPDX-License-Identifier:
<LICENSE-ID>" (replacing <LICENSE-ID> with the project's actual SPDX identifier)
at the very top of the file before the existing module doc comment (the line
containing "//! Polynomial arithmetic implementation."). Ensure the SPDX line
exactly matches the project's license string used across the repository.
- Around line 139-147: Update the documentation for Polynomial::degree to match
the current implementation: state that degree returns
self.coefficients.len().saturating_sub(1) (i.e., the highest exponent implied by
the coefficient vector) and that a coefficients vector of length n (including
all-zero vectors like Polynomial::zero(n)) yields degree n-1; alternatively, if
you want trimming behavior instead, normalize/trim trailing zeros from
self.coefficients (e.g., in Polynomial::new or Polynomial::zero) before
computing degree in degree() — reference the degree method, the coefficients
field, and Polynomial::zero in your change.
- Around line 31-55: The Display implementation skips printing a leading '-'
when the first non-zero coefficient is negative; update the sign handling in the
loop inside the Display impl (the variables/logic around first, coeff,
abs_coeff) so the sign is emitted for the leading term: if first and coeff < 0
write "-" (no surrounding spaces) before printing abs_coeff, else keep the
existing " + " or " - " branches for subsequent terms; ensure first is set to
false after handling the sign so the rest of the formatting (abs_coeff and
degree handling) remains unchanged.
- Around line 176-183: The trim_leading_zeros method currently uses repeated
remove(0) causing O(n²) behavior and can leave coefficients empty; change it to
an O(n) approach: find the index of the first non-zero coefficient (using
iterator like position with |c.is_zero()|), then shift/slice the Vec once (e.g.,
drain the prefix or use split_off/overwrite) to drop leading zeros in one
operation; finally, ensure the method normalizes the empty/ all-zero case to a
single zero coefficient (replace an empty coefficients Vec with vec![T::zero()]
or equivalent) so the doc note holds. Reference: trim_leading_zeros and the
coefficients field.
- Around line 286-314: The div implementation (Polynomial::div) assumes exact
integer division by using &remainder[i] / &divisor.coefficients[0], which fails
when the divisor is not monic or the leading terms are not divisible; update the
method to first enforce or detect this: either require the divisor leading
coefficient to be ±1 (check divisor.coefficients[0] == 1 or -1) or, if allowing
arbitrary integer leading coefficients, before assigning coeff compute and
verify exact divisibility (e.g., check remainder[i] % divisor.coefficients[0] ==
0) and if not divisible return an error (use PolynomialError::InvalidPolynomial
with a clear message or add a new error variant); apply this check where coeff
is computed (the loop that writes quotient[i] and updates remainder) and ensure
the function returns an error instead of producing an incorrect remainder when
divisibility doesn't hold.
In `@crates/polynomial/src/utils.rs`:
- Around line 132-142: reduce_in_ring currently uses mem::take on coefficients
before calling reduce_by_cyclotomic, which clears the caller's data on error;
instead clone the input coefficients and construct the Polynomial from that
clone (or otherwise avoid mem::take), call reduce_by_cyclotomic on the cloned
polynomial, and only replace *coefficients with reduced.coefficients after a
successful result; ensure reduce_and_center_coefficients_mut is only invoked
after assignment and return the original error unchanged on failure so the
caller's coefficients are preserved.
- Line 1: Add a single-line SPDX license identifier comment at the very top of
utils.rs to satisfy the license check (e.g., // SPDX-License-Identifier: MIT),
ensuring the identifier matches the repository's chosen license; place it above
the existing module doc comment (//! Utility functions for polynomial
operations.) so the file begins with the SPDX header.
- Around line 99-218: The modulo logic is wrong for values < -p; update
reduce_scalar, reduce_coefficients, and reduce_coefficients_mut to first compute
r = x % p (or coeff % p), then if r is negative add p (i.e. if r < 0 { r += p })
so the result is in [0,p). In reduce_scalar replace (x + modulus) % modulus with
computing x % modulus then adding modulus when negative; in reduce_coefficients
map each coeff to this corrected routine; in reduce_coefficients_mut assign each
element to the corrected r (compute r = coeff % p, if r < 0 set r += p, then
store r). This keeps reduce_coefficients_2d/_3d correct because they call
reduce_coefficients.
🧹 Nitpick comments (1)
crates/polynomial/Cargo.toml (1)
10-23: Gatenum-bigint’s serde feature behind the crate’s serde feature.
Right now serde support is effectively always on vianum-bigint’s feature flag, even when this crate’sserdefeature is off. That undermines optionality and can add unwanted deps. Consider moving the feature linkage into[features].♻️ Suggested change
-[dependencies] -num-bigint = { workspace = true, features = ["serde"] } +[dependencies] +num-bigint = { workspace = true } [features] default = [] -serde = ["dep:serde"] +serde = ["dep:serde", "num-bigint/serde"] bincode = ["dep:bincode", "serde"]In Cargo.toml, how do you enable a dependency feature only when a crate feature is enabled (e.g., num-bigint/serde behind the crate’s serde feature)?
cedoor
force-pushed
the
refactor/bigint-poly
branch
from
6e4ddf4 to
5e53a29
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/polynomial/src/utils.rs`:
- Around line 343-351: The current unit test for reduce_coefficients is passing
by chance; add a new test (e.g., test_reduce_coefficients_large_negative) that
uses coefficients less than -modulus, for example vec![BigInt::from(-10),
BigInt::from(-15)] with modulus BigInt::from(7) and assert the normalized
results are vec![4,6]; if the test fails, fix reduce_coefficients to correctly
normalize negatives for all magnitudes by computing ((coeff % modulus) +
modulus) % modulus (or using BigInt's mod_floor) so values below -modulus are
reduced correctly.
♻️ Duplicate comments (5)
crates/polynomial/README.md (1)
27-29: Add a language specifier to the code fence.The fenced code block for polynomial representation is missing a language tag, which triggers markdownlint MD040.
Proposed fix
-``` +```text a_n * x^n + a_{n-1} * x^{n-1} + ... + a_1 * x + a_0</details> </blockquote></details> <details> <summary>crates/polynomial/Cargo.toml (1)</summary><blockquote> `7-7`: **Repository URL format is invalid for GitHub.** The URL `https://github.com/gnosisguild/enclave/crates/polynomial` is not a valid GitHub path. Use either the repository root or a tree link. <details> <summary>Proposed fix</summary> ```diff -repository = "https://github.com/gnosisguild/enclave/crates/polynomial" +repository = "https://github.com/gnosisguild/enclave/tree/main/crates/polynomial"crates/polynomial/src/utils.rs (3)
164-166: Same modulo reduction bug asreduce_scalar.
(coeff + p) % pfails for coefficients less than-p.Proposed fix
pub fn reduce_coefficients(coefficients: &[BigInt], p: &BigInt) -> Vec<BigInt> { - coefficients.iter().map(|coeff| (coeff + p) % p).collect() + coefficients + .iter() + .map(|coeff| { + let mut r = coeff % p; + if r < BigInt::zero() { + r += p; + } + r + }) + .collect() }
218-223: Same modulo reduction bug in mutable variant.The in-place version has the same issue with values less than
-p.Proposed fix
pub fn reduce_coefficients_mut(coefficients: &mut [BigInt], p: &BigInt) { for coeff in coefficients.iter_mut() { - *coeff += p; - *coeff %= p; + let mut r = &*coeff % p; + if r < BigInt::zero() { + r += p; + } + *coeff = r; } }
114-116: Modulo reduction fails for values less than-modulus.
(x + modulus) % modulusonly produces results in[0, modulus)whenx ∈ [-modulus, modulus). Forx < -modulus, the result remains negative.Example:
x = -10,modulus = 7→(-10 + 7) % 7 = -3(incorrect, expected4).Use the pattern from
reduce_and_center(lines 31-34).Proposed fix
pub fn reduce_scalar(x: &BigInt, modulus: &BigInt) -> BigInt { - (x + modulus) % modulus + let mut r = x % modulus; + if r < BigInt::zero() { + r += modulus; + } + r }
🧹 Nitpick comments (1)
crates/polynomial/src/polynomial.rs (1)
343-346: Consider optimizing remainder trimming to O(n).Similar to the
trim_leading_zerosoptimization, this loop usingremove(0)is O(n²). While the remainder is typically small, the same iterator-based approach could be used for consistency.Proposed fix
// Remove leading zero coefficients from remainder - while !remainder.is_empty() && remainder[0].is_zero() { - remainder.remove(0); - } + if let Some(first_non_zero) = remainder.iter().position(|c| !c.is_zero()) { + remainder.drain(..first_non_zero); + } else if !remainder.is_empty() { + remainder.clear(); + }
- Replace repeated remove(0) calls with single drain operation - Use iterator position to find first non-zero coefficient - Normalize all-zero case to single zero coefficient - Add comprehensive tests for edge cases
- Replace mem::take with clone to avoid clearing caller's data on error - Only assign reduced coefficients after successful cyclotomic reduction - Remove unused std::mem import
- Add COPY command for crates/polynomial/Cargo.toml - Fixes Docker build failure when resolving workspace members
- Check that remainder coefficients are divisible by divisor leading coefficient - Return InvalidPolynomial error when exact divisibility is not satisfied - Prevent incorrect results from truncated integer division - Update documentation to reflect new error condition
- Add COPY instruction for polynomial benches directory - Fixes Docker build error where Cargo validates bench targets before source files are copied
- Update reduce_scalar to compute r = x % modulus, then add modulus if r < 0 - Update reduce_coefficients to use corrected modulo logic for each coefficient - Update reduce_coefficients_mut to compute r = coeff % p, then add p if r < 0 - Add comprehensive tests for edge cases including values < -p - Ensure all results are in range [0, modulus)
cedoor
force-pushed
the
refactor/bigint-poly
branch
from
7b1b045 to
2fb41bb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/polynomial/src/polynomial.rs`:
- Around line 343-348: The remainder normalization loop should be replaced with
the canonical-trimming helper to avoid O(n²) removals and to ensure empty
remainders become the canonical zero representation; remove the while
!remainder.is_empty() { remainder.remove(0); } loop and call the existing
trim_leading_zeros helper on the remainder (e.g., trim_leading_zeros(&mut
remainder) or remainder.trim_leading_zeros()) before constructing the returned
Polynomial::new(remainder).
♻️ Duplicate comments (1)
crates/polynomial/README.md (1)
27-29: Add a language tag to the polynomial representation fence (markdownlint MD040).Still missing a fence language; markdownlint will keep failing.
🛠️ Proposed fix
-``` +```text a_n * x^n + a_{n-1} * x^{n-1} + ... + a_1 * x + a_0</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.husky/pre-commit:
- Line 1: Add a shell shebang and source the Husky shim at the top of the
pre-commit hook so the hook runs as a script and Husky's environment is
initialized before executing the existing pnpm lint-staged command; update the
pre-commit hook (the script that currently contains only "pnpm lint-staged") to
start with a proper shebang (e.g., /usr/bin/env sh), source the Husky
shim/initializer (the _/husky.sh loader that lives next to the hook), then run
pnpm lint-staged, and ensure the hook file is executable.
In @.husky/pre-push:
- Around line 1-3: The pre-push hook lacks a shebang and Husky shim, causing
exec failures; prepend a shell shebang and source Husky’s shim before the
existing commands so the hook runs under a POSIX shell and Husky environment.
Edit the .husky/pre-push file to add a first line like #!/usr/bin/env sh and
immediately source the Husky helper (e.g., . "$(dirname "$0")/_/husky.sh")
before the existing pnpm lint / pnpm check:pnpm / pnpm check:license commands so
the hook executes correctly.
In `@packages/enclave-contracts/package.json`:
- Around line 163-165: The package.json "lint" script currently only runs
solhint ("lint") so CI target pnpm evm:lint skips formatting checks; update the
fix by either extending the "lint" script to run both solhint and the existing
"prettier:check" (e.g., run "prettier:check" before/after solhint) or add a new
CI step that runs the "prettier:check" script directly; modify the package.json
scripts section to reference "prettier:check" alongside "lint" (or add a
separate CI format-check job) so formatting regressions are caught.
♻️ Duplicate comments (1)
crates/polynomial/src/polynomial.rs (1)
379-384: Use canonical trimming for remainder (avoid O(n²) remove loop).Line 379–Line 384: The
remove(0)loop is O(n²) and can leave a non-canonical empty remainder. Usetrim_leading_zerosto normalize.🛠️ Proposed fix
- // Remove leading zero coefficients from remainder - while !remainder.is_empty() && remainder[0].is_zero() { - remainder.remove(0); - } - - Ok((Polynomial::new(quotient), Polynomial::new(remainder))) + let remainder = Polynomial::new(remainder).trim_leading_zeros(); + Ok((Polynomial::new(quotient), remainder))
0xjei
approved these changes
Jan 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1193
⸻
bigint-polycrate from external repository to Enclave monorepo ascrates/polynomialRecent improvements:
polynomial.rs:324-331)reduce_in_ringto preserve coefficients on error instead of clearing caller's data (utils.rs:137-148)polynomial.rs:46-48)trim_leading_zerosfrom O(n²) to O(n) using iterator-based approach (polynomial.rs:192-199)Dockerfile:66)Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.