-
Notifications
You must be signed in to change notification settings - Fork 32
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: get credential registry state (tever vcstate) #282
feat: get credential registry state (tever vcstate) #282
Conversation
src/keri/app/credentialing.ts
Outdated
et: string | ||
} & ({ | ||
et: "iss" | "rev", | ||
ra: Record<string, never> |
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 corresponds to {}
- it's always empty for iss/rev.
src/keri/app/credentialing.ts
Outdated
dt: string, | ||
et: string | ||
} & ({ | ||
et: "iss" | "rev", |
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.
@lenkan Ilks
is an object, and many other things in that file. Maybe a string enum would be better for our typing?
src/keri/app/credentialing.ts
Outdated
s: string, | ||
d: string, | ||
ri: string, | ||
a: { s: number, d: 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.
(really is an int, unlike most s
fields)
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.
@iFergal but it is a sequence number anyway right? If so, Is it the sequence number of the credential state changes, or of the registry event? If the registry event, is it possible we have a similar issue as we had for KEL events where it would not handle s >= 10 (decimal)
correctly?
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.
Yes, it's still a sequence number. It's the sn of the anchoring event in the KEL for the current credential event. sn
is used from below by keripy when exporting this state.
A mix of int and hex seems to be used throughout keripy, I believe it just needs to be hex when it's actually part of a KERI event and this is just an export of current state.
I think it's OK for Signify to consider s: number
as a int, and if it's actually a hex being thrown out cast as number then it's a bug?
@property
def sn(self):
"""
Property sn: sequence number as int
Returns .raw converted to int
"""
return int.from_bytes(self.raw, 'big')
@property
def snh(self):
"""
Property snh: sequence number as hex
Returns .sn int converted to hex str
"""
return f"{self.sn:x}" # "{:x}".format(self.sn)
@@ -248,6 +248,15 @@ test('single signature credentials', async () => { | |||
await waitOperation(issuerClient, op); | |||
}); | |||
|
|||
await step('holder can get the credential status before or without holding', async () => { |
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.
If I can get transaction state noticing working on keripy, this could be extended nicely to check for revocations w/o IPEX.
Ah @2byrds forgot we need a new dev release of KERIA. Do you have permissions for that? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
==========================================
+ Coverage 83.57% 83.59% +0.01%
==========================================
Files 48 48
Lines 4220 4225 +5
Branches 1055 1055
==========================================
+ Hits 3527 3532 +5
Misses 663 663
Partials 30 30 ☔ View full report in Codecov by Sentry. |
lets see @iFergal ! I have launched the docker dev release (same version 0.2.0-dev3) https://github.com/WebOfTrust/keria/actions/runs/11315384968 |
thanks @2byrds - hard to tell if the issues are from the changes I made to the Makefile or were already broken, as it seems to be the only time it was ran from the pipeline: https://github.com/WebOfTrust/keria/actions/workflows/publish-keria.yml |
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, can we just remove the console.log from the unit test?
This corresponds to WebOfTrust/keria#304 - see description there for more details.
Essentially can get credential state (iss/rev) without needing to already hold the credential (or even be the holder).