-
Notifications
You must be signed in to change notification settings - Fork 61
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
Use enum for proof suites #489
Conversation
889cc4c
to
af4aa89
Compare
#[cfg(feature = "tezos")] | ||
Self::P256BLAKE2BDigestSize20Base58CheckEncodedSignature2021 => SignatureType::JWS, | ||
#[cfg(feature = "secp256k1")] | ||
Self::EcdsaSecp256k1Signature2019 => SignatureType::JWS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like ProofSuiteType
, but for this fn what about cases where a user wants to choose proof format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to be able to have either a JWT VC or a LD Proof for the same proof suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah for at least some of them, here secp256k1-2019 is listed in the LD proof suites as is the JsonWebSig, there are probably more examples. Unfortunately I'm not sure where to look for a canonical list of JWT-supporting suites, or if there is one. there's this in the implementers guide but it's not a list :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps some dual-use ProofSuiteType
variants can hold a SignatureType
internally and this fn can either return the internal value (if there is one) or return the supported constant one, e.g.
#[cfg(feature = "tezos")]
Self::P256BLAKE2BDigestSize20Base58CheckEncodedSignature2021 => SignatureType::JWS,
#[cfg(feature = "secp256k1")]
Self::EcdsaSecp256k1Signature2019(t) => t,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, seems like a tricky thing to solve correctly. I guess it would have to be a field in LinkedDataProofOptions
. And for the verification I've been thinking of making the Proof
type an enum to make sure you can't have a jws
and a proof_value
, and then we can separate the proof types/formats, but this probably incurs its own set of problems.
Should we make an issue and address it later, considering this has always been the behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LDPOpts would also be fine and a proof enum would be neat, but yeah I think it would be ok to address later, these are basically the common usage cases already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self { | ||
type_: type_.to_string(), | ||
..Self::default() | ||
type_, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not impl Default
for setting type_
to type_
and everything else to None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence, I felt like deriving Default
didn't make sense because depending on the features enabled you'd need a different default type, and in general it doesn't make sense to pick a type. But then because it's derived it would be available to end users and potentially introduce issues for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, the only question I have is if one could use a Default
impl on ssi-ldp/src/lib.rs#Proof
, but it isn't particularly important.
258c12b
to
e46a949
Compare
Based on #488
This allows to fail early when processing a Credential/Presentation, have a list of types in the code to use when listing the supported types in issuers or verifiers, and is just generally more idiomatic.
In the future we should separate types such as Credential to have a
CredentialGenerics
that accepts anything that implementsProofSuite
so users can extend ssi with their own proof suite. I don't think it's necessary for now as I'm not aware of anyone not using the default types.