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

Ensure created proof option is optional #47

Closed
dlongley opened this issue Jan 2, 2024 · 5 comments
Closed

Ensure created proof option is optional #47

dlongley opened this issue Jan 2, 2024 · 5 comments
Assignees
Labels
CR1 normative This item is a normative change.

Comments

@dlongley
Copy link
Contributor

dlongley commented Jan 2, 2024

The current proof configuration algorithm seems to make created required instead of optional, this should be fixed.

@Wind4Greg Wind4Greg self-assigned this Jan 2, 2024
@Wind4Greg
Copy link
Collaborator

Hi @dlongley (and @msporny) I'm attempting to write up a PR for this but have a question on how we should document things. In the VC-Data-Integrity Spec the various proof attributes are carefully defined and properly marked as OPTIONAL, REQUIRED, or other appropriate indicators. It seems that in the DI-ECDSA specification (and others) we should refer back to VC-Data-Integrity and not repeat ourselves and possibly insert errors.

Would something like the following work for the DataIntegrityProof section of VC-DI-ECDSA:

The attributes of the DataIntegrityProof proof field are defined in VC-Data-Integrity Spec for ECDSA the following attributes take on the following restricted values:

  • type ...
  • cryptosuite ...
  • proofValue ...

Or should we just repeat the language from Data Integrity for the created attribute. Note that we will want to do whatever change looks best to VC-DI-EDDSA and VC-DI-BBS since they also use this language.

@msporny
Copy link
Member

msporny commented Jan 3, 2024

Or should we just repeat the language from Data Integrity for the created attribute. Note that we will want to do whatever change looks best to VC-DI-EDDSA and VC-DI-BBS since they also use this language.

Your instinct is correct, don't repeat language that exists in other specifications... instead, point to the other specification for the definitions and normative language. We can add constraining language in the spec if necessary, but in general, follow "don't repeat yourself" principles for exactly the reasons you outline.

@dlongley
Copy link
Contributor Author

So PR #57 accidentally reverted the work done in #50 to ensure created was optional (here). It should be optional in all suites.

cc: @msporny

@msporny
Copy link
Member

msporny commented Feb 29, 2024

So PR #57 accidentally reverted the work done in #50 to ensure created was optional (here). It should be optional in all suites.

Thanks for catching that, the update should be an editorial fix since the right thing was committed in #50 and then accidentally undone in #57.

@Wind4Greg
Copy link
Collaborator

This was implemented in PR #50 and has been correctly applied after the accidental reversion. See the first sentence here https://w3c.github.io/vc-di-ecdsa/#dataintegrityproof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 normative This item is a normative change.
Projects
None yet
Development

No branches or pull requests

3 participants