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

fix: filter for DIDCommMessaging service type #27

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

TheTechmage
Copy link
Contributor

This prevents exceptions that may occur if the service type is did-communication instead of DIDCommMessaging

This prevents exceptions that may occur if the service type is
did-communication instead of DIDCommMessaging

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage
Copy link
Contributor Author

Looks like the error is coming from black@stable

@dbluhm
Copy link
Member

dbluhm commented Mar 13, 2024

Yeah, seen this in a few other repo's GHA. There's a mismatch between what's installed by the project and what the action is using. We should probably pin to the same version in the action as we're using locally to prevent this kind of issue.

Comment on lines +31 to +32
if did_service.type != "DIDCommMessaging":
continue
Copy link
Member

Choose a reason for hiding this comment

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

Technically, did-communication could be used to identify a DIDComm v2 endpoint. We could consider supporting it if the accept property has didcomm/v2 in it. I haven't fully formed an opinion either way on this yet. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the DIDComm spec encourages DIDCommMessaging, I feel that we should encourage it as well. You brought up an excellent point of making the class more modular in the future in slack, which I think is an excellent idea. Though I'm not sure when we should do that

Copy link
Member

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Sticking to the spec and ignoring service types we don't know is the right choice! We can worry about making the routing service more easily sub-class-able in the future.

@TheTechmage TheTechmage merged commit 3085fe9 into main Mar 14, 2024
4 of 5 checks passed
@TheTechmage TheTechmage deleted the fix/service-type-filter branch March 14, 2024 15:02
@TheTechmage TheTechmage restored the fix/service-type-filter branch March 14, 2024 15:25
@dbluhm dbluhm deleted the fix/service-type-filter branch June 11, 2024 22:00
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