-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat(pollux): add credential abstraction #94
Conversation
b13f899
to
520e8f8
Compare
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.
Looking great, just some minor questions on some wording that I didn't quite understand
import Foundation | ||
|
||
/// `ProofableCredential` is a protocol that adds provability functionality to a credential. | ||
public protocol ProofableCredential { |
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.
not sure about this naming, "Proofable" is not a meaningful term, perhaps "ProvableCredential" or "ProofCredential" would be more suitable?
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.
Yeah not sure as well. Im ok with changing, although I would prefer the ProvableCredential
since it's a protocol/interface/abstraction I prefer it not to be a noun.
In my opinion:
In the case of ProofCredential
as Proof is a noun it relays that the Credential itself is a Proof.
But ProvableCredential
is more in line with the concept that it creates a proof given a context.
But tell me what you think @curtis-h around this?
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.
I like your thinking, let's go with ProvableCredential
then :)
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.
Done ;)
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.
Apart from Curtis' suggestion, looks good to me
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.
LGTM
520e8f8
to
ad02957
Compare
ATL-4786 Besides adding the new credential abstraction this adds the following: - Changes on Pollux to use the new abstraction - Changes on PrismAgent to use new abstraction - Changes on Sample App to for new abstraction - Adds tests for new implementation - Adds Codable to Messages - Updates error codes and documentation - Updates all documentation on new methods
ad02957
to
148186e
Compare
ATL-4786
Besides adding the new credential abstraction this adds the following: