refactor: keep single way to resolve peer dids

[Patrik-Stas]

PR #1097 performed changes to did resolution in did:peer:2 contexts. There's lack of clarity whether these changes should be accepted. This reverts the changes and opens discussion whether the changes in the #1097 should be accepted, or reverted (by merging this PR)

[Patrik-Stas]

Hi @gmulhearn-anonyome @xprazak2 @nain-F49FF806
can you folks have a look and share your thoughts which way you lean toward?

Let me know if you'd need some additional context or background. Not posting initially much so you can form opinion independently, without biases.

In general it's question of:

• option1: using shared general interface of DidResolvable (represented by this revert)
• option2: having additional resolution interface on the bottom-most component and make that public, eg. DidPeer<Numalog2 (represented by keeping Remove didcore generics, update did-exchange #1097 as is, no revert)

[codecov-commenter]

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (ec5bdd4) 0.05% compared to head (63dfdf4) 0.05%.

Files	Patch %	Lines
aries/aries_vcx/tests/test_did_exchange.rs	0.00%	12 Missing ⚠️
aries/aries_vcx/src/utils/didcomm_utils.rs	0.00%	11 Missing ⚠️
did_core/did_methods/did_peer/src/resolver/mod.rs	0.00%	5 Missing ⚠️
...change/state_machine/requester/request_sent/mod.rs	0.00%	3 Missing ⚠️
...hange/state_machine/responder/response_sent/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                        @@
##           diddoc/remove-generics-updates   #1106      +/-   ##
=================================================================
- Coverage                            0.05%   0.05%   -0.01%     
=================================================================
  Files                                 481     481              
  Lines                               24161   24180      +19     
  Branches                             4365    4367       +2     
=================================================================
  Hits                                   13      13              
- Misses                              24147   24166      +19     
  Partials                                1       1              

Flag	Coverage Δ
unittests-aries-vcx	0.05% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gmulhearn]

to be honest, i don't really have strong opinions after looking at this for 15mins. I understand the two options as follows:

1. PeerDid<Numalgo2>::resolve owns the logic for resolving a DidDocument and PeerDidResolver uses it. And most code holding PeerDid<_>s just use the PeerDid::resolve method, since it is more concise then constructing a PeerDidResolver (the conciseness is evident from the positive git diff on this)
2. PeerDidResolver owns the logic for resolving a DidDocument, and PeerDid<Numalgo2>::resolve is removed

IMO it makes sense if PeerDidResolver owns the main logic for resolving a DidDocument (option 2), however this option clearly bloats the code up. So maybe another option:
3. PeerDidResolver owns the main logic for resolving a DidDocument, and the PeerDid<Numalgo2>::resolve still exists, but uses the PeerDidResolver::resolve for resolving. That way, we still have the convenient PeerDid::resolve method for code conciseness, but PeerDidResolver still owns the resolution logic.

but also the problem with 3 is that now PeerDid::resolve is unnecessarily async (whereas in option 1 it is not).

[Patrik-Stas]

I like that. There's no logic duplication, both functions still refer to >>resolving<< (in my original-original version I distinguished "resolving" VS "decoding" which behaved little bit different) and for me, importantly, gives the ergonomic API in contexts where users work with PeerDid instances directly.

The resolve method would then look like this

impl PeerDid<Numalgo2> {
    pub async fn resolve(
        &self,
        public_key_encoding: PublicKeyEncoding,
    ) -> Result<DidDocument, DidPeerError> {
        let DidResolutionOutput { did_document, .. } = PeerDidResolver::new()
            .resolve(
                self.did(),
                &PeerDidResolutionOptions {
                    encoding: Some(public_key_encoding),
                },
            )
            .await?;
        Ok(did_document)
    }
}

and PeerDidResolver goes without modifications.

@mirgee what do you say?

[mirgee]

I say if you both like it then proceed, no need to ask me, who am I to stand in the face of majority anyway? :)

[gmulhearn]

Nice, yea again; I don't feel strongly about any of the options, they all have pros and cons.

Personally if I was consuming Aries-vcx and kept having to type out

let DidResolutionOutput { did_document, .. } = PeerDidResolver::new()
            .resolve(
                self.did(),
                &PeerDidResolutionOptions {
                    encoding: Some(public_key_encoding),
                },
            )
            .await?;
        Ok(did_document)

then I would just make my own helper function/method like PeerDid::resolve and put that code in there

[mirgee]

IMO DidPeerResolver should own the logic for resolving PeerDid into a DidDocument (with only one generic resolution interface which is oblivious to did method or numalgo); the problem with 3. is adding unnecessary dependency of PeerDid on DidPeerResolver. A helper function in a separate module is fine.

[Patrik-Stas]

I omitted my arguments or opinion upon starting this thread, which in nutshell is

• General interfaces for generic use cases, local APIs for local use cases
• Giving PeerDid users the only option through general DidResolvable interface is far less developer friendly
  • Need to deal with all the extra fluff which is useless for the particular need (no use of metadata, function is async even though it's in fact sync)
• The cost of adding fn resolve on PeerDid<N> is API surface expanded by additional function, I think well worth the benefits it brings consumers.

About the option 3:

• It does DidPeer dependent on DidPeerResolver, but it's encapsulated internal dependency, so I don't see this as much of a problem.
• What I see as main downside is rather it's still async even though it doesn't have to be, so not 100% achieving what I am after

You see, in fact the only thing this "DidPeerResolver" does is adding AlsoKnown field, which is even optional. If we were to decide not to do it, DidPeerResolver would in fact do completely nothing. The actual gore did peer resolution lies in decoding the DID ID string, not in adding alsoKnownAs to the decoded did document. The resolution largerly happens in DidPeer itself either way, it's just a question of whether there's exposed additional catered optimized API, or only 1 general interface.

I am opinionated about this not simply because of this particular issue (I could just put helper function to both aries-vcx and aries-vcx-agent and call it a day), but rather for the precedent of how we think about things like this, and what's the approach to APIs (I am quite sure same way goes for others).

I am favoring the mentioned variants in order of preference:

1. option 2
2. option 3
3. option 1

I believe we all understand the options, the trade offs and philosophies between the different options so I see no way out than doing this democratically. Please either write yours or say you abstain from the vote.

@nain-F49FF806 @xprazak2 if you'd like, feel free to join the battlefield here :-D

[mirgee]

I abstain from the vote then.

[nain-F49FF806]

Hi, I took a stroll through

• Aries RFC 0627: Static Peer DIDs
• Peer DID Method Specification
  for context, since I haven't actively interacted with did:peer before.

What I think:

1. Method specific resolution is not sufficient, so better to have a general, common resolution interface.

What I see is that there is a general expectation of flexibility on reading (/resolving?) side.

..MUST accept peer DIDs that use any of these methods
..MUST give peer DIDs that use at least one of these three methods

So having a general, common interface for resolution makes more sense (but may not be so relevant for generation).

The generation code can be modular, and distributed separately for each method. But any resolution is incomplete (in Aries context) without supporting multiple methods. So why separate the resolution logic?

This would perhaps indicate to go with something like @mirgee suggests here.

2. did:peer:4 may require a dedicated resolver due to storage requirement.

Please correct me if I am mistaken on this. My reading of method 4 makes me think it'll be necessary to have some storage to 'remember' mapping of short -> long form representations of did:peer4. In that case, having the resolver external could be necessary/more ergonomic.

Otherwise, we may need to add some storage interface argument to PeerDid's resolve method.
And a storage/IO requirement in any form would force our hand in terms of async anyway.

Conclusion: I like the convenience of having a resolve method local to the PeerDid<_> struct, but if it's not a consistent interface across methods, or has gotchas … then I'd prefer a dedicated revolver interface instead.

Currently, given the above considerations, I would vote for Option 3.

---

p.s: If we consider separating the generation into a separate crate, then Option 1 would help avoid the dependency on DidResolvable/DidPeerResolver. But since currently it's in the same crate, the internal dependency doesn't cost much IMO, and Option 3 does give an added benefit.

p.p.s: Personally, I am drawn towards Option 1 for it jives with one of the guiding principles of PEP 20

There should be one-- and preferably only one --obvious way to do it.

[Patrik-Stas]

Hi @nain-F49FF806 thanks a lot for taking time to go through this! Without previous context and involvement, this is quite a chunk to swallow!

Please correct me if I am mistaken on this. My reading of method 4 makes me think it'll be necessary to have some storage to 'remember' mapping of short -> long form representations of did:peer4. In that case, having the resolver external could be necessary/more ergonomic.

It is a good reminder - for did:peer:4* resolution to be complete, it would need access to some state (and perhaps not just ready, but also writes to enable resolution of future, shortened form of did:peer:4 DIDs), so it wouldn't in fact be much more of a win to have sync resolve() defined for PeerDid<Numalgo4> like it can be done for PeerDid<Numalgo2> (which is always "self-encoding" and synchronous).

You've expressed favor in both 1 and 3 - to prevent any misunderstanding, what's your vote in order of preference?

[nain-F49FF806]

@Patrik-Stas Sorry for the ambiguity. I don't feel qualified to distinguish between the two clearly, since I am not currently a user and can't judge the practical benefits of working at the lower level. So I choose to abstain here.

In a general though, for external (public) interface, I would prefer simplicity and having one good way to do things (unless there is strong benefit otherwise).

[Patrik-Stas]

I want to wrap this up so despite your official abstain so:

• Despite official abstaining from vote, I count mirgee's vote for option 1 given strong held opinions expressed in other channels
• Naian seem bit on the fence, but given the nuance response, it seem to gravitate more toward option 1
• George not strongly opinionated but suggested option 3
• My top preference is for option 2 as I explained why from my perspective option 3 is not achieving goal I am after in my option 2 suggestion

Thank you all for participation! This PR applies option 1 and is now ready for review & merge.