-
Notifications
You must be signed in to change notification settings - Fork 200
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
fix: did:peer:2 creation and parsing #1752
fix: did:peer:2 creation and parsing #1752
Conversation
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
packages/core/src/modules/dids/methods/peer/__tests__/__fixtures__/didPeer2Ez6L.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
} | ||
|
||
// Add all encoded keys ordered by their id (#key-1, #key-2, etc.) | ||
did += keys | ||
.sort((a, b) => a.id.localeCompare(b.id)) |
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 think this will fail in case of >= 10 keys within a did:peer, so we should use a regex/remove '#key-' prefix and do the sorting numerically
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.
Aren't the keys already sorted by default? I think we always process them start to end right?
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.
Given that our input is a modeled DidDocument
, In didDocumentToNumAlgo2Did()
we are sorting by key purpose (first assertionMethods, then keyAgreement, authentication, etc.), so it might happen that a key referenced by a service will not be encoded in the order we expect (e.g. mixing up verification key and key agreement key, or using key from another service). This is mainly the reason for this concern.
@genaris any big changes still needed for this PR? |
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
I think there are no more changes needed from my side, unless we find a better way of handling these changes in the spec. I don't like so much the idea of limiting the key ids to the pattern 'key-N' (any call an existing app makes to |
Agreed. I've had this with several other did methods though, and it may be interesting to look at a better did builder pattern so you can input what type of did doc you want and the did registrar turns it into something compatible with the method. E.g. i want two verificationMethods, one with keyType ed25519, the other one with this specific key. And most fields will just be optional and populated later on. |
// Add all encoded keys | ||
did += encoded.join('') | ||
const prefix = 'key-' | ||
if (!keys.every((key) => key.id.split('#')[1]?.startsWith(prefix))) { |
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 be better/cleaner to use parseDid here?
Changes in did:peer:2 creation and parsing as specified in https://identity.foundation/peer-did-method-spec/#method-2-multiple-inception-key-without-doc. Basically:
I need to add some more tests and use cases check if it might represent an issue with AFJ 0.4.2 (apparently not).
Closes #1682