-
Notifications
You must be signed in to change notification settings - Fork 204
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(core): add support for multi use invitations #460
feat(core): add support for multi use invitations #460
Conversation
Signed-off-by: Timo Glastra <[email protected]>
592d94e
to
956af3e
Compare
Codecov Report
@@ Coverage Diff @@
## main #460 +/- ##
==========================================
+ Coverage 85.65% 85.76% +0.10%
==========================================
Files 253 253
Lines 5467 5480 +13
Branches 878 884 +6
==========================================
+ Hits 4683 4700 +17
+ Misses 783 779 -4
Partials 1 1
Continue to review full report at Codecov.
|
Could someone review please? |
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.
Not too knowledgeale with the multi use specifications, but code-wise it looks good!
@@ -194,6 +199,25 @@ export class ConnectionService { | |||
throw new AriesFrameworkError('Invalid message') | |||
} | |||
|
|||
// Create new connection if using a multi use invitation | |||
if (connectionRecord.multiUseInvitation) { |
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.
Could be combined with the if statement below.
Nice and long-awaited feature! |
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.
Nice improvement. Good job, Timo 👍 I lack domain knowledge here, so I added a few notes more from a curiosity perspective :)
const faberConnection = await faberAgent.connections.getById(faberConnectionId) | ||
// Expect initial connection to still be in state invited | ||
return expect(faberConnection.state).toBe(ConnectionState.Invited) | ||
}) |
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.
Do I understand correctly that faberAgent
will have 3 connection records at the end of the process?
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.
Yes! The first record will always stay in state Invited
and we create new records when people send requests for the invitation. This is how both ACA-Py and .NET have also implemented it
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.
Interesting 🤔
if (messageContext.connection.multiUseInvitation) { | ||
const mediationRecord = await this.mediationRecipientService.discoverMediation() | ||
routing = await this.mediationRecipientService.getRouting(mediationRecord) | ||
} |
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.
Is routing really necessary for multi-use invitations in general? 🤔
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 not. But I would rather take all edge cases into account for these things. Otherwise we need good documentation on what features do and don't work together :)
Signed-off-by: Timo Glastra [email protected]