search

private-attribution / ipa

A raw implementation of Interoperable Private Attribution
MIT License
41 stars 23 forks source link

Hash Owned Values #1253

Closed danielmasny closed 1 week ago

danielmasny commented 1 week ago

allow hashing iterators on owned values + test

Previously, we could only hash iterators over references. This would make it necessary to collect an iterator in a vector and return the vector to the compute_hash function. By allowing hashing owned values, we do not need to collect the iterator into a vector

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.42%. Comparing base (77ac0b0) to head (5a7dd2e). Report is 13 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1253 +/- ## ========================================== - Coverage 92.73% 92.42% -0.31% ========================================== Files 200 200 Lines 31486 31030 -456 ========================================== - Hits 29197 28681 -516 - Misses 2289 2349 +60 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

akoshelev commented 1 week ago

I don't like the callsites of compute_hash but there seem to be no other way to provide this functionality. Trait coherence rules sometimes can be very limiting, I'd love to make compute_hash implementation more complicated and avoid <_, _, _> , but my experiments yielded no results, maybe @andyleiserson has some ideas

andyleiserson commented 1 week ago

How about this: https://github.com/danielmasny/ipa/compare/owned-hashing...andyleiserson:ipa:dm-owned-hashing?expand=1

danielmasny commented 1 week ago

I am happy with your solution Andy. Thanks! Should we merge it in this pull request or do we create a new one?

andyleiserson commented 1 week ago

I think it's better to squash the changes into this PR (it will reduce the diff significantly). One thing that's worth doing before merging is adding a comment on trait SerializeAs. Maybe Serves a role similar to `Borrow<T>` for the type parameter to `compute_hash`. The problem with `Borrow<T>` is that it is implemented for both `T` and `&T`, which then requires explicitly specifying `S` when `compute_hash` is called to resolve the ambiguity.