RustCrypto / PAKEs

Password-Authenticated Key Agreement protocols
105 stars 34 forks source link

Fix `clippy` warnings and bump `srp` and `aucpace` MSRV to `1.61` #144

Closed brxken128 closed 1 year ago

brxken128 commented 1 year ago

While playing around in the workspace (SPAKE2 specifically), I noticed that there were a tonne of clippy warnings so I decided to fix them all.

My only concern is the silencing of write via #[allow(clippy::unused_io_amount)] - I wonder if changing all of those writes to write_all would produce more-than-negligible performance losses. These were changed to write_all for correctness.

This changes the MSRV of srp and aucpace from 1.60.0 to 1.61.0 otherwise they won't build with some const fns (#93706 was added in 1.61.0)

tarcieri commented 1 year ago

cc @tritoke

tritoke commented 1 year ago

Thanks for fixing these, I got quite busy writing my dissertation towards the end of finishing this so it's nice that it's been tidied up!

All of the .write calls should be .write_all, they are only in examples so it's never failed for me in testing but it's always worth using the right method :)

newpavlov commented 1 year ago

It could be worth to also bump Clippy version in our CI job and if there are warnings in other crates, it would be nice to fix them as well.

brxken128 commented 1 year ago

Will draft this temporarily and get all of that done!

tarcieri commented 1 year ago

@brxken128 you can bump aucpace MSRV if need be

brxken128 commented 1 year ago

@brxken128 you can bump aucpace MSRV if need be

I think I broke builds anyway, probably removing the wrong pub(crate). Shouldn't be too hard to fix. I missed out --all-features.

I'll see what the new MSRV should be, from a glance 1.65.0 looks suitable but I'm not 100%.

Edit: 1.64.0 and 1.63.0 work, I forgot what version things were stabilized in so it's just trial-and-error now haha

Further edit: All should be good to go! 1.62.1 seemed to be the lowest, so I changed CI/the Cargo.toml file and the README. CI may need re-running though.

I was using cargo clippy --all-features -- -W clippy::pedantic -W clippy::correctness -W clippy::perf -W clippy::style -W clippy::suspicious -W clippy::complexity -A clippy::missing_errors_doc -W clippy::nursery -A clippy::module-name-repetitions -A clippy::similar-names -A clippy::needless-pass-by-value -D warnings so I hope all crates are thoroughly linted 😀

Another edit: Just realised my CI mistake, reverting now

brxken128 commented 1 year ago

After further testing, 1.61.0 is all that's needed for things to function - both srp and aucpace would need these changes though.

tritoke commented 1 year ago

@brxken128 fwiw there is a tool called cargo-msrv that uses a kind of git bisect style approach to find the MSRV and it means you don't need to install all the toolchains, have found it very useful in the past so I thought I'd mention it :)

brxken128 commented 1 year ago

@brxken128 fwiw there is a tool called cargo-msrv that uses a kind of git bisect style approach to find the MSRV and it means you don't need to install all the toolchains, have found it very useful in the past so I thought I'd mention it :)

Oh wow that does sound incredibly helpful, I'll add it to my toolkit. I have a taskfile which installs all of the helpful tooling and this'll definitely be included - thank you so much!

tarcieri commented 1 year ago

both srp and aucpace would need these changes though.

That's fine... can you bump them to the next minor version in Cargo.toml and add -pre though? (i.e. v0.2.0-pre for aucpace, v0.7.0-pre for srp)

brxken128 commented 1 year ago

both srp and aucpace would need these changes though.

That's fine... can you bump them to the next minor version in Cargo.toml and add -pre though? (i.e. v0.2.0-pre for aucpace, v0.7.0-pre for srp)

Done, and changed all 1.61.0 to 1.61