Conversation
arnaucube
force-pushed
the
migrate-to-pod2-latest
branch
3 times, most recently
from
2948075 to
e28cd7d
Compare
src/ed25519pod.rs
Outdated
| use super::*; | ||
|
|
||
| #[test] | ||
| #[ignore] // This is for the GitHub CI, it takes too long and the CI would fail. |
There was a problem hiding this comment.
Note: this ignore is as @ainta initially had it in PR #2 , I suggested removing it to enforce this test too, but it seems that the GitHub CI fails because it takes too long. Disabling it again, and for now we can double check running the tests locally at new PRs, but eventually we can find a solution (either increasing GitHub's cuota, or by setting our own custom server for the CI).
…line docs, update readme
arnaucube
force-pushed
the
migrate-to-pod2-latest
branch
from
e28cd7d to
73e13b5
Compare
| pub_self_statements(self.msg, self.pk) | ||
| } | ||
|
|
||
| fn serialize_data(&self) -> serde_json::Value { |
There was a problem hiding this comment.
The goal of this method is to serialize the pod, so that later on we can deserialize it!
Could you also add deserialization methods to the eddsa and ed25519 pod?
Here's an example of deserialization in the MainPod (you should use the same function signature): https://github.com/0xPARC/pod2/blob/3c6930dfe61ce28f925bea942dcff839c910ddc7/src/backends/plonky2/mainpod/mod.rs#L647-L664
You may decide to change the data that you're serializing. I think it will be easier if you include the msg and pk instead of the public statements.
It would also be great if you add a test where you serialize and deserialize and check that the pod is equal before and after.
There was a problem hiding this comment.
Makes sense, I wonder then, if we should add the deserialize method into the Pod trait, to enforce that it exists in future implementations of pods (to not depend on reviews detecting it missing it).
There was a problem hiding this comment.
That's a good point. Originally I didn't add this method to the Pod/RecursivePod traits because these traits are used for trait objects. And in a trait object I don't think you can access a method that doesn't use self, and the deserialize method doesn't use self.
But as you point out, having this method in the trait will enforce the developer of an introduction pod to implement it!
There was a problem hiding this comment.
I've created an issue for this 0xPARC/pod2#294
There was a problem hiding this comment.
I've added the deserializer (594cb58), but I think I'm going put back to draft the current PR and then implement 0xPARC/pod2#294 and update the current PR to use the new (future) pod2 version (also adding a test).
There was a problem hiding this comment.
Update: 0xPARC/pod2#300 (which solves 0xPARC/pod2#294 ) has been merged, I've updated the current PR to latest pod2 version including those changes.
arnaucube
force-pushed
the
migrate-to-pod2-latest
branch
from
bd664ce to
9842baf
Compare
arnaucube
force-pushed
the
migrate-to-pod2-latest
branch
from
9842baf to
6348f3f
Compare
arnaucube
force-pushed
the
migrate-to-pod2-latest
branch
from
32687d5 to
6c874e6
Compare
2 participants
resolves #3 (update to latest pod2 version), adds also a bit of inline documentation, and adds references in the readme
update: depends on 0xPARC/pod2#300 .