Skip to content

Discovery Service: change client to manage subjects, not DIDs #3294

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

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

gerardsn
Copy link
Member

@gerardsn gerardsn commented Aug 5, 2024

fixes #3265
This change lets the Discovery Service client manage Subjects rather than individual DIDs.

During activation/refresh it tries to register every DID in a subject. If 1 DID is successfully registered on the service, the operation is considered successful and a refresh is scheduled at 45% of the max_presentation_validitiy. This means that if a second DID fails that should not fail, it will not be retried until the next scheduled refresh. So it will be retried again at 90% of max_presentation_validity. If everything fails, the refresh/activation will be retried at the next configured refresh interval.

@gerardsn gerardsn requested a review from reinkrul August 5, 2024 07:47
@gerardsn gerardsn force-pushed the iss-3265/discovery-management-using-subjects branch from 93ea745 to 91291a1 Compare August 6, 2024 11:46
@gerardsn gerardsn requested a review from woutslakhorst August 6, 2024 11:46
@gerardsn gerardsn force-pushed the iss-3265/discovery-management-using-subjects branch from 91291a1 to 1e19f39 Compare August 6, 2024 11:53
for _, subjectDID := range subjectDIDs {
err := r.registerPresentation(ctx, subjectDID, service)
if err != nil {
loopErrs = append(loopErrs, err)
Copy link
Member

Choose a reason for hiding this comment

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

you might want to distinguish between functional errors and technical errors. A functional error like CredentialNotFound can be ignored, but a socket timeout should be logged.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made an attempt to add this, but it is ugly. I'm happy to change it if you have a better idea


// getSubjectVPsOnService finds all VPs in the service that contain a credential issued to any of the subjectDIDs.
// It returns a (potentially empty) list of VPs for each of the subjectDIDs.
// The list should not contain retractions since these do not contain any credentials.
Copy link
Member

Choose a reason for hiding this comment

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

but in client.go there's a check on retraction type. So does the list contain retractions or not? Also returning a map might be better suited?

Copy link
Member Author

Choose a reason for hiding this comment

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

but in client.go there's a check on retraction type. So does the list contain retractions or not?

'should' is not an acceptabel word to describe the result of a function, so removed the line. My confusion stems from the implementation not matching our own spec #3298

Also returning a map might be better suited?

now returns a map. Not sure if it should contain the keys for DIDs that do not have a VP

@gerardsn gerardsn requested a review from woutslakhorst August 29, 2024 12:34
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

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

we'll see if it's ugly when it shows up in the logs ;)

@gerardsn gerardsn merged commit 72be55e into master Sep 3, 2024
8 of 9 checks passed
@gerardsn gerardsn deleted the iss-3265/discovery-management-using-subjects branch September 3, 2024 12:35
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.

Use subject_id instead of did as parameter (/internal/discovery/v1/{serviceID}/{did}:)
2 participants