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

Feat: adding Proof Configuration to Signature #77

Conversation

koptan
Copy link
Contributor

@koptan koptan commented Dec 6, 2023

Description

PR to include the proof configuration in the final hash during the process of creating and verifying the proof.

Therefore, the verification process should fail if any values in the Proof Configuration attributes change.

This PR should fix #34, also it's merge #38

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

@koptan
Copy link
Contributor Author

koptan commented Dec 7, 2023

@DominikPinsel @matgnt @borisrizov-zf @bjoern-arnold, please for your review.

@matgnt
Copy link

matgnt commented Dec 11, 2023

I would leave this to the others to review. I'm not a Java developer and don't know the libraries internal structure.

Copy link
Contributor

@DominikPinsel DominikPinsel left a comment

Choose a reason for hiding this comment

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

I like the idea with the new ProofSignature objects :)

@koptan koptan changed the title Adding Proof Configuration to Signature Feat: adding Proof Configuration to Signature Jan 9, 2024
@koptan koptan mentioned this pull request Feb 5, 2024
2 tasks
@borisrizov-zf
Copy link
Contributor

@koptan you should rebase, because of #44

@koptan koptan force-pushed the issue-34-signature-proofoptions-not-included branch from 57b7e59 to 8c676be Compare March 6, 2024 09:12
@koptan
Copy link
Contributor Author

koptan commented Mar 6, 2024

@borisrizov-zf Rebase is done, you can merge it 👍

Copy link
Contributor

@borisrizov-zf borisrizov-zf left a comment

Choose a reason for hiding this comment

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

Please re-check your PR, the copyright year seems to be wrong on multiple files. Also method documentation has been removed, but it is essential to have documentation to be Maven Central compliant.
I'll re-do the review after these issues have been fixed.

@koptan koptan requested a review from borisrizov-zf March 7, 2024 08:08
@borisrizov-zf
Copy link
Contributor

@koptan Hi, still many more files have the docs deleted, and the copyright year is not 2024. I haven't marked them explicitly, due to the sheer quantity of files. Please review them and fix these issues, before I can continue.

@koptan koptan force-pushed the issue-34-signature-proofoptions-not-included branch from 6198c2a to f9af565 Compare March 12, 2024 08:18
@koptan koptan requested a review from borisrizov-zf March 20, 2024 13:40
@borisrizov-zf
Copy link
Contributor

@koptan can you point me in the right direction about the following:

verification method can be either a multibase string or a map, but I cannot see where transformation is taking place. The interface which binds these seems to be missing the option to easily extend to something like publicKeyJWK with an EC cipher. Am I missing something?

@koptan
Copy link
Contributor Author

koptan commented Apr 3, 2024

@borisrizov-zf transformation is happening in TransformedLinkedData, we accept any JsonLD object as parameter.

@borisrizov-zf
Copy link
Contributor

@koptan Hi, the branch needs rebasing, you're out of sync with main.

koptan added 2 commits April 10, 2024 10:49
BREAKING CHANGE: a lot of Exception has been added and removed
@koptan koptan force-pushed the issue-34-signature-proofoptions-not-included branch from 66dc967 to 5943b95 Compare April 10, 2024 08:50
@borisrizov-zf borisrizov-zf merged commit 9727c2d into eclipse-tractusx:main Apr 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signature ProofOptions not included
4 participants