-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat: make AffineRepr::xy() more flexible #593
Conversation
Hm can you leave a snippet of your code here? Would like to see an example use case. Also, I believe the curve traits require the underlying data to be owned (ie any type implementing the AffineRepr trait must not contain any non-static references) |
My actual code looks like this: pub struct G1Affine(blstrs::G1Affine);
impl AffineRepr for G1Affine {
…
fn xy(&self) -> Option<(Self::BaseField, Self::BaseField)> {
if self.0.is_identity().into() {
None
} else {
Some((Fp(self.0.x()), Fp(self.0.y())))
}
}
…
} What it shows: I indeed own the underlying data. Though I'm not able to return a reference. One thing is that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, could you just add an entry in the CHANGELOG
under the Breaking changes
section? Thanks!
Currently [`AffineRepr::xy()` returns the field elements as references]. This is only possible if the underlying data structure that implements that traits owns those field elements and can directly have a reference to it. Though there are cases, where providing those direct references is not possible. For example if the underlying structure has a different layout, or if the fields elements you're returning need to be wrapped in a newtype struct (that's my case). With this change, `AffineRepr::xy()` now returns owned data. It's more flexible as now the underlying data structure can have any shape as long as it can return those field elements somehow. [`AffineRepr::xy()` returns the field elements as references]: https://github.com/arkworks-rs/algebra/blob/4e4fc17e69178ab7f929602f17a5e769ebaf34c7/ec/src/lib.rs#L208
ab7284b
to
ba5ed7a
Compare
I've rebased it. If I should also squash the commits into a single one, let me know. |
I can handle that, don't worry about it! Thanks :) |
* feat: make AffineRepr::xy() more flexible Currently [`AffineRepr::xy()` returns the field elements as references]. This is only possible if the underlying data structure that implements that traits owns those field elements and can directly have a reference to it. Though there are cases, where providing those direct references is not possible. For example if the underlying structure has a different layout, or if the fields elements you're returning need to be wrapped in a newtype struct (that's my case). With this change, `AffineRepr::xy()` now returns owned data. It's more flexible as now the underlying data structure can have any shape as long as it can return those field elements somehow. [`AffineRepr::xy()` returns the field elements as references]: https://github.com/arkworks-rs/algebra/blob/4e4fc17e69178ab7f929602f17a5e769ebaf34c7/ec/src/lib.rs#L208 * chore: add changelog entry --------- Co-authored-by: Pratyush Mishra <[email protected]> Co-authored-by: mmagician <[email protected]>
This reverts commit 0dd47b3.
Description
I decided to open a PR instead of an issue, to also illustrate that the impact on the current code base won't be that big (besides it being a breaking change).
Currently
AffineRepr::xy()
returns the field elements as references. This is only possible if the underlying data structure that implements that traits owns those field elements and can directly have a reference to it.Though there are cases, where providing those direct references is not possible. For example if the underlying structure has a different layout, or if the fields elements you're returning need to be wrapped in a newtype struct (that's my case).
With this change,
AffineRepr::xy()
now returns owned data. It's more flexible as now the underlying data structure can have any shape as long as it can return those field elements somehow.closes: #XXXX
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.
Pending
section inCHANGELOG.md
Files changed
in the GitHub PR explorer