Skip to content

Conversation

@survived
Copy link
Contributor

@survived survived commented Jan 27, 2023

Closes #726

@tarcieri
Copy link
Member

tarcieri commented Jan 27, 2023

@survived can you rebase now that I landed #728?

I'd suggest getting rid of the inherent methods for ProjectivePoint::add and use overlapping Add impls for the different EquationAProperties ZSTs, similar to what was done for Double.

@survived
Copy link
Contributor Author

survived commented Jan 27, 2023

Sure! I just wanted to finish algorithms implementation first and see they it work fine. I'll mark PR as ready for review when I'm done :)

@tarcieri
Copy link
Member

FWIW you should be able to use the implementations @newpavlov mentioned in #726, e.g. https://github.com/RustCrypto/elliptic-curves/pull/218/files#diff-cf2092dccb9b015c6defd5f649e6449e52e4cef9b10c77e2a96d6201033c8232R170

@survived
Copy link
Contributor Author

If I'm not missing anything, all algos are already implemented, so I only left to rebase it now and use trait approach. But I'll keep in mind that there's another implementation, thanks!

@tarcieri
Copy link
Member

@survived yeah, rebase on master and copy-and-paste the implementations into impls for the respective traits (Add and Double)

Should be fairly straightforward

@survived
Copy link
Contributor Author

@tarcieri unfortunately, compiler doesn't allow me to implement the same trait twice for ProjectivePoint. Here's minimal example that reproduces this behavior.

@tarcieri
Copy link
Member

Aah, unfortunate.

Another option is moving the implementation to methods on the ZSTs and having e.g. Double call that.

@survived
Copy link
Contributor Author

survived commented Jan 30, 2023 via email

@survived
Copy link
Contributor Author

@tarcieri I came up with similar ZST-based approach, check it out. I added type PointArithmetic: PointArithmetic<C> associated type to the PrimeCurveParams trait, and new PointArithmetic<C> trait which basically defines double, add, and add_mixed functions. Then we don't actually need to change interface of ProjectivePoint, ProjectivePoint::{double, add, add_mixed} would just use PointArithmetic::{double, add, add_mixed}. What do you think?

@tarcieri
Copy link
Member

Looks good overall! That's more or less what I had in mind.

@survived
Copy link
Contributor Author

Do you want to keep Double trait, or shall I revert this change?

@tarcieri
Copy link
Member

I already added it to elliptic-curve, so seems OK to keep. I'll switch to that trait in #730

@survived survived force-pushed the primeorder/generic-a branch from 9714a5f to 963fcff Compare February 6, 2023 16:24
@survived survived force-pushed the primeorder/generic-a branch from 661f5a8 to 97908f4 Compare February 6, 2023 16:51
@survived survived marked this pull request as ready for review February 6, 2023 16:51
@survived
Copy link
Contributor Author

survived commented Feb 6, 2023

@tarcieri PR is ready for review!

@tarcieri tarcieri merged commit d4dbe19 into RustCrypto:master Feb 6, 2023
@tarcieri tarcieri mentioned this pull request Mar 3, 2023
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.

primeorder: supports only curves with a = -3

2 participants