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

Credential Builder #37

Merged
merged 7 commits into from
Mar 7, 2022
Merged

Credential Builder #37

merged 7 commits into from
Mar 7, 2022

Conversation

decentralgabe
Copy link
Member

#31

@decentralgabe decentralgabe marked this pull request as ready for review March 4, 2022 05:40
@decentralgabe
Copy link
Member Author

May add some more tests before merge. I'm happy that the tests pass against the spec's example.

@decentralgabe decentralgabe linked an issue Mar 4, 2022 that may be closed by this pull request
@decentralgabe decentralgabe self-assigned this Mar 4, 2022
@decentralgabe
Copy link
Member Author

Added tests for the builder

Copy link
Member

@mistermoe mistermoe left a comment

Choose a reason for hiding this comment

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

Overall, looks great! left a few nits.

@@ -414,7 +417,7 @@ func (v JSONWebKeyVerifier) Verify(message, signature []byte) error {
return err
}

func NewJSONWebKeyVerifier(key PublicKeyJWK) (*JSONWebKeyVerifier, error) {
func NewJSONWebKeyVerifier(kid string, key PublicKeyJWK) (*JSONWebKeyVerifier, error) {
Copy link
Member

Choose a reason for hiding this comment

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

What was the reasoning behind including kid as a requirement?

Copy link
Member Author

Choose a reason for hiding this comment

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

for linked data things, there's an expected verificationMethod that indicates a key-reference in a DID, or another URI-resolvable document; this is only required for the signer. The verifier technically does not need this; however, the JWK library we use requires the kid in the public key you construct to match the kid in the signed message you're verifying...which is why on this specific verifier it's a required parameter.

// create proof before CVH
proof := j.createProof(s.KeyID())

// prepare proof options
contexts, err := GetContextsFromProvable(p)
if err != nil {
return nil, errors.Wrap(err, "could not get contexts from provable")
return errors.Wrap(err, "could not get contexts from provable")
Copy link
Member

Choose a reason for hiding this comment

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

🎉 meaningful error messages

"testing"

"github.com/goccy/go-json"
Copy link
Member

Choose a reason for hiding this comment

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

solid choice! being able to pass Context.context into Marshal and Unmarshal is awesome

vc/credential.go Outdated
@@ -0,0 +1,236 @@
package vc
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be helpful to include package level documentation (reference). Can also use the doc.go convention

Copy link
Member Author

Choose a reason for hiding this comment

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

opened #40

vc/credential.go Outdated
@@ -0,0 +1,236 @@
package vc
Copy link
Member

Choose a reason for hiding this comment

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

would it make more sense if this file was called builder.go given that's what's in it?

Copy link
Member Author

Choose a reason for hiding this comment

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

will rename

@decentralgabe decentralgabe merged commit 87cb996 into main Mar 7, 2022
@decentralgabe decentralgabe deleted the issue-creds branch March 7, 2022 18:31
@decentralgabe decentralgabe added this to the April MVP milestone Mar 18, 2022
@decentralgabe decentralgabe added enhancement New feature or request tracked labels Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple Credential Issuance and Validation, Verification
2 participants