-
Notifications
You must be signed in to change notification settings - Fork 83
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
[Feature] Support DIDExchange 1.1 #1228 #1230
Conversation
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Some notes on AATH testing: Aries VCX (ACME) <-> ACApy (BOB):RFC0023:
RFC0793:
Aries VCX (BOB) <-> ACApy (ACME):RFC0023:
RFC0793*:
this one felt good, full suite:
Aries VCX <-> Aries VCX:RFC0023:
RFC0793*:
Exceptions
|
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
…ing) Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
aries/aries_vcx/src/protocols/did_exchange/state_machine/requester/request_sent/mod.rs
Outdated
Show resolved
Hide resolved
aries/aries_vcx/src/protocols/did_exchange/state_machine/requester/request_sent/mod.rs
Show resolved
Hide resolved
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Awesome stuff! Been slowly working through the PR review 👍
We might be able to make the default be based on what is specified in a given OOB invite handshake protocols array. If they list 1.0, then that's what we use. If they specify 1.1 then we use 1.1. If this is using did exchange not via oob (not sure if we are/care to support) then defaulting to 1.1 makes sense. Although I suppose it should work fine to use just 1.1 by default and then downgrade to 1.0 if that's what gets returned in the response. And that might be better as that means we try and use 1.1 always first. |
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@JamesKEbert good point, i had a go at this in my latest commit: 5f5133d let me know what you think, i can always revert.
This is the case when we send a request based off an implicit invitation (e.g. a public resolvable DID with a service) |
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.
Awesome stuff!
I definitely agree with the choice to have a common module for all the messages, as it's really just an extra field and does not deserve an entire new full gambit of protocol folder/messages/etc.
I have some concerns about the VCX message structures though, in particular relating to versions and invisible markers on Aries messages--I'd much rather we store the full version with all of the messages. Cause I can also agree that having multiple versions and needing to create AnyXYZ
wrappers is gross. Chatting about it on the community call might be a good idea
aries/aries_vcx/src/protocols/did_exchange/state_machine/responder/response_sent/mod.rs
Show resolved
Hide resolved
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
As discussed on call, i'm changing the approach to NOT have the invisible version marker inside the message. instead we create AnyXYZ wrappers, which have the minor version metadata externally in the enum variants |
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.
Good to go from my POV 👍
This PR closes #1228 by implementing DIDExchange v1.1 support. It also does alot of DIDExchange/OOB/AATH fixes of bugs i found along the way whilst testing this implementation in the AATH setup against itself and against acapy.
DIDExchange v1.1 Implementation
Messages
The previous message structure for DIDExchange were scoped to v1.0. This implementation creates v1.1 message structures to sit alongside v1.0. The approach taken largely follows the guide laid out in the README. This is also the approach i did previously whilst creating issue-credential-v2 & present-proof-v2 message structures.
Where i took a bit of creative liberty though, was with creating a common module called
v1_x
. I felt this was necessary, as all messages in DIDExchange v1.1 & v1.0 are the exact same, except for the RESPONSE message (which simply adds the did_rotate field). So the v1_x module contains the common definitions for thecomplete, problemreport & request
message structures, and some utility code for the dealing with the varyingresponse
messages.Again, taking some creative liberty, i added an invisible
version
marker field on these common messages:This field is invisible to the consumer and to the JSON serialized format, however it's a run-time marker that allows us to convert from and to the
AriesMessage
type without losing the context of which DIDExchange version the message is. The unit tests ensure this is as expected. I modified theAriesMessage
andDidExchange
de/serializers to deal with setting and using this field properly.I also played with another way of doing this, which used a compile-time marker for V1.0 vs V1.1 messages, it looked like this:
this way, at compile time, there is 2 distinct types for v1.1 vs v1.0 messages. However, this got a bit ugly when actually using the implementation, as we need to create
AnyXYZ
wrappers for the two variants of each message, and pass those around everywhere, and then all consumers would need to switch/match on the different variants. I preferred the run-time way, but open to the other way.Implementation (State machine)
The implementation did not have to change dramatically. essentially:
AnyResponse
instead ofResponse
label
- bcus why notFixes for AATH
didcomm_utils
which were a bit slackresolve_base58_key_agreement
not being clear on it's purpose of fetching ed25519 base58 keysresolve_base58_key_agreement
not checking that the keys it fetched were ed25519get_ed25519_base58_routing_keys
^get_ed25519_base58_recipient_keys
, which resolves keys referenced in a given diddoc serviceget_ed25519_base58_recipient_keys
(yields more accurate result than just "get the first key agreement key" as was previously done and is now used as a backup)text/string
. i'm not sure if it's valid, but acapy uses it, and so does the DIDExchange spec.MaybeKnown
for theMimeType
when deserializing. Without this, it would fail to deserialize the whole attachment/message if the mimetype is unrecognised (i ran into this with acapy'stext/string
.)dereference_key
DIDDoc utility will check ALL places that an embedded VerificationMethod could be. Rather than just the "normal" case where it is embedded inverificationMethod
field.MultiKey
, which seems to be the preferred type in the did:peer spec. Also expanded the.public_key
method to know how to resolve this VM type into aKey
.append_encoded_key_segments
was adding some encoded VMs twicerecipientKeys
(acapy uses this correctly))to_did_url_reference
), i'm not sure where this idea came from, but the spec defined way is to just index the keys#key-N
(nth_key_did_url_reference
).