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

Jump to bottom

Comply with the new DID ABNF #8

Closed
mrinalwadhwa opened this issue Feb 22, 2019 · 10 comments
Closed

Comply with the new DID ABNF #8

mrinalwadhwa opened this issue Feb 22, 2019 · 10 comments
Assignees
@mrinalwadhwa
Labels
enhancement New feature or request help wanted

Comments

@mrinalwadhwa
Copy link
Member

There is ongoing discussion around updating the DID ABNF that we would need to update the parser for

w3c-ccg/did-spec#168
https://pr-preview.s3.amazonaws.com/w3c-ccg/did-spec/pull/168.html

screen shot 2019-02-21 at 7 56 50 pm
@mrinalwadhwa mrinalwadhwa self-assigned this Feb 22, 2019
@mrinalwadhwa
Copy link
Member Author

The DID part looks unaffected.
https://github.com/ockam-network/did/blob/bdd1431c112d9fe2579a0d4ae73e680d196146b6/did.abnf#L10-L16

@mrinalwadhwa
Copy link
Member Author

did-reference will need some tweaks.

https://github.com/ockam-network/did/blob/bdd1431c112d9fe2579a0d4ae73e680d196146b6/did.abnf#L8

https://github.com/ockam-network/did/blob/bdd1431c112d9fe2579a0d4ae73e680d196146b6/did.abnf#L20-L31

@mrinalwadhwa mrinalwadhwa added the enhancement New feature or request label Feb 22, 2019
@mrinalwadhwa
Copy link
Member Author

did-fragment is also likely unaffected

@petersng
Copy link

petersng commented Jan 3, 2020

Hi, just wondering if this will be updated at some point? Seems like this project has been somewhat abandoned while the DID spec has been updated to 1.0. I've been using this lib to parse and validate my DIDs and it has been working great, except for when I encounter new characters that weren't in the ABNF before. I could submit a simple patch for my case if that helps as I prob wouldn't have time to work on completely updating the spec.

This code works great otherwise, thanks!

@mrinalwadhwa
Copy link
Member Author

Hi @petersng. Glad you found the code useful ❤️
We don't have an immediate need to update so this its low on priority list ... however we would love to get a PR if you or anyone else is interested in working on updating it.

@hollyfeld
Copy link
Contributor

hollyfeld commented Feb 14, 2020 •
edited

Hi @mrinalwadhwa. I'm not too far off from having a pull request for this, but I do have a couple of questions first:

  1. In the current DID String method, Fragment is only serialized to the string when a Path does not exist. I don't see that restriction in the grammar, and it looks like a did-reference with a Path, Query, and Fragment will parse, but a subsequent call to String on the DID would lose the Fragment data.

Am I reading this wrong, or is there some other reason etc.? Please clarify.

https://github.com/ockam-network/did/blob/863346eac7086a346625dfaf4c461326b2da7cf4/did.go#L101-L105

  1. I've added some parserStep methods that model the existing steps. The code to set the next parserStep gets a bit redundant. A possible solution would be to extract out a method that takes a bitmask with options for transitioning, along with a lambda for validation. Then each parser step configures as needed.

Let me know what you think. I can finish up the current approach and push up and then decide if #2 is worth the effort. Thanks!

@mrinalwadhwa
Copy link
Member Author

Hey @hollyfeld that is awesome! I haven't read the did spec in a some months. I'll give it a quick refresher this afternoon and come back to you with answers. Thank you for working on a PR.

@hollyfeld
Copy link
Contributor

Hi @mrinalwadhwa,

I went ahead and generated a PR here. On the previous questions, I went ahead and removed the constraint on not allowing a fragment when a path or segments exist.

The second item I would just ignore. I looked at it again and it didn't seem worth the trouble.
Thanks!

@mrinalwadhwa
Copy link
Member Author

Awesome. Thank you 🙏. I'll read through the changes tonight.

hollyfeld added a commit to hollyfeld/did that referenced this issue Mar 10, 2020
@hollyfeld
feat: Updated to DID v1.0
c169166 
1. modifies the DID struct and parser to handle the param rule addition
2. removes references to did-reference, and replaces them with did-url
3. removes a constraint on fragment existence in the parser String
   method requiring the existence of a path or path segments
4. adds test and example coverage

For build-trust#8
hollyfeld added a commit to hollyfeld/did that referenced this issue Mar 10, 2020
@hollyfeld
feat: Updates to DID v1.0
e604f9b 
1. modifies the DID struct and parser to handle the param rule addition
2. removes references to did-reference, and replaces them with did-url
3. removes a constraint on fragment existence in the parser String
   method requiring the existence of a path or path segments
4. adds test and example coverage

For build-trust#8
@mrinalwadhwa mrinalwadhwa mentioned this issue Mar 10, 2020
Closed
@mrinalwadhwa
Copy link
Member Author

@hollyfeld Thank you for sending the PR, it's merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mrinalwadhwa mrinalwadhwa

Labels
enhancement New feature or request help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@hollyfeld @mrinalwadhwa @petersng