Update ID rules in NewVerifiableCredentialBuilder #532
Conversation
…esid is a parsable uri in SetID
m-rit
requested review from
decentralgabe,
nitro-neal and
andresuribe87
as code owners
credential/builder.go
Outdated
| func NewVerifiableCredentialBuilder() VerifiableCredentialBuilder { | ||
| contexts := []string{VerifiableCredentialsLinkedDataContext} | ||
| types := []string{VerifiableCredentialType} | ||
| return VerifiableCredentialBuilder{ | ||
| contexts: contexts, | ||
| types: types, | ||
| VerifiableCredential: &VerifiableCredential{ | ||
| ID: uuid.NewString(), | ||
| ID: "", |
There was a problem hiding this comment.
🤔 given that this is a required property I'm not sure it's a good idea to leave this empty
perhaps it should be added as an optional property to the builder, and if that is empty we generate it automatically?
There was a problem hiding this comment.
yeah, it's better to keep an option argument. I'll make the changes, thanks!
There was a problem hiding this comment.
Hi @decentralgabe, I have added an optional argument to keep ID empty. Kindly review.
…dates that Id is a parsable uri in SetID
credential/builder.go
Outdated
| @@ -33,19 +33,25 @@ type VerifiableCredentialBuilder struct { | |||
|
|
|||
| // NewVerifiableCredentialBuilder returns an initialized credential builder with some default fields populated | |||
| // Default id is empty | |||
| func NewVerifiableCredentialBuilder() VerifiableCredentialBuilder { | |||
| func NewVerifiableCredentialBuilder(emptyID ...bool) VerifiableCredentialBuilder { | |||
There was a problem hiding this comment.
what do you think about making this a bit more readabale?
we could change the build to accept a new type like IDvalue for example
type IDValue string
const EmptyIDValue IDValue = ""
func NewVerifiableCredentialBuilder(id IDValue) VerifiableCredentialBuilder {
if id == EmptyIDValue {
// generate ID
}
...
}There was a problem hiding this comment.
Hi @decentralgabe, thanks for your inputs.
Please confirm if the behaviour is as follows:
NewVerifiableCredentialBuilder (EmptyIDValue) -> creates vc with id as uuid
NewVerifiableCredentialBuilder ("xxx") -> generates vc with id as "xxx"
vcb.SetID("") -> sets ID as empty
There was a problem hiding this comment.
ah, good consideration.
Perhaps we should add....
EmptyIDValue -> set ID value to empty
GenerateIDValue -> generate a uuid
CustomIDValue("<your-id-here>") -> use custom ID
to be unambiguous?
There was a problem hiding this comment.
Hi @decentralgabe , I have added IDValue argument here to all three cases (empty ID/ generate ID/ custom ID)
|
thanks @m-rit I approved - you may have to take a look at the build issues |
Head branch was pushed to by a user without write access
|
Hi @decentralgabe, I have fixed all the build issues (the function signature for |
|
thanks @m-rit approved and merging |
Address Issue #400 .
Changes default id in NewVerifiableCredentialBuilder as empty, validates if id is a parsable uri in SetID