module for convert onchain verifiable credential on w3c verifiable cr… #83
Conversation
verifiable/onchain/constants.go
Outdated
|
|
||
| const ( | ||
| BooleanHashTrue = "18586133768512220936620570745912940619677854269274689475585506675881198879027" | ||
| BooleanHashFalse = "19014214495641488759237505126948346942972912379615652741039992445865937985820" |
There was a problem hiding this comment.
You made those vars public deliberately? You are going to use it outside of this package?
verifiable/onchain/parser.go
Outdated
| regenerateID bool | ||
| } | ||
|
|
||
| type Option func(*Parser) |
There was a problem hiding this comment.
In this package (onchain), there are many 'main' types, such as onchain.MTP and onchain.Proof. The type Option does not indicate which type it is related to. I would suggest naming it something like ParserOption, in case we need to add other options in future versions.
There was a problem hiding this comment.
I changed to ConvertorOption
verifiable/onchain/parser.go
Outdated
| } | ||
|
|
||
| // WithMerklizer sets the merklizer instance. | ||
| func WithMerklizer(m merklize.Options) Option { |
There was a problem hiding this comment.
Based on this name, I would expect that the function accepts a *merklizer.Merklizer type instead of merklizer.Options. What do you think about renaming it to WithMerklizerOptions?
verifiable/onchain/parser.go
Outdated
| nonce, err := strconv.ParseUint(credentialStatus.RevocationNonce, 10, 64) | ||
| if err != nil { | ||
| return nil, | ||
| fmt.Errorf("invalid revocationNonce '%s': %v", credentialStatus.RevocationNonce, err) |
There was a problem hiding this comment.
I would use %w verb for err. In this way we can unwrap original error if needed.
There was a problem hiding this comment.
Done. Now I use %w in all places.
verifiable/onchain/parser.go
Outdated
| } | ||
|
|
||
| // ConvertVerifiableCredential converts an onchain verifiable credential to a W3C verifiable credential. | ||
| func (p *Parser) ConvertVerifiableCredential(onchainCredential *Credential) (*verifiable.W3CCredential, error) { |
There was a problem hiding this comment.
If the sole public function in Parser type named ConvertVerifiableCredential why the struct not named Converter? :)
There was a problem hiding this comment.
Renamed to Converter
verifiable/onchain/parser.go
Outdated
| IssuerData: verifiable.IssuerData{ | ||
| ID: issuerDID.String(), | ||
| State: verifiable.State{ | ||
| RootOfRoots: strPtr(rootOfRoots.Hex()), |
There was a problem hiding this comment.
hack to get a pointer to the string )
&[]string{rootOfRoots.Hex()}[0]
There was a problem hiding this comment.
Thanks for the advice. But I do want not to create an array to get a pointer to a string
I prefer old school :)
verifiable/onchain/parser.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("failed hash value '%s' with data type '%s': %v", source, datatype, err) | ||
| } | ||
| origineHash, ok := big.NewInt(0).SetString(origin, 10) |
verifiable/onchain/parser.go
Outdated
| return nil | ||
| } | ||
|
|
||
| type smtproof struct { |
There was a problem hiding this comment.
for me, this struct looks useless. convertChainProofToMerkleProof can accept MTP directly. But may be I missed something.
| @@ -0,0 +1,61 @@ | |||
| package onchain | |||
|
|
|||
| type CredentialSubjectField struct { | |||
There was a problem hiding this comment.
give comments public to structures , not sure why linter didn't fail
verifiable/onchain/credential.go
Outdated
| RevocationNonce string `json:"revocationNonce"` | ||
| } | ||
|
|
||
| type Credential struct { |
There was a problem hiding this comment.
is at a god idea to have such a structure public? Why we can't create some internal model and the W3C credential is the only credential that is public structure?
There was a problem hiding this comment.
We can do it. But in this case function ConvertVerifiableCredential should accept []bytes instead of typed struct.
There was a problem hiding this comment.
In my opinion is okay to have offchain(w3c).VerifiableCredentials and onchain.Verifiable Credential. Since they are different a little bit.
verifiable/onchain/parser.go
Outdated
| "github.com/piprate/json-gold/ld" | ||
| ) | ||
|
|
||
| type Parser struct { |
There was a problem hiding this comment.
I would avoid term Parser due to the collision with JSON / JSONLD parsers. better use DataConvertor. This convertor serves only single purpose.
There was a problem hiding this comment.
Done. I renamed to converter
verifiable/onchain/constants.go
Outdated
| reservedCredentialSubjectKeyType string = "type" | ||
|
|
||
| // nolint:gosec // This is type of the credential, not a secret | ||
| verifiableCredentialType = "VerifiableCredential" |
There was a problem hiding this comment.
we already have constant TypeW3CVerifiableCredential
verifiable/onchain/converter.go
Outdated
| "github.com/piprate/json-gold/ld" | ||
| ) | ||
|
|
||
| type Convertor struct { |
There was a problem hiding this comment.
file is named converter, structure is named convertor
verifiable/onchain/converter.go
Outdated
| // ConvertVerifiableCredential converts an onchain verifiable credential to a W3C verifiable credential. | ||
| // The w3c credential id will be regenerated. | ||
| func ConvertVerifiableCredential(credential *Credential) (*verifiable.W3CCredential, error) { | ||
| id := fmt.Sprintf("urn:uuid:%s", uuid.NewString()) |
There was a problem hiding this comment.
why id of credential is not comming from the contract itself. It is not supposed to be different each time.
There was a problem hiding this comment.
yes, we should have unique, but still deterministic id. E.g. based on DID of issuer (or chainId+contractAddr) + credential id from contract.
There was a problem hiding this comment.
@vmidyllic since we are using int as ID in on-chain issuer. Credentials with id: 1 will not be merklized. We should have valid uri.
There was a problem hiding this comment.
id must have string format, valid urn, it should come as it from contract. urn:uiid:1
No description provided.