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: support invitationDid when creating an invitation #1811

Merged

Conversation

TimoGlastra
Copy link
Contributor

This adds supporting for providing an invitationDid when creating an out of band invitation.

The idea is that you can provide this invitation did to as many oob invitations, to allow for connection reuse. The did will sitill be rotated in the connection response (so each connection still has a new did, mostly to make this PR simpler, I'm not sure if we should change that behaviour before DIDComm v2 where things will become simpler a lot).

It doesn't work yet sadly, as I use the invitationDid from the first connection to create the second OOB invitation, but it will error that the invitationDid from the first connection is not a valid did peer did.

It's kinda true I think as the did is generated a bit weirdly, it only contains the services, but no key references:

CredoError: Unable to resolve did document for did 'did:peer:2.SeyJzIjoicnhqczpmYWJlciIsInQiOiJkaWQtY29tbXVuaWNhdGlvbiIsInByaW9yaXR5IjowLCJyZWNpcGllbnRLZXlzIjpbImRpZDprZXk6ejZNa2hTVldiNUxwOVlNZ3B4ZGg1RlRnWlpOQlVZOTN0dWNjWU5CanE5cWY3eFVTI3o2TWtoU1ZXYjVMcDlZTWdweGRoNUZUZ1paTkJVWTkzdHVjY1lOQmpxOXFmN3hVUyJdLCJyIjpbXX0': notFound resolver_error: Unable to resolve did 'did:peer:2.SeyJzIjoicnhqczpmYWJlciIsInQiOiJkaWQtY29tbXVuaWNhdGlvbiIsInByaW9yaXR5IjowLCJyZWNpcGllbnRLZXlzIjpbImRpZDprZXk6ejZNa2hTVldiNUxwOVlNZ3B4ZGg1RlRnWlpOQlVZOTN0dWNjWU5CanE5cWY3eFVTI3o2TWtoU1ZXYjVMcDlZTWdweGRoNUZUZ1paTkJVWTkzdHVjY1lOQmpxOXFmN3hVUyJdLCJyIjpbXX0': CredoError: did did:peer:2.SeyJzIjoicnhqczpmYWJlciIsInQiOiJkaWQtY29tbXVuaWNhdGlvbiIsInByaW9yaXR5IjowLCJyZWNpcGllbnRLZXlzIjpbImRpZDprZXk6ejZNa2hTVldiNUxwOVlNZ3B4ZGg1RlRnWlpOQlVZOTN0dWNjWU5CanE5cWY3eFVTI3o2TWtoU1ZXYjVMcDlZTWdweGRoNUZUZ1paTkJVWTkzdHVjY1lOQmpxOXFmN3hVUyJdLCJyIjpbXX0 is not a valid peer did

While the Aries RFCS mention (note the endpoint AND key):

If a message has a service block instead of a DID in the services list, you may enable reuse by encoding the key and endpoint of the service block in a Peer DID numalgo 2 and using that DID instead of a service block.

So now I'm wondering whether this is the right approach? Should we allow to reuse the invitationDid? What would work is creating a did first (either did:peer or any other method) and also providing that to the first createInvitation call.

But you'd have to make sure that did document contains valid routing config for mediation and didcomm.

Alternative is to allow the same routing object to be passed to multiple createInvitation calls, but IMO routing objects shouldn't be reused, as it adds a lot of complexities in other places.

Using the same service in multiple invitations is tricky as we don't know then if the routing has already been used for a did. We could add checks for this, whether a did already exists, and if so use that, but I don' t like the magic behind it. So maybe next step is making it easy to generate a did beforehand, which you can then pass to all invitations you create?

Because even if we pass the invitationDid in the second invitation, in the first invitation we used an inline service so it doesn't really match (it will match for Credo, as the service -> did:peer:2 as invitationDid is EXACTLY the same, but i don't see that being true for any other implementation).

I feel like the inline services are great for interop (as you don't have to put a did method in the invitation, so it works with connection protocol and multiple peer did methods), but it makes the reuse flow a lot harder.

So my proposal would be:

  • pass routing -> converted to inline service object, not meant to be reused across invitations
  • pass invitationDid -> used as did in the service array. Meant to be reused across invitations and for connection reuse

What do you think?

@TimoGlastra TimoGlastra requested a review from genaris March 30, 2024 22:15
@TimoGlastra
Copy link
Contributor Author

@genaris you're a master on this front, your input is appreciated!!

@genaris
Copy link
Contributor

genaris commented Mar 31, 2024

So my proposal would be:

  • pass routing -> converted to inline service object, not meant to be reused across invitations
  • pass invitationDid -> used as did in the service array. Meant to be reused across invitations and for connection reuse

What do you think?

I totally agree on this. routing can be kept for some particular/advanced use cases, while invitationDid (a parameter simpler to understand for somebody who don't know Credo internals) can be used to achieve some correlation between different invitations.

As you said in your comment, it will be nice to make it easier to create DIDComm-capable Peer DIDs, so we don't need to write several lines of code to achieve that. Maybe an utility method or an optional parameter for PeerDidCreateOptions might help.

Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

I think your code is similar to the one I've working on for using public dids (which is working fine, but requires to manually set invitations as multiUse). I like the checks you've added on existing DIDs with the same routing keys and the fact that (if I understood correctly) you can create single use invitations, which is something important for instance if we want to create invitation codes for proof requests or credential proposals using a public DID.

Looking forward to get it merged soon!

packages/core/tests/oob.test.ts Show resolved Hide resolved
@genaris
Copy link
Contributor

genaris commented Apr 5, 2024

Hey @TimoGlastra do you want me to continue the work on this PR? I think we are close to have this ready for merging!

@TimoGlastra
Copy link
Contributor Author

Hey @TimoGlastra do you want me to continue the work on this PR? I think we are close to have this ready for merging!

Yes if you could pick it up that would be great :)

@TimoGlastra
Copy link
Contributor Author

So now I'm wondering whether this is the right approach? Should we allow to reuse the invitationDid? What would work is creating a did first (either did:peer or any other method) and also providing that to the first createInvitation call.

I think for this what if we do the following (can be in a follow up PR):

  • we update the process of calculating invitation from service to one that creates a valid did:peer:2 DID, this way you can use the invitation DID from previous OOB invitations for new invitations.
  • when checking if there's invitations already with a specific invitation did, we create both the legacy format, and the new format and check if there's a record for it.
  • (optionally) we do a migration to update all the old invitationDids to the new (correct format).

@TimoGlastra
Copy link
Contributor Author

Let me know if you have time to work on this @genaris, I have a need for this in the near future, so otherwise I might be able to spend some time on it :)

@genaris
Copy link
Contributor

genaris commented Apr 18, 2024

So now I'm wondering whether this is the right approach? Should we allow to reuse the invitationDid? What would work is creating a did first (either did:peer or any other method) and also providing that to the first createInvitation call.

I think for this what if we do the following (can be in a follow up PR):

  • we update the process of calculating invitation from service to one that creates a valid did:peer:2 DID, this way you can use the invitation DID from previous OOB invitations for new invitations.
  • when checking if there's invitations already with a specific invitation did, we create both the legacy format, and the new format and check if there's a record for it.
  • (optionally) we do a migration to update all the old invitationDids to the new (correct format).

I think this is the way to go: it is quite straightforward to implement (we just need to update outOfBandServiceToNumAlgo2Did to call createPeerDidDocumentFromServices), and we can do the trick you mention until we create the next major release (where we can migrate all existing ConnectionRecords to use the new format).

I'll add some commits to this PR following this approach.

@genaris genaris marked this pull request as ready for review April 24, 2024 23:36
@genaris
Copy link
Contributor

genaris commented Apr 25, 2024

I marked it ready for review, since it already implements the new out of band service calculation and it's doing the intended job of supporting did re-use. ConnectionRecord migration considering the new invitationDid encoding will be left for the next major release.

Copy link
Contributor Author

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Nice!!! 👍 some small comments but LGTM either way

packages/core/src/modules/connections/ConnectionsApi.ts Outdated Show resolved Hide resolved
packages/core/src/modules/oob/OutOfBandApi.ts Outdated Show resolved Hide resolved
Comment on lines +390 to +393
const [invitationDid] = [
...oobRecord.outOfBandInvitation.getDidServices(),
...oobRecord.outOfBandInvitation.getInlineServices().map(outOfBandServiceToInlineKeysNumAlgo2Did),
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this work when updating from 0.1-0.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand now 👍

@TimoGlastra TimoGlastra merged commit e5c6698 into openwallet-foundation:main Apr 25, 2024
12 checks passed
@TimoGlastra TimoGlastra deleted the feat/invitation-did branch April 25, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants