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(credential-status-router): Added basic router implementation & verifier/ manager encapsulation #1022

Open
wants to merge 1 commit into
base: feat/credentialStatusList2021-ESM
Choose a base branch
from

Conversation

Eengineer1
Copy link
Contributor

@Eengineer1 Eengineer1 commented Oct 9, 2022

Ref on architecture on Veramo's Discord channel #credential-status.
This is a router that handles top-level routing logic for different status methods in one agent, extending the AbstractStatusMethod abstract class each.

Base methods inherit from existing ICredentialStatusVerifier & ICredentialStatusManager.

Preparing a thorough coverage test case version, but the structure should suffice for credential-status-list-2021 tests.

@mirceanis mirceanis changed the base branch from feat/credentialStatusList2021 to feat/credentialStatusList2021-ESM October 12, 2022 08:33
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Thanks for posting this!

I need help understanding some of the methods defined.
I'd really like to maintain the separation of concerns between Verifier and Manager

/**
* Returns the revocation status of a credential from a given managed method
*/
statusRouterCheckStatus(args: ICheckCredentialStatusArgs, context: IAgentContext<IResolver>): Promise<CredentialStatus>
Copy link
Member

Choose a reason for hiding this comment

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

This method should be described by the ICredentialStatusVerifier interface.

If the ICredentialStatusRouter interface is supposed to be a merge between ICredentialStatusVerifier and ICredentialStatusManager, then we can declare that merger directly instead of redefining the methods.

* @param args - Input arguments to request the verifiable credential status value
* @returns A {@link VerifiableCredential} object
*/
statusRouterParseStatus(args: CredentialStatusRequestArgs, context: IAgentContext<any>): Promise<VerifiableCredential>
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 really understand the use of this method.

Can you help me figure out how it fits into the greater model?

/**
*
*/
credentialStatusRead(args: CredentialStatusRequestArgs, context: IAgentContext<any>): Promise<VerifiableCredential>
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this method?
I think that statusRead/statusCheck operations should be reserved for the ICredentialStatusVerifier interface, but perhaps I'm misunderstanding how this fits in.

Can you add some description to it?

constructor(options: {
statusMethods: Record<string, AbstractStatusMethod>
defaultStatusMethod: string
storage?: AbstractStatusStorage
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the storage parameter is not actually used by this class. It looks safe to remove.

@stale
Copy link

stale bot commented Jan 8, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jan 8, 2023
@mirceanis mirceanis added the pinned don't close this just for being stale label Jan 8, 2023
@stale stale bot removed the wontfix This will not be worked on label Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned don't close this just for being stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants