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

remove credential offer part from credential profiles #121

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

tlodderstedt
Copy link
Collaborator

After the recent changes, there is no longer any format specific part in a credential offer. This PR removes the respective definitions from the credential format profiles.

@Sakurann
Copy link
Collaborator

When paul did a previous PR, I suggested not to remove entirely to show concrete examples how to use credential format specific identifiers, etc..
#109 (review)

@paulbastian
Copy link
Contributor

I don't have strong opinions on this

I think having more than one example is nice and the credential offer in appendix could match to the credential issuer metadata. Especially having some examples for the changes in #95 would be nice.

On the other hand, we don't have examples on e.g. authroization response either and we could move some examples to the credential offer section if we want more examples

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 came along with this.
but as @jogu said during the WG call, would like to see the example files unused elsewhere in the spec removed before we merge this.
also it does not compile as-is..

@Sakurann Sakurann added this to the ID-1 milestone Dec 7, 2023
tlodderstedt and others added 2 commits December 13, 2023 14:02
@paulbastian
Copy link
Contributor

I think it would be good to include at least one of the existing examples into the credential offer section, to see some variations of transaction code

@paulbastian
Copy link
Contributor

paulbastian commented Dec 13, 2023

I did some changes, please review
@tlodderstedt
Also closes #114

@tlodderstedt tlodderstedt merged commit 80722e6 into main Dec 13, 2023
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.

3 participants