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 CacaoZcapProof2022 #436

Closed
wants to merge 7 commits into from
Closed

Add CacaoZcapProof2022 #436

wants to merge 7 commits into from

Conversation

clehner
Copy link
Contributor

@clehner clehner commented May 12, 2022

Importing code and test vectors from https://github.com/spruceid/cacao-zcap-rs.

To have ssi depend on the cacao-zcap crate as it is now would create a circular dependency, since cacao-zcap already depends on ssi. This PR therefore adds the cacao-zcap implementation into the ssi crate directly.

To reduce the coupling so that cacao-zcap (and other proof suites) could be separate from the ssi crate, would require some refactoring. e.g. a "core" part of ssi could be removed into a new crate that the proof suites could depend on, so that ssi could depend on the proof suites as separate crates.

The context file has already been added (#434) - Although it is not needed for just a CacaoZcapProof2022 delegation which does not itself involve JSON-LD processing, it is needed to invoke the delegation with a different (JSON-LD) proof type.

Note: embedding a CacaoProof2022/CacaoZcapProof2022 delegation in an invocation or subdelegation doesn't yet work, because of #437.

@clehner clehner marked this pull request as ready for review May 13, 2022 17:13
@clehner clehner requested a review from cobward May 13, 2022 17:14

mod core;
pub use self::core::*;
#[cfg(feature = "cacao-zcap")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this #[cfg(...)] statement as the entire module is already enabled for this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 891dcf2

Comment on lines 147 to 162
/* Can't call zcap.verify yet because that depends on ssi
* having this proof type.
use crate::vc::Check;
let res = zcap.verify(None, &resolver).await;
assert_eq!(res.errors, Vec::<String>::new());
assert!(res.checks.iter().any(|c| c == &Check::Proof));
*/

/* Can't verify because signature is not real
let proof = zcap.proof.as_ref().unwrap();
let warnings = CacaoZcapProof2022
.verify(proof, &zcap, &resolver)
.await
.unwrap();
dbg!(warnings);
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be uncommented now that it has been added as a proof type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment in e1cbf6c
The second part "Can't verify because signature is not real" is still true; a real signed test vector will need to be created to actually test verify. examples/cacao-zcap-regenerate-test-vectors.rs could be updated to do that.

clehner added 2 commits May 16, 2022 11:35
Partially uncomment, as a negative test.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sbihel
Copy link
Member

sbihel commented Jul 24, 2024

Closing as ssi has been majorly refactored since this PR was opened and we do not need this feature currently

@sbihel sbihel closed this Jul 24, 2024
@sbihel sbihel deleted the feat/cacao-zcap branch July 24, 2024 09:34
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 this pull request may close these issues.

4 participants