feat: Add SerdeObject support for pasta curves #39
Conversation
|
Should I add unit-tests too? I do think so TBH. But wanted to know if they're maybe already done somewhere else in halo2. |
There was a problem hiding this comment.
The implementation looks correct to me! I've added a few comments, mostly about:
- Reducing the number of
unsafe - Exploring code deduplication
I definitely think there should be unit tests here, for each type and for different values: do the serialization and deserialization and test equality.
| // The transmute is the only way to get access to the internal array. | ||
| // Note that this is safe as long as the underlying array length is the same | ||
| // and the types contained are the same too. | ||
| let limbs: [u64; 4] = unsafe { std::mem::transmute_copy(&self) }; |
There was a problem hiding this comment.
Could you use the From<Fp> for AccessibleFp to get the [u64; 4] avoiding the explicit unsafe here?
Maybe like this let value: AccessibleFp = self.clone().into()
There was a problem hiding this comment.
This will force us to clone when not needed in reality. THat's why I don't like it.
I can do it if you feel like it has to. But I'd prefer avoiding and having faster serialization.
| } | ||
| } | ||
|
|
||
| struct AccessibleFq(pub [u64; 4]); |
There was a problem hiding this comment.
The code for Fp and Fq seems the same, but just swapping Fp by Fq, leading to a lot of code duplication.
Both types implement the PrimeField, so I wonder if this fact could be used to do a generic implementation that works for both? Or perhaps another option is to write the code in a macro and then apply it for Fp and Fq.
| } | ||
| } | ||
|
|
||
| struct AccessibleVestaPoint { |
There was a problem hiding this comment.
This one has the same code as AccessiblePallasPoint, the only difference is the call to .is_on_curve(). So I also wonder if the code can be deduplicated 🤔
CPerezz
force-pushed
the
add/pasta-serde-support
branch
from
87acd42
to
1bd3e8e
Compare
|
Addressed most of the review comments. Pending actions:
|
With the current serialization primitives we have here, pasta curves did not have any way to be serialized as we did not implement `SerdeObject` for it. This PR adds this support which resolves #23. The code needs to have `unsafe` chunks as we need to transmute memory in order to be able to access the private fields of the pasta-curves crate structs. This should be revisited and re-thought once privacy-scaling-explorations/halo2#176 is addressed.
CPerezz
force-pushed
the
add/pasta-serde-support
branch
from
1bd3e8e
to
e9d3776
Compare
With the current serialization primitives we have here, pasta curves did not have any way to be serialized as we did not implement
SerdeObjectfor it.This PR adds this support which resolves
#23. The code needs to have
unsafechunks as we need to transmute memory in order to be able to access the private fields of the pasta-curves crate structs.This should be revisited and re-thought once
privacy-scaling-explorations/halo2#176 is addressed.