impl `Absorb` for `String`

arkworks-rs / crypto-primitives

Interfaces and implementations of cryptographic primitives, along with R1CS constraints for them

https://www.arkworks.rs

Apache License 2.0

165 stars 79 forks source link

impl `Absorb` for `String` #138

Closed mmagician closed 6 months ago

mmagician commented 7 months ago

Description

Needed for deriving Absorb for LabeledCommitment in poly-commit.

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.

[ ] Targeted PR against correct branch (main)
[ ] Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
[ ] Wrote unit tests
[ ] Updated relevant documentation in the code
[ ] Added a relevant changelog entry to the Pending section in CHANGELOG.md
[ ] Re-reviewed Files changed in the Github PR explorer

Pratyush commented 7 months ago

Also, I don't think we should derive Absorb for LabelledCommitment. Really the label only exists in the code for debugging purposes; the commitment should already be a unique representative of its polynomial. Absorbing the string would only add inefficiency.

mmagician commented 7 months ago

Also, I don't think we should derive Absorb for LabelledCommitment. Really the label only exists in the code for debugging purposes; the commitment should already be a unique representative of its polynomial. Absorbing the string would only add inefficiency.

Sure, makes sense. We can impl Absorb for String regardless.

burdges commented 6 months ago

If one wants String then one typically wants str instead (or perhaps in addition).

As an aside, it's bizarre LabeledCommitment::label returns &String instead of &str.

I think folks have typically chosen &[u8]instead of the utf8String/str` for labels, but actually the unicode lookalikes are not a problem here, so probably not a real concern.