search

Colfenor / classic-mceliece-rust

This is a pure-rust safe-rust implementation of the Classic McEliece post-quantum scheme
MIT License
24 stars 11 forks source link

refactor!: Re-write public API to take a `CryptoRng` #8

Closed dkales closed 2 years ago

dkales commented 2 years ago

This changes the public API to take a CryptoRng + RngCore instead of the included AES Rng used for the KATs. A lot of dependencies were moved to dev-dependencies, cleaning up the required deps.

BREAKING CHANGE: This changes the public API.

I only quickly tested against the 10 KATs provided by the round3 submission package for the 348864 version, that works, a full KAT test for all features should be done still (@prokls do you have that all set up locally? Otherwise I can do it later).

dkales commented 2 years ago

missed a few calls in other features, should be good now, all tests pass for all features

prokls commented 2 years ago

Awesome! Thanks a lot!

(@prokls do you have that all set up locally? Otherwise I can do it later).

Yes, there is no CI/CD setup, if that answers your question. I can run all tests very soon, yes. I am just busy with other stuff right now. If all tests pass, I will prepare a 2.0 release.

dkales commented 2 years ago

I would not release this as 2.0 yet, I have some other things I'm cleaning up right now as well (e.g., after the above changes there is no need for errors anymore, as all operations are actually infallible). I'll put them in another PR.

prokls commented 2 years ago

Alright, thanks for the feedback. Then let us keep this in a separate branch.

dkales commented 2 years ago

Since your 2 other projects also use this API, it would be good to also move them to this API? A good step would also probably to de-duplicate the NIST AES Rng by moving it to a separate crate, and depending on that crate as a dev-dependency. (ATM it is a bit janky also in this PR since it is duplicated as an internal test mod for both the lib and the example katkem program, and having it as a separate crate would also solve this and make it usable in the 2 other libs).

faern commented 2 years ago

As previously stated, please wait a bit with a 2.0 release. I have some more feedback on the API of this crate. Would be nice to have that looked at as well, so we don't need/want a 3.0 right away. I'll create separate issues for my thoughts.

faern commented 2 years ago

Really nice PR! :tada: I like the new API, it becomes easier to use correctly. My review was mostly nitpicks and some small suggestions. I would love to see this merged.

dkales commented 2 years ago

Can probably merge this into v2 or something?

faern commented 2 years ago

Would be nice to have this merged before the larger kem trait work is started. Both so the PR does not become too huge, and so that it becomes easier to test out the changes incrementally as a consumer of the library.