Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add mul to ModelParam traits #396

Merged
merged 4 commits into from
Mar 17, 2022

Conversation

zhenfeizhang
Copy link
Contributor

@zhenfeizhang zhenfeizhang commented Mar 8, 2022

Description

implement part of the solution suggested via
arkworks-rs/curves#66 (comment)


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

@zhenfeizhang zhenfeizhang marked this pull request as ready for review March 8, 2022 15:04
@Pratyush
Copy link
Member

Pratyush commented Mar 8, 2022

@zhenfeizhang Thanks! do you mind adding undoing the formatting changes? (I think a git reset + git add of just the model parameters file should work)

Also, I think @alexander-zw was also looking into similar things.

CHANGELOG.md Outdated Show resolved Hide resolved
base: &short_weierstrass_jacobian::GroupProjective<Self>,
scalar: &Self::ScalarField,
) -> short_weierstrass_jacobian::GroupProjective<Self> {
base.mul(scalar.into_bigint())
Copy link
Member

Choose a reason for hiding this comment

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

What I had in mind was actually something even more aggressive: to move the actual mul code here, and call P::mul from GroupProjective::mul. That is, the impl of this method in GroupProjective changes to

fn mul(...) {
    P::mul(...)
}

Also, we might want to add both mul_projective and mul_affine methods here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried your suggestion.

  • we cannot add it directly to ModelParameter: we need to specify if it is TE or SW
  • we also cannot directly invoke it from AffineCurve or ProjectiveCurve, the trait is bounded by ModelParameter that does not specify TE or SW

working on the rest of the changes now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a workaround is to add an associated type for ModelParameters but it will be ugly and is a breaking change

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, should have been clearer, I meant that in the current strategy, instead of calling base.mul(...) inside SWModelParameters, we should change the implementation as follows:

// inside SW/TEModelParameters:
pub trait SW/TEModelParameters {
	fn mul_projective(g: GroupProjective<Self>, scalar: ...) -> GroupProjective<Self> {
	    // default double-and-add impl
	} 
}

// inside `ProjectiveCurve`:
pub trait ProjectiveCurve {
	fn mul(...); // <- no default impl anymore
}

// inside sw/te::GroupProjective:
impl ProjectiveCurve for GroupProjective {
    ...
	
	fn mul() {
		P::mul_projective(...)
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you. Should be fixed now.

@Pratyush Pratyush changed the title add mul fn to ModelParam traits add mul to ModelParam traits Mar 17, 2022
@Pratyush Pratyush merged commit de9758b into arkworks-rs:master Mar 17, 2022
@zhenfeizhang zhenfeizhang deleted the glv-param-for-ark-ec branch March 17, 2022 20:18
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.

2 participants