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

[FIX] Loosen "version" restriction on PresentationRequestPayload #1212

Closed gmulhearn-anonyome closed 5 months ago

gmulhearn-anonyome commented 5 months ago

Currently our anoncreds_types crate (which mostly mirrors anoncreds-rs), has an additional restriction on the presentation request payload: it requires that the inner "version" field is either "1.0" or "2.0".

However, the real anoncreds-rs types do not require this. They allow any string: https://github.com/hyperledger/anoncreds-rs/blob/main/src/data_types/pres_request.rs#L18.

The anoncreds spec also seems to imply that the version field is whatever the verifier wants it to be: https://hyperledger.github.io/anoncreds-spec/#create-presentation-request .

I think perhaps we were confusing this field version with the other field ver. which does seem to be required as "1.0" or "2.0": we have correctly implemented that though.

I've kept a default of "1.0" in the builder for backwards compatibility

gmulhearn-anonyome commented 5 months ago

Note that i encountered this bug when sending a proof request from acapy to vcx with version of "1"

JamesKEbert commented 5 months ago

I've asked for some clarification on this in the #anoncreds Discord channel, as I suspect that you're right, but I feel like the spec is vague in what the actual difference is between these two fields.

My only other question would be -- would we want to align completely with what is in AnonCreds-rs, as we want to migrate to using that in the future anyways? The answer can also be that that's a problem for later us :wink: :sweat_smile:

gmulhearn-anonyome commented 5 months ago

@JamesKEbert thanks for seeking clarity on it. Yea i suspect the version is for verifier specific versioning. e.g. a verifier could use the name + version field to create some system around some standardized proof requests that they are known for sending out. Not sure if i'm sold, but either way, i'm fairly sure it's not meant to be locked to 1.0 or 2.0, anoncreds-rs is somewhat a source of truth here

gmulhearn-anonyome commented 5 months ago

My only other question would be -- would we want to align completely with what is in AnonCreds-rs, as we want to migrate to using that in the future anyways? The answer can also be that that's a problem for later us 😉 😅

yea i think it's a problem for later. I'll need to revise, but i believe the vcx guys created the anoncreds_types as a thin layer of "anoncreds" type definitions that can be used across the whole repo. The layer is essentially a copy paste of the types within the anoncreds-rs repo, however the motivation is that it does not carry with it the other dependencies of anoncreds-rs (making it a thinner layer).

It also seemed 'cleaner' to have public functions within the aries-vcx repo return types that were owned by the aries-vcx repo (as opposed to returning types from anoncreds-rs). If we were to fully lean into anoncreds-rs, then we could potentially remove the thin anoncreds_types crate within aries-vcx, and just import and use the types from anoncreds-rs everywhere... Noting the bloat that would bring... IMO it would be nice if the anoncreds-rs repo would expose a thin crate called anoncreds_data_types, which has nothing but type definitions (like our anoncreds_types, but official.

gmulhearn-anonyome commented 5 months ago

for reference, the PR description of anoncreds_types somewhat covers the motivation: https://github.com/hyperledger/aries-vcx/pull/1116

JamesKEbert commented 5 months ago

yea i think it's a problem for later.

Makes sense to me :saluting_face:

Thanks for the context, quite helpful :+1:

I lean towards getting the types crate contributed upstream or removed entirely, as the maintenance costs of making sure the layer stays inline with the upstream can be brutal when it is not co-located with the code it's typing or wrapping (in my experience at least, although anoncreds-rs isn't changing nearly as frequently as my past experiences). I could definitely see an argument being brought to the anoncred's maintainers--but I'd like to get a little bit of a better handle on Aries VCX before a conversation is broached on that topic there IMO.

JamesKEbert commented 5 months ago

Followup question--might be a dumb one. You mentioned:

The layer is essentially a copy paste of the types within the anoncreds-rs repo, however the motivation is that it does not carry with it the other dependencies of anoncreds-rs

Aren't we already going to be consuming anoncreds-rs, so it wouldn't really matter whether the types are separate or not given that we're already including anoncreds-rs? I could see it helping if we were keeping credx around long-term, but we're not (unless there's needs there I don't know about)

gmulhearn commented 5 months ago

Aren't we already going to be consuming anoncreds-rs, so it wouldn't really matter whether the types are separate or not given that we're already including anoncreds-rs?

