search

ZenGo-X / curv

Rust language general purpose elliptic curve cryptography.
MIT License
264 stars 111 forks source link

Unify & optimise serialization in v0.7 #141

Closed nskybytskyi closed 1 year ago

nskybytskyi commented 2 years ago

You are probably wondering how I got here Background: earlier today I opened #140, where @survived suggested either to wait a couple of months or open a PR. The former is not a viable option for us, hence here comes the latter.

Resolves #138 and #140 in v0.7:

The main question here is if you consider serialization format public API or not. I am very tempted to say that it is not, and the only publicly guaranteed invariants are Deserialize(Serialize(Point)) = Point and Serialize(Deserialize(String)) = String.

However, my opinion is clearly biased, and there are different approaches to this issue. For example, one may argue that if they got a file on a hard drive produced by the former version of Serialize, then they won't be able to Deserialize it by the current version of Deserialize, which is not ideal, to say the least.

However, #139 appears to be a bug-fix either way, so the situation above can occur in the current setting as well (when using different BigInt backends), so I assume it has to be addressed somehow...

survived commented 2 years ago

The main question here is if you consider serialization format public API or not

Indeed, we should freeze serialization format at some point in the future. It's not expected that bumping curv v0.7.1 to v0.7.2 will invalidate all serialized points/bigints. However, we do not follow any policy regarding serialization format, so it may be considered unstable.

nskybytskyi commented 2 years ago

LGTM!

I added only the basic requested changes yesterday and still want to change from hex to raw bytes, so please don't merge it in yet. Thank you for your patience. 🙏🏼

survived commented 2 years ago

Sure, take your time! I won't merge until PR is marked as ready for review. I just noticed that there are no unwraps anymore. Converting to raw bytes is also an important optimisation!

survived commented 2 years ago

BigInt deserialization implementation has been improved in latest curv, it was not compatible with serde_json. See the PR: #145. Points/scalars deserialization has been changed as well: #143