search

SpinResearch / RustySecrets

🔑 Threshold Shamir's secret sharing in Rust
BSD 3-Clause "New" or "Revised" License
258 stars 33 forks source link

Various lints #30

Closed a-dma closed 7 years ago

a-dma commented 7 years ago

Hi,

I've added a few lints to clean up the code. Tests still pass. The most notable change is that I have broken the public API, recover_secret now takes a slice rather than a Vec.

A couple more things that I didn't fix but that should be addressed:

One last thing, are you planning a release of the signed shares stuff?

romac commented 7 years ago

Cue, copy-pasta of my comment on #29:

First of all, sorry for the late reply, and thank you very much for the PR!

Sadly, I won't be able to merge it, as we are currently working on a big refactoring (+ more cool features), which encompasses the ones you submitted. We will release those changes publicly very soon (at least as a PR open for comments), and thus hope that you will not be too disappointed by the negative reply (which has nothing to do with your PR as such). We always welcome contributions, new issues, new ideas, and criticisms, and are looking forward to hearing your input on the upcoming pre-release!

Thanks again!

PS: I realise that the message above sounds very "corporate", sorry about that, it wasn't my intention when writing it up. I'm just not very good at interacting with people online…

Now onto the issues you raised:

Hi,

I've added a few lints to clean up the code. Tests still pass.

Awesome! This has already been taken care as part of the changes mentioned above, but it's always good to see people actually building the code and looking at the build output :)

The most notable change is that I have broken the public API, recover_secret now takes a slice rather than a Vec.

As it will as well (amongst other things) in the next release :)

The protobuf method make_repeated_bytes_accessor is deprecated. Any reason why that is still used?

Only that that code was generated by a previous version of rust-protobuf, and that we hadn't gotten around to update it.

In wrapped_secrets.rs:311 there is a weird for loop over anOption. Is that on purpose? What are you trying to do? I think you're better off withif let Some(mt)`

Absolutely, it's been fixed as well (thanks Clippy :))

I didn't want to ruin the whole history and send a million lines PR, but you should really consider to format the code using rustfmt.

Of course! It's been done as well, and will soon be enforced on Travis.

One last thing, are you planning a release of the signed shares stuff? Yup, it's coming as well. Please bare with us while we are finishing up the next (pre-)release, and thanks again for your input!

a-dma commented 7 years ago

No worries. I understand what you're saying and it wasn't much work to begin with. So no harm in tossing it.

I'll take the opportunity to ask a couple more questions, I hope you don't mind:

Thanks and I'll be looking forward to a v1.0.0 ;)