-
Notifications
You must be signed in to change notification settings - Fork 86
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: adding Iden3commRevocationStatusV1 #600
feat: adding Iden3commRevocationStatusV1 #600
Conversation
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. Just an extra tab that we should remove.
internal/core/services/revocation.go
Outdated
@@ -106,6 +115,73 @@ func convertCredentialStatus(credStatus interface{}) (verifiable.CredentialStatu | |||
return verifiable.CredentialStatus{}, errors.New("failed cast credential status to verifiable.CredentialStatus") | |||
} | |||
|
|||
func getRevocationStatusFromAgent(ctx context.Context, from, to, endpoint string, nonce uint64) (*verifiable.RevocationStatus, error) { |
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.
you should use this resolver
https://github.com/iden3/iden3comm/blob/main/resolvers/agent.go
as well as you can switch to Credential Registry concept here https://github.com/iden3/go-schema-processor/blob/adbd0a5aa31811583d88aa9f48ff00b4bb1efd9f/verifiable/credential_status.go#L33
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.
this was deleted because It was not used
internal/config/credential_status.go
Outdated
@@ -36,6 +38,11 @@ func (r *DirectStatus) GetURL() string { | |||
return strings.TrimSuffix(r.URL, "/") | |||
} | |||
|
|||
// GetAgentURL returns the URL of the agent endpoint | |||
func (r *DirectStatus) GetAgentURL() string { | |||
return fmt.Sprintf("%s/v1/agent", strings.TrimSuffix(r.URL, "/")) |
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 if it is the right place
Direct status was about an endpoint, this is more like Iden3CommAgentStatus, but maybe it is ok.
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've changed this. It was wrong here
return nil, fmt.Errorf("invalid revocation request body: %w", err) | ||
} | ||
|
||
var revStatus *verifiable.RevocationStatus |
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.
you should reuse 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.
@vmidyllic I'm not sure if we reuse that. ValidateCredentialStatus
function at the end calls the issuer node to get the credential rev status, and here we are resolving that. Maybe we can talk.
…amoy Update libraries to support Amoy and merged with develop
internal/config/credential_status.go
Outdated
sparseMerkleTreeProof = "SparseMerkleTreeProof" | ||
iden3commRevocationStatusV1 = "Iden3commRevocationStatusV1.0" | ||
iden3ReverseSparseMerkleTreeProof = "Iden3ReverseSparseMerkleTreeProof" | ||
iden3OnchainSparseMerkleTreeProof2023 = "Iden3OnchainSparseMerkleTreeProof2023" |
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.
you can reuse these constants from libraries
|
||
func (r *iden3CommRevocationStatusV1Resolver) resolve(credentialStatusSettings config.CredentialStatus, issuerDID w3c.DID, nonce uint64, issuerState string) *verifiable.CredentialStatus { | ||
return &verifiable.CredentialStatus{ | ||
ID: buildDirectRevocationURL(credentialStatusSettings.Iden3CommAgentStatus.GetURL()), |
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.
agent url must be enough, why do you need to build direct url here?
return &verifiable.CredentialStatus{ | ||
ID: buildDirectRevocationURL(credentialStatusSettings.DirectStatus.GetURL(), issuerDID.String(), nonce, credentialStatusSettings.SingleIssuer), | ||
ID: buildDirectRevocationURL(credentialStatusSettings.Iden3CommAgentStatus.GetURL()), |
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.
smt status (direct) is a separate, if you don't need it on the issuer node, maybe you don't need this file? but such fix looks strange.
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.
that's correct, We don't need the file.
host, nonce) | ||
} | ||
return fmt.Sprintf("%s/v1/%s/claims/revocation/status/%d", host, url.QueryEscape(issuerDID), nonce) | ||
func buildDirectRevocationURL(host string) string { |
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 would rename buildDirectRevocationURL to build agent url, not confuse developers in other places.
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 will remove the function because it is no longer needed
|
||
zkpPackerV2 := packers.NewZKPPacker(nil, verifications) |
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.
are you sure you don't need prover here?
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've found the prover configured here is not being used
16.1 -> Waiting for protocol implementation (mobile)