Fix #47

[Pratyush]

Description

closes: #47

---

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]

Upstream tests pass for ark-pallas and ark-vesta!

[ValarDragon]

I'm generally confused as to why ConstantSerializedSize was removed, that seems like a really useful trait to me.

For this new class of curves, presumably the serialization with flags is still constant sized, its just one byte longer than the serialization without flags?

[Pratyush]

It was removed because the size depends dynamically on the flag used (and we can't do generics in constants yet). For example, if I'm serializing with EdwardsFlags, a 255-bit field will still fit into 32 bytes. However, if I'm serializing with SWFlags, a 255-bit field will fit only into 33 bytes.

[ValarDragon]

It was removed because the size depends dynamically on the flag used (and we can't do generics in constants yet). For example, if I'm serializing with EdwardsFlags, a 255-bit field will still fit into 32 bytes. However, if I'm serializing with SWFlags, a 255-bit field will fit only into 33 bytes.

I'm a bit confused about the structure of this. Currently it sounds like the field has a method serialize_with_flags, but why is that done versus the following:

1. Have a method/trait to serialize a field element, and state the exact number of bits used in the serialization. (Basically add serialized_size_bits to CanonicalSerialize)
2. Have a method that generically takes a Flags object, and a b bit serialized blob, and append the flag bits to it. (Named append_flags or append_metadata)

I'm not seeing why we would want to specialize the appending of flags to fields. Is there an efficiency gain in the current form?

[Pratyush]

Not all objects care about Flags; in fact, only Fields care about them. So it doesn't make sense to add these methods to the CanonicalSerialize trait, which is why we have the CanonicalSerializeWithFlags trait

[ValarDragon]

The only method that would be added to CanonicalSerialize would be serialized_size_bits (which could even have an auto-impl to just be 8 * serialized_size).

The concept of flags is essentially taking two serializable objects, and concatenating their serializations at the bit-level instead of the byte level. I don't currently understand why we'd prefer the flag trait versus making a serialization method / deserialization method that concatenates object serializations at the bit-boundary, instead of the byte boundary. Maybe we should put this into an issue, and revisit later? A more general method would also enable saving space in serializing many 257 bit field elements

[Pratyush]

Hmm that is interesting, but out of scope for this PR =P

I've filed an issue about it, so that we can revisit it in the future (it might be that the space saving would be blown out by the implementation complexity)

[Pratyush]

@ValarDragon is there anything else you'd like to see changed here?

[ValarDragon]

LGTM! Thanks for adding more documentation around all of these changes as well