Refactor such that we can derive Encodable and Decodable for crypto crates APIs #4512
Conversation
|
|
||
| let message = b"Hello world!"; | ||
| let ciphertext = pk.encrypt(message); | ||
| let decryption_share = sks.secret_key_share(0).decrypt_share(&ciphertext).unwrap(); |
There was a problem hiding this comment.
Nit: use expect instead of unwrap
| let point = G1Affine::from_compressed(&bytes); | ||
|
|
||
| if point.is_some().unwrap_u8() == 1 { | ||
| Ok(point.unwrap()) |
fedimint-core/src/bls12_381_serde.rs
Outdated
| let scalar = Scalar::from_bytes(&byte_array); | ||
| // FIXME: probably safe with public data, but doesn't look nice | ||
| if scalar.is_some().into() { | ||
| Ok(scalar.unwrap()) |
There was a problem hiding this comment.
On a lower level (mechanicaly what's goin on) it look OK, but on the higher level I'm lost.
The legacy vs new encoding seems like a complication, but for a good reason.
I also don't understand what's the status of "upstream serde support" or what exactly it means here.
@elsirion is probably best person to review
|
Need to take a look, it's less of a priority than other things we might want to get into 0.3 since it's mostly useful for LN and e-cash NG, which are both longer-term research projects. |
joschisan
force-pushed
the
encode_crypto
branch
from
fd89443 to
c5cc88c
Compare
| @@ -31,8 32,6 @@ serde_json = { workspace = true } | |||
| strum = { workspace = true } | |||
| strum_macros = { workspace = true } | |||
| hex = { version = "0.4.3", features = [ "serde"] } | |||
| sha3 = "0.10.8" | |||
| tbs = { package = "fedimint-tbs", version = "0.3.0-alpha", path = "../crypto/tbs" } | |||
There was a problem hiding this comment.
Uff, that was dirty anyway. Much better to just depend on the crypto primitive.
crypto/tbs/src/lib.rs
Outdated
| @@ -15,6 16,9 @@ use rand_chacha::ChaChaRng; | |||
| use serde::{Deserialize, Serialize}; | |||
| use sha3::Digest; | |||
|
|
|||
| // this is a legacy encoding that we need to keep for tbs; everywhere else we | |||
| // want to upstream serde support for bls12_381 such das non-human-readable | |||
| use crate::encoding::{Decodable, DecodeError, Encodable}; | ||
| use crate::module::registry::ModuleDecoderRegistry; | ||
|
|
||
| impl Encodable for threshold_crypto::PublicKeySet { |
There was a problem hiding this comment.
Can't we just derive all this now? Care would need to be taken not to break anything, but worth looking into imo (follow-up, not this PR).
joschisan
force-pushed
the
encode_crypto
branch
from
c5cc88c to
0f6013f
Compare
Previously we derived encodable and decodable by hand for all the public types of tpe. However, with new crypto also built on bls12_381 like lightning-ng and ecash-ng that has more complex public structs this becomes increasingly cumbersome and repetitious. Hence, we refactor the code to be able to derive Encodable and Decodable for all future crypto crates built on bls12_381.
In a next step we want to do the same with serde Serialize and Deserialize by upstreaming an implementation to bls12_381 such that we can derive the traits as well. Notice that we duplicate the serde implementation for Scalar, G1, G2 as we copy it from tbs to core were we need it as well. We leave it in tbs though as the copy in core is supposed to be removed after we upstreamed it.