search

lovesh / signature-schemes

Rust library for BLS signatures, MuSig, PS signatures
Apache License 2.0
66 stars 20 forks source link

Change Vectors of references to impl FromIterator. This is more flexible and idiomatic. #10

Closed JayPavlina closed 4 years ago

JayPavlina commented 4 years ago

Sorry I am trying to figure out how to do the sign off thing

JayPavlina commented 4 years ago

IntoIterator would probably be better, but it requires a little more work to get it working.

JayPavlina commented 4 years ago

I changed the parameters to IntoIterator. This is cool because it now requires no changes to the API. But I also changed the signature AggregatedVerKey::hashed_verkey_for_aggregation to accept bytes so that the bytes can be cached for a performance improvement. It is a public function, but I don't know if anyone is using it. On my benchmarks aggregation is now around 25% faster.

burdges commented 4 years ago

I think &'a Vec<u8> is rarely idiomatic. An impl IntoIterator<Item = &'a Vec<u8>> should normally be

where I: IntoIterator<Item=B>, B: Borrow<[u8]>

unless impl IntoIterator<Item = impl Borrow<[u8]>> now works.

It cannot necessarily apply the deref implicitly however, so maybe not the same interface.

I needed some tricky Borrow usage to abstract over the different BLS abstractions in https://github.com/w3f/bls/

JayPavlina commented 4 years ago

I think &'a Vec<u8> is rarely idiomatic. An impl IntoIterator<Item = &'a Vec<u8>> should normally be where I: IntoIterator<Item=B>, B: Borrow<[u8]>

Thanks. I changed it to impl IntoIterator<Item = impl AsRef<[u8]>>

JayPavlina commented 4 years ago

@lovesh Can you share your thoughts on this pull request?

lovesh commented 4 years ago

@lovesh Can you share your thoughts on this pull request?

@JayPavlina Thanks for this. Will reply in a day.

lovesh commented 4 years ago

I changed the parameters to IntoIterator. This is cool because it now requires no changes to the API. But I also changed the signature AggregatedVerKey::hashed_verkey_for_aggregation to accept bytes so that the bytes can be cached for a performance improvement. It is a public function, but I don't know if anyone is using it. On my benchmarks aggregation is now around 25% faster.

hashed_verkey_for_aggregation shouldn't have been public. Having an API that accepts cached verkeys is good but making hashed_verkey_for_aggregation accept the cached verkey is not enough since its called from other internal functions. I think having a complementary function to from_sigs that accepts a sequence of outputs of AggregatedVerKey::hashed_verkey_for_aggregation will be a way to enable usage of cached results

lovesh commented 4 years ago

@JayPavlina The changes look good to me. Can you please rebase the latest master?

JayPavlina commented 4 years ago

I am just going to open a new pull request because rebasing is too hard