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

Fixes mdoc/mdl section #128

Merged
merged 11 commits into from
Apr 3, 2024
Merged

Fixes mdoc/mdl section #128

merged 11 commits into from
Apr 3, 2024

Conversation

awoie
Copy link
Contributor

@awoie awoie commented Mar 6, 2024

This PR fixes the mso_mdoc sections by referring to the relevant documents that define the profile.

Fixes #62

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

I don't think there was an agreement in issue #62 to remove examples and only point to ISO specs.

also if you want to mention both 18013 and 23220, much more explanation is needed about the relationship between them. I am afraid the current text would confuse those not familiar.

@awoie
Copy link
Contributor Author

awoie commented Mar 6, 2024

I don't think there was an agreement in issue #62 to remove examples and only point to ISO specs.

also if you want to mention both 18013 and 23220, much more explanation is needed about the relationship between them. I am afraid the current text would confuse those not familiar.

I don't think there was an agreement in issue #62 to remove examples and only point to ISO specs.

also if you want to mention both 18013 and 23220, much more explanation is needed about the relationship between them. I am afraid the current text would confuse those not familiar.

Ok, let's discuss in the issue first.

On the PR, I can give an intro sentence on what mdocs are similar to what we did for anoncreds. No need to mention more. For that reason the references for interested readers.

@jogu jogu added the id3 label Mar 21, 2024
@awoie awoie requested a review from Sakurann March 25, 2024 12:59
@awoie
Copy link
Contributor Author

awoie commented Mar 25, 2024

@Sakurann I tried to incorporate your feedback in the PR. Could you please review again.

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

looks pretty good to me. one of the cleanest explanations of the relationship btw 18013, 23220 series and oid4vp that i have seen

@awoie awoie requested review from jogu and Sakurann March 28, 2024 14:35
@awoie
Copy link
Contributor Author

awoie commented Mar 28, 2024

Had to fix some of the references. SessionTranscript is defined in 18013-5/23220-4. OID4VPHandover is defined in 18013-7 and 23220-4. SessionTranscript is always the same for all protocols. OID4VP is just a specific protocol in ISO. Had to adjust the language. For that reason, please review again @Sakurann @jogu .

@jogu jogu added this to the ID-3 milestone Apr 2, 2024
@jogu jogu removed the id3 label Apr 2, 2024
Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

thank you for requesting re-review

Copy link
Contributor

@paulbastian paulbastian 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 to me. Are we sure we want to exclude all examples?

@Sakurann
Copy link
Collaborator

Sakurann commented Apr 2, 2024

Yeah, I actually had the same question in the back of my mind...
examples are super useful but they also depend on 18013-7, so keeping them in oid4vp means dependency on 18013-7 (updating them whenever 18013-7 changes), so probably better to remove them from oid4vp

@jogu
Copy link
Collaborator

jogu commented Apr 2, 2024

Looks good to me. Are we sure we want to exclude all examples?

Aside from the very good reason Kristina mentioned, we also don't actually have any valid examples (there's an error in the ones currently in 18013-7) and they'd be a massive pain to generate.

I think omitting them is the correct thing to do at the current time. Whether we might at some future point want to add them back in is a slightly separate discussion.

@Sakurann Sakurann merged commit e6ebec4 into main Apr 3, 2024
2 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.

Align with ISO/IEC 18013-7 Annex B
6 participants