DIDComm V2 Initial Implementation #2959
Conversation
|
@frostyfrog Some minor conflicts in this PR after #2892 was merged |
|
Why not return a DIDComm v2 problem report? |
|
I hadn't created a base message class for V2 problem reports to be based off of. My focus was on getting a response back that could be decoded on the other end. So I just used what was already available |
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
And move to binding to profile and session from default context Signed-off-by: Daniel Bluhm <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Micah Peltier <[email protected]> Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
TheTechmage
force-pushed
the
feat/didcommv2-proof-of-concept
branch
from
1a2e7a1 to
30107e8
Compare
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
There was a problem hiding this comment.
Some of these comments are requested changes. Others are notes for other reviewers to pay attention to. Others still are open questions. Probably not all of them need to be answered before I would feel good about merging this PR.
That being said, I had a hand in these changes so I defer to other ACA-Py contributors for a full review.
Nice work to this point, @frostyfrog and @mepeltier!
There was a problem hiding this comment.
Calling out a limitation: experimental v2 support will only work using did:peer:2 right now.
Should we implement it for did:peer:4 as well at this stage?
There was a problem hiding this comment.
Given how early we are in the process, I don't think its necessary, but at the same time, it might be worth getting it in before our changes are more solidified. It'll also probably make it easier to switch off of did:peer:2, which is a good thing imo
There was a problem hiding this comment.
I do believe that it's not limited to did:peer:2. Other did:peer methods should be supported as well. Once I have something better to key off of for outbound, things like did:web should work (in theory). Though you are right in that all did methods, aside from did:peer:2, are currently untested
| @abstractmethod | ||
| async def assign_kid_to_key(self, verkey: str, kid: str) -> KeyInfo: | ||
| """Assign a KID to a key. | ||
|
|
||
| This is separate from the create_key method because some DIDs are only | ||
| known after keys are created. | ||
|
|
||
| Args: | ||
| verkey: The verification key of the keypair | ||
| kid: The kid to assign to the keypair | ||
|
|
||
| Returns: | ||
| A `KeyInfo` representing the keypair | ||
|
|
||
| """ | ||
|
|
||
| @abstractmethod | ||
| async def get_key_by_kid(self, kid: str) -> KeyInfo: | ||
| """Fetch a key by looking up its kid. | ||
|
|
||
| Args: | ||
| kid: the key identifier | ||
|
|
||
| Returns: | ||
| The key identified by kid | ||
|
|
||
| """ |
There was a problem hiding this comment.
Calling out a fundamental paradigm shift starting here for ACA-Py: to work with DCv2, these changes start using full DID URL references to verification methods to identify keys instead of the base58 encoding of the public key.
And a limitation: There is only one kid associated with a key. Does it make sense for a key to be looked up by multiple different key ids?
There was a problem hiding this comment.
Now that I think on it, the way we implemented the interface, it doesn't actually preclude the ability to associate a key with multiple kids. But the implementation of the interface does impose this limitation.
| receipt.sender_verkey = message_unpack.sender_kid.split("#")[0] | ||
| receipt.recipient_verkey = message_unpack.recipient_kid.split("#")[0] |
There was a problem hiding this comment.
Calling out another start of a paradigm shift: sender and receiver are identified by DIDs instead of by base58 encoded public keys.
Should the sender/receiver be a kid rather than a DID? Technically, as seen in the unpack result metadata, it is fundamentally a kid that the message is sent to and received by. Perhaps it would be better to prefer keeping that specificity around until it is no longer needed.
| if sender_key and recipient_keys: | ||
| message = await messaging.pack( | ||
| message=message_json, | ||
| to=recipient_keys[0], |
There was a problem hiding this comment.
Should the pack method of DMP that we're using here support multiple recipients?
There was a problem hiding this comment.
It probably should. Another problem that I ran into is that it implicitly forward wraps the message, even if we are responding on the same connection. Something I planned to look at later
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
It was requested in today's ACA-Pug by @PatStLouis to add the didcommv2 extra to the docker images that are built, and multiple people in the call agreed with the idea. Myself included. Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
…of-concept Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
|
@mepeltier is in the process of putting together tests to pass the code coverage threshold SonarCloud is requiring. |
|
FYI. I'm pretty sure we can actually merge this with the sonarcloud check failing. So if we think there is adequate test coverage below 80% we should be able to go ahead. |
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
|
Gentle nudge for a review from @jamshale (so we don't have to keep merging upstream and resolving poetry lock conflicts 😅). I think the 61% coverage is adequate for the nature of these changes. Uncovered code is mostly configuration only used when the experimental flag is enabled. |
|
Day off in Canada. Will prioritize this for tomorrow morning. |
|
I don't know where the conflicts are coming from. I can't resolve them. If they get resolved we can merge this now. |
|
The conflicts are always occurring with the poetry lock. Let me re-lock it to "clear" the conflict (it's been happening for a while) |
Signed-off-by: Micah Peltier <[email protected]>
Signed-off-by: Micah Peltier <[email protected]>
|
Or, Micah will beat me to the punch! xD Should be good to go |
|
The purpose of this PR is to add basic DIDComm V2 support to ACA-Py. It is our intention to add support gradually with small PRs, rather than one massive PR. To that end, at present, here's what we've added:
--experimental-didcomm-v2flag to enable the DIDComm V2 code