Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 more detail to DID Authentication section. #18
Add more detail to DID Authentication section. #18
Changes from 4 commits
2fcee65
0edf045
e753bd0
f1a5e97
0889042
d939bad
8340869
e7c4a51
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 would support making this always be an array instead to ease some concerns from other implementers around native type handling in some languages. I'm fine either way.
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.
In the open/decentralized ecosystem, I believe the common pattern will be for Verifiers to accept multiple types of DID methods, so +1 to always having this as an array.
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.
Ok, I'll make this change and merge it into this PR.
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.
might it make sense to make the
didMethod
array contain objects, instead of just strings? There's a very good chance that various DID methods require multiple parameters for a given method. (For example, how Veres One DID method has a testnet vs main, cryptonyms vs not, etc).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.
We discussed this on the 2022-08-09 telecon. The group seemed to have consensus around the
didMethod
value always being an array. The group didn't object to renamingdidMethod
toacceptedMethods
to ensure that the contents of the array is always a set of objects. Each object MUST contain at least an entry formethod
and MAY contain other parameters that are specific to the DID Method.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.
Fixed in d939bad.
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.
Why is this only a single option? Wouldn't verifiers support multiple crypto suites?
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.
nvm, seen below that a request may contain multiple DIDAuth requests.
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, I went back and forth on this. There is a combinatoric concern here with listing all the DID Methods and all the cryptosuites, making it an NxN selection, which then gets more multidimensional if we have other "possible things you could do". I'm suggesting that we start with a single value and then depend on the ability to send multiple queries (some of them optional) to suggest the support for multiple cryptosuites. The downside there, of course, is if you support multiple cryptosuites, you have a new query for every cryptosuite. I expect that we might just have to make this an array in time, and if that's the case, we should just do it now... but the counter-argument to that is to make it painful to support more than a handful of cryptosuites to force an increase in interop!
In the end, I expect that we'll probably have to support an array of cryptosuites here.
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.
Maybe we should just start with an array of cryptosuites? To match the array of did methods?
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.
We discussed this on the 2022-08-09 telecon. @dlongley suggested that we rename the value to
acceptedCryptosuites
and make the value an array of objects where we could parameterize each cryptosuite. Each object MUST contain acryptosuite
property and MAY contain other cryptosuite-specific parameters.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.
Fixed in e7c4a51.