Connection Handler from_parts constructor

[gmulhearn-anonyome]

Part of #742 - pending validation of issue from vcx team.

[codecov-commenter]

Codecov Report

Merging #743 (f1e07c0) into main (66ee076) will decrease coverage by 0.67%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
- Coverage   54.73%   54.06%   -0.67%     
==========================================
  Files         363      363              
  Lines       37122    37132      +10     
  Branches     8089     8074      -15     
==========================================
- Hits        20317    20077     -240     
- Misses      10832    11111     +279     
+ Partials     5973     5944      -29     

Flag	Coverage Δ
unittests-aries-vcx	53.97% <0.00%> (-0.66%)	⬇️

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

Impacted Files	Coverage Δ
aries_vcx/src/handlers/connection/connection.rs	56.80% <0.00%> (-1.37%)	⬇️
libvdrtools/src/domain/ledger/did.rs	0.00% <0.00%> (-27.09%)	⬇️
...tools/src/domain/anoncreds/requested_credential.rs	21.05% <0.00%> (-21.06%)	⬇️
libvdrtools/src/domain/ledger/attrib.rs	16.00% <0.00%> (-16.00%)	⬇️
libvdrtools/src/utils/logger.rs	0.00% <0.00%> (-12.00%)	⬇️
aries_vcx/src/indy/proofs/prover/prover.rs	38.94% <0.00%> (-11.58%)	⬇️
libvdrtools/src/controllers/anoncreds/verifier.rs	64.00% <0.00%> (-8.00%)	⬇️
libvdrtools/indy-utils/src/crypto/hash/openssl.rs	42.30% <0.00%> (-7.70%)	⬇️
agency_client/src/lib.rs	64.70% <0.00%> (-5.89%)	⬇️
aries_vcx/src/indy/credentials/holder/mod.rs	50.00% <0.00%> (-5.72%)	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[gmulhearn-anonyome]

Hmm. It may not actually be this simple... As a consumer cannot fully construct a SmConnectionState themselves without having a serialized copy of it...

This is because the states submodule of protocols::connection::[invitee | inviter] is private. Meaning that consumers cannot construct the states within InviteeFullState and InviterFullState.

e.g. SmConnectionState::Invitee(InviteeFullState::Invited( InvitedState { ... } )) cannot be constructed by consumers, since InvitedState is a private struct...

@Patrik-Stas I'm unsure of the best solution to this? ^, we could maybe make that states submodule public, are there any consequences to this? - this is all assuming that you agree with me in believing that handlers should have an alternative mechanism for construction other than serialization, which you may not agree with. Keen to hear your opinions 👌

[Patrik-Stas]

Hi @gmulhearn-anonyome I am afraid I didn't properly communicate the Connection refactor PR #739 as under the current plan, this PR #743 wouldn't apply anymore, as new Connection implementation is quite different.
Although #739 was originally set-up as addition only, given the short history of non-mediated Connection, we've decided to apply the refactor directly on it (instead of maintaining 2 implementations of Connection)

If this comes as too sudden breaking change, we can keep Connection (just rename it perhaps), but long term #739 seems superior to the current approach for state machines.

Let's first clarify this and then we can address the actual proposal / problem you are trying to solve.

[gmulhearn]

Oh nice. Thought that might be the case! Ok no worries, I'll need to check in that MR if my issue in connections has been fixed. It ultimately comes down to whether or not the states can be manually reconstructed.

Regardless the issue I created still applies for other handlers in vcx. (I.e. they're missing constructors). So let me know if you agree that's an issue

[Patrik-Stas]

@gmulhearn
Just to make sure to get it 100% straight, let's look at aries-vcx serialized Verifier

{
    "verifier_sm": {
      "source_id": "1",
      "thread_id": "0a794e24-c6b4-46bc-93b6-642b6dc98c98",
      "state": {
        "PresentationRequestSent": {
          "connection_handle": 3330318696,
          "presentation_request": {
            "@id": "testid",
            "comment": "default wants you to share Optional",
            "request_presentations~attach": [
              {
                "mime-type": "application/json",
                "@id": "libindy-request-presentation-0",
                "data": {
                  "base64": "eyJuYW1l....IjEuMCJ9"
                }
              }
            ]
          }
        }
      }
    }

I suspect in this case, the 3 parts you would like to get is (source_id, thread_id, state), and serialize it on your own, presumably in simpler format skipping the redundant top level "verifier_sm" node, and have it serialize into something like:

{
      "source_id": "1",
      "thread_id": "0a794e24-c6b4-46bc-93b6-642b6dc98c98",
      "state": {
        "PresentationRequestSent": {
          "connection_handle": 3330318696,
          "presentation_request": {
            "@id": "testid",
            "comment": "default wants you to share Optional",
            "request_presentations~attach": [
              {
                "mime-type": "application/json",
                "@id": "libindy-request-presentation-0",
                "data": {
                  "base64": "eyJuYW1l....IjEuMCJ9"
                }
              }
            ]
          }
        }
      }

Is that right?

I think your proposal makes totally sense given the current state of art. There's few issues few current default serialization formats so I can see why you would want to serialize on your own into a different one.
I think we can go forward adding the from_parts, into_parts kind of API to other protocol handlers in their current form.

Keep in mind that in relatively soon future (I think in terms of weeks) we would like to deprecate current state machines in favor of their new reimplementation using #739 approach - but this can be separate discussion as how this can be done in a sensible manner to prevent causing too much disruption.

It's actually great you bring this up, cause I find de/serialization as one of the top weaknesses of aries-vcx and along with the #739 topic of de/serialization has been brought up quite a bit and your proposal this PR/linked issue confirms that further. We have a great opportunity to get that fixed while building new #739 state machines. I'll create issue to address current issues with serialization. I think if we make the serialization format better, you perhaps wouldn't need to come up with custom de/serialization in a first place.

[Patrik-Stas]

I've created #745 as proposal for new version of serialization formats. Do you think the proposed format would remove the need to create your own?

Also, I'd like to summon @mirgee @bobozaur what do you guys think?

[gmulhearn]

Yes true, but IMO it would be ideal also if consumers would be able to use constructors to re-construct their handlers from the barebone data. Would be nice if they could not rely on aries-vcx serialized data at all when constructing. This is because consumers could be migrating data from other sources (e.g. another aries agent), or they are storing their protocol state/data in their own format (which IMO is best practice, as aries-vcx serialization of handlers may not be the exact same as it is now - e.g. in years time from now a serialized Verifier is not guaranteed to be formatted the same).

So taking the Verifier handler example, if the handler had a from_parts(source,thread,state) constructor it would almost solve the issue. I say almost because a state: VerifierFullState cannot truly be constructed manually by consumers (without serialization). The reason it can't be constructed is because within VerifierFullState the inner data is only accessible internally in the aries-vcx crate:

pub enum VerifierFullState {
    Initial(InitialVerifierState),
    PresentationRequestSet(PresentationRequestSetState),
    PresentationProposalReceived(PresentationProposalReceivedState),
    PresentationRequestSent(PresentationRequestSentState),
    Finished(FinishedState),
}

where InitialVerifierState, PresentationRequestSetState, etc are within an internal-only aries-vcx module.

This is the case for all handlers as well, even if they had the from_parts constructors, their internal state structs are not publicly constructible.

So I guess the quick-and-easy solution is to add from_parts (or whatever method name is preferred) constructors to handlers and make all these state objects available for construction externally (e.g. making PresentationRequestSetState etc all publically accessible structs for constructing). However, unsure if this would be the cleanest approach? - IMO it's not too controversial giving consumers public access to those structs, as they still indirectly have public access to them via serialization/deserialization.

[gmulhearn]

@bobozaur changes somewhat solve this problem in connection as well, as all the state objects now have a publicly accessible ::new(..) methods so that consumers can manually construct them.

here's two examples on bobozaur's branch of how a consumer can construct handler's using primitive SSI data which would likely be storing anyway:
(please note that i did have to change bobozaur's from_parts construct from pub(crate) to pub for this example.

    let conn = Connection::<Invitee, Invited>::from_parts(
        String::new(),
        PairwiseInfo {
            pw_did: my_did,
            pw_vk: my_verkey,
        },
        Invitee,
        Invited::new(did_doc, invitation),
    );

    let conn = Connection::<Inviter, inviter::states::requested::Requested>::from_parts(
        String::new(),
        PairwiseInfo {
            pw_did: my_did,
            pw_vk: my_verkey,
        },
        Inviter,
        inviter::states::requested::Requested::new(signed_response, did_doc),
    );

So in this example a consumer is able to re-construct their Connection handler using the base SSI primitive data which they would be likely to storing and managing themselves in their own agent.

[gmulhearn]

If this thread is getting too much, would be good to continue the discussion in this week's call if possible 👌

[bobozaur]

I think a discussion in the weekly call is the best way to go about it, but I'll also provide my two cents here:

I believe we should not guarantee a serialization format of the connection and even discourage people from directly accessing the serialized format. Things can still change in terms of the format and it would be best not to make a promise like this unless we are completely sure we won't ever break it. But even then, direct access should not be encouraged and rather working with getters.

I'm quite torn between whether we should provide the additional constructor or not. @gmulhearn makes a good point that people can modify the serialized connection and deserialize to get a different state if they really want to and maybe migrating a connection from a different agent does make sense. However, the ability of constructing an arbitrary state sounds like looking for trouble.

But it might be just an irrational thought. After all, even if a connection is constructed improperly, it doesn't really do much without the counterpart connection of the other agent being in an improper state. So we might as well make it easy and less-error prone by adding from_parts and into_parts methods instead of having people implement some sort of connection replicas just for serialization/deserialization into different states.

[gmulhearn]

Yeah exactly ^. Without going into the nitty gritty right now, we have a particular use case where a handler needs to be restored via protocol data being stored in our own format, and the only way to do that currently is via the connection replica + serialization/deserialization hacky strategy mentioned by Bogdan ^.

Of course my particular use case should not inform aries-vcx design, however I imagine this use case will not be that uncommon to anyone building an agent with persistent storage onto of aries-vcx. And IMO since the goal can be accomplished in a hacky/dangerous way currently, we may as well just make it possible in a less hacky and type-safe way via providing constructors.

anyway, keen to discuss this further in the call

[gmulhearn]

handled in typestate PR