Skip to content
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

Issue Credential V2.0 message structures #990

Merged
merged 16 commits into from
Sep 28, 2023

Conversation

gmulhearn-anonyome
Copy link
Contributor

@gmulhearn-anonyome gmulhearn-anonyome commented Sep 21, 2023

NOTE: I've gone with v2.0 (as opposed to v2.1 & v2.2), this is primarily because it seems to be what others (aca-py & AFJ) are targeting and expect. similarly, V2.0 is what AIP2.0 lists as it's requirement: https://github.com/hyperledger/aries-rfcs/blob/main/concepts/0302-aries-interop-profile/README.md#base-requirements

  • Rename all existing issue-credential message types to have a V1 suffix
  • Put v1 and v2 messages in different structures

Also note, I'm consuming these messages in the Issuer and Holder V2 impl: #987

Also also note, have tested holderv2 against aca-py for issue-credential-v2, which should confirm that the message structures are interoperable with acapy

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2023

Codecov Report

Merging #990 (21b2796) into main (ecc2047) will decrease coverage by 0.19%.
The diff coverage is 18.69%.

@@            Coverage Diff             @@
##             main     #990      +/-   ##
==========================================
- Coverage   30.02%   29.83%   -0.19%     
==========================================
  Files         415      424       +9     
  Lines       26916    27082     +166     
  Branches     5243     5290      +47     
==========================================
  Hits         8081     8081              
- Misses      16639    16802     +163     
- Partials     2196     2199       +3     
Flag Coverage Δ
unittests-aries-vcx 29.83% <18.69%> (-0.19%) ⬇️

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

Files Coverage Δ
...protocols/issuance/holder/states/offer_received.rs 17.85% <100.00%> (ø)
...c/protocols/issuance/holder/states/proposal_set.rs 62.50% <100.00%> (ø)
...rc/protocols/issuance/holder/states/request_set.rs 77.27% <100.00%> (ø)
...protocols/issuance/issuer/states/credential_set.rs 30.00% <100.00%> (ø)
.../src/protocols/issuance/issuer/states/offer_set.rs 93.10% <100.00%> (ø)
...tocols/issuance/issuer/states/proposal_received.rs 100.00% <100.00%> (ø)
...ocols/issuance/issuer/states/requested_received.rs 37.50% <100.00%> (ø)
aries_vcx/tests/utils/scenarios/data.rs 96.49% <100.00%> (ø)
...lds/protocols/cred_issuance/v1/issue_credential.rs 25.00% <ø> (ø)
...s/protocols/cred_issuance/v1/request_credential.rs 28.57% <ø> (ø)
... and 31 more

... and 1 file with indirect coverage changes

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

The only thing I'd change would be renaming/compartmentalizing stuff related to the V1 protocol version a bit better, since now there's V2 stuff laying around.

aries_vcx/src/handlers/util.rs Outdated Show resolved Hide resolved
messages/src/lib.rs Outdated Show resolved Hide resolved
@bobozaur
Copy link
Contributor

@gmulhearn-anonyome I'll try to put together some guidelines for the message crate in a README file to maybe clarify how things get pieced together. But if you have any specific questions before then, feel free to ask here.

messages/src/lib.rs Outdated 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>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn-anonyome gmulhearn-anonyome changed the title Draft: Issue Credential V2.0 message structures Issue Credential V2.0 message structures Sep 25, 2023
@gmulhearn-anonyome gmulhearn-anonyome marked this pull request as ready for review September 25, 2023 00:09
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn-anonyome
Copy link
Contributor Author

@bobozaur thanks for the feedback. let me know what you think about this latest revision

Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there.

The only nitpick I have is about the conversions that arise from introducing the CredentialIssuance wrapper enum (since there are two versions now).

There are all these intermediate enums throughout the crate which are really there simply for composition purposes. You should rarely have to work with the CredentialIssuanceV1 enum directly.

The actual messages all have implementations of From (through the transit_to_aries_msg macro) to convert them to an AriesMessage directly specifically to avoid the need to deal with all the nested enums.

messages/src/msg_fields/protocols/cred_issuance/v1/mod.rs Outdated Show resolved Hide resolved
messages/src/msg_fields/protocols/cred_issuance/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If CI passes then it's all good on my end.

@Patrik-Stas Patrik-Stas merged commit ed39a52 into hyperledger:main Sep 28, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants