Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add trait bounds to types in Signature trait #145

Closed
tessico opened this issue Nov 23, 2022 · 3 comments · Fixed by #148
Closed

Add trait bounds to types in Signature trait #145

tessico opened this issue Nov 23, 2022 · 3 comments · Fixed by #148
Assignees

Comments

@tessico
Copy link
Contributor

tessico commented Nov 23, 2022

As pointed out in this PR's discussion, due to downstream considerations it would be cleaner to add trait bounds to the associated types in Signature and Vrf traits, e.g.:

pub trait SignatureScheme {

    ...
    type SigningKey: Clone + Send + Sync + for<'a> Deserialize<'a> + Serialize;

The challenge currently is that the underlying blst structs used in the BLS Sig/VRF don't implement serde's traits (although they have serialize/deserialize free methods).

One option to go forward with this is re-introducing newtype structs like in 0.1.2, except that with a different inner field (i.e. blst::min_sig::{SecretKey, Signature, etc.} instead of arkworks' GroupAffine), and implementing serde::Serialize and Deserialize by hand, like:

pub use blst::min_sig::SecretKey;

#[derive(Debug, Clone)]
pub struct BLSSignKey(pub SecretKey);

impl Serialize for BLSSignKey {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        let out = &self.0.serialize();
        serializer.serialize_newtype_struct("SecretKey", out)
    }
}

I wonder how the downstream repo's bounds were satisfied previously? Namely, I don't understand how

impl<SIGSCHEME> Debug for JfPubKey<SIGSCHEME>
where
    SIGSCHEME: SignatureScheme<PublicParameter = (), MessageUnit = u8>,
    SIGSCHEME::VerificationKey: Clone + for<'a> Deserialize<'a> + Serialize + Send + Sync
...

was satisfied, if the implementers' (e.g. BLSSignatureScheme) associated types only derived arkworks' Canonical(De)Serialize and not serde::(De)Serialize?

@alxiong @chancharles92 @mrain

@alxiong
Copy link
Contributor

alxiong commented Nov 23, 2022

One option to go forward with this is re-introducing newtype structs like in 0.1.2, except that with a different inner field (i.e. blst::min_sig::{SecretKey, Signature, etc.} instead of arkworks' GroupAffine), and implementing serde::Serialize and Deserialize by hand

this option sounds good to me! I wonder if we could also try to leverage on derive(tagged) to support Serde

I wonder how the downstream repo's bounds were satisfied previously? Namely, I don't understand how was satisfied, if the implementers' (e.g. BLSSignatureScheme) associated types only derived arkworks' Canonical(De)Serialize and not serde::(De)Serialize?

I think it's due to tagged_blob macro here
(also be noted that we have shifting to tagged_base64's impl in this commit, instead of jellyfish's own impl.

@nyospe
Copy link

nyospe commented Nov 23, 2022

Thank you for the edit, you beat me to it. Yes, this is where it comes from, and the preferred usage will be the attribute directly from tagged_base64.

Also, it's worth noting that we currently have a unified list of known tags in https://github.com/EspressoSystems/espresso-systems-common/blob/main/src/lib.rs, but it might be better (@jbearer) to make separate generalized crates for tags for jf, cap, etc, and export those in espresso-systems-common, so that they can be included directly in crates that don't export espresso-specific assets.

@tessico tessico self-assigned this Nov 23, 2022
@tessico
Copy link
Contributor Author

tessico commented Nov 23, 2022

Thanks both, I'll see what I can do about it.

@tessico tessico changed the title Add trait bounds to types in Signature and Vrf traits Add trait bounds to types in Signature trait Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants