search

hyperledger / aries-vcx

aries-vcx is set of crates to work with DIDs, DID Documents, DIDComm, Verifiable Credentials and Hyperledger Aries.
https://didcomm.org
Apache License 2.0
125 stars 83 forks source link

[Feature] Support DIDExchange 1.1 #1228 #1230

Closed gmulhearn closed 5 months ago

gmulhearn commented 5 months ago

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 the complete, problemreport & request message structures, and some utility code for the dealing with the varying response messages.

Again, taking some creative liberty, i added an invisible version marker field on these common messages:

    #[serde(skip, default = "DidExchangeTypeV1::new_v1_1")]
    pub(crate) version: DidExchangeTypeV1,

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 the AriesMessage and DidExchange 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:

// in v1_x module
pub struct RequestContent<MinorVer> {
    ...,
    _marker: PhantomData<MinorVer>
}

// in v1_1 module
pub type RequestV1_1Content = RequestContent<DidExchangeTypeV1_1>;

// in v1_0 module
pub type RequestV1_0Content = RequestContent<DidExchangeTypeV1_0>;

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:

Fixes for AATH

gmulhearn commented 5 months ago

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:

> behave -D Acme=http://0.0.0.0:9020 -D Bob=http://0.0.0.0:9030 -t RFC0793
...
1 feature passed, 0 failed, 14 skipped
7 scenarios passed, 0 failed, 153 skipped
63 steps passed, 0 failed, 1320 skipped, 0 undefined

Aries VCX <-> Aries VCX:

RFC0023:

RFC0793*:

Exceptions

JamesKEbert commented 5 months ago

Awesome stuff! Been slowly working through the PR review 👍

requester impl will always try to request with a 1.1 request (and deal with it if the responder sends back a 1.0 response)

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.

gmulhearn-anonyome commented 5 months ago

@JamesKEbert good point, i had a go at this in my latest commit: https://github.com/hyperledger/aries-vcx/pull/1230/commits/5f5133d57b2a1a8207002a6afd0edc42efcf48d8

let me know what you think, i can always revert.

If this is using did exchange not via oob (not sure if we are/care to support)

This is the case when we send a request based off an implicit invitation (e.g. a public resolvable DID with a service)

gmulhearn-anonyome commented 5 months ago

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