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

Add multibase datatype definition #176

Merged
merged 8 commits into from
Aug 31, 2023
Merged

Add multibase datatype definition #176

merged 8 commits into from
Aug 31, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented Aug 22, 2023

This PR addresses #162 (comment) by defining the multibase datatype as we recently did for cryptosuiteString. It then updates proofValue's range to be multibase.

@iherman, we'll need to define the datatype in the vocabulary.yml file as well, but I didn't see how that was done for cryptosuiteString yet -- perhaps that still needs to be merged?


Preview | Diff

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

small language tweaks

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@iherman
Copy link
Member

iherman commented Aug 23, 2023

@iherman, we'll need to define the datatype in the vocabulary.yml file as well, but I didn't see how that was done for cryptosuiteString yet -- perhaps that still needs to be merged?

Yes, that is part of #171 .

(would be good to merge that one, to avoid unnecessary issues...)

@iherman
Copy link
Member

iherman commented Aug 23, 2023

This PR addresses #162 (comment) by defining the multibase datatype as we recently did for cryptosuiteString. It then updates proofValue's range to be multibase.

Forgive me if my question reveals my missing knowledge, but shouldn't this be the range of secretKeyMultibase and publicKeyMultibase, too? At this moment, those two properties have simply xsd:string as their range.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@msporny
Copy link
Member Author

msporny commented Aug 23, 2023

Forgive me if my question reveals my missing knowledge, but shouldn't this be the range of secretKeyMultibase and publicKeyMultibase, too? At this moment, those two properties have simply xsd:string as their range.

Yes, good catch, I'll make that change in a future revision to this PR.

@msporny
Copy link
Member Author

msporny commented Aug 24, 2023

@iherman wrote:

Forgive me if my question reveals my missing knowledge, but shouldn't this be the range of secretKeyMultibase and publicKeyMultibase, too? At this moment, those two properties have simply xsd:string as their range.

Done in 78c30c0.

@iherman iherman self-requested a review August 25, 2023 06:52
@msporny
Copy link
Member Author

msporny commented Aug 31, 2023

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 716f2a5 into main Aug 31, 2023
2 checks passed
@msporny msporny deleted the msporny-mb-datatype branch August 31, 2023 17:34
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.

6 participants