feat!: Use did:peer:4 by default for DID Exchange #2166
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
🦋 Changeset detectedLatest commit: b623c54 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -767,7 +767,7 @@ describe('out of band', () => { | |||
|
|
|||
| // Use the invitation did from the first connection to create the second connection | |||
| const outOfBandRecord2 = await faberAgent.modules.oob.createInvitation({ | |||
| invitationDid: outOfBandRecord1.outOfBandInvitation.invitationDids[0], | |||
| invitationDid: faberAliceConnection.did, | |||
There was a problem hiding this comment.
This is different right? InvitationDid is usually not the final did
There was a problem hiding this comment.
In this case it should be the same, since we are not setting it as an multiUse invitation. I did this trick because OutOfBandInvitation.invitationdDids is dynamically generating peer:2 DIDs and this does not work directly when querying peer:4 DIDs.
But I'll take a second look to it to see if everything is in place.
| // In case the DID Exchange started with an implicit invitation, take all recipient keys from the corresponding DID | ||
| for (const did of outOfBandRecord.outOfBandInvitation.getDidServices()) { | ||
| invitationRecipientKeys.push( | ||
| ...(await getDidDocumentForCreatedDid(agentContext, parseDid(did).did)).recipientKeys.map( |
There was a problem hiding this comment.
Do we always need to do this, or only for implicit? Because now we run this always?
There was a problem hiding this comment.
I think the comment is misleading. It is useful for resolvable DIDs in general. However I'll review the DID Exchange Response creation more in details just to make sure it works properly in all cases.
| return didRecord.didDocument | ||
|
|
||
| const didsApi = agentContext.dependencyManager.resolve(DidsApi) | ||
| return await didsApi.resolveDidDocument(did) |
There was a problem hiding this comment.
Hmm 🤔
Are there cases where we don't store our own did?
There was a problem hiding this comment.
Not really, but I wanted to take all the advantages of DidsApi.resolveDidDocument() (such as dynamically generating the did:peer:4 DID Document) making sure that it would only resolve DIDs created by us (I can propagate a filtering parameter but it would modify a good number of methods).
There was a problem hiding this comment.
This is okay! Just curious :)
There was a problem hiding this comment.
It also has the benefit of caching, and even DIDs created by us, but not managed by us (e.g. think a did web for which we have one key, but it's stored outside of credo as it's regularly updated)
Signed-off-by: Ariel Gentile <[email protected]>
…change Requests as response to an Out of Band invitation. It is possible to return to previous behaviour by manually setting `peerNumAlgoForDidExchangeRequests` option in DIDComm Connections module config. Signed-off-by: Ariel Gentile <[email protected]>
…ion#2166) Signed-off-by: Ariel Gentile <[email protected]> Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
Up to now, we were using did:peer:1 by default when creating DID Exchange requests. This PR changes this to use the newer did:peer:4, unless the
peerNumAlgoForDidExchangeRequestsin DIDComm connection module config is explicitly set to another one.Note that this PR also applies some fixes found during this update:
routingparameter). I applied this todid_doc~attachas well, although in the RFC it does not explicitly mention that it should use the invitation keys. @TimoGlastra please let me know what's your interpretation for this.theirLabelfor the new connection with the value coming fromlabel, which is supposed to be the opposite (our label instead). So now we putaliasastheirLabel, as it usually happens to connections created with explicit invitations