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

feat!: update Denom regex to support more DID characters #9699

Merged
merged 11 commits into from Sep 27, 2021

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jul 14, 2021

DID method-specific-ids can include :, ., -, and _ but these are not included in the denom regex. This PR adds them to the regex.

DIDs also support percent encoded hex digits, but these are not added here, but could be included in a separate PR.

/cc @ig-shaun @jandrieu


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@orijbot
Copy link

orijbot commented Jul 14, 2021

Visit https://dashboard.github.orijtech.com?pr=9699&repo=cosmos%2Fcosmos-sdk to see benchmark details.

@aaronc aaronc marked this pull request as ready for review July 14, 2021 17:21
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

utACK

@aaronc
Copy link
Member Author

aaronc commented Jul 14, 2021

@alexanderbez do you see any potential issues here?

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM

@ig-shaun
Copy link

Thanks @aaronc Great to see this! Supports the path towards a Tokens 2.0 framework for all token types and classes.
It was not clear to me, does this update allow # fragments, which is a common pattern for locating properties within a DID Document?

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, can we add a changelog entry?

@aaronc
Copy link
Member Author

aaronc commented Jul 19, 2021

Thanks @aaronc Great to see this! Supports the path towards a Tokens 2.0 framework for all token types and classes.
It was not clear to me, does this update allow # fragments, which is a common pattern for locating properties within a DID Document?

Fragments can't be inside a method-specific-id. The only thing left out here is the pct-encoded characters.

@aaronc aaronc added A:automerge Automatically merge PR once all prerequisites pass. C:Types C:x/bank Type: Feature T: State Machine Breaking State machine breaking changes (impacts consensus). labels Jul 19, 2021
@github-actions github-actions bot removed the C:x/bank label Jul 19, 2021
@jandrieu
Copy link

It was not clear to me, does this update allow # fragments, which is a common pattern for locating properties within a DID Document?

Fragments can't be inside a method-specific-id. The only thing left out here is the pct-encoded characters.

@ig-shaun By convention, URL fragments are handled entirely by the client. The #fragment part does not go to the server with traditional http URLs. The browser simply handles the focus when rendering the html.

However, there is an additional component in the DID world, DID URLs:

The DID URL Syntax ABNF Rules

did-url = did path-abempty [ "?" query ] [ "#" fragment ]

So, DID URLs can have fragments. And, in some cases, a DID URL with a fragment may be used to refer to a particular linked resource (or other property value) in a DID Document.
I believe this is a non-issue if, for example, all DID denoms are pure DIDs and not DID URLs. This feels like a reasonable design choice.

The main adjustment would be an expected practice in any case: instead of blindly resolving a DID URL (sending the whole thing), the resolver can (and probably should) clip the query and fragment parts and treat a DID URL at resolution as if it were just the DID part.

FWIW, the DID Resolver specification is still in early development.

Also--although there are 90+ DID methods and I only know a subset--the only planned use I have heard of for % encoding is for query and fragment parts which is fairly familiar from the Web. For robustness, we should plan on getting that support in, but it's unlikely to be a factor with a normal DID method design.

@aaronc aaronc changed the title feat: update Denom regex to support more DID characters feat!: update Denom regex to support more DID characters Jul 22, 2021
@clevinson clevinson self-assigned this Aug 11, 2021
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 26, 2021
@aaronc aaronc added pinned and removed stale labels Sep 27, 2021
@aaronc aaronc merged commit 3138527 into master Sep 27, 2021
@aaronc aaronc deleted the aaronc/iid-denom-regex branch September 27, 2021 20:01
amaury1093 pushed a commit that referenced this pull request Nov 4, 2021
* feat!: support DID method-specific-id regex in denoms

* udpate docs

* add test case

* update CHANGELOG.md

* fix CHANGELOG.md

* fix test

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Types T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants