-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support for retention challenges #219
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
==========================================
+ Coverage 51.62% 51.70% +0.07%
==========================================
Files 32 32
Lines 2774 2791 +17
==========================================
+ Hits 1432 1443 +11
- Misses 1199 1203 +4
- Partials 143 145 +2 ☔ View full report in Codecov by Sentry. |
ready for preliminary review. I still have to add a few more pieces of text around
|
spec/api.yaml
Outdated
type: string | ||
description: A retention proof calculated according to the spec-defined retention proof algorithm. | ||
description: A retention solution calculated according to the spec-defined retention solution algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, I think it's good to avoid conflating the usage of proof
across the two use cases:
- cryptographic provenance using private keys
- evidence of expending energy (AKA PoW)
yeah I think we'd be wise to consolidate our usage of that word for (1) and not for (2)
also sets us up if we ever introduces a spam preventing mechanism other-than PoW
spec/api.yaml
Outdated
"400": | ||
description: Invalid request. | ||
content: | ||
application/json: | ||
schema: | ||
type: string | ||
"401": | ||
description: Invalid signature. | ||
description: Invalid signature or retention solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why wouldn't an invalid retention solution be a 400?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, updating
Ready for final review! I will handle exposing other gateways in a follow-up |
Co-authored-by: Kendall Weihe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@decentralgabe in examining the currently registered properties returned in didResolutionMetadata
and didDocumentMetadata
, it seems like perhaps expiry
would be more appropriately included in DID Document Metadata.
Perhaps types
should be as well?
Rationale:
The DID Core spec states the following about DID Resolution Metadata:
A metadata structure consisting of values relating to the results of the DID resolution process which typically changes between invocations of the resolve and resolveRepresentation functions, as it represents data about the resolution process itself.
and the following about DID Document Metadata:
This structure contains metadata about the DID document contained in the didDocument property. This metadata typically does not change between invocations of the resolve and resolveRepresentation functions unless the DID document changes, as it represents metadata about the DID document.
Neither types
nor expiry
are data "about the resolution process itself." IMHO they would be more appropriately classified as data about the DID & its DID Document.
Additionally, these values "typically [do] not change between invocations of the resolve functions unless the DID document changes." In other words, a DID controller would have to submit a new request to a Gateway in order to change the types
property or to submit a new Retention Solution to extend the expiry
.
Co-authored-by: Frank Hinek <[email protected]>
@frankhinek thanks this is a good callout and I agree with your reasoning, I will update this PR |
|
merging for agility - feel free to leave additional comments if you have them I can address in a follow-up |
Fix #74