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: find existing connection based on invitation did #698

Merged
merged 3 commits into from
Apr 13, 2022
Merged

feat: find existing connection based on invitation did #698

merged 3 commits into from
Apr 13, 2022

Conversation

jakubkoci
Copy link
Contributor

I added a function serviceToNumAlgo2Did but I'm testing it in DidPeer.test, because I was curious if it's possible to create regular PeerDid and then take didDocument from it. As you can see it's not possible right now so I commented the code in the test.

I described what might be a cause of the problem in issue here RFC 0434: Ambiguous description of Peer DID numalgo 2 service encoding.

There is also a simplification of when and what invitation did we store in the connection record. OOB invitation can contain more dids (or service blocks encoded as peer did numalgo2) and we should store only the one we successfully use to send connection request message to.

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
@jakubkoci jakubkoci requested a review from a team as a code owner April 7, 2022 08:50

import { DidDocumentBuilder, Key } from '.'

export function createDidDocumentFromServices(services: DidCommService[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's verify this approach in the Aries WG, and if we have an answer we can add some documentation to the RFC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But we don't have to keep this PR open until then right?

Copy link
Contributor

Choose a reason for hiding this comment

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

No fine to merge!

Comment on lines +474 to 481
if (connections.length === 1) {
const [firstConnection] = connections
return firstConnection
} else if (connections.length > 1) {
this.logger.warn(`There is more than one connection created from invitationDid ${did}. Taking the first one.`)
const [firstConnection] = connections
return firstConnection
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (connections.length === 1) {
const [firstConnection] = connections
return firstConnection
} else if (connections.length > 1) {
this.logger.warn(`There is more than one connection created from invitationDid ${did}. Taking the first one.`)
const [firstConnection] = connections
return firstConnection
}
if (connections.length > 1) {
this.logger.warn(`There is more than one connection created from invitationDid ${did}. Taking the first one.`)
}
// firstConnection will be undefined if length of connections array is 0
const [firstConnection] = connections
return firstConnection

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'd rather be more explicit in the code than add a comment.

Comment on lines +151 to +157
if (recipientKey.startsWith('did:key')) {
didKey = DidKey.fromDid(recipientKey)
} else {
const publicKeyBase58 = recipientKey
const ed25519Key = Key.fromPublicKeyBase58(publicKeyBase58, KeyType.Ed25519)
didKey = new DidKey(ed25519Key)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add this to the DidCommService class? I think we'll have to do this more often and if we can just add recipientKeysPublicKeyBase58 and also an recipientKeysDids (how to name this?) we don't have to guess if the value starts with did:key anywhere else in the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe, it seems there is just one place, so I don't see significant benefits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be more related to the PR #700

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comments on your other PR. To mee it looks like we're doing it quite often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's work on it there then.

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
@jakubkoci jakubkoci merged commit b153acf into openwallet-foundation:feat/out-of-band Apr 13, 2022
jakubkoci added a commit that referenced this pull request Apr 15, 2022
* Extract creation of did doc from services
* Create peer did from service and store it as invitation did

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
jakubkoci added a commit that referenced this pull request Apr 26, 2022
* Extract creation of did doc from services
* Create peer did from service and store it as invitation did

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
jakubkoci added a commit that referenced this pull request May 12, 2022
* Extract creation of did doc from services
* Create peer did from service and store it as invitation did

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
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