Skip to content

Conversation

@Pratyush
Copy link
Member

Description

Needed for arkworks-rs/algebra#639, and is anyway the right thing to do.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@Pratyush Pratyush requested a review from a team as a code owner June 19, 2025 06:40
@Pratyush Pratyush requested review from Copilot, mmagician, weikengchen and z-tech and removed request for a team June 19, 2025 06:40
@Pratyush Pratyush enabled auto-merge June 19, 2025 06:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors how the “infinite” flag is computed for Short Weierstrass curve points by using point.is_zero() instead of a direct point.infinity field.

  • Replaces point.infinity with point.is_zero() in the generic SW impl
  • Makes the same change in the BLS12-specific SW allocation
  • This ensures consistency with the updated group API where the identity is checked via is_zero()

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/groups/curves/short_weierstrass/mod.rs Swapped point.infinity for point.is_zero()
src/groups/curves/short_weierstrass/bls12/mod.rs Updated the closure mapping to use g.is_zero()
Comments suppressed due to low confidence (1)

src/groups/curves/short_weierstrass/mod.rs:168

  • Add a unit test for AffineVar::new that allocates both a finite and the point-at-infinity case, verifying that point.is_zero() correctly drives the Boolean flag.
            let infinity = Boolean::constant(point.is_zero());

Comment on lines 68 to 73
let infinity = Boolean::new_variable(
ark_relations::ns!(cs, "inf"),
|| g1_prep.map(|g| g.infinity),
|| g1_prep.map(|g| g.is_zero()),
mode,
)?;
let g = AffineVar::new(x, y, infinity);
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider updating the namespace label ("inf") or the variable name infinity to something like is_zero or is_identity to align with the is_zero() check and reduce potential confusion.

Copilot uses AI. Check for mistakes.
@Pratyush Pratyush added this pull request to the merge queue Jun 19, 2025
Merged via the queue into master with commit 0c89b0f Jun 19, 2025
11 checks passed
@Pratyush Pratyush deleted the use-is-zero-for-allocating-infinity-flag branch June 19, 2025 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants