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

Fix loosing properties when generating verifiable credentials #432

Closed
wants to merge 1 commit into from

Conversation

icetrust0212
Copy link
Contributor

No description provided.

@icetrust0212 icetrust0212 self-assigned this May 23, 2024
@icetrust0212 icetrust0212 linked an issue May 23, 2024 that may be closed by this pull request
@aurelticot aurelticot changed the title add options parameter in createVerifiableCredential function Fix loosing properties when generating verifiable credentials May 23, 2024
Copy link
Member

@aurelticot aurelticot left a comment

Choose a reason for hiding this comment

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

This doesn't fix the issue for two reasons:

  • createVerifiableCredential is not the only entrypoint.
  • The default options should be set according to the behaviour we want (keep the properties)

@icetrust0212
Copy link
Contributor Author

icetrust0212 commented May 23, 2024

This doesn't fix the issue for two reasons:

  • createVerifiableCredential is not the only entrypoint.
  • The default options should be set according to the behaviour we want (keep the properties)

Even createVerifiableCredential is not the only entrypoint, I don't think it's harmful for now.
I don't have enough context for other projects, but this will be fixing current proof-connector issue and not harmful at least.

Also default options is the same as did-jwt-vc. it's empty object in default. {}
I think we need to set it with empty object ({}) so that it won't affect to other project.

@aurelticot
Copy link
Member

Even createVerifiableCredential is not the only entrypoint, I don't think it's harmful for now. I don't have enough context for other projects, but this will be fixing current proof-connector issue and not harmful at least.

I didn't say it was harmful, I said it's not the only entrypoint, implying the same logic should be applied to the other ones. The proof-connector is supposed to use createVerifiableCredentialRecord which this PR doesn't cover.

Also default options is the same as did-jwt-vc. it's empty object in default. {} I think we need to set it with empty object ({}) so that it won't affect to other project.

It's been flagged as a bug to loose the properties, passing an empty object doesn't fix the bug. Whatever did-jwt-vc uses as default options, if we think these properties should not be lost we should set the options accordingly.

@icetrust0212
Copy link
Contributor Author

icetrust0212 commented May 23, 2024

Even createVerifiableCredential is not the only entrypoint, I don't think it's harmful for now. I don't have enough context for other projects, but this will be fixing current proof-connector issue and not harmful at least.

I didn't say it was harmful, I said it's not the only entrypoint, implying the same logic should be applied to the other ones. The proof-connector is supposed to use createVerifiableCredentialRecord which this PR doesn't cover.

Also default options is the same as did-jwt-vc. it's empty object in default. {} I think we need to set it with empty object ({}) so that it won't affect to other project.

It's been flagged as a bug to loose the properties, passing an empty object doesn't fix the bug. Whatever did-jwt-vc uses as default options, if we think these properties should not be lost we should set the options accordingly.

Yeah, I understand. If it's not the only entrypoint. I will go through other endpoints.

Sure, we won't pass empty options. Empty is just default value if we don't pass the option.
We will pass necessary options when we call function (createVerifiableCredential) in verida-js.
The issue was that there was no way to pass this option when we use verida-js functions. Now we can pass options when we call createVerifiableCredential function.

@icetrust0212 icetrust0212 deleted the fix-vc-generation branch May 24, 2024 01:06
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.

Fix loosing properties when generating verifiable credentials
2 participants