Conversation
0xAndoroid
left a comment
0xAndoroid
left a comment
There was a problem hiding this comment.
Code review — 11 issues found (4 high, 4 medium, 3 low). Arithmetic is sound; Barrett/Montgomery reductions, wide accumulator, and precomputed tables are correct. Issues are API/type-safety concerns. See inline comments.
0xAndoroid
force-pushed
the
jolt-v2/jolt-field
branch
from
0758ae8 to
1b15876
Compare
0xAndoroid
force-pushed
the
jolt-v2/jolt-field
branch
from
74305c7 to
17e1d5d
Compare
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a spec-driven workflow. If this PR is a bug fix, refactor, or doesn't warrant a spec, feel free to ignore this message. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a spec-driven workflow. If this PR is a bug fix, refactor, or doesn't warrant a spec, feel free to ignore this message. |
0xAndoroid
force-pushed
the
jolt-v2/jolt-field
branch
from
329ede7 to
6f82e1f
Compare
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a spec-driven workflow. If this PR is a bug fix, refactor, or doesn't warrant a spec, feel free to ignore this message. |
0xAndoroid
force-pushed
the
jolt-v2/jolt-field
branch
from
6f82e1f to
15bbf97
Compare
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a spec-driven workflow. If this PR is a bug fix, refactor, or doesn't warrant a spec, feel free to ignore this message. |
- Fix negative-zero bug in From<i64/i128> for S96/S160 (val.is_positive() → val >= 0) - Add manual PartialEq for SignedBigIntHi32 that treats ±0 as equal (Ord/PartialEq contract) - Inline sub_assign_in_place to avoid unnecessary copy via Neg - Upgrade debug_assert! to assert! in magnitude_as_limbs_nplus1 and to_signed_bigint_nplus1 - Replace unsafe generic From<S224> for Limbs<N> with concrete From<S224> for Limbs<4> - Restrict fmadd_trunc and bn254_ops functions to pub(crate) visibility - Replace From<BigInt>/Into<BigInt> trait impls on Limbs with pub(crate) inherent methods - Remove Option wrapper from Fr::from_bigint_unchecked
Tests and bench use Fr which requires the bn254 feature. Without the gate, `cargo clippy --no-default-features --all-targets` fails.
- Fix SignedBigInt PartialEq/Ord inconsistency: +0 and -0 now compare equal, matching the Ord impl (consistent with SignedBigIntHi32) - Remove ark_serialize impls from Limbs, SignedBigInt, SignedBigIntHi32 (unused outside tests) - Move BigInt<->Limbs conversion from limbs.rs into arkworks/mod.rs as From impls, eliminating all arkworks references from limbs.rs and signed/ modules - Delete dead code: fmadd_trunc, R2, mac_no_carry, from_barrett_reduce, mul_by_hi_2limbs, mul_u64_accumulate - Comply with workspace lints (allow_attributes, unwrap_used, expect_used)
0xAndoroid
force-pushed
the
jolt-v2/jolt-field
branch
from
15bbf97 to
327de42
Compare
The previous dedup checked for the no-spec label, which races with the label job. Check for an existing warning comment instead.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
- Remove unused `digest` workspace dependency - Remove non-canonical `authors` field from jolt-field - Merge `arkworks` feature into `bn254` (single feature)
0xAndoroid
force-pushed
the
jolt-v2/jolt-field
branch
from
9ebe5ef to
483c23d
Compare
Benchmark comparison (crates) |
2 participants
Summary
Changes
Testing
cargo clippyandcargo fmtpassSecurity Considerations
Breaking Changes
None