-
Notifications
You must be signed in to change notification settings - Fork 202
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
feat: find existing connection based on invitation did #698
Conversation
Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
packages/core/src/modules/dids/domain/createPeerDidFromServices.ts
Outdated
Show resolved
Hide resolved
|
||
import { DidDocumentBuilder, Key } from '.' | ||
|
||
export function createDidDocumentFromServices(services: DidCommService[]) { |
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.
Let's verify this approach in the Aries WG, and if we have an answer we can add some documentation to the RFC.
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. But we don't have to keep this PR open until then 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.
No fine to merge!
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 | ||
} |
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.
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 |
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'd rather be more explicit in the code than add a comment.
if (recipientKey.startsWith('did:key')) { | ||
didKey = DidKey.fromDid(recipientKey) | ||
} else { | ||
const publicKeyBase58 = recipientKey | ||
const ed25519Key = Key.fromPublicKeyBase58(publicKeyBase58, KeyType.Ed25519) | ||
didKey = new DidKey(ed25519Key) | ||
} |
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 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
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.
Hmm, maybe, it seems there is just one place, so I don't see significant benefits.
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.
It might be more related to the PR #700
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.
See my comments on your other PR. To mee it looks like we're doing it quite often.
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.
Let's work on it there then.
Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
* 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>
* 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>
* 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>
I added a function
serviceToNumAlgo2Did
but I'm testing it inDidPeer.test
, because I was curious if it's possible to create regular PeerDid and then takedidDocument
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.