Add serialization for Polynomials and Evaluations - #144
Conversation
| type_id.serialize(&mut writer)?; | ||
|
|
||
| match self { | ||
| GeneralEvaluationDomain::Radix2(d) => d.serialize(&mut writer), |
There was a problem hiding this comment.
Should we instead add a serialization method on the underlying domains, instead of serializing the degree directly?
Feels to weird to only have the general evaluation domain have serialize, and not the underlying domains
There was a problem hiding this comment.
oh I see the underlying methods the d here is domain, not degree. Can the variable be changed throughout the file to be domain instead of d
There was a problem hiding this comment.
Yes. I can do the change. I also have implemented the serialization for domains.
The serialization of domains is needed to serialize the evaluations.
|
Can we ensure that PR's have at least one person reviewing them before merging on algebra? |
| - #126 (ark-ec) Use `ark_ff::batch_inversion` for point normalization | ||
| - #131, #137 (ark-ff) Speedup `sqrt` on fields when a square root exists. (And slows it down when doesn't exist) | ||
| - #141 (ark-ff) Add `Fp64` | ||
| - #144 (ark-poly) Add serialization for polynomials and evaluations |
There was a problem hiding this comment.
This PR is a breaking change, can you move it to the relevant section. (Its added a serialization trait bound to all polynomials)
There was a problem hiding this comment.
All the polynomial implementations in this repo have now implemented the serialization. I assume there is another project also using polynomial?
There was a problem hiding this comment.
IDK if there exists another project that implements polynomial, but if they did their implementation is now broken
Having all the reasons your code may be broken after an upgrade is really useful though
There was a problem hiding this comment.
also could the ark-poly readme be updated for this change (https://github.com/arkworks-rs/algebra/tree/master/poly)
ValarDragon
left a comment
ValarDragon
left a comment
There was a problem hiding this comment.
Please move the changelog update to be a breaking change
| type_id.serialize(&mut writer)?; | ||
|
|
||
| match self { | ||
| GeneralEvaluationDomain::Radix2(d) => d.serialize(&mut writer), |
There was a problem hiding this comment.
oh I see the underlying methods the d here is domain, not degree. Can the variable be changed throughout the file to be domain instead of d
|
Sorry that this PR has caused a few troubles, including one for Tom's PR (#140)... Will surely request a review in the future. |
|
Not a huge issue =) , but at least for |
|
Understood :) |
2 participants
Description
This PR adds serialization for polynomials and evalutions.
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.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer