-
Notifications
You must be signed in to change notification settings - Fork 200
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(indy-vdr): register revocation registry definitions and status list #1693
feat(indy-vdr): register revocation registry definitions and status list #1693
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.
Nice :)
writeRequest = new CustomRequest({ customRequest: endorsedTransaction }) | ||
const operation = JSON.parse(endorsedTransaction)?.operation | ||
// extract the seqNo from the endorsed transaction, which is contained in the ref field of the operation | ||
seqNo = Number(operation?.ref) |
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.
How can we use the seqNo from a transaction that has not been published to the ledger yet? That's not possible right?
It seems that this should be the seqNo of the schema. As I've been looking at the structure of the id and it's as follows:
We should probably rename seqNo
in the method to schemaSeqNo
.
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 think ref
is used in the cred def transaction to reference the schema
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 think this is not fixed yet (not sure if you were still looking at it)
const revocationRegistryDefinitionRequest = new RevocationRegistryEntryRequest({ | ||
submitterDid: namespaceIdentifier, | ||
revocationRegistryEntry: { | ||
ver: '1.0', | ||
value: { | ||
accum: revocationStatusList.currentAccumulator, | ||
}, | ||
}, | ||
revocationRegistryDefinitionType: 'CL_ACCUM', | ||
revocationRegistryDefinitionId: revocationStatusList.revRegDefId, | ||
}) |
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.
We also need to pass the issued/revoked indices here. These are missing in the JS wrapper: hyperledger/indy-vdr#165
Not sure if you can pass them with a ts-ignore for now and they will be passed on automatically to indy-vdr native lib or whether actual changes need to be published for this transaction can work: hyperledger/indy-vdr#165
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.
It seems to be passing the entry value as a whole to indy-vdr, so I think it should just work by passing the values: https://github.com/hyperledger/indy-vdr/blob/main/wrappers/javascript/indy-vdr-nodejs/src/NodeJSIndyVdr.ts#L405
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.
It is still a bit confusing, but the public API now also accepts the issued/revoked arrays as I assumed that that was the point. The changes between indy-sdk, indy-vdr, non-indy ledgers and anoncreds objects vs indy objects is causing quite some confusion ;).
e4a9a1c
to
2d46f52
Compare
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
3ad9693
to
bac520e
Compare
issued?: Array<number> | ||
revoked?: Array<number> |
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.
issued?: Array<number> | |
revoked?: Array<number> |
These should not be added here. The revocation status list is the model as used by AnonCreds now, and defined in the AnonCreds specification. Indy uses deltas and in those deltas are issued
and revoked
keys.
So we need to do the following:
- Whenever we want to issue/revoke we create or update a status list, then in the indy anoncreds registry implementation we take the current status list and compare it to the new status list. Based on that we can get the issued/revoked indices
- Whenever we want to get a status list (this is already implemented before your PR) we get the detlas, and based on all deltas we construct the revocation list.
So it's important to know: Indy ALWAYS uses detlas on ledger, but everywhere else we use revocation status list as much as possible. So the indy way of doing things should only be used in the Indy anonCreds registry and be transformed to the anoncreds public models (which is revocation status list)
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.
How do we construct a status list from the deltas? Is that something that indy-vdr provides?
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.
export function getUnqualifiedRevocationRegistryEntryId( | ||
unqualifiedDid: string, | ||
seqNo: string | number, | ||
schemaSeqNo: string | number, | ||
credentialDefinitionTag: string, | ||
revocationRegistryTag: string | ||
) { | ||
return `${unqualifiedDid}:4:${unqualifiedDid}:3:CL:${seqNo}:${credentialDefinitionTag}:CL_ACCUM:${revocationRegistryTag}` | ||
return `${unqualifiedDid}:5:${unqualifiedDid}:3:CL:${schemaSeqNo}:${credentialDefinitionTag}:CL_ACCUM:${revocationRegistryTag}` | ||
} |
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.
When do we need to retrieve revocation registry entries? We only need to get the deltas right?
revoked: delta.value.revoked, | ||
issued: delta.value.issued, |
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.
We should use the deltas only to create the status list
revoked: delta.value.revoked, | |
issued: delta.value.issued, |
revoked: delta.revoked, | ||
issued: delta.issued, |
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.
revoked: delta.revoked, | |
issued: delta.issued, |
writeRequest = new CustomRequest({ customRequest: endorsedTransaction }) | ||
const operation = JSON.parse(endorsedTransaction)?.operation | ||
// extract the seqNo from the endorsed transaction, which is contained in the ref field of the operation | ||
seqNo = Number(operation?.ref) |
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 think this is not fixed yet (not sure if you were still looking at it)
const schemaSeqNo = Number(schemaMetadata.indyLegderSeqNo) | ||
const didIndyRevocationRegistryEntryId = getDidIndyRevocationRegistryEntryId( | ||
namespace, | ||
namespaceIdentifier, | ||
schemaSeqNo, | ||
credentialDefinition.tag, | ||
revocationRegistryDefinition.tag | ||
) | ||
|
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.
The revocation registry definition id is already passed as part of the input to this method, so I don't think it's needed to fetch the schema + cred def + revoc reg to construct the id.
Rather we can take it from revocationStatusList .revRegDefId
accum: revocationStatusList.currentAccumulator, | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-expect-error | ||
issued: revocationStatusList.issued ?? [], | ||
revoked: revocationStatusList.revoked ?? [], |
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.
So here we need to get the current state, and based on the current statusList and the new statusList we need to calculate the issued
and revoked
indices.
agent.context, | ||
legacyRevocationRegistryId, | ||
entryResponse.result.txnMetadata.txnTime | ||
console.log(JSON.stringify(revRegDefResp)) |
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.
console.log(JSON.stringify(revRegDefResp)) |
c039c16
to
d308681
Compare
6c811a2
to
93b7f2f
Compare
@TimoGlastra I am getting very close, but somehow it can't fetch the delta when I try to make the request (logs here https://github.com/openwallet-foundation/agent-framework-javascript/actions/runs/7531063980/job/20498795265?pr=1693#step:10:93) Do you have any idea? Registration seems to respond with all the correct data. |
93b7f2f
to
5e8667d
Compare
Did you add a sleep? Indy takes a while to process a write. I'll take a good look at it tomorrow |
Yes! But it is just a second, so maybe increasing it will help |
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
5e8667d
to
5c6e03c
Compare
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
0a5421f
to
d3d2993
Compare
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
d3d2993
to
f0969be
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.
Sorry a few more comments. But rest looks good!!
@@ -90,7 +90,7 @@ describe('IndyVdrAnonCredsRegistry', () => { | |||
await agent.wallet.delete() | |||
}) | |||
|
|||
test('register and resolve a schema and credential definition (internal, issuerDid != endorserDid)', async () => { | |||
xtest('register and resolve a schema and credential definition (internal, issuerDid != endorserDid)', 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.
These are disabled and also are not updated with the new register methods.
} | ||
} | ||
|
||
const schemaSeqNo = schemaMetadata.indyLedgerSeqNo |
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.
We can extract the schema seqNo from the cred def id
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.
So we don't have to fetch the schema
agentContext.config.logger.trace( | ||
`Submitting get revocation registry delta request for revocation registry '${revocationRegistryId}' to ledger` | ||
// TODO: this will bypass caching if done on a higher level. | ||
const { credentialDefinition, resolutionMetadata } = await this.getCredentialDefinition( |
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.
Why do we need to fetch the cred def? We can extract all needed parts from the cred def id. And indy will reject if something is wrong
) | ||
|
||
if (revocationRegistryNamespace && revocationRegistryNamespace !== namespace) { | ||
// throw 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.
These are still todo. If we do it in separate pr we should remove it for now
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
2bb1ea1
to
3702324
Compare
…ist (openwallet-foundation#1693) Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: deliver messages individually, not fetching from the queue every time Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: revert to free runners (openwallet-foundation#1662) Signed-off-by: Ry Jones <ry@linux.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: create settings.yml (openwallet-foundation#1663) Signed-off-by: Ry Jones <ry@linux.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: fix ci and add note to readme (openwallet-foundation#1669) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> docs: update active maintainers (openwallet-foundation#1664) Signed-off-by: Karim Stekelenburg <karim@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: did:peer:2 and did:peer:4 support in DID Exchange (openwallet-foundation#1550) Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(presentation-exchange): added PresentationExchangeService (openwallet-foundation#1672) Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: removed jan as maintainer (openwallet-foundation#1678) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> ci: change secret (openwallet-foundation#1679) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: add meta to rxjs timeout errors (openwallet-foundation#1683) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> build(deps): bump ws and @types/ws (openwallet-foundation#1686) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> build(deps): bump follow-redirects from 1.15.2 to 1.15.4 (openwallet-foundation#1694) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: update shared components libraries (openwallet-foundation#1691) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix: properly print key class (openwallet-foundation#1684) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(present-proof): add support for aries RFC 510 (openwallet-foundation#1676) Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io> Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix(present-proof): isolated tests (openwallet-foundation#1696) Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(indy-vdr): register revocation registry definitions and status list (openwallet-foundation#1693) Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: rename to credo-ts (openwallet-foundation#1703) Signed-off-by: Ry Jones <ry@linux.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> ci: fix git checkout path and update setup-node (openwallet-foundation#1709) Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix: remove check for DifPresentationExchangeService dependency (openwallet-foundation#1702) Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> docs: update zoom meeting link (openwallet-foundation#1706) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> refactor(oob)!: make label optional (openwallet-foundation#1680) Signed-off-by: Timo Glastra <timo@animo.id> Co-authored-by: Ariel Gentile <gentilester@gmail.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: support short legacy connectionless invitations (openwallet-foundation#1705) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(dids)!: did caching (openwallet-foundation#1710) feat: did caching Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix: jsonld document loader node 18 (openwallet-foundation#1454) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> build(deps): bump amannn/action-semantic-pull-request from 5.3.0 to 5.4.0 (openwallet-foundation#1656) build(deps): bump amannn/action-semantic-pull-request Bumps [amannn/action-semantic-pull-request](https://github.com/amannn/action-semantic-pull-request) from 5.3.0 to 5.4.0. - [Release notes](https://github.com/amannn/action-semantic-pull-request/releases) - [Changelog](https://github.com/amannn/action-semantic-pull-request/blob/main/CHANGELOG.md) - [Commits](amannn/action-semantic-pull-request@v5.3.0...v5.4.0) --- updated-dependencies: - dependency-name: amannn/action-semantic-pull-request dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: did rotate (openwallet-foundation#1699) Signed-off-by: Ariel Gentile <gentilester@gmail.com> refactor: pickup protocol method names Signed-off-by: Ariel Gentile <gentilester@gmail.com>
IndyVdrAnonCredsRegistry
to register:registerCredentialDefinition
as there is a 90% overlap.