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: did:peer #120

Merged
merged 16 commits into from
Jul 6, 2022
Merged

feat: did:peer #120

merged 16 commits into from
Jul 6, 2022

Conversation

andorsk
Copy link
Contributor

@andorsk andorsk commented Jun 30, 2022

This is an implementation of https://identity.foundation/peer-did-method-spec/ related to

#114.

Implemented:

  1. limited implementation of method 0 and method 2 or did:peer

Also included:

  1. DID and Resolver interfaces
  2. Some shared stuff was refactored such as general key encoding/ errors
  3. Update to the service struct and added some additional support to the DIDDocuments.

There's more to do here, but I will leave that for another PR unless it is necessary to add it to this one. Anything unimplemented I have returning an "unimplemented" error.

@andorsk andorsk marked this pull request as draft June 30, 2022 04:13
@andorsk andorsk changed the title Peer DID feat: Peer DID Jun 30, 2022
@andorsk andorsk changed the title feat: Peer DID feat: did:peer Jun 30, 2022
@andorsk
Copy link
Contributor Author

andorsk commented Jun 30, 2022

Things aren't nearly done here yet, but a couple key things I've realized in this process that I wanted to share:

  1. I've added in an DID interface. I'm pretty sure we'll need this, but I need to align this with the DID specifications on DIF.
  2. Errors should be standardized. Probably in the "errors" file of the "utils", which is where I put things.

I'll update this when I've made more progress.

@andorsk andorsk force-pushed the feat-did-peer branch 2 times, most recently from bf8d4d9 to d5b529f Compare June 30, 2022 12:13
did/interface.go Outdated
// Define the interface more clearly
type DID interface {
IsValid() bool
Resolve() (*DIDDocument, error)
Copy link
Member

Choose a reason for hiding this comment

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

hmm, not sure about this one.
some DIDs (like ION, and others) require network resolvers, which require hosted software. I'd like to avoid making recommendations of which resolvers to use here.

some DIDs (like peer, web, key) can be resolved more simply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'm not sure about this one either. I do think we need a DID interface though! I'm just not sure exactly what goes into it. IsValid() seems reasonable. What else is required in a DID?

Copy link
Member

Choose a reason for hiding this comment

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

hmm...maybe some common methods to get keys? like getAuthenticationMethods, getVerificationMethods and so on?

would validation just be static, or should we eventually expand to determining whether the keys in the doc are themselves valid?

there also needs to be a more flexible way to determine validity across DID methods, as they each have a set of properties/processes that uniquely apply to each method

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 think that's why each DID needs to implement an IsValid method no? That method would determine if the DID is valid or not.

It seems like that the biggest issue here is the network resolvers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For resolution, here's what I ended up doing which works:

type Resolver interface {
	Resolve(d DID) (*DIDDocument, error)
}

Some did's will have resolvers built in, and so will not. So network based resolvers we can ignore fine, it just won't be implementing the Resolver interface

did/peer.go Outdated Show resolved Hide resolved
did/peer.go Outdated Show resolved Hide resolved
@decentralgabe
Copy link
Member

looks great so far. I like where your head is at for (1)..left a comment
(2)- agreed

@andorsk
Copy link
Contributor Author

andorsk commented Jul 1, 2022

thanks @decentralgabe ! yep. Got them. Thanks!

@andorsk andorsk force-pushed the feat-did-peer branch 4 times, most recently from 6def758 to 6736f90 Compare July 4, 2022 12:45
@andorsk
Copy link
Contributor Author

andorsk commented Jul 4, 2022

Right now there is simple encoding/decoding of method0 and method2 of did:peer. I'd prefer to get this into main, and then submit subsequent PR's with additional support. Lmk if that works, otherwise i'll put this back into draft.

The scope of this already went a little out what it was supposed, with the introduction of some general utility stuff (such as errors and the util.go) and introducing some interfaces to the repo.

@andorsk andorsk marked this pull request as ready for review July 4, 2022 13:51
Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

small ones, should be good to go after this

did/model.go Outdated Show resolved Hide resolved
did/model.go Outdated Show resolved Hide resolved
@@ -67,4 +73,15 @@ func (d *DIDDocument) IsValid() error {
return util.NewValidator().Struct(d)
}

func NewDIDDocument() *DIDDocument {
return &DIDDocument{
Authentication: make([]VerificationMethodSet, 0),
Copy link
Member

Choose a reason for hiding this comment

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

would new() work? may be easier to read

did/peer_test.go Outdated Show resolved Hide resolved
did/peer_test.go Outdated Show resolved Hide resolved
did/peer_test.go Outdated Show resolved Hide resolved
util/errors.go Outdated Show resolved Hide resolved
andorsk and others added 2 commits July 6, 2022 10:02
Co-authored-by: Gabe <gabe.l.cohen@gmail.com>
@andorsk
Copy link
Contributor Author

andorsk commented Jul 6, 2022

@decentralgabe updates made. One question for you and then maybe we are good to go?

// offloading pairwise and n-wise relationships to peer DIDs.
//
// Currently only methods 0 and 2 are supported.
// Method 1 will be supported in a future date
Copy link
Member

Choose a reason for hiding this comment

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

created #137


var availablePeerMethods = map[string]Resolver{
"0": PeerMethod0{},
"1": PeerMethod1{},
Copy link
Member

Choose a reason for hiding this comment

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

should 1 be removed since it's not yet supported?

did/peer.go Outdated
Comment on lines 30 to 31
type DIDPeer string
type PurposeType string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type DIDPeer string
type PurposeType string
type (
DIDPeer string
PurposeType string
)

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

gave you a more thorough review this time. really close 😄

did/peer.go Outdated Show resolved Hide resolved
did/peer.go Outdated Show resolved Hide resolved
did/peer.go Outdated Show resolved Hide resolved
did/peer.go Outdated Show resolved Hide resolved
did/peer.go Show resolved Hide resolved
did/peer.go Show resolved Hide resolved
did/peer.go Show resolved Hide resolved
did/peer.go Outdated Show resolved Hide resolved
did/peer.go Outdated Show resolved Hide resolved
did/peer.go Show resolved Hide resolved
@andorsk
Copy link
Contributor Author

andorsk commented Jul 6, 2022

@decentralgabe thanks a ton for going through the PR again. Super appreciate the second pair of eyes. Definitely missed a few things which you caught. Any other issues lmk.

Again, thanks a ton for the review!

@andorsk
Copy link
Contributor Author

andorsk commented Jul 6, 2022

One thing I'll implement in a future PR: We should make a future issue where Resolver is actually re-specified to DIDResolver. There's a chance we have non DIDResolvers down the road and we'd want to not run into confusion/conflict here.

@decentralgabe
Copy link
Member

One thing I'll implement in a future PR: We should make a future issue where Resolver is actually re-specified to DIDResolver. There's a chance we have non DIDResolvers down the road and we'd want to not run into confusion/conflict here.

agreed

@andorsk
Copy link
Contributor Author

andorsk commented Jul 6, 2022

Think we are good, as I've addressed all the comments, but feel free to bring up more critiques. Comments are welcome 👍

@decentralgabe decentralgabe merged commit d5273d2 into TBD54566975:main Jul 6, 2022
This was referenced Jul 6, 2022
decentralgabe added a commit that referenced this pull request Jul 6, 2022
* origin/main:
  feat: did:peer (#120)

# Conflicts:
#	did/key.go
#	did/model.go
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.

None yet

2 participants