Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

feat: Allow DID method to Accept supported DID prefixes #108

Merged
merged 1 commit into from
Aug 14, 2019
Merged

Conversation

fqutishat
Copy link
Contributor

@fqutishat fqutishat commented Aug 14, 2019

closes #101

Signed-off-by: Firas Qutishat [email protected]

@codecov-io
Copy link

codecov-io commented Aug 14, 2019

Codecov Report

Merging #108 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #108   +/-   ##
=======================================
  Coverage   91.91%   91.91%           
=======================================
  Files          19       19           
  Lines         668      668           
=======================================
  Hits          614      614           
  Misses         30       30           
  Partials       24       24
Impacted Files Coverage Δ
pkg/framework/didresolver/didresolver.go 100% <100%> (ø) ⬆️
pkg/framework/didresolver/api.go 100% <100%> (ø) ⬆️
pkg/didmethod/peer/resolver.go 83.33% <100%> (+3.33%) ⬆️
pkg/didmethod/peer/did.go 82.6% <0%> (-2.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b74857f...263918b. Read the comment docs.

@@ -72,6 +72,6 @@ type Opt func(opts *didResolverOpts)
// WithDidMethod to add did method
Copy link
Contributor

Choose a reason for hiding this comment

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

Should update the package docs to indicate that regular expressions are supported and DID methods are checked in the order added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// resolveDidMethod resolve did method
func (r *DIDResolver) resolveDidMethod(method string) (DidMethod, error) {
for _, v := range r.didMethods {
match, _ := regexp.MatchString(v.id, method)
Copy link
Contributor

Choose a reason for hiding this comment

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

The regular expression should be pre-compiled rather than compiled on demand during resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@troyronda
Copy link
Contributor

It would be good to add some description to the commit message so that the reader of the git logs has a better idea of the feature.

@@ -72,6 +72,6 @@ type Opt func(opts *didResolverOpts)
// WithDidMethod to add did method
func WithDidMethod(id string, method DidMethod) Opt {
return func(opts *didResolverOpts) {
opts.didMethods[id] = method
opts.didMethods = append(opts.didMethods, didMethod{id: id, DidMethod: method})
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes using WithDidMethod awkward. The user now must specify a regex instead of just a DID method. Eg. the regex "rie" matches the string "aries" yet they clearly are not the same strings. To lock down the definition, the user is forced to provide "^rie$", which is counter intuitive.

One way to enable "catch-alls" is to invert control of the pattern matching such that the DidMethod is responsible for it. Add an Accept(method string) bool method to DidMethod, thereby freeing the user from such awkwardness and, incidentally, simplifying WithDidMethod by removing the id arg:

resolver := didresolver.New(
    WithDidMethod(peer.NewDIDResolver(store)),
    WithDidMethod(universal.Resolver(url)),
    WithDidMethod(sidetree.Resolver(method, url)),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -12,12 +12,13 @@ import (

// DIDResolver resolver
type DIDResolver struct {
store *DIDStore
store *DIDStore
method string
Copy link
Contributor

@llorllale llorllale Aug 14, 2019

Choose a reason for hiding this comment

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

In the case of peer DIDs the method is always "peer", so this can be hard coded in peer.DIDResolver.Accept()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- Add accpet method in DidMethod interface
- Switch didMethods from map to array

closes #101

Signed-off-by: Firas Qutishat <[email protected]>
@fqutishat fqutishat changed the title feat: Allow regex expression to resolve did method feat: Allow DID method to Accept supported DID prefixes Aug 14, 2019
@llorllale llorllale merged commit 4380aae into hyperledger-archives:master Aug 14, 2019
@fqutishat fqutishat deleted the 101 branch August 14, 2019 18:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow DID method implementations to Accept supported DID prefixes.
4 participants