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

Missing sn in CloneCred after fix in PR #682 #854

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rodolfomiranda
Copy link
Contributor

@rodolfomiranda rodolfomiranda commented Aug 28, 2024

PR #682 omitted specifying sn in the call to cloneTvtAt at line 405. This is important for revoked credentials.
See diff changes on line 405 of PR #682

This PR inserts again the sn parameter.

Additionally, I corrected the sn used to search in the Tel, sn=0 for iss or sn=1 for rev

Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

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

See comment inline and please add a test for this.

status = self.tevers[regk].vcState(saider.qb64)
schemer = db.schema.get(creder.schema)

iss = bytearray(self.cloneTvtAt(creder.said, sn=0 if status.et == coring.Ilks.iss else 1))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the appropriate fix because it puts either an issuance event OR a revocation event in a field called "iss". This PR should load and create a rvk field with the revocation event if one exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we call the field rvk or rev like in the type field? Also, for a revoked credential, should the response also include the iss ?

Copy link
Member

Choose a reason for hiding this comment

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

Good call, make it rev. Yes, you might as well include both events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added rev, revatc, revanc and revancatc. And tests.

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.

2 participants