Yea good question. IMO, it depends on how modular you see the components of this aries-vcx repo. Aries-vcx over the past months/year has gone thru an effort to modularize components into their own crates. And then crates such as aries_vcx (and further up the chain too: aries_vcx_agent, aath_backchannel, etc) bring those individual components together and consume them.

Aside; sorry if i'm repeating stuff you already know, but i'll lay it down for context sake..

The "core" components of the aries-vcx/aries stack are aries_vcx_anoncreds, aries_vcx_ledger, aries_vcx_wallet. Each of these core components exposes interface/trait/s full of functions related (e.g. the BaseAnonCreds trait full of anoncreds related functions). Each of these core components also provides some implementations of the interface, implemented using a different library.

Each of these components also have compile flags (rust feature flags) on them. particularly the flags are for the different implementation variants. So you can choose for each component whether you want to compile aries_vcx_anoncreds with the anoncreds-rs variant, the credx variant, or both, or neither, etc.

For instance, i'm currently consuming the aries_vcx_anoncreds & aries_vcx_wallet crates with no features/implementations included. This is because i implemented BaseWallet & BaseAnonCreds myself (i did this before aries-vcx had implementations for askar wallet and anoncreds-rs anoncreds, i may consider migrating to use the aries-vcx version eventually).

When you compile with one of these feature flags, it will avoid the particular dependencies being pulled into your rust build. e.g. consuming the aries_vcx_wallet with only the askar feature flag will avoid bringing in the legacy indy dependency.

An example i like to think about for why someone might want aries_vcx without any of the core component implementations, is if they are using some sort of multi-tenant cloud wallet for their functionality, and just using aries_vcx to run the protocols. So they would implement the basewallet, baseanoncreds and ledger traits themselves, and their implementation would do some HTTPs comms to their cloud wallet backend to process it.

another example: maybe they are building a solution in the browser, and certain crates (anoncreds-rs, indyvdr, askar) will not work/compile in the browser. this user would want to exclude all these dependencies when compiling aries_vcx, and they'd implement the traits themselves using whatever they can in the browser context.

With all that context rambling out the way:

internally in the aries-vcx repo, we are using our anoncreds_types crate in many places/crates:

So if we switch from our anoncreds_types to using the real anoncreds-rs types, then all these crates would now be required to depend on anoncreds-rs. So if you re-think about the hypothetical users i mentioned above, if anoncreds-rs doesn't compile for in-browser usage, they are screwed.

note: this is all very hypothetical, there is probably other issues currently with compiling aries_vcx for web, and maybe the pure anoncreds_types already doesn't compile for web. But hopefully gives context on why the aries-vcx repo has been trying to avoid forcing dependencies on end-users.

With all that being said, IMO the dream solution might be if anoncreds-rs repo had a seperate lightweight crate dedicated to the pure types, which they consume within anoncreds "core", and we can use to replace our anoncreds_types crate.

gmulhearn commented 5 months ago

This is definitely something we should discuss in the next call, i can probably keep rambling for awhile

JamesKEbert commented 5 months ago

Some really awesome thoughts! I can definitely agree on the value behind the traits/interfaces to allow for various implementations/flexibility.(We did a similar exercise in Credo/AFJ)

What I hadn't thought of was why someone who wants to use AnonCreds wouldn't want to just use anoncreds-rs straight up? And I feel like your browser use case is a compelling argument there that I hadn't thought of.

This is definitely something we should discuss in the next call, i can probably keep rambling for awhile

It's really good rambling! So definitely agree this would be a great discussion for the next call. 👌

JamesKEbert commented 5 months ago

Aside; sorry if i'm repeating stuff you already know, but i'll lay it down for context sake..

And context is quite appreciated! Especially in a project that is as big as this one is and with as much history as it has.

JamesKEbert commented 5 months ago

I have not heard a response back on Discord, but I think we should move forward as this approach is matched in both Credo and ACA-Py: https://github.com/openwallet-foundation/credo-ts/blob/main/packages/anoncreds/src/models/AnonCredsProofRequest.ts#L56 https://github.com/hyperledger/aries-cloudagent-python/blob/1da987291d36e8386e035f123d7015b154b3193f/aries_cloudagent/indy/models/proof_request.py#L190