#3663: change credentialsubject to map, use casting instead of marshalling where possible#3739
#3663: change credentialsubject to map, use casting instead of marshalling where possible#3739
Conversation
…lling where possible
reinkrul
requested review from
gerardsn,
stevenvegt and
woutslakhorst
as code owners
gerardsn
left a comment
gerardsn
left a comment
There was a problem hiding this comment.
and pending performance test of course
| credentialSubject := make([]credential.BaseCredentialSubject, 0) | ||
| err := organization.UnmarshalCredentialSubject(&credentialSubject) | ||
| if err != nil { | ||
| return nil, did.DID{}, fmt.Errorf("unable to get DID from organization credential: %w", err) | ||
| } | ||
| organizationDIDStr := credentialSubject[0].ID | ||
| organizationDID, err := did.ParseDID(organizationDIDStr) | ||
| organizationDID, err := organization.SubjectDID() |
There was a problem hiding this comment.
Note that this is now a stricter check since VerifiableCredential.SubjectDID() checks if all credentialSubject.IDs are the same compared to just parsing the first value.
go.mod
Outdated
| github.com/nats-io/nats.go v1.39.1 | ||
| github.com/nuts-foundation/crypto-ecies v0.0.0-20211207143025-5b84f9efce2b | ||
| github.com/nuts-foundation/go-did v0.15.0 | ||
| github.com/nuts-foundation/go-did v0.15.1-0.20250319081900-aa2a47f72926 |
There was a problem hiding this comment.
Update after go-did is tagged (and probably run go mod tidy for cleanup of go.sum)
|
|
||
| import "encoding/json" | ||
|
|
||
| func remarshal(src interface{}, dst interface{}) error { |
There was a problem hiding this comment.
dst in the signature should probably be a pointer to match unmarshal behavior. the least line should then be return json.Unmarshal(asJSON, dst)
| for _, b := range bl { | ||
| if b.ID != "" { | ||
| // check credentialSubject | ||
| subjectDID, err := did.ParseDID(b.ID) | ||
| if err == nil { | ||
| if !slices.Contains(didMethods, subjectDID.Method) { | ||
| continue outer | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
this used to require only one subjectDID to be valid, but credential.SubjectDID() now requires all to be valid and have the same value.
|
Test
So currently, no measurable benefit. |
|
its hard to give a meaningful interpretation to this without the profiler results |
woutslakhorst
left a comment
woutslakhorst
left a comment
There was a problem hiding this comment.
Slightly better code, but unfortunately no real gain.
|
I still cannot make any conclusions based on these results. Subtracting the wait time and looking at the difference based on the information I have the gain could ~6% (=1-16/17).
|
even without performance gains, it cleans up the code (a bit)? |
Agreed, but other comments above still stand. |
|
Superseded by #3811 |
3 participants
This lower the amount of (now unnecessary) casting and JSON unmarshalling we do.
TODO: