-
Notifications
You must be signed in to change notification settings - Fork 510
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 di support w/ eddsa-jcs-2022 #3181
Add di support w/ eddsa-jcs-2022 #3181
Conversation
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick St-Louis <[email protected]>
I'm having issue with the poetry lock file after adding the |
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.
A few nitpicks. Looks good overall to me!
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.
Nitpick but I think I'd rather have these be closer to the request handlers that use them rather than being in a separate file.
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.
My plan was to put the cryptosuite there and later on build issuing VC with it by adding a layer of data model validation. When you say these, what exactly do you refer to?
@@ -0,0 +1,11 @@ | |||
from .eddsa_jcs_2022 import EddsaJcs2022 | |||
|
|||
CONTEXTS = {"data-integrity-v2": "https://w3id.org/security/data-integrity/v2"} |
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.
When would we look up this context by the string data-integrity-v2
? I get the feeling it might be simpler to just have DIV2_CONTEXT = "..."
.
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 agree, not sure why I settled for this.
I would like to create a file similar to this file, but focused on contexts with their digests.
I'm just not convinced by the current structure of the vc directory. I think we should think about the purpose of this directory and re-organize how files are structured. I would like to create a linked-data sub directory where the document loaders and context uri/files live, and manage all linked-data related operations in there, providing some functions to detect dropped terms, etc...
Then just focus on data integrity proofs...
…oudagent-python into pstlouis/add-di-support
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick St-Louis <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
@dbluhm this is ready for a final review, I've addressed most of the comments and added 4 unit tests. Do you know how to make SonarCloud happy? |
@jamshale can I get a pair of eyes on this pr please? |
I think this looks good 👍 Was the did_key manager stuff needed for this PR or was it an accident? |
from .proof import DIProof | ||
from .proof_options import DIProofOptions, DIProofOptionsSchema | ||
|
||
__all__ = ["DIProof", "DIProofSchema", "DIProofOptions", "DIProofOptionsSchema"] |
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.
You need to import DIProofSchema
to fix the issue reported by sonarcloud.
I think the unit testing looks good and we can probably just ignore the coverage. It just complains if under 80%, but that's not always necessary.
accident, let me fix this |
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Quality Gate failedFailed conditions |
closing as the features are supported by #3261 |
This PR looks to bring Data Integrity to aca-py over arbitrary json (NOT VC).
It adds a eddsa-jcs-2022 cryptosuite to the vc plugin and 2 routes to the wallet plugin (similar to the jwt sign/verify features).
It also moves the request/response schemas to the models directory of the wallet plugin.