Skip to content
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

Conversation

javip97
Copy link
Contributor

@javip97 javip97 commented Jan 16, 2024

16.1 -> Waiting for protocol implementation (mobile)

@javip97 javip97 self-assigned this Jan 16, 2024
@javip97 javip97 requested a review from a team as a code owner January 16, 2024 08:23
@javip97 javip97 added the do not merge Please, do not merge it. label Jan 16, 2024
Copy link
Contributor

@x1m3 x1m3 left a 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/config/credential_status.go Outdated Show resolved Hide resolved
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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

@@ -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, "/"))
Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

you should reuse this

Copy link
Contributor

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.

@martinsaporiti martinsaporiti removed the do not merge Please, do not merge it. label May 29, 2024
@martinsaporiti martinsaporiti self-assigned this May 29, 2024
Comment on lines 8 to 11
sparseMerkleTreeProof = "SparseMerkleTreeProof"
iden3commRevocationStatusV1 = "Iden3commRevocationStatusV1.0"
iden3ReverseSparseMerkleTreeProof = "Iden3ReverseSparseMerkleTreeProof"
iden3OnchainSparseMerkleTreeProof2023 = "Iden3OnchainSparseMerkleTreeProof2023"
Copy link
Contributor

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()),
Copy link
Contributor

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()),
Copy link
Contributor

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.

Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor

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

@martinsaporiti martinsaporiti merged commit 11392d6 into develop May 30, 2024
3 checks passed
@martinsaporiti martinsaporiti deleted the PID-1234-issuer-node-revocation-status-iden-3-comm-revocation-status-v-1-0 branch May 30, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants